fix(client): persist-empty guard + MCP new-tab resume alias#518
Merged
Conversation
… tabs Root cause: loadInitialTabsState() falls back to an empty tabs array whenever the persisted layout can't be turned into a usable state (corrupt JSON, a thrown exception during load, or every tab getting pruned by sanitizeTabsAgainstLayouts because its pane layout was missing). That in-memory emptiness was never distinguished from a genuine "the user has no tabs" state. persistMiddleware's flush() writes the tabs section as part of one combined atomic layout write whenever EITHER tabs or panes are dirty -- so a later panes-only (or activeTabId-only) action would silently overwrite a perfectly good, non-empty persisted layout with an empty one, permanently destroying the user's tab layout. Fix: track whether this session's initial tabs load was a recovery from a failed/partial read (markTabsLoadRecovery/wasTabsLoadRecovery in persistMiddleware.ts, set from loadPersistedLayout()'s failure paths and from tabsSlice.ts's own shape/sanitize/exception failure paths). persistMiddleware's flush() distrusts an empty in-memory tabs array under that condition and skips the write rather than persist it over a non-empty layout already on disk. Trust is restored the moment real tab content is observed in the session, so a genuine user action that later empties tabs (closing the last tab) is still persisted normally -- this guard only blocks writes that were never confirmed by real activity. Tests: new test/unit/client/store/persistTabsEmptyGuard.test.ts covers (1) a transient parse failure followed by an unrelated tabs action no longer overwrites the on-disk layout, (2) a genuine close-all-tabs still persists an empty layout (no regression), and (3) the normal flush path is unaffected. Full test:unit (3754 tests) and test:integration (474 tests) suites pass with no regressions. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Root cause: the MCP "freshell" tool's new-tab action schema only recognized the shorthand `resume` param, rejecting `resumeSessionId` outright as an "Unknown parameter" -- even though POST /api/tabs (the REST route new-tab wraps) accepts a top-level `resumeSessionId` field, the CLI sends exactly that field name (server/cli/index.ts), and it's the exact field name the server itself returns/broadcasts on created panes (server/agent-api/router.ts paneContent.resumeSessionId). Agents naturally echo that field name back on a subsequent new-tab call and hit a hard rejection. Fix: accept `resumeSessionId` as an alias for `resume` in the new-tab action's parameter schema (ACTION_PARAMS) and handler. Both names resolve through the same canonical-sessionRef derivation and raw-Codex-resume rejection guard `resume` already used -- the raw legacy field is still never forwarded over the wire, preserving the existing "resumeSessionId is legacy, sessionRef is canonical" invariant already covered by the "maps resume to canonical sessionRef instead of legacy resumeSessionId" test. This is deliberately NOT a raw pass-through of resumeSessionId to the REST body: doing that would have reintroduced the legacy field this codebase already guards against. Tests: added two cases to test/unit/server/mcp/freshell-tool.test.ts -- resumeSessionId maps to canonical sessionRef exactly like resume, and raw Codex resumeSessionId is rejected by the same guard as raw Codex resume. Full test:integration suite (474 tests, includes this file) passes with no regressions. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
![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: ec4aa18869
ℹ️ 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".
Comment on lines
+508
to
+510
| if ((state.tabs?.tabs?.length ?? 0) > 0) { | ||
| distrustEmptyTabs = false | ||
| } |
There was a problem hiding this comment.
When rehydration fails or sanitization removes every tab, src/App.tsx:1420-1424 automatically dispatches addTab({ mode: 'shell' }) on mount. That fallback makes this condition clear distrustEmptyTabs on the first flush, which then overwrites the existing persisted layout with the new shell tab; consequently, the guard does not prevent the production data-loss scenario it targets. Recovery must distinguish this automatic fallback from genuine user activity rather than trusting any nonempty tab state.
Useful? React with 👍 / 👎.
danshapiro
pushed a commit
that referenced
this pull request
Jul 22, 2026
Fold the seven required changes from the implement-with-changes council verdict (unanimous, 2 rounds, 6 lenses) into the affected sections: 1. Capability-gated single-flight create-dedupe (v1, sec 5.4) closes the two-tab double-respawn data-loss blocker; correct sec 7's inverted 'reduces its window to near-zero' claim (handshake SYNCHRONIZES the two creates); add >=2-live-PTY backstop + two-concurrent-connection e2e. 2. Row-1 lookup extended to exited generations; recover retired identity before declaring fresh; dead_session gated on identity-ever-observed; add 'both live' decision-table row (I6). 3. Write-ahead stamp respecified as atomic-with-registry-insert; replace test 9.1.5 with insert-edge interleave; named spawn-failed exclusion. 4. Bound the unknowns: retry mechanism OPEN (USER DECISION PENDING, both options fair); respawn-generation cap (sec 7.5); exists() semantics + one real-index staleness test. 5. createRequestId as enforced contract; document persistMiddleware.ts:229 re-mint + REST-ingress-mints-none violations; Assumption-1 REST identity crate-boundary gap (registry.rs:273-278) as Phase-1 acceptance check. 6. Commit the finish: Phase-3 trigger (9.2 green), always-run CI, DoD from assessment user-outcome metrics, interim posture, deviation-budget arithmetic (#516/#518), dead-code deletions into Phase-3 acceptance, Incident-4 rescoped to 'reinforces closure' (cite 80772ff), anchors re-verified at HEAD (fix drifted registry.rs:631). 7. Named user-facing invariants (sec 8.1): I6 no-silent-identity-replacement, I7 exit affordances for dead_session and respawn-exhausted terminal state. Add sec 0 Council Review section (verdict, headline debate, standing tradeoff). Doc-only revision; no code or behavior change. Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
danshapiro
pushed a commit
that referenced
this pull request
Jul 24, 2026
Brings the port branch up to date with main (51 commits, through a853ce0) ahead of landing the Rust/Tauri port on main. Conflict resolutions (5 files, all client-side; port side kept): - src/store/persistMiddleware.ts + test/unit/client/store/ persistTabsEmptyGuard.test.ts: port's stateless persist-empty guard v2 (rolling backup + user-close intent) supersedes main's one-shot v1 from #518; the v1-only `distrustEmptyTabs` disarm block was removed. Port test file is a strict superset of main's (same 3 tests + 2 more). - src/store/persistedState.ts + persistedState.test.ts: port's salvage behavior (sanitize invalid mode/codingCliProvider to undefined, drop only corrupt tabs) supersedes main's strict min(1) schema. - src/components/TerminalView.tsx: comment-only; port's non-destructive restore-flag peek comment matches the port's actual behavior. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two client-side fixes bundled into one branch off
origin/main.1. Persist guard — data-loss landmine
Fixes a proven data-loss path surfaced by a production incident investigation (2026-07-18): a transient tab-load failure (a parse error / throw during rehydration) previously became a permanent empty-layout write on the very next persist flush, silently destroying the user's tabs.
2. MCP new-tab
resumeSessionIdaliasThe freshell MCP tool's
new-tabaction rejectedresumeSessionIdeven though the REST route and CLI already accept it. It is now accepted and forwarded, aligning the MCP surface with the other entry points.Test plan
Generated with Amplifier