Skip to content

fix(transcript): surface unsigned ts/seq trust boundary and per-room gaps - #97

Open
PASdeco wants to merge 3 commits into
flop-labs:mainfrom
PASdeco:fix/transcript-timestamp-auth
Open

PASdeco wants to merge 3 commits into
flop-labs:mainfrom
PASdeco:fix/transcript-timestamp-auth

Conversation

@PASdeco

@PASdeco PASdeco commented Sep 4, 2026 •

Copy link
Copy Markdown

Fixes #96

Problem
src/transcript.ts:103 signatures cover room|nonce|line only. seq/ts are venue metadata (transcriptRecord:131 -> Date.parse) but foldTranscript:236 uses record.timestampMs as nowMs for every deadline guard (machine.ts:121,158,178). A file supplier can rewrite ts and flip claimed↔refunded with all signatures valid. Same silent flip happens if a row is deleted (#93) - gap, no BAD verdict.

This PR does not refuse to fold (partial windows/post-reap are legitimate), it reports:

  • TranscriptFoldResult.warnings: string[]
  • per-room gap detected (seq 1->3), seq not strictly increasing, timestamp goes backwards
  • generic note when any deadline frame (accept/lock/reveal/refund) is present: “timestamps and seq are venue metadata, not covered by signature, verify settlement on the rail”

examples/audit-export.mjs prints WARN: and exits 1 on gap/ordering/backwards; generic note is informational. mcp/src/tools.ts:tclk_apply_transcript forwards warnings. SPEC.md:62 documents the boundary: transcript is coordination, not settlement proof.

Verification

No wire change. Follow-up could sort by (room,seq) (#93 B) but reporting is enough for audit use.

…gaps

foldTranscript now returns warnings: gap, seq ordering and timestamp
monotonicity per room, plus a generic note that ts/seq are venue
metadata not covered by the Ed25519 signature (room|nonce|line only).
A file supplier can rewrite ts to move a reveal across refundAfterMs
and flip claimed<->refunded with all signatures valid — the transcript
is coordination, not settlement proof; verify on the rail. This closes
the timestamp-malleability half of flop-labs#93's scope note, and the gap
detection covers the same missing-row flip as flop-labs#93 C without refusing
legitimate partial windows. audit-export.mjs and tclk_apply_transcript
now surface warnings; SPEC 2 documents the boundary.
@thangvmt

thangvmt commented Sep 4, 2026

Copy link
Copy Markdown

Ran this branch against the transcript shapes from #93 rather than reading it, because the two
of us are checking overlapping things in the same two files and it seemed better to know than to
guess. It holds up on both of the cases I had, and misses one I did not expect.

Method: one contract, offer and accept on the board, lock/reveal/refund in the derived
room, exit code taken from the process rather than from a pipeline.

honest                              exit 0   1 warning, no refusal
reveal deleted        (1,_,3)       exit 1   gap detected, refuses
reveal deleted + row repeated (1,2,2,4)  exit 1   seq not strictly increasing, then gap

The duplicate case is the one I want to flag as a point in your favour. Your check compares
consecutive pairs, so 2 -> 2 trips "not strictly increasing" before the arithmetic matters. I
had the same check written as a count against a span width, and it read 1,2,2,4 as whole until
@alitiknazoglu found it an hour ago. Pairwise is the more robust shape and I would say so even
if the PRs were not adjacent.

The case it misses

Delete the reveal and put an unsigned row in its place, so the positions read 1,2,3 again.
Same contract, same minute, both branches:

#97   warnings: the generic ts/seq trust boundary only
      no gap warning, no refusal
      fold → refunded                          exit 0

#94   mb-p-tclk-…: 2 verified rows, seq 1..3 — GAP
      a signed row is missing from the deal room
      fold → refunded                          exit 1

The fold marks the padding row BAD, so it is not invisible, but the honest export also carries
one BAD (the refund arriving after the reveal). Compare the two by warnings and exit code, and
the censored file is the cleaner of the pair.

The cause is that byRoom is built from records directly, so an unverified row still occupies
a position. foldTranscript already computes a verdict per record; grouping only the ones that
verify closes it, and the row still shows up as BAD on its own line either way.

What mine does not do

Ordering and ts are yours and I have nothing equivalent. My check sorts positions, so a room
supplied out of order reads clean to it, and it says nothing at all about timestamps — #96 is a
sharper statement of that than anything I had. It also only asks about a derived deal room, and
requires firstSeq === 1 there, which yours does not need because it never claims to be a
completeness statement.

So the two seem to be complements rather than alternatives: yours catches ordering, timestamps
and repeats, mine catches an unverified row occupying a position and a missing prefix. Whether
that is one check or two is a maintainer's call, and I mention it mostly because we are both
editing src/transcript.ts and examples/audit-export.mjs and someone is going to have to
reconcile them.

Unsigned/BAD rows still appear as BAD in steps but must not make a
censored verified seq look contiguous. Verified 1,3 padded with
unverified 2 at seq 2 now correctly trips gap detected, matching
thangvmt's flop-labs#94 shape. Pairwise seq check already handled
1,2,2,4 duplicate.
@PASdeco

PASdeco commented Sep 4, 2026

Copy link
Copy Markdown
Author

Thanks for running it against #93 shapes, good catch on the unsigned padding.

Fixed: byRoom now built from verified rows only, so 1,3 padded with unsigned 2 still trips gap detected (verified 1->3), while the BAD row stays in steps as before. Your 1,2,2,4 duplicate case stays seq not strictly increasing via the pairwise check and agree that's more robust than span width.

Yours (verified gap + firstSeq===1) and this one (ordering/timestamps/repeats) do feel like complements. Well, happy to let maintainers reconcile to one check or keep both.

@jokazza84 jokazza84 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.

I re-ran the current head addd5db6d7cfffaa70f3ceb7f1de1754e94b4a4b. The verified-row change also fixes the null-record regression from the previous head, so that part now holds.

There is still one correctness gap in the offline-audit claim: the gap detector treats seq as evidence of completeness even though this PR correctly documents that seq is not covered by the signature.

I built one authenticated contract with offer/accept on tclk-offers and lock/reveal/refund at deal-room seq 1,2,3:

honest
  every verifyTranscriptRecord: true
  fold: claimed
  warnings: generic unsigned-metadata warning only
  audit fatal predicate: false
  resulting exit: 0

delete reveal; change refund metadata seq 3 -> 2
  every verifyTranscriptRecord: true
  fold: refunded
  warnings: the same generic warning only
  audit fatal predicate: false
  resulting exit: 0

No signed byte changes: the signature is over room|nonce|line, so changing seq does not invalidate the refund record. The supplied file therefore removes the claim, reports a contiguous 1,2, changes the terminal outcome, and still passes the new selective fatal check.

The per-room warnings are useful diagnostics for accidental truncation or reordering, but an offline file cannot use unsigned seq to establish completeness against its supplier. Please make that limitation structural in audit-export.mjs: for example, do not give a terminal fold a proof-like success status when the only completeness evidence is unsigned metadata (or distinguish “replay succeeded” from “audit evidence verified”). A focused regression for delete-and-renumber should pin whichever contract you choose.

thangvmt added a commit to thangvmt/tclk that referenced this pull request Sep 4, 2026
…upports

The tool printed one line, `fold → claimed`, and exited 0. A reader hears that as an audit
result. It is not one: the fold says what these records amount to, never that these are all the
records, and the gap check cannot close that difference because `seq` is venue metadata outside
the signed preimage. A supplier that deletes a row and renumbers what it keeps leaves no gap,
and one that drops the last row leaves none either.

The conclusion is now two claims that cannot be read as one:

```
replay   → refunded
evidence → completeness NOT established. A gap is evidence of absence; the absence of
           a gap is not evidence of presence …
           Exit 0 means the replay reached a terminal status. It does not mean audited.
```

The honest transcript prints that too. Nothing this tool can see distinguishes a complete file
from a renumbered one, so nothing it prints should sound like it can. A detected gap still says
INCOMPLETE and exits 1, which is the one claim the counting does support.

Raised by @jokazza84 on flop-labs#97, against that PR's version of the same check. It applies here
unchanged, and prose in the caveat line was the wrong place for it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@thangvmt

thangvmt commented Sep 4, 2026

Copy link
Copy Markdown

@jokazza84's point applies to #94 unchanged, and I checked rather than assumed. Delete the
reveal, renumber the refund from 3 to 2, and my tool prints no gap detected and exits 0 on a
deal that was claimed. Same hole, different arithmetic — counting distinct positions does not
help when the positions are rewritten.

What I changed there, in case it is useful here. The conclusion is now two claims that cannot be
read as one:

replay   → refunded
evidence → completeness NOT established. A gap is evidence of absence; the absence of
           a gap is not evidence of presence, because seq is venue metadata outside the
           signature …
           Exit 0 means the replay reached a terminal status. It does not mean audited.

The honest transcript prints that same evidence line. Nothing either of our checks can see
distinguishes a complete file from a renumbered one, so nothing either prints should sound like
it can. A detected gap still exits 1, which is the one claim the counting does support.

The caveat was already in my output as prose and it was not enough. It sat under a line that
read like a verdict, and a reader takes the verdict.

hayulpapax added a commit to hayulpapax/tclk that referenced this pull request Sep 5, 2026
A reordering check built on `seq` is defeated by editing `seq`. Both open
branches on flop-labs#93 read venue metadata to decide order, and flop-labs#93's matrix records
that relabelling a row beats them: moving one row the supplier already holds
costs a character and reads the same afterwards.

`nonce` is different. It is inside the signed preimage `room|nonce|line`, and
the venue requires it to exceed the last nonce that key used in that room, so a
signer's own records carry an ordering the supplier cannot rewrite. Reordering
two of them moves their nonces out of order, and putting the nonces back means
forging a signature over a different preimage.

`checkNonceOrder(records)` reports those positions. Scope is per `(room,
sender)`, which is the scope the venue enforces: two parties give two attested
chains and no attested interleaving, so it makes no claim about a payer's record
relative to a payee's. It does not detect deletion or end-truncation — nothing
in an export does — and it ignores records whose signature does not verify,
because an unverified nonce is asserted rather than attested.

Deliberately a separate exported function rather than a change to
`foldTranscript`'s result shape, so it lands whichever of flop-labs#94/flop-labs#97 goes in and
either can call it.

Measured on live exports before writing it: grouping signed records by sender in
export order, per-key nonce order held in 585 records over 24 multi-record keys
on one mailbox room and 55 over 7 on `signing-messages`, with zero violations,
and 12,387 over 565 keys on `tclk-offers` with one. That one is a pair the venue
itself ordered against its own clock — the key's nonces are ordered and the
venue's `seq` is not — which is the boundary the doc comment names: the venue
only looks for a key's last nonce within the newest portion of a room it scans,
so a long busy board can legitimately show a lower nonce. A derived deal room is
never near it.

SPEC.md and the code agree; this adds a check over what §2 already says about
which fields a signature covers, and changes no wire format.

Hostile counterparty: nothing. The function reads records and returns positions;
it holds no secret, performs no write, and refuses to draw a conclusion from a
record it could not authenticate.

CI locally: build clean; `pnpm -r --include-workspace-root test` gives 108
passing against 103 before, plus mcp's 40 and 33. The one failure is
`schema-conformance` on a Windows checkout, present identically before this
change and tracked as flop-labs#90.
jokazza84 review: delete reveal + renumber refund 3->2 leaves verified
1,2 contiguous with all signatures valid (seq/ts unsigned), so gap
check cannot prove completeness. audit-export now prints replay vs
evidence separately — replay -> status, evidence -> completeness NOT
established / INCOMPLETE — exit 0 means replay hit terminal, not
audited. Honest files print it too. Adds delete-and-renumber
regression pinning the limit.
@PASdeco

PASdeco commented Sep 5, 2026

Copy link
Copy Markdown
Author

I re-ran the current head addd5db6d7cfffaa70f3ceb7f1de1754e94b4a4b. The verified-row change also fixes the null-record regression from the previous head, so that part now holds.

There is still one correctness gap in the offline-audit claim: the gap detector treats seq as evidence of completeness even though this PR correctly documents that seq is not covered by the signature.

I built one authenticated contract with offer/accept on tclk-offers and lock/reveal/refund at deal-room seq 1,2,3:

honest
  every verifyTranscriptRecord: true
  fold: claimed
  warnings: generic unsigned-metadata warning only
  audit fatal predicate: false
  resulting exit: 0

delete reveal; change refund metadata seq 3 -> 2
  every verifyTranscriptRecord: true
  fold: refunded
  warnings: the same generic warning only
  audit fatal predicate: false
  resulting exit: 0

No signed byte changes: the signature is over room|nonce|line, so changing seq does not invalidate the refund record. The supplied file therefore removes the claim, reports a contiguous 1,2, changes the terminal outcome, and still passes the new selective fatal check.

The per-room warnings are useful diagnostics for accidental truncation or reordering, but an offline file cannot use unsigned seq to establish completeness against its supplier. Please make that limitation structural in audit-export.mjs: for example, do not give a terminal fold a proof-like success status when the only completeness evidence is unsigned metadata (or distinguish “replay succeeded” from “audit evidence verified”). A focused regression for delete-and-renumber should pin whichever contract you choose.

@jokazza84 Thanks - fixed in aedd1c0.

Please make that limitation structural in audit-export.mjs: for example, do not give a terminal fold a proof-like success status when the only completeness evidence is unsigned metadata

Done: audit-export now prints replay → status separate from evidence → completeness NOT established / INCOMPLETE. exit 0 means replay hit terminal, not audited. Honest files print it too. Gap still INCOMPLETE exit 1.

A focused regression for delete-and-renumber should pin whichever contract you choose.

Added: delete-and-renumber leaves no gap in tests/transcript.test.ts - honest claimed, renumbered 3->2 contiguous 1,2 refunded all-ok no-gap. Pins the limit.

@thangvmt

thangvmt commented Sep 6, 2026

Copy link
Copy Markdown

Patch is up as PASdeco#1 against this branch, carrying the three pieces @sv named on #94.

Two corrections to what I had on #94, both found while porting rather than after:

audit-export.mjs exited 0 on a censored 2,3,4 transcript. The fatal predicate recognised only gap detected, seq not strictly increasing and timestamp goes backwards, so the new warning printed while replay still said claimed and the process succeeded. The anchor is in that predicate now, with a CLI test that runs the real script and pins the exit code as well as the message.

The MCP fixture numbered seq globally across tclk-offers and the deal room, which started the derived room at 2 and tripped the anchor on the happy path. Nothing asserted on warnings, so it passed with a false warning. records() numbers per room now, and there is a test for propagation through tclk_apply_transcript.

Fresh clone at aedd1c0 with pnpm install: build clean, root 108/108, mcp 41/41, worker 33/33, and red-green holds on both new tests.

@PASdeco yours to take or leave, and happy to reshape it if you would rather fold the check in differently.

@luch91 luch91 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.

I audited the current head aedd1c087415e453decd757565910ac8e4ed4d0c against main.

The revised implementation now:

  • computes per-room ordering/gap warnings from authenticated rows only, while retaining rejected rows in steps;
  • preserves the distinction between a malformed/reordered supplied stream and a partial but otherwise valid window;
  • separates replay -> status from evidence -> completeness NOT established in audit-export.mjs;
  • keeps tclk_apply_transcript warnings visible to MCP callers; and
  • pins the important delete-and-renumber case: all retained signatures still verify, the replay can change terminal status, and no gap warning can prove completeness when seq is unsigned.

The current source and tests are consistent with the documented trust boundary. GitHub does not expose a full runtime check for this fork head here, so this approval is a source-level review and does not claim an additional local test run.

@khenzarr

Copy link
Copy Markdown

Independent validation against current main 5cc4ab93efbc8999a3a7e1471b639deca25998ea and this PR head aedd1c0632373d434033479871b037368ee209d0 found a performance regression in the new warning pass.

I used 2,000 deterministic, locally signed TranscriptRecord values (same valid Ed25519 signature tuple, sequential venue metadata) and timed one foldTranscript(records) call after building each tree:

main    2000 records:  3852.5 ms
#97     2000 records: 11297.4 ms

The ~2.93x result matches the source path: each valid record is verified once while building byRoom, again inside hasDeadlineFrame, and a third time in the actual fold. Ed25519 verification dominates large retained exports; this makes the warning feature roughly triple the cost even for records whose lines do not decode as TCLK frames.

The functional checks otherwise pass: build passes; all 101 root tests excluding the unrelated Windows CRLF schema-conformance failure pass; MCP 40/40 and Worker 33/33 pass. Current main reproduces that same CRLF failure (#90/#121).

Could the first traversal retain each record's verification result (and decoded frame where available) and reuse it for gap analysis, deadline detection, and the fold? A focused regression can instrument or inject the verifier if you want a structural one-call-per-record assertion; otherwise a sufficiently large deterministic timing test would be much more fragile. This is not a wire/state-machine objection—the replay/evidence wording and delete-and-renumber coverage address the earlier correctness concern.

@osr21

osr21 commented Sep 21, 2026

Copy link
Copy Markdown

Reconciliation result after running both current heads locally:

I also attempted the merge of #110 into #97. src/transcript.ts merges cleanly; the only textual conflicts are examples/audit-export.mjs and the import/test additions in tests/transcript.test.ts. They are complementary, not semantic conflicts.

Intended combined behavior

  1. Preserve fix(transcript): surface unsigned ts/seq trust boundary and per-room gaps #97's language and output distinction: a terminal fold is a replay, never evidence that the supplied file is complete.
  2. Keep fix(transcript): surface unsigned ts/seq trust boundary and per-room gaps #97's authenticated-row gap/timestamp/sequence warnings and fatal audit handling.
  3. Add feat(transcript): detect reordering the supplier cannot renumber away #110's checkNonceOrder(...) result to the same auditor run. Print its per-signer diagnostics and refuse an evidence conclusion when it reports a signed-order violation.
  4. Keep both regression suites, including fix(transcript): surface unsigned ts/seq trust boundary and per-room gaps #97's delete-and-renumber case and feat(transcript): detect reordering the supplier cannot renumber away #110's 5, 3, 4 predecessor case plus its documented substitution limit.

Merge blocker: authenticate once per call

#97 currently calls verifyTranscriptRecord while building byRoom, again in hasDeadlineFrame, and again during the fold. Its existing benchmark review measured about 2.93x main's time on 2,000 records. Calling #110's standalone checkNonceOrder from the merged auditor would add another full verification walk.

Please factor a per-call authenticated-record analysis/cache before combining the branches: each record should be verified once, with that verification result (and decoded frame where useful) reused by the warning analysis, deadline detection, fold, and nonce-order walk. The public checkNonceOrder API can remain standalone; the combined auditor should consume the shared analysis rather than trigger a second pass.

That leaves the security meaning precise: gap checks detect some omissions, nonce order detects some reorderings even after unsigned seq/ts are relabelled, and neither absence of a warning nor a terminal replay proves transcript completeness or settlement.

@luch91 luch91 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.

The latest exact-head validation found a material performance regression: the new warning pass verifies each record repeatedly (about 2.93× the current main cost on a deterministic 2,000-record run). The combined #110 review also identifies another full verification walk if nonce-order analysis is added. Please introduce a per-call authenticated-record analysis/cache so gap analysis, deadline detection, folding, and nonce-order checks reuse one verification result per record, then add focused coverage and rerun the full workspace gates.

@vanilla3

Copy link
Copy Markdown

One interaction with #206 that the fixtures here don't cover. foldTranscript builds byRoom from every verified row, so the deal's own offer and accept on tclk-offers go through the gap rule too, and examples/audit-export.mjs treats gap detected as fatal. Every contract shares the board, so those two seqs are adjacent only if nothing else was posted between them.

I ran audit-export.mjs on byte-exact exports, signed with the test-suite seeds from tests/transcript.test.ts. Every row has an honest deal room (lock at seq 1, reveal at seq 2); only the board seqs change:

offer → accept seqs on the board main 5cc4ab9 this PR aedd1c0
1 → 2 exit 0, claimed exit 0, claimed
100 → 102 (one unrelated offer between) exit 0, claimed exit 1, gap detected (seq 100 -> 102 …), INCOMPLETE
8812301 → 8813977 (the seqs from WIZARDspace's #206 review) exit 0, claimed exit 1, INCOMPLETE

If this merges as is, the preserved-handshake flow in #206 would end at exit 1 whenever the saved offer and accept are not adjacent on the board. #206 says to keep the original seq, and a non-adjacent pair gets through only if its unsigned seqs are rewritten to be adjacent, which is the kind of edit this PR warns auditors about. tclk_apply_transcript passes the same warning on, but there it is not fatal.

As thangvmt put it on #93, a gap between one contract's board rows carries no information. Locally, skipping the gap branch for OFFER_ROOM makes all three rows exit 0. The root vitest suite passes 105/105 with it, and a real deal-room gap (1 → 3) is still refused with exit 1. Another way would be to scope the check to rooms matching DEAL_ROOM_NAME, as thangvmt's PASdeco#1 does for its seq-1 anchor. Every board fixture in this PR and in PASdeco#1 uses adjacent seqs, so a regression test with a non-adjacent pair would pin it.

I can share the fixture generator if that helps.

This branch has not been deployed

No deployments
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.

foldTranscript uses unsigned venue ts for deadline guards - editing ts flips claimed↔refunded with all signatures valid

7 participants