feat(agents): cross-session plan store with file-level locking [Phase 4.2]#67542
feat(agents): cross-session plan store with file-level locking [Phase 4.2]#67542100yenadmin wants to merge 16 commits into
Conversation
Phase 4.2 of the GPT 5.4 parity sprint. Adds a PlanStore class for cross-session task coordination, modeled after Claude Code's Tasks API concept. ## PlanStore (plan-store.ts) - read(namespace): loads plan from ~/.openclaw/plans/<ns>/plan.json - write(namespace, plan): persists plan to disk with auto-mkdir - lock(namespace): file-level exclusive lock with O_EXCL + 10s stale-lock auto-cleanup to prevent deadlocks from crashes - mergeSteps(): merge incoming steps into existing plan by matching step text, appending new steps, preserving existing order. Tracks updatedBy (session key) and updatedAt per step. ## Schema (StoredPlan, StoredPlanStep) - StoredPlanStep: step, status (4 values), activeForm?, updatedBy?, updatedAt? - StoredPlan: namespace, steps[], createdAt, updatedAt ## Tests (8 tests) - Read/write round-trip + nested directory creation - Lock acquisition, release, and concurrent contention - Merge: update by text match, append new, preserve order Default behavior (no namespace configured): plan is session-scoped, PlanStore is not used. Cross-session coordination only activates when planStore.namespace is set in config. Tracking: #66345, #67523.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78fc4cd8a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds a persistent, cross-session PlanStore implementation intended to enable multi-agent plan coordination when a namespace is configured.
Changes:
- Introduces
PlanStorewith disk-backedread/write, namespace-scoped locking, andmergeStepssemantics. - Implements a simple file-based exclusive lock with stale-lock cleanup.
- Adds Vitest unit coverage for read/write, locking behavior, and merge semantics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/agents/plan-store.ts | New persistent plan store (JSON on disk) with file lock + merge logic. |
| src/agents/plan-store.test.ts | Unit tests covering read/write roundtrip, locking, and merge behavior. |
Greptile SummaryAdds
Confidence Score: 4/5Safe to merge after fixing the TypeScript type error in the symlink confinement test — one-line fix, no logic change needed. One P1 finding: the incomplete object literal passed to write() in the symlink test is a TypeScript error that fails the pnpm tsgo gate, which is a hard landing requirement per CLAUDE.md. All P0/security concerns from prior rounds are resolved. Remaining findings are P2 (stale docstring, duplicate comment). src/agents/plan-store.test.ts — symlink confinement test at line 249 needs required StoredPlan fields added. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/plan-store.test.ts
Line: 248-250
Comment:
**TypeScript error — incomplete `StoredPlan` fails `pnpm tsgo`**
`{ steps: [...] }` is missing the three required `StoredPlan` fields (`namespace`, `createdAt`, `updatedAt`). TypeScript will reject this with "Type is missing the following properties: namespace, createdAt, updatedAt", failing the `pnpm tsgo` gate. At runtime the call still throws at `confine()` before those fields are read, but the compiler doesn't know that.
```suggestion
await expect(
store.write("hostile", {
namespace: "hostile",
steps: [{ step: "x", status: "pending" }],
createdAt: 0,
updatedAt: 0,
}),
).rejects.toThrow(/escapes base directory/);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/plan-store.ts
Line: 322-325
Comment:
**Stale docstring — says "10s", constant is 60s**
`LOCK_STALE_MS` was bumped to `60_000` ms (line 121) but the JSDoc still says "older than 10s", and the inline comment on line 336 likewise says ">10s". Misleads anyone debugging a stuck lock.
```suggestion
/**
* Acquires a file-level lock for a namespace.
* Returns a release function. Stale locks (older than 60s) are
* cleaned up opportunistically by the next lock() caller.
*/
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/plan-store.ts
Line: 35-40
Comment:
**Duplicate JSDoc — comment belongs to `sanitizePlanShape`, not `VALID_STEP_STATUSES`**
This block (lines 35–40) is an exact duplicate of the opening two sentences of the real JSDoc at lines 43–55 for `sanitizePlanShape`. As written it currently documents the `VALID_STEP_STATUSES` constant with the wrong description. Remove this block entirely.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix(plan-store): #67542 — full per-step ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… errors, lock ownership - Validate namespace to prevent path traversal (reject "..", absolute paths) - Atomic write: write to temp file then rename to prevent mid-write corruption - Fix file handle leak: ensure handle.close() in finally on open errors - Distinguish ENOENT from parse/permission errors in read() instead of swallowing all errors - Verify lock ownership token before unlinking on release to prevent one process from deleting another's lock
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30a981e5cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6192fa4602
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replace O(n) linear scan with a Set when checking whether incoming steps already exist, improving merge performance for large plans.
Three fixes from Copilot review: 1. mergeSteps: only set updatedBy when sessionKey is truthy, avoiding undefined overwriting existing attribution 2. write(): validate plan.namespace matches the namespace argument to prevent accidental mismatches 3. Added tests: path traversal rejection + namespace mismatch in write
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed82cc2ab5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…g in mergeSteps 1. read(): verify plan.namespace matches requested namespace to catch file corruption or misrouted reads 2. mergeSteps(): deduplicate incoming steps by tracking appended step texts in a Set, preventing duplicate step entries when incoming contains the same step text multiple times
Change from truthy check to explicit undefined check so that plans with no namespace field (backward-compat) pass through, while non-undefined mismatches still throw a corruption error.
|
Merge order: 8/8. See #66345 for the full merge sequence. |
Path traversal namespace like '../escape' was caught by the mismatch check (comparing against plan.namespace) before reaching validateNamespace(), producing a confusing error message instead of the security-relevant one. Swap the order so validateNamespace runs first.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88e188de43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Plan-mode rollout series — status updateThis PR is part of a 10-PR plan-mode rollout. The cumulative state ships locally as Series overview + landing order: see #68441 (latest comment). This PR's role in the series: see the original PR description above. Live test status: covered by the cumulative install — all behaviors from this PR are verified in production-like usage on Telegram + webchat. No regressions detected since the live install (2026-04-18). |
📋 Architecture & Status — single source of truth (commit
|
|
To use Codex here, create an environment for this repo. |
| // Atomic write: write to a temp file in the same directory, then rename. | ||
| const tmpFile = path.join(dir, `.plan-${crypto.randomBytes(4).toString("hex")}.tmp`); | ||
| try { | ||
| await fs.writeFile(tmpFile, JSON.stringify(plan, null, 2), { | ||
| encoding: "utf-8", | ||
| mode: 0o600, | ||
| }); | ||
| await fs.rename(tmpFile, planFile); | ||
| } catch (err) { |
There was a problem hiding this comment.
The atomic write path creates tmpFile and writes it via fs.writeFile(tmpFile, ...). This follows symlinks and doesn’t use exclusive create, so an attacker (or another process) could pre-create tmpFile (including as a symlink) and cause the write to clobber an unintended target, and there’s also a low-probability name-collision risk. Prefer opening the temp file with exclusive creation (e.g., open with wx plus O_NOFOLLOW where supported), writing via the returned handle, and then renaming.
| // Atomic write: write to a temp file in the same directory, then rename. | |
| const tmpFile = path.join(dir, `.plan-${crypto.randomBytes(4).toString("hex")}.tmp`); | |
| try { | |
| await fs.writeFile(tmpFile, JSON.stringify(plan, null, 2), { | |
| encoding: "utf-8", | |
| mode: 0o600, | |
| }); | |
| await fs.rename(tmpFile, planFile); | |
| } catch (err) { | |
| // Atomic write: create a temp file securely in the same directory, write via | |
| // the opened handle, then rename into place. | |
| const tmpFile = path.join(dir, `.plan-${crypto.randomBytes(4).toString("hex")}.tmp`); | |
| let handle: fs.FileHandle | undefined; | |
| try { | |
| handle = await fs.open( | |
| tmpFile, | |
| fsConstants.O_WRONLY | fsConstants.O_CREAT | fsConstants.O_EXCL | fsConstants.O_NOFOLLOW, | |
| 0o600, | |
| ); | |
| const stat = await handle.stat(); | |
| if (!stat.isFile()) { | |
| throw new Error(`Temporary plan path is not a regular file: ${tmpFile}`); | |
| } | |
| await handle.writeFile(JSON.stringify(plan, null, 2), { | |
| encoding: "utf-8", | |
| }); | |
| await handle.close(); | |
| handle = undefined; | |
| await fs.rename(tmpFile, planFile); | |
| } catch (err) { | |
| if (handle) { | |
| try { | |
| await handle.close(); | |
| } catch { | |
| /* ignore */ | |
| } | |
| } |
|
Closing in favor of consolidated PR #68939. Rationale: this branch was 734 commits behind upstream/main and the The full iteration history + decision log lives in |
TL;DR
Persistent `PlanStore` for sharing plans across multiple sessions and agents, with file-level exclusive locking and stale-lock cleanup.
Tracking
What this PR does
Default behavior
No behavior change when unconfigured. Plans remain session-scoped. Cross-session coordination only activates when `planStore.namespace` is explicitly set.
Path traversal fix
`write()` now validates namespace BEFORE checking plan.namespace mismatch, ensuring security-relevant validation runs first.
Files changed
Dependencies
What follows