mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(findings): path-shaped file refs + per-round collapsible findings (#687)
* 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 <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -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))
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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"]
|
||||
|
||||
Reference in New Issue
Block a user