fix(codexcli): emit project-root filesystem globs - #1602
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a93763b4c
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (Object.keys(projectRootFilesystem).length > 0) { | ||
| if (Object.keys(projectRootFilesystem).some((pattern) => pattern.includes("**"))) { | ||
| filesystem.glob_scan_max_depth = CODEX_GLOB_SCAN_MAX_DEPTH; |
There was a problem hiding this comment.
When a rulesync deny/read-ask rule uses ** to block files throughout the project, such as **/*.env, this line emits glob_scan_max_depth = 8; Codex documents this setting as the maximum depth for expanding unreadable glob patterns, so files nested beyond that depth are not masked. Rulesync's ** is unbounded, so the generated Codex permissions become fail-open for deeper matching files; omit the cap or make it opt-in instead of adding it automatically.
Useful? React with 👍 / 👎.
|
Status update: the PR is still mergeable and the full required matrix is green, including Code Quality & Tests, Build binaries, External Contributor Policy, and E2E on ubuntu, macOS, macOS Intel, and Windows. The diff remains scoped to Codex CLI filesystem rule emission and its tests. |
dyoshikawa-claw
left a comment
dyoshikawa-claw
left a comment
There was a problem hiding this comment.
This PR correctly routes relative file globs under :project_roots, which aligns with Codex CLI's expectation that top-level filesystem keys be absolute paths or named roots. The round-trip support on import is a nice touch.
I left a few line comments with specific suggestions. Nothing blocking — the implementation is solid, the tests cover the happy paths, and all CI is green.
|
|
||
| if (Object.keys(projectRootFilesystem).length > 0) { | ||
| if (Object.keys(projectRootFilesystem).some((pattern) => pattern.includes("**"))) { | ||
| filesystem.glob_scan_max_depth = CODEX_GLOB_SCAN_MAX_DEPTH; |
There was a problem hiding this comment.
The glob_scan_max_depth is only set when unbounded ** patterns exist — single-level globs like src/* do not trigger it. Could you add a comment explaining the reasoning? It would help future maintainers understand why the condition exists and whether it should ever change.
| /^[A-Za-z]:[\\/]/.test(pattern) || | ||
| pattern.startsWith("~/") || | ||
| pattern === "~" || | ||
| pattern.startsWith(":") |
There was a problem hiding this comment.
pattern.startsWith(":") matches any colon-prefixed string as a top-level key. While this correctly handles :project_roots and :home, it would also classify an arbitrary :something_else as a named root. If a user accidentally writes a colon-prefixed glob, it silently lands in the wrong table. Consider narrowing to known named roots, or logging a warning for unrecognized :-prefixed patterns.
|
|
||
| const content = rulesFile.getFileContent(); | ||
| expect(rulesFile.getRelativeDirPath()).toBe(".codex/rules"); | ||
| expect(rulesFile.getRelativeDirPath()).toBe(join(".codex", "rules")); |
There was a problem hiding this comment.
The change from ".codex/rules" to join(".codex", "rules") is a good cross-platform fix, but it is unrelated to the project-root glob feature. Consider separating such cleanup into its own commit in the future for a cleaner history.
dyoshikawa-claw
left a comment
dyoshikawa-claw
left a comment
There was a problem hiding this comment.
The implementation correctly routes relative file globs under :project_roots and handles round-trip import. All CI is green, tests cover the happy paths, and there are no blocking issues.
…-1619 fix(codexcli): address review findings from #1602
3 participants
Summary
:project_rootsnamed root instead of the top-level filesystem table.glob_scan_max_depth = 8when project-root rules contain unbounded**patterns.Fixes #1597
Testing
npx pnpm@10.25.0 exec vitest run --silent=true src/features/permissions/codexcli-permissions.test.tsnpx pnpm@10.25.0 exec oxfmt --check src/features/permissions/codexcli-permissions.ts src/features/permissions/codexcli-permissions.test.ts src/e2e/e2e-permissions.spec.ts docs/reference/file-formats.md skills/rulesync/file-formats.mdnpx pnpm@10.25.0 exec oxlint src/features/permissions/codexcli-permissions.ts src/features/permissions/codexcli-permissions.test.ts src/e2e/e2e-permissions.spec.ts --max-warnings 0npx pnpm@10.25.0 run typecheckNote:
npx pnpm@10.25.0 exec vitest run --config vitest.e2e.config.ts --silent=false src/e2e/e2e-permissions.spec.tscurrently fails before assertions in this Windows environment because the E2E helper spawnsnode_modules/.bin/tsxwithout the Windows.cmdshim.