Skip to content

Load every project doc and confine its @imports to the project root - #13

Closed
thedancingdeveloper wants to merge 7 commits into
fork/mainfrom
fork/instructions-directory
Closed

thedancingdeveloper wants to merge 7 commits into
fork/mainfrom
fork/instructions-directory

Conversation

@thedancingdeveloper

Copy link
Copy Markdown

Summary

  • project_doc_mode = "all" keeps every matching filename per directory instead of the first, then .claude/rules/*.md in sorted order, and expands line-only @path imports. Unset or anything else stays on the upstream first-match behaviour.
  • Imports and rules are read through the executor filesystem and refused once they canonicalize outside the project root: absolute path, ~/, a .. climb and a symlink escape each have a test.
  • The untrusted-project skip still runs before any of this is read.

Depends on #12 (WI-1131), whose three commits are in this branch. WI-1132. Do not merge; the epic lead reviews.

Test plan

  • cargo test -p codex-core --offline --lib agents_md_compat — 3 passed (every doc plus sorted rules, in-project import expanded, the four escapes refused)
  • cargo clippy -p codex-core --offline --lib -- -D warnings
  • cargo test -p codex-core --offline --lib agents_md — the existing first-match suite, not yet rerun on this branch

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: no blockers in WI-1132's own commits (head 2f98c50; reviewed 76658cb + 2f98c50 on top of #12's 93f35c0).

Security, checked against the requirements on WI-1132:

  • Untrusted projects: load_project_instructions still returns early for an untrusted project (agents_md.rs:64), before any project_doc_all path.
  • The root can't be widened by the repo: project_root merges markers only from layers above the project layer. With no marker it falls back to cwd, which is stricter.
  • Imports: resolve_import refuses ~, absolute and scheme (:) references outright. Every doc, import and .claude/rules dir is canonicalized through the executor filesystem (so the sandbox applies) and must satisfy canonical.starts_with(root). PathUri::starts_with is segment-wise (native_path_segments_start_with), so /proj2 can't pass as /proj.
  • Tests: a symlink escape, ../secret.md, /etc/hostname and ~/secret.md are all refused, and in-root imports are expanded.
  • project_doc_mode defaults to first (upstream behaviour). all loads every matching file plus sorted .claude/rules.

Non-blocking: root isn't canonicalized before the comparison. If the project path itself goes through a symlink, legitimate docs will be refused, which fails closed but is still a functional bug. Canonicalize root once at the top.

Merge order: this PR carries #12's three commits, based on #12's older head 93f35c0. Merge #12 first, then rebase this onto fork/main so the PR shows only its two commits, and fork-ci green on that head. If the clippy gate flags upstream debt in agents_md.rs, the line-based gate follow-up (WI-1123) is the fix, not an edit here.

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: no blockers in WI-1132's commits (head d8bcd12)

It's now stacked on #12's corrected head f9883a5, which is on fork/main feaf20e. Its own two commits (ccb7a04, d8bcd12) are identical (=) to the reviewed 76658cb and 2f98c50, so the security review stands. Merge #12 first. After that this PR shows only its two commits, and the merge condition is fork-ci green on the head at that point.

@thedancingdeveloper
thedancingdeveloper force-pushed the fork/instructions-directory branch from d8bcd12 to 981a252 Compare October 9, 2026 19:10

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: no blockers (head 981a252)

This is a rebase onto fork/main ef97503, which includes #17's workspace bin build. git range-diff against the previously reviewed head shows every commit identical (=): its 2 commits on top of #12's 11aa919; merge after #12. Merge condition: fork-ci green on this head.

When claude_compat_instructions is set, the global user-instructions
snapshot also includes ~/.claude/CLAUDE.md and ~/.claude/rules/*.md in
sorted order. Line-only @path imports expand to depth 5, are deduped by
path, symlink and body, and count against project_doc_max_bytes. The
switch defaults off, so upstream behaviour is unchanged.

WI-1131
expand_imports and expand_imports_all take allowed_roots. None leaves the
global user-instructions path unrestricted; a caller that passes project
roots gets every file canonicalized and refused, with a warning, when it
falls outside them. A symlink or .. climb is caught because the check runs
after canonicalization.

WI-1131
The fork field is required on Config, and the sample builds a Config
literal by hand. Default it off so the workspace check compiles.

WI-1131
cargo fmt -p codex-home. No behaviour change.

WI-1131
The let binding existed only to be returned, which clippy's let_and_return
lint flags, and the gate lints every file this PR touches.

WI-1131
`project_doc_mode = "all"` keeps every matching filename in a directory
instead of the first, then `.claude/rules/*.md` in sorted order, and
expands line-only @path imports. Anything else, including an unset value,
keeps the upstream first-match behaviour.

Imports and rules are read through the executor filesystem and refused
once they canonicalize outside the project root, so a repo cannot pull in
an absolute path, a ~/ reference, a .. climb or a symlink that escapes.
The untrusted-project skip still runs first, so none of this is read for
an untrusted project.

WI-1132
cargo fmt -p codex-core. No behaviour change.

WI-1132
@thedancingdeveloper

Copy link
Copy Markdown
Author

Superseded: these commits were cherry-picked into #22 (fork features, merged to fork/main as c2d74d2) and ship in release fork-c2d74d2d9. Closing so the work isn't merged twice. The branch is kept.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant