Skip to content

fix(agent-core): allow benign session rewrites with different inode#88653

Closed
raymondjxj wants to merge 1 commit into
openclaw:mainfrom
raymondjxj:fix/session-fence-inode-false-positive
Closed

fix(agent-core): allow benign session rewrites with different inode#88653
raymondjxj wants to merge 1 commit into
openclaw:mainfrom
raymondjxj:fix/session-fence-inode-false-positive

Conversation

@raymondjxj

@raymondjxj raymondjxj commented May 31, 2026

Copy link
Copy Markdown

Summary

Compaction rewrites session files via writeFile+rename, which always creates a new inode. Previously, sessionFenceRewriteIsBenign rejected such rewrites at the inode identity gate (sameSessionFileIdentity), so the content validation that follows never ran, causing EmbeddedAttemptSessionTakeoverError false positives.

Root Cause

In sessionFenceRewriteIsBenign (line 266), the sameSessionFileIdentity check compares dev + ino. When compaction runs, it replaces the session file with a new inode, causing this check to return false and the function to exit early — without ever performing content validation.

The actual error path:

  1. releaseForPrompt() records fingerprint (inode A)
  2. Compaction runs, renames new file (inode B)
  3. assertSessionFileFence() compares fingerprint: inode B ≠ inode A
  4. sessionFenceRewriteIsBenign called, fails sameSessionFileIdentity check
  5. EmbeddedAttemptSessionTakeoverError thrown — false positive

Fix

Remove the inode identity gate from sessionFenceRewriteIsBenign while preserving all content validation. The content checks remain fully in place: file must exist, end with a newline, append only valid transcript lines, and stay within size limits. A file with a different inode but valid compaction-style content is indistinguishable from a compaction from the agent's perspective.

Source

src/agents/embedded-agent-runner/run/attempt.session-lock.ts, function sessionFenceRewriteIsBenign, line 266.

Testing

  • Unit test: verify sessionFenceRewriteIsBenign returns true for same-content file with different inode
  • Integration test: run compaction concurrently with message processing, verify no EmbeddedAttemptSessionTakeoverError

Hermes
fix(agent-core): allow benign session rewrites with different inode
7e6995d 
Compaction rewrites session files via writeFile+rename, which always
creates a new inode. Previously, sessionFenceRewriteIsBenign rejected
such rewrites at the identity check (sameSessionFileIdentity), so the
content validation that follows never ran, causing
EmbeddedAttemptSessionTakeoverError false positives.

This change removes the inode identity gate from sessionFenceRewriteIsBenign
while preserving all content validation: file must exist, end with a
newline, append only valid transcript lines. A file with a different inode
but valid compaction-style content is indistinguishable from a compaction
from the agent's perspective, so the inode check was both too strict and
redundant with the content checks.

Fixes EmbeddedAttemptSessionTakeoverError during concurrent compaction.
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. triage: refactor-only Candidate: refactor/cleanup-only PR without maintainer context. labels May 31, 2026
@clawsweeper

clawsweeper Bot commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close: the compaction false-positive this PR was trying to address is now handled by the merged owned-compaction persistence path in current main and v2026.6.6, while this branch still weakens the session takeover boundary by trusting content-only replacement rewrites.

Canonical path: The related merged PR https://github.com/openclaw/openclaw/pull/90775 explicitly fixes owned auto-compaction fence refresh, keeps raw external compaction edits rejected, and merged at 2026-06-06T00:05:36Z with merge commit bbfe8cc.

So I’m closing this here and keeping the remaining discussion on #90775.

Review details

Best possible solution:

Keep the shipped owned-compaction persistence hook and continue rejecting content-only replacement rewrites unless a future narrow, proof-backed owner signal is needed.

Do we have a high-confidence way to reproduce the issue?

No high-confidence current-main reproduction remains for the original compaction false positive: current main and v2026.6.6 contain the owned compaction persistence path and regression coverage. Source inspection does show the proposed patch would broaden replacement-file trust if merged.

Is this the best way to solve the issue?

No: this is not the best way to solve the issue. The merged replacement fixes owned compaction at the writer/provenance boundary while preserving rejection of raw external session edits.

Security review:

Security review needs attention: The patch weakens a session takeover safety check by trusting replacement-file contents without inode or owned-write provenance.

  • [medium] Replacement file can bypass takeover provenance — src/agents/embedded-agent-runner/run/attempt.session-lock.ts:339
    Dropping the same-file identity guard means a different inode can be accepted when it preserves prior lines and appends content that matches the helper's limited benign-rewrite checks, weakening protection against unowned transcript replacement.
    Confidence: 0.9

AGENTS.md: found and applied where relevant.

