Resolve Symlinks Before Accepting a Canonical File

[ptr727]

CodeRabbit finding on the develop -> main promotion PR (#978), against
spec/validate.py's new is_file() checks from PR #977: a tracked
in-repo symlink whose target escapes ROOT passes both
escapes_repo_root() (a lexical string check) and a bare
Path.is_file() (which follows the link) the same way a real file
would. The audit engine would then read that external target's live
filesystem content while dating it from the symlink's own git history.

Add canonical_file_in_root(), which resolves the candidate strictly
and requires the result to both exist as a file and stay under ROOT,
in place of the two is_file() calls.

Verified by hand: symlinked a path inside this checkout to a file
outside it, confirmed the new check rejects it while still accepting
a real hub file and still rejecting a missing path and a directory,
removed the symlink after. --selftest, ruff, mypy,
prose_lint.py, repo_gate.py, and the scripts/tests/ pytest suite
(838 passed) all clean.

Summary by CodeRabbit

• Bug Fixes
  • Improved validation of referenced files to ensure paths resolve to existing files within the repository.
  • Added protection against references that use symlinks or path variations to escape the repository boundary.
  • Updated reference and intentRef validation for more accurate and reliable results.
  • Invalid or out-of-bound file references are now rejected consistently.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 76ad1d10-9387-4de5-8921-d14f67d40d5e

📥 Commits

Reviewing files that changed from the base of the PR and between d0a11ea and eecc19e.

📒 Files selected for processing (1)

• spec/validate.py

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

---

📝 Walkthrough

Walkthrough

The change adds canonical, symlink-aware path validation. Intent references must resolve to existing files within the repository root.

Changes

Intent reference validation

Layer / File(s)	Summary
Canonical path validation and integration spec/validate.py	Adds canonical_file_in_root() and uses it for both reference and intentRef validation. Symlinks that resolve outside ROOT and unresolved paths now fail validation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to eecc1

The change tightens canonical-file validation for symlinks without introducing an identified merge-blocking issue; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 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: resolving symlinks before accepting canonical files.
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 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-canonical-symlink-escape-978

---

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Resolve symlinks before accepting canonical file paths in validation

🐞 Bug fix🕐 10-20 Minutes

AI Description

• Prevent repo-tracked symlinks from referencing files outside the repo root.
• Add a strict canonicalization check and use it for files.json intent path validation.
• Ensure audit-time reads match what validation intended to verify.

Diagram

graph TD
A["spec/files.json"] --> B["spec/validate.py"] --> C["escapes_repo_root()"] --> D["canonical_file_in_root()"] --> F[("Repo-root file") ] --> E["Accept / error"]
D -. "reject if resolves outside ROOT" .-> G[("External file")]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Reject all symlinks (lstat-based)

• ➕ Eliminates all symlink-related ambiguity and escape vectors.
• ➕ Simpler mental model: only regular files allowed.
• ➖ Breaks legitimate in-repo symlink use cases (if any exist).
• ➖ Harder migration if repos currently rely on symlinks for shared content.

2. Repo-aware canonicalization via Git plumbing

• ➕ Can validate against the index/tree rather than the working filesystem.
• ➕ Avoids mismatches between tracked content and live filesystem state.
• ➖ More complex and slower; requires invoking git and handling edge cases.
• ➖ Overkill for a validation script that primarily checks the checkout.

3. Lexical check + realpath prefix (no strict resolve)

• ➕ Avoids raising on missing targets and can provide finer-grained errors.
• ➕ Potentially fewer filesystem calls.
• ➖ Easy to get wrong across platforms (case sensitivity, path normalization).
• ➖ Less robust than resolve(strict=True) + is_relative_to(ROOT).

Recommendation: The PR’s approach (strict resolve + in-root enforcement) is the best balance: it fixes the symlink escape without banning symlinks outright and uses standard pathlib primitives (resolve(strict=True) and is_relative_to(ROOT)) to avoid subtle cross-platform path bugs.

Files changed (1) +16 / -2

Bug fix (1) +16 / -2

validate.pyAdd strict canonical in-root file check to block symlink escapes+16/-2

Add strict canonical in-root file check to block symlink escapes

• Introduces 'canonical_file_in_root()' to resolve candidate paths strictly and require that the resolved target is an existing file under 'ROOT'. Replaces two direct 'Path.is_file()' checks for 'reference' and 'intentRef' validation to prevent in-repo symlinks from pointing outside the checkout.

spec/validate.py

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0)📘 Rule violations (0)📜 Skill insights (0)

1. canonical_file_in_root() docstring too long✓ Resolved📜 Skill insight⚙ Maintainability

Description

The new canonical_file_in_root() docstring is a multi-line prose block with wrapped sentences and
extended rationale/implementation context. This violates the repository comment/docstring style
rules and makes the function contract harder to read and maintain.

Code

spec/validate.py[R75-78]

+ """Whether `ROOT / rel_path` resolves, symlinks followed, to an existing file that stays+ under ROOT. `escapes_repo_root()` only reads the lexical string, so a tracked symlink whose+ target escapes ROOT would otherwise pass it and then `Path.is_file()` too, since both follow+ the link. The audit engine would then read that external target's live content while dating

Relevance

●●● Strong

Team consistently accepts splitting overlong prose docstrings/comments per style rules (PR #901,
#921, #910).

PR-#901
PR-#910
PR-#921

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The added docstring for canonical_file_in_root() spans multiple wrapped lines and contains
multiple sentences of background/rationale (including references to the audit engine and prior
checks), which violates the comment length/structure rules and the docstring behavior-contract focus
requirement.

spec/validate.py[74-80]
Skill: comment-and-doc-style
Skill: python-codestyle

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
`canonical_file_in_root()` adds a long, wrapped, multi-sentence docstring that includes extended rationale/implementation context.
## Issue Context
Repo style requires comments/docstrings to be concise, one sentence per line, and to describe the behavior contract rather than extended background.
## Fix Focus Areas
- spec/validate.py[74-80]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 70 rules

✅ Skills: 5 invoked

comment-and-doc-style

dotnet-codestyle

python-codestyle

shell-codestyle

workflow-ci-contract

✅ Web pages:

🌐 pathlib.Path.resolve() mishandles symlink loops

🌐 pathlib — Object-oriented filesystem paths — Python 3.14.7 documentation

🌐 bpo-43757: make pathlib use os.path.realpath() to resolve all symlinks in a path · Pull Request #25264 · python/cpython

+2 more

Review mode: ⚖️ Balanced

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