Reconcile doc-review (#1835) into dev: master-only feature discovered at beta.45 promote - #2247
Conversation
📝 WalkthroughWalkthroughAdds SQLite-backed document-review state management, project-scoped HTTP routes, agent-token authorization, application lifecycle integration, registry scope support, desktop Files app integration, documentation, and asynchronous tests. ChangesDocument review workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FilesApp
participant AuthMiddleware
participant project_doc_review
participant DocReviewStore
participant SQLite
FilesApp->>AuthMiddleware: Request document-review state
AuthMiddleware->>project_doc_review: Pass authorized request
project_doc_review->>DocReviewStore: Read or update review
DocReviewStore->>SQLite: Load or persist review state
SQLite-->>DocReviewStore: Return review record
DocReviewStore-->>project_doc_review: Return review data
project_doc_review-->>FilesApp: Return HTTP response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 QodoAdd project doc-review stamps with agent-scoped API + persistence
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
|
Reviewed + lead-completed. The backend half was a byte-perfect port (store/routes/tests identical to master) with correct extras (scope in _ALLOWED_SCOPES + test, docs line). Two notes: (1) the _csrf mount differs from master's bare mount - kept, it is the sibling-router idiom and all 21 doc-review tests pass through the real app with it; (2) the SPA half was missing (FilesApp UI + projects.docReviews client), which would have left the next promote's tree diff dirty - pushed it onto this branch myself (master-verbatim FilesApp hunks, client block matching the promote branch), SPA builds clean. Merging on green. After this + PR 2246 land, |
|
nemotron-ultra-kilo review VERDICT: The implementation is functionally correct with comprehensive tests, but has fragile error handling and a few style concerns.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
Code Review by Qodo
1. Non-atomic state transitions
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| from tinyagentos.auth_context import CurrentUser | ||
| from tinyagentos.routes.projects import _get_owned_project | ||
|
|
There was a problem hiding this comment.
1. Route cross-imports routes.projects 📜 Skill insight ⌂ Architecture
tinyagentos/routes/project_doc_review.py imports _get_owned_project from another route module, creating forbidden coupling and risk of circular imports. This violates the rule prohibiting cross-importing between route modules.
Agent Prompt
## Issue description
`tinyagentos/routes/project_doc_review.py` imports `_get_owned_project` from `tinyagentos/routes/projects.py`, which violates the no-cross-imports rule for route modules.
## Issue Context
Route modules should not depend on other route modules; shared logic should live in a non-route module (e.g. `tinyagentos/projects/...` or `tinyagentos/authz/...`) to avoid circular import risk and improve modularity.
## Fix Focus Areas
- tinyagentos/routes/project_doc_review.py[11-33]
- tinyagentos/routes/projects.py[1-120]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| project_canvas_store = ProjectCanvasStoreImpl(data_dir / "projects.db", broker=project_event_broker) | ||
| doc_review_store = DocReviewStore(data_dir / "projects.db") | ||
| from tinyagentos.decisions.decision_store import DecisionStore |
There was a problem hiding this comment.
4. docreviewstore reuses projects.db 📜 Skill insight ⌂ Architecture
DocReviewStore is instantiated with data_dir / "projects.db", violating the requirement that each store use its own SQLite file and avoid cross-store database sharing. This increases coupling and risks schema collisions across stores.
Agent Prompt
## Issue description
`DocReviewStore` is created using `projects.db`, but compliance requires each store to use its own SQLite file (no shared DB across stores).
## Issue Context
The rule explicitly forbids cross-store DB sharing; even without cross-table queries, sharing the same SQLite file couples schemas and migrations.
## Fix Focus Areas
- tinyagentos/app.py[398-426]
- tinyagentos/projects/doc_review_store.py[1-26]
- tests/test_doc_review.py[1-358]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| """Doc-review stamp store: state machine, actor recording, invalid-transition | ||
| rejects, and project-scoped agent-token gating on writes. | ||
|
|
||
| The store (tinyagentos/projects/doc_review_store.py) is exercised directly for |
There was a problem hiding this comment.
6. Test file name doesn't mirror module 📜 Skill insight ⚙ Maintainability
A new route module tinyagentos/routes/project_doc_review.py was added but the corresponding test file is named tests/test_doc_review.py, not tests/test_project_doc_review.py. This violates the required test naming convention that mirrors module structure.
Agent Prompt
## Issue description
The new route module name `project_doc_review.py` is not mirrored by the test filename; the PR adds `tests/test_doc_review.py` instead of `tests/test_project_doc_review.py`.
## Issue Context
The checklist requires tests to follow `tests/test_<module>.py` mirroring `routes/<module>.py` for discoverability and consistency.
## Fix Focus Areas
- tests/test_doc_review.py[1-358]
- tinyagentos/routes/project_doc_review.py[1-112]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| await self._db.execute( | ||
| f"UPDATE doc_reviews SET {', '.join(sets)} WHERE project_id = ? AND doc_path = ?", | ||
| params, | ||
| ) |
There was a problem hiding this comment.
7. Non-atomic state transitions 🐞 Bug ≡ Correctness
DocReviewStore.set_review_state() validates transitions against a previously-read state but UPDATEs rows without guarding on the prior review_state (and without a transaction), so concurrent writers can persist a transition that would be invalid from the actually-committed state. The same TOCTOU pattern can also raise an unhandled UNIQUE constraint error on concurrent first insert for the same (project_id, doc_path), causing 500s.
Agent Prompt
## Issue description
`DocReviewStore.set_review_state()` performs read/validate/write across multiple statements without atomicity:
- Two concurrent requests can both read the same `review_state`, both pass validation, and then both write; the last write wins even if it represents an invalid transition from the first write’s committed state.
- Two concurrent “first writes” for the same `(project_id, doc_path)` can both observe `existing is None` and attempt `INSERT`, causing an uncaught `aiosqlite.IntegrityError` due to the unique index.
## Issue Context
This store is reachable by both admin sessions and agent tokens (project_doc_review scope), so parallel writes are plausible.
## Fix Focus Areas
- tinyagentos/projects/doc_review_store.py[61-123]
### Implementation notes
Use one of these patterns (either is fine):
1) **Transactional + locking**: `BEGIN IMMEDIATE` around the read/validate/write sequence; on `IntegrityError` rollback and re-read.
2) **Optimistic concurrency**: `UPDATE ... WHERE project_id=? AND doc_path=? AND review_state=?` and check `cursor.rowcount==1`; if 0, re-fetch current row and raise a conflict/ValueError. For insert, either `INSERT ... ON CONFLICT(project_id, doc_path) DO NOTHING` + re-read, or catch `IntegrityError` and re-read.
Also ensure you `ROLLBACK` on exceptions inside the transaction to avoid leaving the connection mid-transaction.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ) as cur: | ||
| rows = await cur.fetchall() | ||
| return [self._row_to_review(r, cur.description) for r in rows] |
There was a problem hiding this comment.
8. Cursor metadata after context 🐞 Bug ☼ Reliability
DocReviewStore.list_reviews() accesses cur.description after the async-with cursor context has exited, which is an unsafe cursor-lifetime pattern and can break list responses depending on cursor finalization behavior. Elsewhere in the repo, similar code snapshots the description inside the context before exiting.
Agent Prompt
## Issue description
`list_reviews()` uses `cur.description` outside the `async with self._db.execute(...) as cur:` block. After the context exits, the cursor is closed/finalized, so accessing cursor metadata afterward is not a safe contract.
## Issue Context
Several other stores in this repo capture `desc = cur.description` inside the context manager and then use `desc` after the block.
## Fix Focus Areas
- tinyagentos/projects/doc_review_store.py[125-140]
### Implementation notes
Inside each `async with` branch:
- assign `desc = cur.description` before leaving the block
Then return using `desc`:
- `return [self._row_to_review(r, desc) for r in rows]`
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tinyagentos/routes/project_doc_review.py (1)
70-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve exception context on the re-raised
HTTPExceptions.Both
raise HTTPException(...)statements insideexcept ValueError as exc:drop the original traceback context. Addfrom excso the original store-level error stays visible in logs and tracebacks.♻️ Proposed fix
except ValueError as exc: detail = str(exc) if "invalid transition" in detail: - raise HTTPException(status_code=409, detail=detail) - raise HTTPException(status_code=400, detail=detail) + raise HTTPException(status_code=409, detail=detail) from exc + raise HTTPException(status_code=400, detail=detail) 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/routes/project_doc_review.py` around lines 70 - 74, Update both HTTPException raises in the ValueError handler to explicitly chain the original exception with from exc, preserving the store-level traceback while retaining the existing status codes and detail values.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/projects/doc_review_store.py`:
- Around line 51-123: Guard the read-then-write sequence in set_review_state
with an instance asyncio.Lock, initialized as self._write_lock like the existing
AgentGrantsStore.init and UserSharesStore.init patterns. Acquire the lock across
get_review, transition validation, INSERT/UPDATE, commit, and the final read so
concurrent first writes for the same project/document are serialized; preserve
the existing transition behavior and return value.
---
Nitpick comments:
In `@tinyagentos/routes/project_doc_review.py`:
- Around line 70-74: Update both HTTPException raises in the ValueError handler
to explicitly chain the original exception with from exc, preserving the
store-level traceback while retaining the existing status codes and detail
values.
🪄 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: 125eac26-6af9-4dc7-b970-a671448c34b7
📒 Files selected for processing (9)
docs/agent-coordination.mdtests/test_agent_registry.pytests/test_doc_review.pytinyagentos/app.pytinyagentos/auth_middleware.pytinyagentos/projects/doc_review_store.pytinyagentos/routes/__init__.pytinyagentos/routes/agent_registry.pytinyagentos/routes/project_doc_review.py
| async def set_review_state( | ||
| self, | ||
| project_id: str, | ||
| doc_path: str, | ||
| new_state: str, | ||
| actor_id: str, | ||
| ) -> dict: | ||
| if new_state not in VALID_TRANSITIONS: | ||
| raise ValueError(f"invalid review state: {new_state}") | ||
|
|
||
| now = time.time() | ||
| existing = await self.get_review(project_id, doc_path) | ||
|
|
||
| if existing is None: | ||
| if new_state != "awaiting_review" and new_state not in VALID_TRANSITIONS.get("awaiting_review", []): | ||
| raise ValueError( | ||
| f"invalid transition: (new) -> {new_state}; " | ||
| f"first state must be awaiting_review or a direct transition target" | ||
| ) | ||
| review_id = new_id("rev") | ||
| reviewed_by = None | ||
| reviewed_at = None | ||
| changes_requested_by = None | ||
| changes_requested_at = None | ||
| if new_state == "approved": | ||
| reviewed_by = actor_id | ||
| reviewed_at = now | ||
| elif new_state == "changes_requested": | ||
| changes_requested_by = actor_id | ||
| changes_requested_at = now | ||
| await self._db.execute( | ||
| """INSERT INTO doc_reviews | ||
| (id, project_id, doc_path, review_state, | ||
| reviewed_by, reviewed_at, | ||
| changes_requested_by, changes_requested_at, | ||
| created_at, updated_at) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)""", | ||
| ( | ||
| review_id, project_id, doc_path, new_state, | ||
| reviewed_by, reviewed_at, | ||
| changes_requested_by, changes_requested_at, | ||
| now, now, | ||
| ), | ||
| ) | ||
| await self._db.commit() | ||
| return await self.get_review(project_id, doc_path) | ||
|
|
||
| current_state = existing["review_state"] | ||
| allowed = VALID_TRANSITIONS.get(current_state, []) | ||
| if new_state not in allowed: | ||
| raise ValueError( | ||
| f"invalid transition: {current_state} -> {new_state}" | ||
| ) | ||
|
|
||
| sets: list[str] = ["review_state = ?", "updated_at = ?"] | ||
| params: list = [new_state, now] | ||
|
|
||
| if new_state == "approved": | ||
| sets.append("reviewed_by = ?") | ||
| sets.append("reviewed_at = ?") | ||
| params.extend([actor_id, now]) | ||
| elif new_state == "changes_requested": | ||
| sets.append("changes_requested_by = ?") | ||
| sets.append("changes_requested_at = ?") | ||
| params.extend([actor_id, now]) | ||
|
|
||
| params.extend([project_id, doc_path]) | ||
| await self._db.execute( | ||
| f"UPDATE doc_reviews SET {', '.join(sets)} WHERE project_id = ? AND doc_path = ?", | ||
| params, | ||
| ) | ||
| await self._db.commit() | ||
| return await self.get_review(project_id, doc_path) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard set_review_state against a concurrent first-write race.
set_review_state reads existing and then inserts or updates without holding a lock. Two concurrent calls for the same new (project_id, doc_path) can both see existing is None at line 64. Both then attempt the INSERT at line 81. The idx_doc_reviews_project_path unique index (lines 21-22) rejects the second insert with aiosqlite.IntegrityError. The route only catches ValueError (tinyagentos/routes/project_doc_review.py lines 66-74), so this exception propagates as an unhandled 500 instead of a clean 409 or a correct state transition.
Other stores in this codebase (for example AgentGrantsStore.init, UserSharesStore.init) create a self._write_lock = asyncio.Lock() and hold it across read-then-write sequences for this exact reason. Add the same pattern here.
🔒 Proposed fix: serialize read-then-write with a lock
+import asyncio
import time
from tinyagentos.base_store import BaseStore
from tinyagentos.projects.ids import new_id
@@
class DocReviewStore(BaseStore):
SCHEMA = DOC_REVIEW_SCHEMA
+ async def init(self) -> None:
+ await super().init()
+ self._write_lock = asyncio.Lock()
+
def _row_to_review(self, row, description) -> dict:
keys = [d[0] for d in description]
return dict(zip(keys, row))
@@
async def set_review_state(
self,
project_id: str,
doc_path: str,
new_state: str,
actor_id: str,
) -> dict:
if new_state not in VALID_TRANSITIONS:
raise ValueError(f"invalid review state: {new_state}")
- now = time.time()
- existing = await self.get_review(project_id, doc_path)
+ async with self._write_lock:
+ now = time.time()
+ existing = await self.get_review(project_id, doc_path)
+ return await self._apply_state_change(
+ project_id, doc_path, new_state, actor_id, now, existing
+ )🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 118-121: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🪛 Ruff (0.16.0)
[error] 119-119: Possible SQL injection vector through string-based query construction
(S608)
🤖 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/projects/doc_review_store.py` around lines 51 - 123, Guard the
read-then-write sequence in set_review_state with an instance asyncio.Lock,
initialized as self._write_lock like the existing AgentGrantsStore.init and
UserSharesStore.init patterns. Acquire the lock across get_review, transition
validation, INSERT/UPDATE, commit, and the final read so concurrent first writes
for the same project/document are serialized; preserve the existing transition
behavior and return value.
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)
desktop/src/apps/FilesApp.tsx (1)
824-834: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore trash-specific error state for empty-trash failures.
handleEmptyWorkspaceTrash's catch block now callssetError(msg)instead ofsetWorkspaceTrashError(msg). Every other trash action (handleWorkspaceTrashRestore,handleWorkspaceTrashPurge) still usessetWorkspaceTrashError.This action is only triggered from the Recycle Bin view (
recycleBinUI, rendered whenlocation === "recycle", Line 1918).recycleBinUI's error banner readsrecycleError || workspaceTrashError(Lines 1347-1350); it does not read the generalerrorstate, which is only rendered inmainContentUI(Lines 1596-1602). SincemainContentUIis not shown whilelocation === "recycle", a failed "Empty Trash" action now sets an error that is never displayed to the user.🔧 Proposed fix: use the trash-specific error state
} catch (e: unknown) { - const msg = e instanceof Error ? e.message : "Empty trash failed"; - setError(msg); + const msg = e instanceof Error ? e.message : "Empty trash failed"; + setWorkspaceTrashError(msg); } }, [workspaceTrashItems.length, fetchWorkspaceTrash]);🤖 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 `@desktop/src/apps/FilesApp.tsx` around lines 824 - 834, Update the catch block in handleEmptyWorkspaceTrash to call setWorkspaceTrashError(msg) instead of setError(msg), ensuring empty-trash failures appear in the recycleBinUI error banner alongside other trash actions.
🧹 Nitpick comments (1)
desktop/src/lib/projects.ts (1)
359-374: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply
DocReviewStateconsistently instead ofstring. The exportedDocReviewStateunion ("awaiting_review" | "approved" | "changes_requested") is defined inprojects.tsand already used inFilesApp.tsx'scycleDocReview, but the client API and UI props widen it back tostring, losing compile-time protection against invalid state values.
desktop/src/lib/projects.ts#L359-L374: typelist'sstate?: stringandset'sstate: stringparameters asDocReviewState.desktop/src/apps/FilesApp.tsx#L378-L380: typeFileRowProps.reviewState(andReviewBadge'sstateprop) asDocReviewState | nullinstead ofstring | null.🤖 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 `@desktop/src/lib/projects.ts` around lines 359 - 374, Replace the widened string state types with the existing DocReviewState union: update list’s optional state and set’s state in desktop/src/lib/projects.ts lines 359-374, and update FileRowProps.reviewState and ReviewBadge.state in desktop/src/apps/FilesApp.tsx lines 378-380 to DocReviewState | null. Preserve the existing API behavior while enforcing valid review states at compile time.
🤖 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 `@desktop/src/apps/FilesApp.tsx`:
- Around line 302-345: Update ReviewBadge’s onClick rendering to use a span with
role="button" and tabIndex={0} instead of a nested button, preserving click
handling and adding keyboard activation for Enter and Space. Keep the
non-interactive span path unchanged, and apply the same pattern to the
corresponding grid file-card usage.
---
Outside diff comments:
In `@desktop/src/apps/FilesApp.tsx`:
- Around line 824-834: Update the catch block in handleEmptyWorkspaceTrash to
call setWorkspaceTrashError(msg) instead of setError(msg), ensuring empty-trash
failures appear in the recycleBinUI error banner alongside other trash actions.
---
Nitpick comments:
In `@desktop/src/lib/projects.ts`:
- Around line 359-374: Replace the widened string state types with the existing
DocReviewState union: update list’s optional state and set’s state in
desktop/src/lib/projects.ts lines 359-374, and update FileRowProps.reviewState
and ReviewBadge.state in desktop/src/apps/FilesApp.tsx lines 378-380 to
DocReviewState | null. Preserve the existing API behavior while enforcing valid
review states at compile time.
🪄 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: 33b64c5b-02ae-413e-bed4-d59a5abcc42f
📒 Files selected for processing (2)
desktop/src/apps/FilesApp.tsxdesktop/src/lib/projects.ts
| const REVIEW_BADGE_STYLES: Record<string, string> = { | ||
| approved: "bg-emerald-500/15 text-emerald-400 border-emerald-500/20", | ||
| changes_requested: "bg-amber-500/15 text-amber-400 border-amber-500/20", | ||
| awaiting_review: "bg-zinc-500/10 text-zinc-400 border-zinc-500/15", | ||
| }; | ||
|
|
||
| const REVIEW_BADGE_LABEL: Record<string, string> = { | ||
| approved: "Approved", | ||
| changes_requested: "Changes requested", | ||
| awaiting_review: "Awaiting review", | ||
| }; | ||
|
|
||
| function ReviewBadge({ | ||
| state, | ||
| onClick, | ||
| }: { | ||
| state: string | null; | ||
| onClick?: () => void; | ||
| }) { | ||
| if (!state) return null; | ||
| const style = REVIEW_BADGE_STYLES[state] ?? REVIEW_BADGE_STYLES.awaiting_review; | ||
| const label = REVIEW_BADGE_LABEL[state] ?? state; | ||
| if (onClick) { | ||
| return ( | ||
| <button | ||
| type="button" | ||
| onClick={onClick} | ||
| title={`Review state: ${label} (click to change)`} | ||
| className={`shrink-0 text-[10px] px-1.5 py-0.5 rounded-full border ${style} hover:brightness-125 transition`} | ||
| > | ||
| {label} | ||
| </button> | ||
| ); | ||
| } | ||
| return ( | ||
| <span | ||
| className={`shrink-0 text-[10px] px-1.5 py-0.5 rounded-full border ${style}`} | ||
| title={`Review state: ${label}`} | ||
| > | ||
| {label} | ||
| </span> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg 'desktop/src/apps/FilesApp\.tsx$' || true
echo "== outline relevant symbols =="
ast-grep outline desktop/src/apps/FilesApp.tsx --view compact | sed -n '1,220p' || true
echo "== relevant sections =="
sed -n '280,360p' desktop/src/apps/FilesApp.tsx
echo "===== 1640-1775 ====="
sed -n '1640,1775p' desktop/src/apps/FilesApp.tsx
echo "== search ReviewBadge usages in FilesApp =="
rg -n "ReviewBadge|review.*badge|Review badge|review" desktop/src/apps/FilesApp.tsxRepository: jaylfc/taOS
Length of output: 10279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DocReviewBadge section =="
sed -n '430,475p' desktop/src/apps/FilesApp.tsx
echo "== list view containing interactive elements near ReviewBadge =="
sed -n '1775,1835p' desktop/src/apps/FilesApp.tsx
echo "== parse JSX nesting for ReviewBadge onClick usages =="
python3 - <<'PY'
from pathlib import Path
import re
p = Path("desktop/src/apps/FilesApp.tsx")
s = p.read_text()
for var, body in re.findall(r"function\s+([A-Za-z_][A-Za-z0-9_]*)\s*\(\{\s*(.*?)\s*\}\)\s*\{", s, re.S):
if var in {"DocReviewBadge", "FilesApp"}:
print(f"--- {var} ---")
start = s.index(f"function {var}")
brace = s.index("{", s.index("(", start))
depth = 0
for i in range(brace, len(s)):
if s[i] == "{":
depth += 1
elif s[i] == "}":
depth -= 1
if depth == 0:
print(s[start : i+1])
break
PYRepository: jaylfc/taOS
Length of output: 4590
Fix nested <button> inside the grid file-card button.
In grid view, the clickable ReviewBadge with onClick is rendered inside the file-open <button>, which makes the card button become invalid HTML. Use the same non-button interactive pattern as the delete overlay: a <span role="button" tabIndex={0} ...> with keyboard activation.
🔧 Proposed fix
if (onClick) {
return (
- <button
- type="button"
+ <span
+ role="button"
+ tabIndex={0}
onClick={onClick}
+ onKeyDown={(e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ onClick();
+ }
+ }}
title={`Review state: ${label} (click to change)`}
className={`shrink-0 text-[10px] px-1.5 py-0.5 rounded-full border ${style} hover:brightness-125 transition`}
>
{label}
- </button>
+ </span>
);
}Also applies to: 1712-1725
🤖 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 `@desktop/src/apps/FilesApp.tsx` around lines 302 - 345, Update ReviewBadge’s
onClick rendering to use a span with role="button" and tabIndex={0} instead of a
nested button, preserving click handling and adding keyboard activation for
Enter and Space. Keep the non-interactive span path unchanged, and apply the
same pattern to the corresponding grid file-card usage.
Completes the reconciliation: the backend port alone would have left the SPA half master-only and the next promote's tree diff dirty. FilesApp hunks are master's verbatim; the client block and types match the beta.45 promote branch (master's redundant docReview singular block deliberately omitted). SPA builds clean.
…lary + changelog line The scope-parity test rightly caught _ALLOWED_SCOPES diverging from VALID_SCOPES; the doc-gate rightly demanded the changelog entry.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tinyagentos/routes/project_doc_review.py (2)
17-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestrict
stateto the known review states at the schema level.
DocReviewUpdate.stateaccepts anystr. Restricting it to aLiteralof the valid states documents the accepted values in the OpenAPI schema and lets FastAPI reject typos with a 422 before the request reaches the store.♻️ Proposed refactor
+from typing import Literal + class DocReviewUpdate(BaseModel): - state: str = Field(..., description="Target review state") + state: Literal["awaiting_review", "approved", "changes_requested"] = Field( + ..., description="Target review state" + )🤖 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/project_doc_review.py` around lines 17 - 18, Update the DocReviewUpdate.state field to use a Literal containing the known valid review-state values instead of unrestricted str, and add or reuse the necessary typing import. Preserve the existing field description so FastAPI and OpenAPI expose the accepted states and reject invalid values during request validation.
21-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the bare tuple return with a typed structure.
_authorize_doc_review_actorreturnstuple[str, bool, dict] | JSONResponse. At both call sites (actor_id, _is_agent, _project = auth), the reader must go back to this function to learn what each position means. A small dataclass (orNamedTuple) documents the fields once and removes the need to infer them from destructuring order.♻️ Proposed refactor
+from dataclasses import dataclass + + +@dataclass +class AuthorizedDocReviewActor: + actor_id: str + is_agent: bool + project: dict + + async def _authorize_doc_review_actor( request: Request, pstore, project_id: str -) -> "tuple[str, bool, dict] | JSONResponse": +) -> "AuthorizedDocReviewActor | JSONResponse": uid = getattr(request.state, "user_id", None) if uid: user = CurrentUser( user_id=uid, is_admin=bool(getattr(request.state, "is_admin", False)), ) project_or_err = await _get_owned_project(pstore, project_id, user) if isinstance(project_or_err, JSONResponse): return project_or_err - return (user.user_id, False, project_or_err) + return AuthorizedDocReviewActor(user.user_id, False, project_or_err)🤖 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/project_doc_review.py` around lines 21 - 48, Replace the positional tuple returned by _authorize_doc_review_actor with a small typed structure, such as a dataclass or NamedTuple, exposing named fields for actor ID, agent status, and project. Update both call sites that currently destructure auth into actor_id, _is_agent, and _project to access the corresponding named fields while preserving the existing JSONResponse error returns and values.tests/test_doc_review.py (1)
287-358: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd HTTP-level coverage for the list route's project-boundary and filter behavior.
TestAgentScopeGatingcovers the PUT and single-doc GET routes thoroughly, but no test exercisesGET /api/projects/{project_id}/doc-reviewswith an agent token bound to a different project, and no test exercises thestatequery filter through the HTTP route (onlytest_list_reviews_filters_by_statecovers this at the store level, lines 144-156). Add a test mirroringtest_agent_other_project_is_404for the list endpoint, and one owner-session test that filters/doc-reviews?state=approvedover HTTP.🤖 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_doc_review.py` around lines 287 - 358, Add HTTP-level tests to TestAgentScopeGating for GET /api/projects/{project_id}/doc-reviews: verify an agent token bound to another project receives the same existence-hiding 404 as test_agent_other_project_is_404, and verify an owner session requesting ?state=approved returns only approved reviews. Use the existing project, authentication, and response assertion helpers while preserving current route behavior.tinyagentos/projects/doc_review_store.py (1)
64-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the first-write validation; it is currently unreachable.
Trace the two guards together. Line 58-59 already rejects any
new_statenot inVALID_TRANSITIONS(its three keys:awaiting_review,changes_requested,approved). By the time execution reaches line 65,new_statecan only be one of those three values. The conditionnew_state != "awaiting_review" and new_state not in VALID_TRANSITIONS.get("awaiting_review", [])then requiresnew_stateto be neither"awaiting_review"nor in["approved", "changes_requested"]— but those three values are exhaustive, so the branch can never raise today.This is not a functional bug (tests confirm every first-write case currently succeeds), but the intent — guarding against a future terminal state that should not be a valid initial state — is hard to read from this expression. Rewrite it to state the intent directly.
♻️ Proposed clarification
if existing is None: - if new_state != "awaiting_review" and new_state not in VALID_TRANSITIONS.get("awaiting_review", []): - raise ValueError( - f"invalid transition: (new) -> {new_state}; " - f"first state must be awaiting_review or a direct transition target" - ) + allowed_initial_states = {"awaiting_review", *VALID_TRANSITIONS["awaiting_review"]} + if new_state not in allowed_initial_states: + raise ValueError( + f"invalid transition: (new) -> {new_state}; " + f"first state must be one of {sorted(allowed_initial_states)}" + )🤖 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/projects/doc_review_store.py` around lines 64 - 69, Rewrite the first-write validation in the existing-state check around existing and new_state so it directly rejects any initial state other than awaiting_review, rather than relying on VALID_TRANSITIONS membership. Preserve the existing ValueError behavior and message intent, while leaving the earlier transition validation unchanged.
🤖 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/routes/agent_auth_requests.py`:
- Around line 81-84: The project_doc_review scope is accepted but not treated as
project-bound during approval. Update _PROJECT_SCOPES to include
project_doc_review, and update approve_request_record to use the project-bound
approval-path values when issuing the granted token so project_doc_review
approvals require and retain project_id.
In `@tinyagentos/routes/project_doc_review.py`:
- Around line 66-74: Update the review-state handling around set_review_state to
catch a distinct InvalidTransitionError and return HTTP 409, while catching
remaining ValueError instances as HTTP 400 without inspecting exception message
text. Define and raise InvalidTransitionError in the store’s invalid-transition
path, and add from exc to both HTTPException raises to preserve exception
chaining.
---
Nitpick comments:
In `@tests/test_doc_review.py`:
- Around line 287-358: Add HTTP-level tests to TestAgentScopeGating for GET
/api/projects/{project_id}/doc-reviews: verify an agent token bound to another
project receives the same existence-hiding 404 as
test_agent_other_project_is_404, and verify an owner session requesting
?state=approved returns only approved reviews. Use the existing project,
authentication, and response assertion helpers while preserving current route
behavior.
In `@tinyagentos/projects/doc_review_store.py`:
- Around line 64-69: Rewrite the first-write validation in the existing-state
check around existing and new_state so it directly rejects any initial state
other than awaiting_review, rather than relying on VALID_TRANSITIONS membership.
Preserve the existing ValueError behavior and message intent, while leaving the
earlier transition validation unchanged.
In `@tinyagentos/routes/project_doc_review.py`:
- Around line 17-18: Update the DocReviewUpdate.state field to use a Literal
containing the known valid review-state values instead of unrestricted str, and
add or reuse the necessary typing import. Preserve the existing field
description so FastAPI and OpenAPI expose the accepted states and reject invalid
values during request validation.
- Around line 21-48: Replace the positional tuple returned by
_authorize_doc_review_actor with a small typed structure, such as a dataclass or
NamedTuple, exposing named fields for actor ID, agent status, and project.
Update both call sites that currently destructure auth into actor_id, _is_agent,
and _project to access the corresponding named fields while preserving the
existing JSONResponse error returns and values.
🪄 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: b25f8369-9205-4b43-a09b-1a0dbbbaac43
📒 Files selected for processing (13)
CHANGELOG.mddesktop/src/apps/FilesApp.tsxdesktop/src/lib/projects.tsdocs/agent-coordination.mdtests/test_agent_registry.pytests/test_doc_review.pytinyagentos/app.pytinyagentos/auth_middleware.pytinyagentos/projects/doc_review_store.pytinyagentos/routes/__init__.pytinyagentos/routes/agent_auth_requests.pytinyagentos/routes/agent_registry.pytinyagentos/routes/project_doc_review.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tinyagentos/routes/agent_registry.py
- tinyagentos/routes/init.py
- tinyagentos/app.py
- tests/test_agent_registry.py
- docs/agent-coordination.md
- desktop/src/lib/projects.ts
- desktop/src/apps/FilesApp.tsx
| # Doc-review stamps: read/set review state on a project's docs | ||
| # (project-bound like project_tasks). Reconciled from master at beta.45 - | ||
| # the routes shipped on every install while the scope was missing here. | ||
| "project_doc_review", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'_PROJECT_SCOPES|VALID_SCOPES|project_doc_review|effective_project|project_id' \
tinyagentos/routes/agent_auth_requests.py \
tinyagentos/auth_middleware.py \
testsRepository: jaylfc/taOS
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== agent_auth_requests.py: relevant definitions =="
sed -n '1,130p' tinyagentos/routes/agent_auth_requests.py
echo
echo "== agent_auth_requests.py: approval path =="
sed -n '350,455p' tinyagentos/routes/agent_auth_requests.py
echo
echo "== direct matches in targeted files =="
rg -n '_PROJECT_SCOPES|VALID_SCOPES|project_doc_review|effective_project|project_id' tinyagentos/routes/agent_auth_requests.py tinyagentos/auth_middleware.py
echo
echo "== doc_review tests matches =="
rg -n 'project_doc_review|_grant_.*doc|create.*grant|create.*token|approve_request' tests/test_doc_review.py || true
echo
echo "== any doc_review scopes/tests =="
rg -n 'doc_review|project_doc_review' tests tinyagentos/routes tinyagentos/auth_middleware.pyRepository: jaylfc/taOS
Length of output: 23654
Require project-doc-review on the approval path when granting project_doc_review.
VALID_SCOPES accepts project_doc_review, but _PROJECT_SCOPES at tinyagentos/routes/agent_auth_requests.py:207 only includes task, canvas, and file scopes. Since approve_request_record only enforces project_id for project-scoped grants through _PROJECT_SCOPES, approve project_doc_review without a project and the route gets a global grant instead of a project-bound token. Add project_doc_review to the project-bind list and use bound approval-path values for the granted token.
🤖 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_auth_requests.py` around lines 81 - 84, The
project_doc_review scope is accepted but not treated as project-bound during
approval. Update _PROJECT_SCOPES to include project_doc_review, and update
approve_request_record to use the project-bound approval-path values when
issuing the granted token so project_doc_review approvals require and retain
project_id.
| try: | ||
| review = await store.set_review_state( | ||
| project_id, doc_path, payload.state, actor_id | ||
| ) | ||
| except ValueError as exc: | ||
| detail = str(exc) | ||
| if "invalid transition" in detail: | ||
| raise HTTPException(status_code=409, detail=detail) | ||
| raise HTTPException(status_code=400, detail=detail) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'project_doc_review.py|doc_review_store.py|test_doc_review.py|*doc_review*' . | sed 's#^\./##'
echo "== git status/stat =="
git diff --stat || true
echo "== route relevant lines =="
if [ -f tinyagentos/routes/project_doc_review.py ]; then
sed -n '1,120p' tinyagentos/routes/project_doc_review.py | cat -n
fi
echo "== store relevant lines =="
if [ -f tinyagentos/projects/doc_review_store.py ]; then
wc -l tinyagentos/projects/doc_review_store.py
rg -n "invalid (transition|review state)|set_review_state|class .*Error|def set_review_state" tinyagentos/projects/doc_review_store.py
sed -n '1,220p' tinyagentos/projects/doc_review_store.py | cat -n
fi
echo "== tests relevant lines =="
if [ -f tests/test_doc_review.py ]; then
wc -l tests/test_doc_review.py
rg -n "invalid (transition|review state)|status_code|409|400|review" tests/test_doc_review.py
sed -n '90,135p' tests/test_doc_review.py | cat -n
fi
echo "== search usages for set_review_state and InvalidTransitionError =="
rg -n "set_review_state|InvalidTransitionError|invalid transition|invalid review state" .Repository: jaylfc/taOS
Length of output: 582
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import ast
route = Path("tinyagentos/routes/project_doc_review.py")
store = Path("tinyagentos/projects/doc_review_store.py")
test = Path("tests/test_doc_review.py")
print("== AST exception handling at route ==")
if route.exists():
tree = ast.parse(route.read_text())
for node in ast.walk(tree):
if isinstance(node, ast.ExceptHandler):
exc_name = ast.unparse(node.type) if node.type else None
catch = [
(n.lineno, ast.unparse(n) or getattr(n, "id", None))
for n in [node.exc, *node.body]
if isinstance(n, ast.AST)
]
print("try line", node.lineno, "handler:", exc_name)
for li in node.body:
print(" line", li.lineno, ast.unparse(li)[:220])
print(" contains substring check", any(isinstance(n, ast.Compare) and any(isinstance(op, ast.In) for op in n.ops) for n in ast.walk(node)))
print("== store custom exception/class occurrences ==")
if store.exists():
tree = ast.parse(store.read_text())
for cls in ast.walk(tree):
if isinstance(cls, ast.ClassDef) and any(isinstance(b, ast.ExceptionHandler) for b in cls.bases):
print(cls.lineno, "class", cls.name, "bases", ast.unparse(cls))
print("top-level except handlers count (data only):", sum(isinstance(n, ast.ExceptHandler) for n in ast.walk(tree)))
print("== tests status assertions ==")
if test.exists():
text = test.read_text()
for needle in ["invalid transition", "invalid review state", "status_code=409", "status_code=400"]:
lines = [str(i+1) for i,line in enumerate(text.splitlines()) if needle in line]
print(needle, lines)
print("== source message invariant ==")
for name, path in [("route", route), ("store", store), ("test", test)]:
if path.exists():
text = path.read_text()
for needle in ["invalid transition", "invalid review state"]:
print(name, needle, text.count(needle))
PYRepository: jaylfc/taOS
Length of output: 323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'project_doc_review.py|doc_review_store.py|test_doc_review.py|doc_review' . | sed 's#^\./##'
echo "== route relevant lines =="
if [ -f tinyagentos/routes/project_doc_review.py ]; then
sed -n '1,130p' tinyagentos/routes/project_doc_review.py | cat -n
fi
echo "== store relevant lines =="
if [ -f tinyagentos/projects/doc_review_store.py ]; then
wc -l tinyagentos/projects/doc_review_store.py
rg -n "invalid (transition|review state)|set_review_state|class .*Error|def set_review_state" tinyagentos/projects/doc_review_store.py || true
sed -n '1,240p' tinyagentos/projects/doc_review_store.py | cat -n
fi
echo "== tests relevant lines =="
if [ -f tests/test_doc_review.py ]; then
wc -l tests/test_doc_review.py
rg -n "invalid (transition|review state)|status_code|409|400|review" tests/test_doc_review.py || true
sed -n '90,135p' tests/test_doc_review.py | cat -n
fi
echo "== search usages =="
rg -n "set_review_state|InvalidTransitionError|invalid transition|invalid review state|from exc" . || trueRepository: jaylfc/taOS
Length of output: 28294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import ast
route = Path("tinyagentos/routes/project_doc_review.py")
store = Path("tinyagentos/projects/doc_review_store.py")
test = Path("tests/test_doc_review.py")
print("== AST exception handling at route ==")
if route.exists():
tree = ast.parse(route.read_text())
for node in ast.walk(tree):
if not isinstance(node, ast.ExceptHandler):
continue
exc_name = ast.unparse(node.type) if node.type else None
print("try line", node.lineno, "handler:", exc_name)
for child in node.body:
print(" line", child.lineno, ast.unparse(child)[:220])
checks = []
for comp in ast.walk(node):
if not isinstance(comp, ast.Compare):
continue
for cmp in comp.comparators:
value = ast.unparse(cmp)
if value == '"invalid transition"':
checks.append(comp.lineno)
print(" contains substring check", checks)
print("== store classes and custom exceptions ==")
if store.exists():
tree = ast.parse(store.read_text())
for cls in ast.walk(tree):
if isinstance(cls, ast.ClassDef):
bases = ast.unparse(cls) if cls.bases else None
if bases and "Exception" in bases:
print(cls.lineno, "class", cls.name, "bases", bases)
for node in ast.walk(tree):
if isinstance(node, ast.ClassDef):
for base in node.bases:
if isinstance(base, ast.Name) and base.id == "ValueError":
print("ValueError subclass subclassed at", node.lineno, node.name)
break
print("== tests status/message assertions ==")
if test.exists():
text = test.read_text()
for needle in ["invalid transition", "invalid review state", "status_code=409", "status_code=400"]:
lines = [str(i+1) for i,line in enumerate(text.splitlines()) if needle in line]
print(needle, lines)
print("== source message occurrences ==")
for name, path in [("route", route), ("store", store), ("test", test)]:
if path.exists():
text = path.read_text()
for needle in ["invalid transition", "invalid review state"]:
print(name, needle, text.count(needle))
PYRepository: jaylfc/taOS
Length of output: 1046
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import ast
route = Path("tinyagentos/routes/project_doc_review.py")
store = Path("tinyagentos/projects/doc_review_store.py")
test = Path("tests/test_doc_review.py")
print("== AST exception handling at route ==")
if route.exists():
tree = ast.parse(route.read_text())
for node in ast.walk(route):
pass
for node in ast.walk(tree):
if not isinstance(node, ast.ExceptHandler):
continue
exc_name = ast.unparse(node.type) if node.types else None
print("try line", node.lineno, "handler:", exc_name)
for child in node.body:
print(" line", child.lineno, ast.unparse(child)[:220])
for cmp in ast.walk(node):
if not isinstance(cmp, ast.Compare):
continue
for sub in ast.walk(cmp):
if isinstance(sub, ast.Constant) and sub.value == "invalid transition":
print(" contains substring check at line", cmp.lineno)
print("== store classes and custom exceptions ==")
if store.exists():
tree = ast.parse(store.read_text())
for cls in ast.walk(tree):
if isinstance(cls, ast.ClassDef):
bases = ast.unparse(cls) if cls.bases else None
if bases and "Exception" in bases:
print(cls.lineno, "class", cls.name, "bases", bases)
for node in ast.walk(tree):
if isinstance(node, ast.ClassDef):
if node.bases and isinstance(node.bases[0], ast.Name) and node.bases[0].id == "ValueError":
print("ValueError subclass at", node.lineno, node.name)
print("== tests status/message assertions ==")
if test.exists():
text = test.read_text()
for needle in ["invalid transition", "invalid review state", "status_code=409", "status_code=400"]:
lines = [str(i+1) for i,line in enumerate(text.splitlines()) if needle in line]
print(needle, lines)
print("== source message occurrences ==")
for name, path in [("route", route), ("store", store), ("test", test)]:
if path.exists():
text = path.read_text()
for needle in ["invalid transition", "invalid review state"]:
print(name, needle, text.count(needle))
PYRepository: jaylfc/taOS
Length of output: 623
Avoid branching on exception message text.
routes/project_doc_review.py chooses between 409 and 400 by checking "invalid transition" in str(exc), while doc_review_store.py uses "invalid transition: ..." for transitions and "invalid review state: ..." for unknown states. Store-level wording changes would silently change HTTP status codes. Raise a distinct InvalidTransitionError from doc_review_store.py for the conflict case, catch it here, and keep other ValueErrors as 400. Use from exc on both FastAPI raises so the original traceback is preserved.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 73-73: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 74-74: 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/routes/project_doc_review.py` around lines 66 - 74, Update the
review-state handling around set_review_state to catch a distinct
InvalidTransitionError and return HTTP 409, while catching remaining ValueError
instances as HTTP 400 without inspecting exception message text. Define and
raise InvalidTransitionError in the store’s invalid-transition path, and add
from exc to both HTTPException raises to preserve exception chaining.
|
nemotron-ultra-orB review VERDICT: Blocking issues found — UI state transition logic violates backend state machine; silent error handling in frontend; potential key collision in review state map.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
|
nemotron-super review VERDICT: No blocking issues found. Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
1 participant
CARD TITLE (intent, not commit subject): Reconcile doc-review (#1835) into dev: master-only feature discovered at beta.45 promote
Autonomous build of board card tsk-737pd7.
Files:
tests/test_doc_review.py | 358 +++++++++++++++++++++++++++++++
tinyagentos/app.py | 5 +
tinyagentos/auth_middleware.py | 21 ++
tinyagentos/projects/doc_review_store.py | 149 +++++++++++++
tinyagentos/routes/init.py | 3 +
tinyagentos/routes/agent_registry.py | 1 +
tinyagentos/routes/project_doc_review.py | 112 ++++++++++
9 files changed, 660 insertions(+)
Summary by CodeRabbit
New Features
Documentation