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) => ( -… {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"]