Conversation
Owner
Author
Code Review — Cycle 1Full summary withheld (public repository).
No CRITICAL findings in any focus. Full report: /Users/dean/Sandbox/croxy/.devflow/docs/reviews/feat-astra-sse-capture/2026-09-13_2137/review-summary.md (not committed; ask the author) |
… clarify effort scope
Extract the two arms of the isSse branch into module-level siblings recordSseStream() and pipeThrough(). handleRequest goes from 162 to 94 lines and max nesting from 6 to 4; both arm bodies move verbatim, so behaviour is unchanged and the next batches get clean seams (complexity-04). Build the println/separator/indent helpers once per server with a module-level createPrinter() under the existing Printing banner instead of allocating three closures per request, drop the `const println = output` rename alias, and name the bare 64 as SEPARATOR_WIDTH (complexity-01). Pin both with a transcript test asserting a separator rule of exactly 64 columns, the numbered event lines, and the SSE TOTAL count. The byte-for-byte pass-through guard stays the three existing deepEqual assertions, unchanged (ADR-010).
Resolve five triaged issues in the per-model reasoning-effort path. consistency-01: reasoningEffortsForModel closed over the module-global MODEL_REGISTRY while every other registry consumer in models.ts takes `registry` as its first parameter. Take it explicitly and thread MODEL_REGISTRY from the codex-request call site, so the per-model rule is testable against a synthetic registry instead of whichever real ids happen to declare reasoningEfforts. (applies ADR-005, ADR-006) architecture-02: the effort vocabulary was spelled in two unlinked places and translateEffort picked between them with a double-negated ternary. Declare DEFAULT_REASONING_EFFORTS once next to the registry, make the accessor total (per-model list, else the default set), drop CODEX_EFFORT_VALUES, and collapse the ternary to a single positive membership test. Wire input stays `string`: unknown values still degrade with unsupported_effort_dropped rather than 400. (avoids PF-014, PF-004) reliability-09: buildRoutingTable now reports entries whose reasoningEfforts are not a subset of the default set, as a returned diagnostic rather than a throw — it stays total. A typo like "xhig" is otherwise silent: the model simply stops accepting an effort it should. complexity-06: document the canonical-id precondition on translateEffort and the accessor; an alias would fall back to the default set and silently widen validation. (applies ADR-007) consistency-03: record why gpt-6-astra keeps five efforts and does not adopt the sixth (`ultra`) advertised by the native-CLI fixture — that fixture describes the reverse-ingress leg, not the /responses leg.
isOpenaiModelName hand-listed the registry's family names (sol|terra|luna| astra) in a regex with no structural link to MODEL_REGISTRY. validClaudeAlias is the only gate on codexIngress.claude.aliases, so a future registry family the regex never learned about could be claimed by a Claude alias and hijacked on the reverse leg — the mirror image of PF-007 (applies ADR-005/ADR-006, avoids PF-028). - buildOpenaiModelNamePredicate(registry) derives the family/id alternation at module load, escaping metacharacters and deduping; the gpt-, o[134], codex: arms, the (?:$|\[) suffix handling and the i flag are unchanged. An empty registry emits no alternation arm rather than an empty one that would match every name. - test/unit/claude-models.test.ts: registry invariant (every id and family is reserved), a synthetic-registry liveness control, and the validClaudeAlias test moved out of reverse-adapter.test.ts. - test/unit/config.test.ts: consumer-level coverage for the codexIngress.claude.aliases refine, which was previously unasserted.
consistency-02: ModelRow mirrored every other ModelEntry field except reasoningEfforts, so a model's narrowed effort vocabulary never reached `subswitch models --json` — the machine-readable registry. A consumer reading it would offer gpt-6-astra the two backend-wide efforts it rejects. Add the field and emit it by conditional spread only when the entry declares one: absent means DEFAULT_REASONING_EFFORTS applies, so filling in the default would claim a narrowing that is not there and re-spell a vocabulary that has one home (avoids PF-014, PF-004). Additive, so schemaVersion stays 1. formatModelsReport is left alone — its AliasTableRow shows neither family, preview nor retired, so it is not the view that mirrors every optional field. reliability-09: buildDeps destructured the five known diagnostics by name, so 51ff365's unknownReasoningEfforts was returned and tested but surfaced nowhere at runtime. Add the field and one loop mirroring its neighbours, annotating the id with the offending values exactly as ambiguous_family annotates its provider list. testing-05: the routableModelCount canary compared the function against an inline re-spelling of its own filter, so both sides moved together and it stayed green when this PR took the count 4 -> 5. Pin the literal 5 instead — one side of a control must be independent of the code under test (avoids PF-011). Proven RED at 4. testing-03: the astra `models --json` test dropped the file's assertion messages and left `gen` unpinned, though astra is the registry's only single-element gen tuple. Both restored, plus the omission half of the conditional spread on gpt-5.6-sol. consistency-13: the Astra effort test bundled three behaviours into one `it` and omitted the `result.value.effort` assertion its neighbours make. Split into forward / drop / unnarrowed-neighbour, restore the surrounding fixture style, and assert the outcome's effort on both the accept and the drop path — the body alone does not pin what the handler logs. Live ids stay: translateRequest threads MODEL_REGISTRY internally and takes no registry parameter.
…nt-type The headerless-SSE eligibility predicate re-decoded and re-parsed the whole request body inside the upstream response callback, duplicating the parse bodyShape had already done. Codex bodies carry entire conversation histories, so that sat on the time-to-first-byte path. The body is now parsed exactly once after it is read; the predicate consumes the resulting boolean. new URL(path, ...) sat outside the try that wrapped only JSON.parse, inside the response callback and therefore outside the promise handleRequest awaits: a malformed request target (POST // HTTP/1.1) escaped the handler's .catch() and killed the recorder on uncaughtException. The parse now lives inside the guard - a malformed target is ineligible, not fatal - and the CLI entry point registers an uncaughtException logger so an unattended capture survives a stray throw. The factory stays import-safe and touches no process globals. The four loose primitives - including two adjacent transposable strings - collapse into one named StreamProbe, with RESPONSES_PATH_SUFFIX, PATH_PARSE_BASE, SSE_CONTENT_TYPE and isSuccessStatus replacing the inlined magic values, and the arm dispatch reading isSseResponse(probe). Reading content-type through an Array.isArray branch narrowed to never, so never[0] was any and the whole contentType expression collapsed to any, leaving .toLowerCase() and === "" unchecked. IncomingHttpHeaders types that header string | undefined, so the impossible branch is gone. All four PF-013 clauses are preserved exactly and ADR-010 byte-transparency is unchanged. Refs: performance-02, complexity-02, security-02, typescript-01, complexity-07
…ailure The capture arm is chosen from the REQUEST shape (PF-013), so a 2xx response with no blank-line delimiter reached it and was retained whole by `lineBuf`, with `split` re-scanning the accumulation per chunk. Measured: a 16 MiB undelimited body took the recorder heap from 10 MiB to 232 MiB in 1002 ms. MAX_SSE_EVENTS caps printing, not memory. - Hold undelivered capture text as an array of segments, joined only on the chunk that completes an event, mirroring the production `createSseParser`. Bound it at MAX_SSE_BUFFER_CHARS (4 Mi UTF-16 code units); on overflow print one `<cap:sse-residual>` notice, drop the residual and stop parsing — while every byte keeps flowing to the client untouched (ADR-010, ADR-012). Same body now: 6.1-7.2 MiB growth in 33 ms, bytes byte-identical. - Decode with one StringDecoder per response, so a multi-byte sequence split across two writes no longer becomes two replacement characters. - Write to the client before any capture work and honour `res.write`'s return value, pausing the upstream until drain. Both drain and close detach the pair, so a socket that closes mid-drain cannot strand the upstream paused (PF-009 class). - Give teardown its own predicate instead of the `headersSent` reply latch (PF-022): before the reply synthesize a status, after it destroy the socket. Applied to both arms' upstream-error paths and the outer catch — a mid-stream upstream death left the client's chunked body unterminated. - Destroy the upstream request when the client disconnects mid-stream; the handler promise still settles exactly once. All five controls proven RED against the prior implementation first (PF-011); the seven pre-existing tests stayed green untouched.
Four recorder-hygiene defects, each proven RED first (PF-011); the twelve pre-existing tests stayed green untouched. - `event.type` and the `usage` numbers came off the UPSTREAM stream and were interpolated raw into the capture, contradicting the file's own header invariant that message text is never printed. One crafted event forged four capture lines: a fake `REQUEST HEADERS:` block carrying a plausible bearer token, a fake `[99] type=response.completed`, and an ANSI `\x1b[2K` that erases the real prefix in a terminal. Captures are hand-converted into fixtures, so a forged line is a live hazard. Both now pass through capture hygiene at the single print site: `renderCaptureToken` strips the C0/DEL/C1 range and caps length at 128 chars, exactly as `renderToken` does in src/logger.ts (ADR-008), and `usage` is projected down to its finite numbers with bounded depth and field count, so a crafted string value is dropped rather than rendered. Nested `input_tokens_details.cached_tokens` survives. - Hoist the nested ternary-in-template into `formatUsageSuffix`. - `requestSeq` stayed module-global after the factory made the module multi-instance, so several recorders in one process shared one counter and their `REQUEST #n` labels depended on execution order. The counter now lives in the factory closure and the per-request `seq` is threaded into `handleRequest`. - The factory accepted any upstream URL while `buildForwardHeaders` forwards `authorization`, `chatgpt-account-id` and `cookie` verbatim — the `REDACT_*` sets govern the transcript, not the wire — so a recorder pointed at `http://evil.test` shipped the user's ChatGPT OAuth token in cleartext to an arbitrary host. ADR-009 now applies at construction, before anything listens: https to chatgpt.com, or any scheme to loopback, judged by src/config.ts's exact-form `isLoopbackHost` rather than a hand-rolled prefix test (PF-026: `startsWith("127.")` admits `127.0.0.1.evil.test`). Any other https host needs CODEX_RECORDER_ALLOW_INSECURE_UPSTREAM=1; cleartext http off loopback is refused outright. The CLI catches the refusal and exits 1 rather than letting the `uncaughtException` guard swallow it. - Add `e2e/capture/**/*.ts` to tsconfig.json `include`: the recorder was type-checked only transitively, via the test's import. tsconfig.build.json stays src-only, so it still never reaches dist. Its header claimed exclusion from the typecheck program and the npm test globs, false on both counts (PF-023); rewritten to state what is actually true, along with the CLI banner and header text describing the upstream override.
…pling The headerless-SSE eligibility rule had no negative test for the method clause: deleting `method === "POST"` left the suite green because every other negative was stopped by a cheaper clause first. `recordEvent`'s 200-event print cap and its terminal-usage latch were prose only — fixtures emitted one or two events, so the cap branch never ran and both re-coupling the usage print to the cap and deleting the latch stayed green. - add a headerless 2xx GET /responses carrying `stream: true`, which clears every other clause so the method is the only one left refusing - add a 205-event fixture (an independent literal — importing MAX_SSE_EVENTS would make the test agree with the code rather than pin it) with a completion on each side of the cap, asserting 200 printed lines, one cap notice, one TERMINAL USAGE carrying the first value, and byte-for-byte forwarding - add the post-cap-only completion case, so usage that arrives past the print cap is pinned as terminal accounting rather than event detail - bound every client/close helper at 5 s, so a hang fails as a named assertion at its call site instead of the anonymous 30 s suite timeout Mutations proven RED, each reverted: `method === "POST"` → `true`; `!terminalUsagePrinted` dropped (usage printed twice); usage print moved inside the cap branch (post-cap usage lost). Refs PF-011, PF-012, PF-013, ADR-010
The recorder section described a CLI that no longer tells the whole story. Three drifts, each checked against the current code (PF-023): - the output-format list never gained `TERMINAL USAGE:`, which the integration suite asserts literally, and its cap sentence claimed the 200-event cap governs the usage print — the code deliberately decouples them, so a completion past the cap still reports usage - `<cap:sse-residual>` had a Bounds row but no entry in the output list, and the `usage` object is not printed "verbatim": only finite numbers survive, keys and types pass capture hygiene, and booleans in a body shape render as `<bool:…>` - the module is import-safe and exports `createCodexRecorderServer` / `RecorderOutput`, but only the `npx tsx` invocation was documented; add a "Using the recorder from a test" subsection covering the current three-argument signature, the ephemeral-port dance, and the output sink - document the ADR-009 upstream rule the factory enforces: loopback on any scheme, `https://chatgpt.com` by default, any other https host behind `CODEX_RECORDER_ALLOW_INSECURE_UPSTREAM=1` / `allowInsecureUpstream`, and cleartext off-loopback refused outright, fatally and before listening Refs ADR-009, ADR-010, PF-013, PF-023, PF-026
Behaviour-preserving cleanup across the twelve issue-resolution commits,
written by seven agents that each spelled the shared parts differently.
- recorder: both response arms ended a mid-stream upstream failure with the
same three lines and the same client-visible message literal; name the rule
once as `failUpstreamStream` so the two arms cannot drift apart on status or
message. The connection-error path keeps its own wording and stays inline.
- recorder: `forward()` registered two byte-identical closures, one per event,
plus a third to detach them. One listener on both events says the same thing
and makes the accompanying comment literally true.
- recorder: drop three `err as Error` assertions. Node types an `"error"`
listener as `(err: Error)` on both Readable and Writable, so the casts
asserted a type the compiler already had.
- recorder: `const { println } = printer`, matching how `handleRequest`
destructures the same object two functions away.
- comments: replace three file:line references with the symbol names they
point at. All three were accurate today and silently rot on the next edit.
- tests: `send()` carried a `method = "POST"` default that neither call site
uses — both pass the method explicitly, which is what `post()` exists for.
- tests: `SYNTHETIC` -> `SYNTHETIC_REGISTRY`, the name the sibling synthetic
registry in claude-models.test.ts already uses for the same device.
No production behaviour, transcript output, log field, or assertion changes.
tsc clean; the eight in-scope files stay at 297 passing.
…order SSE capture Documents gpt-6-astra's per-model reasoningEfforts, DEFAULT_REASONING_EFFORTS/ reasoningEffortsForModel/unknownReasoningEfforts, buildOpenaiModelNamePredicate deriving the claude-ingress alias gate from the registry, and the PF-013 fix in codex-recorder.ts (headerless-SSE capture eligibility, MAX_SSE_BUFFER_CHARS, upstream vetting). Cross-references the same additions into the cli-ux KB.
This was referenced Sep 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
1 participant
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
gpt-6-astra/astrarouting, discovery, reverse alias protection, and model-specific effort validationContent-TypeVerification
npm run checknpm run buildbash scripts/smoke-tarball.shCloses #50
Closes #21