Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 130 additions & 0 deletions tests/test_taosctl_decisions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
"""Tests for the taosctl decisions command group."""
from __future__ import annotations

import pytest

from tinyagentos.cli.taosctl import client as cli_client
from tinyagentos.cli.taosctl import __main__ as cli_main


class _FakeClient:
def __init__(self, *a, **k):
self.calls = []
self.base_url = "http://x"
self.token = "t"
self._raise = None

def get(self, path, params=None):
self.calls.append(("GET", path, params))
if self._raise:
raise self._raise
return {"items": []}

def post(self, path, body=None, params=None, json=None):
self.calls.append(("POST", path, body))
if self._raise:
raise self._raise
return {"id": "d1"}


def _run(monkeypatch, argv, fake):
monkeypatch.setattr(cli_main, "TaosClient", lambda **k: fake)
return cli_main.main(argv)


def test_list_default_sends_limit(monkeypatch):
fake = _FakeClient()
rc = _run(monkeypatch, ["decisions", "list"], fake)
assert rc == 0
assert ("GET", "/api/decisions", {"limit": 200}) in fake.calls


def test_list_filters_status_and_project(monkeypatch):
fake = _FakeClient()
rc = _run(monkeypatch, ["decisions", "list", "--status", "pending",
"--project", "prj-1", "--limit", "5"], fake)
assert rc == 0
assert ("GET", "/api/decisions",
{"limit": 5, "status": "pending", "project_id": "prj-1"}) in fake.calls


def test_list_rejects_nonpositive_limit(monkeypatch):
fake = _FakeClient()
with pytest.raises(SystemExit):
_run(monkeypatch, ["decisions", "list", "--limit", "0"], fake)


def test_get_calls_endpoint(monkeypatch):
fake = _FakeClient()
rc = _run(monkeypatch, ["decisions", "get", "d9"], fake)
assert rc == 0
assert ("GET", "/api/decisions/d9", None) in fake.calls


def test_history_calls_endpoint(monkeypatch):
fake = _FakeClient()
rc = _run(monkeypatch, ["decisions", "history", "d9"], fake)
assert rc == 0
assert ("GET", "/api/decisions/d9/history", None) in fake.calls


def test_answer_single_value_is_a_string(monkeypatch):
fake = _FakeClient()
rc = _run(monkeypatch, ["decisions", "answer", "d9", "--value", "approve"], fake)
assert rc == 0
assert ("POST", "/api/decisions/d9/answer", {"value": "approve"}) in fake.calls


def test_answer_json_array_is_parsed(monkeypatch):
fake = _FakeClient()
rc = _run(monkeypatch, ["decisions", "answer", "d9", "--value", '["a","b"]',
"--answered-by", "jay"], fake)
assert rc == 0
assert ("POST", "/api/decisions/d9/answer",
{"value": ["a", "b"], "answered_by": "jay"}) in fake.calls


def test_post_minimal_free_text(monkeypatch):
fake = _FakeClient()
rc = _run(monkeypatch, ["decisions", "post", "--from-agent", "@taOS-dev",
"--question", "Which path?", "--type", "free_text"], fake)
assert rc == 0
posted = [c for c in fake.calls if c[0] == "POST" and c[1] == "/api/decisions"]
assert posted, fake.calls
body = posted[0][2]
assert body["from_agent"] == "@taOS-dev"
assert body["type"] == "free_text"
assert body["options"] == []
assert body["priority"] == "normal"
# Unset optional fields are omitted, not sent as None.
assert "project_id" not in body


def test_post_select_with_options_and_project(monkeypatch):
fake = _FakeClient()
opts = '[{"label":"A","value":"a","recommended":true,"rationale":"safest"}]'
rc = _run(monkeypatch, ["decisions", "post", "--from-agent", "@taOS-dev",
"--question", "Pick", "--type", "single_select",
"--options-json", opts, "--project", "prj-7",
"--priority", "blocking"], fake)
assert rc == 0
body = [c for c in fake.calls if c[1] == "/api/decisions"][0][2]
assert body["options"][0]["value"] == "a"
assert body["project_id"] == "prj-7"
assert body["priority"] == "blocking"


def test_api_error_maps_to_exit_2(monkeypatch, capsys):
fake = _FakeClient()
fake._raise = cli_client.ApiError(404, "not found")
rc = _run(monkeypatch, ["decisions", "get", "nope"], fake)
assert rc == 2
assert "not found" in capsys.readouterr().err


