Skip to content

fix(trust): per-baseline audit lookup, per-finding override tier gate, fatal optional-pack pin (#1030) - #1094

Open
pengfei-threemoonslab wants to merge 4 commits into
mainfrom
claude/issue-1030-trust-config
Open

pengfei-threemoonslab wants to merge 4 commits into
mainfrom
claude/issue-1030-trust-config

Conversation

@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor

Closes #1030

Summary

Three trust-configuration checks compared against the wrong reference. Each now compares against the right one, and each fix extends the mechanism it repairs instead of adding a parallel rule.

  • Per-baseline audit lookup (core/baseline_audit.py, core/baseline.py). Every baseline in a directory appends to one baseline-audit.log, and verify_baseline compared the file with the log's last row, whichever baseline wrote it. Verification now uses the latest row recorded for that baseline. The match is on the final component of the row's baseline_path, with either separator, because released versions record the path exactly as the saving command spelled it: relative to that command's working directory, absolute, or Windows-style. If the log has rows but none for this baseline, the result is missing_audit_log (critical, exit 6 under --strict). Its title names the baseline, the files the log does record, and the baseline save that records it. Verification never falls back to another baseline's row.
  • CRLF and the hash docstring (Fix stale cli/scan.py:_dynamic_check_defaults references after PR #106 rename #109). I corrected the docstring rather than switching to raw-bytes hashing. Details are under Design decisions below. The normalisation is now explicit code (normalize_baseline_line_endings) instead of a side effect of read_text, and the bytes come through the static-input snapshot, so the hash covers exactly the bytes load_baseline parsed.
  • Per-finding override tier gate (core/severity_overrides.py, core/check_ids.py, cli/scan/decision.py). resolve_severity_overrides takes this scan's pre-override findings. If an override would lower a finding it governs that was emitted above both the catalog default and the dynamic default, that finding's severity becomes the tier-crossing base. This extends the existing max(catalog, dynamic) base and adds no new rule. "Governs" is a single rule, governing_configured_check_id, which apply_severity_overrides also uses: the exact id first, then the first configured alias or umbrella that expands to it. So the gate judges exactly the findings the override will change. The base never drops below the old one, so every override that needed an acknowledgement still needs one. An override that lowers no emitted finding (for example one to critical) keeps the audit row it had.
  • Fatal pin on optional packs (inputs/policy_packs.py). optional: true now tolerates only absence: nothing at all exists at the declared path, checked lexically, so a dangling link counts as present. Everything else (a pin mismatch, an unreadable or non-regular file, a path outside the manifest directory, a malformed or invalid pack) raises ConfigError, the same as for a required pack. load_policy_packs is the only loader. scan, verify (head and base), doctor/inspect_sources, baseline save and the Action (through scan/verify) all reach it. org status / org policy-packs only report pin status, as the policy_pack_pin_mismatch violation they already emit.

Docs: STABILITY migration note #trust-config-1030, § Baseline Integrity (which row vouches, what the hash covers), § Severity-override floor (per-finding tier gate), and the loaded_policy_packs line. Also docs/baseline.md, docs/policy-packs.md, docs/manifest-v0.1.md, docs/organization.md, docs/checks.md and the check catalog's fires_when / evidence_fields, with docs/checks.json and llms-full.txt regenerated. CHANGELOG has an ## Unreleased › ### Changes entry.

Acceptance criteria

  • Two baselines in one directory; saving one leaves baseline verify --strict on the other at exit 0, and tampering with either is still detected.
    • tests/test_baseline_integrity.py::test_two_baselines_in_one_directory_verify_independently: real baseline save ×2 (the second with --owner other-team) on a copy of the support-refund sample. Both verify --strict exit 0, scan --baseline baseline.json emits no SHIP-BASELINE-INTEGRITY-MISMATCH, and tampering with either baseline exits 6 with hash_mismatch. On the unfixed source it fails with [critical] hash_mismatch … exit 6.
    • test_a_sibling_baselines_content_is_not_vouched_for_by_its_row: copying the baseline saved last over another one is now hash_mismatch. Before the fix it verified clean.
    • Issue repro on samples/large_multi_framework_agent: verify --baseline baseline.json --strict went from exit 6 to exit 0.
  • The CRLF behaviour matches the docstring. test_baseline_hash_reads_line_endings_as_lf_as_its_docstring_states checks the docstring's statement and the behaviour together. CRLF and lone-CR conversions verify clean, including --strict exit 0. A content edit made under CRLF is still hash_mismatch, and a BOM is refused by load_baseline. On the unfixed source the docstring assertion fails.
  • An override that lowers a dynamically critical SHIP-SIDEFX-* finding without acknowledgement is rejected.
    • tests/test_severity_override_floor.py::test_scan_rejects_unacknowledged_override_of_dynamically_critical_sidefx runs end to end on the support-refund sample (stripe.create_refund has a retry policy, so the finding is critical). SHIP-SIDEFX-IDEMPOTENCY-MISSING: high now raises ConfigError (exit 2). With an acknowledge_overrides entry the override applies and policy_audit records critical → high, downgrade, tier_crossed: true, with the reason. On the unfixed source: DID NOT RAISE ConfigError.
    • Unit tests cover the resolver message, ack auditing, the unchanged no-emission case, the alias spelling, and test_no_builtin_check_emitted_above_its_default_lowers_without_ack (every built-in check below critical across the catalog).
  • An optional pack with a wrong sha256 is an error; a missing optional pack is a warning.
    • tests/test_policy_packs.py::test_optional_policy_pack_with_wrong_sha256_is_an_error: a verified pin loads. After an edit that drops block: true, scan raises the pin ConfigError and doctor --json exits 2. On the unfixed source: DID NOT RAISE.
    • test_optional_policy_pack_missing_with_a_pin_still_warns and the existing test_optional_missing_policy_pack_warns cover the missing case. test_optional_policy_pack_that_exists_but_does_not_load_is_an_error[malformed|directory|dangling_link] covers the rest, pinned and unpinned, and fails on the unfixed source.

I proved every acceptance test fails without the fix by checking src/ out at the branch base, rerunning, and restoring. Result: 11 failed, all with the bug's own symptom (no import or attribute errors).

Design decisions (please check)

  1. CRLF: I corrected the docstring instead of hashing bytes, against the issue's stated preference. Hashing raw bytes would turn every Git-for-Windows checkout of an untouched baseline (core.autocrlf=true is the installer default, and GitHub's Windows runners use it) into a critical, unsuppressible SHIP-BASELINE-INTEGRITY-MISMATCH. That is the same false-critical class this issue fixes. A line-ending conversion cannot change the parsed baseline: JSON treats CR and LF as insignificant whitespace and cannot carry either raw inside a string. It would also break every released Windows-written audit row, because released hash_after values are LF-normalised. Every content edit is still detected. If you want bytes anyway, it is a two-line change plus newline="\n" on the writer. The cost is a documented .gitattributes requirement (.agents-shipgate/** -text) for Windows adopters.
  2. Per-baseline matching is by file name. Released rows record baseline_path relative to the saving command's working directory, so nothing richer can be matched across machines without a schema change. BaselineAuditEntry is extra="forbid", so a new row field would break every released reader of a log this version writes. The known limit: two same-named baselines in different directories can share one log only through baseline.audit_log, and they are not told apart. When that happens it fails closed as a false mismatch, which is the pre-fix behaviour. It is documented in STABILITY and the docstring.
  3. optional tolerates only a missing file, as the issue's proposed fix says. Narrowing it to "pin mismatch only" would have left the same bypass open through a pinned pack swapped for a symlink, a directory or an oversized file, each of which raised a non-pin error that optional downgraded. A present-but-malformed optional pack is now exit 2. That is a deliberate break and is in the migration note.
  4. The tier gate now depends on the scan's findings. The same manifest can pass today and exit 2 later, when a tool gains a retry policy. That follows from "judge the severity it was emitted at", and the error names the emitted severity. In verify, a base whose manifest trips the gate makes the base scan scan_failed, which leaves the head gate unchanged (existing handling), so the fixing PR can still go through verify.

Type

  • Check or risk-model change
  • Input adapter change
  • CLI or GitHub Action behavior
  • Report, schema, or SARIF output
  • Documentation only

No new public surface: no CLI flag, schema version, check ID, control state, or exit code. Two additive evidence keys go on the existing SHIP-BASELINE-INTEGRITY-MISMATCH (recorded_baselines, latest_audit_baseline_path), listed in the catalog's evidence_fields. Sample goldens are unchanged (regenerate_goldens.py --check: 24 artifacts, 0 changed), and docs/distribution-surfaces.md needs no row change.

Authorship

Verification

Run locally on a scratch venv built from constraints/dev.txt (--require-hashes, pydantic 2.14.1) plus pip install -e . --no-deps. agents_shipgate.__file__ is under this worktree. Branch base: b980d4a981 (after #1093).

Command Exit
pytest -n auto -m "not perf" --ignore=tests/test_adapter_static_only.py 1, known local noise only. The 2 failures are test_check_unmodelled_host_config_keys.py[local_settings_enabled_plugins-{worktree,git_range}]: git commit -qm base fails because this machine's global gitignore hides the fixture file. Totals: 23316 passed, 14 skipped, 120 xfailed.
pytest tests/test_adapter_static_only.py 0
pytest tests/test_latency_budget.py -m perf 0
pytest on every test file that touches overrides, baselines or packs (19 files), rerun after the last gate refinement 0
pytest tests/test_distribution_surface_parity.py tests/test_install_hooks.py tests/test_cold_start_replay.py tests/test_semantic_cold_start.py tests/test_cold_reader_order.py tests/test_public_surface_contract.py tests/test_regenerate_goldens.py tests/test_schema_roundtrip.py 0
ruff check . 0
python -m compileall -q src tests 0
python scripts/regenerate_goldens.py --check 0 (24 artifacts, 0 changed)
python scripts/generate_schemas.py --check 0 (docs/checks.json regenerated for the catalog text)
python scripts/build-llms-full.py regenerated (docs/checks.md is a source)
Acceptance tests with src/ at the branch base 11 failed, as intended

Issue reproductions before → after: #11 verify --strict on the untouched baseline.json 6 → 0. #37 scan accepted → ConfigError naming 'critical' → 'high'. #41 loaded=0 plus a warning → ConfigError for the pin mismatch. #109 is unchanged (exit 0) by design, and the docstring now says so.

Release-readiness notes

  • No user-code import added to default scan paths
  • No network access added to default scan paths
  • New or changed check IDs are documented in docs/checks.md (no new IDs; the SHIP-BASELINE-INTEGRITY-MISMATCH text is updated)
  • Report/schema changes are additive or documented in STABILITY.md

🤖 Generated with Claude Code

Every baseline in a directory appends to one baseline-audit.log, and
verify_baseline compared the file with the log's last row whichever
baseline wrote it. Saving baseline-prod.json turned an untouched
baseline.json into a critical SHIP-BASELINE-INTEGRITY-MISMATCH, while a
copy of the baseline saved last, or a renamed baseline, verified clean.

Verification now uses the latest row recorded for that baseline's file
name (the final component of baseline_path, either separator, which is
how released versions recorded it). A log with rows but none for the
baseline is missing_audit_log, titled with the file, what the log
records and the baseline save that records it. Released audit logs
verify unchanged.

The hash reads CRLF and lone CR as LF, as it always has; the docstring
that claimed line endings were detected is corrected, and the
normalisation is explicit and reads through the static-input snapshot.
…tted at (#1030)

The severity-override tier gate compared a downgrade with the catalog
default. SHIP-SIDEFX-IDEMPOTENCY-MISSING is critical when a tool's retry
policy is known (default high), so an override to high lowered it with
no acknowledgement; a deprecated alias whose own default is lower than
its expansions did the same.

resolve_severity_overrides now takes the scan's pre-override findings.
When an override would lower a finding it governs that was emitted
above both defaults, that severity is the tier-crossing base. Governance
is one rule, governing_configured_check_id, shared with
apply_severity_overrides. The base never drops below the defaults.
load_policy_packs downgraded every load error of an optional pack to a
warning, so a sha256 pin mismatch, or a pinned pack replaced by a
malformed file, a directory or a link, dropped the pack's rules with
only a warning. optional now tolerates exactly one failure: nothing
exists at the declared path. Anything else is a config error, as for a
required pack. Every pack-loading command goes through this loader.
Migration note #trust-config-1030; Baseline Integrity (which row
vouches, what the hash covers), Severity-override floor (per-finding
tier gate) and the loaded_policy_packs pin line updated.
@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor Author

🤖 Engineering review — implementer self-review

I reviewed git diff origin/main...HEAD adversarially against the four questions in the brief, then made each acceptance test fail on the unfixed source. Method: check src/ out at the branch base, rerun, restore. 11 failed, every one with the bug's own symptom.

Can a baseline still be tampered with undetected?

  • Edits to either sibling are still hash_mismatch; test is test_two_baselines_in_one_directory_verify_independently.
  • Two attacks that passed before now fail:
    • copying the sibling saved last over another baseline (test_a_sibling_baselines_content_is_not_vouched_for_by_its_row);
    • renaming an audited baseline to a name with no rows, which is now missing_audit_log (test_baseline_with_no_row_of_its_own_fails_with_an_actionable_message).
  • Rename into the gated name (mv baseline-lax.json baseline.json) is compared with baseline.json's own latest row, so it is a mismatch.
  • Residual:
    • Forged rows. Forging a row in the log is still undetected by the hash alone. This is the existing "tamper-evident, not tamper-proof" contract. .agents-shipgate/** is a trust root in verify.
    • Shared logs. Same-named baselines in different directories sharing one log through baseline.audit_log are not told apart, the same as before this PR. This is documented, and it fails closed (a false mismatch, not a pass).
  • Found and fixed while reviewing. The renamed-baseline case was a silent pass on main: the hash equalled the log's last row. I first had it falling into the generic missing_audit_log text. I gave it its own title naming the file, what the log records, and the baseline save to run, so the old-log migration path is explicit.

Can an override still lower a critical side-effect finding another way?

  • Other spellings.
    • A wildcard (SHIP-SIDEFX-*) is still an unknown-id ConfigError.
    • Legacy alias: SHIP-API-OPERATIONAL-READINESS is itself a catalog entry with default medium. On main, SHIP-API-OPERATIONAL-READINESS: medium lowered a high SHIP-API-RETRY-WITHOUT-IDEMPOTENCY with no acknowledgement. That is a second bypass the issue did not name. The per-finding gate closes it, because governance uses the same governing_configured_check_id rule as apply_severity_overrides. Test: test_alias_override_is_judged_by_the_findings_it_governs.
    • Split umbrellas (SHIP-VERIFY-POLICY-WEAKENED) go through the same rule.
  • Other config paths.
    • Policy packs cannot override built-ins: their rule ids cannot start with SHIP-.
    • Plugins and conditional emitters are covered generically. test_no_builtin_check_emitted_above_its_default_lowers_without_ack sweeps the whole catalog.
    • Overrides are resolved once, in cli/scan/decision.py, after dedupe and the policy-eligibility filter and before suppression. Every finding the override touches is in the set it is judged by.
  • Out of scope, flagged: risk_overrides, policies.require_idempotency_for_tools and checks.ignore can make the SIDEFX finding not fire or be suppressed. Those are separate, reason-bearing declarations, not severity overrides.

Does any pack-loading path still downgrade a pin mismatch?

  • No. load_policy_packs is the only loader. scan, verify (head and base), doctor/inspect_sources, baseline save and the Action (through scan/verify) reach it. optional now short-circuits only when os.path.lexists is false.
  • org status / org policy-packs report pins without loading packs. They already report policy_pack_pin_mismatch regardless of optional.
  • Found while reviewing. Restricting the fix to the pin-mismatch error alone would still have let a pinned optional pack be swapped for a symlink, a directory or an oversized file. Each raises a different error that optional downgraded. Hence the "only absence" rule from the issue, tested pinned and unpinned (…exists_but_does_not_load_is_an_error[malformed|directory|dangling_link]).

Do released baselines and audit logs still work?

  • Yes, with no migration. The audit row shape and audit_schema_version are unchanged; no field was added, because BaselineAuditEntry is extra="forbid" and released readers would reject a new one.
  • The hash function is byte-for-byte what every release computed.
  • In the default one-baseline directory, "latest row for this file" is the log's last row.
  • Rows recorded relative, absolute or Windows-style all match. Test: test_audit_log_from_a_released_version_still_verifies, which rewrites a log as 1.2.0 would have written it on two machines.
  • The only case that changes is a baseline that verified by borrowing another file's row. It now fails with the actionable message above.

Left for the owner

  1. CRLF. I corrected the docstring instead of hashing bytes, against the issue's stated preference. The reasoning is in the PR body: Git for Windows' autocrlf would make every Windows checkout a critical integrity failure, and released Windows-written rows would break. Say the word if you want bytes plus a .gitattributes requirement instead.
  2. Present-but-broken optional packs are now exit 2, not just pin mismatches. This is deliberate and follows the issue's "optional only tolerates a missing file", but it is a behaviour break for anyone relying on optional to skip a malformed pack.
  3. The tier gate now depends on findings, so a manifest can start failing when a tool gains a retry policy. In verify, a base that trips it degrades only the base comparison (scan_failed), so the fixing PR can go through.
  4. Not done: a catalog test that fails when an emitter exceeds its default without dynamic_default. The per-finding gate makes that flag irrelevant to the tier gate, so the catalog-wide sweep above pins the property directly instead.

Fixed during self-review

  • Audit-row churn in the tier gate. My first cut raised the tier base to the strongest emitted severity unconditionally. So SHIP-SIDEFX-IDEMPOTENCY-MISSING: critical (a pure upgrade) read direction: same instead of upgrade. The base is now raised only when the override would lower that finding. Rows for overrides that lower nothing are byte-for-byte what they were. Test: test_override_that_lowers_no_emitted_finding_keeps_its_audit_row.
  • Write path. write_baseline caught only OSError around hash_before. A non-UTF-8 or non-regular prior file would have crashed the save that repairs it. It now also catches ValueError.
  • Bypass classes, not instances. I widened the pack fix from "pin mismatch" to "only absence is tolerated". I widened the gate from "SIDEFX" to every governed finding, which covered the alias bypass above.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

trust config: per-baseline audit lookup, per-finding tier gate, fatal pack pin mismatch

1 participant