Jelajahi Sumber

s2: reject-flow fixed and verdict wording server-owned — submitPlanRemarks resolves the block from the actual [data-plan-review] container (any open block, not just goal.plan_pending_block — the completion-review reject button), remark-less rejection allowed when the feedback textarea has content, and the goal page posts verdict markers / raw feedback while the SERVER wraps the stable per-kind prefixes (hidden-input constants removed from every template; approvals byte-identical, goal feedback keeps the prefix + blank-line shape, task feedback its single-newline shape).

clanker 1 bulan lalu
induk
melakukan
f3e779df22

+ 26 - 3
src/actions/GoalActions.vala

@@ -336,9 +336,15 @@ namespace ClankerSaddle {
      * {@link GoalService.answer_block} is the ONE service path every
      * surface (goal page, task-page deep link, completion accept)
      * funnels into — a single row settles in one write, so the pile
-     * card cannot outlive its answer. An empty reply with a pending
-     * option list selects the first option (the plain reply form's
-     * affordance).
+     * card cannot outlive its answer. The parked-resume machinery
+     * then wakes the agent with the verdict as its resume_notice.
+     * Plan 16 S2/R4: the server owns the verdict wording — the goal
+     * page's forms post a `verdict` marker (`accept`) or raw
+     * `feedback` (never the stable constants); the block's KIND picks
+     * the stable string (plan vs completion/review), stored
+     * byte-identically to the wording the daemon's resume matching
+     * reads. An empty reply with a pending option list selects the
+     * first option (the plain reply form's affordance).
      */
     public class AnswerGoalBlockAction : TypedStatumAction<GoalBlockAnswerPrivate> {
 
@@ -366,9 +372,26 @@ namespace ClankerSaddle {
 
             string answer = Forms.field(request, "answer");
             string option = Forms.field(request, "option");
+            string verdict = Forms.field(request, "verdict");
+            string feedback = Forms.field_raw(request, "feedback");
             if (answer.length == 0 && option.length > 0) {
                 answer = option;
             }
+            if (answer.length == 0
+                    && (verdict.length > 0 || feedback.strip().length > 0)) {
+                var block = yield goals.get_block(block_id);
+                bool plan_kind = block != null
+                    && ((!)block).block_type == GOAL_BLOCK_PLAN;
+                if (verdict == "accept") {
+                    answer = plan_kind
+                        ? NOTICE_PLAN_APPROVED : NOTICE_REVIEW_APPROVED;
+                } else if (feedback.strip().length > 0) {
+                    // The goal page's byte shape: the stable prefix, a
+                    // blank line, then the client-composed remarks
+                    // body (numbered remarks, quotes, extra text).
+                    answer = @"$(plan_kind ? NOTICE_PLAN_FEEDBACK : NOTICE_REVIEW_FEEDBACK)\n\n$(feedback.strip())";
+                }
+            }
             if (answer.length == 0) {
                 return directives().notify("error", "An answer is required")
                     .navigate(@"/goals/$(goal_id)");

+ 26 - 13
src/actions/TaskActions.vala

@@ -231,10 +231,13 @@ namespace ClankerSaddle {
      * Review and plan blocks submit a `feedback` reply which is stored
      * wrapped in the stable verdict wording ("Review feedback
      * received:\n…" / "Plan feedback received:\n…") — the answer is the
-     * resume_notice the daemon reads. Accepting a plan (the stable
-     * approval wording) also applies the submitted plan markdown to
-     * task.plan. Question/RFC answers are stored verbatim — the
-     * resume-notice builder applies the "Human selected: …" formatting.
+     * resume_notice the daemon reads. Plan 16 S2/R4: accept forms post
+     * a `verdict` marker (`accept`) instead of the stable constants —
+     * the server resolves the block kind and owns every verdict string.
+     * Accepting a plan (the stable approval wording) also applies the
+     * submitted plan markdown to task.plan. Question/RFC answers are
+     * stored verbatim — the resume-notice builder applies the "Human
+     * selected: …" formatting.
      */
     public class AnswerBlockAction : TypedStatumAction<BlockAnswerPrivate> {
 
@@ -260,22 +263,32 @@ namespace ClankerSaddle {
             string other = Forms.field_raw(request, "other");
             string answer = Forms.field_raw(request, "answer");
             string feedback = Forms.field_raw(request, "feedback");
+            string verdict = Forms.field(request, "verdict");
             if (option == "__other__") {
                 answer = other.strip().length > 0 ? other.strip() : answer;
             } else if (option.length > 0) {
                 answer = option;
-            } else if (feedback.strip().length > 0) {
-                var block = yield tasks.get_task(task_id);
+            } else if (verdict == "accept" || feedback.strip().length > 0) {
+                // S2/R4: the server owns the verdict wording — the
+                // block's kind picks the stable string.
                 string type = "";
-                foreach (var candidate in ((!)block).blocks) {
-                    if (candidate.id == ((!)ctx).block_id) {
-                        type = candidate.block_type;
-                        break;
+                var detail = yield tasks.get_task(task_id);
+                if (detail != null) {
+                    foreach (var candidate in ((!)detail).blocks) {
+                        if (candidate.id == ((!)ctx).block_id) {
+                            type = candidate.block_type;
+                            break;
+                        }
                     }
                 }
-                answer = type == BLOCK_PLAN
-                    ? @"$(NOTICE_PLAN_FEEDBACK)\n$(feedback.strip())"
-                    : @"$(NOTICE_REVIEW_FEEDBACK)\n$(feedback.strip())";
+                if (verdict == "accept") {
+                    answer = type == BLOCK_PLAN
+                        ? NOTICE_PLAN_APPROVED : NOTICE_REVIEW_APPROVED;
+                } else {
+                    answer = type == BLOCK_PLAN
+                        ? @"$(NOTICE_PLAN_FEEDBACK)\n$(feedback.strip())"
+                        : @"$(NOTICE_REVIEW_FEEDBACK)\n$(feedback.strip())";
+                }
             }
             if (answer.length == 0) {
                 return directives().notify("error", "An answer is required").navigate(@"/tasks/$(task_id)");

+ 45 - 27
src/pages/goal_detail.html

@@ -207,36 +207,56 @@
                 if (!review) return;
                 var id = review.getAttribute('data-block-id');
                 var list = remarksByBlock[id] || [];
-                if (list.length === 0) {
-                    if (window.showToast) window.showToast('Highlight some plan text and add a remark first', 'error');
+                var extra = review.querySelector('[data-plan-feedback] textarea[name="feedback"]');
+                var extraText = extra && extra.value.trim().length > 0 ? extra.value.trim() : '';
+                // Remark-less rejection is allowed when the feedback
+                // textarea carries the review's substance — one of the
+                // two must be non-empty (plan 16 S2).
+                if (list.length === 0 && extraText.length === 0) {
+                    if (window.showToast) window.showToast('Highlight some text and add a remark, or write feedback first', 'error');
                     return;
                 }
+                var block = resolveReviewBlock(id);
+                if (!block || !block.answer_action) return;
+                // The rejection body rides `feedback` and the SERVER
+                // wraps it with the per-kind stable prefix (plan 16
+                // S2/R4 — the template carries no verdict constants,
+                // any [data-plan-review] container resolves its own
+                // block). The body keeps the historical shape: every
+                // queued remark NUMBERED in queue order, each with its
+                // selection quote prefixed like a comment, then the
+                // free-text feedback.
+                var feedback = list.map(function (item, index) {
+                    return (item.quote
+                        ? '> ' + item.quote.replace(/\n/g, '\n> ') + '\n\n'
+                        : '') + (index + 1) + '. ' + item.body;
+                }).join('\n\n');
+                if (extraText.length > 0) {
+                    feedback += (feedback.length > 0 ? '\n\n' : '') + extraText;
+                }
+                remarksByBlock[id] = [];
+                rejectOpenByBlock[id] = false;
+                statum.act(block.answer_action, { feedback: feedback }).catch(function () { });
+            }
+
+            // Plan 16 S2: the block for a review container resolves
+            // from the ACTUAL [data-plan-review] element — the pending
+            // completion review renders as an attention block (NOT the
+            // plan section), so goal.plan_pending_block alone never
+            // resolves it; every open block plus the plan section's
+            // row are all candidates.
+            function resolveReviewBlock(id) {
                 var goal = statum.read('goal');
+                if (!goal) return null;
                 var block = null;
-                if (goal && goal.plan_pending_block
+                (goal.open_blocks || []).forEach(function (candidate) {
+                    if (String(candidate.id) === String(id)) block = candidate;
+                });
+                if (!block && goal.plan_pending_block
                         && String(goal.plan_pending_block.id) === String(id)) {
                     block = goal.plan_pending_block;
                 }
-                if (!block || !block.answer_action) return;
-                // Every queued remark rides the rejection NUMBERED, in
-                // queue order, each with its selection quote prefixed
-                // like a comment; the wrapped answer IS the resume
-                // notice the agent reads.
-                var answer = 'Plan feedback received:';
-                list.forEach(function (item, index) {
-                    answer += '\n\n';
-                    if (item.quote) {
-                        answer += '> ' + item.quote.replace(/\n/g, '\n> ') + '\n\n';
-                    }
-                    answer += (index + 1) + '. ' + item.body;
-                });
-                var extra = review.querySelector('[data-plan-feedback] textarea[name="feedback"]');
-                if (extra && extra.value.trim().length > 0) {
-                    answer += '\n\n' + extra.value.trim();
-                }
-                remarksByBlock[id] = [];
-                rejectOpenByBlock[id] = false;
-                statum.act(block.answer_action, { answer: answer }).catch(function () { });
+                return block;
             }
 
             function syncPlanRemarks() {
@@ -406,8 +426,7 @@
                              stm-markdown="block.body">report</div>
                         <div class="actions">
                             <form stm-action="block.answer_action" class="inline">
-                                <input type="hidden" name="answer"
-                                       value="Review approved. Continue with the task.">
+                                <input type="hidden" name="verdict" value="accept">
                                 <button type="submit" class="approve" data-plan-accept>Accept goal</button>
                             </form>
                             <button type="button" data-plan-toggle>Reject with feedback</button>
@@ -489,8 +508,7 @@
                          stm-markdown="goal.plan_pending_block.body">revision</div>
                     <div class="actions">
                         <form stm-action="goal.plan_pending_block.answer_action" class="inline">
-                            <input type="hidden" name="answer"
-                                   value="Plan approved. Proceed with implementation.">
+                            <input type="hidden" name="verdict" value="accept">
                             <button type="submit" class="approve" data-plan-accept>Approve plan</button>
                         </form>
                         <button type="button" data-plan-toggle>Reject with feedback</button>

+ 6 - 6
src/pages/task_detail.html

@@ -423,11 +423,11 @@
                     <p>Branch <strong stm-text="task.task_branch">branch</strong>
                         <a stm-if="task.review_diff_url.length > 0" stm-attribute.href="task.review_diff_url"
                            target="_blank" rel="noopener">· compare changes</a></p>
-                    <div class="actions">
-                        <form stm-action="block.answer_action" class="inline">
-                            <input type="hidden" name="answer" value="Review approved. Continue with the task.">
-                            <button type="submit" class="approve">Approve</button>
-                        </form>
+                     <div class="actions">
+                         <form stm-action="block.answer_action" class="inline">
+                             <input type="hidden" name="verdict" value="accept">
+                             <button type="submit" class="approve">Approve</button>
+                         </form>
                         <form stm-action="block.answer_action" class="inline">
                             <textarea name="feedback" rows="2" placeholder="What should change?"></textarea>
                             <button type="submit" class="reject">Request changes</button>
@@ -440,7 +440,7 @@
                     <div class="md rfc-body" stm-markdown="block.body" data-plan-body>plan</div>
                     <div class="actions">
                         <form stm-action="block.answer_action" class="inline">
-                            <input type="hidden" name="answer" value="Plan approved. Proceed with implementation.">
+                            <input type="hidden" name="verdict" value="accept">
                             <button type="submit" class="approve">Accept plan</button>
                         </form>
                         <button type="button" data-plan-toggle>Reject with feedback</button>

+ 112 - 16
test/web_stack_test.py

@@ -1293,16 +1293,16 @@ def bundle_pages(ctx):
 
     # Accept stores the stable approval wording as the answer (it
     # becomes the resume notice the daemon reads; task-level plans are
-    # gone — the goal owns the approved markdown).
-    act(auth, blocks["plan"]["answer_action"],
-        {"answer": "Plan approved. Proceed with implementation."})
+    # gone — the goal owns the approved markdown). Plan 16 S2/R4: the
+    # form posts the verdict MARKER — the server owns the constant.
+    act(auth, blocks["plan"]["answer_action"], {"verdict": "accept"})
     detail = task_state(auth, plan_task)
     plan_block = [b for b in detail["blocks"] if b["block_type"] == "plan"][0]
     check(plan_block.get("show_body") is True,
           "answered plan review shows the body again (review section gone)")
     check(plan_block.get("answered") is True
           and plan_block.get("answer") == "Plan approved. Proceed with implementation.",
-          "accept stores the stable approval wording (the resume notice)")
+          "verdict=accept stores the stable approval wording server-side (the resume notice)")
 
     print("== B1: highlight-to-comment quote dialog ==")
     status, page = http("GET", "/tasks/%d" % plan_task, timeout=15)
@@ -1329,12 +1329,21 @@ def bundle_pages(ctx):
           "pending review block hides its duplicate body preview")
     check(review_block.get("body", "").startswith("# The change"),
           "review body itself still arrives in state (B2 counterpart)")
-    act(auth, review_block["answer_action"],
-        {"answer": "Review approved. Continue with the task."})
+    act(auth, review_block["answer_action"], {"verdict": "accept"})
     detail = task_state(auth, plan_task)
     review_block = [b for b in detail["blocks"] if b["block_type"] == "review"][0]
     check(review_block.get("show_body") is True,
           "answered review shows the body again (banner gone)")
+    check(review_block.get("answer") == "Review approved. Continue with the task.",
+          "the review accept form's verdict marker resolves to the stable wording")
+    # Plan 16 S2/R4: no verdict constant ships in any template — the
+    # accept forms carry only the verdict marker.
+    task_page_html = http_get_page(auth, "/tasks/%d" % plan_task)
+    check("Plan approved. Proceed with implementation." not in task_page_html
+          and "Review approved. Continue with the task." not in task_page_html,
+          "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")
 
     print("== B2: question block body above options ==")
     # Contract side (A2 will restore bodies end-to-end): a question
@@ -4614,8 +4623,9 @@ def bundle_answers(ctx):
     plan_block = park_on_plan(accept_task, "# Steps\n\n1. Wire\n2. Test")
     check(plan_block.get("answered") is not True,
           "the plan block waits for a human verdict")
-    act(auth, plan_block["answer_action"],
-        {"answer": "Plan approved. Proceed with implementation."})
+    # Plan 16 S2: the accept form posts the verdict marker; the server
+    # builds the stable constant (byte-identical wire).
+    act(auth, plan_block["answer_action"], {"verdict": "accept"})
     check(wait_for(lambda: any(
         int(p.get("task", {}).get("id") or 0) == accept_task
         and p.get("session_id") != accept_parked_session
@@ -4719,8 +4729,7 @@ def bundle_answers(ctx):
     detail = task_state(auth, review_task)
     verdict_block = [b for b in detail["blocks"]
                      if b["block_type"] == "review"][0]
-    act(auth, verdict_block["answer_action"],
-        {"answer": "Review approved. Continue with the task."})
+    act(auth, verdict_block["answer_action"], {"verdict": "accept"})
     reviewer.kill()
     act(auth, saddle_card(auth, "resume-reviewer")["refresh"], {})
     check(wait_for(lambda: saddle_card(auth, "resume-reviewer")["status"] == "offline"
@@ -5431,10 +5440,11 @@ def bundle_goals(ctx):
           and (state["plan"] or "") == "",
           "goals.plan stays empty while the revision is only pending (accept-gated, S1)")
     # Approve: goal.plan = v2, status active, agent resumes and parks.
+    # Plan 16 S2: the accept posts the verdict MARKER — the server
+    # resolves the goal-plan kind and owns the constant.
     fake.goal_script = [("suspend",)]
     pending = state["plan_pending_block"]
-    act(auth, pending["answer_action"],
-        {"answer": "Plan approved. Proceed with implementation."})
+    act(auth, pending["answer_action"], {"verdict": "accept"})
     check(wait_for(lambda: goal_state(goal1)["status"] == "active" or False,
                    timeout=60, what="approved plan activates the goal") is not None,
           "approving the plan activates the goal")
@@ -5735,14 +5745,26 @@ def bundle_goals(ctx):
           "the goal-run task parks on the completion review")
     check(state["compare_url"].endswith("/compare/main...goal/flange"),
           "the completion review links the compare view")
-    # Reject first: feedback returns the goal to active.
+    # Reject first: feedback returns the goal to active. Plan 16 S2:
+    # the goal page's reject posts raw `feedback` — the SERVER wraps
+    # it with the completion-review prefix (byte-identical to the
+    # historical client-built answer).
     fake.goal_script = [("suspend",)]
     act(auth, state["completion_pending_block"]["answer_action"],
-        {"answer": "Not done — the docs are missing"})
+        {"feedback": "Not done — the docs are missing"})
     check(wait_for(lambda: goal_state(goal1)["status"] == "active" or False,
                    timeout=30, what="rejected completion reactivates") is not None,
           "rejecting the completion returns the goal to active")
     conn = db()
+    rejected_answer = conn.execute(
+        "select answer from task_blocks where task_id = ?"
+        " and type = 'completion' order by id desc", (run_task,)).fetchone()[0]
+    conn.close()
+    check(rejected_answer
+          == "Review feedback received:\n\nNot done — the docs are missing",
+          "the completion rejection wraps feedback with the stable prefix (%r)"
+          % rejected_answer)
+    conn = db()
     rejected_report = conn.execute(
         "select completion_report from goals where id = ?", (goal1,)).fetchone()[0]
     conn.close()
@@ -5825,8 +5847,11 @@ def bundle_goals(ctx):
     check(no_push_column,
           "no stored push snapshot exists (F9: the goal row never records it)")
     state = goal_state(goal1)
+    # The passing accept posts the verdict MARKER (the goal page's
+    # accept form shape, plan 16 S2) — the server resolves the
+    # completion kind and stores the stable approval wording.
     act(auth, state["completion_pending_block"]["answer_action"],
-        {"answer": "Review approved. Continue with the task."})
+        {"verdict": "accept"})
     check(wait_for(lambda: goal_state(goal1)["status"] == "complete" or False,
                    timeout=30, what="accepted completion completes") is not None,
           "accepting the completion completes the goal once origin is current")
@@ -7039,7 +7064,7 @@ def bundle_goalui(ctx):
           "the done entry + state pair raises nothing more (one block, ever)")
     state = goal_state(goal)
     act(auth, state["completion_pending_block"]["answer_action"],
-        {"answer": "Review approved. Continue with the task."})
+        {"verdict": "accept"})
     check(wait_for(lambda: goal_state(goal)["status"] == "complete" or False,
                    timeout=30, what="goalui goal completes") is not None,
           "accepting the completion completes the goal")
@@ -7053,6 +7078,77 @@ def bundle_goalui(ctx):
     check(stragglers == 0,
           "no unanswered straggler rows survive the completion")
 
+    print("== S2: server-owned verdicts + the container-resolved reject ==")
+    # A fresh goal drives the verdict paths the goal page's forms now
+    # use: verdict markers for accept, raw `feedback` for reject, and
+    # the block resolution living in the review CONTAINER (not
+    # goal.plan_pending_block — the diagnosed completion-review bug).
+    fake.goal_script = [("plan", "# S2 plan\n\nOne step")]
+    act(auth, slot_public(auth, "/goals", "goals")["create"], {
+        "project_id": "1", "title": "S2 verdict goal",
+        "brief": "Covers the plan-16 S2 verdict paths.",
+        "branch_name": "goal/s2-verdicts"})
+    conn = db()
+    s2_goal = int(conn.execute(
+        "select id from goals where title = 'S2 verdict goal'").fetchone()[0])
+    conn.close()
+    act(auth, goal_state(s2_goal)["start"], {})
+    check(wait_for(lambda: goal_state(s2_goal)["status"] == "plan_review" or False,
+                   timeout=90, what="S2 goal enters plan review") is not None,
+          "the S2 goal enters plan review")
+    s2_state = goal_state(s2_goal)
+    s2_pending = s2_state["plan_pending_block"]
+
+    # (1) A remark-less, text-less rejection is refused: the action
+    # errors and the block keeps waiting.
+    refusal = act(auth, s2_pending["answer_action"], {"feedback": "   "})
+    check(any(d.get("type") == "notify" and d.get("kind") == "error"
+              and "answer is required" in (d.get("message") or "").lower()
+              for d in refusal),
+          "a remark-less, text-less rejection is refused with an error")
+    conn = db()
+    s2_still_open = conn.execute(
+        "select answer from task_blocks where id = ?",
+        (s2_pending["id"],)).fetchone()[0]
+    conn.close()
+    check(s2_still_open is None,
+          "the refused rejection leaves the block unanswered")
+
+    # (3) The goal page template ships no verdict constants and its
+    # script resolves the review block from ANY [data-plan-review]
+    # container via the open-blocks list (the completion-review fix).
+    # Fetched while the review is open so the accept form renders.
+    s2_page = http_get_page(auth, "/goals/%d" % s2_goal)
+    check("Plan approved. Proceed with implementation." not in s2_page
+          and "Review approved. Continue with the task." not in s2_page,
+          "the goal page ships no verdict constants (server-owned wording)")
+    check('name="verdict" value="accept"' in s2_page,
+          "the goal page's accept forms post the verdict marker")
+    check("resolveReviewBlock" in s2_page and "goal.open_blocks" in s2_page,
+          "the reject flow resolves its block from the open-blocks list (container-keyed)")
+    check("statum.act(block.answer_action, { feedback: feedback })" in s2_page,
+          "the reject flow posts raw feedback (the server wraps the prefix)")
+    check("extraText.length === 0" in s2_page,
+          "a remark-less rejection with textarea content is allowed (S2)")
+
+    # (2) The goal page's reject rides `feedback` (the client composes
+    # the numbered-remarks body) and the SERVER wraps it with the plan
+    # prefix — byte-identical to the historical client-built answer.
+    act(auth, s2_pending["answer_action"],
+        {"feedback": "> ## One step\n\n1. Tighten it\n\n2. Name the owner"})
+    conn = db()
+    s2_wrapped = conn.execute(
+        "select answer from task_blocks where id = ?",
+        (s2_pending["id"],)).fetchone()[0]
+    conn.close()
+    check(s2_wrapped
+          == "Plan feedback received:\n\n"
+             "> ## One step\n\n1. Tighten it\n\n2. Name the owner",
+          "the plan rejection wraps the client-shaped body with the stable prefix (%r)"
+          % s2_wrapped)
+    check(goal_state(s2_goal)["status"] == "planning",
+          "the feedback rejection returns the agent to planning")
+
 
 BUNDLES_ORDER = ("pages", "dispatch", "subtasks", "lineage",
                  "allocation", "lifecycle", "answers", "push", "goals",