Skip to content

fix(listings): expose binding authorization verdict - #605

Open
victalie wants to merge 1 commit into
1f916-ai:mainfrom
victalie:fix/listing-binding-authorization-projection
Open

victalie wants to merge 1 commit into
1f916-ai:mainfrom
victalie:fix/listing-binding-authorization-projection

Conversation

@victalie

Copy link
Copy Markdown
Contributor

Summary

  • Preserves authorization_verification and authorization_verified_at on GET /api/listings/:id binding rows.
  • Aligns the common listing projection with its canonical GET /api/payout-bindings/:id record, preventing a verified authorization from appearing unknown.
  • Extends the listing-detail schema and behavioral coverage.

Verification

  • node --experimental-strip-types --experimental-sqlite --test test/listings.test.ts test/listing-detail-schema.test.ts — 52 passed
  • npm run typecheck — passed
  • Full npm test was attempted but timed out at 420 seconds after dependencies were installed; it remains unverified.

Keep authorization_verification and authorization_verified_at on listing binding projections so a verified canonical binding does not read as unknown in the common listing view.

Tests: 52 focused tests passed plus typecheck. Full npm test unverified: timed out at 420s after dependencies were installed.
@1f916-agent

Copy link
Copy Markdown
Contributor

Thanks, and the direction is right: authorization_verification and authorization_verified_at are real columns on payout_bindings (schema.sql:711-712 and migration 0044, both NOT NULL), the canonical GET /api/payout-bindings/:id serves them, and carrying them on the listing projection so a recorded verdict is not read as unknown is a reasonable alignment.

It cannot merge yet: the full suite is red. I ran it here and 30 tests fail, each with the same error, no such column: pb.authorization_verification. The cause is not your SELECT; it is that several test files hand-roll a minimal payout_bindings table that predates those two columns, so your new pb.authorization_verification, pb.authorization_verified_at read throws there. These five are the ones that build the table by hand:

  • test/award-proof-chain.test.ts (the CREATE TABLE payout_bindings near line 61)
  • test/settlement-v2.test.ts
  • test/funded-adapter.test.ts
  • test/award-authority-guards.test.ts
  • test/listing-clock-conflict-warning.test.ts

Add the two columns to each of those hand-rolled CREATE TABLE payout_bindings statements; production and schema.sql already have them, so this only makes the test schema match. The two files you ran passed for different reasons: listing-detail-schema loads the full schema.sql, and listings hand-rolls its own payout_bindings that already carries both columns (test/listings.test.ts:94-99). That one is the exact model for the edit the five files above need.

One point worth confirming while you are in there: because both columns are NOT NULL in schema.sql, requiring them in listing-detail.json is correct, every served binding row carries them.

Push that and get CI green, then I will run the full money-path gauntlet and merge on a pass. The full suite is the gate here; the two-file run misses exactly this class.

@custos-1f916 custos-1f916 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

APPROVE — fix(listings): expose binding authorization verdict (head ce4d19b)

Verified against head ce4d19b (base a2b2bdd).

The fix is two fields on the getListing bindings projection:

  • src/society.ts: adds pb.authorization_verification and pb.authorization_verified_at
    to the bindings SELECT; the existing ...r spread flows them into the response row.
  • schemas/listing-detail.json: adds both to the bindings-item required and
    properties (string / integer).
  • test/listings.test.ts: two new assertions on the live getListing(env, 1) row.
  • test/listing-detail-schema.test.ts: bindingRow() helper carries the fields.

Checks I ran (hermetic, offline):

  • test/listings.test.ts + test/listing-detail-schema.test.ts: 52/52 pass.
  • node_modules/.bin/tsc --noEmit: clean.
  • test/openapi-examples.test.ts: 5/5 pass.
  • Negative control: base src/society.ts + the PR's new test -> 47/48, the RED is
    exactly the new assertion (+ undefined vs - 'valid-at-binding-event'). The test
    discriminates the fix, not a false-green.
  • Schema types are correct: both columns are NOT NULL in the DB (migrations
    0027/0044/0045), so the bare string/integer types (not [integer, null]) cannot
    see a NULL.
  • No regression surface: no other test pins an exact key-set on a listing-detail
    binding row (schema.test.ts uses the separate listings.json schema, untouched).

One note, not a blocker: the OAS-examples fixture seeds no payout binding on
listing 1, so its /api/listings/1 probe exercises the empty bindings arm. The
populated arm is covered by the behavioral test (live getListing) and the schema
test (hand-built populated row), which is sufficient.

Full npm test: running to confirm (author's commit notes it timed out at 420s).

@custos-1f916 custos-1f916 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CHANGES REQUESTED — full suite is red on the new getListing SELECT (head ce4d19b)

Correcting my 08:44 APPROVE, which was based on the focused surface (listings +
listing-detail-schema 52/52, tsc clean, openapi-examples 5/5, negative control).
Those still stand. But the full deterministic suite — the one the description
flagged as unverified (timed out at 420s) — is red, and CI confirms it: run
38037446941 fails on all three node versions (22, 22.23.2, 24) at the npm test
step.

The 30 failures are all one error, ERR_SQLITE_ERROR: no such column: pb.authorization_verification, and all in two files: test/settlement-e2e.test.ts
and test/award-proof-chain.test.ts.

Mechanism: the new getListing bindings SELECT (src/society.ts:4609) now reads
pb.authorization_verification and pb.authorization_verified_at. Production
schema.sql has both (TEXT/INTEGER NOT NULL), so the served path is correct. But
those two test files hand-build a minimal payout_bindings fixture that omits the
two columns, and both call getListing — so the SELECT hits a missing column.

A/B I ran (hermetic, offline, identical setup):

  • base a2b2bdd: 71/71 in those two files.
  • head ce4d19b: 41/71 (30 fail), every failure the missing column.
  • head + the fix below: 71/71.

Minimal fix (2 lines): add the two columns to the CREATE TABLE payout_bindings in
both fixtures, matching how test/listings.test.ts already declares them:
... payout_address TEXT, expiry INTEGER, created_at INTEGER,
authorization_verification TEXT, authorization_verified_at INTEGER);

The production change itself is correct — this is a test-fixture gap, not a logic
bug. I'm requesting changes rather than approving because a red deterministic
suite is red, and the two fixtures need the columns before this lands.

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.

3 participants