fix(agents): stop false-positive session-takeover on runner's own transcript appends#84046
fix(agents): stop false-positive session-takeover on runner's own transcript appends#84046kays0x wants to merge 1 commit into
Conversation
|
Codex review: found issues before merge. Latest ClawSweeper review: 2026-05-20 20:05 UTC / May 20, 2026, 4:05 PM ET. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. Source inspection shows current main throws on any session-file stat mismatch after prompt-lock release, and the supplied before/after gateway logs plus production validation show the failure and improvement on long multi-tool turns. PR rating Rank-up moves:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. Real behavior proof Risk before merge
Maintainer options:
Next step before merge Security Review findings
Review detailsBest possible solution: Choose one canonical session-lock model, then land a rebased implementation that either strictly validates runner-owned append-only tails or routes all runner-owned writes through the owned-write context without leaving competing paths. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main throws on any session-file stat mismatch after prompt-lock release, and the supplied before/after gateway logs plus production validation show the failure and improvement on long multi-tool turns. Is this the best way to solve the issue? Not yet. The approach is plausible and well covered, but the current head needs a rebase against the owned-write context work and should fail closed on empty appended tail segments before merge. Label changes:
Label justifications:
Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 1a7669bc63a0. |
…unner-owned shape ClawSweeper review on openclaw#84046 flagged the prior count-only check as too broad: concurrent transcript rewrites, file replacements, branch changes, and same-user-count mutations could all keep the user-message count unchanged while changing session state. Tighten the fence disambiguation so a stat-fingerprint mismatch is only tolerated when ALL of these hold: - dev/ino match (no atomic replacement onto a new inode) - birthtimeNs matches (no unlink+recreate-same-inode case; common on tmpfs/ext4 with rapid recreation, where dev and ino are reused) - size grew (file is append-only between fence-set and assert) - bytes [0, fenceSize) hash identical to the fenced prefix hash (no in-place rewrite of earlier transcript content) - bytes [fenceSize, currentSize) contain only message entries with role assistant or tool (no new user-role turn, no malformed tail) Anything else — replacement, shrink, in-place rewrite, new user turn, or compaction-style mutation — remains a real takeover. Snapshot now captures the prefix hash alongside the fingerprint at releaseForPrompt and refreshSessionFileFence so the slow path can verify rigorously. Adds tests covering in-place rewrite and unlink+recreate replacement (both with prefix bytes coincidentally matching and roles that would otherwise look runner-owned); existing append-only and new-user-turn tests preserved. Refs: openclaw#83436, openclaw#83615 Addresses ClawSweeper P1 finding on prior revision of openclaw#84046.
|
@clawsweeper re-review Revision addresses your P1 finding from the prior review: the fence now requires append-only growth, identical inode + birthtime + prefix-hash, and assistant/tool-only tail roles. In-place rewrites, file replacements, and same-user-count mutations all trip takeover. Real behavior proof added to PR body (before/after gateway log evidence from a 217s DM run that previously false-tripped). 16/16 tests passing, check:changed clean. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Revision addresses both P1 findings from your last review:
Tests: existing acceptance test exercises all three runner-owned roles with their actual schema; new test asserts a |
…t non-message tail entries ClawSweeper re-review on openclaw#84046 flagged two P1 issues on the prior revision of the append-only fence: - The role allowlist used 'tool', but OpenClaw persists tool outputs as message.role === 'toolResult' (see src/agents/pi-embedded-runner/transcript-file-state.ts:182). Real tool-using turns would still false-trip the takeover. Update the allowlist to match isAgentMessage's runner-owned roles: assistant, toolResult, bashExecution. - The slow path silently 'continue'd through non-message tail entries (custom, branch_summary, compaction, session). That let a different owner mutate session state during the released-lock window without a new user turn. Fail closed: any non-message entry in the tail rejects the fence. Test updates: existing acceptance test now appends assistant + toolResult + bashExecution (the three valid persisted runner-owned roles); new test asserts a 'type: custom' tail entry trips takeover even though it grew append-only. Refs: openclaw#83436, openclaw#83615 Addresses ClawSweeper P1 findings on second revision of openclaw#84046.
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
464cb15 to
42ff382
Compare
|
Addressing both your P1 (null/non-object tail) and a related fence-consistency issue surfaced by a separate review. Changes:
Tests added:
78 tests passing across 4 files; @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
42ff382 to
ac07599
Compare
|
Addressing the snapshot stat/read consistency finding, and stating the contract explicitly so we can draw the line on hypothetical filesystem races below the lock-held invariant. Change: Both Tests added:
Both tests interpose between stat and read by spying 82 tests passing across 4 files; Lock-held invariant (now documented in the PR summary): both @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
ClawSweeper PR egg 🔥 Warming up: real-behavior proof passed; findings, security review, or rank-up moves are still in progress. Hatch commandComment Hatchability rules:
What is this egg doing here?
|
|
Closing this as stale against the current runner layout. This PR edits the old Thanks for the original investigation; the fix needs to happen on the current runner surface now. |
Summary
The embedded attempt session-lock fence uses a
stat()-based fingerprint (dev,ino,size,mtimeNs,ctimeNs,birthtimeNs) to detect "another lane queued a new user turn against this session" while the runner releases its coarse lock for the LLM prompt window. The fingerprint is a proxy that overfires: every transcript-append the runner itself performs during the released window (appendSessionTranscriptMessageLockedwrites tool calls, tool results, and assistant replies viaacquireSessionWriteLock(..., allowReentrant: true)) changes the file'ssize/mtimeNs/ctimeNswithout going throughrefreshSessionFileFence. Long DM turns with tool calls then tripassertSessionFileFenceand throwEmbeddedAttemptSessionTakeoverErrorafter the reply has already been delivered, surfacing a misleading "Agent failed before reply" message on top of a successful turn.Keep the stat fingerprint as the fast path. On mismatch, only tolerate the runner's append-only assistant/tool transcript shape; treat everything else as a real takeover. The verification requires ALL of:
snapshotSessionFileFenceandverifyAppendOnlyRunnerOwnedExtensionrun a post-readstatand compare every fingerprint field (size, dev, ino, mtimeNs, ctimeNs, birthtimeNs) against the pre-read fingerprint. Any mismatch fails closed: the snapshot returnsprefixHashHex: null(forces the next assertion to treat the file as taken over) and the slow path rejects directly. Catches the same-size in-place rewrite race where byte length alone would not detect drift.[0, fenceSize)hash identical to the fenced prefix hash: no in-place rewrite of earlier transcript content.[fenceSize, currentSize)are canonical session entries: every appended line must satisfyisSessionEntryfromtranscript-file-state.ts(record shape, type,id,parentId,timestamp, plus the per-type message-shape contract viaisAgentMessage), with role narrowed toassistant,toolResult, orbashExecution. Non-object parsed values, user-role entries, custom entries, branch_summary entries, and any entry missing the outer base fields remain a real takeover signal.Anything else (replacement, shrink, in-place rewrite at any size, new user turn, compaction, a malformed appended entry, a non-object tail value, or a stat/read inconsistency window) remains a real takeover. The fence snapshot captures the prefix SHA-256 alongside the fingerprint at
releaseForPromptandrefreshSessionFileFenceso the slow path can verify rigorously without keeping a file handle open.The slow-path tail validation delegates to the canonical
isSessionEntrywhole-entry validator (nowexport-visible fromtranscript-file-state.ts, signature widened tounknownwith an internalisRecordguard so non-record JSON values fail closed). No local mirror of the persisted message contract is maintained: if the upstreamFileEntry/SessionEntrycontract changes, the fence's accept-set tracks it automatically.The
nextSnapshotreturned from a successful slow-path verification hashes the full new buffer (not the pre-fence prefix). Otherwise, when the guarded operation throws betweenassertSessionFileFenceandrefreshSessionFileFence, the stored snapshot would describe the new size but hold the old prefix hash, and the next legitimate runner append would false-positive on prefix-hash mismatch.Lock-held invariant
Both
snapshotSessionFileFencecall sites run while the session write lock is held:releaseForPrompt(snapshot at L452 beforelock.release()at L454) andrefreshSessionFileFence(called insidewithSessionWriteLock'srunWithLockwrapped byactiveWriteLock.run(lock, ...)). Canonical transcript writers (appendSessionTranscriptMessageLocked,migrateLinearTranscriptToParentLinked,ensureTranscriptHeader) all acquire that same lock. The stat/read consistency guard above is belt-and-suspenders for this invariant: it defends against multi-process scenarios (a second gateway against the same session file), an external editor, or an internal bug that bypasses the lock.Closes #83436.
Refs #83615 (this PR addresses the EmbeddedAttemptSessionTakeoverError surface; the issue also tracks unrelated kimi-k2.6 schema and DNS-fetch failures).
Refs #84059 (active multi-environment hub for this error: Feishu/Slack/WebChat across macOS APFS, Linux ext4, WSL2, Docker; all are the general internal-write path this PR covers, not the delivery-mirror path addressed by #84437).
Real behavior proof
Behavior addressed: Embedded runner false-positive
EmbeddedAttemptSessionTakeoverErrorfires after long Telegram DM turns whose reply was already delivered, surfacing a misleading "Agent failed before reply" message on top of a successful run.Real environment tested: OpenClaw 2026.5.19-beta.1, Linux x86_64, Node 24.13.0. Long-running Telegram DM session against
claude-opus-4-7(~3.5 min turn with multipleexecandmessagetool calls), reproduced reliably before this patch. Production behavior probed via runtime-patcheddist/selection-BpjGe-Y0.jscarrying the same fix semantics, then long DM runs (41s to 122s) exercised through the Telegram channel post-restart.Exact steps or command run after this patch:
Evidence after fix:
Pre-patch failure mode, redacted runtime log from gateway (
/tmp/openclaw/openclaw-2026-05-19.log):Note
isError=falseat03:30:06.971: the assistant turn andmessagetool delivery both succeeded, followed 47ms later by the takeover throw, surfacing as a misleading Telegram error on top of a delivered reply.Post-patch behavior on the same gateway, four long DM runs after gateway restart at 03:46 EDT, every one delivering cleanly with zero takeover errors (redacted runtime log):
The post-patch window contains only two "Embedded agent failed before reply" entries (at 04:05:06 and 04:07:35), both with cause
LLM request timed out, unrelated to the takeover path. ZeroSessionTakeoverErroroccurrences post-patch.Observed result after fix: Existing acceptance test exercises all three runner-owned roles (
assistant,toolResult,bashExecution) with their canonical persisted shapes. Rejection tests cover: another owner queueing a new user turn, a runner-owned role with malformed message contents, a runner-owned message entry missing its outerid, a non-message entry (e.g.custom/branch_summary/compaction), a tail line that parses to a non-object value (e.g. literalnull), in-place rewrite (rejected via prefix hash mismatch), unlink+recreate-same-inode replacement (rejected viabirthtimeNsmismatch), a same-size in-place rewrite landing during the snapshot's stat-to-read window (rejected via post-read re-stat), and the same race during the slow-path verification's read (rejected via the same guard). A fence-consistency test covers the post-throw path: after a guarded operation throws between assert and refresh, the next valid runner-owned append must not false-positive on prefix-hash mismatch. Targeted Vitest output:check:changed --staged=falseis clean across the full repo lane (lint, typecheck, import-cycles, dependency guards, build artifacts, security/quality lanes).What was not tested: Multi-process write contention on the same
sessionFilefrom a different gateway process (no test infrastructure for cross-process locks in this surface). The prefix-hash check isO(fenceSize)I/O per fence-set; in production sessions seen so far (~150 KB and below) this is sub-millisecond. Both snapshot producers do one extrafs.stat(sub-millisecond on local disk) to enforce the stat/read consistency guard.Notes
readSessionFileFingerprint:FenceSnapshottype,snapshotSessionFileFence(used atreleaseForPromptandrefreshSessionFileFence), andverifyAppendOnlyRunnerOwnedExtension(used in the slow path ofassertSessionFileFence). No new module-level dependencies beyondnode:crypto'screateHash.fenceFingerprint: SessionFileFingerprint | undefined) withfenceSnapshot: FenceSnapshot | undefined. No new long-lived resources (no held file handles).isSessionEntryfromtranscript-file-state.tsso the fence's slow path can delegate the whole-entry contract to the canonical validator. The signature is widened tounknownwith an internalisRecordguard; non-record JSON values fail closed at the canonical boundary instead of needing per-callsite type-shape checks.isAgentMessageis not exported (the fence reaches it transitively throughisSessionEntry).