Derive the four telemetry pull requests from the landed recorder - #5347
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (51)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dea80a9cb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import type { | ||
| AttemptRecoveryKind, |
There was a problem hiding this comment.
Changing this import to a multiline form breaks the existing assertion at tests/usage/request-outcome-agreement.test.ts:194, which searches for the literal substring import type { AttemptRecoveryKind. Running that focused test at this commit fails on this assertion, so either preserve the single-line import prefix or update the oracle to recognize multiline imports. This source-as-data test also requires explicit coverage because import-graph selection cannot discover it.
AGENTS.md reference: AGENTS.md:L225-L230
Useful? React with 👍 / 👎.
| {detailFailure.cause && ( | ||
| <> | ||
| <span className="muted">{t("logs.detail.cause.label")}</span> | ||
| <span> | ||
| {detailFailure.cause} | ||
| {detailFailure.stage && ` (${t("logs.detail.stage.label")}: ${detailFailure.stage})`} |
There was a problem hiding this comment.
For an incomplete request, deriveRequestFailureAttribution intentionally persists failureStage while deriveRequestFailureCause returns no cause, but this condition hides the entire stage display whenever detailFailure.cause is absent. Consequently incomplete turns carry stage telemetry through the management API yet the dashboard never shows it; render the stage independently or gate the block on either field.
AGENTS.md reference: gui/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| const index = buffer.subarray(0, read).indexOf(0x0a); | ||
| if (index >= 0) return position + index + 1; |
There was a problem hiding this comment.
When captured.size - target is exactly the start of a row, the byte immediately before from is already an LF, but this search ignores that boundary and returns the LF after the row, unnecessarily deleting the first whole row that fits in the retention target. Check whether from is zero or preceded by LF before scanning forward; otherwise the implementation contradicts the documented newest-whole-rows contract in structure/gui-and-management-api.md:542-545.
AGENTS.md reference: structure/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
리뷰 · 우선순위 56 / 80이 PR은 예전에 따로 열려 있던 텔레메트리 네 갈래(#2366·#3748·#3983·#5063의 크기 제한 절반)를, 이미 들어온 사용량 recorder 위에서만 다시 만든 것입니다. 새 SQLite·새 타임라인 저장소는 없습니다. 실패한 요청은 닫힌 목록의 라인 - 메인테이너의 판단이 필요한 지점 이 PR이 머지되면 초안 #2366·#3983은 Closes로 닫히고, #3748은 “조회 파라미터만 있고 전용 페이지는 없다”고 조율자 판단에 맡깁니다. #3748을 이걸로 닫을지, 운영자 UI를 남길지 정해 주세요. #5063은 Usage 페이지·관리 라우트·카탈로그가 없어 일부러 안 닫습니다. 동의합니다. 드래프트 #5305(
너의 추천 한 줄 import 문자열 검사를 여러 줄 import에도 맞게 고친 뒤에만 머지하세요. 가능하면 최종 귀속의 이 댓글은 grok-bot이 작성했습니다 |
dea80a9 to
45dca12
Compare
⏳ DRAFT
What to do
Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required |
#2366 asked for durable failure attribution and shipped its own FailureSide and seven-member FailureStage to carry it. Lane C2 took the rehydration half and left that vocabulary behind, because defining a second one beside the stage and cause model that had just landed is the class of defect that blocked 2.60.0. This is the same answer expressed in the landed vocabulary. PersistedUsageAttempt and PersistedUsageEntry now carry failureStage and failureCause, both closed roster members. The resend verdict they imply is NOT stored: it is derived at read time, so a row written by an older build can never carry a verdict the current table would no longer reach. The derivation reads only closed values -- an HTTP status, a terminal status, a close reason, a transport phase, a recovery kind. errorCode and upstreamError are deliberately excluded: both are assembled partly from upstream text, so a classification keyed on them is a different answer per provider and per locale, and a grouping key built from them cannot promise it carries no content. That exclusion is what lets the pair be a Prometheus label and a fingerprint component without a masking pass. It runs at addFinalRequestLog, the one seam every request passes exactly once whatever transport served it, and before the attempt snapshot, so the row that reaches disk and the live attempt object carry the same pair. addRequestLog rebuilds the persisted row field by field rather than spreading it, so the pair is written there explicitly -- a field omitted at that line reaches /api/logs and never reaches usage.jsonl, which is the surface the derived projection reads. The stage and cause rosters move to src/usage/telemetry-contract.ts and src/lib/request-failure-model.ts re-exports them, the same relocation lane C2 made for the recovery roster and for the same reason: the dashboard renders a label per member, and a type-only import of the table module would drag its import graph into the browser project. The decision tables stay where they were. The test runs over a cross product built from the rosters themselves rather than a written-out list, so a member added later widens the space instead of leaving a case nobody wrote. Co-authored-by: chilung <b0423031@gmail.com>
Completes the agreement condition for the attribution the previous commit
records. The durable row carried a cause and nothing showed it, which is the same
shape as the defect lane C2 fixed: a real cause reaching the operator as an
absence of one.
The exporter gains opencodex_request_failures_total{protocol,cause}. It counts the
value the recorder derived rather than deriving one of its own, because the
recorder is the only place that sees the transport facts a cause needs, and two
derivations of one answer is exactly the disagreement this batch exists to remove.
The label set IS the shared dictionary rather than a copy of it. Cardinality is
fifteen causes across four protocols -- sixty series, fixed for the lifetime of the
roster, every value from a frozen list -- and it labels a counter, never a
histogram; a case asserts both.
/api/logs computes resendPermission at read time for the row and for each attempt.
It is never stored: the tables that decide it live in this build, and a row written
by an older one must not assert a permission the current tables would refuse. A
case asserts the pair is in the ledger module and the verdict is not.
The Logs detail dialog shows the cause, the stage it reached and the resend verdict,
and the attempt table leads its reason column with the cause, keeping the exact wire
errorCode behind it because that is what a bug report needs. Three satisfies clauses
make a missing label a typecheck failure rather than a silent fallback, and the
existing catalog oracle now covers the new key groups.
This trips the missing_ui_screenshot gate. This lane may not build or run the GUI,
so it cannot produce the screenshot; the gate fires on changed paths under gui/,
not on words in the description. The visible change is three rows added to the
detail dialog for a failed request and a named cause where the attempt table
previously showed a bare wire code.
Co-authored-by: chilung <b0423031@gmail.com>
…tore #3748 proposed a privacy-safe failure ledger and built it as a second SQLite store beside usage.jsonl, keyed by a free-text signature that regular expressions tried to mask. Both halves are replaced. The store becomes a projection rebuilt from the canonical ledger. It holds a count and two timestamps per group and nothing else, so deleting a row from usage.jsonl removes it from this grouping on the next rebuild -- which is what it means for retention to have one owner instead of four. It reads through the existing scanUsageLedgerCooperatively and therefore inherits every bound that scanner already enforces: the 1 MiB row ceiling, the 1 MiB chunk, the cooperative yield, the opened-EOF snapshot boundary, and the path/device/inode/birthtime identity with its 64 KiB boundary digest. A same-size file whose revision metadata moved forces a rebuild rather than an append, so a replaced ledger can never extend stale groups. The masked signature becomes a fixed-arity tuple of closed roster members. A regular expression can only assert that it removed what it matched; a tuple whose every slot is a member of a frozen list has nothing to remove. The input type cannot express a model, an account, an error message, a prompt, a request id or a timestamp, so no amount of upstream text can reach a fingerprint. Absent facts are explicit nulls in fixed positions, because omitting them would let [a, null, b] and [a, b] collide. The configured provider name is the one input that starts as free text -- users name their own provider entries -- so it is resolved against the provider registry and becomes null when it is not a registry member. A provider named after its owner groups under null, which is the honest answer. This exposed a real hole the fingerprint would otherwise have inherited: terminalStatus was persisted as a plain string and copied through the normalizer on truthiness alone, unlike the inbound protocol, transport phase and terminal source beside it. Harmless while it was only rendered; not harmless as a grouping-key slot, because the value is assembled from an upstream terminal frame. It is now the closed type, derived from the outcome roster rather than restated, and validated on read back. Two parts of the original are deliberately absent. The occurrence list is a second copy of history with its own retention policy. The mutable monitoring/dispatched/fixed/ignored status and its notes are operator state, which cannot be reconstructed from immutable request rows; presenting them as a derived ledger would be presenting a claim this projection cannot make. They need their own owner, keyed by the fingerprint, if they are wanted. The reader is GET /api/usage?failures=1 rather than a new route: it answers a different question from the usage summary and costs a scan, so it is opt-in and a dashboard asking for spend does not pay for it. Co-authored-by: SB Yoon <44089734+yansigit@users.noreply.github.com>
#3983 wanted the signals a stream diagnostic gives -- a missing terminal, adapter-to-client loss, empty output, partial output size -- and emitted one debug line per event to get them. Two things make that the wrong shape. It is a second durable record. emitDebugLine writes the in-process ring AND stderr, and stderr is redirected to the service log under both launchd and systemd, so an installed service accumulates a per-event history beside the ledger with its own retention, sequencing, request identity and masking. And per-event lines needed a per-payload fingerprint to correlate; under a process-global random key that makes every repeated prompt fragment, tool name and error message correlatable for the lifetime of the process. Five bounded counts on PersistedUsageAttempt answer the same questions and cannot carry content at all. They ride the attempt, so they inherit the ledger's normalization, masking and retention instead of acquiring their own, and the debug ring now FORMATS one line per finalized attempt from what the recorder already counted -- appendDebugLogLine directly, never emitDebugLine, so the ring is a live view of the durable record rather than a parallel source for it. The counting point matters. Adapter events are counted at the one seam every adapter parse already passes; relayed frames are counted after a SUCCESSFUL controller enqueue in the SSE bridge. Counting both at the reader would make the two numbers equal by construction and erase the one discrepancy they exist to expose. The recorder is bound to the request's translator budget -- an object every bridge on the delivery path already receives -- and reaches the current attempt through a callback rather than holding one, so a mid-request attempt rotation credits the attempt that is live rather than one already finalized. sideEffectEvents feeds the failure stage, which makes side-effect reachable for the first time: a relayed tool call is an externally visible effect, so the resend verdict refuses. Counting it at the transport rather than the adapter is what makes that correct -- an emitted tool call the client never received has committed nothing. Two things from the original are deliberately absent: run-turn-execution.ts is untouched, because its accounting distinguishes adapters that report their own physical sends and carrying the PR's unconditional pre-count would double-charge them; and no content HMAC exists anywhere here. Also narrows the 400 refinement added earlier in this branch, after review: it now consults only the LAST recovery recorded on the attempt, and a finalizer that can prove a cause passes it directly instead. The key-account rotation now attributes the attempt it seals, which previously reached the ledger with no attribution at all because the finalization seam only ever sees the last attempt of a request. Co-authored-by: yansigit <yansigit@users.noreply.github.com>
…contract #5063 proposed retention on the canonical ledger, which is the right architecture: the alternative is a projection that hides rows the ledger still has, and that is a second retention policy. What its implementation could not promise is that a row appended between its size snapshot and its rename survived -- it captured a size, copied a suffix, and renamed over whatever was there. Its own concurrency test performed two sequential calls and said so. Two things close that here. The append is synchronous and the compaction runs inside the same call stack, with no await between the append and the publication, so no in-process append can interleave; a second server on the same home cannot append at all, because it is refused by the existing ledger-owner lease at startup, which is why the hook is installed after ownership rather than before. And validateBeforeRename re-opens the target immediately before the rename and refuses unless identity, size and revision metadata are byte-for-byte what was copied -- so an append from anywhere else aborts the replacement rather than losing the row. Both the original file and that append survive, and the next append retries from a fresh revision. A test drives exactly that window through an injected hook, because a contract nothing can drive is a contract nobody has checked. Publication goes through the shared atomic writer rather than a hand-rolled temp lifecycle, which is where the exclusive private temp, the identity assertions, the platform-aware replace and the residual cleanup already live. The writer gains a streaming form so the retained span is copied in bounded chunks instead of held in memory as one string, and that form fsyncs the temp before the rename and does not swallow the failure: a replacement whose replacement is not on disk can lose the rows it was meant to keep. Rows are copied byte for byte and never parsed or re-serialized. A retention pass that understood the row shape would silently drop every field it was written before, which for this branch would mean the failure stage and cause it just added. The invalidation half was missing entirely from the original. Deleting rows invalidates three readers that do not watch the file: the 2,000-entry Logs ring, which otherwise keeps serving rows the ledger no longer has until eviction or a restart; the retained usage aggregate and failure projection, whose checkpoints now point past a boundary that moved; and the request-history index, whose source identity changed. All three are discarded after a replacement. This does NOT close #5063. The Usage-page control it also asks for is not here: this branch may not build or run the GUI, so it cannot produce the screenshot that gate requires, and shipping an unverifiable control is worse than shipping the policy the control would set. The limit is settable in config.json today and the docs say so. Co-authored-by: Vocllum <149675937+Vocllum@users.noreply.github.com>
Adversarial review of this branch found three cases where the derived cause was wrong against real request paths rather than against the fabricated facts the first test used. A stream that dies mid-flight is reported as a SYNTHETIC 502 -- a tail this proxy wrote, with transportPhase mid_stream and the attempt marked aborted. Read in status order that 502 became upstream-fault, which claims the origin answered when it did not. Transport evidence now outranks the numeric status. Both causes refuse an automatic resend, so this is an accuracy fix rather than a safety one, but a label an operator cannot trust is a label they stop reading. 402 had no branch and fell through to payload-rejected, which made quota-exhausted unreachable and pointed an operator at the payload when the account is what has to change. transport-unsent was reachable only through a fabricated status 0: a real connect failure is formatted as 502 by the dispatch path. Worse, it was the FALL-THROUGH, and it is the one transport cause that permits an automatic resend. It is now reachable only through causeHint, from a site that classified a pre-connect failure and can prove it; everything else answers transport-ambiguous, which is the honest classification for an unknown execution state and the safe direction for a permission decision. Review also found the streamed atomic replacement fsynced the temp's contents and not the directory entry recording the rename, so a host losing power after a successful call could leave the old ledger or an indeterminate directory. The streaming form now syncs the parent directory. Only that form does: it is the one making a durability claim, and charging every config write for a promise its callers were never given is a different change. The regression cases now use the production shapes -- a synthetic 502 after mid_stream, an aborted stream, an upstream 502 that stays an upstream fault -- rather than a status no transport produces.
…ating them Three findings from the second adversarial review round. A non-streaming turn delivers its whole answer as one body and calls no per-frame recorder, so every buffered response persisted adapter events with zero relayed ones. That is the adapter-to-client loss signal, raised on every buffered request, which makes the signal worthless. The buffered seam now records its delivery from the body it built: everything the adapter produced did reach the client, in one piece, and the semantic bytes and side effects are read from the assembled output. The body is read by field name rather than by the adapter event union, so a member added later is not a merge-time exhaustiveness failure in a counter that does not need one. Two tests claimed their cross products came from the declared vocabularies and then wrote the members out by hand, which is how an added member leaves an exhaustive test green without being exercised. They now read REQUEST_TERMINAL_STATUSES, REQUEST_CLOSE_REASONS and a transport-phase roster that is declared once in the contract leaf and consumed by the ledger validator instead of being stated twice. INV-RESEND-01 named two enforcing tests while the structure checker binds only the first, so the second was prose-only assurance. The attribution rule is now its own INV-ATTRIBUTION-01 with one binding, and the test names it so the binding is readable from both sides. Adds the lane record, including the two limits this branch does not close: the six intermediate attempt finalizers that still reach the ledger unattributed, and the successful-recovery case that can still misattribute a 400. It also records a pre-existing defect found while reviewing the atomic writer -- its scrub fallback opens with "wx" and so always fails on an existing temp -- which is left alone because it predates this branch and sits on a security-adjacent path.
The file-size ratchet reported NEW_OVERSIZED on the first exact-head run. src/server/request-log.ts carries the whole request-logging surface and was 1,962 lines against the repository's 2,000-line seed threshold; the attribution wiring pushed it to 2,015. The remedy is a move, never a number: the cap only ever goes down, and a threshold is not something to negotiate with. The two places a stage and cause are decided and written -- the finalization seam, and the attempt sealed by a key-account rotation -- now live in src/server/request-log-failure-attribution.ts. Behaviour is unchanged: the same facts go in, the same attempt is stamped before the snapshot, and the same pair reaches the row. request-log.ts is 1,979 lines after the move. That is 21 lines of headroom, which the lane record notes for whoever touches this file next.
Three exact-head failures, all from the new failure-cause counter and all in
assertions that counted by hand.
management-metrics-export.test.ts already derives its sample total from the closed
vocabularies -- its own comment says the literal "went stale the moment a bounded
label value was added, which is the failure mode this repository keeps hitting in
merges". The new counter's contribution is added to that arithmetic the same way.
Its HELP/TYPE assertion was the literal 7 the comment warns about, so it now reads
the metric names out of the exposition and asserts the two groups name the same set
exactly once each, which is what deterministic grouping means and what no added
metric can make stale.
The dashboard-union assertion matched the literal string "import type {
AttemptRecoveryKind", which broke when the import wrapped across lines to take the
three new names. It now matches the property it was testing -- the name arrives from
the contract leaf and the page declares no union of its own -- without depending on
how the import is formatted.
The public metrics table in the management-API reference gains the new series.
…wo of them The streamed writer added a third openSync(path, "wx", 0o600) and the portability test counted exactly two. The count was the weaker form of what it meant: the property is that no temp writer in atomic-write.ts drops the O_CREAT bit, and that holds for however many writers exist. It is now a set comparison over every openSync on the temp path, which a fourth writer cannot make stale and an unsafe spelling cannot pass. The edit is line-neutral because that file sits exactly at its ratchet cap.
cdbd802 to
714ddbf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ea3974bb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ...(signal ? { signal } : {}), | ||
| onEntry: entry => accumulator.add(entry), | ||
| }); | ||
| retained = retain(accumulator, scan, false); |
There was a problem hiding this comment.
When GET /api/usage?failures=1 scans a large ledger containing many distinct failure tuples, this assignment retains the entire group map indefinitely, but the new cache is absent from APP_OWNED_RETAINED_STORE_REGISTRATIONS and never calls enforceAppOwnedMemoryBudget() after publication. Consequently it can exceed appOwnedMemoryBudgetMb while remaining invisible to memory accounting and eviction; expose retained byte/age statistics, register the projection as an evictable cache, and enforce the budget after publishing it.
Useful? React with 👍 / 👎.
| if (url.searchParams.get("failures") === "1") { | ||
| const projection = await getFailureProjection(); | ||
| return jsonResponse({ |
There was a problem hiding this comment.
This adds a public management-API mode whose response completely replaces the normal usage summary, but the edited management reference still documents only generic GET /api/usage and never mentions failures=1, its fields, or the scan cost. Operators therefore cannot discover or safely consume the new surface; add the query mode and response contract to docs-site/src/content/docs/reference/management-api.md.
AGENTS.md reference: AGENTS.md:L434-L435
Useful? React with 👍 / 👎.
1 participant
Summary
The four telemetry pull requests lane C deferred "as implemented", rebuilt as consumers of the
recorder that #5266 and #5300 landed. Reimplements #2366, #3748 and #3983; carries the size-limit
half of #5063. No new store: the durable shapes stay
PersistedUsageAttemptandPersistedUsageEntry, and every projection reads them.Why a failed request is now attributable (#2366).
failureStageandfailureCauseride theattempt that ended a request and the logical row, both closed roster members. The PR's own
FailureSideand seven-memberFailureStageare not here — two attribution vocabularies for onequestion is the class that blocked 2.60.0 — and its widening of
transportPhaseandterminalSourceto arbitrary strings is not here either.terminalStatus, which was a plainstring, joins the closed validators beside it, because it is now a grouping-key slot assembledfrom an upstream frame. The derivation reads only closed values;
errorCodeandupstreamErrorareexcluded because both carry upstream text, so a classification keyed on them is a different answer
per provider and per locale. That exclusion is what lets the pair be a Prometheus label and a
fingerprint component with no masking pass. The resend verdict they imply is never stored —
/api/logscomputes it at read time, so a row written by an older build cannot assert a permissionthe current tables refuse.
Why recurring failures group without a second ledger (#3748).
src/telemetry/and its SQLitestore are not built. Failed rows are folded during the existing cooperative ledger scan into a
versioned fingerprint over a fixed-arity tuple of closed members, inheriting that scanner's 1 MiB
row ceiling, chunk size, checkpoint identity and boundary digest. The free-text signature and its
regex masking are replaced by construction rather than by a better regex: an expression can only
assert it removed what it matched, while a tuple whose every slot comes from a frozen list has
nothing to remove. Read with
GET /api/usage?failures=1, opt-in because it costs a scan.Why stream diagnostics need no second emission path (#3983).
emitDebugLinewrites the ringand stderr, and a service manager redirects stderr to a file, so the PR's per-event lines would
give an installed service a durable per-event history beside the ledger; its per-payload HMAC used a
process-global key, making every repeated prompt fragment and tool name correlatable for the process
lifetime. Five bounded counts on the attempt answer the same questions and cannot carry content.
Adapter events are counted at the adapter-parse seam and relayed frames after a successful enqueue —
counting both at the reader would make them equal by construction and erase the loss signal. The
debug ring now formats one line per finalized attempt from those counts.
Why retention can delete rows safely (#5063).
usageLedgerMaxBytesis unset by default andunset means unlimited. The defect this closes: #5063 captured a size, copied a suffix and renamed
over whatever was there, so a row appended in between was silently dropped. Now the compaction runs
inside the synchronous append call stack, a second server on the same home cannot append at all
because the existing ledger-owner lease refuses it, and
validateBeforeRenamere-opens the targetimmediately before the rename and refuses unless identity, size and revision metadata are
byte-for-byte what was copied. Rows are copied and never parsed, which is what keeps a field a newer
build wrote intact through a compaction. A compaction also discards the Logs ring, the retained
aggregates and the request-history index — otherwise
/api/logskeeps serving rows the ledger nolonger has.
Before/after, concretely: a turn whose upstream stream died mid-flight was a green row with a bare
upstream_server_error; it is nowtransport-ambiguousatprotocol-prelude, reported identicallyby the dashboard, the durable row and
opencodex_request_failures_total, with a resend verdict thatnames which refusal it is.
Closes #2366
Closes #3983
#3748's grouping is built and readable, but its operator surface is one query parameter rather than
a page; the coordinator decides whether that closes it.
#5063 is deliberately not closed. Its Usage-page control is not here. This lane may not build or
run the GUI, so it cannot produce the screenshot that gate requires, and shipping an unverifiable
control is worse than shipping the policy it would set. Remaining scope: the dashboard control, its
management route, and the ten catalog strings. The limit is settable in
config.jsontoday and theconfiguration reference documents it.
Verification
Static source review plus exact-head hosted CI, and four high-effort adversarial reviews — one per
carried pull request before implementation, then three over the finished branch covering typecheck
hazards, repository gates, and runtime correctness and privacy. Their findings are in the branch:
transport evidence now outranks a synthetic status, 402 maps to
quota-exhausted,transport-unsentis no longer the fall-through (it permits a resend, so it is reachable only froma site that can prove a pre-connect failure), the streamed writer fsyncs the parent directory,
buffered responses no longer read as total relay loss, two tests read rosters instead of restating
them, and the invariant is split so each of INV-RESEND-01 and INV-ATTRIBUTION-01 binds exactly one
test.
Checked statically on this branch:
src/server/index.tssits at 884 against893, and the ten locale catalogs are exempt;
scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.jsonagree key for key,and each new test's regex seed resolves to the domain it is registered to;
src/usage/telemetry-contract.tsstill has no imports,src/usage/request-outcome.tsstill reachesnothing but it, and
gui/src/pages/Logs.tsxstill never namessrc/usage/log;NOT RUN on this branch, by instruction, and not to be recorded as passing:
bun run test, anyindividual
bun testfile,bun run typecheck,bun run build:gui,bun run lint:gui,bun install,bun run structure:check,bun run privacy:scan, and any liveocxexecution.The GUI screenshot gate
missing_ui_screenshotfires, and it is a fair gate here: this adds UI, not only strings. Whatfollows is the evidence for a judgement, not an argument for a waiver.
What is new on screen. Three rows in the Logs detail dialog's Basic section, and one changed
value in the attempt table:
errorCode, e.g.upstream_server_errorerrorCoderemains the next fallbackWhy the layout risk is low, stated precisely. Both new rows are label/value pairs inside the
existing
.log-detail-grid, which is a two-columnmax-content minmax(0, 1fr)grid that alreadyrenders Time, Outcome and Upstream sends the same way. No CSS is added or changed, no new class
appears, no column is added to any table, and nothing becomes conditional that was not already —
the adjacent Upstream sends row uses the identical
{cond && (<>…</>)}pattern. The only genuinelynew visual is a longer value string in the second grid column, which that column already wraps via
minmax(0, 1fr).What a screenshot would still add. Text length. The longest English value is "Not resent: the
same request would fail again", and German and Vietnamese are longer still; at the 7rem label
column the narrow breakpoint uses, that wraps. Wrapping is the expected behaviour of this grid, but
"expected" is a source claim, and only a capture shows whether it reads well next to the row above
it.
What source checking already settles. The ten catalog edits are +29 lines and 0 removed in each
file, every one of the 29 keys exists in all ten catalogs, and three
satisfiesclauses make amissing label a typecheck failure rather than the silent fallback that previously rendered four real
recovery kinds as "Unknown recovery reason".
Two limits this branch does not close
Only the attempt that ends a request, plus the one sealed by a key-account rotation, carry
attribution; six intermediate finalizers in
policy-fallback.tsandcore-combo.tsstill reach theledger unattributed. Each has different evidence in scope, and a branch verified by static review
alone should not add six classification call sites at once. The logical row is attributed in every
case, which is what the projection and the exporter read.
A ciphertext or reasoning-parameter recovery that succeeded, followed by an unrelated 400 on the same
attempt, still reads as that recovery's cause. The rule is narrowed to the last recorded kind on the
matching status; the proper fix belongs in the recovery path.
Both are recorded in
devlog/_plan/260920_round2_followups/050_lane_r5.md, along with a pre-existingdefect found while reviewing the atomic writer: its scrub fallback opens with
"wx"and so alwaysfails on an existing temp. That one predates this branch and sits on a security-adjacent path, so it
is left for its own change rather than fixed inside a telemetry branch.
Checklist
Summary by CodeRabbit
New Features
Localization
Documentation