feat: distinguishable side-effect failures via error_class (feature 036) - #435
Merged
Merged
Conversation
Feature 036 SC-002 requires that an audit with zero environmental failures produce byte-for-byte identical markdown / JSON / SARIF output to the pre-feature implementation. Verifying that needs a baseline captured from unmodified code -- goldens generated during implementation would lock post-feature behavior instead of proving pre-feature equivalence. Captures two purely-deterministic controls (OSPS-DO-01.01 README presence, OSPS-LE-03.01 LICENSE presence -- the same pair the parity fixture corpus uses) against the mixed_repo fixture. Yields 1 PASS + 1 FAIL, so the baseline exercises both the success path and the clean-failure path that must NOT carry an error_class once the feature lands. capture_baseline.py is committed alongside so the baseline is regenerable and the scrubbing rules are reviewable. Three fields are scrubbed as run- or machine-varying, none related to this feature: - fixture tempdir path (machine-dependent) - format_results_markdown's "Generated At:" stamp (run-dependent). This is a real Tier 1 determinism gap -- markdown audit reports can never be byte-for-byte reproducible while the formatter embeds a wall-clock timestamp. Out of scope here; follow-up on darnitdevorg#418. - pass_history[].duration_ms (load-dependent; reads 0 for these fast checks in practice but nothing guarantees it) Verified reproducible: two consecutive captures are byte-identical.
…e 2) Foundational half of feature 036. Adds the ErrorClass type and threads the field through the three result objects, with the CEL post-step fix that the field would otherwise be silently dropped by. New module core/error_class.py exports two names: - ErrorClass: Literal of the six v0 values (network, auth, timeout, rate_limit, not_found, crashed). - ERROR_CLASSES: the same six as a frozenset. Both are needed because typing.Literal is erased at runtime and enforces nothing on its own -- the frozenset is what makes validation possible (FR-002a). This mirrors core/authority.py's Literal+frozenset pairing, but diverges in how the frozenset is used: authority.py backs a fail-safe (is_terminal_authority returns False for unknowns, so an unknown authority can never conclude a control), whereas an unknown error_class has no safe default -- it is neither "the check failed" nor "the check could not run" -- so this module's frozenset backs a raise. Field additions, all additive: - HandlerResult.error_class, plus a __post_init__ enforcing both contract invariants: unknown values are rejected (FR-002a), and error_class alongside status=PASS is rejected (a handler that could not complete cannot have produced a real pass). - SieveResult.error_class, populated from the RESOLVING pass only per FR-009a. CONCLUDE_PASS is deliberately excluded from the threading: it fires only when handler_status is PASS, which HandlerResult makes unrepresentable alongside error_class, so there is provably nothing to carry. - CheckResult["error_class"] as NotRequired[str], emitted from to_legacy_dict only when set. Typed str rather than ErrorClass because the TypedDict is the deserialization boundary. The CEL fix is the subtle part. _apply_cel_expr builds brand-new HandlerResult objects at three construction sites rather than mutating the input, so any field not threaded explicitly is lost with no error -- the verdict stays correct and only the failure cause disappears. Feature 026 hit this identical bug with authority; the fix follows the same shape. Tests were written first and confirmed failing on exactly the three constructing branches (the five pass-through paths were already safe), then passing after. Full framework + baseline suite: 2816 passed, 13 skipped. Baseline from 4d3d37f re-verified byte-identical: zero happy-path drift.
…wn (036 US1) User Story 1 of feature 036: an operator running an audit with an expired token can now tell from the markdown report alone which controls failed because the environment was broken and which failed because the repository is genuinely non-compliant. exec handler classification (contract section 2.1): - subprocess.TimeoutExpired -> timeout - FileNotFoundError -> not_found (the binary is absent from PATH) - undeclared non-zero exit -> classified from stderr via _classify_exec_failure: GitHub rate-limit patterns first, then auth patterns, else network Rate-limit is deliberately checked before auth because GitHub answers 403 for both rate limits and permission failures, and the rate-limit body is the more specific signal. Two paths deliberately carry NO error_class: - pass_exit_codes (the check succeeded) - declared fail_exit_codes (the check RAN and concluded non-compliance) That second one is the invariant that keeps the feature honest. If a declared failure code started producing an error_class, every real finding would read as an infrastructure blip and the annotation would become noise. There is an explicit test for it. Pattern constants are GitHub-only for v0 per clarify Q4. The exec handler sees only stdout/stderr/exit-code -- no response headers -- so classification is substring matching against `gh` CLI stderr shape. Other targets (git, curl, syft) fall through to network. Environmental failures now log at WARN via _log_environmental_failure, naming the control, the handler, and the error_class. Previously these were silent or DEBUG-only, so a degraded audit was invisible unless the operator knew to raise verbosity. Markdown formatter annotates the control line with `[error_class]` when present, leaving genuine findings unannotated. 21 new tests. Full suite: 2837 passed, 13 skipped. Baseline from 4d3d37f re-verified byte-identical.
User Story 2 of feature 036: environmental failures now surface at the
default log level. Previously they were DEBUG-only or silent, so an
operator whose audit was half-degraded by a broken token or an
unreachable MCP server had no way to notice without knowing to raise
verbosity first.
MCP handler (contract section 2.2): all nine exception paths now carry
an error_class. FR-006 names three; the remaining six are the ones the
FR's final clause delegates to the contract table.
McpToolTimeout -> timeout
McpServerHandshakeFailed -> network
McpServerBinaryMissing -> not_found
McpServerVerificationFailed -> auth (a trust relationship to fix)
McpServerUnusable -> network
McpToolError -> crashed (ran, errored internally)
McpToolResponseNotJson -> crashed (ran, output uninterpretable)
UnknownMcpServer -> not_found (server never configured)
bare Exception -> crashed
The error_info tuple widened from (status, message) to
(status, message, error_class) to carry the classification to the
single construction site.
Orchestrator: a handler that raises now yields error_class="crashed"
and logs at WARN naming the control. A crashed handler and a genuine
ERROR verdict were previously indistinguishable in default logs.
Context auto-detect: _get_remote_url distinguishes two cases that were
both silent before. A non-zero exit is git's legitimate answer ("no
such remote") and stays quiet -- warning there would make every
single-remote repo noisy, which is how loud logging becomes ignored
logging. An exception (timeout / missing git / OSError) means we could
not consult git at all, and now warns with the matching error_class.
Also fixes a bug I introduced in feature 418 (PR darnitdevorg#424): inserting
_atomic_write_text directly after MCP_DEFAULT_TIMEOUT_SECONDS orphaned
that constant's attribute docstring, leaving it dangling as a bare
string expression after an unrelated function. Docstring reattached.
18 new tests (7 MCP mappings parameterized, orchestrator crash path,
3 auto-detect cases, happy-path no-WARN guard, dependency assertions).
Full suite: 2855 passed, 13 skipped. Baseline byte-identical.
…US4) Completes feature 036. Machine-readable surfaces plus a spec gap found during the manual walkthrough. Output surfaces: - JSON: the full shape already passed error_class through (it serializes CheckResult verbatim); the summary shape stripped it. Extracted _compact_result to make FR-011 testable and to keep the field -- a summary that hides "we could not verify" would let a CI job report an unreachable network as a compliance failure. - SARIF: properties["errorClass"], camelCase to match the sibling resolvingPassHandler / passHistory keys. - Attestation predicate: additive next to authority, no schema version bump (feature 025 set that precedent). A signed attestation carrying a bare FAIL when the check never reached GitHub is exactly the misleading claim Principle II forbids. - CLI text output: also annotated. The quickstart promised "FAIL [auth]" in terminal output, and only the MCP-side markdown formatter had been wired -- the doc was writing a check the code could not cash. Spec gap found by the T032 walkthrough, worth calling out: FR-009a says error_class comes from the RESOLVING pass. But when every pass is inconclusive there IS no resolving pass, and that is the most common shape for a degraded audit -- so a fully broken environment reported a bare "manual verification required" with no hint the token had expired. That defeats US1 entirely. Added a scoped fallback: with no conclusion to supersede it, the most recent environmental classification is the best available explanation. This extends FR-009a's letter while serving its intent (a later conclusion supersedes an earlier failure); it only fires where FR-009a is silent. Getting the fallback right took two attempts. "Last pass's value" read plausibly but was wrong: nearly every baseline control ends with a `manual` pass, an ask-a-human placeholder that always returns INCONCLUSIVE and can never conclude anything. Treating that as "the final attempt ran cleanly" wiped the real exec failure preceding it. Corrected to last non-None, with a test for that exact shape. Verified end to end against a live repo with an invalid GH_TOKEN: 26 results split 20 unannotated / 6 classified (1 auth, 4 network, 1 not_found). The auth case is correctly distinguished from the 404s, and every genuine finding stays unannotated. Also fixes a bug from feature 418 (PR darnitdevorg#424): inserting _atomic_write_text after MCP_DEFAULT_TIMEOUT_SECONDS orphaned that constant's attribute docstring. Full suite: 2870 passed, 13 skipped. Baseline byte-identical. validate_sync passes. ruff clean.
mlieberman85
force-pushed
the
036-tier2-error-class
branch
from
September 8, 2026 22:08
3af6099 to
290b6a0
Compare
The fallback was added during implementation (T032 manual walkthrough) and until now existed only in code comments and the PR body. Folding it into the spec so the behavior is a stated requirement rather than an undocumented implementation detail. FR-009a says error_class comes from the RESOLVING pass. When every pass returns INCONCLUSIVE and the control terminates WARN, there is no resolving pass, so FR-009a has nothing to select -- and the strict reading leaves the single most common degraded-audit shape with no explanation at all. An operator whose token expired sees a bare "Could not automatically verify" and never learns why. FR-009b covers that gap: with no conclusion to supersede it, the most recent non-null error_class across the chain propagates. It applies only where FR-009a is silent and preserves FR-009a's ordering intent, so the two do not conflict. Non-null rather than simply most-recent is the load-bearing detail. Nearly every OpenSSF Baseline control ends with a `manual` pass, an ask-a-human placeholder that always returns INCONCLUSIVE and can never conclude anything. Reading it as "the final attempt ran cleanly" erases the real exec failure preceding it -- exactly the case the requirement exists to serve. Contract section 3.2 carries the truth table. Also records in the spec-quality checklist that FR-009b was added retroactively, so a later reader can see the requirement postdates the clarify session rather than assuming it was designed up front. No code change -- the implementation already matches, with coverage in TestAllInconclusiveWarnFallback.
Collaborator
|
Tested on Linux, works as described: with an invalid |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #419.
Summary
An audit run behind an expired token, a corporate proxy, or a GitHub rate
limit currently produces failures that are indistinguishable from a check
that ran cleanly and found the repository non-compliant. Both gate the
audit identically (WARN counts as FAIL for compliance math, unchanged),
but they demand opposite operator responses: fix the runner vs fix the
repo.
This adds a structured
error_classto the result envelope, classified atfour producer sites, surfaced in every output surface, and carried into
the in-toto attestation.
Before:
After:
Design
ErrorClassis a six-valueLiteral(network,auth,timeout,rate_limit,not_found,crashed) in a newcore/error_class.py,paired with a runtime
ERROR_CLASSESfrozenset.Both are needed.
typing.Literalis erased at runtime and enforcesnothing, so the frozenset is what
HandlerResult.__post_init__validatesagainst. This mirrors
core/authority.py's Literal+frozenset pairing butdiverges in how the frozenset is used:
authority.pybacks a fail-safe(unknown authority can never conclude a control, which is safe because
"cannot conclude" is a conservative default). An unknown
error_classhas no safe default -- it is neither "the check failed" nor "the check
could not run" -- so this module's frozenset backs a raise.
Two invariants enforced in
__post_init__:error_classalongsidestatus=PASSrejected -- a handler that couldnot complete cannot have produced a real pass.
Additive throughout:
HandlerResult,SieveResult, andCheckResult["error_class"](NotRequired, emitted only when set), andthe attestation predicate gets a conditional field with no schema version
bump -- the same treatment feature 025 gave
authority.Classification
exechandler: timeout ->timeout, missing binary ->not_found,undeclared non-zero exit classified from stderr (GitHub rate-limit
patterns first, then auth, else
network). Rate-limit before authbecause GitHub answers 403 for both and the rate-limit body is the more
specific signal.
mcphandler: all nine exception paths mapped.Orchestrator: a handler that raises ->
crashed.Context auto-detect: git failures classified and logged. A non-zero exit
is git's legitimate answer ("no such remote") and stays quiet -- warning
there would make every single-remote repo noisy, which is how loud
logging becomes ignored logging.
Two paths deliberately carry no
error_class: success, and a declaredfail_exit_codeshit. That second one is the invariant that keeps thefeature honest -- if real findings started getting annotated, the
annotation would become noise. There is an explicit test for it.
The subtle bug this had to fix
_apply_cel_exprbuilds brand-newHandlerResultobjects at threeconstruction sites rather than mutating the input, so any field not
threaded explicitly is lost with no error -- the verdict stays correct
and only the failure cause disappears. Feature 026 hit this identical bug
with
authority.Tests were written first and confirmed failing on exactly the three
constructing branches (the five pass-through paths were already safe),
then passing after the fix.
Spec gap found during the manual walkthrough
FR-009a says
error_classcomes from the RESOLVING pass. But when everypass is inconclusive there IS no resolving pass -- and that is the most
common shape for a degraded audit. A fully broken environment reported a
bare "manual verification required" with no hint the token had expired,
which defeats the feature's primary user story.
Added a scoped fallback: with no conclusion to supersede it, the most
recent environmental classification is the best available explanation. It
extends FR-009a's letter while serving its intent, and only fires where
FR-009a is silent. Worth a look during review -- this is a semantic
decision that may belong back in the spec.
Getting it right took two attempts. "Last pass's value" read more
principled but was wrong: nearly every baseline control ends with a
manualpass, an ask-a-human placeholder that always returnsINCONCLUSIVE and can never conclude anything. Treating that as "the final
attempt ran cleanly" wiped the real exec failure preceding it. Corrected
to last non-None, with a test for that exact shape.
SC-002: byte-for-byte happy-path invariance
The baseline in
tests/darnit/fixtures/error_class_baseline/was capturedfrom unmodified
mainin its own commit (75c3bbf) BEFORE anyimplementation landed. Goldens generated during implementation would be
circular -- they would lock post-feature behavior rather than prove
pre-feature equivalence.
Deliberately plain file comparison, not
syrupy:--snapshot-updatewould let a real regression be absorbed into the expected value.
Three fields are scrubbed as run- or machine-varying, none related to this
feature: the fixture tempdir path,
pass_history[].duration_ms, andformat_results_markdown'sGenerated At:stamp. That last one is agenuine Tier 1 determinism gap -- markdown audit reports can never be
byte-for-byte reproducible while the formatter embeds a wall-clock
timestamp. Out of scope here; flagged as a follow-up on #418.
Test plan
preservation across all transitions, WARN logging at all four sites,
validation rules, output surfaces, happy-path invariance, no-new-dep).
point).
validate_sync.pypasses.ruff checkclean.GH_TOKEN: 26results split 20 unannotated / 6 classified (1
auth, 4network,1
not_found). The auth case is correctly distinguished from 404s andevery genuine finding stays unannotated.
Also in here
Fixes a bug I introduced in #424: inserting
_atomic_write_textdirectlyafter
MCP_DEFAULT_TIMEOUT_SECONDSorphaned that constant's attributedocstring, leaving it dangling as a bare string expression after an
unrelated function.
Follow-ups
format_results_markdown'sGenerated At:timestamp (Determinism Tier 1: normally deterministic operations must be deterministic across runs #418 territory).audits show they are needed -- clarify Q4 scoped v0 to GitHub only.
dataclasses.replace()refactor of_apply_cel_exprthat wouldmake field-dropping structurally impossible. Right long-term fix for
that bug class, larger blast radius than this feature warranted.
error_classin attestation alongside LLMreasoning.
Spec
Full spec, clarifications, plan, research, data model, contract, and
tasks in
specs/036-tier2-error-class/.