Skip to content

Commit 735e249

Browse files
committed
Bound the plan-step label, and stop assuming the fixture's shape
agentPlanSteps embedded the raw requirement verbatim in a plan step's redacted detail, which Validate refuses for trailing whitespace -- and the acceptance blocks PIPE-019 makes mandatory produce exactly that. So the full Run path could not reach a bound plan at all. The whitespace was the symptom. A step's detail is a label, and an arbitrarily long user-supplied block does not belong in one whatever it ends with -- a forty-line acceptance block would produce an unreadable step even if it trimmed cleanly. requirementSummary strips the acceptance block, which is machine-checked metadata rather than what the step is for, takes the first line of what remains, and caps it. A test drives the full Run and confirms a real bound plan revision, which was previously unreachable. Acceptance argument lines are tokenised with quote handling instead of split on spaces, so an example can pass an argument containing one. The acceptance oracle and adversarial probe resolve the module root rather than assuming the worktree root, and both acceptance forms accept build tags. The primary validation gate still assumes worktree root in files another lane held, so regression, mutation, coverage and control-flow are unfixed for a non-root module. PIPE-124 is diagnosed and not implemented: checkAtoms hard-fails on zero atoms and takes no attribution parameter, so it cannot use PIPE-111a's established flag. The exact fix is recorded rather than guessed at. Also carries this session's TODOS record. Committed with --no-verify at the user's explicit request. TODO: PIPE-019b, PIPE-121, PIPE-123 Change-Log: CL-20260802-080 Dev-Log: DL-20260802-090 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 2b5b12e commit 735e249

10 files changed

Lines changed: 960 additions & 32 deletions

CHANGELOG

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,37 @@ Dev-Log:
5050

5151
Entries
5252
-------
53+
Change-ID: CL-20260802-080
54+
Commit: pending
55+
Date: 2026-08-02
56+
Type: Fix -- bounded plan-step detail, and quoted acceptance arguments
57+
Request-or-TODO: PIPE-019b, PIPE-121, PIPE-123
58+
Outcome: agentPlanSteps embedded the raw requirement verbatim in a plan step's
59+
redacted detail, which Validate refuses for trailing whitespace -- and the
60+
acceptance blocks PIPE-019 makes mandatory produce exactly that, so the full
61+
Run path could not reach a bound plan at all. requirementSummary strips the
62+
acceptance block, takes the first line of what remains, and caps it, which
63+
addresses the real defect: a step's detail is a label, and an arbitrarily long
64+
user block does not belong in one whatever it ends with.
65+
Acceptance argument lines are tokenised with quote handling instead of split
66+
on spaces, so an example can pass an argument containing one.
67+
The acceptance oracle and adversarial probe now resolve the module root rather
68+
than assuming the worktree root, and both acceptance forms accept build tags
69+
Affected-behavior: A multi-line acceptance requirement now reaches a bound plan
70+
through the real run. Unquoted argument lines parse identically to before
71+
Compatibility-or-migration: None
72+
Verification: A test drives the full Run and confirms a real bound plan
73+
revision, which was previously unreachable. Discrimination confirmed by
74+
reverting and reproducing "agent plan step 1 is invalid". The quoting
75+
discriminator was verified to fail against the old code
76+
Known-limitations: The primary validation gate still assumes the worktree root
77+
in agent_execution.go and the agent_stage_* files, so regression, mutation,
78+
coverage and control-flow stages are unfixed for a non-root module. PIPE-124
79+
is diagnosed and not implemented: checkAtoms hard-fails on zero atoms and
80+
takes no attribution parameter, so it cannot use PIPE-111a's established flag;
81+
the exact fix is recorded in TODOS.md
82+
Dev-Log: DL-20260802-090
83+
5384
Change-ID: CL-20260802-079
5485
Commit: pending
5586
Date: 2026-08-02

DEVLOG

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,32 @@ Next-safe-step:
2525

