Decisions app: no Other or free-text escape on select decisions - #2412
Conversation
… select decisions single_select and multi_select decisions now always include an Other option with a free-text field. The backend accepts free-text answers flagged via other_value while keeping strict option validation for declared choices. Answers are stored with other_value and note so the agent receives free-text responses in the same shape as option answers, with notes appended to the routed message.
|
ⓘ 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: 17 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}
| for o in (existing.get("options") or []) | ||
| if o.get("value") is not None | ||
| } | ||
| if valid and any(v not in valid for v in vals): |
There was a problem hiding this comment.
CRITICAL: Missing TypeError guard in multi_select Other-path validation
When body.other_value is set for a multi_select decision, any(v not in valid for v in vals) raises TypeError if body.value contains an unhashable type (e.g. a dict or nested list), returning 500 instead of 400. The option-only path already wraps this in try/except TypeError, but the new Other-path branch does not.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| for o in (existing.get("options") or []) | ||
| if o.get("value") is not None | ||
| } | ||
| if valid and any(v not in valid for v in vals): |
There was a problem hiding this comment.
CRITICAL: Missing TypeError guard in multi_select Other-path validation (agent route)
Same issue as the human path at line 436: any(v not in valid for v in vals) raises TypeError on unhashable body.value items when other_value is provided, returning 500 instead of 400. The option-only path has the guard; the new Other-path branch does not.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <Button | ||
| type="button" | ||
| disabled={submitting || multi.length === 0} | ||
| disabled={submitting || (multi.length === 0 && !otherSelected)} |
There was a problem hiding this comment.
WARNING: multi_select submit button enabled when Other selected but no text entered
The disabled condition multi.length === 0 && !otherSelected allows the button to be enabled when otherSelected is true but otherText is empty and no regular options are chosen, permitting an empty answer submission. The single_select path already disables submit when Other is selected without text.
| disabled={submitting || (multi.length === 0 && !otherSelected)} | |
| disabled={submitting || (multi.length === 0 && (!otherSelected || !otherText.trim()))} |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| selected={singleSelected === opt.value} | ||
| disabled={submitting} | ||
| onClick={() => submit(opt.value)} | ||
| onClick={() => setSingleSelected(opt.value)} |
There was a problem hiding this comment.
WARNING: otherText not cleared when switching from Other to a regular option
When the user types in the Other field then selects a regular option, the stale otherText is still sent to onAnswer. The backend rejects the value + other_value combination for single_select with a 400. Clear otherText when switching away from Other so the submission reflects the current selection.
| onClick={() => setSingleSelected(opt.value)} | |
| onClick={() => { setSingleSelected(opt.value); setOtherText(""); }} |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
Previous Review Summary (commit a8a3785)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a8a3785)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (6 files)
Reviewed by step-3.7-flash · Input: 179.6K · Output: 50.7K · Cached: 5M |
|
nemotron-super review VERDICT: No blocking issues found
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
Both doc-gate layers fired legitimately: tinyagentos/routes/decisions.py changed, and answering behaviour is user-visible. The doc entry records the per-type rules (single_select rejects value plus other_value; multi_select still validates every declared element and appends the free-text one), the note field, and that the strict path is unchanged when other_value is absent. It also records two things the diff changes without stating: there is no allow_other flag, so no decision author can enforce a closed option set, and the agent mirror path gained the same escape, so an agent holding decisions_write can now record a value outside the declared options. Both verified against the source, and raised on the PR for a product call rather than treated as blockers.
…nteraction Both doc-gate layers fired legitimately: routes/decisions.py changed, and what an agent can read back is user-visible. Recorded deliberately as a table, because the route now behaves three different ways depending on grant shape, and the global case is the surprising one: a global (null-project) grant returns null-project decisions ONLY, not every project's. Anything built against the older wider behaviour will see fewer rows. Also recorded that the filter-before-limit fix covers the global and single-project paths but NOT the two-or-more-project path, which still fetches 500 rows and filters in Python afterwards. Stated as a narrower blast radius rather than a closed issue, and raised on the PR. Section inserted before 'Config save and restore' rather than at the end of the file, so it cannot conflict with the section PR #2412 adds to the same doc.
doc-gate cleared (both layers). Two things the diff changes without saying so.First, to be clear I read the card the right way round: the title "no Other or free-text escape on select decisions" is the problem statement, so adding the escape is the fix. The diff matches the intent. Validation on the new path is sound, and I checked it rather than assuming: 1. There is no per-decision opt-outSo the free-text path is available on every select decision. A decision author cannot declare a closed option set and have it enforced. The comment this PR replaces asserted the opposite invariant:
That invariant now holds only for callers who do not send 2. The agent mirror path gained the same escape
So an agent holding Neither is a blocker and I have not treated them as one. Both are documented as they behave in |
1 participant
CARD TITLE (intent, not commit subject): Decisions app: no Other or free-text escape on select decisions
Autonomous build of board card tsk-4vutsd.
single_select and multi_select decisions now always include an Other option
with a free-text field. The backend accepts free-text answers flagged via
other_value while keeping strict option validation for declared choices.
Answers are stored with other_value and note so the agent receives free-text
responses in the same shape as option answers, with notes appended to the
routed message.
Files:
desktop/src/apps/DecisionsApp.test.tsx | 82 ++++++++++++++++
desktop/src/apps/DecisionsApp.tsx | 98 ++++++++++++++++--
tests/test_routes_decisions.py | 169 ++++++++++++++++++++++++++++++++
tests/test_routes_decisions_agent.py | 58 +++++++++++
tinyagentos/decisions/decision_store.py | 4 +-
tinyagentos/routes/decisions.py | 163 +++++++++++++++++++++---------
6 files changed, 516 insertions(+), 58 deletions(-)