Skip to content

fix(paths): reject every path anchor in repo-relative guards, not just absolute ones - #65

Open
Hotragn wants to merge 1 commit into
AlmanacCode:mainfrom
Hotragn:fix/repo-relative-path-guards
Open

fix(paths): reject every path anchor in repo-relative guards, not just absolute ones#65
Hotragn wants to merge 1 commit into
AlmanacCode:mainfrom
Hotragn:fix/repo-relative-path-guards

Conversation

@Hotragn

@Hotragn Hotragn commented Aug 19, 2026

Copy link
Copy Markdown

Summary

  • is_absolute() was used as the "must be repo-relative" containment guard in two places. It does not hold on Windows.
  • Both guards now share core.paths.is_rooted, which rejects any path anchor (absolute, rooted-without-drive, or drive-without-root).
  • Restores an existing assertion in test_filesystem_source_runtime.py that fails on Windows today.

Why

On Windows, PureWindowsPath reports two anchored forms as relative:

input is_absolute() drive root
/tmp/wiki False '' '\'
C:wiki False 'C:' ''

Both still discard the left-hand side when joined, which is exactly what a containment guard exists to prevent:

PureWindowsPath("C:/repo") / "/tmp/wiki"   # -> C:/tmp/wiki
PureWindowsPath("D:/repo") / "C:wiki"      # -> C:wiki

Two guards relied on that check, and both make an explicit promise in their own error message:

  • SourceRuntimeContext.ignored_directories"source runtime ignored directories must be repo-relative" (services/sources/requests.py)
  • require_default_almanac_root"Almanac root must be a repo-relative path" (services/repositories/roots.py)

This is not a hypothetical: tests/test_filesystem_source_runtime.py::test_source_runtime_context_rejects_unsafe_ignored_directories[directory0] already asserts that /tmp/wiki is rejected, and that assertion fails on Windows. The repo already decided this input is unsafe; the guard just does not enforce it off POSIX.

It matters most for ignored_directories, since source runtime context is populated from wiki page frontmatter that agents author, so the value is not necessarily developer-typed. require_default_almanac_root is hardened for consistency — its later != almanac check already rejected these inputs, so that half is defence in depth rather than a live escape.

Verification

# Windows 11, Python 3.13 (uv-managed), uv 0.12.0
uv run pytest tests/test_core_paths.py tests/test_filesystem_source_runtime.py \
              tests/test_repository_store.py tests/test_sources_service.py -q
# 44 passed, 1 skipped

uv run pytest -q          # 12 failed, 568 passed, 1 skipped
# baseline on main:         13 failed, 550 passed, 1 skipped
# (+18 new parametrized cases, and 1 pre-existing failure fixed)
uv run ruff check .       # All checks passed!
git diff --check          # clean
uv build --out-dir dist   # ok

Confirmed the tests pin the bug rather than describe it — with the predicate reverted to is_absolute() only, these fail:

FAILED tests/test_core_paths.py::test_windows_anchored_paths_are_rooted[/tmp/wiki]
FAILED tests/test_core_paths.py::test_windows_anchored_paths_are_rooted[C:wiki]
FAILED tests/test_core_paths.py::test_source_runtime_context_rejects_rooted_ignored_directory
FAILED tests/test_filesystem_source_runtime.py::test_source_runtime_context_rejects_unsafe_ignored_directories[directory0]

The remaining 12 failures are pre-existing and unrelated (macOS launchd tests plus non-portable fixtures).

Docs and wiki

  • Not applicable — tightens an existing guard to match its stated contract; no new user-facing behaviour.

Notes for reviewers

  • Zero behaviour change on macOS and Linux. On POSIX a root implies an absolute path and drive is always '', so is_rooted is exactly equivalent to is_absolute() there. The predicate only widens on Windows.
  • Shared, not duplicated. Rather than patching each call site, the check lives once in core/paths.py beside normalize_path, which both services already import. Two call sites with the same subtle rule is how the next one drifts.
  • The tests avoid the platform-native Path on purpose. Path("C:wiki") is a legitimate relative filename on POSIX and an anchored path on Windows, so a test written with Path can only ever check the host it runs on. Using explicit PureWindowsPath/PurePosixPath pins both platforms' semantics, which means this fix is fully verifiable on the current Linux-only CI — no Windows runner needed to review it.
  • Deliberately out of scope: resolve_user_path (services/sources/address_path.py) and transcript_path (integrations/sources/transcripts/paths.py) use the inverse check, if not path.is_absolute(): path = cwd / path. Those have a related Windows wart — /tmp/x takes the "relative" branch and then resolves drive-relative instead of under cwd — but they are a resolution concern rather than a containment guard, and both are meant to accept absolute input. Happy to send that as its own PR if you want it fixed; I left it out to keep this one reviewable.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…t absolute ones

`is_absolute()` is not a containment check on Windows. `PureWindowsPath`
reports "/tmp/wiki" as relative because it carries no drive, and "C:wiki" as
relative because it carries no root. Both still discard the left-hand side when
joined, so a guard built on `is_absolute()` accepts inputs that escape the repo:

    PureWindowsPath("C:/repo") / "/tmp/wiki"  ->  C:/tmp/wiki
    PureWindowsPath("D:/repo") / "C:wiki"     ->  C:wiki

Two guards depended on that check:

- `SourceRuntimeContext.ignored_directories`, which promises
  "source runtime ignored directories must be repo-relative"
- `require_default_almanac_root`, which promises
  "Almanac root must be a repo-relative path"

`tests/test_filesystem_source_runtime.py` already asserted that "/tmp/wiki" is
rejected, and that assertion failed on Windows. This restores it.

Both guards now share `core.paths.is_rooted`, which rejects any anchor. On POSIX
a root implies an absolute path and `drive` is always empty, so the predicate is
exactly equivalent to the old check and macOS/Linux behaviour is unchanged.

The new tests use explicit `PureWindowsPath`/`PurePosixPath` inputs rather than
the platform-native `Path`, so both platforms' semantics are pinned on any
runner and this stays verifiable on the current Linux-only CI.
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