From 7bbecd75bfcf04979fd7b304c5aba892ff3937ef Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 1 Oct 2026 16:32:31 -0400 Subject: [PATCH 1/3] fix(client): a 304 confirms a payload rather than establishing one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A poll adopted the response `ETag` after any body that did not throw, and a 304 published a first payload unconditionally. Between them, a body this SDK could not apply lent its etag to the next request, and the 304 that answered it reported the empty committed set as current: initialized, healthy, and with nothing in a 304 to notice it on. `isInitialized()` is what authorizes `writeSkills('*')` to prune, so that store read as an environment whose every skill was revoked and deleted the last known good `SKILL.md` files on disk — the widest version of the hazard the probe exists to prevent. Three claims narrowed to what actually happened: - `payloadTransferred` reports a commit only when it applied a pending set. A `none` intent builds none, nor does an `intentCode` this SDK does not recognise, and a foreign payload's contents are declined — those transfers now report nothing, where they reported a commit. They no longer adopt the selector of a payload that was never applied either, which on its own left a store resuming from content it did not hold while every diagnostic read healthy. - A poll adopts the response `ETag` only from a body that completed an exchange: a committed payload, or a `none` intent, which is the server saying the content held is what the etag describes. An unrecognised intent says the opposite — the body carried objects `ignoreUnderUnknownIntent` dropped — so leaving the poll unconditional is also what keeps that body arriving and its warning repeating, rather than silenced behind a 304. - A 304 no longer publishes a first payload. It confirms the payload the store holds; the exchange it stands in for, the `none` intent, does not publish one either. TESTING.md §3.25 defines `isInitialized()` as true "once a payload has committed", and §3.21/§3.22 turn prune suppression on it, so none of this is a new rule — it is the existing one reaching the poll path. Python carries the same fix, and the two transports stay line-for-line on this. Two tests pinned the old behaviour and now pin the corrected one. The polling one justified it as "a reconnect with a cached basis", which this transport has no mechanism for: `basis` and `etag` both start `null` with no injection point, so a 304 reaching a store that holds nothing takes a server answering a request that carried no etag at all. The lifecycle one asserted a 304 over an empty store "counts as initialized — the payload held is confirmed current", with no payload held; it now exercises the 304 the store can actually reach, offering an etag it earned. `changes: []` and `basis: null` are stated explicitly on the new outcome rather than left off, because Python's dataclass defaults give exactly those and this is the same outcome object on both sides. Co-Authored-By: Claude Opus 5 --- packages/client/agents.md | 2 + .../client/src/__tests__/skills-fdv2.test.ts | 116 ++++++++++++++++-- packages/client/src/skills-fdv2.ts | 80 +++++++++--- 3 files changed, 175 insertions(+), 23 deletions(-) diff --git a/packages/client/agents.md b/packages/client/agents.md index 9534cca7..8807aa0c 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -65,6 +65,8 @@ The `3` after the delimiter is what a `{key, version}` reference pins and what b **Changes commit at `payload-transferred`, not as objects arrive.** A payload version is the unit of consistency: a half-applied full transfer would publish a state the server never described, and would briefly empty the store — which, with pruning on, is the difference between a reconcile and deleting a customer's skill files. An interrupted transfer therefore leaves last known good intact, and listeners fire at commit — once per changed object, all of them at `payload-transferred` (which is why `watchSkills` debounces: a per-object notification does not coalesce a payload on its own). +**A commit is the only thing that publishes a first payload, and a 304 is not one.** `isInitialized()` — the fact `writeSkills('*')` authorizes a prune on — goes true when a payload *commits*, so every other answer has to stop short of claiming one. A `payload-transferred` that applied nothing reports neither a commit nor an up-to-date answer, and does not adopt the selector of a payload it never applied: a `none` intent builds no pending set, nor does an `intentCode` this SDK does not recognise, and a foreign payload's contents are declined. A poll adopts the response `ETag` only from a body that completed an exchange — a commit, or a `none` intent, which is the server saying the content held is what the etag describes; an unrecognised intent says the opposite, since the body carried objects `ignoreUnderUnknownIntent` dropped, and leaving that poll unconditional is what keeps the body arriving and the warning repeating instead of silenced behind a 304. And a 304 *confirms* the payload held rather than establishing one, because the exchange it stands in for cannot establish one either. Loosen any of the three and the other two carry a store that received nothing into a prune of every managed `SKILL.md` on disk: an empty committed set reads as an environment that revoked every skill, and a 304 carries nothing to notice it on. There is no cached basis to make it safe — `basis` and `etag` both start `null` with no injection point, so a 304 reaching a store that holds nothing takes a server answering a request that carried no etag at all. + **`objectsRevoked` counts revocations, not tombstones.** A full transfer revokes by omission, so `payloadTransferred` diffs the committed set against the pending one and pushes the departures into `changes` as tombstones at `(key, version)` granularity — a listener that reads versions needs both halves of a version move. The *counter* is coarser on purpose: only a key the payload dropped altogether counts, because a key surviving under a new version was never revoked, and this is a field operators alert on. `keysFullyRevoked` is where the two granularities part. **The first payload intent is read, and is assumed to be the skill payload.** The `kinds` declaration narrows the connection to one payload and the protocol requires a client to ignore all but the first payload intent, so `payloads[0]` is both what arrives and what the protocol says to read. The cost of that assumption is that an `xfer-full` for somebody *else's* payload would start an empty pending set, and the next `payload-transferred` would publish it — every skill reported revoked, and with pruning on, a customer's files deleted. `ProtocolReader` therefore learns which payload skills arrive on, from the intent's `id` or from the `(p::)` selector, and declines to apply a transfer of any other: once at warning level, counted in `diagnostics.payloadsIgnored`, holding last known good. A transfer that names no payload is applied, since one-payload delivery is the common case. The residual is the first transfer of a connection — before a skill has arrived there is nothing to compare against — which is what the separate warning on a multi-payload intent is for. diff --git a/packages/client/src/__tests__/skills-fdv2.test.ts b/packages/client/src/__tests__/skills-fdv2.test.ts index 387aab71..3d6da1fb 100644 --- a/packages/client/src/__tests__/skills-fdv2.test.ts +++ b/packages/client/src/__tests__/skills-fdv2.test.ts @@ -841,12 +841,60 @@ describe('protocol reader', () => { expect(outcome.fatal).toBeTruthy(); }); - it('holds everything and commits on transfer-none', () => { + it('holds everything and commits nothing on transfer-none', () => { + // `none` is the server saying the payload held is current, so nothing is + // applied and nothing is committed. It is a *completed exchange* — which is + // what breaks a row of failures, and what lets the next request offer the + // body's etag — but it is not a payload. Publishing one would make + // `isInitialized()` true over whatever the store happens to hold, and that is + // the fact `writeSkills('*')` prunes on. const held = new SkillObjectSet(); const reader = new ProtocolReader(held); drive(reader, fullPayload([['put-object', putSkill()]])); - drive(reader, events(['server-intent', serverIntent('none')], ['payload-transferred', transferred('basis-2')])); + const [intent, outcome] = drive( + reader, + events(['server-intent', serverIntent('none')], ['payload-transferred', transferred('basis-2')]), + ); expect(held.size).toBe(1); + expect(intent?.healthy).toBe(true); + expect(outcome?.committed).not.toBe(true); + // The intent event above already carried the up-to-date answer; the transfer + // completing it adds nothing to report. + expect(outcome?.healthy).not.toBe(true); + // Nor does it move the resume point. `basis-2` names a payload this store was + // never sent, and resuming from it would ask every later connection for + // changes since a payload it never applied. + expect(outcome?.basis).toBeNull(); + }); + + it('does not report a commit for a transfer that applied nothing', () => { + // Three shapes reach `payload-transferred` with no pending set to apply: an + // intent code this SDK does not recognise, a `none` intent, and a lone + // transfer under no intent at all. None of them applied anything, so none of + // them claims anything — neither a commit, which is what publishes the first + // payload `writeSkills('*')` prunes on, nor an up-to-date answer, which only + // the server can give and only the `none` intent does, on its own event. + // + // The transfer is still a wire fact, counted either way. + const shapes = [ + events( + ['server-intent', serverIntent('xfer-future')], + ['put-object', putSkill()], + ['payload-transferred', transferred('basis-1')], + ), + events(['server-intent', serverIntent('none')], ['payload-transferred', transferred('basis-1')]), + events(['payload-transferred', transferred('basis-1')]), + ]; + for (const shape of shapes) { + const held = new SkillObjectSet(); + const reader = new ProtocolReader(held); + const outcome = drive(reader, shape).at(-1); + expect(outcome?.committed).not.toBe(true); + expect(outcome?.healthy).not.toBe(true); + expect(outcome?.basis).toBeNull(); + expect(held.size).toBe(0); + expect(reader.diagnostics.payloadsTransferred).toBe(1); + } }); it('treats an object arriving with no intent as a delta', () => { @@ -1362,15 +1410,59 @@ describe('polling against the endpoint', () => { expect(store.getObject(SKILL_OBJECT_KIND, 'pdf-extraction')).not.toBeNull(); expect(store.diagnostics.payloadsTransferred).toBe(1); expect(store.failed).toBeNull(); + // A payload arrived, and a 304 does not take that back. + expect(store.isInitialized()).toBe(true); }); - it('releases waitForSkills on a 304 before any payload', async () => { - // A reconnect with a cached basis has nothing to transfer; boot must not - // block on a payload the server has no reason to send. + it('confirms a payload on a 304 but cannot establish one', async () => { + // A 304 answers for content this store already holds, and the etag that asked + // for it is only ever adopted from a body that completed an exchange. + // Reaching one with nothing held therefore takes a server answering a request + // that carried no etag at all, and that 304 says nothing about a payload this + // store never received. + // + // Releasing `waitForSkills` on it would make `isInitialized()` true over an + // empty committed set — the fact `writeSkills('*')` prunes on — so a reconcile + // that raced delivery would delete every managed skill on disk instead of + // reporting the retrieval unavailable (§3.21). Failing closed costs a boot + // that is genuinely waiting nothing it was not already waiting for. endpoint.queuePoll([], { status: 304 }); const store = pollStore(); store.start(); - expect(await store.waitForSkills(5000)).toBe(true); + expect(await store.waitForSkills(500)).toBe(false); + expect(store.isInitialized()).toBe(false); + // Not a failure either: the poll was answered, and the store is still asking. + expect(store.failed).toBeNull(); + expect(endpoint.requests[0].ifNoneMatch).toBeUndefined(); + }); + + it('does not lend the etag of an intent it cannot apply to a 304', async () => { + // The chain this closes: a body under a future intent code announces and + // transfers a payload this SDK cannot apply, its etag is adopted as though the + // body had been applied in full, and the next 304 reports the empty store it + // left behind as current. That store is initialized, healthy, and authorizes a + // prune of every managed skill on disk, with nothing in the 304 to notice it + // on. + // + // An etag is adopted only from a body that completed an exchange, so the + // second request carries none and the endpoint's standing 304 cannot answer + // for content that never arrived. + endpoint.queuePoll( + events( + ['server-intent', serverIntent('xfer-future')], + ['put-object', putSkill()], + ['payload-transferred', transferred('basis-1')], + ), + { etag: 'W/"v1"' }, + ); + const store = pollStore(); + store.start(); + expect(await waitUntil(() => endpoint.requests.length >= 2)).toBe(true); + expect(store.isInitialized()).toBe(false); + expect(store.getObject(SKILL_OBJECT_KIND, 'pdf-extraction')).toBeNull(); + expect(endpoint.requests[1].ifNoneMatch).toBeUndefined(); + // Nor is the selector of a payload it could not apply a resume point. + expect(endpoint.requests.slice(0, 2).map((r) => r.query.basis)).toEqual([undefined, undefined]); }); it('yields only the skill from a mixed payload', async () => { @@ -3337,12 +3429,22 @@ describe('lifecycle', () => { expect(delivering.isInitialized()).toBe(true); }); - it('a 304 counts as initialized — the payload held is confirmed current', async () => { + it('stays initialized across a 304 that confirms the payload held', async () => { + // The 304 the store can actually reach: it offers an etag earned by a body it + // applied in full, and the answer confirms that content is still current. A + // payload arrived, and a 304 does not take that back. + endpoint.queuePoll(fullPayload([['put-object', putSkill()]], 'basis-1'), { etag: 'W/"v1"' }); + // The unconditional poll that follows a commit, answered `none`: a completed + // exchange, so its etag is the one the third request gets to offer. + endpoint.queuePoll(events(['server-intent', serverIntent('none')]), { etag: 'W/"v2"' }); endpoint.queuePoll([], { status: 304 }); const store = pollStore(); store.start(); expect(await store.waitForSkills(5000)).toBe(true); + expect(await waitUntil(() => endpoint.requests.length >= 3)).toBe(true); + expect(endpoint.requests[2].ifNoneMatch).toBe('W/"v2"'); expect(store.isInitialized()).toBe(true); + expect(store.getObject(SKILL_OBJECT_KIND, 'pdf-extraction')).not.toBeNull(); }); it('times out waitForSkills rather than hanging', async () => { diff --git a/packages/client/src/skills-fdv2.ts b/packages/client/src/skills-fdv2.ts index 027f532a..fdd89dff 100644 --- a/packages/client/src/skills-fdv2.ts +++ b/packages/client/src/skills-fdv2.ts @@ -984,6 +984,10 @@ export class ProtocolReader { // and the transfer that completes it still names a payload whose selector // must not become the resume point if it is not the payload skills arrive on. const foreign = this.isForeignPayload(payloadId); + // Whether this transfer moved the committed set. A `none` intent builds no + // pending set, nor does an intent code this SDK does not recognise, and a + // foreign payload's contents are declined below. + const applied = !foreign && this.pending !== null; if (foreign) { this.warnForeignPayload(payloadId); this.diagnostics.payloadsIgnored += 1; @@ -1014,14 +1018,40 @@ export class ProtocolReader { const { changes } = this; this.changes = []; this.diagnostics.payloadsTransferred += 1; + if (!applied) { + // A transfer that applied nothing claims nothing: not a commit, and not an + // up-to-date answer either. + // + // Not a commit, because a commit publishes a first payload, and + // `isInitialized()` is the fact `writeSkills('*')` prunes on. An empty + // committed set reported as a payload reads as an environment whose every + // skill was revoked, which deletes the last known good copy on disk. + // + // Not up to date either, because only the server can say that, and only the + // `none` intent does — on its own event, which has already reported it by + // the time a transfer completing it arrives. An intent code this SDK does + // not recognise says the opposite: the body carried objects this reader + // dropped, so the content held is *not* what that body describes, and an + // etag adopted from it would let a 304 report the store as current for as + // long as the server kept re-announcing it. Claiming nothing is what leaves + // the poll unconditional, so the body keeps arriving and keeps being visible + // — counted under `objectsIgnored`, with the warning that names the intent. + // + // The selector goes the same way: resuming from a payload this store never + // applied would ask every later connection for changes since content it + // does not hold, with every diagnostic reading healthy. + // + // The transfer is still a wire fact: `payloadsTransferred` counts it above + // either way. + // + // `changes` and `basis` are stated rather than left off: Python's defaults + // make them an empty list and null, and this is the same outcome object. + return { changes: [], basis: null }; + } return { committed: true, changes, - // A declined payload must not move the resume point. Adopting the selector - // of a transfer whose contents this layer just threw away would ask the - // next poll or stream to resume from someone else's payload, and skill - // updates could stop arriving while every diagnostic still read healthy. - basis: foreign || typeof state !== 'string' || state === '' ? null : state, + basis: typeof state !== 'string' || state === '' ? null : state, }; } @@ -2241,21 +2271,39 @@ export class FDv2SkillStore implements SkillStore { const etag = this.etagBasis === basis ? this.etag : null; const result = await this.requester.poll(basis, etag, signal); if (result.notModified) { - // A 304 is a successful, current answer: the payload we hold is the payload - // the server has, because the etag that asked for it was issued for a body - // this store applied in full. It counts as a first payload so a boot that - // reconnects with a cached basis is not blocked on a transfer the server - // has no reason to send. - this.markFirstPayload(); + // A 304 *confirms* the payload this store holds. It cannot establish one, + // and it is not a first payload: the exchange it stands in for is the + // `none` intent, which does not publish one either (see `apply`), and a 304 + // carries nothing a store holding nothing could be initialized from. + // Publishing here would make `isInitialized()` true over an empty committed + // set, which is what authorizes `writeSkills('*')` to prune, so a 304 + // answering a request that carried no etag — the only way to reach one with + // nothing held — would delete the last known good copy on disk. `run` + // counts the poll as a healthy answer either way. return; } + let completed = false; for (const [name, data] of result.events) { - this.dispatch(this.apply(name, data)); + const outcome = this.apply(name, data); + completed = completed || outcome.committed === true || outcome.healthy === true; + this.dispatch(outcome); + } + if (!completed) { + // Adopted only from a body that completed an exchange: one that committed a + // payload, or a `none` intent, which is the server saying the content held + // is what the etag describes. A body that threw partway — an `error` or + // `goodbye` after an announced transfer — never reaches here. One that + // merely transferred nothing, under an intent code this SDK does not + // recognise, reaches here having committed nothing: its etag describes a + // body whose contents this store does not hold, and keeping it would let + // the next `304` report a store missing that payload as current and + // healthy. + // + // The etag already held is left alone rather than cleared: it was earned by + // a body that did complete, and it still validates that content for as long + // as the basis it was paired with holds. + return; } - // Adopted only once the whole body has been applied. A body that threw - // partway — an `error` or `goodbye` after an announced transfer — left the - // payload it described unapplied, and keeping its etag would let the next - // `304` report a store that is missing that payload as current and healthy. this.etag = result.etag; this.etagBasis = basis; } From 43bfbc866eb3319a82f86a322de6f634fa39ff87 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Fri, 2 Oct 2026 10:53:20 -0400 Subject: [PATCH 2/3] docs(client): trim the 304/etag comments to what a consumer needs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three blocks the previous commit added were arguing the whole case inline — the prune chain, the forward-compatibility reasoning, the cross-language note — in 50-odd lines of comment on a 14-line change. This source gets read by customers and their agents working out how to consume the SDK, and that much nuance about internals they cannot reach is a wall to read past rather than help. Each block now states the rule and the one consequence that is visible from outside: `is_initialized()` / `isInitialized()` goes true on a commit, and that is what authorizes a prune of the files on disk. The long version already has two homes it belongs in — `agents.md` in this package, and TESTING.md §3.25 — so nothing is lost, and the clause that stops a plausible wrong fix ("not up to date either: only the `none` intent says that") is kept. Comments only; no behaviour change. Tests untouched, where the long-form reasoning is the point. Co-Authored-By: Claude Opus 5 --- packages/client/src/skills-fdv2.ts | 69 ++++++++---------------------- 1 file changed, 17 insertions(+), 52 deletions(-) diff --git a/packages/client/src/skills-fdv2.ts b/packages/client/src/skills-fdv2.ts index fdd89dff..49a613a6 100644 --- a/packages/client/src/skills-fdv2.ts +++ b/packages/client/src/skills-fdv2.ts @@ -984,9 +984,8 @@ export class ProtocolReader { // and the transfer that completes it still names a payload whose selector // must not become the resume point if it is not the payload skills arrive on. const foreign = this.isForeignPayload(payloadId); - // Whether this transfer moved the committed set. A `none` intent builds no - // pending set, nor does an intent code this SDK does not recognise, and a - // foreign payload's contents are declined below. + // A `none` intent builds no pending set, nor does an intent code this SDK + // does not recognise, and a foreign payload's contents are declined below. const applied = !foreign && this.pending !== null; if (foreign) { this.warnForeignPayload(payloadId); @@ -1019,33 +1018,12 @@ export class ProtocolReader { this.changes = []; this.diagnostics.payloadsTransferred += 1; if (!applied) { - // A transfer that applied nothing claims nothing: not a commit, and not an - // up-to-date answer either. - // - // Not a commit, because a commit publishes a first payload, and - // `isInitialized()` is the fact `writeSkills('*')` prunes on. An empty - // committed set reported as a payload reads as an environment whose every - // skill was revoked, which deletes the last known good copy on disk. - // - // Not up to date either, because only the server can say that, and only the - // `none` intent does — on its own event, which has already reported it by - // the time a transfer completing it arrives. An intent code this SDK does - // not recognise says the opposite: the body carried objects this reader - // dropped, so the content held is *not* what that body describes, and an - // etag adopted from it would let a 304 report the store as current for as - // long as the server kept re-announcing it. Claiming nothing is what leaves - // the poll unconditional, so the body keeps arriving and keeps being visible - // — counted under `objectsIgnored`, with the warning that names the intent. - // - // The selector goes the same way: resuming from a payload this store never - // applied would ask every later connection for changes since content it - // does not hold, with every diagnostic reading healthy. - // - // The transfer is still a wire fact: `payloadsTransferred` counts it above - // either way. - // - // `changes` and `basis` are stated rather than left off: Python's defaults - // make them an empty list and null, and this is the same outcome object. + // Nothing applied, so nothing to report — and in particular not a commit, + // because a commit publishes the first payload, and `isInitialized()` (what + // `writeSkills('*')` authorizes a prune on) must not go true over a store + // that received nothing. Not up to date either: only the `none` intent says + // that, on its own event. The selector is withheld too — it names a payload + // never applied. return { changes: [], basis: null }; } return { @@ -2271,15 +2249,10 @@ export class FDv2SkillStore implements SkillStore { const etag = this.etagBasis === basis ? this.etag : null; const result = await this.requester.poll(basis, etag, signal); if (result.notModified) { - // A 304 *confirms* the payload this store holds. It cannot establish one, - // and it is not a first payload: the exchange it stands in for is the - // `none` intent, which does not publish one either (see `apply`), and a 304 - // carries nothing a store holding nothing could be initialized from. - // Publishing here would make `isInitialized()` true over an empty committed - // set, which is what authorizes `writeSkills('*')` to prune, so a 304 - // answering a request that carried no etag — the only way to reach one with - // nothing held — would delete the last known good copy on disk. `run` - // counts the poll as a healthy answer either way. + // A 304 confirms the payload this store holds; it cannot establish one. + // `isInitialized()` stays false until something commits, so a store that + // has received nothing never authorizes a prune of the files on disk. `run` + // counts the poll as a healthy answer. return; } let completed = false; @@ -2289,19 +2262,11 @@ export class FDv2SkillStore implements SkillStore { this.dispatch(outcome); } if (!completed) { - // Adopted only from a body that completed an exchange: one that committed a - // payload, or a `none` intent, which is the server saying the content held - // is what the etag describes. A body that threw partway — an `error` or - // `goodbye` after an announced transfer — never reaches here. One that - // merely transferred nothing, under an intent code this SDK does not - // recognise, reaches here having committed nothing: its etag describes a - // body whose contents this store does not hold, and keeping it would let - // the next `304` report a store missing that payload as current and - // healthy. - // - // The etag already held is left alone rather than cleared: it was earned by - // a body that did complete, and it still validates that content for as long - // as the basis it was paired with holds. + // An etag describes the body it came with, so it is adopted only when that + // body is also what the store now holds: a commit, or a `none` intent. A + // transfer this SDK could not apply is neither, and keeping its etag would + // let the next 304 confirm content never applied. Any etag already held + // stays valid, so it is left alone, not cleared. return; } this.etag = result.etag; From 44a3cf37d6cc06d679c60c02556e9dbc1f93bd41 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Mon, 5 Oct 2026 14:52:25 -0400 Subject: [PATCH 3/3] test(client): assert a declined foreign payload is not a commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TESTING.md §3.25 asks for all four shapes that reach payload-transferred with nothing to apply. The loop covered three; the declined foreign payload was missing, so dropping `!foreign` from `applied` still passed the suite, because the foreign tests assert only the basis. Add it, priming the reader with a skill commit so the payload is recognisably foreign, and assert the committed set is unchanged rather than empty. Co-Authored-By: Claude Opus 5.5 --- .../client/src/__tests__/skills-fdv2.test.ts | 54 +++++++++++++------ 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/packages/client/src/__tests__/skills-fdv2.test.ts b/packages/client/src/__tests__/skills-fdv2.test.ts index 22c5e2c9..51045b31 100644 --- a/packages/client/src/__tests__/skills-fdv2.test.ts +++ b/packages/client/src/__tests__/skills-fdv2.test.ts @@ -871,32 +871,52 @@ describe('protocol reader', () => { }); it('does not report a commit for a transfer that applied nothing', () => { - // Three shapes reach `payload-transferred` with no pending set to apply: an - // intent code this SDK does not recognise, a `none` intent, and a lone - // transfer under no intent at all. None of them applied anything, so none of - // them claims anything — neither a commit, which is what publishes the first - // payload `writeSkills('*')` prunes on, nor an up-to-date answer, which only - // the server can give and only the `none` intent does, on its own event. + // Four shapes reach `payload-transferred` with nothing to apply: an intent + // code this SDK does not recognise, a `none` intent, a lone transfer under no + // intent at all, and a foreign payload, which built a pending set and threw + // it away. None of them applied anything, so none of them claims anything — + // neither a commit, which is what publishes the first payload + // `writeSkills('*')` prunes on, nor an up-to-date answer, which only the + // server can give and only the `none` intent does, on its own event. // // The transfer is still a wire fact, counted either way. - const shapes = [ - events( - ['server-intent', serverIntent('xfer-future')], - ['put-object', putSkill()], - ['payload-transferred', transferred('basis-1')], - ), - events(['server-intent', serverIntent('none')], ['payload-transferred', transferred('basis-1')]), - events(['payload-transferred', transferred('basis-1')]), + const shapes: Array<{ prior: WireEvent[]; shape: WireEvent[] }> = [ + { + prior: [], + shape: events( + ['server-intent', serverIntent('xfer-future')], + ['put-object', putSkill()], + ['payload-transferred', transferred('basis-1')], + ), + }, + { + prior: [], + shape: events(['server-intent', serverIntent('none')], ['payload-transferred', transferred('basis-1')]), + }, + { prior: [], shape: events(['payload-transferred', transferred('basis-1')]) }, + { + // A payload is foreign only once the skill payload is known, which takes + // a commit first. + prior: fullPayload([['put-object', putSkill()]], 'basis-skills'), + shape: events( + ['server-intent', serverIntent('xfer-full', 'env-flags')], + ['put-object', putSkill('other')], + ['payload-transferred', transferred('basis-flags')], + ), + }, ]; - for (const shape of shapes) { + for (const { prior, shape } of shapes) { const held = new SkillObjectSet(); const reader = new ProtocolReader(held); + drive(reader, prior); + const before = held.allRaw(); + const transfers = reader.diagnostics.payloadsTransferred; const outcome = drive(reader, shape).at(-1); expect(outcome?.committed).not.toBe(true); expect(outcome?.healthy).not.toBe(true); expect(outcome?.basis).toBeNull(); - expect(held.size).toBe(0); - expect(reader.diagnostics.payloadsTransferred).toBe(1); + expect(held.allRaw()).toEqual(before); + expect(reader.diagnostics.payloadsTransferred).toBe(transfers + 1); } });