feat(sdk): add principal, reporting, and controller support - #1121
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean additive public-surface expansion with no blocking or medium findings.
What I checked:
- Purely additive public surface across new modules (
adcp.principal,adcp.reporting_inspection,adcp.server.principal) and type re-exports; correctfeat:semver signal — no breaking removals/renames/required-flips that would demandfeat!:. - Import layering intact:
aliases.pymaps traversal-orderResult6/17/19to stable names; no non-allowlisted module imports generated identifiers. - Security-critical paths are fail-closed and tested:
HttpsReportingResourceReaderis redirect-free / DNS-pinned / trusted-origins-only / IP-literal-rejecting; server principal CAS + credential-stripping keeps secrets out of state and the idempotency replay cache; reporting tiers rejectreconciled_billingwithoutmanaged_delivery. - Public API snapshot updated to match.
high_risk note: flag is true, but reasons are new files (added) or (modified) files with no medium-or-higher findings — presumed safe per the change-kind rule. No deletions. Not escalation-worthy on the flag alone.
gated_paths: false — row 2 does not apply despite review_decision: REVIEW_REQUIRED.
No no-auto-approve team match. No findings at any tier. One non-blocking observation from the reviewer: enforcing the canonical comply_test_controller schema now requires account on every call (sandbox-only, pre-1.0 conformance tightening) — worth a changelog/migration note, but not a blocking or medium finding.
Rows 1–8 do not fire → row 9 approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Additive PR (#1121) introducing principal/reporting/test-controller support (~4900 lines across new and modified non-generated files). No blocking or medium findings.
Checked:
aliases.pytraversal-number→semantic mappings verified correct against generated Result classes; no codegen renumber churn breaking the alias layer.- Import layering and PEP 562 lazy facade preserved; no direct generated-name imports leaking out of the allowlist.
- Semver signal holds — purely additive
feat, no removals, renames, required↔optional flips, or response-shape changes on the public surface. - Idempotency digest correctly strips
context/governance_context; no credentials landing inctx_metadata. - SSRF/gzip/schema hardening in
reporting_inspection.pysound; CAS concurrency loops inserver/principal.pyre-fence on miss.
Two non-blocking notes (not findings): the canonical schema now makes account required for every controller request including the list_scenarios probe (deliberate spec conformance — worth a changelog line), and the full suite was rerunning at authoring time (CI gates the merge).
high_risk: true is driven by modified/added files under src/adcp/server/** and src/adcp/types/**, but the reviewer surfaced no medium-or-higher concern on any modified file, and the one added file (server/principal.py) is new scaffolding — neither triggers escalation. No gated paths, no no-auto-approve team match. Rows 1–8 do not fire; falls through to row 9.
Summary
Safety and compatibility
Verification
Closes #1110
Closes #1106
Closes #1103