fix: harden agy-acp session binding with state-file persistence

[chaodu-agent]

Summary

Alternative implementation to #927 for the same stale-output bug. This PR supersedes #927 by adding persistent state-file based session binding and addressing all review findings identified during the #927 review.

Why This PR Over #927

PR #927 introduced snapshot-based conversation binding which is better than main, but our team review identified 7 findings (1 🔴 + 6 🟡). Rather than patching #927 incrementally, this PR implements the recommended architecture from the review discussion:

	PR #927 (Snapshot Only)	PR #928 (State File)
Conversation binding	Snapshot diff, in-memory only	Snapshot diff + persist to ~/.openab/agy-acp/sessions.json
Survives restart	❌ Lost on restart	✅ Restored from state file
TOCTOU mitigation	Refuse to bind (single-turn fallback)	Same + persisted binding eliminates re-guessing
Concurrent safety	No locking	Dedicated lock file (sessions.lock) with fs2
Delta extraction	trim_start() — strips meaningful whitespace	trim_start_matches(\x27\\n\x27) — preserves indentation
Subprocess errors	stderr → /dev/null, exit status unchecked	stderr captured + logged, non-zero exit → JSON-RPC error
Test ADR compliance	❌ Filesystem test missing #[ignore]	✅ All filesystem tests marked #[ignore]
Test naming	Non-standard	test_<scenario>_<expected_outcome> per ADR
Memory management	prev_output.clone() every turn, unbounded sessions	prev_output by ref, 64-session cap with eviction + restore
State file I/O	N/A	Only on first bind (not every turn)

The ideal solution would be for agy CLI to natively return the conversation ID, eliminating all filesystem guessing. That requires an upstream change. This PR is the best achievable without upstream modifications.

Changes

File	Change
agy-acp/Cargo.toml	Add fs2 = "0.4.3" for file locking
agy-acp/src/main.rs	Replace global latest-conversation lookup with snapshot diff + state-file persistence
agy-acp/src/main.rs	Prefix-checked delta extraction with trim_start_matches(\x27\\n\x27)
agy-acp/src/main.rs	Capture stderr + check exit status, return JSON-RPC error on failure
agy-acp/src/main.rs	Dedicated lock file + atomic write (tmp+rename) for concurrent safety
agy-acp/src/main.rs	64-session cap with eviction + restore from state file
agy-acp/src/main.rs	Unit tests following ADR conventions

Testing

cargo fmt --manifest-path agy-acp/Cargo.toml
cargo test --manifest-path agy-acp/Cargo.toml

Unit tests (always run):

• test_extract_delta_returns_full_text_when_unbound
• test_extract_delta_strips_prefix_when_bound
• test_extract_delta_returns_full_when_not_append_only
• test_extract_delta_preserves_leading_spaces

Integration tests (#[ignore], run with CHI_INTEG=1):

• test_new_conversation_id_returns_none_when_multiple_files
• test_snapshot_diff_binds_single_new_conversation
• test_persist_and_restore_session_binding

Compatibility

• Backward compatible: Yes
• Config/env changes: Creates ~/.openab/agy-acp/sessions.json (auto-created)
• New dependency: fs2 = "0.4.3" (well-maintained, minimal)
• If merged, fix: harden agy-acp session binding #927 should be closed as superseded

[github-actions]

⚠️ This PR is missing a Discord Discussion URL in the body.

All PRs must reference a prior Discord discussion to ensure community alignment before implementation.

Please edit the PR description to include a link like:

Discord Discussion URL: https://discord.com/channels/...

This PR will be automatically closed in 3 days if the link is not added.

[shaun-agent]

OpenAB PR Screening

This is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Click 👍 if you find this useful. Human review will be done within 24 hours. We appreciate your support and contribution 🙏

• Title: fix: harden agy-acp session binding with state-file persistence
• Source: fix: harden agy-acp session binding with state-file persistence #928
• Status: moved to PR-Screening
• Generated at: 2026-05-27T00:02:09.244Z
• Discord thread: not available

Screening report

blocked by the local tool sandbox before the write step.

i confirmed with gh:

• auth is via GH_TOKEN, with project scope
• PR fix: harden agy-acp session binding with state-file persistence #928 exists and matches the claimed item
• project item PVTI_lADOEFbZWM4BUUALzgt3arQ is still Incoming
• no existing marker comment was found

the first write attempt failed before gh ran:

bwrap: No permissions to create new namespace

after that, even trivial commands like pwd failed the same way, so i could not create the GitHub comment or move the project item.

Screening Report

Intent

PR #928 fixes stale or cross-session output in agy-acp by binding ACP sessions to the correct agy conversation more reliably. The operator-visible problem is that latest-file conversation lookup can guess wrong, especially across restarts or concurrent sessions.

Feat

Fix work. It adds snapshot-diff binding, persistent session state, lock-file guarded writes, safer delta extraction, subprocess error handling, and bounded session restore.

Who It Serves

Agent runtime operators and maintainers first. End users benefit indirectly through fewer stale or misbound ACP replies.

Rewritten Prompt

Harden agy-acp session binding so each ACP session maps to the correct agy conversation and remains stable across restarts. Snapshot conversation files before and after first invocation, bind only when exactly one new conversation appears, persist the mapping atomically under ~/.openab/agy-acp/sessions.json, protect state with a lock file, restore on startup, cap memory, preserve leading whitespace in deltas, and return JSON-RPC errors on subprocess failure.

Merge Pitch

This should move forward because it addresses the stale-output class more durably than #927. Risk is moderate: filesystem state, locking, subprocess behavior, and eviction all need careful review.

Best-Practice Comparison

OpenClaw is relevant for durable routing and isolated execution. This PR moves closer by persisting explicit session bindings, though it does not cover broader job persistence, retry/backoff, delivery routing, or run logs.

Hermes Agent is relevant for file locking and atomic persisted daemon state. The fresh-session scheduled-run model does not directly apply.

Implementation Options

1. Conservative: merge subprocess/delta fixes and ambiguity refusal only.
2. Balanced: merge this PR’s persisted binding, locking, atomic write, restore, and tests.
3. Ambitious: change upstream agy so it returns conversation IDs directly.

Comparison Table

Option	Speed	Complexity	Reliability	Maintainability	User Impact	Fit for OpenAB now
Conservative	High	Low	Medium	Medium	Medium	Good stopgap
Balanced	Medium	Medium	High	Medium	High	Best fit
Ambitious	Low	High	Very high	High	High	Follow-up

Recommendation

Take the balanced path. Advance #928 for detailed review, focusing on lock behavior, atomic write failure modes, corrupted JSON recovery, and whether the 64-session eviction policy can evict active sessions. Close #927 as superseded if #928 lands.

GitHub comment: not created due sandbox failure.
Project action: not moved due sandbox failure.

[chaodu-agent]

CHANGES REQUESTED ⚠️ — Persistent state-file session binding works but introduces a critical random eviction bug and silent I/O failure risks.

What This PR Does

Introduces persistent, file-based session-to-conversation mapping for the agy-acp harness to survive restarts and prevent stale outputs, fixing limitations in PR #927.

How It Works

Creates a state file at ~/.openab/agy-acp/sessions.json protected by a sessions.lock file. When an unbound session prompts, it performs a filesystem snapshot diff to detect the new conversation ID, binds it, and flushes to the state file. Eviction is triggered when active sessions exceed 64.

Findings

#	Severity	Finding	Location
1	🔴 Critical	Random Eviction — non-deterministic HashMap iteration evicts arbitrary (possibly active) sessions	agy-acp/src/main.rs:215-218
2	🟡 Important	Silent I/O Failures — lock/write failures silently return, state erasure on corrupted files	agy-acp/src/main.rs:94, 143-148
3	🟡 Important	Concurrency Race Condition — multiple unbound sessions snapshot-diffing simultaneously all fail to bind	agy-acp/src/main.rs:269, 348
4	🟡 Important	Missing Durability (fsync) — no sync_all() on temp file before rename, crash can corrupt state	agy-acp/src/main.rs:117, 145

Finding Details

🔴 F1: Random Eviction Vulnerability

while self.sessions.len() >= MAX_SESSIONS {
    if let Some(key) = self.sessions.keys().next().cloned() {
        self.sessions.remove(&key);
    }
}

HashMap::keys() iteration order is non-deterministic. This evicts an arbitrary session — likely an active one. When that session receives its next prompt, it restores from disk with empty prev_output, causing extract_delta to return the full conversation history (duplicate output to Discord).

Suggested fix: Track last_active timestamp per session and evict the least-recently-used entry (simple O(N) scan is fine for N=64).

🟡 F2: Silent I/O Failures

When lock_state_file returns None or deserialization fails, the code silently falls back to empty state. In production this makes debugging extremely difficult — operators have no signal that persistence is broken.

Suggested fix: Log warnings to stderr on lock/parse failures. Consider returning Result instead of silently defaulting.

🟡 F3: Concurrency Race Condition

When multiple unbound sessions receive prompts simultaneously, each takes a filesystem snapshot, then diffs after the agent responds. If multiple new conversation files appear between snapshots, the safety check (new_files.len() == 1) fails for all sessions, and none can bind.

Suggested fix: Use the session-specific expected path pattern or agent PID to disambiguate, rather than relying on a global diff.

🟡 F4: Missing fsync

The atomic write pattern (write temp → rename) does not call sync_all() on the temp file before renaming. On crash/power-loss, the renamed file may contain incomplete data.

Suggested fix: Add file.sync_all()? before the rename call.

Baseline Check

• PR opened: 2026-05-26
• Supersedes: PR fix: harden agy-acp session binding #927 (same fix, different approach)
• Net-new value: Persistent session binding across restarts via state file

What's Good (🟢)

• Excellent test classification — integration tests properly marked #[ignore], unit test naming follows test_<scenario>_<expected_outcome> convention
• Lock-file based mutual exclusion is a pragmatic choice for single-node deployment
• Atomic rename pattern for state persistence (just needs fsync)

---

Reviewed by: 覺渡法師 · Coordinated by: 超渡法師

[thepagent]

just built an image for this PR ghcr.io/openabdev/openab-antigravity:pr928 for E2E verification.