feat(grok): auto-configure Grok Build and harden strict-client compatibility#403
Conversation
📝 WalkthroughWalkthroughThis PR adds Grok Build model synchronization and managed configuration lifecycle handling, normalizes usage-detail fields across Responses and Chat Completions, preserves raw usage provenance, updates stop-error handling, and adds documentation, plans, receipts, and regression tests. ChangesGrok Build bridge integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Reviewed — four blocking findings:
Per MAINTAINERS.md this touches config-ownership/lifecycle boundaries and warrants a human security/lifecycle review even after fixes. Happy to re-review. |
.codexclaw/는 9d4f58e8부터 .gitignore 대상이었으나 68f88fb3에서 강제 add로 추적이 시작돼 이후 커밋과 main 프로모션까지 타고 갔다. gitignore는 미추적 파일에만 적용되므로 한번 추적되면 묵시적으로 계속 따라간다. git rm -r --cached로 추적만 해제. 로컬 goalplan 상태 파일은 그대로 유지되고(cxc orchestrate가 계속 갱신), ignore가 다시 적용돼 앞으로는 커밋되지 않는다.
…t clients grok-build's pinned async-openai fork deserializes input_tokens_details/ output_tokens_details as required (non-Option) fields, so routed models that report no cache/reasoning numbers made Grok Build exit 1 after successful text. Emit both detail objects with zero defaults from responsesUsage() and chatCompletionsUsage(); OpenAI's own ResponseUsage schema marks them required whenever usage is present, so this is spec-conformant normalization.
…callback The always-emitted zero-default detail objects made request-log wire re-parsing read synthetic zeros as measured cache/reasoning numbers, suppressing cache_detail_missing in cost estimates. The bridge now reports raw adapter usage (pre-normalization) through an onUsage callback at every terminal path (done, max_tokens, incomplete, error-with-usage, adapter EOF), and applyResponseLogMetadata skips wire re-parsing when the bridge already reported (usageFromBridge). Native passthrough is unchanged.
…ig.toml Additive [model_providers.opencodex] + thin [model.ocx-*] aliases inside fence markers; one-time backup, user-owned-provider conflict skip, CRLF preservation, orphan-marker fail-safe. Pure module (grokHome injectable) with isolated tmpdir tests.
…onfig ocx start injects a fenced block of [model.ocx-*] entries (direct base_url/api_backend/api_key per model) into ~/.grok/config.toml when ~/.grok exists; stop/eject/uninstall/graceful-shutdown strip only the fenced block. Per-model direct fields are used because grok 0.2.101's [model_providers] inheritance parses but does not apply the inherited base_url to inference routing (falls through to cli-chat-proxy, 401 — verified live).
- Orphaned begin marker now refuses mutation (inject AND strip) instead of treating everything to EOF as managed — user tables after a damaged fence can no longer be erased. - Atomic temp-write/rename (atomicWriteFile) instead of in-place truncation. - User-owned [model.*] aliases outside the fence are reserved so injection never emits a duplicate table header (whole-file TOML invalidation). - Dropped the obsolete model_providers guard/constant (no provider table is emitted since the per-model direct-fields deviation). - handleStop survives a throwing stopServiceIfInstalled (home-mismatch guard) so proxy stop, Codex restore, env revert, and grok strip still run. - ok:false results now surface as warnings on start/stop/eject paths. - Tests updated: per-model direct-field contract, user-alias collision, orphan refusal both ways.
…alias reservation [model."ocx-mine"], [ model . ocx-mine ] and single-quoted forms all define the same table; canonicalize every equivalent spelling when reserving user-owned aliases so injection can never emit a duplicate table header.
…regression - syncGrokConfig helper (deps-injectable, mirrors codex/sync.ts) shared by start and both handleEnsure branches; the ensure parent injects after waitForProxy so the fence exists before ensure returns (spawned-child readiness race), and the live branch passes live.hostname from proxy-liveness instead of possibly-drifted config.hostname. - Catalog failure surfaces ok:false instead of silently skipping. - Regression pins that responsesSseToChatCompletionsSse never forwards a raw response.heartbeat frame (why the injected config pins chat_completions against grok's strict Responses decoder).
The home-mismatch guard throw previously set stopFailed and exited 1, which aborted ocx restart before its ensure step could bring the proxy back. The warning is preserved; local proxy/codex/grok teardown and the subsequent ensure are independent of the service manager.
Auto-registration lifecycle, manual direct-fields recipe (model_providers inheritance bug on 0.2.101), loopback auth note, and known limitations (heartbeat vs strict Responses decoder, service-installed restart persistence loss, config hot-reload behavior). EN source only; sidebar entry with locale labels, Starlight EN fallback for locales.
…er proof, catalog, reasoning bridge)
…y + upstream source evidence + 5 implementation cycles
…sing non-loopback auto-registration (env_key leaks the xAI session token), B5 makes inject injective, 030/040 close the teardown hole and the unsafe tests
…g byte-for-byte B3: the alias-collision scan only matched a literal first segment, so a user-owned ["model"."x"] or ['model'.x] table went unreserved and we emitted a duplicate [model.x]. grok rejects the ENTIRE config layer on a duplicate key, taking every unrelated user setting with it. Both segments are now canonicalized through one KEY_SEGMENT grammar, and array-of-table ([[model.x]]) plus sub-table ([model.x.sub]) spellings — which collide the same way — are reserved too. B5: injection added "\n\n" to a config lacking a trailing newline but strip removed only one, so `ocx stop` left an extra newline behind. The real problem was that "X" and "X\n" produced byte-identical files after inject, making restore impossible. Injection now always adds exactly one separator newline, and strip undoes exactly that — verified byte-for-byte across no/one/two/three trailing newlines, mid-file blocks, uniform CRLF, and five inject/strip cycles.
A non-loopback bind needs the real admission token, but a REGENERATED block cannot carry one safely. Writing the literal token puts the user's secret in their own config and overwrites whatever they set on the next start/ensure/restart — the REAL_TOKEN_PRESERVED=false reproduction. Dropping api_key in favour of env_key is worse: with no model_provider to fail closed, an unresolved env_key makes grok fall through to its xAI session bearer and send it to our plaintext LAN base_url (upstream resolve_credentials, config.rs). So we stop auto-registering there. Injection returns skippedReason "non-loopback" with guidance to configure models outside the fence, removes any stale block left from a previous loopback bind, and never touches the file again — so a user-managed api_key now survives repeated syncs. Wildcard binds (0.0.0.0, ::) count as non-loopback: they expose every interface. isLoopbackHostname is exported for this decision because providerBaseHost answers a different question (what to dial).
…eardown Ownership failures were indistinguishable from stop failures, so `ocx stop` warned and carried on: it stripped the shared ~/.grok fence and restored native Codex while the service installed under another home kept running and respawned the proxy. ServiceOwnershipError now marks that case, handleStop skips shared teardown and reports failure, and the daemon's own exit cleanup applies the same check -- without it, SIGTERM would strip the fence through the path handleStop just declined. Explicit teardown now includes the fence everywhere it should: `ocx service stop`, `ocx service uninstall`, and POST /api/stop all strip it, while the OCX_SERVICE crash/respawn exclusion stays. /api/stop also stops leaking the ownership error as a 500 that left the proxy running; it answers 409, stopProxy refuses to escalate that to SIGTERM, and the dashboard clears its stopping state instead of hanging. Also: guard the service stop on an actual installation (it ran launchctl unload regardless), de-nest the start-time Grok sync from the Desktop-3P try that swallowed catalog failures, fail `ocx stop` when a strip is refused, and return from handleStop rather than exiting so restart and the tray can react. Review fix on the previous commit: the sub-table tail [^\]]* matched newlines, so an unclosed header inside a multiline string swallowed the following lines and hid a real [model.<alias>] header -- reintroducing the duplicate-key config B3 exists to prevent. Every class is newline-free now, with a regression for it.
…and reload The guide told users to swap api_key on a non-loopback bind, which no longer matches the code (auto-registration is refused there) and never addressed the unreachable 127.0.0.1 base_url. It now explains why the fence is loopback-only, gives a manual recipe with a reachable host and the admission token, and warns against env_key -- an unresolved one makes Grok fall through to the xAI session token and send it to whatever base_url the entry names. The reload note claimed hot reload without a version guarantee. Grok does watch config.toml and reload on a real [model] change, so the note now describes the observed mechanism, points at grok inspect for verification (config sources and rejected fields, not a model list), and records that one TOML error invalidates the whole user layer -- which is why we write atomically. Also corrects devlog 020, which recommended the responses backend against its own receipt: Grok's Responses event enum has no catch-all, so an unknown top-level type ends the turn with a non-retryable Serialization error.
fcd3d68 to
092dd74
Compare
|
Rebased onto current Blocking findings1. Service ownership guard failure ignored — Two holes the fix would otherwise have left open:
2. Non-loopback placeholder overwritten — auto-registration is now refused on non-loopback binds, including the wildcards I looked at whether 3. Quoted TOML key collision — both key segments now go through one A correction to my own plan: I had claimed sub-table headers collide. A TOML parser says they do not, in either order. The reservation stays as conservatism, not as a collision fix. The first version of this regex had a real defect that review caught: the sub-table tail used 4. No lifecycle regression —
Other Codex P2 findingsTrailing newline not restored — the real problem was upstream of Fence left behind on intentional shutdown — Docs and devlogThe guide now explains why the fence is loopback-only, gives a manual recipe with a reachable host and the admission token rather than an unreachable Beyond the review listThe audit found four defects nobody had reported, all fixed here: the start-time Grok sync was nested inside the Desktop-3P try, so a catalog failure silently skipped the fence; Two residual risks are logged rather than fixed, both out of scope for a review-blocker pass: VerificationAt
Planning and evidence live in |
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 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/260723_grok_build_bridge/010_usage_details_always.md`:
- Around line 25-27: The request-log risk assessment incorrectly describes
optional reads as harmless. Update the risk-check bullet in “리스크 점검” to
reference the `usageFromBridge` guard documented in `011_receipt.md`, noting
that it prevents synthetic zero-valued usage details from overwriting raw
adapter usage and its provenance.
In `@devlog/_plan/260723_grok_build_bridge/041_receipt.md`:
- Around line 13-16: The restart flow must not continue teardown after
stopServiceIfInstalled() reports a service-home ownership mismatch. Propagate
the ownership failure or require positive confirmation that the target proxy
stopped before allowing fence stripping, restart, or ensure operations; update
the stopFailed handling associated with the restart sequence while preserving
normal successful-stop behavior.
In `@devlog/_plan/260726_grok_build_prod/000_blocker_inventory.md`:
- Around line 7-11: Correct the future-dated audit metadata: in
devlog/_plan/260726_grok_build_prod/000_blocker_inventory.md lines 7-11, update
the collection date and source timestamp to the current review date of July 25,
2026; in devlog/_plan/260726_grok_build_prod/000_plan.md lines 7-16, update the
investigation date or explicitly label it as pending.
In `@devlog/_plan/260726_grok_build_prod/000_plan.md`:
- Around line 18-21: Make automatic registration explicitly loopback-only: in
devlog/_plan/260726_grok_build_prod/000_plan.md lines 18-21, remove the claim
that env_key satisfies non-loopback requirements and state that non-loopback
binds require manual configuration with an explicit reachable base_url and
admission token; in
devlog/_plan/260723_grok_build_bridge/020_docs_and_residual_smoke.md lines
18-24, document that non-loopback auto-registration is refused rather than
merely token-authenticated.
In `@devlog/_plan/260726_grok_build_prod/010_config_safety_core.md`:
- Around line 32-49: Update KEY_SEGMENT so both basic-quoted and literal-quoted
TOML key alternatives reject carriage returns and newlines, while basic-string
escape matching is limited to characters within the same line. Preserve
canonicalKeySegment behavior and ensure MODEL_HEADER cannot span lines through
quoted key segments.
In `@devlog/_plan/260726_grok_build_prod/010_config_safety.md`:
- Around line 94-108: The strip logic around the removal algorithm must match
the final one-separator-newline injection algorithm. Update the authoritative
injection and stripping behavior so files without a trailing newline retain one
newline after removal, then revise the related tests to exercise this exact
inject/strip pair rather than the obsolete two-newline assumption.
- Around line 5-17: Replace the non-loopback env_key design around
buildGrokManagedBlock with loopback-only automatic registration: skip
managed-fence generation for non-loopback binds and preserve manual credentials.
In devlog/_plan/260726_grok_build_prod/010_config_safety.md lines 5-17, document
this contract; update lines 111-120 to test skipping, cleanup, and credential
preservation. Update devlog/_plan/260726_grok_build_prod/030_docs_truth.md lines
5-10 to document manual non-loopback configuration, and remove automatic env_key
export instructions from devlog/_plan/260726_grok_build_prod/050_docs_truth.md
lines 17-27.
In `@devlog/_plan/260726_grok_build_prod/020_lifecycle_teardown.md`:
- Around line 18-27: Use typed ownership handling in both teardown paths: at
devlog/_plan/260726_grok_build_prod/020_lifecycle_teardown.md lines 18-27, set
sharedTeardownSafe to false only when isServiceOwnershipError(err) identifies a
ServiceOwnershipError, while preserving ordinary stop failures’ existing failure
semantics; at lines 104-123, return 409 and skip restore, strip, and drain only
for ownership refusal, and rethrow unrelated errors.
In `@devlog/_plan/260726_grok_build_prod/020_nonloopback_auth.md`:
- Around line 50-63: Update the non-loopback branch guarding isLoopbackHostname
to call stripGrokConfig before returning. Include the cleanup result in the
returned value and set changed based on whether the managed Grok configuration
was removed, while preserving the existing skippedReason and explanatory
message.
In `@devlog/_plan/260726_grok_build_prod/030_lifecycle_teardown.md`:
- Around line 177-179: Update
devlog/_plan/260726_grok_build_prod/030_lifecycle_teardown.md:177-179 to define
and wire an injectable shutdown dependency in ManagementApiDeps, replacing
direct process.exit(0) in the setTimeout → drainAndShutdown flow with that hook
while preserving production shutdown behavior. Update
devlog/_plan/260726_grok_build_prod/040_lifecycle_regressions.md:87-90 to run
the /api/stop success-path test only after substituting the shutdown hook so the
test runner remains active.
In `@devlog/_plan/260726_grok_build_prod/040_lifecycle_regressions.md`:
- Around line 67-76: The lifecycle regression coverage must execute handleStop
rather than only inspecting its source, using dependency injection or an
isolated CLI subprocess. Assert that a ServiceOwnershipError leaves the
ownership fence intact, sets a nonzero process.exitCode, and prevents
stripGrokConfig while preserving intended local-only cleanup; keep syncCleanup’s
OCX_SERVICE exclusion unchanged.
In `@docs-site/src/content/docs/guides/grok-build.md`:
- Around line 2-3: Update the frontmatter description and the
“Auto-registration” paragraph in the Grok Build guide to state that managed
~/.grok/config.toml registration occurs only when ocx start uses a loopback
bind; clarify that non-loopback binds skip registration and remove any existing
managed block. Keep the wording synchronized with the CLI behavior.
- Around line 33-36: Update the teardown behavior description in the guide to
avoid promising byte-for-byte restoration universally. State that user content
is preserved for supported uniform-EOL files, document that mixed-EOL files are
normalized by applyEol and therefore cannot be restored byte-for-byte, and keep
the service-mode block-preservation behavior accurate.
In `@gui/src/App.tsx`:
- Around line 181-191: Update the stop-request error handling around the fetch
logic in App.tsx to use localized fallback text from the GUI i18n locale files
instead of exposing backend English directly. Add the required locale key in
each supported locale, prefer mapping a stable API error code to localized text,
and retain the server message only as optional diagnostic detail; always show a
localized explanation when the response lacks a usable message.
In `@src/bridge.ts`:
- Around line 704-710: Replace every direct options?.onUsage?.(...) call in the
streaming response paths with the existing reportUsage(...) helper, including
the occurrences near the incomplete, completed, and other listed branches.
Update buildResponseJSON to use the same best-effort reportUsage isolation so
observer exceptions cannot convert successful responses into failures or prevent
returning the response.
In `@src/cli/index.ts`:
- Around line 455-470: Introduce an explicit shutdown result distinguishing not
installed, stopped, and failed states, and use it to gate shared teardown. In
src/cli/index.ts lines 455-470, allow restoreNativeCodex and stripGrokConfig
only for the not-installed or stopped states, not merely when ownershipBlocked
is false; in src/cli/index.ts lines 519-523, skip shared restoration after any
ownership or proxy-stop failure. Preserve existing failure reporting while
preventing teardown after an unconfirmed shutdown.
In `@src/grok/inject.ts`:
- Around line 193-206: Update the non-loopback branch in the surrounding
injection function to inspect the full result from stripGrokConfig, not only
removed.changed. When removal returns ok: false, propagate that failure result
or return an equivalent unsuccessful response so callers surface the
configuration error; retain the successful policy-skip response only when
cleanup succeeds.
🪄 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: 16586dd0-69b7-4e86-81ab-738c70e59072
📒 Files selected for processing (29)
devlog/_plan/260723_grok_build_bridge/000_plan.mddevlog/_plan/260723_grok_build_bridge/001_sol_source_analysis.mddevlog/_plan/260723_grok_build_bridge/010_usage_details_always.mddevlog/_plan/260723_grok_build_bridge/011_receipt.mddevlog/_plan/260723_grok_build_bridge/020_docs_and_residual_smoke.mddevlog/_plan/260723_grok_build_bridge/030_grok_config_autoinject.mddevlog/_plan/260723_grok_build_bridge/031_receipt.mddevlog/_plan/260723_grok_build_bridge/040_production_hardening.mddevlog/_plan/260723_grok_build_bridge/041_receipt.mddevlog/_plan/260723_grok_build_bridge/042_receipt.mddevlog/_plan/260726_grok_build_prod/000_blocker_inventory.mddevlog/_plan/260726_grok_build_prod/000_plan.mddevlog/_plan/260726_grok_build_prod/001_grok_source_evidence.mddevlog/_plan/260726_grok_build_prod/010_config_safety.mddevlog/_plan/260726_grok_build_prod/010_config_safety_core.mddevlog/_plan/260726_grok_build_prod/020_lifecycle_teardown.mddevlog/_plan/260726_grok_build_prod/020_nonloopback_auth.mddevlog/_plan/260726_grok_build_prod/030_docs_truth.mddevlog/_plan/260726_grok_build_prod/030_lifecycle_teardown.mddevlog/_plan/260726_grok_build_prod/040_lifecycle_regressions.mddevlog/_plan/260726_grok_build_prod/050_docs_truth.mddocs-site/astro.config.mjsdocs-site/src/content/docs/guides/grok-build.mdgui/src/App.tsxsrc/bridge.tssrc/chat/outbound.tssrc/cli/index.tssrc/codex/inject.tssrc/grok/inject.ts
| ### 4. 리스크 점검 | ||
| - 소비자: Codex CLI/App(관대), Claude inbound(`src/claude/outbound.ts`는 자체 변환 — 영향 경로 typecheck로 확인), request-log 파서(`src/server/request-log.ts:352-375`는 optional 읽기라 무해). | ||
| - `cached_tokens: 0` 상시 방출이 GUI 비용 계산에 미치는 영향: 0은 no-op이므로 없음 (확인: `usageDisplayTotalTokens` 경로). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Correct the request-log risk assessment.
The statement that optional request-log reads are harmless is false: the synthetic zero-valued detail objects initially caused wire usage to overwrite raw adapter usage and lose provenance. Update this section to reference the usageFromBridge guard documented in devlog/_plan/260723_grok_build_bridge/011_receipt.md.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 25-25: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 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/260723_grok_build_bridge/010_usage_details_always.md` around
lines 25 - 27, The request-log risk assessment incorrectly describes optional
reads as harmless. Update the risk-check bullet in “리스크 점검” to reference the
`usageFromBridge` guard documented in `011_receipt.md`, noting that it prevents
synthetic zero-valued usage details from overwriting raw adapter usage and its
provenance.
| ## c2 — restart 왕복 (라이브) | ||
|
|
||
| - 1회차 restart에서 **기존 결함 발견**: service-manager stop throw(home-mismatch)가 `stopFailed`→`process.exit(1)`로 이어져 ensure에 도달 못 하고 프록시가 죽은 채 종료. → `7c521a6c`: 경고는 유지하되 stopFailed로 승격하지 않음 (로컬 teardown/ensure는 service manager와 독립). | ||
| - 수정 후: `restart` → 구 pid stop + fence strip → 새 pid 기동 + fence 1 재주입. 연속 2회 왕복 모두 성공 (pid 56377→57004, uptime 리셋 확인). |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Do not continue teardown after a service-ownership failure.
A service-home mismatch means stopServiceIfInstalled() may have thrown before stopping the target service. Treating that as a warning and continuing can strip the shared Grok fence while the other service remains alive, leaving its proxy without routing configuration. Propagate the ownership failure, or require positive evidence that the target proxy stopped before stripping or restarting.
🤖 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/260723_grok_build_bridge/041_receipt.md` around lines 13 - 16,
The restart flow must not continue teardown after stopServiceIfInstalled()
reports a service-home ownership mismatch. Propagate the ownership failure or
require positive confirmation that the target proxy stopped before allowing
fence stripping, restart, or ensure operations; update the stopFailed handling
associated with the restart sequence while preserving normal successful-stop
behavior.
| # 000 — PR #403 리뷰 블로커 인벤토리 (실측) | ||
|
|
||
| 대상: `codex/260726-grok-build-prod` (PR #403 재기반), 로컬 `dev` @`5a550867` 위 15커밋. | ||
| 수집일: 2026-07-26. 출처: `gh api repos/lidge-jun/opencodex/pulls/403/comments`, | ||
| `gh api repos/lidge-jun/opencodex/issues/403/comments` (메인테이너 리뷰 2026-07-24T13:05:24Z). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Correct the audit dates before treating these records as evidence.
Both documents claim work occurred on July 26, 2026, but the current review date is July 25, 2026. Update the dates or mark these sections as planned/future work; otherwise the receipts falsely present future verification as completed.
devlog/_plan/260726_grok_build_prod/000_blocker_inventory.md#L7-L11: correct the collection date and source timestamp.devlog/_plan/260726_grok_build_prod/000_plan.md#L7-L16: correct the investigation date or label it as pending.
📍 Affects 2 files
devlog/_plan/260726_grok_build_prod/000_blocker_inventory.md#L7-L11(this comment)devlog/_plan/260726_grok_build_prod/000_plan.md#L7-L16
🤖 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/260726_grok_build_prod/000_blocker_inventory.md` around lines 7
- 11, Correct the future-dated audit metadata: in
devlog/_plan/260726_grok_build_prod/000_blocker_inventory.md lines 7-11, update
the collection date and source timestamp to the current review date of July 25,
2026; in devlog/_plan/260726_grok_build_prod/000_plan.md lines 7-16, update the
investigation date or explicitly label it as pending.
| opencodex 쪽 경계: | ||
|
|
||
| - `isApiAuthRequired(config) = !isLoopbackHostname(config.hostname)` (`src/server/auth-cors.ts:121`). 비루프백 바인드에서는 모든 데이터플레인 요청이 admission 토큰을 요구하므로, 현재 주입되는 `api_key = "opencodex-loopback"`은 **반드시 401**이 된다. 리뷰 지적이 실재한다. | ||
| - `~/.grok/config.toml`은 공유 파일이므로 실제 토큰을 직렬화하면 안 된다. `env_key = "OPENCODEX_API_AUTH_TOKEN"`이 두 요구를 동시에 만족한다. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Make the automatic-registration policy loopback-only.
The env_key approach is not a safe non-loopback solution: if resolution fails, Grok may fall back to a session credential and send it to the configured LAN endpoint. The final contract must refuse automatic registration on non-loopback binds and direct users to manual configuration with an explicit reachable base_url and admission token.
devlog/_plan/260726_grok_build_prod/000_plan.md#L18-L21: remove the claim thatenv_keysatisfies the non-loopback requirement.devlog/_plan/260723_grok_build_bridge/020_docs_and_residual_smoke.md#L18-L24: document non-loopback auto-registration as refused, not merely token-authenticated.
📍 Affects 2 files
devlog/_plan/260726_grok_build_prod/000_plan.md#L18-L21(this comment)devlog/_plan/260723_grok_build_bridge/020_docs_and_residual_smoke.md#L18-L24
🤖 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/260726_grok_build_prod/000_plan.md` around lines 18 - 21, Make
automatic registration explicitly loopback-only: in
devlog/_plan/260726_grok_build_prod/000_plan.md lines 18-21, remove the claim
that env_key satisfies non-loopback requirements and state that non-loopback
binds require manual configuration with an explicit reachable base_url and
admission token; in
devlog/_plan/260723_grok_build_bridge/020_docs_and_residual_smoke.md lines
18-24, document that non-loopback auto-registration is refused rather than
merely token-authenticated.
| ```ts | ||
| // bare | basic-string | literal-string — TOML이 허용하는 키 세그먼트 세 형태. | ||
| const KEY_SEGMENT = String.raw`(?:[A-Za-z0-9_-]+|"(?:[^"\\]|\\.)*"|'[^']*')`; | ||
| const MODEL_HEADER = new RegExp( | ||
| String.raw`^\s*\[\s*(${KEY_SEGMENT})\s*\.\s*(${KEY_SEGMENT})\s*\]\s*(?:#.*)?$`, | ||
| "gm", | ||
| ); | ||
| ``` | ||
|
|
||
| 그리고 세그먼트 → 실제 키 문자열로 되돌리는 단일 정규화 함수를 도입한다: | ||
|
|
||
| ```ts | ||
| /** TOML 키 세그먼트(bare/basic/literal)를 그것이 가리키는 실제 키로 되돌린다. */ | ||
| function canonicalKeySegment(raw: string): string | null { | ||
| if (raw.startsWith('"')) return decodeTomlBasicString(raw.slice(1, -1)); | ||
| if (raw.startsWith("'")) return raw.slice(1, -1); // literal string: 이스케이프 없음 | ||
| return raw; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Exclude newlines from every quoted TOML key segment.
KEY_SEGMENT uses [^"\\] and '[^']*', both of which can match \r/\n. That contradicts the stated newline-safety guarantee and allows the header regex to span multiline strings, potentially missing a real [model.<alias>] collision. Exclude line breaks from both quoted alternatives and ensure escaped characters cannot cross a line.
Also applies to: 95-109
🤖 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/260726_grok_build_prod/010_config_safety_core.md` around lines
32 - 49, Update KEY_SEGMENT so both basic-quoted and literal-quoted TOML key
alternatives reject carriage returns and newlines, while basic-string escape
matching is limited to characters within the same line. Preserve
canonicalKeySegment behavior and ensure MODEL_HEADER cannot span lines through
quoted key segments.
| - **Removed on teardown:** `ocx stop`, `ocx eject`, `ocx uninstall`, and graceful | ||
| non-service daemon shutdown strip the fenced block and restore your file | ||
| byte-for-byte. Under a service manager, teardown goes through `ocx stop`/`ocx | ||
| uninstall` (service-mode processes intentionally keep the block across respawns). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not promise byte-for-byte restoration for every file.
The configuration algorithm preserves user content for supported uniform EOL inputs, but mixed-EOL files are normalized by applyEol; those files cannot be restored byte-for-byte. Replace this with a precise preservation statement and document the mixed-EOL limitation.
As per path instructions, docs-site content must remain synchronized with actual CLI behavior.
🤖 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/grok-build.md` around lines 33 - 36, Update
the teardown behavior description in the guide to avoid promising byte-for-byte
restoration universally. State that user content is preserved for supported
uniform-EOL files, document that mixed-EOL files are normalized by applyEol and
therefore cannot be restored byte-for-byte, and keep the service-mode
block-preservation behavior accurate.
Source: Path instructions
| try { | ||
| const res = await fetch(`${API_BASE}/api/stop`, { method: "POST" }); | ||
| // A refusal (409: a service under another home owns this proxy) returns normally instead | ||
| // of dropping the connection, so the button would otherwise sit in "stopping…" forever | ||
| // with nothing explaining why. | ||
| if (!res.ok) { | ||
| setStopping(false); | ||
| const detail = await res.json().catch(() => null) as { message?: string } | null; | ||
| if (detail?.message) alert(detail.message); | ||
| } | ||
| } catch { /* connection drops — the proxy is going down as expected */ } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add localized fallback text for failed stops.
alert(detail.message) exposes backend English copy directly, and responses without a message show no explanation. Add a locale key for the failure fallback and preferably map a stable API error code to localized text while retaining the server detail as optional diagnostics.
As per path instructions, GUI user-visible strings should flow through the i18n locale files.
🤖 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/App.tsx` around lines 181 - 191, Update the stop-request error
handling around the fetch logic in App.tsx to use localized fallback text from
the GUI i18n locale files instead of exposing backend English directly. Add the
required locale key in each supported locale, prefer mapping a stable API error
code to localized text, and retain the server message only as optional
diagnostic detail; always show a localized explanation when the response lacks a
usable message.
Source: Path instructions
| options?.onUsage?.(event.usage); | ||
| emit("response.incomplete", { response }); | ||
| reportTerminal("incomplete"); | ||
| } else { | ||
| const response = { ...responseSnapshot("completed", finishedItems, event.endTurn), usage: responsesUsage(event.usage) }; | ||
| options?.onCompletedResponse?.(response, event.providerState); | ||
| options?.onUsage?.(event.usage); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent the usage observer from breaking successful responses.
These callbacks execute inside response generation. If onUsage throws, the streaming path converts an otherwise successful completion into response.failed, while buildResponseJSON throws instead of returning the response. Treat this request-log observer as best-effort, as already done for onFirstOutput.
Proposed fix
+const reportUsage = (usage: OcxUsage | undefined): void => {
+ try {
+ options?.onUsage?.(usage);
+ } catch {
+ // Observability must not alter the response.
+ }
+};Replace each direct options?.onUsage?.(...) invocation with reportUsage(...); apply the same isolation in buildResponseJSON.
Also applies to: 727-727, 751-751, 809-809, 1130-1130
🤖 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 704 - 710, Replace every direct
options?.onUsage?.(...) call in the streaming response paths with the existing
reportUsage(...) helper, including the occurrences near the incomplete,
completed, and other listed branches. Update buildResponseJSON to use the same
best-effort reportUsage isolation so observer exceptions cannot convert
successful responses into failures or prevent returning the response.
| if (!ownershipBlocked) { | ||
| const r = restoreNativeCodex(); | ||
| console.log(`↩️ ${r.message}`); | ||
| } | ||
| // revertSystemEnv is NOT gated: it carries its own ownership check and concerns launchctl | ||
| // user env, not CODEX_HOME. Safety net for when the daemon's syncCleanup didn't run (SIGKILL). | ||
| try { revertSystemEnv(); } catch { /* best-effort */ } | ||
| if (stopFailed) process.exit(1); | ||
| if (!ownershipBlocked) { | ||
| // Same safety net for the Grok Build managed block (marker-owned, idempotent). | ||
| try { | ||
| const g = stripGrokConfig(); | ||
| if (g.changed) console.log(`↩️ ${g.message}`); | ||
| // A refused strip (e.g. orphaned marker) leaves the fence pointing at a dead proxy — | ||
| // reporting success there hides a broken end state. | ||
| else if (!g.ok) { stopFailed = true; console.error(`⚠️ ${g.message}`); } | ||
| } catch { /* best-effort */ } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Require confirmed service/proxy shutdown before shared configuration teardown.
Both lifecycle paths can remove routing state after a failed stop:
src/cli/index.ts#L455-L470: gate native restoration and Grok stripping on confirmed service/proxy shutdown, not merely!ownershipBlocked.src/cli/index.ts#L519-L523: skip shared restoration steps once uninstall records an ownership or proxy-stop failure.
Use an explicit shutdown result that distinguishes “not installed,” “stopped,” and “failed,” then permit shared teardown only for the first two states.
📍 Affects 1 file
src/cli/index.ts#L455-L470(this comment)src/cli/index.ts#L519-L523
🤖 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/cli/index.ts` around lines 455 - 470, Introduce an explicit shutdown
result distinguishing not installed, stopped, and failed states, and use it to
gate shared teardown. In src/cli/index.ts lines 455-470, allow
restoreNativeCodex and stripGrokConfig only for the not-installed or stopped
states, not merely when ownershipBlocked is false; in src/cli/index.ts lines
519-523, skip shared restoration after any ownership or proxy-stop failure.
Preserve existing failure reporting while preventing teardown after an
unconfirmed shutdown.
| if (!isLoopbackHostname(opts.hostname)) { | ||
| const removed = stripGrokConfig({ ...(opts.grokHome !== undefined ? { grokHome: opts.grokHome } : {}) }); | ||
| const cleanup = removed.changed | ||
| ? " Removed the previously generated block, which pointed at a loopback address." | ||
| : ""; | ||
| return { | ||
| ok: true, // a deliberate policy skip, not a failure — it must never block startup | ||
| changed: removed.changed, | ||
| skippedReason: "non-loopback", | ||
| message: `Grok auto-registration skipped: opencodex is bound to the non-loopback host ` | ||
| + `"${opts.hostname}", where requests need your admission token. A managed block would ` | ||
| + `either store that secret in ~/.grok/config.toml or overwrite it on the next start, so ` | ||
| + `add the models yourself OUTSIDE the opencodex markers (see the Grok Build guide).${cleanup}`, | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not report a successful non-loopback transition when fence removal failed.
stripGrokConfig() can return ok: false for an orphaned marker or write failure, but this branch ignores that result and returns ok: true. The stale loopback block remains active while startup reports only a policy skip, so callers never surface the broken configuration.
Proposed fix
if (!isLoopbackHostname(opts.hostname)) {
const removed = stripGrokConfig({
...(opts.grokHome !== undefined ? { grokHome: opts.grokHome } : {}),
});
+ if (!removed.ok) {
+ return {
+ ...removed,
+ message: `Grok auto-registration skipped for non-loopback binding, but the existing managed block could not be removed. ${removed.message}`,
+ };
+ }
const cleanup = removed.changed📝 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 (!isLoopbackHostname(opts.hostname)) { | |
| const removed = stripGrokConfig({ ...(opts.grokHome !== undefined ? { grokHome: opts.grokHome } : {}) }); | |
| const cleanup = removed.changed | |
| ? " Removed the previously generated block, which pointed at a loopback address." | |
| : ""; | |
| return { | |
| ok: true, // a deliberate policy skip, not a failure — it must never block startup | |
| changed: removed.changed, | |
| skippedReason: "non-loopback", | |
| message: `Grok auto-registration skipped: opencodex is bound to the non-loopback host ` | |
| + `"${opts.hostname}", where requests need your admission token. A managed block would ` | |
| + `either store that secret in ~/.grok/config.toml or overwrite it on the next start, so ` | |
| + `add the models yourself OUTSIDE the opencodex markers (see the Grok Build guide).${cleanup}`, | |
| }; | |
| if (!isLoopbackHostname(opts.hostname)) { | |
| const removed = stripGrokConfig({ ...(opts.grokHome !== undefined ? { grokHome: opts.grokHome } : {}) }); | |
| if (!removed.ok) { | |
| return { | |
| ...removed, | |
| message: `Grok auto-registration skipped for non-loopback binding, but the existing managed block could not be removed. ${removed.message}`, | |
| }; | |
| } | |
| const cleanup = removed.changed | |
| ? " Removed the previously generated block, which pointed at a loopback address." | |
| : ""; | |
| return { | |
| ok: true, // a deliberate policy skip, not a failure — it must never block startup | |
| changed: removed.changed, | |
| skippedReason: "non-loopback", | |
| message: `Grok auto-registration skipped: opencodex is bound to the non-loopback host ` | |
| `"${opts.hostname}", where requests need your admission token. A managed block would ` | |
| `either store that secret in ~/.grok/config.toml or overwrite it on the next start, so ` | |
| `add the models yourself OUTSIDE the opencodex markers (see the Grok Build guide).${cleanup}`, | |
| }; |
🤖 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/grok/inject.ts` around lines 193 - 206, Update the non-loopback branch in
the surrounding injection function to inspect the full result from
stripGrokConfig, not only removed.changed. When removal returns ok: false,
propagate that failure result or return an equivalent unsuccessful response so
callers surface the configuration error; retain the successful policy-skip
response only when cleanup succeeds.
|
Self-review note at head This should not land as one commit. It changes roughly 40 files and mixes four separable concerns: strict usage-shape compatibility, the Grok config parser/injector, lifecycle/teardown, and docs/GUI. It also carries unrelated devlog churn. Split into four, in that order — the strict-client slice is independently useful and reviewable, while the lifecycle slice needs to be re-cut against whatever the service/process-control contracts look like when it lands. The Outstanding defects found:
Docs overclaim. "restore your file byte-for-byte" is not the actual contract: uniform-EOL content is preserved, but mixed-EOL files are normalized by Suggested regression once Recorded during a maintainer PABCD pass. Held pending split and second-maintainer review. |
통합(dev push 완료): #437 803807a, #460 74ddd96, #431 82a47db, #405 be16c1d, #468 후속 테스트 bcaf029. close: 통합 4건 + 통합 없이 1건(#459, 설계 충돌). #457은 OPEN 유지. 이슈 close 0건 — dev에서 고쳐졌는데 안 닫힌 이슈가 없었다. 무효화: WP4(#466)와 WP6(#467)은 동료가 직접 머지했고, 우리 분석이 지적한 결함을 동료 커밋이 같은 방향으로 해소했다. 보안 보류 5건에 신규 발견 게시: #429 재분류, #355 OAuth 토큰 목적지, #424 유료 브리지 기본 ON, #408 설치 락 해제, #403 4분할 요청. A-gate는 사이클마다 3~5라운드를 돌았다. 반복된 원인은 계획서에 테스트나 코드를 적으면서 실물 소스를 확인하지 않은 것이었다.
|
Closing this PR because its work has landed on Containment proof. This PR's head commit It arrived via the integration merge One conflict was resolved on the way in. The full Grok Build surface merged intact: the fenced managed-block inject/strip for The head branch |
Summary
~/.grok/config.toml.usageFromBridge).Motivation
Grok Build's pinned OpenAI client requires usage detail objects that some routed providers do not naturally report. Omitting those objects can make an otherwise successful turn fail during response deserialization.
Grok Build also does not reliably apply inherited provider
base_urlsettings to inference routing. The generated configuration therefore uses direct per-model fields and keeps the visible opencodex catalog synchronized through a single managed block.Configuration safety
Rebase note
Rebased onto current
origin/dev(was 209 behind, conflicting insrc/cli/index.tsandsrc/server/responses.ts). Theresponses.tsonUsageprovenance hooks were re-applied to the post-splitsrc/server/responses/core.ts; the CLI inject/strip calls were merged with the currentcurrentExternalCodexModelProviderguard.Tests
bun run typecheck✅bun run test✅ (4049 pass / 0 fail)bun run privacy:scan✅Focused coverage includes: always-present Responses and Chat Completions usage detail objects; raw adapter usage provenance across completed, incomplete, failed, and EOF paths; idempotent Grok config injection and cleanup; user alias collision handling and malformed-fence safety; catalog refresh through
syncGrokConfig; heartbeat consumption without leaking raw Responses frames.Summary by CodeRabbit
response.heartbeatframes from leaking into Chat Completions outputs.