test(context_engine): skip the chmod attachment case when the runner is root

[Handsome-wzw]

Summary

test_an_attachment_that_cannot_be_read_costs_a_note_not_the_turn makes a file
unreadable with chmod 000 and asserts the renderer notes the failed attachment
instead of failing the whole turn. chmod 000 does not block a root user, so
the file stays readable, the error path is never entered, and the case fails for
anyone running the suite as root. It passes in CI, which does not run as root,
so the failure is invisible there and shows up only on a developer's machine.

Skipped for root rather than reworked: the behaviour under test is what happens
when a read raises, and making that happen without permissions means faking the
error, which would test the mock. The case is worth keeping for the runs that
can exercise it honestly.

The commit is 0xKT's, cherry-picked from the GitLab trunk where it already
landed. Authorship is preserved.

Type

• Fix
• Feature
• Docs
• CI / tooling
• Refactor
• Other

Test-only.

Verification

uv run pytest tests/test_read_file_image.py    105 passed, 1 skipped

Before this, the same command as root gave 1 failed, 105 passed, with the
failure on this case. The skip reason names the cause, so a reader is not left
guessing why it is absent.

• Relevant tests pass locally
• Relevant lint / type checks pass locally
• User-facing docs or screenshots are updated when needed

Risk

None to the product: no source file is touched. Non-root runs, CI included,
execute the case exactly as before.

• Security impact considered
• Backward compatibility considered
• Rollback path is clear for risky changes

Related Issues

N/A

[0xKT]

@claude review

[claude]

Claude finished @0xKT's task in 2m 19s —— View job

---

Review: test(context_engine): skip the chmod attachment case when the runner is root

Verdict: looks good, no blocking issues.

What the diff does

tests/test_read_file_image.py:

• adds import os at module scope (line 12)
• adds @pytest.mark.skipif(os.geteuid() == 0, reason="chmod 000 does not block root") above test_an_attachment_that_cannot_be_read_costs_a_note_not_the_turn (line 1619)
• drops the now-redundant local import os inside the test body

Checks performed

• Correctness of the guard: chmod 0o000 (line 1629) genuinely does not block reads for euid == 0 (root ignores DAC permission bits), so the read succeeds, render.build_user_content returns content blocks instead of the "could not be read" string, and the assertions on lines 1635-1637 would fail under root. The skipif correctly prevents that false failure without weakening the assertions for the normal (non-root) path.
• Consistency: the guard's predicate and reason string are copied verbatim from the existing precedent in tests/test_importer_hermes_skills.py:71 and :517 (os.geteuid() == 0, "chmod 000 does not block root"), so this follows an established repo pattern rather than inventing a new one.
• No leftover duplicate import: confirmed only one import os remains in the file (module scope), no shadowing.
• Scope not silently narrowed: the finally: os.chmod(locked, 0o644) cleanup and the rest of the test body are untouched — only the skip condition and import location changed.
• AGENTS.md conventions:
  • branch name test/skip_chmod_case_as_root matches <type>/<snake_desc> (§2.1)
  • commit header test(context_engine): skip the chmod attachment case when the runner is root — valid type, context_engine is a real top-level raven/ subpackage, lowercase start, no trailing period, 76/100 chars (§3.1)
  • commit body is English, ASCII-only (verified via grep -P "[^\x00-\x7F]", no hits) (§3.1.1)
  • Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com> trailer present (§3.3)
  • test file naming (tests/test_read_file_image.py) is an existing file, unchanged filename, consistent with §5.1

Minor observations (non-blocking)

• The PR body says this is cherry-picked from a GitLab trunk commit authored by 0xKT, with authorship preserved — consistent with the commit's Author: field (0xKT <74288668+0xKT@users.noreply.github.com>).
• Could not execute uv run pytest in this sandbox (Bash tool calls requiring approval were denied in this session), so I verified the logic by static inspection and by cross-referencing the identical guard pattern already in use elsewhere in the repo, rather than by executing the suite. The PR description's own verification output (105 passed, 1 skipped locally, 1 failed, 105 passed before the fix as root) is consistent with the code change.

No changes requested.

[gloryfromca]

No blockers; this can merge as far as I am concerned.

What I checked

The diff itself, the surrounding test, the repo's own conventions (AGENTS.md sections 1/2/3/5), whether the skip weakens coverage, CI configuration, and prior art in the tree.

The skip is honest, not a green-wash. The concern with skipif is always that a case stops asserting anything. Here the guard is keyed to os.geteuid() == 0 only, so every non-root run still executes the full body. Verified locally at euid 501:

uv run pytest tests/test_read_file_image.py -q -rs
106 passed in 16.44s        # 0 skipped, the case ran its assertions
uv run ruff check tests/test_read_file_image.py
All checks passed!

That matches the description's root-side figure (105 passed, 1 skipped) as the complement. The unit matrix in .github/workflows/ci.yml is ubuntu-latest and does not run as root, so CI keeps exercising the case exactly as before; the skip only fires on a root developer box.

The alternative would have been worse. The behaviour under test is what build_user_content does when the read raises. As root, DAC is bypassed, so the only ways to reach that path are patching the read or forcing a delete between resolution and read -- both of which assert against a mock rather than against the renderer. Guarding the case is the right trade, and the reason string names the cause so nobody has to rediscover it.

It matches prior art. tests/test_importer_hermes_skills.py already carries the identical decorator and reason string at two sites, so this is the established spelling in this repo rather than a new local invention. Moving import os to module scope is required, since the decorator is evaluated at collection time.

Conventions. Branch test/skip_chmod_case_as_root fits the <type>/<snake_desc> form; the commit header is 74 chars, lowercase, no trailing period, scope is a real subpackage; commit message and PR description are both ASCII-clean (grepped with [^\x00-\x7F], zero matches); the Co-authored-by trailer is present in the commit and absent from the PR description, which is what squash-merge wants. No source file is touched, so there is no backward-compatibility surface.

One pre-existing note, not a finding against this diff: os.geteuid does not exist on Windows, so a module-scope call would be an AttributeError at collection there. This is already the situation in test_importer_hermes_skills.py, the unit matrix never runs on Windows, and the one windows-latest job runs a single unrelated integration file -- so nothing this PR does makes any run fail. Worth a sys.platform guard someday if the suite ever needs to collect on Windows; out of scope here.