From b11228192de835b92efe622022f5e9be8bd12f27 Mon Sep 17 00:00:00 2001 From: siundu254 Date: Thu, 1 Oct 2026 11:04:08 +0300 Subject: [PATCH] Resolve PR Comments --- docs/integrations/ci-cd.mdx | 6 ++--- docs/integrations/github-actions.mdx | 4 +-- docs/usage/cli.mdx | 8 ++++-- .../ci-security-scanning-with-strix/SKILL.md | 2 +- .../penetration-testing-with-strix/SKILL.md | 6 ++--- strix/interface/main.py | 16 +++++++++--- tests/test_cli_fail_on.py | 25 +++++++++++++------ 7 files changed, 46 insertions(+), 21 deletions(-) diff --git a/docs/integrations/ci-cd.mdx b/docs/integrations/ci-cd.mdx index e8361e0e..3044319c 100644 --- a/docs/integrations/ci-cd.mdx +++ b/docs/integrations/ci-cd.mdx @@ -23,11 +23,11 @@ strix -n --target ./app --scan-mode quick --scope-mode diff --diff-base origin/m | Code | Meaning | |------|---------| -| 0 | No vulnerabilities found | +| 0 | No vulnerabilities found (with `--fail-on`, none at or above the threshold) | | 1 | Execution error | -| 2 | Vulnerabilities found | +| 2 | Vulnerabilities found (with `--fail-on`, at least one at or above the threshold) | -To fail only on serious findings, pass `--fail-on` with a minimum severity. Lower findings are still reported but exit `0`: +To fail only on serious findings, pass `--fail-on` with a minimum severity. Lower findings are still reported but exit `0`, so a passing job does not mean the report is empty. The threshold only applies to scans that completed; a scan that stopped early fails on any finding: ```bash strix -n --target ./app --scan-mode quick --fail-on high diff --git a/docs/integrations/github-actions.mdx b/docs/integrations/github-actions.mdx index 4dd3886a..f7d94f4f 100644 --- a/docs/integrations/github-actions.mdx +++ b/docs/integrations/github-actions.mdx @@ -46,10 +46,10 @@ The workflow fails when vulnerabilities are found: | Code | Result | |------|--------| -| 0 | Pass — No vulnerabilities | +| 0 | Pass — No vulnerabilities (with `--fail-on`, none at or above the threshold) | | 2 | Fail — Vulnerabilities found | -Add `--fail-on high` (or `critical`, `medium`, `low`) to fail only on findings at or above that severity. Lower findings still appear in the report. +Add `--fail-on high` (or `critical`, `medium`, `low`) to fail only on findings at or above that severity. Lower findings still appear in the report, so a passing run is not necessarily finding-free. A scan that stops before completing fails on any finding. ## Scan Modes for CI diff --git a/docs/usage/cli.mdx b/docs/usage/cli.mdx index 4e38b31d..eccc2926 100644 --- a/docs/usage/cli.mdx +++ b/docs/usage/cli.mdx @@ -66,6 +66,10 @@ strix (--target | --target-list ) [options] threshold are still written to every report artifact. A finding whose severity Strix does not recognize always counts, so the gate never passes on a value it cannot rank. Omit the flag to exit `2` on any finding. + + The threshold only applies to a scan that completed. If the run stopped + early (for example at `--max-budget` or `--max-turns`), any finding exits `2`, + because the scan may not have reached its more serious findings. @@ -170,6 +174,6 @@ strix --target https://app.com --workspace-file ./openapi.yaml:specs/openapi.yam | Code | Meaning | |------|---------| -| 0 | Scan completed successfully (interactive mode always exits `0`; in headless mode, `0` means no vulnerabilities were found) | +| 0 | Scan completed successfully (interactive mode always exits `0`; in headless mode, `0` means no vulnerabilities were found, or with `--fail-on`, none at or above the threshold; lower findings are still in the report) | | 1 | A fatal error occurred before or during the scan (e.g. missing environment variables, Docker unavailable, invalid config file, diff-scope resolution failure, or an unhandled error) | -| 2 | Vulnerabilities found (headless mode only; with `--fail-on`, only findings at or above that severity) | +| 2 | Vulnerabilities found (headless mode only; with `--fail-on`, at least one finding at or above that severity, or any finding if the scan stopped before completing) | diff --git a/skills/ci-security-scanning-with-strix/SKILL.md b/skills/ci-security-scanning-with-strix/SKILL.md index 41293f63..5cf07508 100644 --- a/skills/ci-security-scanning-with-strix/SKILL.md +++ b/skills/ci-security-scanning-with-strix/SKILL.md @@ -67,7 +67,7 @@ Then tell the user to add two repository secrets: `STRIX_LLM` (model id, for exa Notes: - In CI/headless runs Strix automatically scopes to the PR's changed files (`--scope-mode auto`). If diff resolution fails, keep `fetch-depth: 0` or set `--diff-base` to the PR's actual base branch — use `origin/${{ github.base_ref }}` in GitHub Actions rather than a hard-coded `origin/main`, since repos use different default branches. -- Exit codes: `0` pass, `2` vulnerabilities found (fails the job), `1` setup error. Add `--fail-on high` to fail only on high/critical findings; lower ones are still reported. +- Exit codes: `0` pass, `2` vulnerabilities found (fails the job), `1` setup error. Add `--fail-on high` to fail only on high/critical findings; lower ones are still reported, so a passing job is not a finding-free report. A scan that stops before completing fails on any finding. - The runner needs Docker (default GitHub-hosted Ubuntu runners have it). - **Size the budget so the scan completes — do not let it fail open.** A `0` exit means "no validated vulnerabilities in what was analyzed"; if `--max-budget` is hit before the diff is fully covered, the scan wraps up early and can still exit `0`. The "Fail unless the scan completed" step above narrows the gap: `strix_runs//run.json` is `"stopped"` when the scan was cut off at the hard budget limit without a final report. It is not a complete guard — the agents get graduated wrap-up warnings before that limit, and a run that wraps up on a warning still calls `finish_scan` and records `"completed"` with partial coverage. So keep that step in any pipeline that gates merges **and** give the scan real headroom (compare `run.json`'s `llm_usage.cost` against `--max-budget`; if it ran right up to the cap, raise it). For a `quick` diff-scoped PR scan `--max-budget 10` is usually ample, raise it for large diffs. diff --git a/skills/penetration-testing-with-strix/SKILL.md b/skills/penetration-testing-with-strix/SKILL.md index 7090d2b5..7a1f4776 100644 --- a/skills/penetration-testing-with-strix/SKILL.md +++ b/skills/penetration-testing-with-strix/SKILL.md @@ -94,7 +94,7 @@ Key flags: | `--workspace-file PATH[:DEST]` | Copy a file from this machine into `/workspace` before the scan, for a wordlist, a spec, or notes. Repeatable. | | `--max-budget USD` | Hard LLM spend cap; scan wraps up cleanly at the limit. | | `--max-turns N` | Per-agent turn cap (default 500). | -| `--fail-on SEVERITY` | Headless only: exit `2` only for findings at or above `critical`/`high`/`medium`/`low`/`info`. Default: any finding. | +| `--fail-on SEVERITY` | Headless only: exit `2` only for findings at or above `critical`/`high`/`medium`/`low`/`info` on a completed scan (a stopped scan fails on any finding). Default: any finding. | | `--resume RUN_NAME` | Resume a prior run from `strix_runs/`, with its agent history and targets. Cannot be combined with `-t`. | | `--scope-mode` | For code targets: `auto` (diff-scope in CI/headless), `diff` (force changed files only), `full` (whole tree). | | `--diff-base REF` | Branch or commit that `diff` scope compares against. Defaults to the repo's default branch. | @@ -103,9 +103,9 @@ Scans take minutes (`quick`) to hours (`deep`). Run them in the background and p ### Exit codes (headless) -- `0` — finished with no validated vulnerabilities **in what was analyzed** +- `0` — finished with no validated vulnerabilities **in what was analyzed** (with `--fail-on`, none at or above the threshold; lower ones are still in the artifacts) - `1` — fatal error (missing env vars, Docker down, bad config) -- `2` — vulnerabilities found (with `--fail-on`, only at or above that severity; lower findings still land in the artifacts) +- `2` — vulnerabilities found (with `--fail-on`, at least one at or above that severity, or any finding if the scan stopped early) A `0` is not proof of full coverage: if `--max-budget`/`--max-turns` is reached before the scan completes, it wraps up early and still exits `0`. When you need assurance the scan finished, give it enough budget and check `strix_runs//run.json`: a hard budget stop leaves `status: "stopped"`, but an agent that wrapped up early on a budget *warning* still calls `finish_scan` and records `"completed"` — so also sanity-check the run's cost against `--max-budget` and the report's stated coverage before treating a clean result as full coverage. diff --git a/strix/interface/main.py b/strix/interface/main.py index b86ed141..59449e94 100644 --- a/strix/interface/main.py +++ b/strix/interface/main.py @@ -316,17 +316,23 @@ def display_completion_message(args: argparse.Namespace, results_path: Path) -> notify_update(console) -def findings_fail_build(reports: list[dict[str, Any]], fail_on: str | None) -> bool: +def findings_fail_build( + reports: list[dict[str, Any]], fail_on: str | None, *, completed: bool +) -> bool: """Whether headless findings should exit 2 under the ``--fail-on`` threshold. With no threshold any finding fails. Otherwise a finding fails when its severity is at or above the threshold. A severity outside the known scale fails too, so a gate never passes on a value it cannot rank. ``none`` is a known level below ``info`` and only fails without a threshold. + + The threshold only applies to a completed run. A run that stopped early + (budget, turn limit, interrupt) may not have reached its serious findings, + so any finding fails it, as without ``--fail-on``. """ if not reports: return False - if fail_on is None: + if fail_on is None or not completed: return True threshold = FAIL_ON_SEVERITIES.index(fail_on) for report in reports: @@ -526,7 +532,11 @@ def main() -> None: if args.non_interactive: report_state = get_global_report_state() - if report_state and findings_fail_build(report_state.vulnerability_reports, args.fail_on): + if report_state and findings_fail_build( + report_state.vulnerability_reports, + args.fail_on, + completed=report_state.run_record.get("status") == "completed", + ): sys.exit(2) diff --git a/tests/test_cli_fail_on.py b/tests/test_cli_fail_on.py index d0525b14..1d5a0689 100644 --- a/tests/test_cli_fail_on.py +++ b/tests/test_cli_fail_on.py @@ -26,13 +26,13 @@ def _reports(*severities: str | None) -> list[dict[str, Any]]: def test_no_findings_never_fail() -> None: - assert not cli_main.findings_fail_build([], None) - assert not cli_main.findings_fail_build([], "info") + assert not cli_main.findings_fail_build([], None, completed=True) + assert not cli_main.findings_fail_build([], "info", completed=True) @pytest.mark.parametrize("severity", ["critical", "high", "medium", "low", "info", "none"]) def test_without_threshold_any_finding_fails(severity: str) -> None: - assert cli_main.findings_fail_build(_reports(severity), None) + assert cli_main.findings_fail_build(_reports(severity), None, completed=True) @pytest.mark.parametrize( @@ -52,20 +52,31 @@ def test_without_threshold_any_finding_fails(severity: str) -> None: ], ) def test_threshold_compares_severity(fail_on: str, severity: str, expected: bool) -> None: - assert cli_main.findings_fail_build(_reports(severity), fail_on) is expected + assert cli_main.findings_fail_build(_reports(severity), fail_on, completed=True) is expected def test_one_finding_at_threshold_fails_a_mixed_run() -> None: - assert cli_main.findings_fail_build(_reports("info", "low", "high"), "high") + assert cli_main.findings_fail_build(_reports("info", "low", "high"), "high", completed=True) @pytest.mark.parametrize("severity", ["severe", "", None]) def test_unrecognized_severity_fails_closed(severity: str | None) -> None: - assert cli_main.findings_fail_build(_reports(severity), "critical") + assert cli_main.findings_fail_build(_reports(severity), "critical", completed=True) def test_none_severity_passes_any_threshold() -> None: - assert not cli_main.findings_fail_build(_reports("none"), "info") + assert not cli_main.findings_fail_build(_reports("none"), "info", completed=True) + + +@pytest.mark.parametrize("severity", ["medium", "low", "info", "none"]) +def test_threshold_ignored_when_run_did_not_complete(severity: str) -> None: + # A run stopped early (budget, turn limit) may not have reached its serious + # findings, so the threshold must not let it pass. + assert cli_main.findings_fail_build(_reports(severity), "high", completed=False) + + +def test_incomplete_run_without_findings_does_not_fail() -> None: + assert not cli_main.findings_fail_build([], "high", completed=False) def test_parse_fail_on_is_case_insensitive(monkeypatch: pytest.MonkeyPatch) -> None: