Repository navigation
Continue initial planning through original role discovery - #1092
Conversation
PR #1092: Continue initial planning through original role discovery20 files, +722 / -256 Scope🔴 PR has 978 lines changed. Split into focused PRs. 🟡 978 lines changed. PRs under 400 receive more thorough review. Structural✅ No structural bloat detected. Slop✅ Slop indicators look low. Static Analysis✅ Oxlint found no issues. CorrectnessNo extraneous code patterns detected. |
taras
left a comment
There was a problem hiding this comment.
Claude Fable 5.1 — full feature review at 4f8872f59e3d2be8ccc927b8fda51f50b5fc9e34: changes required.
The reviewer inspected the complete successor diff against b45bb166, including production launch, Phase and Ticket routing, retained-state consumers, schema requirements and feature witnesses. This continues the same reviewer’s earlier test-repair inspection.
Blocking finding: preserve the accepted plan when the Implementor selects handoff documents.
examples/adversarial-implementation/components/Ticket/Advance.md:51 sets feedbackInputs to result.handoffInputs ?? state.assignment.handoffInputs, so the accepted plan reaches Architect/Planner reviewers only when the Implementor omits handoffInputs. Base run.md (b45bb16 lines 119/123) put plan.handoffInputs into manifest.acceptedInputs, delivered once to every session; new run.md:103 keeps acceptedInputs = task.inputs only. Trigger: a real Implementor on the planned route returns its own handoffInputs (Phase/Instructions.md:102 tells it to select documents), which replaces rather than joins the plan; reviewers told at Instructions.md:61 to review 'against the accepted plan' never receive it, and the plan path is also absent from the review assignment's artifacts. Evidence: after.log lines 429/656/866 show G1 planned scenarios pass only because the non-#1084 stub omits handoffInputs; the journey case passes because tests/components/Phase/Role.md:95 echoes assignment.handoffInputs. Smaller fix: union state.assignment.handoffInputs with result.handoffInputs at Advance.md:51 (or restore them as accepted inputs) and add one witness where the Implementor returns a different handoffInputs list; A1 and A5 both claim this delivery.
Nonblocking observations:
-
Advance.md:34 returns status 'running' with dispatchable=false and hold=null when a discovery execution failed after writing a canonical result (execution.status === 'failed'), while the implementation branch at Advance.md:49 labels the same case 'paused'. Continue then reports nextAction resolve_feedback_hold with no reason. The record is nondispatchable and inert, so no unauthorized work follows, but the status label is inconsistent with the README's paused/hold vocabulary.
-
Advance.md:30-31 recomputes the planning/architect-discovery continuation that Gate.md:21-22 already declares, and Gate.md:41 overrides assignment.continuation at dispatch, so the Advance-side continuation field is redundant state; deriving it only in Gate would remove one of two places that must agree.
-
Implementation.test.md now exposes one outer test named 'Implementation fixture journeys'; the G1/G6/G7/G8/A1-A4 group names and per-group deadlines are only visible inside the captured child output on failure, so a hosted failure report names the whole group rather than the failing journey. Diagnostic cost only; the Fail message still carries the child stdout/stderr.
-
A smaller composition exists outside this correction's declared boundary: passing --plugin packages/cli/tests/fixtures/workflow-test-permissions.mjs from packages/cli/tests/adversarial-implementation.test.ts for the Implementation suite would let Fixture.Implementation expand in-process and remove the remaining subprocess and the nested-Testing wrapper. Not required for the accepted behavior; noted for a later minimalism pass.
Validation standing: the provider execution completed, but the workflow rejected result extraction; the saved summary also has 1288 characters against its 1,200-character bound. This comment publishes the actual returned findings under the approved review/comment scope. It does not satisfy the independent-review gate or waive validation. No automatic retry or source edit occurred. Exact-head CI2392 passed; the blocking finding remains despite green CI.
taras
left a comment
There was a problem hiding this comment.
Claude Fable 5.1 — targeted repair closeout at e1033863703f812028813aaf56b73a5b47c3cb36: PASS, all six acceptance clauses covered; no blocking findings.
This continues the full feature review. The accepted-plan delivery blocker is closed. The reviewer inspected the three-file repair and traced the stable union of retained feedback documents, incoming assignment documents and newly selected Implementor documents through actual review delivery, per-session filtering, correction and retained continuation. The identical final regression fails against the parent production code and passes with the repair, including nonempty document contents and duplicate handling. Committed files match the passing focused candidates. The original Planner also passed this exact repair.
CI2399 passed all 44 runtime shards and the other required branch jobs; only main-green and the aggregate green failed because Main was red. The user explicitly directed delivery to proceed without waiting for Main health. This review pass does not waive branch protection or claim aggregate CI green. No additional test suite or live-provider qualification was needed for the closeout.
Nonblocking observations (the four earlier observations remain, plus one new unmeasured cap risk):
-
Advance.md:52 accumulates feedbackInputs across every Implementor completion, and Gate.md:41 joins that list into each reviewer's assignment.handoffInputs, which Phase/Schema.md:367 caps at maxItems 12 and Phase/Run.md:85 parses before any provider call. Base code bounded reviewer handoffInputs to one Implementor selection; after several multi-document corrections the union can exceed 12, making Phase.Run fail at parse and Settle hold with execution_failure. No unauthorized work or paid call occurs, so it is a truthful hold, but the cause would read as a provider failure. Unmeasured risk; the regression reaches four entries.
-
Advance.md:34 returns status 'running' with dispatchable=false and hold=null when a discovery execution failed after writing a canonical result (execution.status === 'failed'), while the implementation branch at Advance.md:50 labels the same case 'paused'. Continue then reports nextAction resolve_feedback_hold with no reason. The record is nondispatchable and inert, so no unauthorized work follows, but the status label is inconsistent with the README's paused/hold vocabulary.
-
Advance.md:30-31 recomputes the planning/architect-discovery continuation that Gate.md:21-22 already declares, and Gate.md:41 overrides assignment.continuation at dispatch, so the Advance-side continuation field is redundant state; deriving it only in Gate would remove one of two places that must agree.
-
Implementation.test.md now exposes one outer test named 'Implementation fixture journeys'; the G1/G6/G7/G8/A1-A4 group names and per-group deadlines are only visible inside the captured child output on failure, so a hosted failure report names the whole group rather than the failing journey. Diagnostic cost only; the Fail message still carries the child stdout/stderr.
-
A smaller composition exists outside this correction's declared boundary: passing --plugin packages/cli/tests/fixtures/workflow-test-permissions.mjs from packages/cli/tests/adversarial-implementation.test.ts for the Implementation suite would let Fixture.Implementation expand in-process and remove the remaining subprocess and the nested-Testing wrapper. Not required for the accepted behavior; noted for a later minimalism pass.
The completed provider execution, saved reply and canonical receipt agree, and the unchanged review schema accepted this result. Earlier replies and their validation failures remain preserved; no automatic response-repair prompt occurred.
f57f840 to
00a5349
Compare
Why
Initial planning stops before a continuable Ticket exists when Planner needs Architect discovery, leaving the coordinating chat to reconstruct the handoff. Closes #1084.
What changes
Before: an initial discovery response returns a hold before the implementation manifest is prepared.
After: the same Ticket carries initial Planner → original Architect → same Planner → implementation. Automatic discovery is finite; further discovery pauses with its prepared next assignment for explicit continuation. Accepted plan documents stay available to reviewers when the Implementor selects additional documents, including across corrections and retained continuation.
How it works
Ticket retains original sessions, initialization, selected inputs, contracts and separate correction limits. Each discovery disposition records source state and requires clean unchanged launch source before another role runs. Phase owns attempts and reply validation. Genuine holds remain inert.
Review guide
Start with
examples/adversarial-implementation/run.mdandcomponents/Ticket/Continue.md, then inspectTicket/Advance.md,Ticket/Gate.mdandTicket/Settle.md.Check dispatchable discovery pauses versus holds, source checks after rejected or absent replies, and retention of accepted inputs through subsequent role assignments.
The original Architect and Planner passed the initial implementation. The original Planner passed the plan-retention repair at
e1033863703f812028813aaf56b73a5b47c3cb36. Claude Fable 5.1’s closeout passed all six acceptance clauses and closed the blocker from its full feature review. The unchanged workflow schema accepted the closeout; no automatic response retry occurred.How to verify it
deno task test packages/cli/tests/adversarial-implementation.test.tspasses all six groups. The actual Node and Bun adapters pass the four affected Workflow, Friction, Implementation and LaunchPolicy groups.main-greenand aggregategreenfailed because Main CI2398 was red. Complete first failure logs remain preserved.f57f84046dad6c3842646da8db16a7fb6bb4f963, integrating published Mainab314aa4e583efa9948085ce875cbfaaf7fe8cbe. All 20 feature files are byte-identical to reviewede103386; its full tree exactly matches CI2399’s tested generated merge. Review provenance remains at e103386.Scope
This PR changes initial planning/discovery continuation in the example workflow, its existing test journeys and incident fixtures. Foundation #1064 is merged and current published Main is integrated. No dependency, schema, runtime corpus or generated artifact changes are introduced by this feature.
Risks and limitations
Five nonblocking review observations remain documented in the closeout. These include an unmeasured risk that accumulated handoff documents exceed the existing twelve-document cap and produce a truthful hold. The other observations concern status terminology, redundant continuation state, test diagnostics and possible fixture simplification.
An interrupted role continues through its retained execution; an ordinary diagnostic journal does not resume it. Delivery proceeds without waiting for Main health as requested. Required branch protection remains authoritative before merge.
Scope confirmation