feat(cli): --plan replay execution wires generateReview (#878 A2-3-impl)#935
Merged
Conversation
Final functional slice of #878. runReviewExecReplay now executes the source plan's selectedSkills against the current diff instead of echoing findings: []. Per docs/development/a2-3-replay-execution-design.md: - Replay does NOT re-plan: selectedSkills come verbatim from the source plan. - Diff is resolved from the current working tree / --artifact, NOT snapshotted in the plan (intentional — avoids artifact bloat). - Carry-over context (fileTypes / relatedADRs / reviewMode / riskAssessment) is read from the source plan's debug.execution.snapshot (#933 A2-3-runners). When the source predates the snapshot, generateReview uses engine defaults (graceful degradation, not silent skip). - debug.execution.replaySnapshotUsed records whether the carry-over was present. Behavior matrix: - exec --plan <p> --artifact diff=... → executes, findings populated - exec --plan <p> (no diff) → echo contract (findings: []) - exec --plan <p> --dry-run → echo only, no execution #927's todo-marked failing test is now a passing integration test (3 cases: execution reach, no-diff echo fallback, dry-run echo). Action dist rebuilt (Node 22) — cli.mjs transitively bundled. drift detection (debug.replay.drift) is deferred to a follow-up per Codex slice judgment; this PR delivers execution + carry-over.
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
River Reviewer
選択されたスキル (1)
スキップされたスキル (87)
優先度サマリー
スコア (参考値)結果(スコア): 100/100 内訳:
指摘No findings. |
Contributor
PlanGate Review
PlanGate review decision: pass
ポリシー: critical=fail / major=fail-if-required (warn) / minor=comment-only / info=skipped — spec: |
This was referenced May 28, 2026
s977043
added a commit
that referenced
this pull request
May 30, 2026
README.md: - Mark npm publish workflow as implemented (#942, not roadmap) - Name the three publishable packages (@river-reviewer/*) - Note NPM_TOKEN activation step; npx river try is still separate - Update Epic 1 roadmap row: B2 implemented, token setup pending pages/reference/cli-review-exec-spec.md/.en.md: - Add addendum: river review exec --plan <file> is now implemented via PR #935 (2026-05-29), produces Review Artifact via generateReview - Note: exec without --plan still exits 3 Co-authored-by: Claude Sonnet 4.6 <[email protected]>
3 tasks
s977043
added a commit
that referenced
this pull request
May 30, 2026
…ills (#950) docs/review/troubleshooting.md: - Update exec --plan note: PR #935 (v0.68.0) now invokes generateReview, findings are real (not always []) docs/development/execution-context-contract.md: - Add PR #935 addenda to replay contract section (Stage 3 now invoked) pages/reference/plangate-cli-roadmap.md: - Mark public entrypoint row as implemented (v0.68.0+); review plan/exec/verify are wired in src/cli.mjs runners/github-action/action.yml: - Fix output_format validation regex: add yaml to allowed values (documented but previously rejected) skills: promote design-token-enforcement and design-system-component-reuse - Add golden outputs for all 4 fixtures (rubric verified) - Flip recommended: false → true in registry.yaml Co-authored-by: Claude Sonnet 4.6 <[email protected]>
s977043
added a commit
that referenced
this pull request
Jun 1, 2026
Version pins: - README.md/en: @v0.42.0 → @v0.70.0 in Action examples - pages/tutorials: @v0.68.0 → @v0.70.0 version references - pages/guides: @v0.68.0 → @v0.70.0, actions/checkout@v6 → @v4 Content accuracy: - README.md/en: mark river review exec --plan as shipped (v0.68.0/#935) - README.md/en: update Epic 1/2 roadmap rows to reflect current state - README.en.md: align npm distribution note with README.md (PR #942 shipped) - pages/explanation/skills.md: remove unimplemented skill: prefix example - pages/explanation/river-architecture.en.md: add missing Mermaid diagrams (component, GitHub Actions sequence, local CLI sequence) matching JA version - pages/reference/skill-metadata.md: remove duplicate Phase type declaration Runbook: - docs/runbook/community-skill-eval.md: document agent-based eval path (nightly-eval.yml / skill-eval.yml / evaluate-all.mjs) alongside promptfoo Co-authored-by: Claude Sonnet 4.6 <[email protected]>
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
Final functional slice of #878.
runReviewExecReplaynow executes the source plan'sselectedSkillsagainst the current diff instead of echoingfindings: []. This lifts the last silent-skip on the exec path.The #927 todo-marked failing test is now a passing integration test.
Behavior
exec --plan <p> --artifact diff=...exec --plan <p>(no diff resolved)findings: [])exec --plan <p> --dry-runContract (per design doc #910)
selectedSkillscome verbatim from the source plan.resolveAllArtifacts/--artifact, NOT snapshotted in the plan (avoids artifact bloat — intentional design decision).fileTypes/relatedADRs/reviewMode/riskAssessmentread from the source plan'sdebug.execution.snapshot(the field feat(runners): buildExecutionPlan emits snapshot carry-over (#878 A2-3-runners) #933 A2-3-runners made buildExecutionPlan emit). When the source predates the snapshot, generateReview falls back to engine defaults — graceful degradation, not silent skip.debug.execution.replaySnapshotUsedrecords whether the carry-over was present.Files
src/lib/review-plan.mjs—runReviewExecReplaygains execution params (executeReview,cwd,cliArtifacts,artifactsDir, impls) and the generateReview wiring.src/cli.mjs— replay dispatch passesexecuteReview: !parsed.dryRun+ cwd + artifacts.tests/cli-review-exec-replay.test.mjs— 3 passing cases (was 1 todo).runners/github-action/dist/**— rebuilt on Node 22.Test plan
node --test tests/cli-review-exec-replay.test.mjs→ 3/3 passnpm test→ 1113/1113 pass, 0 todo (was 1106 + 1 todo)npm run build:actionNode 22node scripts/fix-dashes.mjs --checkcleanSelf-review (per AGENTS.md from #904)
debug.execution.snapshot→sourceSnapshotis null, generateReview uses defaults;replaySnapshotUsed: falserecords it. Verified by the "no-diff echo" + manual older-plan path.parseUnifiedDiffreturns empty files, generateReview runs with no changed files (no crash).--dry-runwith a diff present → must NOT execute. Covered by the dry-run test case.debug.replay.drift) is deferred to a follow-up slice per Codex; this PR scopes to execution + carry-over only.skillsExecuted,replaySnapshotUsed), not finding volume, so CI without API keys is deterministic.#878 epic status after this merge
Remaining optional follow-up:
debug.replay.driftsummary (separate issue if wanted). #878 can close on this merge; drift is a nice-to-have, not a silent-skip.