Skip to content

feat(taosctl): decisions command group#1413

Merged
jaylfc merged 4 commits into
devfrom
feat/taosctl-decisions-group
Jun 23, 2026
Merged

feat(taosctl): decisions command group#1413
jaylfc merged 4 commits into
devfrom
feat/taosctl-decisions-group

Conversation

@jaylfc

@jaylfc jaylfc commented Jun 23, 2026

Copy link
Copy Markdown
Owner

What

Adds the taosctl decisions command group over the existing Decisions inbox API.

The Decisions human-in-the-loop backend (store + /api/decisions routes, L1 supersede, A2A answer routing) shipped in #1331 / #1339 / #1336 but had no CLI surface. Decisions is agent-facing by design: an agent posts a choice it needs from the user, moves on, and polls for the answer on its next turn. The CLI group is the natural driver for that loop.

Verbs

  • list [--status] [--project] [--limit] — poll pending decisions
  • get <id> / history <id> — fetch one + its L1 supersession lineage
  • answer <id> --value [--answered-by] — record an answer (JSON array coerced for multi_select)
  • post --from-agent --question --type [...] — queue a new decision (all four types; options via --options-json; branching fields passthrough)

Tests

  • tests/test_taosctl_decisions.py — 10 unit tests (filters, limit validation, value coercion, optional-field omission, error mapping)
  • Passes the static test_taosctl_route_coverage gate (every path maps to a real route+method)
  • Full taosctl unit suite green (304 passed)

Surgical: one new command module + its test, no changes to existing routes or groups.

Summary by CodeRabbit

  • New Features

    • Added taosctl decisions CLI command group with subcommands to list, get, history, answer, and post decisions.
    • Supports filtering by status and project, configurable limits, and priority.
    • answer now safely handles --value inputs (only coercing well-formed bracketed arrays), and post supports --options-json as a validated JSON array with correct payload construction.
  • Tests

    • Expanded pytest coverage for command wiring, request/query formation, input validation, and exit codes for API/transport errors.

The Decisions inbox backend (store + /api/decisions routes, L1 supersede, A2A
answer routing) shipped in #1331/#1339/#1336, but had no taosctl surface.
Decisions is agent-facing: agents post the choices they need and poll for
answers between turns, so the CLI group is the natural driver. Wraps the five
routes (post, list, get, history, answer) with status/project filters,
positive-int limit validation, and JSON-array coercion for multi_select
answers. Covered by unit tests and the static route-coverage gate.
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@jaylfc, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 39 minutes and 24 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7c227763-dd4b-4e9b-8b3e-945ada4884c4

📥 Commits

Reviewing files that changed from the base of the PR and between cc0c500 and 432e114.

📒 Files selected for processing (2)
  • tests/test_taosctl_decisions.py
  • tinyagentos/cli/taosctl/commands/decisions.py
📝 Walkthrough

Walkthrough

Adds tinyagentos/cli/taosctl/commands/decisions.py, which implements the taosctl decisions subcommand group with five handlers (list, get, history, answer, post) and a _coerce_value helper. Also adds json_array validator to argtypes.py. A corresponding pytest suite in tests/test_taosctl_decisions.py validates endpoint wiring, payload formation, and error-to-exit-code mapping using a _FakeClient stub.

Changes

taosctl decisions CLI command and tests

Layer / File(s) Summary
JSON array argument type validator
tinyagentos/cli/taosctl/argtypes.py
Adds json_array argparse type validator that parses and validates JSON array input, rejecting malformed or non-array JSON at CLI parse time.
Module constants and subcommand registration
tinyagentos/cli/taosctl/commands/decisions.py
Defines NOUN constant and CLI constants for decision types, priorities, and statuses; implements register(subparsers) wiring all five subcommands with argument definitions and handler defaults.
Subcommand handlers and value coercion
tinyagentos/cli/taosctl/commands/decisions.py
Implements cmd_list, cmd_get, cmd_history, cmd_answer, and cmd_post handlers assembling query params or JSON bodies for /api/decisions endpoints; adds _coerce_value that JSON-parses --value only when it is a bracketed array.
Test infrastructure and endpoint wiring tests
tests/test_taosctl_decisions.py
Defines _FakeClient and _run helper; verifies decisions list default limit, --status/--project mapping, zero-limit rejection, and exact GET paths for decisions get and decisions history.
Payload formation and error handling tests
tests/test_taosctl_decisions.py
Validates decisions answer sends string or parsed JSON-array values with optional answered_by; checks decisions post body for free_text and single_select including option parsing and optional-field omission; confirms ApiError → exit 2 and TransportError → exit 1.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • jaylfc/taOS#1331: Introduces the backend /api/decisions routes and payload contracts that the taosctl decisions CLI and its tests directly wire against.

