Skip to content

test(config): canonicalize the owner-registry ACL test root (macOS) - #6411

Merged
lidge-jun merged 1 commit into
devfrom
codex/rel-l4-owner-registry-ci
Oct 1, 2026
Merged

lidge-jun merged 1 commit into
devfrom
codex/rel-l4-owner-registry-ci

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Oct 1, 2026 •

Copy link
Copy Markdown
Owner

Summary

Since #6375 (6e5101115c), dev fails on macOS in tests/config/owner-registry-acl.test.ts:72, "registry publication requires the owner-only Windows directory ACL before writing an entry". readOwnerRegistry().homes is [] where [home] is expected. The test fails in macos 2/2 and macos control, for example job 110434978022.

The cause is in the test. atomicWriteFileNoFollow names its temp file from resolveWriteTarget(dirname(path)), which calls realpathSync. The test enables simulated Windows hardening, so the writer runs the mocked icacls on that temp path, and the runner asserts args[0].startsWith(root + sep). On macOS tmpdir() is under /var, a symlink to /private/var. The canonical temp path fails that assertion, the required entry hardening throws, registerOwnerRegistryHome swallows the error, and no entry is written. Linux passes because its temp directory is not a symlink. Production is unaffected because the real icacls has no prefix check.

The fix canonicalizes the test root with realpathSync, the same function the writer uses. The runtime code and the owner-only, no-follow guarantees from #6375 are unchanged.

The Windows tests/cli/sibling-home-client-sync.test.ts failures seen at the same time ("timed out waiting for secondary runtime record") are not a #6375 regression. They came from a slow runner: in job 110434332429 every test in that file that starts a child ran about 3× slower. Two later lane=all runs on the same dev base, which includes #6375, passed all four of those tests in 4.5–8.7 s: windows 3/9 110435440871 and windows 3/9 110446096011. Pre-#6375 timings were 4.6–7.0 s. Registration also cannot delay the record the test waits for, because writeRuntimePort writes runtime-port.json before it calls registerOwnHome(). With real icacls on the runner, "publishing a runtime record registers the home for cross-home discovery" passed in 137–166 ms.

Verification

  • I did not run local test suites or typecheck. That is an explicit instruction from the project owner, not the AGENTS.md resource exception. Hosted CI at the exact head is the evidence, including a dispatched lane=all run that covers the macOS and Windows legs.
  • git diff --check passes. The test file is not in the file-size baseline.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. Test-only change.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. Test-only. The owner-registry hardening code is untouched, and an independent review is bound to the head below.

Summary by CodeRabbit

  • Tests
    • Access-control registration tests now validate paths consistently when temporary directories resolve through symbolic links or other filesystem aliases. Existing registration scenarios and expected access-control behavior remain unchanged.

@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner October 1, 2026 16:05
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-01T16:07:17.953178Z d11845f PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

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: f8b4e1ba-cb70-472c-a112-58464f6023dd

📥 Commits

Reviewing files that changed from the base of the PR and between bd3d303 and d11845f.

📒 Files selected for processing (1)
  • tests/config/owner-registry-acl.test.ts

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


📝 Walkthrough

Walkthrough

The ACL registration test setup now resolves its temporary directory to a real path before assigning root. The existing registration tests and assertions remain unchanged.

Changes

ACL test setup

Layer / File(s) Summary
Canonicalize temporary root
tests/config/owner-registry-acl.test.ts
The test imports realpathSync and resolves the temporary directory before assigning root for path-prefix assertions.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to d1184

The ACL test now uses the same canonical path as the writer, and cleanup still targets that directory. No actionable merge-blocking risk remains.

Architecture Summary

Architecture risk: 🔵 Low · up to d1184

The change affects 1 system.

Changed systems: tests

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — tests (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in tests/config/owner-registry-acl.test.ts: The filesystem imports now include realpathSync.
  • observed — Modified behavior in tests/config/owner-registry-acl.test.ts: beforeEach now resolves the temporary directory to its real path before assigning root; it previously used the raw path returned by mkdtempSync.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 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: canonicalizing the owner-registry ACL test root for macOS compatibility.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
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.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • 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 chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Oct 1, 2026
@lidge-jun

Copy link
Copy Markdown
Owner Author

Maintainer integration record (MAINTAINERS.md, dev-only)

  • Decision: integrate test(config): canonicalize the owner-registry ACL test root (macOS) #6411 into dev without a second maintainer approval, as the project owner authorized on 2026-10-01. It fixes the macOS failure that fix(config): require owner-only no-follow registry publication #6375 introduced on dev.
  • Exact head: d11845fdfaefb977e9700a343b94d17cc59360cf. Base 404ca8b551. It is 8 commits behind the current dev, and git merge-tree --write-tree is clean.
  • Hosted CI at this head: PR-lane Cross-platform CI run 36889285375 succeeded, with test 1/4 to test 4/4, gates, storage policy, api usage, docker smoke, the keyring jobs and ci. React Doctor run 36889285387 succeeded. The dispatched lane=all run 36884687869 also ran at this head. In macos 2/2 (job 110449136523), all seven owner-registry ACL cases passed, including the one at line 72 that had failed. All nine Windows shards succeeded.
  • Windows sibling-home-client-sync: this is not a fix(config): require owner-only no-follow registry publication #6375 regression. The red run (job 110434332429) had every test that starts a child about 3× slower. Two lane=all runs on the same dev base passed all four of those tests in 4.5–8.7 s (jobs 110435440871 and 110446096011). The runtime record is written before registration, so registration cannot delay it. The real-icacls registration test passed in 137–166 ms.
  • Local suites: not run, by explicit owner instruction.
  • Security review: an independent read-only reviewer returned VERDICT: PASS — HEAD d11845f. The change is test-only, every assertion is unchanged, and canonicalizing the root makes the prefix checks stricter.
  • Attribution: maintainer change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant