chore(agents): bind pipeline evidence to candidate commits - #6931
Merged
matthewevans merged 2 commits intoAug 2, 2026
Conversation
matthewevans
enabled auto-merge
August 2, 2026 18:09
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR updates the implementation executor, implementation pipeline, review skill, and legacy workflows. It adds immutable SHA and worktree controls, canonical receipts, measurement-only execution, detached completion checks, parser evidence, Checkpoint Mode review gates, and general-agent dispatch for legacy workflows. ChangesImplementation and review workflow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Orchestrator
participant Executor
participant Measurement
participant Completion
participant Review
Orchestrator->>Executor: run implementation or fix mode
Executor-->>Orchestrator: return preparatory evidence
Orchestrator->>Orchestrator: create and validate candidate commit
Orchestrator->>Measurement: measure the committed candidate
Measurement-->>Orchestrator: return hashes, projections, and artifacts
Orchestrator->>Completion: verify detached candidate worktree
Completion-->>Orchestrator: return completion checks
Orchestrator->>Review: validate current-head-bound evidence
Review-->>Orchestrator: return checkpoint gate statuses
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.5).claude/workflows/contribute-card.jsFile contains syntax errors that prevent linting: Line 432: Illegal return statement outside of a function .claude/workflows/deck-contribute.jsFile contains syntax errors that prevent linting: Line 530: Illegal return statement outside of a function; Line 535: Illegal return statement outside of a function; Line 600: Illegal return statement outside of a function; Line 627: Illegal return statement outside of a function Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/agents/engine-implementation-executor.md:
- Around line 119-120: Update the formatting step around cargo fmt --all so it
cannot modify or validate unrelated Rust files outside the frozen authorized
scope. Format only the frozen Rust paths, or run workspace formatting in a
disposable worktree and copy back only authorized changes before the exact-delta
checkpoint.
- Line 27: Define a single failure-state contract for measurement-only runs
across the instructions and review gates: either introduce an explicit
operational-failure state or consistently map every failed identity,
detached/clean worktree, command, and related operational check to
CANNOT_ANSWER. Update the receipt requirements and the sections around the
measurement-only rules, worktree validation, and review gates so they use the
same classification without leaving failed commands ambiguous.
In @.claude/skills/engine-implementer/SKILL.md:
- Around line 111-115: Update the completion-verification instructions around
“Committed-candidate completion verification” so reused executor verification
blocks are parameterized by the target worktree and SHA, with every completion
command explicitly substituted to use COMPLETION_WORKTREE and CANDIDATE_SHA.
Require recorded evidence to demonstrate this substitution, preventing
IMPLEMENTATION_WORKTREE from being validated accidentally.
- Around line 95-109: Update the executor orchestration in contribute-card.js
and deck-contribute.js to use the immutable-candidate contract: pass
implementation/fix mode, BASE_SHA, START_SHA, frozen scope and path-list digest,
named implementation and completion worktrees, receipt metadata, and prior
findings. Replace uncommitted-diff review with the orchestrator-owned
explicit-path checkpoint and candidate commit, then invoke measurement-only
review against detached BASE_SHA/CANDIDATE_SHA worktrees and verify
COMPLETION_WORKTREE before opening a PR; preserve the existing review-loop
behavior for findings and reruns.
In @.claude/skills/review-impl/SKILL.md:
- Around line 15-16: Update the engine/parser evidence requirement in Checkpoint
Mode so local reviews of BASE_SHA..CANDIDATE_SHA accept the canonical
receipt/projection artifact instead of requiring a PR-head parse-diff sticky
comment. Preserve the current PR-head SHA validation for actual PR reviews, or
add an explicit push/update step before enforcing it.
- Around line 34-42: Update Checkpoint Mode in the review workflow to emit a
machine-readable structured result containing clean: boolean and findings:
string[]. Set clean to true only when Semantic-Impact, Completion, and
Maintainer-Simulation gates all pass with no findings; otherwise set clean to
false and add each failed gate to findings, while preserving the existing
checkpoint header lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 341853c9-6e99-4cf9-846e-fad032a12ea1
📒 Files selected for processing (3)
.claude/agents/engine-implementation-executor.md.claude/skills/engine-implementer/SKILL.md.claude/skills/review-impl/SKILL.md
Comment on lines
+95
to
+109
| **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 <resource> --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 <n>` 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. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'engine-implementation-executor|clusterImplementPrompt|fixImplPrompt|reviewImplPrompt' .claudeRepository: phase-rs/phase
Length of output: 29401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- deck-contribute.js agent wrapper and pipeline ---'
sed -n '1,120p' .claude/workflows/deck-contribute.js
sed -n '430,560p' .claude/workflows/deck-contribute.js
printf '%s\n' '--- contribute-card.js agent wrapper and pipeline ---'
sed -n '1,90p' .claude/workflows/contribute-card.js
sed -n '300,375p' .claude/workflows/contribute-card.js
printf '%s\n' '--- all executor callers and worktree/completion identifiers ---'
rg -n -C 4 'agentType:\s*['\"'\"']engine-implementation-executor|engine-implementation-executor|COMPLETION_WORKTREE|IMPLEMENTATION_WORKTREE|CANDIDATE_SHA|START_SHA' .claudeRepository: phase-rs/phase
Length of output: 19481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- executor caller locations ---'
rg -n -C 5 "agentType: ['\"]engine-implementation-executor|engine-implementation-executor" .claude
printf '%s\n' '--- identity and worktree fields in workflows ---'
rg -n -C 3 'COMPLETION_WORKTREE|IMPLEMENTATION_WORKTREE|START_SHA|CANDIDATE_SHA|BASE_SHA|receipt|checkpoint|measurement-only' .claude/workflows .claude/agents
printf '%s\n' '--- implementation prompt definitions ---'
rg -n -C 12 'function (implementPrompt|clusterImplementPrompt|fixImplPrompt)|const (implementPrompt|clusterImplementPrompt|fixImplPrompt)' .claude/workflows
printf '%s\n' '--- agent helper definition ---'
rg -n -C 12 'async function agent|function agent|export .*agent|const agent' .claude/workflowsRepository: phase-rs/phase
Length of output: 44237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workflow prompt and branch sections ---'
sed -n '145,220p' .claude/workflows/contribute-card.js
sed -n '330,370p' .claude/workflows/contribute-card.js
sed -n '320,430p' .claude/workflows/deck-contribute.js
printf '%s\n' '--- agent runtime references ---'
rg -n -C 5 'agent\s*\(|function agent|const agent|agent\s*=' .claude --glob '*.js' --glob '*.mjs' --glob '*.ts'
printf '%s\n' '--- deterministic caller contract scan ---'
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path('.claude/workflows/contribute-card.js'),
Path('.claude/workflows/deck-contribute.js'),
]
required = [
'mode',
'BASE_SHA',
'START_SHA',
'CANDIDATE_SHA',
'IMPLEMENTATION_WORKTREE',
'COMPLETION_WORKTREE',
'receipt',
]
for path in files:
text = path.read_text()
calls = list(re.finditer(r"agentType:\s*['\"]engine-implementation-executor['\"]", text))
print(f'{path}: {len(calls)} executor call(s)')
for i, m in enumerate(calls, 1):
start = max(0, text.rfind('agent(', 0, m.start()))
end = text.find('\n })', m.end())
block = text[start:end if end != -1 else m.end()]
print(f' call {i}: line {text.count(chr(10), 0, m.start()) + 1}')
print(' ' + ', '.join(k for k in required if k in block) if any(k in block for k in required) else ' no required identity/receipt fields in call block')
print(' pipeline tokens:', {k: (k in text) for k in required})
PYRepository: phase-rs/phase
Length of output: 28577
Migrate all executor callers to the immutable-candidate contract. .claude/workflows/contribute-card.js and .claude/workflows/deck-contribute.js pass no mode, SHA identities, frozen scope, named worktrees, or receipt metadata to implementation or fix executors. Their review loops still inspect uncommitted working-tree diffs and omit checkpoint, measurement-only, and COMPLETION_WORKTREE verification. Add the orchestrator-owned checkpoint and SHA-bound candidate review before opening a PR.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/engine-implementer/SKILL.md around lines 95 - 109, Update the
executor orchestration in contribute-card.js and deck-contribute.js to use the
immutable-candidate contract: pass implementation/fix mode, BASE_SHA, START_SHA,
frozen scope and path-list digest, named implementation and completion
worktrees, receipt metadata, and prior findings. Replace uncommitted-diff review
with the orchestrator-owned explicit-path checkpoint and candidate commit, then
invoke measurement-only review against detached BASE_SHA/CANDIDATE_SHA worktrees
and verify COMPLETION_WORKTREE before opening a PR; preserve the existing
review-loop behavior for findings and reruns.
matthewevans
deleted the
ship/bind-pipeline-evidence-to-candidate-commits
branch
August 2, 2026 18:35
Parse changes introduced by this PRBaseline pending — no synthetic merge parent is available for PR head |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit