fix(hooks): keep rtk init --codex inside the project and off the user's files

[KuSh]

Fixes the third blocker from the release review on #3979, plus the symlink regression beside it. Both come from #3552, which added rtk init --codex.

Part of a set of six, one per originating PR: #3681, #3552 (this), #3265, #3772, #3857, #3941.

Blocker 2 — init --codex --uninstall deletes an unrelated RTK.md

--codex is the one init mode whose paths are the project root, where RTK.md is a name RTK does not own. Uninstall removed whatever file sat there — no content check, no backup, no prompt:

$ ls                          # a project with its own notes
AGENTS.md  RTK.md
$ cat RTK.md
my own notes about rtk
$ rtk init --codex --uninstall
RTK uninstalled for Codex CLI:
  - RTK.md: RTK.md            # gone

Install was as quiet, overwriting a user-authored RTK.md in place.

The file now says whose it is

Init writes an ownership line above the awareness payload and both sides read it, on the first non-blank line:

<!-- rtk-owned: written by `rtk init --codex`, removed by `rtk init --codex --uninstall` -->

A marker rather than a content comparison, because the payload changes between releases: checking against the running build's copy stops recognising RTK's own file the moment the wording moves on — orphaning it on uninstall and leaving a numbered .bak on every upgrade after.

Only the first non-blank line counts. RTK writes its claim at the top of a file it wrote whole, so a marker — or an RTK-written block — that a user pasted into the middle of their own notes does not hand the rest of the file over with it:

$ cat RTK.md
# My own notes about rtk
...
<!-- rtk-instructions v1 -->      # moved here from CLAUDE.md
...
$ rtk init --codex --uninstall    # before: the whole file was removed
Kept RTK.md: RTK.md (it carries no rtk-owned marker; remove it yourself if you want it gone)

Installs predating the marker are recognised too, so the change strands none of them:

• Up to v0.48.0 the payload opened with # RTK - Rust Token Killer (Codex CLI), which names both RTK and the mode and which nothing writes any more. That heading alone is enough.
• v0.49.0 and the builds after it wrote the shared awareness text, whose headings (# RTK, # Command output) name neither and which a user's own notes may legitimately open with. Keying on those would cost someone their file, so those payloads are matched whole instead, against frozen SHA-256 digests of the bytes that shipped. Frozen rather than compared against this build's copy: rewording the awareness text must not change which files uninstall recognises as RTK's own.

Carriage returns are dropped before hashing, so a checkout that rewrote the line endings still matches, and a leading BOM is ignored.

--global is a different question

In global scope RTK.md lives in $CODEX_HOME, a directory RTK created it in and nothing else claims the name in. Reading the marker there would strand every file written by a released rtk:

$ rtk init -g --codex --uninstall   # with the v0.48.0 payload in ~/.codex/RTK.md
Kept RTK.md: …/.codex/RTK.md (it carries no rtk-owned marker; remove it yourself if you want it gone)
$ rtk init -g --codex
             your existing RTK.md was saved to …/.codex/RTK.md.bak

The ownership test is now scoped: in the Codex home the file is RTK's unconditionally, at the project root only the marker or the frozen legacy heading settles it.

.codex/hooks.json followed symlinks out of the project

The project paths are relative names RTK joins itself, so a symlinked .codex sent the new hooks.json write somewhere the user never named:

$ ln -s /elsewhere .codex
$ rtk init --codex
  Hook:      .codex/hooks.json (registered)
$ ls /elsewhere
hooks.json

A planted hooks.json.bak did the same through fs::copy, which follows a symlink at the destination, even with .codex a perfectly ordinary directory — so the existing hooks file was carried out of the project.

Resolving one link was not enough, and a chain of two got through — a hole that is pre-existing, not introduced here: it passes on develop as well. With .codex/hooks.json.bak -> ../mid and mid -> <outside>/evil.json whose target does not exist yet, canonicalize gives up at the dangling end and reports the whole chain as sitting where it broke, so the middle hop looked like an ordinary in-project path:

$ rtk init --codex                # before
  Hook: .codex/hooks.json (registered)     # exit 0, no warning
$ cat <outside>/evil.json
{"hooks":{"PreToolUse":[{"hooks":[{"type":"command","command":"echo attacker"}]}]}}

Both paths are now resolved link by link, up to a bound that makes a cycle terminate, and refused when the end of the chain lands outside the project. The walk covers a link above the file, not only the file's own, since canonicalize cannot see past a dangling one wherever it sits, and .. after a component that does not exist yet is folded in rather than compared as written. A link standing where the file itself goes also has to sit inside the project rather than merely point there, because a write that cannot resolve its path replaces that link instead of following it; a link further up is always traversed, so it is judged by where it leads — whole directory trees hang off one on macOS, where /var is a symlink. The refusal names the real destination:

$ rtk init --codex                # after
rtk: .codex/hooks.json.bak resolves to <outside>/evil.json, outside the project at <project>.
Remove the symlink, or use --global to configure Codex outside the project.
$ echo $?
1

What is guaranteed, and what is not

The guarantee is about .codex/hooks.json and the hooks.json.bak written beside it: those two are required to resolve inside the project, and install writes nothing at all when they do not.

AGENTS.md and RTK.md are deliberately not guarded, and git stores symlinks, so a cloned repository can ship either pointing outside the clone. This is documented rather than blocked, because atomic_write following a user-maintained symlink is behaviour existing tests pin:

• AGENTS.md is followed and left in place, so the @RTK.md reference is appended to whatever it names. That line is inert text, where .codex/hooks.json registers a hook that runs shell commands — which is why the hook is the path that gets the guard.
• RTK.md pointing at a file RTK did not write has the link moved to RTK.md.bak; nothing outside is touched. Pointing at a file RTK did write, it is followed and rewritten in full.

The Codex section of docs/guide/getting-started/supported-agents.md states all three, so the behaviour is discoverable before someone installs into a repository they have not read.

Only the hook is given up when the check fails. Refusing the whole uninstall left RTK.md and the @RTK.md reference behind with no command that removes them — --global acts on ~/.codex, which is not where they are. Install still refuses outright rather than half-configure.

Not changed, deliberately

The third item in that group — "Codex hook asserts permissionDecision: \"allow\" unconditionally, the deny branch is unreachable" — is left alone. permissions::load_rules_for(Host::Codex) returns empty rule sets by design, so Deny and AllowRewrite are unreachable by construction, and hooks/codex/README.md documents that Codex requires the protocol-level allow for updatedInput and that its own approval and sandbox checks still run on the rewritten command. That is a documented design decision with its reasoning written down, not a regression.

Verification

Ownership: carried by the file rather than by the build (a payload from another release, an edited copy of RTK's own file, a leading BOM, CRLF endings, a reindented marker, a whitespace-only first line, the frozen legacy heading, each shipped payload, and an RTK block sitting inside the user's own notes, which stays the user's); a user-authored RTK.md surviving uninstall and being moved aside by install; numbered backups not reusing a slot; dry-run moving nothing; a file RTK cannot read left alone; the Codex home owning its RTK.md with no marker of any kind.

Containment, on Unix: a symlinked .codex, a symlinked hooks.json.bak, a two-link chain, a dangling symlinked ancestor, a target climbing out with .., a link that sits outside while pointing back in, an absolute target travelling through a symlinked ancestor back into the project, an in-project chain that must still be accepted, and a cycle that must terminate.

The guard's call sites are covered by driving the commands, not the helper: project-scope install and uninstall over a real symlinked .codex and over a planted hooks.json.bak, and over a user-authored RTK.md. Every one of these was checked by mutation — deleting or inverting the code it covers makes that test, and only that test, fail.

AGENTS.md and RTK.md still resolve through a user-maintained symlink, and uninstall still cleans the project when the hook path is given up, saying on stdout that it did not check the hook rather than reporting a clean uninstall.

Three existing fixtures that wrote a placeholder as RTK.md now write the real marked payload, since their intent was "uninstall removes RTK's own file". The Codex section of docs/guide/getting-started/supported-agents.md documents the ownership rules and the containment refusal.

cargo fmt --all, cargo clippy --all-targets and cargo test --all are green, rebased on current develop.

🤖 Generated with Claude Code

[rtk-wshm-sync-bot]

wshm · Automated triage by AI

📊 Automated PR Analysis

🐛 Type	bug-fix
🟡 Risk	medium

Summary

Fixes two bugs in rtk init --codex: uninstall could silently delete a user-authored RTK.md and install could overwrite one without backup, since ownership wasn't tracked; also fixes a symlink vulnerability where .codex/hooks.json (and its .bak) could resolve outside the project via symlinked directories or fs::copy's destination-following behavior. Adds an ownership marker written into RTK-authored RTK.md files, backup-on-conflict logic, and path-containment checks before writing/re…

Review Checklist

• Tests present
• Breaking change
• Docs updated

---

Analyzed automatically by wshm · This is an automated analysis, not a human review.

[pszymkowiak]

Reviewed by building the branch and running both the develop (0924356b) and PR binaries in throwaway directories with HOME/CODEX_HOME isolated. The two reported regressions are fixed and well tested, but two gaps were reproduced independently three times, so requesting changes.

Verified fixed

• Uninstall on a user-authored RTK.md: develop deletes it; this PR keeps it with "Kept RTK.md … remove it yourself".
• Install over a user-authored RTK.md: develop overwrites silently; this PR moves it to RTK.md.bak and writes the marked payload. Install → uninstall round trip removes only the rtk-owned file. Second install creates no extra .bak.
• Single-level symlink escapes (.codex -> /elsewhere, .codex/hooks.json.bak -> /elsewhere/x): refused on install with exit 1, hook step skipped on uninstall while RTK.md and the AGENTS.md reference are still cleaned.

1. A two-link chain still writes outside the project (ensure_inside_root, src/hooks/init.rs:4197-4222)

The guard calls fs::read_link once. If .codex/hooks.json.bak -> ../mid and mid -> <outside>/evil.json where evil.json does not exist yet, canonicalize fails on the dangling target, canonicalize_path_for_comparison falls back to the deepest existing ancestor plus the literal tail, which is <project>/mid, and starts_with(root) passes. fs::copy in backup_and_atomic_write (:706-716) then follows both links:

$ mkdir .codex
$ echo '{"hooks":{"PreToolUse":[{"hooks":[{"type":"command","command":"echo attacker"}]}]}}' > .codex/hooks.json
$ ln -s ../mid .codex/hooks.json.bak
$ ln -s $OUT/evil.json mid          # $OUT exists, evil.json does not
$ rtk init --codex
  Hook: .codex/hooks.json (registered)      # exit 0, no warning
$ cat $OUT/evil.json
{"hooks":{"PreToolUse":[{"hooks":[{"type":"command","command":"echo attacker"}]}]}}

Variants checked: final target already existing → refused; single link to a dangling outside path → refused; middle link outside the project → refused. Only the in-project dangling middle hop gets through. The same chain also passes on develop, so this is not introduced here, but the description says the paths "are now required to resolve inside the project" and this is the same primitive as the case that was fixed. The realistic scenario is a cloned repo shipping .codex/hooks.json plus the two links pointing at ~/.codex/hooks.json on a machine where it does not exist yet: a Codex hook that runs shell commands, written by rtk init.

Suggested fix, either is enough: resolve the leaf with read_link in a bounded loop (re-anchoring relative hops) and check every hop, or never let the backup copy follow a destination link (remove_file any existing hooks.json.bak symlink before fs::copy, or write the backup through the same tempfile + rename path as atomic_write). A test with this exact chain would pin it.

Related, lower priority: AGENTS.md is deliberately not guarded (:2803-2805), so AGENTS.md -> <outside file> still gets @RTK.md appended outside the project. Worth a line in the description at least.

2. The ownership check in --global mode orphans every RTK.md written by a released rtk (:1273-1282, :2877)

rtk init -g --codex has shipped since v0.33.0, and v0.46.0 through v0.48.0 write hooks/codex/rtk-awareness.md to ~/.codex/RTK.md with no marker (# RTK - Rust Token Killer (Codex CLI) first line); develop writes the unmarked # Command output payload. The marker check runs in both scopes, nothing branches on global. With the exact v0.48.0 payload in $CODEX_HOME/RTK.md:

$ rtk init --codex --global --uninstall
  Kept RTK.md: …/.codex/RTK.md (it carries no rtk-owned marker; remove it yourself if you want it gone)
$ rtk init --codex --global
  your existing RTK.md was saved to …/.codex/RTK.md.bak

develop removes the file on uninstall. The leftover is inert once the AGENTS.md reference is stripped and the .bak is created only once, so the impact is a stray file and a misleading message, but it is the exact "orphaned on uninstall" outcome the description gives as the reason for choosing a marker. Inside ~/.codex/ the name is unambiguous: if global || rtk_md_is_rtk_authored(path) fixes it, or recognise the two legacy first lines as rtk-authored (that would also cover project-mode users on develop).

Minor

• is_rtk_authored_md (:230-232) also accepts the rtk-instructions marker, but no release ever wrote that block into a file named RTK.md (checked v0.30 → v0.48); a user who moved it there from CLAUDE.md next to their own notes loses the whole file on uninstall. Restricting to rtk-owned on the first non-blank line would be tighter.

Everything else is in good shape: free_backup_path works on the OsString, uninstall degrades to skipping only the hook, fixtures now carry the real marked payload, CI green on all three OSes.

[KuSh]

Thanks — all three are fixed, and your reproductions were exact in every case. The head has moved since your review, from f4ad97d8 to 5ac8af08, and the containment primitive is not the one you read: PR #3875 needs the same symlink resolver, so this PR carries a shared one and #3875 builds on it. first_symlink_component and MAX_CONTAINMENT_LINK_HOPS no longer exist. All 11 checks green.

1. Two-link chain. Two rules, deliberately separate.

ensure_chain_sits_inside walks the write site's own chain link by link and requires every link on it to sit inside the project, not merely to point back in. A link in a directory the project does not own is a place the write can land and a place someone else can re-aim, so pointing it inward is not enough. Ancestor components are traversed and never sited — whole directory trees hang off one symlink on macOS, so siting those would refuse every absolute target under /var.

Resolution is the shared resolver. It separates three facts a bare Option conflates: not a symlink, a link that cannot be read, and a chain it gave up on. The middle one is load-bearing — lstat says symlink, readlink fails, and reporting that as "not a symlink" hands back a path nothing resolved, which then compares as though it had. The hop bound is inclusive, so a chain of exactly the documented length resolves rather than being refused.

Your exact layout against 5ac8af08:

develop:  exit 0, no warning, {"hooks":…"echo attacker"…} written to <outside>/evil.json
this PR:  exit 1, "…/hooks.json.bak resolves to <outside>/evil.json, outside the project at …", 0 files outside

Of your two suggested fixes I took the guard rather than unlinking the .bak before fs::copy: it closes the primitive for both guarded paths rather than one, and fs::remove_file in production trips the blocking semgrep filesystem-deletion rule. The commit adds no fs::remove_*.

Your chain is pinned by test_a_symlink_chain_is_followed_past_its_first_hop, the middle-link variant by test_a_link_partway_along_the_chain_may_not_sit_outside, and the macOS shape by test_an_absolute_target_may_travel_through_a_symlinked_ancestor.

2. --global orphaning. You were right, and it was worse than the v0.48.0 boundary suggested: v0.49.0 is a released tag that is not an ancestor of develop, so Cargo.toml at HEAD reporting 0.48.0 gives the wrong answer about what is deployed. It writes RTK_AWARENESS_FULL, first line # RTK, with no marker — as does every develop build since.

Fixed two ways rather than one. RtkMdScope (CodexHome / ProjectRoot) makes $CODEX_HOME own its RTK.md outright, since nothing else claims that name there — your if global || rtk_md_is_rtk_authored(path). And RTK_MD_UNMARKED_PAYLOAD_DIGESTS holds frozen SHA-256 values of the payloads releases actually shipped, so a project-root upgrade is recognised too. Digests rather than the live constants deliberately: rewording the awareness text must not silently change recognition. CR is stripped before hashing so a CRLF checkout still matches.

v0.49.0 payload at the project root:
  install    -> 0 .bak files          (was: a 1121-byte .bak, and a message claiming it was yours)
  uninstall  -> removed cleanly       (was: kept, "carries no rtk-owned marker")
user notes opening "# RTK" or "# Command output" -> still survive uninstall

The bare headings are not whitelisted on their own: a user's notes may legitimately open with either, and taking their file is the worse error.

Minor. is_rtk_authored_md matches only the first non-blank line, and only rtk-owned or the frozen legacy heading; there is no rtk-instructions arm, and a UTF-8 BOM is stripped first. Your scenario verified at project scope: notes containing an rtk-instructions block are deleted by develop and survive here. Note the interaction with fix 2 — at --global scope $CODEX_HOME/RTK.md is owned unconditionally and is removed, which is what finding 2 asks for.

On your two related notes, both confirmed and both filed rather than folded in, since they are pre-existing on develop and not introduced here:

• AGENTS.md as a relative symlink escaping a clone — reproduced, including the mode reset 644 → 600 from NamedTempFile::persist replacing the inode. rtk init --codex writes through a symlinked AGENTS.md, so a cloned repo can redirect the write outside the project #4156. The description states the three cases separately: AGENTS.md followed and appended to; a symlinked RTK.md pointing at a user file has the link moved to .bak; one pointing at an RTK-written file is followed and rewritten.
• The TOCTOU residue between the check and the copy — fs::copy onto a .bak path follows a symlink there, so a cloned repo can redirect rtk init writes outside the project #4157, written against the deterministic case, which needs no race and reproduces on develop today.

Every fix is pinned by a test verified to fail when that fix alone is reverted.

[pszymkowiak]

Re-verified on 5ac8af08 (now on develop 727ee6e6), same method as round 1: built the head, reproduced against the develop binary, three independent passes. Local gate: fmt and clippy clean, 3920 tests pass.

Finding 1, containment: fixed. The round-1 layout exits 1 with nothing written outside; develop still escapes. Tried to get past the new resolver with a 3-hop chain, a chain at exactly the 16-hop bound and one past it, a middle link through an in-project directory symlinked outside, .codex itself pointing at an existing and at a dangling outside path, hooks.json itself as a dangling chain, an unreadable link mid-chain, and a .. chain that leaves and re-enters. All refused. A normal install under /tmp (symlinked ancestor on macOS) still succeeds. Only residue is the in-project .bak -> ../AGENTS.md clobber, which is #4157.

Finding 2, ownership: fixed. Rebuilt the ground truth from the tags: v0.31.0 through v0.48.0 share one byte-identical payload (recognised by the frozen heading), v0.49.0 and develop write the three awareness payloads whose digests are in the list. Upgrade from v0.48.0, v0.49.0 and develop at both scopes, plus CRLF and BOM variants: uninstall removes, install writes no .bak. User notes opening with # RTK or carrying an rtk-instructions block survive uninstall (develop deleted them). Residue: a v0.49.0 payload the user appended to, or whose trailing newline an editor stripped, is treated as the user's, which is the safe direction.

Follow-ups, none blocking

1. uninstall_codex (init.rs:1341-1367): when the hook path fails the guard, uninstall cleans RTK.md/AGENTS.md, prints "Not checked: the Codex hook … may still be registered", and returns Ok(()), so rtk init --codex --uninstall && … cannot tell the hook survived. Cleaning what you can is right; exiting non-zero afterwards would be too. Your call.
2. The MAX_BACKUP_ATTEMPTS comment ("the two callers had before they shared this") and the refactor title ("both init paths") refer to the Pi/OMP caller, which is not on this branch; at head there is one caller of the resolver and one of the slot picker.
3. test_rtk_md_ownership_is_carried_by_the_file_not_by_the_build will fail with a bare assertion on the next awareness-text edit; the remedy lives only in a comment two lines up. A message saying "drop these asserts, do not add a digest" would save the next editor a detour.
4. backup_path_for (unnumbered, overwritten every run, follows a destination link) and numbered_backup_path now coexist; the seven JSON writers still use the former. That is where #4157 lives.

Approving. Both fixes are pinned by tests that discriminate the fixed behaviour (the chain test asserts the canonical final target, which canonicalize cannot produce; the refusal tests assert no file was created), and no non-codex init mode reaches the new helpers.

[KuSh]

Thanks for the second pass — the adversarial set in particular (3-hop, the bound and one past it, .codex itself dangling, an unreadable link mid-chain, the .. chain that leaves and re-enters) covered shapes I had not tried.

All four follow-ups are placed. Two landed in the PR before merge, two are tracked:

2 and 3 — fixed in f6e41c94, merged as 0c279f54.

The MAX_BACKUP_ATTEMPTS comment and the commit title both described a caller that was not on the branch; the comment now states the ceiling on its own terms and the title is "make symlink resolution total and backup slots content-aware". The remedy for test_rtk_md_ownership_is_carried_by_the_file_not_by_the_build moved out of the comment and into the assertion message, so a rewording now fails with "the awareness text was reworded: delete these two assertions rather than adding a digest…" instead of a bare line number. Verified by editing hooks/rtk-awareness.md and watching it print.

4 — #4157, which I widened from the single function to the pattern: choose a .bak path, then fs::copy onto it.

Checking it against the Pi/OMP work turned up a second site with the same escape, and a detail worth recording: a slot picker that probes content does not close it. fs::read returns NotFound for both "nothing is here" and "a dangling link to nothing is here", and the second is the case that escapes.

case                   symlink_metadata.is_symlink   fs::read
absent file            false                         Err(NotFound)
dangling link          true                          Err(NotFound)     <-- reported free, escapes
link to existing       true                          Ok(25 bytes)      <-- skipped, no escape
ordinary file          false                         Ok(6 bytes)       <-- skipped, no escape

The control is in the issue too, because it narrows the fix: a link to an existing readable file is skipped, so the probe closes three of four cases and only the dangling one slips. fs::symlink_metadata separates rows 1 and 2, which fixes the decision rather than the copy and so holds whether a caller copies or renames.

You had already noted this PR is unaffected because its backup moves rather than copies. That is now written down as an invariant — fs::rename replacing a destination symlink instead of following it is load-bearing for containment, and nothing in for_rename's name or signature said so.

1 — #4164. Filed rather than folded into #4157, since that one is pre-existing on develop and is a traversal, while this is a state the containment guard introduced: a refusal reported as success. Reproduced at the merged head — rtk init --codex --uninstall && echo done prints done with the hook still registered.

I left the exit code itself open in the issue rather than picking one. A non-zero exit from --uninstall may break teardown scripts that pass today, and "uninstalled what I could reach" is genuinely different from "failed", so a dedicated code may serve better than 1. Cleaning the rest is not in question — refusing to touch RTK.md and AGENTS.md because the hook path is unreachable would leave more behind, not less.