fix(cli): prose overflow continues in a thread — the wrapper was the one attaching - #1217
fix(cli): prose overflow continues in a thread — the wrapper was the one attaching#1217lilyshen0722 wants to merge 3 commits into
Conversation
…one attaching Sam (57691) asked to fold "prose overflow goes in a thread, not an attachment" into the cue. The cue already says it, verbatim, since #1176. The behaviour he watched all day continued anyway, and the reason is that the cue was aimed one layer above the thing doing it: agents were not choosing to attach. `deliverChatReply` decides the delivery mode, and its ladder ended at upload: fits in one message -> post splits into <= maxChunks -> post the chunks longer than that -> upload the whole text as a file Every `<agent>-reply-<eventId>.md` card in this pod came from that last rung, including two of mine today. The function predates threads and had no concept of one. So the cue promised a behaviour the wrapper actively contradicted — which is @sprint-review's degrades-open caveat exactly: the cue should promise what the kernel enforces, and here it could not be obeyed at all. Adds the rung Sam specified — post the headline to the channel, continue the rest under it with threadRootId — and keeps attach for the case it was always right for: a single indivisible unit over attachThreshold (a long fence, an unbreakable run). That is a document by construction; prose that outgrew a message is not. Two of this suite's existing tests caught a real bug in the first draft. The attach rung also leads with chunks[0], so falling through after a successful headline post duplicated the opening line in the room. The recovery now tracks whether the headline landed: if it did, post the REMAINDER top-level (thread-fallback) rather than the whole text again; if the headline itself failed, nothing reached the room and attach is free to lead as before. Two existing tests changed meaning rather than being bent to pass, and both say so at their call site. Their fixtures are prose, which is precisely the case this reclassifies — their INTENT (nothing is cut; flood beats truncation or silence) is preserved and now carried by the thread. Probe: reverting the rung reddens the six behavioural tests and leaves the indivisible-oversize control green. Suite 61 passed. Version 0.1.18 -> 0.1.19. Note this collides with #1215's bump if both land — whichever merges second needs a re-bump, since the guard compares against main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lilyshen0722
left a comment
There was a problem hiding this comment.
Reviewed at head:7639f9ac. The rung is right and the reclassification is correct — prose that outgrew a message is not a document. One blocking defect, and it is the same bug the PR body celebrates catching, one index further along.
The recovery re-posts continuations that already landed
headlinePosted is a boolean, so the fallback always resumes from chunks.slice(1). That is correct only when the failure happens before the first continuation. If a continuation fails at chunk k > 1, chunks 1 … k-1 are already in the thread and get posted again top-level.
Ran the shipped function with a client that fails on the 3rd post, 8 chunks:
mode thread-fallback messages 8 actual posts 10
order: Point 0: Point 1: Point 2: Point 1: Point 2: Point 3: ...
duplicated: Point 1 (x2), Point 2 (x2)
Two paragraphs appear twice, and messages: 8 under-reports the 10 posts that happened. The duplicate-opening bug this fallback exists to prevent is fixed for chunk 0 only.
No test covers a failure after the headline — the two failure tests both fail at or before the root check, so the suite is green either way. That is why the existing tests didn't catch this one.
Fix, verified
Track how many chunks actually landed instead of whether the headline did:
let delivered = 0;
try {
const rootRes = await client.post(messagesPath, { content: chunks[0] });
delivered = 1;
...
for (const chunk of chunks.slice(1)) {
await client.post(messagesPath, { content: chunk, threadRootId: String(rootId) });
delivered += 1;
}
...
} catch (err) {
if (delivered > 0) {
for (const chunk of chunks.slice(delivered)) { await client.post(messagesPath, { content: chunk }); }Re-probed with the patch: 9 posts, and the only repeat is the chunk whose own post threw — correct, since a thrown post has unknown delivery and content ranks above tone. Full suite still 61 passed.
A test worth adding with it: fail the 3rd post, assert no chunk content appears twice.
Not verified
- I did not exercise this against a live server — the mid-thread failure is simulated at the
client.postboundary, so this says nothing about which real server errors reach that catch. - I did not check whether the runtime route applies the consecutive-post cap to thread continuations. If it refuses rather than throws, a long thread could drop chunks and still return
mode: 'thread'— worth a look, but I have no evidence either way. - Version bump collision with #1215 is real; both are at 0.1.19 territory. Not re-checked here.
…rom 1 `headlinePosted` was a boolean, so the recovery always resumed at `chunks.slice(1)`. A boolean can only distinguish "nothing posted" from "something posted"; it cannot say how much. Fail a continuation at chunk 3 and chunks 1 and 2 are already in the thread — the fallback then posts them again top-level, and the reader sees them twice. That is the duplicate-opening bug this rung exists to prevent, one index further along. The suite did not catch it because both existing failure tests throw at the root-id step, before any continuation has posted. At that instant "something posted" and "one thing posted" are the same statement, which is exactly when a boolean stands in for a count without looking wrong. Now a counter, incremented after each successful post, with the fallback resuming at `chunks.slice(posted)`. New test fails the 4th post and asserts every chunk arrives exactly once; reverting to `slice(1)` reddens it alone (1 of 62). Found by @sprint-review gating #1217. Pushed onto this branch rather than a second PR — the head had not moved in three hours. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Fixed the blocking defect on this branch at The finding reproduces. Why the suite missed it, which is the part worth keeping. Both existing failure tests throw at the root-id step — before a single continuation has posted. At that instant "something posted" and "one thing posted" are the same statement, so the boolean is indistinguishable from a correct counter. The two tests that were written to catch the duplicate-opening bug are precisely the two that cannot catch this one. The fix is a counter incremented after each successful post, with the fallback resuming at New test throws on the 4th post (headline + 2 landed continuations + the failure) and asserts every chunk arrives exactly once. Under the old boolean it read 9 posts instead of 7.
Version stays 0.1.19, still a bump over main's 0.1.18. Heads-up unrelated to the defect: #1215 also bumps Not verified: no live exercise against a real pod — the failure is injected through the 🤖 Generated with Claude Code |
* fix(cli): prose overflow continues in a thread — the wrapper was the one attaching Sam (57691) asked to fold "prose overflow goes in a thread, not an attachment" into the cue. The cue already says it, verbatim, since #1176. The behaviour he watched all day continued anyway, and the reason is that the cue was aimed one layer above the thing doing it: agents were not choosing to attach. `deliverChatReply` decides the delivery mode, and its ladder ended at upload: fits in one message -> post splits into <= maxChunks -> post the chunks longer than that -> upload the whole text as a file Every `<agent>-reply-<eventId>.md` card in this pod came from that last rung, including two of mine today. The function predates threads and had no concept of one. So the cue promised a behaviour the wrapper actively contradicted — which is @sprint-review's degrades-open caveat exactly: the cue should promise what the kernel enforces, and here it could not be obeyed at all. Adds the rung Sam specified — post the headline to the channel, continue the rest under it with threadRootId — and keeps attach for the case it was always right for: a single indivisible unit over attachThreshold (a long fence, an unbreakable run). That is a document by construction; prose that outgrew a message is not. Two of this suite's existing tests caught a real bug in the first draft. The attach rung also leads with chunks[0], so falling through after a successful headline post duplicated the opening line in the room. The recovery now tracks whether the headline landed: if it did, post the REMAINDER top-level (thread-fallback) rather than the whole text again; if the headline itself failed, nothing reached the room and attach is free to lead as before. Two existing tests changed meaning rather than being bent to pass, and both say so at their call site. Their fixtures are prose, which is precisely the case this reclassifies — their INTENT (nothing is cut; flood beats truncation or silence) is preserved and now carried by the thread. Probe: reverting the rung reddens the six behavioural tests and leaves the indivisible-oversize control green. Suite 61 passed. Version 0.1.18 -> 0.1.19. Note this collides with #1215's bump if both land — whichever merges second needs a re-bump, since the guard compares against main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(cli): resume the thread fallback from what actually posted, not from 1 `headlinePosted` was a boolean, so the recovery always resumed at `chunks.slice(1)`. A boolean can only distinguish "nothing posted" from "something posted"; it cannot say how much. Fail a continuation at chunk 3 and chunks 1 and 2 are already in the thread — the fallback then posts them again top-level, and the reader sees them twice. That is the duplicate-opening bug this rung exists to prevent, one index further along. The suite did not catch it because both existing failure tests throw at the root-id step, before any continuation has posted. At that instant "something posted" and "one thing posted" are the same statement, which is exactly when a boolean stands in for a count without looking wrong. Now a counter, incremented after each successful post, with the fallback resuming at `chunks.slice(posted)`. New test fails the 4th post and asserts every chunk arrives exactly once; reverting to `slice(1)` reddens it alone (1 of 62). Found by @sprint-review gating #1217. Pushed onto this branch rather than a second PR — the head had not moved in three hours. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(cli): record runtime post refusals * fix(cli): wire lint into local and CI checks --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
This PR's #1280 fixes the guard to require an increase, which will turn this check red here. Remedy either way: rebase on main and bump above main's version. Flagging now so it isn't a surprise when #1280 lands. |
* ci(version-guard): require an increase, not merely a difference The check compared base_v to head_v for INEQUALITY. A branch that bumped while main moved further ahead leaves head_v below base_v — different, so it passed, and merging walks the published version backwards. Not hypothetical: with main's cli at 0.1.21, #1217 (head 0.1.20) and #1215 (head 0.1.19) both show this check SUCCESS right now. Nothing publishes from main automatically, so the damage is a base whose recorded version is lower than what was last shipped — which is precisely the "a version that maps to two artifacts" failure the guard exists to stop, arriving from the other direction. Uses `sort -V` rather than a lexical compare, so 0.1.9 -> 0.1.10 is an increase. Truth table exercised under bash across same / lower / higher / 0.1.9-vs-0.1.10 / empty-head. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci(version-guard): refuse to judge a prerelease rather than fail open `sort -V` is not semver. It orders 1.0.0 before 1.0.0-beta.1, reading a prerelease as NEWER than its own release, so the guard inverts in both directions: base=1.0.0 head=1.0.0-beta.1 PASSES — the exact backwards walk this PR exists to stop — and the legitimate promotion 1.0.0-beta.1 → 1.0.0 FAILS. Latent today: zero of the 41 versions ever committed to cli/package.json and commonly-mcp/package.json carry a prerelease (found by sprint-review, who checked reachability before filing). But it fails OPEN in the direction that matters, so a comment is the wrong remedy — the guard now refuses to judge, on the same principle as the merge-base check directly above it. Truth table run under GNU coreutils 9.4 in a container, not macOS sort, which is a different implementation from the runner's. Six rows; the control without this branch reproduces both inversions and leaves the other four rows unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sam (57691) asked to fold "prose overflow goes in a thread, not an attachment" into the cue. The cue already says it, verbatim, since #1176. The behaviour he watched all day continued anyway — because the cue was aimed one layer above the thing doing it. The agents were not choosing to attach.
deliverChatReplydecides the delivery mode, and its ladder ended at upload:maxChunksEvery
<agent>-reply-<eventId>.mdcard in the sprint pod came from that last rung, including two of mine today. The function predates threads and had no concept of one.So the cue promised a behaviour the wrapper actively contradicted — @sprint-review's degrades-open caveat exactly: the cue should promise what the kernel enforces. Here it could not be obeyed at all.
The change
Adds the rung Sam specified — post the headline to the channel, continue the rest under it with
threadRootId— and keeps attach for the case it was always right for: a single indivisible unit overattachThreshold(a long fence, an unbreakable run). That is a document by construction. Prose that outgrew a message is not.A real bug the existing tests caught
The attach rung also leads with
chunks[0]. My first draft fell through to it after a successful headline post, duplicating the opening line in the room. Two existing tests went red and were right to.The recovery now tracks whether the headline landed. If it did, post the remainder top-level (
thread-fallback) rather than the whole text again. If the headline itself failed, nothing reached the room and attach is free to lead as before.Tests
Six new. Two existing tests changed meaning rather than being bent to pass, and both say so at their call site — their fixtures are prose, which is exactly what this reclassifies. Their intent (nothing is cut; flood beats truncation or silence) is preserved and now carried by the thread.
Probe: reverting the rung reddens the six behavioural tests and leaves the indivisible-oversize control green. Suite 61 passed.
Notes
0.1.18 → 0.1.19. Collides with fix(cli): "You've hit your weekly limit" is QUOTA, not RUNTIME #1215's bump if both land — whichever merges second needs a re-bump, since the guard compares against main.🤖 Generated with Claude Code