Repository navigation
fix(release): preserve beta validation and document adoption contracts - #1461
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean PR (+93/-9, 6 files), no blocking or medium findings.
Checked:
ci.ymlconcurrency change is correct: push triggers only on main/setup branch; grouping non-PR runs bygithub.shawithcancel-in-progress:falsegives each main commit its own CI group (a tagged beta's CI survives later main merges), while PR runs still cancel superseded refs.- Release-publish gate still pins
--commit "$RELEASE_SHA"atheadBranch==mainsuccess before detached checkout — now locked by the new policy test. WEBHOOK_ACCEPTED_ADCP_USESis an additive, behavior-preserving extraction insrc/adcp/signing/constants.py; README/migration references all check out.- New webhook-purpose conformance test asserts step 8 /
webhook_signature_key_purpose_invalid.
High-risk flag is true only via (modified) entries under src/adcp/signing/**, but with zero medium-or-higher findings the modifications are presumed safe (row 5 does not fire without a medium finding). gated_paths is false, no no-auto-approve team match. Rows 1–8 do not fire; falls through to row 9.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, net-new surface since the prior clean approve is documentation only.
No blocking findings. No medium findings.
Checked:
- README.md (SDK 9 beta section, nullable-path cache guidance) and docs/types-9-migration.md (strict ProductFormatDeclaration guidance) — every documented symbol and import path (nullable_request_paths, is_canonical_format_kind, CanonicalFormatKindStr, LegacyProductFormatDeclaration, FormatKindNotInClosedSetError, validate_format_kind_in_options, project_declaration_to_v1, params_as/legacy_format_refs) verified to exist at base.
- high_risk is true only because src/adcp/signing/** files appear as (modified), but that delta is main-merge churn already present in the base SHA and absent from the diff vs base — no medium-or-higher finding attaches, so no escalation under rows 3–5.
- gated_paths is false (row 2 N/A despite review_decision=REVIEW_REQUIRED).
- No no-auto-approve team match (row 7 N/A).
- README documenting 9.0.0b3 vs manifest beta.2 is expected release-prep behavior, not a finding.
Prior decision was approve, so sticky escalation (row 6) does not apply. No rows 1–8 fire → row 9 approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Delta since the prior clean approve is one commit of CI/release budget plumbing (ci.yml, release-publish.yml) plus a regression test. Step budgets raised to 70/55 min with +15 job overhead (85/70), publish wait extended to 220×30s=110 min, build-job timeout to 120 min. The release gate stays fail-closed (exit 1 after the loop). New test asserts the budgets cover the full-suite steps — assertions hold, including several at exact boundaries. The signing files (constants.py, webhook_verifier.py) carry no public-surface, wire-shape, or type-layering impact.
No Critical/High/Medium findings. high_risk is true only for (modified) signing files, but the reviewer found no medium-or-higher concerns there, so the modifications are presumed safe (not an escalation trigger on the flag alone). gated_paths is false; no no-auto-approve team match. Rows 1–8 do not fire → row 9 approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — scheme-normalization hardening on geo-place-system key validation, clean pass.
The delta adds a literal https:// prefix check in _geo_place_keys._validate_geo_system_key before handing the value to the AnyUrl adapter. This closes a real gap: AnyUrl lowercases the scheme, so HTTPS://... would have slipped past allowed_schemes=["https"] even though the schema's ^https:// pattern applies to the raw opaque string (geo-place-system.json treats URI systems as exact opaque strings). The fix preserves registered-token bypass, keeps the adapter's host/URI checks, and rejects no valid value. Tests cover uppercase, mixed-case, leading-space, and backslash forms.
Decision table walk-through:
- Rows 1 (critical/high): no findings.
- Row 2 (gated_paths): gated_paths is false — not triggered despite review_decision REVIEW_REQUIRED.
- Row 3 (deleted high-risk): the two signing-path files are (modified), not deleted.
- Rows 4/5 (medium findings): no medium findings exist.
- Row 6 (sticky escalate): prior decision was approve, not escalate.
- Row 7 (no-auto-approve team): no team match.
- Row 8 (≥3 medium): none.
- Row 9: approve.
high_risk is true only because signing files were modified, but the reviewer surfaced no medium-or-higher concern on them, so the modifications are presumed safe. Prior run on an earlier head was also a clean approve. Falls through to row 9.
Later main merges could cancel CI required to publish an already tagged beta. Give each main commit its own CI group, while superseded PR runs still cancel. The publisher continues to require successful full main CI at the exact release SHA. Recent main coverage took 53 minutes against a 55-minute step cap; raise coverage/full-test budgets to 70/55 minutes, keep 15 minutes of job overhead, and extend the publication wait and build-job budget to cover the longest main lane plus queueing.
Also expose
WEBHOOK_ACCEPTED_ADCP_USESfromadcp.signing.constantsand have webhook verification use it. Like the existingADCP_USE_*constants, it is intentionally imported from the constants module rather than the package root. Document SDK 9 prerelease installation and the per-task/per-version nullable-path cache, including the distinction between deadlines before and after dispatch. Preserve the geographic namespace schema's literalhttps://prefix before URL parser normalization. Qualify the open-format migration guidance to reflect the strictProductFormatDeclarationclass restored in #1447.Validation:
Fixes #1422
Fixes #1448
Fixes #1418
Fixes #1381