fix(sse): accept unspaced data: fields across six parsers (#1170) - #1194
Conversation
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
|
📝 WalkthroughWalkthroughThe PR adds a roadmap for untouched bugs and contributor PR replacements. It also adds shared SSE field helpers, updates six parsers to accept unspaced fields, and adds unit and integration coverage. ChangesUntouched-bug remediation planning
Unspaced SSE field support
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260807_untouched_bug_stack/000_plan.md`:
- Around line 11-21: Revise the CI admission backlog wording so approval of the
open-PR subset is described as a prerequisite only for dispositions addressing
that backlog. Remove the implication that it gates every disposition, while
preserving the separate, independent handling of defects with no PR.
- Around line 63-72: The phase 010 roadmap entry must document the complete
`#1170` parser boundary rather than relying on call-site counts. Expand it to
cover ProviderAdapter.parseStream and optional parseResponse, define the exact
sseFieldValue and sseFieldOffset semantics, and specify Claude event: handling;
add CRLF and multiline exclusions under Out of scope, and revise audit item 1 to
identify them as intentional scope boundaries rather than unresolved blockers.
In `@devlog/_plan/260807_untouched_bug_stack/010_sse_unspaced_data_fields.md`:
- Around line 33-52: Define both shared helpers beside the SSE decoder:
sseFieldValue(line, field) should return null for non-matching or colonless
fields and otherwise return the field value after removing at most one leading
ASCII space; sseFieldOffset(line, field) should return the corresponding value
start offset or null under the same matching rules. Update the raw-frame tests
to cover non-matching, unspaced, and one-space fields, and ensure the outbound
parser uses sseFieldOffset without changing reserveTransient, commitRetained,
releaseRetained, or existing byte totals.
- Around line 87-103: Add tests in tests/sse-unspaced-data-fields.test.ts for
the shared SSE parsing helper, covering field boundaries, colonless fields, and
data values formatted as data:, data: , and data: x. Assert both each parsed
value and its returned offset, while leaving the existing parser-level tests
unchanged.
In `@devlog/_plan/260807_untouched_bug_stack/030_windows_acl_harden_envelope.md`:
- Around line 29-34: Bound the aggregate ACL hardening wait in loadConfig rather
than allowing three sequential harden calls to each consume the full per-call
timeout. Use one shared startup deadline or derive each call’s remaining budget
from it, while preserving HARDEN_DEADLINE_MAX_MS, OPENCODEX_ACL_TIMEOUT_MS,
clamping, and the shared-envelope structure. Add tests covering the override
path and aggregate timeout behavior.
In
`@devlog/_plan/260807_untouched_bug_stack/040_npm_cache_preflight_replacement.md`:
- Around line 42-46: Update the npm cache preflight design in “bounded Unix
cache inspection” to validate effective read, directory traversal, and write
access in addition to lstat and ownership checks, while retaining bounded
probing and safe reason codes. Ensure inaccessible parents and mode-restricted
entries cause the preflight to abort rather than be skipped, and add a
regression test covering a current-user-owned but inaccessible cache path.
- Around line 68-79: Expand the listed behavioral tests to cover an anchorless
Windows path and a username containing spaces. In the persisted-log tests,
assert both stored fields and emitted log lines contain sanitized values with no
original path or username PII, alongside the existing profile/cache and UID/GID
cases.
In
`@devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.md`:
- Around line 82-90: Make every advisory use of
currentExternalCodexModelProvider non-throwing by adding a readiness/helper
wrapper that catches config-read failures and returns an unverifiable result.
Replace or guard the call sites in src/cli/index.ts startup and cleanup paths
and in the relevant src/codex/inject.ts flows, including install/start/ensure
and syncCleanup, so failures preserve successful command exit behavior. Add
coverage for unreadable or racing CODEX_CONFIG_PATH during startup and cleanup,
ensuring warnings disclose neither proxy URLs nor credentials.
- Around line 94-97: Update the stack planning entry around `#1155` to identify
`#1095` as its required integration base, document the applicable rebase/order
sequence, and remove the “free-standing” characterization. State that the named
web-search regression tests must be rerun on the post-#1095 tree, preserving
both the buffered-upstream policy and lease-release fix across the overlapping
src/server/responses/core.ts changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9d3e1062-c162-4b6d-850c-00a82070ce32
📒 Files selected for processing (16)
devlog/_plan/260807_untouched_bug_stack/000_plan.mddevlog/_plan/260807_untouched_bug_stack/010_sse_unspaced_data_fields.mddevlog/_plan/260807_untouched_bug_stack/020_routed_reasoning_effort.mddevlog/_plan/260807_untouched_bug_stack/030_windows_acl_harden_envelope.mddevlog/_plan/260807_untouched_bug_stack/040_npm_cache_preflight_replacement.mddevlog/_plan/260807_untouched_bug_stack/050_adopt_as_is_replacements.mddevlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.mdsrc/adapters/openai-chat.tssrc/chat/outbound.tssrc/claude/outbound.tssrc/lib/sse-decoder.tssrc/server/claude-messages.tssrc/web-search/parse.tstests/claude-messages-endpoint.test.tstests/claude-outbound.test.tstests/sse-unspaced-data-fields.test.ts
| The first is a **CI admission backlog**. Eight bug-fix PRs were reported as | ||
| "never ran CI", which reads like contributor neglect but is not: 524 workflow | ||
| runs sat in `action_required`, waiting on maintainer approval. Thirty-nine of | ||
| them belonged to branches with an open PR. The readiness gate cannot verify the | ||
| `ci` check on a run that was never allowed to start, so those PRs could not | ||
| leave draft no matter what their authors did. Approving the open-PR subset is | ||
| the precondition for every disposition below; approving all 524 is not, because | ||
| most belong to branches already merged or abandoned. | ||
|
|
||
| The second is a set of **defects with no PR at all** — issues where a reporter | ||
| filed evidence and nothing was ever opened against it. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict the CI approval dependency to existing PRs.
Line 16 makes approval of the open-PR subset a precondition for every disposition in the table. Lines 20-21 define a separate backlog of defects with no PR, and Lines 40-42 include new fixes for that backlog. Those fixes do not depend on approving action_required runs.
Change the wording to limit this prerequisite to dispositions for the open-PR CI backlog.
Proposed wording
-Approving the open-PR subset is the precondition for every disposition below;
+Approving the open-PR subset is the precondition for dispositions concerning the open-PR CI backlog;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The first is a **CI admission backlog**. Eight bug-fix PRs were reported as | |
| "never ran CI", which reads like contributor neglect but is not: 524 workflow | |
| runs sat in `action_required`, waiting on maintainer approval. Thirty-nine of | |
| them belonged to branches with an open PR. The readiness gate cannot verify the | |
| `ci` check on a run that was never allowed to start, so those PRs could not | |
| leave draft no matter what their authors did. Approving the open-PR subset is | |
| the precondition for every disposition below; approving all 524 is not, because | |
| most belong to branches already merged or abandoned. | |
| The second is a set of **defects with no PR at all** — issues where a reporter | |
| filed evidence and nothing was ever opened against it. | |
| The first is a **CI admission backlog**. Eight bug-fix PRs were reported as | |
| "never ran CI", which reads like contributor neglect but is not: 524 workflow | |
| runs sat in `action_required`, waiting on maintainer approval. Thirty-nine of | |
| them belonged to branches with an open PR. The readiness gate cannot verify the | |
| `ci` check on a run that was never allowed to start, so those PRs could not | |
| leave draft no matter what their authors did. Approving the open-PR subset is the precondition for dispositions concerning the open-PR CI backlog; | |
| approving all 524 is not, because most belong to branches already merged or abandoned. | |
| The second is a set of **defects with no PR at all** — issues where a reporter | |
| filed evidence and nothing was ever opened against it. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260807_untouched_bug_stack/000_plan.md` around lines 11 - 21,
Revise the CI admission backlog wording so approval of the open-PR subset is
described as a prerequisite only for dispositions addressing that backlog.
Remove the implication that it gates every disposition, while preserving the
separate, independent handling of defects with no PR.
| ## Roadmap | ||
|
|
||
| Implementation phases, one decade doc each, one PABCD cycle each: | ||
|
|
||
| - `010` — #1170 unspaced SSE field parsing (6 call sites, 2 shared primitives) | ||
| - `020` — #1100 routed reasoning-effort propagation | ||
| - `030` — #1156 Windows ACL harden envelope | ||
| - `040` — #557 replacement: npm cache preflight + log sanitization | ||
| - `050` — adopt-as-is PR replacements (#1159, #1171) | ||
| - `060` — adopt-with-changes PR replacements (#1155, #1152, #1169) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 \
'parseStream|parseResponse|sseFieldValue|sseFieldOffset|event:' \
src testsRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== plan file slice =="
fd -a '000_plan\.md$' . | sed 's#^\./##' | head -20
for f in $(fd '000_plan\.md$' devlog); do
echo "--- $f ---"
wc -l "$f"
sed -n '1,15p;55,125p' "$f"
done
echo "== adapter/decoder locations =="
fd 'base\.ts$|sse-decoder\.ts$' src
for f in $(fd 'base\.ts$|sse-decoder\.ts$' src); do
echo "--- $f ---"
sed -n '1,90p' "$f"
done
echo "== `#1170` references =="
rg -n "`#1170`|1170|unspaced|sseFieldValue|sseFieldOffset|parseResponse|parseStream" src tests devlog --glob '!tests/chat-completions-endpoint.test.ts' --glob '!tests/**' | head -250Repository: lidge-jun/opencodex
Length of output: 1611
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== exact plan file =="
ls -l devlog/_plan/260807_untouched_bug_stack/000_plan.md
wc -l devlog/_plan/260807_untouched_bug_stack/000_plan.md
sed -n '1,140p' devlog/_plan/260807_untouched_bug_stack/000_plan.md
echo "== relevant source files =="
fd -a 'base\.ts$|sse-decoder\.ts$' src | sed 's#^\./##'
for f in $(fd 'base\.ts$|sse-decoder\.ts$' src); do
echo "--- $f ---"
sed -n '1,120p' "$f"
done
echo "== targeted `#1170` and SSE parsing references =="
rg -n -C 3 '`#1170`|1170|unspaced|sseFieldValue|sseFieldOffset|parseResponse|parseStream' \
src tests devlog/_plan/260807_untouched_bug_stack/000_plan.md \
| sed -n '1,260p'Repository: lidge-jun/opencodex
Length of output: 36474
Document the full #1170 parser boundary in phase 010.
devlog/_plan/260807_untouched_bug_stack/000_plan.md:67 only says “6 call sites, 2 shared primitives,” while ProviderAdapter has both parseStream and optional parseResponse in src/adapters/base.ts:17-42. Add explicit coverage for both parser paths, the exact sseFieldValue/sseFieldOffset semantics, and Claude event: handling so the phase cannot pass by call-site count alone. Add CRLF/multiline exclusions to Out of scope and rewrite audit item 1 to record them as intentional scope boundaries, not unresolved blockers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260807_untouched_bug_stack/000_plan.md` around lines 63 - 72,
The phase 010 roadmap entry must document the complete `#1170` parser boundary
rather than relying on call-site counts. Expand it to cover
ProviderAdapter.parseStream and optional parseResponse, define the exact
sseFieldValue and sseFieldOffset semantics, and specify Claude event: handling;
add CRLF and multiline exclusions under Out of scope, and revise audit item 1 to
identify them as intentional scope boundaries rather than unresolved blockers.
| Export one pure helper from `src/lib/sse-decoder.ts`, beside the decoder whose | ||
| semantics it mirrors: | ||
|
|
||
| ```ts | ||
| export function sseFieldValue(line: string, field: string): string | null; | ||
| ``` | ||
|
|
||
| Returns `null` when the line is not that field. Otherwise returns the value with | ||
| at most one leading ASCII space removed — one, not `trimStart()`, because | ||
| leading whitespace beyond the first character is payload. | ||
|
|
||
| Then replace the strict prefix checks with calls to it — **six sites, not | ||
| five**. `src/claude/outbound.ts` has two independent parsers (`:591-605` and | ||
| `:864-865`); both need it, and both need it for `event` as well as `data`, since | ||
| the `event: ` check carries the identical defect. | ||
|
|
||
| The `:591-605` site is the delicate one: it reserves and commits translator | ||
| budget per fragment, so the edit must change only which offset the fragment | ||
| starts at, leaving every `reserveTransient` / `commitRetained` / | ||
| `releaseRetained` call and its byte accounting untouched. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define both shared SSE helper contracts.
The plan specifies sseFieldValue, but the PR objective also requires sseFieldOffset for src/claude/outbound.ts:591-605. That parser performs byte-budget accounting. A value-only API leaves offset calculation at the call site and can duplicate matching logic or miscount the optional space.
Specify both return contracts for non-matching, unspaced, one-space, and colonless fields. Keep the raw-frame test tied to these contracts and to the existing budget totals.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 47-47: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260807_untouched_bug_stack/010_sse_unspaced_data_fields.md`
around lines 33 - 52, Define both shared helpers beside the SSE decoder:
sseFieldValue(line, field) should return null for non-matching or colonless
fields and otherwise return the field value after removing at most one leading
ASCII space; sseFieldOffset(line, field) should return the corresponding value
start offset or null under the same matching rules. Update the raw-frame tests
to cover non-matching, unspaced, and one-space fields, and ensure the outbound
parser uses sseFieldOffset without changing reserveTransient, commitRetained,
releaseRetained, or existing byte totals.
| ## Tests | ||
|
|
||
| Each asserts real content arrives rather than a silently empty turn. | ||
|
|
||
| | File | Test | Assertion | | ||
| |---|---|---| | ||
| | `tests/openai-chat-hardening.test.ts` | `accepts unspaced data fields and finish_reason without DONE (#1170)` | text delta, `done`, stop reason, usage | | ||
| | `tests/chat-completions-endpoint.test.ts` | `collectChatCompletion accepts unspaced data fields` | final `message.content` | | ||
| | `tests/claude-outbound.test.ts` | `collectAnthropicMessage accepts unspaced event and data fields` | completed text and stop reason | | ||
| | `tests/web-search-parse.test.ts` | `parseSidecarSSE accepts unspaced data fields` | completed text and source extraction | | ||
| | `tests/claude-outbound.test.ts` | `raw-frame parser accepts unspaced event and data fields` | the `:591-605` parser, with budget accounting intact | | ||
| | `tests/claude-messages-endpoint.test.ts` | `usage extraction accepts unspaced data fields` | the `src/server/claude-messages.ts:174` site: usage extraction and finalization | | ||
| | `tests/claude-outbound.test.ts` | `characterizes CRLF framing and multiline data joining` | records today's behavior for the two deferred defects | | ||
|
|
||
| Every one of these fails before the change: the frames are dropped and the | ||
| assertions see empty output. Each of the six production call sites has a test | ||
| that covers it. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add direct regression tests for the shared helpers.
The table lists parser-level tests, but it does not name tests/sse-unspaced-data-fields.test.ts. Add direct cases for field boundaries, colonless fields, data:, data: , and data: x. Cover both the returned value and the returned offset.
This isolates regressions in the shared rule before they affect six parser implementations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260807_untouched_bug_stack/010_sse_unspaced_data_fields.md`
around lines 87 - 103, Add tests in tests/sse-unspaced-data-fields.test.ts for
the shared SSE parsing helper, covering field boundaries, colonless fields, and
data values formatted as data:, data: , and data: x. Assert both each parsed
value and its returned offset, while leaving the existing parser-level tests
unchanged.
| `src/lib/windows-secret-acl.ts:234` — raise `HARDEN_DEADLINE_DEFAULT_MS` from | ||
| `5_000` to `30_000`. | ||
|
|
||
| Keep everything else: the 60-second `HARDEN_DEADLINE_MAX_MS` cap, the | ||
| `OPENCODEX_ACL_TIMEOUT_MS` override, the clamp, and the shared-envelope | ||
| structure. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound the aggregate startup wait, including the override path.
loadConfig() performs three harden calls sequentially. With the proposed 30-second default, all three can consume about 90 seconds. With OPENCODEX_ACL_TIMEOUT_MS clamped to the 60-second maximum, the aggregate can reach about 180 seconds.
The documented bounds cover only the default path. Add an overall startup deadline or derive each call's budget from one shared loadConfig() deadline. Add a test for the override and aggregate timeout behavior.
Also applies to: 54-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260807_untouched_bug_stack/030_windows_acl_harden_envelope.md`
around lines 29 - 34, Bound the aggregate ACL hardening wait in loadConfig
rather than allowing three sequential harden calls to each consume the full
per-call timeout. Use one shared startup deadline or derive each call’s
remaining budget from it, while preserving HARDEN_DEADLINE_MAX_MS,
OPENCODEX_ACL_TIMEOUT_MS, clamping, and the shared-envelope structure. Add tests
covering the override path and aggregate timeout behavior.
| New `src/update/npm-cache-preflight.mjs`: bounded Unix cache inspection | ||
| returning structured reason codes. `lstat` nested symlinks and verify ownership, | ||
| then **skip traversal** rather than rejecting — normal `_npx`, `node_modules`, | ||
| and `.bin` links must not block an update. Never surface arbitrary worker text | ||
| into logs. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Check access, not only ownership.
The plan says inaccessible entries must abort the update, but the implementation description only specifies lstat and ownership checks. A current-user-owned path can still have mode 000 or an inaccessible parent. lstat can succeed while npm cannot read or write the cache.
Add bounded read, traversal, and write checks, or an equivalent safe probe. Include a current-user-owned but inaccessible regression test. Otherwise the preflight can pass and the update can still stop the proxy before installation fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@devlog/_plan/260807_untouched_bug_stack/040_npm_cache_preflight_replacement.md`
around lines 42 - 46, Update the npm cache preflight design in “bounded Unix
cache inspection” to validate effective read, directory traversal, and write
access in addition to lstat and ownership checks, while retaining bounded
probing and safe reason codes. Ensure inaccessible parents and mode-restricted
entries cause the preflight to abort rather than be skipped, and add a
regression test covering a current-user-owned but inaccessible cache path.
| ## Tests | ||
|
|
||
| - `tests/update-npm-cache-preflight.test.ts` (new): foreign or inaccessible | ||
| entries abort before stop; normal nested symlinks pass without target | ||
| traversal; timeout and malformed worker output fail closed; the Windows skip | ||
| does not spawn npm. | ||
| - `tests/update-stop-first.test.ts:21`: the gate runs before shutdown. | ||
| - `tests/update-job.test.ts`: persisted logs contain no profile path, cache | ||
| path, or UID/GID. | ||
|
|
||
| Tests must exercise behavior. #557 had source-text assertions that passed | ||
| without running the code they described. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make the required sanitization edge cases explicit in tests.
The objections require anchorless Windows paths and multi-word usernames, but the test list only names profile paths, cache paths, and UID/GID. Add fixtures for an anchorless Windows path and a username containing spaces. Assert that persisted fields and emitted log lines are sanitized.
A sanitizer that handles only rooted paths or single-token usernames could pass the current test description and still leak PII.
Also applies to: 81-91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@devlog/_plan/260807_untouched_bug_stack/040_npm_cache_preflight_replacement.md`
around lines 68 - 79, Expand the listed behavioral tests to cover an anchorless
Windows path and a username containing spaces. In the persisted-log tests,
assert both stored fields and emitted log lines contain sanitized values with no
original path or username PII, alongside the existing profile/cache and UID/GID
cases.
| One defect: `currentExternalCodexModelProvider()` can throw on an unreadable or | ||
| racing config, which turns a successful install into a failing command. An | ||
| advisory probe must never do that. | ||
|
|
||
| Targets: `src/cli/index.ts:1035-1041`; a readiness helper beside `src/cli/`; | ||
| `src/codex/inject.ts:83-86`. | ||
|
|
||
| Catch probe failure as "unverifiable" and keep exit 0. Test the unreadable-config | ||
| path and assert the warning discloses neither the proxy URL nor credentials. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 '\bcurrentExternalCodexModelProvider\s*\(' \
src/cli/index.ts src/codexRepository: lidge-jun/opencodex
Length of output: 4185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '^src/(cli/index\.ts|codex/(inject|external).*?\.ts)$' || true
echo "== currentExternalCodexModelProvider definition =="
sed -n '70,90p' src/codex/inject.ts
echo "== externalCodexModelProvider references =="
rg -n -C 3 '\bexternalCodexModelProvider\b|CODEX_CONFIG_PATH|readFileSync|existsSync' src/codex/inject.ts src/codex | head -200
echo "== cli call sites =="
sed -n '175,210p' src/cli/index.ts
sed -n '272,290p' src/cli/index.ts
sed -n '355,370p' src/cli/index.ts
sed -n '400,412p' src/cli/index.ts
echo "== plan context =="
sed -n '70,95p' devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.mdRepository: lidge-jun/opencodex
Length of output: 18725
Guard every advisory currentExternalCodexModelProvider() use.
currentExternalCodexModelProvider() reads CODEX_CONFIG_PATH unguarded at src/codex/inject.ts:83-86, so existsSync() can race and readFileSync() can throw. src/cli/index.ts uses it directly at startup and cleanup: src/cli/index.ts:199, src/cli/index.ts:284, src/cli/index.ts:364, and src/cli/index.ts:406; src/codex/inject.ts also uses it at src/codex/inject.ts:1360 and src/codex/inject.ts:1505. Any of these calls can terminate a successful install/start/ensure or syncCleanup. Make the advisory helper non-throwing and replace/guard each of these call sites, then cover startup and cleanup, not only the install shim warning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.md`
around lines 82 - 90, Make every advisory use of
currentExternalCodexModelProvider non-throwing by adding a readiness/helper
wrapper that catches config-read failures and returns an unverifiable result.
Replace or guard the call sites in src/cli/index.ts startup and cleanup paths
and in the relevant src/codex/inject.ts flows, including install/start/ensure
and syncCleanup, so failures preserve successful command exit behavior. Add
coverage for unreadable or racing CODEX_CONFIG_PATH during startup and cleanup,
ensuring warnings disclose neither proxy URLs nor credentials.
| `#1155` overlaps `src/server/responses/core.ts` with the #1095 rewrite, which is | ||
| deliberately out of this unit — so within this stack it is free-standing. | ||
| `#1152` and `#1169` touch disjoint files. Order: #1152, #1169, #1155, putting | ||
| the largest surface last. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file =="
git ls-files | rg '060_adopt_with_changes_replacements\.md|responses/core\.ts|1095|1155|1152|1169' || true
echo
echo "== plan lines 80-110 =="
if [ -f "devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.md" ]; then
cat -n "devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.md" | sed -n '80,110p'
fi
echo
echo "== repo references to issue/PR numbers and paths =="
rg -n "`#1095`|`#1155`|`#1152`|`#1169`|src/server/responses/core\.ts|responses/core\.ts|lease|buffered|policy" -S . --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 200Repository: lidge-jun/opencodex
Length of output: 21817
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== plan file line count and nearby context =="
wc -l "devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.md"
sed -n '1,120p' "devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.md" | cat -n
echo
echo "== source file around core.ts =="
wc -l "src/server/responses/core.ts"
sed -n '1,260p' "src/server/responses/core.ts" | cat -nRepository: lidge-jun/opencodex
Length of output: 18216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== core.ts relevant ranges =="
sed -n '2500,2545p' "src/server/responses/core.ts" | cat -n
sed -n '1,80p' "src/server/responses/core.ts" | cat -n
echo
echo "== response/replay/release related code outside core.ts =="
rg -n "previousResponse|release|lease|Intercepted|buffered|web-search|openai-responses|parseResponse" src tests --glob '!src/server/responses/core.ts' | head -n 220
echo
echo "== git refs files if present =="
git rev-parse --show-toplevel
git rev-parse HEAD
git rev-parse origin/main 2>/dev/null || true
git diff --name-only HEAD~1..HEAD 2>/dev/null || trueRepository: lidge-jun/opencodex
Length of output: 28624
🌐 Web query:
GitHub lidge-jun opencodex #1095 responses/core.ts
💡 Result:
The query refers to the opencodex repository maintained by lidge-jun, which provides a universal proxy allowing users to run OpenAI Codex and Claude Code using various LLM backends [1][2][3]. Regarding responses/core.ts (located at src/server/responses/core.ts), this file contains logic for handling response streams and compaction requests [4]. Specifically, it manages the routing of compaction tasks, distinguishing between native passthrough providers and those that require synthetic compaction (a bridge-side process) [4]. While there is no single "issue #1095" currently listed in the repository's public issue tracker (the issue count is significantly lower, typically under 100 open issues, with recent activity reaching into the 600s [5][6]), the file src/server/responses/core.ts has been the subject of several technical discussions and fixes related to compaction logic [4]. For example, developers have addressed bugs where openai-responses providers incorrectly assumed native compaction support, leading to potential failures [4]. Other related commits in the src/server/responses/ path have addressed issues with multi-agent guidance, reasoning turn folding, and developer message injection [7][8][9]. If you are looking for a specific fix or behavior related to responses/core.ts, it is likely associated with the broader development effort to standardize response handling and multi-agent compatibility within the project [4][7][8].
Citations:
- 1: https://github.com/lidge-jun/opencodex
- 2: https://opencodex.me/
- 3: https://deps.dev/project/github/lidge-jun%2fopencodex
- 4: Remote compaction v2 fatals on API-key-based
openai-responsesproviders — passthrough assumes native compaction support #422 - 5: https://github.com/lidge-jun/opencodex/issues
- 6: Unexpected interruption continues to occur #624
- 7: 8db9f27
- 8: 91b14bb
- 9: 7c78c5b
🌐 Web query:
GitHub lidge-jun opencodex 1095 src/server/responses/core.ts
💡 Result:
In the lidge-jun/opencodex repository, src/server/responses/core.ts is a core module that serves as the mutually-recursive request engine for the project's Responses API handling [1]. This file manages the primary logic for processing and routing responses within the proxy [1]. Key aspects of this module include: - Core Engine Functionality: It orchestrates the handling of responses through functions such as handleComboResponses and handleResponses [1]. It also manages internal outcome and authentication helpers, including codexLogAccountId, usesCodexForwardPoolAuth, and codexForwardTerminalOutcomeRecorder [1]. - Passthrough Logic: The file contains logic for managing SSE (Server-Sent Events) passthrough, including selecting between different relay shapes (such as tee-based background inspection vs. gated eager relay) [2]. - Compaction Logic: It implements compaction requests for specific providers (such as those using openai-responses), where it manages routed versus passthrough execution paths to determine how prompt compaction and tool stripping are handled [3]. The file is part of a modularized responses architecture introduced to split the original src/server/responses.ts into a set of leaf modules and a core engine [1]. It acts as a central engine that imports leaf modules like collaboration, encrypted-payload, and fetch-helpers, while being re-exported through the src/server/responses.ts facade to maintain the public API surface [1].
Citations:
- 1: https://cephalochromoscope.net/a24b20ff-f4e3-42c2-b5b5-4df14c0767e5
- 2: https://cephalochromoscope.net/14b82f82-614e-4114-8b5b-c49ec8dc0056
- 3: Remote compaction v2 fatals on API-key-based
openai-responsesproviders — passthrough assumes native compaction support #422
🌐 Web query:
GitHub lidge-jun opencodex web-search loop bounded-body releaseTranslatedEvent
💡 Result:
OpenCodex (lidge-jun/opencodex) is a lightweight local proxy designed to bridge Codex, Claude Code, and other similar applications with various LLM providers, allowing them to use models outside their native defaults [1][2][3]. The technical terms you mentioned are part of OpenCodex's web-search sidecar implementation [4][5]: web-search: This refers to a sidecar feature in OpenCodex that provides a synthetic web-search tool for routed models [4][5]. When a model triggers this tool, OpenCodex manages an agentic loop to execute the search via a configured backend (e.g., GPT-5.6 or Claude Sonnet) and feeds the results back to the model [4][5]. bounded-body: This is a mechanism used within OpenCodex, particularly in the image proxy, to handle and limit request body sizes to prevent memory exhaustion and ensure safety [6]. releaseTranslatedEvent: This is associated with the event-driven architecture of OpenCodex. OpenCodex bridges model-native request formats (like Codex's Responses API) into a unified internal event stream that handles streaming, tool calls, and reasoning tokens [1][3]. Specifically, in the context of the web-search sidecar and bridge logic, events such as tool calls (e.g., web_search_call) are translated into standardized server_tool_use and tool_result frames to ensure compatibility across different vendor protocols (like Anthropic Messages vs. OpenAI-compatible endpoints) [7][8]. The loop behavior you referenced describes the web-search sidecar's execution pattern: OpenCodex runs the model in a loop, processing search tool calls until the model provides a final answer or reaches a configured limit (such as maxSearchesPerTurn, default 3) [4]. The system uses various timeout contracts—such as the routedModelStallTimeoutMs—to manage these iterations and ensure the connection remains healthy during extended search operations [4][6]. Top sources for further exploration: - Official Website/Docs: https://opencodex.me/ [2] - Architecture Reference: https://opencodex.me/reference/architecture/ [5] - Sidecars Guide: https://opencodex.me/guides/sidecars/ [4] - GitHub Repository: https://github.com/lidge-jun/opencodex [1]
Citations:
- 1: https://github.com/lidge-jun/opencodex
- 2: https://opencodex.me/
- 3: https://dev.to/creeta/codex-cli-dropped-chat-wire-opencodex-picks-up-the-routing-2emb
- 4: https://opencodex.me/guides/sidecars/
- 5: https://opencodex.me/reference/architecture/
- 6: 1f146ad
- 7: 82ec47f
- 8: da236d0
Tie #1155 to the #1095 integration base.
This stack calls #1155 free-standing at devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.md:94-97 despite src/server/responses/core.ts overlap. If #1095 lands first, #1155 can drift from the web-search response contract; if #1155 lands first, #1095 can drop the buffered-upstream policy or lease-release fix. Record the required base commit and rebase order, and rerun the named web-search regressions on the post-#1095 tree.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.md`
around lines 94 - 97, Update the stack planning entry around `#1155` to identify
`#1095` as its required integration base, document the applicable rebase/order
sequence, and remove the “free-standing” characterization. State that the named
web-search regression tests must be rerun on the post-#1095 tree, preserving
both the buffered-upstream policy and lease-release fix across the overlapping
src/server/responses/core.ts changes.
![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: a495bd8f41
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| This phase touches ACL/credential-permission handling, which requires explicit | ||
| security review under `MAINTAINERS.md` — CI green is not sufficient. Run |
There was a problem hiding this comment.
This new plan is stored under tracked devlog/_plan/ while it identifies an unfixed ACL/credential-permission defect and records the planned remediation; the same plan unit also includes dependency-install/log-sanitization triage. Because pushing devlog publishes these pre-fix details immediately, keep open security notes/patch plans in .tmp/ or a mktemp directory until the fix/outcome is public.
AGENTS.md reference: AGENTS.md:L61-L68
Useful? React with 👍 / 👎.
| const payload = line.slice(6).trim(); | ||
| const rawPayload = sseFieldValue(line, "data"); | ||
| if (rawPayload === null) return "continue"; | ||
| const payload = rawPayload.trim(); |
There was a problem hiding this comment.
When an upstream sends an empty data: event, for example as an SSE heartbeat before later JSON chunks, sseFieldValue now treats that formerly ignored unspaced line as an empty payload, and this handler immediately reaches JSON.parse("") and terminates the stream as malformed. That leaves a compliant unspaced producer broken; match the collectors by continuing on an empty payload before parsing.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
The space after the colon is optional in text/event-stream, so `data:{...}` is
as valid as `data: {...}`. Six parsers hardcoded the spaced form and silently
dropped every frame from a producer that omits it. To the user that looked like
a completed turn with no content.
The same wire format was already accepted on the relay path
(`sse-decoder.ts`, `relay.ts`, `google.ts`) and rejected on the adapter path.
That split is the bug.
Adds two primitives beside the decoder whose rule they mirror:
- `sseFieldValue(line, field)` for the five string-slicing parsers
- `sseFieldOffset(text, start, end, field)` for the live Claude relay, whose
translator-budget accounting reserves bytes by offset — materializing the
line first would allocate the string the budget exists to bound
Call sites fixed: `adapters/openai-chat.ts`, `chat/outbound.ts`,
`claude/outbound.ts` (two independent parsers, both `event` and `data`),
`web-search/parse.ts`, `server/claude-messages.ts`.
Both helpers strip at most one leading space, matching the decoder, so a
payload that legitimately begins with whitespace keeps the rest of it. Neither
trims: callers own that choice and some intentionally keep trailing bytes.
Deferred deliberately, not fixed here: `\r\n\r\n` frame delimiting and
multiline `data` joining without the spec's `\n` separator. Both are
frame-level rather than field-level and carry a different blast radius.
Verification: each new test was confirmed to fail with the fix reverted.
…ll six call sites
Audit found two real gaps in the previous commit.
`sseFieldValue("data", "data")` returned null while `decodeServerSentEvents`
treats a colonless field as an empty value (`colon < 0` -> valueStart =
line.length, sse-decoder.ts:240). Two helpers that mirror the decoder must not
disagree with it. Both now return the empty value, and `sseFieldOffset` returns
the end-of-line offset for the same case.
Regression coverage was also incomplete: `chat/outbound.ts` and
`claude/outbound.ts` had no unspaced test, so two of the six fixed call sites
were unverified. Both now have one, and each was confirmed to fail with its fix
reverted.
Also corrects the remaining stale "5 call sites" claims in the plan unit.
…elds (#1170) The offset-based parser inside responsesSseToAnthropicSse was covered only indirectly. This drives it head-on: the same six Responses frames in spaced and unspaced form, asserting identical event names, identical translated text, and identical budget accounting. The budget assertion compares the two paths rather than asserting zero. This translator leaves a small residue at stream end on the spaced path too (51 bytes for this fixture), so zero would be asserting something that was never true. Equality is the contract the offset arithmetic must satisfy. Confirmed to fail when the offset fix is reverted.
a495bd8 to
8662a09
Compare
1 participant
Summary
Closes #1170.
The space after the colon is optional in
text/event-stream: a compliant producer may senddata:{"choices":[...]}. Six parsers hardcoded the spaced form and silently dropped every frame without it, which reached the user as a completed turn with no content.The same wire format was already accepted on the relay path (
src/lib/sse-decoder.ts,src/server/relay.ts,src/adapters/google.ts) and rejected on the adapter path. That split is the defect.The reporter named the OpenAI Chat adapter. Reading the tree found five more:
src/adapters/openai-chat.ts:950src/chat/outbound.ts:674collectChatCompletionsrc/claude/outbound.ts:591src/claude/outbound.ts:864collectAnthropicMessagesrc/web-search/parse.ts:190src/server/claude-messages.ts:174Two primitives are added beside the decoder whose rule they mirror:
sseFieldValue(line, field)for the five string-slicing parsers.sseFieldOffset(text, lineStart, lineEnd, field)for the live Claude relay, whose translator budget reserves bytes by offset — materializing the line first would allocate the very string the budget exists to bound.Both strip at most one leading space, so a payload that legitimately begins with whitespace keeps the rest of it. Neither trims: callers own that choice and some intentionally keep trailing bytes. A colonless field line returns an empty value, matching
decodeServerSentEvents'colon < 0handling — an earlier revision of this PR got that wrong and audit caught it.event:is fixed alongsidedata:in both Claude parsers; it carried the identical latent defect.Deliberately not fixed here, and recorded in the plan unit:
\r\n\r\nframe delimiting (claude/outbound.ts:567,server/claude-messages.ts:171) and multilinedatajoining without the spec's\nseparator (:605,:174). Both are frame-level rather than field-level and carry a different blast radius; folding them in would make this diff unreviewable.Verification
The full
prepushgate (typecheck, frontend lint, full test suite, privacy scan) ran and passed on push. No frontend files are touched by this PR —git diff --name-only origin/dev...HEADreturns zero paths under that directory, so there is no UI change to screenshot.Every new test was confirmed to fail with its fix reverted, then restored — a passing test that never proved the defect is not a regression test:
openai-chat.tsserver/claude-messages.tschat/outbound.ts+claude/outbound.ts:864claude/outbound.ts:591offset parserThe budget assertion on the offset parser compares the unspaced path against the spaced path rather than asserting zero: this translator leaves a 51-byte residue at stream end on the spaced path too, so zero would assert something that was never true. Equality is the contract the offset arithmetic must satisfy.
Planning and audit record:
devlog/_plan/260807_untouched_bug_stack/(000_plan.md,010_sse_unspaced_data_fields.md). This plan failed its first independent audit with six blockers; all were corrected before implementation and the record is in000_plan.md.Checklist
Summary by CodeRabbit
Bug Fixes
Tests
Documentation