Repository navigation
feat(reporting): make the grant journal the one source of authorized_at - #1740
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — additive feat slice, grant journal becomes the single source of authorized_at.
What I checked:
- Semver signal: all changes additive, no public signature breaks, forward-compat unions and generated code untouched — non-breaking prefix is correct.
- Import layering holds; no credentials land in
ctx_metadata. - Chain rule routes through
coverage._chainsso projection and coverage agree; source round-trips byte-identically.
Medium findings (1):
- src/adcp/reporting/subscriptions/managed.py:933 — list_accounts projection does one store
_mutateper account, making list_accounts/sync_accounts O(N) round-trips on a load-bearing read. Worth a follow-up (batch the projection), plus the noted divergence whenallowed_tasks is Noneskips projection for unscoped grants.
Decision path: No critical/high findings (row 1 n/a). gated_paths is false (row 2 n/a). high_risk is true but reasons are (added)/(modified) with no deletions (row 3 n/a). The single medium is a performance concern, not data-loss/schema/infra (row 4 n/a). Row 5 requires a medium on a (modified) high-risk file — the lone medium is in managed.py, not a high-risk-path file, so row 5 does not fire. No prior decision (row 6 n/a). No no-auto-approve team (row 7 n/a). Only 1 medium, below the 3-medium threshold (row 8 n/a). Falls through to row 9 → approve.
Medium findings
- src/adcp/reporting/subscriptions/managed.py:933 — list_accounts projection does one store _mutate per account
478f129 to
7853e83
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — net improvement over prior run, one non-blocking medium remaining.
What I checked
- Delta adds a grant journal:
authorization_chain_start,observe_grant(s),SubscriptionGrants.changed, request-pathproject_accounts,wind_down_grant. - Prior Medium (managed.py:933 — one
_mutateper account) is RESOLVED:observe_grantsnow does one load + at most one CAS per page, with a test asserting loads==1. Good iteration. - Projection is fail-open and bounded (no write for non-subscribers, none when document unchanged); encode/decode
sourceround-trip and_grant_sourceszip-strict are sound.
Medium findings (non-blocking)
- src/adcp/reporting/subscriptions/managed.py:1028 — Request-path projection skips full grants whose authorization carries no
allowed_tasks, so request-path authorized_at/regression guarantee diverges from reconcile for the unscoped case. Single medium, category is not data-loss/schema/infra, so no escalation trigger.
Decision trace
- Row 1 (critical/high): none present.
- Row 2 (gated_paths): false — N/A; also review_decision=APPROVED.
- Row 3 (deleted high-risk): no deletions.
- Row 4 (medium + data-loss/schema/infra): the lone medium is a reporting-projection correctness gap, not data-loss/schema/infra.
- Row 5 (high_risk modified + medium): high_risk is true via decisioning/handler.py and server/init.py (modified), but the single medium is in managed.py — NOT in a modified high-risk file. Row 5 requires the medium to co-occur with a modified high-risk path; the reviewer placed the medium outside those files, so this row does not fire. The reporting_grants.py high-risk match is (added), inherently low risk.
- Row 6 (sticky escalate): prior decision was approve, not escalate.
- Row 7 (no-auto-approve team): no match.
- Row 8 (≥3 mediums): only 1 medium.
- Row 9: approve.
Medium findings
- src/adcp/reporting/subscriptions/managed.py:1028 — Request-path projection skips full grants (authorization without allowed_tasks)
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — prior Medium finding resolved, no current findings.
This is a subsequent review following a prior approve. The developer's delta resolves the previously-noted Medium in managed.py:1028: the request-path projection no longer gates on allowed_tasks is not None and now emits a full-grant observation (allowed_tasks=None) for authorizations lacking the key, matching _covers() and the canonical reconcile vectors (null = "restricts no task"). The fix correctly preserves the [] vs None distinction, and an added test covers the previously-skipped authorized_at regression path.
Current reviewer findings: none.
Decision-table walk:
- Rows 1 (critical/high): no findings → skip.
- Row 2 (gated_paths): false → skip. (review_decision is also APPROVED.)
- Row 3 (high-risk deleted): high_risk is true, but reasons are (modified)/(added) only, no deletions → skip.
- Row 4 (medium data-loss/schema/infra): no medium findings → skip.
- Row 5 (high-risk modified + medium): no medium findings → skip.
- Row 6 (sticky escalate): prior decision was approve, not escalate → skip.
- Row 7 (no-auto-approve team): no team match → skip.
- Row 8 (≥3 medium): zero medium → skip.
- Row 9: approve.
high_risk is true only on (modified)/(added) sensitive paths with no medium-or-higher concern, so per the high-risk guidance those modifications are presumed safe. Nets to a clean improvement.
e1aac65 to
457e803
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
Checked:
- Grant journal is now the single source of truth for
authorization.authorized_atacrosscoverage.py:142(authorization_chain_start),engine.py(_observe/observe_grants/refresh), request-path projection inmanaged.py(project_accounts) andhandler.py:3317/3360for servedlist_accounts/sync_accounts, pushed grants viareporting.grants.changed, and the newwind_down_grantinreporting_grants.py. - Re-grant dating / chain-hold logic in
_observe(engine.py:446) traced against shared vectors: min(reported,at) adoption, retention-bounded chain continuation, and regression rejection all line up;_chainsalways seeds chain[0] sochain_starthas no IndexError. - Changes are additive to the public surface under
feat(reporting):/fix(reporting):— correct semver, no breaking wire or discriminated-union change; journalsourcefield round-trips byte-compatibly for legacy documents. - Largest-file rule satisfied via test_reporting_subscription_grant_journal.py (330 new lines) exercising journal +
_coverscoverage end to end.
Decision path: no critical/high findings (row 1 no); gated_paths false and review_decision APPROVED (row 2 no); high_risk true but reasons are (modified) with no medium findings and one (added) new file — no (deleted) (rows 3-5 no); prior decision was approve (row 6 no); no no-auto-approve team match (row 7 no); zero medium findings (row 8 no). Falls through to row 9 — approve.
457e803 to
30526b4
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or Medium findings.
Checked the decision table top-down:
- No no-auto-approve team match (HARD RULE n/a).
- gated_paths: false, and review_decision is APPROVED anyway (row 2 n/a).
- high_risk is true, but reasons are
(modified)and(added)only — no(deleted)(row 3 n/a). Reviewer surfaced no Medium-or-higher finding on the modified sensitive files (decisioning/handler.py,server/__init__.py), so rows 4/5 do not fire. - Prior decision was
approve, notescalate(row 6 n/a). - Zero findings total — rows 7/8 n/a.
- Falls through to row 9 → approve.
Substance verified by the reviewer: the delta is additive (feat: is the correct semver signal — new exports wind_down_grant, REPORTING_WIND_DOWN_TASKS, GrantObservation, SubscriptionGrants, authorization_chain_start, grants; authorization_changed gains keyword-only params with defaults, no break). The one-page observation write collapses a served page into a single CAS (engine.py:319, covered by test), the no-tasks-authorization fix maps missing allowed_tasks to a full grant (managed.py:659), zip(..., strict=True) is safe due to padding/truncation, and the source key omission keeps older documents byte-identical. Request-path projection is fail-open, idempotent, and introduces no unbounded I/O. New public exports and reporting-grants system land as additive scaffolding with accompanying tests.
2ecc739 to
d257b16
Compare
The reporting-subscription engine now journals every grant it learns of and is the only source of authorization.authorized_at (DX plan slice P6). - The chain rule lives in one place, authorization_chain_start: a re-grant before unauthorized_at + status_retention_days continues the chain, so authorized_at does not move; a later re-grant starts a new chain. Coverage uses the same rule. - On the managed path, served list_accounts and sync_accounts responses carry the journal's authorized_at, the value coverage uses. Each account they show is first observed by the journal: a new grant, a re-grant, or a reduction below get_reporting_status, which reads as a loss. A host value is adopted on first observation; one that later moves backwards is rejected, logged as authorized_at_regressed and counted. Absence from a response never revokes. - reporting.grants.changed(principal, account_id, at=, authorized=, allowed_tasks=) pushes a grant change into the journal and re-evaluates without listing, so a lagging listing cannot undo it. engine.authorization_changed gains allowed_tasks and source. - Grant checks read the journal, which these request-path observations and pushes keep current, so a reduction the caller was shown is enforced on its next reporting task (the staleness #1728 left). - wind_down_grant(current=None) and REPORTING_WIND_DOWN_TASKS (adcp.server) return the reduced grant that ends a relationship without cutting reconciliation, keeping the current authorized_at, as in adcp-client#3543. Journal entries record their source (listed, pushed); listed entries omit it, so existing state documents stay byte-identical. The shared grant-cases.json vectors from adcp-client#3543 run through the Python engine. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A subscribed caller's list_accounts or sync_accounts response used to cost one engine-state load and compare-and-swap per account. engine.observe_grants records every grant a page shows in one write, and observe_grant delegates to it. GrantObservation names one account's served grant. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
An authorization without allowed_tasks restricts no task. The reconcile already read it as a full grant; the request-path projection skipped it and served the host's raw authorized_at. It is now observed and projected like any other grant. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
30526b4 to
14be044
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — 1 medium finding, no blocking issues.
What I checked:
- Grant-journal +
wind_down_grantchanges are clean: credentials stripped before projection,sourcefield is journal-internal (no wire change), projection fail-opens deliberately, and state writes stay bounded by_mutate. No credential-in-ctx_metadatarisk, no public-surface semver break, no generated-code hand-edits. - High-risk flag is true (modified
decisioning/handler.py,server/__init__.py; addedserver/reporting_grants.py), but the sole finding is a non-idempotency nit on a modified non-sensitive file — not a destructive change, sensitive-category problem, or schema/infra/data-loss concern.
Medium findings:
- src/adcp/reporting/subscriptions/engine.py:1057 — authorized_at regression counter/log is non-idempotent under CAS retry: a single host regression can be counted/logged N times under contention since the
_observelogic runs inside the retryable_mutateclosure. Worth a human glance but does not corrupt durable state.
Decision path: No critical/high findings (row 1 no). gated_paths false (row 2 no). No (deleted) high-risk reason (row 3 no). The medium finding's category is observability/idempotency, not data-loss/schema/infra (row 4 no). Row 5 requires a (modified) high-risk entry AND a medium finding — the engine.py medium finding is NOT on a high-risk path (high-risk files are handler.py, server/init.py, reporting_grants.py), so row 5 does not fire. Prior decision was approve (row 6 no). No no-auto-approve team (row 7 no). Only 1 medium finding (row 8 no). Falls through to row 9 → approve.
Medium findings
- src/adcp/reporting/subscriptions/engine.py:1057 — authorized_at regression counter/log non-idempotent under CAS retry
Summary
Gate slice P6 of the reporting-subscriptions DX plan (§2.2, §4 items 3, 4 and 15, §6.2), stacked on #1739. Review only the commit
feat(reporting): make the grant journal the one source of authorized_at.The engine now journals every grant it learns of and is the only source of
authorization.authorized_at. Hosts report grants; they never compute chains.The journal
Chain rule, in one place.
authorization_chain_start(authorizations, status_retention_days=):unauthorized_at + status_retention_dayscontinues the chain, soauthorized_atdoes not move;Coverage evaluates the same chains, so the two cannot disagree.
Projection. On the managed path, served
list_accountsandsync_accountsresponses (onADCPHandlerand decisioning installs) carry the journal'sauthorized_at, the value coverage uses.Request-path observation. Each account a response shows with an authorization is observed first, in one journal write per page (
engine.observe_grants). An authorization withoutallowed_tasksreads as a full grant:get_reporting_status, which reads as a loss.Absence from a response never revokes, because responses are paged and filtered. Only callers holding subscriptions are touched, and a failure never fails the response.
Regressions. A host value is adopted on first observation. One that later moves behind the chain start is rejected, logged as
authorized_at_regressedand counted inengine.authorized_at_regressions.Push.
await reporting.grants.changed(principal, account_id, at=, authorized=, allowed_tasks=)records an exactly dated change, then re-evaluates without listing, so a lagging listing cannot undo it.engine.authorization_changedgainsallowed_tasksandsource.Grant checks read the journal. The last reconcile, these observations and pushes all keep it current. A reduction the caller was shown on
list_accountsis enforced on its next reporting task, which fixes the staleness feat(reporting): check reporting tasks against the current subscription grant #1728 left.Wind down, don't revoke (#8607)
wind_down_grant(current=None)andREPORTING_WIND_DOWN_TASKSinadcp.servermatch adcp-client#3543'swindDownGrant:get_reporting_status,sync_reporting_receiptsandsync_reporting_status, withread_only: false;authorized_at, because a reduced grant continues the chain;field_scopesandscope_nameare dropped.Revocation remains no access at all.
Storage
Journal entries record their
source(listed,pushed). Listed entries omit it, so existing state documents stay byte-identical.Deviations from the plan
request_accountgrants are not journaled yet. Python sellers have noaccounts.requesthook; it arrives with P15/P16. Thesourcefield is ready for it.grant: reduced (wind-down)step belongs to P11.authorized_at_regressedis a log line and an in-process counter. OTel metrics are P17.Tests
tests/test_reporting_subscription_grant_journal.py(23 tests):grant-cases.jsonfrom adcp-client#3543, vendored verbatim: 9 grant cases and 5 timeline cases through the real engine, checking authority,accounts_unauthorizedand every owed period;authorized_at;Refs #1695
🤖 Generated with Claude Code