Conversation
771e70f to
a0a5aaa
Compare
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — fail-closed buyer frozen-read reconciliation rewrite in _reconcile.py plus adversarial test coverage.
Checked:
- Definitive/missing-period claims correctly gated on a fingerprinted complete read.
- Receipt-chain and legacy-fanout paths fail closed and are work-bounded.
- Wire secrets redacted from all error surfaces.
- Three new adversarial conformance/unit test files exercise the new branches.
No Critical/High/Medium findings. No CI gate was disabled, no public-surface break, no type-layering or generated-code violations.
Nit (non-blocking): the full make test and complete PG core conformance lane are still marked pending in the PR body — ensure both are green before merge.
Decision path: no blocking findings; gated_paths false; high_risk false; author not on a no-auto-approve team; zero medium findings — falls through to row 9 (approve). Note review_decision is REVIEW_REQUIRED, but that only forces escalate when gated_paths is true, which it is not here.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — test-only refactor with no blocking or medium findings.
This PR reroutes publication-clock checks in tests/conformance/reporting/test_reporting_publication_time.py through the public load_reporting_ledger path instead of a hand-assembled ReportingLedger. Per-page reporting-revision schema validation and clock assertions are preserved; outcome.ledger.revisions is valid and correctly typed.
Checked against the decision table:
- No critical/high findings (row 1 n/a).
- gated_paths is false (row 2 n/a).
- high_risk is false (rows 3/5 n/a).
- No medium findings, so no schema/data-loss/infra escalation (row 4 n/a).
- Prior decision was
approve, notescalate(row 6 n/a). - No no-auto-approve team match (row 7 n/a).
- Zero medium findings, well under the three-medium threshold (row 8 n/a).
Falls through to row 9 → approve. The sole reviewer note is a Low observation that the request's max_results:1 pagination hint is stripped by the loader and thus inert — non-blocking. Clean pass.
bokelley
left a comment
bokelley
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES
(Filed as a review comment: GitHub refuses a formal REQUEST_CHANGES because the reviewer session and the PR author share the bokelley account. Treat this as blocking — the P1 below needs a fix on this branch.)
Reviewed at 737c3698 (rebased on 5fd54334). Ran locally: make lint OK, make typecheck-all OK, and pytest tests/test_reporting_frozen_read_safety.py tests/test_reporting_frozen_read_corrections.py tests/test_reporting_reconciliation.py tests/conformance/reporting/ -> 2206 passed, 2044 skipped, 0 failed (the skips are the PG lane; CI is green on both PG conformance jobs). The ownerless-status bound (unresolved_by_scope + AMBIGUOUS_CONSUMER_STATUS_OWNERSHIP), the absent-vs-contradicting optional-count split in _obligation_history, the read fingerprint / UNVERIFIED_LEDGER_SNAPSHOT gate, and the _receipt_leaves chain validation all look right and are well covered. Two findings.
P1 — delivery_only obligations with an unreconciled adjustment are reported definitive=True
evaluate_reporting_ledger only consults adjustment receipts inside the consumer_receipt branch (_reconcile.py:1618-1642), and this PR removes the unconditional ADJUSTMENT_RECONCILIATION_REQUIRED reason that main appended for any obligation with adjustment_count or an adjustment against the selected revision.
Net effect: a delivery_only obligation carrying a post-official adjustment that nothing has receipted now yields no reason at all, so both ObligationReconciliation.definitive (not reasons) and the top-level definitive are True — while the seller is simultaneously declaring pending_adjustment_count=1.
_validate_read_ledger does read pending_adjustment_count, but only to bound it against the leaf receipts; outside consumer_receipt the value never becomes a reason.
Repro on this branch:
raw = _history(adjustments=True) # one post-official adjustment
raw["receipts"] = []
raw["adjustment_receipts"] = [] # nothing has receipted the correction
_counts(raw)
raw["periods"][0].update(
reconciliation_mode="delivery_only",
reconciliation_status="not_required",
receipt_count=None, accepted_receipt_count=None,
adjustment_receipt_count=None, accepted_adjustment_receipt_count=None,
pending_adjustment_count=1, # seller: 1 unreconciled correction
)
result = evaluate_reporting_ledger(await _load(raw), expected_periods=_expected(), now=NOW)
# definitive: True reasons: ()On main the same ledger gives ADJUSTMENT_RECONCILIATION_REQUIRED / definitive=False. Telling a buyer the revision is definitive while the seller has published an unreconciled correction to it is the failure mode this PR is otherwise closing.
Either fix works:
- append a reason for all modes (reuse
ADJUSTMENT_RECONCILIATION_REQUIRED, or gate on unaccepted leaves /pending_adjustment_count), or - reject adjustments against
delivery_onlyobligations in_index_read_ledgerasINVALID_LEDGER_DEPENDENCY.
Please add coverage either way — _partly_owned_legacy_history is the only delivery_only fixture and it pins adjustment_count=0, so this path is currently untested.
P2 — REVISION_SCOPE_MISMATCH is retired and its triggers become whole-ledger exceptions; not mentioned in the description
REVISION_SCOPE_MISMATCH no longer exists anywhere in src/ or tests/. Both of its main triggers are now hard failures in _index_read_ledger: a materialization whose revision does not match its obligation, and two obligations materializing the same revision.
# two obligations materializing the same revision, on this branch:
# ReportingReconciliationError INVALID_REVISION_OWNERSHIP
# (INVALID_LEDGER_DEPENDENCY without the ownership extension)On main that returned a usable result with a per-obligation REVISION_SCOPE_MISMATCH reason. Escalating to an exception is defensible for a frozen read, and nothing in-repo or in docs/ consumes the string — but under a fix(reporting): title it is an adopter-visible change from "degraded result" to "no result", so please call it out in the PR description (rule 5) and confirm the escalation is intended.
Non-blocking, no action needed: evaluate_reporting_ledger and reconcile_reporting each re-run the full _validate_read_ledger walk that load_reporting_ledger already did; the association_limit bounds it, so it is cost rather than correctness.
|
P1 is fixed on this branch: the selected revision's adjustment current-leaf check now runs in both reconciliation modes. Missing, rejected, or partially accepted adjustment evidence prevents a definitive result; optional count applicability is unchanged. Added 20 public loader/evaluator cases and four real MCP/A2A cases over memory/PostgreSQL using a legal analytics delivery-only feed. Before the fix: 16 failed, 8 passed. After the fix: all 24 pass within a 408-test focused run. P2 is intentional and now explicit in the PR's migration guidance: revision/materialization scope conflicts abort the frozen read with
|
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean subsequent pass with no blocking, medium, or unresolved prior findings.
What I checked:
- The delta in
src/adcp/reporting/_reconcile.py:1136lifts the applicable-corrections evidence requirement out of theconsumer_receipt-only branch into a broaderif revision:guard, soMISSING_MATCHING_ADJUSTMENT_RECEIPTnow correctly fires in delivery_only mode too. This is a correctness tightening. - Two well-constructed new tests back the change; test files across the conformance and unit suites were expanded.
- No public-surface removals/renames/type-signature changes, no discriminated-union fallback removals, no type-system import-layering violations, no generated-code hand-edits, no CI gate disablement, no credentials in ctx_metadata.
Gate checks:
- No no-auto-approve team match (row 7 N/A).
gated_paths: false— row 2 N/A even thoughreview_decisionis REVIEW_REQUIRED.high_risk: false— rows 3/5 N/A.- Prior decision was
approve, so sticky escalation (row 6) N/A. - Zero findings — rows 1, 4, 8 N/A.
Falls through to row 9 → approve.
bokelley
left a comment
bokelley
left a comment
There was a problem hiding this comment.
APPROVE
(Filed as a review comment for the same reason as last time: GitHub refuses a formal APPROVE when the reviewer session and the PR author share the bokelley account.)
Re-reviewed at 735168bd. Both findings are resolved.
P1 — fixed
evaluate_reporting_ledger now runs the adjustment-leaf check under its own if revision: block (_reconcile.py:1628-1640) instead of inside the consumer_receipt branch. Verified independently on this branch, not just via the new tests:
| case | result |
|---|---|
delivery_only + unreceipted adjustment, pending_adjustment_count=1 (my original repro) |
definitive=False, ('MISSING_MATCHING_ADJUSTMENT_RECEIPT',) |
same, seller omits pending_adjustment_count |
definitive=False, ('MISSING_MATCHING_ADJUSTMENT_RECEIPT',) |
delivery_only + accepted adjustment leaf |
definitive=True, () — no false block |
delivery_only, no adjustments |
definitive=True, () — no false block |
consumer_receipt + accepted adjustment receipt |
definitive=True, () — unchanged |
consumer_receipt + unreceipted adjustment |
definitive=False, single reason — no double-append |
Since _validate_read_ledger already requires an adjustment's target to be official, and select_reporting_revision rejects multiple officials, an applicable adjustment always targets the selected revision — so this restores main's ADJUSTMENT_RECONCILIATION_REQUIRED conservatism while correctly clearing once the leaf is accepted.
Coverage is better than I asked for. test_delivery_only_adjustments_require_each_accepted_current_leaf is 20 cases (missing / rejected / accepted / accepted-leaf / partial × declared-vs-omitted pending count × implicit-vs-explicit ownership) and validates the fixture against the real core/reporting-obligation.json. test_delivery_only_adjustment_blocks_definitive_without_receipt_counts covers it end to end over MCP and A2A against a real seller projection, asserting health == "complete" and all three receipt counts None — precisely the shape that previously read as definitive.
P2 — resolved
The description now names REVISION_SCOPE_MISMATCH, both replacement codes (INVALID_REVISION_OWNERSHIP / INVALID_LEDGER_DEPENDENCY), states the escalation is intentional, and tells callers to handle the typed error and re-read. That was the whole ask.
What I ran
make lint OK, make typecheck-all OK. pytest tests/test_reporting_frozen_read_safety.py tests/test_reporting_frozen_read_corrections.py tests/test_reporting_reconciliation.py tests/conformance/reporting/ -> 2225 passed, 2042 skipped, 7 errors. All 7 errors are built_distribution-fixture packaging tests (test_reporting_notification_packaging.py, test_reporting_materializer_packaging.py) that shell out to python -m build; the repo's own harness classifies them {"trace":[[152,["disk_full"]]],"classification":"recognized"} and this sandbox is at 99% disk. Neither file is touched by the PR, they passed on 737c3698 earlier today, and the Installed production reporting / Installed frozen feed CI jobs cover them properly. I also ran the other 17 tests/test_reporting_*.py files -> 456 passed. The two new tests pass locally on the memory backend (mcp-memory, a2a-memory); the postgres parameterizations skip here and are covered by the PG conformance jobs in CI.
No further findings. Good to merge once CI is green on 735168bd.
|
I checked the current merge conflict read-only: it is confined to |
735168b to
2622275
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — buyer frozen-read/reconciliation rewrite with comprehensive test coverage.
This PR rewrites the buyer frozen-read/reconciliation path in src/adcp/reporting/_reconcile.py (+827/−140) and adds ~1,700 lines of memory + PostgreSQL, MCP + A2A conformance/unit tests. Changes stay within the reporting module — no touches to type-system layering, wire/creative models, ctx_metadata/credentials, generated code, or CI-gate config.
Verification highlights:
- Fail-closed load/mutation-detection logic and receipt-chain integrity verified.
- Redaction behavior confirmed.
- The
fix:prefix is defensible:reasonsistuple[str,...](not an enum),max_bytesis additive, and the documented raise-vs-return behavior change is confined to an unreleased rc6 slice — no shipped semver break.
No Critical/High/Medium findings. Not high-risk, no gated paths, no author-team gate. Prior decision was also approve; this run remains clean. Falls through to row 9.
Non-blocking nit: confirm full CI is green post-rebase.
bokelley
left a comment
bokelley
left a comment
There was a problem hiding this comment.
APPROVE (unchanged — rebase verified)
(Review comment again: GitHub refuses a formal APPROVE when reviewer and author share the bokelley account.)
Re-checked at 2622275f on c67b97e0. origin/main is an ancestor; six commits, with the duplicate PG-timeout commit correctly dropped.
The change I approved is byte-identical. git diff 735168bd 2622275f -- src/adcp/reporting/_reconcile.py is empty, and none of the four newly merged commits (#1216 / #1218 / #1219 / #1220) touched that file. The PR's own test files are unchanged across the rebase too — the only tree delta is main's new work.
The clean textual merge is also a clean semantic one. _reconcile.py imports nothing from adjustment_evidence, submissions, or service_lifecycle, and no generated_poc/core/reporting_* model changed in the merged set — so #1220's raw-adjustment-evidence work and this PR's _validate_read_ledger digest checks meet only at the unchanged model contract.
Re-ran my own P1 probes on the rebased head — identical to 735168bd:
| case | result |
|---|---|
delivery_only + unreceipted adjustment, pending_adjustment_count=1 |
False, ('MISSING_MATCHING_ADJUSTMENT_RECEIPT',) |
| same, pending count omitted | False, same reason |
delivery_only + accepted adjustment leaf |
True, () |
delivery_only, no adjustments |
True, () |
consumer_receipt + accepted adjustment receipt |
True, () |
consumer_receipt + unreceipted adjustment |
False, single reason |
Local run: make lint OK, make typecheck-all OK, pytest tests/test_reporting_frozen_read_safety.py tests/test_reporting_frozen_read_corrections.py tests/test_reporting_reconciliation.py tests/test_reporting_adjustment_evidence.py tests/conformance/reporting/ -> 2373 passed, 2149 skipped, 0 failed, 0 errors. The 7 disk_full packaging errors I reported on the previous head are gone — sandbox disk went 99% -> 79%, so that selection now includes the built_distribution packaging tests passing.
On the conflict resolution: agree with keeping main's config. Main gives the core lane 30 job-minutes and process 15, against prior observed runtimes of 15m58s and 8m57s — comfortable either way. One small delta for awareness, not worth a commit on its own: the dropped commit also added a step-level timeout-minutes: 25 to the "Run Postgres conformance tests" step, which main does not have. That only affects diagnostics (pytest reporting its own timeout before GitHub kills the job), not the budget.
No new findings. Approval stands; good to merge once CI is green on 2622275f.
|
The last required check failed on infrastructure timing, not an assertion: |
bokelley
left a comment
bokelley
left a comment
There was a problem hiding this comment.
APPROVE (workflow delta only)
(Review comment again: GitHub refuses a formal APPROVE when reviewer and author share the bokelley account.)
Reviewed 01944f0e. git diff 2622275f 01944f0e touches .github/workflows/ci.yml and nothing else, and src/adcp/reporting/_reconcile.py is still byte-identical to 735168bd — the buyer code is exactly what I approved.
One functional change (45 -> 55 on the 3.12 coverage step); the rest of the -16/+6 is comment rewriting. Parsed the result to be sure:
test job timeout-minutes: 60
step: Run tests -> 45 | if: matrix.python-version != '3.12'
step: Run tests with coverage -> 55 | if: matrix.python-version == '3.12'
The budget checks out against the run that failed. From the 3.12 job in run 36258899936:
| segment | measured |
|---|---|
| set-up job -> type-ignore contract (checkout, Python, install, ruff, mypy, mypy --strict) | 17:45:52 -> 17:47:21 = 1m29s |
Run tests with coverage |
17:47:21 -> 18:32:33 = 45m12s (hit the old bound) |
| post-steps + complete job | 18:32:33 -> 18:32:34 = 1s |
So 55m step + ~1m30s overhead = 56m30s against the 60-minute ceiling, leaving 3m30s of margin. The invariant the old comment was protecting — step bound fires before the job ceiling, so a stuck suite fails one named step instead of being silently cancelled — still holds. The suite was at 99% when killed at 45m12s, so ~46m is the realistic need and 55m is roughly 20% headroom. tests/ collects 14,175 here, which matches the "over 14k" in the commit message.
Dropping the old comment's specific figures is fine — they had gone stale (it claimed ~16m plain and ~30m coverage; actuals are ~35m and 45m+), and the replacement keeps the part that matters, which is why the step bound must sit below the job ceiling.
Approval stands. No findings on this delta.
Non-blocking: 3.10 is the next lane to trip
Same run, Test Python 3.10: the plain Run tests step took 17:34:47 -> 18:09:53 = 35m06s against the unchanged 45-minute bound — 78% consumed, ~22% headroom. That is the same growth curve that just pushed 3.12 past 45m. Not something to change in this PR, but worth raising the plain-suite bound proactively rather than after a red matrix, or splitting the suite.
Also for awareness: main moved to 7c5a1239 (#1223) after your rebase. It only touches src/adcp/server/serve.py and tests/test_serve_lifespan_hooks.py, so there is no ci.yml conflict and GitHub still reports MERGEABLE — no action needed.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Subsequent pass over PR #1222. The prior run was clean (no findings) and the delta since is a single non-generated file, .github/workflows/ci.yml, well under the largest-file threshold.
Medium findings
.github/workflows/ci.yml— Raising the coverage step to 55m without lifting the 60m job ceiling leaves ~5m for the 3.12 leg's checkout/install/ruff/mypy/type-ignore pre-work plus cleanup; a hung coverage run could trip the job ceiling before the named step timeout. Worth a look but non-blocking.
No critical/high findings. Only one medium finding (well under the 3-medium comment threshold), not in a data-loss/schema/infra category. high_risk is false and gated_paths is false, so the REVIEW_REQUIRED status does not gate. No no-auto-approve team match. Prior decision was approve. Falls through the decision table to row 9 → approve.
|
The Medium timeout-margin finding is blocking. #1221 applied the same narrow correction: 55-minute Python 3.12 coverage step with a 70-minute Python 3.12 job cap, leaving other matrix cells unchanged. Please push the equivalent fix now so the current CI run need not finish before the next review. |
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — CI timeout tuning only, no blocking findings.
This delta pass covers a single file, .github/workflows/ci.yml. The change raises the 3.12 job cap to 70 min (60 elsewhere) and the coverage step bound 45→55, keeping ~15 min setup/cleanup headroom on both lanes and keeping each step bound below its job cap so a hang fails the named step rather than the whole job. Conditional-expression syntax is valid; no code, wire-shape, public export, or CI gate is affected — the gates stay armed. The prior run was a clean approve, and no blockers were introduced.
Decision-table walk: no critical/high/medium findings (row 1 n/a); gated_paths false (row 2 n/a); high_risk false with no deletions (rows 3/5 n/a); no medium data-loss/schema/infra (row 4 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). Falls through to row 9 → approve.
Note: review_decision is REVIEW_REQUIRED, but that only forces escalate when gated_paths is true (row 2), which it is not here.
bokelley
left a comment
bokelley
left a comment
There was a problem hiding this comment.
APPROVE (renewed on 4c05106a)
(Review comment again: GitHub refuses a formal APPROVE when reviewer and author share the bokelley account.)
git diff 01944f0e 4c05106a touches .github/workflows/ci.yml and nothing else, and src/adcp/reporting/_reconcile.py is still byte-identical to 735168bd. Buyer code unchanged; this is a three-line workflow delta.
timeout-minutes: ${{ matrix.python-version == '3.12' && 70 || 60 }}This is the right fix, and I was wrong to wave the previous margin through. My last pass measured the actual 3.12 overhead at 1m29s and called 3m30s of slack "comfortable" — true as an arithmetic fact, but aao-secretariat's structural point is the better invariant: every lane should keep the same setup allowance, so the step bound stays the thing that fires. 4c05106a restores it exactly:
| lane | job cap | step bound | non-test allowance |
|---|---|---|---|
| 3.12 (coverage) | 70 | 55 | 15m |
| 3.10 / 3.11 / 3.13 | 60 | 45 | 15m |
Against measured overhead of 1m29s (3.12) and 54s (3.10), 15m is ~10-16x headroom on every lane. Verified by parsing the file:
test job timeout-minutes: "${{ matrix.python-version == '3.12' && 70 || 60 }}"
step: Run tests -> 45 | if: matrix.python-version != '3.12'
step: Run tests with coverage -> 55 | if: matrix.python-version == '3.12'
Checks that matter here:
- Expression is legal in this position.
pg-conformancein the same file already usestimeout-minutes: ${{ matrix.timeout_minutes }}and resolves correctly in practice (core 30 / process 15, observed at 15m58s and 8m57s), so job-leveltimeout-minutesdemonstrably accepts matrix context here. - Ternary is sound. GHA's
a && b || cfalls through tocwhenbis falsy;bis70, non-zero, so 3.12 -> 70 and everything else -> 60. - Required check names are untouched.
name: Test Python ${{ matrix.python-version }}is unchanged, so branch protection keeps matching. workflow-securityrunsactionlint+zizmoron every push, so this is machine-validated in CI too — currently pending on4c05106a.
Approval stands. No findings.
Non-blocking
The neighbouring pg-conformance job expresses the same idea as a matrix.include with an explicit timeout_minutes per lane. That form is immune to the && || falsy-operand footgun if someone later edits the number, and would read consistently with the rest of the file. Not worth a commit now — just something to prefer if this grows a third case.
My earlier note still stands on its own: 3.10's plain Run tests step ran 35m06s against its unchanged 45-minute bound, so that lane is the next to trip.
This branch has not been deployed
1 participant
Reconciliation could certify incomplete or ambiguous histories and compare an accepted receipt with a newer delivery attempt. The buyer now requires an unchanged, complete authorized frozen periods read before definitive outcomes or missing-obligation claims.
Missing-period proof is intentionally conservative:
ExpectedReportingPeriodhas no trusted finality requirement, so a response denominator containing only one finality cannot prove an absent period belongs to it. Returned obligations still reconcile normally, and an unproven expected period prevents overall success. This can suppress valid absence claims from complete official-only or snapshot-only configurations.Migration: load before evaluating; manually assembled or modified ledgers cannot certify completeness. Inconsistent retained evidence now fails earlier and requires a fresh read after correction. Public positional fields and protocols remain compatible;
max_bytesis additive. No database migration is needed.Scope conflicts that previously returned the per-obligation
REVISION_SCOPE_MISMATCHreason now abort the read withReportingReconciliationError(INVALID_REVISION_OWNERSHIPorINVALID_LEDGER_DEPENDENCY). This escalation is intentional: conflicting revision/materialization ownership invalidates the frozen history's partition proof. Callers should handle the typed error and read again after the seller corrects the history.Validation:
make lint typecheck-all validate-generated: passed.make testafter rebase: 11,999 passed, 2,191 skipped, 9 deselected, 1 xfailed; 81.83% coverage.actionlintand repository hooks passed for the coverage deadline change. The growing full suite reached 99% before the former 45-minute limit; the coverage step now allows 55 minutes within a 70-minute Python 3.12 job cap, preserving 15 minutes for setup and cleanup.Raw client evidence capture and composition of submission intents in the later facade remain follow-up work. Strict rc6 wire validation still rejects legacy received statuses without an owner; diagnostic tolerance of already-typed legacy records does not establish mounted wire compatibility.
Refs #1167
Refs #1172
Open workspace in Conductor