Skip to content

fix(codex): verify service manager identity before census delegation - #6379

Closed
luvs01 wants to merge 9 commits into
devfrom
fix/luvs01-682-service-probe-boundary-20261001
Closed

luvs01 wants to merge 9 commits into
devfrom
fix/luvs01-682-service-probe-boundary-20261001

Conversation

@luvs01

@luvs01 luvs01 commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

A managed service could select a recorded runtime without proving that the live service-manager definition belonged to the same OpenCodex and Codex homes. Bind census delegation to that definition and refuse missing, ambiguous, or mismatched identity evidence before probing or launching a candidate.

Decode the quoted home assignments emitted by the systemd writer, including escaped percent signs, backslashes, and quotes. Reject unsupported physical line continuations before recognizing directives so an override cannot disappear from the identity check. Also invalidate definitions with EnvironmentFile, PassEnvironment, UnsetEnvironment, or directive keys whose effect cannot be proven harmless, including escaped or malformed keys. Ordinary generated assignments, comments, section headings, and plain non-environment directives such as X-Custom remain supported. The existing newer-runtime selection and fallback behavior remain within that verified home identity.

Source history is preserved through commit 768ad08ba65d97ce2c805162fbbe3d6807417ea0, including the continuation fix b5263bb2e54abfb2d82bbc52f2bed817e32655a9 and the bot's additional directive guard. The fresh local validation below ran on that source HEAD, which includes dev at 0328373fb88fe0d019b29ae278e153d7fed4bcc7. Validation of the later upstream-base integration is a separate publication gate.

Verification

Current follow-up

Follow-up verification for 1d75b0267cd0797dddfa818f0b178e2d49fac978:

Canonicalize the test fixture root to match getCodexHome realpath resolution. Path-specific read-error injection now reaches the same file under an aliased temp path; existing 500 status, error shape and path-nondisclosure assertions stay intact. No production or service probe code changed.

A temporary path alias reproduced the original expected-500/received-200 failure. Canonicalizing only the fixture path retained the error and nondisclosure assertions; 157 focused role-route/service/systemd tests then passed with 574 assertions. The temporary setup experiments that stalled or failed before the test body are not counted as passing tests.

Executed focused commands:

  • bun test tests/server/codex-agent-role-routes.test.ts tests/codex-integration/codex-service-manager-probe.test.ts tests/codex-integration/codex-systemd-env-probe.test.ts --timeout 30000 — passed.

Required typecheck, structure, privacy and file-size checks passed. Full local suite and unfiltered test:changed were not run; the documented focused/resource exception and remaining coverage limitations remain in effect. No blocked broad Kiro/SOCKS transport test or real-provider validation is claimed.

Cross-platform CI succeeded for this exact HEAD. Checkout b57a383c53bccc79306e146c0b9127197991fe89 and the PR HEAD share tree 2c2323fc751833a6799fa501d1d0fbc29c00aacc. Skipped jobs remain skipped.

Independent maintainer approval, explicit security review and any required sponsorship remain pending before merge.

Earlier integration evidence

Current upstream integration: a6114b62ed1b65dede802359ccd373d913979b24; published HEAD 73332579e937081bb14c7f96492fc421efa30ace. The source history through 768ad08ba65d97ce2c805162fbbe3d6807417ea0 is retained without rewriting authors or commits. The PR-specific change was checked again after this integration; typecheck, structure, privacy and file-size gates passed. CI was pending when this earlier integration checkpoint was published; the current follow-up is verified above.

Windows, Bun 1.4.0, isolated test temporary directory:

  • bun test tests/codex-integration/codex-service-manager-probe.test.ts tests/codex-integration/codex-systemd-env-probe.test.ts --timeout 30000: 150 passed, 0 failed, 525 assertions. Both suites ran in full, including all 22 added cases. Fixtures cover online/offline manager output, generated home escaping, continuation rejection, unsupported environment directives, malformed directive keys, accepted ordinary directives, and ownership comparisons with a fake probe runner.
  • bun node_modules/typescript/bin/tsc --noEmit: passed on the current source HEAD.
  • bun scripts/structure-ssot.ts: passed on the current source HEAD.
  • bun scripts/privacy-scan.ts: passed on the current source HEAD.
  • bun scripts/file-size-ratchet.ts: passed on the current source HEAD.
  • git diff --check 366b9f9ec2f2f22251d5ea79ea3a34c59be6e899 768ad08ba65d97ce2c805162fbbe3d6807417ea0: passed.

