test(reporting): add auditable interoperability orchestration

[bokelley]

Summary

• Add the reliable-reporting interoperability harness, public controller fixtures, and fail-closed orchestration for the five installed storyboards (89 steps / 84 stateful).
• Preserve exact artifact/protocol/runtime selection and explicit positive-versus-negative execution accounting.
• Add controlled-clock Core/status, managed/reconciled seller adapters, owned PostgreSQL/seller/storyboard lifecycles, and observable cleanup/error propagation.
• Keep historical/shared-seller and negative-skew controls supplemental: they cannot satisfy current rc.6 or positive-skew cells.

Current review fixes

• Bound every external install/version/cosign/GitHub command and convert command timeouts to InputError, retaining partial logged output where applicable.
• Bound all foundation PostgreSQL connects, statements, and locks; cleanup attempts every exact owned database and aggregates failures.
• Run storyboard orchestration in its own session and signal the process group on timeout even when the direct parent already exited; escalate SIGINT to SIGKILL under a second bound while retaining partial output and cleanup errors.
• Align produced cell records with the evaluator's database_identity contract. Cluster identity uses PostgreSQL's canonical pg_control_system().system_identifier plus pg_postmaster_start_time(); required_cell_credit remains an independent mandatory gate.
• Support external LOGIN CREATEDB non-superuser roles without pg_read_all_settings. A real-PostgreSQL regression creates that restricted role, exercises database creation/identity end-to-end, and cleans up the database and role.
• Keep subprocess credentials out of argv, use narrow child environments, own managed server bind/listen failures, and preserve exactly-once ordered resource cleanup.

Validation

• 89 passed:
  • tests/test_reporting_interop_corpus.py
  • tests/test_reporting_storyboard_orchestration.py
  • tests/test_reporting_interop_matrix.py
• The restricted-role PostgreSQL regression is collected from tests/conformance/reporting/test_reporting_interop_foundation_database.py; it skips locally when ADCP_PG_TEST_URL/the PostgreSQL extra is unavailable and executes in the PostgreSQL CI lane.
• make lint passed.
• make typecheck-all passed, including strict adopter fixtures and the no-type: ignore contract.
• make test passed on exact head e1cac81d7: 11,932 passed, 2,256 skipped, 9 deselected, 1 xfailed in 35m32s; coverage 81.56% (required 80%).
• Commit hooks passed: Black, Ruff, mypy, adopter type checks, Bandit, whitespace/EOF checks, merge/case-conflict checks, private-key detection, and conventional subject validation.
• Independent exact-head review reproduced the parent-exited/descendant-held-pipe timeout and confirmed the process group is gone after cleanup.
• Current-head CI and exact-head formal review are required before merge.

Deliberate non-acceptance

• This PR builds and validates the harness; it does not claim six-cell matrix acceptance.
• No fresh six-cell acceptance execution or complete 89-step storyboard acceptance run is reported by this PR.
• Inventory, focused unit/fake tests, supplemental historical controls, negative refusal, and source validation do not count as required-cell execution.
• required_cell_credit remains false for the current supplemental controls; shared seller/database controls cannot earn required-cell credit.
• Positive S1/S2 supported-skew evidence, final Python artifact selection, full installed package/lock qualification, final release-guard wiring, and release acceptance remain separate gates.
• The rc.47 TypeScript identity remains a development input until the executable package/lock/member selection requirements are satisfied; historical rc.45 results retain their original identity.

---

Open workspace in Conductor

[gitguardian]

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

---

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

[bokelley]

GitGuardian failure — diagnosed, and the obvious fix would break the test.

The failing check traces to the one file on this branch carrying credential-shaped strings:
scripts/ci/reporting_interop/security/resource_location_sentinels.json.

Confirmed new here, not inherited:

• AKIAIOSFODNN7EXAMPLE appears on this branch in that file only, and nowhere on main.
• Same file also carries X-Amz-Signature= / X-Amz-Credential=, an Azure SAS sig=, access_token=, password=, api_key=, and a bare JWT (eyJhbGciOiJIUzI1NiIs…).
• There is no .gitguardian.yaml on the branch, so nothing suppresses them.

These are all intentional negative-test sentinels — the redaction logic is supposed to reject every one of them ("expect":"reject"). They're synthetic: .test TLDs throughout, RLXSENTINEL000N payloads, and AKIAIOSFODNN7EXAMPLE is AWS's own documented example key. No live credential is involved, so this is a scanner true-positive but a security false-positive.

Worth flagging because deleting or mangling the literals to make the scanner green would silently gut the test — the sentinels only prove anything if they look real enough to match a detector.

