Skip to content

fix(config): coordinate Grok and link writers - #6869

Merged
lidge-jun merged 2 commits into
lidge-jun:devfrom
luvs01:submission/grok-link-writers-20261010
Oct 10, 2026
Merged

lidge-jun merged 2 commits into
lidge-jun:devfrom
luvs01:submission/grok-link-writers-20261010

Conversation

@luvs01

@luvs01 luvs01 commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Grok injection and cleanup now hold the existing canonical config-write lock across read, transform, backup, and publication. A config alias retarget after the read previously allowed a write to the replacement target. Publication now validates the held destination before every rename, including retry hooks; a refresh-only destination change before rename returns the existing skip result while preserving files. Refresh-only admission and no-follow checks remain in place. Unsafe namespaces refuse without writes and remain distinct from retryable contention.

Link create/delete, compensation, and listener-port persistence now use the shared config mutation transaction. The listener rechecks the recorded links and port before activation: deleting the last link during bind closes the unneeded listener without publishing its port.

This is an independent backend change. Codex and agent-settings coordination already merged in #6811 is unchanged. Coupon state/route changes are submitted separately. The commits carry the original contributor credit.

Verification

At current head 9e9a4a552c4bcf1acd4a4e5514c160b44368f2aa (tree d5c1258bcffa5caa92e006d63bd17395888e9db1), the affected Grok and unattended-refresh coverage passed:

bun scripts/test.ts --parallel=1 tests/clients/sync-client-integrations-unattended.test.ts tests/providers/xai/grok-config-write-lock.test.ts tests/providers/xai/grok-config-inject.test.ts tests/providers/xai/grok-effort-inject.test.ts tests/codex-integration/catalog-auto-refresh-client-fanout.test.ts

Result: 128 pass / 0 fail, including destination drift after temp write. Typecheck, structure, and privacy checks passed on the same tree. The initial hosted CI exposed one return-contract regression in that existing symlink test; the follow-up commit preserves the no-write result and restores refresh-only skip semantics.

Supporting validation at initial head f8c00b579de8d6a508bfd8ca708eb700b7daa2cc (tree ba24d78bac10cfa92a2f5bb6f793dab78e9569d0):

bun scripts/test.ts --parallel=1 tests/providers/xai/grok-config-write-lock.test.ts tests/providers/xai/grok-config-inject.test.ts tests/providers/xai/grok-effort-inject.test.ts tests/server/link-listener-mutation.test.ts tests/server/link-listener-lifecycle.test.ts tests/server/link-listener-admission.test.ts tests/server/link-management-routes.test.ts tests/clients/link-store.test.ts tests/clients/link-compensation.test.ts tests/codex-integration/catalog-auto-refresh-client-fanout.test.ts

Result: 124 pass / 0 fail. Alias retargets, held/unsafe locks, new and known-port bind deletion, cross-process DELETE contention, compensation, and refresh-only fanout are covered with synthetic fixtures and mocked transport.

  • bun run typecheck, bun run structure:check, bun run privacy:scan: pass.
  • bun scripts/test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts tests/ci-workflows/file-size-ratchet.test.ts: 28 pass / 0 fail.
  • Prior combined local candidate: scoped import-connected changed tests 515 pass, native Grok/core-Lab boundary 49 pass. These counts support the initial split tree; the current follow-up has separate affected-path validation above.
  • Full local suite and native Windows/macOS execution were not run. Local preparation used focused coverage because multiple existing worktrees share this environment; hosted CI results are recorded below.
  • Common reviewed base: d25391ddccb0d9d03128ea5f9bc734e904c84e24. Current dev a47a1b52a94ef3d65d8435944bd4e6bf34a2e622 is two test-runner/CLI commits ahead; runtime and docs are identical, and the static merge is clean. The installed identity policy/hook is unchanged, and all candidate commits after the pinned base pass it.
  • Cross-platform CI and React Doctor passed for the updated head. Hosted tests checked out GitHub merge commit 49bab6a0648592bc1bf5ff51bf3f708744f47cd8 against dev. All four Linux shards, GUI gate (3,023 tests), type/privacy/structure gates, Docker, storage/API gates, and Ubuntu/Windows keyring and npm-global smokes passed. The original refresh-only symlink assertion also passed in hosted shard 3. Windows nine-shard and macOS jobs were skipped by path selection. The initial head failed the return-contract assertion and was superseded; it is not reported as passing. Author security review is complete for this exact head: the canonical lock is held through backup/publication, destination validation is passed to every rename attempt, refresh-only no-follow/admission checks remain, Link compensation preserves failure evidence, and no management-auth, key scope, remote-bind or secret-logging change is introduced. Existing exact-head CI and synthetic compensation/admission regressions passed. Ready for maintainer review; maintainer merge approval remains separate.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Bug Fixes
    • Link changes now preserve concurrent updates, and the listener won’t remain active if its links are removed while it is starting.
    • Grok configuration updates now avoid publishing changes when another write is in progress or the destination cannot be safely validated. These cases are reported as either retryable or non-retryable.
    • Link removal now reports a failure and retains the existing record when the update cannot be completed.

Hold the canonical config lock through Grok injection and cleanup, validate publication destinations, and preserve refresh-only admission. Coordinate link store mutations and revalidate recorded links before activating a bound listener.

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 81c4b4f9-7c69-461c-82ba-40f9119f1cce

📥 Commits

Reviewing files that changed from the base of the PR and between a47a1b5 and 9e9a4a5.


📒 Files selected for processing (10)
  • scripts/test-layout/layout.json
  • src/grok/inject.ts
  • src/server/index/link-listener.ts
  • src/server/management/link-routes.ts
  • structure/remote-link.md
  • structure/runtime.md
  • tests/fixtures/test-layout-expected.json
  • tests/providers/xai/grok-config-write-lock.test.ts
  • tests/server/link-listener-mutation.test.ts
  • tests/server/link-management-routes.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.



📝 Walkthrough

Walkthrough

Grok configuration injection and cleanup now use the shared config-write lock and atomic publication. Remote-link management and listener binding now coordinate store updates under the config mutation lock. New tests cover lock refusals, destination changes, concurrent link updates, and listener bind cancellation.

Changes

Grok configuration writes

Layer / File(s) Summary
Locked injection and cleanup
src/grok/inject.ts
Injection and cleanup run under the config-write lock. They validate destinations and publish atomically. Lock contention returns a retryable locked refusal; other lock or destination verification failures return a non-retryable unsafe refusal.
Write-path tests and mapping
tests/providers/xai/grok-config-write-lock.test.ts, scripts/test-layout/layout.json, tests/fixtures/test-layout-expected.json
Tests cover lock contention, an unsafe lock namespace, and symlink retargeting during injection and cleanup. The new test is mapped to providers/xai.

Remote-link store mutations

Layer / File(s) Summary
Locked link updates and listener binding
src/server/management/link-routes.ts, src/server/index/link-listener.ts
Link creation, compensation, and removal perform store read-modify-write operations under the mutation lock. Listener binding rechecks link presence and the stored port under the lock before adopting the listener.
Mutation tests and documentation
tests/server/link-listener-mutation.test.ts, tests/server/link-management-routes.test.ts, scripts/test-layout/layout.json, tests/fixtures/test-layout-expected.json, structure/remote-link.md, structure/runtime.md
Tests cover store changes during binding, cancellation when links disappear, and removal while another process holds the lock. Test mappings and structure documentation describe the updated behavior.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix


Merge Risk: ⚪ Minimal · up to 9e9a4

Grok config writes and remote-link store updates now coordinate through the shared config locks. Lock contention and unsafe states return explicit refusals, and listener binding rechecks the recorded links and port before it activates. No outstanding defects were identified, so the change appears ready to merge.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 6 files. (4 skipped: 4 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: coordinating Grok configuration writes and link writers through shared locking.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 6 files. (4 skipped: 4 unsupported.)



  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 10, 2026
@luvs01
luvs01 marked this pull request as ready for review October 10, 2026 02:57
@chatgpt-codex-connector

Copy link
Copy Markdown

The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins.

@lidge-jun

Copy link
Copy Markdown
Owner

Coordinator security sign-off for squash merge at head 9e9a4a552c4bcf1acd4a4e5514c160b44368f2aa.

An independent bounded review traced lock ordering, reentrancy and deadlock risk, rollback, and cross-process contention, and found no defects. Grok inject/strip take only the canonical file lock with synchronous callbacks. Link operations take only the SQLite mutation lock (BEGIN IMMEDIATE, busy_timeout=0), so contention fails fast instead of blocking. Focused regressions passed 30/30, adjacent lock and link suites passed 163/163, and typecheck, privacy:scan and structure:check all passed. Exact-head Cross-platform CI 38017215189: aggregate ci, test 1-4/4, gates and hygiene succeeded. Windows, macOS and docs shards were skipped by the changes filter.

@lidge-jun
lidge-jun merged commit be23d01 into lidge-jun:dev Oct 10, 2026
49 of 50 checks passed
@lidge-jun lidge-jun mentioned this pull request Oct 10, 2026
3 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants