mirror of
https://github.com/usestrix/strix.git
synced 2026-11-14 09:01:25 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
parent
5379e243a7
commit
7eccdf8b9b
3 changed files with 84 additions and 18 deletions
|
|
@ -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/<subdir>/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/<subdir>/..., where <subdir> 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 <subdir>/ prefix
|
||||
# breaks that mapping. Drop the mount component too. A bare /workspace/<subdir>
|
||||
# (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.
|
||||
|
|
|
|||
|
|
@ -3,8 +3,9 @@
|
|||
Agents see the target at /workspace/<subdir>/... 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/<subdir> 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
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue