Repository navigation
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one consistency gap worth a look.
Reviewed changes
- Shared idle helper — adds
isPiIdleTitletopi-state-title-marker.ts, true for the legacyπ -prefix or a state-marker title whose status isidle. - Terminal-wait gate —
computeExplicitIdleStatusFromTitlenow callsisPiIdleTitle(title)in place of the inline legacy-prefix check, soπ > dirreads idle. ThedetectAgentStatusFromTitle(title) === 'idle'gate still rejectsπ !/π :first, so this only widens explicit-idle evidence. - Test — new
terminal-wait-pi-idle-title.test.tspins legacy idle, state-marker idle, and the two not-idle markers; discriminating because it fails onmainforπ > dir.
ℹ️ Renderer send path still uses the legacy-only prefix
src/renderer/src/lib/agent-send-title-status.ts is a second consumer of the same idle-evidence rule and still checks only title.startsWith('\u03c0 - '), so an OMP pane sitting at π > dir is classified idle by classifyTitleActivity but then rejected by isExplicitIdleSendTitle — the exact drift this shared helper prevents. It does not affect the wait fix in this PR, but the renderer file can import isPiIdleTitle the same way terminal-wait-detection.ts now does.
Technical details
# Renderer send-target gate left on the legacy prefix
## Affected sites
- `src/renderer/src/lib/agent-send-title-status.ts:8` — `PI_IDLE_PREFIX = '\u03c0 - '`
- `src/renderer/src/lib/agent-send-title-status.ts:34` — `title.startsWith(PI_IDLE_PREFIX)`
- consumed by `src/renderer/src/lib/running-agent-targets.ts:120` and `src/renderer/src/lib/notes-send-agent-targets.ts:39,47`
## Required outcome
- OMP state-marker idle titles (`\u03c0 > dir`, `OMP > dir`, wrapper-prefixed variants) should
read as send-ready idle the same way they now read as wait-ready idle.
## Suggested approach (optional)
- Replace the local constant + `startsWith` with the shared `isPiIdleTitle` from
`../../../shared/pi-state-title-marker`, mirroring `terminal-wait-detection.ts`.
## Open questions for the human
- Is the send path intentionally kept on the legacy rule, or is this just out of scope for the
#24068 wait fix? If intentional, a comment stating why would stop the next reader "fixing" it.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)📝 WalkthroughWalkthroughTerminal wait detection now recognizes Pi/OMP idle state-marker titles through Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to In a narrow case, a valid idle Pi/OMP title shown by the renderer may not be recognized as ready while the underlying OSC title was stale-cleared. The effect is a missed readiness signal, not data loss. Scope the guard to the OSC title before or soon after merge. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change distinguishes genuine idle titles from timer-generated ones. That distinction is preserved during live operation, but its behavior after restoration remains unresolved. The identified uncertainty is limited to the affected terminal. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Good catch, a |
There was a problem hiding this comment.
ℹ️ No blocking issues — the stale-cleared guard is sound. A few consistency notes below.
Reviewed changes
- Added a
lastOscTitleStaleClearedprovenance flag so aπ > dirsynthesized by main's 3s stale timer is no longer accepted as explicit idle evidence. - Wired the flag through the pty/leaf records, the renderer-publish rebuild, and the provider-generation reset.
- Guarded
hasExplicitIdleTitleandgetAdoptedPtyExplicitIdleStatusagainst stale-cleared idle titles, with tests for both an agent-emitted and a synthesizedπ > dir.
ℹ️ Nitpicks
getAdoptedPtyExplicitIdleStatus(orca-runtime-resolve-exit-waiters.ts:223) gates onpty.lastOscTitleStaleClearedbut tests a title returned bygetAdoptedPtyTitle(pty)— the leaf'spaneTitleor the tab title. The flag describespty.lastOscTitle, so if those sources diverge a genuine idle could be suppressed; the pty branch ofhasExplicitIdleTitle(same flag againstpty.lastOscTitle) is the consistent one.- The rationale at
tui-idle-evidence.ts:45-48still warns that a bespoke provenance field "is silently dropped on any renderer publish and the verdict silently flips". This commit now stamps exactly such a field and depends on the explicit copy atorca-runtime-sync-window-graph.ts:147; worth rewording so the premise reads true. applySeededAgentStatus(orca-runtime-maybe-hydrate-headless-from-renderer.ts:137,145) rewriteslastOscTitlewithout resettinglastOscTitleStaleCleared. I could not construct a reachable sequence that flips a genuine title to suppressed, but a one-line reset there preserves the invariant for future edits.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| @@ -69,6 +71,11 @@ export function hasExplicitIdleTitle( | |||
| // one dropped an explicit `Codex ready` to the tier-3 lane and delayed it by the | |||
| // whole quiescence window. | |||
| for (const title of [rendererTitle, record.lastOscTitle]) { | |||
| // Why: main's 3s stale timer rewrites OMP's static `π : dir` to `π > dir` mid-turn; only a | |||
| // `π > dir` the agent emitted itself is idle evidence. | |||
| if (record.lastOscTitleStaleCleared && title && getPiStateTitleStatus(title) === 'idle') { | |||
There was a problem hiding this comment.
Please confirm the legacy exemption is intentional. The stale timer also synthesizes the legacy idle form — clearWorkingIndicators('⠋ π - cwd') returns π - cwd — but this guard only matches the state-marker form via getPiStateTitleStatus(title) === 'idle'. The identical predicate sits at orca-runtime-resolve-exit-waiters.ts:223.
Technical details
# Legacy `π - ` idle still counts as explicit idle when stale-cleared
## Affected sites
- `src/main/runtime/tui-idle-evidence.ts:76` — guard matches only `getPiStateTitleStatus(title) === 'idle'`
- `src/main/runtime/orca-runtime-resolve-exit-waiters.ts:223` — same predicate
## Required outcome
- Decide whether a stale-timer-synthesized legacy idle title (`π - cwd`, produced when `clearWorkingIndicators` strips the braille off `⠋ π - cwd`) should be excluded the same way `π > cwd` now is.
## Suggested approach (optional)
- If it should: gate on `isPiIdleTitle(title)` in both guards, and flip the `terminal-wait-pi-idle-title.test.ts` case that currently asserts legacy stays idle when stale-cleared.
- If legacy is intentionally exempt: a one-line comment stating why would stop the next reader "fixing" it.
## Open questions for the human
- Is legacy Pi's stale-clear the intended fallback idle signal while the state-marker stale-clear is not? `pi-state-title-marker.test.ts:42` shows `⠋ π - my-project` classifies as working and is cleared to the `π - ` idle form.
…al wait terminal.wait --for tui-idle only accepted the legacy 'π - ' idle prefix, so a Pi/OMP pane titled 'π > dir' (state-marker idle) was never seen as ready. Add isPiIdleTitle next to the marker table so both Pi title protocols are read from one place. Fixes stablyai#24068.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2feeb29 to
b28465c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 973ebc9e-eee4-44fa-ab38-0eb0701d3451
📥 Commits
Reviewing files that changed from the base of the PR and between 2feeb292c6b9dd70d79af3935d6ac34a7b1e5de0 and b28465c.
📒 Files selected for processing (3)
src/main/runtime/orca-runtime-apply-tracked-pty-title.tssrc/main/runtime/terminal-wait-detection.tssrc/main/runtime/tui-idle-evidence.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
There was a problem hiding this comment.
ℹ️ No new issues — the two commits were force-pushed onto
main's rule-file readiness refactor and re-applied cleanly.
Reviewed changes
- Rebased
isPiIdleTitle(π >state-marker idle) ontoterminal-wait-detection.ts's new rule-file engine. The explicit-idle gate still evaluates before the rule lane, and no rule-file title anchor matchesπ >(onlypi.json's^π -), so the helper is still required and not redundant. - Re-applied the
lastOscTitleStaleClearedguard throughhasExplicitIdleTitle,getAdoptedPtyExplicitIdleStatus, and the leaf rebuild. Verified the refactor's newreadAgentRuleVerdictweak lane cannot promote a stale-clearedπ >to strong idle (the Pi/OMP title rule isweak+requiresQuiet), andgetAdoptedPtyIdleStatusmaps to the guardedgetAdoptedPtyExplicitIdleStatus. - Confirmed the added test remains discriminating and the related suites pass:
terminal-wait-pi-idle-title.test.ts,tui-idle-evidence.test.ts,agent-state-rules-engine.test.ts— 113 passed.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
ELI5
OMP 18.4's idle title
π > dirwas not seen as idle, soworker-start --agent omptimed out.What Changed
computeExplicitIdleStatusFromTitlenow accepts the state-marker idle title via a newisPiIdleTitleinpi-state-title-marker.ts(legacy prefix, orgetPiStateTitleStatus(title) === 'idle').π :andπ !stay not idle.Why
Reuses the marker table;
terminal-wait-detection.tsis at itsmax-lineslimit.Linked Issue
Fixes #24068
Visual Proof
N/A, no UI change.
Testing
terminal-wait-pi-idle-title.test.ts, viapnpm exec vitest run --config config/vitest.config.ts <file>:main1 failed / 3 passed, fix 4 passed; related suites 330 passed.tsc(node, web),oxlint, anti-slop,oxfmt --check, max-lines ratchet: clean. Linux only.AI Disclosure
Written with Claude Code; reviewed and tested by me.
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Title check only; no platform impact.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)🤖 Generated with Claude Code