mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
feat(auditor): waive_finding verb + findings queue panel
Wire the long-unwired mark_waived repo method to a new auditor-only flow verb waive_finding, severity-scoped to minor/nit (blocker/major must be fixed, never waived), requiring a note, with a task.finding_waived audit event and no task status change. Add the verb to the IntentSpec table (auto-derived into the auditor manifest), the flow_auditor route, and the flow_server MCP tool. Surface open review findings (cross-task, blocking-first) on the auditor dashboard via ReviewFindingsRepository.list_open_findings and a new findings field on AuditorDashboard. Restore the panel's 4-card auditor layout with a new read-only FindingsQueuePanel as the 4th card.
This commit is contained in:
Renn F
@@ -214,7 +214,7 @@ backlog -> pending -> claimed -> in_progress -> [blocked|paused] -> verifying
|
||||
|
||||
**Revision findings ledger (always-on, no flag — core lifecycle).** QA/PR-gate/PM/CEO bounce feedback used to be prose-only: `issues: list[str]` flattened into free text with no structural anchor, `request_changes`/`ceo_reject` had no structured note at all, and the raw `dev_notes` append both used was silently overwritten by the very next `note(scope='handoff')` call — a live data-loss bug. `fail_review` (QA), `pr_fail` (in-path PR gate), `request_changes` (PM merge reject), and `ceo_reject` all now take structured `findings: list[dict]` — validated into `Finding` (`file` repo-relative ≤300 chars/no `..`, `line` ≥1, `severity` blocker\|major\|minor\|nit, `criterion` must match an AC id or its exact text, `expected`/`actual` ≤300, `fix` ≤500, `evidence` ≤2000) — with a soft nudge above 5 findings and a hard reject above 10 in one call (`roboco/services/gateway/choreographer/findings.py`). `issues=[...]` still works this release as a shim (each string → a file-less `severity=major` finding, deprecation-logged) and merges with `findings` rather than one silently dropping the other. Every producer inserts one append-only row per finding into `task_review_findings` (migration 071; `origin` qa\|pr_gate\|pm\|ceo, `round` = `revision_count+1` read pre-transition, `status` open→addressed→verified\|waived) via `ReviewFindingsRepository`, then writes a structured note whose `summary` IS the deterministic per-finding rendering `[F-id8] file:line (severity) — expected → actual → fix`, mirrored into `qa_notes`/`pr_reviewer_notes`/the new `pm_notes` column (new `PmReviewContent` "pm_review" content type). `ceo_reject` now validates its reason (previously could 500 on an empty/trivial one) and stamps it as one `origin=ceo` `blocker` finding; on a branchless coordination root — which routes to `pending` via `admin_set_status`, bypassing the normal audit chokepoint — it bumps `revision_count` and emits `task.ceo_reject` directly instead of silently skipping both. New audit events `task.request_changes`/`task.ceo_reject` join `task.qa_fail`/`task.pr_fail` in `_audit_events_for` so rework metrics attribute every bounce kind, not just QA/PR-gate.
|
||||
|
||||
Resolution: `i_am_done`/`submit_up`/`submit_root` all gain `resolved_findings` (`{finding_id, commit?, note?}`), gated by a new `Requirement.FINDINGS_ADDRESSED` — every OPEN finding on the task must be named (a fuzzy 8-char-prefix match against `[F-id8]`) or the envelope rejects, listing the still-open ids. `pass_review`/`pr_pass`/`complete` bulk-verify their own origin's `addressed` findings same-transaction (a stamp failure fails the verb outright, not best-effort); `ceo_approve` stamps `ceo`-origin best-effort. `mark_waived` exists on the repository but no verb calls it yet — a deliberate unwired follow-up.
|
||||
Resolution: `i_am_done`/`submit_up`/`submit_root` all gain `resolved_findings` (`{finding_id, commit?, note?}`), gated by a new `Requirement.FINDINGS_ADDRESSED` — every OPEN finding on the task must be named (a fuzzy 8-char-prefix match against `[F-id8]`) or the envelope rejects, listing the still-open ids. `pass_review`/`pr_pass`/`complete` bulk-verify their own origin's `addressed` findings same-transaction (a stamp failure fails the verb outright, not best-effort); `ceo_approve` stamps `ceo`-origin best-effort. `mark_waived` is wired to the auditor-only `waive_finding` flow verb (severity-scoped: blocker/major must be fixed, never waived; only minor/nit open findings are waivable, with a required note and a `task.finding_waived` audit event; no task status change).
|
||||
|
||||
Delivery: `evidence()`/`build_task_handoff` carry `revision_findings` (open only, capped) so a bounced dev finally gets what `developer.md` promises instead of nothing; `claim_review`/`claim_gate_review` additionally carry `prior_findings` (the full ledger) so a round-2+ reviewer checks prior findings instead of re-deriving them blind. The orchestrator's `REVISION_REQUIRED` dev prompt and the PM triage "bounced" block render open findings inline with the same rendering; A2A fail bodies share it. `GET /api/tasks/{id}/findings` (capped 500, SQL-aggregated per-origin/status summary + `total`/`truncated`) backs the panel's task-detail Findings tab and a `bounced xN` header chip (`revision_count`); metrics attribute `pm_rejects`/`ceo_rejects` + open/total findings counts per task; vault task notes render a capped `## Findings` section (fail-open fetch, never blocks the note write).
|
||||
|
||||
@@ -349,7 +349,7 @@ Each agent gets a **spawn manifest** at `/app/tool-manifest.json` listing the ve
|
||||
| pr_reviewer | `give_me_work`, `claim_pr_review`, `post_pr_review` (inbound external/fork PRs), `claim_gate_review`, `pr_pass`, `pr_fail` (in-path assembled-PR gate), `unclaim` |
|
||||
| product_owner | `triage`, `escalate_to_ceo` |
|
||||
| head_marketing| `triage`, `escalate_to_ceo` |
|
||||
| auditor | `triage` (read-only — no `dm`) |
|
||||
| auditor | `triage`, `waive_finding` (read-only — no `dm`) |
|
||||
| prompter | (none beyond `i_am_idle` — not a delivery-lifecycle role; intake interviewer, human-only) |
|
||||
| secretary | (none beyond `i_am_idle` — human-only chief-of-staff; reads company state + runs gated CEO directives) |
|
||||
|
||||
|
||||
@@ -10,6 +10,7 @@ import {
|
||||
import { QualityMetricsPanel } from "./quality-metrics-panel";
|
||||
import { FlaggedItemsPanel } from "./flagged-items-panel";
|
||||
import { ReportsPanel } from "./reports-panel";
|
||||
import { FindingsQueuePanel } from "./findings-queue-panel";
|
||||
import { Button } from "@/components/ui/button";
|
||||
import { FileText } from "lucide-react";
|
||||
import { toast } from "sonner";
|
||||
@@ -73,11 +74,17 @@ export function AuditorDashboard() {
|
||||
</div>
|
||||
</div>
|
||||
|
||||
{/* Quality Metrics */}
|
||||
{/* Top Row: Open Findings + Quality Metrics */}
|
||||
<div className="grid grid-cols-1 lg:grid-cols-2 xl:grid-cols-2 2xl:grid-cols-2 gap-6">
|
||||
<FindingsQueuePanel
|
||||
findings={dashboard?.findings}
|
||||
isLoading={loadingDashboard}
|
||||
/>
|
||||
<QualityMetricsPanel
|
||||
metrics={dashboard?.metrics}
|
||||
isLoading={loadingDashboard}
|
||||
/>
|
||||
</div>
|
||||
|
||||
{/* Bottom Row: Flagged Items + Reports */}
|
||||
<div className="grid grid-cols-1 lg:grid-cols-2 xl:grid-cols-2 2xl:grid-cols-2 gap-6">
|
||||
|
||||
@@ -0,0 +1,120 @@
|
||||
"use client";
|
||||
|
||||
import { AuditorFinding } from "@/types";
|
||||
import { Card, CardContent, CardHeader, CardTitle } from "@/components/ui/card";
|
||||
import { Badge } from "@/components/ui/badge";
|
||||
import { Skeleton } from "@/components/ui/skeleton";
|
||||
import { ScrollArea } from "@/components/ui/scroll-area";
|
||||
import { ListChecks } from "lucide-react";
|
||||
import Link from "next/link";
|
||||
|
||||
interface FindingsQueuePanelProps {
|
||||
findings: AuditorFinding[] | undefined;
|
||||
isLoading: boolean;
|
||||
}
|
||||
|
||||
// Severity stored as the finding's lowercase value (blocker/major/minor/nit).
|
||||
const severityColors: Record<string, string> = {
|
||||
blocker: "bg-red-100 text-red-700",
|
||||
major: "bg-orange-100 text-orange-700",
|
||||
minor: "bg-yellow-100 text-yellow-700",
|
||||
nit: "bg-blue-100 text-blue-700",
|
||||
};
|
||||
|
||||
const severityOrder: Record<string, number> = {
|
||||
blocker: 0,
|
||||
major: 1,
|
||||
minor: 2,
|
||||
nit: 3,
|
||||
};
|
||||
|
||||
export function FindingsQueuePanel({
|
||||
findings,
|
||||
isLoading,
|
||||
}: FindingsQueuePanelProps) {
|
||||
// The API already returns blocking-first, but keep the sort stable client-side
|
||||
// in case a later re-fetch reorders.
|
||||
const sorted = [...(findings ?? [])].sort((a, b) => {
|
||||
const diff = (severityOrder[a.severity] ?? 9) - (severityOrder[b.severity] ?? 9);
|
||||
if (diff !== 0) return diff;
|
||||
return (b.created_at ?? "").localeCompare(a.created_at ?? "");
|
||||
});
|
||||
|
||||
return (
|
||||
<Card>
|
||||
<CardHeader className="pb-3">
|
||||
<div className="flex items-center justify-between">
|
||||
<div className="flex items-center gap-2">
|
||||
<CardTitle className="text-lg flex items-center gap-2">
|
||||
<ListChecks className="h-5 w-5" />
|
||||
Open Findings
|
||||
</CardTitle>
|
||||
{sorted.length > 0 && (
|
||||
<Badge variant="destructive">{sorted.length}</Badge>
|
||||
)}
|
||||
</div>
|
||||
</div>
|
||||
</CardHeader>
|
||||
<CardContent>
|
||||
{isLoading ? (
|
||||
<div className="space-y-3">
|
||||
{[...Array(3)].map((_, i) => (
|
||||
<Skeleton key={i} className="h-24" />
|
||||
))}
|
||||
</div>
|
||||
) : sorted.length === 0 ? (
|
||||
<div className="text-center py-8 text-muted-foreground text-sm">
|
||||
<ListChecks className="h-8 w-8 mx-auto mb-2 opacity-50" />
|
||||
No open review findings
|
||||
</div>
|
||||
) : (
|
||||
<ScrollArea className="h-[400px] pr-4">
|
||||
<div className="space-y-3">
|
||||
{sorted.map((finding) => (
|
||||
<div
|
||||
key={finding.id}
|
||||
className="p-4 rounded-lg border bg-muted/50"
|
||||
>
|
||||
<div className="flex items-center gap-2 mb-1 flex-wrap">
|
||||
<Badge
|
||||
className={
|
||||
(severityColors[finding.severity] ?? "") + " text-xs"
|
||||
}
|
||||
>
|
||||
{finding.severity}
|
||||
</Badge>
|
||||
<Badge variant="outline" className="text-xs">
|
||||
{finding.origin}
|
||||
</Badge>
|
||||
<span className="text-xs text-muted-foreground">
|
||||
round {finding.round}
|
||||
</span>
|
||||
</div>
|
||||
<p className="text-sm text-muted-foreground mb-2">
|
||||
{finding.actual ?? finding.expected ?? finding.criterion ?? "—"}
|
||||
</p>
|
||||
<div className="flex items-center gap-4 text-xs text-muted-foreground">
|
||||
{finding.file && (
|
||||
<span className="font-mono">
|
||||
{finding.file}
|
||||
{finding.line ? `:${finding.line}` : ""}
|
||||
</span>
|
||||
)}
|
||||
<Link
|
||||
href={"/tasks/" + finding.task_id}
|
||||
prefetch={false}
|
||||
>
|
||||
<span className="text-primary hover:underline">
|
||||
Task #{finding.task_id.slice(0, 8)}
|
||||
</span>
|
||||
</Link>
|
||||
</div>
|
||||
</div>
|
||||
))}
|
||||
</div>
|
||||
</ScrollArea>
|
||||
)}
|
||||
</CardContent>
|
||||
</Card>
|
||||
);
|
||||
}
|
||||
@@ -1187,6 +1187,7 @@ export const mockAuditorDashboard = {
|
||||
},
|
||||
],
|
||||
recent_reports: mockAuditorReports,
|
||||
findings: [],
|
||||
};
|
||||
|
||||
// Mock mode: dev = mock, production = real backend
|
||||
|
||||
@@ -611,6 +611,20 @@ export interface AuditorReport {
|
||||
sent_at: string | null;
|
||||
}
|
||||
|
||||
export interface AuditorFinding {
|
||||
id: string;
|
||||
task_id: string;
|
||||
origin: string;
|
||||
severity: string;
|
||||
file: string | null;
|
||||
line: number | null;
|
||||
criterion: string | null;
|
||||
expected: string | null;
|
||||
actual: string | null;
|
||||
round: number;
|
||||
created_at: string | null;
|
||||
}
|
||||
|
||||
export interface AuditorDashboard {
|
||||
flagged_items: AuditorFlag[];
|
||||
metrics: Record<string, number>;
|
||||
@@ -621,6 +635,7 @@ export interface AuditorDashboard {
|
||||
team: string | null;
|
||||
}>;
|
||||
recent_reports: AuditorReport[];
|
||||
findings: AuditorFinding[];
|
||||
}
|
||||
|
||||
export interface TeamHealth {
|
||||
|
||||
@@ -117,6 +117,7 @@ async def get_auditor_dashboard(
|
||||
metrics=metrics,
|
||||
audit_queue=audit_queue,
|
||||
recent_reports=recent_reports,
|
||||
findings=await service.get_open_findings(limit=20),
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -11,7 +11,11 @@ from guard_core.handlers.behavior_handler import BehaviorRule
|
||||
|
||||
from roboco.api.deps import get_choreographer
|
||||
from roboco.api.routes.v1._role_dep import envelope_to_response, require_auditor
|
||||
from roboco.api.schemas.v1.flow import IAmIdleRequest, TriageRequest
|
||||
from roboco.api.schemas.v1.flow import (
|
||||
IAmIdleRequest,
|
||||
TriageRequest,
|
||||
WaiveFindingRequest,
|
||||
)
|
||||
from roboco.security import guard_deco
|
||||
from roboco.services.gateway.choreographer import Choreographer
|
||||
|
||||
@@ -44,6 +48,20 @@ async def triage(
|
||||
return envelope_to_response(env, request)
|
||||
|
||||
|
||||
@router.post("/waive_finding")
|
||||
@guard_deco.rate_limit(requests=30, window=60)
|
||||
@guard_deco.content_type_filter(["application/json"])
|
||||
@guard_deco.behavior_analysis(_RUNAWAY_RULES)
|
||||
async def waive_finding(
|
||||
request: Request,
|
||||
body: WaiveFindingRequest,
|
||||
x_agent_id: _AgentIdHeader,
|
||||
choreographer: _ChoreographerDep,
|
||||
) -> dict:
|
||||
env = await choreographer.waive_finding(x_agent_id, body.finding_id, body.note)
|
||||
return envelope_to_response(env, request)
|
||||
|
||||
|
||||
@router.post("/i_am_idle")
|
||||
@guard_deco.rate_limit(requests=30, window=60)
|
||||
@guard_deco.content_type_filter(["application/json"])
|
||||
|
||||
@@ -54,6 +54,7 @@ class AuditorDashboard(BaseModel):
|
||||
metrics: dict[str, Any]
|
||||
audit_queue: list[dict[str, Any]]
|
||||
recent_reports: list[AuditorReport]
|
||||
findings: list[dict[str, Any]] = Field(default_factory=list)
|
||||
|
||||
|
||||
class TeamHealth(BaseModel):
|
||||
|
||||
@@ -284,6 +284,18 @@ class TriageRequest(BaseModel):
|
||||
"""Empty request body."""
|
||||
|
||||
|
||||
class WaiveFindingRequest(BaseModel):
|
||||
finding_id: UUID
|
||||
note: str = Field(
|
||||
...,
|
||||
min_length=1,
|
||||
description=(
|
||||
"Why this finding is waived rather than fixed — recorded on the "
|
||||
"ledger row and in audit. Only minor/nit findings are waivable."
|
||||
),
|
||||
)
|
||||
|
||||
|
||||
class UnblockRequest(BaseModel):
|
||||
task_id: UUID
|
||||
reason: str = Field(
|
||||
|
||||
@@ -1591,6 +1591,19 @@ _INTENT_VERBS: dict[str, IntentSpec] = {
|
||||
side_effects=(),
|
||||
next_hint=lambda _t: "act on a listed task or i_am_idle",
|
||||
),
|
||||
"waive_finding": IntentSpec(
|
||||
name="waive_finding",
|
||||
allowed_roles=frozenset({Role.AUDITOR}),
|
||||
description=(
|
||||
"Waive one minor/nit review finding by id with a required note. "
|
||||
"Blocker/major findings must be fixed, never waived. No task "
|
||||
"status change."
|
||||
),
|
||||
composes=(),
|
||||
extra_preconditions=(),
|
||||
side_effects=(),
|
||||
next_hint=lambda _t: "finding waived; triage() for next item or i_am_idle",
|
||||
),
|
||||
}
|
||||
|
||||
|
||||
|
||||
@@ -879,6 +879,18 @@ def escalate_to_ceo(task_id: str, reason: str) -> dict[str, Any]:
|
||||
return _post(_role_path("escalate_to_ceo"), {"task_id": task_id, "reason": reason})
|
||||
|
||||
|
||||
def waive_finding(finding_id: str, note: str) -> dict[str, Any]:
|
||||
"""Auditor: waive one minor/nit review finding by id with a required note.
|
||||
|
||||
Blocker/major findings must be fixed, never waived. The finding id is the
|
||||
``[F-<id8>]`` prefix shown in task notes / triage. No task status changes.
|
||||
"""
|
||||
return _post(
|
||||
_role_path("waive_finding"),
|
||||
{"finding_id": finding_id, "note": note},
|
||||
)
|
||||
|
||||
|
||||
# ---------- Cell PM + Main PM extras ----------
|
||||
# i_will_plan, delegate, submit_up, give_me_work — restore the pre-Phase-4
|
||||
# PM lifecycle so PMs can drive parent tasks instead of stalling.
|
||||
@@ -1108,6 +1120,8 @@ _TOOLS: dict[str, Any] = {
|
||||
"declare_coverage": declare_coverage,
|
||||
# board / main pm
|
||||
"escalate_to_ceo": escalate_to_ceo,
|
||||
# auditor
|
||||
"waive_finding": waive_finding,
|
||||
}
|
||||
|
||||
|
||||
|
||||
@@ -319,6 +319,38 @@ class DashboardService(BaseService):
|
||||
"active_blockers": blockers.active_blockers,
|
||||
}
|
||||
|
||||
async def get_open_findings(self, limit: int = 20) -> list[dict[str, Any]]:
|
||||
"""Open review findings across all tasks for the auditor's findings queue.
|
||||
|
||||
Blocking severity first then newest. Fails open (empty list) — the
|
||||
dashboard must never break on a findings-query error.
|
||||
"""
|
||||
from roboco.services.repositories.review_findings import (
|
||||
ReviewFindingsRepository,
|
||||
)
|
||||
|
||||
try:
|
||||
repo = ReviewFindingsRepository(self.session)
|
||||
rows = await repo.list_open_findings(limit=limit)
|
||||
except Exception:
|
||||
return []
|
||||
return [
|
||||
{
|
||||
"id": str(r.id),
|
||||
"task_id": str(r.task_id),
|
||||
"origin": r.origin,
|
||||
"severity": r.severity,
|
||||
"file": r.file,
|
||||
"line": r.line,
|
||||
"criterion": r.criterion,
|
||||
"expected": r.expected,
|
||||
"actual": r.actual,
|
||||
"round": r.round,
|
||||
"created_at": r.created_at.isoformat() if r.created_at else None,
|
||||
}
|
||||
for r in rows
|
||||
]
|
||||
|
||||
async def get_all_agent_status(self, team: Team | None = None) -> dict[str, Any]:
|
||||
"""Return agent-status summary (counts + per-agent snapshot)."""
|
||||
query = select(AgentTable)
|
||||
|
||||
@@ -14,9 +14,15 @@ the actual class is composed in ``__init__.py`` and inherits from
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import contextlib
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
from roboco.foundation.policy.content import Severity
|
||||
from roboco.services.gateway.envelope import Envelope
|
||||
from roboco.services.repositories.review_findings import (
|
||||
STATUS_OPEN,
|
||||
ReviewFindingsRepository,
|
||||
)
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from uuid import UUID
|
||||
@@ -27,6 +33,9 @@ if TYPE_CHECKING:
|
||||
else:
|
||||
_Base = object
|
||||
|
||||
# Severity stored as the enum's string value (``f.severity.value`` at insert).
|
||||
_BLOCKING = frozenset({Severity.BLOCKER.value, Severity.MAJOR.value})
|
||||
|
||||
|
||||
class BoardMixin(_Base):
|
||||
"""Board (Product Owner + Head Marketing) + Auditor verbs."""
|
||||
@@ -88,3 +97,76 @@ class BoardMixin(_Base):
|
||||
next="no anomalies — i_am_idle",
|
||||
context_briefing=await self._briefing_for(auditor_agent_id, None),
|
||||
)
|
||||
|
||||
async def waive_finding(
|
||||
self, auditor_agent_id: UUID, finding_id: UUID, note: str
|
||||
) -> Envelope:
|
||||
"""Waive one minor/nit finding by id with a required note.
|
||||
|
||||
The auditor is the only role that can close a finding without a dev
|
||||
fix — but only for non-blocking severity (minor/nit). Blocker/major
|
||||
must be fixed, never waived. No task status change: the ledger row
|
||||
moves ``open -> waived`` and an audit event records the decision.
|
||||
``mark_waived`` is the long-unwired repo method this finally calls.
|
||||
"""
|
||||
repo = ReviewFindingsRepository(self.task.session)
|
||||
row = await repo.get(finding_id)
|
||||
if row is None:
|
||||
return Envelope.not_found(
|
||||
message=f"finding {str(finding_id)[:8]} not found.",
|
||||
remediate=(
|
||||
"find open findings via the task's GET /findings or triage()."
|
||||
),
|
||||
)
|
||||
if row.status != STATUS_OPEN:
|
||||
return Envelope.invalid_state(
|
||||
message=(
|
||||
f"finding {str(finding_id)[:8]} is already {row.status}; "
|
||||
"only open findings can be waived."
|
||||
),
|
||||
remediate=(
|
||||
"pick an open finding — waived/addressed/verified "
|
||||
"rows are immutable."
|
||||
),
|
||||
)
|
||||
if row.severity in _BLOCKING:
|
||||
return Envelope.invalid_state(
|
||||
message=(
|
||||
f"finding {str(finding_id)[:8]} is {row.severity} — "
|
||||
"blocker/major findings must be fixed, never waived."
|
||||
),
|
||||
remediate=(
|
||||
"leave it for the dev to address; waive only minor/nit findings."
|
||||
),
|
||||
)
|
||||
clean_note = note.strip()
|
||||
if not clean_note:
|
||||
return Envelope.invalid_state(
|
||||
message=(
|
||||
"a waive requires a note explaining why this finding "
|
||||
"is not worth a fix."
|
||||
),
|
||||
remediate="pass note=<why this minor/nit is waived>.",
|
||||
)
|
||||
await repo.mark_waived(finding_id, clean_note)
|
||||
with contextlib.suppress(Exception):
|
||||
await self.audit.log_task_event(
|
||||
event_type="task.finding_waived",
|
||||
task_id=row.task_id,
|
||||
agent_id=auditor_agent_id,
|
||||
severity="info",
|
||||
details={
|
||||
"finding_id": str(finding_id),
|
||||
"severity": row.severity,
|
||||
"origin": row.origin,
|
||||
"note": clean_note[:300],
|
||||
},
|
||||
)
|
||||
return Envelope.ok(
|
||||
status="waived",
|
||||
task_id=str(row.task_id),
|
||||
next=(
|
||||
f"finding {str(finding_id)[:8]} waived; triage() for next item "
|
||||
"or i_am_idle"
|
||||
),
|
||||
)
|
||||
|
||||
@@ -162,3 +162,25 @@ class ReviewFindingsRepository(BaseRepository[TaskReviewFindingTable]):
|
||||
row.resolution_note = note[:_RESOLUTION_NOTE_CAP]
|
||||
await self.session.flush()
|
||||
return True
|
||||
|
||||
async def list_open_findings(
|
||||
self, *, limit: int = 20
|
||||
) -> list[TaskReviewFindingTable]:
|
||||
"""Cross-task open findings, blocking severity first then newest.
|
||||
|
||||
For the auditor dashboard's findings queue — the backlog of unresolved
|
||||
review findings the auditor can triage / waive. Capped (the dashboard
|
||||
is a glance, not a ledger view — ``GET /api/tasks/{id}/findings`` is
|
||||
the full per-task ledger).
|
||||
"""
|
||||
stmt = (
|
||||
select(TaskReviewFindingTable)
|
||||
.where(TaskReviewFindingTable.status == STATUS_OPEN)
|
||||
.order_by(
|
||||
TaskReviewFindingTable.severity.asc(),
|
||||
TaskReviewFindingTable.created_at.desc(),
|
||||
)
|
||||
.limit(limit)
|
||||
)
|
||||
result = await self.session.execute(stmt)
|
||||
return list(result.scalars().all())
|
||||
|
||||
@@ -0,0 +1,35 @@
|
||||
"""IntentSpec for the auditor ``waive_finding`` verb.
|
||||
|
||||
``mark_waived`` sat unwired on the repository since the findings ledger
|
||||
landed (PR #486) — a deliberate follow-up. The auditor is the role that
|
||||
can close a finding without a dev fix, but only for non-blocking severity.
|
||||
This pins the spec: auditor-only, severity-scoped at the verb body (the
|
||||
IntentSpec carries no task precondition — ``composes=()`` like ``triage``).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from roboco.foundation.identity import Role
|
||||
from roboco.foundation.policy.lifecycle import intents_for_role
|
||||
from roboco.services.gateway.role_config import _AUDITOR_FLOW
|
||||
|
||||
|
||||
def test_waive_finding_is_an_auditor_flow_verb() -> None:
|
||||
assert "waive_finding" in intents_for_role(Role.AUDITOR)
|
||||
assert "waive_finding" in _AUDITOR_FLOW
|
||||
|
||||
|
||||
def test_waive_finding_is_auditor_only() -> None:
|
||||
for role in (
|
||||
Role.DEVELOPER,
|
||||
Role.QA,
|
||||
Role.DOCUMENTER,
|
||||
Role.CELL_PM,
|
||||
Role.MAIN_PM,
|
||||
Role.PR_REVIEWER,
|
||||
Role.PRODUCT_OWNER,
|
||||
Role.HEAD_MARKETING,
|
||||
):
|
||||
assert "waive_finding" not in intents_for_role(role), (
|
||||
f"{role} must not get waive_finding"
|
||||
)
|
||||
@@ -0,0 +1,169 @@
|
||||
"""Choreographer.waive_finding — the auditor's close-without-fix verb.
|
||||
|
||||
``mark_waived`` was the long-unwired repo method; this is its only caller.
|
||||
Severity-scoped: blocker/major must be fixed, never waived. Only open
|
||||
findings are waivable, and a note is required. No task status changes —
|
||||
the ledger row ``open -> waived`` plus a ``task.finding_waived`` audit
|
||||
event is the durable record.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps
|
||||
|
||||
|
||||
def _make_deps() -> ChoreographerDeps:
|
||||
base = {
|
||||
"task": AsyncMock(),
|
||||
"work_session": AsyncMock(),
|
||||
"git": AsyncMock(),
|
||||
"a2a": AsyncMock(),
|
||||
"journal": AsyncMock(),
|
||||
"audit": AsyncMock(),
|
||||
"evidence_repo": AsyncMock(),
|
||||
}
|
||||
repo = base["evidence_repo"]
|
||||
for method in (
|
||||
"list_unread_a2a",
|
||||
"list_unread_mentions",
|
||||
"list_pending_notifications",
|
||||
"task_metadata_gaps",
|
||||
"recent_team_activity",
|
||||
"blockers_in_lane",
|
||||
"journal_highlights_for_task",
|
||||
):
|
||||
getattr(repo, method).return_value = []
|
||||
return ChoreographerDeps(**base)
|
||||
|
||||
|
||||
def _finding_row(
|
||||
*,
|
||||
severity: str = "minor",
|
||||
status: str = "open",
|
||||
origin: str = "qa",
|
||||
) -> MagicMock:
|
||||
row = MagicMock()
|
||||
row.id = uuid4()
|
||||
row.task_id = uuid4()
|
||||
row.severity = severity
|
||||
row.status = status
|
||||
row.origin = origin
|
||||
return row
|
||||
|
||||
|
||||
def _patch_repo(monkeypatch: pytest.MonkeyPatch, row: Any | None) -> MagicMock:
|
||||
"""Patch the board-module ReviewFindingsRepository to return ``row`` from
|
||||
``get`` and a recording ``mark_waived``. ``row=None`` simulates not-found."""
|
||||
repo_mock = MagicMock()
|
||||
repo_mock.get = AsyncMock(return_value=row)
|
||||
repo_mock.mark_waived = AsyncMock(return_value=True)
|
||||
monkeypatch.setattr(
|
||||
"roboco.services.gateway.choreographer.board.ReviewFindingsRepository",
|
||||
lambda *_a, **_k: repo_mock,
|
||||
)
|
||||
return repo_mock
|
||||
|
||||
|
||||
def _choreographer(monkeypatch: pytest.MonkeyPatch, row: Any | None):
|
||||
deps = _make_deps()
|
||||
deps.task.session = MagicMock()
|
||||
c = Choreographer(deps)
|
||||
return c, _patch_repo(monkeypatch, row)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_waive_minor_open_finding_succeeds(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
row = _finding_row(severity="minor", status="open")
|
||||
c, repo_mock = _choreographer(monkeypatch, row)
|
||||
|
||||
env = await c.waive_finding(uuid4(), row.id, "cosmetic, not worth a fix")
|
||||
|
||||
assert env.error is None
|
||||
assert env.status == "waived"
|
||||
repo_mock.mark_waived.assert_awaited_once_with(row.id, "cosmetic, not worth a fix")
|
||||
c.audit.log_task_event.assert_awaited_once()
|
||||
assert c.audit.log_task_event.await_args.kwargs["event_type"] == (
|
||||
"task.finding_waived"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_waive_nit_open_finding_succeeds(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
row = _finding_row(severity="nit", status="open")
|
||||
c, repo_mock = _choreographer(monkeypatch, row)
|
||||
env = await c.waive_finding(uuid4(), row.id, "preference only")
|
||||
assert env.error is None
|
||||
repo_mock.mark_waived.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_waive_rejects_blocker(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
row = _finding_row(severity="blocker", status="open")
|
||||
c, repo_mock = _choreographer(monkeypatch, row)
|
||||
env = await c.waive_finding(uuid4(), row.id, "try to skip the fix")
|
||||
assert env.error == "invalid_state"
|
||||
repo_mock.mark_waived.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_waive_rejects_major(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
row = _finding_row(severity="major", status="open")
|
||||
c, repo_mock = _choreographer(monkeypatch, row)
|
||||
env = await c.waive_finding(uuid4(), row.id, "try to skip the fix")
|
||||
assert env.error == "invalid_state"
|
||||
repo_mock.mark_waived.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_waive_rejects_already_addressed(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
row = _finding_row(severity="minor", status="addressed")
|
||||
c, repo_mock = _choreographer(monkeypatch, row)
|
||||
env = await c.waive_finding(uuid4(), row.id, "already closed")
|
||||
assert env.error == "invalid_state"
|
||||
repo_mock.mark_waived.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_waive_rejects_blank_note(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
row = _finding_row(severity="minor", status="open")
|
||||
c, repo_mock = _choreographer(monkeypatch, row)
|
||||
env = await c.waive_finding(uuid4(), row.id, " ")
|
||||
assert env.error == "invalid_state"
|
||||
repo_mock.mark_waived.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_waive_unknown_finding_returns_not_found(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
c, repo_mock = _choreographer(monkeypatch, None)
|
||||
env = await c.waive_finding(uuid4(), uuid4(), "note")
|
||||
assert env.error == "not_found"
|
||||
repo_mock.mark_waived.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_waive_succeeds_even_if_audit_log_fails(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""The audit event is best-effort: a log failure must not undo the waive."""
|
||||
row = _finding_row(severity="minor", status="open")
|
||||
c, repo_mock = _choreographer(monkeypatch, row)
|
||||
c.audit.log_task_event = AsyncMock(side_effect=RuntimeError("db gone"))
|
||||
|
||||
env = await c.waive_finding(uuid4(), row.id, "still waive me")
|
||||
assert env.error is None
|
||||
repo_mock.mark_waived.assert_awaited_once()
|
||||
@@ -0,0 +1,98 @@
|
||||
"""flow_server exposes the auditor's ``waive_finding`` tool.
|
||||
|
||||
The verb is auto-derived into the auditor manifest via
|
||||
``intents_for_role(Role.AUDITOR)``; this pins that the MCP layer registers
|
||||
it under the public name and POSTs the right payload to the auditor path.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib
|
||||
import json
|
||||
from typing import TYPE_CHECKING, Any
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
from roboco.foundation.identity import Role
|
||||
from roboco.foundation.policy.lifecycle import intents_for_role
|
||||
|
||||
if TYPE_CHECKING:
|
||||
import types
|
||||
from pathlib import Path
|
||||
|
||||
|
||||
def _auditor_manifest() -> dict[str, object]:
|
||||
return {
|
||||
"agent_id": "00000000-0000-0000-0000-000000000004",
|
||||
"role": "auditor",
|
||||
"team": "board",
|
||||
"workspace_path": "/tmp/test",
|
||||
"flow_tools": list(intents_for_role(Role.AUDITOR)),
|
||||
"do_tools": [],
|
||||
"read_tools": [],
|
||||
"write_tools": [],
|
||||
"bash_allowed": True,
|
||||
"subagent_allowed": False,
|
||||
"subagent_model": None,
|
||||
"env": {},
|
||||
}
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def flow_module_auditor(
|
||||
monkeypatch: pytest.MonkeyPatch, tmp_path: Path
|
||||
) -> types.ModuleType:
|
||||
manifest_path = tmp_path / "tool-manifest.json"
|
||||
manifest_path.write_text(json.dumps(_auditor_manifest()))
|
||||
monkeypatch.setenv("ROBOCO_AGENT_ID", "00000000-0000-0000-0000-000000000004")
|
||||
monkeypatch.setenv("ROBOCO_AGENT_ROLE", "auditor")
|
||||
monkeypatch.setenv("ROBOCO_ORCHESTRATOR_URL", "http://test-orchestrator:8000")
|
||||
monkeypatch.setenv("ROBOCO_SDK_URL", "http://test-sdk:9000")
|
||||
monkeypatch.setenv("ROBOCO_TOOL_MANIFEST_PATH", str(manifest_path))
|
||||
import roboco.mcp.flow_server as srv
|
||||
|
||||
importlib.reload(srv)
|
||||
return srv
|
||||
|
||||
|
||||
def test_waive_finding_registers_for_auditor_manifest(
|
||||
flow_module_auditor: types.ModuleType,
|
||||
) -> None:
|
||||
registered = flow_module_auditor._register_tools()
|
||||
assert "waive_finding" in registered, (
|
||||
f"waive_finding not registered for auditor manifest. "
|
||||
f"Registered: {sorted(registered)}"
|
||||
)
|
||||
|
||||
|
||||
def test_waive_finding_posts_to_auditor_path(
|
||||
flow_module_auditor: types.ModuleType,
|
||||
) -> None:
|
||||
captured: list[tuple[str, Any]] = []
|
||||
|
||||
def _client_factory(*_a: object, **_kw: object) -> MagicMock:
|
||||
client = MagicMock()
|
||||
client.__enter__ = MagicMock(return_value=client)
|
||||
client.__exit__ = MagicMock(return_value=False)
|
||||
|
||||
def _post(url: str, **kwargs: object) -> MagicMock:
|
||||
captured.append((url, kwargs.get("json", {})))
|
||||
resp = MagicMock()
|
||||
resp.json.return_value = {"status": "waived", "error": None}
|
||||
return resp
|
||||
|
||||
client.post.side_effect = _post
|
||||
return client
|
||||
|
||||
finding_id = "11111111-1111-1111-1111-111111111111"
|
||||
with patch("httpx.Client", side_effect=_client_factory):
|
||||
result = flow_module_auditor.waive_finding(finding_id, "cosmetic nit")
|
||||
|
||||
assert result["status"] == "waived"
|
||||
orch_calls = [(u, b) for u, b in captured if "test-orchestrator" in u]
|
||||
assert len(orch_calls) == 1
|
||||
url, body = orch_calls[0]
|
||||
assert url.endswith("/api/v1/flow/auditor/waive_finding"), (
|
||||
f"waive_finding must POST to /auditor/waive_finding, got {url}"
|
||||
)
|
||||
assert body == {"finding_id": finding_id, "note": "cosmetic nit"}
|
||||
@@ -289,3 +289,55 @@ async def test_mark_waived_requires_note(db_session: AsyncSession) -> None:
|
||||
async def test_mark_waived_unknown_id_returns_false(db_session: AsyncSession) -> None:
|
||||
repo = ReviewFindingsRepository(db_session)
|
||||
assert await repo.mark_waived(uuid4(), "note") is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_open_findings_cross_task_blocking_first(
|
||||
db_session: AsyncSession,
|
||||
) -> None:
|
||||
"""list_open_findings returns OPEN rows across tasks, blocker first."""
|
||||
agent_id = await _seed_agent(db_session)
|
||||
task_a = await _seed_task(db_session, agent_id)
|
||||
task_b = await _seed_task(db_session, agent_id)
|
||||
repo = ReviewFindingsRepository(db_session)
|
||||
|
||||
await repo.insert_many(
|
||||
task_id=task_a,
|
||||
origin="qa",
|
||||
round=1,
|
||||
author_slug="be-qa",
|
||||
findings=[_finding(severity=Severity.MINOR)],
|
||||
)
|
||||
await repo.insert_many(
|
||||
task_id=task_b,
|
||||
origin="qa",
|
||||
round=1,
|
||||
author_slug="be-qa",
|
||||
findings=[_finding(severity=Severity.BLOCKER)],
|
||||
)
|
||||
# Waive the minor one so only the blocker is OPEN.
|
||||
open_rows = await repo.list_for_task(task_a, status=STATUS_OPEN)
|
||||
await repo.mark_waived(UUID(str(open_rows[0].id)), "waived in test")
|
||||
|
||||
result = await repo.list_open_findings(limit=20)
|
||||
assert len(result) == 1
|
||||
assert result[0].severity == Severity.BLOCKER.value
|
||||
assert str(result[0].task_id) == str(task_b)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_open_findings_excludes_non_open(
|
||||
db_session: AsyncSession,
|
||||
) -> None:
|
||||
agent_id = await _seed_agent(db_session)
|
||||
task_id = await _seed_task(db_session, agent_id)
|
||||
repo = ReviewFindingsRepository(db_session)
|
||||
rows = await repo.insert_many(
|
||||
task_id=task_id,
|
||||
origin="qa",
|
||||
round=1,
|
||||
author_slug="be-qa",
|
||||
findings=[_finding(severity=Severity.NIT)],
|
||||
)
|
||||
await repo.mark_waived(UUID(str(rows[0].id)), "nit, skip")
|
||||
assert await repo.list_open_findings(limit=20) == []
|
||||
|
||||
Reference in New Issue
Block a user