2626
Entries
2727
-------
28+
Dev-Log: DL-20260802-090
29+
Date: 2026-08-02
30+
Status: Three closed, one diagnosed and blocked
31+
Change-ID: CL-20260802-080
32+
Commit: pending
33+
Request-or-TODO: PIPE-019b, PIPE-121, PIPE-123, PIPE-124
34+
Goal: Stop a mandatory acceptance block making a plan unbuildable, and let the
35+
flow work on a repository that is not shaped like the fixture
36+
Assumptions: The whitespace was the symptom, not the defect
37+
Decisions: Bounded the summary rather than trimming the requirement. A step's
38+
detail is a label; a forty-line acceptance block would produce an unreadable
39+
step even if it trimmed cleanly, so trimming would have left the defect.
40+
Stripped the acceptance block from the summary rather than including it,
41+
because it is machine-checked metadata and not what the step is for.
42+
Stopped at PIPE-124 rather than editing held files, and recorded the exact
43+
fix: give checkAtoms an attribution parameter and return skipped when the
44+
attribution is established and empty, reusing PIPE-111a's flag rather than
45+
inventing a second one
46+
Files-or-schemas: agent_narration.go, agent_acceptance.go,
47+
agent_acceptance_named_test_form.go, agent_adversarial.go, three new tests
48+
Validation: A full-Run test reaching a bound plan revision; discrimination by
49+
reverting and reproducing the exact refusal
50+
Failures-or-discarded-approaches: None
51+
Known-limitations: The primary validation gate still assumes worktree root
52+
Next-safe-step: PIPE-124, then the rest of PIPE-123 once its files are free
53+
2854
Dev-Log: DL-20260802-089
2955
Date: 2026-08-02
3056
Status: Wired and called

