Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Admission checks and sync outcomes src/grok/sync.ts, tests/providers/xai/grok-sync.test.ts |
syncGrokConfig accepts an optional admission callback. If admission is denied before discovery or after discovery, sync returns success without changes. Tests cover denied, changed, and continuing admission. |
CLI admission wiring and race coverage src/cli/ensure-desired-integrations.ts, src/cli/index.ts, structure/runtime.md, tests/cli/ensure-desired-integrations-race.test.ts |
The ensure and startup callers supply callbacks that recheck the persisted Grok setting. Startup also rejects sync during shutdown or after cleanup. The race test checks that disabling Grok during catalog discovery prevents injection. Runtime documentation records the checks and hostname handling. |
Startup discovery integration coverage tests/codex-integration/codex-composed-acceptance.test.ts, tests/fixtures/grok-startup-discovery-preload.ts |
The integration test holds startup discovery while Grok is disabled or shutdown begins. It checks that Grok configuration remains unchanged and, during shutdown, that the admitted request completes before the server exits. The fixture controls discovery and can signal shutdown. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Suggested reviewers: lidge-jun
Merge Risk: 
7e263
-
Grok synchronization now rechecks disable and shutdown state before writing. No actionable merge-blocking risk remains.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "fix(grok): honor disable and shutdown during startup sync" matches the main change. The raw summary shows caller-admission checks added to syncGrokConfig, the startup path in src/cli/index.… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "fix(grok): honor disable and shutdown during startup sync" matches the main change. The raw summary shows caller-admission checks added to syncGrokConfig, the startup path in src/cli/index.ts, and the ensure/restart path. These checks stop Grok config writes after the user disables Grok or shutdown begins while catalog discovery is pending. The tests cover these cases. The title is specific and concise, and it uses the conventional commit form.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 7 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
|
✅ Deterministic PR hygiene checks passed. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
No description provided. |
…x-grok-startup-admission-20261030
|
No description provided. |
Current startup regression follow-up at
7e2638c5795e68220c3839274ee0920faa3c0682The two new cases execute the real CLI startup, actual Grok sync and actual writer. A child-only preload holds the Grok-specific catalog result after the real gather, allowing earlier Desktop/Claude catalog consumers to finish normally. During that await, one case persists Grok OFF and one invokes the installed SIGINT handler while a real admitted chat turn keeps drain open. Both check the original config, absent backup and absent update log before cleanup can hide a transient write.
Deleting only the post-await admission check makes both cases fail on an actual config rewrite. The production SHA was restored; no production code is changed by this follow-up. Final Windows Bun1.4.4 focused validation: 3 pass / 24 assertions including the existing startup/stop/restart control, typecheck 2,252 source files, structure/privacy/file-size gates pass. Initial broad provider holds were discarded as post-await evidence because they could target an earlier registry consumer; the committed fixture pins the Grok await. Exact-head fork CI41128440963/Catalog41128440953/Service41128440957 and upstream CI41128443210/Catalog41128443372/Service41128443209 succeeded. All upstream required checks pass; no unresolved review threads remain. Independent read-only review found no required changes. Earlier results below remain historical. Duri owns upstream lidge-jun#8017 CI, body and Ready metadata.
Summary
Turning Grok off while startup or
ensureis waiting for model discovery could still allow the old sync to recreate the managed block. Startup could also finish that write after shutdown cleanup had begun.Add an optional caller-admission check before discovery and immediately before the synchronous writer. Startup rechecks the durable switch and its shutdown/cleanup latches; ensure/restart rechecks the current switch. Deferred-catalog tests cover disable, shutdown, unchanged admission, and the real ensure wiring. The runtime contract documents the same-process guarantee; this does not introduce a cross-process transaction between the desired-state and Grok files.
This is a separate follow-up to the investigation around #803. The stale-sync path is deterministically reproduced, but the event ordering of the earlier Ubuntu CI failure is not established.
Verification
Windows 11 x64, pinned Bun 1.4.4, isolated homes and the normal test preload/lock:
bun test ./tests/providers/xai/grok-sync.test.ts ./tests/cli/ensure-desired-integrations-race.test.ts ./tests/codex-integration/native-grok-toggle.test.ts ./tests/codex-integration/codex-desired-state.test.ts ./tests/cli/hub-gated-local-clients.test.ts ./tests/server/proxy-liveness-bind-scope.test.ts— 142 pass, 636 assertions.bun test ./tests/codex-integration/codex-composed-acceptance.test.ts --test-name-pattern 'Grok E2E: route-disabled'— 1 pass, 8 assertions through real startup/stop/restart.bun test ./tests/ci-workflows/file-size-ratchet.test.ts --test-name-pattern 'file-size ratchet: repository'— 1 pass, 6 assertions.bun scripts/structure-ssot.ts,bun scripts/privacy-scan.ts, andgit diff --checkpassed.The full import-graph suite is left to hosted CI: this CLI dependency area selected over 51,000 tests in the related run, so local validation used the affected behavior, management integration, and real startup E2E rather than another full run on the shared Windows host. Earlier-head d23419b Cross-platform CI 41049743384, Catalog 41049743470, and Service lifecycle 41049743428 passed. That earlier changed-test log confirms the five admission/ensure cases ran. The integrated 22b3e07 head now passes Cross-platform CI41109075143, Catalog41109075076, Service41109075062 and all required checks. No unresolved review threads were present at readiness readback; earlier-head results were not carried forward. No live user configuration or service was changed.
Checklist
Summary by CodeRabbit