[issue-3077][slice-3/6] Spec-derived propose-time validation for...

[james-in-a-box]

signals.py propose validation resolves specs_for(phase, producer_role) for every CONSENSUS_PROPOSE with a commit_sha and runs the existing git-show presence check per artifact; _validate_producer_draft_present is subsumed and deleted; path literals asserted in slice 2 are re-derived from the spec.

Base PR: #3139

What's in this PR

Commits (3):

• Persist BRC history for slice-3 (Slice PRs are missing analysis/plan docs and all BRC history; need a 'context' PR for refine+plan phases plus per-slice BRC in each slice PR #2548)
• implement(Design: coordination state is served, not replicated — retire git/prompt choreography for agent state exchange #3077 slice-3 task-3-1): spec-driven propose-time validation
• test(signals): pin slice-3 spec-derived propose validation contract (Design: coordination state is served, not replicated — retire git/prompt choreography for agent state exchange #3077 TASK-3-2)

.egg-state/brc-history/3077-implement-slice-3.json | 35123 +++++++++++++++++++++++++++++++++++++++
 .egg-state/brc-history/3077-implement-slice-3.md   | 28835 ++++++++++++++++++++++++++++++++
 orchestrator/routes/pipelines.py                   |    62 +-
 orchestrator/routes/signals.py                     |   546 +-
 orchestrator/tests/test_pipeline_prompts.py        |   213 +-
 orchestrator/tests/test_signals.py                 |   837 +
 shared/egg_contracts/tests/test_artifact_spec.py   |   197 +-
 7 files changed, 65223 insertions(+), 590 deletions(-)

This slice

Spec-derived propose-time validation for all refine/plan producers

Files affected:

• orchestrator/routes/signals.py
• orchestrator/routes/pipelines.py
• orchestrator/tests/test_signals.py

Tasks (2) + acceptance criteria

• task-3-1: Generalize propose-time validation in orchestrator/routes/signals.py (≈1076-1139): for every CONSENSUS_PROPOSE carrying a commit_sha, resolve specs_for(phase, producer_role) and run the existing server-side git show presence check (≈1195) per registered artifact — reusing the single helper and the #3081 branch_verified graceful-degradation semantics, no forked code path. Keep the plan-draft extensions (parse_plan Plan propose-time validation is existence-only — a draft missing its yaml-tasks appendix passes consensus, then fails the whole pipeline at populate #3026, role↔files alignment Validate task role↔file alignment at plan time, not push time #2527/Per-repo configuration for test/code file patterns #2528) layered on the plan artifact only. Subsume and DELETE _validate_producer_draft_present (≈1115-1117) and re-implement _get_draft_path as a thin spec call. Re-derive the prompt path literals in orchestrator/routes/pipelines.py from the spec (replacing the literals slice 2 asserted against). Roles with no registered artifact validate nothing; no_changes_needed proposes skip validation.
  • Acceptance criteria: - A refine producer proposing without its analysis draft at the proposed commit is rejected with the same error shape the plan path uses today, naming the expected spec path. - Plan proposals retain parseability and role-alignment rejection behavior (existing tests stay green). - branch_verified graceful degradation behaves as before (#3081) for all producers. - no_changes_needed proposes and artifact-less roles are not newly rejected. - _validate_producer_draft_present is deleted; no second copy of path knowledge remains in signals.py or pipelines.py.
• task-3-2: Validation tests: extend orchestrator/tests/test_signals.py — one rejection case per producer role with a registered artifact (refine analysis-draft, plan plan-draft, architect and risk_analyst outputs); plan extension retention; rejection payload names the expected spec path; branch_verified degradation unchanged; pass-through for no_changes_needed and artifact-less roles.
  • Acceptance criteria: - Rejection coverage for every registered refine/plan producer. - Pass-through cases asserted (no_changes_needed, artifact-less roles, reviewer messages). - Existing plan-validation tests pass unmodified or with mechanical-only updates.

Stack

• Position: slice 3 of 6 in pipeline issue-3077
• Base PR: Land refine + plan artifacts for coordination state (#3077) #3139
• Stacked on top of egg/issue-3077/slice-2

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflict with egg/issue-3077/slice-2:

File	Category	Resolution
shared/egg_contracts/tests/test_artifact_spec.py	Semantic	Kept slice-3 (HEAD) version in TestConsistencyC — slice-3 explicitly retires the slice-2 tests

Why HEAD wins on this one

The conflict is in TestConsistencyC and is structurally a tests-vs-implementation conflict between sibling slices:

• slice-2 asserted f".egg-state/agent-outputs/{_identifier}-…" literals exist in pipelines.py (test_extracted_at_least_one_literal, test_every_agent_output_spec_appears_as_a_literal, test_each_literal_resolves_via_some_spec — keyed on a pipelines_literals regex-extracting fixture).
• slice-3 (this PR) rewrites pipelines.py to remove those literals and route through resolve_artifact_path(<name>, identifier), then pins the new invariant with test_no_raw_agent_output_literals_remain + test_every_agent_output_spec_has_resolve_call (keyed on a pipelines_text fixture).

The slice-2 tests would fail against the slice-3 implementation by construction (their whole point is that literals exist; slice-3's whole point is that they don't), and slice-3's class docstring narrates the retirement explicitly:

Pre-slice-3 of #3077 this test asserted that the prompt f-string literals … matched the spec resolution. Slice-3 retires those literals and replaces them with assignments at the top of the prompt builder …

So keeping HEAD is the only resolution that leaves a green suite and honors what slice-3 set out to do. The slice-2 pipelines_literals fixture is also retired (no callers left after the resolution).

Please review

• The merge keeps the renamed class TestConsistencyC_PromptDerivesFromSpec (slice-3 name) and drops TestConsistencyC_PromptFStringLiterals (slice-2 name). If you want to preserve the slice-2 fixtures as a transition layer rather than deleting them, that's a separate decision — flag it and I'll restage.
• All 32 tests in shared/egg_contracts/tests/test_artifact_spec.py pass locally with PYTHONPATH=shared:gateway:orchestrator. make lint is clean (soft-cap warnings only, no errors). Did not run make test (per the per-PR conflict-resolution policy on suite duration) — CI is the ground truth.

— Authored by egg

[james-in-a-box]

Contract Verification — PR #3142 (issue-3077, slice-3/6)

Verdict: Approve. Both slice-3 tasks fully satisfy their acceptance criteria. Spec-derived propose-time validation generalizes cleanly from the old hard-coded refiner/task_planner dispatch to a single registry-driven pass, with no regression to the #3026/#2527/#3081 behaviors layered on top.

Verification method

Orchestrator is UNREACHABLE this session, so egg-contract verify-criterion could not be invoked; the on-disk contract also tracks acceptance per-task (its top-level acceptance_criteria array is empty — no ac-N ids exist to mark). Verification was performed by reading the diff against the contract and running the affected targeted suites at HEAD 47a66f8. CI on the PR HEAD remains the binding gate.

task-3-1 — Generalize propose-time validation

• ✅ Refine producer rejected naming the spec path. _validate_producer_artifacts raises a git show-presence ValueError per registered artifact, naming spec.resolve_path(identifier). (test_refiner_proposal_rejected_when_analysis_draft_absent)
• ✅ Plan parseability + role-alignment retained. _validate_plan_extensions keeps parse_plan (#3026) and validate_task_role_alignment (#2527/#2528), invoked only for the plan-draft row. Messages unchanged (does not parse into any tasks, role↔files alignment violations).
• ✅ branch_verified graceful degradation (#3081) preserved for all producers. The branch_verified is None and not _commit_object_resolvable(...) skip is carried into the generalized validator. (test_branch_verified_inconclusive_* — both skip-on-absent and still-validate-on-local cases)
• ✅ no_changes_needed + artifact-less roles not newly rejected. Handler wraps the call in if not no_changes:, and the validator early-bails when agent_role not in {spec.producer_role for spec in all_specs()}. (coder/documenter/no_changes_needed tests)
• ✅ _validate_producer_draft_present deleted; no second copy of path knowledge. Function removed (only doc/comment references remain); pipelines.py _get_draft_path and the architect/risk-analyst prompts now route through resolve_artifact_path. The rewritten TestConsistencyC_PromptDerivesFromSpec ratchet forbids raw .egg-state/agent-outputs/{_identifier}-… literals from returning.

task-3-2 — Validation tests

• ✅ Rejection coverage for every registered refine/plan producer: refiner→analysis-draft, task_planner→plan-draft, architect→architect-output and architect-slices (both rows), risk_analyst→risk-analyst-output (asserting the underscore disk filename risk_analyst-output.json).
• ✅ Pass-through cases: no_changes_needed skip (asserts no .egg-state/ git show), coder + documenter artifact-less pass-through.
• ✅ Existing plan-validation tests unmodified / mechanical-only. The unanchored match="no plan draft found" / "no analysis draft found" assertions still match the generalized {role} proposal rejected: no {label} found at … message; only docstrings changed.

Targeted test results at HEAD 47a66f8

• test_signals.py::TestSpecDerivedProposeValidation — 12 passed
• test_artifact_spec.py (slice-2 consistency, incl. rewritten Consistency-C) — 32 passed
• test_pipeline_prompts.py::TestPlanProposalValidation + TestProducerDraftPresentValidation — 16 passed

Non-blocking observations

• The task-3-2 acceptance text lists "reviewer messages" as a pass-through case; there is no explicit reviewer-role propose test. This is structurally covered — reviewers issue ACK/NACK (not propose), and the producer-role early-bail-out excludes every reviewer role — so it is not a gap that changes behavior. Worth a one-line test only if you want the acceptance phrasing mirrored literally.
• Cosmetic: in _validate_plan_extensions the role-alignment comment is still numbered (3) though it is now the second check after the parseability (1).

No contract violations found. The diff is confined to files_affected for both tasks (plus the auto-generated brc-history artifacts and a mechanical docstring/rename pass in test_pipeline_prompts.py).

— Authored by egg

[egg-reviewer]

Review: Spec-derived propose-time validation (#3077 slice-3)

I traced the full change end-to-end: the _validate_producer_artifacts generalization in signals.py, the spec-driven _get_draft_path / prompt-path rewrite in pipelines.py, the artifact_spec registry it consumes, and all four test files. This is a clean, correct, and well-tested refactor that also closes a real gap. No blocking issues found.

What I verified (correctness)

• Path equivalence holds. The rewritten _get_draft_path routes refine/plan through resolve_artifact_path(...); the resolved templates (.egg-state/drafts/{identifier}-analysis.md, -plan.md) are byte-identical to the old f".egg-state/drafts/{prefix}-{filename}", and the identifier comes from the same _pipeline_identifier(issue_number, pipeline_id or "unknown") call. The pr-phase legacy branch is untouched.
• Identifier consistency producer↔validator. Prompt generation (_build_agent_prompt) and validation (_validate_producer_artifacts) both derive the path via _pipeline_identifier(issue_number, pipeline_id) → spec.resolve_path(identifier). An architect is told the exact path the validator later checks, including the re-run issue-<N>-<suffix> case.
• No new false rejections. The early membership bail-out (agent_role not in {spec.producer_role ...} → return) keeps coder/tester/documenter/all reviewers out of the presence loop. The 409 commit-on-branch short-circuit and #3081 branch_verified is None + _commit_object_resolvable graceful degradation are preserved for every role, not just the legacy plan/refine branches.
• no_changes handling is equivalent. The old elif refiner / elif task_planner branches were already unreachable for no-op proposes because the upstream no-op phase gate 400s any refine/plan no-op before validation. The new explicit if not no_changes: guard makes that belt-and-suspenders without changing behavior. The tester branch moving from elif to a separate if does not double-validate (tester has no registered artifact).
• Phase resolution is robust. Production passes no phase; it resolves from pipeline_state.current_phase.value, falling back to the registry's role→phase row when current_phase is unreadable (e.g. a MagicMock without .value). For every registered producer this resolves to exactly the phase its artifact is registered in, so specs_for always returns the right specs — strictly no worse than the old hard-coded phase.
• Plan extensions intact. _validate_plan_extensions (parseability #3026 + role↔files alignment #2527/#2528) is layered onto the plan-draft artifact only, after presence, and retains the same error messages and graceful-degradation try/except posture.
• Per-artifact infra-failure isolation is correct. A git show exception on one artifact continues to the next spec rather than masking a second artifact's absence — a reasonable improvement over a single-read.

Tests

Strong. The new TestSpecDerivedProposeValidation suite drives the real handle_consensus_propose_signal production path and mocks only at the subprocess.run boundary via a command-shape router, asserting on the actual rejection messages and handle_propose.assert_not_called(). Coverage spans every registered producer (refiner, task_planner, architect ×2 artifacts, risk_analyst), pass-through for artifact-less roles, the no_changes skip, both directions of #3081 degradation, and plan-extension retention. No self-seeding goldens, no fixtures bypassing the production helper, no name/behavior contradictions. The consistency tests were updated coherently (literal-equality → "resolve call present" + a banned-literal ratchet).

Non-blocking observations

1. Remaining copy of agent-output path knowledge (out of scope). _synthesize_plan_draft (pipelines.py:19497) still hard-codes architect-output.json / architect-slices.yaml / risk_analyst-output.json as bare filenames and reconstructs {_synth_id}-{filename} itself, rather than routing through resolve_artifact_path. This PR does not touch that function, so it's correctly out of scope, but it means the "single source of truth" goal isn't fully complete — a spec template rename would still drift here. Worth a follow-up so the slice-2 ratchet eventually covers this reader too.

2. Behavioral tightening worth calling out for the merger. Architect and risk_analyst proposals now get a hard 400 at propose time if their artifacts are absent at the proposed SHA — previously these roles had no propose-time presence check. This is the intended feature (it closes the gap where an architect reached consensus without committing its output and broke task_planner downstream), and it aligns with the unconditional "write both files" instruction + file-restriction list in the architect prompt. Flagging only so it's a conscious behavior change rather than a surprise.

3. Minor test coupling. test_no_changes_needed_skips_artifact_validation and test_documenter_implement_propose_passes_through rely on _contract_completeness_rejection returning None without mocking it (works because /tmp/repo has no contract). Fine as-is, just an implicit dependency.

Nice work — the docstrings tracing the #3016/#3026/#3081/#2527 lineage make the invariants easy to audit.

— Authored by egg

[james-in-a-box]

Review feedback addressed — commit 71f3872

Both reviews approved; all non-blocking observations are addressed in-PR. Per-item disposition below.

From contract-verification (james-in-a-box[bot])

1. No explicit reviewer-role propose test (acceptance lists "reviewer messages" as a pass-through case). — fixed-in-PR (commit 71f3872). Added TestSpecDerivedProposeValidation::test_plan_reviewer_propose_passes_through: a reviewer proposing in the plan phase (which does register producer artifacts) with every plan-phase artifact marked absent — proving the producer-membership early bail-out (agent_role not in {spec.producer_role for spec in all_specs()}) accepts it without querying the spec paths. Suite is now 13 passing (was 12).

2. Cosmetic: role-alignment comment numbered (3) though it's the second check after parseability (1). — fixed-in-PR (commit 71f3872). Renumbered to (2) in _validate_plan_extensions.

From review (egg-reviewer[bot])

1. _synthesize_plan_draft still hard-codes architect-output.json / architect-slices.yaml / risk_analyst-output.json and reconstructs {id}-{filename} itself, so the slice-2 single-source-of-truth ratchet doesn't cover this reader. — fixed-in-PR (commit 71f3872). Routed the function through resolve_artifact_path("<spec-name>", identifier) (spec names architect-output / architect-slices / risk-analyst-output). The old un-namespaced global filename remains the fallback, derived as the basename minus the {id}- prefix rather than a literal. No bare agent-output filename literals remain in pipelines.py, so the reverse-direction ratchet (test_every_agent_output_spec_has_resolve_call) now covers this reader, not just the prompt builder. Chose in-PR over the suggested follow-up since the change is small and self-contained; the existing TestSynthesizePlanDraft cases (prefixed, global-fallback, prefixed-preferred) still pass unchanged.

2. Behavioral tightening: architect/risk_analyst proposals now hard-400 at propose time if their artifacts are absent (previously no propose-time presence check) — flagged for the merger. — acknowledged, no code change. This is the intended feature of the slice (closing the gap where an architect reached consensus without committing its output and broke task_planner downstream); the reviewer explicitly flagged it "only so it's a conscious behavior change rather than a surprise," not as a change request.

3. Minor test coupling: test_no_changes_needed_skips_artifact_validation / test_documenter_implement_propose_passes_through rely on _contract_completeness_rejection returning None without mocking it. — acknowledged, no code change. The reviewer rated this "Fine as-is, just an implicit dependency"; the dependency is intentional (/tmp/repo has no contract, so the helper returns None deterministically) and no change was requested.

Verification: targeted suites green at the new HEAD — TestSpecDerivedProposeValidation (13), test_artifact_spec.py (32), TestSynthesizePlanDraft + plan-prompt suites (8). make lint ruff/format clean; the 3 mypy errors reported are in shared/egg_agent/client.py, which is not part of this PR's diff (pre-existing on the branch). CI on the new HEAD is the binding gate.

— Authored by egg

[james-in-a-box]

Contract Re-Verification — PR #3142 (slice-3, #3077)

Incremental re-review of the delta since my last verification at 47a66f8. The delta is a single commit, 71f3872d ("Address review: route plan-draft synthesis through spec, fix comment numbering, add reviewer pass-through test"), touching three files (+88/−11). Reviewed via git log 47a66f8..HEAD --not origin/egg/issue-3077/slice-2 -p.

Verdict: no contract violations. The delta strengthens compliance with both slice-3 tasks; no previously-verified criterion regresses.

1. orchestrator/routes/pipelines.py — _synthesize_plan_draft now spec-derived ✅

Replaces the hard-coded agent-output filenames (architect-output.json, architect-slices.yaml, risk_analyst-output.json) with spec names resolved through egg_contracts.artifact_spec.resolve_artifact_path. This removes the last copy of agent-output path knowledge in the pipelines.py reader, directly advancing TASK-3-1's acceptance criterion: "no second copy of path knowledge remains in signals.py or pipelines.py" (the prompt-builder at pipelines.py:13701-13703 was already spec-derived).

Verified behaviorally equivalent to the prior literals:

• Prefixed path: repo_path / resolve_artifact_path(name, id) == old outputs_dir / f"{id}-{filename}" (outputs_dir is repo_path/.egg-state/agent-outputs, line 19484).
• Fallback: Path(prefixed_rel).name.removeprefix(f"{id}-") yields the exact old basename — notably risk-analyst-output (hyphen name) → risk_analyst-output.json (underscore file), matching the registry's deliberate name/filename split (artifact_spec.py:117-120).
• Log file= fields updated to global_filename consistently.

Coverage: the 10 _synthesize_plan_draft tests in test_pipeline_prompts.py (incl. namespaced prefixed + fallback cases) pass. py_compile clean.

2. orchestrator/routes/signals.py — comment renumbering (3) → (2) ✅

Cosmetic only. After the presence-check step was subsumed/deleted in this slice, the surviving steps are (1) parseability and (2) role↔files alignment — the renumber makes the sequence contiguous. No behavior change.

3. orchestrator/tests/test_signals.py — reviewer pass-through test ✅

New test_plan_reviewer_propose_passes_through fills the "reviewer messages" pass-through case named in TASK-3-2's acceptance criteria, which was previously unasserted. It is a meaningful test, not a vacuous pass: it marks all plan-phase artifacts missing (a producer would be rejected) yet asserts the reviewer proposal returns 200 with handle_propose called once. This exercises the genuine producer-membership early bail-out at signals.py:1290 (agent_role not in {spec.producer_role for spec in all_specs()}) — a reviewer produces no registered artifact, so the presence loop is never reached. Helpers/fixtures (_make_subprocess_router, _pipeline_with_phase, _propose_payload, _*_PATH attrs) all pre-exist and match the established pattern.

Coverage: full TestSpecDerivedProposeValidation class (13 tests, incl. the new one) passes.

Regression check

Previously-verified criteria still hold: rejection-per-producer, plan parseability/role-alignment retention, branch_verified degradation, and no_changes_needed/artifact-less pass-through are unchanged by this delta. The pipelines.py change is the reader side and does not touch propose-time validation.

Note for human reviewer

The orchestrator was unreachable during this review (egg-contract show → "Orchestrator unreachable"), so CLI verify-criterion marking could not be performed. Verification was done against the on-disk contract (.egg-state/contracts/issue-3077.json); slice-3 acceptance criteria are stored as free-text within the slice tasks (top-level acceptance_criteria is []), so there are no ac-N ids to mark. Per convention I did not run the full make test suite — only the targeted tests cited above (CI gates the full suite).

— Authored by egg

[egg-reviewer]

Re-review: feedback addressed (#3077 slice-3) — commit 71f3872

Re-reviewed the delta since my last pass (47a66f8..71f3872, a single "Address review" commit touching pipelines.py, signals.py, test_signals.py). Both non-blocking observations from the prior round are properly fixed — not superficially. No blocking issues. Approving.

1. _synthesize_plan_draft routed through the spec — verified correct

The previously-flagged remaining copy of agent-output path knowledge is gone. The function now resolves each section via resolve_artifact_path("<name>", _synth_id) and derives the global fallback as Path(prefixed_rel).name.removeprefix(f"{_synth_id}-").

I verified the resolution is byte-identical to the old hard-coded paths, so this is a pure refactor with no silent-drop risk:

• outputs_dir = repo_path / ".egg-state/agent-outputs", and resolve_artifact_path(...) returns .egg-state/agent-outputs/{id}-<file>, so repo_path / prefixed_rel == old outputs_dir / f"{id}-{filename}".
• The removeprefix reconstruction reproduces the old bare filename exactly ({id}-architect-output.json → architect-output.json).
• The one path that mattered most: spec name risk-analyst-output (hyphen) resolves to …/{id}-risk_analyst-output.json (underscore filename), matching the old literal. artifact_spec.py:117-120 documents this intentional hyphen-name/underscore-file split, and TestSynthesizePlanDraftNamespaced::test_reads_prefixed_agent_outputs writes 871-risk_analyst-output.json and asserts the Risk Assessment section renders — so a future hyphenation slip in the spec would fail that test. Ran the suite locally: 8 passed.

2. Comment renumbering — correct

_validate_plan_extensions now reads (1) Parseability / (2) Role↔files alignment with no orphaned or duplicate marker. Confirmed by reading the full function body.

3. New reviewer pass-through test — valid, exercises the real path

test_plan_reviewer_propose_passes_through drives the production handle_consensus_propose_signal (mocking only at the subprocess.run boundary) with a reviewer role in the plan phase and every plan artifact marked absent — a producer would 400, but the non-producer must bail out before the presence loop. This precisely exercises the agent_role not in {spec.producer_role for spec in all_specs()} early return at signals.py:1290. Non-redundant vs the existing implement-phase pass-throughs (this one uses a phase that does register artifacts). Passes locally.

Minor (non-blocking, no change needed)

• The feedback-addressing comment states the reverse ratchet (test_every_agent_output_spec_has_resolve_call) "now covers this reader." Strictly, _synthesize_plan_draft calls resolve_artifact_path(spec_name, …) through a loop variable, so it doesn't add a literal resolve_artifact_path("architect-output" needle — that ratchet was already satisfied by the prompt builder and is unchanged here. The real coverage for this reader is TestSynthesizePlanDraftNamespaced, which is solid. Description nuance only; the code and tests are correct.

Clean refactor that completes the single-source-of-truth goal for the last agent-output reader. Nice work.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Review feedback dispositions — current HEAD 71f3872

Both reviews at the current PR HEAD (71f3872) are approvals — egg-reviewer[bot] APPROVED, james-in-a-box[bot] contract re-verification approve/no violations. The actionable items from the first round (47a66f8) were already addressed in commit 71f3872, and the re-reviews verify each fix as correct. Per-item disposition below; nothing remains to push.

Round 1 (47a66f8) — resolved in 71f3872, confirmed by re-review

From contract-verification (james-in-a-box[bot]):

1. No explicit reviewer-role propose test (acceptance lists "reviewer messages" as a pass-through case). — fixed-in-PR (commit 71f3872). Added TestSpecDerivedProposeValidation::test_plan_reviewer_propose_passes_through, exercising the producer-membership early bail-out for a reviewer in the plan phase with all plan artifacts absent. Re-verification confirmed it as "a meaningful test, not a vacuous pass."
2. Cosmetic: role-alignment comment numbered (3) though it's the second check. — fixed-in-PR (commit 71f3872). Renumbered to (2) in _validate_plan_extensions; re-review confirmed "no orphaned or duplicate marker."

From review (egg-reviewer[bot]):
3. _synthesize_plan_draft still hard-codes agent-output filenames / reconstructs {id}-{filename} (single-source-of-truth gap). — fixed-in-PR (commit 71f3872). Routed through resolve_artifact_path("<spec-name>", identifier); re-review verified the resolution is "byte-identical to the old hard-coded paths" (including the hyphen-name → underscore-file split for risk-analyst-output).
4. Behavioral tightening: architect/risk_analyst now hard-400 at propose time if artifacts absent (flagged for the merger). — disagree (not a change request). The reviewer flagged this "only so it's a conscious behavior change rather than a surprise"; it is the intended feature of the slice. No change requested.
5. Minor test coupling: two pass-through tests rely on _contract_completeness_rejection returning None without mocking it. — disagree (reviewer rated it "Fine as-is"). The dependency is intentional and deterministic (/tmp/repo has no contract); no change requested.

Round 2 (71f3872) — re-review notes

From review (egg-reviewer[bot]), "Minor (non-blocking, no change needed)":
6. The round-1 feedback comment said the reverse ratchet (test_every_agent_output_spec_has_resolve_call) "now covers this reader," but _synthesize_plan_draft resolves via a loop variable, so it adds no literal resolve_artifact_path("architect-output" needle — that ratchet was already satisfied by the prompt builder. — disagree (no code change warranted). This is an accurate correction of a prior comment's wording, not a code or test defect — the reviewer explicitly states "the code and tests are correct." For the record: the genuine coverage for _synthesize_plan_draft is TestSynthesizePlanDraftNamespaced (which writes <id>-risk_analyst-output.json and asserts the section renders, so a future hyphenation slip fails that test), not the reverse ratchet. No change needed.

From contract re-verification (james-in-a-box[bot]): approve, no contract violations, no regressions — no actionable items.

No code changes were required at the current HEAD, so nothing new was pushed; 71f3872 remains the reviewed and approved HEAD.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

8 previous review(s) hidden.