Poem

🐇 Hop hop, the decisions are in!
A list, a get, a history spin,
POST an answer—string or array fair,
ApiError? Exit two with flair!
TransportError? One, and hop away,
The taosctl CLI is here to stay! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.55% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(taosctl): decisions command group' accurately reflects the main change: adding a new decisions command group to the taosctl CLI tool. It is concise, clear, and specific enough that reviewers understand the primary contribution.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/taosctl-decisions-group

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@jaylfc jaylfc disabled auto-merge June 23, 2026 17:26
# free of server imports (matching the other command groups).
_TYPES = ("single_select", "multi_select", "free_text", "approve_deny")
_PRIORITIES = ("normal", "blocking")
_STATUSES = ("pending", "answered", "expired", "superseded")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: _STATUSES includes expired, which is not a real decision status. The store only writes pending, answered, or superseded (tinyagentos/decisions/decision_store.py:32, 87, 138, 151); nothing in the codebase ever sets a decision to expired. Passing --status expired will pass the choices= validator on the CLI and be sent to the server, where WHERE status = 'expired' simply returns no rows — no error, just an empty inbox. This is a real footgun for an agent that polls --status pending then widens to look for expired ones. Either drop expired from _STATUSES (so argparse rejects the value upfront with a clear message) or add a server-side sweeper that actually flips past-deadline decisions to expired.


# Mirrors tinyagentos.decisions.decision_store; kept inline so the CLI stays
# free of server imports (matching the other command groups).
_TYPES = ("single_select", "multi_select", "free_text", "approve_deny")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUGGESTION: The comment on line 17 claims _TYPES "Mirrors tinyagentos.decisions.decision_store", but the server's DECISION_TYPES is ("single_select", "multi_select", "approve_deny", "free_text") (free_text and approve_deny are swapped). Functionally harmless because choices= does a set comparison, but the comment is misleading. Either reorder to match the server (so a future maintainer greps in one place) or update the comment to say "same members as the server; order differs for --help readability".



def _post(args, client):
options = json.loads(args.options_json) if args.options_json else []

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: json.loads(args.options_json) will raise a raw json.JSONDecodeError traceback to stderr on malformed input (e.g. a forgotten comma, a trailing comma, an unescaped quote). The user sees a Python traceback instead of an actionable CLI error, and exits with the default traceback code rather than the CLI's documented 1 (transport/local) or 2 (API). Wrap the call and raise a clean argparse.ArgumentTypeError / SystemExit so the user gets error: --options-json is not valid JSON: <reason> and a proper exit code. The _coerce_value helper for --value below has the same issue.

Parse only when the value looks like JSON so a plain string answer that
happens to contain a bracket is not mangled."""
text = raw.strip()
if text[:1] in ("[", "{"):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: _coerce_value silently swallows JSONDecodeError and returns the original raw string (line 130). This means a typo like --value '["a" "b"]' (missing comma) is sent to the server as a string — the server then validates the value against the declared options for multi_select, returns a generic 400, and the user has no idea the problem was a JSON typo at the CLI layer. The whole point of this helper is to distinguish "string answer" from "JSON-array answer"; right now it can disguise the latter as the former. Consider: (a) only falling back to the raw string if text[:1] in ("[", "{") AND text[-1:] in ("]", "}") (a clearly-bracketed intent), and (b) surfacing the parse error for malformed input rather than silently forwarding it.


lp = verbs.add_parser("list", help="List decisions (filter by status/project)")
lp.add_argument("--status", choices=list(_STATUSES), help="Filter by status")
lp.add_argument("--project", dest="project_id", help="Filter by project id")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUGGESTION: --project has no choices= and no length/format validation, and _list (line 76) silently drops an empty string via if args.project_id:. An agent that script-constructs a project id and ends up with None or "" will get an unfiltered inbox with no error — likely the opposite of what it intended. Add default=None and a real truthiness check, or document the silent-drop behavior in the help text. Also worth noting: the server's list endpoint accepts a user_id filter (decision_store.py:117), but the CLI doesn't expose it — probably out of scope for the agent-self-poll use case, but flagging in case admins need it.

Comment on lines +99 to +100
def _post(args, client):
options = json.loads(args.options_json) if args.options_json else []

@gitar-bot gitar-bot Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Edge Case: Invalid --options-json crashes post with an uncaught traceback

In _post (tinyagentos/cli/taosctl/commands/decisions.py:100), json.loads(args.options_json) is called without any error handling. If the user (or, per the design, an agent) passes malformed JSON to --options-json, json.JSONDecodeError propagates out of args.func(args, client) in __main__.main. That call site (tinyagentos/cli/taosctl/main.py:42-49) only catches ApiError and TransportError, so the exception escapes as an unhandled traceback rather than mapping to a clean exit code.

This contradicts the CLI's stated agent-friendliness / exit-code design (0 success, 1 local/usage error, 2 API error) and is inconsistent with _coerce_value, which deliberately guards json.loads against JSONDecodeError. An agent driving this CLI would receive a stack trace and a non-deterministic failure instead of a parseable error.

Wrap the parse in a try/except and surface a clean local error so it maps to exit code 1.

Catch malformed --options-json and raise TransportError so main() maps it to exit code 1 (local error) instead of an uncaught traceback. Add from ..client import TransportError at the top of the module.:

def _post(args, client):
    if args.options_json:
        try:
            options = json.loads(args.options_json)
        except json.JSONDecodeError as exc:
            raise TransportError(f"--options-json is not valid JSON: {exc}")
    else:
        options = []

Was this helpful? React with 👍 / 👎

Comment on lines +121 to +131
def _coerce_value(raw: str):
"""A multi_select answer is a JSON array; everything else is the raw string.
Parse only when the value looks like JSON so a plain string answer that
happens to contain a bracket is not mangled."""
text = raw.strip()
if text[:1] in ("[", "{"):
try:
return json.loads(text)
except json.JSONDecodeError:
return raw
return raw

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Edge Case: _coerce_value parses JSON objects, not just arrays, for any answer

_coerce_value (tinyagentos/cli/taosctl/commands/decisions.py:121-131) is type-blind: it parses any --value that begins with [ or { as JSON. The docstring states only a multi_select answer is a JSON array, but a free_text answer that legitimately starts with { or [ (e.g. a JSON snippet the user wants to store as a literal string) will be silently coerced into a dict/list and sent with a different type than intended. Since the CLI does not know the decision's type, it cannot distinguish these cases. This is an edge case but could lead to surprising payloads. Consider documenting the limitation, or only coercing into a list ([) since multi_select is the only documented JSON-array case, leaving objects as raw strings.

Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-bot Bot commented Jun 23, 2026

Copy link
Copy Markdown

Note

Your trial team has used its Gitar budget, so automatic reviews are paused. Upgrade now to unlock full capacity. Comment "Gitar review" to trigger a review manually.
Learn more about usage limits

Code Review ⚠️ Changes requested 0 resolved / 2 findings

Implements the taosctl decisions command group to interface with the inbox API. Changes requested because invalid --options-json triggers an uncaught traceback and _coerce_value incorrectly attempts to parse JSON objects.

⚠️ Edge Case: Invalid --options-json crashes post with an uncaught traceback

📄 tinyagentos/cli/taosctl/commands/decisions.py:99-100

In _post (tinyagentos/cli/taosctl/commands/decisions.py:100), json.loads(args.options_json) is called without any error handling. If the user (or, per the design, an agent) passes malformed JSON to --options-json, json.JSONDecodeError propagates out of args.func(args, client) in __main__.main. That call site (tinyagentos/cli/taosctl/main.py:42-49) only catches ApiError and TransportError, so the exception escapes as an unhandled traceback rather than mapping to a clean exit code.

This contradicts the CLI's stated agent-friendliness / exit-code design (0 success, 1 local/usage error, 2 API error) and is inconsistent with _coerce_value, which deliberately guards json.loads against JSONDecodeError. An agent driving this CLI would receive a stack trace and a non-deterministic failure instead of a parseable error.

Wrap the parse in a try/except and surface a clean local error so it maps to exit code 1.

Catch malformed --options-json and raise TransportError so main() maps it to exit code 1 (local error) instead of an uncaught traceback. Add `from ..client import TransportError` at the top of the module.
def _post(args, client):
    if args.options_json:
        try:
            options = json.loads(args.options_json)
        except json.JSONDecodeError as exc:
            raise TransportError(f"--options-json is not valid JSON: {exc}")
    else:
        options = []
💡 Edge Case: _coerce_value parses JSON objects, not just arrays, for any answer

📄 tinyagentos/cli/taosctl/commands/decisions.py:121-131

_coerce_value (tinyagentos/cli/taosctl/commands/decisions.py:121-131) is type-blind: it parses any --value that begins with [ or { as JSON. The docstring states only a multi_select answer is a JSON array, but a free_text answer that legitimately starts with { or [ (e.g. a JSON snippet the user wants to store as a literal string) will be silently coerced into a dict/list and sent with a different type than intended. Since the CLI does not know the decision's type, it cannot distinguish these cases. This is an edge case but could lead to surprising payloads. Consider documenting the limitation, or only coercing into a list ([) since multi_select is the only documented JSON-array case, leaving objects as raw strings.

🤖 Prompt for agents
Code Review: Implements the `taosctl decisions` command group to interface with the inbox API. Changes requested because invalid `--options-json` triggers an uncaught traceback and `_coerce_value` incorrectly attempts to parse JSON objects.

1. ⚠️ Edge Case: Invalid --options-json crashes post with an uncaught traceback
   Files: tinyagentos/cli/taosctl/commands/decisions.py:99-100

   In `_post` (tinyagentos/cli/taosctl/commands/decisions.py:100), `json.loads(args.options_json)` is called without any error handling. If the user (or, per the design, an agent) passes malformed JSON to `--options-json`, `json.JSONDecodeError` propagates out of `args.func(args, client)` in `__main__.main`. That call site (tinyagentos/cli/taosctl/__main__.py:42-49) only catches `ApiError` and `TransportError`, so the exception escapes as an unhandled traceback rather than mapping to a clean exit code.
   
   This contradicts the CLI's stated agent-friendliness / exit-code design (0 success, 1 local/usage error, 2 API error) and is inconsistent with `_coerce_value`, which deliberately guards `json.loads` against `JSONDecodeError`. An agent driving this CLI would receive a stack trace and a non-deterministic failure instead of a parseable error.
   
   Wrap the parse in a try/except and surface a clean local error so it maps to exit code 1.

   Fix (Catch malformed --options-json and raise TransportError so main() maps it to exit code 1 (local error) instead of an uncaught traceback. Add `from ..client import TransportError` at the top of the module.):
   def _post(args, client):
       if args.options_json:
           try:
               options = json.loads(args.options_json)
           except json.JSONDecodeError as exc:
               raise TransportError(f"--options-json is not valid JSON: {exc}")
       else:
           options = []

2. 💡 Edge Case: _coerce_value parses JSON objects, not just arrays, for any answer
   Files: tinyagentos/cli/taosctl/commands/decisions.py:121-131

   `_coerce_value` (tinyagentos/cli/taosctl/commands/decisions.py:121-131) is type-blind: it parses any `--value` that begins with `[` or `{` as JSON. The docstring states only a multi_select answer is a JSON array, but a free_text answer that legitimately starts with `{` or `[` (e.g. a JSON snippet the user wants to store as a literal string) will be silently coerced into a dict/list and sent with a different type than intended. Since the CLI does not know the decision's type, it cannot distinguish these cases. This is an edge case but could lead to surprising payloads. Consider documenting the limitation, or only coercing into a list (`[`) since multi_select is the only documented JSON-array case, leaving objects as raw strings.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Important

Your trial ends in 3 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

Was this helpful? React with 👍 / 👎 | Gitar

@kilo-code-bot

kilo-code-bot Bot commented Jun 23, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

The incremental commit is a single-line style change in tinyagentos/cli/taosctl/commands/decisions.py:13 — switching the argtypes import from a relative path (from ..argtypes import …) to an absolute path (from tinyagentos.cli.taosctl.argtypes import …) to align with the 11 other command modules. No semantic change, no behavior change, no test impact.

Verified the changed line against gh pr diff 1413 and the import path is commentable (visible as a + line at line 13 of the new file). No new issues to flag.

Files Reviewed (1 file in incremental diff)
  • tinyagentos/cli/taosctl/commands/decisions.py - 0 issues (1-line style alignment)
Previous Review Summaries (3 snapshots, latest commit 9c098a2)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 9c098a2)

Status: No Issues Found | Recommendation: Merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 0

The previous review's WARNING (_coerce_value's [...] heuristic hard-erroring on legitimate free-text answers wrapped in brackets) is resolved in this incremental change.

Verification of changed code (decisions.py lines 125-141, plus test updates)

The new _coerce_value:

  • Returns the raw string on JSONDecodeError instead of raising TransportError[hello world] now forwards as a string with exit 0, matching the lenient pre-original behavior.
  • Returns the raw string when the parsed value is not a list (object, number, etc.) — same as before, no regression.
  • Returns the parsed list only when both conditions hold ([-prefixed AND valid JSON AND isinstance list).
  • The now-unused TransportError import was correctly removed.
  • The docstring accurately describes the new trade-off: the CLI cannot see the decision type, so a malformed multi_select array is forwarded as a string and rejected server-side.

Test coverage updated in lockstep:

  • test_answer_object_like_value_stays_string — comment updated to reflect the "parses to a non-list → literal string" framing.
  • test_answer_bracketed_free_text_stays_string — new positive test asserting [hello world] is forwarded as a string with exit 0 (proves the fix).
  • The prior negative-path test (test_answer_malformed_array_maps_to_exit_1) was correctly removed since the behavior intentionally changed.

No new issues found on the changed lines.

Files Reviewed (2 files in incremental diff)
  • tinyagentos/cli/taosctl/commands/decisions.py - 0 issues (resolved previous WARNING)
  • tests/test_taosctl_decisions.py - 0 issues (test renames + new test align with source change)

Previous review (commit cc0c500)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0

The previous review's findings are addressed: expired is gone from _STATUSES (the test now proves argparse rejects it), json.loads on raw input is gone in favor of a json_array argtype that yields a clean error: line, and _coerce_value now raises TransportError (caught in __main__ for exit 1) instead of silently forwarding a malformed answer as a string. The _TYPES comment was also corrected to acknowledge the order-only difference. One new concern surfaced from the broader [...] heuristic.

Issue Highlights

  • The new text[:1] == "[" and text[-1:] == "]" check in _coerce_value will hard-error on legitimate free-text answers that happen to be wrapped in brackets (e.g. [hello world]), which the previous lenient code would have forwarded as a raw string.
Issue Details (click to expand)

WARNING

File Line Issue
tinyagentos/cli/taosctl/commands/decisions.py 133 New bracketed-array heuristic rejects legitimate free-text answers wrapped in [ ] (regression from previous lenient behavior)
Files Reviewed (3 files)
  • tinyagentos/cli/taosctl/commands/decisions.py - 1 issue
  • tinyagentos/cli/taosctl/argtypes.py - 0 issues (json_array cleanly rejects malformed JSON and non-array payloads at parse time)
  • tests/test_taosctl_decisions.py - 0 issues (good coverage for the new error paths)

Fix these issues in Kilo Cloud

Previous review (commit 5e86fc3)

Status: 5 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 3
SUGGESTION 2

The PR is a clean, well-scoped addition: a new taosctl decisions command group with thorough unit tests, and the only non-trivial findings are around consistency with the server-side store (one status value doesn't actually exist on the server) and graceful error handling for user-supplied JSON. Nothing blocks merge on correctness — the command module, route paths, and test coverage all match the existing taosctl pattern.

Issue Highlights

  • _STATUSES includes expired, which the server never sets — --status expired silently returns an empty inbox with no error. Either drop it from choices or wire a real expiry sweeper.
  • Both json.loads(args.options_json) (line 100) and the _coerce_value fallback (line 130) will either surface a raw Python traceback or silently forward a malformed JSON-array answer as a string, masking the actual CLI-layer typo from the user. Both need friendly errors.
  • The "mirrors decision_store" comment on _TYPES is wrong about order (members match; ordering doesn't). Cosmetic but will trip up future grep-based cross-references.
Issue Details (click to expand)

WARNING

File Line Issue
tinyagentos/cli/taosctl/commands/decisions.py 21 expired is not a real decision status — the store only writes pending/answered/superseded; --status expired silently returns empty
tinyagentos/cli/taosctl/commands/decisions.py 100 json.loads(args.options_json) raises a raw JSONDecodeError traceback on malformed input; no friendly CLI error or proper exit code
tinyagentos/cli/taosctl/commands/decisions.py 126 _coerce_value silently swallows JSON parse errors and forwards the raw string to the server, masking CLI-layer typos from the user

SUGGESTION

File Line Issue
tinyagentos/cli/taosctl/commands/decisions.py 19 Comment claims _TYPES mirrors decision_store.DECISION_TYPES but ordering differs (members match); misleading for future grep-based cross-references
tinyagentos/cli/taosctl/commands/decisions.py 30 --project silently drops empty/None values (if args.project_id:) — an agent that script-constructs the id can end up with an unfiltered inbox; consider default=None + explicit check, or document the behavior
Files Reviewed (2 files)
  • tinyagentos/cli/taosctl/commands/decisions.py - 5 issues
  • tests/test_taosctl_decisions.py - 0 issues (test coverage is solid: filters, limit validation, value coercion, optional-field omission, error mapping)

Fix these issues in Kilo Cloud


Reviewed by minimax-m3 · Input: 22.7K · Output: 763 · Cached: 87.6K

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
tests/test_taosctl_decisions.py (1)

111-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make POST-call extraction failure explicit for clearer test diagnostics.

Using [...][0] here can produce an unhelpful IndexError. Prefer next(...) (or assert-first) so failures clearly indicate missing /api/decisions calls.

Suggested change
-    body = [c for c in fake.calls if c[1] == "/api/decisions"][0][2]
+    call = next((c for c in fake.calls if c[1] == "/api/decisions"), None)
+    assert call is not None, fake.calls
+    body = call[2]
🤖 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 `@tests/test_taosctl_decisions.py` at line 111, The line extracting the body
variable using list indexing with [0] will produce an unhelpful IndexError if no
calls match the /api/decisions filter. Replace the list comprehension indexing
pattern (where you filter fake.calls for c[1] == "/api/decisions" and then
access [0][2]) with the next() function instead, which will provide a clearer
error message indicating that the expected /api/decisions call is missing from
the test data.

Source: Linters/SAST tools

🤖 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 `@tinyagentos/cli/taosctl/commands/decisions.py`:
- Around line 126-128: In the condition check at line 126 in the decisions.py
file, modify the if statement that currently checks if text[:1] is in both "["
and "{" to only check for "[". Remove "{" from the condition so that the JSON
parsing logic only accepts array-based JSON (starting with "["), not
object-based JSON (starting with "{"). This ensures the coercion behavior
matches the documented multi-select array-based contract and prevents invalid
answer payloads from being generated.
- Around line 100-101: The line parsing options_json does not validate the JSON
structure or handle parsing errors gracefully. Wrap the json.loads call for
args.options_json in a try-except block to catch JSONDecodeError and raise a
clean CLI error with a user-friendly message. Additionally, after successfully
parsing the JSON, validate that the result is a list or array type; if it is
not, raise a CLI error indicating that options_json must be a JSON array. This
ensures malformed JSON and incorrect data types surface as proper CLI errors
rather than uncaught exceptions that crash the command.

---

Nitpick comments:
In `@tests/test_taosctl_decisions.py`:
- Line 111: The line extracting the body variable using list indexing with [0]
will produce an unhelpful IndexError if no calls match the /api/decisions
filter. Replace the list comprehension indexing pattern (where you filter
fake.calls for c[1] == "/api/decisions" and then access [0][2]) with the next()
function instead, which will provide a clearer error message indicating that the
expected /api/decisions call is missing from the test data.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a3c0e026-4930-45c2-aaf2-c1aba3a27a34

📥 Commits

Reviewing files that changed from the base of the PR and between 0fdb721 and 5e86fc3.

📒 Files selected for processing (2)
  • tests/test_taosctl_decisions.py
  • tinyagentos/cli/taosctl/commands/decisions.py

Comment thread tinyagentos/cli/taosctl/commands/decisions.py Outdated
Comment thread tinyagentos/cli/taosctl/commands/decisions.py Outdated
- --options-json: validate as a JSON array at parse time via a new
  argtypes.json_array, so malformed or non-array input is a clean CLI error
  instead of an uncaught json.JSONDecodeError traceback (gitar Edge-Case;
  kilo + coderabbit concur).
- _coerce_value: only coerce a clearly-bracketed array ([...]); a brace-prefixed
  or otherwise non-array answer stays a literal string, and a bracketed-but-
  malformed value surfaces as a clean exit-1 error rather than being silently
  forwarded as a string (kilo + gitar).
- _STATUSES: drop 'expired' -- the store never writes it, so offering it as a
  filter would just return a silently-empty inbox (kilo footgun).
- Correct the _TYPES mirror comment (kilo).
untouched. A bracketed-but-malformed value is a usage error, surfaced
cleanly (exit 1) rather than silently forwarded to the server as a string."""
text = raw.strip()
if text[:1] == "[" and text[-1:] == "]":

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: The new text[:1] == "[" and text[-1:] == "]" check is stricter than the previous heuristic and will reject legitimate free-text answers that happen to start with [ and end with ]. Example: --value '[hello world]' on a free_text decision will now exit 1 with --value looks like a JSON array but is invalid: ... instead of being stored as the literal string [hello world]. The previous code would have fallen through to the return raw branch on a JSONDecodeError; the new code raises a hard error. If the goal is to be stricter about real multi_select answers, consider restricting the heuristic further (e.g. only when the second char is a quote/digit/-/t/f/n, or by attempting json.loads first and checking the resulting type), or document the new restriction in the --value help text so users know to escape or rephrase bracketed free-text answers.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tinyagentos/cli/taosctl/argtypes.py (1)

36-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Preserve exception cause when remapping JSON parse errors.

Use explicit exception chaining so traceback clearly attributes the original JSONDecodeError.

Proposed fix
     try:
         parsed = json.loads(value)
     except json.JSONDecodeError as exc:
-        raise argparse.ArgumentTypeError(f"not valid JSON: {exc}")
+        raise argparse.ArgumentTypeError(f"not valid JSON: {exc}") from exc
🤖 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 `@tinyagentos/cli/taosctl/argtypes.py` around lines 36 - 37, The except block
that catches json.JSONDecodeError and raises argparse.ArgumentTypeError is not
preserving the exception cause, which loses the original traceback context. Fix
this by using Python's explicit exception chaining syntax with the `from`
keyword when raising the ArgumentTypeError, so that the original JSONDecodeError
is preserved as the cause and appears in the traceback for better debugging.

Source: Linters/SAST tools

🤖 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 `@tinyagentos/cli/taosctl/commands/decisions.py`:
- Around line 136-137: In the exception handler where json.JSONDecodeError is
caught as exc and TransportError is raised, add exception chaining by using
`from exc` in the raise statement. Change the raise statement to use `raise
TransportError(...) from exc` instead of just `raise TransportError(...)`. This
preserves the original JSONDecodeError context in the traceback, making it
easier to debug the actual JSON parsing failure that caused the TransportError.

