Repository navigation
feat(goals): carry rejection into bounded repairs (#606) - #608
PeterGuy326 wants to merge 15 commits into
Conversation
Current-head CI and merge handoffPR #608 is frozen at 1a4702f. All 11 executed checks passed, including both complete Node 24 jobs, CodeQL, dependency review, scorecard, both staging smokes, both native installers and layout parity.
No independent approval is recorded. Bindy-lbb and sun-970 have been requested through the standard review mechanism. GitHub reports REVIEW_REQUIRED; auto-merge is disabled. Author self-approval or administrator bypass would violate the inherited contribution/engineering policy, so no merge was attempted without the required review. After an eligible reviewer approves this exact head, re-read the head/base/checks and squash-merge using --match-head-commit. Do not publish a desktop release as part of this change. Maintenance disposition: four original PRs closed (#563/#505 absorbed; #560/#524 retired implementations with requirements retained), two completed implementation issues closed (#347/#511), eight original PRs and 39 original issues retained with reasons in #607. Current totals are 9 open PRs and 41 open issues, including new #606/#607/#608. #606 and #607 remain open because the merge gate is outstanding. Responsibilities remain: general execution/review in RoleWeave; reusable business implementations in workbench-examples; enterprise facts, ontology and evidence in Enterprise Context. The first real factory pilot remains receivables reconciliation and customer follow-up with human confirmation; no customer data, financial writes or automatic messages were introduced here. |
PeterGuy326
left a comment
PeterGuy326
left a comment
There was a problem hiding this comment.
Reviewed at exact head 1a4702fe (base 471beb3e). The design is sound: repair context is derived only from existing acceptance/execution records, the attempt ceiling is read from the durable ledger, history-read failures fail closed, and completion still requires a human verdict. I cannot approve this PR because I am the author of record, so this is a review comment only; approval must come from another CODEOWNER.
Blocking
1. apps/server/src/goals/rework.ts:24-28 — group turns that reference a rejected task now hard-fail with 409.
executeTurn calls prepareGoalRework(..., group ? undefined : session?.sessionId) (apps/server/src/routes/turns.ts:257), so for every group spawn sessionId is undefined. The third clause rejection.source.sessionId !== sessionId is then unconditionally true, and the function throws goal_conflict instead of returning [].
Group requests are allowed to carry goalId/branchId (apps/server/src/routes/groups.ts:105-190) and the spawn body persists them (apps/server/src/routes/turns.ts:432-433). So a group message bound to a task whose latest verdict is rejected now fails before any model call, where it previously ran normally. Repair injection is a personal-session feature; a group turn is not attempting a repair, so it should be skipped rather than rejected.
Suggested fix: return [] when sessionId === undefined (or when group !== undefined) before the ownership checks, and keep the 409 for the personal-session case where the session genuinely differs from rejection.source.sessionId.
2. Same block — no test covers the group path. All nine fixtures go through POST /sessions/:id/turns. Please add one case that posts a group turn (or calls prepareGoalRework with sessionId === undefined) against a rejected task and asserts the previous behaviour (dispatch, no repair context) or the explicitly intended 409.
Should fix
3. apps/server/src/goals/rework.ts:29-32 — goal.updatedAt is the record clock, not a plan revision.
At decision time recordAcceptance sets updatedAt = decidedAt (apps/server/src/goals/store.ts:607-615), so the equality holds initially. But updatedAt is also bumped by getDetail whenever computed health changes (apps/server/src/goals/store.ts:455-478), i.e. by a plain GET /goals/:id after a failed or indeterminate turn, and by any update(). Nothing on the plan changed in those cases, yet every later repair dispatch returns 409 with "repair criteria or goal state changed; review the plan before continuing", and the only way back is to record another verdict. Today this is latent because branches is never populated (so computeHealthFromTurns returns early), but the coupling becomes a real trap the moment branch health is persisted.
Suggestion: store a plan fingerprint on the acceptance record (criteria snapshot is already there — extend it with the work-item assignee/status rows and the branch set) and compare that, instead of the whole-record updatedAt.
4. apps/server/src/goals/rework.ts:40-42 — the ceiling and its message duplicate the constant. MAX_TASK_REPAIRS = 3 is exported, but the operator-facing text hardcodes "(3)". Derive the number from the constant so the two cannot drift.
Notes
reportRecordsthrows once the workspace ledger exceedsMAX_HISTORY_BYTES, so a large workspace can never repair (500, no dispatch). Fail-closed is correct; worth stating as a known limitation in the docs paragraph rather than only in tests.taskTurnscounts every turn bound to the task, including group spawns that per (1) can never be repairs. Once (1) is resolved those turns will still consume the ceiling; consider counting only turns whoseconversationRef === sessionId.- The 8 KiB guard covers
feedbackonly.outputis capped separately by the assembler (MAX_FIELD_BYTES), and the docs already state that excerpts may be truncated — no change requested, just confirming the boundary is intentional.
Verified
- Nine new tests exercise real HTTP/session/store boundaries: limit across store restarts, foreign session, changed criteria, unavailable ledger, redaction, concurrent dispatch, digest tamper, failed attempts, oversize feedback. The
[200, 409]concurrency assertion matches therunningTurnsreservation semantics I checked inexecuteTurn. runtime-layout.cjsaddsdist/src/goals/rework.jstoSERVER_RUNTIME_FILES, so the packaged server imports the new module rather than failing at runtime.- CHANGELOG entry under
[Unreleased]usesRefs #606; docs section scopes the feature as human-started, not an autonomous loop. No secrets, no customer data, no dependency changes in the diff. - All 11 executed checks pass at
1a4702fe, including both Node 24 legs, CodeQL, dependency review, scorecard, both staging smokes, both unsigned installers and layout parity.
Approval still required from a non-author CODEOWNER (Bindy-lbb or waterbro-8) at head 1a4702fe; no administrator bypass was attempted.
Refs #606 (R1). Bind repair eligibility to the saved plan rather than health refresh clocks; retain legacy acceptance reads and count personal repair attempts only.
PeterGuy326
left a comment
PeterGuy326
left a comment
There was a problem hiding this comment.
Re-reviewed at c04c7ac (the two follow-up commits after 1a4702fe). All three findings from review 5563796272 are resolved, and I could not reproduce a new blocking path.
1. Group regression (was blocking) — fixed. apps/server/src/routes/turns.ts now passes group ? undefined : session?.sessionId, and prepareGoalRework returns [] before touching the ledger when sessionId is undefined, so group turns keep the ordinary execution path. Covered by "a group turn bound to a rejected task still dispatches without repair context" and "group spawns do not consume the personal repair ceiling".
2. Plan-unchanged predicate (was should-fix) — fixed properly. goalPlanDigest excludes branch createdAt/updatedAt and is persisted on the acceptance record (store.ts), so a health refresh no longer invalidates a repair while real edits to criteria, work items or the branch plan do. The legacy fallback (planDigest === undefined → goal.updatedAt === rejected.decidedAt) is conservative, documented in docs/thread-context-and-collaboration.md, and tested ("legacy rejection records remain readable and retain conservative revision checks").
3. Hard-coded limit message (was should-fix) — fixed. The 409 message is built from MAX_TASK_REPAIRS.
Two things I checked beyond my previous review and that hold up:
- The full-ledger
turnStore.reportRecordsread and the 8 KiB feedback cap only run after a savedrejecteddecision exists, so ordinary turns that merely carrygoalId/branchIdneither pay the ledger read nor inherit the fail-closed behaviour on an oversized ledger. The "unavailable execution history fails closed" and "oversized feedback" tests pin both. - Concurrency: "concurrent repair requests dispatch once and require a fresh human verdict" asserts exactly one 200 and one 409 with a single model call, which is the failure mode that would otherwise double-spend a repair attempt.
No blocking findings from me at c04c7ac. Since the author is PeterGuy326, this review cannot carry the approval: per #607 AC-004 this needs one independent approval on this exact head from Bindy-lbb or sun-970, then squash with --match-head-commit. If a follow-up commit lands, this review is stale and the new head needs its own verification.
|
Current review head: fc12e8d. Integrated current main (#612/#616) with ordinary merges; full final-source local gate PASS 3054, 0 failed, 3 existing conditional skips, audit 0. Hosted CI is running; previous-head checks are historical only. @Bindy-lbb @sun-970 please review this exact head after its required checks pass. The maintainer authorized normal guarded squash merge after independent approval; no author self-approval or protection bypass. |
|
Integrated approved main #614 (2dc7764) without conflicts or history rewrite. Current review head d45cff4. Final-source full local check: 3071 PASS, 0 FAIL, 3 existing conditional skips, audit 0. All 11 preceding-head checks passed but are historical now; new CI is running. Existing independent reviewer requests are retained; no approval is supplied by the author. Scope remains the generic #606 repair path, with no duplicate task store or business-case logic. |
|
Main advanced through independently approved #506 (8930e45). Integrated it without conflicts or history rewrite; current head 85a6d24. Fresh install and full final-source check: 3082 PASS, 0 FAIL, 3 existing conditional skips, audit 0. New hosted CI is running; independent non-author approval remains required on this head. The #606 scope and existing review requests are unchanged. |
|
Approved main #618 advanced the relay/thread-context path. Integrated it by ordinary conflict-free merge; current head d506942. Re-inspected the shared dispatch seam: group repair skip and shipped relay handoff remain separate; no duplicate task/context store. Fresh npm ci + full final-source gate PASS 3097, 0 failed, 3 existing conditional skips, audit 0. New CI is running; independent approval is still needed on this exact head. Consolidated the PR body so the current evidence is clear; previous receipts remain in comments/#607. |
Review 结论：建议修复 3 个问题后再合并审阅固定版本： 1. [P1 / High] 失败返工的普通“重试”会绕过反馈注入和三次上限位置：routes/turns.ts:259–261，关联 rework.ts:16。
复现：完成原始任务 → 人工退回 → 第一次返工超时 → 连续重试 5 次。5 次请求全部返回 HTTP 200，模型累计调用 7 次（初次交付 + 首次返工 + 5 次重试），5 次重试均没有 建议：先从同会话的可信 retry source 解析任务身份，再执行返工校验。失败返工的重试应明确遵循返工策略：保留反馈并计入限额，或明确拒绝；不能退化成普通未绑定会话而跳过限制。请补实际 UI 请求形状（仅 2. [P1 / High] 返工进入工具审批后，审批恢复不能产出可验收的同任务结果位置：rework.ts:47–51，关联新增的执行入口 hook。 精确 head 复现：人工退回 → 返工产生 当前 main 集成预览复现：#620 会为审批恢复补回任务 briefing/绑定，此时恢复进入这里，但 建议：将批准后的 continuation 绑定为同一次返工，恢复其原退回来源和反馈，按审批规则重新验证；不要要求先验收一个尚待工具批准、没有完成交付的回合，也不要把恢复重复算作一次新返工。请覆盖单项和批量审批恢复。 3. [P2 / Medium] 反馈的 8-KiB 检查在过滤之前，过滤扩容后会静默丢失 verdicts 和原因位置：rework.ts:58–63。 此处检查原始 JSON 的大小，随后 shared context assembler 才过滤凭据，并把每个字段截到 8 KiB。替换为 复现：使用合法的单条 criteria： 建议：使用实际的同一过滤器处理完整 feedback 后再校验 UTF-8 字节数；无法完整携带时明确拒绝/要求拆分，不依赖后续静默截断。补一条过滤后扩容的边界回归。 验证与合并前事项
本次结果仅发布在 PR 评论，未向该分支推送代码或文档改动。 |
|
Integrated main through #620/#624 to cdf8998, preserving both imports/runtime modules. Added a real HTTP integration regression: retry restoring saved briefing IDs must still enforce repair review. It failed before the ordering fix (200 instead of409); moving the existing guard after server-owned binding restoration makes it pass without another model call. Focused57/57 and restarted full final-source gate3131PASS/0FAIL/3existing skips/audit0. New hosted CI and independent approval pending. Existing reviewer requests retained; no self-approval or bypass. Pending #606 changelog entries stay Unreleased. |
PeterGuy326
left a comment
PeterGuy326
left a comment
There was a problem hiding this comment.
Review at current head a4176fc
Re-reviewed the current head rather than carrying forward the 10-19 verdict, since main has moved through #610/#612/#614/#616/#618/#620/#622/#624/#626/#628 since then.
Scope check. The feature diff against main is eight files: apps/server/src/goals/rework.ts (new, 64 lines), goals/store.ts (+goalPlanDigest), routes/turns.ts (+5), packages/shared/src/goals.ts (+6-1), the test file, CHANGELOG.md, runtime-layout.cjs, and the collaboration doc. No second task store, no second acceptance store, no pilot business code — consistent with #607 AC-004.
The three findings from my 10-18/10-19 reviews are still fixed at this head:
prepareGoalReworkreturns[]before any work whensessionId === undefined, andturns.tspassesgroup ? undefined : session?.sessionId. A group turn therefore dispatches normally instead of hittinggoal_conflict— the 409 regression I reported is gone, and it is now pinned by the test "a group turn bound to a rejected task still dispatches without repair context".goalPlanDigestexcludesbranches[].createdAt/updatedAt("Bind repairs to the human plan, excluding health and record refresh clocks"), andplanDigestis written into the acceptance record at decide time with a conservativegoal.updatedAtfallback for legacy records. The health-bump pollution path I flagged is closed, and "health refreshes preserve repair eligibility but task plan edits invalidate it" / "legacy rejection records remain readable and retain conservative revision checks" cover both branches.MAX_TASK_REPAIRSreplaces the hardcoded 3, and the ceiling is reported back to the model asrepairAttempt/maxRepairsin the feedback payload.
What I re-verified beyond the prior review (this is the part that changed under me):
- Group spawns cannot consume the personal repair ceiling: the repair query filters on
conversationRef === sessionId && conversationId === sessionId && !turn.groupRef, and the group path never reaches it. Covered by "group spawns do not consume the personal repair ceiling". - The ordering fix from the
#620integration still holds: the guard runs after the server-owned retry identity is restored from the saved briefing (turns.tscomputesrepairContextaftertaskBriefingBinding), which is exactly the regression that produced 200-instead-of-409 in the earlier iteration, and it is now pinned by "briefing retry cannot omit its restored task binding to bypass repair review". - Fail-closed behaviour on an unavailable ledger (
reportRecordsrejection propagates rather than resetting the attempt count) and the 8 KiB feedback bound are both present and tested. - The repair context is appended to
supplementalContextafterassertTurnWorkspace, so it inherits the existing context-assembly and sanitisation boundary rather than adding a parallel path.
Verdict: no blocking findings from me at a4176fc.
All 11 hosted checks are green on this exact head (Node 24 ubuntu/macos, CodeQL javascript-typescript, Dependency review, OpenSSF Scorecard, layout parity, both staging smokes, both unsigned installer legs).
The remaining gate is not technical. It is the independent exact-head approval required by #607 AC-004. I am the author of this PR, so my approval cannot satisfy it and I will not use administrator bypass. Bindy-lbb or sun-970 needs to approve this exact SHA; if the head moves again, that approval has to be repeated on the new head.
Delta review after baseline integrationMy last review covered
I also ran |
Refs #606 R1. Address independent review findings for legacy retry binding, durable approval continuations and sanitized feedback bounds.
|
Addressed the three findings from the independent review in
Before-fix evidence ( Please re-review this exact head after its hosted CI completes. Existing non-author review requests are retained. The author has not approved or merged this PR; normal exact-head guarded squash merge remains contingent on all required checks and independent approval. No task/context/approval store, dependency upgrade or business-case implementation was added. |
PeterGuy326
left a comment
PeterGuy326
left a comment
There was a problem hiding this comment.
Re-reviewed at 8af40a00 (the new fix: preserve repair guards across retries and approvals commit on top of the 48692cff baseline I approved conceptually yesterday). I cannot approve my own PR, so this is a substantive comment only — approval still has to come from @Bindy-lbb or @sun-970 at this exact head.
What the new commit does, verified
- One repair, one count across tool pauses.
linksmapsapproval.execution.turnId → approval.source.turnIdandrepairsnow excludes linked turns, so a repair that pauses for a tool approval and resumes is counted once instead of twice. The link is only accepted when the source turn actually carries the matchingapproval.requestedevent for the samerunId, so a forged or cross-run link cannot shrink the counter. Good. - Continuation is authenticated, not asserted.
links.get(continuation.turnId) !== continuation.sourceTurnIdrejects a continuation that claims an unrelated source, the walk detects cycles, and the chain root must be at or afterrejection.decidedAt— an old paused turn cannot be resumed after a newer human rejection. - Delivery comparison stays honest. Paused turns are excluded from
projectTaskDeliveriesfor the guard, so the digest check still compares the last real delivery againstrejection.sourcerather than against the half-finished paused turn. Traced the third-repair test by hand: projection lands on the previous repair turn, which is exactly what the rejection points at. - Sanitisation happens before the size cap.
feedbackis sanitised first, then measured against 8 KiB, so redaction expansion cannot silently truncate a verdict list. TheJSON.parseguard after sanitisation fails closed if redaction breaks the structure, andredactedis propagated so the receipt reflects that the dispatcher did not send the raw text. - Retry identity restoration. Forcing
goalId/branchIdback from the original turn — and 400-ing when a retry explicitly asks for a different task — closes the "omit the task binding to skip repair review" hole for legacy turns without a briefing. The new test parameterisation overbriefing: false/truecovers that path.
Tests: the matrix (batch × briefing × changedPlan) plus the two sanitisation cases read as genuinely adversarial rather than confirmatory. The changedPlan branch asserting inputs.length === 4 (the continuation is refused, no extra model call) is the assertion I would have asked for.
Non-blocking follow-ups
linksconstruction is O(approvals × taskTurns): one linearfindper approval over the whole branch turn list, on every repair attempt. Anew Map(taskTurns.map(t => [t.turnId, t]))lookup would make it linear and is a two-line change.- If
readApprovals()throws (corrupt or over-capacity ledger), the caller surfaces the raw store error rather than agoal_conflict409, so the operator sees an opaque failure instead of the "ledger unavailable for repair" wording documented indocs/thread-context-and-collaboration.md. Mapping it to a conflict message would match the documented contract. continuation ? repairs.length > MAX_TASK_REPAIRS : repairs.length >= MAX_TASK_REPAIRSis correct but reads as an off-by-one. Worth a one-line comment stating that the in-flight continuation is already insiderepairs, otherwise the next reader will "fix" it back to>=.- Product question, not a defect: granting an approval whose source turn predates the latest rejection now returns 409 from the root check. That is defensible (a rejection invalidates older work) but the already-accepted approval is left unresumable; if operators are expected to be able to clear stale pending approvals, that path needs an explicit exit (e.g. auto-expire) instead of a hard conflict on execution.
Verdict: no code change requested from me. Blocking item remains a non-author approval at 8af40a00.
Outcome and scope
Closes #606. Consumed revision: R1. Maintenance decisions: #607.
When an operator reruns a rejected project task in its assigned employee's original personal session, the executor includes the saved human verdicts, rejection reason and exact rejected answer. Every repaired result still requires human acceptance. A durable three-attempt ceiling includes failures, survives restart and excludes group turns and tool-approval continuations.
Candidate:
8af40a004a64794dc3d49dab230d149da2cc8bd1, based on main0ce175ae85763a7099800baf56056f0198af3d70(including #634). Existing branch/history are preserved. Feature entries remain Unreleased; this PR does not publish a desktop release.Independent review follow-up
Addresses all three findings in Bindy-lbb's independent review:
The pre-fix HTTP reproduction returned 200 instead of 409 for legacy retries and sanitizer expansion; the briefing approval continuation became indeterminate, while an unbriefed continuation lost its task binding. Logs are retained outside the checkout. New tests cover both legacy/briefed requests, single/batch approval at the third attempt, plan changes during approval, restart readback and malformed/expanded sanitized feedback.
Acceptance and validation
work/roleweave-maintenance-20261018/608-review-focused.log.work/roleweave-maintenance-20261018/608-review-final-check.log.npm audit --audit-level=highexits 0, with 2 moderate findings inherited unchanged from main, 0 high/critical. This is not a zero-finding audit. No dependency change/force-upgrade was bundled.git diff --checkpasses; final checkout is clean.8af40a004a64794dc3d49dab230d149da2cc8bd1, including both Node 24 gates, CodeQL, dependency review, Scorecard, staging/installers and layout parity. Independent re-review/approval remains pending.No coverage percentage, live-model quality, cross-platform local execution or real customer acceptance is claimed. No gate was disabled.
Boundaries and rollback
General execution and review stay in RoleWeave; reusable business cases stay in workbench-examples; enterprise facts, ontology and evidence stay in Enterprise Context. No business workflow, scheduler, automatic message sending or financial write is introduced.
Feedback is untrusted data, never tool authorization. Oversized/unavailable execution or approval history prevents repair. Visible answer excerpts may be truncated by the existing assembler with receipts; mandatory sanitized feedback may not. New acceptance records add optional planDigest; older records remain readable. When reverting execution behavior, retain the additive acceptance reader and preserve audit history rather than deleting records to downgrade.
Checklist
Normal squash merge with
--match-head-commitand completed-branch deletion is authorized once every gate passes. No author approval, protection changes or administrator bypass. AI-assisted implementation; no automated Git co-author.