Skip to content

fix(pi): harden resume target handling#224

Merged
vakovalskii merged 1 commit into
vakovalskii:mainfrom
timseriakov:fix/pi-resume-review-hardening
May 26, 2026
Merged

fix(pi): harden resume target handling#224
vakovalskii merged 1 commit into
vakovalskii:mainfrom
timseriakov:fix/pi-resume-review-hardening

Conversation

@timseriakov
Copy link
Copy Markdown
Contributor

@timseriakov timseriakov commented May 25, 2026

Summary

Follow-up to #221. The Pi / Oh My Pi support PR was merged while a later independent review comment was still being handled, so this PR carries the review hardening fixes as a focused patch instead of rewriting the merged history.

Why

The merged feature works, but the review found several places where the new Pi resume-target flow was broader than necessary or could be made safer:

  • non-Pi Resume buttons were routed through the Pi-specific helper by mistake
  • /api/launch accepted a client-provided resumeTarget and then forwarded it to terminal launch more broadly than needed
  • Pi JSONL header.id was trusted before validating it as a safe session id
  • symlinked JSONL entries inside Pi/OhMyPi session directories could be scanned, even though the configured/root agent directory itself may legitimately be a symlink
  • the Settings detection label and Copy Resume command did not consistently use the Pi vs Oh My Pi session variant

What changed

  • Scope launchPiSession to tool === 'pi'; all other agents use the regular launchSession path again.
  • Replace boolean resume-target validation with getValidatedPiResumeTarget(...), which returns only the server-discovered, resolved JSONL path after validation.
  • Pass project into findSessionFile(sessionId, project) during Pi resume-target validation to avoid cold-cache misses.
  • Forward a resume target to openInTerminal only when it is the validated Pi target; non-Pi tools get an empty target.
  • Reject symlinked JSONL session entries at Pi resume validation time and skip symlinked JSONL entries during scanning/cache fingerprinting; configured/root agent directories such as ~/.omp/agent -> ... still work because only entries inside sessions/ are skipped.
  • Validate Pi JSONL header.id with the safe session id regex before indexing.
  • Fix Settings detection label to Pi/OhMyPi.
  • Rename Pi/OhMyPi mtime cache variables so the name matches what is tracked.
  • Make Copy Resume choose omp --resume vs pi --session from session.agent_variant instead of installed-binary detection.
  • Use a static agent not installed launch error message.

Test plan

  • node --check src/server.js && node --check src/frontend/detail.js && node --check src/data.js && node --check src/frontend/app.js
  • node --test test/pi-session.test.js test/frontend-escaping.test.js test/agents-detect.test.js test/terminals-windows-launch.test.js → 31 pass, 0 fail
  • node --test → 146 pass, 0 fail, 2 skipped

@timseriakov timseriakov force-pushed the fix/pi-resume-review-hardening branch from 4e2f3e7 to a4c435b Compare May 25, 2026 18:08
Copy link
Copy Markdown
Owner

@vakovalskii vakovalskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✅ Right cleanup — these fall out cleanly because Pi was the only resumeTarget consumer at the time of #221.

Verified locally:

  • Full suite 148/0/2 pass (3 new tests in pi-session.test.js, 2 in frontend-escaping, 1 in agents-detect)
  • CI 5/6 green + 1 cancelled (re-running)

Squash-merging — will ride in 7.5.1.

@vakovalskii vakovalskii merged commit 77dfd8c into vakovalskii:main May 26, 2026
5 of 6 checks passed
@vakovalskii vakovalskii mentioned this pull request May 26, 2026
3 tasks
vakovalskii added a commit that referenced this pull request May 26, 2026
Patch — picks up the design port (#225) and Pi resume-target hardening
(#224) shipped since 7.5.0.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants