fix(client): a 304 confirms a payload rather than establishing one

packages/client/agents.md

@@ -76,6 +76,8 @@ The `3` after the delimiter is what a `{key, version}` reference pins; it become
 
 **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 briefly empty the store, which with pruning on deletes skill files. An interrupted transfer keeps last known good. Listeners fire once per changed object, all at `payload-transferred` — which is why `watchSkills` debounces.
 
+**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 that no object followed 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 departures into `changes` as tombstones per `(key, version)`: a listener that reads versions needs both halves of a version move. The counter, which operators alert on, counts only a key the payload dropped altogether; a key surviving under a new version was not revoked. `keysFullyRevoked` is where the two granularities part.
 
 **The first payload intent is read, and assumed to be the skill payload.** The `kinds` declaration narrows the connection to one payload and the protocol says to ignore all but the first intent, so `payloads[0]` is read. The risk: an `xfer-full` for *another* payload would start an empty pending set, and the next `payload-transferred` would publish it — every skill revoked, and with pruning on, files deleted.

packages/client/src/__tests__/skills-fdv2.test.ts

@@ -844,12 +844,80 @@ 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', () => {
+    // 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: 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 { 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.allRaw()).toEqual(before);
+      expect(reader.diagnostics.payloadsTransferred).toBe(transfers + 1);
+    }
   });
 
   it('applies a put that follows a none intent on the same connection (§3.25)', () => {
@@ -1408,15 +1476,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 () => {
@@ -3699,12 +3811,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 () => {

packages/client/src/skills-fdv2.ts

@@ -795,6 +795,10 @@ export class ProtocolReader {
     // Checked even with no pending set (a `none` intent), so a foreign payload's
     // selector never becomes the resume point.
     const foreign = this.isForeignPayload(payloadId);
+    // Nothing is applied without a pending set — a `none` intent no object
+    // followed, or 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;
@@ -821,12 +825,19 @@ export class ProtocolReader {
     const { changes } = this;
     this.changes = [];
     this.diagnostics.payloadsTransferred += 1;
+    if (!applied) {
+      // 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 {
       committed: true,
       changes,
-      // A declined payload must not move the resume point, or skill updates
-      // could silently stop arriving.
-      basis: foreign || typeof state !== 'string' || state === '' ? null : state,
+      basis: typeof state !== 'string' || state === '' ? null : state,
     };
   }
 
@@ -1886,16 +1897,26 @@ 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 counts as a first payload: the etag belongs to a body this store
-      // applied in full.
-      this.markFirstPayload();
+      // 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;
     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) {
+      // 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;
     }
-    // Adopted only after the whole body applied, so a body that broke off
-    // partway cannot earn a later 304.
     this.etag = result.etag;
     this.etagBasis = basis;
   }