From 2964538e74730bcecfee157fa600a33eafaf8c94 Mon Sep 17 00:00:00 2001 From: Nelson Spence Date: Tue, 21 Apr 2026 20:42:25 -0500 Subject: [PATCH 01/10] chore(deps): bump navi-sanitize 0.1.0 -> 0.2.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brings in material sanitizer upgrades directly relevant to the new nboot audit feature planned next: - Invisible-char coverage expanded from 411 to 492 (U+FE00-FE0F variation selectors, math invisible operators, deprecated format controls, braille blank, ogham space, Hangul fillers, Mongolian FVS, Arabic letter mark). - C0/C1 control-char stripping — terminal-injection / ANSI-escape defense. - Homoglyph map expanded 54 -> 66 pairs (Greek lowercase, Cyrillic extended, Latin dotless i). - NFD decompose before homoglyph scan — closes the NFKC composition-bypass vector. - Iterative walker (no recursion, no stack overflow); max_depth parameter prevents crash on pathological nesting. API surface unchanged: clean / walk / jinja2_escaper / path_escaper all import and behave the same. 381 tests pass (including full adversarial suite that depends on sanitizer behaviour). --- pyproject.toml | 2 +- uv.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 25735c0..1f83775 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -24,7 +24,7 @@ dependencies = [ "click>=8.1.0", "jinja2>=3.1.0", "jsonschema>=4.20.0", - "navi-sanitize>=0.1.0", + "navi-sanitize>=0.2.1", "pyyaml>=6.0", ] diff --git a/uv.lock b/uv.lock index 750d367..a2a6a43 100644 --- a/uv.lock +++ b/uv.lock @@ -414,7 +414,7 @@ requires-dist = [ { name = "click", specifier = ">=8.1.0" }, { name = "jinja2", specifier = ">=3.1.0" }, { name = "jsonschema", specifier = ">=4.20.0" }, - { name = "navi-sanitize", specifier = ">=0.1.0" }, + { name = "navi-sanitize", specifier = ">=0.2.1" }, { name = "pyyaml", specifier = ">=6.0" }, ] @@ -432,11 +432,11 @@ docs = [{ name = "zensical", specifier = ">=0.0.24" }] [[package]] name = "navi-sanitize" -version = "0.1.0" +version = "0.2.1" source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/8b/1a/39992bde198be4ec2f2978ea0cd6519ed77b6d0973f30fc76abe2826082f/navi_sanitize-0.1.0.tar.gz", hash = "sha256:0f20aa257963a371dc2d2475060d97f9f5bc317713597d7be8cc38c77d4ac9ee", size = 49661, upload-time = "2026-03-01T19:06:44.663Z" } +sdist = { url = "https://files.pythonhosted.org/packages/5b/48/68760569ea1e9a5dcbf0a0ce4437b31e81056a001f369a55acfb53371376/navi_sanitize-0.2.1.tar.gz", hash = "sha256:6d18801dcc8af74bb27c6cad88d19b7f8c87dc757dea0c7306852b37cda49c1c", size = 207622, upload-time = "2026-04-05T04:40:57.037Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/84/4c/d6db86dc84308c564c5624834114085182daf6f665ecf9b5f25f7a52e334/navi_sanitize-0.1.0-py3-none-any.whl", hash = "sha256:58c9230227de26ab90702a35cf2133b626064d90739da2f7882c2d875f096c0f", size = 15017, upload-time = "2026-03-01T19:06:43.542Z" }, + { url = "https://files.pythonhosted.org/packages/fd/13/5e1b36e3b61a1553ca978b48ea62a5251463bd4d6f6331acab20b0bfbc93/navi_sanitize-0.2.1-py3-none-any.whl", hash = "sha256:64214b060d6d1a4ebd32916c627fbac73733aef2255138f274c0a943eb25ac73", size = 17863, upload-time = "2026-04-05T04:40:55.753Z" }, ] [[package]] From 349cccd1fc33a5cf248fb7cbf69088f8adc4fce2 Mon Sep 17 00:00:00 2001 From: Nelson Spence Date: Tue, 21 Apr 2026 20:55:40 -0500 Subject: [PATCH 02/10] feat(audit): pack-conformance auditor + SARIF 2.1.0 emitter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `nboot audit` — the pack-as-conformance-spec mode Codex review carved out from the original two-mode design. Reuses the existing plan + render_to_files + compute_diffs pipeline in memory; outputs human text or GitHub-compatible SARIF 2.1.0. New modules: - src/navi_bootstrap/sarif.py — hand-rolled SARIF emitter (no new runtime dep). Declares two rules (pack-drift-missing, pack-drift-changed) with stable partialFingerprints for GitHub cross-run deduplication. - src/navi_bootstrap/audit.py — AuditFinding dataclass + run_audit + findings_to_sarif / findings_to_text helpers. AuditError wraps all pipeline-stage failures (spec/manifest/resolve/plan/render). CLI: - `nboot audit --spec … --pack … --target … [--format text|sarif] [--output path] [--resolve] [--exit-zero]` - Offline by default — audits don't depend on GitHub API reachability. - Exits 1 on drift so CI fails; `--exit-zero` for report-only surveys. Single-sourced `__version__` via importlib.metadata so the SARIF report tracks installed package version automatically (removes a 0.1.1-vs-0.1.2 drift between __init__.py and pyproject.toml). Invisible-prompt-injection scan (Mode B from the original design) is deferred — per Codex's grumpy review, clean() is string-in/string-out with no offsets or vector classification. Filed upstream in navi-sanitize#38 requesting a structured scan() API. Once that lands, `nboot audit --scan-injections` becomes a thin addition. Tests: 26 new (11 SARIF emitter + 15 audit pipeline + CLI). Fixtures use the real scaffold pack end-to-end — renders, mutates one file, confirms the drift is reported. 407 tests pass, mypy strict clean (14 files), ruff + ruff-format clean, all 8 existing packs still validate against the schema. Docs: docs/reference/audit.md covers usage, SARIF upload pattern, exit codes, and relationship to `diff` / `apply`. --- docs/reference/audit.md | 116 ++++++++++++ src/navi_bootstrap/__init__.py | 10 +- src/navi_bootstrap/audit.py | 192 +++++++++++++++++++ src/navi_bootstrap/cli.py | 86 +++++++++ src/navi_bootstrap/sarif.py | 140 ++++++++++++++ tests/test_audit.py | 326 +++++++++++++++++++++++++++++++++ tests/test_sarif.py | 95 ++++++++++ 7 files changed, 963 insertions(+), 2 deletions(-) create mode 100644 docs/reference/audit.md create mode 100644 src/navi_bootstrap/audit.py create mode 100644 src/navi_bootstrap/sarif.py create mode 100644 tests/test_audit.py create mode 100644 tests/test_sarif.py diff --git a/docs/reference/audit.md b/docs/reference/audit.md new file mode 100644 index 0000000..18c17fe --- /dev/null +++ b/docs/reference/audit.md @@ -0,0 +1,116 @@ +# `nboot audit` — pack-conformance audit + +Check whether an existing project still matches a navi-bootstrap pack, and +emit findings as either human-readable text or SARIF 2.1.0 for upload to +GitHub's Security tab. + +## Why + +Most templating tools (Cookiecutter, Backstage Scaffolder, Yeoman) are +**create-only** — they generate a project once, then walk away. Copier has +an `update` flow but depends on the target having been Copier-generated in +the first place. + +`nboot audit` flips the problem: it re-renders a pack **in memory** and +compares the output to an existing project's files on disk. No merge, no +write, no state. You get a list of files that are missing or drifted from +the pack — the template becomes a living specification. + +This is especially useful for: + +- **Fleet surveys** — "which of our 100 repos still conform to the + `security-scanning` pack?" +- **CI gates** — run `nboot audit … --format sarif` in a nightly job and + upload via [`github/codeql-action/upload-sarif`] so drift appears in the + Security tab alongside CodeQL and Semgrep. +- **Regression detection** — after a bulk refactor, confirm no workflow or + pre-commit config silently fell out of conformance. + +[`github/codeql-action/upload-sarif`]: https://github.com/github/codeql-action + +## Usage + +```bash +uv run nboot audit \ + --spec nboot-spec.json \ + --pack security-scanning \ + --target /path/to/existing/project +``` + +Drift is reported and the command exits non-zero so CI fails: + +``` +Audit found 3 drift finding(s): + +Missing files (2): + - .github/workflows/codeql.yml + - .github/workflows/scorecard.yml + +Changed files (1): + - .github/dependabot.yml +``` + +## Flags + +| Flag | Default | Effect | +|---|---|---| +| `--spec` | (required) | Path to the project spec JSON | +| `--pack` | (required) | Pack name to audit against (`scaffold`, `base`, `security-scanning`, …) | +| `--target` | (required) | Existing project directory to inspect | +| `--format` | `text` | `text` for humans, `sarif` for GitHub Security tab | +| `--output` | stdout | Write to a file instead of stdout (useful with `--format sarif`) | +| `--resolve` | off | Resolve action SHAs via `gh` before planning (default: offline) | +| `--exit-zero` | off | Exit 0 even when drift is found (report-only CI surveys) | + +By default `audit` runs **offline** — no GitHub API calls. This lets fleet +audits run reliably from air-gapped or rate-limited environments. Pass +`--resolve` if the pack's rendered output depends on freshly-resolved +action SHAs. + +## SARIF output + +The SARIF 2.1.0 report declares two rules: + +- `pack-drift-missing` — file expected by the pack but absent from the target +- `pack-drift-changed` — file content differs from the pack's rendered output + +Each finding includes a stable `partialFingerprints.primaryLocationLineHash` +so GitHub's Security tab deduplicates across runs. + +Upload it in CI: + +```yaml +- name: Audit against security-scanning pack + run: | + uv run nboot audit \ + --spec nboot-spec.json \ + --pack security-scanning \ + --target . \ + --format sarif \ + --output audit.sarif.json \ + --exit-zero + +- name: Upload audit findings + uses: github/codeql-action/upload-sarif@v4 + with: + sarif_file: audit.sarif.json + category: nboot-audit +``` + +## Exit codes + +| Exit | Meaning | +|---|---| +| 0 | Target fully conforms to the pack, OR drift found with `--exit-zero` | +| 1 | Drift found without `--exit-zero` | +| >1 | Pipeline error (bad spec, missing pack, template render failure) | + +## Relationship to other verbs + +| Verb | Writes? | Output | Use when | +|---|---|---|---| +| `nboot diff` | No | Unified diff text | Human preview before `apply` | +| `nboot audit` | No | Finding list / SARIF | CI gate, fleet survey, Security-tab upload | +| `nboot apply` | Yes | Files on disk | Remediate drift by overwriting / merging | + +`diff` and `audit` run the same pipeline; they differ in output shape. diff --git a/src/navi_bootstrap/__init__.py b/src/navi_bootstrap/__init__.py index d3bec18..85d1319 100644 --- a/src/navi_bootstrap/__init__.py +++ b/src/navi_bootstrap/__init__.py @@ -3,9 +3,15 @@ """navi-bootstrap: Jinja2 rendering engine and template packs.""" +from importlib.metadata import PackageNotFoundError +from importlib.metadata import version as _pkg_version + from navi_bootstrap.packs import get_ordered_packs from navi_bootstrap.spec import build_spec_for_new -__all__ = ["build_spec_for_new", "get_ordered_packs"] +try: + __version__ = _pkg_version("navi-bootstrap") +except PackageNotFoundError: # pragma: no cover — only during dev without install + __version__ = "0.0.0+unknown" -__version__ = "0.1.1" +__all__ = ["__version__", "build_spec_for_new", "get_ordered_packs"] diff --git a/src/navi_bootstrap/audit.py b/src/navi_bootstrap/audit.py new file mode 100644 index 0000000..0d75e4a --- /dev/null +++ b/src/navi_bootstrap/audit.py @@ -0,0 +1,192 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 Project Navi + +"""Pack-conformance auditing. + +Runs the engine pipeline (resolve, validate, plan, render) in memory, then +diffs the rendered output against an existing target project. Reports files +that are missing or drifted relative to the conformance pack. + +Audit = diff-as-conformance-check, with structured output (SARIF or text). +Compared to `nboot diff` (human preview), audit is designed for: + - Fleet conformance surveys (run across N repos, aggregate findings) + - CI gating (fail a build when a repo drifts from a required pack) + - GitHub Security tab integration via SARIF upload +""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path + +import jinja2 + +from navi_bootstrap.diff import DiffResult, compute_diffs +from navi_bootstrap.engine import plan, render_to_files +from navi_bootstrap.manifest import ManifestError, load_manifest +from navi_bootstrap.packs import PackError, resolve_pack +from navi_bootstrap.resolve import ResolveError, resolve_action_shas +from navi_bootstrap.sanitize import sanitize_manifest, sanitize_spec +from navi_bootstrap.sarif import SarifReport, SarifResult +from navi_bootstrap.spec import SpecError, load_spec + + +class AuditError(Exception): + """Raised for any failure inside run_audit before findings can be produced.""" + + +@dataclass(frozen=True) +class AuditFinding: + """One audit finding — a file that's missing or drifted vs the pack. + + The `kind` field maps 1:1 to SARIF rule ids in sarif.py. + """ + + kind: str # "missing" | "changed" + dest: str # path relative to target repo + pack: str + + @property + def rule_id(self) -> str: + return {"missing": "pack-drift-missing", "changed": "pack-drift-changed"}[self.kind] + + @property + def message(self) -> str: + if self.kind == "missing": + return ( + f"File '{self.dest}' is missing; pack '{self.pack}' would " + f"create it. Run `nboot apply --pack {self.pack}` to remediate." + ) + return ( + f"File '{self.dest}' differs from pack '{self.pack}'. " + f"Run `nboot diff --pack {self.pack}` to review the drift." + ) + + def to_sarif_result(self) -> SarifResult: + return SarifResult( + rule_id=self.rule_id, + message=self.message, + artifact_uri=self.dest, + ) + + +def _diff_result_to_finding(diff: DiffResult, pack: str) -> AuditFinding: + return AuditFinding( + kind="missing" if diff.is_new else "changed", + dest=diff.dest, + pack=pack, + ) + + +def run_audit( + spec_path: Path, + pack: str, + target: Path, + *, + skip_resolve: bool = False, +) -> list[AuditFinding]: + """Run a pack-conformance audit against an existing project. + + Returns a list of AuditFinding (possibly empty for a fully-conforming repo). + Raises AuditError on any pipeline-stage failure (bad spec, missing pack, + template render error). Network operations (action-SHA resolution) can be + skipped via ``skip_resolve=True``; this is the default for ``nboot audit`` + because conformance checks shouldn't depend on GitHub API availability. + """ + try: + pack_dir = resolve_pack(pack) + except PackError as e: + raise AuditError(str(e)) from e + + try: + spec_data = load_spec(spec_path) + except SpecError as e: + raise AuditError(str(e)) from e + spec_data = sanitize_spec(spec_data) + + try: + manifest = load_manifest(pack_dir / "manifest.yaml") + except ManifestError as e: + raise AuditError(str(e)) from e + manifest = sanitize_manifest(manifest) + + # Stage 0 — resolve action SHAs (optional for audit; offline by default). + action_shas_config = manifest.get("action_shas", []) + try: + shas, versions = resolve_action_shas(action_shas_config, skip=skip_resolve) + except ResolveError as e: + raise AuditError(str(e)) from e + + # Stage 2 — plan. + templates_dir = pack_dir / "templates" + try: + render_plan = plan(manifest, spec_data, templates_dir) + except (jinja2.TemplateError, TypeError) as e: + raise AuditError(f"Template planning error: {e}") from e + + # Stage 3 — render to memory (no filesystem writes). + try: + rendered = render_to_files( + render_plan, + spec_data, + templates_dir, + action_shas=shas, + action_versions=versions, + ) + except (jinja2.TemplateError, TypeError) as e: + raise AuditError(f"Template render error: {e}") from e + + # Diff rendered-in-memory vs existing target filesystem. + diffs = compute_diffs(rendered, target, pack_name=pack) + + return [_diff_result_to_finding(d, pack) for d in diffs] + + +def findings_to_sarif( + findings: list[AuditFinding], + *, + tool_name: str, + tool_version: str, +) -> SarifReport: + """Build a SARIF report from a list of audit findings.""" + report = SarifReport(tool_name=tool_name, tool_version=tool_version) + for finding in findings: + report.add_result(finding.to_sarif_result()) + return report + + +def findings_to_text(findings: list[AuditFinding]) -> str: + """Human-readable audit summary. + + Used by `nboot audit` when ``--format text`` (the default). + """ + if not findings: + return "OK — target conforms to the pack." + + lines: list[str] = [ + f"Audit found {len(findings)} drift finding(s):", + "", + ] + by_kind: dict[str, list[AuditFinding]] = {"missing": [], "changed": []} + for f in findings: + by_kind[f.kind].append(f) + + for label, kind in (("Missing files", "missing"), ("Changed files", "changed")): + items = by_kind[kind] + if not items: + continue + lines.append(f"{label} ({len(items)}):") + for f in items: + lines.append(f" - {f.dest}") + lines.append("") + + return "\n".join(lines).rstrip() + "\n" + + +__all__ = [ + "AuditError", + "AuditFinding", + "findings_to_sarif", + "findings_to_text", + "run_audit", +] diff --git a/src/navi_bootstrap/cli.py b/src/navi_bootstrap/cli.py index 3c2f75f..9de9a51 100644 --- a/src/navi_bootstrap/cli.py +++ b/src/navi_bootstrap/cli.py @@ -14,6 +14,8 @@ import click import jinja2 +from navi_bootstrap import __version__ +from navi_bootstrap.audit import AuditError, findings_to_sarif, findings_to_text, run_audit from navi_bootstrap.diff import compute_diffs from navi_bootstrap.engine import plan, render, render_to_files from navi_bootstrap.hooks import run_hooks @@ -371,6 +373,90 @@ def diff_cmd(spec: Path, pack: str, target: Path, skip_resolve: bool) -> None: raise SystemExit(1) +@cli.command("audit") +@click.option( + "--spec", + required=True, + type=click.Path(exists=True, path_type=Path), + help="Path to the project spec JSON file", +) +@click.option("--pack", required=True, type=str, help="Name of the conformance pack") +@click.option( + "--target", + required=True, + type=click.Path(exists=True, file_okay=False, path_type=Path), + help="Project directory to audit against the pack", +) +@click.option( + "--format", + "output_format", + type=click.Choice(["text", "sarif"]), + default="text", + help="Report format (text for humans, sarif for GitHub Security tab)", +) +@click.option( + "--output", + type=click.Path(path_type=Path), + default=None, + help="Write report to this file instead of stdout", +) +@click.option( + "--resolve", + is_flag=True, + default=False, + help="Resolve action SHAs via gh before planning (default: offline)", +) +@click.option( + "--exit-zero", + is_flag=True, + default=False, + help="Exit 0 even when drift is found (report-only mode for CI surveys)", +) +def audit_cmd( + spec: Path, + pack: str, + target: Path, + output_format: str, + output: Path | None, + resolve: bool, + exit_zero: bool, +) -> None: + """Audit a project against a pack for conformance drift. + + Reports files that are missing or drifted relative to what the pack would + render. Supports --format=sarif for upload to GitHub's Security tab via + github/codeql-action/upload-sarif. + + Exits 0 when the target fully conforms; exits 1 when drift is found + (override with --exit-zero for report-only CI surveys). + """ + # Offline by default — conformance audits shouldn't depend on network. + skip_resolve = not resolve + if resolve and not gh_available(): + click.echo(_GH_NOTICE, err=True) + skip_resolve = True + + try: + findings = run_audit(spec, pack, target, skip_resolve=skip_resolve) + except AuditError as e: + raise click.ClickException(str(e)) from e + + if output_format == "sarif": + report = findings_to_sarif(findings, tool_name="nboot-audit", tool_version=__version__) + rendered = report.to_json() + else: + rendered = findings_to_text(findings) + + if output is None: + click.echo(rendered) + else: + output.write_text(rendered if rendered.endswith("\n") else rendered + "\n") + click.echo(f"Wrote {len(findings)} finding(s) to {output}", err=True) + + if findings and not exit_zero: + raise SystemExit(1) + + @cli.command("list-packs") def list_packs_cmd() -> None: """List all bundled template packs.""" diff --git a/src/navi_bootstrap/sarif.py b/src/navi_bootstrap/sarif.py new file mode 100644 index 0000000..04067ef --- /dev/null +++ b/src/navi_bootstrap/sarif.py @@ -0,0 +1,140 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 Project Navi + +"""SARIF 2.1.0 emitter for nboot audit output. + +Hand-rolled to avoid adding a runtime dependency. Produces GitHub-compatible +SARIF that lands in the Security tab when uploaded via +github/codeql-action/upload-sarif. + +Only the subset we need is implemented: + - One run per report + - One tool driver with pre-declared rules + - Per-finding result with physicalLocation artifactLocation + - partialFingerprints for cross-run deduplication +""" + +from __future__ import annotations + +import hashlib +import json +from dataclasses import dataclass, field +from typing import Any + +SARIF_SCHEMA_URI = ( + "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json" +) +SARIF_VERSION = "2.1.0" + +TOOL_INFORMATION_URI = "https://github.com/Project-Navi/navi-bootstrap" + +# Rule registry — every audit finding must cite one of these rule IDs. +AUDIT_RULES: tuple[dict[str, Any], ...] = ( + { + "id": "pack-drift-missing", + "name": "PackDriftMissing", + "shortDescription": { + "text": "File is expected by the pack but missing from the target.", + }, + "fullDescription": { + "text": ( + "The target project is missing a file the conformance pack " + "would render. Run `nboot apply --pack --target ` " + "to create it, or mark the pack as optional for this project." + ), + }, + "helpUri": "https://github.com/Project-Navi/navi-bootstrap/blob/main/docs/reference/audit.md", + "defaultConfiguration": {"level": "warning"}, + }, + { + "id": "pack-drift-changed", + "name": "PackDriftChanged", + "shortDescription": { + "text": "File content differs from what the pack would render.", + }, + "fullDescription": { + "text": ( + "The target project has a file whose content no longer matches " + "the conformance pack. Review `nboot diff --pack --target ` " + "for the unified diff; `nboot apply` will overwrite (create mode) " + "or merge (append mode)." + ), + }, + "helpUri": "https://github.com/Project-Navi/navi-bootstrap/blob/main/docs/reference/audit.md", + "defaultConfiguration": {"level": "warning"}, + }, +) + + +@dataclass +class SarifResult: + """A single SARIF result — one audit finding.""" + + rule_id: str + message: str + artifact_uri: str + level: str = "warning" + + def fingerprint(self) -> str: + """Stable hash for cross-run deduplication in the GitHub UI.""" + basis = f"{self.rule_id}:{self.artifact_uri}" + return hashlib.sha256(basis.encode("utf-8")).hexdigest() + + def to_dict(self) -> dict[str, Any]: + return { + "ruleId": self.rule_id, + "level": self.level, + "message": {"text": self.message}, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {"uri": self.artifact_uri}, + }, + }, + ], + "partialFingerprints": { + "primaryLocationLineHash": self.fingerprint(), + }, + } + + +@dataclass +class SarifReport: + """A SARIF 2.1.0 report — one tool run + N results.""" + + tool_name: str + tool_version: str + results: list[SarifResult] = field(default_factory=list) + rules: tuple[dict[str, Any], ...] = AUDIT_RULES + + def add_result(self, result: SarifResult) -> None: + # Defensive: reject results referencing unknown rules rather than emitting + # invalid SARIF that GitHub would silently drop. + known_ids = {rule["id"] for rule in self.rules} + if result.rule_id not in known_ids: + raise ValueError( + f"Unknown SARIF rule id {result.rule_id!r}; expected one of {sorted(known_ids)}" + ) + self.results.append(result) + + def to_dict(self) -> dict[str, Any]: + return { + "$schema": SARIF_SCHEMA_URI, + "version": SARIF_VERSION, + "runs": [ + { + "tool": { + "driver": { + "name": self.tool_name, + "version": self.tool_version, + "informationUri": TOOL_INFORMATION_URI, + "rules": list(self.rules), + }, + }, + "results": [r.to_dict() for r in self.results], + }, + ], + } + + def to_json(self, *, indent: int | None = 2) -> str: + return json.dumps(self.to_dict(), indent=indent, sort_keys=False) diff --git a/tests/test_audit.py b/tests/test_audit.py new file mode 100644 index 0000000..5c24562 --- /dev/null +++ b/tests/test_audit.py @@ -0,0 +1,326 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 Project Navi + +"""Tests for the pack-conformance audit pipeline.""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path + +import pytest +from click.testing import CliRunner + +from navi_bootstrap.audit import ( + AuditError, + AuditFinding, + findings_to_sarif, + findings_to_text, + run_audit, +) +from navi_bootstrap.cli import cli + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +def _write_spec(tmp_path: Path) -> Path: + """Write a minimal but complete spec that scaffold / base will render against.""" + spec = tmp_path / "nboot-spec.json" + spec.write_text( + json.dumps( + { + "name": "audit-fixture", + "version": "0.0.1", + "description": "Fixture project for audit tests.", + "license": "MIT", + "language": "python", + "python_version": "3.12", + "author": {"name": "Test"}, + "structure": {"src_dir": "src/audit_fixture", "test_dir": "tests"}, + "dependencies": {"runtime": [], "dev": ["pytest"], "optional": {}}, + "features": {"ci": False, "pre_commit": False}, + "github": {"org": "test", "repo": "audit-fixture"}, + "release": {"has_docker": False}, + "recon": { + "test_framework": "pytest", + "test_count": 0, + "coverage_pct": 0, + "python_test_versions": ["3.12"], + "codeql_languages": ["python"], + "existing_ci": [], + "existing_tools": {}, + "has_pyproject_toml": False, + "has_github_dir": False, + "updated_at": "2026-04-22T00:00:00+00:00", + }, + } + ) + ) + return spec + + +# --------------------------------------------------------------------------- +# AuditFinding unit tests +# --------------------------------------------------------------------------- + + +class TestAuditFinding: + def test_missing_finding_maps_to_correct_rule(self) -> None: + f = AuditFinding(kind="missing", dest="README.md", pack="scaffold") + assert f.rule_id == "pack-drift-missing" + assert "missing" in f.message.lower() + assert "scaffold" in f.message + + def test_changed_finding_maps_to_correct_rule(self) -> None: + f = AuditFinding(kind="changed", dest=".github/workflows/tests.yml", pack="base") + assert f.rule_id == "pack-drift-changed" + assert "differs" in f.message.lower() + + def test_to_sarif_result_carries_fields(self) -> None: + f = AuditFinding(kind="missing", dest="x/y.py", pack="scaffold") + r = f.to_sarif_result() + assert r.rule_id == "pack-drift-missing" + assert r.artifact_uri == "x/y.py" + + +# --------------------------------------------------------------------------- +# findings_to_text / findings_to_sarif unit tests +# --------------------------------------------------------------------------- + + +class TestFindingsToText: + def test_empty_says_ok(self) -> None: + out = findings_to_text([]) + assert "OK" in out + + def test_sections_group_by_kind(self) -> None: + findings = [ + AuditFinding(kind="missing", dest="a", pack="p"), + AuditFinding(kind="changed", dest="b", pack="p"), + AuditFinding(kind="missing", dest="c", pack="p"), + ] + out = findings_to_text(findings) + assert "Missing files (2)" in out + assert "Changed files (1)" in out + assert "- a" in out + assert "- b" in out + assert "- c" in out + + +class TestFindingsToSarif: + def test_empty_produces_valid_report(self) -> None: + r = findings_to_sarif([], tool_name="t", tool_version="v") + d = r.to_dict() + assert d["runs"][0]["results"] == [] + assert d["runs"][0]["tool"]["driver"]["name"] == "t" + + def test_findings_appear_as_results(self) -> None: + findings = [ + AuditFinding(kind="missing", dest="a.py", pack="scaffold"), + AuditFinding(kind="changed", dest="b.py", pack="scaffold"), + ] + r = findings_to_sarif(findings, tool_name="nboot-audit", tool_version="0.1.2") + results = r.to_dict()["runs"][0]["results"] + assert len(results) == 2 + assert {r["ruleId"] for r in results} == {"pack-drift-missing", "pack-drift-changed"} + + +# --------------------------------------------------------------------------- +# run_audit — end-to-end against real packs +# --------------------------------------------------------------------------- + + +class TestRunAudit: + def test_empty_target_flags_every_pack_file_as_missing(self, tmp_path: Path) -> None: + spec = _write_spec(tmp_path) + target = tmp_path / "empty-target" + target.mkdir() + + findings = run_audit(spec, "scaffold", target, skip_resolve=True) + + assert findings # scaffold has at least pyproject.toml + README + LICENSE + assert all(f.kind == "missing" for f in findings) + assert all(f.pack == "scaffold" for f in findings) + + def test_conforming_target_has_no_findings(self, tmp_path: Path) -> None: + # Run nboot new to produce a conforming project, then audit it against + # scaffold — expect zero findings (full conformance). + spec = _write_spec(tmp_path) + target = tmp_path / "conforming" + subprocess.run( + [ + "uv", + "run", + "nboot", + "apply", + "--spec", + str(spec), + "--pack", + "scaffold", + "--target", + str(target), + "--skip-resolve", + ], + check=False, + capture_output=True, + cwd=Path(__file__).resolve().parents[1], + ) + # Engine's apply refuses to write to a non-existent target, so pre-create. + # Instead we use render/apply via the engine directly: + from navi_bootstrap.engine import plan, render_to_files, write_rendered + from navi_bootstrap.manifest import load_manifest + from navi_bootstrap.packs import resolve_pack + from navi_bootstrap.sanitize import sanitize_manifest, sanitize_spec + from navi_bootstrap.spec import load_spec + + target.mkdir() + pack_dir = resolve_pack("scaffold") + spec_data = sanitize_spec(load_spec(spec)) + manifest = sanitize_manifest(load_manifest(pack_dir / "manifest.yaml")) + render_plan = plan(manifest, spec_data, pack_dir / "templates") + rendered = render_to_files( + render_plan, + spec_data, + pack_dir / "templates", + action_shas={}, + action_versions={}, + ) + write_rendered(rendered, target, pack_name="scaffold") + + findings = run_audit(spec, "scaffold", target, skip_resolve=True) + assert findings == [], f"Expected full conformance, got drift: {findings}" + + def test_single_drift_reported_as_changed(self, tmp_path: Path) -> None: + # Render scaffold, mutate one file, audit — expect exactly one "changed". + from navi_bootstrap.engine import plan, render_to_files, write_rendered + from navi_bootstrap.manifest import load_manifest + from navi_bootstrap.packs import resolve_pack + from navi_bootstrap.sanitize import sanitize_manifest, sanitize_spec + from navi_bootstrap.spec import load_spec + + spec = _write_spec(tmp_path) + target = tmp_path / "drifted" + target.mkdir() + + pack_dir = resolve_pack("scaffold") + spec_data = sanitize_spec(load_spec(spec)) + manifest = sanitize_manifest(load_manifest(pack_dir / "manifest.yaml")) + render_plan = plan(manifest, spec_data, pack_dir / "templates") + rendered = render_to_files( + render_plan, + spec_data, + pack_dir / "templates", + action_shas={}, + action_versions={}, + ) + write_rendered(rendered, target, pack_name="scaffold") + + # Introduce drift + readme = target / "README.md" + readme.write_text(readme.read_text() + "\n\n\n") + + findings = run_audit(spec, "scaffold", target, skip_resolve=True) + changed = [f for f in findings if f.kind == "changed"] + assert any(f.dest == "README.md" for f in changed), findings + + def test_unknown_pack_raises_audit_error(self, tmp_path: Path) -> None: + spec = _write_spec(tmp_path) + target = tmp_path + with pytest.raises(AuditError): + run_audit(spec, "no-such-pack-here", target, skip_resolve=True) + + +# --------------------------------------------------------------------------- +# CLI integration +# --------------------------------------------------------------------------- + + +class TestAuditCli: + def test_text_format_default(self, tmp_path: Path) -> None: + spec = _write_spec(tmp_path) + target = tmp_path / "target" + target.mkdir() + runner = CliRunner() + result = runner.invoke( + cli, + ["audit", "--spec", str(spec), "--pack", "scaffold", "--target", str(target)], + ) + # Drift present -> exit 1 by default + assert result.exit_code == 1 + assert "drift" in result.output.lower() or "missing" in result.output.lower() + + def test_sarif_format_produces_valid_json(self, tmp_path: Path) -> None: + spec = _write_spec(tmp_path) + target = tmp_path / "target" + target.mkdir() + runner = CliRunner() + result = runner.invoke( + cli, + [ + "audit", + "--spec", + str(spec), + "--pack", + "scaffold", + "--target", + str(target), + "--format", + "sarif", + ], + ) + assert result.exit_code == 1 # drift -> exit 1 + parsed = json.loads(result.output) + assert parsed["version"] == "2.1.0" + assert parsed["runs"][0]["tool"]["driver"]["name"] == "nboot-audit" + assert parsed["runs"][0]["results"] + + def test_exit_zero_flag_allows_success_with_drift(self, tmp_path: Path) -> None: + spec = _write_spec(tmp_path) + target = tmp_path / "target" + target.mkdir() + runner = CliRunner() + result = runner.invoke( + cli, + [ + "audit", + "--spec", + str(spec), + "--pack", + "scaffold", + "--target", + str(target), + "--exit-zero", + ], + ) + assert result.exit_code == 0 + assert "drift" in result.output.lower() or "missing" in result.output.lower() + + def test_output_file_written(self, tmp_path: Path) -> None: + spec = _write_spec(tmp_path) + target = tmp_path / "target" + target.mkdir() + out_file = tmp_path / "report.sarif.json" + runner = CliRunner() + result = runner.invoke( + cli, + [ + "audit", + "--spec", + str(spec), + "--pack", + "scaffold", + "--target", + str(target), + "--format", + "sarif", + "--output", + str(out_file), + ], + ) + assert result.exit_code == 1 # drift found + assert out_file.exists() + parsed = json.loads(out_file.read_text()) + assert parsed["version"] == "2.1.0" diff --git a/tests/test_sarif.py b/tests/test_sarif.py new file mode 100644 index 0000000..2dc6be8 --- /dev/null +++ b/tests/test_sarif.py @@ -0,0 +1,95 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 Project Navi + +"""Tests for the hand-rolled SARIF 2.1.0 emitter.""" + +from __future__ import annotations + +import json + +import pytest + +from navi_bootstrap.sarif import ( + AUDIT_RULES, + SARIF_SCHEMA_URI, + SARIF_VERSION, + SarifReport, + SarifResult, +) + + +class TestSarifResult: + def test_fingerprint_is_stable(self) -> None: + r1 = SarifResult(rule_id="pack-drift-changed", message="x", artifact_uri="a/b.py") + r2 = SarifResult( + rule_id="pack-drift-changed", message="different msg", artifact_uri="a/b.py" + ) + # Fingerprints must ignore message text to survive wording tweaks + assert r1.fingerprint() == r2.fingerprint() + + def test_fingerprint_differs_for_different_files(self) -> None: + r1 = SarifResult(rule_id="pack-drift-changed", message="x", artifact_uri="a.py") + r2 = SarifResult(rule_id="pack-drift-changed", message="x", artifact_uri="b.py") + assert r1.fingerprint() != r2.fingerprint() + + def test_fingerprint_differs_for_different_rules(self) -> None: + r1 = SarifResult(rule_id="pack-drift-missing", message="x", artifact_uri="a.py") + r2 = SarifResult(rule_id="pack-drift-changed", message="x", artifact_uri="a.py") + assert r1.fingerprint() != r2.fingerprint() + + def test_to_dict_shape(self) -> None: + r = SarifResult(rule_id="pack-drift-missing", message="hi", artifact_uri="x/y.yml") + d = r.to_dict() + assert d["ruleId"] == "pack-drift-missing" + assert d["level"] == "warning" + assert d["message"]["text"] == "hi" + assert d["locations"][0]["physicalLocation"]["artifactLocation"]["uri"] == "x/y.yml" + assert "primaryLocationLineHash" in d["partialFingerprints"] + + +class TestSarifReport: + def test_empty_report_valid_shape(self) -> None: + report = SarifReport(tool_name="nboot-audit", tool_version="0.1.2") + d = report.to_dict() + assert d["$schema"] == SARIF_SCHEMA_URI + assert d["version"] == SARIF_VERSION + assert len(d["runs"]) == 1 + assert d["runs"][0]["tool"]["driver"]["name"] == "nboot-audit" + assert d["runs"][0]["tool"]["driver"]["version"] == "0.1.2" + assert d["runs"][0]["results"] == [] + + def test_rules_are_declared_on_driver(self) -> None: + report = SarifReport(tool_name="nboot-audit", tool_version="0.1.2") + rule_ids = {r["id"] for r in report.to_dict()["runs"][0]["tool"]["driver"]["rules"]} + assert rule_ids == {"pack-drift-missing", "pack-drift-changed"} + + def test_rule_metadata_has_required_sarif_fields(self) -> None: + for rule in AUDIT_RULES: + assert "id" in rule + assert "name" in rule + assert "shortDescription" in rule and "text" in rule["shortDescription"] + assert rule["defaultConfiguration"]["level"] == "warning" + + def test_add_result_rejects_unknown_rule(self) -> None: + report = SarifReport(tool_name="nboot-audit", tool_version="0.1.2") + bad = SarifResult(rule_id="made-up", message="x", artifact_uri="a.py") + with pytest.raises(ValueError, match="Unknown SARIF rule id"): + report.add_result(bad) + + def test_add_result_accepts_known_rule(self) -> None: + report = SarifReport(tool_name="nboot-audit", tool_version="0.1.2") + report.add_result(SarifResult(rule_id="pack-drift-missing", message="m", artifact_uri="a")) + report.add_result(SarifResult(rule_id="pack-drift-changed", message="m", artifact_uri="b")) + assert len(report.to_dict()["runs"][0]["results"]) == 2 + + def test_to_json_is_valid_json_and_round_trips(self) -> None: + report = SarifReport(tool_name="nboot-audit", tool_version="0.1.2") + report.add_result(SarifResult(rule_id="pack-drift-missing", message="m", artifact_uri="a")) + parsed = json.loads(report.to_json()) + assert parsed == report.to_dict() + + def test_to_json_schema_hint_present(self) -> None: + # GitHub's SARIF upload accepts reports without $schema but tooling + # is happier when it's there. + report = SarifReport(tool_name="nboot-audit", tool_version="0.1.2") + assert "$schema" in json.loads(report.to_json()) From 0a12aa362c4b3a1b78ed36b5a57220be353a6a4d Mon Sep 17 00:00:00 2001 From: Nelson Spence Date: Tue, 21 Apr 2026 21:01:55 -0500 Subject: [PATCH 03/10] fix(audit): use canonical manifest pack name (not raw CLI arg) for diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex stop-time review caught a real bug: `resolve_pack()` accepts both a bundled pack name ('base') and a filesystem path ('/abs/path/to/base'), but the append-mode marker block is keyed by the manifest's canonical pack name — which is what `apply()` writes. `run_audit` was passing the raw CLI arg to `compute_diffs(pack_name=...)`. When the user invoked `nboot audit --pack /abs/path/to/some-pack`, the marker-replace regex in `_compute_append_content` looked for `# --- nboot: /abs/path/to/some-pack ---` which never matches what `apply` actually wrote, and every append-mode file then reported false drift. Fix: after `plan()`, pull `render_plan.pack_name` (manifest's 'name' field) and pass THAT to both `compute_diffs` and the AuditFinding. Matches what `apply_cmd` already does (cli.py:360). Regression test `test_pack_filesystem_path_does_not_cause_false_append_drift` renders `base` (which has append-mode entries) via `write_rendered` using the canonical name, then audits the target passing the filesystem path — asserts zero drift and asserts name/path forms produce identical findings. The test skips if `base` ever loses its append entries so the suite stays green without masking regression coverage. --- src/navi_bootstrap/audit.py | 9 +++++-- tests/test_audit.py | 51 +++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/navi_bootstrap/audit.py b/src/navi_bootstrap/audit.py index 0d75e4a..faa2bf8 100644 --- a/src/navi_bootstrap/audit.py +++ b/src/navi_bootstrap/audit.py @@ -136,10 +136,15 @@ def run_audit( except (jinja2.TemplateError, TypeError) as e: raise AuditError(f"Template render error: {e}") from e + # Use the manifest's canonical pack name (what `apply` writes into append + # marker blocks), NOT the raw CLI arg — `resolve_pack` accepts a filesystem + # path and the two forms should not affect drift detection. + canonical_pack_name = render_plan.pack_name + # Diff rendered-in-memory vs existing target filesystem. - diffs = compute_diffs(rendered, target, pack_name=pack) + diffs = compute_diffs(rendered, target, pack_name=canonical_pack_name) - return [_diff_result_to_finding(d, pack) for d in diffs] + return [_diff_result_to_finding(d, canonical_pack_name) for d in diffs] def findings_to_sarif( diff --git a/tests/test_audit.py b/tests/test_audit.py index 5c24562..eae70c8 100644 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -232,6 +232,57 @@ def test_unknown_pack_raises_audit_error(self, tmp_path: Path) -> None: with pytest.raises(AuditError): run_audit(spec, "no-such-pack-here", target, skip_resolve=True) + def test_pack_filesystem_path_does_not_cause_false_append_drift(self, tmp_path: Path) -> None: + """resolve_pack() accepts a filesystem path, but the append-mode + marker block is keyed by the manifest's canonical pack name (what + apply() writes). run_audit must use the canonical name when diffing, + otherwise append-mode files are reported as drifted just because the + CLI arg was a path rather than a bare name. + + Regression guard for Codex stop-time review finding. + """ + from navi_bootstrap.engine import plan, render_to_files, write_rendered + from navi_bootstrap.manifest import load_manifest + from navi_bootstrap.packs import resolve_pack + from navi_bootstrap.sanitize import sanitize_manifest, sanitize_spec + from navi_bootstrap.spec import load_spec + + spec = _write_spec(tmp_path) + target = tmp_path / "applied" + target.mkdir() + + # Pick the base pack — it has append-mode entries in its manifest. + pack_dir = resolve_pack("base") + spec_data = sanitize_spec(load_spec(spec)) + manifest = sanitize_manifest(load_manifest(pack_dir / "manifest.yaml")) + render_plan = plan(manifest, spec_data, pack_dir / "templates") + + # Guard: test is only meaningful when the pack actually has append + # entries; skip otherwise so the suite stays green if base is edited. + if not any(e.mode == "append" for e in render_plan.entries): + pytest.skip("base pack has no append-mode entries; regression not reproducible") + + rendered = render_to_files( + render_plan, + spec_data, + pack_dir / "templates", + action_shas={}, + action_versions={}, + ) + # apply() uses the canonical manifest name for marker blocks. + write_rendered(rendered, target, pack_name=render_plan.pack_name) + + # Now audit passing the FILESYSTEM PATH as the pack argument. + # Before the fix this produced false drift on every append-mode file. + findings_by_path = run_audit(spec, str(pack_dir), target, skip_resolve=True) + findings_by_name = run_audit(spec, "base", target, skip_resolve=True) + + assert findings_by_path == findings_by_name + assert findings_by_path == [], ( + "Expected no drift when auditing a just-applied pack via filesystem " + f"path, got: {findings_by_path}" + ) + # --------------------------------------------------------------------------- # CLI integration From e7527273a090812ab93e4a848e532f381ad3f717 Mon Sep 17 00:00:00 2001 From: Nelson Spence Date: Tue, 21 Apr 2026 23:14:53 -0500 Subject: [PATCH 04/10] fix(audit): path confinement, distinct exit code, flag-agnostic notice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses five real bot findings on PR #51: 1. [Qodo SECURITY] compute_diffs() read boundary now confines paths to the target directory — mirrors engine.write_rendered's symlink / traversal / absolute-path defense. A crafted pack with dest '../escape.txt' or a dest that resolves through an out-of-target symlink now raises ValueError; run_audit() wraps it as AuditError('Path confinement error: ...'). The audit read boundary is now at least as strict as the write boundary. 2. [Copilot] audit_cmd now exits 2 on AuditError (pipeline failure) vs 1 on drift — lets CI distinguish 'audit ran, drift found' from 'audit failed to run'. SystemExit(2) is emitted to stderr and is never suppressed by --exit-zero. Docs table updated to match. 3. [Copilot] _GH_NOTICE reworded to be flag-agnostic — previous text said 'Re-run without --skip-resolve' which misleads audit_cmd users (audit uses --resolve, opposite polarity). Now reads 'Continuing with placeholder action SHAs. Install gh to enable full SHA resolution.' 4. [Copilot] test_conforming_target_has_no_findings dropped the vestigial 'uv run nboot apply ...' subprocess.run call whose result was ignored anyway. The in-process render+write+audit setup is now the only path — faster test, no PATH dependency, no dead code. 5. [Qodo / Codex] Pack-name fix was already in HEAD from 0bc5d4f — both bots re-scanned the pre-fix commit. No action needed. Skipped (low-value style nits): - cli.py __all__ add — file has no __all__, so not reachable - action_shas robustness — schema already tightens shape - logging/audit-trail — deferrable - atomic output-file writes — edge case - 'race condition if target mutates mid-audit' — documentation only Regression tests (4 new in test_audit.py): - test_symlink_escape_is_rejected - test_traversal_dest_is_rejected - test_confinement_violation_surfaces_as_audit_error - test_pipeline_error_exits_with_code_2 412 tests pass, mypy strict clean (14 files), ruff + format clean. --- docs/reference/audit.md | 5 +- src/navi_bootstrap/audit.py | 9 ++- src/navi_bootstrap/cli.py | 9 ++- src/navi_bootstrap/diff.py | 22 +++++++ tests/test_audit.py | 122 ++++++++++++++++++++++++++++-------- 5 files changed, 137 insertions(+), 30 deletions(-) diff --git a/docs/reference/audit.md b/docs/reference/audit.md index 18c17fe..f179d32 100644 --- a/docs/reference/audit.md +++ b/docs/reference/audit.md @@ -103,7 +103,10 @@ Upload it in CI: |---|---| | 0 | Target fully conforms to the pack, OR drift found with `--exit-zero` | | 1 | Drift found without `--exit-zero` | -| >1 | Pipeline error (bad spec, missing pack, template render failure) | +| 2 | Pipeline error (bad spec, missing pack, path-confinement violation, template render failure). Always emitted to stderr, never suppressed by `--exit-zero` | + +Exit 1 vs 2 lets CI distinguish "the audit ran and reported drift" from "the +audit failed to run". Wire your pipeline so only exit 1 gates the merge. ## Relationship to other verbs diff --git a/src/navi_bootstrap/audit.py b/src/navi_bootstrap/audit.py index faa2bf8..7d5f325 100644 --- a/src/navi_bootstrap/audit.py +++ b/src/navi_bootstrap/audit.py @@ -141,8 +141,13 @@ def run_audit( # path and the two forms should not affect drift detection. canonical_pack_name = render_plan.pack_name - # Diff rendered-in-memory vs existing target filesystem. - diffs = compute_diffs(rendered, target, pack_name=canonical_pack_name) + # Diff rendered-in-memory vs existing target filesystem. compute_diffs + # raises ValueError on path-confinement / symlink-escape; surface that as + # an AuditError so callers get one error type. + try: + diffs = compute_diffs(rendered, target, pack_name=canonical_pack_name) + except ValueError as e: + raise AuditError(f"Path confinement error: {e}") from e return [_diff_result_to_finding(d, canonical_pack_name) for d in diffs] diff --git a/src/navi_bootstrap/cli.py b/src/navi_bootstrap/cli.py index 9de9a51..b9ca36b 100644 --- a/src/navi_bootstrap/cli.py +++ b/src/navi_bootstrap/cli.py @@ -30,7 +30,8 @@ _GH_NOTICE = ( "Notice: gh CLI not found — SHA resolution requires gh " "(https://cli.github.com).\n" - " Action SHAs left as placeholders. Re-run without --skip-resolve after installing gh." + " Continuing with placeholder action SHAs. Install gh to enable " + "full SHA resolution." ) @@ -439,7 +440,11 @@ def audit_cmd( try: findings = run_audit(spec, pack, target, skip_resolve=skip_resolve) except AuditError as e: - raise click.ClickException(str(e)) from e + # Exit 2 (distinct from drift=1) so CI can tell "audit ran; drift + # found" apart from "audit failed to run". Never suppressed by + # --exit-zero since pipeline errors must always be visible. + click.echo(f"Audit pipeline error: {e}", err=True) + raise SystemExit(2) from e if output_format == "sarif": report = findings_to_sarif(findings, tool_name="nboot-audit", tool_version=__version__) diff --git a/src/navi_bootstrap/diff.py b/src/navi_bootstrap/diff.py index 367284f..d3d2497 100644 --- a/src/navi_bootstrap/diff.py +++ b/src/navi_bootstrap/diff.py @@ -66,13 +66,35 @@ def compute_diffs( Returns a list of DiffResult for files that would change. Unchanged files are omitted. + + Path confinement: every destination must resolve inside ``target``. A + crafted pack/spec with an absolute path, traversal (``..``), or a + symlink pointing outside the target is rejected with ``ValueError``. + This mirrors the write-side defense in ``engine.write_rendered`` — the + read boundary should be at least as strict as the write boundary. """ results: list[DiffResult] = [] + target_resolved = target.resolve() + for rf in rendered_files: file_path = target / rf.dest + + # Pre-existence confinement check: even for new files we must not + # follow a traversal or absolute path that would escape the target. + try: + file_path.resolve().relative_to(target_resolved) + except ValueError: + raise ValueError(f"Path escapes outside target directory: {rf.dest}") from None + is_new = not file_path.exists() + # Post-existence symlink defense: a pre-existing symlink at + # file_path that points outside target would otherwise let a + # crafted pack read an arbitrary file during the diff. + if not is_new and file_path.resolve() != (target / rf.dest).resolve(): + raise ValueError(f"Path escapes outside target directory (symlink): {rf.dest}") + if is_new: # New file: diff against empty # For append mode, wrap in marker blocks like the engine would diff --git a/tests/test_audit.py b/tests/test_audit.py index eae70c8..5ebce20 100644 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -6,7 +6,6 @@ from __future__ import annotations import json -import subprocess from pathlib import Path import pytest @@ -146,36 +145,15 @@ def test_empty_target_flags_every_pack_file_as_missing(self, tmp_path: Path) -> assert all(f.pack == "scaffold" for f in findings) def test_conforming_target_has_no_findings(self, tmp_path: Path) -> None: - # Run nboot new to produce a conforming project, then audit it against - # scaffold — expect zero findings (full conformance). - spec = _write_spec(tmp_path) - target = tmp_path / "conforming" - subprocess.run( - [ - "uv", - "run", - "nboot", - "apply", - "--spec", - str(spec), - "--pack", - "scaffold", - "--target", - str(target), - "--skip-resolve", - ], - check=False, - capture_output=True, - cwd=Path(__file__).resolve().parents[1], - ) - # Engine's apply refuses to write to a non-existent target, so pre-create. - # Instead we use render/apply via the engine directly: + # Render scaffold in-process, then audit it — expect zero findings. from navi_bootstrap.engine import plan, render_to_files, write_rendered from navi_bootstrap.manifest import load_manifest from navi_bootstrap.packs import resolve_pack from navi_bootstrap.sanitize import sanitize_manifest, sanitize_spec from navi_bootstrap.spec import load_spec + spec = _write_spec(tmp_path) + target = tmp_path / "conforming" target.mkdir() pack_dir = resolve_pack("scaffold") spec_data = sanitize_spec(load_spec(spec)) @@ -284,6 +262,74 @@ def test_pack_filesystem_path_does_not_cause_false_append_drift(self, tmp_path: ) +class TestAuditPathConfinement: + """compute_diffs (via run_audit) must refuse to read outside the target. + + Qodo code-review finding on PR #51: the audit read boundary must be at + least as strict as the engine's write boundary — a crafted pack/spec + with a traversal, absolute path, or symlink pointing outside the target + should raise rather than silently read arbitrary files. + """ + + def test_symlink_escape_is_rejected(self, tmp_path: Path) -> None: + from navi_bootstrap.diff import compute_diffs + from navi_bootstrap.engine import RenderedFile + + secret_area = tmp_path / "outside" + secret_area.mkdir() + (secret_area / "secret.txt").write_text("leaked") + + target = tmp_path / "target" + target.mkdir() + # A symlink inside target pointing at the secret file outside target. + escape = target / "escape.txt" + escape.symlink_to(secret_area / "secret.txt") + + # A crafted pack "render" whose dest is the symlink. + rendered = [RenderedFile(dest="escape.txt", content="anything", mode="create")] + + with pytest.raises(ValueError, match="escapes outside target"): + compute_diffs(rendered, target, pack_name="crafted") + + def test_traversal_dest_is_rejected(self, tmp_path: Path) -> None: + from navi_bootstrap.diff import compute_diffs + from navi_bootstrap.engine import RenderedFile + + target = tmp_path / "target" + target.mkdir() + # Crafted pack with a '..' escape in dest. + rendered = [ + RenderedFile(dest="../escaped.txt", content="x", mode="create"), + ] + with pytest.raises(ValueError, match="escapes outside target"): + compute_diffs(rendered, target, pack_name="crafted") + + def test_confinement_violation_surfaces_as_audit_error(self, tmp_path: Path) -> None: + """run_audit should wrap ValueError from compute_diffs into AuditError + so callers get a single error type.""" + # Monkey-patch the engine's render step to emit a traversal dest — + # simulates a hostile pack without needing to author one on disk. + import navi_bootstrap.audit as audit_mod + from navi_bootstrap.audit import run_audit as _run_audit + from navi_bootstrap.diff import compute_diffs as _compute_diffs # noqa: F401 + from navi_bootstrap.engine import RenderedFile + + spec = _write_spec(tmp_path) + target = tmp_path / "target" + target.mkdir() + + def _hostile_render_to_files(*args, **kwargs): # type: ignore[no-untyped-def] + return [RenderedFile(dest="../escape.txt", content="x", mode="create")] + + original = audit_mod.render_to_files + audit_mod.render_to_files = _hostile_render_to_files + try: + with pytest.raises(AuditError, match="Path confinement error"): + _run_audit(spec, "scaffold", target, skip_resolve=True) + finally: + audit_mod.render_to_files = original + + # --------------------------------------------------------------------------- # CLI integration # --------------------------------------------------------------------------- @@ -349,6 +395,32 @@ def test_exit_zero_flag_allows_success_with_drift(self, tmp_path: Path) -> None: assert result.exit_code == 0 assert "drift" in result.output.lower() or "missing" in result.output.lower() + def test_pipeline_error_exits_with_code_2(self, tmp_path: Path) -> None: + """AuditError must exit 2, not 1, so CI can tell drift apart from + pipeline failure. Never suppressed by --exit-zero.""" + spec = _write_spec(tmp_path) + target = tmp_path / "target" + target.mkdir() + runner = CliRunner() + # Unknown pack triggers AuditError inside run_audit. + result = runner.invoke( + cli, + [ + "audit", + "--spec", + str(spec), + "--pack", + "no-such-pack-exists", + "--target", + str(target), + "--exit-zero", # must NOT suppress pipeline errors + ], + ) + assert result.exit_code == 2, ( + f"Expected exit 2 for pipeline error, got {result.exit_code}: " + f"stdout={result.output!r} stderr={getattr(result, 'stderr_bytes', b'')!r}" + ) + def test_output_file_written(self, tmp_path: Path) -> None: spec = _write_spec(tmp_path) target = tmp_path / "target" From 360f10366162d0c8695c90485615c049a74e7602 Mon Sep 17 00:00:00 2001 From: Nelson Spence Date: Tue, 21 Apr 2026 23:17:42 -0500 Subject: [PATCH 05/10] fix(diff): wrap compute_diffs ValueError; drop dead symlink self-compare MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex stop-time review caught two bugs I introduced in 5fe1668 when adding path-confinement to compute_diffs: 1. `nboot diff` now had a new uncaught ValueError path. compute_diffs raises ValueError on traversal / symlink / absolute-path escape, but diff_cmd in cli.py called it without a try/except. A crafted pack or a symlinked target would surface a raw Python traceback instead of a clean ClickException. run_audit already wrapped this correctly; diff_cmd did not. Fix: wrap the compute_diffs call in diff_cmd with try/except ValueError → ClickException('Path confinement error: ...'). 2. The 'post-existence symlink defense' block I added was a no-op. The comparison `file_path.resolve() != (target / rf.dest).resolve()` reduces to comparing .resolve() against itself — file_path IS target/rf.dest, so the two expressions always yield the same value. The real confinement work is done by the relative_to() check above it. Removed the dead block (and noted: the analogous pattern in engine.write_rendered is also a no-op, but that's pre-existing cleanup for another PR). Regression test in test_audit.py::TestAuditCli:: test_diff_cmd_friendly_error_on_confinement: - Builds a symlink escape inside target - Monkey-patches the engine's render to emit a dest hitting it - Runs `nboot diff` via CliRunner - Asserts exit 1, 'Path confinement error' in output, no 'Traceback' 413 tests pass (+1 regression), mypy strict clean, ruff + format clean. All 3 existing path-confinement tests still pass after removing the dead block — confirming that relative_to() alone does the work. --- src/navi_bootstrap/cli.py | 9 +++++-- src/navi_bootstrap/diff.py | 6 ----- tests/test_audit.py | 50 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 8 deletions(-) diff --git a/src/navi_bootstrap/cli.py b/src/navi_bootstrap/cli.py index b9ca36b..a7c1f43 100644 --- a/src/navi_bootstrap/cli.py +++ b/src/navi_bootstrap/cli.py @@ -357,8 +357,13 @@ def diff_cmd(spec: Path, pack: str, target: Path, skip_resolve: bool) -> None: except (jinja2.TemplateError, TypeError) as e: raise click.ClickException(f"Template error: {e}") from e - # Compute diffs - diffs = compute_diffs(rendered_files, target, pack_name=render_plan.pack_name) + # Compute diffs — path-confinement violations surface as ValueError from + # compute_diffs; convert to a clean ClickException so users see a one-line + # error instead of a Python traceback on crafted / symlinked targets. + try: + diffs = compute_diffs(rendered_files, target, pack_name=render_plan.pack_name) + except ValueError as e: + raise click.ClickException(f"Path confinement error: {e}") from e if not diffs: click.echo("No changes — target is up to date.") diff --git a/src/navi_bootstrap/diff.py b/src/navi_bootstrap/diff.py index d3d2497..a9600c5 100644 --- a/src/navi_bootstrap/diff.py +++ b/src/navi_bootstrap/diff.py @@ -89,12 +89,6 @@ def compute_diffs( is_new = not file_path.exists() - # Post-existence symlink defense: a pre-existing symlink at - # file_path that points outside target would otherwise let a - # crafted pack read an arbitrary file during the diff. - if not is_new and file_path.resolve() != (target / rf.dest).resolve(): - raise ValueError(f"Path escapes outside target directory (symlink): {rf.dest}") - if is_new: # New file: diff against empty # For append mode, wrap in marker blocks like the engine would diff --git a/tests/test_audit.py b/tests/test_audit.py index 5ebce20..e31d8a2 100644 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -395,6 +395,56 @@ def test_exit_zero_flag_allows_success_with_drift(self, tmp_path: Path) -> None: assert result.exit_code == 0 assert "drift" in result.output.lower() or "missing" in result.output.lower() + def test_diff_cmd_friendly_error_on_confinement(self, tmp_path: Path) -> None: + """Regression: `nboot diff` must turn compute_diffs's path-confinement + ValueError into a ClickException (one-line error), not a bare + traceback. Codex stop-time review flagged 5fe1668 as introducing an + uncaught ValueError in diff_cmd. + """ + # Build a symlink-escape target and monkey-patch the engine's render + # step to emit a dest hitting it. Exercises the same boundary as audit + # but via the `diff` CLI verb. + import navi_bootstrap.cli as cli_mod + from navi_bootstrap.engine import RenderedFile + + secret_area = tmp_path / "outside" + secret_area.mkdir() + (secret_area / "secret.txt").write_text("leaked") + target = tmp_path / "target" + target.mkdir() + escape = target / "escape.txt" + escape.symlink_to(secret_area / "secret.txt") + + spec = _write_spec(tmp_path) + + def _hostile_render_to_files(*args, **kwargs): # type: ignore[no-untyped-def] + return [RenderedFile(dest="escape.txt", content="x", mode="create")] + + original = cli_mod.render_to_files + cli_mod.render_to_files = _hostile_render_to_files + try: + runner = CliRunner() + result = runner.invoke( + cli, + [ + "diff", + "--spec", + str(spec), + "--pack", + "scaffold", + "--target", + str(target), + "--skip-resolve", + ], + ) + finally: + cli_mod.render_to_files = original + + # ClickException exits 1 with a clean 'Error: ...' line, no traceback. + assert result.exit_code == 1 + assert "Path confinement error" in result.output + assert "Traceback" not in result.output + def test_pipeline_error_exits_with_code_2(self, tmp_path: Path) -> None: """AuditError must exit 2, not 1, so CI can tell drift apart from pipeline failure. Never suppressed by --exit-zero.""" From e6e9d11e4079f1ccd4664f4f68a00a8965e363bb Mon Sep 17 00:00:00 2001 From: Nelson Spence Date: Sun, 26 Apr 2026 15:22:46 -0500 Subject: [PATCH 06/10] docs(audit): document __version__ fallback + audit threat model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two non-blocking Grippy advisories on PR #51: 1. [LOW __init__.py:9] Document why __version__ may fall back to '0.0.0+unknown' — happens only when the package isn't installed (editable-install dev sessions where the package was deleted, or a source checkout being imported via PYTHONPATH). Adds a comment explaining the rationale + flagging that downstream consumers (notably SARIF tool.driver.version) must tolerate the form. 2. [HIGH diff.py:72 / docs/reference/audit.md:94] Add an 'Operational notes' section to the audit reference doc covering: - TOCTOU window: path-confinement check is a snapshot - Mid-run symlink creation by other processes - Privilege guidance — run as low as can read the target Includes the realistic 'most CI usage doesn't hit this' framing. No code changes — pure doc polish. 413 tests still pass, mypy strict clean, ruff + format clean. --- docs/reference/audit.md | 25 +++++++++++++++++++++++++ src/navi_bootstrap/__init__.py | 9 +++++++++ 2 files changed, 34 insertions(+) diff --git a/docs/reference/audit.md b/docs/reference/audit.md index f179d32..6364376 100644 --- a/docs/reference/audit.md +++ b/docs/reference/audit.md @@ -97,6 +97,31 @@ Upload it in CI: category: nboot-audit ``` +## Threat model and operational notes + +`nboot audit` is a **defence-in-depth** tool, not a hardened sandbox. The +path-confinement check in `compute_diffs` resolves every destination +relative to the target directory and rejects traversal, absolute paths, +and symlink escapes — but it operates at the moment the audit runs. + +Known limits: + +- **TOCTOU.** The check resolves paths once; in a shared or mutable + environment a path may flip from safe to unsafe between the check + and any subsequent read. For audits that matter (CI gates, fleet + surveys), run against a freshly-cloned working tree or a read-only + mount. +- **Chained symlinks created mid-run.** If another process creates new + symlinks under `--target` while audit is iterating, files added after + the resolve check are not re-confined. Same mitigation: avoid running + audit on a directory another process is actively writing to. +- **Privilege.** Run audit with the lowest privilege that can read the + target. Don't run as root unless the target requires it. + +For most CI usage — clone, audit, exit — these limits don't apply. They +only matter if the audit is exposed to an attacker who can mutate the +target while audit is running. + ## Exit codes | Exit | Meaning | diff --git a/src/navi_bootstrap/__init__.py b/src/navi_bootstrap/__init__.py index 85d1319..54a94d8 100644 --- a/src/navi_bootstrap/__init__.py +++ b/src/navi_bootstrap/__init__.py @@ -9,6 +9,15 @@ from navi_bootstrap.packs import get_ordered_packs from navi_bootstrap.spec import build_spec_for_new +# Single-source the version from installed package metadata so __init__.py +# tracks pyproject.toml automatically. The fallback only triggers when the +# package isn't installed (uncommon: editable-install dev sessions where the +# package was deleted, or a source checkout being imported via PYTHONPATH +# without `pip install -e .`). Downstream consumers — notably the SARIF +# `tool.driver.version` field emitted by `nboot audit` — must tolerate the +# `0.0.0+unknown` form. The pragma excludes it from coverage because it +# only fires in that uncommon dev configuration and isn't worth simulating +# in tests. try: __version__ = _pkg_version("navi-bootstrap") except PackageNotFoundError: # pragma: no cover — only during dev without install From e0e83d4b8afb5f717e436447a1efb5d4122cffea Mon Sep 17 00:00:00 2001 From: Nelson Spence Date: Sun, 26 Apr 2026 15:31:43 -0500 Subject: [PATCH 07/10] fix(audit): catch ValueError from plan/render in run_audit Codex P1 review finding on PR #51: `run_audit` documented an exit-2 contract for all pipeline-stage failures and audit_cmd relied on it, but the except clause around `engine.plan()` only caught TemplateError and TypeError. plan() can raise ValueError (e.g., when a loop expands past _MAX_LOOP_ITEMS), and that escaped as a raw traceback instead of a clean AuditError. Fix: extend both plan() and render_to_files() except clauses to include ValueError. The compute_diffs() ValueError path already had its own wrapping (path confinement) and continues to surface as 'Path confinement error: ...' specifically. Regression test `test_plan_value_error_surfaces_as_audit_error` monkey-patches plan() to raise ValueError and asserts the AuditError wrap (and its 'Template planning error' message prefix) appears. 414 tests pass (+1 regression), mypy strict clean, ruff + format clean. --- src/navi_bootstrap/audit.py | 13 +++++++++---- tests/test_audit.py | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/navi_bootstrap/audit.py b/src/navi_bootstrap/audit.py index 7d5f325..2c163da 100644 --- a/src/navi_bootstrap/audit.py +++ b/src/navi_bootstrap/audit.py @@ -117,14 +117,19 @@ def run_audit( except ResolveError as e: raise AuditError(str(e)) from e - # Stage 2 — plan. + # Stage 2 — plan. plan() can raise ValueError on _MAX_LOOP_ITEMS overflow + # in addition to Jinja2 TemplateError / TypeError; all three must surface + # as AuditError so audit_cmd's exit-code-2 contract holds (no raw + # tracebacks). templates_dir = pack_dir / "templates" try: render_plan = plan(manifest, spec_data, templates_dir) - except (jinja2.TemplateError, TypeError) as e: + except (jinja2.TemplateError, TypeError, ValueError) as e: raise AuditError(f"Template planning error: {e}") from e - # Stage 3 — render to memory (no filesystem writes). + # Stage 3 — render to memory (no filesystem writes). render_to_files can + # also raise ValueError (e.g. duplicate dest in create mode is enforced + # downstream but ValueError can leak up through Jinja extensions). try: rendered = render_to_files( render_plan, @@ -133,7 +138,7 @@ def run_audit( action_shas=shas, action_versions=versions, ) - except (jinja2.TemplateError, TypeError) as e: + except (jinja2.TemplateError, TypeError, ValueError) as e: raise AuditError(f"Template render error: {e}") from e # Use the manifest's canonical pack name (what `apply` writes into append diff --git a/tests/test_audit.py b/tests/test_audit.py index e31d8a2..ff0bc8a 100644 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -261,6 +261,31 @@ def test_pack_filesystem_path_does_not_cause_false_append_drift(self, tmp_path: f"path, got: {findings_by_path}" ) + def test_plan_value_error_surfaces_as_audit_error(self, tmp_path: Path) -> None: + """run_audit must wrap ValueError from engine.plan() (e.g. loop + expansion exceeding _MAX_LOOP_ITEMS) as AuditError so audit_cmd's + exit-code-2 contract holds. + + Regression guard for Codex P1 finding on PR #51: plan() can raise + ValueError but the original except only caught TemplateError/TypeError. + """ + import navi_bootstrap.audit as audit_mod + + spec = _write_spec(tmp_path) + target = tmp_path / "target" + target.mkdir() + + def _hostile_plan(*args, **kwargs): # type: ignore[no-untyped-def] + raise ValueError("Loop over 'spec.exploit' has 99999 items (max 1000)") + + original = audit_mod.plan + audit_mod.plan = _hostile_plan + try: + with pytest.raises(AuditError, match="Template planning error"): + run_audit(spec, "scaffold", target, skip_resolve=True) + finally: + audit_mod.plan = original + class TestAuditPathConfinement: """compute_diffs (via run_audit) must refuse to read outside the target. From ebdb07f2cd1d8c93a4b3271c03150dabd76c114a Mon Sep 17 00:00:00 2001 From: Nelson Spence Date: Sun, 26 Apr 2026 18:30:06 -0500 Subject: [PATCH 08/10] fix(audit): address Grippy HIGH + Copilot findings on PR #51 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 5 distinct findings across 3 reviewers, batched here: 1. [Copilot sarif.py:42, 58] SARIF rule fullDescription text was telling users to run `nboot apply --pack --target ` and `nboot diff --pack --target ` — both omit the required --spec flag, so the suggested commands would error out. Both texts now include `--spec nboot-spec.json` so the suggested remediation actually works. 2. [Copilot sarif.py:116] add_result() rebuilt `known_ids` set from self.rules on every call. Audits with thousands of findings would spend cumulative time recomputing the same set. Cached as `_known_rule_ids: frozenset[str] | None` populated lazily on first add_result(). Hidden from to_dict / repr / compare via field flags. 3. [Grippy HIGH diff.py:66, 72] TOCTOU + chained-symlink concern. Code had no inline comment about the threat-model caveat (only the docs added in 0122244 mentioned it). Added an explicit comment block in compute_diffs explaining that resolve() walks the full symlink chain (catches arbitrary-depth + relative links) but is a single-resolve snapshot vulnerable to mid-run mutation. 4. [Grippy HIGH 'retest with chained and relative links'] Two new regression tests prove the existing resolve()-based check catches: - test_chained_symlink_escape_is_rejected (target/a -> target/b -> outside/secret) - test_relative_symlink_escape_is_rejected (target/escape -> ../outside/secret, relative target) 5. [github-code-quality test_audit.py:272/337/432] Mixed import style: inside test bodies the module was re-imported as 'audit_mod' / 'cli_mod' even though the same names had top-level 'from … import …'. Hoisted module-level `import navi_bootstrap.audit as audit_mod` and `import navi_bootstrap.cli as cli_mod` so the file uses each module in exactly one import style. Skipped from this batch: - Grippy MEDIUM 'no line/region context in SARIF' — fundamental design choice (drift is per-file not per-line); revisit when Mode B (invisible-injection scan) lands and needs span data. - Grippy LOW '__version__ fallback' — already addressed in 0122244. 416 tests pass (+2 regression), mypy strict clean, ruff + format clean. --- src/navi_bootstrap/diff.py | 10 ++++++++ src/navi_bootstrap/sarif.py | 30 ++++++++++++++++------ tests/test_audit.py | 51 ++++++++++++++++++++++++++++++++++--- 3 files changed, 80 insertions(+), 11 deletions(-) diff --git a/src/navi_bootstrap/diff.py b/src/navi_bootstrap/diff.py index a9600c5..504c9b9 100644 --- a/src/navi_bootstrap/diff.py +++ b/src/navi_bootstrap/diff.py @@ -82,6 +82,16 @@ def compute_diffs( # Pre-existence confinement check: even for new files we must not # follow a traversal or absolute path that would escape the target. + # Path.resolve() follows the entire symlink chain, so this catches + # arbitrary-depth chained symlinks and relative-target symlinks + # alike — the resolved real path either lives under target_resolved + # or it doesn't. + # + # Threat-model caveat: this is a single-resolve TOCTOU snapshot. A + # process mutating the target between resolve() and any subsequent + # read can defeat it. See docs/reference/audit.md "Threat model and + # operational notes" — run audit on a fresh checkout or read-only + # mount when the target is exposed to untrusted writers. try: file_path.resolve().relative_to(target_resolved) except ValueError: diff --git a/src/navi_bootstrap/sarif.py b/src/navi_bootstrap/sarif.py index 04067ef..9420a18 100644 --- a/src/navi_bootstrap/sarif.py +++ b/src/navi_bootstrap/sarif.py @@ -39,8 +39,9 @@ "fullDescription": { "text": ( "The target project is missing a file the conformance pack " - "would render. Run `nboot apply --pack --target ` " - "to create it, or mark the pack as optional for this project." + "would render. Run `nboot apply --spec nboot-spec.json " + "--pack --target ` to create it, or mark the " + "pack as optional for this project." ), }, "helpUri": "https://github.com/Project-Navi/navi-bootstrap/blob/main/docs/reference/audit.md", @@ -55,8 +56,9 @@ "fullDescription": { "text": ( "The target project has a file whose content no longer matches " - "the conformance pack. Review `nboot diff --pack --target ` " - "for the unified diff; `nboot apply` will overwrite (create mode) " + "the conformance pack. Review `nboot diff --spec nboot-spec.json " + "--pack --target ` for the unified diff; " + "`nboot apply` (with the same flags) will overwrite (create mode) " "or merge (append mode)." ), }, @@ -106,14 +108,26 @@ class SarifReport: tool_version: str results: list[SarifResult] = field(default_factory=list) rules: tuple[dict[str, Any], ...] = AUDIT_RULES + # Cache of valid rule ids for O(1) add_result validation. Populated lazily + # by add_result so the dataclass stays trivially constructible (e.g. tests + # that build a SarifReport without going through any factory). Never + # exposed in to_dict / to_json output. + _known_rule_ids: frozenset[str] | None = field(default=None, repr=False, compare=False) def add_result(self, result: SarifResult) -> None: # Defensive: reject results referencing unknown rules rather than emitting - # invalid SARIF that GitHub would silently drop. - known_ids = {rule["id"] for rule in self.rules} - if result.rule_id not in known_ids: + # invalid SARIF that GitHub would silently drop. The set is computed + # once and cached — audits with thousands of findings would otherwise + # rebuild it on every call. + if self._known_rule_ids is None: + object.__setattr__( + self, "_known_rule_ids", frozenset(rule["id"] for rule in self.rules) + ) + assert self._known_rule_ids is not None # narrows for mypy + if result.rule_id not in self._known_rule_ids: raise ValueError( - f"Unknown SARIF rule id {result.rule_id!r}; expected one of {sorted(known_ids)}" + f"Unknown SARIF rule id {result.rule_id!r}; " + f"expected one of {sorted(self._known_rule_ids)}" ) self.results.append(result) diff --git a/tests/test_audit.py b/tests/test_audit.py index ff0bc8a..4671170 100644 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -11,6 +11,8 @@ import pytest from click.testing import CliRunner +import navi_bootstrap.audit as audit_mod +import navi_bootstrap.cli as cli_mod from navi_bootstrap.audit import ( AuditError, AuditFinding, @@ -269,7 +271,6 @@ def test_plan_value_error_surfaces_as_audit_error(self, tmp_path: Path) -> None: Regression guard for Codex P1 finding on PR #51: plan() can raise ValueError but the original except only caught TemplateError/TypeError. """ - import navi_bootstrap.audit as audit_mod spec = _write_spec(tmp_path) target = tmp_path / "target" @@ -329,12 +330,57 @@ def test_traversal_dest_is_rejected(self, tmp_path: Path) -> None: with pytest.raises(ValueError, match="escapes outside target"): compute_diffs(rendered, target, pack_name="crafted") + def test_chained_symlink_escape_is_rejected(self, tmp_path: Path) -> None: + """Path.resolve() walks the entire symlink chain, so a multi-hop + link (target/a → target/b → outside/secret) must still be caught. + + Regression guard for Grippy HIGH advisory on PR #51 ("retest with + chained and relative links"). + """ + from navi_bootstrap.diff import compute_diffs + from navi_bootstrap.engine import RenderedFile + + secret_area = tmp_path / "outside" + secret_area.mkdir() + (secret_area / "secret.txt").write_text("leaked") + + target = tmp_path / "target" + target.mkdir() + # Chain: a → b → outside/secret.txt + (target / "b").symlink_to(secret_area / "secret.txt") + (target / "a").symlink_to(target / "b") + + rendered = [RenderedFile(dest="a", content="x", mode="create")] + with pytest.raises(ValueError, match="escapes outside target"): + compute_diffs(rendered, target, pack_name="crafted") + + def test_relative_symlink_escape_is_rejected(self, tmp_path: Path) -> None: + """Relative-target symlinks (target/escape → ../outside/secret) must + also be caught — Path.resolve() handles relative resolution. + + Regression guard for Grippy HIGH advisory on PR #51. + """ + from navi_bootstrap.diff import compute_diffs + from navi_bootstrap.engine import RenderedFile + + secret_area = tmp_path / "outside" + secret_area.mkdir() + (secret_area / "secret.txt").write_text("leaked") + + target = tmp_path / "target" + target.mkdir() + # Relative symlink crossing out and back in is still an escape. + (target / "escape").symlink_to(Path("../outside/secret.txt")) + + rendered = [RenderedFile(dest="escape", content="x", mode="create")] + with pytest.raises(ValueError, match="escapes outside target"): + compute_diffs(rendered, target, pack_name="crafted") + def test_confinement_violation_surfaces_as_audit_error(self, tmp_path: Path) -> None: """run_audit should wrap ValueError from compute_diffs into AuditError so callers get a single error type.""" # Monkey-patch the engine's render step to emit a traversal dest — # simulates a hostile pack without needing to author one on disk. - import navi_bootstrap.audit as audit_mod from navi_bootstrap.audit import run_audit as _run_audit from navi_bootstrap.diff import compute_diffs as _compute_diffs # noqa: F401 from navi_bootstrap.engine import RenderedFile @@ -429,7 +475,6 @@ def test_diff_cmd_friendly_error_on_confinement(self, tmp_path: Path) -> None: # Build a symlink-escape target and monkey-patch the engine's render # step to emit a dest hitting it. Exercises the same boundary as audit # but via the `diff` CLI verb. - import navi_bootstrap.cli as cli_mod from navi_bootstrap.engine import RenderedFile secret_area = tmp_path / "outside" From 787fe1d9653c2e7f9afe0ab3e87b51c20e787987 Mon Sep 17 00:00:00 2001 From: Nelson Spence Date: Sun, 26 Apr 2026 18:56:09 -0500 Subject: [PATCH 09/10] fix(audit): kind enum validation, monkeypatch fixture, TOCTOU docstring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Latest sweep across Grippy + Copilot + github-code-quality findings on PR #51. Six addressed: 1. [Grippy MEDIUM audit.py:51] AuditFinding.kind was an unrestricted str — a typo'd value would raise KeyError when rule_id was accessed. Tightened to Literal['missing', 'changed'] + __post_init__ that rejects any other value with a clear ValueError. mypy enforces at type-check time, runtime check catches dynamic constructions. 2. [Copilot] AuditFinding.message remediation hints now include --spec nboot-spec.json (paralleling the SARIF fix in 9242ee3). 'nboot apply' and 'nboot diff' both require --spec; the previous suggestion was un-runnable. 3. [Grippy HIGH diff.py — third repeat] Reinforced the TOCTOU caveat: added a Sphinx '.. warning::' block to compute_diffs's docstring (so it surfaces in IDE help() and any future generated API docs) and a paragraph in audit_cmd's CLI --help text. The threat-model doc section was already in audit.md. 4. [Grippy LOW cli.py newline] Centralised the trailing-newline contract: SarifReport.to_json now appends '\n', findings_to_text already did. audit_cmd just forwards the rendered string verbatim (echo with nl=False; write_text without conditional fixup). 5. [github-code-quality test_audit.py imports] Dropped the 'import x as alias' lines entirely. Switched the three monkey-patch tests to pytest's monkeypatch fixture — auto-restoring, idiomatic, and clears the mixed-import lint. 6. [new regression coverage] - test_invalid_kind_rejected_at_construction (kind validation) - test_message_includes_spec_flag_for_apply_remediation (paths the runnable suggestion through both kinds) Skipped from the same review batch: - Grippy LOW 'enhance error messages with hints' — vague, deferrable - Grippy LOW 'no explicit no-drift message when --output' — edge case - Grippy MEDIUM 'exit codes praise' — advisory only 418 tests pass (+3), mypy strict clean, ruff + format clean. --- src/navi_bootstrap/audit.py | 40 ++++++++++++---- src/navi_bootstrap/cli.py | 16 +++++-- src/navi_bootstrap/diff.py | 23 ++++++++-- src/navi_bootstrap/sarif.py | 5 +- tests/test_audit.py | 91 +++++++++++++++++++++---------------- 5 files changed, 116 insertions(+), 59 deletions(-) diff --git a/src/navi_bootstrap/audit.py b/src/navi_bootstrap/audit.py index 2c163da..b78a592 100644 --- a/src/navi_bootstrap/audit.py +++ b/src/navi_bootstrap/audit.py @@ -18,6 +18,7 @@ from dataclasses import dataclass from pathlib import Path +from typing import Literal import jinja2 @@ -35,31 +36,53 @@ class AuditError(Exception): """Raised for any failure inside run_audit before findings can be produced.""" +FindingKind = Literal["missing", "changed"] +_FINDING_KINDS: tuple[FindingKind, ...] = ("missing", "changed") +_RULE_ID_BY_KIND: dict[FindingKind, str] = { + "missing": "pack-drift-missing", + "changed": "pack-drift-changed", +} + + @dataclass(frozen=True) class AuditFinding: """One audit finding — a file that's missing or drifted vs the pack. - The `kind` field maps 1:1 to SARIF rule ids in sarif.py. + The `kind` field maps 1:1 to SARIF rule ids in sarif.py. Only the values + listed in :data:`FindingKind` are accepted; constructing with anything + else raises ``ValueError`` so a typo can't silently produce invalid + SARIF downstream. """ - kind: str # "missing" | "changed" + kind: FindingKind dest: str # path relative to target repo pack: str + def __post_init__(self) -> None: + if self.kind not in _FINDING_KINDS: + raise ValueError( + f"Invalid AuditFinding.kind {self.kind!r}; expected one of {list(_FINDING_KINDS)}" + ) + @property def rule_id(self) -> str: - return {"missing": "pack-drift-missing", "changed": "pack-drift-changed"}[self.kind] + return _RULE_ID_BY_KIND[self.kind] @property def message(self) -> str: + # Suggested commands include --spec because both `apply` and `diff` + # require it; omitting it would print a Click usage error to anyone + # who copy-pastes the suggestion. if self.kind == "missing": return ( f"File '{self.dest}' is missing; pack '{self.pack}' would " - f"create it. Run `nboot apply --pack {self.pack}` to remediate." + f"create it. Run `nboot apply --spec nboot-spec.json --pack " + f"{self.pack} --target ` to remediate." ) return ( f"File '{self.dest}' differs from pack '{self.pack}'. " - f"Run `nboot diff --pack {self.pack}` to review the drift." + f"Run `nboot diff --spec nboot-spec.json --pack {self.pack} " + f"--target ` to review the drift." ) def to_sarif_result(self) -> SarifResult: @@ -71,11 +94,8 @@ def to_sarif_result(self) -> SarifResult: def _diff_result_to_finding(diff: DiffResult, pack: str) -> AuditFinding: - return AuditFinding( - kind="missing" if diff.is_new else "changed", - dest=diff.dest, - pack=pack, - ) + kind: FindingKind = "missing" if diff.is_new else "changed" + return AuditFinding(kind=kind, dest=diff.dest, pack=pack) def run_audit( diff --git a/src/navi_bootstrap/cli.py b/src/navi_bootstrap/cli.py index a7c1f43..bc05b78 100644 --- a/src/navi_bootstrap/cli.py +++ b/src/navi_bootstrap/cli.py @@ -434,7 +434,14 @@ def audit_cmd( github/codeql-action/upload-sarif. Exits 0 when the target fully conforms; exits 1 when drift is found - (override with --exit-zero for report-only CI surveys). + (override with --exit-zero for report-only CI surveys); exits 2 on + pipeline errors (bad spec, missing pack, path-confinement violation). + + Threat model: audit's path-confinement check is a Path.resolve() snapshot + and does not defend against a hostile process mutating the target tree + mid-run (TOCTOU). Run on a freshly cloned working tree or read-only mount + when --target may be exposed to untrusted writers. See + docs/reference/audit.md for the full threat model. """ # Offline by default — conformance audits shouldn't depend on network. skip_resolve = not resolve @@ -457,10 +464,13 @@ def audit_cmd( else: rendered = findings_to_text(findings) + # Centralised contract: report generators always return a trailing-newline + # terminated string (findings_to_text already does; SarifReport.to_json + # appends one). The CLI just forwards verbatim. if output is None: - click.echo(rendered) + click.echo(rendered, nl=False) else: - output.write_text(rendered if rendered.endswith("\n") else rendered + "\n") + output.write_text(rendered) click.echo(f"Wrote {len(findings)} finding(s) to {output}", err=True) if findings and not exit_zero: diff --git a/src/navi_bootstrap/diff.py b/src/navi_bootstrap/diff.py index 504c9b9..6430a8b 100644 --- a/src/navi_bootstrap/diff.py +++ b/src/navi_bootstrap/diff.py @@ -67,11 +67,24 @@ def compute_diffs( Returns a list of DiffResult for files that would change. Unchanged files are omitted. - Path confinement: every destination must resolve inside ``target``. A - crafted pack/spec with an absolute path, traversal (``..``), or a - symlink pointing outside the target is rejected with ``ValueError``. - This mirrors the write-side defense in ``engine.write_rendered`` — the - read boundary should be at least as strict as the write boundary. + Path confinement + ---------------- + Every destination must resolve inside ``target``. A crafted pack/spec + with an absolute path, traversal (``..``), or a symlink pointing + outside the target is rejected with ``ValueError``. This mirrors the + write-side defense in ``engine.write_rendered`` — the read boundary + should be at least as strict as the write boundary. + + .. warning:: + + The check is a single ``Path.resolve()`` snapshot. It catches + chained symlinks (``a -> b -> outside``) and relative-target + symlinks (``a -> ../outside/secret``), but it cannot defend + against another process mutating the target between the resolve + call and any subsequent read (TOCTOU). Run audit on a fresh + clone or read-only mount when the target is exposed to + untrusted writers. See ``docs/reference/audit.md`` — + "Threat model and operational notes". """ results: list[DiffResult] = [] diff --git a/src/navi_bootstrap/sarif.py b/src/navi_bootstrap/sarif.py index 9420a18..45bca1c 100644 --- a/src/navi_bootstrap/sarif.py +++ b/src/navi_bootstrap/sarif.py @@ -151,4 +151,7 @@ def to_dict(self) -> dict[str, Any]: } def to_json(self, *, indent: int | None = 2) -> str: - return json.dumps(self.to_dict(), indent=indent, sort_keys=False) + # Trailing newline is part of the contract: callers (cli.py audit_cmd) + # expect every report-generator to terminate with '\n' so they can + # forward output verbatim without per-format newline handling. + return json.dumps(self.to_dict(), indent=indent, sort_keys=False) + "\n" diff --git a/tests/test_audit.py b/tests/test_audit.py index 4671170..5a135ac 100644 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -11,8 +11,6 @@ import pytest from click.testing import CliRunner -import navi_bootstrap.audit as audit_mod -import navi_bootstrap.cli as cli_mod from navi_bootstrap.audit import ( AuditError, AuditFinding, @@ -86,6 +84,27 @@ def test_to_sarif_result_carries_fields(self) -> None: assert r.rule_id == "pack-drift-missing" assert r.artifact_uri == "x/y.py" + def test_invalid_kind_rejected_at_construction(self) -> None: + """Grippy MEDIUM finding on PR #51: an unrecognised `kind` value + would have thrown a downstream KeyError when rule_id was accessed. + After the Literal + __post_init__ validation, construction itself + fails with a clear ValueError. + """ + with pytest.raises(ValueError, match=r"Invalid AuditFinding\.kind"): + AuditFinding(kind="removed", dest="x", pack="p") # type: ignore[arg-type] + with pytest.raises(ValueError, match=r"Invalid AuditFinding\.kind"): + AuditFinding(kind="", dest="x", pack="p") # type: ignore[arg-type] + + def test_message_includes_spec_flag_for_apply_remediation(self) -> None: + """The remediation hint must be a runnable command — `nboot apply` + and `nboot diff` both require --spec, so the message must include + it (Copilot finding on PR #51, paralleling the SARIF rule fix).""" + m = AuditFinding(kind="missing", dest="x", pack="base").message + assert "--spec nboot-spec.json" in m + assert "--pack base" in m + c = AuditFinding(kind="changed", dest="x", pack="base").message + assert "--spec nboot-spec.json" in c + # --------------------------------------------------------------------------- # findings_to_text / findings_to_sarif unit tests @@ -263,7 +282,9 @@ def test_pack_filesystem_path_does_not_cause_false_append_drift(self, tmp_path: f"path, got: {findings_by_path}" ) - def test_plan_value_error_surfaces_as_audit_error(self, tmp_path: Path) -> None: + def test_plan_value_error_surfaces_as_audit_error( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: """run_audit must wrap ValueError from engine.plan() (e.g. loop expansion exceeding _MAX_LOOP_ITEMS) as AuditError so audit_cmd's exit-code-2 contract holds. @@ -279,13 +300,9 @@ def test_plan_value_error_surfaces_as_audit_error(self, tmp_path: Path) -> None: def _hostile_plan(*args, **kwargs): # type: ignore[no-untyped-def] raise ValueError("Loop over 'spec.exploit' has 99999 items (max 1000)") - original = audit_mod.plan - audit_mod.plan = _hostile_plan - try: - with pytest.raises(AuditError, match="Template planning error"): - run_audit(spec, "scaffold", target, skip_resolve=True) - finally: - audit_mod.plan = original + monkeypatch.setattr("navi_bootstrap.audit.plan", _hostile_plan) + with pytest.raises(AuditError, match="Template planning error"): + run_audit(spec, "scaffold", target, skip_resolve=True) class TestAuditPathConfinement: @@ -376,13 +393,13 @@ def test_relative_symlink_escape_is_rejected(self, tmp_path: Path) -> None: with pytest.raises(ValueError, match="escapes outside target"): compute_diffs(rendered, target, pack_name="crafted") - def test_confinement_violation_surfaces_as_audit_error(self, tmp_path: Path) -> None: + def test_confinement_violation_surfaces_as_audit_error( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: """run_audit should wrap ValueError from compute_diffs into AuditError so callers get a single error type.""" # Monkey-patch the engine's render step to emit a traversal dest — # simulates a hostile pack without needing to author one on disk. - from navi_bootstrap.audit import run_audit as _run_audit - from navi_bootstrap.diff import compute_diffs as _compute_diffs # noqa: F401 from navi_bootstrap.engine import RenderedFile spec = _write_spec(tmp_path) @@ -392,13 +409,9 @@ def test_confinement_violation_surfaces_as_audit_error(self, tmp_path: Path) -> def _hostile_render_to_files(*args, **kwargs): # type: ignore[no-untyped-def] return [RenderedFile(dest="../escape.txt", content="x", mode="create")] - original = audit_mod.render_to_files - audit_mod.render_to_files = _hostile_render_to_files - try: - with pytest.raises(AuditError, match="Path confinement error"): - _run_audit(spec, "scaffold", target, skip_resolve=True) - finally: - audit_mod.render_to_files = original + monkeypatch.setattr("navi_bootstrap.audit.render_to_files", _hostile_render_to_files) + with pytest.raises(AuditError, match="Path confinement error"): + run_audit(spec, "scaffold", target, skip_resolve=True) # --------------------------------------------------------------------------- @@ -466,7 +479,9 @@ def test_exit_zero_flag_allows_success_with_drift(self, tmp_path: Path) -> None: assert result.exit_code == 0 assert "drift" in result.output.lower() or "missing" in result.output.lower() - def test_diff_cmd_friendly_error_on_confinement(self, tmp_path: Path) -> None: + def test_diff_cmd_friendly_error_on_confinement( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: """Regression: `nboot diff` must turn compute_diffs's path-confinement ValueError into a ClickException (one-line error), not a bare traceback. Codex stop-time review flagged 5fe1668 as introducing an @@ -490,25 +505,21 @@ def test_diff_cmd_friendly_error_on_confinement(self, tmp_path: Path) -> None: def _hostile_render_to_files(*args, **kwargs): # type: ignore[no-untyped-def] return [RenderedFile(dest="escape.txt", content="x", mode="create")] - original = cli_mod.render_to_files - cli_mod.render_to_files = _hostile_render_to_files - try: - runner = CliRunner() - result = runner.invoke( - cli, - [ - "diff", - "--spec", - str(spec), - "--pack", - "scaffold", - "--target", - str(target), - "--skip-resolve", - ], - ) - finally: - cli_mod.render_to_files = original + monkeypatch.setattr("navi_bootstrap.cli.render_to_files", _hostile_render_to_files) + runner = CliRunner() + result = runner.invoke( + cli, + [ + "diff", + "--spec", + str(spec), + "--pack", + "scaffold", + "--target", + str(target), + "--skip-resolve", + ], + ) # ClickException exits 1 with a clean 'Error: ...' line, no traceback. assert result.exit_code == 1 From 83d93761c3fac65f7828ec7f717012e847dbedb7 Mon Sep 17 00:00:00 2001 From: Nelson Spence Date: Sun, 26 Apr 2026 19:08:32 -0500 Subject: [PATCH 10/10] fix(audit): newline contract on empty findings, defensive Exception catch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings on rebase commit 787fe1d: 1. [Copilot audit.py:199] findings_to_text() returned the empty-case string 'OK — target conforms to the pack.' WITHOUT a trailing newline, breaking the trailing-newline contract that audit_cmd relies on (it forwards with click.echo(..., nl=False) and write_text() with no fixup). Empty path now returns 'OK — target conforms to the pack.\n'. Docstring updated to spell out the contract for both branches. 2. [Grippy MEDIUM audit.py:168] audit_cmd only caught AuditError; an unexpected exception class escaping run_audit (e.g. from a future engine refactor introducing a new Error type) would surface as a raw Python traceback. Added a defensive 'except Exception' branch that prints a clean error + 'file an issue' hint and still exits 2, so CI behaviour is consistent and users get actionable feedback. Skipped from this round of bot findings: - Grippy HIGH diff.py:76 TOCTOU — fourth repeat. Already exhaustively documented (code comment, .. warning:: docstring, audit.md threat model section, CLI --help text). The 'fail-fast on root' suggestion would break legitimate containerised CI; not applying. - Grippy LOW pyproject.toml upper bounds — Python ecosystem standard for libraries is no upper bound; pinning would create churn. - Grippy LOW 'log when report written' — already done for --output case ('Wrote N finding(s) to ...'); stdout case IS the log. - Grippy LOW __version__ fallback governance — addressed in 0122244. - Stale older-commit threads (cli.py:398 newline, audit.py:51 kind validation, test_audit.py imports) — already fixed; will be resolved via GraphQL. 418 tests still pass, mypy strict clean, ruff + format clean. --- src/navi_bootstrap/audit.py | 7 +++++-- src/navi_bootstrap/cli.py | 12 ++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/navi_bootstrap/audit.py b/src/navi_bootstrap/audit.py index b78a592..f502ab7 100644 --- a/src/navi_bootstrap/audit.py +++ b/src/navi_bootstrap/audit.py @@ -193,10 +193,13 @@ def findings_to_sarif( def findings_to_text(findings: list[AuditFinding]) -> str: """Human-readable audit summary. - Used by `nboot audit` when ``--format text`` (the default). + Used by `nboot audit` when ``--format text`` (the default). Always + returns a string with a trailing newline so the caller (audit_cmd) + can forward verbatim without per-format newline handling — both the + empty-findings path and the multi-line summary respect this contract. """ if not findings: - return "OK — target conforms to the pack." + return "OK — target conforms to the pack.\n" lines: list[str] = [ f"Audit found {len(findings)} drift finding(s):", diff --git a/src/navi_bootstrap/cli.py b/src/navi_bootstrap/cli.py index bc05b78..bf05fc7 100644 --- a/src/navi_bootstrap/cli.py +++ b/src/navi_bootstrap/cli.py @@ -457,6 +457,18 @@ def audit_cmd( # --exit-zero since pipeline errors must always be visible. click.echo(f"Audit pipeline error: {e}", err=True) raise SystemExit(2) from e + except Exception as e: # pragma: no cover - defence in depth + # Defence in depth (Grippy MEDIUM): if a stage starts raising a + # new exception class run_audit hasn't been taught to wrap as + # AuditError, surface a clean error rather than letting a raw + # traceback leak. Still exit 2 so CI sees pipeline failure. + click.echo(f"Audit internal error: {type(e).__name__}: {e}", err=True) + click.echo( + " This is likely a bug — please file at " + "https://github.com/Project-Navi/navi-bootstrap/issues", + err=True, + ) + raise SystemExit(2) from e if output_format == "sarif": report = findings_to_sarif(findings, tool_name="nboot-audit", tool_version=__version__)