fix(hooks): adopt a buddy_react reaction written under another session id - #168
Conversation
…n id
buddy_react fires, buddy_stats reports Source: tool with the model-written
text, and the statusline bubble still shows a canned pool line. Observed
live:
reaction.4758cc43.json source: tool "*ears twitch* thirty-one..."
reaction.d26d5a00.json source: fallback "..."
The MCP server is long-lived and resolves BUDDY_SID once at launch; the
hook and the statusline resolve it per invocation. When those diverge,
buddy_react writes a real reaction into a file nothing renders, this hook
finds an empty file for *its* session, and overwrites the bubble with the
pool — indistinguishable from the reactions being fake, which is the exact
complaint the provenance work set out to answer.
The guard now looks for a fresh tool reaction across every session file
rather than only its own, and adopts it into this session's file so the
statusline actually renders it. Ours still wins when both are fresh.
Found only because of the field: in one file,
in the other.
- server/hooks/buddy-comment.ts
- server/hooks/buddy-comment.test.ts
Fixes #167
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughThe stop hook now finds fresh tool reactions across session-specific files, adopts them into the active session when necessary, preserves their provenance, and prevents fallback overwrites. A cross-session test verifies the adopted reaction remains readable and unchanged. ChangesCross-session reaction handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant StopHook
participant ReactionDiscovery
participant SessionFiles
participant StopMarker
StopHook->>ReactionDiscovery: find fresh tool reaction
ReactionDiscovery->>SessionFiles: scan reaction.*.json files
SessionFiles-->>ReactionDiscovery: return freshest valid reaction
StopHook->>SessionFiles: adopt reaction for current session
StopHook->>StopMarker: update marker timestamp
StopHook-->>StopHook: return source "tool"
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (freshTool) { | ||
| // Adopt it into this session's file so the statusline — which reads only | ||
| // its own session — actually renders the model-authored line. | ||
| const own = readJsonFile<ReactionFile>(reactionPath); | ||
| if (own?.reaction !== freshTool.reaction || own?.source !== "tool") { | ||
| mkdirSync(stateDir, { recursive: true }); | ||
| atomicWriteJson(reactionPath, freshTool); | ||
| } | ||
| atomicWriteTimestamp(stopMarkerFile, now); | ||
| return { source: "none", updated: false }; | ||
| return { source: "tool", updated: false }; |
There was a problem hiding this comment.
📝 Info: Adoption branch writes to disk but returns updated:false
When a fresh tool reaction is adopted from another session file, atomicWriteJson(reactionPath, freshTool) writes the reaction into this session's file, yet the function still returns { source: "tool", updated: false } (server/hooks/buddy-comment.ts:220-226). The updated field is documented as "What produced the bubble ... the hook ran but wrote nothing." Here a write did happen, so updated is arguably inconsistent. This is harmless today because no production consumer reads updated (only the tests and the ignored import.meta.main path), but a future caller relying on updated to trigger a re-render would be misled.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (ts > nowMs + FUTURE_TIMESTAMP_TOLERANCE_MS) return false; | ||
| const lastStopSec = readTimestampSeconds(stopMarkerPath); | ||
| const effectiveLastStopMs = Math.min(lastStopSec * 1000, nowMs); | ||
| return ts > effectiveLastStopMs; |
There was a problem hiding this comment.
📝 Info: New-session-with-no-stop-marker can adopt an old cross-session reaction
For a session with no stop marker file, readTimestampSeconds returns 0 so effectiveLastStopMs is 0 (server/hooks/buddy-comment.ts:144-146), meaning ANY source: "tool" reaction with a positive timestamp within FUTURE_TIMESTAMP_TOLERANCE_MS is treated as fresh. On a brand-new session's first turn, a leftover tool reaction from another session (potentially minutes old, up to the 900s statusline TTL) could be adopted and rendered. This is bounded by the statusline's _sweep_expired_reactions (REACTION_TTL=900s) and mirrors the pre-existing behavior for the session's own file, so it is not a new class of bug, but worth noting as an edge case of the freshness guard.
(Refers to lines 136-147)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
This PR fixes cases where buddy_react writes a model-authored reaction under a different session id than the Stop hook/statusline reads, causing the Stop hook to fall back to (and overwrite with) a canned pool reaction instead of rendering the tool reaction.
Changes:
- Teach the Stop hook to scan all
reaction.*.jsonfiles for a freshsource: "tool"reaction and adopt it into the current session’s reaction file. - Update
ReactionSourcein the hook to include"tool"when the hook preserves/adopts a tool reaction. - Add a regression test covering cross-session adoption of a tool reaction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/hooks/buddy-comment.ts | Adds cross-session discovery/adoption of fresh tool-authored reactions to avoid fallback clobbering. |
| server/hooks/buddy-comment.test.ts | Adds regression coverage ensuring a tool reaction written under another session id is adopted into the current session file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const lastStopSec = readTimestampSeconds(stopMarkerPath); | ||
| const effectiveLastStopMs = Math.min(lastStopSec * 1000, nowMs); | ||
| return ts > effectiveLastStopMs; |
![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 `@server/hooks/buddy-comment.test.ts`:
- Around line 255-280: Add a companion test in the “cross-session buddy_react
adoption” suite covering a brand-new session with no .last_stop_hook state and
only a stale source:"tool" reaction from another session. Assert the stale
reaction is not adopted, preserving the max-age boundary behavior implemented in
handleBuddyComment.
In `@server/hooks/buddy-comment.ts`:
- Around line 136-190: Update isFreshToolReaction, used by
findFreshToolReaction, to reject tool reactions older than a defined absolute
maximum age relative to nowMs, in addition to the existing stop-marker and
future-timestamp checks. Apply this age validation consistently to own and
cross-session reaction files, including when the stop marker is missing.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 424ceb4b-4854-4bd1-9f42-0ac4a9f2c6fd
📒 Files selected for processing (2)
server/hooks/buddy-comment.test.tsserver/hooks/buddy-comment.ts
The dedupe filter matched commands containing 'coding-buddy' or 'claude-buddy', but the installed path is <state>/app/hooks/buddy-comment.sh — neither string appears. So the filter never matched and every install appended another copy. A dogfooding machine was found with EIGHT Stop entries. Duplicates are not cosmetic. The first invocation adopts a buddy_react reaction and stamps the stop marker; the rest then judge that same reaction stale and overwrite the bubble with a canned pool line. The last write wins, so the user always saw canned text no matter how many times the underlying bug was fixed. Two changes: - match buddy hook commands by script name and state path, not package name, and apply it to Stop, PostToolUse and UserPromptSubmit alike - give the hook a same-turn grace window so repeated invocations within one turn cannot demote a reaction the first invocation just adopted - cli/install.ts - server/hooks/buddy-comment.ts - server/hooks/buddy-comment.test.ts
Three reviewers independently flagged the same gap: a session with no stop marker yet has lastStopSec 0, so any positive timestamp read as fresh — letting a leftover tool reaction from an unrelated session surface on a new session's first turn. Bounded to the statusline's default reactionTTL: never adopt something the statusline would already treat as expired. Regression test covers a brand-new session with only a stale cross-session reaction present. Also: adoption writes to disk, so it now reports updated:true rather than false (a consumer using the flag to trigger a re-render would have been misled), and the new tests register their temp dirs with the shared cleanup so a failure cannot leak them into /tmp. - server/hooks/buddy-comment.ts - server/hooks/buddy-comment.test.ts
Four statusline test env blocks pinned BUDDY_STATUSLINE_COLS but left rows to the ambient terminal. Once density keyed on rows, those tests silently became environment-dependent: a developer on a 30-row terminal gets the compact tier, the bubble is dropped, and the rarity-colour assertion fails on main. CI has no tty, so rows fall back to the full-tier default and the failure is structurally invisible there. Same class as the width/tty problem the determinism work fixed — rows just never got the same treatment. - statusline/buddy-status.test.ts
Ships the reaction-pipeline fixes from #168. buddy_react reactions written under a different session id than the statusline reads are now adopted rather than clobbered by the canned pool. Hook registrations no longer accumulate on every install: the dedupe filter matched the package name, but the installed path contains neither 'coding-buddy' nor 'claude-buddy', so a dogfooding machine reached eight Stop entries — each duplicate overwriting the previous one's work. A same-turn grace window stops duplicate invocations demoting a reaction the first one just adopted, and adoption is bounded by the statusline's reaction TTL so a stale cross-session reaction cannot surface on a new session. Together these are why reactions kept rendering as canned text no matter how many times the inference path itself was fixed. - package.json
Fixes #167.
Symptom
buddy_reactfires.buddy_statsreportsSource: toolwith the model-written text. The statusline bubble shows a canned pool line anyway (...,*nods*,*takes notes*).Evidence
Same wall-clock minute, one
buddy_reactcall:CLAUDE_CODE_SESSION_IDresolves tod26d5a00— whatscripts/paths.shderivesBUDDY_SIDfrom, and therefore what the statusline and this hook read. The MCP server resolved4758cc43and wrote there.Cause
The MCP server is long-lived: it resolves
BUDDY_SIDonce at launch and keeps it for the process lifetime. The hook and the statusline resolve it per invocation. When those diverge, two failures stack:The freshness guard added in #159 could not help: it was comparing files belonging to different sessions.
From the outside this is indistinguishable from "the reactions are canned", which is precisely the complaint #154/#159 set out to answer.
Fix
findFreshToolReactionscans everyreaction.*.jsonin the state dir for a freshsource: "tool"entry rather than only the current session's, and adopts it into this session's file so the statusline — which reads only its own — renders the real line. The current session still wins when both are fresh.This treats the symptom at the consumer. Resolving the session id per call in the MCP server would be the deeper fix; #167 records that option, and this change is safe alongside it.
Verification
bun test463 pass / 0 fail,bun run typecheckclean.New regression test writes a tool reaction under a different session id and asserts the hook returns
tool, does not write a pool line, and that the reaction is readable from the current session's file — the only file the statusline reads. It fails if the fix is reverted.Note on the timing assertion
check-statusline.shintermittently fails its 1000ms budget on a loaded machine (observed at load ~174, a different fixture each run). It is not specific to this branch — worth making that assertion resilient (median of N, or a load-aware budget) in follow-up.🤖 This content was generated with AI assistance using Claude Opus 5.
Note
Fix
handleBuddyCommentto adopt abuddy_reacttool reaction written under a different session idfindFreshToolReactionto scan allreaction.*.jsonfiles, preferring the current session but falling back to the newest valid cross-session tool reaction within a 900,000 ms adoption ceiling.isFreshToolReactionto apply a 10,000 ms grace window so near-simultaneous Stop invocations in the same turn don't clobber an adopted reaction.ReactionSourceunion in buddy-comment.ts with'tool'to mirrorserver/state.ts.installHooksin install.ts to use a newisBuddyHookCommandhelper that matches hooks by script filename, preventing duplicate hook registrations when installed paths don't include the package name.Macroscope summarized 77f8d58.
Summary by CodeRabbit