From 551d4e7db6c627494a93237647bfd8e2a6bfdf63 Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Mon, 13 Jul 2026 16:30:14 +1000 Subject: [PATCH] =?UTF-8?q?docs:=20fold=20in=20re-review=20=E2=80=94=20the?= =?UTF-8?q?=20two=20caveats=20that=20would=20have=20bitten=20the=20impleme?= =?UTF-8?q?nter?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01VqgWJcjxrjjL9L9SkpZyXR --- README.md | 2 +- .../2026-07-13-tui-latency/streaming-spike.md | 27 ++++++++++++++++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index e6f6119..d182d78 100644 --- a/README.md +++ b/README.md @@ -1033,7 +1033,7 @@ Then restart OCP. At boot you will see (with the env token set, isolated home au ### What changes / what doesn't - **Callers see no API change.** The response is a normal OpenAI completion object or chunked SSE — identical wire format. -- **No real token streaming *today* — but it is achievable, and planned.** TUI-mode currently buffers the full response then replays it as chunked SSE: you see a delay, then the complete response. This is a limitation of the current implementation, **not** of the path — `claude` fires a `MessageDisplay` hook carrying incremental, byte-faithful `delta`s of the raw reply (they concatenate exactly to the final text, and stay prefix-stable), on the subscription pool, without `-p`. Wiring it into OCP's SSE is tracked as backlog item #2. What is *not* available is token-by-token granularity (the hook fires per rendered block, ~5–7 chunks per answer) — which is plenty for SSE. Evidence: [`docs/plans/2026-07-13-tui-latency/streaming-spike.md`](docs/plans/2026-07-13-tui-latency/streaming-spike.md). +- **No real token streaming *today* — but it is achievable, and planned.** TUI-mode currently buffers the full response then replays it as chunked SSE: you see a delay, then the complete response. This is a limitation of the current implementation, **not** of the path — `claude` fires a `MessageDisplay` hook carrying incremental, byte-faithful `delta`s of the raw reply (they concatenate exactly to the final text, and stay prefix-stable), on the subscription pool, without `-p`. Wiring it into OCP's SSE is tracked as backlog item #2. What is *not* available is token-by-token granularity (the hook fires once per rendered block — roughly one per paragraph, list item, or code block, so the count scales with answer length) — which is plenty for SSE. Evidence: [`docs/plans/2026-07-13-tui-latency/streaming-spike.md`](docs/plans/2026-07-13-tui-latency/streaming-spike.md). - **Cache and singleflight work normally.** TUI-mode writes the buffered response to the cache on success; cache-hits skip the interactive turn entirely. - **The host's `CLAUDE.md` / auto-memory is never injected.** OCP is a proxy — the proxied client (OpenClaw / your IDE) owns its own context and memory. TUI-mode always runs `claude` with `CLAUDE_CODE_DISABLE_CLAUDE_MDS` + `CLAUDE_CODE_DISABLE_AUTO_MEMORY`, so a `CLAUDE.md` on the OCP host can never leak into proxied turns (verified live; see #4). Built-in tool schemas + the interactive system prompt remain (the inherent ~20–35K context floor of interactive mode); MCP is hard-disabled. - **Authenticate via `CLAUDE_CODE_OAUTH_TOKEN` in a credential-isolated home (recommended).** tmux does not forward the parent process's env to the pane, so OCP sets the token explicitly on the spawned `claude` when `CLAUDE_CODE_OAUTH_TOKEN` is present. But passing the token is **not enough on its own**: interactive `claude` *prefers* `~/.claude/.credentials.json` over the env var (unlike the `-p` path), so a stale `credentials.json` would shadow the token. With the env token set and `OCP_TUI_HOME` unset, OCP therefore runs claude in a **credential-isolated home** (`$HOME/.ocp-tui/home`) that has **no `credentials.json`** — so the env token is the only credential and is authoritative, and claude never runs the token-refresh path (so the single-use refresh token can't be corrupted by the spawn/teardown cycle). On a long-running host the credentials.json path produced a permanent `Please run /login · API Error: 401` that re-login could not fix (the next spawn re-corrupted it); the isolated home ends that at the root. Transcripts land under the same isolated home, so the answer-reader is unaffected. Without the env token, claude falls back to the real home's `credentials.json` (byte-for-byte the previous behaviour). (The token is visible in `ps` on the pane command — acceptable for the single-user A-path; the multi-user B-path is refused at boot.) See ADR 0007 PR-C / PR-D amendments. diff --git a/docs/plans/2026-07-13-tui-latency/streaming-spike.md b/docs/plans/2026-07-13-tui-latency/streaming-spike.md index 8800cef..e2b4854 100644 --- a/docs/plans/2026-07-13-tui-latency/streaming-spike.md +++ b/docs/plans/2026-07-13-tui-latency/streaming-spike.md @@ -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` (`/.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