AI Workspace: the failure and replay stories need policies, not patches
A multi-reviewer code review of #1004 surfaced fifteen findings. Twelve were mechanical and are fixed on that branch. Three are not: each one is a place where the workspace behaves defensibly but has never been given a policy, and the right answer is a product decision rather than a code change a reviewer can name.
They are filed together because they share that shape — what should happen when a turn fails partway, and what should happen when the same work is submitted twice.
1. A mid-stream failure silently discards what the user already read, and pays twice
includes/Experiments/AI_Workspace/Prompt_Model_Client.php
When streaming fails after the first delta — a truncated stream, a provider error event, a dropped connection — the driver returns null and the client falls through to generate_buffered(). Two things follow that nobody chose:
- Text already streamed into the transcript is discarded and replaced by an independently regenerated answer. The user watched a reply appear and then saw a different one, with no signal that anything was thrown away.
- The round is billed twice, because the buffered call is a second paid request.
The pre-first-byte cases (connector not approved, transport refused, no model) should fall through — nothing was shown and nothing was spent. The post-first-delta case is different in kind and is currently handled identically.
The decision: after the user has seen partial output, is the honest behavior to fail the round visibly, or to regenerate and say so? The reviewer proposed tracking whether on_text was ever called and returning a WP_Error when it was. That is implementable either way; what it needs is a decision about which the product wants.
2. Concurrent confirmations defeat per-item write idempotency
includes/Experiments/AI_Workspace/Draft_Writer.php
Idempotency is enforced by a per-item meta token: look for an existing post carrying the token, and skip if found. The lookup and the wp_insert_post() that follows are not serialized, so two overlapping execute requests for the same proposal can both miss and both write.
The meta token is the right second line of defence. What is missing is a first line: the proposal record itself is not single-use. A robust fix claims it atomically — refuse when the stored status is not pending, and mark it claimed before the writer runs.
The decision: this is a state-machine change to Proposal_Store, not a patch. It needs a view on what a claimed-but-failed proposal should do — is it retryable, does it expire, does the person see it as spent? Those answers determine the implementation.
(The narrower sibling — the token lookup using post_status => 'any', which excludes trash, so replaying after deleting the draft recreates it — was mechanical and is already fixed on #1004.)
3. Conversation history grows without bound, including full post bodies
includes/Experiments/AI_Workspace/Turn_Runner.php
Every round's tool results are appended to the history and resent verbatim on every subsequent round. Since ai/read-content-bodies returns full post bodies with no per-post length cap, a conversation that reads several long posts carries them in every later request. That same history is persisted to a user-scoped transient for two hours and rehydrated on the next turn.
Nothing truncates, summarizes, or caps it. Token cost, latency, and transient size all compound monotonically over a conversation's life, and a sufficiently long conversation will eventually fail against the model's context window rather than degrade.
The decision: which bound? Truncating the oldest rounds, capping message count, summarizing superseded tool results, or capping body length at read time are all defensible and they trade off differently against the assistant's ability to refer back to something it read earlier. That trade is a product call.
Why not just fix them
Each of these has an obvious-looking patch that would close the finding and leave the real question unanswered — regenerate silently but log it, add a mutex, truncate at some arbitrary N. Picking those defaults inside a feature PR would bury three product decisions in a diff about something else.
Out of scope
Provenance
Found by a multi-reviewer code review of #1004 (correctness, security, adversarial, reliability, performance, testing, maintainability, agent-native, api-contract, frontend-races, previous-comments), with each finding independently validated against the cited code before being reported. Findings 1 and 2 here were each raised by two reviewers working in separate contexts.
AI assistance: Yes. Tool(s): Claude Code (Opus 5). Used for: the code review that produced these findings, and this write-up.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BRSJEyQyYp3XTeum8fiq3L
AI Workspace: the failure and replay stories need policies, not patches
A multi-reviewer code review of #1004 surfaced fifteen findings. Twelve were mechanical and are fixed on that branch. Three are not: each one is a place where the workspace behaves defensibly but has never been given a policy, and the right answer is a product decision rather than a code change a reviewer can name.
They are filed together because they share that shape — what should happen when a turn fails partway, and what should happen when the same work is submitted twice.
1. A mid-stream failure silently discards what the user already read, and pays twice
includes/Experiments/AI_Workspace/Prompt_Model_Client.phpWhen streaming fails after the first delta — a truncated stream, a provider error event, a dropped connection — the driver returns
nulland the client falls through togenerate_buffered(). Two things follow that nobody chose:The pre-first-byte cases (connector not approved, transport refused, no model) should fall through — nothing was shown and nothing was spent. The post-first-delta case is different in kind and is currently handled identically.
The decision: after the user has seen partial output, is the honest behavior to fail the round visibly, or to regenerate and say so? The reviewer proposed tracking whether
on_textwas ever called and returning aWP_Errorwhen it was. That is implementable either way; what it needs is a decision about which the product wants.2. Concurrent confirmations defeat per-item write idempotency
includes/Experiments/AI_Workspace/Draft_Writer.phpIdempotency is enforced by a per-item meta token: look for an existing post carrying the token, and skip if found. The lookup and the
wp_insert_post()that follows are not serialized, so two overlapping execute requests for the same proposal can both miss and both write.The meta token is the right second line of defence. What is missing is a first line: the proposal record itself is not single-use. A robust fix claims it atomically — refuse when the stored status is not
pending, and mark it claimed before the writer runs.The decision: this is a state-machine change to
Proposal_Store, not a patch. It needs a view on what a claimed-but-failed proposal should do — is it retryable, does it expire, does the person see it as spent? Those answers determine the implementation.(The narrower sibling — the token lookup using
post_status => 'any', which excludestrash, so replaying after deleting the draft recreates it — was mechanical and is already fixed on #1004.)3. Conversation history grows without bound, including full post bodies
includes/Experiments/AI_Workspace/Turn_Runner.phpEvery round's tool results are appended to the history and resent verbatim on every subsequent round. Since
ai/read-content-bodiesreturns full post bodies with no per-post length cap, a conversation that reads several long posts carries them in every later request. That same history is persisted to a user-scoped transient for two hours and rehydrated on the next turn.Nothing truncates, summarizes, or caps it. Token cost, latency, and transient size all compound monotonically over a conversation's life, and a sufficiently long conversation will eventually fail against the model's context window rather than degrade.
The decision: which bound? Truncating the oldest rounds, capping message count, summarizing superseded tool results, or capping body length at read time are all defensible and they trade off differently against the assistant's ability to refer back to something it read earlier. That trade is a product call.
Why not just fix them
Each of these has an obvious-looking patch that would close the finding and leave the real question unanswered — regenerate silently but log it, add a mutex, truncate at some arbitrary N. Picking those defaults inside a feature PR would bury three product decisions in a diff about something else.
Out of scope
Provenance
Found by a multi-reviewer code review of #1004 (correctness, security, adversarial, reliability, performance, testing, maintainability, agent-native, api-contract, frontend-races, previous-comments), with each finding independently validated against the cited code before being reported. Findings 1 and 2 here were each raised by two reviewers working in separate contexts.
AI assistance: Yes. Tool(s): Claude Code (Opus 5). Used for: the code review that produced these findings, and this write-up.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BRSJEyQyYp3XTeum8fiq3L