W1: decision blocks render inline in chat (read-only) (#2150) - #2418
Conversation
When an agent raises a decision via request_decision over the openclaw chat
bridge, attach a {kind: decision, decision_id} content block to the in-flight
chat message (using the existing content_blocks convention) so the decision
question surfaces inline in the conversation. The MessagesApp DecisionBlock
component fetches the decision from /api/decisions/{id} and renders it
read-only: the question text, the options as disabled buttons, the decision
type, and the current state (open, or answered with the chosen option plus
answerer and timestamp). Re-opening a thread re-fetches the decision, so an
already-answered question renders in its resolved state. Decisions raised
outside a chat context do not produce a block.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 16 minutes Limit details: You’ve used all 2 included reviews currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| key={opt.value} | ||
| type="button" | ||
| disabled | ||
| aria-pressed={false} |
There was a problem hiding this comment.
[WARNING]: aria-pressed={false} on disabled option buttons
aria-pressed is intended for toggle buttons, not regular buttons. Using it on disabled option buttons could confuse assistive technologies. Consider removing this attribute since the buttons are already non-interactive via disabled.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ...(data.edited_at !== undefined && { edited_at: data.edited_at }), | ||
| ...(data.metadata !== undefined && { metadata: data.metadata }), | ||
| } | ||
| ? { |
There was a problem hiding this comment.
[SUGGESTION]: Indentation inconsistency in message_edit ternary operator
The ? is indented with 15 spaces while the condition on the previous line has 16 spaces. This is inconsistent with other ternary operators in the file (e.g., message_delete at line 1084 aligns ? with :). Consider aligning the ternary operator for readability.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| Returns the updated blocks list, or None if the message was not found. | ||
| """ | ||
| async with self._db.execute( |
There was a problem hiding this comment.
[WARNING]: append_content_block not wrapped in a transaction
The SELECT and UPDATE are not atomic. Between reading content_blocks and writing the updated value, another coroutine could modify the same row, leading to lost updates. Consider wrapping the read-modify-write in a transaction.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if tool_name == "request_decision": | ||
| decision_id = _extract_decision_id(body.get("result")) | ||
| if decision_id and self._chat_messages and self._chat_hub: | ||
| msg_id = session._pending_msg_ids.get(trace_id) |
There was a problem hiding this comment.
[WARNING]: trace_id mismatch could prevent decision block attachment
If a tool_result for request_decision arrives without a trace_id (or with a different one than the originating delta), the lookup session._pending_msg_ids.get(trace_id) returns None and the decision block is never attached. Consider using the openclaw message id or falling back to looking up the most recent pending message for the agent.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous Review Summary (commit f09c31c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f09c31c)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Reviewed by step-3.7-flash · Input: 178.1K · Output: 45.4K · Cached: 1.9M |
|
nemotron-super review VERDICT: No blocking issues found.
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
Reviewed. CI is green on 22 checks, but green describes CI and not the review surface, so I read kilo's findings and checked each against the source. Two are real and I am treating them as must-fix; two are not blocking. Must-fix 1: lost-update race in
|
Converting to draft, and correcting a bad path in my own review aboveBoth must-fixes are still present. I re-verified each against the branch just now rather than inferring from the PR being untouched. I am converting this PR to a draft. Not a comment on the work. Every check here is green, and a green rollup describes CI, never the review surface, so this PR would merge by accident on a routine sweep. A draft cannot. It is the same mechanical block I applied to #2415 for the same reason, and it reverses in one click the moment the two fixes land. Correction to my review comment aboveI cited the ARIA finding as Worth stating plainly because a wrong path in a review costs you a failed command before you can start, and I would rather correct it than have you discover it. I caught it only because I re-checked the finding with a control: my first grep of the wrong path returned nothing, which would have read as "already fixed" had I not confirmed the grep could find What unblocks thisEither fix is independently landable:
Push both and mark ready for review, and I will re-check and merge. |
…rt options append_content_block did SELECT, mutate in Python, UPDATE with no transaction, so concurrent appends interleaved and every write but the last was silently lost. A new test appends 12 blocks concurrently: against the old code exactly one survived, so this was not a rare interleaving but the normal outcome under any concurrency. The append now happens in a single UPDATE via json_insert, which is atomic in the database rather than only within one process. The same file already serialises reaction updates with _reaction_lock for this exact hazard, so the risk was known here; content_blocks just never got the same treatment. Also removes aria-pressed from the read-only decision option buttons. They are disabled and inert, and aria-pressed declares a toggle, so assistive tech announced them as unpressed toggles the user could activate. disabled already conveys non-interactivity.
Both must-fixes are done. I completed them myself, and my previous comment was wrong to ask you to.I closed the last comment with "push the fixes and mark ready for review". There is nobody to do that. Lanes here are one-shot: they post The race was much worse than I reported itI described it as two concurrent appends interleaving and losing one. I wrote a test that appends 12 blocks concurrently and ran it against the unfixed code first: Eleven of twelve blocks lost. Only the last writer survived. This is not a rare interleaving that needs bad luck, it is the normal outcome whenever appends overlap, because every one of them reads the same array before any of them writes. Correcting my own severity estimate upward: I called it a lost-update race, which understated it. The fix, and why this shapeThe append is now a single Worth knowing: this file already defends against this exact hazard elsewhere. The three pre-existing append tests still pass unchanged, so the return value, the Verification, including what I did NOT runBoth run against the worktree copy, asserted rather than assumed: I confirmed I did not run the desktop suite — Still open, unchanged and non-blockingThe ternary indentation at line 1064, and the Taking this out of draft once CI comes back green. |
Resolving the last finding, then merging. All four of kilo's are now closed out.I had flagged the The mechanism is real. But it is pre-existing and this PR does not make it worse. That line is byte-identical on Carded as Final state of kilo's fourWhat I am merging on, stated plainly20 checks green, no reds, no cancellations, on a settled run. CodeRabbit did not review this PR at all — no review from it exists, so I am not claiming bot coverage I do not have. I am merging on kilo's review plus my own line-level review, with every one of kilo's findings either fixed in code or adjudicated above with the evidence. Merging. |
|
|
||
| Returns the updated blocks list, or None if the message was not found. | ||
|
|
||
| The append happens in a single UPDATE so it is atomic. Reading the array |
There was a problem hiding this comment.
[SUGGESTION]: Docstring contradiction — describes old read-modify-write behavior
TheFirst sentence correctly states the append is atomic, but the following
sentences ("Reading the array into Python, appending and writing it back lets
two concurrent appends interleave...") describe the previous non-atomic
implementation. This docstring is now misleading for future maintainers.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| interleave, and the loser is dropped with no error and no log line. | ||
| """ | ||
| cursor = await self._db.execute( | ||
| "UPDATE chat_messages SET content_blocks = json_insert(" |
There was a problem hiding this comment.
[WARNING]: json_insert silently corrupts content_blocks to NULL on invalid JSON
If the existing content_blocks value is not valid JSON (e.g. from a prior
bug or manual edit), json_insert returns NULL and the UPDATE sets the
column to NULL, silently losing data. The previous implementation raised
json.JSONDecodeError on the same input, making corruption visible.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
CARD TITLE (intent, not commit subject): W1: decision blocks render inline in chat (read-only) (#2150)
Autonomous build of board card tsk-4micka.
When an agent raises a decision via request_decision over the openclaw chat
bridge, attach a {kind: decision, decision_id} content block to the in-flight
chat message (using the existing content_blocks convention) so the decision
question surfaces inline in the conversation. The MessagesApp DecisionBlock
component fetches the decision from /api/decisions/{id} and renders it
read-only: the question text, the options as disabled buttons, the decision
type, and the current state (open, or answered with the chosen option plus
answerer and timestamp). Re-opening a thread re-fetches the decision, so an
already-answered question renders in its resolved state. Decisions raised
outside a chat context do not produce a block.
Files:
desktop/src/apps/MessagesApp.tsx | 221 ++++++++++++++++++++-
.../apps/chat/tests/render-helpers.test.tsx | 29 ++-
.../components/tests/DecisionBlock.test.tsx | 215 ++++++++++++++++++++
tests/test_bridge_session.py | 107 ++++++++++
tests/test_chat_messages.py | 34 ++++
tinyagentos/bridge_session.py | 47 +++++
tinyagentos/chat/message_store.py | 20 ++
8 files changed, 666 insertions(+), 8 deletions(-)