Codex/260731 pr merge round - #800
Closed
batuchek68-ux wants to merge 24 commits into
Closed
Conversation
The dev checkout is mid-optimization, so this round stages on codex/260731-pr-merge-round instead of dev. The user merges it into dev and releases once that unit lands. Four reviewers triaged every open PR against origin/dev=356924263 on disjoint slices, and the load-bearing finding is that seven PRs are already fixed on dev by different commits. Merging those heads now would REVERT the newer work. lidge-jun#736 is the clearest case: dev carries decodeSchtasksOutput() at src/service.ts:364-393 because schtasks /query /xml emits UTF-16LE, and the PR head deletes that block and restores encoding: "utf8" -- landing it reopens lidge-jun#722. A clean merge-tree is not permission to merge. An adversarial audit then returned five blockers, all folded in rather than argued away. Two changed what actually gets merged. lidge-jun#744 was routed as a routine catalog fix, but 59d95c0 and 39543a3 change OAuth reconciliation, persist provider settings, and move token resolution around the static branch -- security review per MAINTAINERS.md, so it left the batch. lidge-jun#781's topic commit also swaps three /api/logs test files onto a logsFromApiBody helper that accepts both the array and the {logs} envelope; adopting it would pre-accept the very contract batch B rejects, so only the Anthropic file and its own test come across. The audit also caught three PRs missing from a matrix that claimed to be complete (lidge-jun#750, lidge-jun#746, lidge-jun#644), and lidge-jun#644 additionally carries .codexclaw/goalplans/** and two .DS_Store files. Nothing is merged yet. Batch A is six PRs, batch B rebuilds three whose implementations no longer fit the tree, and 20+ are held for security review, provider evidence, or their own cycle -- each with what would unblock it recorded.
`ocx init` under a pipe span at ~100% CPU forever once stdin reached EOF. `readline.question()` never invokes its callback when the stream closes, so the promise stayed pending, the interface stayed open, and the event loop kept spinning with nothing left to read. The prompt now rejects on EOF and the CLI maps that to a clean non-zero exit instead of a hang. The regression test closes stdin, requires the process to exit 1 within 8 seconds, and asserts no config file was written -- so a silent regression to the hanging behavior fails on the timeout rather than passing quietly. Co-authored-by: bitkyc08-arch <bitkyc08@gmail.com>
…ge-jun#709) Remote clients had no way to read the catalog OpenCodex generates; the reader existed (`src/codex/catalog/parsing.ts`) but nothing exposed it over the management API, so anything not sharing the filesystem had to guess at the model set. The route returns the on-disk bytes and 404s when the file is absent. It deliberately does not trigger a runtime probe: `/api/catalog` is a read of what was generated, not a reason to spawn `codex --version` on the request path, which is the failure mode lidge-jun#606 just closed. Both cases are pinned -- exact bytes with no probe, and the missing-file 404. Co-authored-by: bitkyc08-arch <bitkyc08@gmail.com>
…idge-jun#767) Claude Desktop's model picker showed every `native/*` row, which is noise for users who route through OpenCodex precisely to reach other providers. `claudeCode.desktopNativeModels` gates them, and the gate is applied at every Desktop surface that emits the list -- discovery and profile export both -- rather than only the one the report happened to name. Grok natives stay: they are reachable through Desktop and hiding them would remove working models. The regression test fails without the change by finding `native/*` in both the show and export output. Co-authored-by: bitkyc08-arch <bitkyc08@gmail.com>
A signup-credit row in the free directory, not a canonical registry preset. That distinction is the point: MAINTAINERS.md sets a credential-destination evidence bar for `PROVIDER_REGISTRY` entries, and an inert directory reference is the documented placement when that evidence is not fully assembled. The submission carries a dated end-to-end check (2026-07-30): the OpenAI-shaped `/v1/models` catalog and a real chat completion, both recorded inline next to the entry. Contributed by a Baseten employee; disclosed, which under MAINTAINERS.md is neither disqualifying nor a reason to lower the bar. Promotion to a canonical preset would still need ToS, operating legal entity, and a named maintenance owner -- see lidge-jun#653 for that path. Co-authored-by: bitkyc08-arch <bitkyc08@gmail.com>
Forced and add-account Kiro login shell out to the local CLI by bare name. lidge-jun#710 taught Windows where the SQLite store lives, so a Windows box can import tokens while PATH still has no `kiro-cli` -- and then login fails on a machine that is otherwise correctly installed. The resolver tries PATH first (both `PATH` and `Path`, since Windows uses the latter), then the layouts the installers actually produce: `%LOCALAPPDATA%\Kiro-Cli` and `C:\Program Files\Kiro-Cli` on Windows, `~/.local/bin` and the usual prefixes elsewhere. With nothing found it returns the bare command so spawn still reports the original error rather than a path this code invented. Two additions on top of the contributed patch. `existsSync` alone accepts a *directory* named `kiro-cli`, which then reaches spawn() and fails with EACCES instead of falling through to the next candidate -- candidates now have to be files. And the tests only ever injected `pathEntries`, so the environment parsing itself was never exercised; a case now drives `Path` and `PATH` through the real code. Both new tests were checked by ablation: removing the `isFile` guard fails the directory case (6 pass / 1 fail) and restoring it returns 7/7, so neither passes vacuously. Windows behavior is unverifiable from macOS; the matrix CI on this branch is what confirms it. Co-authored-by: bitkyc08-arch <bitkyc08@gmail.com>
…ks (lidge-jun#765) Four defects an Anthropic-compatible relay exposes, all in the same adapter. A `baseUrl` pasted as `https://host/v1/messages` -- which is what the provider's own docs show -- got `/v1/messages` appended again. The URL is now normalized so both the bare-origin and full-endpoint forms resolve to one request path. `input_json_delta` was accepted regardless of the open block, so a relay emitting JSON deltas outside a `tool_use` block corrupted the assembled arguments; the delta is now scoped to `tool_use`. A `tool_use` block arriving without an `id` was forwarded with an empty one, which downstream treats as unpaired -- a `toolu_` id is synthesized instead. And a stream that reached EOF carrying a `message_delta.stop_reason` but no `message_stop` now settles from that stop reason rather than hanging. Two changes on top of the contributed patch: `toolUseArguments` re-encoded an unparseable string input as a JSON *string*, so `"not json at all"` arrived where the tool contract requires an object. That is the double-encoding half of lidge-jun#765 and it survived the original fix. It degrades to `{}` now, which still fails -- but inside the tool's own argument validation instead of as a type error. A test pins it. `sawContent` was assigned in six places and read in none; removed. The contributed branch also swapped three `/api/logs` test files onto a `logsFromApiBody` helper that accepts both the bare array and a `{logs}` envelope. That helper is not brought across: it would pre-accept the response-shape change this round has not decided on, and a matcher that tolerates both shapes cannot fail when the shape regresses. Only the adapter and its own test come over. lidge-jun#765 stays open -- it asks for broader relay quirk tolerance than these four fixes. Note for whoever runs the suite: `tests/claude-messages-endpoint.test.ts` and `tests/claude-native-passthrough.test.ts` already fail together on an unmodified tree ("unmapped claude model + sk-ant credential passes through verbatim"), and pass individually. That interference predates this commit. Co-authored-by: bitkyc08-arch <bitkyc08@gmail.com>
…stream
The review of the batch caught a real inconsistency I introduced. Making
`toolUseArguments` degrade an unparseable string to `{}` fixed the
non-stream path only: the streaming path forwarded every `partial_json`
fragment verbatim, so one malformed relay payload became `arguments:"not
json"` through `parseStream` and `"{}"` through `parseResponse`. Two
answers to the same defect, and the streaming one is what a live client
actually hits.
Fragments are now buffered per tool block and validated once at
`content_block_stop`, through the same helper. A tool call cannot execute
before its arguments are complete, so buffering costs nothing.
Three tests, each verified by ablation rather than assumed:
- malformed streamed fragments degrade to `{}` (reverting the buffering
fails it)
- valid fragments split mid-token reassemble intact, so buffering does
not corrupt the normal case (also fails when reverted)
- EOF carrying `message_delta.stop_reason` but no `message_stop` settles
with that stop reason instead of erroring
That third one closes the second review finding: the two existing EOF
tests assert the pre-existing error behavior and never send a stop
reason, so they stayed green with the new fallback reverted -- they were
not testing the change they shipped with. Disabling the fallback now
fails this test (14 pass / 1 fail) and restoring it returns 15/15.
Suites: anthropic-stream-hardening, anthropic-adapter, anthropic-hardening,
anthropic-compatible-stream, anthropic-tail-guard, anthropic-empty-content,
anthropic-thinking-signature, anthropic-reasoning, anthropic-tool-schema,
claude-messages-endpoint, anthropic-tool-reorder -- 115 pass, 0 fail.
Co-authored-by: Wibias <37517432+Wibias@users.noreply.github.com>
…med ones
The previous commit fixed the stream/non-stream inconsistency by buffering
`input_json_delta` fragments until block close. That was the wrong repair
and the review caught it: `src/bridge.ts:628` maps every adapter delta to
a client-visible `response.function_call_arguments.delta` frame, and
`tests/responses-stream-tool-events.test.ts:30` pins that split fragments
stay split. Withholding them left a started function call showing empty
arguments until the block ended -- a protocol regression traded for a
consistency fix.
Fragments flow again as they arrive. The assembled copy is still kept, but
only to check at `content_block_stop` whether the payload parses. When it
does not, the turn errors instead of emitting `tool_call_end`: the
fragments are already downstream and cannot be repaired the way
`toolUseArguments` repairs the non-stream path, and a client that received
`not json` must not then be told the call completed normally.
`streamedToolArgumentsParse` treats an empty buffer as valid, because a
no-argument tool call sends no `input_json_delta` at all -- checking for
`"{}"` instead would have failed those and also swallowed a legitimate
empty-object argument.
Four tests, each checked by ablation rather than assumed:
- malformed fragments error the turn and emit no `tool_call_end` or `done`
(disabling the guard: 16 pass / 1 fail)
- valid fragments arrive as two separate deltas, not one joined string --
this is the contract the previous commit broke (removing incremental
forwarding: 15 pass / 2 fail)
- two `tool_use` blocks in one message keep separate buffers, so the
second cannot inherit the first's JSON
- a block with no fragments is not treated as malformed
Suites: anthropic-stream-hardening, responses-stream-tool-events,
anthropic-adapter, anthropic-compatible-stream, anthropic-hardening,
anthropic-tail-guard, claude-messages-endpoint -- 64 pass, 0 fail.
Co-authored-by: Wibias <37517432+Wibias@users.noreply.github.com>
…he rest
Two attempts at making the streaming path agree with the non-stream path
about malformed tool arguments, both rejected by review for a real reason.
Stopping at the third rather than pushing another patch at the same spot.
Attempt one buffered `input_json_delta` fragments and validated at block
close. That broke the streaming contract: `src/bridge.ts:628` turns each
adapter delta into a client-visible
`response.function_call_arguments.delta` frame, and
`tests/responses-stream-tool-events.test.ts:30` pins that split fragments
stay split. A started call would show empty arguments until block close.
Attempt two kept fragments flowing and errored the turn when the assembled
JSON did not parse. Also wrong: the bridge's `error` case closes an open
tool call anyway (`src/bridge.ts:771`), emitting
`response.function_call_arguments.done` and an `output_item.done` with
`status:"completed"` BEFORE `response.failed`. The client is still told the
call completed.
The fix belongs in the bridge -- a terminal error mode that cancels rather
than closes an open tool call. That is out of scope for an Anthropic
adapter PR, affects every adapter's error path, and touches code whose
comment records that `JSON.parse("")` once poisoned whole sessions. It
deserves its own cycle with that history read first, not a third patch
from this end.
Kept, because both stand on their own: the non-stream `toolUseArguments`
degradation to `{}`, which fixes the half of lidge-jun#765 that is actually
fixable here, and the EOF stop-reason regression test that exposed the two
existing EOF tests as vacuous.
Reverted: every streaming-path change. That path now behaves exactly as it
did before this round, so nothing regressed -- the known defect is simply
still there, and lidge-jun#765 stays open for it.
Reasoning recorded in devlog/_plan/260731_pr_merge_round/021.
The contention warning was not enough. It scrolls past, the run starts anyway, and nothing prevents the next agent from doing the same: four concurrent suites drove load average to 10 and stretched a ~210s run past 13 minutes, which reads as a hang rather than as CPU starvation. The detection was already here and already correct -- it just did not act. `bun run test` now waits for the other runners to finish before starting. Agents working in parallel worktrees each believe they are the only runner, so the serialization has to live in the runner rather than in anyone's discipline. Queueing rather than refusing is deliberate. A hard failure invites `bun test ./tests/` directly, which bypasses this file entirely and puts the contention right back. Waiting is the behavior that survives being worked around. Two escape hatches keep it from becoming its own hang: `OCX_TEST_NO_QUEUE=1` for anyone who genuinely wants overlap, and a 45-minute ceiling after which a holder is presumed wedged and this run proceeds regardless. Verified both directions: with a competing runner present the suite waits and starts nothing (20s observed, no test output), and with the opt-out set the same invocation runs immediately (1 pass).
Four abandoned local runners drove load average to 10 and stretched a 210s suite past 13 minutes, which reads as a hang because starvation produces no error. One earlier report blamed other worktrees for those processes; lsof showed all four were mine. Records both fixes -- the runner now queues instead of warning, and heavy suites go to macmini-cf -- plus the bundle transfer that avoids the pre-push hook running the very suite being offloaded, and the detail that macmini has no rg.
The user's constraint is that their GUI layout must not change. lidge-jun#790's direction is right and its defect is real -- dashboard timestamps render in browser-local time -- but it carries two problems, and only one was about layout. It rewrites /api/logs from an array to a {timeZone, logs} envelope while leaving four array consumers unfixed, and the single test it touches accepts both shapes, so it passes with the production change reverted. The narrower path already exists inside that same PR: it adds timeZone to /api/settings and then never uses it, carrying the same value over two routes. SettingsData already flows through the GUI, so the fix needs no new fetch, no session-cache schema change, and no /api/logs change -- only a timezone argument threaded into two formatters at two call sites. Records the RangeError guard (an ICU-unknown zone would otherwise kill the row render), the browser-TZ != server-TZ test that lidge-jun#790's version could not fail on, and the grep that proves no JSX or CSS moved.
…idge-jun#725) A proxy running in KST viewed from a UTC browser reported every request nine hours off: `formatLogTimestamp` and `formatLogDateTime` called `toLocaleTimeString(localeTag)` with no zone, so the viewer's machine decided what time the server logged something. The zone rides on `/api/settings`, which is already an object and already flows through the GUI. PR lidge-jun#790 fixed the same defect by rewriting `/api/logs` from a bare array into a `{timeZone, logs}` envelope -- while leaving four suites that read that response as an array untouched (server-auth:1623, claude-native-passthrough:119, openai-provider-option-e2e:489, server-403-permission-e2e:86). It also added a session-cache schema that accepts both the old and new shapes. Curiously that PR already added `timeZone` to `/api/settings` and then never read it, carrying one value over two routes. Layout is untouched, which was the constraint here. The GUI diff is two call sites gaining a third argument; every `className`, element and stylesheet is byte-identical. `git diff -- gui/` filtered for className/element/style changes returns the two timestamp lines and nothing else. The formatters swallow `RangeError`: a zone string this browser's ICU build does not know would otherwise take the whole row render down, and a timestamp in the wrong zone beats an empty log list. The fetch is a one-shot on mount rather than part of the 2s log poll, since the server's zone cannot change while the page is open, and an older proxy without the field simply keeps browser-local formatting. Tests, both verified by ablation rather than assumed: - removing `timeZone` from /api/settings fails the settings test (4/1) - dropping the formatter argument fails the rendering test (4/1) That second one is the case lidge-jun#790's own test could not catch: it compared the response to itself and passed with the production change reverted. The new test fixes 00:30 UTC and asserts Seoul reads 9:30 while UTC reads 12:30, so an ignored argument shows up as a wrong hour. There is also a test asserting /api/logs is still a bare array, placed at the source so the contract fails here rather than in four unrelated suites. The detail dialog gets the same zone; leaving it on browser time would put two different times for one request on screen at once. Co-authored-by: Wibias <37517432+Wibias@users.noreply.github.com>
…turns (lidge-jun#796) Ark validates an assistant message's text as a required parameter and reads `""` as absent, so any history containing a tool-call-only assistant fails with `MissingParameter: input.content.text`. Single-turn requests work, which is why this only shows up once an agent loop has run a tool. The `""` it rejects is not accidental. xAI rejects the opposite way -- "Each message must have at least one content element" -- and `""` is what satisfies that, per the comment already sitting on the line. The two provider contracts are in direct conflict, so there is no value that satisfies both and the placeholder has to be host-gated. A single space is non-empty for Ark's validator and semantically empty for the model. The gate follows the file's existing pattern (`isXaiSchemaTarget`, `isKimiSchemaTarget`): an exact hostname set, matched on `URL.hostname` so a provider that merely mentions the string in a path is unaffected. Both current Ark regions are covered. All three placeholder sites move together -- tool-call-only, reasoning-only, and the synthesized orphan assistant -- since Ark's validator sees no difference between them. Every test runs the same input through Ark and a generic host and asserts the two diverge, so a mistake in either direction fails. Verified by ablation: - collapse the gate to always `""` -> 2 pass / 3 fail (Ark cases) - apply the space globally -> 2 pass / 3 fail (control + lookalike cases) - restored -> 5/5 That second one is the failure mode worth guarding: a global placeholder would have made the Ark tests pass while silently breaking xAI. Adjacent suites green: openai-chat-dangling-toolcalls, adapter-usage, openai-chat-parallel-stream -- 50 pass, 0 fail.
…complete
At reader EOF the adapter flushed buffered tool calls and only then judged
whether the stream had terminated cleanly. `flushToolCalls()` emits
`tool_call_end`, so a socket that died mid-argument handed the client
`{"cmd":"l` as a COMPLETED call: a truncation reported as a successful tool
invocation, which is worse than an error because nothing downstream can tell
it apart from a real one.
The integrity check now runs before the flush. This adapter can still decide
at that point because it buffers tool calls; the Anthropic adapter forwards
fragments live and genuinely cannot, which is why the equivalent fix there
was reverted (see devlog 260731_pr_merge_round/021).
A usage frame does not launder it either. Usage counts as a terminal signal
for text streams, and without the separate branch a provider that sent usage
after dying mid-tool would still flush the partial call.
What this deliberately does NOT do is the other half of lidge-jun#735. PR lidge-jun#793 (and
lidge-jun#773 before it, merged then reverted by lidge-jun#792 with no recorded reason)
additionally completes text-only streams that end without a terminal frame.
That is a policy change, not a bug fix: it inverts two existing fail-closed
assertions in tests/openai-chat-eof.test.ts, which currently pin "content
without a terminator is truncation". Whether a relay's dropped terminal frame
should be forgiven is the maintainer's call, and the missing revert rationale
on lidge-jun#792 is exactly the context needed to make it. Attempting both halves here
is how the previous round burned three repair rounds on the Anthropic side.
So the narrowing half lands and the widening half does not. lidge-jun#735 stays open.
Tests, verified by ablation (disable the guard -> 13 pass / 2 fail, restore ->
15/15):
- a half-assembled tool call at EOF errors, emitting no tool_call_end or done
- a trailing usage frame does not rescue it
- a tool call closed by finish_reason still completes normally -- the control,
without which the guard could fire on every well-formed tool turn
Adjacent suites green: adapter-usage, openai-chat-parallel-stream.
…er space
The review blocked the previous commit on the right question: what evidence
says Ark accepts a space? None. I had reasoned from "the field must be
non-empty" and picked the smallest non-empty string, and a validator that
trims whitespace would have rejected it just as it rejects "" -- the fix
would have shipped, passed every local test, and still 400'd against the
live endpoint.
Re-reading Ark's error changes the diagnosis. It names `input.content.text`,
a NESTED path. Ark is not asking for non-empty text; it is asking for the
structured content form, `[{type:"text",text:""}]`, which is where a
`content.text` field exists at all. A bare string of any value never provides
that path. The inner text stays empty, so nothing extra reaches the model --
which also retires the "a space may tokenize" objection rather than arguing
with it.
Every other provider keeps the bare `""` that xAI's validator requires, so
the host gate stays.
Also closes the two smaller review findings: the reasoning-only site now has
direct coverage (it is reachable through the public
`preserveReasoningContentModels` option, not a theoretical branch), and the
EOF suite gains a `[DONE]`-only control. That control passes with the guard
reverted, deliberately -- it pins the early-return path the guard must never
begin intercepting, which a guard-sensitive test cannot express.
Still unverified: no request in this round reached a live Ark endpoint, and
the regional hostname set is the two currently documented chat hosts. Both
are recorded in the issue so a user with Ark credentials can confirm before
this is relied on.
67 pass across volcengine-ark-assistant-content, openai-chat-eof,
openai-chat-dangling-toolcalls, adapter-usage, openai-chat-parallel-stream.
…e itself The commit message said the array form was unconfirmed while the code comment stated it as fact. The review caught the mismatch, and the code comment is the one a future reader will actually find. Ark publishes array content only for multimodal USER input; nothing in its reference covers an assistant history entry, so the shape is inferred from the error's nested parameter path and nothing more. That inference is better motivated than the whitespace guess it replaced, but it is still an inference, and the wording now says so at both the helper and the test header. No behavior change.
…tch too (lidge-jun#758) Ollama's registry entry is marked `allowPrivateNetworkByDefault` because a local model server is what it IS. Config validation read that flag; outbound discovery did not, reading only the operator's `allowPrivateNetwork`. So a stock Ollama provider validated fine and was then refused at the fetch -- one provider, two boundaries, two answers. Both now consult the same helper. This grants nothing new: a provider with neither a registry default nor an explicit flag is denied exactly as before, and the three registry entries carrying the default (Ollama, vLLM, LM Studio) are local by definition. The SSRF exclusions are unchanged and, importantly, sit in FRONT of this permission rather than inside it -- metadata, link-local and unspecified destinations are rejected before any private-network question is asked. The tests prove that for Ollama specifically, since it is the provider that gained access: if those exclusions were ever folded into the same check, `169.254.169.254` is where it would surface, and an SSRF there leaks cloud instance credentials. A note on how these tests were built, because the first attempt was wrong. They originally drove the `provider.fetch` executor branch, and the ablation did not fail -- that branch delegates to `providerDestinationConfigError`, which already consulted the registry, so the tests never touched the defect. Real discovery has no injected executor and goes through the pinned transport. The tests now inject `resolveAddresses` there and assert the permission value it receives, which is the actual fix. Ablation with the pinned-path change reverted: 5 pass / 1 fail. Restored: 6/6. Adjacent suites green -- provider-outbound, discovery-contract, provider-connection-test: 46 pass. This is a security-boundary change under MAINTAINERS.md and wants a maintainer's eyes before it reaches dev, even though it only re-uses a permission the config layer already granted.
…idge-jun#696) HKCU Run values are capped at 260 characters. The tray registered a full PowerShell invocation carrying the Bun path, the CLI entry, the tray script and both home directories, so a user whose npm prefix sits under a long Windows profile name blew the cap and the tray silently never launched after sign-in. An owned VBS launcher holds the long command instead, and the Run value becomes `wscript.exe //B //NoLogo <launcher>`. The launcher is written to the config directory, tracked in tray state, ownership-checked before reuse, and rolled back on failure, so a stale or foreign file at that path is not executed. The regression test builds an entry with a realistically long profile path and asserts the Run value stays within 260 characters, names `wscript.exe`, points at the .vbs — and that the PowerShell form still exceeds 260, which is what makes the first three assertions mean something. Verified by ablation, and the first attempt at that was wrong: disabling the 260-character guard did not fail anything, because the guard is a startup assertion rather than the fix. Substituting the old PowerShell command back into `buildWindowsTrayRunCommand` -- the actual pre-fix behavior -- fails the test (0 pass / 1 fail), and restoring passes 18/18 across both tray suites. This PR was blocked earlier in the round by a conflict in tests/windows-tray.test.ts. Landing lidge-jun#768 first resolved it: the branch now merges clean. Windows behavior cannot be exercised from macOS, so the matrix CI is what confirms it; the local evidence is limited to the pure command-construction functions, which is exactly what these tests cover. Co-authored-by: bitkyc08-arch <bitkyc08@gmail.com>
…lidge-jun#764) `ocx service stop` reported success and restored native Codex while a proxy was still listening, leaving both pointing at each other. The cause is that `ops.stop()` reports the outcome of the STOP COMMAND, not of the process. A Windows scheduler task whose wrapper survives `schtasks /end` respawns its child a few seconds later, so a stop that returned success can still leave a live proxy. The tracked-pid cleanup does not catch it either: the respawned child writes a pid this process never recorded, and `stopTrackedProxyIfRunning` returns "none" the moment the pid file is absent. Stop is now verified instead of inferred. After the manager and the tracked proxy are stopped, the command polls for a live proxy across the observed restart window; if one answers, it reports the port, leaves native Codex alone, and exits non-zero. Restoring the config on top of a running proxy is the part that turns a failed stop into a broken installation, so that is the step that must not happen on an unverified stop. Deliberately different from PR lidge-jun#780, which waits only when `schtasks /end` returns an ERROR. The reported case is an /end that SUCCEEDS while the wrapper lives — on that path there is nothing for the command to report, so waiting on its exit status cannot help. That PR's other work (orphan discovery, PID verification, Microsoft-account preflight) is untouched here and still worth reviewing separately. Four tests on an injected clock, so no wall-clock sleeping. Ablation removing the restart-window polling in favour of a single probe: 1 pass / 3 fail, including the respawn case itself; restored 4/4. One test asserts a genuinely stopped proxy returns null — without it the check could report a survivor every time and strand native Codex unrestored, which is worse than the bug. Another asserts a throwing probe is not read as absence, since an unreachable health endpoint means unknown, not stopped. 87 pass across service, proxy-liveness and both tray suites. Windows behavior needs the matrix CI; the logic here is clock-injected and platform-neutral.
… respawn The review caught a real regression in the previous commit: the verification loop ran its full 7-second window on EVERY `ocx service stop`, including the overwhelmingly common case where the proxy had already stopped cleanly. That trades a Windows-specific bug for a delay every macOS and Linux user pays on every stop, which is the worse defect. The wait now applies only where the failure mode exists. A Windows scheduler wrapper can survive `schtasks /end` and respawn its child seconds later -- that is what lidge-jun#764 reports. Nothing brings the proxy back after `launchctl unload` or `systemctl stop`, so there one probe answers the question and the command returns immediately. The masking test is fixed too. The old "genuinely stopped" case asserted `probes > 1`, which quietly required the slow path and would have kept passing while every user waited. It now names the respawn-capable case explicitly, and a new test asserts the clean path performs exactly one probe with the injected clock never advancing -- so a regression to unconditional waiting fails rather than hides. Ablation removing the platform distinction: 4 pass / 1 fail on that new test. Restored: 70 pass across service-stop-verification, service, proxy-liveness.
Every existing case passed canRespawn explicitly, so the derivation itself was untested: a regression to `?? false` would have left all five green while Windows silently lost the restart window that lidge-jun#764 needs. The new case drives both platforms through the default with a mocked process.platform -- Linux completes at fake-clock 0 after one probe, Windows polls past 7000. Ablation to `?? false`: 5 pass / 1 fail; restored 6/6. Worth recording why the default is right, since the review checked it properly: the launchd plist does set KeepAlive and the systemd unit uses Restart=on-failure, so both CAN restart a child. But `ocx service stop` unloads the launchd job and issues a blocking `systemctl --user stop`, and neither is the crash exit those policies react to. The Windows scheduler wrapper surviving `schtasks /end` remains the one real survivor case.
Closes the unit with what actually happened rather than what was planned. Three of this round's own mistakes are recorded, because they are the reusable part: the lidge-jun#796 fix started as a placeholder space with no evidence behind it and would have 400'd live; the lidge-jun#764 fix introduced a 7-second wait on every stop for every platform; and the first lidge-jun#758 test drove a branch that delegates to a helper already consulting the registry, so its ablation could not fail. An independent reviewer caught all three, and making ablation a habit is what surfaced the last two. Also records the disposition of everything left open with the reason -- owner judgement, security review, bridge-level work, missing provider evidence, or its own cycle -- so the next round starts from a list rather than a re-triage.
📝 WalkthroughWalkthroughThis PR documents a merge round for 43 PRs and integrates catalog, provider, CLI, streaming, logging, service-stop, and Windows tray changes with targeted regression tests. ChangesMerge round planning
Catalog and Desktop model integration
Log, CLI, and runtime reliability
Adapter and platform hardening
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/adapters/openai-chat.ts (1)
176-217: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix: the reasoning-only placeholder guard misses the empty-string content case.
Compare the two placeholder assignments:
- Line 209:
if (!chatMsg.content) chatMsg.content = emptyAssistantContent(provider);— falsy check.- Line 211:
if (chatMsg.reasoning_content !== undefined && chatMsg.content === undefined && chatMsg.tool_calls === undefined)— strictundefinedcheck.
chatMsg.contentis set at Line 183 astextParts.map(p => p.text).join(""). IftextParts.length > 0but every part'stextis"",chatMsg.contentbecomes"", notundefined. In that state, withreasoning_contentset and no tool calls, the guard at Line 211 evaluates tofalsebecausechatMsg.content === undefinedisfalse. The message is pushed withcontent: ""at Line 214. For a Volcengine Ark target, this reproduces the exactMissingParameter: input.content.texterror from#796thatemptyAssistantContentexists to prevent.Align the second guard with the first by using a falsy check.
🐛 Proposed fix: use a falsy check consistent with Line 209
- if (chatMsg.reasoning_content !== undefined && chatMsg.content === undefined && chatMsg.tool_calls === undefined) { + if (chatMsg.reasoning_content !== undefined && !chatMsg.content && chatMsg.tool_calls === undefined) { chatMsg.content = emptyAssistantContent(provider); }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." Add a case totests/volcengine-ark-assistant-content.test.tscovering an assistant message with an empty-string text part plusreasoning_contentand no tool calls, targeted at an Ark provider, to lock in this fix.🤖 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/adapters/openai-chat.ts` around lines 176 - 217, Update the reasoning-only placeholder guard in the assistant handling case to use a falsy check for chatMsg.content, matching the earlier tool-call guard, so empty-string content receives emptyAssistantContent(provider). Add a focused regression case in the existing Volcengine Ark assistant-content tests covering an empty text part with reasoning_content and no tool calls.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/_plan/260731_pr_merge_round/000_plan.md`:
- Around line 40-49: Define one complete CI gate covering typecheck, unit tests,
privacy scan, GUI tests, release-helper syntax, GUI lint, GUI build, and CLI
help, including the existing security-review requirements where applicable.
Update devlog/_plan/260731_pr_merge_round/000_plan.md:40-49 to contain or
reference this canonical definition, and update
devlog/_plan/260731_pr_merge_round/020_batch_a_restack.md:171-175 to reference
the same definition; do not only rename the typecheck command.
In `@devlog/_plan/260731_pr_merge_round/022_remote_test_offload.md`:
- Around line 71-81: Update
devlog/_plan/260731_pr_merge_round/022_remote_test_offload.md lines 71-81 to
record the exact pushed/tested commit SHA, each required command’s exit status
0, and required-check status before documenting --no-verify. Reconcile the
reported 6210 versus 6240 pass counts, and update
devlog/_plan/260731_pr_merge_round/040_round_outcome.md lines 6-7 to cite the
same resolved SHA and verified result.
In `@devlog/_plan/260731_pr_merge_round/030_batch_b_rebuild.md`:
- Around line 39-45: Mark the pagination-related entries in this plan as
superseded, including the limit/offset query parameters, ring-buffer expansion,
and pagination work in logs-usage-routes.ts and request-log.ts. Keep the
timezone formatting and existing useDataSurface integration notes intact, and
align the section with the out-of-scope status documented for `#784` and the
settings-only outcome for `#725`.
In `@devlog/_plan/260731_pr_merge_round/040_deferred_and_handoff.md`:
- Around line 6-10: Update the security-review section heading in the documented
handoff plan to state ten items, matching the listed PRs (`#782`, `#779`, `#775`,
`#778`, `#693`, `#616`, `#744`, `#750`, `#746`, and `#644`). Preserve the existing review
guidance and PR list.
In `@gui/src/pages/Logs.tsx`:
- Around line 362-377: Update the useEffect that fetches /api/settings to clear
serverTimeZone immediately when apiBase changes, before starting the request.
Guard the successful response’s setServerTimeZone update so it does not run
after controller.abort(), while preserving browser-local formatting when the
request fails or lacks a valid time zone.
In `@scripts/test.ts`:
- Around line 80-119: Replace the process-scan ownership check in
waitForExclusiveRun with an atomic, cross-platform lock acquired in a
machine-shared location before launching the child test suite. Keep the lock
held until that child exits, release it from the existing finally cleanup, and
use the existing maxWaitMs timeout only to recover stale locks rather than
permitting overlap between active runs; retain OCX_TEST_NO_QUEUE handling as
appropriate.
In `@src/adapters/anthropic.ts`:
- Around line 253-263: Update anthropicMessagesUrl to retain the parsed URL from
validation and apply trailing-slash and /v1/messages or /v1 normalization to its
pathname rather than the raw baseUrl string. Reconstruct the endpoint with the
normalized pathname before the existing query string and fragment, preserving
those components in the returned URL.
In `@src/cli/init.ts`:
- Around line 213-220: Track whether configuration persistence has completed
after saveConfig (including cleanupOpenAiTierBackupAfterInit). In the catch
block, keep the current interactive-terminal error and nonzero exit for stdin
closure before persistence; after persistence, report that the config was saved
and treat the command as successful with no nonzero exit.
In `@src/codex/catalog/metadata.ts`:
- Around line 126-129: Restrict desktopNativeModels handling to Claude Desktop:
in src/codex/catalog/metadata.ts lines 126-129, keep desktopVisibleNativeSlugs
for Desktop profile/config-writing paths, while Claude Code discovery in
src/server/index.ts uses visibleNativeSlugs or an explicit client-surface
distinction. In src/types.ts lines 458-462, retain the “only the Desktop model
list writer” wording once runtime behavior matches it. Add a server regression
test covering a claude-code user agent with anthropic-version when
desktopNativeModels is false, ensuring native aliases remain discoverable.
In `@src/oauth/kiro-credentials.ts`:
- Line 240: The platform fallback in resolveKiroCliExecutable must return the
bare kiro-cli on every platform so Windows PATHEXT can resolve command shims;
also add .cmd and .bat candidates to the PATH search loop in
src/oauth/kiro-credentials.ts at lines 207-212. Update
tests/kiro-windows-cli-executable-path.test.ts lines 66-74 to expect kiro-cli
and rename the test to describe Windows PATHEXT resolution.
In `@src/oauth/kiro.ts`:
- Around line 78-84: Memoize the result of resolveRuntimeKiroCliExecutable for
the process lifetime so repeated runner and logout calls reuse one resolved path
instead of rescanning PATH. Add cache invalidation when a spawn using the cached
executable fails, allowing the next attempt to resolve the path again; keep
resolveKiroCliExecutable’s existing inputs and behavior unchanged.
In `@src/providers/free-directory.ts`:
- Around line 122-124: Add focused coverage in tests/free-directory.test.ts for
the Baseten entry registered by openAi. Assert its signup-credit classification,
openai-chat adapter, endpoint, live discovery behavior, models URL, supported
support level, and official verification status.
In `@src/server/index.ts`:
- Line 409: Add coverage in tests/claude-models-discovery.test.ts for Anthropic
discovery with desktopNativeModels enabled and disabled, covering the native
visibility branches in the server discovery flow. Assert the native alias is
present when enabled and absent when disabled, then resolve the returned alias
through the Desktop registry to verify registry resolution.
In `@src/server/management/model-routes.ts`:
- Line 86: Extend the Bun regression tests for GET /api/catalog to start the
server via startServer, select a runtime version, and issue an actual request
through the server rather than calling handleManagementAPI directly. Assert the
response includes the x-opencodex-codex-version header, while preserving the
existing no-version coverage.
In `@src/server/management/shared.ts`:
- Around line 236-245: Update the family-default repair logic in
buildClaudeDesktopState to use the existing availability data: build the
available route set before this block and filter replacement candidates so only
routes where available.has(route) are considered. Preserve the current family
matching and sorting behavior, and add a regression case covering a native
default alongside an unavailable routed assignment in the same family.
In `@src/service.ts`:
- Around line 1660-1663: Update the proxy polling flow around findProxy and the
deadline check to track whether a probe completed successfully; if all probes
throw, return or throw an explicit error state instead of null. In
serviceCommand, catch that state, set a failing exit code, and skip native Codex
restoration in the affected 1997-2007 path. Add a regression test covering
probes that always throw.
- Around line 1994-2007: Update the stop flow before restoreNativeCodex() to
call proxyStillLiveAfterStop() after ops.uninstall(). If it reports a live
proxy, fail closed using the existing stop-path behavior from the survivor check
near the diff: log the listening port and guidance, set process.exitCode to 1,
and skip native restoration; only call restoreNativeCodex() when no proxy
remains.
In `@tests/init-eof.test.ts`:
- Around line 24-27: Update the Promise.race around proc.exited to retain the
timeout handle, clear it in a finally block regardless of which branch wins, and
kill proc when the timeout branch fires before rejecting. Preserve the existing
timeout error and successful exit behavior.
In `@tests/kiro-windows-cli-executable-path.test.ts`:
- Around line 40-53: Add a test alongside the existing Windows executable
resolution tests that makes both the LOCALAPPDATA and ProgramFiles Kiro CLI
candidates exist, then assert resolveKiroCliExecutable selects the LOCALAPPDATA
candidate. Preserve the documented precedence of PATH, LOCALAPPDATA, then
ProgramFiles and reuse the existing test setup symbols.
---
Outside diff comments:
In `@src/adapters/openai-chat.ts`:
- Around line 176-217: Update the reasoning-only placeholder guard in the
assistant handling case to use a falsy check for chatMsg.content, matching the
earlier tool-call guard, so empty-string content receives
emptyAssistantContent(provider). Add a focused regression case in the existing
Volcengine Ark assistant-content tests covering an empty text part with
reasoning_content and no tool calls.
🪄 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: bff6e2e7-89a2-4ac9-97f7-7b470fe39b8d
📒 Files selected for processing (46)
devlog/_plan/260731_pr_merge_round/000_plan.mddevlog/_plan/260731_pr_merge_round/010_superseded_closeout.mddevlog/_plan/260731_pr_merge_round/020_batch_a_restack.mddevlog/_plan/260731_pr_merge_round/021_stream_tool_argument_validation.mddevlog/_plan/260731_pr_merge_round/022_remote_test_offload.mddevlog/_plan/260731_pr_merge_round/030_batch_b_rebuild.mddevlog/_plan/260731_pr_merge_round/031_log_timezone_no_layout_change.mddevlog/_plan/260731_pr_merge_round/040_deferred_and_handoff.mddevlog/_plan/260731_pr_merge_round/040_round_outcome.mddocs-site/src/content/docs/guides/codex-integration.mddocs-site/src/content/docs/guides/providers.mdgui/src/pages/Logs.tsxgui/src/pages/dashboard-shared.tsscripts/test.tssrc/adapters/anthropic.tssrc/adapters/openai-chat.tssrc/cli/claude-desktop.tssrc/cli/init.tssrc/codex/catalog.tssrc/codex/catalog/metadata.tssrc/lib/destination-policy.tssrc/lib/provider-outbound.tssrc/oauth/kiro-credentials.tssrc/oauth/kiro.tssrc/providers/free-directory.tssrc/server/index.tssrc/server/management/agent-settings-routes.tssrc/server/management/config-routes.tssrc/server/management/model-routes.tssrc/server/management/shared.tssrc/service.tssrc/tray/windows.tssrc/types.tstests/anthropic-stream-hardening.test.tstests/api-catalog-route.test.tstests/claude-desktop-cli.test.tstests/grok-management-api.test.tstests/init-eof.test.tstests/kiro-windows-cli-executable-path.test.tstests/logs-timezone.test.tstests/openai-chat-eof.test.tstests/provider-outbound-private-network.test.tstests/service-stop-verification.test.tstests/volcengine-ark-assistant-content.test.tstests/windows-tray-run-limit.test.tstests/windows-tray.test.ts
Comment on lines
+40
to
+49
| **CI 게이트** (`AGENTS.md` + `MAINTAINERS.md`): | ||
|
|
||
| - PR 타깃은 `dev`만. `enforce-target`이 `main` 조상 위에 있으면서 `dev`보다 한참 | ||
| 뒤처진 PR과 빈약한 설명을 거부한다. | ||
| - 인증/자격증명/OAuth/Actions/릴리스 자동화/의존성 설치를 건드리면 **명시적 보안 | ||
| 리뷰**가 필요하다. | ||
| - 새 provider preset은 자격증명 목적지 변경이다. 문서화된 엔드포인트, ToS와 법인, | ||
| 중개업체면 재판매·라우팅 권한, 유지보수 담당자, 인용 가능한 검증 날짜가 있어야 | ||
| canonical 등록이 된다. 증거가 부족하면 `free-directory.ts`의 비활성 행으로. | ||
| - `bun run typecheck`, `bun run test`, `bun run privacy:scan`이 게이트다. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
printf '%s\n' '--- candidate files ---'
git ls-files \
'devlog/_plan/260731_pr_merge_round/000_plan.md' \
'devlog/_plan/260731_pr_merge_round/020_batch_a_restack.md' \
'AGENTS.md' 'MAINTAINERS.md' 'package.json' 'bun.lockb' 'bun.lock' \
| sort
printf '%s\n' '--- central plan ---'
cat -n devlog/_plan/260731_pr_merge_round/000_plan.md | sed -n '1,90p'
printf '%s\n' '--- batch checklist ---'
cat -n devlog/_plan/260731_pr_merge_round/020_batch_a_restack.md | sed -n '145,190p'
printf '%s\n' '--- repository guidance ---'
for f in AGENTS.md MAINTAINERS.md package.json; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
cat -n "$f" | sed -n '1,240p'
fi
done
printf '%s\n' '--- related command references ---'
rg -n --hidden -S \
'lint:gui|privacy:scan|bun run typecheck|bun x tsc|bun run test|CI 게이트|merge.?gate|검증' \
-g '!node_modules' -g '!dist' -g '!build' .Repository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
for f in \
devlog/_plan/260731_pr_merge_round/000_plan.md \
devlog/_plan/260731_pr_merge_round/020_batch_a_restack.md \
package.json \
AGENTS.md \
MAINTAINERS.md
do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
case "$f" in
*000_plan.md) sed -n '32,58p' "$f" | nl -ba -v32 ;;
*020_batch_a_restack.md) sed -n '165,182p' "$f" | nl -ba -v165 ;;
package.json) sed -n '1,180p' "$f" ;;
*) sed -n '1,220p' "$f" ;;
esac
else
printf '\n--- missing: %s ---\n' "$f"
fi
done
printf '\n--- focused workflow and hook references ---\n'
rg -n -S \
'typecheck|lint:gui|privacy:scan|bun run test|bun x tsc|prepush|pull_request|enforce-target' \
.github package.json scripts 2>/dev/null \
| head -n 240Repository: lidge-jun/opencodex
Length of output: 24799
🏁 Script executed:
printf '%s\n' '--- central plan lines 35-55 ---'
cat -n devlog/_plan/260731_pr_merge_round/000_plan.md | sed -n '35,55p'
printf '%s\n' '--- batch checklist lines 165-182 ---'
cat -n devlog/_plan/260731_pr_merge_round/020_batch_a_restack.md | sed -n '165,182p'
printf '%s\n' '--- CI workflow relevant section ---'
cat -n .github/workflows/ci.yml | sed -n '1,125p'
printf '%s\n' '--- GUI workflow files and relevant commands ---'
find .github/workflows -maxdepth 1 -type f -print | sort
rg -n -C 4 -S 'lint:gui|bun run lint|bun run test|bun x tsc|typecheck|privacy:scan|doctor' \
.github/workflows
printf '%s\n' '--- exact command definitions ---'
python3 - <<'PY'
import json
from pathlib import Path
scripts = json.loads(Path("package.json").read_text())["scripts"]
for name in ("typecheck", "test", "privacy:scan", "lint:gui", "prepush"):
print(f"{name}: {scripts.get(name)}")
PYRepository: lidge-jun/opencodex
Length of output: 13325
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import json, re
pkg = json.loads(Path("package.json").read_text())
print("package scripts:")
for key in ("typecheck", "test", "privacy:scan", "lint:gui", "prepush"):
print(f" {key}: {pkg['scripts'].get(key)}")
print("\nroot test wrapper:")
test_script = Path("scripts/test.ts")
if test_script.exists():
for i, line in enumerate(test_script.read_text().splitlines(), 1):
if i <= 130:
print(f"{i:4}: {line}")
print("\nplan command occurrences:")
for name in (
"devlog/_plan/260731_pr_merge_round/000_plan.md",
"devlog/_plan/260731_pr_merge_round/020_batch_a_restack.md",
):
text = Path(name).read_text()
print(f"\n{name}")
for i, line in enumerate(text.splitlines(), 1):
if 35 <= i <= 60 or 165 <= i <= 182:
if re.search(r"\bbun\b|lint|GUI|게이트|검증|CI", line, re.I):
print(f"{i:4}: {line}")
print("\nCI run commands:")
ci = Path(".github/workflows/ci.yml").read_text().splitlines()
for i, line in enumerate(ci, 1):
if 75 <= i <= 110:
print(f"{i:4}: {line}")
PYRepository: lidge-jun/opencodex
Length of output: 7701
Align the merge-gate definitions with CI.
000_plan.md:40-49 lists only three gates, but .github/workflows/ci.yml:81-107 also requires GUI tests, release-helper syntax, GUI lint, GUI build, and CLI help. 020_batch_a_restack.md:171-175 omits these checks. Define the complete gate once, then reference it from both plan documents. bun x tsc --noEmit is equivalent to bun run typecheck through package.json; changing that spelling alone does not fix the scope mismatch.
📍 Affects 2 files
devlog/_plan/260731_pr_merge_round/000_plan.md#L40-L49(this comment)devlog/_plan/260731_pr_merge_round/020_batch_a_restack.md#L171-L175
🤖 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/260731_pr_merge_round/000_plan.md` around lines 40 - 49, Define
one complete CI gate covering typecheck, unit tests, privacy scan, GUI tests,
release-helper syntax, GUI lint, GUI build, and CLI help, including the existing
security-review requirements where applicable. Update
devlog/_plan/260731_pr_merge_round/000_plan.md:40-49 to contain or reference
this canonical definition, and update
devlog/_plan/260731_pr_merge_round/020_batch_a_restack.md:171-175 to reference
the same definition; do not only rename the typecheck command.
Comment on lines
+71
to
+81
| ## 결과 | ||
|
|
||
| | | 로컬(4개 경합) | macmini-cf | | ||
| |---|---|---| | ||
| | 전체 스위트 | 13분+ (미완료) | **162초** | | ||
| | load average | 10.2 | 2.4 | | ||
| | 결과 | — | 6210 pass / 0 fail | | ||
|
|
||
| 푸시는 `--no-verify`로 했다. 게이트를 건너뛴 게 아니라 **원격에서 이미 통과**한 | ||
| 것을 로컬에서 다시 돌리지 않기 위해서다. typecheck / lint:gui / test / | ||
| privacy:scan 네 가지 모두 macmini에서 green이었다. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
for f in \
devlog/_plan/260731_pr_merge_round/022_remote_test_offload.md \
devlog/_plan/260731_pr_merge_round/040_round_outcome.md
do
echo "===== $f ====="
wc -l "$f"
sed -n '1,120p' "$f"
done
echo "===== repository references ====="
rg -n --hidden -S \
'6210|6240|--no-verify|macmini-cf|codex/260731-pr-merge-round|origin/dev|typecheck|lint:gui|privacy:scan' \
devlog/_plan/260731_pr_merge_round \
.github 2>/dev/null || true
echo "===== concise git state ====="
git status --short
git rev-parse --show-toplevel
git rev-parse HEAD
git branch --show-current
git show -s --format='%H%n%P%n%s' HEADRepository: lidge-jun/opencodex
Length of output: 7980
🏁 Script executed:
#!/bin/bash
set -eu
echo "===== round plan and nearby evidence ====="
sed -n '1,70p' devlog/_plan/260731_pr_merge_round/000_plan.md
sed -n '95,125p' devlog/_plan/260731_pr_merge_round/030_batch_b_rebuild.md
echo "===== all repository references ====="
rg -n --hidden -S \
'6210|6240|--no-verify|macmini-cf|codex/260731-pr-merge-round|356924263|required check|required-check|status check|exit status|exit code|tested commit|tested revision' \
. 2>/dev/null || true
echo "===== CI gate definition ====="
sed -n '65,105p' .github/workflows/ci.yml
echo "===== current document commit ====="
git show --stat --oneline --decorate HEAD
git show --format= --name-status HEAD -- \
devlog/_plan/260731_pr_merge_round/022_remote_test_offload.md \
devlog/_plan/260731_pr_merge_round/040_round_outcome.mdRepository: lidge-jun/opencodex
Length of output: 20141
Bind the remote gate to the pushed commit before using --no-verify.
devlog/_plan/260731_pr_merge_round/022_remote_test_offload.mdlines 56–65 show the transfer and test commands, but lines 71–81 record no resolved commit SHA or exit status.devlog/_plan/260731_pr_merge_round/040_round_outcome.mdlines 6–7 reports 6240 passes, while022_remote_test_offload.mdreports 6210. Neither result identifies the tested revision.- Record the exact tested SHA, command, exit status
0, and required-check status. Reconcile the pass counts and cite the same SHA in both documents before bypassing local hooks with--no-verify.
📍 Affects 2 files
devlog/_plan/260731_pr_merge_round/022_remote_test_offload.md#L71-L81(this comment)devlog/_plan/260731_pr_merge_round/040_round_outcome.md#L6-L7
🤖 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/260731_pr_merge_round/022_remote_test_offload.md` around lines
71 - 81, Update devlog/_plan/260731_pr_merge_round/022_remote_test_offload.md
lines 71-81 to record the exact pushed/tested commit SHA, each required
command’s exit status 0, and required-check status before documenting
--no-verify. Reconcile the reported 6210 versus 6240 pass counts, and update
devlog/_plan/260731_pr_merge_round/040_round_outcome.md lines 6-7 to cite the
same resolved SHA and verified result.
Comment on lines
+39
to
+45
| - `src/server/management/logs-usage-routes.ts` — 배열 본문 유지, 헤더 추가. | ||
| `limit`/`offset` 쿼리 파라미터 수용. | ||
| - `src/server/request-log.ts:128-129` — 200건 링 버퍼 상한을 올린다. `:741-764`에 | ||
| 페이지네이션을 추가한다. | ||
| - `gui/src/pages/Logs.tsx` — HEAD의 `useDataSurface` 리팩터 위에 얹는다. | ||
| #784는 그 이전의 수동 `fetchLogs` 흐름을 가정하고 있어 그대로는 안 맞는다. | ||
| 타임스탬프를 서버 타임존으로 포맷(`:288-293`, `:366-371`). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Mark the pagination plan as superseded.
At Line 39-45, this document includes limit/offset and ring-buffer pagination. devlog/_plan/260731_pr_merge_round/031_log_timezone_no_layout_change.md says #784 pagination is out of scope, and devlog/_plan/260731_pr_merge_round/040_round_outcome.md records #725 as a settings-only change. Mark this section superseded or align all three documents before handoff.
🤖 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/260731_pr_merge_round/030_batch_b_rebuild.md` around lines 39 -
45, Mark the pagination-related entries in this plan as superseded, including
the limit/offset query parameters, ring-buffer expansion, and pagination work in
logs-usage-routes.ts and request-log.ts. Keep the timezone formatting and
existing useDataSurface integration notes intact, and align the section with the
out-of-scope status documented for `#784` and the settings-only outcome for `#725`.
Comment on lines
+6
to
+10
| ## 보안 리뷰가 필요한 6건 | ||
|
|
||
| `MAINTAINERS.md`는 인증·자격증명·OAuth·Actions·릴리스 자동화·의존성 설치를 | ||
| 건드리는 변경에 **명시적 보안 리뷰**를 요구한다. 리뷰는 메인테이너 판단이지 | ||
| 에이전트가 대신할 수 있는 게 아니다. 그래서 분석만 남기고 브랜치에 안 올린다. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Correct the security-review item count.
The heading says six, but the section contains ten PRs: #782, #779, #775, #778, #693, #616, #744, #750, #746, and #644. Keep the heading and handoff list aligned so security-sensitive work is not undercounted.
🤖 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/260731_pr_merge_round/040_deferred_and_handoff.md` around lines
6 - 10, Update the security-review section heading in the documented handoff
plan to state ten items, matching the listed PRs (`#782`, `#779`, `#775`, `#778`, `#693`,
`#616`, `#744`, `#750`, `#746`, and `#644`). Preserve the existing review guidance and PR
list.
Comment on lines
+362
to
+377
| useEffect(() => { | ||
| const controller = new AbortController(); | ||
| void (async () => { | ||
| try { | ||
| const res = await fetch(`${apiBase}/api/settings`, { signal: controller.signal }); | ||
| if (!res.ok) return; | ||
| const body = await res.json() as { timeZone?: unknown }; | ||
| if (typeof body.timeZone === "string" && body.timeZone.trim()) { | ||
| setServerTimeZone(body.timeZone.trim()); | ||
| } | ||
| } catch { | ||
| // Offline or an older proxy without the field: keep browser-local formatting. | ||
| } | ||
| })(); | ||
| return () => controller.abort(); | ||
| }, [apiBase]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear the previous server time zone when apiBase changes.
Line 362 keeps the previous serverTimeZone until the next request sets a new value. If the new request fails, all timestamps continue to render in the previous proxy's zone. Reset the state at the start of this effect. Also guard the state update after the request against an aborted controller.
Proposed fix
useEffect(() => {
const controller = new AbortController();
+ setServerTimeZone(undefined);
void (async () => {
try {
const res = await fetch(`${apiBase}/api/settings`, { signal: controller.signal });
if (!res.ok) return;
const body = await res.json() as { timeZone?: unknown };
- if (typeof body.timeZone === "string" && body.timeZone.trim()) {
+ if (!controller.signal.aborted && typeof body.timeZone === "string" && body.timeZone.trim()) {
setServerTimeZone(body.timeZone.trim());
}📝 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.
Suggested change
| useEffect(() => { | |
| const controller = new AbortController(); | |
| void (async () => { | |
| try { | |
| const res = await fetch(`${apiBase}/api/settings`, { signal: controller.signal }); | |
| if (!res.ok) return; | |
| const body = await res.json() as { timeZone?: unknown }; | |
| if (typeof body.timeZone === "string" && body.timeZone.trim()) { | |
| setServerTimeZone(body.timeZone.trim()); | |
| } | |
| } catch { | |
| // Offline or an older proxy without the field: keep browser-local formatting. | |
| } | |
| })(); | |
| return () => controller.abort(); | |
| }, [apiBase]); | |
| useEffect(() => { | |
| const controller = new AbortController(); | |
| setServerTimeZone(undefined); | |
| void (async () => { | |
| try { | |
| const res = await fetch(`${apiBase}/api/settings`, { signal: controller.signal }); | |
| if (!res.ok) return; | |
| const body = await res.json() as { timeZone?: unknown }; | |
| if (!controller.signal.aborted && typeof body.timeZone === "string" && body.timeZone.trim()) { | |
| setServerTimeZone(body.timeZone.trim()); | |
| } | |
| } catch { | |
| // Offline or an older proxy without the field: keep browser-local formatting. | |
| } | |
| })(); | |
| return () => controller.abort(); | |
| }, [apiBase]); |
🤖 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 `@gui/src/pages/Logs.tsx` around lines 362 - 377, Update the useEffect that
fetches /api/settings to clear serverTimeZone immediately when apiBase changes,
before starting the request. Guard the successful response’s setServerTimeZone
update so it does not run after controller.abort(), while preserving
browser-local formatting when the request fails or lacks a valid time zone.
Source: Path instructions
Comment on lines
+236
to
+245
| if (config.claudeCode?.desktopNativeModels === false) { | ||
| for (const route of Object.keys(profile.assignments)) { | ||
| if (route.startsWith("native/")) delete profile.assignments[route]; | ||
| } | ||
| for (const family of ["opus", "fable", "sonnet", "haiku"] as const) { | ||
| const current = profile.defaults[family]; | ||
| if (current?.startsWith("native/")) { | ||
| profile.defaults[family] = Object.keys(profile.assignments) | ||
| .filter(route => profile.assignments[route]?.family === family) | ||
| .sort()[0] ?? null; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Select only available routes when repairing a family default.
When a native default is removed, this code selects from every remaining assignment. buildClaudeDesktopState retains unavailable routed assignments, so a stale routed route can become the new default and be exported or applied.
Build the available set before this block. Filter replacement candidates with available.has(route). Add a regression case with a native default and an unavailable routed assignment in the same family.
🤖 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/management/shared.ts` around lines 236 - 245, Update the
family-default repair logic in buildClaudeDesktopState to use the existing
availability data: build the available route set before this block and filter
replacement candidates so only routes where available.has(route) are considered.
Preserve the current family matching and sorting behavior, and add a regression
case covering a native default alongside an unavailable routed assignment in the
same family.
Comment on lines
+1660
to
+1663
| } catch { | ||
| // A probe failure is not proof the proxy is gone; keep polling until the deadline. | ||
| } | ||
| if (now() >= deadline) return null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail closed when the final proxy probe fails.
Lines 1660-1663 return null after seven seconds even if every findProxy() call threw. Lines 1997-2007 then restore native Codex because null means no survivor. A persistent health-check failure leaves the proxy state unknown. It must not authorize restoration.
Track whether the final probe completed successfully. If it did not, return an error state or throw. Catch that state in serviceCommand, set a failing exit code, and do not restore native Codex. Add a regression test where every probe throws.
Proposed fix
export async function proxyStillLiveAfterStop(/* ... */) {
+ let lastProbeFailed = false;
for (;;) {
try {
const live = await findProxy();
+ lastProbeFailed = false;
if (live) return live;
} catch {
- // A probe failure is not proof the proxy is gone; keep polling until the deadline.
+ lastProbeFailed = true;
}
- if (now() >= deadline) return null;
+ if (now() >= deadline) {
+ if (lastProbeFailed) throw new Error("Could not verify that the proxy stopped");
+ return null;
+ }
await sleep(1000);
}
}Also applies to: 1997-2007
🤖 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 1660 - 1663, Update the proxy polling flow
around findProxy and the deadline check to track whether a probe completed
successfully; if all probes throw, return or throw an explicit error state
instead of null. In serviceCommand, catch that state, set a failing exit code,
and skip native Codex restoration in the affected 1997-2007 path. Add a
regression test covering probes that always throw.
Comment on lines
+1994
to
+2007
| // Verify rather than trust the stop command: a surviving wrapper respawns its child | ||
| // seconds later, and restoring native Codex on top of a live proxy is the failure #764 | ||
| // reports as "stop reports success without stopping the proxy". | ||
| const survivor = await proxyStillLiveAfterStop(); | ||
| if (survivor) { | ||
| console.error( | ||
| `❌ service stop did not take effect: a proxy is still listening on port ${survivor.port}.` | ||
| + "\nNative Codex was NOT restored, because doing so while the proxy is running leaves" | ||
| + " both pointing at each other. Check for a second service backend (`ocx service status`)" | ||
| + " or a manually started proxy, then re-run `ocx service stop`.", | ||
| ); | ||
| process.exitCode = 1; | ||
| break; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/service.ts --items all --type function --match 'serviceCommand|proxyStillLiveAfterStop'
rg -n -C 14 'case "uninstall"|case "remove"|restoreNativeCodex|proxyStillLiveAfterStop' src/service.ts
rg -n -C 10 'uninstall\s*[(:]|schtasks|WinSW|Task Scheduler|stop\s*[(:]' srcRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- service backend selection and uninstall flow ---'
sed -n '1760,2060p' src/service.ts
printf '%s\n' '--- Windows service implementations ---'
fd -i 'service|scheduler|winsw|windows' src --type f
rg -n -C 18 'uninstall|stopwait|schtasks|taskkill|/end|/delete|runWindowsElevated' src/service.ts src/lib src --glob '*.ts' \
| grep -E 'service\.ts|scheduler|winsw|windows' | head -n 500Repository: lidge-jun/opencodex
Length of output: 44640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- platformOps and scheduler lifecycle ---'
rg -n 'function platformOps|const platformOps|platformOps\(|uninstall\(\)|stop\(\)' src/service.ts
sed -n '1180,1655p' src/service.ts
printf '%s\n' '--- complete WinSW uninstall implementation ---'
sed -n '300,350p' src/lib/winsw.ts
printf '%s\n' '--- process liveness and tracked-proxy cleanup ---'
rg -n -C 25 'stopTrackedProxyForServiceCommand|proxyStillLiveAfterStop|findLiveProxy' src/service.tsRepository: lidge-jun/opencodex
Length of output: 39162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Windows stop helper and dual-backend cleanup ---'
sed -n '1688,1760p' src/service.ts
printf '%s\n' '--- Read-only lifecycle verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
service = Path("src/service.ts").read_text()
winsw = Path("src/lib/winsw.ts").read_text()
def body(text, signature):
start = text.index(signature)
brace = text.index("{", start)
depth = 0
for i in range(brace, len(text)):
if text[i] == "{":
depth += 1
elif text[i] == "}":
depth -= 1
if depth == 0:
return text[start:i + 1]
raise RuntimeError(signature)
scheduler_stop = body(service, "function stopWindows():")
scheduler_uninstall = body(service, "function uninstallWindows():")
native_stop = body(winsw, "export function stopWinswService():")
native_uninstall = body(winsw, "export function uninstallWinswService():")
command = body(service, "export async function serviceCommand(")
print("scheduler stop:", re.findall(r'schtasks\\(\\[[^\\]]+\\]\\)', scheduler_stop))
print("scheduler uninstall:", re.findall(r'schtasks\\(\\[[^\\]]+\\]\\)', scheduler_uninstall))
print("scheduler has post-delete liveness probe:",
"proxyStillLiveAfterStop" in scheduler_uninstall)
print("native stop uses stopwait:", '"stopwait"' in native_stop)
print("native uninstall uses stopwait:", '"stopwait"' in native_uninstall)
print("uninstall restores only after ops.uninstall:",
command.index("ops.uninstall()") < command.index("restoreNativeCodex()", command.index("ops.uninstall()")))
print("uninstall invokes proxyStillLiveAfterStop:",
"proxyStillLiveAfterStop()" in command[command.index('case "uninstall"'):])
PYRepository: lidge-jun/opencodex
Length of output: 3474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba src/service.ts | sed -n '1510,1625p'
nl -ba src/lib/winsw.ts | sed -n '305,348p'
nl -ba src/service.ts | sed -n '2028,2050p'Repository: lidge-jun/opencodex
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk 'NR >= 1510 && NR <= 1625 { printf "%d:%s\n", NR, $0 }' src/service.ts
awk 'NR >= 305 && NR <= 348 { printf "%d:%s\n", NR, $0 }' src/lib/winsw.ts
awk 'NR >= 2028 && NR <= 2050 { printf "%d:%s\n", NR, $0 }' src/service.tsRepository: lidge-jun/opencodex
Length of output: 9310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
grep -n -A8 -B4 'function uninstallWindows' src/service.tsRepository: lidge-jun/opencodex
Length of output: 1065
Add the post-stop liveness gate before restoring native Codex.
src/service.ts:2031-2043 restores native Codex after ops.uninstall(), but the scheduler backend only runs schtasks /end (src/service.ts:1466) and schtasks /delete (src/service.ts:1469-1474). It does not prove that the wrapper and child exited. The WinSW fallback also uses non-waiting sc stop (src/lib/winsw.ts:329-333). Call proxyStillLiveAfterStop() before restoreNativeCodex() and fail closed when a proxy remains live, as the stop path does at src/service.ts:1997-2007.
🤖 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 1994 - 2007, Update the stop flow before
restoreNativeCodex() to call proxyStillLiveAfterStop() after ops.uninstall(). If
it reports a live proxy, fail closed using the existing stop-path behavior from
the survivor check near the diff: log the listening port and guidance, set
process.exitCode to 1, and skip native restoration; only call
restoreNativeCodex() when no proxy remains.
Comment on lines
+24
to
+27
| const exit = await Promise.race([ | ||
| proc.exited, | ||
| new Promise<number>((_, reject) => setTimeout(() => reject(new Error("init did not exit after stdin EOF")), 8_000)), | ||
| ]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Clear the timeout timer and kill the child process.
The setTimeout at Line 26 is never cleared. When proc.exited wins the race, the 8-second timer stays pending. Bun keeps the event loop alive for that timer, so the test file takes up to 8 extra seconds to finish, and the losing branch rejects after the test has already completed. The timeout branch also leaves the spawned init process running, because nothing calls proc.kill().
Clear the timer in a finally block and terminate the child on timeout.
♻️ Proposed cleanup for the race
proc.stdin.end();
- const exit = await Promise.race([
- proc.exited,
- new Promise<number>((_, reject) => setTimeout(() => reject(new Error("init did not exit after stdin EOF")), 8_000)),
- ]);
+ let timer: ReturnType<typeof setTimeout> | undefined;
+ let exit: number;
+ try {
+ exit = await Promise.race([
+ proc.exited,
+ new Promise<number>((_, reject) => {
+ timer = setTimeout(() => {
+ proc.kill();
+ reject(new Error("init did not exit after stdin EOF"));
+ }, 8_000);
+ }),
+ ]);
+ } finally {
+ if (timer) clearTimeout(timer);
+ }
expect(exit).toBe(1);📝 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.
Suggested change
| const exit = await Promise.race([ | |
| proc.exited, | |
| new Promise<number>((_, reject) => setTimeout(() => reject(new Error("init did not exit after stdin EOF")), 8_000)), | |
| ]); | |
| proc.stdin.end(); | |
| let timer: ReturnType<typeof setTimeout> | undefined; | |
| let exit: number; | |
| try { | |
| exit = await Promise.race([ | |
| proc.exited, | |
| new Promise<number>((_, reject) => { | |
| timer = setTimeout(() => { | |
| proc.kill(); | |
| reject(new Error("init did not exit after stdin EOF")); | |
| }, 8_000); | |
| }), | |
| ]); | |
| } finally { | |
| if (timer) clearTimeout(timer); | |
| } | |
| expect(exit).toBe(1); |
🤖 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/init-eof.test.ts` around lines 24 - 27, Update the Promise.race around
proc.exited to retain the timeout handle, clear it in a finally block regardless
of which branch wins, and kill proc when the timeout branch fires before
rejecting. Preserve the existing timeout error and successful exit behavior.
Comment on lines
+40
to
+53
| test("win32 falls back to Program Files\\Kiro-Cli when LOCALAPPDATA binary is absent", () => { | ||
| const exists = (path: string) => path === "C:\\Program Files\\Kiro-Cli\\kiro-cli.exe"; | ||
| expect(resolveKiroCliExecutable({ | ||
| env: { | ||
| PATH: "C:\\Windows\\System32", | ||
| LOCALAPPDATA: "C:\\Users\\u\\AppData\\Local", | ||
| ProgramFiles: "C:\\Program Files", | ||
| }, | ||
| platform: "win32", | ||
| home: WIN_HOME, | ||
| pathEntries: ["C:\\Windows\\System32"], | ||
| exists, | ||
| })).toBe("C:\\Program Files\\Kiro-Cli\\kiro-cli.exe"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a case where both Windows install candidates exist.
This test makes only C:\Program Files\Kiro-Cli\kiro-cli.exe exist, so it proves that the Program Files candidate is reachable. It does not prove the precedence between the two install candidates. If someone swaps the two installCandidates.push arguments in src/oauth/kiro-credentials.ts Lines 220-223, every test in this file still passes, and the documented Windows order (PATH, then %LOCALAPPDATA%\Kiro-Cli\kiro-cli.exe, then C:\Program Files\Kiro-Cli\kiro-cli.exe) silently breaks.
Add a case where both paths exist and assert that %LOCALAPPDATA% wins.
💚 Proposed additional test
+ test("win32 prefers %LOCALAPPDATA% over Program Files when both exist", () => {
+ const exists = (path: string) =>
+ path === "C:\\Users\\u\\AppData\\Local\\Kiro-Cli\\kiro-cli.exe"
+ || path === "C:\\Program Files\\Kiro-Cli\\kiro-cli.exe";
+ expect(resolveKiroCliExecutable({
+ env: {
+ PATH: "C:\\Windows\\System32",
+ LOCALAPPDATA: "C:\\Users\\u\\AppData\\Local",
+ ProgramFiles: "C:\\Program Files",
+ },
+ platform: "win32",
+ home: WIN_HOME,
+ pathEntries: ["C:\\Windows\\System32"],
+ exists,
+ })).toBe("C:\\Users\\u\\AppData\\Local\\Kiro-Cli\\kiro-cli.exe");
+ });
+
test("linux keeps PATH-first resolution and falls back to ~/.local/bin", () => {🤖 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/kiro-windows-cli-executable-path.test.ts` around lines 40 - 53, Add a
test alongside the existing Windows executable resolution tests that makes both
the LOCALAPPDATA and ProgramFiles Kiro CLI candidates exist, then assert
resolveKiroCliExecutable selects the LOCALAPPDATA candidate. Preserve the
documented precedence of PATH, LOCALAPPDATA, then ProgramFiles and reuse the
existing test setup symbols.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d380b1b0e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+18
to
+21
| 위협 모델: opt-in을 켜야만 열린다. 권한 없는 로컬 프로세스가 특권 Windows 서비스의 | ||
| 환경변수를 바꿀 수는 없다. 다만 다중 사용자 호스트에서 디렉터리/파일 ACL이 실제로 | ||
| 약한 상태로 이걸 켜면, 다른 로컬 사용자가 토큰 파일을 읽거나 재시작 전에 바꿔치기해 | ||
| 관리 API 전체를 얻는다. 네트워크 공격자는 여전히 토큰이 필요하다. |
There was a problem hiding this comment.
This newly tracked _plan note contains pre-disclosure security analysis for an unfixed admin-token ACL issue, including the threat model and concrete broken state for /api/settings; because devlog/ is public/tracked, committing this discloses material that the repository policy says must stay in scratch space until the fix has shipped. Please remove these open security-review notes from devlog/ and keep them under .tmp/ or another scratch path instead.
AGENTS.md reference: AGENTS.md:L61-L68
Useful? React with 👍 / 👎.
| ? "desktop3p" as const | ||
| : (/^claude-code\//i.test(req.headers.get("user-agent") ?? "") ? "readable" as const : "desktop3p" as const); | ||
| const data = buildAnthropicModelInfos([...visibleNativeSlugs(config)], goOrdered, resolveAutoContext(config.claudeCode), idStyle, activeDesktop3pAlias); | ||
| const data = buildAnthropicModelInfos([...desktopVisibleNativeSlugs(config)], goOrdered, resolveAutoContext(config.claudeCode), idStyle, activeDesktop3pAlias); |
There was a problem hiding this comment.
When claudeCode.desktopNativeModels is false, this path now uses desktopVisibleNativeSlugs() for every Anthropic-style /v1/models response, but the same branch explicitly serves Claude Code CLI discovery when the user agent is claude-code/... or ?ids=cli; a config knob documented as only hiding native/* rows from Claude Desktop show/export/apply therefore also removes native OpenAI models from Claude Code’s model list. Use the desktop-filtered list only for the Desktop 3P registry/list and keep visibleNativeSlugs(config) for CLI/readable discovery.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
2 tasks
This was referenced Jul 31, 2026
Owner
|
Closing. This pull request contains no work by its author. The head commit here is byte-identical to a branch that already exists in this repository and was authored by the maintainer. All six of these pull requests have the same shape: fork the repository, push the upstream branches back unchanged, and open them as incoming contributions.
Every listed SHA resolves to the same commit on the upstream branch of the same name. The commit authors inside them are This is not a rebase, a resubmission of stalled work, or a fork that drifted. It burns maintainer review time and CI minutes on a diff that is already in the tree, and it presents other people's commits under a new author's pull request. Repository access is being revoked for this account. |
This was referenced Jul 31, 2026
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Checklist
Summary by CodeRabbit
ocx initnow reports closed-input errors cleanly.