From 4b2546ae19a126ec6654aac1b2d4962e93b8fc3d Mon Sep 17 00:00:00 2001 From: Renzo F <45401804+rennf93@users.noreply.github.com> Date: Fri, 24 Jul 2026 17:09:49 +0200 Subject: [PATCH] fix(findings): path-shaped file refs + per-round collapsible findings (#687) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(findings): enforce path-shaped file refs; group panel findings by round - The findings chokepoint rejects a file that is not a repo-relative path shape (prose like a PR reference validated before, and the panel then rendered a doomed file-content fetch for it) — narrative belongs in evidence, the remediate says so. - The task-detail Findings tab groups findings into per-round collapsible sections (newest expanded) and only attempts a code snippet for a path-shaped file ref, so historical prose refs render as plain metadata instead of a broken loader. * fix(findings): admit client-repo path conventions; teach the file-less option - The shape gate reviews arbitrary client projects, not just this repo: plus and at-sign join the character class so SvelteKit route files, @types dirs, and @2x assets stay citable. Spaces stay excluded — they are the prose signal. - The file-rejection remediate names the file-less option for cross-cutting findings. - The client mirror notes its deliberate non-ASCII divergence from the server gate (unicode server-pass renders snippetless, fail-open). --------- Co-authored-by: Renn F --- .../__tests__/tab-findings.test.tsx | 87 ++++++++++++++++++- .../tasks/task-detail/tab-findings.tsx | 86 ++++++++++++------ roboco/foundation/policy/content/models.py | 20 +++++ .../gateway/choreographer/findings.py | 38 +++++++- .../services/gateway/choreographer/pr_gate.py | 14 +-- roboco/services/gateway/choreographer/qa.py | 14 +-- .../policy/content/test_content_models.py | 45 ++++++++++ tests/unit/gateway/test_choreographer_qa.py | 28 ++++++ .../gateway/test_pr_gate_records_verdict.py | 22 +++++ 9 files changed, 302 insertions(+), 52 deletions(-) diff --git a/panel/src/components/tasks/task-detail/__tests__/tab-findings.test.tsx b/panel/src/components/tasks/task-detail/__tests__/tab-findings.test.tsx index 7096ce85..9f019d70 100644 --- a/panel/src/components/tasks/task-detail/__tests__/tab-findings.test.tsx +++ b/panel/src/components/tasks/task-detail/__tests__/tab-findings.test.tsx @@ -44,7 +44,7 @@ describe("TabFindings", () => { ).toBeInTheDocument(); }); - it("groups findings by round and renders severity/status", () => { + it("groups findings by round, expands the newest, collapses older rounds", async () => { const response: TaskFindingsResponse = { findings: [ { @@ -99,8 +99,20 @@ describe("TabFindings", () => { expect(screen.getByText("Round 2")).toBeInTheDocument(); expect(screen.getByText("Round 1")).toBeInTheDocument(); + // The newest round (2) is expanded by default — its finding is visible. + const round2Button = screen.getByText("Round 2").closest("button"); + const round1Button = screen.getByText("Round 1").closest("button"); + expect(round2Button).toHaveAttribute("aria-expanded", "true"); + expect(round1Button).toHaveAttribute("aria-expanded", "false"); expect(screen.getByText("blocker")).toBeInTheDocument(); - expect(screen.getByText("minor")).toBeInTheDocument(); + // Round 1's finding is collapsed, so its content isn't rendered yet. + expect(screen.queryByText("minor")).toBeNull(); + expect(screen.queryByText("abc1234")).toBeNull(); + + // Expanding round 1 reveals its finding. + const user = userEvent.setup(); + await user.click(round1Button!); + expect(await screen.findByText("minor")).toBeInTheDocument(); expect(screen.getByText("abc1234")).toBeInTheDocument(); expect(screen.queryByText(/more not shown/)).toBeNull(); }); @@ -179,4 +191,75 @@ describe("TabFindings", () => { "Must be fixed before this task can pass review.", ); }); + + it("skips the CodeSnippet fetch for a non-path file but still renders it as text", () => { + const response: TaskFindingsResponse = { + findings: [ + { + id: "eeeeeeee-0000-0000-0000-000000000000", + task_id: "t1", + origin: "pm", + round: 1, + author_slug: "be-pm", + file: "PR #676 description", + line: null, + severity: "major", + criterion: null, + expected: "x", + actual: "y", + fix: null, + evidence: null, + status: "open", + addressed_by_commit: null, + resolution_note: null, + created_at: "2026-07-11T00:00:00Z", + updated_at: null, + }, + ], + summary: [], + total: 1, + truncated: false, + }; + useTaskFindings.mockReturnValue({ data: response, isLoading: false }); + render(); + + // The prose file still renders as plain metadata text... + expect(screen.getByText("PR #676 description")).toBeInTheDocument(); + // ...but never drives the doomed CodeSnippet fetch. + expect(screen.queryByTestId("code-snippet")).toBeNull(); + }); + + it("still renders CodeSnippet for a real path-shaped file", () => { + const response: TaskFindingsResponse = { + findings: [ + { + id: "ffffffff-0000-0000-0000-000000000000", + task_id: "t1", + origin: "qa", + round: 1, + author_slug: "be-qa", + file: "roboco/services/task.py", + line: 12, + severity: "major", + criterion: null, + expected: "x", + actual: "y", + fix: null, + evidence: null, + status: "open", + addressed_by_commit: null, + resolution_note: null, + created_at: "2026-07-11T00:00:00Z", + updated_at: null, + }, + ], + summary: [], + total: 1, + truncated: false, + }; + useTaskFindings.mockReturnValue({ data: response, isLoading: false }); + render(); + + expect(screen.getByTestId("code-snippet")).toBeInTheDocument(); + }); }); diff --git a/panel/src/components/tasks/task-detail/tab-findings.tsx b/panel/src/components/tasks/task-detail/tab-findings.tsx index 624f14b7..68abdeeb 100644 --- a/panel/src/components/tasks/task-detail/tab-findings.tsx +++ b/panel/src/components/tasks/task-detail/tab-findings.tsx @@ -9,6 +9,19 @@ import { Skeleton } from "@/components/ui/skeleton"; import { HelpTip } from "@/components/ui/help-tip"; import { ListChecks } from "lucide-react"; import { CodeSnippet } from "@/components/git/code-snippet"; +import { CollapsibleSection } from "./collapsible-section"; + +// Mirrors the server-side shape gate (Finding._file_repo_relative in +// roboco/foundation/policy/content/models.py) — keep the character classes +// in sync, knowing they deliberately diverge on non-ASCII: Python's \w is +// unicode, JS's is ASCII, so a unicode path the server accepted renders +// snippetless here (fail-open — the plain-text metadata still shows). A +// prose `file` (e.g. a PR reference) predates that gate on older ledger +// rows and must not drive a doomed CodeSnippet fetch. +const FILE_SHAPE_RE = /^[\w.\-/()[\]+@]+$/; +function looksLikePath(file: string): boolean { + return FILE_SHAPE_RE.test(file); +} interface TabFindingsProps { task: Task; @@ -43,14 +56,16 @@ const ORIGIN_LABEL: Record = { // findings-ledger domain (roboco/foundation/policy/conventions/findings.py). const SEVERITY_DESCRIPTIONS: Record = { blocker: "Must be fixed before this task can pass review.", - major: "A significant defect; should be fixed but isn't review-blocking alone.", + major: + "A significant defect; should be fixed but isn't review-blocking alone.", minor: "A smaller defect worth fixing.", nit: "A nitpick — cosmetic or stylistic; fix if convenient.", }; const STATUS_DESCRIPTIONS: Record = { open: "Not yet addressed by the assignee.", - addressed: "The assignee says this is fixed — awaiting reviewer verification.", + addressed: + "The assignee says this is fixed — awaiting reviewer verification.", verified: "A reviewer confirmed the fix.", waived: "Explicitly waived — no fix required.", }; @@ -67,7 +82,9 @@ function FindingCard({
- + {finding.severity} @@ -95,7 +112,7 @@ function FindingCard({ )}
- {finding.file && ( + {finding.file && looksLikePath(finding.file) && (

No revision findings recorded yet.

- Findings appear here after the first QA / PR-review / PM / CEO - bounce. + Findings appear here after the first QA / PR-review / PM / CEO bounce.

); @@ -190,27 +206,43 @@ export function TabFindings({ task }: TabFindingsProps) { ))} )} - {rounds.map((group) => ( -
-
- -

- Round {group.round} -

- - - - {ORIGIN_LABEL[group.origin] ?? group.origin} - - -
-
- {group.items.map((f) => ( - - ))} -
-
- ))} + {rounds.map((group, idx) => { + const openCount = group.items.filter((f) => f.status === "open").length; + return ( + + + Round {group.round} + + + + {ORIGIN_LABEL[group.origin] ?? group.origin} + + + + + {group.items.length}{" "} + {group.items.length === 1 ? "finding" : "findings"} + {openCount > 0 ? ` · ${openCount} open` : ""} + + + + } + > +
+ {group.items.map((f) => ( + + ))} +
+ + ); + })} {data?.truncated && (

… {data.total - findings.length} more not shown ({data.total} total) diff --git a/roboco/foundation/policy/content/models.py b/roboco/foundation/policy/content/models.py index aab43949..45256b78 100644 --- a/roboco/foundation/policy/content/models.py +++ b/roboco/foundation/policy/content/models.py @@ -55,6 +55,21 @@ _FINDING_CRITERION_CAP = 500 # agent's own filesystem layout. _WINDOWS_ABS_RE = re.compile(r"^[A-Za-z]:[\\/]") +# A finding's `file` must look like a path, not prose (a live bug: a finding +# with `file = "PR #676 description"` validated, and the panel then tried to +# fetch a git blob literally named that). Word chars/dot/hyphen/slash cover +# every ordinary path segment; parens/brackets/plus/at are additionally +# allowed because findings reference paths in ARBITRARY reviewed projects, +# not just this repo — Next.js route groups (`app/(dashboard)/page.tsx`) and +# dynamic segments (`app/[taskId]/page.tsx`) here, SvelteKit route files +# (`src/routes/+page.svelte`), `@types/` dirs, and `@2x` retina assets in +# client repos. Deliberately excludes spaces — they ARE the prose signal: +# verified via `git ls-files` that only 2 of 2270 tracked paths (both static +# `vault_assets/meta/` template notes, never a code-review target) contain +# one, while every prose example a reviewer might mistakenly pass as `file` +# ("PR #676 description", "the description in the PR") always does. +_PATH_SHAPE_RE = re.compile(r"^[\w.\-/()\[\]+@]+$") + class _Base(BaseModel): """Shared config: drop unknown keys (graceful), validate assignment.""" @@ -138,6 +153,11 @@ class Finding(_Base): raise ValueError( "file must not contain '..' path segments — repo-relative only" ) + if not _PATH_SHAPE_RE.match(v): + raise ValueError( + "file does not look like a path — put narrative/context in " + "`evidence` and reference a real `file:line` in `file`/`line`" + ) return v diff --git a/roboco/services/gateway/choreographer/findings.py b/roboco/services/gateway/choreographer/findings.py index 5a17059a..eb547d58 100644 --- a/roboco/services/gateway/choreographer/findings.py +++ b/roboco/services/gateway/choreographer/findings.py @@ -15,7 +15,12 @@ from typing import TYPE_CHECKING, Any import structlog -from roboco.foundation.policy.content import Finding, Severity +from roboco.foundation.policy.content import ( + ContentValidationError, + Finding, + Severity, + validate_findings, +) from roboco.services.gateway.envelope import Envelope from roboco.services.gateway.evidence_builder import BRIEFING_LIST_CAP from roboco.services.repositories.review_findings import ( @@ -124,6 +129,37 @@ def findings_count_hint(findings: Sequence[Any]) -> str | None: ) +def validate_or_reject( + raw: list[dict[str, Any]], +) -> tuple[list[Finding], Envelope | None]: + """``validate_findings``, converting a ``ContentValidationError`` into a + field-aware rejection instead of the generic one every producer + (fail_review / pr_fail) used to hardcode regardless of which field failed + — unhelpful for a rejected ``file`` in particular, since the generic + remediate says "file ... optional", which reads as though the value + simply shouldn't have been sent rather than naming where it belongs. + """ + try: + return validate_findings(raw), None + except ContentValidationError as exc: + if exc.field == "file": + remediate = ( + "`file` must be a real repo-relative path — put narrative or " + "context in `evidence` instead, and reference a real " + "`file`/`line` here (or omit `file` entirely for a " + "cross-cutting finding not tied to one file)" + ) + else: + remediate = ( + "each finding needs expected + actual (file/line/severity/" + "criterion/fix/evidence optional)" + ) + return [], Envelope.invalid_state( + message=f"malformed finding: {exc.field} — {exc.reason}", + remediate=remediate, + ) + + def unmatched_criteria(task: Any, criteria: list[str]) -> list[str]: """``criteria`` entries matching neither an AC id nor AC text on ``task``. diff --git a/roboco/services/gateway/choreographer/pr_gate.py b/roboco/services/gateway/choreographer/pr_gate.py index cc98ddec..fdf0d720 100644 --- a/roboco/services/gateway/choreographer/pr_gate.py +++ b/roboco/services/gateway/choreographer/pr_gate.py @@ -26,7 +26,6 @@ from roboco.foundation.policy.batch import is_batch_root_subtask from roboco.foundation.policy.content import ( ContentValidationError, markers, - validate_findings, ) from roboco.services.gateway.choreographer import findings as findings_lib from roboco.services.gateway.choreographer.collision import build_collision_context @@ -174,16 +173,9 @@ class PRGateMixin(_Base): ) if cap := findings_lib.findings_count_guard(raw): return [], cap - try: - validated = validate_findings(raw) - except ContentValidationError as exc: - return [], Envelope.invalid_state( - message=f"malformed finding: {exc.field} — {exc.reason}", - remediate=( - "each finding needs expected + actual (file/line/severity/" - "criterion/fix/evidence optional)" - ), - ) + validated, bad = findings_lib.validate_or_reject(raw) + if bad is not None: + return [], bad if t is not None and ( unknown := findings_lib.unknown_finding_criteria(t, validated) ): diff --git a/roboco/services/gateway/choreographer/qa.py b/roboco/services/gateway/choreographer/qa.py index e092f319..e365125a 100644 --- a/roboco/services/gateway/choreographer/qa.py +++ b/roboco/services/gateway/choreographer/qa.py @@ -47,7 +47,6 @@ from roboco.foundation.policy import tracing as _tr from roboco.foundation.policy.content import ( ContentValidationError, markers, - validate_findings, ) from roboco.services.content_notes import apply_structured_note from roboco.services.gateway.choreographer import findings as findings_lib @@ -893,16 +892,9 @@ class QAMixin(_Base): ) if cap := findings_lib.findings_count_guard(raw): return [], cap - try: - validated = validate_findings(raw) - except ContentValidationError as exc: - return [], Envelope.invalid_state( - message=f"malformed finding: {exc.field} — {exc.reason}", - remediate=( - "each finding needs expected + actual (file/line/severity/" - "criterion/fix/evidence optional)" - ), - ) + validated, bad = findings_lib.validate_or_reject(raw) + if bad is not None: + return [], bad if unknown := findings_lib.unknown_finding_criteria(t, validated): return [], findings_lib.criterion_mismatch_rejection(t, unknown) return validated, None diff --git a/tests/unit/foundation/policy/content/test_content_models.py b/tests/unit/foundation/policy/content/test_content_models.py index 087996be..53a9b804 100644 --- a/tests/unit/foundation/policy/content/test_content_models.py +++ b/tests/unit/foundation/policy/content/test_content_models.py @@ -346,6 +346,51 @@ def test_finding_accepts_dot_segment_and_double_dot_substring() -> None: assert ok.file == "./roboco/services/foo..bar.py" +def test_finding_rejects_prose_file_with_spaces() -> None: + # Live bug: a finding's `file` carried a PR reference ("PR #676 + # description") instead of a path — it validated, and the panel then + # tried (and failed) to fetch a git blob literally named that. + with pytest.raises(ValidationError): + Finding.model_validate(_finding(file="PR #676 description")) + with pytest.raises(ValidationError): + Finding.model_validate(_finding(file="the description in the PR")) + + +def test_finding_accepts_real_nested_path() -> None: + ok = Finding.model_validate( + _finding(file="roboco/services/gateway/choreographer/findings.py") + ) + assert ok.file == "roboco/services/gateway/choreographer/findings.py" + + +def test_finding_accepts_nextjs_route_group_and_dynamic_segment_paths() -> None: + # This repo's own tracked tree uses parens (route groups) and brackets + # (dynamic segments) in real, common paths — the shape gate must not + # reject them. + ok = Finding.model_validate( + _finding(file="panel/src/app/(dashboard)/tasks/[taskId]/page.tsx") + ) + assert ok.file == "panel/src/app/(dashboard)/tasks/[taskId]/page.tsx" + + +def test_finding_accepts_client_repo_path_conventions() -> None: + # Findings reference paths in arbitrary reviewed projects, not just this + # repo: SvelteKit route files (+), @types dirs and @2x assets (@) are + # real, common tracked paths a reviewer must be able to cite. + for path in ( + "src/routes/+page.svelte", + "src/@types/foo.d.ts", + "assets/logo@2x.png", + ): + ok = Finding.model_validate(_finding(file=path)) + assert ok.file == path + + +def test_finding_file_none_bypasses_the_shape_gate() -> None: + f = Finding.model_validate(_finding(file=None)) + assert f.file is None + + def test_finding_rejects_non_positive_line() -> None: with pytest.raises(ValidationError): Finding.model_validate(_finding(line=0)) diff --git a/tests/unit/gateway/test_choreographer_qa.py b/tests/unit/gateway/test_choreographer_qa.py index f5cfb271..b9da0c22 100644 --- a/tests/unit/gateway/test_choreographer_qa.py +++ b/tests/unit/gateway/test_choreographer_qa.py @@ -488,6 +488,34 @@ async def test_fail_review_requires_at_least_one_issue() -> None: assert "finding" in body["message"].lower() +@pytest.mark.asyncio +async def test_fail_review_rejects_prose_file_names_evidence_in_remediate() -> None: + qa_id = uuid4() + task_id = uuid4() + t = _qa_owned_task(task_id, qa_id) + task_svc = AsyncMock() + task_svc.get.return_value = t + task_svc.agent_for.return_value = _qa_agent_mock(qa_id) + journal_svc = AsyncMock() + journal_svc.has_learning_for_task.return_value = True + deps = _make_deps(task=task_svc, journal=journal_svc) + c = Choreographer(deps) + + findings = [ + { + "file": "PR #676 description", + "severity": "major", + "expected": "matches the acceptance criteria", + "actual": "diverges from the acceptance criteria", + } + ] + env = await c.fail_review(qa_id, task_id, findings=findings) + body = env.as_dict() + assert body["error"] == "invalid_state" + assert "evidence" in body["remediate"] + assert "file" in body["remediate"] + + @pytest.mark.asyncio async def test_fail_review_not_assigned_returns_not_authorized() -> None: qa_id = uuid4() diff --git a/tests/unit/gateway/test_pr_gate_records_verdict.py b/tests/unit/gateway/test_pr_gate_records_verdict.py index 65fee0d2..d6531268 100644 --- a/tests/unit/gateway/test_pr_gate_records_verdict.py +++ b/tests/unit/gateway/test_pr_gate_records_verdict.py @@ -17,6 +17,7 @@ from uuid import uuid4 from roboco.foundation.policy.content import Finding, Severity from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps +from roboco.services.gateway.choreographer.pr_gate import PRGateMixin def _make_choreographer() -> Choreographer: @@ -194,3 +195,24 @@ def test_pr_fail_embeds_findings_and_summary_does_not_duplicate() -> None: assert "returns 500 on the timestamp branch" not in slot["summary"] # The derived TEXT mirror renders the findings table (render_markdown). assert "returns 500 on the timestamp branch" in t.pr_reviewer_notes + + +def test_pr_fail_findings_validation_rejects_prose_file_names_evidence() -> None: + """The static validator behind ``pr_fail`` — mirrors QA's + ``fail_review`` rejection: a non-path ``file`` is refused, and the + remediate points the reviewer at ``evidence`` instead.""" + findings = [ + { + "file": "PR #676 description", + "severity": "major", + "expected": "matches the acceptance criteria", + "actual": "diverges from the acceptance criteria", + } + ] + validated, rejection = PRGateMixin._validate_pr_fail_findings(None, None, findings) + assert validated == [] + assert rejection is not None + body = rejection.as_dict() + assert body["error"] == "invalid_state" + assert "evidence" in body["remediate"] + assert "file" in body["remediate"]