mirror of
https://github.com/dtzp555-max/ocp.git
synced 2026-07-21 21:15:09 +00:00
docs: fold in re-review — the two caveats that would have bitten the implementer
Re-review of the reversed doc came back APPROVE_WITH_MINOR. The reversal itself was verified complete (worktree-wide grep: no surviving impossibility claim) and NOT over-claimed in the other direction (the reviewer recomputed every headline number from the committed messagedisplay-deltas.jsonl and re-ran the invariant on two further turns: 4 independent turns total, 5/6/4/18 fires, 609/696/239/1973 bytes, concat(deltas) === T TRUE in all four). But two additive caveats were missing, and both are load-bearing for the streaming PR now in flight: 1. CONCURRENCY DEMUX (severe, and live TODAY — not a warm-pool future problem). OCP_TUI_MAX_CONCURRENT defaults to 2, so two `claude` processes already run concurrently. One MessageDisplay hook writing to one shared sink would INTERLEAVE deltas from two different turns into a single stream — request A's client receiving request B's text. A single-request test never surfaces it. The payload carries session_id, so the sink must be keyed by it (which also keeps the design warm-pool compatible: a pre-booted pane's session-id is fixed at boot, so one static hook script serves every pane). Documented, with the required ≥2-concurrent-request test. 2. THINKING-EXCLUSION IS NOT STRESS-TESTED (severe if wrong). The exclusion was inferred from a code snippet that turns out to be the final:true call site, not the incremental one. Four live turns showed no thinking in any delta — but every transcript's thinking block was EMPTY (thinking:"", 0 chars), so it was never actually stressed. If thinking deltas do fire on Opus/xhigh, concat(deltas) !== T AND OCP streams the model's private reasoning to the caller; the concat === T assertion detects that but cannot un-send an SSE delta. Flagged as a must-verify-before-shipping item. Also: the "5-7 chunks per answer" figure is size-dependent (18 fires on a ~2 KB answer) — rescoped to "once per rendered block, scales with answer length" in both the doc and the README, so no implementer hard-codes a chunk-count assumption. Both caveats were relayed to the streaming implementation immediately rather than waiting for this merge. Docs-only. No code change, no version bump. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VqgWJcjxrjjL9L9SkpZyXR
This commit is contained in:
@@ -71,13 +71,32 @@ This is exactly the contract a streaming design needs: deltas forward straight i
|
||||
|
||||
### Caveats for the implementer
|
||||
|
||||
- **Block-level granularity, not token-level** — 5–7 chunks for a ~600-byte answer, not one per token.
|
||||
Plenty for SSE (`delta.content` has no minimum size), but do not promise token-by-token output.
|
||||
- **Block-level granularity, not token-level** — the hook fires **once per rendered block** (roughly one
|
||||
per paragraph / list item / code block), so the chunk count **scales with answer length**: 7 fires for a
|
||||
~600-byte answer, **18 for a ~2 KB one**. Plenty for SSE (`delta.content` has no minimum size), but do
|
||||
not promise token-by-token output, and do not hard-code any assumption about chunk count.
|
||||
- **🔴 The sink MUST be keyed by `session_id` — this is live TODAY, not a future concern.**
|
||||
`OCP_TUI_MAX_CONCURRENT` defaults to **2**, so **two `claude` processes already run concurrently**. One
|
||||
hook command writing to one shared sink would **interleave deltas from two different turns into one
|
||||
stream** — request A's client receiving request B's text, the worst failure a proxy can have, and one a
|
||||
single-request test will never surface. The payload carries `session_id` (and `turn_id` / `message_id`),
|
||||
so demux is easy: derive the sink path from `session_id` (`<dir>/<session_id>.jsonl`) and read only your
|
||||
own turn's file. This *also* keeps the design **warm-pool compatible**, because a pre-booted pane's
|
||||
session-id is fixed at boot — one static hook script serves every pane. **Test it with ≥2 concurrent
|
||||
streaming requests carrying distinguishable prompts and assert zero cross-contamination.**
|
||||
- **⚠️ `forceSyncExecution: true` in the hook's source — `claude` BLOCKS on the hook.** A slow hook
|
||||
adds latency to *every* delta. The hook must write and exit immediately (e.g. write to a FIFO / unix
|
||||
socket that OCP reads; never work inline). **Measure the added per-delta latency.**
|
||||
- Only `text` blocks fire it (`content.map(c => c.type === "text" ? c.text : "")`) — **thinking blocks
|
||||
are excluded**, which is what OCP wants.
|
||||
- **Thinking blocks appear to be excluded — but this is NOT yet stress-tested. Verify before shipping.**
|
||||
The exclusion is inferred from `content.map(c => c.type === "text" ? c.text : "")` — but that snippet is
|
||||
from the **`final:true`** call site, not the incremental one. Four live turns (incl. two at `--effort
|
||||
high`) showed no thinking text in any delta and `concat === T` held — **but each transcript's thinking
|
||||
block was empty (`thinking:""`, 0 chars)**, so the exclusion was never actually stressed. **The failure
|
||||
mode is severe**: if thinking deltas *do* fire on some config (Opus, `xhigh`), `concat(deltas) !== T`
|
||||
**and OCP streams the model's private reasoning to the caller**. The end-of-turn `concat === T` assertion
|
||||
would *detect* that but **cannot prevent** it — SSE deltas cannot be un-sent. **Before shipping, run a
|
||||
turn on a model+effort that produces substantive thinking** (a hard reasoning prompt on Opus / `xhigh`)
|
||||
and confirm both (a) no thinking text in any delta and (b) `concat === T` still holds.
|
||||
- OCP already owns the spawn (isolated HOME, its own flags), so injecting `--settings` with a
|
||||
`MessageDisplay` hook sits inside the existing architecture.
|
||||
- **`ALIGNMENT.md`**: this consumes `claude`'s **own** hook surface as emitted — forwarding, not
|
||||
|
||||
Reference in New Issue
Block a user