feat(backlog): persist full raw output when a headless triage/review call fails - #328
Open
tstapler wants to merge 1 commit into
Open
feat(backlog): persist full raw output when a headless triage/review call fails#328tstapler wants to merge 1 commit into
tstapler wants to merge 1 commit into
Conversation
…call fails Investigating stuck triage item be676dab (an 8h52m headless triage session with no usable result) showed there was no way to recover what the LLM actually returned: ParseHeadlessTriageResult's parse-failure log line only includes a ~200-byte preview, and the log file itself rotates out of ~/.stapler-squad/logs/ within a few hours — by the time an operator investigates, both the full output and the log line describing it are gone. Nothing durable/queryable in the DB recorded a call failure's raw output either; end_reason (classifyHeadlessCallError's bucket) already existed but was never even surfaced over the wire. Root cause: TriggerTriage's callErr and parseErr branches (server/services/backlog_service_triage.go) discarded `raw` entirely on failure, and TriggerReReview's real callErr branch didn't even create an ItemSession row to record anything against. Fix, reusing this repo's existing scrollback/transcript-file precedent (session/review_transcript.go's WriteReviewTranscriptFile) rather than a new storage mechanism: - session.WriteHeadlessFailureCapture writes the size-capped (256KB, tail-kept) raw output to a durable file under a new config dir (~/.stapler-squad/headless-failures/, config.HeadlessFailureCaptureDirOrDefault), deliberately NOT inside the existing per-item triage-artifacts dir (which readPlanFile feeds into later review/triage prompts — writing there would leak raw failure text into future LLM context). - New ItemSession.failure_capture_path column (ent schema + migration) references the file; a new orthogonal Update method sets it alongside the existing end_reason. TriggerTriage wires this into both its callErr and parseErr branches; TriggerReReview's callErr branch now also creates a best-effort audit ItemSession row (previously nothing was persisted there at all) with the same capture + classified end_reason. - classifyHeadlessCallError generalized to take an explicit call budget (was hardcoded to triageCallBudget) so TriggerReReview can share it. - end_reason and failure_capture_path added to the ItemSession proto message and threaded through to the frontend; BlockedNotice now renders the classified failure reason + capture path instead of an unexplained "No diagnostic data recorded." for a failed headless call. Separately investigated (per the same brief) whether the zombie-reviewer distinction (list_workspace_peers' status=Active/lifecycle=gone) that PR #320 fixed is persisted anywhere durable: confirmed it already is — reconcileStuckReviewItems' zombie-session detection writes a DB-backed BacklogStuckState row (survives restart, queryable via FindOpenStuckStates), not just an in-memory/log signal. No gap there; no change made. Regression tests: session/headless_failure_capture_test.go (write/truncate/ no-op behavior) plus three end-to-end tests exercising the real TriggerTriage/ TriggerReReview goroutines through a fake headless pool, asserting the full raw output survives to a durable, DB-referenced file after both a parse failure and a call error. make build && make lint: clean. go test ./session/... ./config/... ./server/services/...: all green. Full `make test`/frontend jest hit two failures under parallel load — the documented pre-existing session/tmux flake (TestEnsureServerRunning_NoOp, PR #320 precedent) and an unrelated CAS-racer test (TestReportDuplicate_ReportsDistinctMessage_WhenCASPreconditionFails); both pass reliably in isolation (confirmed 5/5). Two frontend suites (SessionDetail.embedded.test.tsx, BacklogEmptyState.test.tsx) also fail in isolation on components this change never touches — pre-existing on this branch, unrelated to this diff (3701 tests / 269 suites green otherwise). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3683CH7Fs9zYR2yP3Dpba
Contributor
✅ Registry ValidationTest Coverage: 25/181 features have
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
E2E RPC Latency |
Contributor
Frontend Terminal Throughput |
Contributor
UX Analysis
|
Contributor
📊 Feature E2E CoverageFeature coverage report unavailable
|
Contributor
🎬 E2E Feature Demos2 shard(s) recorded feature flows for this PR. recordings shard 1 Demo preview opens directly in browser (single-file HTML). Raw WebM recordings in ZIP. Expires after 30 days. |
tstapler
marked this pull request as ready for review
August 10, 2026 16:16
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds durable capture + surfacing of full raw headless triage/review output on call/parse failures to close an observability gap (recoverable beyond log rotation), and threads the failure metadata through DB → RPC → UI.
Changes:
- Persist size-capped raw headless output to a durable file and store its path on
ItemSession(failure_capture_path) along with a surfacedend_reason. - Wire capture + persistence into
TriggerTriagefailure branches and add best-effort auditItemSessioncreation onTriggerReReviewcall failure. - Expose the new fields to the frontend and render better failure detail in
BlockedNoticewith new tests.
Reviewed changes
Copilot reviewed 16 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web-app/src/lib/hooks/useBacklogService.ts | Maps endReason + failureCapturePath from proto into LinkedSession. |
| web-app/src/gen/session/v1/backlog_pb.ts | Regenerated TS proto types including new ItemSession fields. |
| web-app/src/components/backlog/detail/BlockedNotice.tsx | Renders headless failure reason + capture path in the “missing diagnostic data” notice. |
| web-app/src/components/backlog/detail/BlockedNotice.test.tsx | Adds UI tests for rendering endReason/failureCapturePath behavior + precedence. |
| session/storage_backlog.go | Adds repository method to persist failure_capture_path. |
| session/storage.go | Adds storage façade method for updating failure_capture_path. |
| session/repository.go | Extends ItemSessionSummary to include FailureCapturePath. |
| session/headless_failure_capture.go | Adds helper to write size-capped raw output to a durable “headless-failures” dir. |
| session/headless_failure_capture_test.go | Adds unit tests for capture helper behavior (tail truncation, no-op, durability). |
| session/ent_repository_backlog.go | Maps ent ItemSession.FailureCapturePath → ItemSessionSummary. |
| session/ent/schema/item_session.go | Adds failure_capture_path column to ent schema. |
| server/services/backlog_service_triage.go | Adds capture helper usage; persists capture path; generalizes error classifier signature; adds re-review audit session persist. |
| server/services/backlog_service_triage_test.go | Adds regression tests covering triage parse failure capture, call error capture, and re-review call error capture. |
| server/services/backlog_service.go | Threads end_reason and failure_capture_path into the proto mapping. |
| proto/session/v1/backlog.proto | Adds end_reason and failure_capture_path to ItemSession. |
| config/config.go | Adds HeadlessFailureCaptureDirOrDefault() for capture file location. |
Files not reviewed (9)
- gen/proto/go/session/v1/backlog.pb.go: Generated file
- session/ent/itemsession.go: Generated file
- session/ent/itemsession/itemsession.go: Generated file
- session/ent/itemsession/where.go: Generated file
- session/ent/itemsession_create.go: Generated file
- session/ent/itemsession_update.go: Generated file
- session/ent/migrate/schema.go: Generated file
- session/ent/mutation.go: Generated file
- session/ent/runtime.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1893
to
+1908
| func (s *BacklogService) captureHeadlessFailure(sessionUUID, raw string) string { | ||
| if raw == "" { | ||
| return "" | ||
| } | ||
| dir, dirErr := s.cfg.HeadlessFailureCaptureDirOrDefault() | ||
| if dirErr != nil { | ||
| log.WarningLog.Printf("[captureHeadlessFailure] resolve capture dir: %v", dirErr) | ||
| return "" | ||
| } | ||
| path, writeErr := session.WriteHeadlessFailureCapture(dir, sessionUUID, raw, session.DefaultHeadlessFailureCaptureMaxBytes) | ||
| if writeErr != nil { | ||
| log.WarningLog.Printf("[captureHeadlessFailure] write capture file session=%s: %v", sessionUUID, writeErr) | ||
| return "" | ||
| } | ||
| return path | ||
| } |
Comment on lines
+2527
to
+2536
| errType := classifyHeadlessCallError(callErr, time.Since(callStart), callTimeout) | ||
| capturePath := s.captureHeadlessFailure(headlessReReviewUUIDPrefix+uuid.New().String(), reviewResult) | ||
| log.ErrorLog.Printf("[TriggerReReview] headless re-review call failed item=%s errType=%s capture=%s: %v", item.ID, errType, capturePath, callErr) | ||
| failCleanupCtx, failCleanupCancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| if failIS, failCreateErr := s.storage.CreateItemSession(failCleanupCtx, session.ItemSessionData{ | ||
| ItemID: item.ID, | ||
| SessionUUID: headlessReReviewUUIDPrefix + uuid.New().String(), | ||
| SessionRole: session.SessionRoleReview, | ||
| AcSnapshot: session.AcCriteriaJSON(acSnapshotJSON), | ||
| }); failCreateErr != nil { |
Comment on lines
+70
to
+75
| absPath = filepath.Join(dir, headlessFailureCaptureFilePrefix+sessionUUID+".txt") | ||
| if writeErr := os.WriteFile(absPath, []byte(content), 0o644); writeErr != nil { | ||
| return "", fmt.Errorf("failed to write headless failure capture file: %w", writeErr) | ||
| } | ||
|
|
||
| return absPath, nil |
Comment on lines
+61
to
+64
| content := raw | ||
| if int64(len(content)) > maxBytes { | ||
| content = headlessFailureCaptureTruncationMarker + content[int64(len(content))-maxBytes:] | ||
| } |
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.
Summary
Investigating stuck backlog item
be676dab(an 8h52m headless triage session that produced no usable result) surfaced a real observability gap: there was no way to recover what the LLM actually returned.ParseHeadlessTriageResult's parse-failure log line only includes a ~200-byte preview of the raw output.~/.stapler-squad/logs/within roughly an hour — by the time anyone investigates a stuck item, both the full text and the log line describing it are usually gone.end_reason(classifyHeadlessCallError's bucket) already existed as a DB column but was never even surfaced over the RPC wire.TriggerReReview's real call-error branch didn't create anItemSessionrow at all — a failed re-review call was invisible even in the DB schema that already existed for triage.Root cause
TriggerTriage'scallErr/parseErrbranches (server/services/backlog_service_triage.go) discarded the LLM's rawstdoutentirely once they'd logged a preview of it.Fix
Reuses this repo's existing scrollback/transcript-file precedent (
session/review_transcript.go'sWriteReviewTranscriptFile) rather than inventing a new storage mechanism:session.WriteHeadlessFailureCapture(session/headless_failure_capture.go) writes the size-capped (256KB, tail-kept — the interesting content in a parse failure is usually near the end) raw output to a durable file under a new dir,config.HeadlessFailureCaptureDirOrDefault()(~/.stapler-squad/headless-failures/). Deliberately not inside the existing per-itemtriage-artifactsdir —readPlanFilefeeds that directory's contents into later review/triage prompts, so writing raw failure text there would leak it into a future LLM's context.ItemSession.failure_capture_pathcolumn (ent schema + generated migration) references the file. A new, orthogonalUpdateItemSessionFailureCapturesets it alongside the existingend_reasoncolumn.TriggerTriagewires this into both itscallErrandparseErrbranches.TriggerReReview'scallErrbranch now also creates a best-effort auditItemSessionrow (previously nothing was persisted there at all) with the same capture + a classifiedend_reason, before still returning the original RPC error.classifyHeadlessCallErrorgeneralized to take an explicit call budget (was hardcoded totriageCallBudget) soTriggerReReviewcan share it with its owncallTimeout.end_reasonandfailure_capture_pathadded to theItemSessionproto message and threaded through to the frontend.BlockedNoticenow renders the classified failure reason + capture path instead of an unexplained "No diagnostic data recorded." for a failed headless call — closing the loop from DB → RPC → UI for this one case, though there's no in-browser file-content viewer yet (see Follow-up below).Separately investigated: zombie-reviewer durability
The brief also asked whether the zombie-reviewer distinction (
list_workspace_peers'status: Active/lifecycle: gone, the bug PR #320 fixed) is persisted anywhere durable, or only visible transiently. Confirmed it already is:reconcileStuckReviewItems' zombie-session detection (session/backlog_lifecycle.go) writes a DB-backedBacklogStuckStaterow viamarkAbandonedReview, which survives a restart and is queryable viaFindOpenStuckStates— not just an in-memory map or a log line. No gap found there; no change made.Tests
session/headless_failure_capture_test.go— write/truncate-direction/no-op/durability behavior of the new capture helper.server/services/backlog_service_triage_test.go— three new end-to-end regression tests driving the realTriggerTriage/TriggerReReviewgoroutines through a fake headless pool:TestTriggerTriage_should_PersistFullRawOutputToDurableFile_When_HeadlessResultFailsToParseTestTriggerTriage_should_PersistFailureCapture_When_HeadlessCallItselfErrorsTestTriggerReReview_should_PersistFailureCapture_When_HeadlessCallItselfErrorsweb-app/src/components/backlog/detail/BlockedNotice.test.tsx— newdescribeblock covering theendReason/failureCapturePathrendering, including thatreviewVerdict.summarystill wins when both happen to be present.Verification
make build,make lint: clean.go test ./session/... ./config/... ./server/services/...: all green.make test/ frontendjesthit two failures under parallel load, both confirmed pre-existing/unrelated (pass reliably 5/5 in isolation):TestEnsureServerRunning_NoOp(session/tmux) — the documented pre-existing flake per PR fix(backlog): request_review permanently stuck once a zombie reviewer's FAIL verdict is auto-processed #320.TestReportDuplicate_ReportsDistinctMessage_WhenCASPreconditionFails(server/mcp) — an unrelated CAS-racer test I never touched.SessionDetail.embedded.test.tsxandBacklogEmptyState.test.tsxalso fail in isolation on components this diff never touches — pre-existing on this branch. Everything else: 269 suites / 3701 tests green.Follow-up (intentionally out of scope here)
Surfacing
failureCapturePathin the UI currently shows the server-local path as text (useful for someone with server/SSH access) rather than fetching the file's content into the browser — that would need a new RPC (+ this repo's feature-registry + e2e-test overhead) to stream the capture file, which felt like its own PR rather than bundled into the core capture mechanism.🤖 Generated with Claude Code
https://claude.ai/code/session_01W3683CH7Fs9zYR2yP3Dpba