Skip to content

test(reporting): prove JS and Python chunks verify in one database and bucket - #1470

Merged
bokelley merged 2 commits into
mainfrom
claude/issue-1457-js-interop
Oct 11, 2026
Merged

bokelley merged 2 commits into
mainfrom
claude/issue-1457-js-interop

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Part of #1457 (Stage 2 of adcontextprotocol/adcp#7996). The acceptance gate for shared row storage; builds on the merged storage, providers and retention (#1464 to #1469).

Change

JS-written chunks must verify in Python and Python-written chunks in JS, against one database and one bucket. This PR adds that proof:

  • tests/interop/reporting_row_storage_js_driver.cjs drives the shipped @adcp/sdk readers and writers against the same adcp_reporting_row_* tables and one filesystem bucket root, in both directions, for PostgreSQL-bound and object-bound revision and rows-only row sets. The live test runs when ADCP_JS_SDK_DIR points at a built adcp-client checkout.
  • The rows each SDK wrote are committed under tests/fixtures/reporting-row-storage-interop-v1/ and replayed here into a fresh database and bucket, so Python CI needs no Node.
  • Both SDKs resolve the same binding identity digest and namespace, and read each other's filesystem native versions (inode:birthtimeNs:mtimeNs:size).

How to run the live JS interop test

cd <adcp-client> && npm ci && npm run build
ADCP_JS_SDK_DIR=<adcp-client> ADCP_PG_TEST_URL=postgresql://... \
  pytest tests/conformance/reporting/test_reporting_row_storage_interop_pg.py

Without ADCP_JS_SDK_DIR the test replays the committed fixtures only. docs/reporting-row-storage.md gains the interop section.

Merged by the repo owner's watcher without waiting for code-owner review; @rachitm022 is requested for visibility and will test the finished stack.

🤖 Generated with Claude Code

…d bucket

Adds the Stage 2 acceptance gate from adcontextprotocol/adcp#7996: the shipped
@adcp/sdk row storage and this SDK write and read the same adcp_reporting_row_*
tables and one filesystem bucket root, in both directions, for PostgreSQL-bound
and object-bound revision and rows-only row sets.

- tests/interop/reporting_row_storage_js_driver.cjs drives the JS readers and
  writers; the live test runs when ADCP_JS_SDK_DIR points at a built adcp-client.
- The rows each SDK wrote are committed under
  tests/fixtures/reporting-row-storage-interop-v1/ and replayed here in a fresh
  database and bucket, so Python CI needs no Node.
- Both SDKs resolve the same binding identity digest and namespace, and read each
  other's filesystem native versions.

Part of #1457.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Comment thread tests/conformance/reporting/_row_storage_interop.py
aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Oct 11, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — test-only interop acceptance gate (Stage 2), no blocking findings.

This PR adds docs, two test modules, a JS driver, and two committed fixtures. No src/ or public-surface changes. The reviewer verified the replay path is correct (create_schema → ready_row_storage registers bindings before replay_state's row_sets inserts, satisfying the row_binding_id FK), native_versions are correctly regenerated on replay while bytes/digests remain the other SDK's, and the byte-level cross-SDK decompression check is genuine.

Decision trace: 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 (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.

Note: review_decision is REVIEW_REQUIRED, but with gated_paths false that does not gate approval.

Nit (non-blocking): tests/conformance/reporting/_row_storage_interop.py:265 — dead namespace_key param + misleading re-root comment in replay_state. Operational note from reviewer: run_js timeout (240s) does not kill the node subprocess, but this path is gated behind ADCP_JS_SDK_DIR and does not run in standard CI — low impact.

… replay

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — test-only interop refactor, no blocking findings.

This PR is a test-only change: it refactors replay_state in tests/conformance/reporting/_row_storage_interop.py to drop the namespace_key parameter and the namespace re-rooting, storing each recorded object at its verbatim key. The reviewer confirmed the change is self-consistent: replay inserts the writer's object_key verbatim (L303), puts the object at the same key (L267), and python_verify resolves reads by the stored object_key column — so verbatim keys faithfully model the interop contract. The old re-rooting rewrote the namespace segment and masked the portability assertion. New fixtures and docs round out the change; no src/adcp/** surface is touched.

Checked against repo gates: no public-API/semver changes, no generated-code hand-edits, no forward-compat union changes, no import-layering violations, no disabled tests or blanket type-ignores. No findings at any severity.

Decision table: 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); prior decision was approve (row 6 n/a); no no-auto-approve team match (row 7 n/a). Falls through to row 9 — approve. Consistent with the prior approve on the preceding head.

@bokelley
bokelley merged commit b1b7d67 into main Oct 11, 2026
46 checks passed
@bokelley
bokelley deleted the claude/issue-1457-js-interop branch October 11, 2026 17:25
@KonstantinMirin

Copy link
Copy Markdown
Collaborator

This test is the right acceptance gate for shared row storage. JS-written chunks verify through
the Python readers against one schema and one filesystem root, and Python-written chunks verify
through the JS readers. Both SDKs derive the same binding identity and namespace key. I have four
follow-ups after the merge, and the first one matters most.

Run the live test in CI

pytest skips test_one_database_and_one_bucket_in_both_directions unless ADCP_JS_SDK_DIR
points at a built adcp-client checkout and Node is installed (lines 62-66). No workflow sets the
variable, so the only test that runs JS code has not run in continuous integration (CI). The
replay tests do run, in the pg-conformance lane when the scope selects Postgres
(ci.yml:189-271). Add a job that checks out adcp-client at a pinned tag, runs
npm ci && npm run build, and runs this test against the service Postgres.
scripts/ci/reporting_interop/ pins @adcp/sdk versions for the cross-SDK matrix, and the same
shape works here.

Make the replay tests fail, not skip, without fixtures

test_rows_written_by_the_other_sdk_verify_here and
test_the_recorded_object_bytes_decompress_to_the_golden_chunks call pytest.skip(...) when a
fixture file is absent (lines 122 and 139). With tests/fixtures/reporting-row-storage-interop-v1/
removed, the module reports 4 skipped and stays green. The fixtures are committed, so assert
that they exist.

Fixture size

js-written.json and python-written.json are 12,914 lines each (164 KB). Each is a full dump of
adcp_reporting_row_sets, adcp_reporting_row_chunks, adcp_reporting_chunk_bodies, and the six
bucket objects, written with indent=1 (line 113). The dump covers 12 row sets derived from the
3,216-line golden fixture that the repository already has. The replay needs the 12 descriptors in
"expected", the chunk manifests, the object bytes, and the binding rows. Write the fixture
compactly and drop what replay_state recomputes, so that a reviewer can read the diff. These
fixtures also pin the chunk format, so a format change in adcp#7996 means regenerating them.

Smaller items

Four smaller items remain:

  • setup() asserts on store._row_storage (lines 58-59). Expose what the test needs, or go through
    the public reader.
  • The driver resolves the SDK entry from dist/lib/reporting/ledger/index.js when it exists, and
    otherwise from @adcp/sdk/reporting/ledger (tests/interop/reporting_row_storage_js_driver.cjs:36-38).
    Pin one; a stale dist/ tests a different build than the package.
  • ADCP_UPDATE_INTEROP_FIXTURES=1 writes fixtures from inside a test (lines 106-115). Move
    regeneration to a script under scripts/.
  • docs/reporting-row-storage.md:304 says "The JavaScript SDK can replay python-written.json the
    same way." adcp-client has no such test (main at 30e553ad). Either add it there or drop the
    sentence. The same section can state what the SDKs share. They share the row-set tables and
    bytes; each SDK keeps its revision headers and obligations in its own tables until Stage 3. That
    is why the different account lock keys are tolerable (the feat(reporting): write row chunks to object storage with fenced uploads #1467 lock audit).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants