From be8a0665231dd9436b53b956f2910d619b0a3ff2 Mon Sep 17 00:00:00 2001 From: Bren Pike Date: Fri, 25 Sep 2026 15:24:59 -0600 Subject: [PATCH 01/10] fix(policy): add shared checked-discovery helper and ADR-0031 --- docs/adr/0031-checked-discovery-policy.md | 41 ++++++ tools/policy_check.sh | 166 ++++++++++++++++++++++ 2 files changed, 207 insertions(+) create mode 100644 docs/adr/0031-checked-discovery-policy.md diff --git a/docs/adr/0031-checked-discovery-policy.md b/docs/adr/0031-checked-discovery-policy.md new file mode 100644 index 00000000..0f09dba3 --- /dev/null +++ b/docs/adr/0031-checked-discovery-policy.md @@ -0,0 +1,41 @@ +# 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 uses `find -L` (or the equivalent checked-discovery helper) instead of 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, as a shared checked-discovery helper generalized from CHECK 15's sentinel-status helper, and every discovery call site is routed through it rather than reimplementing `find` invocations locally. The helper is status-bearing: callers can distinguish "found N paths" from "discovery itself failed," and a status-bearing gate classifies each discovered path (files, directories, or raw/unfiltered) so call sites keep the granularity they had before, without reintroducing a second traversal mechanism to get it. + +This is a single discovery engine for the script. No check gets a second, separate way to walk the filesystem. + +The policy is witnessed by a committed `DISCOVERY` canary: a committed symlinked-directory fixture (proving a symlinked directory is now descended and its contents reach the read gate) and a committed dangling-symlink fixture (proving an unresolvable target surfaces as a finding rather than vanishing), plus a `-P` negative control (proving the canary fixtures would NOT be caught under the old default, so the canary is actually exercising the new policy and not passing by accident). + +## 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). diff --git a/tools/policy_check.sh b/tools/policy_check.sh index b1975494..6a746075 100644 --- a/tools/policy_check.sh +++ b/tools/policy_check.sh @@ -279,6 +279,172 @@ file_candidates() { printf '%s' "$out" } +# ── Checked discovery ─────────────────────────────────────────────────────── +# The single file-discovery engine for this script. Contract: +# +# Policy: every discovery find runs with DISCOVERY_FIND_BASE (-L), so +# symlinks are FOLLOWED script-wide -- a symlinked file or directory is +# scanned as its target. Failures are loud, never silently skipped: +# - a symlink loop makes find print an error and exit non-zero (it keeps +# traversing), so discover_paths returns that status and the caller +# reports it through flag_discovery_failure; +# - a dangling symlink is printed by find -L with exit 0, so it is the +# `files` gate of discovery_gate_status that rejects it (missing), and +# the caller reports it through flag_discovery_gate. +# Precondition: every ROOT must exist. This is the CALLER's job; there is +# no swallow mode and find's stderr is never suppressed, so a missing root +# fails discovery with find's non-zero status and must be reported. +# Findings: both reporting wrappers emit through add_finding, so --strict +# and the allowlist apply. They do NOT set any per-check found/pass flag; +# the caller owns that. +# Residuals: symlink-loop reporting is not witnessed by a committed fixture; +# a symlinked directory that points back inside a scanned root materialises +# the same file under two paths (it is scanned twice, never skipped). + +DISCOVERY_FIND_BASE=(-L) +DISCOVERY_FIND_STATUS_TAG='__DISCOVERY_FIND_STATUS=' +DISCOVERY_RC_TRUNCATED=20 +DISCOVERY_RC_USAGE=21 +DISCOVERY_GATE_RC_MISSING=22 +DISCOVERY_GATE_RC_NOT_REGULAR=23 +DISCOVERY_GATE_RC_UNREADABLE=24 +DISCOVERY_GATE_RC_NOT_DIR=25 +DISCOVERY_GATE_RC_UNKNOWN_GATE=26 + +# discover_paths DEST_ARRAY ROOT... -- FIND_ARGS... +# Materialises the paths +# find "${DISCOVERY_FIND_BASE[@]}" ROOT... FIND_ARGS... -print0 +# emits into the caller-named array DEST_ARRAY (replacing its contents), and +# returns find's exit status, DISCOVERY_RC_TRUNCATED when the stream does not +# end in exactly one status record, or DISCOVERY_RC_USAGE (with a stderr +# message) when no ROOT or no `--` separator is given. Paths found before a +# failure are still materialised so they are scanned; the non-zero status is +# what keeps the caller from reading the list as clean. +# +# DEST_ARRAY is bound by nameref, so nested discovery (a discovery inside +# another discovery's loop) must use distinct destination names; it must also +# not be one of this function's own `discovery_*` locals. +# +# INVARIANT: a failing producer inside `< <(...)` is invisible to the reading +# loop (see the materialisation invariant above), so the producer appends its +# own status as a trailing NUL-delimited sentinel record. The +# `|| discovery_find_status=$?` is load-bearing: errexit is inherited by the +# process substitution, so a bare `find ...; printf ... "$?"` dies before the +# sentinel is written whenever find fails. +# +# INVARIANT: every emitted path begins with one of the ROOTs, so a path record +# can only collide with DISCOVERY_FIND_STATUS_TAG if a ROOT itself begins with +# it; callers pass absolute roots. +discover_paths() { + local -n discovery_dest_ref="$1" + shift + local -a discovery_roots=() + local discovery_saw_separator=false + while [[ $# -gt 0 ]]; do + if [[ "$1" == '--' ]]; then + discovery_saw_separator=true + shift + break + fi + discovery_roots+=("$1") + shift + done + if [[ "$discovery_saw_separator" != true || "${#discovery_roots[@]}" -eq 0 ]]; then + echo "discover_paths: usage: discover_paths DEST_ARRAY ROOT... -- FIND_ARGS..." >&2 + return "$DISCOVERY_RC_USAGE" + fi + local discovery_record discovery_status_record='' + local discovery_sentinel_total=0 discovery_last_was_sentinel=false + discovery_dest_ref=() + while IFS= read -r -d '' discovery_record; do + if [[ "$discovery_record" == "$DISCOVERY_FIND_STATUS_TAG"* ]]; then + discovery_sentinel_total=$((discovery_sentinel_total + 1)) + discovery_status_record="$discovery_record" + discovery_last_was_sentinel=true + else + discovery_dest_ref+=("$discovery_record") + discovery_last_was_sentinel=false + fi + done < <(discovery_find_status=0; find "${DISCOVERY_FIND_BASE[@]}" "${discovery_roots[@]}" "$@" -print0 || discovery_find_status=$?; printf '%s%d\0' "$DISCOVERY_FIND_STATUS_TAG" "$discovery_find_status") + if [[ "$discovery_sentinel_total" -ne 1 || "$discovery_last_was_sentinel" != true ]]; then + return "$DISCOVERY_RC_TRUNCATED" + fi + return "${discovery_status_record#"$DISCOVERY_FIND_STATUS_TAG"}" +} + +# discovery_gate_status PATH GATE +# Status-bearing classifier for one discovered PATH (symlinks resolved, per +# the -L policy). GATE is one of: +# files -- 0 only for a readable regular file; otherwise +# DISCOVERY_GATE_RC_MISSING (missing or dangling symlink), +# DISCOVERY_GATE_RC_NOT_REGULAR, or DISCOVERY_GATE_RC_UNREADABLE; +# dirs -- 0 for a directory (a symlinked directory passes); otherwise +# DISCOVERY_GATE_RC_MISSING or DISCOVERY_GATE_RC_NOT_DIR; +# raw -- always 0; the caller does its own read gating. +# An unknown GATE returns DISCOVERY_GATE_RC_UNKNOWN_GATE with a stderr message. +discovery_gate_status() { + local candidate_path="$1" gate_name="$2" + case "$gate_name" in + files) + if [[ ! -e "$candidate_path" ]]; then + return "$DISCOVERY_GATE_RC_MISSING" + fi + if [[ ! -f "$candidate_path" ]]; then + return "$DISCOVERY_GATE_RC_NOT_REGULAR" + fi + if [[ ! -r "$candidate_path" ]]; then + return "$DISCOVERY_GATE_RC_UNREADABLE" + fi + ;; + dirs) + if [[ ! -e "$candidate_path" ]]; then + return "$DISCOVERY_GATE_RC_MISSING" + fi + if [[ ! -d "$candidate_path" ]]; then + return "$DISCOVERY_GATE_RC_NOT_DIR" + fi + ;; + raw) + ;; + *) + echo "discovery_gate_status: unknown gate '${gate_name}' (expected files, dirs, or raw)" >&2 + return "$DISCOVERY_GATE_RC_UNKNOWN_GATE" + ;; + esac + return 0 +} + +# flag_discovery_failure RULE ROOT LABEL STATUS +# Thin reporting wrapper: records the RULE finding for a discover_paths call +# (described by LABEL, anchored at ROOT) that returned non-zero STATUS. A +# partial path list is never a clean one. +flag_discovery_failure() { + local rule_name="$1" discovery_root="$2" discovery_label="$3" discovery_rc="$4" failure_reason + case "$discovery_rc" in + "$DISCOVERY_RC_TRUNCATED") failure_reason='its path stream ended without exactly one trailing find-status record (truncated)' ;; + "$DISCOVERY_RC_USAGE") failure_reason='discover_paths was called without a ROOT or without the -- separator' ;; + *) failure_reason="find exited ${discovery_rc} (a missing root, an unreadable directory, or a symlink loop)" ;; + esac + add_finding "$rule_name" "$discovery_root" 0 \ + "Discovery of ${discovery_label} failed: ${failure_reason}, so paths in it may never have been checked -- fix the tree; a failed discovery is NOT clean" +} + +# flag_discovery_gate RULE PATH RC +# Thin reporting wrapper: records the RULE finding for a discovered PATH that +# discovery_gate_status rejected with non-zero RC. +flag_discovery_gate() { + local rule_name="$1" candidate_path="$2" gate_rc="$3" rejection_reason + case "$gate_rc" in + "$DISCOVERY_GATE_RC_MISSING") rejection_reason='is missing or is a dangling symlink' ;; + "$DISCOVERY_GATE_RC_NOT_REGULAR") rejection_reason='is not a regular file' ;; + "$DISCOVERY_GATE_RC_UNREADABLE") rejection_reason='is not readable' ;; + "$DISCOVERY_GATE_RC_NOT_DIR") rejection_reason='is not a directory' ;; + *) rejection_reason="was rejected by the discovery gate with status ${gate_rc}" ;; + esac + add_finding "$rule_name" "$candidate_path" 0 \ + "this discovered path ${rejection_reason}, so it was never checked -- fix or remove it; an unchecked path is NOT clean" +} + # ── Timing instrumentation ────────────────────────────────────────────────── # Permanent per-check profiling (#305, precedent #304). Emits one # "[TIME]