TODOS.md

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4304,7 +4304,16 @@ ledger can be treated as authoritative.
43044304
- `internal/coordinator/provider_switch_authority_test.go` pins both facts: the escalation site keeps its approval check and its narration, and the two unwired symbols are asserted still unwired with a message saying to close this ticket rather than delete the assertion when they gain a caller.
43054305
- Remaining: decide whether a configured ladder rung constitutes the "explicit user authority" `M12-052` requires — if it does, the settings revision is the decision identity and escalation should call `ValidateProviderSwitch` with it; if not, a per-switch approval is needed. That is a plan decision, not a reconciliation, which is why this item stays open.
43064306
- Verified: `go test ./internal/coordinator/ -run TestAUDIT019` green.
4307-
- [ ] `AUDIT-020 REVIEW BLOCKER TEST` Reconcile `M14-021`: complete the real StartTask path through worktree creation, durable scheduling, worker launch, provider/tool loop, and awaiting-review; make `TestRequirementReachesAStartedRunThroughTheRealApplication` pass without a recorded-but-idle run.
4307+
- [x] `AUDIT-020 REVIEW BLOCKER TEST` Reconcile `M14-021`: complete the real StartTask path through worktree creation, durable scheduling, worker launch, provider/tool loop, and awaiting-review; make `TestRequirementReachesAStartedRunThroughTheRealApplication` pass without a recorded-but-idle run.
4308+
- **Closed. A run reaches `awaiting-review`, verified independently** — `task tsk_019fc594… reached awaiting-review`, 12.3s, and the `CODEFLUX_AUDIT020` gate is removed.
4309+
- **The prime suspect recorded here was wrong, and the lane disproved it rather than working around it.** This entry named a final `go test ./...` as the likely cause of the earlier hang. The re-attempt built a completion path that re-runs **nothing** — it reuses the pass/fail fact the run's own loop already established — and it still hung.
4310+
- **The real defect: a failed write masquerading as a hang.** `AgentExecutionPersistence.PersistPlanStepTransition`, newly wired to the loop's `PlanSteps` port, referenced tool requests by a **SHA-256 hashed ID** while the production `agentToolJournal` stores them under the **raw** ID. Nothing had ever exercised `PlanSteps` — it was a no-op — so the mismatch was invisible. Every run's first plan-step transition failed a durable consistency trigger, `Run` returned early, and the test polled its full budget for a file that could never appear.
4311+
- **Five further defects surfaced en route, each with its own constraint failure as evidence rather than a guess:** the validation profile must be one of the plan's own risk-derived names, not free text; `CommandExecutionID` must stay nil when nothing executed; `RecordPlanValidationAttributions` already auto-promotes plan steps, so a manual promotion conflicted with it; `CreateValidation`'s summary round-trips into the failure column and must be nil for a pass; and the deepest one, filed as `AUDIT-020a`.
4312+
- A flake in the lane's own test was found and fixed: the wait loop treated the transient `validating` state as terminal, which a loaded machine could sample mid-transition. Reproduced twice under load before the fix, passed twice after — a race in the test, not the run.
4313+
- Verified: engine canary 3.8s → 3.87s isolated; full `internal/coordinator` and `internal/storage` suites green over two consecutive runs; `build`, `gofmt`, `vet`, `lint` all clean. `MEM-001a` agreement confirmed by reading both files and running the full `TestMEM` suite — no double-close, no permanently-open episode.
4314+
- [ ] `AUDIT-020a` Acknowledge a run's own durable state transition from `starting` to `running`. **No production code anywhere performed it** — confirmed by full symbol search: `domain.RunStateRunning` had zero non-test references outside `domain/states.go`, so every run's row stayed `starting` for its whole life while the task moved through its own states independently.
4315+
- `AUDIT-020` bridges that one step inside `TransitionRunToValidation`, using the domain's already-declared transition and scoped to that call site alone. It is deliberately not a general fix, and its doc comment says so.
4316+
- The general fix belongs with whoever owns run-lifecycle acknowledgement: a run should reach `running` when work actually begins, not as a side effect of the step that moves it to `validating`.
43084317
- **Root cause found and reproduced. Not fixed — the fix is an integration, not a reconciliation.**
43094318
- Most of the path does work. `internal/coordinator/requirement_to_review_test.go` drives the real application and confirms the run acts: the worktree is created, the agent loop writes the requested file, and `agent_plan_revisions` and `agent_tool_results` are both populated. So "recorded-but-idle" is only true of the *end* of the journey.
43104319
- **The task never leaves `running`.** `RepairCompletionService.PrepareCompletion` is what records the completion candidate and moves the task to `awaiting-review`, and it has **no production caller** — `NewRepairCompletionService` is constructed nowhere outside its own tests. A run does the work and sits in `running` for ever, so no run can ever become reviewable and the whole "diff, validation, evidence, repair, rollback" journey is unreachable.
@@ -4555,7 +4564,9 @@ Depends on: `PIPE-045` for a number the ledger accepts, and `PIPE-117`, whose an
45554564
- The silent swallow is fixed: the ambiguity check now records `StageClarification` as **failed with the analysis error in its detail**, instead of leaving the stage absent so `ledger.close()` later mislabels it not-implemented.
45564565
- `TestPIPE019a_TrimmingRequirementOfAloneDoesNotFixIt` was rewritten rather than deleted. Its old premise — that `RecordTaskRequirement` refuses a hand-trimmed analysis — is gone now that both sides normalise centrally, so it asserts the new invariant: the untrimmed requirement analyses and records successfully with no call-site trim, which is the whole point. `TestPIPE019a_MultiLineAcceptanceRequirementReachesABoundPlan` covers the case `PIPE-019` made mandatory.
45574566
- `agent_plan_repository.go` and `agent_plan_record.go` needed no change once the shared function was correct — which is the evidence that the single-definition choice was the right one.
4558-
- [ ] `PIPE-019b` Stop `agentPlanSteps` embedding the raw requirement in a plan step's redacted detail. `internal/coordinator/agent_narration.go` builds `"Write " + file + " — " + requirement` verbatim, and `storage.AgentPlan.Validate` requires `DetailRedacted` to carry no leading or trailing whitespace. Any requirement ending in whitespace — which an acceptance block produces — is therefore refused at `BuildAgentPlan` with "agent plan step 1 is invalid".
4567+
- [x] `PIPE-019b` Stop `agentPlanSteps` embedding the raw requirement in a plan step's redacted detail. `internal/coordinator/agent_narration.go` builds `"Write " + file + " — " + requirement` verbatim, and `storage.AgentPlan.Validate` requires `DetailRedacted` to carry no leading or trailing whitespace. Any requirement ending in whitespace — which an acceptance block produces — is therefore refused at `BuildAgentPlan` with "agent plan step 1 is invalid".
4568+
- **Closed, and the fix addresses the label rather than the whitespace.** `requirementSummary` strips the `<<<ACCEPTANCE…>>>` block — machine-checked metadata, not what the step is for — takes the first line of what remains, and caps it at 200 bytes, matching this file's existing bounding convention. It falls back to a fixed label when the requirement is nothing but an acceptance block.
4569+
- **Evidence it unblocks the real path:** a test drives the **full `Run()`** and confirms `GetCurrentPlanRevision` returns a real bound revision. That was previously unreachable — `recordDurablePlan` was refused at `BuildAgentPlan` with "agent plan step 1 is invalid". Discrimination confirmed by reverting and reproducing that exact failure.
45594570
- **Surfaced by fixing `PIPE-019a`, and previously unreachable:** `recordDurablePlan`'s own `AnalyzeTaskRequirement` call always failed first, so this never got the chance to fire. It fails loudly rather than silently, which is the improvement `PIPE-019a` bought, but it still blocks the full `Run()` path from reaching a bound plan for exactly the multi-line acceptance requirements `PIPE-019` now makes mandatory.
45604571
- The narration should carry a bounded summary of the requirement, not the requirement itself — a plan step's detail is a label, and embedding an arbitrarily long user-supplied block in one is the underlying mistake rather than the whitespace.
45614572
- [x] `PIPE-020` Add the acceptance-oracle stage: every example runs and fails against an empty program, proving the example discriminates before anything is built on it.
@@ -4811,7 +4822,8 @@ Depends on: `PIPE-117`, the spike whose answer decides the rest of the section a
48114822
- Verified: `go test ./internal/coordinator/ -run 'TestPIPE118'` green, and the full `TestPIPE|TestAUDIT|TestEveryStage` suite green.
48124823
- [ ] `PIPE-119` Define what a refactor and a dependency change are judged against, given that both are meant to preserve behaviour: the pre-change suite passing unchanged is the candidate, and it needs stating rather than assuming.
48134824
- [ ] `PIPE-120` Restate the acceptance-oracle gate for repository work: an example must fail against the base revision rather than against an empty program, which is both checkable and the stronger claim.
4814-
- [ ] `PIPE-121` Support quoted arguments in the example format; `args:` splits on spaces, so no argument containing one can be expressed.
4825+
- [x] `PIPE-121` Support quoted arguments in the example format; `args:` splits on spaces, so no argument containing one can be expressed.
4826+
- **Closed.** A small shell-like tokenizer handling single and double quotes, backslash-escaped quotes, and preserved empty quoted arguments, replacing a split on spaces. Unquoted lines parse identically to before. The discriminator is direct: `"Jane Doe and co-workers" --loud` yields 2 arguments rather than 5, and it was verified to fail against the old code.
48154827

48164828
## Repository Reality
48174829

@@ -4966,9 +4978,17 @@ Plan references: §31 Learning Artifact Types, Workspace Facts; Extraction Trigg
49664978

49674979
Depends on: `MEM-001` for the episode, `PIPE-111` for what counts as produced. These are the cheapest artifacts in §31 and the only tier that needs no judgment; the extraction functions are already written and merely uncalled.
49684980

4969-
- [ ] `MEM-006 DATA` Extract the successful build and test commands from attributable executions at episode close through `ExtractAttributableBuildCommandsFromEpisode` and `ExtractAttributableTestCommandsFromEpisode`, so the commands `PIPE-122` discovers are learned once rather than rediscovered per run.
4981+
- [x] `MEM-006 DATA` Extract the successful build and test commands from attributable executions at episode close through `ExtractAttributableBuildCommandsFromEpisode` and `ExtractAttributableTestCommandsFromEpisode`, so the commands `PIPE-122` discovers are learned once rather than rediscovered per run.
4982+
- **`MEM-006` and `MEM-008` are wired and called.** `ExtractDeterministicFactsAtEpisodeClose` runs at the acceptance decision, gated on `EpisodeOutcomeAccepted` — §31's rule that a run nobody accepted has established nothing — and `review_mutations.go` calls it after `closeReviewDecisionEpisode`, best-effort for the same reason the close is: failing to learn from a decision must not make the decision look like it did not happen.
4983+
- **All five extractors already existed** in `internal/storage/memory_fact_extraction_repository.go`, fully implemented and tested since M21, with **no production caller**. Verified by reading rather than assumed. What was missing was the orchestrator and the call site.
4984+
- **The discriminating test is the one that matters:** the same fixture with `EpisodeOutcomeRejected` yields zero facts. The underlying storage extractor gates only on `EpisodeStatusClosed`, so that test isolates the new coordinator-level outcome check as the thing doing the refusing — without it, a passing run would prove nothing about the new code.
4985+
- **`MEM-007` blocked:** the extractor needs the observed changed-file set, and nothing reachable from an episode carries it — the accept site knows a diff identity and never passes it into the close.
4986+
- **`MEM-009` deliberately left uncalled, and that is the safe direction.** It is correctly gated on a granted approval scoped to the task, and **no call site anywhere creates such an approval** for admitting an instruction into memory. `AUDIT-011`'s `repository_instruction_approvals` is a different table for a different decision — first-use injection into a run's context. Inventing an approval path would have opened a prompt-injection route into every later run.
4987+
- **`MEM-010` blocked** on `PIPE-136`, which is unimplemented — there is no base-revision suite result to extract.
4988+
- **`MEM-011` partially real:** `UpsertExtractedMemoryFact` hashes the whole content struct including the exact revision, so re-extraction at a new revision already invalidates the prior fact and mints a fresh identity. Wiring extraction to run at every accepted close re-arms that for the **revision** trigger. **Toolchain-binding and dependency-version triggers have no schema support at all** — no content field captures either — so no fact can currently be invalidated by them moving. That needs new fields and a migration.
4989+
- **`MEM-011a`**: extraction duration is measured; consuming it in the run's cost summary needs a hook in the completion path.
49704990
- [ ] `MEM-007 DATA` Extract file-to-test mappings from observed successful validations, and record which validation established each mapping.
4971-
- [ ] `MEM-008 DATA` Extract formatting and lint conventions from configuration and accepted work, bound to the revision that carried them.
4991+
- [x] `MEM-008 DATA` Extract formatting and lint conventions from configuration and accepted work, bound to the revision that carried them.
49724992
- [ ] `MEM-009 SECURITY` Extract repository instructions only through `ExtractApprovedRepositoryInstruction` and its durable approval identity, because repository text is untrusted input and an unapproved instruction extracted into project memory is a prompt-injection path into every later run.
49734993
- [ ] `MEM-010 DATA` Record the base-revision suite result `PIPE-136` captures as a workspace fact bound to that revision, so the regression stage's oracle is established once per revision rather than recomputed per run. This is the one fact observed before the episode opened and extracted at its close, admissible because the rule forbids minting from work in progress rather than recording what was already true.
49744994
- [ ] `MEM-011` Invalidate every extracted fact when its supporting evidence moves — revision, toolchain binding, or dependency version — rather than letting it age silently into a claim about a repository that has changed.

0 commit comments

Comments
 (0)