[docs] Client-tool interaction lifecycle: research, blame timeline, plan v2 - #5916
Conversation
dosubot
Bot
added
the
size:XL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds lifecycle documentation for client-tool interactions. It records observed defects, historical evidence, a v2 implementation plan, implementation status, and QA coverage. ChangesClient-tool interaction lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 7
🧹 Nitpick comments (1)
docs/design/client-tool-interaction-lifecycle/plan.md (1)
74-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMap validation to each C3 and C4 invariant.
The current cases directly cover settlement and replay. The broad live scenario does not prove the watch subscription, pending-card adoption guard, non-last-message actions, unknown render-kind fallback, or registry consistency. Add deterministic validation cases for these behaviors.
This maps the plan to the defects documented in
research.mdLines 100-141.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3770a2a1-bd90-43f0-b719-bc4a45453703
📒 Files selected for processing (5)
docs/design/client-tool-interaction-lifecycle/README.mddocs/design/client-tool-interaction-lifecycle/context.mddocs/design/client-tool-interaction-lifecycle/plan.mddocs/design/client-tool-interaction-lifecycle/research.mddocs/design/client-tool-interaction-lifecycle/status.md
| responded row survives it by existing semantics. If the respond call fails, the flow | ||
| degrades to today's behavior (message-borne only), never blocks the user. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not preserve the known broken fallback.
If the pending -> responded call fails, the plan resumes with message-borne data only. The research shows that this path leaves client-tool rows pending, after which the next-turn sweep cancels them without an outcome. Define a retry or durable recovery path before dispatching the resume.
This follows research.md Lines 155-164 and Lines 168-173.
🧰 Tools
🪛 LanguageTool
[grammar] ~22-~22: Ensure spelling is correct
Context: ...rvives it by existing semantics. If the respond call fails, the flow degrades to today'...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
There was a problem hiding this comment.
Answered on the current thread for this line. Short version: the "nothing blocks" wording was stale and is fixed (the resume now waits for the answered call under a two second cap, and a late call still lands if the sweep has not run). The durable retry path stays declined on cost, since the answer survives in the conversation records either way.
(The fix is pushed in e019d34.)
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 12 minutes and 6 seconds before sending another message.
| ### C2. One replay precedence rule (no new record kind) | ||
|
|
||
| The closing conversational fact for a client tool already exists: the `tool_result` | ||
| record. Replay (both copies) reads outcomes in fixed precedence: | ||
| 1. a real `tool_result` (wins always, preserves the actual answer), | ||
| 2. else a post-contract row resolution, | ||
| 3. else, for a legacy cancelled row with no result: a neutral, inert "interaction | ||
| ended" state — never inferred success or decline, | ||
| 4. else (still pending): the live, actionable card. | ||
| The v1 idea of emitting `interaction_response` records for client tools is dropped: it | ||
| duplicated an existing fact and created ordering disagreements. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Complete replay precedence for intermediate and synthetic states.
C1 introduces status=responded, but C2 defines rules only for tool_result, resolved rows, cancelled rows, and pending rows. A reload during the answer/resume gap can therefore encounter an undefined state. Also define “real tool_result” using the same predicate in both replay copies. services/runner/src/responder.ts Lines 440-457 excludes isPauseSyntheticResult, and research.md records differing sentinel matching.
This follows the replay contract in services/runner/src/responder.ts Lines 440-457 and research.md Lines 55-61 and 135-137.
🧰 Tools
🪛 LanguageTool
[style] ~34-~34: ‘new record’ might be wordy. Consider a shorter alternative.
Context: ... ### C2. One replay precedence rule (no new record kind) The closing conversational fact ...
(EN_WORDINESS_PREMIUM_NEW_RECORD)
[style] ~38-~38: The adverb ‘always’ is usually put before the verb ‘wins’.
Context: ...ed precedence: 1. a real tool_result (wins always, preserves the actual answer), 2. else ...
(ADVERB_WORD_ORDER)
There was a problem hiding this comment.
Answered on the current thread for this line. Short version: rule 3 now covers every terminal status without a saved answer (cancelled, responded, resolved), which closes the undefined state you found. The exact predicates stay in implementation.md, since plan.md is the plain-language summary.
(The fix is pushed in e019d34.)
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 12 minutes and 3 seconds before sending another message.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
docs/design/client-tool-interaction-lifecycle/qa.md (4)
38-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine “both copies” before making replay equality a gate.
Name the two replay copies or code paths. Define which normalized fields must match, such as interaction identity, terminal status, resolution payload, and ordering. Otherwise, different implementations can satisfy the same prose requirement.
64-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDefine “exactly once” by interaction identity and render surface.
The project scope includes card behavior across render locations. Specify whether “exactly once” means one lifecycle row, one visible card in the driving tab, or one DOM instance across all surfaces. Include the interaction identifier and allowed surfaces in the assertion.
62-69: 🩺 Stability & Availability | 🔵 TrivialIsolate the live smoke data and failure path.
This scenario runs after every deploy on every deployed stack and creates persistent interaction rows. Define a dedicated test tenant or session, unique run identifiers, cleanup, and timeout behavior. A failed answer or reload can leave unresolved
client_toolrows and contaminate the alert described in Lines 73 through 77.
78-80: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEmit the telemetry signal on a state transition, not every render.
A card can render many times while it remains interactive. Require one event per transition, with a stable interaction ID and the observed row status. Deduplicate or rate-limit the event.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c62db5ac-4ae4-4480-95ce-c1a2ff076b2c
📒 Files selected for processing (1)
docs/design/client-tool-interaction-lifecycle/qa.md
dosubot
Bot
added
size:L
…eline, plan v2 The planning workspace for the interaction-card bug class (2026-08-10): context (the user experience and thesis), research (six evidenced mechanisms, full render and lifecycle inventories, the kind-by-outcome settlement table, the blame timeline against v0.108), and plan v2 (four changes, reshaped after an adversarial review found v1 unsound).
…overable no more Why every layer missed it (seam bugs between green units), and six standing checks: the settlement matrix as a permanent acceptance test, replay goldens across both copies and cache states, the geometry invariant, an adoption safety property, the scripted compound journey as the deploy smoke, and two production detection signals.
Per Mahmoud: the docs were too dense to read. Full rewrite in simple technical English — short sentences, one idea per sentence, one name per thing (card, row, sweep, replay, adoption), concepts explained before use, file:line detail moved out of the prose. Content unchanged: same findings, same four-change plan, same six standing checks.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
docs/design/client-tool-interaction-lifecycle/research.md (1)
3-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd durable links to the evidence sources.
This section names “the two inventory reports” and “the PR discussion” but does not identify or link them. Add report paths and stable commit or PR references. Otherwise, readers cannot audit the database, log, and history claims after this PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: bd88bdb3-8127-474f-bc9a-19f3c8ab6fc0
📒 Files selected for processing (6)
docs/design/client-tool-interaction-lifecycle/README.mddocs/design/client-tool-interaction-lifecycle/context.mddocs/design/client-tool-interaction-lifecycle/plan.mddocs/design/client-tool-interaction-lifecycle/qa.mddocs/design/client-tool-interaction-lifecycle/research.mddocs/design/client-tool-interaction-lifecycle/status.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/design/client-tool-interaction-lifecycle/context.md
- docs/design/client-tool-interaction-lifecycle/README.md
b0005fb to
6e5ec9c
Compare
dosubot
Bot
removed
the
size:L
dosubot
Bot
added
the
size:XXL
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
docs/design/client-tool-interaction-lifecycle/qa.md (1)
25-27: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExercise the production cancellation path for
walk away.For
walk away, create apendingrow, run the samecancel-staleor expiry path used in production, then assertcancelledwith no resolution. A direct transition call does not verify the production cancellation path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c546000-4516-4b90-82e8-cb5686bff700
📒 Files selected for processing (4)
docs/design/client-tool-interaction-lifecycle/implementation.mddocs/design/client-tool-interaction-lifecycle/plan.mddocs/design/client-tool-interaction-lifecycle/qa.mddocs/design/client-tool-interaction-lifecycle/status.md
| If the "answered" call fails, nothing blocks: the flow falls back to today's behavior. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
Do not keep the known broken fallback.
If the answered call fails, this plan still falls back to the flow described at Lines 14-15: the row remains pending, and the sweep can mark it cancelled. Define an idempotent retry or durable recovery path instead of silently using the path that loses the answer.
There was a problem hiding this comment.
Fixed in part. The stale half was real: the plan still said "nothing blocks", written before the review moved the ordering. It now describes what shipped, which is a bounded wait rather than an unguarded fallback: the resume waits for the answered call, the wait is capped at two seconds, and a late answered call still lands if the sweep has not run yet. So the failure path costs at most today's behavior.
Declining the durable retry queue. It was considered and rejected on cost: the answer already survives in the conversation records, which is how the agent learns it today, so a lost row costs a display state and not the user's work. A client-side retry store for a two second window would add persistence and replay machinery to recover a status word. If the alarm in qa.md check 6 shows this failing in practice, that is the moment to build it.
(The fix is pushed in e019d34.)
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 12 minutes and 9 seconds before sending another message.
| - The three code paths that only looked at the last message now scan the whole chat, | ||
| like the status fix (#5913) already does. | ||
| - The one-line dispatch repair from research Finding 3 lands immediately, outside this | ||
| project. | ||
| - The card tool-name list gets one shared source. | ||
|
|
||
| ## Filed separately (not in this project) | ||
|
|
||
| - Mobile answering of form and connect cards. | ||
| - Making the agent's "am I connected?" check verify the connection is valid. | ||
| - Reusing an existing connection (issue #5911). | ||
| - Small cleanups: stale form drafts, one missing error catch, one unreachable sweep | ||
| edge case. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
Track carried-sibling re-announcement.
Change 4 still omits the known suppressed re-announcement when a carried sibling is adopted. Add it to the implementation and QA scope, or list it under “Filed separately” with an owner and issue. Otherwise, the project can pass the current card-location checks while the visibility regression remains.
There was a problem hiding this comment.
Fixed. You were reading a finding that the plain-language rewrite dropped without a home, which was our mistake. plan.md "Filed separately" now carries it: announcing a carried approval gate again, with the mechanism in one sentence, the ordering half credited to #5910, and the rest pointed at the open runner race #5907. So it is explicitly out of this project's scope with a tracked home rather than silently missing.
(The fix is pushed in e019d34.)
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 12 minutes and 7 seconds before sending another message.
…rding, sweep-driven walk-away check
1 participant
Context
Today's recurring interaction-card bugs (resurrected questionnaires, dead connect cards, the running-elsewhere strip accusing its own tab, answers ignored) got the full planning treatment instead of more symptom patches. This PR lands the workspace at
docs/design/client-tool-interaction-lifecycle/.What is inside
Related, already merged today: #5909, #5912, #5913, #5910. Open issues it references: #5907, #5911.