fix(git): discriminated provider outcome + one boundary log + unified absence policy (2l6)#96
Conversation
… absence policy (2l6) Sheriff findings #2+#8: git/cache providers swallowed ~10 failure paths (stash failure rendered as 0, repoName failure as basename, failed status as a clean fallback branch) and projectGitInfo coerced absence to ''/0 while MetricsPayload preserved it — two opposite absence rules in one file, with render-payload's "providers log their own rejections" comment false for exactly these two providers. [LAW:types-are-the-program] New Outcome<T> = ok | absent | failed(reason) (src/utils/outcome.ts). execGitAsync returns the LaunchResult instead of flattening it to a throw; one classifier maps it per command — a non-zero exit that is the domain answering "there is none" (no upstream, no tags, no remote) is absent; transport failures are failed. GitInfo's on-demand fields are Outcome-typed; getGitInfo returns Outcome<GitInfo>; the basename repoName survives only as the explicit no-remote policy, never as an error fallback. readTail/cacheExpiresAt distinguish ENOENT (absent) from real read errors (failed). [LAW:effects-at-boundaries][LAW:single-enforcer] Failure flows as a value to each consumption surface's edge and is logged exactly once there: buildRenderPayload (pull path, via deps.log=dlog) and GitDataProvider's subscribe delivery fold. projectGitInfo is now a pure fold returning {git?, failures[]}; absent and failed both project as MISSING payload fields into applyInput's default + last_error chain. [LAW:one-type-per-behavior] GitPayload adopts MetricsPayload's preserve-absence policy (all fields optional); the coerce-to-''/0 shim is gone, so "no stashes" and "stash count unknown" are no longer the same rendered 0. Verified live against an isolated daemon: healthy repo renders with zero warnings (absent logs nothing), a corrupt .git produces exactly one warn line with the real git error while the bar still renders. Ticket: brandon-git-segment-2l6 (epic brandon-law-remediation-58c)
There was a problem hiding this comment.
Z.ai Coding Agent Review
This PR introduces a discriminated Outcome<T> (ok | absent | failed) across the git and cache provider stack, replacing T | null and catch-and-swallow patterns. The refactoring is well-executed and consistent with the laws cited inline.
No must-change items found. Notable design points worth the author's awareness (all pre-existing or intentional, none newly introduced):
-
Double-logging surface: A coalesced fetch whose failure is consumed by both the subscribe path (
deliverablelogs at LINE 328) and the render path (buildRenderPayloadlogs at LINE 416) will produce two warn lines for the same underlying failure. The comments explicitly acknowledge these as two distinct consumption paths with independent diagnostic purposes, so this is by design — but worth noting for anyone grepping logs. -
ascasts forABSENT:Promise.resolve(ABSENT) as Promise<Outcome<GitInfo>>at render-payload.ts:379 and :406 is needed becauseABSENTis typedOutcome<never>. The cast is sound (absent is a validOutcome<T>for anyT) but could alternatively bePromise.resolve<Outcome<GitInfo>>(ABSENT)to avoid the rawas. -
Unconditional
aheadBehind:getAheadBehindAsyncalways runs even when ahead/behind data isn't needed by the caller (pre-existing; the old code ran it unconditionally too). If this ever becomes a latency concern, ashowAheadBehindopt-in flag would be the seam.
The test coverage is strong — new real-git integration tests (git-service-outcomes.test.ts, render-payload-outcomes.test.ts), converted worktree tests from mocks to real repos, and existing cache/git tests updated for the new type.
✅ Approved
Summary
Implements sheriff law-remediation ticket brandon-git-segment-2l6 (findings #2 + #8, folded): git/cache providers swallowed ~10 failure paths with bare catches — two actively changing data meaning (stash failure →
0, repoName failure →path.basename) — whileprojectGitInfocoerced absence to''/0against MetricsPayload's preserve-absence rule, and render-payload's "provider rejections are logged by the underlying data-provider layer" comment was false for exactly these two providers.The shape
Outcome<T> = ok | absent | failed(reason)(src/utils/outcome.ts) — "this value is unknown because the fetch failed" is now representable, distinct from a real0/''.[LAW:types-are-the-program]execGitAsyncreturns theLaunchResultinstead of flattening launch's typed termination cause into a thrownError; one classifier maps it per command, with "is a non-zero exit a domain answer?" passed as data ("absent" | "failed"). No upstream / no tags / no remote →absent; timeout / spawn-error / signal →failed.[LAW:dataflow-not-control-flow][LAW:single-enforcer][LAW:effects-at-boundaries]:buildRenderPayloadlogs the pull path (viadeps.log = dlog);GitDataProvider's subscribe delivery fold logs the push path. The interior never logs.projectGitInfois a pure fold returning{git?, failures[]}— the boundary performs the effect.[LAW:one-type-per-behavior]:GitPayloadfields are all optional likeMetricsPayload;absentandfailedboth flow as MISSING fields intoapplyInput's default +last_errorchain. The coerce-to-''/0shim is deleted.readTail/cacheExpiresAtdistinguish ENOENT (absent, the everyday fresh-session case) from real read errors (failed).Verification
pnpm typecheck,pnpm lint,pnpm check:protocolclean; targeted suites (11 files incl. 2 new) pass — 200+ tests.test/git-service-outcomes.test.ts(real-git classification:ok(0)stashes vsabsentupstream vsok(basename)policy) andtest/render-payload-outcomes.test.ts(boundary: failed → missing field + exactly one warn log; absent → missing field + no log).test/git-worktree.test.tsrewritten against real worktrees — itsexecmock was dead since the launch migration and only passed through the old swallowing path..gitproduces exactly one warn line (provider fetch failed: git status --porcelain -b: non-zero, exit 128, fatal: not a git repository…) while the bar still renders.Ticket: brandon-git-segment-2l6 · Epic: brandon-law-remediation-58c