From 4189e7c5b7d199cec634856fc11c17be11a1253e Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 8 May 2026 15:04:43 -0400 Subject: [PATCH 1/2] feat(review): warn user when review manifest is not persisted When `entire review` ran but the review session was not tagged as `KindAgentReview` (env-var handshake did not reach the hook), the manifest write would silently no-op. Three downstream features (`--findings`, `--fix`, end-of-run footer) all gate on the same manifest, so a single silent miss broke them as a unit with no user-visible signal. This change makes the silent failure modes legible: - `lifecycle.go`: debug-log when adoption is skipped because `ENTIRE_REVIEW_SESSION` is unset on the hook process. Other adoption-skip branches already log at WARN; this was the only silent one. DEBUG level keeps it quiet for normal coding turns unless `ENTIRE_LOG_LEVEL=debug` is set. - `review/cmd.go`: when `writePostReviewManifest` skips at any of its three early-return branches (state-store load error, no matching review sessions, manifest write error), print a user-visible note explaining what happened, that `--findings` / `--fix` will not see this run, and how to get diagnostic detail. Replaces three silent debug-only returns. Happy path is unchanged: working reviews print exactly the same output they do today. The warning only fires on failure paths that today produce zero user-visible signal. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: fa74d61feb1b --- cmd/entire/cli/lifecycle.go | 2 ++ cmd/entire/cli/review/cmd.go | 16 +++++++++ cmd/entire/cli/review/manifest_test.go | 46 ++++++++++++++++++++++++++ 3 files changed, 64 insertions(+) diff --git a/cmd/entire/cli/lifecycle.go b/cmd/entire/cli/lifecycle.go index 9fac200610..ea78c1e408 100644 --- a/cmd/entire/cli/lifecycle.go +++ b/cmd/entire/cli/lifecycle.go @@ -1040,6 +1040,8 @@ func adoptReviewEnv(ctx context.Context, state *session.State, expectedAgent str return } if os.Getenv(review.EnvSession) != "1" { + logging.Debug(ctx, "review env adoption skipped: ENTIRE_REVIEW_SESSION not set", + slog.String("expected_agent", expectedAgent)) return } envAgent := os.Getenv(review.EnvAgent) diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index 68865c154c..ac8732d833 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -648,19 +648,35 @@ func writePostReviewManifest( manifest, err := localReviewManifestFromCurrentState(ctx, worktreeRoot, headSHA, summary, aggregateOutput) if err != nil { logging.Debug(ctx, "review manifest not written", slog.String("error", err.Error())) + warnManifestNotWritten(out, "could not load session state: "+err.Error()) return } if len(manifest.Sources) == 0 { logging.Debug(ctx, "review manifest not written: no matching review sessions") + warnManifestNotWritten(out, "review session was not tagged as a review (env-var handshake did not reach the hook)") return } if err := writeLocalReviewManifest(ctx, manifest); err != nil { logging.Debug(ctx, "review manifest write failed", slog.String("error", err.Error())) + warnManifestNotWritten(out, "write to disk failed: "+err.Error()) return } writeReviewCompletionFooter(out, manifest) } +// warnManifestNotWritten prints a user-visible note explaining that the +// review skills ran but findings were not persisted, so `entire review +// --findings` and `entire review --fix` will not see this run. The reason +// string is appended verbatim and should describe the underlying cause in +// terms the user can act on (or at least diagnose with debug logs). +func warnManifestNotWritten(out io.Writer, reason string) { + fmt.Fprintln(out) + fmt.Fprintln(out, "Note: review skills ran successfully but findings were not persisted.") + fmt.Fprintf(out, " Reason: %s\n", reason) + fmt.Fprintln(out, " `entire review --findings` and `entire review --fix` will not see this run.") + fmt.Fprintln(out, " Re-run with `ENTIRE_LOG_LEVEL=debug` for diagnostic detail.") +} + func composeSingleAgentSinks(in singleAgentSinkInputs) []reviewtypes.Sink { if !in.isTTY || !in.canPrompt { fmt.Fprintf(in.out, "Running review with %s...\n", in.agentName) diff --git a/cmd/entire/cli/review/manifest_test.go b/cmd/entire/cli/review/manifest_test.go index ace0a17945..e7c3bbdccf 100644 --- a/cmd/entire/cli/review/manifest_test.go +++ b/cmd/entire/cli/review/manifest_test.go @@ -379,3 +379,49 @@ func TestBuildLocalReviewManifestFromSummary_GroupsAgentSessionsAndAggregate(t * t.Fatalf("AggregateOutput = %q", manifest.AggregateOutput) } } + +func TestWarnManifestNotWritten_PrintsReasonAndDiagnosticHints(t *testing.T) { + var b strings.Builder + + warnManifestNotWritten(&b, "test reason text") + + got := b.String() + for _, want := range []string{ + "Note: review skills ran successfully but findings were not persisted.", + "Reason: test reason text", + "`entire review --findings` and `entire review --fix` will not see this run.", + "`ENTIRE_LOG_LEVEL=debug`", + } { + if !strings.Contains(got, want) { + t.Fatalf("warning missing %q:\n%s", want, got) + } + } +} + +func TestWritePostReviewManifest_WarnsWhenNoMatchingSessions(t *testing.T) { + repoRoot := t.TempDir() + testutil.InitRepo(t, repoRoot) + t.Chdir(repoRoot) + + var out strings.Builder + summary := reviewtypes.RunSummary{ + StartedAt: time.Now(), + AgentRuns: []reviewtypes.AgentRun{ + {Name: "claude-code", Status: reviewtypes.AgentStatusSucceeded}, + }, + } + + // SHA is irrelevant: matcher never runs since no session states exist. + writePostReviewManifest(context.Background(), &out, repoRoot, "abc123", summary, "") + + got := out.String() + if !strings.Contains(got, "Note: review skills ran successfully but findings were not persisted.") { + t.Fatalf("expected warning to fire when no sessions match; got:\n%s", got) + } + if !strings.Contains(got, "env-var handshake did not reach the hook") { + t.Fatalf("expected handshake-failure reason; got:\n%s", got) + } + if strings.Contains(got, "Review complete.") { + t.Fatalf("happy-path footer must not print when manifest is empty; got:\n%s", got) + } +} From 00fc0eb4bb8a841cc1ae31b1bbf02f51bd7fa8b5 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 8 May 2026 15:13:23 -0400 Subject: [PATCH 2/2] review: address Copilot feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - lifecycle.go: log message said "not set" but the branch fires for any value other than "1" (including "0"). Reword to "is not \"1\"" and include the observed value as a structured slog field. - cmd.go: warning text said "review skills ran successfully" but writePostReviewManifest can be reached when individual AgentRuns have AgentStatusFailed (the early return only checks Cancelled and zero-runs). Drop the "successfully" claim — per-agent statuses are already shown earlier in the review output, so the warning doesn't need to assert success it can't guarantee. - manifest_test.go: update assertion strings to match new wording. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 9847dd86142a --- cmd/entire/cli/lifecycle.go | 7 ++++--- cmd/entire/cli/review/cmd.go | 2 +- cmd/entire/cli/review/manifest_test.go | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/cmd/entire/cli/lifecycle.go b/cmd/entire/cli/lifecycle.go index f9ebf770cb..f62868caf3 100644 --- a/cmd/entire/cli/lifecycle.go +++ b/cmd/entire/cli/lifecycle.go @@ -1113,9 +1113,10 @@ func adoptReviewEnv(ctx context.Context, state *session.State, expectedAgent str if state.Kind != "" { return } - if os.Getenv(review.EnvSession) != "1" { - logging.Debug(ctx, "review env adoption skipped: ENTIRE_REVIEW_SESSION not set", - slog.String("expected_agent", expectedAgent)) + if envSession := os.Getenv(review.EnvSession); envSession != "1" { + logging.Debug(ctx, "review env adoption skipped: ENTIRE_REVIEW_SESSION is not \"1\"", + slog.String("expected_agent", expectedAgent), + slog.String("observed_value", envSession)) return } envAgent := os.Getenv(review.EnvAgent) diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index ac8732d833..07b0f38c65 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -671,7 +671,7 @@ func writePostReviewManifest( // terms the user can act on (or at least diagnose with debug logs). func warnManifestNotWritten(out io.Writer, reason string) { fmt.Fprintln(out) - fmt.Fprintln(out, "Note: review skills ran successfully but findings were not persisted.") + fmt.Fprintln(out, "Note: review skills ran but findings were not persisted.") fmt.Fprintf(out, " Reason: %s\n", reason) fmt.Fprintln(out, " `entire review --findings` and `entire review --fix` will not see this run.") fmt.Fprintln(out, " Re-run with `ENTIRE_LOG_LEVEL=debug` for diagnostic detail.") diff --git a/cmd/entire/cli/review/manifest_test.go b/cmd/entire/cli/review/manifest_test.go index e7c3bbdccf..146bff19d7 100644 --- a/cmd/entire/cli/review/manifest_test.go +++ b/cmd/entire/cli/review/manifest_test.go @@ -387,7 +387,7 @@ func TestWarnManifestNotWritten_PrintsReasonAndDiagnosticHints(t *testing.T) { got := b.String() for _, want := range []string{ - "Note: review skills ran successfully but findings were not persisted.", + "Note: review skills ran but findings were not persisted.", "Reason: test reason text", "`entire review --findings` and `entire review --fix` will not see this run.", "`ENTIRE_LOG_LEVEL=debug`", @@ -415,7 +415,7 @@ func TestWritePostReviewManifest_WarnsWhenNoMatchingSessions(t *testing.T) { writePostReviewManifest(context.Background(), &out, repoRoot, "abc123", summary, "") got := out.String() - if !strings.Contains(got, "Note: review skills ran successfully but findings were not persisted.") { + if !strings.Contains(got, "Note: review skills ran but findings were not persisted.") { t.Fatalf("expected warning to fire when no sessions match; got:\n%s", got) } if !strings.Contains(got, "env-var handshake did not reach the hook") {