fix(governance): delegation + org-model hardening (#1661/#1662 retro, #174) - #1712
Conversation
…174) Retrospective audit fixes for the agent org model (#1661) and heartbeat (#1662), all verified against current dev. 1. Deny-event audit rows were unfindable. A policy deny recorded the board audit event with a synthetic agent:{from_agent} task_id and a blank project_id, so recent_for_project never surfaced it. Now the deny is keyed on the real task (when present) and always carries the real project_id, so it shows in both the project feed and the task history. 2. Raw task id leaked to the human approval inbox. The blocking Decision question showed the raw task_id when no task_title was given. The existing task's title is now threaded through and used, falling back to task_title then task_id. 3. Assignee identity mismatch. complete_delegation wrote assignee_id as the agent NAME, but beads, project members, and the heartbeat sweep all key on the config hex id, so delegated tasks were invisible to id-keyed lookups. The name is now resolved to the hex id for assignee_id (name kept for display/notify), with a graceful fallback when id == name. 4. set_reporting cycle-check TOCTOU race. The check-then-write walked the reporting chain across multiple awaits on the shared aiosqlite connection with no serialization, so two concurrent edits (A->B and B->A) could both pass and both write a persisted cycle. The sequence is now held under an asyncio.Lock. 5. PUT /org empty-body no-op and missing "" normalization. An all-None body returned 200 while changing nothing, and role/title were stored verbatim including whitespace. An all-empty body is now rejected with 400, and empty/whitespace-only role/title clear the field (NULL), matching the reports_to convention.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR changes org-field updates to reject empty payloads and normalize clearing semantics, adds locking and targeted updates in the agent registry store, and updates delegation to resolve stable assignee ids, adjust denial audit payloads, and refine approval-question text. Tests cover the new behavior. ChangesRole/title clearing and reporting-cycle locking
Delegation assignee resolution and audit fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if title is not None: | ||
| cols.append("title = ?") | ||
| vals.append(title or None) | ||
| if not cols: |
There was a problem hiding this comment.
SUGGESTION: The new set_role_title has a dead-code branch that returns the pre-update record.
The if not cols: return record branch only fires when both role and title are None, but the public route (update_org_fields) already rejects the all-None body with 400. Inside the store it's safer to treat role=None, title=None as a no-op that still reflects disk truth — either drop the early return and let the execute run harmlessly, or replace it with a single return await self.get(canonical_id) so the returned row is consistent with any concurrent write.
| if not cols: | |
| if not cols: | |
| return await self.get(canonical_id) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # edits (A->B and B->A) can't both pass the cycle guard and then both write | ||
| # a persisted cycle. The shared aiosqlite connection yields on every await | ||
| # inside the check, so the read chain and the write must be held together. | ||
| _reporting_lock: asyncio.Lock |
There was a problem hiding this comment.
WARNING: The class-level _reporting_lock: asyncio.Lock annotation declares the attribute without a value, so instances do NOT actually have _reporting_lock until init() runs.
The runtime RuntimeError("AgentRegistryStore not initialised") guard on self._db is None happens to cover this in practice because self._db is also assigned in init() on the same line order, but the lock is not a defensive default (a class-level asyncio.Lock() default would be). Any future caller that bypasses init() (e.g. a test stub that sets _db directly) will get AttributeError: 'AgentRegistryStore' object has no attribute '_reporting_lock' instead of a clear initialization error.
Recommend either initializing at class scope (_reporting_lock: asyncio.Lock = asyncio.Lock() — note asyncio.Lock instances must be created in a running loop, so keep the init() assignment) or asserting in set_reporting before the lock acquisition.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Normalize whitespace-only values to "" so set_role_title clears the | ||
| # field (NULL), matching the reports_to ""-clears convention below; | ||
| # a None field is left untouched. | ||
| role = body.role.strip() if body.role is not None else None |
There was a problem hiding this comment.
SUGGESTION: body.role.strip() and body.title.strip() will raise AttributeError if the JSON body sends a non-string (e.g. null is fine but 42 or an array slips past pydantic only if the type is str).
Pydantic's Optional[str] does coerce/validate, but consider explicitly documenting in OrgUpdateRequest that role and title MUST be strings when present, and use a field_validator to reject non-string types with a 422 rather than a 500. Today a body like {"role": 42} would be accepted by pydantic (coerced to "42") and silently stripped to "42" — surprising.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| agent = find_agent(config, name) | ||
| if agent is None: | ||
| return name | ||
| return agent.get("id") or name |
There was a problem hiding this comment.
WARNING: The docstring claims fallback behavior is for agents where id == name, but the implementation just does agent.get("id") or name.
For an agent whose config entry has "id": "" (empty string), the code returns the name (correct) — but for "id": null, agent.get("id") is None, also returning name (correct). However, for an agent that has id set to a value that does not match the slug (which is the intended shape post-fix), the code uses the id — which is the desired behavior. The doc is slightly misleading about WHY the fallback fires; consider rewording to "falls back to name when the agent has no resolvable id (missing, empty, or null)" so the comment matches the actual semantics.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # to_agent is the config slug (name); task assignee_id must be the agent's | ||
| # stable hex id so the heartbeat sweep and id-keyed member lookups can see | ||
| # the delegated task. The name is still used below for the notify text. | ||
| assignee_id = _resolve_agent_id(request, to_agent) |
There was a problem hiding this comment.
WARNING: _resolve_agent_id silently returns name unchanged when find_agent(config, name) returns None.
In delegate_task the slug is already validated by the preceding _resolve_agent_name call (which 404s on unknown agents), so for the direct-allow path this is safe. But complete_delegation is also called from routes/decisions.py:_apply_delegation_grant (line 330), where to_agent comes from decision.metadata.to_agent — a value persisted at decision-creation time. If the config agent is renamed or removed between the gate being raised and the gate being answered, _resolve_agent_id will silently fall back to the now-stale slug, and the task will be assigned to an assignee that no longer exists in config.agents. The PR claims this fixes the assignee mismatch — the heartbeat sweep and id-keyed member lookups still won't find the task.
Recommend: log a warning when the fallback fires, or re-validate the slug against current config and return a clear error to the decision-approval path.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return await self.update(canonical_id, role=role, title=title) | ||
| if self._db is None: | ||
| raise RuntimeError("AgentRegistryStore not initialised") | ||
| record = await self.get(canonical_id) |
There was a problem hiding this comment.
SUGGESTION: The new set_role_title re-implements update logic instead of delegating to self.update() and is not protected by the new _reporting_lock.
The new function does its own SELECT → UPDATE → SELECT rather than calling the existing update() method (which had the same semantics). More importantly, set_role_title and set_reporting are not held under a common lock, so a concurrent call pair like set_role_title("X", title="lead") interleaved with set_reporting("X", "Y") could see a stale get() and overwrite a concurrent change. The PR's stated race is real for set_reporting, but the same TOCTOU exists across the new set_role_title and set_reporting boundary. Consider widening the lock to cover both org-mutating methods, or add a brief comment noting the intentional scope.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
The incremental commit (8b9a6a2) addresses the previously reported WARNING on _reporting_lock by moving its creation from init() into init. The lock now exists at construction time, independent of init ordering, so any caller will see a real asyncio.Lock instance and the AttributeError regression vector from the prior review is closed. Files Reviewed (1 file)
Previous Review Summary (commit ca384fc)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ca384fc)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Reviewed by minimax-m3 · Input: 37.8K · Output: 3.7K · Cached: 485.6K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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_registry.py (1)
758-771: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRole/title writes commit before reports_to validation, enabling partial writes on failure.
If a caller sends
role/titletogether with an invalidreports_to(self-report, cycle, missing manager),set_role_title(line 764) commits immediately, thenset_reportingfails and the route returns 400. The client sees a failure butrole/titlewere silently persisted — an inconsistent partial update from a single logical request.Validating
reports_tofirst (it already fails fast via exception, without any side effect on role/title) avoids applying role/title when the overall request will be rejected.♻️ Proposed reordering
- if body.role is not None or body.title is not None: - # Normalize whitespace-only values to "" so set_role_title clears the - # field (NULL), matching the reports_to ""-clears convention below; - # a None field is left untouched. - role = body.role.strip() if body.role is not None else None - title = body.title.strip() if body.title is not None else None - await store.set_role_title(canonical_id, role=role, title=title) - - if body.reports_to is not None: - manager_id = body.reports_to or None # "" clears the manager - try: - await store.set_reporting(canonical_id, manager_id) - except (ValueError, KeyError) as exc: - return JSONResponse({"error": str(exc)}, status_code=400) + if body.reports_to is not None: + manager_id = body.reports_to or None # "" clears the manager + try: + await store.set_reporting(canonical_id, manager_id) + except (ValueError, KeyError) as exc: + return JSONResponse({"error": str(exc)}, status_code=400) + + if body.role is not None or body.title is not None: + # Normalize whitespace-only values to "" so set_role_title clears the + # field (NULL), matching the reports_to ""-clears convention below; + # a None field is left untouched. + role = body.role.strip() if body.role is not None else None + title = body.title.strip() if body.title is not None else None + await store.set_role_title(canonical_id, role=role, title=title)🤖 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_registry.py` around lines 758 - 771, The route handler in agent_registry currently applies role/title updates before validating reports_to, which can leave a partial commit when set_reporting later rejects the request. Reorder the logic in the update path so reports_to is validated and applied first via store.set_reporting (handling ValueError/KeyError as it already does), and only call store.set_role_title after that succeeds; keep the existing normalization of body.role/body.title and body.reports_to behavior intact.
🤖 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_registry.py`:
- Around line 758-771: The route handler in agent_registry currently applies
role/title updates before validating reports_to, which can leave a partial
commit when set_reporting later rejects the request. Reorder the logic in the
update path so reports_to is validated and applied first via store.set_reporting
(handling ValueError/KeyError as it already does), and only call
store.set_role_title after that succeeds; keep the existing normalization of
body.role/body.title and body.reports_to behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c2446e7-7ca3-420c-816e-c3ce8b9f098f
📒 Files selected for processing (6)
tests/test_agent_registry_store.pytests/test_routes_agent_org.pytests/test_routes_delegation.pytinyagentos/agent_registry_store.pytinyagentos/routes/agent_registry.pytinyagentos/routes/delegation.py
Fold Kilo review WARNING: the class-level _reporting_lock annotation created no attribute, so set_reporting relied on init() having run. Move creation into __init__ so the lock always exists before any caller, independent of init ordering. asyncio.Lock() binds lazily on first use, so constructing it without a running loop is safe.
|
Folded the _reporting_lock WARNING: the lock is now created in init so it always exists before any caller, independent of init ordering. Tracking the remaining Kilo WARNING as a follow-up: _resolve_agent_id falls back to the stale slug if a config agent is renamed or removed between a delegation gate's creation and its approval, which would leave that one delegated task invisible to id-keyed lookups. Low impact (needs an agent to vanish mid-approval) and resolving it cleanly means capturing the id at gate-creation time, which is a larger change than this PR's scope. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Retrospective audit fixes for the merged agent org model (#1661) and heartbeat (#1662), all verified against current
dev. Each fix ships with a test. Relates to #174.Fixes
1. CRITICAL - deny-event audit rows were unfindable. On a policy
deny,delegation.pyrecorded the board-audit event with a synthetictask_id=f"agent:{from_agent}"and noproject_id(defaults to""). Sincerecent_for_projectfilters onproject_id, deny events never appeared in any project feed. Now the deny is keyed on the realtask_idwhen a task is present (so it also shows in the task's history) and always carries the realproject_id; with no task it records apolicy:delegateevent rather than a fabricated agent-as-task id.2. WARNING - raw task id leaked to the human approval inbox. When delegating an existing task with no
task_title, the blocking Decision question read "wants to delegate<raw task_id>". The task's title (already fetched indelegate_task) is now threaded into_check_delegation_policyand used, falling back totask_titlethentask_id.3. MEDIUM - assignee identity mismatch (from #1662).
complete_delegationwroteassignee_id=to_agent, but that is the agent NAME (slug), while beads, project members, and the heartbeat sweep (list_ready_tasks_for_assignee) all key on the config HEX id (config.pymintsuuid4().hex[:12]). Delegated tasks were therefore invisible to the heartbeat and id-keyed member lookups. The name is now resolved to the hex id forassignee_id(name kept for display/notify), with a graceful fallback when the agent has no distinct id (id == name).4. WARNING - set_reporting TOCTOU cycle race. The cycle guard walked the reporting chain via multiple
await self.get(...)reads and then issued a separate UPDATE with no serialization. On the shared aiosqlite connection the awaits yield, so two concurrent edits (A->B and B->A) could both pass the check then both write, persisting a cycle. The check-then-write is now held under anasyncio.Lock.5. WARNING - PUT /org empty-body no-op + no "" normalization. A body of
{}(all fields None) returned 200 while changing nothing, androle/titlewere stored verbatim including whitespace, whereas the codebase convention (applied toreports_to) is that""clears a nullable field. An all-empty body is now rejected with 400, and empty/whitespace-onlyrole/titleclear the field (NULL).Tests
tests/test_routes_delegation.py: fixtures given distinctidvsname(which masked Authentication system for web GUI #3); new tests for the findable deny audit event, the title-not-raw-id approval question, and theid == namefallback.tests/test_agent_registry_store.py: empty-string/whitespace clear forset_role_title; concurrent A->B / B->A edits cannot persist a cycle.tests/test_routes_agent_org.py: empty body 400, clear role via"", whitespace-only title cleared.All affected and adjacent suites pass (
test_routes_delegation,test_routes_agent_org,test_agent_registry_store,test_agent_registry,test_board_audit,test_routes_decisions,test_agent_heartbeat).Summary by CodeRabbit
{}(HTTP 400) and properly clearsrole/titlewhen empty or whitespace is provided, returningnullin responses.