Suggested fix: add .gitguardian.yaml scoping the ignore to this path, e.g.

secret:
  ignored-paths:
    - scripts/ci/reporting_interop/security/resource_location_sentinels.json

Prefer a path-scoped ignore over ignored-matches on the raw values, so a genuinely new secret added elsewhere in the harness still trips the scanner.

[bokelley]

The successor corrections are pushed on the PR branch at 38efbf8. The TS reviewer assigned to #1217 can review this head directly on the PR; GitGuardian and both PostgreSQL lanes are green, with the remaining CI checks still running.

[bokelley]

Current-head CI has one red job: Installed production reporting (pg-vcs, Python 3.10) (run 36258575498). The failure is the installed child hitting its 1,800-second aggregate deadline (deadline_kind=aggregate, phase call, clean termination), not a reported test assertion; see job 108449917569. Please review the bound against the retained installed test selection before marking ready.

[bokelley]

Review verdict: CHANGES REQUESTED (posting as a comment — GitHub blocks a formal review state because this PR is authored by the same account I'm authenticated as).

Reviewed at head 9db35341, in a clean worktree. Ran the suites and two mutation controls rather than reasoning from the source alone. 67 tests pass locally (28 corpus + 39 storyboard).

One blocking finding, one nit. Everything else I set out to check holds.

---

P2 — test_ts_version_and_abort_helpers_fail_closed_without_sdk_execution can pass while running none of its assertions

tests/test_reporting_interop_corpus.py:886 has exactly the false-pass shape this PR just fixed for the managed test: an async IIFE ending in .catch(error => { console.error(error); process.exitCode = 1; }), no completion marker, and subprocess.run([...], check=True) with no timeout.

A pending promise never reaches that .catch. Node drains the event loop, exits 0, and check=True accepts it.

Demonstrated, not inferred — I injected a non-settling await at the top of the IIFE (line 716), which skips every assertion in the script:

(async () => {
  await new Promise(() => {});   // injected

1 passed, 27 deselected in 23.13s

Every assertion skipped, test still green.

The fix already exists a few hundred lines down at :1135-1148 — apply the same three pieces:

process.stdout.write('ts-version-abort-helpers-complete\n');   # after all assertions, outside the catch
...
completed = subprocess.run([...], check=True, capture_output=True, text=True, timeout=30, cwd=ROOT)
assert completed.stdout.splitlines() == ["ts-version-abort-helpers-complete"]

Worth fixing on the branch because it's the same defect class already adjudicated here, and this is the only remaining instance — test_reporting_storyboard_orchestration.py has 25 timeouts and none of this pattern, and the corpus file has exactly one timeout=.

Nit — narrow-env discipline applied to 7 of 24 child launches

run_foundation_matrix.py is wholly new in this PR (2581 lines, 0 deletions), so this is new code rather than inherited: it introduces NARROW_RUNTIME_ENV_KEYS / _narrow_child_environment / _buyer_environment / _ts_server_environment and uses them for the six buyer handoffs and the Core seller seam, while 17 other child launches forward the full parent environment (381, 887, 950, 964, 1031, 1043, 1096, 1175, 1194, 1205, 1229, 1252, 1265, 1323, 1345, 1823, 2321).

To be fair to the design: this does not leak harness credentials. The per-run tokens and startup proofs are local objects bound explicitly into child environments, never written to the launcher's os.environ. And most of these sites run node/npm/python tooling that legitimately wants ambient config (HOME, npm settings, NODE_PATH). The residual is only ambient CI secrets reaching SDK/third-party child code — defense-in-depth, not a live exposure.

Not blocking. A one-line comment on _narrow_child_environment saying the narrow path is for credential-bearing handoffs and full env is deliberate elsewhere would stop the next reader from reading the split as an oversight.

---

Verified good

• The 009 guard actually works. I mutated out the fix (server.on('error', serverErrorListener) at ts_managed_reporting_server.cjs:221) and the regression test failed, as it should. This guard had only ever been assessed statically; it now has an executed negative control.
• serve() attach is still race-free. :337 calls api.server.serve(...) with no await, and ownServer attaches at :359 in the same synchronous block. Since published serve is a sync function (serve.js:70, doListen() at :545, return httpServer at :547), there's no suspension point between listen() and the attach. Worth preserving deliberately: changing :337 to const server = await api.server.serve(...) would reintroduce the race, because await defers to a microtask while a synchronous bind failure (EADDRINUSE) is queued on process.nextTick, which drains first.
• Bearer no longer on the argv. No --auth in any Python argv builder; _storyboard_environment (:1064) hands off via ADCP_AUTH_TOKEN, which is the published CLI's supported fallback.
• Private-input guards intact. ≥32 length enforced on all five inputs, Core A/B distinctness enforced.
• No shortcuts. No pytest.skip/xfail/TODO/FIXME anywhere in either suite; storyboard denominator pinned at EXPECTED_TOTALS = {"storyboards": 5, "steps": 89, "stateful_steps": 84} — the corrected 89/84, not the old 64.
• GitGuardian is green now that the sentinel file is scoped.

[bokelley]

Correction to my own P2 fix suggestion — don't copy it verbatim, the escaping differs.

I said "apply the same three pieces" from :1135-1148. I tested that before you spend time on it, and a verbatim copy breaks. The two tests use different string literal types:

• :900 (managed test, where the working fix lives) — script = r""" , a raw string. \n reaches JavaScript as the two characters \ + n.
• :670 (the test needing the fix) — script = f""" , a non-raw f-string. \n becomes a real newline character, landing inside a single-quoted JS string literal.

Copying it as-is gives:

SyntaxError: Invalid or unexpected token

In the f-string it has to be a doubled backslash:

process.stdout.write('ts-version-abort-helpers-complete\\n');

(Also note the f-string needs {{/}} for any JS braces you add — the marker line has none, so it's unaffected.)

I ran all three variants against the real suite:

variant	result
verbatim \n copied from :900	SyntaxError: Invalid or unexpected token
\\n + marker + timeout=30 + stdout assert	passes
that same fix, with a non-settling await injected at the IIFE	fails: AssertionError: assert [] == ['ts-version-abort-helpers-complete']

So the corrected form both passes normally and actually detects the false-pass it exists to catch. The full working change is the marker line above placed after boundary.dispose(); and outside the .catch, plus:

completed = subprocess.run(
    ["node", "-e", script],
    check=True,
    cwd=ROOT,
    capture_output=True,
    text=True,
    timeout=30,
)
assert completed.stdout.splitlines() == ["ts-version-abort-helpers-complete"]

Nothing changed in my verdict or the finding itself — only the escape form in the remedy I gave you.

[bokelley]

@bokelley P2 fixed at exact head 3775bfe1d316d932609cb3db5a68b2b719d56734: the helper probe now emits a terminal marker after all assertions, the parent requires the exact captured line, and the subprocess has a 30s timeout. Both suites pass (67/67). I also reran your non-settling-await mutation: it now fails on the missing marker instead of exiting green. Please re-review this exact head.

[bokelley]

Re-review at 3775bfe1 — P2 resolved. My blocking finding is cleared.

The fix is the corrected form, not the verbatim copy: 'ts-version-abort-helpers-complete\\n' with the doubled backslash the f-string needs, marker placed after boundary.dispose(); and outside the .catch, plus capture_output / text / timeout=30 and the exact stdout assertion.

Verified by running it, not by reading it:

check	result
both interop suites at this head	67 passed (28 corpus + 39 storyboard)
the fixed test, with a non-settling await injected at the IIFE	fails — AssertionError: assert [] == ['ts-version-abort-helpers-complete']

That second row is the one that matters: the test now detects the false pass it previously accepted. Before this commit the same injection produced 1 passed.

The fix is also complete rather than spot-applied — both subprocess.run(["node", ...]) call sites in the file now carry a completion marker and a timeout (2 markers, 2 timeouts), and there are no remaining untimed node -e invocations anywhere under tests/ or scripts/ci/reporting_interop/.

Still carried forward, unchanged and non-blocking:

• The narrow-env nit from my earlier review — 17 full-parent-env child launches in run_foundation_matrix.py alongside the narrow helper used for the 7 credential handoffs. Not a credential leak; a comment explaining the deliberate split would help the next reader. Fine to land as-is or defer.

Everything I verified in the first pass still holds at this head: the 009 guard genuinely fails when its fix is mutated out, serve() attach remains race-free at :337/:359 (keep it un-awaited), bearer stays off the argv, ≥32 input guards and Core A/B distinctness intact, 89/84 pinned, no skips or xfail.

From my side this is an approve at this exact head — I can't set the formal state (GitHub blocks review states on a PR authored by the account I'm authenticated as), so recording it here. CI restarted on this push and is still in flight (40 pending, 0 failures so far); the merge gate remains that going green plus aao-secretariat's exact-head approval.

[bokelley]

@bokelley Rebased onto current main 2a45bafd and force-pushed exact head c5b5826d6caea2eb267ead5c288c4068438ed0cf. The only conflict was the installed-production timeout change; I retained main’s stronger 2400s PG / 1800s base budget with 45/50-minute workflow headroom. Post-rebase validation: 76 passed, 1 skipped across the two interop suites plus production-harness tests; lint and typecheck-all pass. Please perform the technical re-review on this exact head.

[bokelley]

Re-review at c5b5826d (rebase onto 2a45bafd) — verdict unchanged, still cleared.

I diffed the PR's content against its merge base at both heads rather than diffing the heads directly, so main's 73-file contribution doesn't mask a PR-side change. Result: my reviewed content is byte-identical across the rebase — zero diff in scripts/ci/reporting_interop/, test_reporting_interop_corpus.py and test_reporting_storyboard_orchestration.py.

One thing worth stating explicitly, since it looks alarming in a raw diff and is the only PR-side delta:

The rebase dropped 9db35341 test(reporting): allow installed production headroom — the timeout-minutes: 40→45 / 35→40 and timeout=1800→2100 changes are no longer in the PR diff. That is correct, not a lost fix. Main independently raised the same limits further, and the rebase kept main's values:

	PR at 3775bfe1	main / PR at c5b5826d
ci.yml job	40 → 45	50
ci.yml step	35 → 40	45
_production_packaging.py	1800 → 2100	2400 (if not driver_absent, else 1800)

Both files are now byte-identical to main, so the headroom went up, not down. If the rebase had resolved the other way it would have silently cut main's CI budget — it didn't.

Re-ran everything at this head:

check	result
both interop suites	67 passed
P2 control — non-settling await injected	fails (assert [] == ['ts-version-abort-helpers-complete'])
009 control — server.on('error', …) mutated out	fails

Both guards still fire after the rebase, so main's changes didn't erode either one.

Nothing else changed from my 3775bfe1 review: the narrow-env nit remains the only open item and is still non-blocking.

Approve at this exact head from my side, recorded here since I can't set the formal state. CI is re-running on the rebase (37 pending, 9 pass, 0 failures); the gate is that completing green plus aao-secretariat's approval on c5b5826d.

[bokelley]

@aao-secretariat #1217 is ready for formal review at exact head c5b5826d6caea2eb267ead5c288c4068438ed0cf. Current-head CI is fully green; local make test, make lint, make typecheck-all, and focused post-rebase validation pass. @bokelley cleared the exact-head technical review. Please post the required exact-head APPROVE for merge.

[bokelley]

@aao-secretariat Formal review handoff for exact head c5b5826d6caea2eb267ead5c288c4068438ed0cf: all CI/test/static checks are green, local make test, make lint, and make typecheck-all pass, and @bokelley cleared the exact-head technical review. Ladon run 36270719842 failed twice only because its successful analysis remained needs_finalization after the finalization retry; both runs explicitly report this is not a code rejection and produced no finding. Please post the required exact-head APPROVE so Brian can merge.

[bokelley]

CI recovery diagnosis for exact head c5b5826d6caea2eb267ead5c288c4068438ed0cf:

• Both Ladon attempts completed the primary analysis successfully with zero persisted findings, then remained needs_finalization.
• The finalization-only --resume invocation returned success in 101 ms with num_turns: 0 / $0 usage, so it never called mcp__ladon_findings__finalize_review. The Internal error: directory mismatch .../tsconfig.json text in the log is only an action-script comment explaining why --tsconfig-override is omitted; no such runtime error was emitted.
• A third rerun at the same action pins has no identified recovery mechanism and would repeat an infrastructure path that already failed identically twice.

Narrow recovery: do not change #1217 product or test code and do not keep rerunning Ladon. code_review is not a required main status context; all required contexts are green. The branch ruleset instead requires one independent approval (including last-push approval) and resolved threads; threads are resolved. Please have aao-secretariat post the formal APPROVE directly on this exact head. Track the zero-turn Ladon resume/finalization defect separately in adcontextprotocol/actions; it should be repaired there before relying on another automated retry.

[bokelley]

@aao-secretariat Fresh ready_for_review run 36271793703 also completed primary analysis successfully with zero persisted findings, then failed identically in needs_finalization; no approval was posted. This rules out stale rerun state. Exact head remains c5b5826d6caea2eb267ead5c288c4068438ed0cf, all 16 required contexts are green, and zero threads are unresolved. Please provide the direct exact-head APPROVE; no further Ladon rerun is planned.

[bokelley]

Ahead of the fix for aao-secretariat's four threads — I verified all four against source at c5b5826d and they're correct. Three additions so this can land in one pass instead of a second round.

Scope correction on my own earlier review first: I reviewed scripts/ci/reporting_interop/ and the two interop test files. scripts/ci/reporting_interop_matrix.py sits one level up in scripts/ci/, so it was outside what I covered — my "no untimed invocations under tests/ or scripts/ci/reporting_interop/" was accurate but did not include this file. I also didn't audit run_foundation_matrix.py for PG lifecycle. aao caught both; the findings are theirs.

1. A fourth unbounded connect — run_foundation_matrix.py:956

Thread 4113256653 lists _create_database/_drop_database (actual lines 450, 456, 468). There's a fourth:

with psycopg.connect(database_url, autocommit=True) as connection:
    connection.execute(migration)

Same missing connect_timeout/statement_timeout/lock_timeout, and this one runs a migration — the case most likely to block on a lock rather than fail fast. Worth folding into the same fix so all four sites match storyboard_orchestration.py:697-698, 821-822.

2. Three more unbounded subprocess calls in reporting_interop_matrix.py

Thread 4113251298 cites L523, L545, L785. All six subprocess.run calls in the file lack timeout= — also L507 (cosign version --json), L696 (node --version), L699 (npm --version).

Being fair about severity: these three are local version probes, nowhere near the risk of a hung npm install or gh api. But if the remedy is a timeout= plus TimeoutExpired → InputError conversion, applying it to 3 of 6 leaves the file inconsistent in exactly the way the thread objects to. Cheap to include.

3. Confirming the remedy for thread 4113257015 matches existing convention

start_new_session=True is already used in that file at L762, L1463 and L1808 — every long-lived Popen server launch. The storyboard subprocess.run at L1723 is the only spawn without it. So "launch it in a new session and group-kill on timeout" isn't introducing a new pattern, it's closing the one exception. Worth noting since the group-kill also needs os.killpg on TimeoutExpired — subprocess.run's own timeout handling still only kills the direct child even with start_new_session.

I'll re-review and post an exact-head verdict once the fix is pushed, including the TimeoutExpired → InputError conversion and the focused tests.

[bokelley]

Verified thread 4113269095 against source at c5b5826d. Confirmed — and one caution on the remedy.

Independently reproduced every element:

• _execution_credit_errors (L2149-2154) requires observed["database_identity"] ⊇ {name, cluster_identity}.
• All five cell sites assign the local database_identity = _create_database(...) but write it under the key "database" (L981, L1525, L1661, L1980, L2084). The variable name matches; the dict key doesn't.
• potentially_credited (L2211) keeps only cells with zero errors, so cell["database_identity"] at L2225/L2233 never evaluates today — the KeyError is unreachable rather than latent-crashing.

One correction to the thread's reasoning, which doesn't change the conclusion: _execution_credit_errors does not early-return on the credit gate at L2117 — it appends observed_control_disclaims_required_credit and keeps going. So the database_identity check genuinely is reached and does append unconditionally, exactly as stated. Every cell accumulates both errors, potentially_credited is always empty, and the machinery is inert and fail-closed. Agreed on Medium.

The caution: the thread offers "either write database_identity/cluster_identity at the cell sites or read "database" here." The second branch alone will not work. _create_database returns:

return {"name": row[0], "encoding": row[1], "collation": row[2], "version": version[0]}

No cluster_identity. So renaming the read to "database" still fails the {name, cluster_identity} subset check — cluster_identity has to be produced either way. Worth saying explicitly because the read-side rename is the cheaper-looking option and would leave the credit path just as dead.

And cluster_identity is load-bearing, not decorative. The uniqueness check at L2233 exists to prove each credited cell used a distinct database. Keyed on the current "database" payload the identity would be {name, encoding, collation, version} — encoding/collation/version are near-constant across the matrix, so the key degenerates to the name, and two same-named databases on different clusters would collide. The natural source is the cluster's own identifier:

SELECT system_identifier FROM pg_control_system()

Adding that to _create_database's returned dict satisfies the subset check and makes the uniqueness guarantee actually hold across clusters — which matters here given --external-pg-admin-dsn.

I'll verify this end-to-end on the next pushed head: that a cell can now reach potentially_credited, that the uniqueness check discriminates, and that required_cell_credit still gates as intended.

[bokelley]

@bokelley please re-review exact head 0bdf79c1c for the five confirmed Medium findings (bounded commands/DB operations, aggregate cleanup, owned process-group timeout, and executable database identity credit contract). Focused interop tests are 88/88; hooks, lint, and strict type checks pass. Full make test is still running locally. @aao-secretariat please provide a renewed exact-head formal review after CI is green.

[bokelley]

Coordinator check at 0bdf79c: the new storyboard timeout teardown still guards both os.killpg calls with process.poll() is None. communicate() can time out after the direct parent exits if a descendant keeps stdout/stderr open; after SIGINT, the parent can also exit while a descendant survives. In either case poll() is non-None and the process group is never killed, so timeout cleanup remains incomplete. Please signal the dedicated group independent of the parent exit state (handling ProcessLookupError) and add a parent-exited/descendant-alive regression. The current test keeps poll() as None until SIGKILL and misses this case.

[bokelley]

@bokelley follow-up at exact head 8e691e2f7: process-group timeout cleanup now signals the dedicated group regardless of direct-parent poll(). Focused regressions cover (1) parent exited while a descendant retains pipes and (2) a SIGINT-resistant descendant requiring SIGKILL. Focused interop suites are 89/89 and commit hooks pass. Please re-review this exact head; @aao-secretariat please renew formal review after CI.

[bokelley]

Verified 8e691e2 closes my process-group cleanup finding. I reran the real parent-exited/descendant-held-pipe reproduction against this head: it timed out at 1s, reported parent returncode 0, and the dedicated process group was gone after settlement. The previous head left that group alive. Targeted suite is still running; I will post its result separately.

[bokelley]

Exact-head focused validation on 8e691e2: 72 passed in tests/test_reporting_storyboard_orchestration.py and tests/test_reporting_interop_corpus.py; git diff --check clean. The actual parent-exited process-group reproduction also settled cleanly as noted above. No further blocker from my review; awaiting current-head CI and independent formal approval.

[aao-secretariat]

Ladon verdict: Approve

Approve — defensive hardening of new CI interop-harness code under scripts/ci/reporting_interop/ and tests/.

The reviewer verified operational-readiness concerns (bounded subprocess/DB timeouts, owned process-group SIGINT→SIGKILL cleanup, fail-closed drop-all) and confirmed test coverage. The database→database_identity key rename aligns emitters with the execution-credit checker. No public adcp.* surface, wire-shape, type-layering, or credential concerns apply.

Decision-table walkthrough:

• Row 1 (critical/high): no such findings.
• Row 2 (gated_paths + not APPROVED): gated_paths is false; does not fire.
• Row 3 (high_risk deleted): high_risk is false.
• Rows 4–5: no medium findings and high_risk is false.
• Row 6: no prior decision.
• Row 7: no no-auto-approve team match.
• Row 8: zero medium findings.
• Row 9: approve.

No blocking or medium findings reported.

[bokelley]

Expanded exact-head check on 8e691e2: all three interop suites pass, 89 tests total (corpus, matrix, storyboard orchestration). Process-group live reproduction and diff check also pass. No further blocker from my review.

[aao-secretariat]

Ladon verdict: Approve

Approve.

Operational-readiness pass on the reporting interop CI harness (scripts/ci/reporting_interop/), no changes to the public adcp.* surface. Bounded psycopg connections with statement/lock timeouts, subprocess timeouts on all external I/O (uv/npm/cosign/gh), process-group SIGINT→SIGKILL cleanup retaining partial output, aggregated fail-closed DB cleanup, and a database→database_identity key unification with nested cluster-identity validation. Logic and matching tests are correct.

No Critical/High/Medium findings. One non-blocking follow-up: the new _create_database SELECT (pg_control_system/current_setting('data_directory')) now requires a superuser-privileged admin DSN — worth documenting, but not a blocker.

Decision path: no critical/high findings (row 1 n/a); gated_paths false (row 2 n/a); high_risk false (rows 3/5 n/a); no medium findings (rows 4/8 n/a); no prior decision (row 6 n/a); no no-auto-approve team match (row 7 n/a). Falls through to row 9 → approve.

[bokelley]

Re-review at 8e691e2f. All five aao threads and my three added sites are closed in source. 89 tests pass (up from 67). One new Medium that the thread-5 fix introduced — details below; everything else is clear.

The residual: confirmed on the old head, fixed on this one

I reproduced it against the shipped _run_storyboard_process at both heads — same function, same input, only the head differs. A parent that exits while a descendant holds the pipes:

head	result	descendants surviving
0bdf79c1	HarnessError: …cleanup failed: process group did not exit after SIGKILL	1
8e691e2f	clean timeout, no cleanup failure	0

The old failure message was also actively misleading: it reported SIGKILL failing when the poll() gate meant SIGKILL was never sent. Both gates are gone now and the comment names the case precisely.

The new test_foundation_storyboard_timeout_signals_group_after_parent_exits genuinely guards it — I reintroduced the poll() gate and it fails (Right contains one more item: (4343, <Signals.SIGINT: 2>)). Not a decorative test.

Threads closed

• T1 + my added 507/696/699 — subprocess.run went 6 → 3, all with timeout= and TimeoutExpired handling. The version probes were consolidated into the bounded helpers rather than individually patched, which is better than what I asked for.
• T2 + my added L956 — all four connects collapsed into one _database_connect setting connect_timeout, statement_timeout, lock_timeout. Exactly one psycopg.connect remains in the file.
• T3 — _drop_databases isolates per item, attempts every name, aggregates into one HarnessError.
• T5 — fixed on the producer side (the correct branch): pg_control_system() + pg_postmaster_start_time()::text added, all seven cell sites now write database_identity, and producer/reader shapes align with non-empty-string validation at creation time.

Also re-ran my two earlier controls at this head — the P2 false-pass guard and the 009 listener guard each still fail when mutated out.

---

New Medium — the T5 fix requires superuser, breaking --external-pg-admin-dsn

_create_database now selects current_setting('data_directory'). That parameter is superuser-gated. Tested on a real PostgreSQL 16.14 with a LOGIN CREATEDB non-superuser — the realistic managed/external admin role:

data_directory:    ERROR: permission denied to examine "data_directory"
                   DETAIL: Only roles with privileges of the "pg_read_all_settings" role may examine this parameter.
pg_control_system: 7689990248917255199      ← works
postmaster_start:  2026-09-26 23:29:39+00   ← works

The exact harness SQL fails as a whole for that role. Only data_directory is privileged; the other two new fields are fine.

This isn't confined to the credit path — it's in _create_database, so the harness fails at database creation for any non-superuser admin DSN. CI won't catch it: the local Postgres service runs as superuser, so it stays green while the external-PG configuration that T4 was written about is broken.

Cheapest fix is to drop data_directory — system_identifier is the canonical cluster identity, and postmaster_started_at already discriminates restarts, so it adds little and is the only privileged one. Alternatives: grant pg_read_all_settings, or make that field best-effort with the identity still valid without it.

Non-blocking observation

Every cell still sets required_cell_credit: False, so no cell reaches potentially_credited and the credit path remains unexercised at runtime. The shape mismatch is genuinely fixed and test_foundation_database_identity_matches_execution_credit_contract covers the contract directly, so this is fine for a preflight reporting acceptance: False — flagging only so it's a deliberate choice rather than an assumption.

Everything except the data_directory finding is an approve from me at this head; recording it here since I can't set a formal state.

[bokelley]

@bokelley exact-head follow-up e1cac81d7: removed privileged current_setting('data_directory'); cluster identity is now the canonical pg_control_system().system_identifier plus pg_postmaster_start_time(). Added a real ADCP_PG_TEST_URL conformance regression that creates a LOGIN CREATEDB non-superuser and exercises _create_database end-to-end with owned cleanup. Local focused result: 89 passed, 1 PG-only skip; commit hooks pass. Please re-review this exact head. @aao-secretariat please renew formal exact-head review after CI, including the Postgres lane.

[bokelley]

e1cac81 removes the privileged data_directory read from database creation and credit identity; source diff is narrow and clean. I requested the restricted-role PG16 _create_database rerun from the reviewer, because the hosted PG lane uses superuser. Current-head CI/review are pending.

[aao-secretariat]

Ladon verdict: Approve

Approve — no blocking or Medium findings.

Subsequent pass on PR #1217. The delta drops data_directory from the reporting-interop harness Postgres cluster identity (it required pg_read_all_settings/superuser), re-indexes the row tuple correctly, removes the field from producer, consumer (_execution_credit_errors), and all fixtures in lockstep, and adds a real-Postgres contract test proving _create_database works for a CREATEDB non-superuser. Changes are confined to scripts/ci/ and tests/ — no public SDK surface, wire shape, or credential concerns.

Gate check: no critical/high/medium findings (row 1 n/a); gated_paths is false (row 2 n/a); high_risk false (rows 3/5 n/a); no data-loss/schema/infra medium (row 4 n/a); prior decision was approve, not escalate (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. Prior run also approved on a clean diff; this pass remains clean and is a net improvement (adds a real-Postgres contract test).

[bokelley]

Correction to my prior note: I initially filtered the diff to three paths and missed the new real-PG regression in tests/conformance/reporting/test_reporting_interop_foundation_database.py. It creates a LOGIN CREATEDB non-superuser, exercises _create_database end-to-end, verifies rolsuper=false and cluster identity, and cleans up. This directly guards the privilege failure. Reviewer retest and current-head CI remain in progress.

[bokelley]

Re-review at e1cac81d — the data_directory finding is closed, verified against a live unprivileged role. 89 tests pass. No open findings from me at this head.

Retested on real PostgreSQL 16.14

Spun up a cluster, created a LOGIN CREATEDB non-superuser (usesuper: False), and called the shipped _create_database from each head against that role:

head	result
8e691e2f	InsufficientPrivilege: permission denied to examine "data_directory"
e1cac81d	SUCCESS — cluster_identity = {postmaster_started_at, system_identifier}

Values returned for the unprivileged role: system_identifier='7689992252097386679', postmaster_started_at='2026-09-26 23:37:26.224307+00'.

I also closed the loop the mocks can't: fed those real identities into _execution_credit_errors. Under both superuser and unprivileged roles, database_identity_missing is not appended — so the producer/reader contract from thread 5 holds against real Postgres output, not just a fixture row.

The index shift is correct: row[3] → postmaster_started_at, row[4] → system_identifier, with validation updated to match.

One correction to the brief

No real restricted-role regression was added, only mocked row adjusted.

A real restricted-role test was added — tests/conformance/reporting/test_reporting_interop_foundation_database.py::test_create_database_supports_createdb_non_superuser. It's easy to miss because it lives under tests/conformance/reporting/ rather than top-level tests/, and is skipped unless ADCP_PG_TEST_URL is set. It creates a genuine CREATE ROLE … LOGIN CREATEDB and calls the real _create_database through it — no mock.

I verified it's a working guard rather than a passing formality:

• Against my live PG16 at this head: passes.
• Reintroduce current_setting('data_directory') into the query: fails, with exactly psycopg.errors.InsufficientPrivilege: permission denied to examine "data_directory".

And it does run in CI, which was the open question — a guard nobody executes is not a guard. Replaying the core lane's exact --ignore set from ci.yml:190-211 over tests/conformance/reporting/, the file is collected (none of the status_*, receipt_*, feed_*, production*, projection* globs match test_reporting_interop_foundation_*), and that job sets ADCP_PG_TEST_URL with a superuser admin, which is what the CREATE ROLE needs.

Status of everything else

All five aao threads and my three added sites remain closed; the storyboard process-group A/B and the P2/009 mutation controls all still hold from the 8e691e2f pass. The credit path is still inert at runtime (required_cell_credit is False everywhere) — unchanged, non-blocking, and now covered contractually by a real-Postgres test.

Trivial leftover, not worth a push on its own: tests/test_reporting_storyboard_orchestration.py:1031 still has identity = {"data_directory": "/owned"} in an unrelated fake Local class. It's inert — that attribute is never matched against the production identity shape — but it's a stale-looking reference if anyone greps for data_directory later.

Approve at this exact head from my side; recorded here since I can't set a formal state. Test cluster torn down, worktree clean, all spawned processes reaped.

[bokelley]

Addendum — reviewing the new guard itself, having now read it end to end (my verdict above covered running it, not its construction).

test_create_database_supports_createdb_non_superuser is well built, and stronger than I gave it credit for. Two properties matter:

It can't pass by accident. It asserts role_row == (False, True) — rolsuper=False, rolcreatedb=True — straight from pg_roles. So it proves the connection really was unprivileged. Without that, a misconfigured fixture that quietly connected as superuser would pass while testing nothing, which is exactly the false-pass shape we fixed earlier in this PR.

It fails two independent ways. The assertion is exact set equality:

assert set(identity["cluster_identity"]) == {"system_identifier", "postmaster_started_at"}

so reintroducing data_directory trips it either through InsufficientPrivilege on the unprivileged role (what my mutation produced) or through set equality if a future reader happens to have pg_read_all_settings. A subset check would have caught only the first.

Cleanup is correct too: the finally runs DROP DATABASE IF EXISTS … WITH (FORCE) and DROP ROLE IF EXISTS, so a mid-test failure doesn't strand a role or database in a shared cluster — the partial-creation path my mutation exercised left nothing behind.

Nothing to change here. Combined with the earlier confirmation that CI's core lane actually collects this file with ADCP_PG_TEST_URL set, the data_directory regression now has a real guard that runs, rather than a fixture that would have gone green regardless.

[bokelley]

Current-head local gate complete on e1cac81d7: make test passed — 11,932 passed, 2,256 skipped, 9 deselected, 1 xfailed in 35m32s; coverage 81.56% (80% required). The PR validation section now records this result. Head remains unchanged; awaiting the final required CI jobs.