ソースを参照

s5: UI polish — mechanical rows ('session context restored', 'parked until') filtered from every user-facing timeline (write-side filtering on the daemon-ingest path, the web's restore comment removed, read-side filters on the goal narrative/task thread/dashboard so legacy rows vanish too; the raw terminal keeps everything), label reconciliation (a goal in completion review reads 'awaiting your review', the run-task status card agrees via display_map_for/display_of_detail — no bare 'blocked' for review parks), and verdict-shaped blocks pinned to their explicit accept/reject + free-text forms with NULL-options questions staying question-shaped.

clanker 1 ヶ月 前
親
コミット
7db4cac172

+ 6 - 0
src/Services/GoalService.vala

@@ -845,10 +845,16 @@ namespace ClankerSaddle.Services {
                 }
                 // The ONE comment thread: agent/user rows as footnote
                 // comments, harness-authored rows as system events.
+                // Plan 16 S5 (U2): mechanical rows (restores, park
+                // notices) never reach the user-facing narrative.
                 foreach (var comment in yield tasks.list_comments(((!)run).id)) {
                     if (comment.created_at == null) {
                         continue;
                     }
+                    if (comment.author == COMMENT_AUTHOR_SYSTEM
+                            && comment_is_mechanical(comment.body)) {
+                        continue;
+                    }
                     rows.add(new NarrativeRow() {
                         kind = comment.author == COMMENT_AUTHOR_SYSTEM
                             ? "system" : "comment",

+ 0 - 15
src/Services/SaddlePollState.vala

@@ -32,7 +32,6 @@ namespace ClankerSaddle.Services {
         private Dictionary<string, string> probe_contact = new Dictionary<string, string>();
         private HashSet<string> delivered_answers = new HashSet<string>();
         private HashSet<string> delivered_comments = new HashSet<string>();
-        private HashSet<string> resumed_notified = new HashSet<string>();
         private Dictionary<string, string> ask_outcome = new Dictionary<string, string>();
         private HashSet<string> ask_seen = new HashSet<string>();
         private Dictionary<string, string> wake_at_ms = new Dictionary<string, string>();
@@ -96,20 +95,6 @@ namespace ClankerSaddle.Services {
             state_lock.unlock();
         }
 
-        /**
-         * Marks `session_id`'s daemon-side resume as notified; returns
-         * false when the progress entry already went out.
-         */
-        public bool mark_resumed(int64 session_id) {
-            state_lock.lock();
-            bool already = resumed_notified.has(session_id.to_string());
-            if (!already) {
-                resumed_notified.add(session_id.to_string());
-            }
-            state_lock.unlock();
-            return !already;
-        }
-
         /**
          * Marks `block_id`'s answer as delivered; returns false when it
          * had already been handed over.

+ 15 - 23
src/Services/SaddleService.vala

@@ -397,11 +397,10 @@ namespace ClankerSaddle.Services {
 
         /**
          * Records a /status observation: the health snapshot, the
-         * daemon-reported ssh key, and — because the daemon surfaces
-         * `resumed` on the FIRST status after a workspace restore,
-         * whichever probe happens to receive it (the dispatcher's poll,
-         * a heal pass, a manual refresh) — the once-per-session resume
-         * progress entry.
+         * daemon-reported ssh key, and the goal-branch push flags
+         * (plan 15 C4). The `resumed` flag needs no projection — the
+         * restore's record is the daemon's own system entry in the
+         * raw terminal log (plan 16 S5/U2).
          */
         private async void record_status_probe(Saddle saddle, Json.Object status) throws Error {
             string state = status.has_member("state")
@@ -433,22 +432,11 @@ namespace ClankerSaddle.Services {
                 }
             }
 
-            // The "interrupted -> resumed" timeline arc. Absent on older
-            // daemons; marked once per session in SaddlePollState.
-            if (status.has_member("resumed")
-                    && status.get_member("resumed").get_value_type() == typeof(bool)
-                    && status.get_boolean_member("resumed")
-                    && saddle.current_task_id > 0) {
-                var session = yield task_service.open_session_for_task(saddle.current_task_id);
-                if (session != null && poll_state.mark_resumed(((!)session).id)) {
-                    // Plan 15 C2: the restoration is harness
-                    // bookkeeping — a distinct system event on the
-                    // thread, never a green progress line (the feed
-                    // carries agent work only).
-                    yield task_service.add_system_comment(saddle.current_task_id,
-                        "Saddle restored the session context; task resumed");
-                }
-            }
+            // Plan 16 S5 (U2): the daemon's own system entry (typed
+            // 'system' in the results stream) is the raw-terminal
+            // record of a workspace restore — the web no longer authors
+            // a duplicate timeline row for it. The `resumed` flag
+            // carries no further web-side projection.
         }
 
         /**
@@ -1299,8 +1287,12 @@ namespace ClankerSaddle.Services {
                         // Plan 15 C2: harness-authored bookkeeping (the
                         // session-restore footnote, the goal park
                         // notice) — the distinct system-event class on
-                        // the ONE thread, never an agent row.
-                        if (output.length > 0) {
+                        // the ONE thread, never an agent row. Plan 16
+                        // S5 (U2): the MECHANICAL rows (restores, park
+                        // notices) never author comment rows at all —
+                        // they stay in the raw terminal log only.
+                        if (output.length > 0
+                                && !comment_is_mechanical(output)) {
                             yield tasks.add_system_comment(task_id, output);
                         }
                         break;

+ 63 - 12
src/Services/TaskService.vala

@@ -38,6 +38,20 @@ namespace ClankerSaddle.Services {
      */
     public const string COMMENT_AUTHOR_SYSTEM = "system";
 
+    /**
+     * Plan 16 S5 (U2): MECHANICAL harness bookkeeping that every
+     * user-facing timeline filters out — the session-restore footnote
+     * and the park-until notice (both remain in the raw terminal log,
+     * which keeps everything). Matched by substring so the daemon's
+     * [harness]-prefixed wordings and the web's own phrasing all stay
+     * covered; substantive system rows (aborts, requeues, retries)
+     * never match.
+     */
+    public static bool comment_is_mechanical(string body) {
+        return body.contains("session context restored")
+            || body.contains("parked until");
+    }
+
     /**
      * The agent-celebration entry type (`saddle yeehaw <message>`): the
      * daemon pushes it as a round-results entry carrying the message in
@@ -60,12 +74,18 @@ namespace ClankerSaddle.Services {
     public const string NOTICE_REVIEW_APPROVED = "Review approved. Continue with the task.";
     public const string NOTICE_REVIEW_FEEDBACK = "Review feedback received:";
 
+    /** The display label of a goal-run parked on a review-family block (U3). */
+    public const string DISPLAY_AWAITING_REVIEW = "awaiting your review";
+
     /**
      * A task's computed presentation state (stored status + unanswered
      * blocks): `review requested` while a review block is unanswered
      * (even when the stored status is terminal — the post-mortem's
      * `done`-with-open-review case), `parked` for the suspended state
-     * (a goal agent between events), else the stored status. Pure
+     * (a goal agent between events), else the stored status. Plan 16
+     * S5 (U3): a GOAL-RUN task parked on a review-family block (plan
+     * or completion review) reads "awaiting your review" — agreeing
+     * with its goal's label, never a bare "blocked". Pure
      * presentation — the stored statuses and the dispatcher are
      * untouched.
      */
@@ -74,12 +94,15 @@ namespace ClankerSaddle.Services {
         public bool terminal { get; set; }
         public bool failed { get; set; }
 
-        public static TaskDisplay compute(TaskEntity task, bool review_pending) {
+        public static TaskDisplay compute(TaskEntity task, bool review_pending,
+                bool goal_review_pending = false) {
             var view = new TaskDisplay() {
                 terminal = task.status == STATUS_DONE || task.status == STATUS_FAILED,
                 failed = task.status == STATUS_FAILED
             };
-            if (review_pending) {
+            if (goal_review_pending && task.kind == TASK_KIND_GOAL_RUN) {
+                view.status = DISPLAY_AWAITING_REVIEW;
+            } else if (review_pending) {
                 view.status = "review requested";
             } else if (task.status == STATUS_SUSPENDED) {
                 view.status = "parked";
@@ -241,28 +264,46 @@ namespace ClankerSaddle.Services {
         /**
          * Computed display states for a batch of tasks (all lists share
          * this), keyed by task id string. Unanswered review blocks load
-         * once, not per task.
+         * once, not per task. Plan 16 S5 (U3): a goal-run task with a
+         * pending review-family block (plan/completion/review) reads
+         * "awaiting your review" — the run-task card agrees with its
+         * goal.
          */
         public async Dictionary<string, TaskDisplay> display_map_for(
                 Lot<TaskEntity> tasks) throws Error {
             var wanted = new Invercargill.DataStructures.HashSet<string>();
+            var goal_runs = new Invercargill.DataStructures.HashSet<string>();
             foreach (var task in tasks) {
                 wanted.add(task.id.to_string());
+                if (task.kind == TASK_KIND_GOAL_RUN) {
+                    goal_runs.add(task.id.to_string());
+                }
             }
 
             var review_pending = new Invercargill.DataStructures.HashSet<string>();
-            foreach (var block in yield db.query<TaskBlock>()
-                    .where(expr("type == $0", elem(BLOCK_REVIEW)))
-                    .materialise_async()) {
-                if (block.answered_at == null && wanted.has(block.task_id.to_string())) {
-                    review_pending.add(block.task_id.to_string());
+            var goal_review_pending = new Invercargill.DataStructures.HashSet<string>();
+            foreach (var family in new string[] { BLOCK_REVIEW, BLOCK_COMPLETION, BLOCK_PLAN }) {
+                foreach (var block in yield db.query<TaskBlock>()
+                        .where(expr("type == $0", elem(family)))
+                        .materialise_async()) {
+                    if (block.answered_at != null
+                            || !wanted.has(block.task_id.to_string())) {
+                        continue;
+                    }
+                    if (block.block_type == BLOCK_REVIEW) {
+                        review_pending.add(block.task_id.to_string());
+                    }
+                    if (goal_runs.has(block.task_id.to_string())) {
+                        goal_review_pending.add(block.task_id.to_string());
+                    }
                 }
             }
 
             var map = new Dictionary<string, TaskDisplay>();
             foreach (var task in tasks) {
                 map.set(task.id.to_string(),
-                    TaskDisplay.compute(task, review_pending.has(task.id.to_string())));
+                    TaskDisplay.compute(task, review_pending.has(task.id.to_string()),
+                        goal_review_pending.has(task.id.to_string())));
             }
             return map;
         }
@@ -273,12 +314,22 @@ namespace ClankerSaddle.Services {
          */
         public static TaskDisplay display_of_detail(TaskDetail detail) {
             bool review_pending = false;
+            bool goal_review_pending = false;
             foreach (var block in detail.blocks) {
-                if (block.block_type == BLOCK_REVIEW && block.answered_at == null) {
+                if (block.answered_at != null) {
+                    continue;
+                }
+                if (block.block_type == BLOCK_REVIEW) {
                     review_pending = true;
                 }
+                if (detail.task.kind == TASK_KIND_GOAL_RUN
+                        && (block.block_type == BLOCK_REVIEW
+                            || block.block_type == BLOCK_COMPLETION
+                            || block.block_type == BLOCK_PLAN)) {
+                    goal_review_pending = true;
+                }
             }
-            return TaskDisplay.compute(detail.task, review_pending);
+            return TaskDisplay.compute(detail.task, review_pending, goal_review_pending);
         }
 
         public async ImmutableLot<TaskEntity> list_tasks(string? status, int64? project_id) throws Error {

+ 7 - 0
src/Services/TimelineService.vala

@@ -156,6 +156,13 @@ namespace ClankerSaddle.Services {
                 if (comment.created_at == null || run_tasks.has(comment.task_id.to_string())) {
                     continue;
                 }
+                // Plan 16 S5 (U2): mechanical rows (restores, park
+                // notices) never reach the dashboard timeline — they
+                // live in the raw terminal log only.
+                if (comment.author == COMMENT_AUTHOR_SYSTEM
+                        && comment_is_mechanical(comment.body)) {
+                    continue;
+                }
                 events.add(new TimelineEvent() {
                     kind = "comment", at_epoch = ((!)comment.created_at).to_unix(),
                     task_id = comment.task_id,

+ 5 - 1
src/entrypoints/GoalDetailEntrypoint.vala

@@ -363,7 +363,11 @@ namespace ClankerSaddle {
             pub.branch_name = goal.branch_name;
             pub.base_branch = goal.base_branch ?? "";
             pub.status = goal.status;
-            pub.display = goal.status.replace("_", " ");
+            // Plan 16 S5 (U3): a goal awaiting its completion review
+            // reads "awaiting your review" — the run-task status card
+            // agrees (no bare "blocked" for review parks).
+            pub.display = goal.status == GOAL_COMPLETION_REVIEW
+                ? "awaiting your review" : goal.status.replace("_", " ");
             pub.priority = goal.priority;
             pub.plan = goal.plan ?? "";
             pub.completion_report = goal.completion_report ?? "";

+ 7 - 0
src/entrypoints/TaskDetailEntrypoint.vala

@@ -279,6 +279,13 @@ namespace ClankerSaddle {
 
             var comments = new Vector<CommentView>();
             foreach (var comment in ((!)detail).comments) {
+                // Plan 16 S5 (U2): mechanical rows (restores, park
+                // notices) never reach the user-facing thread — they
+                // live in the raw terminal log only.
+                if (comment.author == COMMENT_AUTHOR_SYSTEM
+                        && comment_is_mechanical(comment.body)) {
+                    continue;
+                }
                 comments.add(new CommentView() {
                     author = comment.author,
                     body = comment.body,

+ 85 - 6
test/web_stack_test.py

@@ -1344,20 +1344,43 @@ def bundle_pages(ctx):
           "the task page ships no verdict constants (server-owned wording)")
     check('name="verdict" value="accept"' in task_page_html,
           "the accept forms post the verdict marker")
+    # Plan 16 S5 (U1): verdict-shaped blocks keep their explicit
+    # buttons + free-text path; a question with NULL options renders
+    # the question form (radio "other" + free reply), never a
+    # free-text-only verdict.
+    check("Request changes" in task_page_html and 'name="feedback"' in task_page_html,
+          "the review verdict keeps Request changes with free-text feedback (U1)")
+    check('value="__other__"' in task_page_html and 'name="other"' in task_page_html,
+          "question blocks render the option/other question form (U1)")
+    status, body = saddle_api("POST", "/api/saddle/%d/update" % fake_id, fake_token,
+                              {"type": "block", "session_id": session_id, "data": {
+                                  "question": "Free-text thought?", "type": "question"}})
+    check(status == 200 and body.get("block_id"),
+          "a NULL-options question block raises")
+    bare_question = [b for b in task_state(auth, plan_task)["blocks"]
+                     if b["question"] == "Free-text thought?"][0]
+    check(bare_question.get("options") == []
+          and bare_question.get("block_type") == "question",
+          "a NULL-options question stays a question (no options, not a verdict)")
 
     print("== B2: question block body above options ==")
     # Contract side (A2 will restore bodies end-to-end): a question
     # block raised with a heredoc body carries it; the body preview
     # rides above the options (template order) and survives answering
-    # alongside the selected answer.
+    # alongside the selected answer. (Blocks list newest-first; the
+    # U1 fixture above raised a second, options-less question.)
     detail = task_state(auth, plan_task)
-    question = [b for b in detail["blocks"] if b["block_type"] == "question"][0]
+    question = [b for b in detail["blocks"]
+                if b["block_type"] == "question"
+                and b["body"] == "# Storage choice"][0]
     check(question.get("body") == "# Storage choice"
           and question.get("show_body") is True,
           "question block keeps its body markdown with the preview on")
     act(auth, question["answer_action"], {"option": "SQLite"})
     detail = task_state(auth, plan_task)
-    question = [b for b in detail["blocks"] if b["block_type"] == "question"][0]
+    question = [b for b in detail["blocks"]
+                if b["block_type"] == "question"
+                and b["body"] == "# Storage choice"][0]
     check(question.get("answered") is True and question.get("answer") == "SQLite"
           and question.get("body") == "# Storage choice"
           and question.get("show_body") is True,
@@ -2681,10 +2704,9 @@ def bundle_dispatch(ctx):
     check(wait_task_done(auth, flaky, timeout=60) is not None,
           "retried task completes on the restored workspace")
     detail = task_state(auth, flaky)
-    check(any("restored the session context" in c["body"]
-              and c["author"] == "system" and c.get("is_system")
+    check(all("restored the session context" not in c["body"]
               for c in detail.get("comments", [])),
-          "daemon resume reported on the first status after the restore")
+          "the session-restore stays OUT of the user-facing thread (U2: raw terminal only)")
     check(all("restored the session context" not in p["summary"]
               for p in detail.get("progress", [])),
           "the resume note stays OUT of the progress feed (F8: status is not progress)")
@@ -5401,6 +5423,10 @@ def bundle_goals(ctx):
     state = goal_state(goal1)
     check(task_state(auth, run_task)["status"] == "blocked",
           "the goal-run task parks on the plan review")
+    # U3's plan-review counterpart: the run-task card never reads as a
+    # bare "blocked" while a review-shaped block parks it.
+    check(task_state(auth, run_task)["display"] == "awaiting your review",
+          "the run-task card reads 'awaiting your review' during the plan review (U3)")
     pending = state.get("plan_pending_block")
     check(pending and "# Plan v1" in (pending.get("body") or ""),
           "the pending plan review carries the submitted markdown")
@@ -5830,6 +5856,12 @@ def bundle_goals(ctx):
           "the pending report lives ONLY in its block (goal row clean, S1)")
     check(task_state(auth, run_task)["status"] == "blocked",
           "the goal-run task parks on the completion review")
+    # Plan 16 S5 (U3): label reconciliation — the goal awaiting its
+    # completion review reads "awaiting your review" and the run-task
+    # status card agrees (never a bare "blocked" for a review park).
+    check(goal_state(goal1)["display"] == "awaiting your review"
+          and task_state(auth, run_task)["display"] == "awaiting your review",
+          "the goal and the run-task card agree: 'awaiting your review' (U3)")
     check(state["compare_url"].endswith("/compare/main...goal/flange"),
           "the completion review links the compare view")
     # Reject first: feedback returns the goal to active. Plan 16 S2:
@@ -7105,6 +7137,46 @@ def bundle_goalui(ctx):
     check("comment.is_system" in task_page and "system-label" in task_page,
           "the task page renders system rows with the same style")
 
+    print("== S5 U2: mechanical rows filtered from user-facing timelines ==")
+    # The daemon's mechanical system entries (the park notice, the
+    # restore footnote) ride the wire typed 'system': the raw terminal
+    # row STORES (session_commands), but no comment row, narrative
+    # event or dashboard timeline row is authored for them.
+    open_run = open_session(run_task)
+    mechanical = [
+        {"id": 9501, "command": "saddle suspend",
+         "output": "goal agent parked until an owned task completes or"
+                   " fails; resume via /start resume_notice",
+         "exit_code": 0, "type": "system", "timestamp": "2026-09-12T00:00:00Z"},
+        {"id": 9502, "command": "harness resume",
+         "output": "[harness] session context restored — task resumed",
+         "exit_code": 0, "type": "system", "timestamp": "2026-09-12T00:00:01Z"},
+    ]
+    status, _ = saddle_api("POST", "/api/saddle/results", fake_token,
+                           {"session_id": open_run, "state": "suspended",
+                            "entries": mechanical})
+    check(status == 200, "the mechanical system entries push cleanly")
+    conn = db()
+    raw_rows = conn.execute(
+        "select count(*) from session_commands where session_id = ?"
+        " and type = 'system'", (open_run,)).fetchone()[0]
+    conn.close()
+    check(raw_rows >= 2,
+          "the raw terminal keeps the mechanical rows (typed entries store)")
+    check(all("parked until" not in c["body"]
+              and "session context restored" not in c["body"]
+              for c in task_state(auth, run_task)["comments"]),
+          "no mechanical comment row lands on the thread (U2)")
+    check(all("parked until" not in e["text"]
+              and "session context restored" not in e["text"]
+              for e in goal_state(goal)["events"]),
+          "no mechanical narrative event lands on the goal page (U2)")
+    dash_events = slot_public(auth, "/", "dashboard")["activity"]
+    check(all("parked until" not in e["text"]
+              and "session context restored" not in e["text"]
+              for e in dash_events),
+          "no mechanical row lands on the dashboard timeline (U2)")
+
     print("== C4: head line, answered-blocks expander, footnote comments ==")
     state = goal_state(goal)
     check(state["current_activity"] == "parked awaiting its tasks' terminal transitions",
@@ -7147,6 +7219,13 @@ def bundle_goalui(ctx):
             conn.close()
     check(completion_block_count() == 1,
           "the done state raises exactly ONE completion block")
+    # Plan 16 S5 (U1): the verdict-shaped block renders the rich review
+    # form — explicit accept/reject affordances with the free-text
+    # feedback path, never a free-text-only verdict.
+    goal_page = http_get_page(auth, "/goals/%d" % goal)
+    check("Accept goal" in goal_page and "Reject with feedback" in goal_page
+          and 'name="feedback"' in goal_page,
+          "the completion review presents explicit accept/reject + free-text feedback (U1)")
     # The burst shape: the done ENTRY and the done STATE in one push
     # (fresh entry ids), then the whole pair redelivered — the
     # pending-report idempotency leaves the one row alone.