From cbe943fcdfca97035721487a2a17b42cd5c924d3 Mon Sep 17 00:00:00 2001 From: Henry Park <16583448+henrypark133@users.noreply.github.com> Date: Mon, 24 Aug 2026 17:35:58 +0000 Subject: [PATCH] refactor(ci): close the three open audit findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three were NORMAL, none blocking; closing them so review sees the design rather than a list of known nits. **One owner for the debug-info policy** (converged finding, reported independently by both system-audit lanes). The migration deleted these env pairs from five workflow job envs on the strength of Cargo.toml's `[profile.dev] debug = 0` owning the value — but left the identical `:-0` defaults standing in scripts/ci/quality_gate.sh and reborn-local-coverage-ratchet.sh. The PR's own claim of a single owner was not true of the tree it shipped. Both removed; no behaviour change, the values already agreed, and a developer's `CARGO_PROFILE_DEV_DEBUG=2` still reaches cargo because `env` inherits what it does not override. Deleting the lines alone would just let them come back, so `validate_single_debug_policy_owner` makes it structural: no script under scripts/ may ASSIGN a CARGO_PROFILE_*_DEBUG value. Assignments only — run-hermetic-test-process.sh names the same variables in a passthrough allowlist (a `case` pattern, no `=`), which is exactly how that override survives the hermetic barrier, and matching it would break the documented escape hatch. Test files are skipped: they carry the string on purpose as fixtures, and scanning them would make the contract untestable. **No speculative escape hatch.** `ACCEPTED_RUST_BOOTSTRAPS` was a per-path override with symmetric over-use AND under-use validation, plus a test pinning it empty — built in full for a case the module's own comment says does not exist. Removed; the rule is now unconditional. If an unavoidable bootstrap ever appears, the hatch gets added then, with that lane as its first entry. The test that asserted the dict stays empty now asserts the mechanism is gone rather than merely empty. **One job-boundary walk.** `_job_blocks` reimplemented the slice-between- consecutive-headings logic that `extract_job_block` already did in the same subsystem. `job_blocks()` in workflow_text.py is now the single primitive: `extract_job_block` filters it to one named job and keeps its exactly-one refusal, and the toolchain contracts enumerate it from the `jobs:` offset. That offset is load-bearing and stays — JOB_HEADING matches any two-space key, so an unbounded walk treats `on:`'s children as jobs. Verified: ws12 141 tests OK, live gate passed, planner 90 OK, staged-paths 4 OK, classify-test-scope exit 0, check-guidance OK, both edited shell scripts pass `bash -n`. The debug-policy guard proven red-first by restoring one deleted line and watching the gate name it. Co-Authored-By: Claude Fable 5 --- scripts/ci/lib/rust_toolchain_contracts.py | 103 ++++++++++++-------- scripts/ci/lib/workflow_text.py | 27 +++++ scripts/ci/quality_gate.sh | 1 - scripts/ci/reborn-local-coverage-ratchet.sh | 2 - scripts/ci/test_ws12_workflow_contracts.py | 58 ++++++++++- scripts/ci/ws12_workflow_contracts.py | 11 +-- 6 files changed, 150 insertions(+), 52 deletions(-) diff --git a/scripts/ci/lib/rust_toolchain_contracts.py b/scripts/ci/lib/rust_toolchain_contracts.py index a97dbaffa7..7651f4d42f 100644 --- a/scripts/ci/lib/rust_toolchain_contracts.py +++ b/scripts/ci/lib/rust_toolchain_contracts.py @@ -30,7 +30,7 @@ from __future__ import annotations import re from pathlib import Path -from workflow_text import JOB_HEADING, job_body, step_body +from workflow_text import JOB_HEADING, job_blocks, job_body, step_body ROOT = Path(__file__).resolve().parents[3] @@ -205,8 +205,9 @@ RUST_BOOTSTRAP_PATTERNS = ( # No workflow may bootstrap Rust outside the composite. cargo-dist # re-includes .github/dist-build-setup.yml on every regeneration, so the # release build jobs install Rust through the composite from there — there -# is no lane this contract cannot cover. -ACCEPTED_RUST_BOOTSTRAPS: dict[str, int] = {} +# is no lane this contract cannot cover, and so no exemption mechanism here. +# If a genuinely unavoidable bootstrap ever appears, add the escape hatch +# then, with that lane as its first entry and its reason in the comment. def validate_no_unmanaged_rust_bootstrap(workflows: dict[str, str]) -> list[str]: @@ -223,28 +224,12 @@ def validate_no_unmanaged_rust_bootstrap(workflows: dict[str, str]) -> list[str] hits = sum(text.count(pattern) for pattern in RUST_BOOTSTRAP_PATTERNS) if not hits: continue - allowed = ACCEPTED_RUST_BOOTSTRAPS.get(path, 0) - if hits > allowed: - errors.append( - f"{path}: {hits} raw Rust bootstrap(s) " - f"({', '.join(RUST_BOOTSTRAP_PATTERNS)}); " - f"{allowed} accepted here. Install Rust through " - ".github/actions/setup-rust so the toolchain stays pinned, " - "mold stays wired, and rust-toolchain.toml stays enforced. " - "A genuinely unavoidable bootstrap must be added to " - "ACCEPTED_RUST_BOOTSTRAPS with the reason." - ) - for path, expected in ACCEPTED_RUST_BOOTSTRAPS.items(): - text = workflows.get(path) - if text is None: - continue - hits = sum(text.count(pattern) for pattern in RUST_BOOTSTRAP_PATTERNS) - if hits < expected: - errors.append( - f"{path}: expected {expected} accepted Rust bootstrap(s), " - f"found {hits}. If the generator stopped emitting it, drop " - "the entry from ACCEPTED_RUST_BOOTSTRAPS." - ) + errors.append( + f"{path}: {hits} raw Rust bootstrap(s) " + f"({', '.join(RUST_BOOTSTRAP_PATTERNS)}). Install Rust through " + ".github/actions/setup-rust so the toolchain stays pinned, mold " + "stays wired, and rust-toolchain.toml stays enforced." + ) return errors @@ -268,25 +253,14 @@ def _composite_anchors(text: str) -> set[str]: def _job_blocks(text: str) -> list[tuple[str, str]]: - """(job name, block) for each job, bounded to the `jobs:` mapping. + """Every job in the file, bounded to the `jobs:` mapping. - JOB_HEADING matches ANY two-space `key:` line, so the `on:` trigger's - children (`push:`, `workflow_call:`, ...) match too — slicing from the - first heading blindly truncated the preamble at the first trigger and - made the workflow-level checks dead code on every real workflow. + Slicing from the first heading blindly truncated the preamble at the + first `on:` trigger and made the workflow-level checks dead code on every + real workflow, so the `jobs:` offset is load-bearing here. """ jobs_key = JOBS_KEY.search(text) - jobs_start = jobs_key.start() if jobs_key else 0 - headings = [h for h in JOB_HEADING.finditer(text) if h.start() >= jobs_start] - blocks = [] - for index, heading in enumerate(headings): - end = ( - headings[index + 1].start() - if index + 1 < len(headings) - else len(text) - ) - blocks.append((heading.group("name"), text[heading.start():end])) - return blocks + return job_blocks(text, jobs_key.start() if jobs_key else 0) def _reaches_composite(block: str, anchors: set[str]) -> bool: @@ -381,6 +355,53 @@ def validate_release_workflow_installs_rust( return errors +# Cargo.toml's `[profile.dev] debug = 0` owns the debug-info policy. Anything +# else ASSIGNING one of these is a second writer of the same value -- harmless +# while the values agree, and a silent divergence the day someone bumps one. +# An assignment only: `run-hermetic-test-process.sh` names the same variables +# in a passthrough allowlist (a `case` pattern, no `=`), which is how a +# developer's `CARGO_PROFILE_DEV_DEBUG=2` override survives the hermetic +# barrier. That entry must keep working, so it must not match here. +DEBUG_POLICY_ASSIGNMENT = re.compile(r"CARGO_PROFILE_[A-Z]+_DEBUG\s*=") +DEBUG_POLICY_OWNER = "Cargo.toml" + + +def validate_single_debug_policy_owner(root: Path = ROOT) -> list[str]: + """Only Cargo.toml may set the debug-info profile values. + + The migration deleted these env pairs from five workflow job envs on the + strength of the profile block owning them, but left the identical `:-0` + defaults standing in two scripts -- so the change's own claim of a single + owner was not true of the whole tree. Two audit lanes reported it + independently. + """ + + errors: list[str] = [] + for path in sorted((root / "scripts").rglob("*")): + if not path.is_file() or path.suffix not in (".sh", ".py"): + continue + # Test files carry the forbidden string on purpose, as fixtures and + # as the sabotage input that proves this check fires. Scanning them + # would make the contract unable to have a regression test at all. + if path.name.startswith(("test_", "test-")): + continue + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as error: + errors.append(f"{path}: could not read: {error}") + continue + for number, line in enumerate(text.splitlines(), start=1): + if DEBUG_POLICY_ASSIGNMENT.search(line.split("#")[0]): + errors.append( + f"{path.relative_to(root)}:{number}: assigns a " + "CARGO_PROFILE_*_DEBUG value, which " + f"{DEBUG_POLICY_OWNER}'s `[profile.dev]` block owns. Two " + "writers of one value diverge the day either is bumped; " + "delete this and let the profile decide." + ) + return errors + + def validate_toolchain_pin_sync(root: Path = ROOT) -> list[str]: """rust-toolchain.toml and the composite's default must name one version.""" try: diff --git a/scripts/ci/lib/workflow_text.py b/scripts/ci/lib/workflow_text.py index 3a5d86bf50..ba63f4a1b9 100644 --- a/scripts/ci/lib/workflow_text.py +++ b/scripts/ci/lib/workflow_text.py @@ -26,6 +26,33 @@ JOB_HEADING = re.compile(r"^ (?P[A-Za-z0-9_-]+):[ \t]*$", re.MULTILINE) STEP_HEADING = re.compile(r"^[ \t]*- name: (?P.+)$", re.MULTILINE) +def job_blocks(text: str, start: int = 0) -> list[tuple[str, str]]: + """(job name, block) for every two-space job key at or after `start`. + + The one place that answers "where does a job's YAML block end" — the next + job heading, or end of file. Both callers that need job boundaries build + on this: `extract_job_block` filters it to one named job and refuses + anything but an exact match, and the toolchain contracts enumerate it from + the `jobs:` key onward. + + `start` exists because JOB_HEADING matches ANY two-space `key:` line, so + the `on:` trigger's children (`push:`, `workflow_call:`, ...) match too. + A caller that must not treat those as jobs passes the offset of `jobs:`. + """ + headings = [h for h in JOB_HEADING.finditer(text) if h.start() >= start] + return [ + ( + heading.group("name"), + text[ + heading.start() : headings[index + 1].start() + if index + 1 < len(headings) + else len(text) + ], + ) + for index, heading in enumerate(headings) + ] + + def step_body(text: str, step_name: str) -> str | None: """Return one workflow step's body, bounded by the next step heading.""" for heading in STEP_HEADING.finditer(text): diff --git a/scripts/ci/quality_gate.sh b/scripts/ci/quality_gate.sh index 40d39ff6dd..ea873a3e72 100755 --- a/scripts/ci/quality_gate.sh +++ b/scripts/ci/quality_gate.sh @@ -16,7 +16,6 @@ run_cargo_ci() { -u LLM_BACKEND \ IRONCLAW_DISABLE_OS_KEYCHAIN="${IRONCLAW_DISABLE_OS_KEYCHAIN:-1}" \ CARGO_INCREMENTAL="${CARGO_INCREMENTAL:-0}" \ - CARGO_PROFILE_TEST_DEBUG="${CARGO_PROFILE_TEST_DEBUG:-0}" \ RUST_MIN_STACK="${RUST_MIN_STACK:-67108864}" \ "$@" } diff --git a/scripts/ci/reborn-local-coverage-ratchet.sh b/scripts/ci/reborn-local-coverage-ratchet.sh index 435ef259e5..24ab2166bb 100755 --- a/scripts/ci/reborn-local-coverage-ratchet.sh +++ b/scripts/ci/reborn-local-coverage-ratchet.sh @@ -45,8 +45,6 @@ run_cargo_cov_env() { -u IRONCLAW_LLM_MODEL \ IRONCLAW_DISABLE_OS_KEYCHAIN="${IRONCLAW_DISABLE_OS_KEYCHAIN:-1}" \ CARGO_INCREMENTAL="${CARGO_INCREMENTAL:-0}" \ - CARGO_PROFILE_DEV_DEBUG="${CARGO_PROFILE_DEV_DEBUG:-0}" \ - CARGO_PROFILE_TEST_DEBUG="${CARGO_PROFILE_TEST_DEBUG:-0}" \ RUST_MIN_STACK="${RUST_MIN_STACK:-8388608}" \ "$@" } diff --git a/scripts/ci/test_ws12_workflow_contracts.py b/scripts/ci/test_ws12_workflow_contracts.py index b19d654a7a..b131e7c46b 100755 --- a/scripts/ci/test_ws12_workflow_contracts.py +++ b/scripts/ci/test_ws12_workflow_contracts.py @@ -22,6 +22,7 @@ from rust_toolchain_contracts import ( # noqa: E402 validate_release_workflow_installs_rust, validate_rust_jobs_reach_the_composite, validate_setup_rust_action, + validate_single_debug_policy_owner, validate_toolchain_pin_sync, ) from workflow_text import JOB_HEADING, STEP_HEADING, job_body, step_body # noqa: E402 @@ -59,6 +60,43 @@ SCCACHE_SETUP_ACTION = ( ) +class SingleDebugPolicyOwnerTests(unittest.TestCase): + """Cargo.toml's [profile.dev] is the only writer of the debug-info value.""" + + def tree(self, body: str) -> Path: + root = Path(self.enterContext(tempfile.TemporaryDirectory())) + (root / "scripts" / "ci").mkdir(parents=True) + (root / "scripts" / "ci" / "gate.sh").write_text(body, encoding="utf-8") + return root + + def test_a_second_writer_is_rejected(self) -> None: + root = self.tree('CARGO_PROFILE_TEST_DEBUG="${CARGO_PROFILE_TEST_DEBUG:-0}"\n') + errors = validate_single_debug_policy_owner(root) + self.assertEqual(1, len(errors), errors) + self.assertIn("scripts/ci/gate.sh:1", errors[0]) + self.assertIn("Cargo.toml", errors[0]) + + def test_the_hermetic_passthrough_allowlist_still_passes(self) -> None: + """A `case` pattern naming the vars is not a second writer. + + run-hermetic-test-process.sh lists them so a developer's + `CARGO_PROFILE_DEV_DEBUG=2` override survives the hermetic barrier. + Matching that entry would break the documented escape hatch. + """ + root = self.tree( + " CARGO_INCREMENTAL|CARGO_PROFILE_DEV_DEBUG|" + "CARGO_PROFILE_TEST_DEBUG|CARGO_TEST_ARGS|\\\n" + ) + self.assertEqual([], validate_single_debug_policy_owner(root)) + + def test_a_comment_mentioning_the_override_still_passes(self) -> None: + root = self.tree("# override per-run: CARGO_PROFILE_DEV_DEBUG=2 cargo test\n") + self.assertEqual([], validate_single_debug_policy_owner(root)) + + def test_the_live_tree_has_exactly_one_owner(self) -> None: + self.assertEqual([], validate_single_debug_policy_owner(ROOT)) + + class RustJobsReachTheCompositeTests(unittest.TestCase): """Every job that runs cargo must reach the composite, not just the release lane. @@ -522,8 +560,24 @@ class UnmanagedRustBootstrapTests(unittest.TestCase): errors = validate_no_unmanaged_rust_bootstrap(workflows) self.assertTrue(any("ironclaw-release" in e for e in errors), errors) - def test_no_accepted_bootstraps_remain(self): - self.assertEqual({}, rust_toolchain_contracts.ACCEPTED_RUST_BOOTSTRAPS) + def test_a_bootstrap_is_rejected_with_no_exemption_available(self) -> None: + """There is no allowlist to add a lane to; the rule is unconditional. + + The empty `ACCEPTED_RUST_BOOTSTRAPS` dict this replaces was an escape + hatch, fully built with symmetric over/under-use validation, for a + case the module's own comment said did not exist. Flagged by the + structural-discipline audit as speculative generality. + """ + errors = validate_no_unmanaged_rust_bootstrap( + {".github/workflows/demo.yml": " - run: curl https://sh.rustup.rs | sh\n"} + ) + self.assertEqual(1, len(errors), errors) + self.assertIn("raw Rust bootstrap", errors[0]) + self.assertNotIn("ACCEPTED", errors[0]) + self.assertFalse( + hasattr(rust_toolchain_contracts, "ACCEPTED_RUST_BOOTSTRAPS"), + "the exemption mechanism should be gone, not merely empty", + ) def test_alternate_vendor_toolchain_actions_are_caught(self): """The composite is the only sanctioned installer. diff --git a/scripts/ci/ws12_workflow_contracts.py b/scripts/ci/ws12_workflow_contracts.py index a500755dae..94e06c0fdf 100755 --- a/scripts/ci/ws12_workflow_contracts.py +++ b/scripts/ci/ws12_workflow_contracts.py @@ -28,9 +28,10 @@ from rust_toolchain_contracts import ( # noqa: E402 validate_release_workflow_installs_rust, validate_rust_jobs_reach_the_composite, validate_setup_rust_action, + validate_single_debug_policy_owner, validate_toolchain_pin_sync, ) -from workflow_text import JOB_HEADING, STEP_HEADING, job_body, step_body # noqa: E402 +from workflow_text import STEP_HEADING, job_blocks, job_body, step_body # noqa: E402 REQUIRED_MARKERS: dict[str, tuple[str, ...]] = { ".github/workflows/reborn-tests.yml": ( @@ -455,16 +456,13 @@ def extract_job_block(text: str, job: str) -> tuple[str | None, str]: fail-closed stance as `extract_scope_regex`). """ - headings = list(JOB_HEADING.finditer(text)) - matches = [match for match in headings if match.group("name") == job] + matches = [block for name, block in job_blocks(text) if name == job] if len(matches) != 1: return None, ( f"expected exactly one {job!r} job, found {len(matches)} — the " "scoped libsql-scripted-memory contract cannot resolve its block" ) - start = matches[0].start() - following = next((m for m in headings if m.start() > start), None) - return text[start : following.start() if following else len(text)], "" + return matches[0], "" def extract_continued_commands(text: str, executable: str) -> list[str]: """Return shell commands whose first line is ` \\`. @@ -1790,6 +1788,7 @@ def validate_workflow_texts( errors.extend(validate_rust_jobs_reach_the_composite(workflows)) errors.extend(validate_no_job_env_rustflags_with_setup_rust(workflows)) errors.extend(validate_toolchain_pin_sync(root)) + errors.extend(validate_single_debug_policy_owner(root)) return errors