What I checked:

  • PR diff relaxes rewrite identity guard: The submitted hunk deletes the sameSessionFileIdentity(params.previous.fingerprint, params.current) check from sessionFenceRewriteIsBenign, making replacement-inode trust depend on content validation alone. (src/agents/embedded-agent-runner/run/attempt.session-lock.ts:339, 7e6995d0fbd5)
  • Current main keeps replacement rewrites out of the benign path: Current main still requires the previous and current session file to share the same dev and ino before sessionFenceRewriteIsBenign reads and validates rewritten content. (src/agents/embedded-agent-runner/run/attempt.session-lock.ts:335, 7a7165ad22c4)
  • Current main trusts owned compaction through the guarded session manager: The embedded attempt setup now passes withCompactionPersistence into guardSessionManager, routing appendCompaction through sessionLockController.withOwnedSessionFileWrite(...) instead of broadening the rewrite whitelist. (src/agents/embedded-agent-runner/run/attempt.ts:2125, 7a7165ad22c4)
  • Compaction append validation is narrow: installSessionToolResultGuard wraps appendCompaction and validates that exactly one appended JSONL line has type: "compaction" and the returned entry id before publishing the write as owned. (src/agents/session-tool-result-guard.ts:68, 7a7165ad22c4)
  • Regression coverage preserves the security boundary: Current tests accept owned SessionManager.appendCompaction(...) during prompt release and reject raw external compaction-looking appends or interleaved external edits. (src/agents/embedded-agent-runner/run/attempt.session-lock.test.ts:1065, 7a7165ad22c4)
  • Canonical merged replacement: The related merged PR https://github.com/openclaw/openclaw/pull/90775 explicitly fixes owned auto-compaction fence refresh, keeps raw external compaction edits rejected, and merged at 2026-06-06T00:05:36Z with merge commit bbfe8cc. (bbfe8ccaf60e)

Likely related people:

  • jalehman: Authored the merged replacement path in https://github.com/openclaw/openclaw/pull/90775, which added the owned compaction persistence hook, regression coverage, and Testbox proof for the same compaction fence failure. (role: canonical fix author; confidence: high; commits: 054ea49f5754, bbfe8ccaf60e; files: src/agents/embedded-agent-runner/run/attempt.session-lock.ts, src/agents/embedded-agent-runner/run/attempt.ts, src/agents/session-tool-result-guard.ts)
  • steipete: The all-ref shortlog shows the most activity across the central embedded runner lock, attempt, and session guard files, with multiple recent refactor and transcript/tool-result commits in this area. (role: heavy adjacent area contributor; confidence: medium; commits: 0c278bb93c2d, 5586b3fd19cd, ee03ade0d63f; files: src/agents/embedded-agent-runner/run/attempt.session-lock.ts, src/agents/embedded-agent-runner/run/attempt.ts, src/agents/session-tool-result-guard.ts)
  • shakkernerd: Recent current-main blame on the touched session-lock, attempt, and guard files points to Shakker's session test/env cleanup, so they are a recent routing candidate for this exact surface. (role: recent area contributor; confidence: medium; commits: a1b7f3570ce3, befa421a57cf; files: src/agents/embedded-agent-runner/run/attempt.session-lock.ts, src/agents/embedded-agent-runner/run/attempt.ts, src/agents/session-tool-result-guard.ts)
  • Takhoffman: Recent history on the touched agent/session surfaces includes context-window and tool-result truncation work that shares the session/context boundary involved here. (role: adjacent context-window contributor; confidence: medium; commits: 4f00b769251d, 7fc1a74ee9e3; files: src/agents/embedded-agent-runner/run/attempt.ts, src/agents/session-tool-result-guard.ts)

Codex review notes: model internal, reasoning high; reviewed against 7a7165ad22c4; fix evidence: release v2026.6.6, commit bbfe8ccaf60e.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels May 31, 2026
@lykeion-dev lykeion-dev mentioned this pull request Jun 1, 2026
Open
@jalehman jalehman mentioned this pull request Jun 5, 2026
Merged
@clawsweeper

clawsweeper Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper applied the proposed close for this PR.

  • Action: closed this PR.
  • Close reason: duplicate or superseded.
  • Evidence: durable ClawSweeper review.
  • Coverage proof: PR B clearly implements the useful remaining behavior behind PR A by fixing the same compaction fence false positive through the safer owned-write path, while preserving the security boundary that PR A would weaken. Any leftover PR A details are incidental or undesirable rather than independent work needing review. Covering PR: fix: refresh prompt fence after compaction writes #90775.

@clawsweeper clawsweeper Bot closed this Jun 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. triage: refactor-only Candidate: refactor/cleanup-only PR without maintainer context.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@raymondjxj