fix: bound the six remaining retained-state leaks (#820 wave 2) - #892
fix: bound the six remaining retained-state leaks (#820 wave 2)#892lidge-jun wants to merge 77 commits into
Conversation
…d mechanisms wp-a: modelWireDefaults on the github-copilot registry entry (mechanism already on tree: hard pin > modelAdapters > registry defaults > provider adapter); conservative 6-model set, nano/sol lead-only via modelAdapters. wp-b: adopt PR #860's capability file map + #875 root cause found — sanitizeReasoningInputContent blanks plaintext reasoning for every Responses provider (openai-responses.ts:35, called :1027); scope it. wp-c: registry.ts:217 map + authoritative-window [1m] predicate.
1: docs target corrected to reference/configuration/providers.md + locales 2: exact seed before/after array, defaultModel unchanged, nano/sol excluded 3: evidence table moved to 000 (LEXICO-SPLIT-01) 4: per-model pi.dev provenance + two-leg selection rule (sol/nano out)
…ve seed policy 1: gpt-5.6-sol meets the two-leg rule identically to luna/terra (#748 field report + pi.dev Responses declaration + JetBrains conditional chat failure); excluding it was an inconsistent rule application. nano stays out (no field report leg). 2: seed policy made explicitly additive (no removals); gpt-5-mini added as a verified chat model so the chat regression fixture is honest; scenario 4 corrected.
…ecedence fixture claim ledger and selection reference now say seven built-in / sol verified; scenario 3 uses gpt-5.4-nano or gpt-5-mini for the opt-in direction since sol is itself a default and cannot prove override precedence.
github-copilot fronts a mixed-wire catalog: gpt-5.3-codex, gpt-5.4, gpt-5.4-mini, gpt-5.5, gpt-5.6-luna, gpt-5.6-sol, gpt-5.6-terra reject /chat/completions for real Codex-agent traffic (function tools + reasoning). Declare them as registry modelWireDefaults so every inbound wire rides Responses while the provider-wide adapter stays openai-chat for the chat-served catalog. Explicit modelAdapters still win in both directions; gpt-5.4-nano stays out (no field report) as a documented override example. Evidence: issue #748 field runs, pi.dev wire declarations, litellm#23332 (gpt-5.4), JetBrains LLM-29711 (gpt-5.6-sol). Tests: tests/github-copilot-wire-defaults.test.ts — 22 cases incl. captured-upstream-URL replay proof on all three inbound wires. Consolidates the routing half of PR #746 (its sampling/credential-replay half remains a separate parity/security unit). Closes #748.
… bounds Three explorer passes found wave-1 landings already on dev (77243d9 framework, d1408b9 continuation cap+spill, 034d320 cache caps, a616078 translator budgets), so the campaign narrows to refinements: #841 admission boundary (direct-spill oversized, bounded snapshot read, bounded replay), #847 collector per-call scope + mandatory budget + 502 normalization, #844 incremental frames + typed partial-EOF, #845 NOOP (superseded), #843 fixed-size SHA-256 key identities, #840 ACL timeout-memo release + destination keying.
All 7 blockers accepted: blob-ID key channel unbounded (045 becomes a real fix), antigravity caps exclude key bytes, NUL-separator collision ambiguity (length-prefix instead), cursor backlog/parser-cursor state machine + frameWork-deferred EOF, collector test math + dual bridge budget sites + exact 413 sites, ACL temp-only release API, state typed-observability seams + failure/replacement scenarios.
001 synced with amended designs; 020 same-ID replacement split 8a/8b with pruning qualification; 030 scenario-7 asserts lease release after full serialized owner charge; 040 explicit rawBacklog byte cap incl. headers + drain-to-quiescence EOF; 045 validate/digest before hex expansion with separate key-bytes counter (64 MiB payload cap intact); 050 incremental length-prefixed hashing + replay_signature_oversized skip-replay overflow contract.
…l replay Oversized candidates (> 64 MiB resident cap) now write directly to durable spill and install only the stub — they never become resident and never demote unrelated chains; candidates above the new 256 MiB single-spill payload ceiling are tombstoned at admission instead of being retained unreadably. Snapshot loading refuses to parse files over 32 MiB and applies the same admission boundary to oversized rows. Spill replay refuses payloads above the ceiling before read/parse with a typed too_large reason (surfaced as spill_too_large; wire shape unchanged). Snapshot selection now measures UTF-8 bytes, not UTF-16 length. Refines #841 on top of the wave-1 spill design.
…s escape hatch reference/configuration/providers.md (EN + ko/ja/zh-cn/ru): the modelAdapters row now names the built-in Copilot Responses-only defaults and the opt-in path for models without one (gpt-5.4-nano example). guides/providers.md gains the routing-precedence note (pin > modelAdapters > registry default > provider adapter).
Envelope truth: the payload ceiling is enforced against the real
{version,...} spill envelope after publication (a candidate within the
wrapper's size of the cap was previously retained unreadably). Snapshot
size gate uses statSync so symlinked oversized snapshots cannot bypass
it. Over-ceiling same-ID tombstones now defer the old generation's
unlink until the tombstone is durable (same crash rule as
replaceSpillEntryAtomically, whose catch path gets the same fix).
directSpills only counts actual stub installs. Tests: exact-envelope
boundary, no-read proof (too_large beats missing), deferred-unlink
ordering for tombstone and spill replacement, symlinked snapshot
refusal, tighter same-ID accounting bound.
…ntics adopts #860's reviewed fail-closed semantics (undefined strips; explicit config is the escape hatch); names preserveResponsesReasoningContent as the reasoning-replay flag following the statelessResponses flow; maps every edit point on the current tree.
A snapshot symlinked to a FIFO or device (isFile=false, size=0, e.g. /dev/zero) fell through to readFileSync and could block or stream unbounded input; only regular files within the size ceiling are parsed now.
1: research survey moved to 000 (LEXICO-SPLIT-01) 2: guides/codex-app-models.md x5 locales added to docs file map 3: options.serviceTier clearing required + tested 4: ocxr1-strip-under-preservation negative case added 5: comment wording calibrated (accepted replay = verified; required = inference)
…to 502 The non-stream Chat collector charged streamed tool arguments to a generic retained_collectors scope, so one call could consume nearly the whole 32 MiB turn budget instead of the 2 MiB per-call limit; arguments now open a per-call scope (kind tool_args, wire-index key) and close it only after the final serialized owner is charged, with scopes released on every error path. Provider-controlled overflow in the collector is now 502 upstream_error on all three mapping sites (was 413 invalid_request_error on two), matching the adapter/bridge contract. buildResponseJSON/bridgeToResponsesSSE no longer have an unbounded no-budget path: omission creates a default turn budget (disposed with the call/stream) instead of skipping accounting.
…epSeek reasoning replay Two DeepSeek wire fixes, one capability flow: 1. service_tier is an OpenAI-only Responses parameter, but fast mode injected it for every Responses provider. A provider-level supportsServiceTier capability now gates it after the final route is settled: canonical openai/openai-apikey keep fast-mode inject/remove (unset fast mode preserves a caller value); deepseek and volcengine-agent-plan strip it; unclassified providers fail closed unless explicitly opted in. Stripping also clears options.serviceTier so logging never mislabels a removed tier. Adopts PR #860's reviewed fail-closed semantics. 2. sanitizeReasoningInputContent blanked reasoning content for EVERY Responses provider — a rule only the ChatGPT native backend needs. DeepSeek's Responses API accepts plaintext reasoning replay, so providers flagged preserveResponsesReasoningContent keep it (ocxr1 envelopes are still stripped). Fixes the local half of #875: continuations after tool calls no longer reach DeepSeek with emptied reasoning items. Both fields flow registry -> providerConfigSeed -> enrichProviderFromRegistry -> router backfill without overriding explicit config. Tests: tests/service-tier-capability.test.ts + tests/deepseek-reasoning-replay.test.ts (18 cases incl. live handleResponses payload capture).
…ent reference rows, capability-gated fast-tier guides reference/configuration/providers.md (EN/ko/ja/zh-cn/ru): rows for the two new provider fields. guides/codex-app-models.md (all five locales): the blanket 'routed non-OpenAI models strip service-tier metadata' wording is now the capability-gated fail-closed behavior with the explicit opt-in — closing #860's open docs review issue.
Final owner transfer is wrapped: a copy-charge overflow now releases the copies already charged, closes every open call scope, and surfaces the same typed 502 upstream_error instead of a raw budget exception. The translation_buffer_limit classification is 502 upstream_error on the remaining streaming sites (fail() override, response.failed handler, processing catch) and the defensive JSON replay path, so streaming, collector, and non-stream clients agree; genuine client-request 413s are untouched. Owned default SSE budgets are disposed at every stream-death path (terminal close, incomplete terminal, torn-down controller, heartbeat failure, cancel) after the final charges, never inside reportTerminal. New test seam translatorLiveBudgetCountForTests proves disposal; collector tests now cover final-copy overflow cleanup and exact surviving charge.
A client cancel during a pending upstream next() disposed the owned budget while the in-flight step still processed the late event, reanimating charges outside liveBudgets; step() now bails immediately after the await when closed/cancelled (emit() was already guarded). The defensive JSON replay status ternary for translation_buffer_limit is 502 (was 413) — every path now agrees. The final-copy overflow test threshold is corrected so the overflow actually activates in the final owner transfer (250, not 150). New seams: translatorAggregateCurrentBytesForTests; cancel-during-pending-next regression asserts live count and aggregate bytes return to baseline; endpoint regression pins the replay 502/upstream_error shape.
Seeding them via providerConfigSeed broke the management API's canonical openai seed comparison (exact key set) — 4 management-provider-validation failures. Follow the modelWireDefaults philosophy instead: the registry holds the defaults, providerConfigSeed stays free of them so an explicit user value stays distinguishable, and enrichProviderFromRegistry + the router backfill supply them from the entry directly.
Breaking to upstreamDone still ran the EOF path, whose closeCurrentMessage()/retainFinishedItem() charges the disposed budget directly (the emit() closed-guard does not cover retention); a cancelled step now returns immediately like the gated early-return path. The regression now synchronizes on the second upstream next() having actually started before cancelling.
…ts-inject + generator override desktop-profile and model-info guards already landed; the open halves are the registry map, #854's withSubagentContextMarker port, and the stale jawcode generator override contradicting its own committed output.
… EOF
The live transport concatenated every incoming chunk with the entire
pending remainder (O(n^2) on a large incomplete frame) and settled
stream end immediately: complete frames plus a trailing partial settled
as silent success. The backlog is now a {buf,start,end} cursor with
amortized growth (per-chunk O(chunk)) and lazy prefix compaction; a new
framing export consumeConnectFrames reports consumed raw bytes without
materializing a remainder copy. Accounting moves to raw used bytes
(headers included), closing the tiny-frame/header-flood gap in the
payload-only counter. Stream end now drains frameWork to quiescence,
fails typed frame_incomplete on any unconsumed remainder (unless the
close was expected), and keeps the zero-frame unexpected-EOF
classification.
…ts + full regression set my stale check misread the bedrock rows: the anthropic section of the generated metadata still pins sonnet-4-6 at 200k with a catalog test blessing it; file map now includes the generated rows, codex-catalog test, registry-parity assertions, and #854's complete test set (modified 372k test + five additions incl incomplete-metadata branch).
…tative [1m] in generated profiles Two halves, consolidated from PRs #839 and #854: 1. ANTHROPIC_MODEL_CONTEXT_WINDOWS omitted claude-opus-4-7, claude-opus-4-6, and claude-sonnet-4-6 although all three ship in ANTHROPIC_MODELS — they advertised max_input_tokens null, emitted no [1m] picker row, and Claude Code accounted them at its 200k default. All three are documented at 1M by Anthropic (Opus 4.6 2026-02-05, Opus 4.7 2026-04-16, Sonnet 4.6 2026-02-17). The generated jawcode metadata and its 200k-pinned catalog test move with the registry; the stale CONTEXT_WINDOW_OVERRIDES generator pin is removed. 2. Generated subagent defs marked [1m] with the MAIN-SESSION auto-context predicate, so a 372K route was written [1m] into generated profiles — accounted at 1M with no compaction pairing in the subagent. Generated defs now mark only authoritative >=1M windows, strip inherited unsafe markers to bare, preserve genuine routed [1m] ids (kimi/k3[1m]) and provider caps, and keep selectors with unknown windows as-was. Main-session env-slot marking is unchanged. Tests: 372k roster test corrected + five new regressions (catalog-derived 1M markers, routed [1m] preservation, 350K cap unmarking, marker-case precedence, incomplete-metadata preservation), catalog 1M contract, registry parity assertions for all three windows.
consumeConnectFrames now returns payload VIEWS into the backlog (safe: append-only at end, compaction/growth replace the buffer), so the charge transfers to the frame lifecycle instead of reserving a second copy — an exact 16 MiB effective payload peaks at 16 MiB + 5 and completes, matching the documented boundary. One idempotent releaseBacklogLease owns the raw remainder on every terminal path (failAndClear, EOF classifications, first-frame timeout); the data-handler catch no longer releases the chunk charge after a successful append, which understated the retained backlog. Tests: exact-boundary live probe, lease-to-zero after frame_incomplete, 1/3/7/64 KiB chunk sweep (RED on both counts before this repair).
When reserveTransportBytes rejected an over-cap chunk, the data-handler catch released the chunk size anyway, debiting unrelated live ownership (clamps masked the imbalance). The catch now releases only when the reservation succeeded but the append never ran; after an append the terminal backlog cleanup owns the bytes. Regression: two transports on one shared budget — A parks an incomplete frame, B's own incomplete frame overflows; A's lease is byte-exact throughout and the budget returns to zero.
… release with symlink targets The conflict splice dropped the merge parent marker and the closing braces of the timeout-keying describe; both describe blocks are kept (symlink preservation + ACL timeout keying) with the wt2 ephemeral release on the wt4 realpath rename targets.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@AGENTS.md`:
- Around line 117-121: Update the “Ask once, not forever” guidance in AGENTS.md
to distinguish agent-relayed No responses from permanent refusals: either define
and implement a trusted refusal record the CLI can receive through the agent
relay, or narrow the permanence language to refusals made through the
interactive CLI prompt. Keep `.star-deferred` described as deferral only,
consistent with star-prompt.ts and the existing deferral test.
In
`@devlog/_plan/260802_wt1_update_path_star_prompt/011_cycle1_bug_a_stale_check.md`:
- Around line 45-54: Remove the detailed unresolved-defect record from the
tracked devlog section, including exact preview versions, the already_latest
behavior, and the implementation plan. Move that audit detail to an approved
untracked location such as .tmp/ or a mktemp scratch path, leaving tracked
documentation high-level and sanitized.
In `@devlog/_plan/260802_wt1_update_path_star_prompt/012_cycle2_bug_b_plan.md`:
- Around line 3-19: Remove the consent-bypass analysis, guard details,
root-cause discussion, and unreleased implementation plan from the tracked
devlog document. Move the audit record to an appropriate untracked scratch
location such as .tmp/ or a private issue, and retain only a sanitized
high-level design in the tracked documentation.
In `@README.md`:
- Around line 83-86: Document the seven-day re-arm rule in README.md and
docs-site/src/content/docs/getting-started/for-agents.md: clarify that a
different version may ask again only when the deferral is at least seven days
old. Synchronize the same wording across all translated locale pages for the
getting-started agent documentation.
In `@tests/star-deferral.test.ts`:
- Around line 64-89: Update the test setup and teardown around
maybeShowStarPrompt to save, clear, and restore process.env.OCX_SERVICE using
the same pattern as OPENCODEX_HOME, ensuring the variable cannot short-circuit
tests and the original environment is restored afterward.
- Around line 55-61: Export AGENT_ENV_VARS from src/cli/agent-driven.ts and
import that shared symbol in tests/star-deferral.test.ts, removing the
duplicated local list. Keep the existing cleanup logic unchanged so it clears
every production-defined agent environment variable.
🪄 Autofix (Beta)
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: 25e28e65-86f9-4395-8241-6c214f7dff03
📒 Files selected for processing (19)
AGENTS.mdREADME.mddevlog/_plan/260802_wt1_update_path_star_prompt/011_cycle1_bug_a_stale_check.mddevlog/_plan/260802_wt1_update_path_star_prompt/012_cycle2_bug_b_plan.mddocs-site/src/content/docs/getting-started/for-agents.mddocs-site/src/content/docs/ja/getting-started/for-agents.mddocs-site/src/content/docs/ko/getting-started/for-agents.mddocs-site/src/content/docs/ru/getting-started/for-agents.mddocs-site/src/content/docs/zh-cn/getting-started/for-agents.mdreadme/README.ja.mdreadme/README.ko.mdreadme/README.ru.mdreadme/README.zh-CN.mdsrc/cli/star-prompt.tssrc/update/notify.tstests/star-deferral.test.tstests/startup-prompt.test.tstests/update-job.test.tstests/update-notify.test.ts
| - **Ask once, not forever.** An unanswered question settles nothing — silence is | ||
| deferred, never a Yes and never a recorded No. Do not repeat the question in | ||
| later replies or later sessions: the CLI re-arms the deferral at most once | ||
| per opencodex version (never more than once a week), and a later version | ||
| re-asks on its own. Do not decide it yourself in either direction. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'star-prompted|star-deferred|agent_consent_required|declined|refus' src testsRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant files ---'
git ls-files | rg '(^|/)(AGENTS\.md|.*star.*|.*prompt.*|.*260802_wt1_update_path_star_prompt.*)$' | head -80
printf '%s\n' '--- root guidance and target lines ---'
sed -n '1,150p' AGENTS.md
printf '%s\n' '--- star implementation and tests ---'
rg -n -C 8 'maybeShowStarPrompt|star-prompted|star-deferred|CODEX_THREAD_ID|agent' src tests --glob '*star*' --glob '*.ts' | head -500
printf '%s\n' '--- design record references ---'
rg -n -C 8 'star-prompted|star-deferred|agent-relayed|relayed|No|declin' devlog 2>/dev/null | head -500Repository: lidge-jun/opencodex
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- scoped source guidance ---'
cat src/AGENTS.md
printf '%s\n' '--- star-related implementation references ---'
rg -n -C 12 'maybeShowStarPrompt|star-prompted|star-deferred|agent_consent_required|CODEX_THREAD_ID' \
src/cli src/server tests \
--glob '*.ts' \
--glob '*star*' \
--glob '*prompt*' \
--glob '*agent*'
printf '%s\n' '--- design record ---'
cat devlog/_plan/260802_wt1_update_path_star_prompt/012_cycle2_bug_b_plan.md
printf '%s\n' '--- implementation plan ---'
cat devlog/_plan/260802_wt1_update_path_star_prompt/010_implementation.mdRepository: lidge-jun/opencodex
Length of output: 49881
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- star prompt implementation ---'
cat -n src/cli/star-prompt.ts | sed -n '1,230p'
printf '%s\n' '--- exact AGENTS.md consent section ---'
cat -n AGENTS.md | sed -n '95,135p'
printf '%s\n' '--- focused tests around consent and deferral ---'
cat -n tests/star-deferral.test.ts | sed -n '49,135p'
cat -n tests/startup-prompt.test.ts | sed -n '45,115p'
printf '%s\n' '--- standalone behavior check ---'
python3 - <<'PY'
from pathlib import Path
source = Path("src/cli/star-prompt.ts").read_text()
agents = Path("AGENTS.md").read_text()
tests = Path("tests/star-deferral.test.ts").read_text()
agent_branch = source[source.index("if (isAgentDriven())"):source.index("const yes = await", source.index("if (isAgentDriven())"))]
human_branch = source[source.index("const yes = await"):]
checks = {
"agent branch writes deferral": "writeFileSync(deferral" in agent_branch or "writeFileSync(DEFERRAL" in agent_branch,
"agent branch writes prompted marker": "MARKER" in agent_branch and "writeFileSync(marker" in agent_branch,
"human branch writes prompted marker": "writeFileSync(marker" in human_branch,
"human no is persisted by prompted marker": "if (!yes) return;" in human_branch and "writeFileSync(marker" in human_branch,
"runtime test covers agent then human": "delete process.env.CODEX_THREAD_ID" in tests,
"runtime test covers agent-relayed no": "agent" in tests.lower() and "false" in tests.lower(),
"policy says explicit no is permanent": "An explicit no ends the matter permanently" in agents,
}
for name, result in checks.items():
print(f"{name}: {result}")
PYRepository: lidge-jun/opencodex
Length of output: 23450
Do not describe an agent-relayed No as permanent.
src/cli/star-prompt.ts:178-191 writes only .star-deferred and leaves .star-prompted absent. A later interactive run therefore asks the real prompt again, as covered by tests/star-deferral.test.ts:113-125. .star-deferred records deferral, not refusal. Either add a trusted refusal record that the CLI can receive from the agent relay, or scope AGENTS.md:122-123 to refusals made through the interactive CLI prompt.
🤖 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 `@AGENTS.md` around lines 117 - 121, Update the “Ask once, not forever”
guidance in AGENTS.md to distinguish agent-relayed No responses from permanent
refusals: either define and implement a trusted refusal record the CLI can
receive through the agent relay, or narrow the permanence language to refusals
made through the interactive CLI prompt. Keep `.star-deferred` described as
deferral only, consistent with star-prompt.ts and the existing deferral test.
| ## Known limitation (audit blocker 1, folded) | ||
|
|
||
| Respin preview tags exist in the wild: `v2.7.9-preview.20260712.1` / `.2` — | ||
| i.e. `x.y.z-preview.YYYYMMDD[.r]`. `parsePreview`'s | ||
| `/^(\d+)\.(\d+)\.(\d+)-preview\.(\d+)$/` rejects the trailing `.r`, so installs | ||
| on a respin preview remain stuck at `already_latest` even after this fix. This | ||
| gap predates the fix on BOTH channels and expanding the comparator would fork | ||
| behavior from PR #871, so it stays out of scope here. Follow-up candidate: | ||
| widen `parsePreview` to `(\d+)(?:\.(\d+))?$` and treat the respin counter as an | ||
| extra `gt` tuple element (needs its own cycle + tests). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move the live defect record out of tracked devlog/.
These lines preserve exact failing versions, the observable already_latest behavior, and a follow-up implementation plan for an unresolved defect. Move this audit detail to .tmp/, a mktemp -d scratch path, or a private issue. Keep tracked documentation high-level and sanitized.
As per path instructions, devlog/**/* must not contain unfixed-defect reproductions or pre-disclosure patch plans; use .tmp/ or a mktemp -d scratch path.
🤖 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/260802_wt1_update_path_star_prompt/011_cycle1_bug_a_stale_check.md`
around lines 45 - 54, Remove the detailed unresolved-defect record from the
tracked devlog section, including exact preview versions, the already_latest
behavior, and the implementation plan. Move that audit detail to an approved
untracked location such as .tmp/ or a mktemp scratch path, leaving tracked
documentation high-level and sanitized.
Source: Path instructions
| ## Non-goals (consent invariant — a diff touching any of these is a C-gate FAIL) | ||
|
|
||
| - `src/server/management/sidebar-routes.ts` — `403 agent_consent_required` and the | ||
| `isAgentDriven() && !hasBrowserSessionEvidence(req)` shape stay byte-untouched. | ||
| - The human interactive path: TTY gate, `ghAvailable()` gate, Yes/No selector, | ||
| marker `.star-prompted` written BEFORE the question, `if (!yes) return;`. | ||
| - `hasStarPromptRun()` semantics — `src/update/notify.ts:135` yield behavior unchanged. | ||
| - An agent never answers, auto-dismisses, or stars; the `gh api -X PUT` instruction | ||
| stays gated on an explicit user yes. | ||
|
|
||
| ## Root cause (code-verified in issue #879; no external claims — no search lane needed) | ||
|
|
||
| 1. Agent path leaves `.star-prompted` unwritten → every agent-driven start re-prints | ||
| `printAgentDeferral()` (`src/cli/star-prompt.ts:139`). | ||
| 2. Deferral text + AGENTS.md demand repeat-forever relay ("at the top of your next | ||
| reply, unchanged" / "Silence is not a No"). | ||
| 3. Agent PTYs pass the TTY gate → fires during routine edit/test cycles. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not track consent bypass analysis in devlog/.
These lines expose guard shapes, refusal responses, TTY triggering, and the root cause of an unreleased consent defect. The rest of the file continues with a detailed pre-disclosure implementation plan. Move the audit record to .tmp/ or a private issue, and retain only a sanitized high-level design in tracked documentation.
As per path instructions, tracked devlog/**/* must not contain exploit or bypass reasoning or pre-disclosure patch plans; use .tmp/ or a mktemp -d scratch path.
🤖 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/260802_wt1_update_path_star_prompt/012_cycle2_bug_b_plan.md`
around lines 3 - 19, Remove the consent-bypass analysis, guard details,
root-cause discussion, and unreleased implementation plan from the tracked
devlog document. Move the audit record to an appropriate untracked scratch
location such as .tmp/ or a private issue, and retain only a sanitized
high-level design in the tracked documentation.
Source: Path instructions
| > "want to give it a star?" and not buried at the end of a long message. An unanswered question | ||
| > settles nothing — silence is deferred, never a Yes and never a recorded No — but do not repeat | ||
| > the question in later replies: the CLI re-arms the deferral at most once per opencodex version, | ||
| > and a later version asks again on its own. Star only on an explicit yes; an explicit no settles |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the same seven-day re-arm rule on both user-facing surfaces.
The implementation keeps a deferral current for seven days across version changes. Both pages say that a later version asks again without stating this delay.
README.md#L83-L86: state that a different version can ask again only after the deferral is at least seven days old.docs-site/src/content/docs/getting-started/for-agents.md#L51-L53: apply the same wording and synchronize the translated locale pages.
📍 Affects 2 files
README.md#L83-L86(this comment)docs-site/src/content/docs/getting-started/for-agents.md#L51-L53
🤖 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 `@README.md` around lines 83 - 86, Document the seven-day re-arm rule in
README.md and docs-site/src/content/docs/getting-started/for-agents.md: clarify
that a different version may ask again only when the deferral is at least seven
days old. Synchronize the same wording across all translated locale pages for
the getting-started agent documentation.
Source: Path instructions
| const AGENT_ENV_VARS = [ | ||
| "CLAUDECODE", "CLAUDE_CODE_ENTRYPOINT", "CLAUDE_CODE_SSE_PORT", | ||
| "CODEX_THREAD_ID", "CODEX_SHELL", "CODEX_CI", "CODEX_SANDBOX", "CODEX_SANDBOX_NETWORK_DISABLED", | ||
| "CURSOR_TRACE_ID", "CURSOR_SESSION_TOKEN", "CURSOR_AGENT", | ||
| "AIDER_CHAT", "OPENCODE_BIN_PATH", "GEMINI_CLI", | ||
| "REPL_ID", "CI", "GITHUB_ACTIONS", "GITLAB_CI", "BUILDKITE", "JENKINS_URL", "TEAMCITY_VERSION", "CODESPACES", | ||
| ]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'AGENT_ENV_VARS' src/cli/agent-driven.ts
rg -n 'export const AGENT_ENV_VARS' src -g '*.ts'Repository: lidge-jun/opencodex
Length of output: 335
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- src/cli/agent-driven.ts ---'
cat -n src/cli/agent-driven.ts | sed -n '1,90p'
printf '%s\n' '--- tests/star-deferral.test.ts ---'
cat -n tests/star-deferral.test.ts | sed -n '1,110p'
printf '%s\n' '--- references and test scripts ---'
rg -n 'isAgentDriven|AGENT_ENV_VARS|star-deferral|typecheck|privacy:scan' src tests package.json bunfig.toml 2>/dev/null | head -200Repository: lidge-jun/opencodex
Length of output: 13617
Share AGENT_ENV_VARS with the deferral test.
src/cli/agent-driven.ts:24-54 defines the production list, while tests/star-deferral.test.ts:55-61 duplicates it. If the production list changes, the test cleanup at lines 67-81 can leave a host environment variable set and cause unexpected agent deferral. Export AGENT_ENV_VARS and import it in this test.
🤖 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 `@tests/star-deferral.test.ts` around lines 55 - 61, Export AGENT_ENV_VARS from
src/cli/agent-driven.ts and import that shared symbol in
tests/star-deferral.test.ts, removing the duplicated local list. Keep the
existing cleanup logic unchanged so it clears every production-defined agent
environment variable.
| beforeEach(() => { | ||
| home = mkdtempSync(join(tmpdir(), "ocx-star-deferral-")); | ||
| process.env.OPENCODEX_HOME = home; | ||
| for (const name of AGENT_ENV_VARS) { | ||
| savedAgentEnv.set(name, process.env[name]); | ||
| delete process.env[name]; | ||
| } | ||
| Object.defineProperty(process.stdin, "isTTY", { value: true, configurable: true }); | ||
| Object.defineProperty(process.stdout, "isTTY", { value: true, configurable: true }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| setStarPromptDepsForTests(null); | ||
| for (const name of AGENT_ENV_VARS) { | ||
| const value = savedAgentEnv.get(name); | ||
| if (value === undefined) delete process.env[name]; | ||
| else process.env[name] = value; | ||
| } | ||
| Object.defineProperty(process.stdin, "isTTY", { value: stdinTTY, configurable: true }); | ||
| Object.defineProperty(process.stdout, "isTTY", { value: stdoutTTY, configurable: true }); | ||
| if (priorThread === undefined) delete process.env.CODEX_THREAD_ID; | ||
| else process.env.CODEX_THREAD_ID = priorThread; | ||
| if (priorHome === undefined) delete process.env.OPENCODEX_HOME; | ||
| else process.env.OPENCODEX_HOME = priorHome; | ||
| rmSync(home, { recursive: true, force: true }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Save and restore OCX_SERVICE alongside the other guarded env vars.
maybeShowStarPrompt() returns immediately when process.env.OCX_SERVICE is set. This suite guards OPENCODEX_HOME, CODEX_THREAD_ID, the agent-env list, and TTY flags, but not OCX_SERVICE. If that variable is set in the environment that runs this suite, the whole test silently short-circuits and the resulting assertion failures give no hint that OCX_SERVICE is the cause.
Add the same save/clear/restore pattern used for OPENCODEX_HOME.
🔧 Proposed fix
let home: string;
const priorHome = process.env.OPENCODEX_HOME;
const priorThread = process.env.CODEX_THREAD_ID;
+ const priorService = process.env.OCX_SERVICE;
const stdinTTY = process.stdin.isTTY;
const stdoutTTY = process.stdout.isTTY; beforeEach(() => {
home = mkdtempSync(join(tmpdir(), "ocx-star-deferral-"));
process.env.OPENCODEX_HOME = home;
+ delete process.env.OCX_SERVICE;
for (const name of AGENT_ENV_VARS) { if (priorHome === undefined) delete process.env.OPENCODEX_HOME;
else process.env.OPENCODEX_HOME = priorHome;
+ if (priorService === undefined) delete process.env.OCX_SERVICE;
+ else process.env.OCX_SERVICE = priorService;
rmSync(home, { recursive: true, force: true });📝 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.
| beforeEach(() => { | |
| home = mkdtempSync(join(tmpdir(), "ocx-star-deferral-")); | |
| process.env.OPENCODEX_HOME = home; | |
| for (const name of AGENT_ENV_VARS) { | |
| savedAgentEnv.set(name, process.env[name]); | |
| delete process.env[name]; | |
| } | |
| Object.defineProperty(process.stdin, "isTTY", { value: true, configurable: true }); | |
| Object.defineProperty(process.stdout, "isTTY", { value: true, configurable: true }); | |
| }); | |
| afterEach(() => { | |
| setStarPromptDepsForTests(null); | |
| for (const name of AGENT_ENV_VARS) { | |
| const value = savedAgentEnv.get(name); | |
| if (value === undefined) delete process.env[name]; | |
| else process.env[name] = value; | |
| } | |
| Object.defineProperty(process.stdin, "isTTY", { value: stdinTTY, configurable: true }); | |
| Object.defineProperty(process.stdout, "isTTY", { value: stdoutTTY, configurable: true }); | |
| if (priorThread === undefined) delete process.env.CODEX_THREAD_ID; | |
| else process.env.CODEX_THREAD_ID = priorThread; | |
| if (priorHome === undefined) delete process.env.OPENCODEX_HOME; | |
| else process.env.OPENCODEX_HOME = priorHome; | |
| rmSync(home, { recursive: true, force: true }); | |
| }); | |
| const priorService = process.env.OCX_SERVICE; | |
| beforeEach(() => { | |
| home = mkdtempSync(join(tmpdir(), "ocx-star-deferral-")); | |
| process.env.OPENCODEX_HOME = home; | |
| delete process.env.OCX_SERVICE; | |
| for (const name of AGENT_ENV_VARS) { | |
| savedAgentEnv.set(name, process.env[name]); | |
| delete process.env[name]; | |
| } | |
| Object.defineProperty(process.stdin, "isTTY", { value: true, configurable: true }); | |
| Object.defineProperty(process.stdout, "isTTY", { value: true, configurable: true }); | |
| }); | |
| afterEach(() => { | |
| setStarPromptDepsForTests(null); | |
| for (const name of AGENT_ENV_VARS) { | |
| const value = savedAgentEnv.get(name); | |
| if (value === undefined) delete process.env[name]; | |
| else process.env[name] = value; | |
| } | |
| Object.defineProperty(process.stdin, "isTTY", { value: stdinTTY, configurable: true }); | |
| Object.defineProperty(process.stdout, "isTTY", { value: stdoutTTY, configurable: true }); | |
| if (priorThread === undefined) delete process.env.CODEX_THREAD_ID; | |
| else process.env.CODEX_THREAD_ID = priorThread; | |
| if (priorHome === undefined) delete process.env.OPENCODEX_HOME; | |
| else process.env.OPENCODEX_HOME = priorHome; | |
| if (priorService === undefined) delete process.env.OCX_SERVICE; | |
| else process.env.OCX_SERVICE = priorService; | |
| rmSync(home, { recursive: true, force: true }); | |
| }); |
🤖 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 `@tests/star-deferral.test.ts` around lines 64 - 89, Update the test setup and
teardown around maybeShowStarPrompt to save, clear, and restore
process.env.OCX_SERVICE using the same pattern as OPENCODEX_HOME, ensuring the
variable cannot short-circuit tests and the original environment is restored
afterward.
resolveWriteTarget returns the literal path for an absent destination, and the old guard skipped whenever target === path — so a first write beneath a symlinked config dir landed in the protected home with no re-check (reviewer reproduced a protected pid file being created). assertResolvedTargetAllowed now resolves the parent directory on that path too. Regression: a first write beneath a symlinked parent is refused end-to-end in a child probe.
…hardenConfigDir hardenConfigDir chmodded the (symlink-resolved) directory before the atomic write's guard refused it — the protected directory was mutated even though the write itself was rejected. The assertion now runs before any mkdir/chmod in writePid and writeRuntimePort, and at the top of hardenConfigDir for every other caller. Regression extended: writePid is refused through a symlinked parent and the protected directory's mode is untouched.
…llback An elevated schtasks /create can finish before the non-elevated view catches up. finalize verified exactly once, so a task that was merely not visible yet read as 'not installed' and got rolled back milliseconds before Task Scheduler would have listed it. Verification now re-checks on a bounded 1.1s backoff, but only while the failure looks like a lagging view: the task is not visible, or it is visible without a fully published registration. Every other verdict keeps its meaning and spends no delay at all. Proven WinSW presence is rejected independently of conflict, because conflict only becomes true once the task itself is visible -- while it is still invisible the pair is conflict:false with nativeServiceAbsent:false, and retrying that would wait for a service that is already proven present. Rollback deletes a real task, so it now takes the same ownership fence the state write already had; a stale attempt can no longer delete a task a newer attempt owns. Eight regressions cover the settle path and each fail-closed class; four of them fail without this change.
doctor kept telling Windows users to set OPENCODEX_BUN_PATH even when the override was already active. The payload it reads had a Bun version but no runtime origin, so the auto-known-bad branch could not tell an override from the bundled binary and printed the same remedy either way. Origin cannot be recovered after the fact: resolving it at report time answers 'what would this shell pick now', which is a different question from 'what was the service started with', and the two diverge exactly when it matters. So the selecting launcher now stamps it. Every real launch path carries the marker -- npm launcher, scheduler wrapper, WinSW, launchd, systemd, plus the Codex autostart shim and the tray host, which relaunch the proxy themselves and would otherwise erase it. Path and provenance come from one resolution at each site, so the marker cannot describe a binary other than the one baked. Read-back allowlists the three values and never falls back to resolving locally. A service installed before the marker existed reports nothing, and doctor says the origin is unknown instead of guessing -- an absent marker is an answer, not a gap to fill. bunRevision stays informational and the conservative auto-known-bad result for canaries is untouched; bun-stream-caps.ts and responses/core.ts are absent from this diff. Fixes #848.
Review found the marker stopped at the launchers that resolve a binary. The ones that re-exec process.execPath -- ocx ensure, GUI/Claude/OpenCode start, POST /api/system/restart, the update relaunch -- copied the parent environment and handed the daemon nothing, so a service the launcher knew the origin of still reported unknown. withProcessRuntimeProvenance() covers those seven sites. Re-execing the current runtime does not change how that runtime was obtained, so an inherited marker is preserved and only its absence records 'process'; an unrecognized inherited value is replaced rather than forwarded. The shim builders no longer default provenance to 'bundled'. A default let a caller pass an override binary and label it something else, which is the path/marker disagreement the marker exists to prevent, so the argument is now required. Regressions: the launch sites are pinned so a future launcher that copies process.env cannot silently drop the marker again, and the npm launcher's transport is asserted inside the spawn env rather than inferred from the resolver's return shape. Reverting bin/ocx.mjs to its pre-provenance state fails that test. structure/05: the provenance section had been inserted between the sidebar stop-button heading and its own paragraph; restored.
Review caught the marker leaking. A shim wraps the real codex, so exporting the marker into the shim's own environment handed it to Codex and everything Codex spawns. A shell beneath that running a different Bun directly would carry a provenance describing a binary it was not executing -- and the execPath relaunch paths would faithfully preserve that contradiction into the daemon. Every flavor now scopes it to the ensure invocation: a one-shot assignment prefix in sh, a setlocal/endlocal pair in cmd, and save/restore in PowerShell. Nothing downstream of the shim inherits it. Inheritance is also no longer trusted on its own. withProcessRuntimeProvenance carries a claim forward only when re-resolving it still lands on process.execPath; a stale marker from an ancestor falls back to what this executable actually is. That keeps a genuine restart labelled correctly without letting a marker outlive the binary it was minted for. Also fixes an old-signature caller in tests/openai-provider-option-tooling that passed the token path where provenance now goes -- it wrote a filesystem path into the marker and silently used the default token file, so its sentinel assertion was partly vacuous.
The corroboration added last round asked the wrong question. It re-derived the original selection to decide whether an inherited marker was still true, but a service installed with a shell-local override keeps neither that shell nor its OPENCODEX_BUN_PATH -- so a correct 'override' was demoted to 'process' on the service's first relaunch, and doctor could again suggest setting an override that was already in use. The marker now carries the binary it was minted for. OCX_BUN_RUNTIME_PATH is stamped beside the source at every launcher, and a relaunch keeps the claim when that recorded path is the executable about to run. No re-derivation, so a launcher's own environment no longer has to survive for its provenance to. Comparison goes through realpath, so symlinks, junctions, and Windows case differences no longer reject a valid match. The fallback resolves path and source from one durableBunRuntime() call rather than two, which was a small window where the pair could disagree. Full suite 7030 pass / 0 fail; bun-stream-caps.ts and responses/core.ts remain absent from every commit in this unit.
… permanent scheduler XML reportedBunRuntimeSource trusted a bare source marker even when the recorded path was absent or named another executable — a provenance that does not describe the running binary. The allowlisted source now reports only when the recorded path matches process.execPath (the pair contract currentRuntimeProvenance already enforced downstream). Scheduler verification distinguishes pending/unreadable publication (transient, settles) from published-but-policy-violating XML (permanent, registrationInvalid, rolls back with zero settle delays). Regressions for both, including the <Data> violation form.
…ermanent path Fixtures that construct the scheduler verification now declare registrationInvalid explicitly (an absent field read as transient by the guard). The transient budget case keeps its delays; the new permanent-invalid case asserts one probe, zero settle delays, and an immediate rollback.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
devlog/_plan/260802_wt5_windows_service_doctor/000_plan.md (1)
12-12: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the Bug A root-cause statement.
Line 12 says that post-create verification retried non-transient states. The implementation roadmap in
devlog/_plan/260802_wt5_windows_service_doctor/010_implementation.mdLines 14-21 says verification runs once, misses transient Task Scheduler visibility, and rolls back immediately.Keep one verified failure mode in both plans. Otherwise, implementation and test work can target a retry loop that does not exist.
Suggested wording
-- Root cause (from PR body, re-verify): post-create Task Scheduler verification retried non-transient states, so registration verification could fail to settle. +- Root cause: post-create Task Scheduler verification performed one immediate check, so transient task visibility or XML health lag could trigger rollback before registration settled.🤖 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/260802_wt5_windows_service_doctor/000_plan.md` at line 12, Update the Bug A root-cause statement in the plan to match the implementation roadmap: post-create verification runs once, can miss transient Task Scheduler visibility or XML health, and rolls back immediately. Remove references to retrying non-transient states or late reconciliation, keeping the failure mode consistent with the implementation and tests.
🤖 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/260802_wt4_server_config_security/011_wp1_execution_notes.md`:
- Around line 5-14: Remove the tracked devlog file containing pre-disclosure
security investigation and patch-planning details from the change. Do not
replace it with sanitized content yet; keep any unreleased notes only in ignored
scratch space, and add a retrospective only after disclosure.
In `@devlog/_plan/260802_wt4_server_config_security/020_wp2_execution_notes.md`:
- Around line 3-38: Remove the tracked devlog file containing unreleased
security analysis from the change. Preserve any needed investigation notes only
in ignored scratch space such as .tmp/ or a temporary directory, and ensure no
pre-disclosure security material remains under devlog/.
In `@docs-site/src/content/docs/ru/reference/configuration/server.md`:
- Line 20: Update the Firefox and Safari extension-origin lifetime guidance in
docs-site/src/content/docs/reference/configuration/server.md:20,
docs-site/src/content/docs/ja/reference/configuration/server.md:19,
docs-site/src/content/docs/ko/reference/configuration/server.md:19,
docs-site/src/content/docs/ru/reference/configuration/server.md:20, and
docs-site/src/content/docs/zh-cn/reference/configuration/server.md:20: state
that Firefox preserves its moz-extension UUID across browser restarts, while
Safari uses a session/instance-scoped UUID that may change when Safari launches
or the extension instance is recreated.
In `@src/codex/shim.ts`:
- Around line 515-527: Update the PowerShell ensure block in the shim generator
to add a catch that swallows failures from the bun ensure invocation, while
preserving the existing finally-based environment restoration so codex always
proceeds. Add a regression assertion near the existing codex shim tests that
verifies the generated PowerShell shim includes catch handling around ensure.
In `@src/config.ts`:
- Around line 182-185: Extend stale temporary-file recovery to cover
config-writer files matching config.json.ocx.*.tmp in both the literal and
resolved configuration directories. Reuse the existing age and process-ID safety
checks from recoverStaleResponseStateTemps, and ensure cleanup runs for these
files during removeOwnedConfigState so crash leftovers are removed without
deleting active writer files.
In `@src/server/management/system-restart.ts`:
- Line 35: Add a regression test for the detached restart flow around the
process-spawn logic, capturing the child process options and asserting its
environment includes OCX_SERVICE, OCX_BUN_RUNTIME_SOURCE, and
OCX_BUN_RUNTIME_PATH. Reuse the existing system-restart test setup and mocks,
and verify the provenance values are propagated when the restart process is
detached.
In `@src/service.ts`:
- Around line 994-1055: Add a debug log within
verifyWindowsSchedulerInstallAfterSettle for each retry, recording the probe
attempt and delay before the next verification; keep the existing retry
predicate, ownership checks, delay budget, rollback behavior, and state updates
unchanged.
- Around line 820-825: Update the registrationInvalid calculation in the
scheduler verification flow to treat whitespace-only XML as unpublished by
checking the trimmed XML length, preserving transient settling behavior while
retaining permanent invalidation for non-blank invalid XML. Add coverage in
windows-scheduler-install-verification.test.ts for whitespace-only XML with an
installed task, asserting registrationInvalid is false and
schedulerVerificationMaySettle returns true.
In `@src/tray/windows.ts`:
- Around line 24-25: Update persisted tray-state handling around readState and
the corresponding JSON write boundary to validate bunRuntimeSource against
BunRuntimeSource, while accepting legacy records that omit it. For legacy state,
preserve compatibility by omitting -BunRuntimeSource when constructing the tray
start, stop, or refresh commands in the flows around lines 144 and 179, allowing
PowerShell’s optional default to apply; alternatively implement an explicit
migration with a state-version bump.
In `@tests/config.test.ts`:
- Around line 1744-1804: Guard POSIX-only symlink fixtures against Windows
privilege failures: in tests/config.test.ts lines 1744-1804, define and use a
posixOnly describe wrapper; apply the same wrapper to the async symlink suite at
lines 1806-1854; and in tests/responses-state.test.ts lines 1452-1471, use an
equivalent Windows-skipping test guard for the symlink test.
In `@tests/memory-watchdog.test.ts`:
- Around line 264-299: Update the test setup around the read helper to also save
the inherited OCX_BUN_RUNTIME_PATH value and delete that environment variable
before the initial “source alone” assertions. In the finally block, restore the
original path when it existed, otherwise delete it, while preserving the
existing source restoration behavior.
In `@tests/windows-elevation-spawn.test.ts`:
- Around line 1008-1031: Update the test using unhealthyVerify() so it uses the
non-retryable invalidVerify() verdict, allowing the settle loop to break and
exercise the pre-rollback ownership fence in
finalizeWindowsSchedulerServiceRegistration(). Preserve the existing
unhealthyVerify() scenario as a separate focused test renamed to describe
ownership loss detected inside the settle loop, ensuring both ownership checks
remain covered.
---
Outside diff comments:
In `@devlog/_plan/260802_wt5_windows_service_doctor/000_plan.md`:
- Line 12: Update the Bug A root-cause statement in the plan to match the
implementation roadmap: post-create verification runs once, can miss transient
Task Scheduler visibility or XML health, and rolls back immediately. Remove
references to retrying non-transient states or late reconciliation, keeping the
failure mode consistent with the implementation and tests.
🪄 Autofix (Beta)
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: 0fd65cac-9982-414c-9d93-23a027f0f6ad
📒 Files selected for processing (44)
bin/ocx.mjsdevlog/_plan/260802_wt4_server_config_security/011_wp1_execution_notes.mddevlog/_plan/260802_wt4_server_config_security/020_wp2_execution_notes.mddevlog/_plan/260802_wt5_windows_service_doctor/000_plan.mddevlog/_plan/260802_wt5_windows_service_doctor/010_implementation.mddocs-site/src/content/docs/ja/reference/configuration/server.mddocs-site/src/content/docs/ko/reference/configuration/server.mddocs-site/src/content/docs/reference/configuration/server.mddocs-site/src/content/docs/ru/reference/configuration/server.mddocs-site/src/content/docs/zh-cn/reference/configuration/server.mdsrc/cli/claude.tssrc/cli/doctor.tssrc/cli/index.tssrc/cli/opencode.tssrc/codex/shim.tssrc/config.tssrc/lib/bun-runtime.tssrc/lib/winsw.tssrc/responses/state.tssrc/server/auth-cors.tssrc/server/management/system-restart.tssrc/server/management/system-routes.tssrc/service.tssrc/tray/windows-tray.ps1src/tray/windows.tssrc/types.tssrc/update/index.tsstructure/05_gui-and-management-api.mdtests/bun-runtime.test.tstests/codex-shim.test.tstests/config.test.tstests/doctor.test.tstests/memory-watchdog.test.tstests/ocx-launcher-source.test.tstests/openai-provider-option-tooling.test.tstests/responses-state.test.tstests/server-auth.test.tstests/server-loopback-host-gate.test.tstests/service.test.tstests/test-home-guard.test.tstests/windows-elevation-spawn.test.tstests/windows-scheduler-install-verification.test.tstests/windows-tray.test.tstests/winsw.test.ts
| - `010_implementation.md` line refs: auth-cors.ts `isExtraAllowedOrigin` confirmed live at :81-89 in the pre-fix form; `setCorsOrigin` :21. `origin_rejected` sites in `src/server/index.ts` = 10 (grep-confirmed), all funneled through `isAllowedRequestOrigin`/`isAllowedManagementOrigin` → both call `isExtraAllowedOrigin`, so ONE predicate fix covers every data-plane + management-plane rejection site. | ||
| - Bun URL behavior verified locally (primary proof): `chrome-extension://abc123/page.html`, `moz-extension://u-u-i-d/`, `safari-web-extension://UUID/` all serialize `.origin` as `"null"` with `host` populated. | ||
| - Docs drift: `docs-site` configuration reference was split into domain subpages (commit 7fdb2cb8e). The `corsAllowOrigins` row now lives at `docs-site/src/content/docs/<locale>/reference/configuration/server.md` (:19-20) for all five locales (EN, zh-cn, ko, ja, ru). The PR #850 doc hunks DO NOT APPLY to current dev — docs must be re-based onto the subpage rows. | ||
| - PR #850 state: OPEN, CONFLICTING vs dev. Maintainer review blocker (locale doc drift) was already fixed by the contributor in a346ad60, so the fetched diff head is review-clean. Code+test hunks APPLY CLEAN to dev@478354ee8 (`git apply --check --include='src/**' --include='tests/**'`). | ||
|
|
||
| ## wp1 execution decision | ||
|
|
||
| Apply the maintainer-reviewed PR #850 diff for `src/server/auth-cors.ts`, `src/types.ts`, `tests/server-auth.test.ts`, `tests/server-loopback-host-gate.test.ts` verbatim (credit eachann1024), then re-write the five `server.md` rows by hand in the new subpage location with the same content the PR used (authority-based extension origins supported; `*` is not a wildcard). | ||
|
|
||
| Rationale: the implementation is small, already maintainer-reviewed, and its test shape (live server preflight + data-plane + unit-level cross-scheme and `*` rejection) matches the acceptance criteria in `010_implementation.md` exactly. Hand-rewriting an equivalent fix would add risk, not value. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove pre-disclosure security investigation material from devlog/.
Lines 5-14 record the pre-fix security mechanism, affected request paths, and patch-application plan. This tracked file will preserve that material in repository history.
Remove this file from the PR. Keep unreleased security notes only in ignored scratch space such as .tmp/ or mktemp -d. Add a sanitized retrospective only after disclosure.
As per coding guidelines, unreleased security work must not be committed, and devlog/ must not contain pre-disclosure security material.
🤖 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/260802_wt4_server_config_security/011_wp1_execution_notes.md`
around lines 5 - 14, Remove the tracked devlog file containing pre-disclosure
security investigation and patch-planning details from the change. Do not
replace it with sanitized content yet; keep any unreleased notes only in ignored
scratch space, and add a retrospective only after disclosure.
Source: Coding guidelines
| ## Stale check results | ||
|
|
||
| - `atomicWriteFile` at `src/config.ts:107`, `atomicWriteFileAsync` at :187 — both confirmed in the pre-fix form (temp `${path}.ocx.${pid}.${seq}.tmp` beside the LITERAL path; `io.rename(tmp, path)`). | ||
| - PR #869 diff (`/tmp/wt4-pr869.diff`, 336 lines) applies CLEAN to dev@478354ee8 + wp1 commit (`git apply --check` passes). | ||
| - PR #869 state: OPEN, MERGEABLE, quality gates green, no maintainer blocker comments. | ||
| - The diff's `src/responses/state.ts` hunk expects `enforceAppOwnedMemoryBudget` (77243d932) — present on current dev. | ||
|
|
||
| ## PR #869 implementation shape (what will be applied) | ||
|
|
||
| 1. `resolveWriteTarget(path)` (exported): `realpathSync(path)`, fallback to literal path when unresolvable (first write of a not-yet-created file). | ||
| 2. `assertResolvedTargetAllowed(path, target)`: re-applies the test-only real-home guard (`assertNotRealHomeUnderTest`) to the RESOLVED target — a symlink escaping a temp fixture home into the protected home is refused even though the caller's dir-level check passed. Inert in production. | ||
| 3. Both sync + async writers compute `tmp` beside the RESOLVED target and rename onto it. | ||
| 4. `src/responses/state.ts` snapshot load sweeps stale temps in BOTH the literal and the resolved directory (a symlinked snapshot strands temps in the real dir). | ||
| 5. Tests: `tests/config.test.ts` symlink suite (sync+async: link survives + target updated, no temps left, plain destination unaffected, first-write creation, dangling symlink replaced), `tests/responses-state.test.ts` (sweep in resolved dir), `tests/test-home-guard.test.ts` (escape-refused probe). | ||
|
|
||
| ## Caller audit (criterion c5) — grep `atomicWriteFile` across `src/` @ dev 478354ee8 | ||
|
|
||
| | Caller | Writes into | Symlink exposure | | ||
| |---|---|---| | ||
| | `src/config.ts` (owner; config.json :1627, pid :2070, runtime port :2098) | OPENCODEX_HOME | direct — config dir is dotfiles-managed in the reported case | | ||
| | `src/oauth/store.ts` | OPENCODEX_HOME credential store | high-value target; fix protects token files behind symlinked dirs | | ||
| | `src/codex/inject.ts`, `journal.ts`, `history-provider.ts`, `account-store.ts`, `quota.ts`, `refresh.ts`, `runtime.ts`, `features.ts` | `~/.codex/*` | the reported dotfiles case (`config.toml` symlink) | | ||
| | `src/claude/desktop-3p.ts` | Claude Desktop config | user-managed file, symlink plausible | | ||
| | `src/grok/inject.ts` | grok config | same shape | | ||
| | `src/responses/state.ts` | OPENCODEX_HOME snapshot | covered by the sweep-in-both-dirs hunk | | ||
| | `src/update/job.ts`, `notify.ts` | OPENCODEX_HOME update state | covered by shared helper | | ||
| | `src/codex/catalog/*` (aggregation, bundled, effort, metadata, parsing, provider-fetch, sync) | catalog caches | covered by shared helper | | ||
|
|
||
| No caller needs an individual change: the fix lives in the shared writer + the one stale-temp sweep that scans directories. | ||
|
|
||
| ## Known design decisions to audit | ||
|
|
||
| - Dangling symlink: realpath fails → literal path → rename REPLACES the dangling link (PR test asserts this). Debate point: silently replacing a dotfiles link whose target dir is temporarily unmounted. PR chose "replace"; the alternative (refuse) breaks first-write-into-new-target-dir. | ||
| - TOCTOU: link swapped between realpath and rename lands the write at the old target. Accepted, documented in `010_implementation.md`; not claimed race-free. | ||
| - Windows: `realpathSync` resolves junctions/symlinks on win32 too; temp stays same-volume because it sits beside the resolved target. | ||
| - wt2 coordination: #840's memo-release touches `atomicWriteFileAsync` timeout-memo area; this diff does not overlap those lines (memo keyed by `path` argument, unchanged). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove unreleased security analysis from devlog/.
This tracked file records an open security fix, affected credential-store targets, symlink exposure, and accepted TOCTOU behavior. This is pre-disclosure security material.
Move the investigation to ignored scratch space such as .tmp/ or a mktemp -d directory. Remove this file from the tracked change before merge.
As per coding guidelines, “Security work involving unreleased findings or pre-disclosure material must be written only to ignored scratch space.” As per path instructions, “do not place unreleased security findings, draft advisories, exploit reasoning, or other pre-disclosure security material” in devlog/.
🤖 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/260802_wt4_server_config_security/020_wp2_execution_notes.md`
around lines 3 - 38, Remove the tracked devlog file containing unreleased
security analysis from the change. Preserve any needed investigation notes only
in ignored scratch space such as .tmp/ or a temporary directory, and ensure no
pre-disclosure security material remains under devlog/.
Sources: Coding guidelines, Path instructions
| | `shutdownTimeoutMs?` | `number` | `5000` | Дедлайн graceful-drain до принудительного прерывания активных turn'ов. | | ||
| | `websockets?` | `boolean` | `false` | Объявлять `supports_websockets` для WebSocket-пути Responses. Значение false удерживает HTTP/SSE. | | ||
| | `corsAllowOrigins?` | `string[]` | `[]` | Дополнительные точные CORS-origin'ы. Loopback-origin'ы разрешены всегда. | | ||
| | `corsAllowOrigins?` | `string[]` | `[]` | Дополнительные точные origin, разрешённые CORS. Loopback-origin разрешены всегда. Поддерживаются authority-based origin браузерных расширений, например `chrome-extension://<extension-id>`; `*` не является маской. Firefox и Safari пересоздают UUID расширения (при каждой установке/запуске браузера), поэтому обновляйте запись при смене origin. | |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate documentation files ---'
git ls-files 'docs-site/src/content/docs/*/reference/configuration/server.md' \
'docs-site/src/content/docs/reference/configuration/server.md' \
'docs-site/src/content/docs/guides/providers.md' \
'docs-site/src/content/docs/reference/adapters.md'
printf '%s\n' '--- localized and English CORS documentation ---'
for f in \
docs-site/src/content/docs/ru/reference/configuration/server.md \
docs-site/src/content/docs/zh-cn/reference/configuration/server.md \
docs-site/src/content/docs/reference/configuration/server.md
do
if [ -f "$f" ]; then
echo "### $f"
cat -n "$f" | sed -n '1,45p'
fi
done
printf '%s\n' '--- repository references to extension origins and UUID lifetime ---'
rg -n -i --glob '!docs-site/src/content/docs/ru/reference/configuration/server.md' \
--glob '!docs-site/src/content/docs/zh-cn/reference/configuration/server.md' \
'chrome-extension|moz-extension|safari.*uuid|uuid.*safari|extension.*uuid|corsAllowOrigins|CORS' .Repository: lidge-jun/opencodex
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all locale rows ---'
for f in \
docs-site/src/content/docs/reference/configuration/server.md \
docs-site/src/content/docs/ja/reference/configuration/server.md \
docs-site/src/content/docs/ko/reference/configuration/server.md \
docs-site/src/content/docs/ru/reference/configuration/server.md \
docs-site/src/content/docs/zh-cn/reference/configuration/server.md
do
echo "### $f"
nl -ba "$f" | sed -n '18,22p'
done
printf '%s\n' '--- CORS implementation outline and focused symbols ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline src/server/auth-cors.ts
fi
rg -n -A18 -B8 \
'function isExtraAllowedOrigin|const isExtraAllowedOrigin|isExtraAllowedOrigin|function isAllowedRequestOrigin|isAllowedRequestOrigin|corsAllowOrigins' \
src tests
printf '%s\n' '--- focused extension-origin tests ---'
rg -n -A12 -B8 \
'chrome-extension|moz-extension|safari-web-extension|extension origin|corsAllowOrigins' \
tests src --glob '!**/*.snap'Repository: lidge-jun/opencodex
Length of output: 50375
🌐 Web query:
Firefox WebExtensions moz-extension UUID generated installation browser restart extension origin lifetime official documentation
💡 Result:
In Firefox, the moz-extension://[UUID] host component is a machine-specific, randomly generated identifier assigned to an extension upon installation [1][2][3]. Key characteristics of this UUID include: Persistence and Lifetime: The UUID is persistent across browser restarts for a given installation [1][4]. Once assigned, it is stored in the user profile (specifically tracked via the extensions.webextensions.uuids preference) and associated with the extension's principal, including its IndexedDB, localStorage, and permissions [1][5][3]. It does not change upon restarting the browser, which ensures that storage and permissions associated with the extension remain intact [1][4]. Reason for Generation: Firefox generates a random UUID for each installation to improve user privacy and security [1][3]. By using a random UUID instead of the extension's static ID in the moz-extension:// URL, Firefox makes it more difficult for websites to detect whether a user has a specific extension installed by attempting to load its web-accessible resources [1][2][3]. Distinction from Extension Origin: It is important to distinguish the random moz-extension://[UUID] origin used for standard resources from the deterministic origin used for certain APIs like WebAuthn [6]. For WebAuthn, Firefox uses a stable, deterministic origin (a SHA-256 hash of the extension ID) that is identical for all users of that extension, allowing relying party servers to correctly identify and allowlist the extension [6]. Development Context: During development, if an extension is manually loaded (e.g., via about:debugging), Firefox often treats this as a fresh installation, resulting in a new UUID being generated [7]. The UUID is only considered fixed once the extension is properly packaged and installed [7]. To programmatically obtain the correct moz-extension URL for a resource, developers should always use the runtime.getURL API, which transparently handles the current UUID [2][8][3].
Citations:
- 1: https://github.com/mozilla-firefox/firefox/blob/d2f488f844f353d539899c261814e136f5e95d4a/toolkit/components/extensions/Extension.sys.mjs
- 2: https://raw.githubusercontent.com/mozilla-firefox/firefox/main/toolkit/components/extensions/Extension.sys.mjs
- 3: https://stackoverflow.com/questions/47042507/can-a-firefox-addons-internal-uuid-value-be-made-stable
- 4: https://bugzilla.mozilla.org/show_bug.cgi?id=1218756
- 5: https://bugzilla.mozilla.org/show_bug.cgi?id=1974419
- 6: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Use_the_web_authn_api
- 7: https://discourse.mozilla.org/t/is-it-possible-to-have-fixed-url-for-extension-page-without-the-random-internal-uuid/126226
- 8: https://firefox-source-docs.mozilla.org/toolkit/components/extensions/webextensions/reference.html
🌐 Web query:
Safari WebExtension safari-web-extension UUID browser session reinstall origin official documentation
💡 Result:
In Safari, Safari Web Extensions are treated as distinct instances when they are reinstalled or enabled in different browser profiles [1]. Each instance is assigned a unique, system-generated UUID, which is reflected in the origin (e.g., safari-web-extension://) [2]. This UUID is not a static identifier tied to the extension's code or bundle; rather, it is ephemeral and changes whenever the extension is re-initialized by the browser [1][2]. Because this identifier is dynamic, developers should not rely on the safari-web-extension:// origin for hard-coded configurations such as static OAuth redirect URLs or origin-locked callback endpoints [2]. Instead, developers must implement dynamic handling for these paths, such as utilizing backend services that can support templated or dynamic redirects, or managing allowed origin lists dynamically through server-side API updates [2]. Key technical points regarding this behavior include: - Extension Instances: When an extension is turned on or reinstalled, it is treated as a new instance with its own unique background page, storage, and UUID [1]. - Profiles: Extensions are scoped per browser profile; an extension enabled in one profile is entirely separate from the same extension enabled in another [1]. - Persistence: Data stored via the standard WebExtensions storage.local API will persist for that specific instance, but because the UUID—and consequently the origin—changes upon reinstallation, the extension is treated as a fresh entity by the browser's security model [1][2]. - Development Testing: During development, these UUID changes can make testing specific installation flows difficult [2][3]. Developers are encouraged to avoid relying on hard-coded origins and instead use relative paths or dynamic discovery methods where possible [2][4].
Citations:
- 1: https://developer.apple.com/videos/play/wwdc2023/10119/
- 2: https://community.auth0.com/t/dynamic-origin-of-safari-web-extension-and-static-callback-urls/123814
- 3: https://developer.apple.com/forums/thread/679429
- 4: https://wwdcnotes.com/documentation/wwdc22-10099-whats-new-in-safari-web-extensions/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all locale rows ---'
for f in \
docs-site/src/content/docs/reference/configuration/server.md \
docs-site/src/content/docs/ja/reference/configuration/server.md \
docs-site/src/content/docs/ko/reference/configuration/server.md \
docs-site/src/content/docs/ru/reference/configuration/server.md \
docs-site/src/content/docs/zh-cn/reference/configuration/server.md
do
echo "### $f"
awk 'NR >= 19 && NR <= 21 { printf "%d:%s\n", NR, $0 }' "$f"
done
printf '%s\n' '--- focused auth-cors implementation ---'
sed -n '54,106p' src/server/auth-cors.ts
printf '%s\n' '--- focused extension-origin tests ---'
rg -l -i 'chrome-extension|moz-extension|safari-web-extension|extension origin' tests src | while IFS= read -r f; do
echo "### $f"
rg -n -i -A10 -B6 'chrome-extension|moz-extension|safari-web-extension|extension origin' "$f"
doneRepository: lidge-jun/opencodex
Length of output: 10224
🌐 Web query:
site:developer.apple.com Safari Web Extension UUID changes browser launch session safari-web-extension origin
💡 Result:
In Safari, the UUID portion of the safari-web-extension:// origin is ephemeral and designed to change to prevent website fingerprinting [1]. Specifically, this identifier may change upon each launch of the browser or when the extension is toggled off and on again [1][2]. Key points regarding this behavior include: Fingerprinting Protection: Apple intentionally uses a unique, rotating UUID for the safari-web-extension:// origin to prevent websites from tracking users across browser sessions via stable extension identifiers [1]. Persistence: Because the extension ID is not stable, developers cannot rely on it for persistent identification, such as in OAuth redirect URIs or hardcoded CORS allowlists that depend on a fixed origin [1][3]. Profile Separation: When an extension is enabled within a specific Safari Profile, it creates a new instance of the extension, which includes its own unique UUID, background page, and storage [2]. This ensures data separation across different profiles [2]. Development Impacts: - CORS and Origins: Developers often encounter issues where requests are blocked because the origin changes [1]. Safari enforces CORS strictly, and developers must account for the fact that the origin is dynamic [1]. - @@extension_id: In CSS or manifest files, the @@extension_id placeholder is intended to resolve to the current extension's origin; however, this mechanism has been subject to regressions and inconsistencies across Safari versions [4]. - OAuth: Implementing OAuth flows that require a stable redirect URI is challenging in Safari because the extension origin is not static and is often blocked from being used as a valid redirect target [3]. For data that needs to persist across these changes, you should use the browser.storage.local API, which remains independent of the ephemeral origin UUID [2][5]. When communicating with a native host app, use the profile-specific identifiers provided in the extension's context to maintain data separation [2][6].
Citations:
- 1: https://developer.apple.com/forums/thread/654839
- 2: https://developer.apple.com/videos/play/wwdc2023/10119/
- 3: https://developer.apple.com/forums/thread/670165?answerId=661186022
- 4: https://developer.apple.com/forums/thread/763545
- 5: https://developer.apple.com/forums/thread/764057
- 6: https://developer.apple.com/documentation/safariservices/messaging-between-the-app-and-javascript-in-a-safari-web-extension
🌐 Web query:
site:developer.apple.com "safari-web-extension://" UUID origin
💡 Result:
The safari-web-extension:// origin is a unique, ephemeral scheme used by Safari to identify and isolate the resources of a web extension [1][2]. Key characteristics and technical details include: Purpose and Security The UUID portion of the URL serves as a unique identifier for the extension instance [1][2]. Safari generates these dynamic identifiers—and changes them across launches or environments—to prevent fingerprinting of extension users [1]. Because the identifier is not static, developers cannot hard-code URLs using this scheme [1][3]. Accessing Resources To correctly reference extension resources (such as HTML, images, or CSS files), developers must use the runtime API rather than constructing strings manually [1]. The standard method is to call browser.runtime.getURL('/path/to/resource.ext'), which dynamically resolves the correct base URL for the current session [1][4]. While developers historically used special strings like @@extension_id in CSS to resolve paths, this practice has faced compatibility issues in newer Safari versions, leading to recommendations to use relative URLs where possible [5]. Profile Isolation With the introduction of per-profile extensions in Safari, each instance of an extension running in a different profile is treated as a distinct entity with its own unique UUID, background page, and storage [6]. When communicating between the native host app and the extension's JavaScript, developers can use the SFExtensionProfileKey to identify and handle data specifically associated with a user's profile [6][7]. Development Challenges Because this origin is dynamically generated and restricted, developers often encounter common hurdles, such as CORS errors or issues with OAuth redirect flows, which expect stable, reachable URLs [8][3]. When testing, it is critical to ensure that all extension files are correctly included in the Xcode project's build phases so that Safari can properly resolve and serve them under the extension's generated origin [4][9].
Citations:
- 1: https://developer.apple.com/videos/play/wwdc2020/10665/
- 2: https://developer.apple.com/forums/thread/774351
- 3: https://developer.apple.com/forums/thread/670165
- 4: https://developer.apple.com/forums/thread/712596
- 5: https://developer.apple.com/forums/thread/763545
- 6: https://developer.apple.com/videos/play/wwdc2023/10119/
- 7: https://developer.apple.com/documentation/safariservices/messaging-between-the-app-and-javascript-in-a-safari-web-extension
- 8: https://developer.apple.com/forums/thread/708171
- 9: https://developer.apple.com/forums/thread/710164
Separate Firefox and Safari UUID lifetimes.
Firefox assigns the moz-extension:// UUID at installation and preserves it across browser restarts. Safari uses a session/instance-scoped UUID that can change when Safari launches or the extension instance is recreated. Update the English source and all four localized rows: docs-site/src/content/docs/reference/configuration/server.md:20, ja/.../server.md:19, ko/.../server.md:19, ru/.../server.md:20, and zh-cn/.../server.md:20. The runtime compares the configured scheme and authority exactly in src/server/auth-cors.ts:81-100, so inaccurate lifetime guidance can leave Safari origins stale or cause unnecessary Firefox allowlist changes.
📍 Affects 2 files
docs-site/src/content/docs/ru/reference/configuration/server.md#L20-L20(this comment)docs-site/src/content/docs/zh-cn/reference/configuration/server.md#L20-L20
🤖 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 `@docs-site/src/content/docs/ru/reference/configuration/server.md` at line 20,
Update the Firefox and Safari extension-origin lifetime guidance in
docs-site/src/content/docs/reference/configuration/server.md:20,
docs-site/src/content/docs/ja/reference/configuration/server.md:19,
docs-site/src/content/docs/ko/reference/configuration/server.md:19,
docs-site/src/content/docs/ru/reference/configuration/server.md:20, and
docs-site/src/content/docs/zh-cn/reference/configuration/server.md:20: state
that Firefox preserves its moz-extension UUID across browser restarts, while
Safari uses a session/instance-scoped UUID that may change when Safari launches
or the extension instance is recreated.
Source: Path instructions
| if (-not $skipEnsure) { | ||
| & ${psString(bunPath)} ${psString(cliPath)} ensure *> $null | ||
| $priorRuntimeSource = $env:${BUN_RUNTIME_SOURCE_ENV} | ||
| $priorRuntimePath = $env:${BUN_RUNTIME_PATH_ENV} | ||
| $env:${BUN_RUNTIME_SOURCE_ENV} = ${psString(bunRuntimeSource)} | ||
| $env:${BUN_RUNTIME_PATH_ENV} = ${psString(bunPath)} | ||
| try { & ${psString(bunPath)} ${psString(cliPath)} ensure *> $null } | ||
| finally { | ||
| if ($null -eq $priorRuntimeSource) { Remove-Item Env:\\${BUN_RUNTIME_SOURCE_ENV} -ErrorAction SilentlyContinue } | ||
| else { $env:${BUN_RUNTIME_SOURCE_ENV} = $priorRuntimeSource } | ||
| if ($null -eq $priorRuntimePath) { Remove-Item Env:\\${BUN_RUNTIME_PATH_ENV} -ErrorAction SilentlyContinue } | ||
| else { $env:${BUN_RUNTIME_PATH_ENV} = $priorRuntimePath } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a catch to the PowerShell ensure invocation so a failed ensure cannot block the real codex.
The three shim flavors are supposed to fail open around ensure. The Unix flavor appends || true at Line 421. The batch flavor discards the exit code at Line 480. The PowerShell flavor now uses try { … } finally { … } with no catch.
Failure mode: & <bunPath> <cliPath> ensure throws a terminating CommandNotFoundException when bunPath no longer exists, for example after a partial uninstall or an ocx update that moved the bundled Bun. The finally block restores the environment, then the exception propagates out of the shim. Lines 528-529 never run, so codex never starts and the user sees a PowerShell error instead of Codex. The Unix and batch shims survive the same condition.
*> $null only redirects streams; it does not convert a terminating error into a non-terminating one.
🛡️ Proposed fix: swallow ensure failures like the other two flavors
try { & ${psString(bunPath)} ${psString(cliPath)} ensure *> $null }
+ catch { }
finally {
if ($null -eq $priorRuntimeSource) { Remove-Item Env:\\${BUN_RUNTIME_SOURCE_ENV} -ErrorAction SilentlyContinue }
else { $env:${BUN_RUNTIME_SOURCE_ENV} = $priorRuntimeSource }
if ($null -eq $priorRuntimePath) { Remove-Item Env:\\${BUN_RUNTIME_PATH_ENV} -ErrorAction SilentlyContinue }
else { $env:${BUN_RUNTIME_PATH_ENV} = $priorRuntimePath }
}Add a regression test near tests/codex-shim.test.ts:223 that asserts the generated PowerShell shim contains a catch around the ensure call.
📝 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.
| if (-not $skipEnsure) { | |
| & ${psString(bunPath)} ${psString(cliPath)} ensure *> $null | |
| $priorRuntimeSource = $env:${BUN_RUNTIME_SOURCE_ENV} | |
| $priorRuntimePath = $env:${BUN_RUNTIME_PATH_ENV} | |
| $env:${BUN_RUNTIME_SOURCE_ENV} = ${psString(bunRuntimeSource)} | |
| $env:${BUN_RUNTIME_PATH_ENV} = ${psString(bunPath)} | |
| try { & ${psString(bunPath)} ${psString(cliPath)} ensure *> $null } | |
| finally { | |
| if ($null -eq $priorRuntimeSource) { Remove-Item Env:\\${BUN_RUNTIME_SOURCE_ENV} -ErrorAction SilentlyContinue } | |
| else { $env:${BUN_RUNTIME_SOURCE_ENV} = $priorRuntimeSource } | |
| if ($null -eq $priorRuntimePath) { Remove-Item Env:\\${BUN_RUNTIME_PATH_ENV} -ErrorAction SilentlyContinue } | |
| else { $env:${BUN_RUNTIME_PATH_ENV} = $priorRuntimePath } | |
| } | |
| } | |
| if (-not $skipEnsure) { | |
| $priorRuntimeSource = $env:${BUN_RUNTIME_SOURCE_ENV} | |
| $priorRuntimePath = $env:${BUN_RUNTIME_PATH_ENV} | |
| $env:${BUN_RUNTIME_SOURCE_ENV} = ${psString(bunRuntimeSource)} | |
| $env:${BUN_RUNTIME_PATH_ENV} = ${psString(bunPath)} | |
| try { & ${psString(bunPath)} ${psString(cliPath)} ensure *> $null } | |
| catch { } | |
| finally { | |
| if ($null -eq $priorRuntimeSource) { Remove-Item Env:\\${BUN_RUNTIME_SOURCE_ENV} -ErrorAction SilentlyContinue } | |
| else { $env:${BUN_RUNTIME_SOURCE_ENV} = $priorRuntimeSource } | |
| if ($null -eq $priorRuntimePath) { Remove-Item Env:\\${BUN_RUNTIME_PATH_ENV} -ErrorAction SilentlyContinue } | |
| else { $env:${BUN_RUNTIME_PATH_ENV} = $priorRuntimePath } | |
| } | |
| } |
🤖 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 `@src/codex/shim.ts` around lines 515 - 527, Update the PowerShell ensure block
in the shim generator to add a catch that swallows failures from the bun ensure
invocation, while preserving the existing finally-based environment restoration
so codex always proceeds. Add a regression assertion near the existing codex
shim tests that verifies the generated PowerShell shim includes catch handling
around ensure.
Source: Path instructions
| recordOwnedConfigPath(resolveConfigDir(), path); | ||
| const tmp = `${path}.ocx.${process.pid}.${++_atomicSeq}.tmp`; | ||
| const target = resolveWriteTarget(path); | ||
| assertResolvedTargetAllowed(path, target); | ||
| const tmp = `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace the owned-config-path registry and every temp-sweeping consumer.
set -euo pipefail
echo '--- recordOwnedConfigPath definition and all call sites ---'
rg -n -C 8 'recordOwnedConfigPath' --type=ts
echo '--- ephemeral secret path tracking ---'
rg -n -C 8 'forgetEphemeralSecretPath|ephemeralSecretPath|EphemeralSecret' --type=ts
echo '--- every consumer that sweeps or recovers .ocx temp files ---'
rg -n -C 10 'recoverStaleResponseStateTemps|recoverOrphaned|\.ocx\.' --type=ts
echo '--- resolveWriteTarget call sites: who compensates for the resolved dir ---'
rg -n -C 6 'resolveWriteTarget' --type=tsRepository: lidge-jun/opencodex
Length of output: 217
🏁 Script executed:
set -euo pipefail
echo '--- candidate files ---'
git ls-files | rg '(^|/)(config|state)\.ts$|src/'
echo '--- relevant symbols and temp suffixes ---'
rg -n -C 6 'recordOwnedConfigPath|resolveWriteTarget|forgetEphemeralSecretPath|\.ocx\.|recoverStaleResponseStateTemps|ephemeral' .Repository: lidge-jun/opencodex
Length of output: 50378
🏁 Script executed:
set -euo pipefail
for f in $(git ls-files 'src/config.ts' 'src/responses/state.ts'); do
echo "--- $f ---"
wc -l "$f"
rg -n 'recordOwnedConfigPath|resolveWriteTarget|forgetEphemeralSecretPath|\.ocx\.|recover|owned|ephemeral' "$f" || true
doneRepository: lidge-jun/opencodex
Length of output: 2232
🏁 Script executed:
set -euo pipefail
echo '--- config target and writers ---'
sed -n '110,225p' src/config.ts
sed -n '250,315p' src/config.ts
echo '--- state temp recovery and startup callers ---'
sed -n '480,630p' src/responses/state.ts
echo '--- ownership registry implementation and consumers ---'
rg -n -C 12 'recordOwnedConfigPath|CONFIG_OWNER|owned.*path|manifest|removeOwnedConfigState' src/lib/config-ownership.ts src --glob '*.ts' | head -n 600
echo '--- all exact temp cleanup/recovery consumers ---'
rg -l '\.ocx\.|recoverStaleResponseStateTemps|forgetEphemeralSecretPath|removeOwnedConfigState' src --glob '*.ts' | sortRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
set -euo pipefail
echo '--- ownership source ---'
f=$(git ls-files | rg 'src/lib/config-ownership\.ts$' | head -n 1)
cat -n "$f" | sed -n '1,330p'
echo '--- temp cleanup implementation ---'
rg -n -C 14 'forgetEphemeralSecretPath|removeManagementTokenPathBestEffort|hardenSecretPath|ephemeral' src/lib src/config.ts --glob '*.ts'
echo '--- all callers of state recovery ---'
rg -n -C 8 'recoverStaleResponseStateTemps|recoverOrphanedResponseSpills|load.*State|startup' src --glob '*.ts'Repository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
set -euo pipefail
echo '--- exact temp-name patterns and all source references ---'
rg -n -C 5 'RESPONSE_STATE_TEMP_NAME|STALE_TEMP|\.ocx\.\(\[0-9\]|\.ocx\.\*|ocx\.\$|ocx\.' src --glob '*.ts'
echo '--- ownership-root consumers ---'
rg -n -C 8 'listLiveConfigOwnershipRoots|reconcileConfigOwnershipRoots|removeOwnedConfigState|CONFIG_UNINSTALL_MANIFEST' src --glob '*.ts'
echo '--- tests for symlinked config writes and stale-temp recovery ---'
rg -n -C 8 'symlink|recoverStaleResponseStateTemps|ocx\.\$\{|\.ocx\.|resolvedDir|literal.*dir|real.*directory' tests/config.test.ts tests/responses-state.test.ts tests --glob '*.ts' | head -n 800Repository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import re
src = Path("src")
producers = []
for path in src.rglob("*.ts"):
text = path.read_text()
for n, line in enumerate(text.splitlines(), 1):
if ".ocx." in line and ("tmp" in line or "temp" in line):
producers.append((str(path), n, line.strip()))
state = (src / "responses" / "state.ts").read_text()
patterns = re.findall(r"const\s+([A-Z0-9_]*TEMP[A-Z0-9_]*)\s*=\s*([^;]+);", state)
recovery_calls = [
(n, line.strip())
for n, line in enumerate(state.splitlines(), 1)
if "recoverStaleResponseStateTemps(" in line
]
ownership = (src / "lib" / "config-ownership.ts").read_text()
manifest_paths = re.findall(r'"([^"]+)"', ownership[ownership.find("INITIAL_OWNED_PATHS"):ownership.find("] as const")])
print("TEMP_PRODUCERS")
for item in producers:
print(f"{item[0]}:{item[1]}: {item[2]}")
print("STATE_TEMP_PATTERNS")
for item in patterns:
print(item)
print("STATE_RECOVERY_CALLS")
for item in recovery_calls:
print(item)
print("MANIFEST_HAS_OCX_SUFFIX", any(".ocx." in p or p.endswith(".tmp") for p in manifest_paths))
print("MANIFEST_CONFIG_ENTRIES", [p for p in manifest_paths if p in {"config.json", "responses-state.json"}])
PYRepository: lidge-jun/opencodex
Length of output: 1014
Add recovery for resolved atomic-write temporary files.
recordOwnedConfigPath records manifest entries only. removeOwnedConfigState does not sweep *.ocx.*.tmp files. recoverStaleResponseStateTemps scans both directories but matches only responses-state.json.ocx.*.tmp, so a crash during atomicWriteFile can leave secret-bearing config.json.ocx.*.tmp files in the resolved directory indefinitely. Add age- and PID-aware recovery for config-writer temporary files in both the literal and resolved directories.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@src/config.ts` around lines 182 - 185, Extend stale temporary-file recovery
to cover config-writer files matching config.json.ocx.*.tmp in both the literal
and resolved configuration directories. Reuse the existing age and process-ID
safety checks from recoverStaleResponseStateTemps, and ensure cleanup runs for
these files during removeOwnedConfigState so crash leftovers are removed without
deleting active writer files.
| /** | ||
| * Bounded post-create backoff, 1.1s total. Task Scheduler's non-elevated view can | ||
| * lag an elevated `/create` by a few hundred milliseconds, so a single verification | ||
| * would roll back a task that is merely not visible yet. | ||
| */ | ||
| const SCHEDULER_SETTLE_DELAYS_MS = [50, 150, 300, 600] as const; | ||
|
|
||
| /** | ||
| * Whether a failed verification is still worth re-checking after a short delay. | ||
| * | ||
| * Retrying is confined to states that a lagging scheduler view actually produces: | ||
| * the task is not visible yet, or it is visible but its registration has not been | ||
| * published in full. Everything else keeps its existing fail-closed meaning and is | ||
| * rejected here so no delay can turn it into a pass: | ||
| * | ||
| * - a proven conflict (both backends present) is a real dual-backend install; | ||
| * - missing assets are missing on disk, which no amount of waiting creates; | ||
| * - a WinSW service that is proven present (`started`/`stopped`) is never absent | ||
| * later. This is checked independently of `conflict`, which only becomes true | ||
| * once the task itself is visible — while the task is still invisible the pair | ||
| * is `conflict: false` with `nativeServiceAbsent: false`, and that must not retry; | ||
| * - unknown SCM status is unproven rather than transient, and has its own | ||
| * task-preserving branch below. | ||
| */ | ||
| /** Exported for tests: the transient-vs-permanent settle decision. */ | ||
| export function schedulerVerificationMaySettle(v: WindowsSchedulerInstallVerification): boolean { | ||
| if (v.ok) return false; | ||
| if (v.conflict) return false; | ||
| if (!v.assetsHealthy) return false; | ||
| if (!v.nativeServiceAbsent) return false; | ||
| // A published-but-invalid registration is permanent: no delay repairs it. | ||
| if (v.registrationInvalid) return false; | ||
| return !v.taskInstalled || !v.registrationHealthy; | ||
| } | ||
|
|
||
| function settleDelay(ms: number): Promise<void> { | ||
| const hook = finalizeHooks?.settleDelay; | ||
| if (hook) return hook(ms); | ||
| return new Promise(resolve => setTimeout(resolve, ms)); | ||
| } | ||
|
|
||
| /** | ||
| * Verify the elevated install, re-checking only while the failure looks like a | ||
| * scheduler view that has not caught up yet. Returns `null` when this attempt lost | ||
| * ownership mid-settle: a newer attempt owns the task, so this one must neither | ||
| * write install state nor roll anything back. | ||
| */ | ||
| async function verifyWindowsSchedulerInstallAfterSettle( | ||
| options: ApplyElevatedOptions, | ||
| ): Promise<WindowsSchedulerInstallVerification | null> { | ||
| const verify = finalizeHooks?.verify ?? verifyWindowsSchedulerInstall; | ||
| let verification = verify(); | ||
| for (const delayMs of SCHEDULER_SETTLE_DELAYS_MS) { | ||
| if (!schedulerVerificationMaySettle(verification)) break; | ||
| if (!attemptStillOwned(options)) return null; | ||
| await settleDelay(delayMs); | ||
| if (!attemptStillOwned(options)) return null; | ||
| verification = verify(); | ||
| } | ||
| return verification; | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
The settle loop, the ownership fences, and the retry predicate are correct as written.
I traced the four states that must never retry against evaluateWindowsSchedulerInstallVerification:
conflictis only reachable withtaskInstalled === true, so Line 1023 (!v.nativeServiceAbsent) is required to block the invisible-task-plus-running-WinSW pair. The predicate has it, andtests/windows-elevation-spawn.test.ts:925-957pins it.nativeStatusUnknownsetsnativeServiceAbsent === false, so Line 1023 also short-circuits it before the task-preserving branch at Lines 1091-1103 runs. That matches the zero-delay expectation attests/windows-elevation-spawn.test.ts:984-985.- The delay budget sums to 1100 ms, which matches the comment at Line 995, and yields 5 probes with 4 delays as asserted at
tests/windows-elevation-spawn.test.ts:837-838. - Returning
nullon ownership loss correctly bypasses both the rollback at Line 1107 and the state write at Line 1125.
One operational note: the settle loop runs on the install path and adds up to 1100 ms of wall time before a genuine failure is reported. That is bounded and acceptable, but consider emitting a debug log line on each retry so a support bundle shows how many probes the scheduler needed.
Also applies to: 1085-1087, 1104-1106
🤖 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 `@src/service.ts` around lines 994 - 1055, Add a debug log within
verifyWindowsSchedulerInstallAfterSettle for each retry, recording the probe
attempt and delay before the next verification; keep the existing retry
predicate, ownership checks, delay budget, rollback behavior, and state updates
unchanged.
| /** Provenance of `bun`, resolved together with it. */ | ||
| bunRuntimeSource: BunRuntimeSource; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Migrate and validate persisted bunRuntimeSource values.
Line 25 makes bunRuntimeSource required. Existing tray-state records do not contain this new field. readState accepts those records without checking the field at Lines 208-222, then Lines 144 and 179 pass undefined as -BunRuntimeSource.
This can break existing tray start, stop, or refresh commands after an upgrade. Validate the enum at both JSON boundaries. For legacy state, omit -BunRuntimeSource so the PowerShell optional default applies, or add an explicit state migration and version bump.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile, execFileSync, spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@src/tray/windows.ts` around lines 24 - 25, Update persisted tray-state
handling around readState and the corresponding JSON write boundary to validate
bunRuntimeSource against BunRuntimeSource, while accepting legacy records that
omit it. For legacy state, preserve compatibility by omitting -BunRuntimeSource
when constructing the tray start, stop, or refresh commands in the flows around
lines 144 and 179, allowing PowerShell’s optional default to apply;
alternatively implement an explicit migration with a state-version bump.
| describe("config.ts – atomic writes preserve symlinked destinations", () => { | ||
| test("a symlinked destination survives the write and the real file receives it", () => { | ||
| // Dotfiles shape: ~/.codex/config.toml -> ~/dotfiles/.codex/config.toml | ||
| const repoDir = join(testDir, "dotfiles"); | ||
| mkdirSync(repoDir, { recursive: true }); | ||
| const realFile = join(repoDir, "config.toml"); | ||
| writeFileSync(realFile, "original", "utf-8"); | ||
| const link = join(testDir, "config.toml"); | ||
| symlinkSync(realFile, link); | ||
|
|
||
| atomicWriteFile(link, "rewritten"); | ||
|
|
||
| expect(lstatSync(link).isSymbolicLink()).toBe(true); | ||
| expect(readlinkSync(link)).toBe(realFile); | ||
| expect(readFileSync(realFile, "utf8")).toBe("rewritten"); | ||
| expect(readFileSync(link, "utf8")).toBe("rewritten"); | ||
| }); | ||
|
|
||
| test("no temp file is left beside the link or its target", () => { | ||
| const repoDir = join(testDir, "dotfiles-clean"); | ||
| mkdirSync(repoDir, { recursive: true }); | ||
| const realFile = join(repoDir, "config.toml"); | ||
| writeFileSync(realFile, "original", "utf-8"); | ||
| const link = join(testDir, "config-clean.toml"); | ||
| symlinkSync(realFile, link); | ||
|
|
||
| atomicWriteFile(link, "rewritten"); | ||
|
|
||
| expect(readdirSync(repoDir).filter(name => name.includes(".ocx."))).toEqual([]); | ||
| expect(readdirSync(testDir).filter(name => name.includes(".ocx."))).toEqual([]); | ||
| }); | ||
|
|
||
| test("a plain destination is unaffected", () => { | ||
| const destination = join(testDir, "plain.toml"); | ||
| atomicWriteFile(destination, "first"); | ||
| atomicWriteFile(destination, "second"); | ||
|
|
||
| expect(lstatSync(destination).isSymbolicLink()).toBe(false); | ||
| expect(readFileSync(destination, "utf8")).toBe("second"); | ||
| }); | ||
|
|
||
| test("a destination that does not exist yet is created at the literal path", () => { | ||
| const destination = join(testDir, "created.toml"); | ||
| expect(existsSync(destination)).toBe(false); | ||
|
|
||
| atomicWriteFile(destination, "fresh"); | ||
|
|
||
| expect(readFileSync(destination, "utf8")).toBe("fresh"); | ||
| }); | ||
|
|
||
| test("a dangling symlink is preserved and the write is refused", () => { | ||
| const link = join(testDir, "dangling.toml"); | ||
| symlinkSync(join(testDir, "gone", "config.toml"), link); | ||
|
|
||
| // The target volume may only be temporarily unavailable; replacing the link | ||
| // would recreate the dotfiles divergence this fix exists to prevent. | ||
| expect(() => atomicWriteFile(link, "recovered")).toThrow(/unresolvable symlinked write target/); | ||
| expect(lstatSync(link).isSymbolicLink()).toBe(true); | ||
| expect(existsSync(join(testDir, "gone"))).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Symlink test fixtures are created without a POSIX platform guard. Three new test suites call symlinkSync during setup. On Windows, symlinkSync requires Developer Mode or an elevated process and otherwise throws EPERM, so these tests fail while building the fixture and never reach an assertion. The reported failure names the atomic writer or the snapshot sweeper, but the real cause is symlink creation privilege. This PR also adds Windows ACL and Windows scheduler tests, so a Windows run is a real scenario.
tests/config.test.ts#L1744-L1804: declareconst posixOnly = process.platform === "win32" ? describe.skip : describe;above the block and changedescribe(...)toposixOnly(...)for the sync symlink suite.tests/config.test.ts#L1806-L1854: apply the sameposixOnlyguard to the async symlink suite so both suites skip together.tests/responses-state.test.ts#L1452-L1471: gate this test with an equivalentprocess.platform === "win32" ? test.skip : testguard, matching the guard already requested for the symlink tests later in the same file.
📍 Affects 2 files
tests/config.test.ts#L1744-L1804(this comment)tests/config.test.ts#L1806-L1854tests/responses-state.test.ts#L1452-L1471
🤖 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 `@tests/config.test.ts` around lines 1744 - 1804, Guard POSIX-only symlink
fixtures against Windows privilege failures: in tests/config.test.ts lines
1744-1804, define and use a posixOnly describe wrapper; apply the same wrapper
to the async symlink suite at lines 1806-1854; and in
tests/responses-state.test.ts lines 1452-1471, use an equivalent
Windows-skipping test guard for the symlink test.
| test("serializes only an allowlisted Bun runtime provenance, omitting it otherwise (#848)", async () => { | ||
| const inherited = process.env.OCX_BUN_RUNTIME_SOURCE; | ||
| const read = async (): Promise<{ bunRuntimeSource?: unknown; bunRevision?: unknown }> => { | ||
| const req = new Request("http://127.0.0.1:10100/api/system/memory"); | ||
| const res = await handleManagementAPI(req, new URL(req.url), config()); | ||
| return await res!.json() as { bunRuntimeSource?: unknown; bunRevision?: unknown }; | ||
| }; | ||
| try { | ||
| for (const source of ["override", "bundled", "process"]) { | ||
| process.env.OCX_BUN_RUNTIME_SOURCE = source; | ||
| // Source alone is not enough: the marker must name THIS executable. | ||
| expect((await read()).bunRuntimeSource).toBeUndefined(); | ||
| process.env.OCX_BUN_RUNTIME_PATH = process.execPath; | ||
| expect((await read()).bunRuntimeSource).toBe(source); | ||
| delete process.env.OCX_BUN_RUNTIME_PATH; | ||
| } | ||
| // A mismatched recorded path describes another binary — stay absent. | ||
| process.env.OCX_BUN_RUNTIME_SOURCE = "override"; | ||
| process.env.OCX_BUN_RUNTIME_PATH = "/usr/local/bin/definitely-not-this-bun"; | ||
| expect((await read()).bunRuntimeSource).toBeUndefined(); | ||
| delete process.env.OCX_BUN_RUNTIME_PATH; | ||
| delete process.env.OCX_BUN_RUNTIME_SOURCE; | ||
| // An unset or unrecognized marker must leave the field absent rather than | ||
| // shipping a value doctor would then have to distrust. | ||
| const unset = await read(); | ||
| expect(unset.bunRuntimeSource).toBeUndefined(); | ||
| expect(typeof unset.bunRevision).toBe("string"); | ||
|
|
||
| process.env.OCX_BUN_RUNTIME_SOURCE = "system"; | ||
| expect((await read()).bunRuntimeSource).toBeUndefined(); | ||
| } finally { | ||
| if (inherited === undefined) delete process.env.OCX_BUN_RUNTIME_SOURCE; | ||
| else process.env.OCX_BUN_RUNTIME_SOURCE = inherited; | ||
| delete process.env.OCX_BUN_RUNTIME_PATH; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve and clear the inherited runtime path.
Line 265 saves only OCX_BUN_RUNTIME_SOURCE. If OCX_BUN_RUNTIME_PATH is already set to process.execPath, Line 275 reports the newly assigned source instead of undefined. Line 297 then deletes the caller's original path.
Save OCX_BUN_RUNTIME_PATH, delete it before the first “source alone” assertion, and restore it in finally.
Proposed fix
- const inherited = process.env.OCX_BUN_RUNTIME_SOURCE;
+ const inheritedSource = process.env.OCX_BUN_RUNTIME_SOURCE;
+ const inheritedPath = process.env.OCX_BUN_RUNTIME_PATH;
const read = async (): Promise<{ bunRuntimeSource?: unknown; bunRevision?: unknown }> => {
// ...
};
try {
+ delete process.env.OCX_BUN_RUNTIME_PATH;
for (const source of ["override", "bundled", "process"]) {
// ...
}
} finally {
- if (inherited === undefined) delete process.env.OCX_BUN_RUNTIME_SOURCE;
- else process.env.OCX_BUN_RUNTIME_SOURCE = inherited;
- delete process.env.OCX_BUN_RUNTIME_PATH;
+ if (inheritedSource === undefined) delete process.env.OCX_BUN_RUNTIME_SOURCE;
+ else process.env.OCX_BUN_RUNTIME_SOURCE = inheritedSource;
+ if (inheritedPath === undefined) delete process.env.OCX_BUN_RUNTIME_PATH;
+ else process.env.OCX_BUN_RUNTIME_PATH = inheritedPath;
}📝 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.
| test("serializes only an allowlisted Bun runtime provenance, omitting it otherwise (#848)", async () => { | |
| const inherited = process.env.OCX_BUN_RUNTIME_SOURCE; | |
| const read = async (): Promise<{ bunRuntimeSource?: unknown; bunRevision?: unknown }> => { | |
| const req = new Request("http://127.0.0.1:10100/api/system/memory"); | |
| const res = await handleManagementAPI(req, new URL(req.url), config()); | |
| return await res!.json() as { bunRuntimeSource?: unknown; bunRevision?: unknown }; | |
| }; | |
| try { | |
| for (const source of ["override", "bundled", "process"]) { | |
| process.env.OCX_BUN_RUNTIME_SOURCE = source; | |
| // Source alone is not enough: the marker must name THIS executable. | |
| expect((await read()).bunRuntimeSource).toBeUndefined(); | |
| process.env.OCX_BUN_RUNTIME_PATH = process.execPath; | |
| expect((await read()).bunRuntimeSource).toBe(source); | |
| delete process.env.OCX_BUN_RUNTIME_PATH; | |
| } | |
| // A mismatched recorded path describes another binary — stay absent. | |
| process.env.OCX_BUN_RUNTIME_SOURCE = "override"; | |
| process.env.OCX_BUN_RUNTIME_PATH = "/usr/local/bin/definitely-not-this-bun"; | |
| expect((await read()).bunRuntimeSource).toBeUndefined(); | |
| delete process.env.OCX_BUN_RUNTIME_PATH; | |
| delete process.env.OCX_BUN_RUNTIME_SOURCE; | |
| // An unset or unrecognized marker must leave the field absent rather than | |
| // shipping a value doctor would then have to distrust. | |
| const unset = await read(); | |
| expect(unset.bunRuntimeSource).toBeUndefined(); | |
| expect(typeof unset.bunRevision).toBe("string"); | |
| process.env.OCX_BUN_RUNTIME_SOURCE = "system"; | |
| expect((await read()).bunRuntimeSource).toBeUndefined(); | |
| } finally { | |
| if (inherited === undefined) delete process.env.OCX_BUN_RUNTIME_SOURCE; | |
| else process.env.OCX_BUN_RUNTIME_SOURCE = inherited; | |
| delete process.env.OCX_BUN_RUNTIME_PATH; | |
| } | |
| }); | |
| test("serializes only an allowlisted Bun runtime provenance, omitting it otherwise (`#848`)", async () => { | |
| const inheritedSource = process.env.OCX_BUN_RUNTIME_SOURCE; | |
| const inheritedPath = process.env.OCX_BUN_RUNTIME_PATH; | |
| const read = async (): Promise<{ bunRuntimeSource?: unknown; bunRevision?: unknown }> => { | |
| const req = new Request("http://127.0.0.1:10100/api/system/memory"); | |
| const res = await handleManagementAPI(req, new URL(req.url), config()); | |
| return await res!.json() as { bunRuntimeSource?: unknown; bunRevision?: unknown }; | |
| }; | |
| try { | |
| delete process.env.OCX_BUN_RUNTIME_PATH; | |
| for (const source of ["override", "bundled", "process"]) { | |
| process.env.OCX_BUN_RUNTIME_SOURCE = source; | |
| // Source alone is not enough: the marker must name THIS executable. | |
| expect((await read()).bunRuntimeSource).toBeUndefined(); | |
| process.env.OCX_BUN_RUNTIME_PATH = process.execPath; | |
| expect((await read()).bunRuntimeSource).toBe(source); | |
| delete process.env.OCX_BUN_RUNTIME_PATH; | |
| } | |
| // A mismatched recorded path describes another binary — stay absent. | |
| process.env.OCX_BUN_RUNTIME_SOURCE = "override"; | |
| process.env.OCX_BUN_RUNTIME_PATH = "/usr/local/bin/definitely-not-this-bun"; | |
| expect((await read()).bunRuntimeSource).toBeUndefined(); | |
| delete process.env.OCX_BUN_RUNTIME_PATH; | |
| delete process.env.OCX_BUN_RUNTIME_SOURCE; | |
| // An unset or unrecognized marker must leave the field absent rather than | |
| // shipping a value doctor would then have to distrust. | |
| const unset = await read(); | |
| expect(unset.bunRuntimeSource).toBeUndefined(); | |
| expect(typeof unset.bunRevision).toBe("string"); | |
| process.env.OCX_BUN_RUNTIME_SOURCE = "system"; | |
| expect((await read()).bunRuntimeSource).toBeUndefined(); | |
| } finally { | |
| if (inheritedSource === undefined) delete process.env.OCX_BUN_RUNTIME_SOURCE; | |
| else process.env.OCX_BUN_RUNTIME_SOURCE = inheritedSource; | |
| if (inheritedPath === undefined) delete process.env.OCX_BUN_RUNTIME_PATH; | |
| else process.env.OCX_BUN_RUNTIME_PATH = inheritedPath; | |
| } | |
| }); |
🤖 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 `@tests/memory-watchdog.test.ts` around lines 264 - 299, Update the test setup
around the read helper to also save the inherited OCX_BUN_RUNTIME_PATH value and
delete that environment variable before the initial “source alone” assertions.
In the finally block, restore the original path when it existed, otherwise
delete it, while preserving the existing source restoration behavior.
| test("ownership lost around a non-retryable failure skips rollback too", async () => { | ||
| // The settle loop never awaits for a non-retryable verdict, so this is the only | ||
| // path that reaches the pre-rollback ownership fence: a stale attempt must not | ||
| // delete a task that a newer attempt now owns. | ||
| mockParentRollbackSpawn(); | ||
| let owned = true; | ||
| let probes = 0; | ||
| setFinalizeWindowsSchedulerHooksForTests({ | ||
| elevateCreateAndRun: succeedingElevation(), | ||
| verify: () => { | ||
| probes += 1; | ||
| owned = false; | ||
| return unhealthyVerify(); | ||
| }, | ||
| settleDelay: async () => { throw new Error("must not settle a non-retryable verdict"); }, | ||
| stillOwnsAttempt: () => owned, | ||
| writeInstallState: () => { writeCount += 1; }, | ||
| }); | ||
|
|
||
| await expect(finalizeWindowsSchedulerServiceRegistration()).resolves.toEqual({ kind: "done" }); | ||
| expect(probes).toBe(1); | ||
| expect(writeCount).toBe(0); | ||
| expect(parentRollbackLaunches).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This test does not reach the pre-rollback ownership fence it claims to cover; the comment at Lines 1009-1011 is inaccurate.
The test returns unhealthyVerify(), which is a retryable verdict: taskInstalled: true, registrationHealthy: false, registrationInvalid: false, assetsHealthy: true, nativeServiceAbsent: true. schedulerVerificationMaySettle therefore returns true at src/service.ts:1026.
Execution path today: the loop body at src/service.ts:1047 passes the maySettle check, then attemptStillOwned at Line 1048 sees owned === false and returns null. The run exits from inside the settle loop. It never reaches the new fence at src/service.ts:1104-1106. That is also why the settleDelay trap at Line 1022 never fires — the loop returns before awaiting, not because the verdict was non-retryable.
Consequence: the fence added at src/service.ts:1104-1106 — the one that stops a stale attempt from deleting a newer attempt's real Task Scheduler task — has no coverage in this suite. Use a non-retryable verdict so the loop breaks and returns the verification, which is the only way to reach Line 1106.
💚 Proposed fix: use the non-retryable `invalidVerify()` verdict
test("ownership lost around a non-retryable failure skips rollback too", async () => {
- // The settle loop never awaits for a non-retryable verdict, so this is the only
- // path that reaches the pre-rollback ownership fence: a stale attempt must not
- // delete a task that a newer attempt now owns.
+ // A published-but-invalid registration is non-retryable, so the settle loop breaks
+ // and returns the verification instead of null. That is the only path that reaches
+ // the pre-rollback ownership fence: a stale attempt must not delete a task that a
+ // newer attempt now owns.
mockParentRollbackSpawn();
let owned = true;
let probes = 0;
setFinalizeWindowsSchedulerHooksForTests({
elevateCreateAndRun: succeedingElevation(),
verify: () => {
probes += 1;
owned = false;
- return unhealthyVerify();
+ return invalidVerify();
},
settleDelay: async () => { throw new Error("must not settle a non-retryable verdict"); },
stillOwnsAttempt: () => owned,
writeInstallState: () => { writeCount += 1; },
});Keep the existing unhealthyVerify() variant as a separate test named for what it actually pins — ownership loss detected inside the settle loop — so both fences stay covered.
As per path instructions: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
🤖 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 `@tests/windows-elevation-spawn.test.ts` around lines 1008 - 1031, Update the
test using unhealthyVerify() so it uses the non-retryable invalidVerify()
verdict, allowing the settle loop to break and exercise the pre-rollback
ownership fence in finalizeWindowsSchedulerServiceRegistration(). Preserve the
existing unhealthyVerify() scenario as a separate focused test renamed to
describe ownership loss detected inside the settle loop, ensuring both ownership
checks remain covered.
Source: Path instructions
…m] picker rows The service-tier gate stripped the field for UNCLASSIFIED custom providers too, silently rewriting caller requests against unknown gateways; only an explicit supportsServiceTier: false strips now (true supports, false strips, undefined preserves-without-injection). The Claude picker emitted [1m] rows for sub-1M models under auto-context (the #854 defect at the picker level — a 372K route marked [1m] makes Claude Code account 1e6); variants now require an authoritative >=1M window. Adds the real DeepSeek tool-call continuation shape (reasoning → call → output) to the replay tests.
…locales The public contract text still said 'false or absent strips' after the tri-state runtime fix. types.ts, the EN providers reference, the codex-app-models guide, and all four locales (ja/ko/ru/zh-cn) now state the real rule: true permits injection, false strips, undefined preserves caller values without injection. Also drops the stale auto-context widening comment and the now-unused mode parameter from the picker's push1mVariant, and fixes the test preamble.
… add) go/internal/cli/config_parity.go was untracked in 2101d50 and swept back in three minutes later by a sibling session's git add -A. It stays on disk, untracked; go/ is retired and new work does not go there.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config.ts (1)
107-192: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for first writes through a symlinked parent directory.
tests/config.test.tscovers existing symlinks, literal first writes, dangling links, and successful cleanup beside both the link and resolved target. It does not cover an absent destination whose parent is a symlink. Add focused synchronous and asynchronous tests for this case to exerciseassertResolvedTargetAllowedand confirm that the write reaches the resolved directory without leaving a temporary file.🤖 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 `@src/config.ts` around lines 107 - 192, Add focused synchronous and asynchronous tests in the config test suite for writing an absent file beneath a symlinked parent directory. Exercise atomicWriteFile and its async counterpart so assertResolvedTargetAllowed is covered, verify the content is written to the resolved directory, and assert no temporary file remains beside either the symlinked path or resolved target.Source: Coding guidelines
🤖 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/260802_wt3_provider_wire/000_plan.md`:
- Line 3: Update the worktree execution entry to remove the absolute
`/Users/jun/.codex/...` paths while preserving the commit and branch
identifiers; refer to the worktrees using repository-relative names or other
machine-independent identifiers.
In `@docs-site/src/content/docs/guides/providers.md`:
- Around line 317-324: Update the Copilot routing description around the model
adapter precedence to replace “every other Copilot model stays on chat
completions” with wording that says those models default to Chat Completions
unless overridden by an explicit modelAdapters entry. Preserve the existing
built-in GPT-5 routing and gpt-5.4-nano opt-in example.
In `@docs-site/src/content/docs/reference/configuration/providers.md`:
- Line 55: Update the provider capability wording in
docs-site/src/content/docs/reference/configuration/providers.md:55-55 to
explicitly state canonical OpenAI (true), DeepSeek (false), and Volcengine Ark
(false). Apply the equivalent Japanese wording including DeepSeek (false) in
docs-site/src/content/docs/ja/reference/configuration/providers.md:44-44 and the
equivalent Korean wording in
docs-site/src/content/docs/ko/reference/configuration/providers.md:44-44.
In `@src/server/responses/core.ts`:
- Around line 802-812: Update the normalization paths in handleResponses around
applyServiceTierGate, including the fast-mode path and the other gate call, so
requestedServiceTier and requestedSpeedLabel are refreshed from the post-gate
parsed options/body rather than retaining pre-normalization snapshots. Preserve
configuredServiceTier as the independent fallback, ensure request classification
and persistence reflect the tier actually sent upstream, and add a regression
test covering removal of an unsupported service_tier.
---
Outside diff comments:
In `@src/config.ts`:
- Around line 107-192: Add focused synchronous and asynchronous tests in the
config test suite for writing an absent file beneath a symlinked parent
directory. Exercise atomicWriteFile and its async counterpart so
assertResolvedTargetAllowed is covered, verify the content is written to the
resolved directory, and assert no temporary file remains beside either the
symlinked path or resolved target.
🪄 Autofix (Beta)
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: 24c2b373-d8cf-4b7f-b2a1-3cde7c41a6fb
⛔ Files ignored due to path filters (1)
src/generated/jawcode-model-metadata.tsis excluded by!**/generated/**
📒 Files selected for processing (32)
devlog/_plan/260802_wt3_provider_wire/000_plan.mddevlog/_plan/260802_wt3_provider_wire/010_bug_a_copilot_mixed_wire.mddevlog/_plan/260802_wt3_provider_wire/020_bug_b_deepseek_service_tier.mddevlog/_plan/260802_wt3_provider_wire/030_bug_c_claude_1m_windows.mddocs-site/src/content/docs/guides/codex-app-models.mddocs-site/src/content/docs/guides/providers.mddocs-site/src/content/docs/ja/guides/codex-app-models.mddocs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ko/guides/codex-app-models.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/ru/guides/codex-app-models.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/guides/codex-app-models.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mdscripts/generate-jawcode-metadata.tssrc/adapters/openai-responses.tssrc/claude/agents-inject.tssrc/claude/model-info.tssrc/config.tssrc/providers/derive.tssrc/providers/registry.tssrc/router.tssrc/server/responses/core.tssrc/types.tstests/claude-agents-inject.test.tstests/claude-model-info.test.tstests/codex-catalog.test.tstests/deepseek-reasoning-replay.test.tstests/github-copilot-wire-defaults.test.tstests/provider-registry-parity.test.tstests/service-tier-capability.test.ts
| @@ -0,0 +1,78 @@ | |||
| # wt3 — Provider wire correctness (research) | |||
|
|
|||
| Executing worktree: `/Users/jun/.codex/worktrees/8e2b/opencodex` (branch `codex/wt3-exec`, off dev@478354ee8). A spare prepared worktree also exists at `/Users/jun/.codex/worktrees/260802-wt3-provider-wire`. | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Remove machine-specific worktree paths.
Line [3] commits /Users/jun/.codex/... paths. This exposes a local username and filesystem layout. It also becomes stale outside that checkout. Keep the commit and branch identifiers, or use repository-relative worktree names.
🤖 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/260802_wt3_provider_wire/000_plan.md` at line 3, Update the
worktree execution entry to remove the absolute `/Users/jun/.codex/...` paths
while preserving the commit and branch identifiers; refer to the worktrees using
repository-relative names or other machine-independent identifiers.
| Copilot fronts a mixed-wire catalog: its GPT-5 family (`gpt-5.3-codex`, `gpt-5.4`, | ||
| `gpt-5.4-mini`, `gpt-5.5`, `gpt-5.6-luna`, `gpt-5.6-sol`, `gpt-5.6-terra`) rejects | ||
| `/chat/completions` for agent traffic, so opencodex routes those models over the | ||
| Responses API by built-in default while every other Copilot model stays on chat | ||
| completions. The precedence is: hard wire pin → your explicit | ||
| [`modelAdapters`](/reference/configuration/providers/) entry → registry default → | ||
| provider-wide adapter. To opt a model without a built-in default (for example | ||
| `gpt-5.4-nano`) into Responses, set `"modelAdapters": { "gpt-5.4-nano": "openai-responses" }`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe Chat Completions as the default, not an invariant.
Line 320 says that every other Copilot model stays on Chat Completions. The resolver permits an explicit modelAdapters entry to override the registry default, and the next sentence documents this opt-in for gpt-5.4-nano.
Replace “stays on chat completions” with “defaults to chat completions unless overridden by modelAdapters.”
🤖 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 `@docs-site/src/content/docs/guides/providers.md` around lines 317 - 324,
Update the Copilot routing description around the model adapter precedence to
replace “every other Copilot model stays on chat completions” with wording that
says those models default to Chat Completions unless overridden by an explicit
modelAdapters entry. Preserve the existing built-in GPT-5 routing and
gpt-5.4-nano opt-in example.
Source: Path instructions
| | `adapter` | `string` | One of `openai-chat`, `openai-responses`, `anthropic`, `google`, `kiro`, `cursor`, `azure-openai` (or alias `azure`). | | ||
| | `baseUrl` | `string` | Upstream API base URL. Most built-in fixed endpoints ignore a mismatch; collision-safe key presets preserve an older same-named custom destination. | | ||
| | `responsesPath?` | `string` | Relative resource path for key-auth `openai-responses` requests. It must start with `/` and contain no scheme, query, or fragment. | | ||
| | `supportsServiceTier?` | `boolean` | Tri-state `service_tier` capability. `true`: fast mode may inject and caller values are preserved. `false`: the field is stripped and never injected (the upstream documented as not supporting it must not receive it). Absent: the provider is unclassified — caller-supplied values are preserved untouched and fast mode never injects. The registry classifies canonical OpenAI (`true`), DeepSeek, and Volcengine Ark (`false`); set it explicitly only for custom gateways that genuinely support tiers. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the unsupported-provider classification explicit in every reference locale.
The current wording can assign false only to Volcengine Ark and leave DeepSeek ambiguous. State false separately for both providers.
docs-site/src/content/docs/reference/configuration/providers.md#L55-L55: write “canonical OpenAI (true), DeepSeek (false), and Volcengine Ark (false)”.docs-site/src/content/docs/ja/reference/configuration/providers.md#L44-L44: use the equivalent Japanese wording withDeepSeek (false).docs-site/src/content/docs/ko/reference/configuration/providers.md#L44-L44: use the equivalent Korean wording withDeepSeek (false).
📍 Affects 3 files
docs-site/src/content/docs/reference/configuration/providers.md#L55-L55(this comment)docs-site/src/content/docs/ja/reference/configuration/providers.md#L44-L44docs-site/src/content/docs/ko/reference/configuration/providers.md#L44-L44
🤖 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 `@docs-site/src/content/docs/reference/configuration/providers.md` at line 55,
Update the provider capability wording in
docs-site/src/content/docs/reference/configuration/providers.md:55-55 to
explicitly state canonical OpenAI (true), DeepSeek (false), and Volcengine Ark
(false). Apply the equivalent Japanese wording including DeepSeek (false) in
docs-site/src/content/docs/ja/reference/configuration/providers.md:44-44 and the
equivalent Korean wording in
docs-site/src/content/docs/ko/reference/configuration/providers.md:44-44.
Source: Path instructions
| // Fast mode override for OpenAI-routed models, only where the provider's Responses | ||
| // route documents `service_tier` support (capability gate below strips everywhere else). | ||
| if (config.fastMode !== undefined && route.provider.adapter === "openai-responses" && route.provider.supportsServiceTier === true) { | ||
| const tier = config.fastMode ? "priority" : undefined; | ||
| if (parsed._rawBody && typeof parsed._rawBody === "object") { | ||
| if (tier) (parsed._rawBody as Record<string, unknown>).service_tier = tier; | ||
| else delete (parsed._rawBody as Record<string, unknown>).service_tier; | ||
| } | ||
| parsed.options.serviceTier = tier; | ||
| } | ||
| applyServiceTierGate(route.provider, parsed._rawBody, parsed.options); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/server/responses/core.ts \
--match 'applyFinalRouteRequestNormalization|applyServiceTierGate' \
--view expanded
rg -n -C 6 \
'requestedServiceTier|configuredServiceTier|serviceTier|applyServiceTierGate|applyFinalRouteRequestNormalization' \
src/server/responses/core.ts tests/service-tier-capability.test.tsRepository: lidge-jun/opencodex
Length of output: 12444
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- normalization and logging code ---'
sed -n '772,870p' src/server/responses/core.ts
sed -n '1235,1390p' src/server/responses/core.ts
printf '%s\n' '--- RequestLogContext definition and service-tier consumers ---'
rg -n -C 8 \
'interface RequestLogContext|type RequestLogContext|requestedServiceTier|configuredServiceTier|requestedSpeedLabel|configuredSpeedLabel|modelSupportsServiceTier|requestLogSpeedLabel' \
src testsRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- capability classification implementation ---'
rg -n -C 12 \
'function catalogModelSupportsServiceTier|const catalogModelSupportsServiceTier|catalogModelSupportsServiceTier' \
src/codex src
printf '%s\n' '--- service-tier live-path tests ---'
sed -n '80,190p' tests/service-tier-capability.test.ts
printf '%s\n' '--- exact request-log finalization path ---'
sed -n '760,825p' src/server/request-log.tsRepository: lidge-jun/opencodex
Length of output: 21214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
source = Path("src/server/responses/core.ts").read_text()
request_log = Path("src/server/request-log.ts").read_text()
initial = source.index("logCtx.requestedServiceTier = parsed.options.serviceTier;")
gate = source.index("applyServiceTierGate(route.provider, parsed._rawBody, parsed.options);")
classification = source.index("logCtx.modelSupportsServiceTier = catalogModelSupportsServiceTier(", gate)
gate_impl = request_log.index("export function catalogModelSupportsServiceTier")
assert initial < gate < classification
assert "options.serviceTier = undefined;" in source[source.index("export function applyServiceTierGate"):source.index("export async function handleResponses")]
assert "logCtx.requestedServiceTier" not in source[gate:classification]
assert "logCtx.requestedSpeedLabel" not in source[gate:classification]
# Demonstrate the stale-state transition represented by the source:
options = {"serviceTier": "priority"}
log_ctx = {
"requestedServiceTier": options["serviceTier"],
"requestedSpeedLabel": "fast",
"configuredServiceTier": None,
}
del options["serviceTier"]
assert log_ctx["requestedServiceTier"] == "priority"
assert log_ctx["requestedSpeedLabel"] == "fast"
assert options.get("serviceTier") is None
print("ordered snapshot -> gate -> classification: stale requested logging fields remain")
print("configuredServiceTier is an independent fallback field and is not cleared by the gate")
PY
printf '%s\n' '--- service-tier cost precedence ---'
rg -n -C 8 \
'function effectiveServiceTier|const effectiveServiceTier|effectiveServiceTier\\(' \
src testsRepository: lidge-jun/opencodex
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
source = Path("src/server/responses/core.ts").read_text()
request_log = Path("src/server/request-log.ts").read_text()
initial = source.index("logCtx.requestedServiceTier = parsed.options.serviceTier;")
gate = source.index("applyServiceTierGate(route.provider, parsed._rawBody, parsed.options);")
classification = source.index("logCtx.modelSupportsServiceTier = catalogModelSupportsServiceTier(", gate)
gate_impl = source.index("export function applyServiceTierGate")
assert initial < gate < classification
assert "options.serviceTier = undefined;" in source[gate_impl:source.index("export async function handleResponses")]
assert "logCtx.requestedServiceTier" not in source[gate:classification]
assert "logCtx.requestedSpeedLabel" not in source[gate:classification]
options = {"serviceTier": "priority"}
log_ctx = {
"requestedServiceTier": options["serviceTier"],
"requestedSpeedLabel": "fast",
"configuredServiceTier": None,
}
del options["serviceTier"]
assert log_ctx["requestedServiceTier"] == "priority"
assert log_ctx["requestedSpeedLabel"] == "fast"
assert options.get("serviceTier") is None
print("ordered snapshot -> gate -> classification: stale requested logging fields remain")
print("configuredServiceTier is an independent fallback field and is not cleared by the gate")
PY
printf '%s\n' '--- service-tier cost precedence ---'
rg -n -C 8 \
'function effectiveServiceTier|const effectiveServiceTier|effectiveServiceTier\\(' \
src testsRepository: lidge-jun/opencodex
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
source = Path("src/server/responses/core.ts").read_text()
for needle in [
"logCtx.requestedServiceTier = parsed.options.serviceTier;",
"applyServiceTierGate(route.provider, parsed._rawBody, parsed.options);",
"logCtx.modelSupportsServiceTier = catalogModelSupportsServiceTier(",
"export function applyServiceTierGate(",
"options.serviceTier = undefined;",
]:
print(f"{needle!r}: {source.find(needle)}")
start = source.find("export function applyServiceTierGate(")
end = source.find("export async function handleResponses(", start)
print("--- gate implementation ---")
print(source[start:end])
PYRepository: lidge-jun/opencodex
Length of output: 883
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
source = Path("src/server/responses/core.ts").read_text()
for needle in [
"logCtx.requestedServiceTier = parsed.options.serviceTier;",
"applyServiceTierGate(route.provider, parsed._rawBody, parsed.options);",
"logCtx.modelSupportsServiceTier = catalogModelSupportsServiceTier(",
"export function applyServiceTierGate(",
"options.serviceTier = undefined;",
]:
print(f"{needle!r}: {source.find(needle)}")
start = source.find("export function applyServiceTierGate(")
end = source.find("export async function handleResponses(", start)
print("--- gate implementation ---")
print(source[start:end])Repository: lidge-jun/opencodex
Length of output: 970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
source = Path("src/server/responses/core.ts").read_text()
handle_start = source.index("export async function handleResponses(")
normalization_start = source.index("async function applyFinalRouteRequestNormalization(")
normalization_end = source.index("export async function handleComboResponses(", normalization_start)
normalization = source[normalization_start:normalization_end]
handle = source[handle_start:]
snapshot = handle.index("logCtx.requestedServiceTier = parsed.options.serviceTier;")
normalization_call = handle.index("await applyFinalRouteRequestNormalization(")
assert snapshot < normalization_call
gate = normalization.index("applyServiceTierGate(route.provider, parsed._rawBody, parsed.options);")
classification = normalization.index("logCtx.modelSupportsServiceTier = catalogModelSupportsServiceTier(", gate)
assert gate < classification
assert "logCtx.requestedServiceTier" not in normalization[gate:classification]
assert "logCtx.requestedSpeedLabel" not in normalization[gate:classification]
gate_start = source.index("export function applyServiceTierGate(")
gate_end = source.index("export async function handleResponses(", gate_start)
assert "options.serviceTier = undefined;" in source[gate_start:gate_end]
options = {"serviceTier": "priority"}
log_ctx = {
"requestedServiceTier": options["serviceTier"],
"requestedSpeedLabel": "fast",
"configuredServiceTier": None,
}
del options["serviceTier"]
assert options.get("serviceTier") is None
assert log_ctx["requestedServiceTier"] == "priority"
assert log_ctx["requestedSpeedLabel"] == "fast"
print("snapshot precedes normalization call")
print("gate clears parsed.options.serviceTier but not RequestLogContext.requestedServiceTier/requestedSpeedLabel")
print("configuredServiceTier remains an independent fallback field")
PY
printf '%s\n' '--- effective service-tier precedence ---'
rg -n -C 10 \
'effectiveServiceTier' src testsRepository: lidge-jun/opencodex
Length of output: 13246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
source = Path("src/server/responses/core.ts").read_text()
handle_start = source.index("export async function handleResponses(")
normalization_start = source.index("async function applyFinalRouteRequestNormalization(")
normalization_end = source.index("export async function handleComboResponses(", normalization_start)
normalization = source[normalization_start:normalization_end]
handle = source[handle_start:]
snapshot = handle.index("logCtx.requestedServiceTier = parsed.options.serviceTier;")
normalization_call = handle.index("await applyFinalRouteRequestNormalization(")
assert snapshot < normalization_call
gate = normalization.index("applyServiceTierGate(route.provider, parsed._rawBody, parsed.options);")
classification = normalization.index(
"logCtx.modelSupportsServiceTier = catalogModelSupportsServiceTier(",
gate,
)
assert gate < classification
assert "logCtx.requestedServiceTier" not in normalization[gate:classification]
assert "logCtx.requestedSpeedLabel" not in normalization[gate:classification]
gate_start = source.index("export function applyServiceTierGate(")
gate_end = source.index("export async function handleResponses(", gate_start)
assert "options.serviceTier = undefined;" in source[gate_start:gate_end]
options = {"serviceTier": "priority"}
log_ctx = {
"requestedServiceTier": options["serviceTier"],
"requestedSpeedLabel": "fast",
"configuredServiceTier": None,
}
del options["serviceTier"]
assert options.get("serviceTier") is None
assert log_ctx["requestedServiceTier"] == "priority"
assert log_ctx["requestedSpeedLabel"] == "fast"
print("snapshot precedes normalization call")
print("gate clears parsed.options.serviceTier but not RequestLogContext.requestedServiceTier/requestedSpeedLabel")
print("configuredServiceTier remains an independent fallback field")
PY
printf '%s\n' '--- effective service-tier precedence ---'
rg -n -C 10 'effectiveServiceTier' src testsRepository: lidge-jun/opencodex
Length of output: 13246
Synchronize request logging after applyServiceTierGate
handleResponses snapshots requestedServiceTier and requestedSpeedLabel at src/server/responses/core.ts:1266-1267 before normalization. When applyServiceTierGate at lines 812 and 1158-1167 removes an unsupported provider’s service_tier, those fields remain stale. Lines 864 and src/server/request-log.ts:799-803 can then classify and persist a tier that was not sent upstream. Synchronize the requested fields and speed label after the gate, while keeping configuredServiceTier as an independent fallback, and add a regression test.
🤖 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 `@src/server/responses/core.ts` around lines 802 - 812, Update the
normalization paths in handleResponses around applyServiceTierGate, including
the fast-mode path and the other gate call, so requestedServiceTier and
requestedSpeedLabel are refreshed from the post-gate parsed options/body rather
than retaining pre-normalization snapshots. Preserve configuredServiceTier as
the independent fallback, ensure request classification and persistence reflect
the tier actually sent upstream, and add a regression test covering removal of
an unsupported service_tier.
…CTOU, canonical depth, frame pinning, abandoned budgets, docs) - prompt-journal: the journal temp hardens required (fail closed like the token/tray writers — it carries full config.toml bytes); durableDelete propagates non-ENOENT failures; recovery revalidates each target immediately before its own restore and maps delete failures to recovery_required instead of lying committed/rolled-back. - antigravity canonicalization caps recursion depth at 128 (byte/keys budgets did not stop a nesting bomb). - Cursor framing: payloads <= 64 KiB are copied, not viewed, so small frames never pin the 32 MiB backlog buffer in the work queue. - bridge: an abandoned stream (never read, never cancelled) gets an unref'd watchdog that disposes its owned default budget instead of holding it in liveBudgets for the process lifetime. - README: weekly re-arm bound documented on the star surface too. - chore: untrack go/internal/cli/config_parity.go again (re-added by a broad add); move the completed wt2 unit to devlog/_fin.
|
CodeRabbit round folded (a86ee03):
Notes on the remaining comments: the devlog pre-disclosure flags (wt1/wt4 execution notes) pass the repo's own test — every defect they describe already has a public diff (the original PRs and this one), so they disclose nothing new and stay as implementation records. The other devlog plan-doc nits belong to sibling workstreams outside this PR's scope. The abandoned-budget alternative you suggested (defer to first |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
devlog/_fin/260802_wt2_zero_leak_bounds/050_fix_antigravity_key_identities.md (1)
8-10: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse UTF-16 code units in the replay-key contract.
Line 8 says the keys use length-prefixed UTF-8 components, but Line 35 and the implemented contract use length-prefixed UTF-16 code units. These encodings differ for lone surrogates. Keep the documentation aligned with the implementation to avoid reintroducing cross-session key collisions.
Proposed documentation fix
- - `replayKey` (:57): SHA-256 hex over LENGTH-PREFIXED UTF-8 components + - `replayKey` (:57): SHA-256 hex over LENGTH-PREFIXED UTF-16 code-unit components🤖 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/_fin/260802_wt2_zero_leak_bounds/050_fix_antigravity_key_identities.md` around lines 8 - 10, Update the replayKey and functionCallKey documentation to describe length-prefixed UTF-16 code-unit components instead of UTF-8, matching the implemented contract and existing documentation. Preserve the stated fixed-length hashing and canonicalization behavior.devlog/_fin/260802_wt2_zero_leak_bounds/045_fix_blob_id_keys.md (1)
11-14: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRecord the implemented blob-ID key contract.
Line 12 still leaves a reject-or-digest decision and typed errors unresolved. The supplied
070_campaign_close.mdrecord states that IDs up to 64 bytes useh:plus hex, while larger IDs used:plus SHA-256 hex. Update this final record to describe that contract and its shared store/lookup derivation.Proposed documentation fix
- - Admission boundary ... either (a) rejected typed (`blob_id_invalid`/`blob_id_too_large`) or (b) stored under a fixed-size derived key `sha256(id)` ... + - Admission boundary ... IDs up to 64 bytes use `h:` plus hex; larger IDs use `d:` plus SHA-256 hex. Apply the same derivation to storage and lookup ...🤖 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/_fin/260802_wt2_zero_leak_bounds/045_fix_blob_id_keys.md` around lines 11 - 14, Update the final record to document the established blob-ID key contract: IDs up to 64 bytes use the h: prefix plus hex encoding, while larger IDs use the d: prefix plus SHA-256 hex. State that store admission and all lookup paths, including getBlobArgs, hydration, and scope pins, use this same derivation, with derived fixed-size key bytes included in byte accounting.src/codex/prompt-journal.ts (1)
220-306: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
recoverIfNeededstill restores/deletes whatever absolute paths the journal names, with no binding to caller-expected targets.Line 220 keeps the signature
recoverIfNeeded(journalPath: string): RecoveryOutcome. Lines 233-234 readrecord.configPathandrecord.storePathstraight from the decoded journal, and lines 269 and 287 pass them directly torestore(), which callsdurableWriteordurableDeleteon those paths. The checksum validated bydecodeJournalis an unkeyed SHA-256 over the journal body: it proves internal consistency, not that this process wrote the journal or that the named paths belong to this store.The revalidation added at lines 260-267 and 278-285 (re-classifying the target immediately before
restore) is a solid TOCTOU narrowing for content drift, but it does not address path authenticity: a crafted or corrupted journal naming an out-of-treeconfigPath/storePath, with apostConfig/postStorehash matching that file's current bytes, still causesrestore()to overwrite or delete an arbitrary file at the service's privilege level. This is the same gap raised in a prior review round and remains unresolved in the shown code.Bind recovery to caller-supplied targets: pass
{ configPath, storePath }from the caller (per the prior review,commitinsrc/codex/prompt-layers.tsalready computes both before invoking recovery) and reject the journal outright when its recorded paths differ from the expected ones, before anyclassify,restore, ordurableDeletecall.🛡️ Proposed fix: bind recovery to caller-supplied targets
-export function recoverIfNeeded(journalPath: string): RecoveryOutcome { +export function recoverIfNeeded( + journalPath: string, + expected: { configPath: string; storePath: string }, +): RecoveryOutcome { const raw = readOrNull(journalPath); if (raw === null) return { ok: true, action: "none" }; const record = decodeJournal(raw); if (record === null) { return { ok: false, error: "recovery_required", detail: `journal at ${journalPath} failed its checksum or is truncated; nothing was read from it`, }; } + // The envelope checksum is not a MAC: it proves internal consistency, not + // authorship. Never write or delete a path the caller did not name. + if (record.configPath !== expected.configPath || record.storePath !== expected.storePath) { + return { + ok: false, + error: "recovery_required", + detail: `journal at ${journalPath} names targets this store does not own`, + }; + } + const config = classify(readOrNull(record.configPath), record.preConfig, record.postConfig);Update the call site in
src/codex/prompt-layers.tsto pass{ configPath, storePath }intorecoverJournal/recoverIfNeeded.As per path instructions for
src/**: "Watch for credential handling: tokens and OAuth material must never be logged or serialized into responses."🤖 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 `@src/codex/prompt-journal.ts` around lines 220 - 306, Update recoverIfNeeded to accept caller-supplied expected configPath and storePath targets, and update the prompt-layers commit call site to pass { configPath, storePath }. Immediately after decoding the journal and before any classify, restore, or durableDelete operation, reject recovery when either recorded path differs from its expected target, returning recovery_required without touching either path.Source: Path instructions
🤖 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/_fin/260802_wt2_zero_leak_bounds/040_fix_cursor_incremental_frames.md`:
- Line 19: Update the framing record around consumeConnectFrames to match the
implementation: document the returned consumedBytes field instead of nextOffset,
and describe the 64 KiB payload copy threshold with subarray views for larger
payloads rather than claiming all frames are slice copies.
In `@devlog/_fin/260802_wt2_zero_leak_bounds/070_campaign_close.md`:
- Line 32: Reconcile the test count in the final validation entry: update the
recorded total to 7,261 to match the supplied PR validation, or explicitly label
7,162 as the historical worktree result if that count must be retained. Keep
only one authoritative current count.
In `@src/bridge.ts`:
- Around line 227-237: The owned-budget watchdog in bridgeToResponsesSSE must
track stream inactivity rather than elapsed stream lifetime, so active streams
cannot be disposed while step() is processing events. Consolidate stream-death
cleanup to clear ownedWatchdog and the heartbeat interval, cancel upstream work,
and dispose the budget on terminal, cancellation, and enqueue-failure paths
including the handlers near lines 318 and 1230. Add a focused regression test
covering an active stream that remains alive beyond the configured watchdog
delay.
In `@tests/bridge.test.ts`:
- Around line 1099-1110: Strengthen the watchdog test around
bridgeToResponsesSSE by asserting translatorLiveBudgetCountForTests() equals
before + 1 immediately after creating the stream, before waiting for
abandonment. Keep the existing post-watchdog assertion that the count returns to
before, and preserve the current cleanup behavior.
---
Duplicate comments:
In `@devlog/_fin/260802_wt2_zero_leak_bounds/045_fix_blob_id_keys.md`:
- Around line 11-14: Update the final record to document the established blob-ID
key contract: IDs up to 64 bytes use the h: prefix plus hex encoding, while
larger IDs use the d: prefix plus SHA-256 hex. State that store admission and
all lookup paths, including getBlobArgs, hydration, and scope pins, use this
same derivation, with derived fixed-size key bytes included in byte accounting.
In
`@devlog/_fin/260802_wt2_zero_leak_bounds/050_fix_antigravity_key_identities.md`:
- Around line 8-10: Update the replayKey and functionCallKey documentation to
describe length-prefixed UTF-16 code-unit components instead of UTF-8, matching
the implemented contract and existing documentation. Preserve the stated
fixed-length hashing and canonicalization behavior.
In `@src/codex/prompt-journal.ts`:
- Around line 220-306: Update recoverIfNeeded to accept caller-supplied expected
configPath and storePath targets, and update the prompt-layers commit call site
to pass { configPath, storePath }. Immediately after decoding the journal and
before any classify, restore, or durableDelete operation, reject recovery when
either recorded path differs from its expected target, returning
recovery_required without touching either path.
🪄 Autofix (Beta)
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: e377b6d5-1509-4bf6-9e4c-8b798185f6b3
📒 Files selected for processing (16)
README.mddevlog/_fin/260802_wt2_zero_leak_bounds/000_plan.mddevlog/_fin/260802_wt2_zero_leak_bounds/001_root_cause_delta.mddevlog/_fin/260802_wt2_zero_leak_bounds/010_implementation.mddevlog/_fin/260802_wt2_zero_leak_bounds/020_fix_responses_state_admission.mddevlog/_fin/260802_wt2_zero_leak_bounds/030_fix_tool_arg_collector_scope.mddevlog/_fin/260802_wt2_zero_leak_bounds/040_fix_cursor_incremental_frames.mddevlog/_fin/260802_wt2_zero_leak_bounds/045_fix_blob_id_keys.mddevlog/_fin/260802_wt2_zero_leak_bounds/050_fix_antigravity_key_identities.mddevlog/_fin/260802_wt2_zero_leak_bounds/060_fix_acl_memo_release.mddevlog/_fin/260802_wt2_zero_leak_bounds/070_campaign_close.mdsrc/adapters/cursor/framing.tssrc/adapters/google-antigravity-replay.tssrc/bridge.tssrc/codex/prompt-journal.tstests/bridge.test.ts
|
|
||
| 1. Replace `pending` with `{ buf, start, end }` (cursor + capacity growth): append copies ONLY the new chunk (grow capacity ≤ cap, compact consumed prefix when `start` crosses a threshold); per-chunk cost O(chunk) amortized. | ||
| 2. Raw cap INCLUDING headers: `end - start + chunk.byteLength > CURSOR_TRANSPORT_MAX_BUFFERED_BYTES` → typed overflow (`cursor_transport`). This closes the tiny-frame/header-flood gap (payload-only accounting missed headers). `transportBufferedBytes` semantics shift from payload-bytes to raw-used-bytes — a STRICTER counter; flow-control thresholds unchanged. | ||
| 3. Drain without remainder copies: add a framing.ts export `consumeConnectFrames(input, start, maxPayloadBytes, availableSlots, reservePayloadCopy)` returning `{ frames, nextOffset }` (same inspect loop + per-frame reservations, NO remainder allocation); advance `start`. Frames stay slice copies with their existing reservation lifecycle. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Align the Cursor framing record with consumeConnectFrames.
Line 19 documents a { frames, nextOffset } result and says that frames remain slice copies. The implementation in src/adapters/cursor/framing.ts returns consumedBytes, copies payloads up to 64 KiB, and uses subarray views for larger payloads. Keep this field name and copy/view threshold accurate because they control caller accounting and backlog-buffer pinning.
Proposed documentation fix
- returning `{ frames, nextOffset }` ... Frames stay slice copies
+ returning `{ frames, consumedBytes }` ... Payloads up to 64 KiB are copied; larger payloads use subarray views📝 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.
| 3. Drain without remainder copies: add a framing.ts export `consumeConnectFrames(input, start, maxPayloadBytes, availableSlots, reservePayloadCopy)` returning `{ frames, nextOffset }` (same inspect loop + per-frame reservations, NO remainder allocation); advance `start`. Frames stay slice copies with their existing reservation lifecycle. | |
| 3. Drain without remainder copies: add a framing.ts export `consumeConnectFrames(input, start, maxPayloadBytes, availableSlots, reservePayloadCopy)` returning `{ frames, consumedBytes }` (same inspect loop + per-frame reservations, NO remainder allocation); advance `start`. Payloads up to 64 KiB are copied; larger payloads use subarray views. |
🤖 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/_fin/260802_wt2_zero_leak_bounds/040_fix_cursor_incremental_frames.md`
at line 19, Update the framing record around consumeConnectFrames to match the
implementation: document the returned consumedBytes field instead of nextOffset,
and describe the 64 KiB payload copy threshold with subarray views for larger
payloads rather than claiming all frames are slice copies.
|
|
||
| - `bun x tsc --noEmit`: PASS (repeated across all cycles). | ||
| - `bun run privacy:scan`: PASS (wp1; re-verified wp7 by reviewer). | ||
| - Full `bun run test`: **PASS** — 7162 pass, 8 skip, 0 fail, 34,386 expect() calls across 487 files (256.21s), recorded 2026-08-02 on this branch. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the final test count.
Line 32 records 7,162 passing tests, while the supplied PR validation reports 7,261 passing tests. Keep one authoritative count. If 7,162 is the historical worktree result, label it as historical; otherwise update the line to 7,261.
🤖 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/_fin/260802_wt2_zero_leak_bounds/070_campaign_close.md` at line 32,
Reconcile the test count in the final validation entry: update the recorded
total to 7,261 to match the supplied PR validation, or explicitly label 7,162 as
the historical worktree result if that count must be retained. Keep only one
authoritative current count.
| // A dropped stream (never read, never cancelled) reaches no terminal path, | ||
| // so the owned budget would sit in liveBudgets for the process lifetime. | ||
| // One unref'd watchdog per owned budget bounds that to a timeout and clears | ||
| // itself on any settle (the delay is test-overridable). | ||
| const ownedWatchdog = ownsBudget | ||
| ? setTimeout(() => disposeOwnedBudget(), ownedBudgetAbandonedMs) | ||
| : undefined; | ||
| ownedWatchdog?.unref?.(); | ||
| const clearOwnedWatchdog = () => { | ||
| if (ownedWatchdog !== undefined) clearTimeout(ownedWatchdog); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the owned-budget watchdog follow stream liveness.
ownedWatchdog starts when bridgeToResponsesSSE creates the stream and remains armed until terminal or cancellation. A valid stream that runs longer than ten minutes can therefore call budget.dispose() while step() still processes events. Later reservations or retained-byte charges can then run against a disposed budget and abort a valid request.
The cleanup is also incomplete when enqueueing fails. Lines 318 and 1230 set closed and dispose the budget, but they do not clear ownedWatchdog or the heartbeat interval. A never-read stream leaves the heartbeat interval running after the watchdog fires.
Use one lifecycle cleanup path. Arm the watchdog only for the unconsumed state, or make it an inactivity watchdog. Clear both timers and cancel upstream work on every stream-death path. Add a regression test for an active stream that outlives the configured delay.
As per path instructions, behavior changes in src/ require a focused regression test near the existing tests for that subsystem.
Also applies to: 318-318, 1230-1230
🤖 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 `@src/bridge.ts` around lines 227 - 237, The owned-budget watchdog in
bridgeToResponsesSSE must track stream inactivity rather than elapsed stream
lifetime, so active streams cannot be disposed while step() is processing
events. Consolidate stream-death cleanup to clear ownedWatchdog and the
heartbeat interval, cancel upstream work, and dispose the budget on terminal,
cancellation, and enqueue-failure paths including the handlers near lines 318
and 1230. Add a focused regression test covering an active stream that remains
alive beyond the configured watchdog delay.
Source: Path instructions
| test("an abandoned stream's owned budget is disposed by the watchdog", async () => { | ||
| resetTranslatorAggregateForTests(); | ||
| setOwnedBudgetAbandonedMsForTests(10); | ||
| try { | ||
| const before = translatorLiveBudgetCountForTests(); | ||
| bridgeToResponsesSSE(replay([{ type: "text_delta", text: "never read" }]), "mock/test-model"); | ||
| await new Promise(resolve => setTimeout(resolve, 40)); | ||
| expect(translatorLiveBudgetCountForTests()).toBe(before); | ||
| } finally { | ||
| setOwnedBudgetAbandonedMsForTests(null); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The watchdog test does not prove a budget was ever created before checking it is disposed.
The test at line 1103 captures before = translatorLiveBudgetCountForTests(), then at line 1104 calls bridgeToResponsesSSE(...) without options, so ownsBudget is true and createTranslatorBudget() runs synchronously inside bridgeToResponsesSSE (per src/bridge.ts, the budget assignment happens at function-body scope, before any stream is read). After the 40 ms wait, line 1106 asserts translatorLiveBudgetCountForTests() equals before.
The gap: this assertion also passes if the owned budget is never created at all, for example a future regression in the ownsBudget = !options?.translatorBudget computation, or in the watchdog wiring itself. Since before and the post-wait count would then trivially match, this specific regression — the exact one the watchdog feature and this PR round target — would slip through undetected.
Add an intermediate assertion right after creating the stream, confirming the count rose to before + 1, before waiting for the watchdog to bring it back down.
🐛 Proposed strengthening of the regression assertion
const before = translatorLiveBudgetCountForTests();
bridgeToResponsesSSE(replay([{ type: "text_delta", text: "never read" }]), "mock/test-model");
+ // Prove the owned budget was actually created before checking it is disposed;
+ // otherwise a regression that skips creation would pass this test vacuously.
+ expect(translatorLiveBudgetCountForTests()).toBe(before + 1);
await new Promise(resolve => setTimeout(resolve, 40));
expect(translatorLiveBudgetCountForTests()).toBe(before);Separately, the 10 ms abandonment timeout (line 1101) paired with a 40 ms real-timer wait (line 1105) gives only a 4x margin; on a loaded CI runner this class of real-timer test can flake. Not a blocker given the existing margin, but worth keeping in mind if this test becomes flaky.
As per path instructions for tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
📝 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.
| test("an abandoned stream's owned budget is disposed by the watchdog", async () => { | |
| resetTranslatorAggregateForTests(); | |
| setOwnedBudgetAbandonedMsForTests(10); | |
| try { | |
| const before = translatorLiveBudgetCountForTests(); | |
| bridgeToResponsesSSE(replay([{ type: "text_delta", text: "never read" }]), "mock/test-model"); | |
| await new Promise(resolve => setTimeout(resolve, 40)); | |
| expect(translatorLiveBudgetCountForTests()).toBe(before); | |
| } finally { | |
| setOwnedBudgetAbandonedMsForTests(null); | |
| } | |
| }); | |
| test("an abandoned stream's owned budget is disposed by the watchdog", async () => { | |
| resetTranslatorAggregateForTests(); | |
| setOwnedBudgetAbandonedMsForTests(10); | |
| try { | |
| const before = translatorLiveBudgetCountForTests(); | |
| bridgeToResponsesSSE(replay([{ type: "text_delta", text: "never read" }]), "mock/test-model"); | |
| // Prove the owned budget was actually created before checking it is disposed; | |
| // otherwise a regression that skips creation would pass this test vacuously. | |
| expect(translatorLiveBudgetCountForTests()).toBe(before + 1); | |
| await new Promise(resolve => setTimeout(resolve, 40)); | |
| expect(translatorLiveBudgetCountForTests()).toBe(before); | |
| } finally { | |
| setOwnedBudgetAbandonedMsForTests(null); | |
| } | |
| }); |
🤖 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 `@tests/bridge.test.ts` around lines 1099 - 1110, Strengthen the watchdog test
around bridgeToResponsesSSE by asserting translatorLiveBudgetCountForTests()
equals before + 1 immediately after creating the stream, before waiting for
abandonment. Keep the existing post-watchdog assertion that the count returns to
before, and preserve the current cleanup behavior.
Source: Path instructions
…en dist is absent The integrations CSRF test fetches the real served page, which only exists when gui/dist is built. CI runs bun test BEFORE GUI build, so the page fell back to the JSON root payload and the meta assertion failed on every fresh checkout (ubuntu + macos on the merge ref). Build once in-file when the artifact is missing.
boundedInjectedInstruction sliced UTF-16 code units at the character budget, which could leave a lone high surrogate at the cut; encoding that substitutes U+FFFD into the injected instruction. The slice now drops the dangling half pair instead. Regression pins the astral char at the exact budget boundary. Found while hunting #904 (Korean U+FFFD reports) — kiro-specific, not the reporter's kimi/opus path.
Three more lone-surrogate U+FFFD producers found by the #904 coverage audit: the v1 compaction retained tail could begin on a lone low surrogate when the budget cut landed mid-pair; the kiro reasoning carry could emit a delta ending on a lone high surrogate; and kiro tool-description truncation could end on one. Each cut now shifts by one code unit to keep pairs whole, with boundary-pinned regressions (the 80,001-code-unit astral repro for compaction included).
1 participant
Summary
Unified bug-fix campaign: the wt2 retained-state hardening plus all four planned workstreams (wt1 update path, wt4 server/config, wt5 Windows service, wt3 provider wire), reviewed per workstream by independent adversarial reviewers and verified end-to-end. Supersedes #840 #841 #843 #844 #845 #847 (closed earlier) and #871 #886 #887 #889 (closed as each workstream landed here).
wt2 — retained-state bounds (tracker #820 wave 2)
upstream_erroreverywhere (was 413 ×4); no unbounded no-budget path; cancel-race fixes.frame_incomplete; terminal backlog lease owner.h:/d:), key bytes counted + classified; zero-payload blobs stay evictable.wt1 — update path
major.minor.patchcore on the latest channel (registry target side stays strict).wt4 — server/config
wt5 — Windows service
wt3 — provider wire
modelAdapterspreserved.service_tiertri-state capability gate (truepermits injection,falsestrips,undefinedpreserves caller values; docs aligned in EN + ja/ko/ru/zh-cn) and reasoning_content replay for tool-call continuations.[1m]picker rows only for authoritative ≥1M windows (no 372K route marked).Validation
bun run test: 7,261 pass, 8 skip, 0 fail (full suite on the merge head); every workstream's focused suites re-run after each stackbun x tsc --noEmit: passbun run privacy:scan: passNotes
Summary by CodeRabbit
New Features
ocx configcommands for viewing, editing, validating, importing, and exporting configuration.Bug Fixes