The full local suite and test:changed were not run under the repository's focused-validation exception: concurrent isolated worktrees made the full suite impractical, and this check was limited to synthetic service probes. No real service installation, service mutation, or installed-runtime handoff was performed. Required upstream CI on the final published HEAD is pending; results from another HEAD are not its verification.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • The service ownership contract and regression tests are updated.
  • Security-sensitive changes have independent maintainer review; sponsorship is required before merge where the sponsored-surface gate applies.
  • Author validation is documented for 1d75b0267cd0797dddfa818f0b178e2d49fac978, known correct findings are addressed, and exact-HEAD CI is complete; author review readiness is confirmed.

Independent maintainer approval, explicit security review, and any required maintainer-sponsored decision remain pending before merge. These are separate from author review readiness; no approval or sponsorship is claimed.

luvs01 and others added 8 commits September 29, 2026 01:41
…d homes

Three review findings: a service claim that omits its home keys now only authorizes a process still resolving its default homes, so a project .env cannot borrow a default installs registration for a project-chosen census directory; launchd environment values are XML-decoded so an escaped home still compares equal to itself; and the manager-definition provenance probe is gated on a recorded newer candidate so an empty census no longer pays a full Task Scheduler listing.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Epinephrine <luvs01@hanmail.net>
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 14 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: dcca3f67-465e-4624-a572-f7dc61ad9dc2

📥 Commits

Reviewing files that changed from the base of the PR and between a6114b6 and 1d75b02.

📒 Files selected for processing (10)
  • scripts/test-layout/layout.json
  • src/config/serving-runtimes.ts
  • src/service-manager-probe.ts
  • src/service/systemd-env.ts
  • structure/ops/service-and-sidecars.md
  • tests/codex-integration/codex-service-manager-probe.test.ts
  • tests/codex-integration/codex-systemd-env-probe.test.ts
  • tests/config/serving-runtimes.test.ts
  • tests/fixtures/test-layout-expected.json
  • tests/server/codex-agent-role-routes.test.ts
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 1, 2026
@luvs01

luvs01 commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up verification for 1d75b0267cd0797dddfa818f0b178e2d49fac978:

Canonicalize the test fixture root to match getCodexHome realpath resolution. Path-specific read-error injection now reaches the same file under an aliased temp path; existing 500 status, error shape and path-nondisclosure assertions stay intact. No production or service probe code changed.

A temporary path alias reproduced the original expected-500/received-200 failure. Canonicalizing only the fixture path retained the error and nondisclosure assertions; 157 focused role-route/service/systemd tests then passed with 574 assertions. The temporary setup experiments that stalled or failed before the test body are not counted as passing tests.

Executed focused commands:

  • bun test tests/server/codex-agent-role-routes.test.ts tests/codex-integration/codex-service-manager-probe.test.ts tests/codex-integration/codex-systemd-env-probe.test.ts --timeout 30000 — passed.

Required typecheck, structure, privacy and file-size checks passed. Full local suite and unfiltered test:changed were not run; the documented focused/resource exception and remaining coverage limitations remain in effect. No blocked broad Kiro/SOCKS transport test or real-provider validation is claimed.

Cross-platform CI succeeded for this exact HEAD. Checkout b57a383c53bccc79306e146c0b9127197991fe89 and the PR HEAD share tree 2c2323fc751833a6799fa501d1d0fbc29c00aacc. Skipped jobs remain skipped.

Independent maintainer approval, explicit security review and any required sponsorship remain pending before merge.

@lidge-jun

Copy link
Copy Markdown
Owner

Carried to #6397 so it can land in the next release. Your commits are kept as they are and the squash will carry Co-authored-by.

#6397 adds one hardening commit, c7c4b98f2e: for the WinSW backend, the census delegation gate also requires trusted sc.exe qc to report the claim's own executable as the registered binary, and refuses delegation otherwise. Other backends are unchanged. Thank you for the change.

lidge-jun added a commit that referenced this pull request Oct 1, 2026
…(carry #6379) (#6397)

A managed service could select a recorded runtime without proving that the live service-manager definition belonged to the same OpenCodex and Codex homes. Bind census delegation to that definition and refuse missing, ambiguous, or mismatched identity evidence before probing or launching a candidate. Decode the quoted home assignments emitted by the systemd writer. For the WinSW backend, also require trusted sc.exe qc to report the definition's own executable as the registered binary.

Carried from #6379.

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev through the maintainer carry #6397 (a9d5a80), which keeps your authorship with a Co-authored-by trailer. Thank you!

@lidge-jun lidge-jun closed this Oct 1, 2026
@lidge-jun
lidge-jun deleted the fix/luvs01-682-service-probe-boundary-20261001 branch October 3, 2026 00:15
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