Skip to content

feat(baseline): ruleset-aware branch-protection verdict (closes #343) - #384

Open
mlieberman85 wants to merge 1 commit into
darnitdevorg:mainfrom
mlieberman85:032-ruleset-branch-protection
Open

feat(baseline): ruleset-aware branch-protection verdict (closes #343)#384
mlieberman85 wants to merge 1 commit into
darnitdevorg:mainfrom
mlieberman85:032-ruleset-branch-protection

Conversation

@mlieberman85

Copy link
Copy Markdown
Contributor

Summary

Follow-up to feature 019, addresses @justaugustus's comment on #343.

Feature 019 taught the sieve to treat a 404 from /repos/{owner}/{repo}/branches/{branch}/protection as a definitive FAIL for the four branch-protection controls (OSPS-AC-03.01, OSPS-AC-03.02, OSPS-QA-03.01, OSPS-QA-07.01). GitHub Repository Rulesets -- the newer protection mechanism -- also produce a 404 on the classic endpoint while genuinely protecting the branch, so the shipped fix produces false FAILs for repos protected via rulesets.

This PR introduces a new sieve handler github_branch_protection (registered by darnit-baseline) that reconciles the two surfaces:

  • Classic 200 with the required signal -> PASS from classic (fast path; rulesets not consulted).
  • Classic 404 or classic 200 without the required signal -> consult rulesets. Any active ruleset whose conditions.ref_name covers the audited branch and carries the right rule type/parameter -> PASS from ruleset. Preserves cross-surface layering.
  • Both surfaces respond and neither protects -> FAIL. Locks the feature 019 invariant on the true-negative path.
  • Either surface returns 401/403/429/5xx or a mid-pagination fetch fails -> INCONCLUSIVE (WARN via the trailing manual pass). Preserves "WARN means unknown" on ambiguous responses.

A shared helper gh_api_with_status is added to darnit.core.utils so callers can distinguish 404 from 403 from 5xx by parsing the HTTP <code>: prefix from gh's stderr. Existing gh_api() and gh_api_safe() become thin wrappers with unchanged call contracts.

Uses gh api --paginate for the rulesets list; a mid-page fetch failure resolves to WARN with source partial-fetch, not a silent truncation. Default-branch value used for ~DEFAULT_BRANCH include-list matching is consumed from HandlerContext.default_branch -- no extra GET /repos/{owner}/{repo} call is introduced (SC-004 API-call budget invariant, locked by a dedicated integration test).

Zero new runtime dependencies. Zero product-source changes under packages/(darnit-gittuf|darnit-reproducibility|darnit-hello)/src/.

Spec: specs/032-ruleset-branch-protection/ (63 tasks, all closed; three clarifications recorded during /speckit-clarify).

Closes #343.

Non-goals deferred to v0.1

  • Organization-level inherited rulesets (this PR checks repo-level rulesets only).
  • Evaluate-mode rulesets (only enforcement = \"active\" counts as protection).
  • Glob-pattern ref-name matching (globs treated as non-matching and surfaced in the FAIL evidence's considered_rulesets).

Test plan

  • pytest tests/darnit_baseline/test_branch_protection_handler.py -- 48 pass (ruleset matching, PASS/FAIL/WARN per requirement, config validation).
  • pytest tests/darnit_baseline/test_branch_protection_integration.py -- 7 pass (one per TOML control PASS via ruleset, one FAIL when no protection, one exact API-call budget assertion locking SC-004, one WARN via manual fallback).
  • pytest tests/darnit_baseline/controls/test_branch_protection.py -- 8 pre-existing feature-019 tests updated to the two-surface semantics; FAIL invariant preserved for the true-negative path.
  • pytest tests/darnit/core/test_gh_api_status.py -- 19 pass (status parsing across 2xx/4xx/5xx, paginate flag, wrapper contracts, gh-not-found).
  • Full workspace pytest tests/ --deselect .../test_upstream_spec_unchanged -- 2815 pass, 17 skip, 0 fail.
  • ruff check on all feature-touched files -- clean.
  • python scripts/validate_sync.py --verbose -- PASS.
  • Structure decision guard: zero files touched under other product packages' src/.
  • FR-013 no-new-runtime-dep guard: zero pyproject.toml diffs.
  • Reviewer: manual smoke-test against a real repo protected exclusively via a default-branch ruleset (audit should now PASS all four affected controls where it previously FAILed). Quickstart in specs/032-ruleset-branch-protection/quickstart.md documents the recipe.

Backward compatibility: additive. The github_branch_protection handler is new; the four TOML controls' passes swap from exec to the new handler while keeping their trailing manual passes unchanged. Every non-branch-protection control's verdict is untouched. The gh_api / gh_api_safe public contracts are preserved.

… don't false-FAIL (closes darnitdevorg#343)

Feature 019 taught the sieve to treat a 404 from
`/repos/{owner}/{repo}/branches/{branch}/protection` as a definitive
FAIL for the four branch-protection controls (OSPS-AC-03.01,
OSPS-AC-03.02, OSPS-QA-03.01, OSPS-QA-07.01). Repository Rulesets --
the newer protection mechanism -- also produce a 404 on that endpoint
while genuinely protecting the branch, so the shipped fix produces
false FAILs for repos protected via rulesets. Reported by
@justaugustus as a follow-up to the 019 fix.

Adds a new sieve handler `github_branch_protection` registered by
`darnit-baseline` that reconciles the two surfaces:

- Classic 200 with the required signal -> PASS from classic
  (rulesets not consulted; fast path).
- Classic did not carry the signal (404 or 200 without the specific
  field) -> consult rulesets. If any active ruleset whose
  `conditions.ref_name` covers the audited branch carries a rule of
  the right type/parameter -> PASS from ruleset.
- Both surfaces respond and neither protects -> FAIL. Locks the 019
  invariant on the true-negative path.
- Either surface returns 401/403/429/5xx or a mid-pagination fetch
  fails -> INCONCLUSIVE, which falls through the trailing manual
  pass to WARN. Preserves the "WARN means unknown" semantic on
  ambiguous responses.

Two-surface layering (Q1 clarification): a repo whose classic
protection requires PRs but delegates status-checks to a ruleset
correctly PASSes OSPS-QA-03.01 via the ruleset without changing the
other controls' verdicts.

Adds a shared helper `gh_api_with_status` in `darnit.core.utils`
that parses `HTTP <code>:` from `gh`'s stderr on non-zero exit so
callers can distinguish 404 from 403 from 5xx. Existing `gh_api()`
and `gh_api_safe()` are refactored as thin wrappers preserving
their exact call contracts (~30 existing callers unchanged).

Uses `gh api --paginate` for the rulesets list so repos with more
rulesets than a single page get full enumeration; a mid-page fetch
failure resolves the affected control to WARN with source
`partial-fetch`, not a silent truncation.

No new runtime dependencies. Zero touches under
`packages/(darnit-gittuf|darnit-reproducibility|darnit-hello)/src/`.
The default-branch value used for `~DEFAULT_BRANCH` include-list
matching is consumed from `HandlerContext.default_branch` (populated
by the audit driver) -- no extra `GET /repos/{owner}/{repo}` call is
introduced, matching SC-004's exact API-call budget.

Test coverage:
- 48 handler unit tests (ruleset matching, PASS/FAIL/WARN paths per
  requirement, config validation).
- 19 unit tests for `gh_api_with_status` (status parsing across
  2xx/4xx/5xx, paginate flag, wrapper contracts).
- 7 integration tests through the sieve orchestrator (one per TOML
  control PASS via ruleset, one FAIL when no protection, one exact
  API-call budget assertion, one WARN via manual fallback).
- 8 pre-existing feature-019 tests updated to the two-surface
  semantics without weakening FAIL invariants.

Full workspace: 2815 pass, 17 skip, 0 fail. Ruff clean.
`validate_sync.py` PASS. Structure decision and FR-013 no-new-dep
guards satisfied.

Non-goals deferred to v0.1:
- Organization-level inherited rulesets (repo-level check only).
- Evaluate-mode rulesets (only `enforcement = "active"` counts).
- Glob-pattern ref-name matching (globs treated as non-matching
  and surfaced in `considered_rulesets` for the operator).

Spec: `specs/032-ruleset-branch-protection/` (63 tasks, all closed).
Closes darnitdevorg#343.
@mlieberman85

Copy link
Copy Markdown
Contributor Author

CI is red on tests/darnit/context/test_dot_project_upstream.py::TestUpstreamSpecSync::test_upstream_spec_unchanged -- the feature-030 CNCF-drift canary. Unrelated to this PR's diff.

Root cause: CNCF added RepositoryEntry to their .project/ types.go on 2026-08-21 (cncf/automation@bd7fec94), which is real schema evolution against our mapper. Filed as #385 with the drift details + a feature-030-style reconciliation plan.

Note the test is @pytest.mark.upstream and its docstring says "It does NOT block PRs -- it's informational to alert maintainers when the upstream spec evolves." It's blocking here because tests/darnit/parity/tier1/conftest.py:32's pytest_collection_modifyitems applies the integration marker to every collected item globally (not just tier1's own tests), so CI's -m integration step picks up the canary. Also noted in #385 as a side item worth constraining, since it will keep catching PRs until either the drift is reconciled or the marker scope is narrowed.

Feature-032 diff itself is green: 82 new + updated tests pass, other 2733 workspace tests pass. Happy to rebase after #385 lands or (if reviewers prefer) chain a small hash-only bump into this branch as a stopgap once we confirm the RepositoryEntry object form isn't in the wild yet.

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.

1 participant