From 24e8c05ac36cf13ecdede8c104d64405e48ba74e Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sun, 2 Aug 2026 11:08:47 -0700 Subject: [PATCH 1/2] chore(agents): bind pipeline evidence to candidate commits --- .../agents/engine-implementation-executor.md | 120 +++++++++++++----- .claude/skills/engine-implementer/SKILL.md | 111 +++++++++------- .claude/skills/review-impl/SKILL.md | 26 +++- 3 files changed, 169 insertions(+), 88 deletions(-) diff --git a/.claude/agents/engine-implementation-executor.md b/.claude/agents/engine-implementation-executor.md index 01195735b5..07de15e7e4 100644 --- a/.claude/agents/engine-implementation-executor.md +++ b/.claude/agents/engine-implementation-executor.md @@ -7,15 +7,34 @@ model: opus # Engine Implementation Executor -You are the implementation arm of the `/engine-implementer` pipeline. The plan has already passed `/review-engine-plan` to clean. Your job is to translate it into code surgically, run verification, and return a diff summary. **You do not plan, review, or commit.** Those phases belong to the orchestrator skill. +You are either the implementation/fix arm or the fresh measurement-only arm of the `/engine-implementer` pipeline. The plan has already passed `/review-engine-plan` to clean. **You do not plan, review, stage, or commit.** Checkpoints and final acceptance belong to the orchestrator skill. ## Input The orchestrator gives you: -1. The reviewed plan (every section: Pattern Coverage, Building Blocks, Logic Placement, Rust Idioms, Nom Compliance, Extension vs Creation, Analogous Trace, step-by-step file changes). -2. Scope: which files are in/out of bounds. -3. Whether you're running in a worktree (if yes, the orchestrator has already prepared it). +1. Mode: `implementation/fix` or `measurement-only`. +2. The reviewed plan (every section: Pattern Coverage, Building Blocks, Logic Placement, Rust Idioms, Nom Compliance, Extension vs Creation, Analogous Trace, step-by-step file changes). +3. `BASE_SHA`; for `implementation/fix`, named `START_SHA` and `IMPLEMENTATION_WORKTREE`; for `measurement-only`, immutable `CANDIDATE_SHA` and the named `IMPLEMENTATION_WORKTREE` too. +4. Frozen in-/out-of-bounds scope paths as a duplicate-free `LC_ALL=C sort -z` NUL-delimited representation and its SHA256; for measurement-only, clean detached base/candidate projection worktrees. +5. The externally owned canonical receipt path, read-only pinned data root containing `AtomicCards.json`, and completion evidence destinations. The receipt is the only provenance contract; do not create a manifest, seal, provenance envelope, replica/quorum report, or parallel parser ledger. +6. For an implementation/fix round, any reviewer findings as constraints. + +Mode is a hard boundary: + + +- **`implementation/fix`:** First verify and report `IMPLEMENTATION_WORKTREE` as clean with `HEAD == START_SHA` and no staged entries. The initial executor has `START_SHA == BASE_SHA`; every fix executor has the prior reviewed `CANDIDATE_SHA` as `START_SHA`, never a moving head. After surgical edits, report only **PREPARATORY** checks and the required end-of-edit stable-HEAD check (`HEAD == START_SHA`, no executor staging, exact authorized unstaged delta). Do not create a candidate commit, receipt, or completion claim. +- **`measurement-only`:** Make no source edits, formatting edits, or commits. First prove the supplied base/candidate worktrees are detached, clean, and at their expected SHAs. Execute `scripts/engine-source-hash.sh` in each respective worktree and the exact NUL-safe four-path `BASE_SHA..CANDIDATE_SHA` authority diff from the receipt contract, then write the bound records. Skip projection only when source hashes are equal **and** that authority-diff artifact is empty; otherwise record the canonical non-`NONE` `projection_forced_reason` and produce exactly one direct base projection and one direct candidate projection against the same pinned `AtomicCards.json`, followed by one base-built comparator invocation. `CANNOT_ANSWER` applies only to a missing/mismatched required source-hash, authority-diff, or projection record, or an extra projection. Record every action in the canonical receipt. + +### Canonical receipt contract + +The un-hashed receipt is UTF-8/LF with exactly one final LF and no CR/NUL. Percent-encode every UTF-8 byte except `[A-Za-z0-9._~-]` with uppercase hex. The frozen scope representation is duplicate-free and made exactly by `printf '%s\0' "${SCOPE_PATHS[@]}" | LC_ALL=C sort -z`; record its path and the SHA256 of its exact bytes. Record the exact canonical scoped diff command: + +```bash +git -C "$IMPLEMENTATION_WORKTREE" -c color.ui=false -c diff.noprefix=false -c core.quotepath=true -c diff.orderFile=/dev/null -c diff.interHunkContext=0 -c diff.suppressBlankEmpty=false diff --no-color --no-ext-diff --no-textconv --no-renames --diff-algorithm=myers --no-indent-heuristic --full-index --binary --src-prefix=a/ --dst-prefix=b/ --unified=3 "$BASE_SHA" "$CANDIDATE_SHA" -- "${SCOPE_PATHS[@]}" +``` + +Write the fixed fields, then the mandatory `source_hash_record.base.*` and `source_hash_record.candidate.*` groups, and then all indexed groups in the exact order defined by `/engine-implementer`: no duplicate/missing keys, unordered index, or partial group is valid. Each source-hash group contains, in order, `command`, `expected_sha`, `head_before`, `detached_before`, `clean_before`, `head_after`, `detached_after`, `clean_after`, `exit`, `stdout_path`, `stdout_sha256`, `stderr_path`, `stderr_sha256`, `artifact_path`, `artifact_size`, and `artifact_sha256`. Each group records its exact `scripts/engine-source-hash.sh` command and SHA-bound output artifact. `detached_before` and `detached_after` each prove `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure, not detached-state evidence. The source-hash groups and `projection_authority_diff_*` fields are mandatory even when the hashes are equal. Run exactly `git -C "$IMPLEMENTATION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- Cargo.toml .cargo/config.toml rust-toolchain.toml scripts/engine-source-hash.sh`, capture its NUL-delimited stdout as the recorded path/size/SHA256 artifact, and set `projection_forced_reason` to `NONE`, `SOURCE_HASH_DIFFERENCE`, `EXCLUDED_PROJECTION_AUTHORITY_INPUT_CHANGED`, or `SOURCE_HASH_DIFFERENCE_AND_EXCLUDED_PROJECTION_AUTHORITY_INPUT_CHANGED` according to the two results. Only `NONE` permits no projection section. Record every artifact path, size, and SHA256, but never record or hash the receipt itself; receipt validation is an external reviewer/final-acceptance gate, never a `completion_check` row or receipt artifact. Any malformed receipt or failed identity/digest check is `CANNOT_ANSWER`. ## Hard Rules @@ -27,6 +46,8 @@ These are non-negotiable judgement-call anchors. When tempted to bend one, **sto - Re-read every file immediately before editing it. The content may have changed since the plan was written. - Use targeted `Edit` calls. Never `Write` to replace a whole file when `Edit` would suffice — whole-file writes destroy concurrent agent work. - If a file you planned to touch has changed in unexpected ways, stop and return that as a "current code contradicts the plan" finding. +- Never stage, commit, amend, or move `HEAD`. The orchestrator exclusively owns frozen scope paths and checkpoint commits. +- In `implementation/fix` mode, stop and return if the start check is not a clean `HEAD == START_SHA`, or if the end-of-edit stable-HEAD check has a changed `HEAD`, executor-owned staging, or a delta outside the declared authorized paths. In `measurement-only` mode, source edits are prohibited. A dirty or non-detached measurement worktree is `CANNOT_ANSWER`. ### Parser nom mandate @@ -88,43 +109,51 @@ A "stop and return" is success, not failure. Bandaids that ship are far worse th ## Verification +### Implementation/fix mode: preparatory evidence only + +Run the following only after implementation/fix edits land. Record the commands, starting SHA, ending SHA, and result as `PREPARATORY`; none completes the candidate gate. The orchestrator derives the committed-candidate completion set from these same surface-specific blocks and must rerun the applicable gates at `CANDIDATE_SHA`, retaining the Tilt-first path and isolated direct fallback specified here; it must not treat this preparatory output as their completion result. Existing discriminating-test, maintainer-simulation, selected-authority/provenance, coverage-honesty, and CR-annotation gates below remain single-sourced and mandatory for implementation/fix mode. + After edits land: ```bash -cargo fmt --all +(cd "$IMPLEMENTATION_WORKTREE" && cargo fmt --all) ``` For Rust / engine / parser work: ```bash -if tilt get uiresource clippy >/dev/null 2>&1; then - ./scripts/tilt-wait.sh --timeout 240 clippy test-engine card-data -else - cargo clippy --all-targets -- -D warnings - cargo test -p phase-engine - ./scripts/gen-card-data.sh -fi +(cd "$IMPLEMENTATION_WORKTREE" && + if tilt get uiresource clippy >/dev/null 2>&1; then + ./scripts/tilt-wait.sh --timeout 240 clippy test-engine card-data + else + cargo clippy --all-targets -- -D warnings + cargo test -p phase-engine + ./scripts/gen-card-data.sh + fi) ``` For frontend work: ```bash -if tilt get uiresource clippy >/dev/null 2>&1; then - ./scripts/tilt-wait.sh --timeout 180 check-frontend -else - (cd client && pnpm run type-check && pnpm lint) -fi +(cd "$IMPLEMENTATION_WORKTREE" && + if tilt get uiresource clippy >/dev/null 2>&1; then + ./scripts/tilt-wait.sh --timeout 180 check-frontend + else + (cd client && pnpm run type-check && pnpm lint) + fi) ``` After a non-zero `tilt-wait.sh`, fetch details with `tilt logs --tail 50 --since 2m`. Distinguish your errors from concurrent-agent errors: if an error appears unrelated to your diff, wait several minutes and re-check before intervening (see `feedback_engine_implementer_runs_review` context — other agents fix their own errors). -### Parser diff gate +### Parser preparatory gate If any modified file is under `crates/engine/src/parser/`, inspect added lines for string dispatch: ```bash -git diff --name-only | grep 'crates/engine/src/parser/' | while read f; do - git diff "$f" | grep '^+' | grep -v '^+++' | grep -vE '^\+\s*//' \ +git -C "$IMPLEMENTATION_WORKTREE" diff --name-only -z "$START_SHA" -- crates/engine/src/parser/ \ + | while IFS= read -r -d '' f; do + git -C "$IMPLEMENTATION_WORKTREE" diff --unified=0 "$START_SHA" -- "$f" \ + | grep '^+' | grep -v '^+++' | grep -vE '^\+\s*//' \ | grep -E '\.(contains|starts_with|ends_with|find|rfind|split|splitn|rsplit|split_once)\(' \ | grep -v '#\[test\]' | grep -v '#\[cfg(test)\]' done @@ -132,14 +161,24 @@ done The `rfind`/`split`/`split_once`/`rsplit` arms are deliberate: `scripts/check-parser-combinators.sh` does not catch them, so a green gate is not proof of combinator compliance — this inline grep covers that blind spot. Any output is a hard failure unless it is a test, comment, explicitly annotated non-dispatch structural use, or `oracle_util.rs` dual-string `TextPair` helper work. -For parser changes always run additionally: +For parser changes always run additionally as preparatory checks: ```bash -./scripts/check-parser-combinators.sh -cargo coverage -cargo semantic-audit +(cd "$IMPLEMENTATION_WORKTREE" && ./scripts/check-parser-combinators.sh) +(cd "$IMPLEMENTATION_WORKTREE" && cargo coverage) +(cd "$IMPLEMENTATION_WORKTREE" && cargo semantic-audit) ``` +`./scripts/gen-card-data.sh` and `cargo coverage` may support preparatory inspection but are never fresh candidate measurement evidence. The candidate semantic-impact result is produced only by `measurement-only` mode below. + +### Measurement-only mode: parser evidence + +Execute `scripts/engine-source-hash.sh "$BASE_SHA"` in the detached base projection worktree and `scripts/engine-source-hash.sh "$CANDIDATE_SHA"` in the detached candidate projection worktree, binding each output to its SHA through the required `source_hash_record.base` / `.candidate` receipt groups, then run and record exactly `git -C "$IMPLEMENTATION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- Cargo.toml .cargo/config.toml rust-toolchain.toml scripts/engine-source-hash.sh`. This NUL-delimited artifact is the excluded projection-authority input check. The source-hash groups and authority-diff record remain mandatory when values are equal; only equal hashes with an empty authority-diff artifact set `projection_forced_reason=NONE`, write `NO_PARSE_AFFECTING_CHANGE`, omit the projection section, and never invoke a parser tool. A source-hash difference and/or any authority-diff path sets its canonical non-`NONE` reason and forces `PROJECTED_PARSE_DIFF`. + +For any non-`NONE` `projection_forced_reason`, pin the read-only `AtomicCards.json` input once and use it directly for both detached worktrees. For each side, execute in that side's detached worktree, set its own receipt-owned `CARGO_TARGET_DIR`, and run exactly `cargo build --profile tool --features cli --bin oracle-gen --bin coverage-report --bin coverage-parse-diff`. Run that side's `oracle-gen` and `coverage-report` directly in that worktree to create exactly one projection. Then invoke only the base-built comparator with both `--base-sha "$BASE_SHA"` and `--head-sha "$CANDIDATE_SHA"`. Each build, generator, report, and comparator has a complete indexed receipt group: command/environment, worktree and target, expected SHA, `head_before`, `detached_before`, `head_after`, `detached_after`, `clean_before`, `clean_after`, exit result, stdout/stderr paths and SHA256s, and produced artifacts. `detached_before` and `detached_after` each prove that `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure. The artifact groups enumerate every input/output with path, size, and SHA256. A source-hash difference or excluded-authority-input change is valid; parser evidence is `CANNOT_ANSWER` only for a missing/mismatched required source-hash, authority-diff, or projection record, or an extra projection. + +The receipt names `BASE_SHA` and `CANDIDATE_SHA`, records raw artifacts and their digests, and sets either `NO_PARSE_AFFECTING_CHANGE`, `PROJECTED_PARSE_DIFF`, or `CANNOT_ANSWER`. Do not claim completion verification: that belongs to the orchestrator after this mode returns. + ### Discriminating-test gate Every behavioral change MUST ship at least one test that drives the real pipeline (`apply()` / the scenario runner / the cast-pipeline harness) and **would fail if the fix were reverted**. A test that only asserts the parsed AST shape — an `assert_eq!` on a parsed `AbilityDefinition` / `Effect` / `StaticMode` without resolving it through the engine — does NOT satisfy this gate. It is a shape test, not a regression test. @@ -211,18 +250,29 @@ Any `UNVERIFIED:` line is a hard stop — the rule number does not exist in the ## Output -Return a structured report to the orchestrator. This structured report is your return value and is the contract — always emit it as your final text. You also have the `SendMessage` teammate tool: use it to send the lead a brief progress update or completion notice while you work, and to acknowledge a `shutdown_request` so you can be culled gracefully instead of being tmux-pane-killed. `SendMessage` is purely additive — it never replaces this final structured report. +Return a structured report to the orchestrator. This structured report is your return value and is the contract — always emit it as your final text. It must begin with `Mode`, `BASE_SHA`, and, in implementation/fix mode, `START_SHA` and `IMPLEMENTATION_WORKTREE`; measurement-only mode also names `CANDIDATE_SHA`. Record each canonical-receipt field and artifact at creation. You also have the `SendMessage` teammate tool: use it to send the lead a brief progress update or completion notice while you work, and to acknowledge a `shutdown_request` so you can be culled gracefully instead of being tmux-pane-killed. `SendMessage` is purely additive — it never replaces this final structured report. + +### Implementation/fix output 1. **Diff summary** — files touched, grouped by subsystem, with a one-line purpose per file. -2. **Verification results** — which Tilt resources are green; any failures with `tilt logs` excerpts (own vs unrelated). -3. **Parser diff gate** — pass/fail with offending lines if any. -4. **Discriminating-test gate** — the full production-path coverage map for every behavioral claim, including changed seam/function, production entry point, test name, revert-failing assertion, and sibling/negative cases. Explicitly list any unmapped seam as a stop-and-return item. Confirm no production-reachable arm is left covered only by a degenerate fixture. State if any test is shape-only and whether that is acceptable because semantics remain unsupported/red. -5. **Maintainer-simulation matrix** — the full matrix described above. Explicitly list incomplete rows as stop-and-return items. -6. **CR-annotation diff gate** — the grep result; list any `UNVERIFIED:` rule, or confirm zero. -7. **Judgement calls** — any place you had to choose between two readings of the plan, with the reasoning. -8. **Stop-and-return items** — any places you stopped rather than improvise. -9. **CR annotations added/changed** — each one with the grep command that verified it. -10. **Deviations from the plan** — what changed vs. the plan and why. -11. **Risks** — anything the orchestrator's `/review-impl` loop should pay extra attention to. +2. **Worktree record** — `START_SHA`, `IMPLEMENTATION_WORKTREE`, clean-start and stable-HEAD/end-of-edit attestations, frozen scope-path SHA256, and the canonical receipt path. State explicitly that preparatory evidence is not completion evidence. +3. **PREPARATORY verification results** — which Tilt resources are green; any failures with `tilt logs` excerpts (own vs unrelated). State explicitly that this is not completion evidence. +4. **Parser preparatory gate** — pass/fail with offending lines if any. +5. **Discriminating-test gate** — the existing full production-path coverage map for every behavioral claim, including changed seam/function, production entry point, test name, revert-failing assertion, and sibling/negative cases. Explicitly list any unmapped seam as a stop-and-return item. Confirm no production-reachable arm is left covered only by a degenerate fixture. State if any test is shape-only and whether that is acceptable because semantics remain unsupported/red. +6. **Maintainer-simulation matrix** — the existing full matrix. Explicitly list incomplete rows as stop-and-return items. +7. **CR-annotation diff gate** — the grep result; list any `UNVERIFIED:` rule, or confirm zero. +8. **Judgement calls** — any place you had to choose between two readings of the plan, with the reasoning. +9. **Stop-and-return items** — any places you stopped rather than improvise. +10. **CR annotations added/changed** — each one with the grep command that verified it. +11. **Deviations from the plan** — what changed vs. the plan and why. +12. **Risks** — anything the orchestrator's checkpoint, measurement, completion, or `/review-impl` loop should pay extra attention to. + +### Measurement-only output + +1. **Identity and worktree record** — `BASE_SHA`, `CANDIDATE_SHA`, `IMPLEMENTATION_WORKTREE`, detached/clean start and end checks, frozen scope-path SHA256, canonical receipt path, and all artifact paths. +2. **Source-hash records** — both `engine-source-hash.sh` outputs bound to their SHAs and their equality/difference result. +3. **Parser evidence** — the two SHA-bound source-hash outputs, the exact authority-diff artifact and `projection_forced_reason`, and either `NO_PARSE_AFFECTING_CHANGE` with no projection section, or the pinned `AtomicCards.json`, exact isolated tool-build records, base/candidate direct projections, base-built comparator with both SHA flags, and every indexed receipt/artifact record. +4. **No-edit/no-commit attestation** — confirm source diff and `HEAD` did not change during measurement. +5. **Stop-and-return items, deviations, and risks** — especially any condition that makes the ledger `CANNOT_ANSWER`. Do NOT commit. Do NOT push. The orchestrator decides what to stage and when. diff --git a/.claude/skills/engine-implementer/SKILL.md b/.claude/skills/engine-implementer/SKILL.md index dddd8e1e19..ee58d1356e 100644 --- a/.claude/skills/engine-implementer/SKILL.md +++ b/.claude/skills/engine-implementer/SKILL.md @@ -15,15 +15,44 @@ This is the orchestrator for the phase.rs implementation pipeline. It runs as a |---|---|---| | 1. Produce plan | **Spawned `general-purpose` agent** invoking `/engine-planner` | Fresh context = plan is shaped by the task, not by the conversation history that led here | | 2. Review plan | **Spawned `general-purpose` agent** invoking `/review-engine-plan` | Fresh context = honest architectural review, independent of the planner | -| 3. Implement | **Spawned `engine-implementation-executor` agent** | Surgical edits + Tilt verification; worktree-isolatable | -| 4. Spot-check verification | This thread | Re-run anything the executor skipped; confirm formatting | -| 5. Review implementation | **Spawned `general-purpose` agent** invoking `/review-impl` | Independent reviewer, not the implementer | -| 6. Commit | This thread | Owner of the working tree decides what gets staged | +| 3. Implement | **Spawned `engine-implementation-executor` agent** | Baseline measurement, surgical edits, and preparatory checks; never commits | +| 4. Checkpoint + measure | This thread, then a fresh measurement executor | Orchestrator creates the candidate commit; isolated executor measures that immutable candidate | +| 5. Complete verification | This thread | Verify the committed candidate, never an in-flight working tree | +| 6. Review implementation | **Spawned `general-purpose` agent** invoking `/review-impl` | Independent review of the immutable base-to-candidate diff | +| 7. Final acceptance | This thread | Accept only the exact reviewed checkpoint candidate | **Runtimes without subagent spawning (contributor environments — Codex CLI, plain LLM sessions).** The pipeline's value comes from context isolation between author and reviewer, not from the spawning mechanism. If your runtime cannot spawn agents, do NOT silently degrade to reviewing your own work in the same context — that is the failure mode this skill exists to prevent. Instead: run each step against a fresh context (new session/conversation per step when your runtime supports it), and for every review step hand the reviewer ONLY the artifact under review (the full plan, or the unified diff), the original task description, `CLAUDE.md`, and the relevant skill (`/review-engine-plan` or `/review-impl`) — never the conversation that produced it. If even that is impossible, say so explicitly in the final report and in the PR body under a "Validation Failures" heading; do not claim the review loop ran clean. The orchestrator never authors content itself. Its only jobs are: spawn agents, route their output to the next step, loop review steps until clean, own the commit, and gracefully cull each spawned agent once its output is consumed (send a `shutdown_request` and wait for the `shutdown_response` ack — spawned agents now carry `SendMessage`, so they cull gracefully instead of being pane-killed). The structured report each agent returns stays the authoritative step handoff; SendMessage is an additive progress/acknowledgment channel, not a replacement. +## Run ownership, checkpoint identity, and the canonical receipt + +Before dispatching an executor, the orchestrator records `BASE_SHA`, a frozen in-scope path representation, its SHA256, and an externally owned run directory. The representation is a file made exactly by `printf '%s\0' "${SCOPE_PATHS[@]}" | LC_ALL=C sort -z`, with no duplicate path records; `scope_paths_sha256` is the SHA256 of those exact NUL-delimited bytes. `BASE_SHA` never changes. The orchestrator alone stages or commits scope paths; executors never stage, commit, amend, or move `HEAD`. + +Every implementation/fix dispatch has a named `START_SHA` and `IMPLEMENTATION_WORKTREE`. The first round has `START_SHA == BASE_SHA`; a fix round starts from the prior reviewed `CANDIDATE_SHA` in a fresh implementation worktree. Before edits and immediately before checkpoint, record that `HEAD == START_SHA`, the index has no executor staging, and the authorized unstaged delta is exactly the frozen scope. A changed `HEAD`, staged entry, unexpected path, or changed diff digest stops the run. The checkpoint commit is the sole candidate identity: explicitly stage only the frozen paths, commit only those paths, record `CANDIDATE_SHA`, and immediately prove `rev-parse HEAD == CANDIDATE_SHA`. Never measure an uncommitted tree or a moving `HEAD`. + +Keep artifacts outside all worktrees. Each checkpoint owns one un-hashed canonical receipt, for example `/engine-implementer-runs//candidates//receipt`. It is the only provenance contract: do not create manifests, seals, provenance envelopes, replica/quorum records, or a parallel ledger. + +The receipt is UTF-8 with LF line endings and exactly one final LF. It has one `key=value` line per field, no CR or NUL, and percent-encodes every UTF-8 byte except `[A-Za-z0-9._~-]` using uppercase hex. Keys appear exactly once in this fixed order: `format=engine-implementer-receipt-v1`, `base_sha`, `start_sha`, `candidate_sha`, `head_sha`, `scope_paths_path`, `scope_paths_sha256`, `scoped_diff_command`, `scoped_diff_path`, `scoped_diff_size`, `scoped_diff_sha256`, `base_source_hash`, `candidate_source_hash`, `projection_authority_diff_command`, `projection_authority_diff_path`, `projection_authority_diff_size`, `projection_authority_diff_sha256`, `projection_forced_reason`, and `parser_evidence`. `scope_paths_path` names the frozen NUL-delimited, `LC_ALL=C sort -z` representation above. `scoped_diff_command` is exactly: + +```bash +git -C "$IMPLEMENTATION_WORKTREE" -c color.ui=false -c diff.noprefix=false -c core.quotepath=true -c diff.orderFile=/dev/null -c diff.interHunkContext=0 -c diff.suppressBlankEmpty=false diff --no-color --no-ext-diff --no-textconv --no-renames --diff-algorithm=myers --no-indent-heuristic --full-index --binary --src-prefix=a/ --dst-prefix=b/ --unified=3 "$BASE_SHA" "$CANDIDATE_SHA" -- "${SCOPE_PATHS[@]}" +``` + +Next come the mandatory `source_hash_record.base.*` and `source_hash_record.candidate.*` groups, each in this order: `command`, `expected_sha`, `head_before`, `detached_before`, `clean_before`, `head_after`, `detached_after`, `clean_after`, `exit`, `stdout_path`, `stdout_sha256`, `stderr_path`, `stderr_sha256`, `artifact_path`, `artifact_size`, `artifact_sha256`. Each group records the exact `scripts/engine-source-hash.sh` invocation and its SHA-bound output artifact. `detached_before` and `detached_after` prove `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure, not detached-state evidence. These source-hash groups and the `projection_authority_diff_*` record are mandatory even when hashes are equal. `projection_authority_diff_command` is exactly `git -C "$IMPLEMENTATION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- Cargo.toml .cargo/config.toml rust-toolchain.toml scripts/engine-source-hash.sh`; capture its NUL-delimited stdout as the recorded path/size/SHA256 artifact. `projection_forced_reason` is exactly one of `NONE`, `SOURCE_HASH_DIFFERENCE`, `EXCLUDED_PROJECTION_AUTHORITY_INPUT_CHANGED`, or `SOURCE_HASH_DIFFERENCE_AND_EXCLUDED_PROJECTION_AUTHORITY_INPUT_CHANGED`. Next comes no projection section only when `projection_forced_reason=NONE`; otherwise write `projection_step_count` followed by every `projection_step..*` group in strictly ascending zero-based `N`; each group contains, in this order, `side`, `command`, `env`, `worktree`, `target`, `expected_sha`, `head_before`, `detached_before`, `head_after`, `detached_after`, `clean_before`, `clean_after`, `exit`, `stdout_path`, `stdout_sha256`, `stderr_path`, `stderr_sha256`, `produced_artifacts`. The detached fields use the same exact exit-`1` / empty-stdout-and-stderr proof. Then write `completion_check_count` and each strictly ascending `completion_check..*` group with the same record fields, `artifact_count` and each strictly ascending `artifact..path`, `.size`, `.sha256`, then `declared_absent_input_count` and each strictly ascending `declared_absent_input..path`. No indexed member may be omitted; a conditional section is omitted only as stated. Artifact rows cover every source-hash output, authority-diff output, projection output, command stdout/stderr capture, scoped diff, and completion artifact. The receipt itself is never listed or hashed; receipt validation is an external reviewer/final-acceptance gate and never a `completion_check` row or receipt artifact. A malformed, unordered, duplicate, missing, or digest-mismatched receipt is `CANNOT_ANSWER`. + +The parser-impact decision happens only after the checkpoint. Record `scripts/engine-source-hash.sh "$BASE_SHA"` and `scripts/engine-source-hash.sh "$CANDIDATE_SHA"` in the mandatory `source_hash_record.base` / `.candidate` receipt groups, including their SHA-bound output artifacts. Also run and record the exact `projection_authority_diff_command`. Set `projection_forced_reason=NONE` only when the source hashes are equal and that NUL-delimited path artifact is empty. A source-hash difference and/or any path in that artifact forces `parser_evidence=PROJECTED_PARSE_DIFF`, exactly one direct base projection, and exactly one direct candidate projection from the same read-only, pinned `AtomicCards.json`; choose the canonical reason that names both causes when both apply. Only `NONE` permits `parser_evidence=NO_PARSE_AFFECTING_CHANGE` and no projection inputs, projection records, or comparator outputs. Record the pinned `AtomicCards.json` path and SHA256 as artifacts whenever projection is forced. `CANNOT_ANSWER` applies only when a required source-hash, authority-diff, or projection record is missing or mismatched, or an extra projection is present. Do not download, regenerate, copy, sample, or compare replicas. + +For each projection use its detached clean worktree and an isolated target directory. Build exactly with `CARGO_TARGET_DIR= cargo build --profile tool --features cli --bin oracle-gen --bin coverage-report --bin coverage-parse-diff`. Run that side's `oracle-gen` directly against the pinned data root, write its `card-data.json` and `card-names.json` under the projection directory, then run that side's `coverage-report` directly against the projection directory to write `coverage-data.json`. Capture the exact command, environment, worktree identity/clean checks, binary target, exit status, stdout/stderr, and every produced artifact in a projection-step receipt group. Each group's ordered `detached_before` and `detached_after` fields must prove `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure. Invoke the **base-built** comparator directly once against the two projected coverage files: + +```bash +"$BASE_TARGET/tool/coverage-parse-diff" "$BASE_PROJECTION/coverage-data.json" "$CANDIDATE_PROJECTION/coverage-data.json" \ + --base-sha "$BASE_SHA" --head-sha "$CANDIDATE_SHA" \ + --markdown "$RUN_ROOT/parse-diff.md" --json "$RUN_ROOT/parse-diff.json" --max-clusters +``` + +Record that comparator invocation and both outputs as projection steps/artifacts. `parser_evidence=PROJECTED_PARSE_DIFF` only after all required base, candidate, and comparator records verify. Do not put these operational artifacts in the implementation diff unless the reviewed plan explicitly scopes them. + ## Inputs Either: @@ -31,7 +60,7 @@ Either: 1. A task description (cards, CR rules, Oracle text patterns, affected subsystems, expected behavior), or 2. A pre-existing plan — treat as a draft unless it has already passed `/review-engine-plan` to clean. -If running in worktree-isolation mode, prepare the worktree before Step 3 and pass its path to the executor agent. Per `feedback_session_default_no_worktree`, do not re-ask about worktrees during an active pipeline session — use the session default. +Before Step 3, prepare and verify a clean `IMPLEMENTATION_WORKTREE` at `START_SHA`. After its checkpoint, prepare clean detached base and candidate projection worktrees at `BASE_SHA` and `CANDIDATE_SHA`, and a distinct clean detached `COMPLETION_WORKTREE` at `CANDIDATE_SHA`; no projection or completion worktree is used for implementation. Per `feedback_session_default_no_worktree`, do not re-ask about worktrees during an active pipeline session — use the session default. ## Pipeline @@ -63,70 +92,58 @@ Each review must run in a fresh agent context — never reuse the previous revie Spawn the `engine-implementation-executor` agent. -**Spawn inputs:** the reviewed clean plan in full; in-bounds / out-of-bounds file scope; worktree path if applicable; any prior reviewer findings (none on first round). +**Spawn inputs:** mode `implementation/fix`; the reviewed clean plan in full; `BASE_SHA`; named `START_SHA`; frozen in-bounds / out-of-bounds path list and its SHA256; named `IMPLEMENTATION_WORKTREE`; the canonical receipt path; and any prior reviewer findings (none on first round). First round: `START_SHA == BASE_SHA`. Fix round: `START_SHA` is the previously reviewed `CANDIDATE_SHA`, never a moving branch head. -The executor edits files, runs Tilt-first verification, runs the parser diff gate if any parser file changed, and returns a structured report (diff summary, verification results, judgement calls, stop-and-return items, CR annotations verified, deviations, risks). +The implementation executor edits only its frozen scope and runs **preparatory** checks. Preparatory success is not completion evidence. Its existing discriminating-test, selected-authority, coverage-honesty, maintainer-simulation, and CR-annotation gates remain the authoritative gates; do not restate or replace them here. If the executor returns "stop and return" items (plan contradicts current code, ad hoc parser dispatch unavoidable, CR uncertain), do NOT improvise around them. Loop back to Step 1, feed the executor's findings into `/engine-planner` as new constraints, and re-run Steps 1–3. -### Step 4 — Spot-check verification - -The executor already ran the appropriate Tilt block. Re-run only what the executor skipped or what changed because of intervening commits from other agents. Always confirm formatting: - -```bash -cargo fmt --all -``` - -After a non-zero `tilt-wait.sh`, fetch details with `tilt logs --tail 50 --since 2m`. Distinguish your diff's errors from concurrent-agent errors per CLAUDE.md's "Defer to other active agents" guidance. - -Confirm the executor's pre-commit artifacts came back complete: the **discriminating-test gate** (a complete production-path coverage map for every behavioral claim — changed seam/function, production entry point, test name, revert-failing assertion, and sibling/negative cases), the **maintainer-simulation matrix** (selected authority, binding time, storage, consuming function, invalidation behavior, hostile fixture rows, and serialized-surface impact for each claim/seam), and the **CR-annotation diff gate** (every added/changed `CR ` resolves in `docs/MagicCompRules.txt`). Do not accept generic "gate: pass" summaries. If any changed seam is unmapped, any maintainer-simulation row is missing or superficial, the executor shipped only shape tests for runtime semantics or coverage-support claims, parser work accepts Oracle text while dropping semantics without preserving an honest `Unimplemented`/coverage gap, a rules-bearing "this way" / "that source" / "chosen" / "cast using" / "from among them" / duration-bound "you" path relies on unproven global rescanning, or any CR annotation came back `UNVERIFIED`, loop back to Step 3 with that as a fix constraint — do not commit. +**Large JSON fixture constraint.** Any repository-bound JSON fixture ≳100KB (test fixtures, game-state dumps, generated maps — not runtime/config JSON whose consumers read plain `.json`) gets `gzip -9 -n` (`-n` keeps the archive byte-reproducible) and loads via the established inflate pattern: `include_bytes!("….json.gz")` + a test-local `gunzip` helper using `flate2::read::GzDecoder` (examples: `tests/integration/combo_infinite_pile.rs`, `cr733_resolved_commands_p0.rs`). Never commit the uncompressed twin alongside the `.json.gz`. If a fixture is regenerated by a script, note in the reading test that regeneration requires re-gzipping. -### Step 5 — Review implementation until clean (unbounded loop) +### Step 4 — Checkpoint, then measure the committed candidate -Spawn a `general-purpose` agent and instruct it to invoke `/review-impl` against the implementation diff. The reviewer MUST also verify the originally reported bug or requirement is actually fixed via a discriminating runtime test — not just that the code looks clean (`feedback_review_impl_verify_bug_fixed`). The reviewer MUST audit the executor's production-path coverage map, maintainer-simulation matrix, and parser coverage-honesty statement; a clean review is invalid unless it explicitly confirms those artifacts are complete or returns findings. +Before Step 3, the orchestrator records `IMPLEMENTATION_WORKTREE`'s staged/unstaged path snapshot and clean `HEAD == START_SHA` attestation. Before staging, it repeats the stable-HEAD check and records its exact implementation delta: the approved path list and the SHA256 of each approved path's `START_SHA..working-tree` diff. It must first prove, from that snapshot, that no pre-existing staged or unstaged change overlaps an approved path; if attribution is ambiguous, stop and return rather than unstage, sweep in, or overwrite another agent's work. The checkpoint is the candidate commit: stage each approved path by explicit pathspec — never `git add -A` — and never commit without explicit pathspec because the shared index can sweep in other agents' staged files (`feedback_git_add_file_bundles_concurrent_work`, `feedback_shared_index_commit_pathspec`). Stage and commit only the explicit approved paths, never alter unrelated index entries, then record the full `CANDIDATE_SHA`. Immediately run `git -C "$IMPLEMENTATION_WORKTREE" rev-parse HEAD` and compare its output to `CANDIDATE_SHA`; a mismatch stops the run. Verify that `START_SHA..CANDIDATE_SHA` contains only the recorded authorized delta (paths and diff digests), and retain the original `BASE_SHA..CANDIDATE_SHA` diff for final review; otherwise stop and return. This is an orchestrator-only commit; the executor never performs it. Do not measure an uncommitted tree or use a moving `HEAD` as the candidate identity. Verify HEAD is on a branch before any explicitly requested push (`feedback_verify_head_attached_before_push`), never pipe `git push` into `tail`/`head` (`feedback_git_push_no_pipe`), and never push unless explicitly requested. -**Reviewer spawn inputs:** `git diff` of the in-flight branch against its base; the original task description; the reviewed plan; the executor's discriminating-test map and maintainer-simulation matrix. +Spawn a **fresh** `engine-implementation-executor` in mode `measurement-only` with `BASE_SHA`, `CANDIDATE_SHA`, named `IMPLEMENTATION_WORKTREE`, frozen scope paths, the canonical receipt path, a detached clean base worktree, a detached clean candidate worktree, and the read-only pinned data root containing `AtomicCards.json`. This executor makes no source edits and no commits. -If the reviewer returns findings, spawn a **fresh** `engine-implementation-executor` agent to apply fixes: +The measurement-only executor runs `scripts/engine-source-hash.sh` in the respective detached base/candidate projection worktrees at `BASE_SHA` and `CANDIDATE_SHA`, stores both SHA-bound outputs in the mandatory `source_hash_record.base` / `.candidate` receipt groups, and runs the exact four-path NUL-safe `projection_authority_diff_command` from the receipt contract. Equal hashes set `parser_evidence=NO_PARSE_AFFECTING_CHANGE` and permit no parser projection only when that authority-diff artifact is empty (`projection_forced_reason=NONE`). A source-hash difference or any authority-diff path forces the canonical non-`NONE` reason and requires the exact one base projection, one candidate projection, and base-built comparator defined in the receipt contract. Only a missing/mismatched source-hash, authority-diff, or projection record, or an extra projection, is `CANNOT_ANSWER`. It records every command, environment, base/candidate worktree identity before and after the operation, clean state before and after, output capture, produced artifact, and artifact digest. `./scripts/gen-card-data.sh` and `cargo coverage` are never projection evidence. -**Fix-round executor spawn inputs:** the reviewed plan; current `git diff HEAD` of the in-flight branch; the reviewer findings as the fix constraints; same scope and worktree as the original Step 3 spawn. +### Step 5 — Committed-candidate completion verification -Then spawn a **fresh** review agent against the new diff. Repeat until a full review round returns zero findings. Per `feedback_engine_implementer_runs_review`, never self-review — always spawn an isolated reviewer. +Completion verification occurs only after the checkpoint, in the distinct clean detached `COMPLETION_WORKTREE` at `CANDIDATE_SHA`, and records detached identity, `HEAD`, and clean checks at start and end. Every completion or parser command must either use `git -C "$COMPLETION_WORKTREE"` or execute with `COMPLETION_WORKTREE` as its working directory. The candidate parser gate must enumerate its range NUL-safely with `git -C "$COMPLETION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- crates/engine/src/parser/`; any loop reading it uses `IFS= read -r -d ''`. The required set is both scope-/plan-derived checks and every surface-derived gate applicable to the changed paths in the executor's existing implementation/fix verification blocks: formatting for implementation changes, the Rust/engine/parser Tilt-first-or-isolated-direct block for Rust/engine/parser paths, the frontend Tilt-first-or-isolated-direct block for frontend paths, and the parser preparatory gate for parser paths. Reuse those existing blocks by surface; do not duplicate their command recipes here. Use candidate-SHA-bound CI or Tilt evidence only when the evidence itself proves that binding; otherwise use the isolated direct fallback already specified by the applicable block. Never promote the implementation executor's preparatory result to completion evidence. Its receipt completion-check groups must enumerate every required check and, for each, record the exact command, exit result, detached `CANDIDATE_SHA` identity and clean status at start and end, including ordered `detached_before` and `detached_after` proofs that `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure. A missing check, nonzero/unknown exit result, identity mismatch, detached-state failure, or dirty status fails completion. -**Repeat until a full review round returns zero findings.** No iteration cap. Per `feedback_engine_implementer_runs_review`, never self-review — always spawn an isolated reviewer. +For Markdown-only policy updates, the mandatory completion-check set is limited to scope-path, SHA-identity, and Markdown/diff checks; do not run Cargo or Tilt. Receipt validation is an external reviewer/final-acceptance gate, never a self-referential completion-check record or receipt artifact. -### Step 6 — Commit +### Step 6 — Review the immutable candidate -Commit only after: +Spawn a fresh `general-purpose` agent and instruct it to invoke `/review-impl` against `BASE_SHA..CANDIDATE_SHA`, the original task, reviewed plan, frozen scope paths, `START_SHA`, implementation-worktree start/end stable-HEAD attestations, the detached `COMPLETION_WORKTREE` start/end identity attestations, and the canonical receipt. The reviewer validates the receipt before applying the universal lenses: exact fixed-field order; percent encoding; no CR/NUL; every conditional/indexed section; candidate/head and worktree identity; the frozen NUL-sorted scope representation and hash; the exact scoped-diff and authority-diff commands and bytes; both mandatory source-hash records even on equal hashes; every artifact size/SHA256; and every projection/completion group's ordered `detached_before`/`detached_after` proof that `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr (any other result is an operational failure). It must accept an omitted projection only if the source hashes are equal, the authority-diff artifact is empty, and `projection_forced_reason=NONE`; otherwise, it must reject it. It must reject any forced-projection receipt only when it lacks the pinned `AtomicCards.json`, isolated exact tool builds, exactly one direct base and candidate projection, or one base-built comparator invocation with both `--base-sha` and `--head-sha`. After validating it, the reviewer emits the receipt SHA256 externally in its review result; that digest never appears in the receipt. It must validate that first-round `START_SHA == BASE_SHA`, each fix `START_SHA` is the prior reviewed candidate, and the implementation worktree was clean at start and stable at checkpoint. The reviewer MUST fail missing or unsuccessful mandatory completion checks, verify the originally reported bug or requirement is actually fixed via the existing discriminating runtime-test gate, and audit the existing maintainer-simulation and coverage-honesty artifacts. -- Step 2 plan-review loop is clean, -- Step 4 verification passes (or unrelated failures are clearly isolated to other agents), -- Step 5 implementation-review loop is clean. +If review returns findings, spawn a **fresh** implementation/fix executor with the findings as constraints. The round is always: -Stage by pathspec — never `git add -A` and never `git commit` without a pathspec, because the shared index can sweep in other agents' staged files (`feedback_git_add_file_bundles_concurrent_work`, `feedback_shared_index_commit_pathspec`): - -```bash -git status --short -git diff --stat -git diff --cached # confirm nothing unrelated is staged -git commit -m ": " +```text +edit + preparatory checks → orchestrator checkpoint → fresh measurement-only executor +→ committed-candidate completion verification → fresh BASE_SHA..CANDIDATE_SHA review ``` -Verify HEAD is on a branch before any push (`feedback_verify_head_attached_before_push`). Never pipe `git push` into `tail`/`head` (`feedback_git_push_no_pipe`). Do not push unless explicitly requested. +Every round keeps the original `BASE_SHA` and frozen scope paths. Every checkpoint receives its own receipt. Never review a diff that includes a later unmeasured fix. + +### Step 7 — Final acceptance -**Large JSON fixtures must be gzipped before commit.** Any repository-bound JSON fixture ≳100KB (test fixtures, game-state dumps, generated maps — not runtime/config JSON whose consumers read plain `.json`) gets `gzip -9 -n` (`-n` keeps the archive byte-reproducible) and loads via the established inflate pattern: `include_bytes!("….json.gz")` + a test-local `gunzip` helper using `flate2::read::GzDecoder` (examples: `tests/integration/combo_infinite_pile.rs`, `cr733_resolved_commands_p0.rs`). Never commit the uncompressed twin alongside the `.json.gz`. If a fixture is regenerated by a script, note in the reading test that regeneration requires re-gzipping. +Accept only when the plan-review loop is clean, the reviewer emitted a matching external receipt SHA256, final acceptance independently revalidates the receipt and every recorded artifact, parser evidence and completion checks pass, and the fresh implementation review returns zero findings. The receipt validation/hash are acceptance evidence, never completion-check rows or receipt artifacts. Immediately run `git -C "$IMPLEMENTATION_WORKTREE" rev-parse HEAD` and compare its output to `CANDIDATE_SHA`; if it differs, the review is stale and the current head must repeat the checkpoint-to-review sequence. Do not treat review of an ancestor as review of current work. ## Final Report -Return after the commit: +Return after final acceptance: 1. Plan-review rounds (count) and final clean result. 2. What changed, grouped by subsystem and file. 3. Key architectural decisions. -4. Verification commands run and results (executor's + your spot-checks). -5. Implementation-review rounds (count) and final clean result. -6. Commit hash and staged file list. -7. Coverage impact for parser changes. -8. Deviations from the plan with reasons. -9. Self-flagged risks and judgment calls (yours + executor's). -10. Remaining items, if any, with reasons. +4. `BASE_SHA`, accepted `CANDIDATE_SHA`, frozen scope paths, and run-artifact root. +5. `START_SHA`/`IMPLEMENTATION_WORKTREE` records for every round; the canonical receipt path, parser-evidence branch, direct-projection records when hashes differ, and completion-check identity. +6. Verification commands run and results, separated into preparatory and completion evidence. +7. Implementation-review rounds (count), reviewed SHA, and final clean result. +8. Checkpoint commit hash and staged file list. +9. Coverage impact for parser changes. +10. Deviations from the plan with reasons. +11. Self-flagged risks and judgment calls (yours + executor's). +12. Remaining items, if any, with reasons. diff --git a/.claude/skills/review-impl/SKILL.md b/.claude/skills/review-impl/SKILL.md index bcaa01b480..af946c512a 100644 --- a/.claude/skills/review-impl/SKILL.md +++ b/.claude/skills/review-impl/SKILL.md @@ -12,8 +12,8 @@ Review for gaps: things that are missing or wrong. Do not spend findings on styl 1. Identify the changed surface from the diff, commit, or named files. 2. Classify the surface area: engine logic, parser, frontend/UI, multiplayer/transport, AI heuristics, deck/format/feeds, build/CI/release, or docs. 3. Apply only the relevant lenses below. -4. If the scope is a PR, fetch whatever external review comments exist (CodeRabbit, human reviewers) and confirm-or-refute each against the current head with code evidence, folding confirmed findings into your own. **Assume none exist by default** — Gemini Code Assist has been sunset, so no bot is guaranteed to have pre-screened this PR. Your own lenses are the complete review, not a supplement to a bot's; do not under-invest expecting a backstop. Where an external finding *does* exist, silently omitting it — or returning a verdict less severe than an open, unrefuted finding from another reviewer — is itself a defect. -5. If the scope is a PR touching engine/parser source, the parse-diff sticky comment (marker ``) is required evidence: fetch its full body and confront the card-level diff against the PR's claimed scope. Unexplained gained/lost/changed cards are findings (unintended parser blast radius). A *Baseline pending* body means the diff is unavailable — flag it so the handler brings the branch current to regenerate it; an absent comment despite changed engine source means CI evidence is missing for the current head. +4. If the scope is a PR, fetch whatever external review comments exist (CodeRabbit, human reviewers) and confirm-or-refute each against the current head with code evidence, folding confirmed findings into your own. **Assume none exist by default** — Gemini Code Assist has been sunset, so no bot is guaranteed to have pre-screened this PR. Your own lenses are the complete review, not a supplement to a bot's; do not under-invest expecting a backstop. Where an external finding *does* exist, silently omitting it — or returning a verdict less severe than an open, unrefuted finding from another reviewer — is itself a defect. Review comments, checks, and uploaded/sticky artifacts count only when their evidence identifies the current PR head SHA; otherwise report the evidence as missing rather than attributing it to the current diff. +5. If the scope is a PR touching engine/parser source, the parse-diff sticky comment (marker ``) is required evidence: fetch its full body and confront the card-level diff against the PR's claimed scope. Unexplained gained/lost/changed cards are findings (unintended parser blast radius). A *Baseline pending* body means the diff is unavailable — flag it so the handler brings the branch current to regenerate it; an absent comment despite changed engine source, or a comment/artifact not bound to the current PR head SHA, means CI evidence is missing for the current head. 6. Report findings only. Silence means LGTM. When `pr-contribution-handler` explicitly requests the manual quality gate, add `Quality Gate: PASS|FAIL` before findings. PASS requires all three current-PR facts: (1) claimed parse-impact count equals the measured parse-diff count and the normalized card sets are identical, using the full artifact when the sticky comment truncates examples; (2) the change is at an existing authority/right seam and reuses its vocabulary; and (3) a production-pipeline test is demonstrated to fail when the production change is reverted. On PASS, return the applicable existing praise tokens (`right-seam`, `scope-discipline`, `discriminating-runtime-test`, `parameterized-not-proliferated`) for the ordinary review/enqueue event. Never infer quality from Tier or standing and never create a `quality_recommended` event. @@ -25,15 +25,29 @@ Skip checks CI already enforces: - `scripts/coverage-regression-check.sh --fail-on-engine` - TypeScript `pnpm type-check` and `pnpm lint` -## Engine-Implementer Matrix Mode +## Engine-Implementer Checkpoint Mode -Default review output is findings-only. Exception: when `/engine-implementer` invokes this skill with an executor maintainer-simulation matrix, emit one short line before findings: +Default review output is findings-only. Exception: when `/engine-implementer` invokes this skill against a checkpointed candidate, it supplies `BASE_SHA`, `CANDIDATE_SHA`, the committed reproducible `BASE_SHA..CANDIDATE_SHA` diff, frozen scope paths, the named `START_SHA`/`IMPLEMENTATION_WORKTREE` clean-start/stable-HEAD attestations, the detached `COMPLETION_WORKTREE` start/end identity attestations, the canonical receipt, completion evidence, and the existing maintainer-simulation matrix. Validate the receipt before applying universal lenses. Confirm the first round has `START_SHA == BASE_SHA`, each fix starts from the prior reviewed candidate, the implementation worktree was clean at start and stable at checkpoint, the completion worktree was detached, clean, and exactly at `CANDIDATE_SHA` before and after completion, and the receipt's candidate/head values are exactly `CANDIDATE_SHA`. + +The receipt must be the un-hashed UTF-8/LF `engine-implementer-receipt-v1` document with one final LF, no CR/NUL, percent-encoded values (only `[A-Za-z0-9._~-]` raw), the fixed fields in their mandated order, the mandatory `source_hash_record.base.*` and `.candidate.*` groups, and complete ascending indexed groups. Verify the frozen scope representation is duplicate-free NUL-delimited bytes from `printf '%s\0' "${SCOPE_PATHS[@]}" | LC_ALL=C sort -z`, and verify its recorded path and SHA256. Verify `scoped_diff_command` is exactly `git -C "$IMPLEMENTATION_WORKTREE" -c color.ui=false -c diff.noprefix=false -c core.quotepath=true -c diff.orderFile=/dev/null -c diff.interHunkContext=0 -c diff.suppressBlankEmpty=false diff --no-color --no-ext-diff --no-textconv --no-renames --diff-algorithm=myers --no-indent-heuristic --full-index --binary --src-prefix=a/ --dst-prefix=b/ --unified=3 "$BASE_SHA" "$CANDIDATE_SHA" -- "${SCOPE_PATHS[@]}"`, then reproduce and hash those scoped-diff bytes. Verify `projection_authority_diff_command` is exactly `git -C "$IMPLEMENTATION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- Cargo.toml .cargo/config.toml rust-toolchain.toml scripts/engine-source-hash.sh`, reproduce and hash its NUL-delimited output, and require its recorded path/size/SHA256. Each source-hash group must contain, in order, `command`, `expected_sha`, `head_before`, `detached_before`, `clean_before`, `head_after`, `detached_after`, `clean_after`, `exit`, `stdout_path`, `stdout_sha256`, `stderr_path`, `stderr_sha256`, `artifact_path`, `artifact_size`, and `artifact_sha256`; it must bind the exact `scripts/engine-source-hash.sh` command and output artifact to its expected SHA. Verify every source-hash, projection, and completion group contains ordered `detached_before` and `detached_after` fields, each proving `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure, not detached-state evidence. Verify every recorded artifact path, size, and SHA256, but do not expect a hash for the receipt itself. Its fixed identity fields, scoped-diff command/path/size/digest, source hashes, authority-diff command/path/size/digest, canonical `projection_forced_reason`, mandatory source-hash records, parser evidence, completion records, artifacts, and declared absent inputs are mandatory; the projection section is omitted only when source hashes are equal, the authority-diff output is empty, and `projection_forced_reason=NONE`. A missing, duplicate, unordered, malformed, or digest-mismatched field is a blocking finding. + +In this mode, emit these lines before findings: ```text +Review Head: +Receipt SHA256: +Semantic-Impact Gate: PASS|FAIL +Completion Gate: PASS|FAIL Maintainer-Simulation Gate: PASS|FAIL ``` -Use `PASS` only if every changed seam has a concrete row covering production entry, first production branch reached, selected authority / bound value when applicable, binding time, live vs snapshotted semantics, storage, consuming function, invalidation behavior, hostile fixtures, and serialized-surface impact. Use `FAIL` when any row is missing, superficial, or contradicted by the diff, and report the specific gap as a normal finding. Outside this scoped mode, keep silence-as-LGTM behavior. +`Review Head` must be the supplied `CANDIDATE_SHA`, the reviewed diff must reproduce from exactly `BASE_SHA..CANDIDATE_SHA`, and `Receipt SHA256` must be the SHA256 of the validated receipt bytes emitted externally in this review result; otherwise report a blocking finding. The receipt never contains or hashes itself. `Semantic-Impact Gate` passes only when the receipt validates and either (a) equal SHA-bound source hashes, an empty authority-diff output, and `projection_forced_reason=NONE` set `NO_PARSE_AFFECTING_CHANGE` with no projection section, or (b) a non-`NONE` canonical forced reason sets `PROJECTED_PARSE_DIFF` and contains all direct-projection records. + +`Semantic-Impact Gate` rejects missing/mismatched source-hash, authority-diff, or projection evidence, including either mandatory source-hash record, an extra projection, missing receipt evidence, or `CANNOT_ANSWER`. It passes only when equal source hashes retain complete `source_hash_record.base` / `.candidate` groups, the exact authority-diff artifact is empty, and `projection_forced_reason=NONE` has `NO_PARSE_AFFECTING_CHANGE` with no projection section, or when a source-hash difference and/or authority-diff path has the matching canonical non-`NONE` reason and exactly one base and one candidate direct projection with the complete evidence below. + +For any non-`NONE` forced-reason receipt, require the pinned `AtomicCards.json` artifact, exactly one detached clean base projection and one detached clean candidate projection, an isolated `cargo build --profile tool --features cli --bin oracle-gen --bin coverage-report --bin coverage-parse-diff` record for each side, and one base-built comparator record that contains both `--base-sha` and `--head-sha`. Every build/projection/comparison record must include command, environment, worktree, target, expected SHA, ordered `head_before`, `detached_before`, `head_after`, `detached_after`, `clean_before`, `clean_after`, exit, stdout/stderr paths and SHA256s, and produced artifacts; each detached field must prove `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr, and any other result is an operational failure. A source-hash difference or excluded-authority-input change is valid; any replica, quorum, manifest, seal, ledger, provenance-envelope, stale identity, omitted indexed field, noncanonical receipt encoding/order, missing/mismatched required source-hash, authority-diff, or projection record, extra projection, a projection when `projection_forced_reason=NONE`, or missing/mismatched artifact is a blocking finding. + +`Completion Gate` passes only when the receipt's completion-check groups enumerate every scope-/plan-derived mandatory check and every applicable surface-derived check from the executor's existing implementation/fix verification blocks: formatting for implementation changes; the Rust/engine/parser Tilt-first-or-isolated-direct block for Rust/engine/parser paths; the frontend Tilt-first-or-isolated-direct block for frontend paths; and the parser preparatory gate for parser paths. Validate the set against the candidate diff; do not accept a generic plan-only list or preparatory executor success in place of candidate-SHA-bound records. Each entry must give its exact command, successful exit result, detached `CANDIDATE_SHA` identity and clean status at start, and detached `CANDIDATE_SHA` identity and clean status at end, including ordered `detached_before` and `detached_after` proof that `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure. Every completion/parser command must either use `git -C "$COMPLETION_WORKTREE"` or execute in that worktree. For parser paths, validate the candidate range was enumerated NUL-safely with `git -C "$COMPLETION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- crates/engine/src/parser/` and `IFS= read -r -d ''`. Missing or unsuccessful mandatory checks fail the gate. For Markdown-only policy work, scope-path, SHA-identity, and Markdown/diff checks are the complete mandatory set, and Cargo/Tilt are not required. The reviewer validates the receipt before gate evaluation and final acceptance validates the receipt and its artifacts after review; neither action is a self-referential `completion_check` row or receipt artifact. `Maintainer-Simulation Gate` passes only if every changed seam has a concrete row covering production entry, first production branch reached, selected authority / bound value when applicable, binding time, live vs snapshotted semantics, storage, consuming function, invalidation behavior, hostile fixtures, and serialized-surface impact. Use `FAIL` when any row is missing, superficial, or contradicted by the diff, and report the specific gap as a normal finding. Outside this scoped mode, keep silence-as-LGTM behavior. ## Universal Lenses @@ -113,4 +127,4 @@ Severity calibration: a latent bug — one not reachable today because a guard o Findings first. No praise, no diff recap. -Exception: in Engine-Implementer Matrix Mode, the single `Maintainer-Simulation Gate: PASS|FAIL` line precedes findings. +Exception: in Engine-Implementer Checkpoint Mode, the `Review Head`, `Semantic-Impact Gate`, `Completion Gate`, and `Maintainer-Simulation Gate` lines precede findings. From 425c6703cf9e806d33da84415cffcb2c0ec6ca13 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sun, 2 Aug 2026 11:30:35 -0700 Subject: [PATCH 2/2] fix(agents): harden checkpoint review contract --- .../agents/engine-implementation-executor.md | 14 +++++++------- .claude/skills/engine-implementer/SKILL.md | 10 +++++----- .claude/skills/review-impl/SKILL.md | 8 +++++--- .claude/workflows/contribute-card.js | 12 ++++-------- .claude/workflows/deck-contribute.js | 17 +++++++---------- 5 files changed, 28 insertions(+), 33 deletions(-) diff --git a/.claude/agents/engine-implementation-executor.md b/.claude/agents/engine-implementation-executor.md index 07de15e7e4..379d228ad6 100644 --- a/.claude/agents/engine-implementation-executor.md +++ b/.claude/agents/engine-implementation-executor.md @@ -24,7 +24,7 @@ Mode is a hard boundary: - **`implementation/fix`:** First verify and report `IMPLEMENTATION_WORKTREE` as clean with `HEAD == START_SHA` and no staged entries. The initial executor has `START_SHA == BASE_SHA`; every fix executor has the prior reviewed `CANDIDATE_SHA` as `START_SHA`, never a moving head. After surgical edits, report only **PREPARATORY** checks and the required end-of-edit stable-HEAD check (`HEAD == START_SHA`, no executor staging, exact authorized unstaged delta). Do not create a candidate commit, receipt, or completion claim. -- **`measurement-only`:** Make no source edits, formatting edits, or commits. First prove the supplied base/candidate worktrees are detached, clean, and at their expected SHAs. Execute `scripts/engine-source-hash.sh` in each respective worktree and the exact NUL-safe four-path `BASE_SHA..CANDIDATE_SHA` authority diff from the receipt contract, then write the bound records. Skip projection only when source hashes are equal **and** that authority-diff artifact is empty; otherwise record the canonical non-`NONE` `projection_forced_reason` and produce exactly one direct base projection and one direct candidate projection against the same pinned `AtomicCards.json`, followed by one base-built comparator invocation. `CANNOT_ANSWER` applies only to a missing/mismatched required source-hash, authority-diff, or projection record, or an extra projection. Record every action in the canonical receipt. +- **`measurement-only`:** Make no source edits, formatting edits, or commits. Its only successful result is `MEASURED`; every non-success is `CANNOT_ANSWER`. In particular, an identity, detached-state, clean-state, command, source-hash, authority-diff, projection, receipt, or artifact/digest failure must end the run as `CANNOT_ANSWER`; retain any completed records but do not claim parser evidence. First prove the supplied base/candidate worktrees are detached, clean, and at their expected SHAs. Execute `scripts/engine-source-hash.sh` in each respective worktree and the exact NUL-safe four-path `BASE_SHA..CANDIDATE_SHA` authority diff from the receipt contract, then write the bound records. Skip projection only when source hashes are equal **and** that authority-diff artifact is empty; otherwise record the canonical non-`NONE` `projection_forced_reason` and produce exactly one direct base projection and one direct candidate projection against the same pinned `AtomicCards.json`, followed by one base-built comparator invocation. Record every action in the canonical receipt. ### Canonical receipt contract @@ -34,7 +34,7 @@ The un-hashed receipt is UTF-8/LF with exactly one final LF and no CR/NUL. Perce git -C "$IMPLEMENTATION_WORKTREE" -c color.ui=false -c diff.noprefix=false -c core.quotepath=true -c diff.orderFile=/dev/null -c diff.interHunkContext=0 -c diff.suppressBlankEmpty=false diff --no-color --no-ext-diff --no-textconv --no-renames --diff-algorithm=myers --no-indent-heuristic --full-index --binary --src-prefix=a/ --dst-prefix=b/ --unified=3 "$BASE_SHA" "$CANDIDATE_SHA" -- "${SCOPE_PATHS[@]}" ``` -Write the fixed fields, then the mandatory `source_hash_record.base.*` and `source_hash_record.candidate.*` groups, and then all indexed groups in the exact order defined by `/engine-implementer`: no duplicate/missing keys, unordered index, or partial group is valid. Each source-hash group contains, in order, `command`, `expected_sha`, `head_before`, `detached_before`, `clean_before`, `head_after`, `detached_after`, `clean_after`, `exit`, `stdout_path`, `stdout_sha256`, `stderr_path`, `stderr_sha256`, `artifact_path`, `artifact_size`, and `artifact_sha256`. Each group records its exact `scripts/engine-source-hash.sh` command and SHA-bound output artifact. `detached_before` and `detached_after` each prove `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure, not detached-state evidence. The source-hash groups and `projection_authority_diff_*` fields are mandatory even when the hashes are equal. Run exactly `git -C "$IMPLEMENTATION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- Cargo.toml .cargo/config.toml rust-toolchain.toml scripts/engine-source-hash.sh`, capture its NUL-delimited stdout as the recorded path/size/SHA256 artifact, and set `projection_forced_reason` to `NONE`, `SOURCE_HASH_DIFFERENCE`, `EXCLUDED_PROJECTION_AUTHORITY_INPUT_CHANGED`, or `SOURCE_HASH_DIFFERENCE_AND_EXCLUDED_PROJECTION_AUTHORITY_INPUT_CHANGED` according to the two results. Only `NONE` permits no projection section. Record every artifact path, size, and SHA256, but never record or hash the receipt itself; receipt validation is an external reviewer/final-acceptance gate, never a `completion_check` row or receipt artifact. Any malformed receipt or failed identity/digest check is `CANNOT_ANSWER`. +Write the fixed fields, then the mandatory `source_hash_record.base.*` and `source_hash_record.candidate.*` groups, and then all indexed groups in the exact order defined by `/engine-implementer`: no duplicate/missing keys, unordered index, or partial group is valid. Each source-hash group contains, in order, `command`, `expected_sha`, `head_before`, `detached_before`, `clean_before`, `head_after`, `detached_after`, `clean_after`, `exit`, `stdout_path`, `stdout_sha256`, `stderr_path`, `stderr_sha256`, `artifact_path`, `artifact_size`, and `artifact_sha256`. Each group records its exact `scripts/engine-source-hash.sh` command and SHA-bound output artifact. `detached_before` and `detached_after` each prove `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result makes the measurement result `CANNOT_ANSWER`, not detached-state evidence. The source-hash groups and `projection_authority_diff_*` fields are mandatory even when the hashes are equal. Run exactly `git -C "$IMPLEMENTATION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- Cargo.toml .cargo/config.toml rust-toolchain.toml scripts/engine-source-hash.sh`, capture its NUL-delimited stdout as the recorded path/size/SHA256 artifact, and set `projection_forced_reason` to `NONE`, `SOURCE_HASH_DIFFERENCE`, `EXCLUDED_PROJECTION_AUTHORITY_INPUT_CHANGED`, or `SOURCE_HASH_DIFFERENCE_AND_EXCLUDED_PROJECTION_AUTHORITY_INPUT_CHANGED` according to the two results. Only `NONE` permits no projection section. Record every artifact path, size, and SHA256, but never record or hash the receipt itself; receipt validation is an external reviewer/final-acceptance gate, never a `completion_check` row or receipt artifact. Any malformed receipt or failed identity/digest check is `CANNOT_ANSWER`. ## Hard Rules @@ -113,10 +113,10 @@ A "stop and return" is success, not failure. Bandaids that ship are far worse th Run the following only after implementation/fix edits land. Record the commands, starting SHA, ending SHA, and result as `PREPARATORY`; none completes the candidate gate. The orchestrator derives the committed-candidate completion set from these same surface-specific blocks and must rerun the applicable gates at `CANDIDATE_SHA`, retaining the Tilt-first path and isolated direct fallback specified here; it must not treat this preparatory output as their completion result. Existing discriminating-test, maintainer-simulation, selected-authority/provenance, coverage-honesty, and CR-annotation gates below remain single-sourced and mandatory for implementation/fix mode. -After edits land: +After edits land, derive `RUST_PATHS` from the frozen authorized path list (only `*.rs` entries). If it is empty, skip formatting. Otherwise format only those exact paths; never run workspace-wide formatting in `IMPLEMENTATION_WORKTREE`, because it can create an out-of-scope delta: ```bash -(cd "$IMPLEMENTATION_WORKTREE" && cargo fmt --all) +(cd "$IMPLEMENTATION_WORKTREE" && cargo fmt --all -- "${RUST_PATHS[@]}") ``` For Rust / engine / parser work: @@ -175,9 +175,9 @@ For parser changes always run additionally as preparatory checks: Execute `scripts/engine-source-hash.sh "$BASE_SHA"` in the detached base projection worktree and `scripts/engine-source-hash.sh "$CANDIDATE_SHA"` in the detached candidate projection worktree, binding each output to its SHA through the required `source_hash_record.base` / `.candidate` receipt groups, then run and record exactly `git -C "$IMPLEMENTATION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- Cargo.toml .cargo/config.toml rust-toolchain.toml scripts/engine-source-hash.sh`. This NUL-delimited artifact is the excluded projection-authority input check. The source-hash groups and authority-diff record remain mandatory when values are equal; only equal hashes with an empty authority-diff artifact set `projection_forced_reason=NONE`, write `NO_PARSE_AFFECTING_CHANGE`, omit the projection section, and never invoke a parser tool. A source-hash difference and/or any authority-diff path sets its canonical non-`NONE` reason and forces `PROJECTED_PARSE_DIFF`. -For any non-`NONE` `projection_forced_reason`, pin the read-only `AtomicCards.json` input once and use it directly for both detached worktrees. For each side, execute in that side's detached worktree, set its own receipt-owned `CARGO_TARGET_DIR`, and run exactly `cargo build --profile tool --features cli --bin oracle-gen --bin coverage-report --bin coverage-parse-diff`. Run that side's `oracle-gen` and `coverage-report` directly in that worktree to create exactly one projection. Then invoke only the base-built comparator with both `--base-sha "$BASE_SHA"` and `--head-sha "$CANDIDATE_SHA"`. Each build, generator, report, and comparator has a complete indexed receipt group: command/environment, worktree and target, expected SHA, `head_before`, `detached_before`, `head_after`, `detached_after`, `clean_before`, `clean_after`, exit result, stdout/stderr paths and SHA256s, and produced artifacts. `detached_before` and `detached_after` each prove that `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure. The artifact groups enumerate every input/output with path, size, and SHA256. A source-hash difference or excluded-authority-input change is valid; parser evidence is `CANNOT_ANSWER` only for a missing/mismatched required source-hash, authority-diff, or projection record, or an extra projection. +For any non-`NONE` `projection_forced_reason`, pin the read-only `AtomicCards.json` input once and use it directly for both detached worktrees. For each side, execute in that side's detached worktree, set its own receipt-owned `CARGO_TARGET_DIR`, and run exactly `cargo build --profile tool --features cli --bin oracle-gen --bin coverage-report --bin coverage-parse-diff`. Run that side's `oracle-gen` and `coverage-report` directly in that worktree to create exactly one projection. Then invoke only the base-built comparator with both `--base-sha "$BASE_SHA"` and `--head-sha "$CANDIDATE_SHA"`. Each build, generator, report, and comparator has a complete indexed receipt group: command/environment, worktree and target, expected SHA, `head_before`, `detached_before`, `head_after`, `detached_after`, `clean_before`, `clean_after`, exit result, stdout/stderr paths and SHA256s, and produced artifacts. `detached_before` and `detached_after` each prove that `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is `CANNOT_ANSWER`. The artifact groups enumerate every input/output with path, size, and SHA256. A source-hash difference or excluded-authority-input change is valid; any missing/mismatched required source-hash, authority-diff, or projection record, extra projection, or operational failure is `CANNOT_ANSWER`. -The receipt names `BASE_SHA` and `CANDIDATE_SHA`, records raw artifacts and their digests, and sets either `NO_PARSE_AFFECTING_CHANGE`, `PROJECTED_PARSE_DIFF`, or `CANNOT_ANSWER`. Do not claim completion verification: that belongs to the orchestrator after this mode returns. +On `MEASURED`, the receipt names `BASE_SHA` and `CANDIDATE_SHA`, records raw artifacts and their digests, and sets either `NO_PARSE_AFFECTING_CHANGE` or `PROJECTED_PARSE_DIFF`. On `CANNOT_ANSWER`, retain only completed records and do not claim parser evidence or completion verification; completion belongs to the orchestrator after this mode returns. ### Discriminating-test gate @@ -273,6 +273,6 @@ Return a structured report to the orchestrator. This structured report is your r 2. **Source-hash records** — both `engine-source-hash.sh` outputs bound to their SHAs and their equality/difference result. 3. **Parser evidence** — the two SHA-bound source-hash outputs, the exact authority-diff artifact and `projection_forced_reason`, and either `NO_PARSE_AFFECTING_CHANGE` with no projection section, or the pinned `AtomicCards.json`, exact isolated tool-build records, base/candidate direct projections, base-built comparator with both SHA flags, and every indexed receipt/artifact record. 4. **No-edit/no-commit attestation** — confirm source diff and `HEAD` did not change during measurement. -5. **Stop-and-return items, deviations, and risks** — especially any condition that makes the ledger `CANNOT_ANSWER`. +5. **Stop-and-return items, deviations, and risks** — especially any condition that makes measurement `CANNOT_ANSWER`. Do NOT commit. Do NOT push. The orchestrator decides what to stage and when. diff --git a/.claude/skills/engine-implementer/SKILL.md b/.claude/skills/engine-implementer/SKILL.md index ee58d1356e..ba0ba04f30 100644 --- a/.claude/skills/engine-implementer/SKILL.md +++ b/.claude/skills/engine-implementer/SKILL.md @@ -39,11 +39,11 @@ The receipt is UTF-8 with LF line endings and exactly one final LF. It has one ` git -C "$IMPLEMENTATION_WORKTREE" -c color.ui=false -c diff.noprefix=false -c core.quotepath=true -c diff.orderFile=/dev/null -c diff.interHunkContext=0 -c diff.suppressBlankEmpty=false diff --no-color --no-ext-diff --no-textconv --no-renames --diff-algorithm=myers --no-indent-heuristic --full-index --binary --src-prefix=a/ --dst-prefix=b/ --unified=3 "$BASE_SHA" "$CANDIDATE_SHA" -- "${SCOPE_PATHS[@]}" ``` -Next come the mandatory `source_hash_record.base.*` and `source_hash_record.candidate.*` groups, each in this order: `command`, `expected_sha`, `head_before`, `detached_before`, `clean_before`, `head_after`, `detached_after`, `clean_after`, `exit`, `stdout_path`, `stdout_sha256`, `stderr_path`, `stderr_sha256`, `artifact_path`, `artifact_size`, `artifact_sha256`. Each group records the exact `scripts/engine-source-hash.sh` invocation and its SHA-bound output artifact. `detached_before` and `detached_after` prove `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure, not detached-state evidence. These source-hash groups and the `projection_authority_diff_*` record are mandatory even when hashes are equal. `projection_authority_diff_command` is exactly `git -C "$IMPLEMENTATION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- Cargo.toml .cargo/config.toml rust-toolchain.toml scripts/engine-source-hash.sh`; capture its NUL-delimited stdout as the recorded path/size/SHA256 artifact. `projection_forced_reason` is exactly one of `NONE`, `SOURCE_HASH_DIFFERENCE`, `EXCLUDED_PROJECTION_AUTHORITY_INPUT_CHANGED`, or `SOURCE_HASH_DIFFERENCE_AND_EXCLUDED_PROJECTION_AUTHORITY_INPUT_CHANGED`. Next comes no projection section only when `projection_forced_reason=NONE`; otherwise write `projection_step_count` followed by every `projection_step..*` group in strictly ascending zero-based `N`; each group contains, in this order, `side`, `command`, `env`, `worktree`, `target`, `expected_sha`, `head_before`, `detached_before`, `head_after`, `detached_after`, `clean_before`, `clean_after`, `exit`, `stdout_path`, `stdout_sha256`, `stderr_path`, `stderr_sha256`, `produced_artifacts`. The detached fields use the same exact exit-`1` / empty-stdout-and-stderr proof. Then write `completion_check_count` and each strictly ascending `completion_check..*` group with the same record fields, `artifact_count` and each strictly ascending `artifact..path`, `.size`, `.sha256`, then `declared_absent_input_count` and each strictly ascending `declared_absent_input..path`. No indexed member may be omitted; a conditional section is omitted only as stated. Artifact rows cover every source-hash output, authority-diff output, projection output, command stdout/stderr capture, scoped diff, and completion artifact. The receipt itself is never listed or hashed; receipt validation is an external reviewer/final-acceptance gate and never a `completion_check` row or receipt artifact. A malformed, unordered, duplicate, missing, or digest-mismatched receipt is `CANNOT_ANSWER`. +Next come the mandatory `source_hash_record.base.*` and `source_hash_record.candidate.*` groups, each in this order: `command`, `expected_sha`, `head_before`, `detached_before`, `clean_before`, `head_after`, `detached_after`, `clean_after`, `exit`, `stdout_path`, `stdout_sha256`, `stderr_path`, `stderr_sha256`, `artifact_path`, `artifact_size`, `artifact_sha256`. Each group records the exact `scripts/engine-source-hash.sh` invocation and its SHA-bound output artifact. `detached_before` and `detached_after` prove `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is `CANNOT_ANSWER` during measurement, not detached-state evidence. These source-hash groups and the `projection_authority_diff_*` record are mandatory even when hashes are equal. `projection_authority_diff_command` is exactly `git -C "$IMPLEMENTATION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- Cargo.toml .cargo/config.toml rust-toolchain.toml scripts/engine-source-hash.sh`; capture its NUL-delimited stdout as the recorded path/size/SHA256 artifact. `projection_forced_reason` is exactly one of `NONE`, `SOURCE_HASH_DIFFERENCE`, `EXCLUDED_PROJECTION_AUTHORITY_INPUT_CHANGED`, or `SOURCE_HASH_DIFFERENCE_AND_EXCLUDED_PROJECTION_AUTHORITY_INPUT_CHANGED`. Next comes no projection section only when `projection_forced_reason=NONE`; otherwise write `projection_step_count` followed by every `projection_step..*` group in strictly ascending zero-based `N`; each group contains, in this order, `side`, `command`, `env`, `worktree`, `target`, `expected_sha`, `head_before`, `detached_before`, `head_after`, `detached_after`, `clean_before`, `clean_after`, `exit`, `stdout_path`, `stdout_sha256`, `stderr_path`, `stderr_sha256`, `produced_artifacts`. The detached fields use the same exact exit-`1` / empty-stdout-and-stderr proof. Then write `completion_check_count` and each strictly ascending `completion_check..*` group with the same record fields, `artifact_count` and each strictly ascending `artifact..path`, `.size`, `.sha256`, then `declared_absent_input_count` and each strictly ascending `declared_absent_input..path`. No indexed member may be omitted; a conditional section is omitted only as stated. Artifact rows cover every source-hash output, authority-diff output, projection output, command stdout/stderr capture, scoped diff, and completion artifact. The receipt itself is never listed or hashed; receipt validation is an external reviewer/final-acceptance gate and never a `completion_check` row or receipt artifact. A malformed, unordered, duplicate, missing, or digest-mismatched receipt is `CANNOT_ANSWER`. -The parser-impact decision happens only after the checkpoint. Record `scripts/engine-source-hash.sh "$BASE_SHA"` and `scripts/engine-source-hash.sh "$CANDIDATE_SHA"` in the mandatory `source_hash_record.base` / `.candidate` receipt groups, including their SHA-bound output artifacts. Also run and record the exact `projection_authority_diff_command`. Set `projection_forced_reason=NONE` only when the source hashes are equal and that NUL-delimited path artifact is empty. A source-hash difference and/or any path in that artifact forces `parser_evidence=PROJECTED_PARSE_DIFF`, exactly one direct base projection, and exactly one direct candidate projection from the same read-only, pinned `AtomicCards.json`; choose the canonical reason that names both causes when both apply. Only `NONE` permits `parser_evidence=NO_PARSE_AFFECTING_CHANGE` and no projection inputs, projection records, or comparator outputs. Record the pinned `AtomicCards.json` path and SHA256 as artifacts whenever projection is forced. `CANNOT_ANSWER` applies only when a required source-hash, authority-diff, or projection record is missing or mismatched, or an extra projection is present. Do not download, regenerate, copy, sample, or compare replicas. +The parser-impact decision happens only after the checkpoint. Measurement-only has exactly two outcomes: `MEASURED` or `CANNOT_ANSWER`. Any identity, detached/clean-state, command, source-hash, authority-diff, projection, receipt, artifact, or digest failure is `CANNOT_ANSWER`; retain completed records but do not claim parser evidence. On `MEASURED`, record `scripts/engine-source-hash.sh "$BASE_SHA"` and `scripts/engine-source-hash.sh "$CANDIDATE_SHA"` in the mandatory `source_hash_record.base` / `.candidate` receipt groups, including their SHA-bound output artifacts. Also run and record the exact `projection_authority_diff_command`. Set `projection_forced_reason=NONE` only when the source hashes are equal and that NUL-delimited path artifact is empty. A source-hash difference and/or any path in that artifact forces `parser_evidence=PROJECTED_PARSE_DIFF`, exactly one direct base projection, and exactly one direct candidate projection from the same read-only, pinned `AtomicCards.json`; choose the canonical reason that names both causes when both apply. Only `NONE` permits `parser_evidence=NO_PARSE_AFFECTING_CHANGE` and no projection inputs, projection records, or comparator outputs. Record the pinned `AtomicCards.json` path and SHA256 as artifacts whenever projection is forced. Do not download, regenerate, copy, sample, or compare replicas. -For each projection use its detached clean worktree and an isolated target directory. Build exactly with `CARGO_TARGET_DIR= cargo build --profile tool --features cli --bin oracle-gen --bin coverage-report --bin coverage-parse-diff`. Run that side's `oracle-gen` directly against the pinned data root, write its `card-data.json` and `card-names.json` under the projection directory, then run that side's `coverage-report` directly against the projection directory to write `coverage-data.json`. Capture the exact command, environment, worktree identity/clean checks, binary target, exit status, stdout/stderr, and every produced artifact in a projection-step receipt group. Each group's ordered `detached_before` and `detached_after` fields must prove `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure. Invoke the **base-built** comparator directly once against the two projected coverage files: +For each projection use its detached clean worktree and an isolated target directory. Build exactly with `CARGO_TARGET_DIR= cargo build --profile tool --features cli --bin oracle-gen --bin coverage-report --bin coverage-parse-diff`. Run that side's `oracle-gen` directly against the pinned data root, write its `card-data.json` and `card-names.json` under the projection directory, then run that side's `coverage-report` directly against the projection directory to write `coverage-data.json`. Capture the exact command, environment, worktree identity/clean checks, binary target, exit status, stdout/stderr, and every produced artifact in a projection-step receipt group. Each group's ordered `detached_before` and `detached_after` fields must prove `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is `CANNOT_ANSWER`. Invoke the **base-built** comparator directly once against the two projected coverage files: ```bash "$BASE_TARGET/tool/coverage-parse-diff" "$BASE_PROJECTION/coverage-data.json" "$CANDIDATE_PROJECTION/coverage-data.json" \ @@ -106,11 +106,11 @@ Before Step 3, the orchestrator records `IMPLEMENTATION_WORKTREE`'s staged/unsta Spawn a **fresh** `engine-implementation-executor` in mode `measurement-only` with `BASE_SHA`, `CANDIDATE_SHA`, named `IMPLEMENTATION_WORKTREE`, frozen scope paths, the canonical receipt path, a detached clean base worktree, a detached clean candidate worktree, and the read-only pinned data root containing `AtomicCards.json`. This executor makes no source edits and no commits. -The measurement-only executor runs `scripts/engine-source-hash.sh` in the respective detached base/candidate projection worktrees at `BASE_SHA` and `CANDIDATE_SHA`, stores both SHA-bound outputs in the mandatory `source_hash_record.base` / `.candidate` receipt groups, and runs the exact four-path NUL-safe `projection_authority_diff_command` from the receipt contract. Equal hashes set `parser_evidence=NO_PARSE_AFFECTING_CHANGE` and permit no parser projection only when that authority-diff artifact is empty (`projection_forced_reason=NONE`). A source-hash difference or any authority-diff path forces the canonical non-`NONE` reason and requires the exact one base projection, one candidate projection, and base-built comparator defined in the receipt contract. Only a missing/mismatched source-hash, authority-diff, or projection record, or an extra projection, is `CANNOT_ANSWER`. It records every command, environment, base/candidate worktree identity before and after the operation, clean state before and after, output capture, produced artifact, and artifact digest. `./scripts/gen-card-data.sh` and `cargo coverage` are never projection evidence. +The measurement-only executor runs `scripts/engine-source-hash.sh` in the respective detached base/candidate projection worktrees at `BASE_SHA` and `CANDIDATE_SHA`, stores both SHA-bound outputs in the mandatory `source_hash_record.base` / `.candidate` receipt groups, and runs the exact four-path NUL-safe `projection_authority_diff_command` from the receipt contract. It returns `CANNOT_ANSWER` for any identity, detached/clean-state, command, source-hash, authority-diff, projection, receipt, artifact, or digest failure; otherwise it returns `MEASURED`. Equal hashes set `parser_evidence=NO_PARSE_AFFECTING_CHANGE` and permit no parser projection only when that authority-diff artifact is empty (`projection_forced_reason=NONE`). A source-hash difference or any authority-diff path forces the canonical non-`NONE` reason and requires the exact one base projection, one candidate projection, and base-built comparator defined in the receipt contract. It records every command, environment, base/candidate worktree identity before and after the operation, clean state before and after, output capture, produced artifact, and artifact digest. `./scripts/gen-card-data.sh` and `cargo coverage` are never projection evidence. ### Step 5 — Committed-candidate completion verification -Completion verification occurs only after the checkpoint, in the distinct clean detached `COMPLETION_WORKTREE` at `CANDIDATE_SHA`, and records detached identity, `HEAD`, and clean checks at start and end. Every completion or parser command must either use `git -C "$COMPLETION_WORKTREE"` or execute with `COMPLETION_WORKTREE` as its working directory. The candidate parser gate must enumerate its range NUL-safely with `git -C "$COMPLETION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- crates/engine/src/parser/`; any loop reading it uses `IFS= read -r -d ''`. The required set is both scope-/plan-derived checks and every surface-derived gate applicable to the changed paths in the executor's existing implementation/fix verification blocks: formatting for implementation changes, the Rust/engine/parser Tilt-first-or-isolated-direct block for Rust/engine/parser paths, the frontend Tilt-first-or-isolated-direct block for frontend paths, and the parser preparatory gate for parser paths. Reuse those existing blocks by surface; do not duplicate their command recipes here. Use candidate-SHA-bound CI or Tilt evidence only when the evidence itself proves that binding; otherwise use the isolated direct fallback already specified by the applicable block. Never promote the implementation executor's preparatory result to completion evidence. Its receipt completion-check groups must enumerate every required check and, for each, record the exact command, exit result, detached `CANDIDATE_SHA` identity and clean status at start and end, including ordered `detached_before` and `detached_after` proofs that `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure. A missing check, nonzero/unknown exit result, identity mismatch, detached-state failure, or dirty status fails completion. +Completion verification occurs only after the checkpoint, in the distinct clean detached `COMPLETION_WORKTREE` at `CANDIDATE_SHA`, and records detached identity, `HEAD`, and clean checks at start and end. Every completion or parser command must either use `git -C "$COMPLETION_WORKTREE"` or execute with `COMPLETION_WORKTREE` as its working directory. The candidate parser gate must enumerate its range NUL-safely with `git -C "$COMPLETION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- crates/engine/src/parser/`; any loop reading it uses `IFS= read -r -d ''`. The required set is both scope-/plan-derived checks and every surface-derived gate applicable to the changed paths in the executor's existing implementation/fix verification blocks: formatting for implementation changes, the Rust/engine/parser Tilt-first-or-isolated-direct block for Rust/engine/parser paths, the frontend Tilt-first-or-isolated-direct block for frontend paths, and the parser preparatory gate for parser paths. Instantiate those blocks only after substituting `COMPLETION_WORKTREE` for `IMPLEMENTATION_WORKTREE` and `CANDIDATE_SHA` for `START_SHA`; every recorded completion command must show those substitutions and must never name `IMPLEMENTATION_WORKTREE` or `START_SHA`. Use candidate-SHA-bound CI or Tilt evidence only when the evidence itself proves that binding; otherwise use the isolated direct fallback already specified by the applicable block. Never promote the implementation executor's preparatory result to completion evidence. Its receipt completion-check groups must enumerate every required check and, for each, record the exact command, exit result, detached `CANDIDATE_SHA` identity and clean status at start and end, including ordered `detached_before` and `detached_after` proofs that `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure. A missing check, nonzero/unknown exit result, identity mismatch, detached-state failure, or dirty status fails completion. For Markdown-only policy updates, the mandatory completion-check set is limited to scope-path, SHA-identity, and Markdown/diff checks; do not run Cargo or Tilt. Receipt validation is an external reviewer/final-acceptance gate, never a self-referential completion-check record or receipt artifact. diff --git a/.claude/skills/review-impl/SKILL.md b/.claude/skills/review-impl/SKILL.md index af946c512a..2fd1f067fe 100644 --- a/.claude/skills/review-impl/SKILL.md +++ b/.claude/skills/review-impl/SKILL.md @@ -13,7 +13,7 @@ Review for gaps: things that are missing or wrong. Do not spend findings on styl 2. Classify the surface area: engine logic, parser, frontend/UI, multiplayer/transport, AI heuristics, deck/format/feeds, build/CI/release, or docs. 3. Apply only the relevant lenses below. 4. If the scope is a PR, fetch whatever external review comments exist (CodeRabbit, human reviewers) and confirm-or-refute each against the current head with code evidence, folding confirmed findings into your own. **Assume none exist by default** — Gemini Code Assist has been sunset, so no bot is guaranteed to have pre-screened this PR. Your own lenses are the complete review, not a supplement to a bot's; do not under-invest expecting a backstop. Where an external finding *does* exist, silently omitting it — or returning a verdict less severe than an open, unrefuted finding from another reviewer — is itself a defect. Review comments, checks, and uploaded/sticky artifacts count only when their evidence identifies the current PR head SHA; otherwise report the evidence as missing rather than attributing it to the current diff. -5. If the scope is a PR touching engine/parser source, the parse-diff sticky comment (marker ``) is required evidence: fetch its full body and confront the card-level diff against the PR's claimed scope. Unexplained gained/lost/changed cards are findings (unintended parser blast radius). A *Baseline pending* body means the diff is unavailable — flag it so the handler brings the branch current to regenerate it; an absent comment despite changed engine source, or a comment/artifact not bound to the current PR head SHA, means CI evidence is missing for the current head. +5. If the scope is a PR touching engine/parser source, the parse-diff sticky comment (marker ``) is required evidence: fetch its full body and confront the card-level diff against the PR's claimed scope. Unexplained gained/lost/changed cards are findings (unintended parser blast radius). A *Baseline pending* body means the diff is unavailable — flag it so the handler brings the branch current to regenerate it; an absent comment despite changed engine source, or a comment/artifact not bound to the current PR head SHA, means CI evidence is missing for the current head. This PR-head requirement does not apply to Engine-Implementer Checkpoint Mode: before a push, validate its canonical receipt and, when forced, its direct projection/comparator artifacts against `BASE_SHA..CANDIDATE_SHA` instead. 6. Report findings only. Silence means LGTM. When `pr-contribution-handler` explicitly requests the manual quality gate, add `Quality Gate: PASS|FAIL` before findings. PASS requires all three current-PR facts: (1) claimed parse-impact count equals the measured parse-diff count and the normalized card sets are identical, using the full artifact when the sticky comment truncates examples; (2) the change is at an existing authority/right seam and reuses its vocabulary; and (3) a production-pipeline test is demonstrated to fail when the production change is reverted. On PASS, return the applicable existing praise tokens (`right-seam`, `scope-discipline`, `discriminating-runtime-test`, `parameterized-not-proliferated`) for the ordinary review/enqueue event. Never infer quality from Tier or standing and never create a `quality_recommended` event. @@ -29,7 +29,7 @@ Skip checks CI already enforces: Default review output is findings-only. Exception: when `/engine-implementer` invokes this skill against a checkpointed candidate, it supplies `BASE_SHA`, `CANDIDATE_SHA`, the committed reproducible `BASE_SHA..CANDIDATE_SHA` diff, frozen scope paths, the named `START_SHA`/`IMPLEMENTATION_WORKTREE` clean-start/stable-HEAD attestations, the detached `COMPLETION_WORKTREE` start/end identity attestations, the canonical receipt, completion evidence, and the existing maintainer-simulation matrix. Validate the receipt before applying universal lenses. Confirm the first round has `START_SHA == BASE_SHA`, each fix starts from the prior reviewed candidate, the implementation worktree was clean at start and stable at checkpoint, the completion worktree was detached, clean, and exactly at `CANDIDATE_SHA` before and after completion, and the receipt's candidate/head values are exactly `CANDIDATE_SHA`. -The receipt must be the un-hashed UTF-8/LF `engine-implementer-receipt-v1` document with one final LF, no CR/NUL, percent-encoded values (only `[A-Za-z0-9._~-]` raw), the fixed fields in their mandated order, the mandatory `source_hash_record.base.*` and `.candidate.*` groups, and complete ascending indexed groups. Verify the frozen scope representation is duplicate-free NUL-delimited bytes from `printf '%s\0' "${SCOPE_PATHS[@]}" | LC_ALL=C sort -z`, and verify its recorded path and SHA256. Verify `scoped_diff_command` is exactly `git -C "$IMPLEMENTATION_WORKTREE" -c color.ui=false -c diff.noprefix=false -c core.quotepath=true -c diff.orderFile=/dev/null -c diff.interHunkContext=0 -c diff.suppressBlankEmpty=false diff --no-color --no-ext-diff --no-textconv --no-renames --diff-algorithm=myers --no-indent-heuristic --full-index --binary --src-prefix=a/ --dst-prefix=b/ --unified=3 "$BASE_SHA" "$CANDIDATE_SHA" -- "${SCOPE_PATHS[@]}"`, then reproduce and hash those scoped-diff bytes. Verify `projection_authority_diff_command` is exactly `git -C "$IMPLEMENTATION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- Cargo.toml .cargo/config.toml rust-toolchain.toml scripts/engine-source-hash.sh`, reproduce and hash its NUL-delimited output, and require its recorded path/size/SHA256. Each source-hash group must contain, in order, `command`, `expected_sha`, `head_before`, `detached_before`, `clean_before`, `head_after`, `detached_after`, `clean_after`, `exit`, `stdout_path`, `stdout_sha256`, `stderr_path`, `stderr_sha256`, `artifact_path`, `artifact_size`, and `artifact_sha256`; it must bind the exact `scripts/engine-source-hash.sh` command and output artifact to its expected SHA. Verify every source-hash, projection, and completion group contains ordered `detached_before` and `detached_after` fields, each proving `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure, not detached-state evidence. Verify every recorded artifact path, size, and SHA256, but do not expect a hash for the receipt itself. Its fixed identity fields, scoped-diff command/path/size/digest, source hashes, authority-diff command/path/size/digest, canonical `projection_forced_reason`, mandatory source-hash records, parser evidence, completion records, artifacts, and declared absent inputs are mandatory; the projection section is omitted only when source hashes are equal, the authority-diff output is empty, and `projection_forced_reason=NONE`. A missing, duplicate, unordered, malformed, or digest-mismatched field is a blocking finding. +The receipt must be the un-hashed UTF-8/LF `engine-implementer-receipt-v1` document with one final LF, no CR/NUL, percent-encoded values (only `[A-Za-z0-9._~-]` raw), the fixed fields in their mandated order, the mandatory `source_hash_record.base.*` and `.candidate.*` groups, and complete ascending indexed groups. Verify the frozen scope representation is duplicate-free NUL-delimited bytes from `printf '%s\0' "${SCOPE_PATHS[@]}" | LC_ALL=C sort -z`, and verify its recorded path and SHA256. Verify `scoped_diff_command` is exactly `git -C "$IMPLEMENTATION_WORKTREE" -c color.ui=false -c diff.noprefix=false -c core.quotepath=true -c diff.orderFile=/dev/null -c diff.interHunkContext=0 -c diff.suppressBlankEmpty=false diff --no-color --no-ext-diff --no-textconv --no-renames --diff-algorithm=myers --no-indent-heuristic --full-index --binary --src-prefix=a/ --dst-prefix=b/ --unified=3 "$BASE_SHA" "$CANDIDATE_SHA" -- "${SCOPE_PATHS[@]}"`, then reproduce and hash those scoped-diff bytes. Verify `projection_authority_diff_command` is exactly `git -C "$IMPLEMENTATION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- Cargo.toml .cargo/config.toml rust-toolchain.toml scripts/engine-source-hash.sh`, reproduce and hash its NUL-delimited output, and require its recorded path/size/SHA256. Each source-hash group must contain, in order, `command`, `expected_sha`, `head_before`, `detached_before`, `clean_before`, `head_after`, `detached_after`, `clean_after`, `exit`, `stdout_path`, `stdout_sha256`, `stderr_path`, `stderr_sha256`, `artifact_path`, `artifact_size`, and `artifact_sha256`; it must bind the exact `scripts/engine-source-hash.sh` command and output artifact to its expected SHA. Verify every source-hash, projection, and completion group contains ordered `detached_before` and `detached_after` fields, each proving `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; a measurement-group failure is `CANNOT_ANSWER`, and a completion-group failure is a gate failure. Verify every recorded artifact path, size, and SHA256, but do not expect a hash for the receipt itself. Its fixed identity fields, scoped-diff command/path/size/digest, source hashes, authority-diff command/path/size/digest, canonical `projection_forced_reason`, mandatory source-hash records, parser evidence, completion records, artifacts, and declared absent inputs are mandatory; the projection section is omitted only when source hashes are equal, the authority-diff output is empty, and `projection_forced_reason=NONE`. A missing, duplicate, unordered, malformed, or digest-mismatched field is a blocking finding. In this mode, emit these lines before findings: @@ -41,11 +41,13 @@ Completion Gate: PASS|FAIL Maintainer-Simulation Gate: PASS|FAIL ``` +After the headers and findings, emit exactly one JSON object on its own line: `{"clean":,"findings":[,...]}`. Set `clean=true` only when all three gates pass and there are no findings; otherwise set it to `false` and include every failed gate and blocking finding in `findings`. + `Review Head` must be the supplied `CANDIDATE_SHA`, the reviewed diff must reproduce from exactly `BASE_SHA..CANDIDATE_SHA`, and `Receipt SHA256` must be the SHA256 of the validated receipt bytes emitted externally in this review result; otherwise report a blocking finding. The receipt never contains or hashes itself. `Semantic-Impact Gate` passes only when the receipt validates and either (a) equal SHA-bound source hashes, an empty authority-diff output, and `projection_forced_reason=NONE` set `NO_PARSE_AFFECTING_CHANGE` with no projection section, or (b) a non-`NONE` canonical forced reason sets `PROJECTED_PARSE_DIFF` and contains all direct-projection records. `Semantic-Impact Gate` rejects missing/mismatched source-hash, authority-diff, or projection evidence, including either mandatory source-hash record, an extra projection, missing receipt evidence, or `CANNOT_ANSWER`. It passes only when equal source hashes retain complete `source_hash_record.base` / `.candidate` groups, the exact authority-diff artifact is empty, and `projection_forced_reason=NONE` has `NO_PARSE_AFFECTING_CHANGE` with no projection section, or when a source-hash difference and/or authority-diff path has the matching canonical non-`NONE` reason and exactly one base and one candidate direct projection with the complete evidence below. -For any non-`NONE` forced-reason receipt, require the pinned `AtomicCards.json` artifact, exactly one detached clean base projection and one detached clean candidate projection, an isolated `cargo build --profile tool --features cli --bin oracle-gen --bin coverage-report --bin coverage-parse-diff` record for each side, and one base-built comparator record that contains both `--base-sha` and `--head-sha`. Every build/projection/comparison record must include command, environment, worktree, target, expected SHA, ordered `head_before`, `detached_before`, `head_after`, `detached_after`, `clean_before`, `clean_after`, exit, stdout/stderr paths and SHA256s, and produced artifacts; each detached field must prove `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr, and any other result is an operational failure. A source-hash difference or excluded-authority-input change is valid; any replica, quorum, manifest, seal, ledger, provenance-envelope, stale identity, omitted indexed field, noncanonical receipt encoding/order, missing/mismatched required source-hash, authority-diff, or projection record, extra projection, a projection when `projection_forced_reason=NONE`, or missing/mismatched artifact is a blocking finding. +For any non-`NONE` forced-reason receipt, require the pinned `AtomicCards.json` artifact, exactly one detached clean base projection and one detached clean candidate projection, an isolated `cargo build --profile tool --features cli --bin oracle-gen --bin coverage-report --bin coverage-parse-diff` record for each side, and one base-built comparator record that contains both `--base-sha` and `--head-sha`. Every build/projection/comparison record must include command, environment, worktree, target, expected SHA, ordered `head_before`, `detached_before`, `head_after`, `detached_after`, `clean_before`, `clean_after`, exit, stdout/stderr paths and SHA256s, and produced artifacts; each detached field must prove `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr, and any other result is `CANNOT_ANSWER`. A source-hash difference or excluded-authority-input change is valid; any replica, quorum, manifest, seal, ledger, provenance-envelope, stale identity, omitted indexed field, noncanonical receipt encoding/order, missing/mismatched required source-hash, authority-diff, or projection record, extra projection, a projection when `projection_forced_reason=NONE`, or missing/mismatched artifact is a blocking finding. `Completion Gate` passes only when the receipt's completion-check groups enumerate every scope-/plan-derived mandatory check and every applicable surface-derived check from the executor's existing implementation/fix verification blocks: formatting for implementation changes; the Rust/engine/parser Tilt-first-or-isolated-direct block for Rust/engine/parser paths; the frontend Tilt-first-or-isolated-direct block for frontend paths; and the parser preparatory gate for parser paths. Validate the set against the candidate diff; do not accept a generic plan-only list or preparatory executor success in place of candidate-SHA-bound records. Each entry must give its exact command, successful exit result, detached `CANDIDATE_SHA` identity and clean status at start, and detached `CANDIDATE_SHA` identity and clean status at end, including ordered `detached_before` and `detached_after` proof that `git symbolic-ref -q HEAD` exited exactly `1` with empty stdout and stderr; any other result is an operational failure. Every completion/parser command must either use `git -C "$COMPLETION_WORKTREE"` or execute in that worktree. For parser paths, validate the candidate range was enumerated NUL-safely with `git -C "$COMPLETION_WORKTREE" diff --name-only -z "$BASE_SHA" "$CANDIDATE_SHA" -- crates/engine/src/parser/` and `IFS= read -r -d ''`. Missing or unsuccessful mandatory checks fail the gate. For Markdown-only policy work, scope-path, SHA-identity, and Markdown/diff checks are the complete mandatory set, and Cargo/Tilt are not required. The reviewer validates the receipt before gate evaluation and final acceptance validates the receipt and its artifacts after review; neither action is a self-referential `completion_check` row or receipt artifact. `Maintainer-Simulation Gate` passes only if every changed seam has a concrete row covering production entry, first production branch reached, selected authority / bound value when applicable, binding time, live vs snapshotted semantics, storage, consuming function, invalidation behavior, hostile fixtures, and serialized-surface impact. Use `FAIL` when any row is missing, superficial, or contradicted by the diff, and report the specific gap as a normal finding. Outside this scoped mode, keep silence-as-LGTM behavior. diff --git a/.claude/workflows/contribute-card.js b/.claude/workflows/contribute-card.js index b0b4afe8dc..3083149d73 100644 --- a/.claude/workflows/contribute-card.js +++ b/.claude/workflows/contribute-card.js @@ -19,10 +19,8 @@ const TIER = 'Frontier' // Published coverage endpoint (AI-CONTRIBUTOR.md §3). const COVERAGE_URL = 'https://data.phase-rs.dev/staging/coverage-data.json' -// This per-card pipeline embodies the /engine-implementer contract: /engine-planner -> -// /review-engine-plan (looped) -> engine-implementation-executor -> /review-impl (looped until -// clean; reviewer confirms the card actually parses correctly). The review caps are runaway-loop -// safeguards, not a "two rounds and ship" gate (which engine-implementer forbids). +// This is a legacy uncommitted-worktree card workflow: it keeps its own planning and review loops. +// It is not /engine-implementer Checkpoint Mode, whose executor requires immutable-candidate inputs. const MAX_PLAN_REVIEW_ROUNDS = 8 const MAX_IMPL_REVIEW_ROUNDS = 8 const MAX_CROSSCHECK_ROUNDS = 2 @@ -336,17 +334,16 @@ async function planCard(card) { } async function implementCard(card, plan) { - // Step 3 of the /engine-implementer contract: surgical edits via the executor agent. + // This legacy workflow intentionally uses a general implementation agent, not the checkpoint executor. return await agent(implementPrompt(card, plan), { label: `implement:${card}`, phase: 'Implement', schema: IMPL_SCHEMA, - agentType: 'engine-implementation-executor', }) } async function reviewImpl(card) { - // /review-impl looped until clean (engine-implementer mandate); fixes via a fresh executor agent. + // This workflow reviews its uncommitted diff and fixes it with a fresh general implementation agent. for (let round = 1; round <= MAX_IMPL_REVIEW_ROUNDS; round++) { const review = await agent(reviewImplPrompt(card), { label: `review-impl:${card}#${round}`, @@ -357,7 +354,6 @@ async function reviewImpl(card) { await agent(fixImplPrompt(card, review.findings), { label: `fix-impl:${card}#${round}`, phase: 'Review', - agentType: 'engine-implementation-executor', }) } return false diff --git a/.claude/workflows/deck-contribute.js b/.claude/workflows/deck-contribute.js index dec2af8561..c2dd3a0d90 100644 --- a/.claude/workflows/deck-contribute.js +++ b/.claude/workflows/deck-contribute.js @@ -19,12 +19,9 @@ export const meta = { const CONTRIBUTE_CARD = '.claude/workflows/contribute-card.js' const TIER = 'Frontier' -// The mechanic-cluster pipeline below EMBODIES THE /engine-implementer SKILL CONTRACT -// (maintainer feedback on PR #3163): /engine-planner -> /review-engine-plan (looped until clean) -// -> engine-implementation-executor agent -> /review-impl (looped until clean; the reviewer must -// confirm the cards actually parse correctly, not just that the diff looks clean). "Two rounds and -// ship" is NOT acceptable, so the review caps below are runaway-loop safeguards (hitting one marks -// the unit `partial` and is surfaced, never silently shipped), not a ship-after-N gate. +// This mechanic-cluster workflow uses an uncommitted-worktree loop distinct from +// /engine-implementer Checkpoint Mode; it must not dispatch that checkpoint-only executor. +// The review caps are runaway-loop safeguards (hitting one marks the unit `partial` and is surfaced). const MAX_PLAN_REVIEW_ROUNDS = 8 const MAX_IMPL_REVIEW_ROUNDS = 8 const MAX_CROSSCHECK_ROUNDS = 2 @@ -494,15 +491,15 @@ async function implementMechanicCluster(mechanic, cards, heterogeneous) { plan = await agent(replanPrompt(label, plan, review.findings), { label: `replan:${mechanic}#${r}`, phase: 'Implement' }) } - // Step 3: engine-implementation-executor agent performs the surgical edits. - const impl = await agent(clusterImplementPrompt(mechanic, cards, plan, heterogeneous), { label: `implement:${mechanic}`, phase: 'Implement', schema: IMPL_SCHEMA, agentType: 'engine-implementation-executor' }) + // This legacy workflow intentionally uses a general implementation agent. + const impl = await agent(clusterImplementPrompt(mechanic, cards, plan, heterogeneous), { label: `implement:${mechanic}`, phase: 'Implement', schema: IMPL_SCHEMA }) - // Step 5: /review-impl, looped until clean; fixes applied by a fresh engine-implementation-executor. + // Step 5: /review-impl, looped until clean; fixes use a fresh general implementation agent. let implReviewClean = false for (let r = 1; r <= MAX_IMPL_REVIEW_ROUNDS && !implReviewClean; r++) { const review = await agent(reviewImplPrompt(label), { label: `review-impl:${mechanic}#${r}`, phase: 'Implement', schema: REVIEW_SCHEMA }) if (review.clean) { implReviewClean = true; break } - await agent(fixImplPrompt(label, review.findings), { label: `fix-impl:${mechanic}#${r}`, phase: 'Implement', agentType: 'engine-implementation-executor' }) + await agent(fixImplPrompt(label, review.findings), { label: `fix-impl:${mechanic}#${r}`, phase: 'Implement' }) } let cross = await agent(crossCheckPrompt(label), { label: `crosscheck:${mechanic}`, phase: 'Implement', schema: CROSSCHECK_SCHEMA })