fix(release): verify all protected main policies - #1201
Conversation
d6e7c22 to
81610de
Compare
|
The PR is ready and aao review is running; branch CI is green. A local run of the new main-push policy found a merge blocker that PR CI does not exercise: nine normal GitHub merge subjects are rejected, and the already-merged #1174 breaking commit has no footer. I am correcting validation of normal two-parent merge messages without skipping their conventional message. The coordinator has been asked whether the stricter footer policy should apply prospectively or keep the historical footer as a blocker. Please hold merge until this is resolved. |
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — release-gate hardening with no blocking findings.
Reviewed: adds native read-only IPR-signature and conventional-commit main-policy CI jobs, folds the classic branch-protection inventory into validate_checks (only ever adds required checks, tightens App-binding to a positive int), fail-closed throughout (unreadable inventory raises, bot exemption keyed on API account type rather than login, breaking footer checked in the actual commit body). No changes to the public adcp.* surface, no changeset impact, and comprehensive test coverage in tests/test_main_release_policies.py and tests/test_release_guard.py.
Decision-table walk: no critical/high/medium findings (row 1 n/a); gated_paths false (row 2 n/a); high_risk false (rows 3/5 n/a); no medium data-loss/schema/infra finding (row 4 n/a); no prior escalation (row 6 n/a); no no-auto-approve team match (row 7 n/a); fewer than three medium findings (row 8 n/a). Falls through to row 9 → approve. Note review_decision is REVIEW_REQUIRED, but with gated_paths: false that does not force a comment or escalate.
Exact-head technical verdict —
|
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean review, no blocking findings.
Checked: this PR hardens CI/release policy tooling. check_main_policies.py adds validate_commit unwrapping of GitHub two-parent merge bodies plus a SHA+subject-pinned historical footer exception for #1174, with matching tests and docs. Verified the exception is fail-closed (only the footer is waived; the load-bearing ! semver signal is preserved) and consistent across code, tests, and both docs. No touches to the public adcp.* surface, generated code, forward-compat/type-layering modules, or ctx_metadata. CI gates remain armed (no disabled tests, no dropped ruff rules, no blanket type-ignores). Changes are confined to CI workflow, release scripts, tests, and docs.
Decision-table walk: no critical/high/medium findings (row 1 n/a); gated_paths=false (row 2 n/a); high_risk=false, no deletions (rows 3–5 n/a); prior decision was approve, not escalate (row 6 n/a); no no-auto-approve team match (row 7 n/a); zero medium findings (row 8 n/a) → row 9 approve. Consistent with the prior clean approve pass; nothing new to surface.
|
Updated to The new actual-history regression failed on #1192 before the fix. The corrected direct main-push check passes all 28 integrations; 282 focused tests, lint, all type checks and normal hooks pass. The migration boundaries are documented in the runbook and release notes. Fresh CI and the full local |
|
Coordinator check at 86adc6d: ran the conventional main-push policy against current main 2bfbca4; all 28 first-parent integrations passed. The historical exceptions are bound to the exact SHA and subject, and 55 focused policy tests passed. Fresh CI and exact-head independent approval are still required before merge. |
Exact-head technical verdict —
|
| case | result |
|---|---|
| exempt subject, different SHA | rejected |
| exempt SHA, different subject | rejected |
| brand-new breaking, no footer, no SHA | rejected |
| brand-new breaking, random SHA | rejected |
| exempt SHA + exempt subject | accepted |
| new breaking with footer | accepted |
Both keys are still required, exact-match, with no pattern matching and no blanket "merges are exempt". The prospective boundary is unchanged: any new breaking commit — merge or squash — still needs its footer.
Your own test_historical_footer_exception_requires_exact_commit_and_subject covers the same eight vectors, including the cross-contamination case (34c8f6d9's SHA paired with the other exception's subject → rejected). Independent probe and the test agree.
The pinned history fixture is a good call
tests/fixtures/release_main_history.json captures real committed messages and parents, so the regression runs on a shallow CI checkout with no network and no dependence on a moving origin/main. That's the right way to keep this honest — the previous two heads were only caught by running against live main, which CI wouldn't have done.
Optional nit: assert len(result["commits"]) == 28 freezes the count alongside the fixture, so the two have to move together. Harmless, just a maintenance coupling worth knowing about.
Carried forward from the earlier heads, still clean
- Policy inventory (
validate_checks): classicbranches/mainprotection read alongside the ruleset;set(contexts) == {checks[].context}prevents a requirement existing in one view without a binding in the other;type(app_id) is int and app_id > 0is a real tightening overis not None; thebranch["commit"]["sha"] == context.targetbind prevents reading protection from another state;CI_FLOORstays satisfiable by the ruleset alone, so classic protection can only add bindings. CI_FLOOR↔ci.yml: all 14 entries resolve, including the four from the templated matrix name.- Artifact byte binding: byte-level — digest compared against downloaded content, exact member set and count (catches duplicate zip names),
MAX_UNPACKEDbound, symlink rejection, noextractall. Pre-existing and untouched here; verified, not credited to this PR. - Workflow hardening: pinned action SHA,
persist-credentials: false, per-jobpermissions, one fewer third-party action,GITHUB_WORKFLOW_SHAbound to the target.
What I ran at 86adc6d0
tests/test_main_release_policies.py+tests/test_release_guard.py— 276 passedmake lint— clean- Direct main-history check via
main_commits()+validate_commit()against2bfbca4b9 - Six independent exception-leak probes
CI at time of writing: 17 SUCCESS / 30 pending, no failures.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
The diff symmetrically extends the historical breaking-footer exception in check_main_policies.py to a second already-merged commit (#1192, 34c8f6d) with a fail-closed SHA+subject guard, paired with matching docs in releasing.md and reporting-release-notes.md, plus a real-history regression fixture and test replaying the 28-commit first-parent chain.
No public-surface/semver, forward-compat, type-layering, generated-code, credential, or CI-gate concerns. No high-risk paths touched; gated_paths is false; no author-team gate. This is a net-positive follow-up to the prior clean approve.
Decision table: rows 1–8 do not fire (no findings, no gate triggers, high_risk false, no no-auto-approve team, fewer than three medium findings) → row 9 approve. Note review_decision is REVIEW_REQUIRED but gated_paths is false, so row 2 does not apply.
|
Lint, all type checks, 282 focused tests and the direct 28-commit main-history policy check also pass. Current-head aao approval is in place, with no open review threads. CI is down to the Python 3.12 coverage job; all other checks are green. #1201 remains ready and held for #1217 to merge first. Rebase only if it conflicts; the final combined main will run fresh CI before any release proposal. |
Release acceptance now checks both effective rulesets and branch-protection inventories, with explicit GitHub App bindings. Main CI runs read-only IPR and conventional-commit checks, and publication/recovery rejects substituted wheel or sdist bytes.
The commit policy validates integrations after published beta.15. Normal two-parent GitHub merges must contain a conventional subject and any required breaking footer in their committed body; a single-parent commit cannot impersonate that merge wrapper. Conventional merge subjects are also validated directly.
The only historical footer exceptions match both the commit and its exact committed conventional subject:
6c5ee7b29ef9d548ae2bb3665032e0c53a8146ac,fix(reporting)!: scope configuration generations by account (#1174).34c8f6d929aeac3407e2f595104a8e903e572623,feat(reporting)!: enable production tier status and ownershipin the merge body.Every other breaking commit requires a footer, including a new commit reusing either title. The runbook and release notes preserve both migration requirements. There is no moving date or history cutoff; historical IPR validation is unchanged.
Validation:
make lint,make typecheck-all, 282 focused release-policy/configuration tests, script mypy, Black/Ruff and normal commit hooks pass. The direct main-push policy passes all 28 current main integrations. The regression fixture uses those actual committed messages and parents so shallow CI checkouts exercise the same history offline; it failed on #1192 before the fix. Full localmake testpassed: 12,187 passed, 2,359 skipped, 9 deselected, 1 xfailed; coverage 81.58% against the unchanged 80% floor.Keep this PR ready and merge after #1217, with green CI and current-head approval. Rebase only if GitHub reports a conflict. The final combined main must pass fresh CI before any release proposal; the coordinator will verify gates and arrange the human merge.