fix(resume-brief): exclude live sessions, show session id, stay silent when clean#227
Open
khustup2 wants to merge 9 commits into
Open
fix(resume-brief): exclude live sessions, show session id, stay silent when clean#227khustup2 wants to merge 9 commits into
khustup2 wants to merge 9 commits into
Conversation
Every goal/KPI write went through an UPDATE that set created_at = now, clobbering the original creation timestamp. Because the listing and VFS bootstrap order by created_at DESC (and KPIs by created_at ASC), any status move or content edit silently reset the timestamp and bumped the row to the top — reshuffling the list on every transition. Add a dedicated updated_at column (auto-healed onto existing tables) and record edit time there, leaving created_at untouched. INSERT paths now populate both. Covers all write sites: VFS upsertGoalRow/upsertKpiRow, the goal/kpi CLI, and the openclaw write-side tools. Adds a regression test asserting a status transition writes updated_at and never touches created_at.
…t when clean
The SessionStart pickup banner surfaced sessions still open in another
terminal, and dressed up finished sessions ("None" next steps) as open work.
- Liveness via the owning process, not an event heartbeat. SessionStart records
the owning `claude` process (/proc walk: pid+comm+starttime) to <sid>.owner;
capture self-heals already-open sessions on their next event. isSessionLive:
ended-marker -> not live; else owner alive->live / dead->not (catches crashes
too); unknown -> fall back to the event-heartbeat window. An idle-but-open
session keeps its process, so it stays excluded.
- resume-brief excludes the current session and any live session before
windowing; shows the surfaced session's id (copy-pasteable for --resume).
- Treat "None - <prose>" / "N/A, ..." Next Steps as wrapped-clean (not just a
bare "none"), without matching real work like "None of the tests pass".
- Stay silent (no banner) when no recent session has open work, instead of a
"nothing pending" line or reaching back to a stale TODO.
Linux-only liveness (/proc); non-Linux degrades to the heartbeat window.
Known limit: local-only - a session on another machine can't be detected.
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 16 files changed
Generated for commit 65a40d7. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements session-aware timestamp tracking and liveness detection. Goals and KPIs tables gain ChangesSession Liveness and Timestamp Lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/session-start.ts`:
- Around line 145-151: When starting/resuming a session, also update the session
activity timestamp so the non-Linux mtime fallback used by isSessionLive() /
activeWindowMs() marks the resumed session as live; specifically, in the
SessionStart block after clearSessionEnded(input.session_id) and
recordSessionOwner(input.session_id) add a call to the helper that updates the
session activity mtime (e.g. touchSessionActivity or
updateSessionActivityTimestamp) for input.session_id — if that helper does not
exist, implement it to persistently update the session’s activity timestamp in
the same state storage isSessionLive() reads.
In `@src/notifications/sources/resume-brief.ts`:
- Around line 107-112: The EMPTY_SECTION regex in resume-brief.ts currently
treats "TBD" with trailing punctuation as empty, suppressing real work in
extractNextSteps(); update the EMPTY_SECTION pattern so that only
none/n\/?a|n\.a\.|nothing(?: pending)? (and the punctuation-only dashes) keep
the relaxed "trailing-clause" group, but match "tbd" only as a standalone token
(no trailing-clause allowed). In practice, refactor the alternation so the (tbd)
branch does not share the \s*(?:[—–\-.,;:].*)? suffix while the other branches
keep it, leaving extractNextSteps() behavior unchanged for the other empty
tokens.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 54444424-19ec-41c6-bca0-9d4a16118d9c
📒 Files selected for processing (17)
openclaw/src/index.tssrc/commands/goal.tssrc/deeplake-schema.tssrc/hooks/capture.tssrc/hooks/session-end.tssrc/hooks/session-start.tssrc/hooks/summary-state.tssrc/notifications/sources/primary-banner.tssrc/notifications/sources/resume-brief.tssrc/shell/deeplake-fs.tstests/claude-code/capture-hook.test.tstests/claude-code/cli-goal.test.tstests/claude-code/deeplake-fs.test.tstests/claude-code/resume-brief.test.tstests/claude-code/session-end-hook.test.tstests/claude-code/summary-state.test.tstests/openclaw/hivemind-tools.test.ts
Comment on lines
+145
to
+151
| // A fresh start or --resume of this session re-activates it: drop any stale | ||
| // ended marker and record the owning `claude` process so other sessions can | ||
| // tell this one is live even while it sits idle waiting on the user. | ||
| if (input.session_id) { | ||
| clearSessionEnded(input.session_id); | ||
| recordSessionOwner(input.session_id); | ||
| } |
There was a problem hiding this comment.
Re-arm the heartbeat fallback on SessionStart.
On non-Linux, Line 150 is a no-op, so isSessionLive() falls back to the existing state-file mtime. If the user resumes an older session and then stays idle, that mtime can already be outside activeWindowMs(), and another terminal can immediately surface this still-open session in the resume banner until the first new event arrives. Touch the session’s activity timestamp here as well so the documented non-Linux fallback actually marks resumed sessions live.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/session-start.ts` around lines 145 - 151, When starting/resuming a
session, also update the session activity timestamp so the non-Linux mtime
fallback used by isSessionLive() / activeWindowMs() marks the resumed session as
live; specifically, in the SessionStart block after
clearSessionEnded(input.session_id) and recordSessionOwner(input.session_id) add
a call to the helper that updates the session activity mtime (e.g.
touchSessionActivity or updateSessionActivityTimestamp) for input.session_id —
if that helper does not exist, implement it to persistently update the session’s
activity timestamp in the same state storage isSessionLive() reads.
Comment on lines
+107
to
+112
| /** Treat these as "nothing left" even when the section is present. Matches the | ||
| * bare tokens ("None", "N/A", "TBD") AND a token followed by a trailing clause | ||
| * introduced by punctuation or a dash ("None — feature complete", "N/A, all | ||
| * shipped", "Nothing pending."). A token followed by a *word* ("None of the | ||
| * tests pass yet") is real open work and deliberately does NOT match. */ | ||
| const EMPTY_SECTION = /^(none|n\/?a|n\.a\.|nothing(?: pending)?|tbd|—|–|-)\s*(?:[—–\-.,;:].*)?$/i; |
There was a problem hiding this comment.
Don't classify TBD: or TBD - … as wrapped-clean.
Line 112 now treats any TBD followed by punctuation as empty, so extractNextSteps() will suppress real open work like TBD: finish rollback handling or TBD - wire the migration. That makes the resume banner go silent for sessions that still have pending work. Keep the trailing-clause relaxation for none/n/a/nothing pending, but only treat TBD as empty when it stands alone.
Suggested fix
-const EMPTY_SECTION = /^(none|n\/?a|n\.a\.|nothing(?: pending)?|tbd|—|–|-)\s*(?:[—–\-.,;:].*)?$/i;
+const EMPTY_SECTION =
+ /^(?:(?:none|n\/?a|n\.a\.|nothing(?: pending)?)(?:\s*(?:[—–\-.,;:].*)?)?|tbd|—|–|-)$/i;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Treat these as "nothing left" even when the section is present. Matches the | |
| * bare tokens ("None", "N/A", "TBD") AND a token followed by a trailing clause | |
| * introduced by punctuation or a dash ("None — feature complete", "N/A, all | |
| * shipped", "Nothing pending."). A token followed by a *word* ("None of the | |
| * tests pass yet") is real open work and deliberately does NOT match. */ | |
| const EMPTY_SECTION = /^(none|n\/?a|n\.a\.|nothing(?: pending)?|tbd|—|–|-)\s*(?:[—–\-.,;:].*)?$/i; | |
| /** Treat these as "nothing left" even when the section is present. Matches the | |
| * bare tokens ("None", "N/A", "TBD") AND a token followed by a trailing clause | |
| * introduced by punctuation or a dash ("None — feature complete", "N/A, all | |
| * shipped", "Nothing pending."). A token followed by a *word* ("None of the | |
| * tests pass yet") is real open work and deliberately does NOT match. */ | |
| const EMPTY_SECTION = | |
| /^(?:(?:none|n\/?a|n\.a\.|nothing(?: pending)?)(?:\s*(?:[—–\-.,;:].*)?)?|tbd|—|–|-)$/i; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/notifications/sources/resume-brief.ts` around lines 107 - 112, The
EMPTY_SECTION regex in resume-brief.ts currently treats "TBD" with trailing
punctuation as empty, suppressing real work in extractNextSteps(); update the
EMPTY_SECTION pattern so that only none/n\/?a|n\.a\.|nothing(?: pending)? (and
the punctuation-only dashes) keep the relaxed "trailing-clause" group, but match
"tbd" only as a standalone token (no trailing-clause allowed). In practice,
refactor the alternation so the (tbd) branch does not share the
\s*(?:[—–\-.,;:].*)? suffix while the other branches keep it, leaving
extractNextSteps() behavior unchanged for the other empty tokens.
- Record the session owner from PreToolUse (synchronous, reliable `claude` ancestor) in addition to SessionStart. The async capture hook can be detached and unable to /proc-walk to claude, so a session already open when this shipped only got an owner record late or not at all — and an idle-but-live session past the heartbeat window was wrongly surfaced. The synchronous hook records it on the session's next tool call; no-op once set. - Surface up to MAX_BRIEF_SESSIONS (2) sessions with open work, newest-first, each as a `📌 <next>` + `↳ session <id> · <age>` block under one CTA. Still silent when nothing has open work.
…ions
When a session's core work was substantive (feature, fix, investigation,
design), the wiki summarizer now treats administrative wrap-up — commit,
push, open/merge PR, deploy, monitor CI — as ALREADY DONE and writes "none"
instead of surfacing it as the next step. So the resume brief stops echoing
procedural closure noise ("Push the branch", "Monitor PR checks") and lets
genuine forgotten work rise. Administrative actions still count when the
session's core purpose WAS the release/ops task. Applied to all four agent
prompt variants (claude / cursor / hermes / codex).
A `claude --resume` (source="resume") already puts the prior thread in front of the user, so the welcome / "where you left off" banner is pure noise. Thread the SessionStart `source` through session-notifications → drainSessionStart → pickPrimaryBanner and return null when source==="resume". Unknown/absent source is treated as a fresh startup so older Claude Code versions that don't send the field keep the banner.
…rth resuming Strengthens the admin-suppression rule: the summarizer no longer mechanically emits a next step. It now defaults to "none" and only names one when the session left genuinely substantive, non-obvious work a returning engineer would actually need flagged — judging whether it's worth mentioning. Natural stopping points, completed work, trivial/obvious follow-ups, open-ended exploration, and administrative wrap-up (commit/push/PR/deploy/CI) all resolve to "none". Stops the resume brief from surfacing procedural noise. Applied to all four agent prompt variants (claude / cursor / hermes / codex).
The pickup query fetched 40 full summaries every SessionStart; most are SessionStart placeholders. As history grew it climbed to ~2.6s and started losing the race against the 3s cap — the banner silently vanished. - Exclude placeholders in SQL (description <> 'in progress'); isPlaceholderSummary still guards in code for any non-standard placeholder. - Drop SCAN_LIMIT 40 -> 20 (the big cushion only existed to skip placeholders). - Raise the query cap 3s -> 4s and the session-notifications hook timeout 5s -> 8s for cold-spike headroom. Measured locally: ~2612ms -> ~550-720ms (~4x), same two real sessions surfaced.
…beat on start - EMPTY_SECTION no longer swallows "TBD: <work>" / "TBD - <work>" as wrapped-clean; only a bare "TBD" counts as empty (none/n-a/nothing-pending keep the trailing-clause relaxation). Real pending work surfaces again. - SessionStart now calls touchSessionActivity() so the non-Linux mtime fallback in isSessionLive() marks a freshly started/resumed session live immediately, before its first captured event (on Linux the owner record already covers this). Seeds empty state without touching periodic counters. - Tests for both, plus recordSessionOwner no-match and ensureSessionOwner no-op-when-exists — summary-state branch coverage 82% -> 86%.
Tests-only. The PR coverage comment showed changed-files branches at 89.3% (<90%). Add coverage for: - session-end recordSessionUsage: transcript with/without memory searches and the parse-error catch (was the biggest gap at 63% branch). - resume-brief: empty-session-id paths (excludeActiveSessions + pathless summaries, with and without an age line) and the default table/apiUrl/ workspaceId fallbacks. - summary-state ownerLiveness: dead-on-comm-mismatch. PR-scope branch coverage now ~90.5%.
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
1 participant
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
Four fixes to the SessionStart pickup banner so it only surfaces genuine open work and never points at a session you're still in.
/procwalk (pid + comm + starttime) recorded at SessionStart. An idle-but-open session keeps its process, so it stays excluded; a crashed session (dead process) becomes eligible immediately; already-open sessions self-heal on their next event.↳ session <uuid>in the banner, copy-pasteable forclaude --resume <id>.None — feature complete,N/A, all shipped,Nothing pending.) count as clean; a realNone of the tests passstill surfaces.Caveats
/proc); other platforms degrade to the heartbeat window.Testing
Full suite: 3766 passing. Built into the bundle and verified live (
sidLinepresent, arrow escaped to↳,EMPTY_SECTIONupdated, sid carried throughselectRealSummaries).Summary by CodeRabbit
New Features
Bug Fixes