fix: per-root hardlink-scan budgets with loud truncation (#646)

[bolichen97]

Problem

The Linux namespace sandbox's pre-exec hardlink scan (Step 7 of the launcher script generated by _build_launcher_script) walks the agent CWD and /tmp looking for hardlinks (st_nlink > 1) aliasing protected credential inodes. The walk was capped by a single counter (_MAX_SCAN = 10000) shared across BOTH roots, and on exhaustion the loops broke silently and fell through to os.execvp exactly as if the scan had passed clean.

Why it matters

Two failure modes, both live in normal operation (issue evidence: /tmp alone held ~11.8k files from routine telemetry/cache churn — no attacker action needed):

1. A truncated scan was indistinguishable from a clean one — no stderr line, no audit signal.
2. The CWD is walked first and consumed the shared budget, so a large worktree starved /tmp — the world-writable root the check exists for — of any scanning at all.

Fix (symptoms → root cause → change)

Silent degradation → one shared budget with a silent break, spent unconditionally on every spawn → three changes confined to the Step-7 scan block of the f-string launcher template in src/kiro_crew/sandbox.py:

1. Arm the walk only when an alias can exist — credential inodes enter the match set only when st_nlink > 1. A single-link inode has no alias anywhere on the filesystem, so on healthy hosts the whole CWD+/tmp walk is skipped and the spawn pays nothing. st_nlink is an inode property (any hardlink raises it on the shared inode), so the gate cannot miss a pre-existing alias; the BLOCKED-exit input set is unchanged for any filesystem state stable across the scan (the walk side already required st_nlink > 1 to match).
2. Independent per-root budgets, sized for the suspicious case — _MAX_SCAN_PER_ROOT = 100000 with _root_scanned reset per _scan_root, so a large CWD can never zero out the /tmp scan, and the budget (only ever paid when a credential is actually aliased) covers the busiest observed /tmp with an order of magnitude to spare. Worst case ≤200k lstats in the child pre-exec, off the gateway loop.
3. Loud, observable truncation — one stderr diagnostic per truncated root (sandbox: WARNING — pre-exec hardlink scan truncated at N files in <root>; scan incomplete (control degrades open)), captured by the parent's stderr drain. Deliberately fail-OPEN: exiting would break sandbox spawns on any host whose /tmp outgrows the budget; rationale documented in the block comment.

Deliberately not included: pruning "known-noise" /tmp subtrees by name prefix. /tmp is world-writable and the sandboxed agent shares the uid, so any name-based skip list is a deterministic bypass (plant the hardlink under /tmp/node-compile-cache-evil/). Both pre-push reviewers flagged this independently; a regression test pins the absence of any prune list.

Tests

test/test_sandbox_argv.py::TestHardlinkScanBudget (8 tests, same generated-script text/compile-assertion pattern as the other launcher tests — the launcher needs unshare and cannot run end-to-end in CI). Red-checked against unfixed code.

• shared _scan_count/_MAX_SCAN counter is gone
• both collection loops carry the st_nlink > 1 gate (exactly 2 occurrences)
• _MAX_SCAN_PER_ROOT = 100000 present
• AST assertion that the _root_scanned = 0 reset is a DIRECT child of the for _scan_root loop body (a byte-offset check could not distinguish a per-root reset from a per-directory one nested in the os.walk loop)
• truncation warning literal present, routed to stderr, with no sys.exit on that path
• regression guard: sandbox: BLOCKED — found hardlink exit path unchanged
• no directory pruning: _SKIP_TMP_DIR_PREFIXES absent from the generated script
• compile(script, "<launcher>", "exec") at all three sandbox levels proves the f-string brace escaping produced valid Python

Manual verification

N/A — unit coverage sufficient: the changed logic is fully exercised through the generated-script assertions plus compile(); the walk itself runs in the sandbox child pre-execvp and needs unshare, which CI lacks. Full backend gates (isort/flake8/mypy/pytest) green locally; pytest failures on the dev host are identical to a clean origin/main baseline (pre-existing environment failures, zero overlap with this change).

Screenshots

N/A — no UI change.

Out of scope (per issue)

Per-session private /tmp namespace; periodic /tmp junk sweep in session maintenance.

Closes #646

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed ca83eb5746fb38cb21d938357fe3fbcab1c8fb34 — this comment is updated in place on each push.

Review details

I've analyzed the diff and the surrounding code. Let me verify my reasoning on the key semantic change (the nlink > 1 gate).

The core change narrows _protected_inodes to only credential inodes with st_nlink > 1. This is logically sound: a dangerous hardlink alias to a credential file would increment the credential inode's link count, so any credential that actually has an alias in the workspace necessarily has nlink > 1. A credential with nlink == 1 provably has no alias, so excluding it cannot miss a real hardlink. The walk itself also only considers nlink > 1 files, and hardlinks can't cross devices so (dev, ino) matching holds. No detection is lost.

The budget change (shared 10000 → per-root 100000) widens coverage and fixes the starvation of the /tmp walk. Truncation now warns on stderr; the fail-open behavior on exhaustion already existed before this PR (the old code also breaks and falls through to execvp), so no guard was removed. This runs in the child launcher pre-exec path, not on the event loop, so the blocking-call rule doesn't apply.

No findings.

[OPUS-REVIEWED] ca83eb5

Verdict parsed from the review's SHA-scoped output markers for commit ca83eb5746fb38cb21d938357fe3fbcab1c8fb34.

False positive or not applicable? A repository writer can comment:
/ai-review override fable ca83eb5746fb38cb21d938357fe3fbcab1c8fb34: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of ca83eb5746fb38cb21d938357fe3fbcab1c8fb34 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/sandbox.py:1499 -- "if _st.st_nlink > 1:" accepts hidden directories duplicated into SENSITIVE_FILES, whose link count is normally ≥2, so healthy hosts still trigger the full walk -> Fix: also require os.path.isfile(_pf).
[GPT-REVIEWED] ca83eb5

False positive or not applicable? A repository writer can comment:
/ai-review override gpt ca83eb5746fb38cb21d938357fe3fbcab1c8fb34: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of ca83eb5746fb38cb21d938357fe3fbcab1c8fb34 — updated in place on each push; does not block merge.

Design-Verdict: PASS

Root-cause fix with the right trade-offs: arm-on-alias gate, per-root budgets, and an explicit fail-open rationale; the name-prune bypass was correctly rejected.

Suggestions

• The degraded-open signal is a stderr line only; a small follow-up teaching the gateway's stderr drain to promote sandbox: WARNING — pre-exec hardlink scan truncated into the SEL audit log would make the control's degradation auditable, not just visible to whoever tails logs.

[DESIGN-REVIEWED] ca83eb5

[bolichen97]

Design Review dispositions (advisory CONCERNS on 16bfd87 — both suggestions adopted, amended into ca83eb5):

1. Gate the walk on the protected files themselves — fixed. Credential inodes now enter the match set only when st_nlink > 1 (both collection loops). A single-link inode has no alias anywhere, so healthy-host spawns skip the CWD+/tmp walk entirely — the chronic-truncation-warning concern disappears at its root rather than being suppressed. Equivalence at the BLOCKED exit was independently verified: the walk side already required st_nlink > 1 to match, so the gated set reaches the same verdicts for any filesystem state stable across the scan. Pinned by test_only_aliased_credential_inodes_arm_the_walk.

2. Raise the per-root budget — fixed. _MAX_SCAN_PER_ROOT raised 10k → 100k. Since the budget is now only spent when a credential is actually aliased (genuinely suspicious state), the cost is acceptable — worst case ≤200k lstats in the sandbox child pre-exec, off the gateway loop — and the documented 11.8k-file /tmp is fully scanned instead of warned about. Truncation is now genuinely exceptional. Pinned by test_per_root_budget_covers_a_busy_tmp.

The attacker-filler scenario (planting files to push a hardlink past the budget) is mitigated but not eliminated — an attacker must now stage ~100k files per root instead of relying on ambient churn, and the truncation warning fires loudly when they do. Fully closing that requires the per-session private /tmp namespace the issue lists as follow-up work.