def test_transport_error_maps_to_exit_1(monkeypatch, capsys):
fake = _FakeClient()
fake._raise = cli_client.TransportError("cannot reach http://x: refused")
rc = _run(monkeypatch, ["decisions", "list"], fake)
assert rc == 1
assert "cannot reach" in capsys.readouterr().err
131 changes: 131 additions & 0 deletions tinyagentos/cli/taosctl/commands/decisions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
"""taosctl decisions -- the human-in-the-loop decision inbox.

Agents post the choices they need from the user (single/multi select, free text,
approve/deny), list what is pending, record an answer, and trace the L1
supersession lineage of a decision. Thin wrapper over the /api/decisions routes;
@taOS-dev uses `list --status pending` to poll for answers between turns.
"""
from __future__ import annotations

import json
from urllib.parse import quote

from ..argtypes import positive_int

NOUN = "decisions"

# 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".

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



def register(subparsers) -> None:
p = subparsers.add_parser(NOUN, help="Post, list, answer, and trace decisions")
verbs = p.add_subparsers(dest="verb", required=True, metavar="<verb>")

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.

lp.add_argument("--limit", type=positive_int, default=200, help="Max rows (default 200)")
lp.set_defaults(func=_list)

gp = verbs.add_parser("get", help="Get one decision by id")
gp.add_argument("decision_id", help="Decision id")
gp.set_defaults(func=_get)

hp = verbs.add_parser("history", help="Show the supersession lineage (oldest first)")
hp.add_argument("decision_id", help="Decision id")
hp.set_defaults(func=_history)

ap = verbs.add_parser("answer", help="Record an answer for a decision")
ap.add_argument("decision_id", help="Decision id")
ap.add_argument("--value", required=True,
help="Answer value. For multi_select pass a JSON array, "
'e.g. \'["a","b"]\'')
ap.add_argument("--answered-by", dest="answered_by",
help="Who answered (defaults to the caller)")
ap.set_defaults(func=_answer)

pp = verbs.add_parser("post", help="Post a new decision to the inbox")
pp.add_argument("--from-agent", dest="from_agent", required=True,
help="Asking agent, e.g. @taOS-dev")
pp.add_argument("--question", required=True, help="The decision question")
pp.add_argument("--type", required=True, choices=list(_TYPES), help="Decision type")
pp.add_argument("--context", default="", help="Supporting detail / the why")
pp.add_argument("--priority", choices=list(_PRIORITIES), default="normal",
help="normal | blocking")
pp.add_argument("--project", dest="project_id", help="Project id this decision is scoped to")
pp.add_argument("--deadline", type=float, help="Optional deadline (epoch seconds)")
pp.add_argument("--parent", dest="parent_decision_id",
help="Parent decision id this one supersedes (L1)")
pp.add_argument("--options-json", dest="options_json",
help="Options as a JSON array of "
"{label,value,recommended,rationale} (select types)")
pp.add_argument("--checkpoint-ref", dest="checkpoint_ref",
help="State pointer the decision is made against (e.g. a git commit)")
pp.add_argument("--timeline-id", dest="timeline_id", help="Timeline/branch this belongs to")
pp.set_defaults(func=_post)


def _list(args, client):
params = {"limit": args.limit}
if args.status:
params["status"] = args.status
if args.project_id:
params["project_id"] = args.project_id
return client.get("/api/decisions", params=params)


def _get(args, client):
return client.get(f"/api/decisions/{quote(str(args.decision_id), safe='')}")


def _history(args, client):
return client.get(f"/api/decisions/{quote(str(args.decision_id), safe='')}/history")


def _answer(args, client):
value = _coerce_value(args.value)
body = {"value": value}
if args.answered_by:
body["answered_by"] = args.answered_by
return client.post(
f"/api/decisions/{quote(str(args.decision_id), safe='')}/answer", body=body
)


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.

@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 👍 / 👎

body = {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
"from_agent": args.from_agent,
"question": args.question,
"type": args.type,
"options": options,
"context": args.context,
"priority": args.priority,
}
for key, val in (
("project_id", args.project_id),
("deadline", args.deadline),
("parent_decision_id", args.parent_decision_id),
("checkpoint_ref", args.checkpoint_ref),
("timeline_id", args.timeline_id),
):
if val is not None:
body[key] = val
return client.post("/api/decisions", body=body)


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 ("[", "{"):

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.

try:
return json.loads(text)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
except json.JSONDecodeError:
return raw
return raw
Comment on lines +125 to +141

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 👍 / 👎

Loading