The two-line comment above `documents/interview/**` says interview prep and
experience records live there. Nothing has ever written to that directory:
/interview saves its pack to
documents/applications/<company>_<role>/interview_prep_<stage>.md, covered by
the documents/applications/** rule. `git grep documents/interview` returns
only the two declarations of the rule itself (.gitignore and
REQUIRED_IGNORE_RULES), `git log --all -- 'documents/interview*'` is empty,
and documents/README.md documents the applications path outright.
Nothing leaks - the comment is the defect, and it is the misleading kind. It
is the one dedicated, well-argued line about interview material in the
personal-data block, so an auditor checking that the framework's most
sensitive artifact is covered reads it and stops, at the only path in the
block with no writer.
The comment's description of what needs protecting was always right; only its
location was wrong. It now sits above documents/applications/**, the rule that
actually provides that protection, so a reader auditing the block finds the
reasoning attached to the rule doing the work. documents/interview/** stays -
REQUIRED_IGNORE_RULES pins it, so dropping it from .gitignore alone turns CI
red, and it is harmless defence in depth - relabelled in both files as
belt-and-braces rather than the primary guard.
The new check-ignore case in GitignorePatternBehaviorTests derives the
prep-pack path from /interview's own spec instead of hardcoding it. That
distinction is the whole value of the test: a hardcoded path pins only that
documents/applications/** still matches that shape, which security_guards.py
already catches first, and stays green if /interview moves its output -
leaving the corrected comment stale exactly the way this issue found it.
Since #329 the spec states the location in two pieces - Step 1 derives the
archive folder, Step 3 names interview_prep_<stage>.md - so the test pins both
fragments separately and composes the concrete path from them. Mutation-
verified on each half: repointing the folder at documents/prep_packs/, and
renaming the file, both fail this test while `python3 tools/security_guards.py`
still reports OK.
The class's temp-repo setup moved to setUp for the second case.
ayobamiseun reviewed the pre-rebase branch and called all three rebase hazards
in advance: the split literal, the released CHANGELOG context, and the setUp
re-merge. Reached independently here during the rebase; the review was posted
first.
The upskill/*.md rule is rooted, but /upskill is a skill and skills
resolve bare relative paths against their own directory - the same
observed behavior the **/job_scraper rules exist for. A report at
.claude/skills/upskill/upskill/report-*.md was not ignored, and an
upskill report records the candidate's skill gaps against named
employers. **/upskill/*.md would also ignore the skill's own SKILL.md
(the directory shares the name), so the new rule pins the report-file
prefix: **/upskill/report-*.md. Added to .gitignore and
REQUIRED_IGNORE_RULES, with a check-ignore-based test pinning both
properties.
check_permissions() read permissions.allow and nothing else, so a `hooks`
block in the same file passed the guard silently.
A hook is strictly more dangerous than a pre-approved permission. A
permission pre-approves something Claude may choose to do; a hook runs
unconditionally when its event fires, with no prompt and no model decision
in between. Cloning the repo and opening it is enough.
This is the vector the Shai-Hulud worm used in its August 2026 wave: a
SessionStart hook in .claude/settings.json chaining to .claude/math_init.js,
executing on session start.
https://research.jfrog.com/post/shai-hulud-is-back-august/
For a template that thousands of people are explicitly invited to fork,
that is the riskiest key in the file this guard already parses.
Follows the established pattern exactly - ALLOWED_HOOKS ships empty, since
the template has no hooks, so any addition must be allowlisted in the same
PR and is therefore explicit and reviewable.
Two details worth reviewing closely:
- The hook check runs *before* the permissions shape guards. Those guards
return early, so a file pairing a malformed permissions block with a live
hook would otherwise skip the hook check entirely - a fail-open. Pinned by
test_hook_is_caught_even_when_permissions_block_is_malformed.
- _hook_commands() fails closed. Any hook layout it does not recognise
yields a marker that cannot be in the allowlist, so an unfamiliar shape is
rejected rather than silently skipped, rather than trusting that the
Claude Code schema will not change.
Verified:
- 8 new HookGuardTests cases; 14 of the suite's 26 tests fail against the
unpatched guard, all 26 pass with it
- injecting the real worm shape into this repo's own settings.json makes
the guard exit 1 naming 'SessionStart:node .claude/math_init.js';
removing it returns OK
- lint_skills, check_framework_version, security_guards all OK;
python3 -m unittest discover -s tests 219 passed
check_gitignore() verified each required personal-data rule was present via set membership, but .gitignore is order-sensitive: a later !pattern re-includes a file an earlier rule excluded, so the required line stays physically present while the file is no longer ignored - the guard failed open on exactly the weakening its docstring claims to catch. Keeps the required-rules-present check and additionally rejects any negation line outside a small reviewed ALLOWED_IGNORE_NEGATIONS allowlist (same explicit-widening pattern as ALLOWED_PERMISSIONS). Fixes#194.
By @thejesh23. Verified: allowlist matches the four negations currently in .gitignore; guard test suite passes locally (17 tests) and in CI.
Closes#194
* ci: run the Python test suite - CONTRIBUTING.md asks for tests CI never executes
CONTRIBUTING.md tells contributors to put Python tool tests in tests/
and run the relevant suites, and tests/ now holds real ones
(test_salary_lookup.py, test_convert_salary_excel.py from #75) - but no
CI job executes them. A suite that never runs in CI can't gate a PR and
silently rots. New python-tests job: unittest discover over tests/,
stdlib only, no new dependencies. Future test files run without any
workflow change.
Also lands tests/test_security_guards.py, which missed #84's merge
window (pushed to the branch as #84 was being merged; the merge took
2a6cb8c, the tests were 260c37a). 13 unittest cases in the existing
tests/ style: each copies the guard script into a synthetic repo tree
and runs it as a subprocess - the same way CI invokes it - asserting
real exit codes and messages. Every forbidden state fails (Bash(*) and
Bash(curl:*) additions, each personal-data gitignore rule removed one
at a time, each forbidden lifecycle script, trustedDependencies,
invalid settings JSON, zero manifests); every non-event passes (dropped
shipped permission, extra ignore rules, benign scripts, hostile
manifest inside node_modules); and the real repo passes its own guards.
22 tests total, all passing locally via the exact command the job runs.
* test: use benign lifecycle-script values in fixtures - AV heuristics flag attack-shaped strings
Review found the curl-pipe-to-sh fixture value matches a real Defender
signature (Trojan:Script/Stealer.HAX!MTB): Windows quarantines the temp
package.json mid-test, making the suite flaky for any Windows
contributor who runs it - while proving nothing extra, since the guard
flags the script KEY and never inspects the value.
Fixture values are now 'echo test' (also in the node_modules-ignored
test, same class of string), with a comment on the key-only test
explaining why the value must stay benign so a future 'make the fixture
realistic' cleanup doesn't reintroduce the quarantine flake. Coverage
is unchanged: same keys, same assertions, 13 tests passing.
Jakob Stender Guldberg