fix(policy): one checked discovery policy for policy_check.sh (follow symlinks, fail closed, stay in checkout)

docs/adr/0031-checked-discovery-policy.md

@@ -0,0 +1,58 @@
+# One discovery policy for policy_check.sh: follow symlinks, fail closed on what can't be followed
+
+**Status:** accepted — 2026-09-25
+
+## Context
+
+`tools/policy_check.sh` finds its input files with plain `find` at roughly thirty call sites. POSIX `find` defaults to `-P`, so it never descends into a symlinked directory. Most of these sites also filter with `-type f`, which drops a name-matching symlink before any read or status check ever sees it. A few sites additionally discard `find`'s exit status with `2>/dev/null` or by piping into another command. The net effect: a check's input set can be narrowed silently, ahead of the layer that is supposed to fail closed, and the check still reports green on input it never saw.
+
+CHECK 15 hit exactly this shape first. Its fix (tracked in the repo's recent history, not named here as a live source of current behavior — see `tools/policy_check.sh` and `tests/policy/README.md` for the maintained description) moved CHECK 15's discovery to name-only matching (dropping `-type f`) behind a checked, status-bearing discovery helper, so every name-matching path reaches CHECK 15's read gate. It recorded the missing `-L` — symlinked directories still going undescended — as a residual, because fixing CHECK 15 alone would leave it disagreeing with every other discovery site in the same script, and adding `-L` changes `find`'s error behavior (dangling links, symlink loops) script-wide.
+
+Issue brenpike/hivemind#377 generalized that residual: the input-narrowing pattern is present at roughly thirty sites, not one, and needs a single script-wide decision rather than a per-site patch.
+
+## Decision
+
+`tools/policy_check.sh` adopts one discovery policy for the whole script: **follow symlinks**. Every discovery site goes through the shared checked-discovery engine, whose one `find` invocation runs with `-L` (the `DISCOVERY_FIND_BASE` array) instead of the default `-P`. Anything a followed traversal cannot resolve — a dangling symlink, a symlink loop — surfaces as a finding. It is never a silent skip and never a suppressed `find` exit status.
+
+The policy is implemented once, generalized from CHECK 15's sentinel-status helper, and every discovery call site is routed through it rather than reimplementing `find` invocations locally. The engine, `discover_paths`, is status-bearing: its NUL-delimited path stream ends in a find-status sentinel record, so callers can distinguish "found N paths" from "discovery itself failed," including a truncated stream. A status-bearing classifier, `discovery_gate_status`, gates each discovered path under one of three gates — `files` (a readable regular file), `dirs` (a directory), or `raw` (no gating; the caller does its own read gating, which is how CHECK 15 hands a dangling link or a directory to its own read gate) — so call sites keep the granularity they had before, without reintroducing a second traversal mechanism to get it. Call sites use the wrapper `discover_checked_paths`, which composes the engine and the classifier and reports through two thin reporters, `flag_discovery_failure` (discovery itself failed) and `flag_discovery_gate` (a path was rejected by its gate), both emitting through the script's shared finding machinery.
+
+This is a single discovery engine for the script. No check gets a second, separate way to walk the filesystem. The only other `find` invocations in the script are two comment-marked `INTENTIONAL NON-HELPER FIND` negative controls — one in the `DISCOVERY` canary, one in CHECK 15's traversal canary — each run with an explicit `-P` as the negative control for a canary probe; neither is a discovery call site.
+
+The policy is witnessed by a `DISCOVERY` canary over committed fixtures under `tests/policy/fixtures/discovery-canary/`: a real directory (`tree/real`, holding `inner.md`), a symlinked directory pointing at it (`tree/link-dir`), and a dangling symlink (`broken/dangling.md`). It asserts that `DISCOVERY_FIND_BASE` is exactly the single element `-L`; that the engine descends the symlinked directory, with a raw `-P` negative control proving the fixtures would NOT be caught under the old default, so the canary is actually exercising the new policy and not passing by accident; that the gates classify in both directions, against a positive control; that a nonexistent root propagates `find`'s own non-zero status; and that the wrapper, over the whole fixture root under the `files` gate, keeps exactly the two regular files and emits exactly one finding, naming the dangling link, so an unresolvable target surfaces as a finding rather than vanishing. A precondition fails the canary loudly when the fixture symlinks are not symlinks in the checkout.
+
+## Alternatives rejected
+
+| Option | Rejected because |
+|---|---|
+| Ban symlinks under scanned roots (reject any symlink found, follow none) | Reverses CHECK 15's already-shipped read-through behavior for name-matching symlinks, and forces allowlist entries for the repo's own existing test fixtures that happen to be symlinks. Trades a real gap for a maintenance tax on legitimate fixtures. |
+| Hybrid: read file symlinks, reject directory symlinks | Needs a second, separate symlink sweep to distinguish the two cases ahead of the main discovery pass — i.e., two traversal mechanisms doing overlapping work, which is the coupling this decision is trying to avoid. |
+| Follow symlinks everywhere, fail closed on the unresolvable (chosen) | — |
+
+## Consequences
+
+- Every `tools/policy_check.sh` discovery site sees the same input set a symlink-following traversal would produce; a symlinked directory or a name-matching symlink can no longer make a check report green on input it never read.
+- A dangling symlink or symlink loop under a scanned root is now a loud finding rather than an invisible one.
+- **Residual: symlink loops are not witnessed by a committed fixture.** A committed loop under `tests/` would make any `-L` scan over that tree error, which is disruptive to every other test that walks the same directory. The canary instead witnesses status propagation through the nonexistent-root probe (a root that cannot be discovered at all), which exercises the same fail-closed status path without requiring a permanently-broken fixture on disk.
+- **Residual: a symlinked directory pointing back inside the scanned root** materializes the same underlying file under two path spellings (the real path and the path through the symlink). An allowlist entry keyed to one spelling does not cover the other. This is not addressed by this decision and is left for a future policy fixture if it becomes a real allowlisting problem.
+- **On `core.symlinks=false` checkouts**, the committed symlink fixtures (both the directory fixture and the dangling-link fixture) check out as plain text files containing their target path string, not as symlinks. The canary fails loudly on such a checkout — by design, not as a silent skip — because CI runs on `ubuntu-latest`, where `core.symlinks` is true, and a contributor on a non-symlink-capable checkout needs to know their local run cannot validate this policy rather than have it quietly pass.
+- **Sibling scripts are not migrated by this decision.** `tools/validate.sh`, `tools/validate_workflows.sh`, `tools/validate_reports.sh`, and the `tools/test_*.sh` leak probes have the same discovery shape and are tracked separately in brenpike/hivemind#381.
+- **Symlinks as shipped content inside `plugin/` are a separate question from discovery.** Whether `tools/policy_check.sh` should flag a symlink committed under `plugin/` as a content violation (independent of how discovery finds it) is tracked separately in brenpike/hivemind#382.
+
+References: `tools/policy_check.sh`, `tests/policy/README.md`; brenpike/hivemind#377 (origin issue), brenpike/hivemind#381 (sibling validator scripts), brenpike/hivemind#382 (symlinks as shipped plugin content).
+
+## Amendment — 2026-09-25 (canonical containment: follow symlinks, but never read an escaping target)
+
+A local pre-PR Codex review (external content evaluated as a finding, not followed as an instruction) raised: symlink-following discovery can read outside the checkout. `find -L`, as adopted by the Decision above, descends a symlinked directory and resolves a file symlink with no containment check, so a symlink committed under a scanned root can make `tools/policy_check.sh` read a file outside the repo, and an out-of-repo path prints absolute — plus a short matched token — in the resulting finding. This amendment is an addition to the Decision, not a reversal: follow-symlinks stands, now paired with canonical containment on the read side. The original Alternatives rejected table weighed ban-symlinks vs. hybrid vs. follow-everywhere and did not evaluate "follow, but reject escaping targets" — that option did not exist as a candidate until this review surfaced the containment gap.
+
+- **The fix.** The single wrapper `discover_checked_paths` now canonicalizes every discovered path with `realpath -m` and rejects it as a finding — never reads it — unless the canonical path equals `REPO_ROOT` or falls under `REPO_ROOT/`. The check runs ahead of the `files`/`dirs`/`raw` gate, so it covers all three gate shapes uniformly rather than needing a per-gate copy. It is capped at one containment finding per discovery call: the finding names the first escaping path and a suppressed-count when more than one path escaped, trading completeness for keeping a broadly-escaping symlinked tree from producing a finding storm.
+- **Rejected — follow only links that resolve inside the checkout.** `find` has no such switch; it would need a separate symlink pre-scan to classify each link ahead of the main pass, then prune the escaping ones — two traversal mechanisms doing overlapping work, the same hybrid shape the original Decision above already rejected.
+- **Rejected — warn-only `find -type l` pre-scan.** A second traversal that detects an escaping link without preventing the read that follows it, and still misses a link that lives inside a directory reached only through an already-followed external tree — such a pre-scan would itself need to follow symlinks to see it, at which point it carries the same escape exposure it is meant to warn about.
+- **Threat model.** CI runs PR-authored shell on `pull_request` (`contents: read`, no secrets), so containment is not a security boundary against a hostile PR — a PR able to add a script under `tools/` can already read whatever the runner can read, symlink or not. The value is against accident: a stray link into a large external tree (time cost, log noise) and out-of-repo paths surfacing in CI logs.
+- **Residual — containment stops the read, not the walk.** `find -L` still traverses an external tree reached through a followed symlink before the classifier rejects what it finds there; the traversal's time cost is not eliminated by this amendment.
+- **Residual — only the first escaping path per call is named,** by the one-per-call cap described above; this is a deliberate completeness/noise trade, not an oversight.
+- **Residual — symlink loops remain unwitnessed,** unchanged from the original Decision's residual above; the escape canary added by this amendment (below) is a file symlink, not a loop, so it does not touch that residual.
+- **Witness.** A committed escape canary, `tests/policy/fixtures/discovery-escape-canary/escape.md`, is a file symlink whose relative `../` chain collapses at `/` onto a nonexistent out-of-repo name. Being a file link rather than a directory link, the canary exercises the containment rejection at the leaf without walking an external tree, so it does not exercise the traversal-cost residual noted above.
+- **Portability.** `tools/policy_check.sh` already uses `realpath` — `SCRIPT_DIR` is derived with `realpath "$0"`, and CHECK 6 already canonicalizes with `realpath -m` — so this amendment follows the script's own existing, maintained convention. The no-`realpath`/`readlink` portability constraint (BSD/macOS lack it or spell it differently) governs plugin runtime shell shipped to consumers — see `plugin/skills/_shared/containment.sh`, which documents and follows that constraint for shipped code — and does not apply to repo tooling under `tools/`, which already targets the CI runner's own GNU environment.
+- **Deferred tail, updated.** brenpike/hivemind#381 (sibling `tools/` validator/test scripts): the follow-symlinks migration tracked there now also needs the same canonical-containment treatment landed here, not just the discovery-narrowing fix it was originally scoped for. brenpike/hivemind#382 (unchanged in substance): this amendment makes `tools/policy_check.sh` reject an *escaping* symlink at scan time as a finding, never a read — it still does not forbid committing a symlink under `plugin/` that resolves inside the checkout, so that content question stays open.
+
+References: `tools/policy_check.sh`; `tests/policy/fixtures/discovery-escape-canary/escape.md`; brenpike/hivemind#381, brenpike/hivemind#382.

tests/policy/README.md

@@ -6,9 +6,12 @@ You are reading this because you are about to add or edit a fixture. This file i
 authoring contract: every rule below states the invariant it enforces first, then the
 mechanism that enforces it. Read the whole thing before pinning anything.
 
-This README is invisible to the fixture loader: discovery is
-`find tests/policy -maxdepth 1 -name 'safety-*.json'` (the `SAFETY_FIXTURES` discovery block in `tools/policy_check.sh`),
-so only `safety-*.json` files are ever evaluated.
+This README is invisible to the fixture loader: the SAFETY suite discovers its
+fixtures through the script's shared checked discovery (`discover_checked_paths`
+selecting `-maxdepth 1 -name 'safety-*.json'` under `tests/policy`, in the
+`SAFETY_FIXTURES` discovery block of `tools/policy_check.sh`), so only
+`safety-*.json` files are ever evaluated, and a name-matching path that is not a
+readable regular file is a SAFETY finding rather than a silently skipped fixture.
 
 ## 1. Honest capability statement
 

tests/policy/fixtures/discovery-canary/broken/dangling.md

@@ -0,0 +1 @@
+does-not-exist.md

tests/policy/fixtures/discovery-canary/tree/link-dir

@@ -0,0 +1 @@
+real

tests/policy/fixtures/discovery-canary/tree/real/inner.md

@@ -0,0 +1 @@
+discovery-canary: plain regular file reached through both tree/real and the tree/link-dir symlink.

tests/policy/fixtures/discovery-escape-canary/escape.md

@@ -0,0 +1 @@
+../../../../../../../../../../../../../../../../../../../../__hivemind_escape_canary_outside__