feat(agent-model-api): agent turn execution slice (rebase) - #2195
feat(agent-model-api): agent turn execution slice (rebase)#2195hognek wants to merge 12 commits into
Conversation
Replace the 501 stub in POST /v1/chat/completions with a real one-shot agent turn via the opencode host-server seam (consent key -> agent -> opencode server + LiteLLM virtual key -> one non-streaming turn -> OpenAI ChatCompletion shape). Reuses ensure_taos_opencode_server and drive_turn from the existing taOS agent chat path. Returns 502 on transport failure; never leaks internals. LIVE ROUND-TRIP: pending consent-key mint (owner session required) — see PR.
Replace the 501-until-implemented test with two mock-driven tests that assert the new turn behaviour headlessly (no opencode binary in CI): a consented request returns the OpenAI ChatCompletion envelope (200), and a transport failure degrades to 502. The runtime calls (ensure_taos_opencode_server, drive_turn) are referenced by module-level name so tests monkeypatch them. Also updates the module docstring which still claimed 501.
…fc#2176) - Per-agent opencode server cache in ensure_taos_opencode_server (dict keyed by agent_id/model) so concurrent agents do not churn a shared singleton and race (Kilo correctness bug). stop_taos_opencode_server stops all entries. - Missing user message -> 400 (invalid_request), not 502 (Kilo). - stream requires explicit JSON boolean; bool('false') no longer coerces to True. - Validate message content is str/list before forwarding to drive_turn. - Add CHANGELOG entry + fix stale 'returns 501' claim in gpu-work-queue.md. - Add test_chat_missing_user_message_returns_400.
Set install.entrypoint to 'python -m taosr1.ui' so the App Store LXC runs the 3-slot assignment UI (the surface Jay opens to attach agents to the 3 Rabbit slots). Slot UI + persistence live in github.com/jaylfc/taOSr1.
This reverts commit 3971883.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe ChangesAgent model turn execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant agent_model_api
participant ensure_taos_opencode_server
participant drive_turn
Client->>agent_model_api: POST /v1/chat/completions
agent_model_api->>ensure_taos_opencode_server: ensure model server
agent_model_api->>drive_turn: drive one agent turn
drive_turn-->>agent_model_api: final reply or error
agent_model_api-->>Client: OpenAI ChatCompletion response or SSE stream
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
|
PR Summary by QodoExecute consented agents through OpenAI chat completions
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Opencode data remains trackable
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
|
||
| data_dir = getattr(app_state, "data_dir", None) | ||
| home = str(data_dir / "taos-agent-opencode") if data_dir else "taos-agent-opencode" | ||
| home = str(data_dir / f"taos-agent-opencode-{model}") if data_dir else f"taos-agent-opencode-{model}" |
There was a problem hiding this comment.
2. Opencode data remains trackable 📜 Skill insight ≡ Correctness
The new per-model opencode home writes runtime files beneath data/taos-agent-opencode-*, but .gitignore contains no matching entry. Generated runtime state can therefore be committed accidentally.
Agent Prompt
## Issue description
Per-model opencode runtime directories under `data/` are not ignored by Git.
## Issue Context
Add an ignore pattern covering every dynamically named `taos-agent-opencode-*` directory without ignoring unrelated data.
## Fix Focus Areas
- .gitignore[18-35]
- tinyagentos/taos_agent_runtime.py[133-134]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| parts.append(p) | ||
| user_text = " ".join(parts) | ||
| else: | ||
| # content is int/null/object — malformed request. |
There was a problem hiding this comment.
3. Comment contains em dash 📜 Skill insight ✧ Quality
The newly added malformed-content comment contains the prohibited em dash character —. The rule explicitly applies to code comments.
Agent Prompt
## Issue description
A newly added code comment uses an em dash prohibited in public-facing text.
## Issue Context
Replace the em dash with a regular hyphen or rewrite the sentence without changing behavior.
## Fix Focus Areas
- tinyagentos/routes/agent_model_api.py[212-212]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| trace_id=None, | ||
| sink=_sink, | ||
| base_url=server.base_url, | ||
| model_id=agent_id, |
There was a problem hiding this comment.
4. Agent id used as model 🐞 Bug ≡ Correctness
The endpoint passes the consented agent identifier directly as the OpenCode/LiteLLM model ID instead of resolving and executing that agent's configured harness. A normal agent such as agent-a configured with model gpt-4o therefore requests nonexistent model agent-a and the advertised agent turn fails.
Agent Prompt
## Issue description
The Agent-as-a-Model endpoint passes an agent identifier as the OpenCode/LiteLLM model identifier. Resolve the consented agent and execute its configured framework/runtime with its configured model instead.
## Issue Context
Agent identity and backend model identity are separate concepts. Existing routing resolves an agent record before selecting the framework and configured model.
## Fix Focus Areas
- tinyagentos/routes/agent_model_api.py[180-242]
- tinyagentos/agent_chat_router.py[181-190]
- tinyagentos/agent_chat_router.py[232-288]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| servers = getattr(app_state, "taos_opencode_servers", None) | ||
| if servers is None: | ||
| servers = {} | ||
| app_state.taos_opencode_servers = servers |
There was a problem hiding this comment.
6. Cold-start cache races 🐞 Bug ☼ Reliability
The new server cache performs an unsynchronized check-then-create sequence containing multiple awaits, so concurrent first requests for one model can create and start duplicate servers before overwriting the same cache entry. This can leak an untracked process, duplicate key operations, or fail both turns through home and port contention.
Agent Prompt
## Issue description
Concurrent cold starts can both observe a cache miss and initialize the same model. Add per-model synchronization or cache a shared initialization task, including cleanup on failed initialization.
## Issue Context
The cache lookup occurs before several awaited settings and key operations, and insertion happens only afterward. Different models may initialize concurrently, but one model must have exactly one in-flight initialization.
## Fix Focus Areas
- tinyagentos/taos_agent_runtime.py[41-53]
- tinyagentos/taos_agent_runtime.py[76-150]
- tinyagentos/taos_agent_runtime.py[163-182]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # The last user message is the prompt; system/earlier messages are context | ||
| # the agent harness already carries per-turn, so we pass the latest user text. |
There was a problem hiding this comment.
8. Conversation context is discarded 🐞 Bug ≡ Correctness
_run_agent_turn forwards only the last user message and drops every system, assistant, and earlier user message from the OpenAI request. Because drive_turn constructs a fresh adapter and session for this invocation, the omitted messages are unavailable and multi-turn prompts or system instructions produce incorrect responses.
Agent Prompt
## Issue description
Preserve the supplied OpenAI message history, including system instructions and prior assistant/user turns, when driving the agent turn.
## Issue Context
The current implementation reduces the request to one string while the runtime creates a new adapter session. Convert supported content parts without dropping message roles or earlier turns.
## Fix Focus Areas
- tinyagentos/routes/agent_model_api.py[180-237]
- tinyagentos/opencode_runtime.py[377-399]
- tinyagentos/agent_chat_router.py[165-224]
- tinyagentos/agent_chat_router.py[272-285]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # opencode server, so we resolve it the same way the chat endpoint does. | ||
| # `stream` must be an explicit JSON boolean; a string like "false" must | ||
| # NOT coerce to True (bool("false") is True). Kilo finding. | ||
| stream = body.get("stream") is True |
There was a problem hiding this comment.
9. Non-boolean stream accepted 🐞 Bug ≡ Correctness
body.get("stream") is True silently treats strings, numbers, objects, and null as non-streaming
instead of rejecting them. This violates the PR's explicit JSON-boolean contract and returns
successful completions for malformed requests.
Agent Prompt
## Issue description
Validate that a present `stream` field is a JSON boolean and return an OpenAI-shaped 400 response for every other type.
## Issue Context
An absent field may retain the non-streaming default, but present strings, numbers, null, arrays, and objects must not be silently coerced to false.
## Fix Focus Areas
- tinyagentos/routes/agent_model_api.py[119-156]
- tests/test_routes_agent_model_api.py[89-195]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@hognek red on a genuine conflict-resolution miss, not infra. Two failures in tests/test_taos_agent_chat.py: AttributeError, SimpleNamespace has no attribute taos_opencode_born_degraded. That state field landed on dev while #2176 sat conflicted; your rebase kept the old app.state initialization and dropped it. Restore dev's born-degraded plumbing in the rebased initialization path (diff your branch's app-state setup against origin/dev to find what else the resolution lost), rerun those two tests locally, push. Same branch, fix-forward. Kilo red is rate-limit, ignore it. Once green with a real bot review I verify against the #2176 conditions (my earlier re-review there listed the substance as met) and merge; #2176 closes superseded at that point, not before. |
The rebase onto origin/dev dropped the app_state attachment for the
per-model taos_opencode_born_degraded dict. getattr(..., {}) created a
temporary dict that was never stored on app_state, so downstream code
mutated a throwaway and tests saw AttributeError on SimpleNamespace.
Fix: initialize born_degraded with the same None-guard pattern used
for servers and sessions, attaching the fresh dict to app_state.
Update the two degraded-birth/self-heal tests to use dict access
(taos_opencode_born_degraded['gpt-4o']) now that the flag is per-model.
|
Fixed: the rebase dropped |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| app_state.taos_opencode_sessions = sessions | ||
|
|
||
| existing: OpenCodeServer | None = servers.get(model) | ||
| born_degraded = getattr(app_state, "taos_opencode_born_degraded", None) |
There was a problem hiding this comment.
CRITICAL: born_degraded = True at line 118 overwrites this dict with a boolean, causing AttributeError at line 149
born_degraded is correctly initialized as a dict here, but the legacy born_degraded = True assignment at line 118 (inside the if stored_key: re-scoping block) overwrites the dict with a bare boolean when key re-scoping returns False. Line 149 then calls born_degraded.get(model, False), which crashes with AttributeError: 'bool' object has no attribute 'get'.
| born_degraded = getattr(app_state, "taos_opencode_born_degraded", None) | |
| born_degraded[model] = True |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous Review Summaries (3 snapshots, latest commit 5e29bbc)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5e29bbc)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 8b20b6d)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous review (commit b556457)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (3 files)
Reviewed by step-3.7-flash · Input: 85.1K · Output: 20.6K · Cached: 798.7K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/taos_agent_runtime.py (1)
105-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
born_degraded = Truereassigns the dict, crashing on the nextborn_degraded[model] = ....At line 118, on the "re-scoping stored key failed/returned False" branch, the local name
born_degraded(the per-model dict from line 54-57) is reassigned to the booleanTrue. This isn't inside the surroundingtryfor that specific statement's later use — it's outside thetry/exceptat line 149 (born_degraded[model] = born_degraded.get(model, False) or born_degraded_now), so any request that reaches this branch now raisesTypeError: 'bool' object does not support item assignment, turning a "key scope may be stale" warning into an unhandled crash insideensure_taos_opencode_server(surfacing as a 502/503 to callers instead of the intended graceful degraded-mode continuation).This looks like leftover code from the pre-refactor singleton-boolean pattern that wasn't updated for the new per-model dict.
🐛 Proposed fix
if not rescoped: logger.warning( "taos_agent_runtime: re-scoping the taOS agent key returned False " "(key scope may be stale)" ) - born_degraded = True + born_degraded[model] = TrueWould you like a regression test added for the
update_agent_keyreturns-False path?🤖 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/taos_agent_runtime.py` around lines 105 - 149, In the stored-key re-scoping branch of ensure_taos_opencode_server, replace the boolean reassignment of born_degraded with an update to the existing per-model degraded-state tracking, preserving the later born_degraded[model] assignment and graceful continuation when update_agent_key returns False. Do not overwrite the born_degraded dictionary.
🧹 Nitpick comments (1)
tinyagentos/routes/agent_model_api.py (1)
94-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale docstring: still describes the old 501 behavior.
The docstring says "a contract-valid request returns 501 rather than a fabricated completion," but the code right below it (143-170) now actually drives a real agent turn and returns a completion. Worth updating to avoid confusing future readers.
📝 Proposed fix
- Enforces the consent contract: a valid key is required, and the requested - model must be one of the agents that key is consented for. Running the turn - through the agent's harness is the next slice (pending the seam choice), so a - contract-valid request returns 501 rather than a fabricated completion. + Enforces the consent contract: a valid key is required, and the requested + model must be one of the agents that key is consented for. A contract-valid + request drives one non-streaming agent turn through the opencode + host-server seam and returns an OpenAI ChatCompletion envelope.🤖 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/routes/agent_model_api.py` around lines 94 - 106, Update the chat_completions docstring to describe the current behavior: after consent validation, the request is routed through the agent harness and returns a real completion. Remove the outdated statement that contract-valid requests return 501.
🤖 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/taos_agent_runtime.py`:
- Around line 148-158: Update the session propagation logic in
ensure_taos_opencode_server so app_state.taos_opencode_session_id is assigned
the current model’s session_id on every path, including when sessions.get(model)
returns None. Preserve the existing cached-session value when present, and
explicitly clear the legacy singleton field for models without a cached session
to prevent cross-model leakage.
- Line 137: Sanitize or validate the model/agent identifier in
ensure_taos_opencode_server before constructing the home path, rejecting path
separators and traversal segments or normalizing them to a safe directory-name
form. Apply this consistently to values originating from SettingsPatch.model,
PermittedModelsUpdate.models, and MintIn.agent_ids while preserving valid
identifiers and the existing OpenCodeServerConfig path behavior.
---
Outside diff comments:
In `@tinyagentos/taos_agent_runtime.py`:
- Around line 105-149: In the stored-key re-scoping branch of
ensure_taos_opencode_server, replace the boolean reassignment of born_degraded
with an update to the existing per-model degraded-state tracking, preserving the
later born_degraded[model] assignment and graceful continuation when
update_agent_key returns False. Do not overwrite the born_degraded dictionary.
---
Nitpick comments:
In `@tinyagentos/routes/agent_model_api.py`:
- Around line 94-106: Update the chat_completions docstring to describe the
current behavior: after consent validation, the request is routed through the
agent harness and returns a real completion. Remove the outdated statement that
contract-valid requests return 501.
🪄 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: e0b8eb5f-ab8d-49e6-b131-1d43a792fce2
📒 Files selected for processing (7)
CHANGELOG.mddocs/design/2026-07-17-gpu-work-queue.mdtests/test_routes_agent_model_api.pytests/test_taos_agent_chat.pytinyagentos/routes/agent_model_api.pytinyagentos/routes/taos_agent.pytinyagentos/taos_agent_runtime.py
|
Rebase verified clean: range-diff shows 2176's substance carried intact, the born-degraded dict fix is correct, both tests pass on your tree, and nothing from dev is lost. Two substance items before merge, both carried over FROM 2176, one of which is on me:
Adjudicated so you do not chase ghosts: qodo bug 3 (cold-start double-spawn race) real but minor, follow-up; bug 5 unconfirmed and counter-evidenced by the live round-trip; bug 8 cosmetic. Bug 2 (multi-turn context discarded, fresh session per call) is REAL and gets its own card; do not fold it here. Both fixes on this branch, then green + a real bot pass on that head and this merges with #2176 closing superseded. |
… stop-on-model-change - Line 118: born_degraded = True rebinds the per-model dict to a bool, causing .get() at line 149 to raise AttributeError. Fixed to born_degraded_now = True. - Restore stop-on-model-change semantics: all per-model servers share TAOS_OPENCODE_PORT, so stop other models' servers before starting a new one. - Add test_ensure_server_rescope_failure_marks_degraded covering the rescope-failure path (update_agent_key returns False).
|
Both items fixed on the same branch (d35c763):
Targeted tests: 14/14 pass (including new rescope-failure test). Full gate running. |
|
Item (a) is fixed and properly tested: born_degraded_now at line 134, the per-model dict is never rebound, and the new rescope-failure test drives update_agent_key False and asserts the dict entry. 27 tests pass on your tree. Item (b) is HALF restored, and the missing half is user-facing. The reaping loop at lines 80-94 correctly stops and pops other-model servers so the shared port is free, which closes the bind-collision concern. But dev also cleared the legacy session id on model change and that was not restored: dev did: taos_opencode_server = None AND taos_opencode_session_id = None Line 166 only reassigns the legacy attr if session_id is not None, so on a first-use model sessions.get(model) is None and the attr keeps the PREVIOUS model's session id. routes/taos_agent.py:440 feeds that attr into adapter.session_id, and opencode_adapter.ensure_session() returns a preset id WITHOUT validating it exists. Each model has its own opencode home, so the stale id does not exist on the new server: the first desktop chat turn after a model switch drives a nonexistent session, and line 476 writes the stale id straight back so it never self-clears. This is CodeRabbit's Major on taos_agent_runtime.py:174 and I adjudicate it correct. FIX: clear the legacy session id in the reaping loop AND in the self-heal branch, or make line 166 an unconditional assignment from sessions.get(model). Add the stop-on-model-change test that switches models and asserts the attr is cleared; there is currently no such test at all. Not in scope here, already carded or noted: multi-turn context (tsk-qb3f23), concurrent different-model cold-start race, agent-id-as-model, non-boolean stream. Do not fold them. Merge gate note: no bot has reviewed d35c763 either; the comments carrying that SHA are re-anchored older reviews. Re-triggering. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 47 minutes. |
Two changes: 1. Reaping loop (line 94): after stopping+polling other-model servers for port binding, explicitly set app_state.taos_opencode_session_id = None so the desktop chat path does not feed a stale session into the new server. 2. Line 170-174: make the legacy attr assignment unconditional — sessions.get(model) may be None for a first-use model, and the guard previously kept the stale session from the previous model alive. Added test_ensure_server_model_switch_clears_legacy_session_id that switches gpt-4o -> claude-sonnet and asserts the legacy attr is None. 15/15 tests pass in test_taos_agent_chat.py.
|
Item (b) stop-on-model-change: legacy session id now cleared on model change (9b0482e). Two changes in
New test: 15/15 |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_taos_agent_chat.py`:
- Around line 481-505: The test currently treats a persistent update_agent_key
failure as born degraded, causing subsequent ensure_taos_opencode_server calls
to recreate the shared server. Update the degradation handling around
ensure_taos_opencode_server to distinguish proxy-startup degradation from
routing-only key-scope failure, preserving the cached server when the proxy is
already running; add a second ensure call and assert it does not restart the
server while retaining the persisted key.
🪄 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: e1b42d83-7c5e-459c-a3b8-9730e93f93dc
📒 Files selected for processing (2)
tests/test_taos_agent_chat.pytinyagentos/taos_agent_runtime.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tinyagentos/taos_agent_runtime.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tinyagentos/routes/agent_model_api.py (1)
28-35: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the import guard's exception type.
Catching bare
Exceptionaround these two imports means any genuine bug insideopencode_runtime/taos_agent_runtime(not just "module absent") silently degradesdrive_turn/ensure_taos_opencode_servertoNone. The failure then only resurfaces later as aTypeError: 'NoneType' object is not callableinside_run_agent_turn, further masked by the outerexcept Exceptioninchat_completionsinto a generic 502 — making the real cause very hard to diagnose in production.♻️ Proposed fix
try: from tinyagentos.opencode_runtime import drive_turn -except Exception: # pragma: no cover - import shape guard +except ImportError: # pragma: no cover - import shape guard drive_turn = None try: from tinyagentos.taos_agent_runtime import ensure_taos_opencode_server -except Exception: # pragma: no cover - import shape guard +except ImportError: # pragma: no cover - import shape guard ensure_taos_opencode_server = None🤖 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/routes/agent_model_api.py` around lines 28 - 35, Replace the broad Exception handlers around the drive_turn and ensure_taos_opencode_server imports with ImportError-specific guards, preserving the None fallback only when the modules cannot be imported. Allow other exceptions raised while importing opencode_runtime or taos_agent_runtime to propagate so the underlying initialization failure remains visible.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/taos_agent_runtime.py`:
- Around line 167-178: Reset the degraded-state flag when rebuilding an evicted
model: in the server-creation path around `servers[model]` and
`born_degraded[model]`, assign `born_degraded[model]` from the current
`born_degraded_now` value instead of OR-ing with the cached flag. Also remove
`born_degraded[other_model]` when the model-switch eviction loop removes that
model’s server and session, and add regression coverage for switching away from
and back to a previously born-degraded model.
---
Nitpick comments:
In `@tinyagentos/routes/agent_model_api.py`:
- Around line 28-35: Replace the broad Exception handlers around the drive_turn
and ensure_taos_opencode_server imports with ImportError-specific guards,
preserving the None fallback only when the modules cannot be imported. Allow
other exceptions raised while importing opencode_runtime or taos_agent_runtime
to propagate so the underlying initialization failure remains visible.
🪄 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: 00b2cace-41aa-4c53-b532-786ebd4cfdcc
📒 Files selected for processing (7)
CHANGELOG.mddocs/design/2026-07-17-gpu-work-queue.mdtests/test_routes_agent_model_api.pytests/test_taos_agent_chat.pytinyagentos/routes/agent_model_api.pytinyagentos/routes/taos_agent.pytinyagentos/taos_agent_runtime.py
…tion leak Three fixes targeting CodeRabbit Major findings (PR jaylfc#2195, review e1b42d83): 1. Remove born_degraded_now=True when update_agent_key returns False while proxy IS running — this is a routing-only key-scope no-op, not a proxy-startup degradation. Marking it degraded caused every ensure_taos_opencode_server call to stop+recreate the server, defeating per-model caching. 2. Pop born_degraded[other_model] in the stop-on-model-change eviction loop (alongside servers+sessions). Without this, an evicted model later re-requested picks up the stale True flag and triggers a spurious self-heal rebuild cycle. 3. born_degraded[model] = born_degraded_now (no OR with stale flag). When building a fresh server, only the current proxy status matters — OR-ing with born_degraded.get(model, False) preserved a stale True from before eviction forever. Test: renamed test_ensure_server_rescope_failure_marks_degraded to test_ensure_server_rescope_failure_keeps_cached_server — now asserts degraded=False when proxy running, and verifies second ensure call reuses the cached server (no restart churn).
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Addendum to the earlier fix list, one NEW blocking item found while adjudicating the bot reviews against head 2a93529. The two routed items (born_degraded, legacy session id) are confirmed FIXED at head; this is separate and introduced by this PR. Path traversal in the opencode home path. Fix both ends: validate For the avoidance of doubt: E (update_agent_key churn) and F (born_degraded OR/eviction) from the 19:0xZ reviews are already fixed at head, do not rework them. D and G are nits, ignore. A full CodeRabbit review has been forced on the current head; expect one more pass of findings after it lands. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/routes/agent_model_api.py (1)
111-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInconsistent
error.codefor the same 400 validation category.The pre-turn validation path uses
code="invalid_request_error"(line 116), while_BadRequestraised from_run_agent_turn(missing/malformed user message) usescode="invalid_request"(line 156). Both are client-side 400s in the "invalid request" family; a caller branching onerror.codewill see two different strings for functionally the same failure class.🔧 Proposed fix (pick one convention)
except _BadRequest as e: - return _openai_error(str(e), type="invalid_request_error", code="invalid_request", status=400) + return _openai_error(str(e), type="invalid_request_error", code="invalid_request_error", status=400)Note:
tests/test_routes_agent_model_api.pyassertscode == "invalid_request"(line 195) and would need updating alongside this fix.Also applies to: 155-156
🤖 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/routes/agent_model_api.py` around lines 111 - 117, Standardize the 400 validation error code across the pre-turn validation path and _BadRequest handling in _run_agent_turn, choosing one consistent invalid-request convention and applying it to both _bad_request and the raised error. Update the affected assertion in tests/test_routes_agent_model_api.py to match the chosen code.</code>
🧹 Nitpick comments (5)
tinyagentos/taos_agent_runtime.py (3)
186-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused loop variable
modelinstop_taos_opencode_server.Flagged by Ruff (B007);
modelisn't referenced in the loop body.🧹 Proposed fix
- for model, server in list(servers.items()): + for _model, server in list(servers.items()):🤖 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/taos_agent_runtime.py` around lines 186 - 205, Update the loop in stop_taos_opencode_server to avoid binding the unused model key, while preserving iteration over all server values and the existing stop and cleanup behavior.Source: Linters/SAST tools
21-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring still describes the old singleton, not the per-model cache.
Says "Stores the server on
app_state.taos_opencode_server" and "the old server is stopped and a new one is created" — this file now maintainsapp_state.taos_opencode_servers/taos_opencode_sessions/taos_opencode_born_degradedas per-model dicts. Worth updating so the doc matches the actual per-model/self-heal contract.📝 Proposed docstring update
- Stores the server on ``app_state.taos_opencode_server``. If the model - changed since last start the old server is stopped and a new one is - created so the LiteLLM provider config and key scope track the chosen model. + Stores the server on ``app_state.taos_opencode_servers[model]`` (one + cached server per model/agent_id). Since all cached servers share + ``TAOS_OPENCODE_PORT``, requesting a different model stops any other + cached server before starting the new one.🤖 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/taos_agent_runtime.py` around lines 21 - 36, Update the ensure_taos_opencode_server docstring to describe the per-model state stored in app_state.taos_opencode_servers, taos_opencode_sessions, and taos_opencode_born_degraded, including per-model replacement and born-degraded self-healing behavior. Remove the outdated singleton wording and preserve the documented permitted_models key scoping and return contract.
155-157: 🚀 Performance & Scalability | 🔵 TrivialPer-model home directories are never cleaned up.
Each distinct
model/agent_idnow gets its own persistedtaos-agent-opencode-{model}directory (previously one shared dir). Stop-on-model-change/eviction only stops the process — it doesn't remove the directory, so long-running deployments with many rotating agent_ids will accumulate opencode state dirs indefinitely.🤖 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/taos_agent_runtime.py` around lines 155 - 157, Update the per-model home-directory lifecycle around the home path construction in the agent runtime so directories created for stopped, changed, or evicted model/agent_id processes are removed during cleanup. Ensure cleanup targets only the corresponding persisted taos-agent-opencode-{model} directory after its process stops, while preserving the current directory selection and active-process behavior.tinyagentos/routes/agent_model_api.py (2)
94-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring is stale: says the endpoint "returns 501," but it now executes a real turn.
This whole slice's point is to replace the 501 stub with actual execution (lines 143-169). The docstring should reflect that this now drives a real turn and describe the resulting error mapping instead.
🤖 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/routes/agent_model_api.py` around lines 94 - 106, The chat_completions docstring still describes the removed 501 stub. Update it to state that contract-valid requests execute a real agent turn, and document the endpoint’s resulting error mapping instead, while retaining the existing authentication and manual body-parsing behavior.
261-269: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSSE stream combines content delta and terminal
finish_reasonin one chunk; also re-importsStreamingResponseunnecessarily.Standard OpenAI streaming clients expect content chunk(s) with
finish_reason: nullfollowed by a separate terminal chunk carryingfinish_reasonwith an emptydelta. Bundling both in a single chunk may not parse cleanly in strict clients. Separately,StreamingResponseis already imported at the module level (line 24); the local import at line 263 is redundant.♻️ Proposed fix
def _chat_completion_stream(content: str, model: str): """OpenAI SSE stream envelope (single completion chunk).""" - from fastapi.responses import StreamingResponse - + completion_id = f"chatcmpl-{_short_id()}" + created = int(time.time()) def _gen(): - yield f"data: {json.dumps({'id': f'chatcmpl-{_short_id()}', 'object': 'chat.completion.chunk', 'created': int(time.time()), 'model': model, 'choices': [{'index': 0, 'delta': {'role': 'assistant', 'content': content}, 'finish_reason': 'stop'}]})}\n\n" + yield f"data: {json.dumps({'id': completion_id, 'object': 'chat.completion.chunk', 'created': created, 'model': model, 'choices': [{'index': 0, 'delta': {'role': 'assistant', 'content': content}, 'finish_reason': None}]})}\n\n" + yield f"data: {json.dumps({'id': completion_id, 'object': 'chat.completion.chunk', 'created': created, 'model': model, 'choices': [{'index': 0, 'delta': {}, 'finish_reason': 'stop'}]})}\n\n" yield "data: [DONE]\n\n" return StreamingResponse(_gen(), media_type="text/event-stream")🤖 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/routes/agent_model_api.py` around lines 261 - 269, Update _chat_completion_stream so the content chunk uses finish_reason: null, then yield a separate terminal SSE chunk with an empty delta and finish_reason set to stop before emitting [DONE]. Remove the redundant local StreamingResponse import and reuse the module-level import.
🤖 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.
Outside diff comments:
In `@tinyagentos/routes/agent_model_api.py`:
- Around line 111-117: Standardize the 400 validation error code across the
pre-turn validation path and _BadRequest handling in _run_agent_turn, choosing
one consistent invalid-request convention and applying it to both _bad_request
and the raised error. Update the affected assertion in
tests/test_routes_agent_model_api.py to match the chosen code.</code>
---
Nitpick comments:
In `@tinyagentos/routes/agent_model_api.py`:
- Around line 94-106: The chat_completions docstring still describes the removed
501 stub. Update it to state that contract-valid requests execute a real agent
turn, and document the endpoint’s resulting error mapping instead, while
retaining the existing authentication and manual body-parsing behavior.
- Around line 261-269: Update _chat_completion_stream so the content chunk uses
finish_reason: null, then yield a separate terminal SSE chunk with an empty
delta and finish_reason set to stop before emitting [DONE]. Remove the redundant
local StreamingResponse import and reuse the module-level import.
In `@tinyagentos/taos_agent_runtime.py`:
- Around line 186-205: Update the loop in stop_taos_opencode_server to avoid
binding the unused model key, while preserving iteration over all server values
and the existing stop and cleanup behavior.
- Around line 21-36: Update the ensure_taos_opencode_server docstring to
describe the per-model state stored in app_state.taos_opencode_servers,
taos_opencode_sessions, and taos_opencode_born_degraded, including per-model
replacement and born-degraded self-healing behavior. Remove the outdated
singleton wording and preserve the documented permitted_models key scoping and
return contract.
- Around line 155-157: Update the per-model home-directory lifecycle around the
home path construction in the agent runtime so directories created for stopped,
changed, or evicted model/agent_id processes are removed during cleanup. Ensure
cleanup targets only the corresponding persisted taos-agent-opencode-{model}
directory after its process stops, while preserving the current directory
selection and active-process behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ef21e7a5-534a-409e-8e90-3c975d9425e6
📒 Files selected for processing (7)
CHANGELOG.mddocs/design/2026-07-17-gpu-work-queue.mdtests/test_routes_agent_model_api.pytests/test_taos_agent_chat.pytinyagentos/routes/agent_model_api.pytinyagentos/routes/taos_agent.pytinyagentos/taos_agent_runtime.py
|
Adjudicated the full CodeRabbit review on head 2a93529: no new blocking items beyond the path traversal already posted (which CodeRabbit did not find). Its per-model home-dir cleanup point is real; fold that into the traversal fix since it touches the same lines. Everything else (error.code inconsistency, B007 loop var, stale docstrings, single-chunk SSE) is nit-level and optional. The blocking list stands: the traversal addendum plus nothing else. |
Validate agent_ids at both the route (Pydantic field_validator) and store (mint()) layers: reject path separators, backslashes, spaces, and anything outside the safe slug set [A-Za-z0-9._-]. The Pydantic layer catches it before the store, the store layer is defense-in-depth for direct callers. Sanitize model name in opencode home path (taos_agent_runtime.py): replace non-safe chars with '_' so 'openai/gpt-4o' → 'openai_gpt-4o' and traversal payloads like '../../x' → '.._.._x' (harmless without real slashes). Add per-model home-dir cleanup in stop_taos_opencode_server so stale configs and serve logs don't accumulate across model switches. Tests: store rejection (7 traversal patterns), route validation (9 patterns), _safe_path_component unit tests (10 patterns). All existing taos_agent_chat tests pass (15/15).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@tests/test_agent_model_keys_route.py`:
- Around line 40-54: Update test_mint_allows_safe_agent_ids to assert the
documented store-initialization failure status, rather than merely checking that
the response is not 422. Alternatively, mock the store initialization and verify
the request reaches it after validation, while retaining coverage for both safe
agent IDs.
- Around line 15-27: Add "." and ".." to the rejected-input parametrization in
tests/test_agent_model_keys_route.py lines 15-27 and the corresponding
store-level parametrization in tests/test_agent_model_key_store.py lines
138-160; make no other changes.
In `@tinyagentos/routes/agent_model_keys.py`:
- Around line 27-40: Reject the exact agent IDs "." and ".." in every validation
layer while preserving the existing safe-slug rules. Update _validate_agent_id
in tinyagentos/routes/agent_model_keys.py:27-40, the store regex/validator in
tinyagentos/agent_model_key_store.py:37-39, and ensure mint() at
tinyagentos/agent_model_key_store.py:115-120 validates before persistence so
neither dot segment can be stored.
🪄 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: 0c58df8c-fefc-4c60-b913-2faeb6e5f36a
📒 Files selected for processing (6)
tests/test_agent_model_key_store.pytests/test_agent_model_keys_route.pytests/test_taos_agent_config.pytinyagentos/agent_model_key_store.pytinyagentos/routes/agent_model_keys.pytinyagentos/taos_agent_runtime.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tinyagentos/taos_agent_runtime.py
| @pytest.mark.parametrize( | ||
| "agent_ids", | ||
| [ | ||
| ["../../x"], | ||
| ["a/b"], | ||
| ["openai/gpt-4o"], # real-world LiteLLM model name with / | ||
| ["a\\b"], | ||
| ["/etc/passwd"], | ||
| ["valid", "../../escape"], | ||
| ["a b"], | ||
| [""], | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover exact dot segments in both regression suites.
tests/test_agent_model_keys_route.py#L15-L27: add"."and".."to the rejected route inputs.tests/test_agent_model_key_store.py#L138-L160: add the same values to the store-level parametrization.
📍 Affects 2 files
tests/test_agent_model_keys_route.py#L15-L27(this comment)tests/test_agent_model_key_store.py#L138-L160
🤖 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_agent_model_keys_route.py` around lines 15 - 27, Add "." and ".."
to the rejected-input parametrization in tests/test_agent_model_keys_route.py
lines 15-27 and the corresponding store-level parametrization in
tests/test_agent_model_key_store.py lines 138-160; make no other changes.
| async def test_mint_allows_safe_agent_ids(self, client): | ||
| """Valid slug-format agent ids pass Pydantic validation (the store is | ||
| reached — a store-init error proves validation passed the 422 gate). | ||
| """ | ||
| resp = await client.post( | ||
| "/api/agent-model-keys", | ||
| json={"agent_ids": ["safe-agent-1", "gpt_4o.test"]}, | ||
| ) | ||
| # The store is not initialised in the default conftest client, so we | ||
| # expect 500 (store error), NOT 422 (validation rejection). A 422 | ||
| # here would mean the regex is too strict and safe slugs are blocked. | ||
| assert resp.status_code != 422, ( | ||
| f"safe slugs should not be rejected by validation, " | ||
| f"got {resp.status_code}: {resp.text}" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the expected failure, not merely “not 422.”
!= 422 also passes for unrelated responses such as 401 or 404. Assert the documented store-initialization response, or mock the store and verify that the request reaches it after validation.
🤖 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_agent_model_keys_route.py` around lines 40 - 54, Update
test_mint_allows_safe_agent_ids to assert the documented store-initialization
failure status, rather than merely checking that the response is not 422.
Alternatively, mock the store initialization and verify the request reaches it
after validation, while retaining coverage for both safe agent IDs.
| # Agent ids are used as filesystem path components (opencode home dirs) and | ||
| # LiteLLM model identifiers; reject traversal, dot-segments, and anything | ||
| # that isn't a safe slug so a malicious mint can't write configs outside | ||
| # the data_dir boundary. Matches the sanitizer in taos_agent_runtime.py. | ||
| _AGENT_ID_RE = re.compile(r"^[A-Za-z0-9._-]+$") | ||
|
|
||
|
|
||
| def _validate_agent_id(v: str) -> str: | ||
| if not _AGENT_ID_RE.match(v): | ||
| raise ValueError( | ||
| f"agent_id {v!r} contains invalid characters; " | ||
| "only A-Z, a-z, 0-9, '.', '_', '-' are allowed" | ||
| ) | ||
| return v |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject exact dot-segment IDs in both validation layers. The shared filesystem-safety contract currently allows "." and "..".
tinyagentos/routes/agent_model_keys.py#L27-L40: reject exact dot segments in_validate_agent_id.tinyagentos/agent_model_key_store.py#L37-L39: update the store regex/validator consistently.tinyagentos/agent_model_key_store.py#L115-L120: ensuremint()rejects them before persistence.
📍 Affects 2 files
tinyagentos/routes/agent_model_keys.py#L27-L40(this comment)tinyagentos/agent_model_key_store.py#L37-L39tinyagentos/agent_model_key_store.py#L115-L120
🤖 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/routes/agent_model_keys.py` around lines 27 - 40, Reject the
exact agent IDs "." and ".." in every validation layer while preserving the
existing safe-slug rules. Update _validate_agent_id in
tinyagentos/routes/agent_model_keys.py:27-40, the store regex/validator in
tinyagentos/agent_model_key_store.py:37-39, and ensure mint() at
tinyagentos/agent_model_key_store.py:115-120 validates before persistence so
neither dot segment can be stored.
|
Red on 8b20b6d is your new validation tests, not the product change: tests/test_agent_model_keys_route.py::TestAgentModelKeysRouteValidation::test_mint_allows_safe_agent_ids fails with RuntimeError: AgentModelKeyStore not initialised, call init() first (multiple shards, same class). Look at how the sibling tests in that file bootstrap the store fixture and use the same path in the new test class. The traversal hardening itself is the right shape from the diff title; I will run the full probe set (mint ../../x rejected, no filesystem write outside data_dir, legit slash-bearing model ids still work) once CI is green. Fix-forward on this branch. |
|
Fix-forward pushed at 5e29bbc: added |
|
Red on 5e29bbc is down to one test and it is a CONTRACT decision, so making the call here: tests/test_routes_agent_model_keys.py::test_mint_without_agent_ids_returns_400 gets 422 because the traversal hardening moved agent_ids validation into the Pydantic model, and Pydantic failures are 422. That pre-existing test IS the public contract for the mint route (taosctl and the UI consume it), so keep 400: leave MintIn permissive (list[str] as before) and do the format validation in the handler or store layer, returning 400 with a clear error naming the offending id, exactly where the old non-emptiness check lived. Same traversal protection, same red-first tests, no contract change. Do not update the old test to expect 422; a stricter body schema silently changing an error contract is how API consumers break without a changelog entry. Everything else from the addendum stands; re-request review when green. |
…dler layer Revert MintIn to permissive list[str] — remove field_validator that caused Pydantic to surface 422 ValidationError instead of 400 on empty/invalid agent_ids. Move the traversal-format check into the mint_key handler inside the existing try/except ValueError block so it returns the original 400 contract. Guards the same surface with the same _validate_agent_id() + non-emptiness checks. Fixes test_mint_without_agent_ids_returns_400 regression from PR jaylfc#2195.
|
Fix pushed: reverted Commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_agent_model_keys_route.py (1)
50-60: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate these tests for the handler-level 400 contract.
Validation was moved out of Pydantic and
mint_keycatchesValueErrorto return HTTP 400. These tests still expect 422, so every unsafe-ID and empty-list case will fail.Proposed test update
- """Pydantic field_validator must reject path traversal before the store.""" + """Handler validation must reject unsafe IDs before minting.""" ... - assert resp.status_code == 422, ( + assert resp.status_code == 400, ( ... - assert resp.status_code == 422 + assert resp.status_code == 400Also applies to: 78-84
🤖 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_agent_model_keys_route.py` around lines 50 - 60, Update test_mint_rejects_unsafe_agent_ids and the related empty-list test to expect HTTP 400 instead of 422, matching mint_key’s ValueError handling after validation moved out of Pydantic. Preserve the existing request payloads and response-detail assertions unless they depend specifically on the old validation status.
🤖 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 `@tests/test_agent_model_keys_route.py`:
- Around line 13-30: Update the _ensure_agent_model_key_store fixture to stop
swallowing exceptions from store.close() during setup and teardown. Let close
failures propagate, or convert the fixture to an async fixture and await init()
and close() directly, ensuring the previous connection is closed before
replacing db_path and teardown failures remain visible.
---
Outside diff comments:
In `@tests/test_agent_model_keys_route.py`:
- Around line 50-60: Update test_mint_rejects_unsafe_agent_ids and the related
empty-list test to expect HTTP 400 instead of 422, matching mint_key’s
ValueError handling after validation moved out of Pydantic. Preserve the
existing request payloads and response-detail assertions unless they depend
specifically on the old validation status.
🪄 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: 517e8cfc-83bc-449f-bff4-21b318b95580
📒 Files selected for processing (2)
tests/test_agent_model_keys_route.pytinyagentos/routes/agent_model_keys.py
| @pytest.fixture(autouse=True) | ||
| def _ensure_agent_model_key_store(client, tmp_path_factory): | ||
| """Init app.state.agent_model_keys on a fresh DB; the test client registers | ||
| the store but does not run the lifespan that init()s it (production does).""" | ||
| store = client._transport.app.state.agent_model_keys | ||
| if store._db is not None: | ||
| try: | ||
| asyncio.get_event_loop().run_until_complete(store.close()) | ||
| except Exception: | ||
| pass | ||
| tmp_dir = tmp_path_factory.mktemp("agent_model_keys_route_test") | ||
| store.db_path = tmp_dir / "agent_model_keys.db" | ||
| asyncio.get_event_loop().run_until_complete(store.init()) | ||
| yield | ||
| try: | ||
| asyncio.get_event_loop().run_until_complete(store.close()) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not silently suppress store-close failures.
If close() fails during setup, the fixture replaces db_path while the previous connection may remain open; teardown can likewise pass while leaking resources. Let cleanup failures surface, or convert this to an async fixture and await init()/close() directly.
🧰 Tools
🪛 Ruff (0.16.0)
[error] 21-22: try-except-pass detected, consider logging the exception
(S110)
[warning] 21-21: Do not catch blind exception: Exception
(BLE001)
[error] 29-30: try-except-pass detected, consider logging the exception
(S110)
[warning] 29-29: Do not catch blind exception: Exception
(BLE001)
🤖 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_agent_model_keys_route.py` around lines 13 - 30, Update the
_ensure_agent_model_key_store fixture to stop swallowing exceptions from
store.close() during setup and teardown. Let close failures propagate, or
convert the fixture to an async fixture and await init() and close() directly,
ensuring the previous connection is closed before replacing db_path and teardown
failures remain visible.
Source: Linters/SAST tools
|
Red on 7309d27 is the last self-inconsistency and it is your own tests from the previous round: the handler now correctly returns 400 with a clear message (exactly as routed), but TestAgentModelKeysRouteValidation::test_mint_rejects_unsafe_agent_ids still asserts the 422 from when validation lived in Pydantic. Update those parametrized assertions to expect 400 and, while there, assert the error body contains the offending id string, which pins the useful half of the message. The rejection set itself is right, including slash-bearing values like openai/gpt-4o: agent_ids are identity slugs and stay strict at mint; the slash-tolerant slugification belongs only to the runtime path-build for MODEL components, which your 8b20b6d already handles. Once green this PR needs a real bot review on the final head before I run the probe set; request it when CI passes. |
2 participants
Rebase of #2176 onto origin/dev (d728dd8).
Conflict: CHANGELOG.md — dev added push notifications entry, PR added Agent-as-a-Model entry. Kept both.
Supersedes #2176 (head on jaylfc repo, cannot update from fork).
Summary by CodeRabbit
/v1/chat/completionsnow executes a real one-shot “agent-as-a-model” turn and returns an OpenAI-compatible response (no longer501).streamis an explicit JSON boolean.usermessages now return400(invalid_request).502(agent_error).agent_idsagainst traversal/unsafe patterns.