feat(pi): add Pi and Oh My Pi support#221
Conversation
Adds Pi and Oh My Pi as first-class agents instead of treating them as a combined sidebar entry. The sidebar exposes separate Pi and Oh My Pi filters, while leaderboard aggregation keeps their stats distinct and renders lowercase badges only in the leaderboard context. Resume reliability: - Scan Pi and Oh My Pi session directories recursively enough to find canonical nested ~/.omp/agent/sessions/<project> JSONL files. - Store resume_target on scanned sessions and use the JSONL path for Pi and Oh My Pi copy, CLI, and UI resume commands. - Pass resumeTarget through /api/launch separately from sessionId so terminal tracking remains keyed by safe IDs while Pi and Oh My Pi resumes can use validated session file paths. - Quote resume targets before passing them to pi/omp launch commands. Analytics/UI: - Split leaderboard agent keys for Pi vs Oh My Pi instead of aggregating under pi. - Remove the combined Pi/OhMyPi install/sidebar item; keep Pi and Oh My Pi as separate sidebar entries. - Keep proper display casing outside Leaderboard and lowercase badges inside Leaderboard. Tests: - node --test - Manual: omp --resume <jsonl path> --print 'respond ok'
6314043 to
1618b5b
Compare
vakovalskii
left a comment
There was a problem hiding this comment.
LGTM ✅ Welcome to the project @timseriakov — really solid first contribution!
Verified:
node --test test/pi-session.test.js→ 11/11 pass- Full suite locally: 144 pass / 0 fail / 2 skipped (up from 127 baseline, +17 new tests across pi-session + extensions)
- Diff stat against fresh
main— no unexpected deletions, additive across all 24 files - Squash-merging now since
mergeable=MERGEABLEand #220 already landed
Security spot-checks:
quotePosixArgcorrectly POSIX-escapes ('value'with internal'→'\\''— the standard one)isValidPiResumeTargethas a defense-in-depth chain: sessionId regex →.jsonlsuffix → reject['\$\\n\r\0]→ **resolved path must equalfindSessionFile(sessionId).file**. The last check is the killer — even if a craftedresumeTarget` slipped past the regex, it has to point at the same file codbash discovered during scan. Attacker can't smuggle in arbitrary paths.- Server now refuses unknown-but-not-detected agents (
agent not installed: <tool>) — closes a small gap where an unknown agent could fall back toclaudesilently - Pi-specific resume target only flows when
isValidPiResumeTargetreturns true; otherwise the existing safe-sessionId path is taken
Design notes I like:
agent_variantdistinguishes Pi from Oh My Pi while sharing thepitool integration — clean way to avoid duplicating scannersresume_targetseparated fromsessionIdso PID-tracking + window-tagging stay simple while resumes can target the exact JSONL file- The companion leaderboard PR is exactly the right way to keep cross-repo style consistent
Thanks again!
|
Approved above and tested locally (144/0/2). This PR is in Draft state though — could you flip it to Ready for review? After that I'll merge immediately. GitHub's CLI also won't run the CI matrix while it's in Draft, so the green checks need to land before the squash anyway. |
Code Review + Security Review — PR #221@timseriakov impressive first contribution — 24 files, clean patterns, solid test coverage. Two independent reviews (code + security) found findings below. The owner's security spot-checks on HIGH (Code)1. All Resume buttons route through In Fix: Scope if (s.tool === 'pi') {
// launchPiSession(...)
} else {
// launchSession(...)
}2. In HIGH (Security)3. TOCTOU between The validation resolves and compares the path at validation time. Between validation and Mitigations (pick one):
4. After the guard check, line 214 passes openInTerminal(..., fresh ? '' : (resumeTarget || ''));For non-Pi tools, Fix: 5.
Fix: Validate against MEDIUM6. Agent label mismatch in
7. Variable name suggests OhMyPi-only but accumulates mtimes for both 8. The 9. Session ID collision: Pi IDs not namespaced
10. The full path (e.g., 11.
12. Environment variables ( If an attacker controls these env vars, they redirect session scanning to arbitrary filesystem paths. Validate resolved paths are under LOW13.
14. if (token[0] === '"' && token[token.length - 1] === '"') {
if (token[0] === "'") return body.replace(...); // unreachableThe inner 15. A user with both 16. The ERE 17. The 18. A comment explaining the version bump reason would help future maintainers. Summary
The most actionable items before merge:
Also: the owner noted this is still in Draft — flip to Ready for Review when the fixes land. |
|
Update: this review-fix commit was pushed after #221 had already been merged, so it is not part of the merged PR. I moved the fixes into a dedicated follow-up PR instead: That follow-up PR explains why it exists and contains the hardening fixes from the post-merge review:
Verification recorded in #224:
|
Summary
Adds first-class Pi and Oh My Pi support to Codbash.
Pi and Oh My Pi (Fork of Pi) sessions now show up in the dashboard alongside the existing agents, can be searched, previewed, opened in detail, resumed from the UI/CLI, included in analytics, and shown separately in leaderboard stats.
Companion PR
What's new
piand Oh My Pi viaomp, with Settings detection metadata and tests.pi --sessionfor Pi,omp --resumefor Oh My Pi) and launches terminal resumes through the existing/api/launchflow.PiandOh My Pifilters in the Agents section and Sidebar settings.Implementation details
Backend
src/data.js.agent_variantso Pi and Oh My Pi can share thepitool integration while remaining distinguishable in filters and analytics.resume_targetfor JSONL-backed sessions so resumes can target the exact session file.Frontend
Pi/Oh My Pi; leaderboard badges remain lowercase (pi,ohmypi) to match existing leaderboard style.Tests
test/pi-session.test.js.Test plan
node --testnode --test test/pi-session.test.js test/sidebar-config.test.js test/frontend-escaping.test.js/api/sessionsreturns Pi and Oh My Pi JSONL sessions withresume_target.sessionIdplus JSONLresumeTargetto/api/launch(fetch intercepted; no real terminal/agent launched).piandohmypirows with violet badge styling.Verified locally
node --test→ 142 pass, 0 fail, 2 skippednode --test test/pi-session.test.js test/sidebar-config.test.js test/frontend-escaping.test.js test/terminals-windows-launch.test.js→ 55 pass, 0 failpi --session <jsonl path> --print 'respond ok'→okomp --resume <jsonl path> --print 'respond ok'→ observedrespond okFiles
README.mddocs/ARCHITECTURE.mdbin/cli.jssrc/agents-detect.jssrc/data.jssrc/frontend/analytics.jssrc/frontend/app.jssrc/frontend/calendar.jssrc/frontend/detail.jssrc/frontend/heatmap.jssrc/frontend/index.htmlsrc/frontend/leaderboard.jssrc/frontend/sidebar-config.jssrc/frontend/styles.csssrc/server.jssrc/settings.jssrc/terminals.jstest/agents-detect.test.jstest/frontend-escaping.test.jstest/pi-session.test.jstest/settings.test.jstest/sidebar-config.test.jstest/terminals-windows-launch.test.jstest/wsl-windows.test.js