fix(cli): propagate availableDependencies to plan layer (#802 Phase 3 A2-fix-2)#869
Merged
Merged
Conversation
… A2-fix-2) Same silent-skip pattern as A2-fix-1: runReviewPlan never forwarded `availableDependencies` to buildExecutionPlan, so any skill that declared a dependency (e.g. `code_search`, `test_runner`) was always selectable regardless of whether the dependency was actually wired in the runtime. What changed - Move resolveAvailableDependencies + dependencyStubs to src/lib/utils.mjs so the helper is now shared between local-runner and review-plan (mirrors the A2-fix-1 split of resolveAvailableContexts). - runReviewPlan accepts a new availableDependencies option and forwards the resolved value (or null = disabled sentinel) to buildExecutionPlan. - CLI passes parsed.availableDependencies through so `--dependency` and RIVER_AVAILABLE_DEPENDENCIES / RIVER_DEPENDENCY_STUBS=1 work on the exec path. - 4 new unit tests cover null default, explicit arg, env var, and stubs. - runners/github-action/dist rebuilt with Node 22.22.2. Out of scope (Codex silent-skip list, follow-ups remaining): - fileTypes / relatedADRs / reviewMode propagation - --plan replay context-snapshot drift Follows: #864 (A2-1) and #865 (A2-fix-1). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
River Reviewer
選択されたスキル (1)
スキップされたスキル (82)
優先度サマリー
スコア (参考値)結果(スコア): 100/100 内訳:
指摘No findings. |
Contributor
PlanGate Review
PlanGate review decision: pass
ポリシー: critical=fail / major=fail-if-required (warn) / minor=comment-only / info=skipped — spec: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
このプルリクエストは、runReviewPlan 関数および CLI に availableDependencies オプションを導入し、依存関係に基づくスキルのスキップ機能を実装するものです。また、resolveAvailableDependencies ヘルパーを src/lib/utils.mjs に集約して共有化し、テストコードを拡充しました。レビューでは、依存関係リストが空（[]）の場合に環境変数へフォールバックされる現在のロジックを改善し、明示的な空リストを正しく処理するための修正が提案されています。
Comment on lines
+85
to
+87
| if (inputDependencies?.length) return [...new Set(inputDependencies)]; | ||
| if (envDeps.length) return [...new Set(envDeps)]; | ||
| if (stubEnabled) return [...dependencyStubs]; |
Contributor
There was a problem hiding this comment.
The current implementation conflates an empty dependency list ([]) with a missing one (null/undefined). If a user explicitly provides an empty list (e.g., via --dependency ""), inputDependencies?.length evaluates to 0 (falsy), causing the function to fall back to environment variables or return null.
In the planning layer, null is the sentinel that disables dependency-based skipping (meaning no skills are skipped due to missing dependencies). However, an explicit empty list should likely enable skipping for all skills that declare dependencies, as none of them are available.
To allow users to explicitly override environment variables with an empty list, consider checking for non-nullity instead of length.
Suggested change
| if (inputDependencies?.length) return [...new Set(inputDependencies)]; | |
| if (envDeps.length) return [...new Set(envDeps)]; | |
| if (stubEnabled) return [...dependencyStubs]; | |
| if (inputDependencies) return [...new Set(inputDependencies)]; | |
| if (envDeps.length) return [...new Set(envDeps)]; | |
| if (stubEnabled) return [...dependencyStubs]; |
4 tasks
s977043
added a commit
that referenced
this pull request
May 21, 2026
…802 Phase 3 A2-fix-3) (#871) Continue the Codex silent-skip cleanup after #865 (availableContexts) and #869 (availableDependencies). The plan layer's `buildExecutionPlan` already derives fileTypes, relatedADRs, and reviewMode (review-runner.mjs:212-217), but runReviewPlan never read them off the plan return and never forwarded them to generateReview. Impact (silent, no error) - fileTypes: generateReview's verifier file-phase coherence check fell back to "lenient when fileTypes not provided". On exec, phase mismatch checks were effectively disabled. - relatedADRs: ADR cross-references were never injected into the prompt on the exec path. Skill outputs lost the "ADR-001 says X" anchors that `river run` produced. - reviewMode: the diff-size-driven context budget preset was always the generateReview default rather than the calibrated 'tiny|medium|large' the plan layer determined. Fix - Pull `plan.fileTypes`, `plan.relatedADRs`, `plan.reviewMode` off the buildExecutionPlan return and pass them to generateReview alongside the existing diff/plan/phase/config. Optional chaining (`?? undefined`) keeps backward-compat: tests that mock buildExecutionPlan with a minimal shape continue to work. Tests - 2 new unit tests in tests/cli-review-plan.test.mjs cover the forward path and the undefined-fallback path. - runners/github-action/dist rebuilt with Node 22.22.2. Follows: #864 (A2-1), #865 (A2-fix-1), #869 (A2-fix-2). Remaining silent-skip risks - --plan replay context-snapshot drift (deferred to A2-3). Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
4 tasks
s977043
added a commit
that referenced
this pull request
May 22, 2026
…872) * docs(troubleshooting): close out silent-skip cleanup through v0.51.1 Codex-recommended docs follow-up after the v0.51.1 release. The existing troubleshooting page only documented the v0.51.0 availableContexts fix; v0.51.1 closed two more silent-skip failure modes (#869 availableDependencies, #871 fileTypes/relatedADRs/reviewMode) that were not yet reflected. What this adds - "Skill appears in skippedSkills with reason missing dependency": new section describing the v0.51.1 fix and the --dependency / RIVER_AVAILABLE_DEPENDENCIES / RIVER_DEPENDENCY_STUBS=1 knobs. - "Finding output looks generic / missing ADR references": new section explaining the three derived-context fields the exec path was silently dropping, and confirming the fix is automatic in v0.51.1+. - "Where the plan layer forwards each context": small reference table that maps each forwarded field to the PR / release that wired it. - "Residual gap: --plan replay does not yet run skills": explicit pointer to the remaining #802 A2-3 work so adopters know the boundary of v0.51.x. No code changes. Follows: #864 (A2-1), #865 (A2-fix-1), #869 (A2-fix-2), #871 (A2-fix-3). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * docs: link the #802 issue reference (Gemini PR #872 nit) Make the introductory mention of #802 a real GitHub link, matching the style of the residual-gap section. No content change. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
4 tasks
s977043
added a commit
that referenced
this pull request
May 22, 2026
…A2-fix-4) (#877) The final remaining silent-skip identified by the multi-perspective session review (Codex + Gemini): runReviewPlan was hard-coding riskMap: undefined when calling buildExecutionPlan, so the riskAssessment that buildExecutionPlan would have produced never appeared on the artifact, and never reached generateReview's prompt on the exec path. What changed - runReviewPlan loads the optional risk map via loadRiskMap (default path .river/risk-map.yaml). Missing file => null (no behaviour change). Malformed file => ReviewPlanError so it fails loudly instead of dropping the risk signal silently. - The loaded riskMap is forwarded to buildExecutionPlan, which already knew how to compute riskAssessment when given a non-null riskMap. - plan.riskAssessment is forwarded to generateReview alongside fileTypes / relatedADRs / reviewMode (the analysis-context bundle finalised in A2-fix-3). - loadRiskMapImpl is injectable so tests can drive the path without a fixture file on disk. Tests - 5 new unit tests cover happy path forwarding, null sentinel, load failure, generateReview hand-off, and the undefined-fallback when the plan omits riskAssessment. - runners/github-action/dist rebuilt with Node 22.22.2. Silent-skip cleanup status (post-A2-fix-4) - availableContexts (#865) - availableDependencies (#869) - fileTypes / relatedADRs / reviewMode (#871) - riskAssessment (this PR) - --plan replay context-snapshot drift is the only remaining item, deferred to A2-3. Multi-perspective review reference - Codex 86/100: "riskAssessment が runReviewPlan で riskMap: undefined 固定で実質ドロップ" 指摘 - Gemini 95/100: "riskAssessment の伝播漏れのみが唯一の明確な減点 対象" - 両者が独立に同じ点を 6 番目の silent-skip として指摘した。 Follows: #864 (A2-1), #865 (A2-fix-1), #869 (A2-fix-2), #871 (A2-fix-3). Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
6 tasks
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
Close the next item on Codex silent-skip list (follow-up to #865 A2-fix-1):
runReviewPlannever forwardedavailableDependenciestobuildExecutionPlan. The plan layer ignored dependency declarations entirely, which masked the same silent-skip class of bugs that #865 fixed for contexts.Changes
Behavior
nullsentinel (= dependency-based skipping disabled, backward-compatible).--dependency code_search,test_runner: forwarded verbatim.RIVER_DEPENDENCY_STUBS=1: enables the default stub set (code_search / test_runner / coverage_report / adr_lookup / repo_metadata / tracing).RIVER_AVAILABLE_DEPENDENCIES=...: parsed and merged when no arg is passed.Out of scope (remaining silent-skip risks)
Codex flagged:
These should follow the same pattern when their use cases land.
Test plan
Follows: #864 (A2-1), #865 (A2-fix-1).
🤖 Generated with Claude Code