From 7eccdf8b9b231a3c4b1903a577bf2b2d4758279f Mon Sep 17 00:00:00 2001 From: Sean Turner Date: Fri, 23 Oct 2026 18:10:59 +0100 Subject: [PATCH] fix(reporting): repo-root-relative code paths + schema-optional gated fields Address the two Greptile findings on #1578: - _to_workspace_relative stripped only /workspace, leaving the per-target mount subdir (e.g. b8bLSpkh/src/x.py). SARIF/report consumers want a repo-root-relative path (strix/report/sarif.py: "File locations must be repo-relative"); the subdir prefix breaks GitHub code-scanning / IDE viewers rooted at the repo. Strip the mount subdir too -> src/x.py. - The flag-gated report fields (poc_script_code under STRIX_REQUIRE_POC, the analysis fields under STRIX_RELAX_FINDING_FIELDS) were declared as schema-required args, so the model was forced to emit (and fabricate) a PoC even when the operator disabled the requirement. Default them to "" (keyword-only) so they are schema-OPTIONAL; the existing flag-aware runtime validators remain the single source of required-ness. The seven always-required essentials stay schema-required. Tests: repo-relative + bare-subdir cases; schema-optional assertion over the generated tool schema. 399 reporting/registration tests pass; ruff clean. Co-Authored-By: Claude Opus 4.8 --- strix/tools/reporting/tool.py | 52 +++++++++++++++++++++------- tests/test_code_location_paths.py | 19 +++++++--- tests/test_reporting_poc_optional.py | 31 +++++++++++++++++ 3 files changed, 84 insertions(+), 18 deletions(-) diff --git a/strix/tools/reporting/tool.py b/strix/tools/reporting/tool.py index 4223f1f99..dd98b5091 100644 --- a/strix/tools/reporting/tool.py +++ b/strix/tools/reporting/tool.py @@ -61,14 +61,15 @@ _WORKSPACE_ROOT = "/workspace" def _to_workspace_relative(path: str) -> str: - """Make a sandbox-absolute code-location path relative to the workspace root. + """Make a sandbox-absolute code-location path repo-root-relative. Agents report the absolute path they see in the sandbox (e.g. ``/workspace//src/x.py``). Rejecting that as "must be relative" is the silent cause of findings-detected-but-never-filed (findings=N filed_reports=0): the report is dropped and the agent cannot file what it found. Strip the - workspace-root prefix so the path validates and files. A path genuinely outside - the workspace is left unchanged for the validator to reject. + workspace root *and* the per-target mount subdir so the path is repo-root-relative + (what SARIF and the report expect) and files. A path genuinely outside the + workspace is left unchanged for the validator to reject. """ if not path: return path @@ -76,9 +77,20 @@ def _to_workspace_relative(path: str) -> str: if not p.is_absolute(): return path try: - return str(p.relative_to(_WORKSPACE_ROOT)) + rel = p.relative_to(_WORKSPACE_ROOT) except ValueError: return path + # The target is mounted at /workspace//..., where is Strix's + # per-target mount point, not part of the scanned repo. SARIF and the report + # want a *repo-root-relative* path (strix/report/sarif.py: "File locations must + # be repo-relative POSIX paths") so a consumer rooted at the repo — GitHub code + # scanning, an IDE SARIF viewer — can resolve it; a leading / prefix + # breaks that mapping. Drop the mount component too. A bare /workspace/ + # (no file beneath it) has nothing to strip and is left as-is. + parts = rel.parts + if len(parts) > 1: + return str(PurePosixPath(*parts[1:])) + return str(rel) def _validate_file_path(path: str) -> str | None: @@ -1102,19 +1114,27 @@ def _caller_identity(ctx: RunContextWrapper) -> tuple[str | None, str | None]: @function_tool(timeout=180, strict_mode=False) async def create_vulnerability_report( ctx: RunContextWrapper, + *, title: str, description: str, impact: str, target: str, - technical_analysis: str, - poc_description: str, - poc_script_code: str, - remediation_steps: str, - evidence: str, - assumptions: str, - counterevidence: str, + # Flag-gated fields default to "" so they are schema-OPTIONAL: STRIX_REQUIRE_POC + # governs poc_script_code and STRIX_RELAX_FINDING_FIELDS the analysis fields. When a + # flag is unset these are still required, but enforcement lives in the flag-aware + # runtime validators (_validate_required_text / _validate_analysis_fields), not in a + # static schema. A required-in-schema field would force the model to emit (and so + # fabricate) a PoC even when the operator disabled the PoC requirement — see #1439. + technical_analysis: str = "", + poc_description: str = "", + poc_script_code: str = "", + remediation_steps: str = "", + evidence: str = "", + assumptions: str = "", + counterevidence: str = "", + severity_change_conditions: str = "", + # Always required (never relaxable); no default keeps them schema-required. confidence: str, - severity_change_conditions: str, fix_effort: str, cvss_breakdown: dict[str, str], endpoint: str | None = None, @@ -1378,7 +1398,13 @@ async def create_vulnerability_report( target: Affected URL / domain / repository. technical_analysis: The mechanism and root cause. poc_description: Step-by-step reproduction (steps only, no code). - poc_script_code: Working PoC (Python preferred). + poc_script_code: Working PoC (Python preferred). Required by default; + omit it only when the operator has set ``STRIX_REQUIRE_POC=0`` for a + raw-findings run — never fabricate a PoC to satisfy the field. + ``technical_analysis``, ``poc_description``, ``remediation_steps``, + ``evidence``, ``assumptions``, ``counterevidence`` and + ``severity_change_conditions`` are likewise required unless the + operator relaxed them via ``STRIX_RELAX_FINDING_FIELDS``. remediation_steps: Specific, actionable fix (prose, no code). evidence: Concrete proof the issue is real and exploitable — request/response excerpts, observed behavior, tool output. diff --git a/tests/test_code_location_paths.py b/tests/test_code_location_paths.py index 753efbf5a..1d5eda736 100644 --- a/tests/test_code_location_paths.py +++ b/tests/test_code_location_paths.py @@ -3,8 +3,9 @@ Agents see the target at /workspace//... and report code_locations with that absolute path. Rejecting it as "must be relative" silently drops the report — the cause of findings-detected-but-never-filed (findings=N filed_reports=0; see -usestrix/strix#1439). The normalizer now strips the workspace root so filing -succeeds, while paths genuinely outside the workspace are still rejected. +usestrix/strix#1439). The normalizer now strips the workspace root and the per-target +mount subdir so the path is repo-root-relative and files, while paths genuinely outside +the workspace are still rejected. """ from __future__ import annotations @@ -16,10 +17,12 @@ from strix.tools.reporting.tool import ( ) -def test_workspace_absolute_becomes_relative() -> None: +def test_workspace_absolute_becomes_repo_relative() -> None: + # The leading mount subdir (b8bLSpkh) is Strix's per-target mount point, not + # part of the repo; SARIF/report consumers want a repo-root-relative path. assert ( _to_workspace_relative("/workspace/b8bLSpkh/src/picklescan/scanner.py") - == "b8bLSpkh/src/picklescan/scanner.py" + == "src/picklescan/scanner.py" ) @@ -31,6 +34,12 @@ def test_outside_workspace_left_absolute() -> None: assert _to_workspace_relative("/etc/passwd") == "/etc/passwd" +def test_bare_mount_subdir_has_nothing_to_strip() -> None: + # /workspace/ with no file beneath it: only the subdir component is + # left after stripping the root, and there is nothing repo-relative to keep. + assert _to_workspace_relative("/workspace/b8bLSpkh") == "b8bLSpkh" + + def test_absolute_workspace_location_normalizes_and_validates() -> None: loc = [ { @@ -42,7 +51,7 @@ def test_absolute_workspace_location_normalizes_and_validates() -> None: ] normalized = _normalize_code_locations(loc) assert normalized is not None - assert normalized[0]["file"] == "b8bLSpkh/src/picklescan/scanner.py" + assert normalized[0]["file"] == "src/picklescan/scanner.py" assert _validate_code_locations(normalized) == [] # files cleanly now diff --git a/tests/test_reporting_poc_optional.py b/tests/test_reporting_poc_optional.py index 0f41d76d8..745135da7 100644 --- a/tests/test_reporting_poc_optional.py +++ b/tests/test_reporting_poc_optional.py @@ -47,3 +47,34 @@ def test_flag_falsey_and_truthy_values(monkeypatch: pytest.MonkeyPatch) -> None: for on in ("1", "true", "yes", ""): # empty/ambiguous -> safe default (required) monkeypatch.setenv("STRIX_REQUIRE_POC", on) assert reporting_tool._poc_required() is True, on + + +# The runtime validators above are the single source of required-ness, because the +# flags are per-run env and the tool schema is built once at import. The schema must +# therefore mark the flag-gated fields OPTIONAL — otherwise a required-in-schema PoC +# forces the model to emit (and fabricate) one even when STRIX_REQUIRE_POC=0. The +# always-required essentials stay schema-required. +_SCHEMA_OPTIONAL = frozenset( + { + "poc_script_code", + "technical_analysis", + "poc_description", + "remediation_steps", + "evidence", + "assumptions", + "counterevidence", + "severity_change_conditions", + } +) +_SCHEMA_REQUIRED = frozenset( + {"title", "description", "impact", "target", "confidence", "fix_effort", "cvss_breakdown"} +) + + +def test_flag_gated_fields_are_schema_optional() -> None: + required = set(reporting_tool.create_vulnerability_report.params_json_schema["required"]) + # every always-required essential is still schema-required + assert required >= _SCHEMA_REQUIRED + # no flag-gated field is schema-required (the model may omit it; the flag-aware + # runtime validator decides whether the omission is allowed) + assert not (_SCHEMA_OPTIONAL & required)