---

Nitpick comments:
In `@tinyagentos/cli/taosctl/argtypes.py`:
- Around line 36-37: The except block that catches json.JSONDecodeError and
raises argparse.ArgumentTypeError is not preserving the exception cause, which
loses the original traceback context. Fix this by using Python's explicit
exception chaining syntax with the `from` keyword when raising the
ArgumentTypeError, so that the original JSONDecodeError is preserved as the
cause and appears in the traceback for better debugging.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d9ca6fbd-36fb-46dd-9d85-237df1c4a0ba

📥 Commits

Reviewing files that changed from the base of the PR and between 5e86fc3 and cc0c500.

📒 Files selected for processing (3)
  • tests/test_taosctl_decisions.py
  • tinyagentos/cli/taosctl/argtypes.py
  • tinyagentos/cli/taosctl/commands/decisions.py

Comment on lines +136 to +137
except json.JSONDecodeError as exc:
raise TransportError(f"--value looks like a JSON array but is invalid: {exc}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Read the file to verify the exact code at lines 136-137
sed -n '130,145p' tinyagentos/cli/taosctl/commands/decisions.py

Repository: jaylfc/taOS

Length of output: 572


🏁 Script executed:

# Check for other similar exception re-raising patterns in the codebase
rg 'raise.*Error.*from' --type py | head -20

Repository: jaylfc/taOS

Length of output: 149


🏁 Script executed:

# Check for JSONDecodeError handling patterns in the codebase
rg 'json\.JSONDecodeError' --type py -A 2 | head -30

Repository: jaylfc/taOS

Length of output: 149


Add exception chaining to preserve JSON parse error context in traceback.

Line 137 raises TransportError without exception chaining, which suppresses the original json.JSONDecodeError context in tracebacks. Use from exc to maintain the full error chain for better debugging.

Suggested fix
     if text[:1] == "[" and text[-1:] == "]":
         try:
             return json.loads(text)
         except json.JSONDecodeError as exc:
-            raise TransportError(f"--value looks like a JSON array but is invalid: {exc}")
+            raise TransportError(
+                f"--value looks like a JSON array but is invalid: {exc}"
+            ) from exc
🧰 Tools
🪛 Ruff (0.15.18)

[warning] 137-137: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

🤖 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 `@tinyagentos/cli/taosctl/commands/decisions.py` around lines 136 - 137, In the
exception handler where json.JSONDecodeError is caught as exc and TransportError
is raised, add exception chaining by using `from exc` in the raise statement.
Change the raise statement to use `raise TransportError(...) from exc` instead
of just `raise TransportError(...)`. This preserves the original JSONDecodeError
context in the traceback, making it easier to debug the actual JSON parsing
failure that caused the TransportError.

Source: Linters/SAST tools

jaylfc added 2 commits June 23, 2026 18:40
My previous _coerce_value tightening hard-errored on a legitimate free_text
answer like '[hello world]' (looks bracketed, not valid JSON). The CLI cannot
see the decision type, so coerce only when the value parses as a JSON list;
non-JSON or non-list values pass through as the literal string and a malformed
multi_select array is rejected server-side against the options. Drops the now
unused TransportError import. (kilo review on #1413.)
Align with the convention used by the 11 other command modules (gitar Quality
nit on the sibling observatory PR; same divergence here).
@jaylfc jaylfc enabled auto-merge (squash) June 23, 2026 17:47
@jaylfc jaylfc merged commit d9249bc into dev Jun 23, 2026
8 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in TinyAgentOS Roadmap Jun 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

1 participant