review: clearer failure reason when findings aren't saved - #1185
Merged
Conversation
…empty
The empty-manifest warning ("Note: review skills ran but findings were not
persisted") previously hardcoded "env-var handshake did not reach the hook"
as the reason. That message is correct for one of at least five causes:
missing review tag, worktree path mismatch, BaseCommit mismatch, session
started outside the run window, or AgentType mismatch.
Replaces the static reason with explainEmptyManifest, which inspects the
candidate session states in the same filter order as matchReviewSessionState
and reports the cause that actually applied — with observed-vs-expected
values where relevant. Users (and bug reports) now carry enough information
to identify the failure without enabling debug logging.
Also drops a stale "gemini-cli" reference in the package doc comment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 0fb4b41ca371
The geminicli reviewer hardcoded "gemini-cli" both as ReviewerTemplate.AgentName
and as the agentName argument to AppendReviewEnv, while the agent registry
registers gemini under AgentNameGemini = "gemini". The lifecycle hook's
review-env adoption compares ENTIRE_REVIEW_AGENT against string(ag.Name()),
so adoption silently returned without tagging the session for every gemini
review — manifests were always empty, findings were never persisted.
The skilldiscovery registry had the same drift ("gemini-cli" keys), so
IsEligible("gemini") returned false and gemini never appeared in the
configure picker. The `entire agent add` help text suggested
`entire agent add gemini-cli`, which fails because the registry only
knows the agent as "gemini".
Aligns all three sites to the registry constant. Adds a registry-key
regression test to all three launchable reviewers (claude-code, codex,
gemini) so future drift between the reviewer's declared name and
agent.AgentName* fails CI rather than silently breaking adoption.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 0bb7377a4f07
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the entire review post-run flow by (1) providing a more specific user-facing diagnostic when the post-review manifest is empty, and (2) aligning Gemini reviewer naming with the agent registry so review-session adoption and picker eligibility work for Gemini.
Changes:
- Add
explainEmptyManifestand thread session states through manifest writing so the “manifest empty” warning can report the most likely rejection filter. - Align Gemini reviewer/env-handshake agent name to
agent.AgentNameGeminiand rekey skill discovery registries fromgemini-cli→gemini. - Add regression tests ensuring reviewer
Name()matches the agent registry key for Claude Code, Codex, and Gemini; update help/docs comments to usegemini.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/review/types/template.go | Update doc comment to refer to gemini (not gemini-cli). |
| cmd/entire/cli/review/types/reviewer.go | Update interface docs/examples to use gemini. |
| cmd/entire/cli/review/marker_fallback.go | Update doc comment to refer to gemini. |
| cmd/entire/cli/review/manifest.go | Return session states from state loading and add explainEmptyManifest diagnostic helper. |
| cmd/entire/cli/review/manifest_test.go | Update manifest warning test expectation; add unit tests covering all explainEmptyManifest branches. |
| cmd/entire/cli/review/cmd.go | Use explainEmptyManifest output for the user-facing “manifest not written” reason and log it at debug. |
| cmd/entire/cli/agent/skilldiscovery/registry.go | Re-key Gemini entries to gemini so eligibility/picker logic matches registry key. |
| cmd/entire/cli/agent/skilldiscovery/registry_test.go | Update tests to expect gemini keys. |
| cmd/entire/cli/agent/geminicli/reviewer.go | Use agent.AgentNameGemini for reviewer name and env handshake. |
| cmd/entire/cli/agent/geminicli/reviewer_test.go | Add test locking Gemini reviewer name to registry constant; update expected name. |
| cmd/entire/cli/agent/codex/reviewer_test.go | Add test locking Codex reviewer name to registry constant. |
| cmd/entire/cli/agent/claudecode/reviewer_test.go | Add test locking Claude Code reviewer name to registry constant. |
| cmd/entire/cli/agent_group.go | Update entire agent add help example from gemini-cli to gemini. |
…t cleanup Addresses review feedback on the empty-manifest diagnostic: 1. explainEmptyManifest now iterates summary.AgentRuns and names the specific agent whose AgentType has no matching tagged state, instead of blaming the first agent in the run list. In multi-agent runs with heterogeneous mismatches this prevents the diagnostic from misleading users into chasing the wrong filter. 2. Return signature extended to (reason, sentinel bool) so the caller can distinguish a known failure cause from the "no filter explained the rejection" terminal sentinel — the latter indicates matcher/explainer drift and now escalates to logging.Warn instead of being buried at Debug level. 3. New TestExplainEmptyManifest_MultiAgentNamesFailingAgent locks the per-agent iteration: a 2-agent run with one tagged Claude Code state must name "codex" (the failing agent) in the reason, not the first agent in the list. 4. New TestBuildLocalReviewManifestFromSummary_PartialMatch_NoWarning pins the intentional behavior that a partial-success run produces a non-empty manifest, so no warning fires. Future refactors that change this have to update the test, forcing the change to be deliberate. 5. Removes TestExplainEmptyManifest_DumpAllReasons — it asserted nothing (only t.Logf) and never could fail. 6. Trims comment rot per review: drops the past-bug reference from geminicli/reviewer.go and removes the hardcoded lifecycle.go path from the gemini test's doc comment so renames don't silently rot it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 3a7fbdf9ee71
Addresses Copilot's review comment that explainEmptyManifest returned on the first state that failed any filter, even when other tagged states passed that filter. Example: 2 tagged states where one fails worktree and the other passes worktree but fails SHA. Old behavior reported "worktree mismatch" against state-A; matcher actually rejected both — state-A by worktree, state-B by SHA. The diagnostic blamed the wrong gate. Refactors the worktree/SHA/StartedAt filters to apply cumulatively via a new applyExplainerFilter helper. Each filter narrows the candidate set; the function reports the filter that empties the set. AgentType still iterates per-agent against the post-cumulative candidate set, so it benefits from the narrowing too. New TestExplainEmptyManifest_CumulativeFiltering locks this with the exact scenario from the Copilot comment: state-A fails worktree, state-B fails SHA — reported cause must be SHA, not worktree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: fe04d8e831be
peyton-alt
changed the title
… test Three small follow-ups from review of this PR: 1. Model empty-SessionID as an explicit rejection cause. The matcher returns these states, but buildLocalReviewManifestFromSummary drops them on st.SessionID == "" before adding a manifest source. Without an explainer step the sentinel fired and surfaced a misleading "report this as a bug" Warn for what is really a partial-write or corrupt-state condition. New filter lives between the tagged-loop and the worktree filter so the cumulative-filter contract is preserved. 2. Accumulate and sort observed AgentTypes instead of overwriting. The diagnostic previously reported whichever mismatched type happened to be iterated last — non-deterministic across store.List orderings. The dedup + sort makes the rendered state field stable and faithful to the full set of mismatched types, which matters when ≥2 distinct types are tagged (e.g., both Codex and Gemini state present, agent looking for Claude Code). 3. picker_test.go: rename the "gemini-cli" literal to "gemini" so the test actually exercises the renamed gemini path. The "empty built-ins → Note" branch is generic to unknown agent names, but the test name and intent are gemini-specific. Two new tests cover the explainer changes: empty-SessionID branch and multi-type aggregation (with states intentionally listed in reverse-sorted order so the test fails if the sort step is removed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…diagnostic # Conflicts: # cmd/entire/cli/review/manifest_test.go
Soph
approved these changes
May 13, 2026
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
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.
https://entire.io/gh/entireio/cli/trails/360
Summary
Two related fixes to the
entire reviewpost-run path:Why
When a review run completes but no findings persist, users see:
That message is correct for one cause out of at least five — but it always blamed the env-handshake. Real failure modes include worktree-path mismatch, BaseCommit≠StartingSHA, agent type mismatch, or stale session state. Without an accurate reason in the user's terminal, every report became a guessing game.
While auditing the env-handshake side, found that
geminicli/reviewer.gohardcodes"gemini-cli"for bothReviewerTemplate.AgentNameand theAppendReviewEnvargument, but the agent registry registers gemini underAgentNameGemini = "gemini". The lifecycle hook'sadoptReviewEnvcomparesENTIRE_REVIEW_AGENTagainststring(ag.Name()), so adoption silently returned for every gemini review. Theskilldiscoveryregistry had the same drift, so gemini never appeared in the picker either.What changed
Commit 1 — manifest diagnostic
explainEmptyManifestinspects candidate session states in the same filter order asmatchReviewSessionStateand reports the cause that actually applied, including observed-vs-expected values.writePostReviewManifestnow calls the helper instead of using a static reason string.DumpAllReasonstest prints the actual wording.Commit 2 — gemini naming alignment
geminicli/reviewer.go: bothAgentNameandAppendReviewEnvargument now usestring(agent.AgentNameGemini). Adoption will now matchstring(ag.Name())in the hook.skilldiscovery/registry.go: rekeysgemini-cli→geminisoIsEligiblereturns true and gemini shows in the configure picker.agent_group.go: help text example updated fromentire agent add gemini-cli(which fails) toentire agent add gemini(which works).*_NameMatchesRegistryKeytest that locks the reviewer's declared name to the correspondingagent.AgentName*constant. Drift between them silently breaks review-session tagging — this test makes the failure loud.review/types/{reviewer,template}.go,review/cmd.go,review/marker_fallback.goto drop stalegemini-clireferences.Test plan
mise run fmt && mise run lint— cleango test ./cmd/entire/cli/agent/... ./cmd/entire/cli/review/...— all passgo test -tags=integration ./cmd/entire/cli/integration_test/ -run "Review|Manifest|Lifecycle.*Review"— passTestExplainEmptyManifest_DumpAllReasons -vprints all 5 reasons cleanly/tmp/entire-handshake agent listshowsgemini(notgemini-cli)/tmp/entire-handshake agent add gemini --helpexample now matches the actual registry keyReview complete.footer renders, no regressionOut of scope
🤖 Generated with Claude Code
Note
Medium Risk
Medium risk because it changes the effective registry key for Gemini from
gemini-clitogemini, which can affect existing configs/CLI usage, and adjusts post-run manifest generation/warnings that run after every review.Overview
Improves
entire reviewpost-run behavior when the local review manifest is empty by computing a specific, user-facing rejection reason (e.g., no session states, not tagged as review, worktree/SHA/time-window/agent-type mismatches) instead of always blaming the env-var handshake.Aligns Gemini’s reviewer and skill-discovery identifiers to the registry-stable key
gemini(including env-var tagging and picker eligibility), updates theagent addhelp example accordingly, and adds regression tests to ensure each launchable reviewer’sName()stays in sync with itsagent.AgentName*constant.Reviewed by Cursor Bugbot for commit 93b82ae. Configure here.