Skip to content

fix(strix): sanitize benign MODEL QUALITY WARNING banner before infra-failure detection - #1311

Open
seonghobae wants to merge 1 commit into
mainfrom
fix/strix-model-quality-banner-false-fail-closed
Open

fix(strix): sanitize benign MODEL QUALITY WARNING banner before infra-failure detection#1311
seonghobae wants to merge 1 commit into
mainfrom
fix/strix-model-quality-banner-false-fail-closed

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Problem

Strix prints a box-drawn MODEL QUALITY WARNING banner at startup whenever the configured model is not on its own hardcoded list of recommended frontier models — a static disclaimer about model choice, unrelated to the scan's actual outcome:

╭─ STRIX ──────────────────────────────────────────────────────────────────────╮
│  MODEL QUALITY WARNING                                                       │
│  'nvidia_nim/nvidia/nemotron-3-super-120b-a12b' is not a recommended         │
│  frontier model for Strix.                                                   │
│  You can continue, but weaker models may miss vulnerabilities or produce     │
│  lower-quality findings.                                                     │
╰──────────────────────────────────────────────────────────────────────────────╯

The banner's literal WARNING text satisfies has_detected_infrastructure_error()'s generic Fatal|Denied|Warn|Warning substring matcher against $STRIX_LOG (and the equivalent has_strix_report_failure_signal() matcher against report .log artifacts), so any clean, 0-vulnerability scan on the org's configured default model is misclassified as a provider infrastructure failure and fails closed, even though the scan itself succeeded (rc=0) with zero findings.

Reproduced directly from a real PR

TEPP#214's strix required check failed with:

Strix run emitted provider infrastructure or failure-signal output; failing closed.

...while its own captured transcript shows a complete penetration-test summary reporting "Low" risk posture and "Vulnerabilities 0". STRIX_LLM_DEFAULT_PROVIDER: nvidia_nim resolves to nvidia_nim/nvidia/nemotron-3-super-120b-a12b, which is not on Strix's recommended list, so this banner — and this false failure — fires on every successful scan using the org's own configured default model.

The same banner text (6 occurrences across fallback attempts) also appears in fast-mlsirm PR #1237's strix job log. Given STRIX_LLM_DEFAULT_PROVIDER is an org-wide default, this likely contributes to the "zero PRs mergeable" pattern tracked in #1212.

Fix

Strip the banner — matched structurally by its box-drawing delimiters plus the MODEL QUALITY WARNING marker line, so it does not depend on the exact recommended-model list or wrapped text — from:

  1. $STRIX_LOG (new sanitize_strix_console_log, called right after preserve_attempt_log so the unmodified transcript is still archived for audit) before has_detected_infrastructure_error() runs, and
  2. report .log files inside sanitize_known_strix_report_warnings() before has_strix_report_failure_signal() runs — mirroring the existing, narrowly-scoped sanitization already used there for Strix's other known-benign internal warning (strix.core.execution: agent ... forcing tool continuation).

This does not touch vulnerability-severity classification (reported_vulnerability_signal, has_blocking_vulnerability_reports, STRIX_FAIL_ON_MIN_SEVERITY) — a real finding still fails closed exactly as before. Only this one cosmetic, always-present-on-non-frontier-models disclaimer is excluded from the generic infra-error matcher.

Testing

New regression case console-model-quality-warning-banner-sanitized reproduces the exact TEPP#214 failure via the existing fake-strix-stub harness (emits the real banner + a clean Vulnerabilities 0 summary + exit 0).

  • Red before fix: git stash of strix_quick_gate.sh alone (keeping the new test) reproduces Strix run emitted provider infrastructure or failure-signal output; failing closed. — 2 failures.
  • Green after fix: STRIX_TEST_CASE_FILTER=console-model-quality-warning-banner-sanitized bash scripts/ci/test_strix_quick_gate.sh exits 0.
  • Full test_strix_quick_gate.sh suite (120 cases) run locally in the background to confirm no regressions in the other scenarios; will also run as this PR's own required CI.

🤖 Generated with Claude Code


Open in Devin Review

…before infra-failure detection

Strix prints a box-drawn "MODEL QUALITY WARNING" banner at startup
whenever the configured model is not on its own hardcoded list of
recommended frontier models -- a static disclaimer about model choice,
unrelated to the scan's actual outcome. The banner's literal "WARNING"
text satisfies has_detected_infrastructure_error()'s generic
Fatal/Denied/Warn/Warning substring matcher (and, when Strix persists
its own console transcript as a report artifact, the equivalent
has_strix_report_failure_signal() matcher too), so any clean,
0-vulnerability scan on the org's configured default model
(nvidia_nim/nvidia/nemotron-3-super-120b-a12b, which is not on Strix's
recommended list) is misclassified as a provider infrastructure
failure and fails closed even though the scan itself succeeded (rc=0)
with zero findings.

Reproduced directly from a real PR run: TEPP#214's "strix" required
check failed with "Strix run emitted provider infrastructure or
failure-signal output; failing closed." while its own captured
transcript shows a complete penetration test summary reporting "Low"
risk posture and "Vulnerabilities 0". The same banner text (6
occurrences across fallback attempts) appears in fast-mlsirm PR
#1237's strix job log, suggesting this contributes to the org-wide
"zero PRs mergeable" pattern tracked in #1212.

Fix: strip the banner (matched structurally by its box-drawing
delimiters plus the "MODEL QUALITY WARNING" marker line, so it does
not depend on the exact recommended-model list or wrapped text) from
$STRIX_LOG before has_detected_infrastructure_error() runs, and from
report .log files inside sanitize_known_strix_report_warnings() before
has_strix_report_failure_signal() runs -- mirroring the existing,
narrowly-scoped sanitization already used for Strix's other known
benign internal warning. This does not touch vulnerability-severity
classification (reported_vulnerability_signal, has_blocking_vulnerability_reports,
STRIX_FAIL_ON_MIN_SEVERITY): a real finding still fails closed exactly
as before. Only this one cosmetic, always-present-on-non-frontier-models
disclaimer is excluded from the generic infra-error matcher.

New regression test console-model-quality-warning-banner-sanitized
reproduces the exact TEPP#214 failure via the existing fake-strix-stub
harness: fails with "Strix run emitted provider infrastructure or
failure-signal output; failing closed." on the pre-fix gate script,
passes on the post-fix version. Verified both directions locally
(git stash of strix_quick_gate.sh alone reproduces the failure; restoring
it passes) via STRIX_TEST_CASE_FILTER=console-model-quality-warning-banner-sanitized.
@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 19 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 94f24c80-64f5-4221-9d26-29c087ed09c4

📥 Commits

Reviewing files that changed from the base of the PR and between 613a33e and 7c6d80c.

📒 Files selected for processing (2)
  • scripts/ci/strix_quick_gate.sh
  • scripts/ci/test_strix_quick_gate.sh

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Comment on lines +181 to +187
model_quality_banner = re.compile(
r"╭─[^\n]*╮\n"
r"(?:[^\n]*\n)*?"
r"[^\n]*MODEL QUALITY WARNING[^\n]*\n"
r"(?:[^\n]*\n)*?"
r"╰─[^\n]*╯\n?"
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Banner regex can span a preceding box

The banner regex (scripts/ci/strix_quick_gate.sh:181-187, 235-241) anchors on the first ╭─...╮ and scans forward to the MODEL QUALITY WARNING line. If Strix emits any box before the warning box, the leading (?:[^\n]*\n)*? consumes it too, deleting both boxes and everything between. Startup content is benign, but a real failure signal printed between those boxes would be stripped.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines 2841 to +2842
preserve_attempt_log "$model" "$rc"
sanitize_strix_console_log "$STRIX_LOG"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Published last-attempt log has banner stripped

preserve_attempt_log copies the raw transcript before sanitization, so the per-attempt archive keeps the banner. But publish_artifact_reports copies $STRIX_LOG to gate-last-attempt.log in the cleanup trap, which runs after sanitization (scripts/ci/strix_quick_gate.sh:131-132), so that published copy loses the banner. Audit fidelity only; the full transcript survives under gate-attempts/.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Note on the failing `strix` check here: it's the exact bug this PR fixes, reproducing on itself. `strix_quick_gate.sh` resolves its "trusted Strix source" from `main` (by design — a PR can't be allowed to rewrite its own security gate to auto-pass), so this PR's own scan still runs the pre-fix script and hits the same MODEL QUALITY WARNING false fail-closed described above (confirmed in the job log: clean "Vulnerabilities 0" summary, then "Strix run emitted provider infrastructure or failure-signal output; failing closed."). This is a chicken-and-egg case — the fix can't pass its own gate until it's merged.

Every other check has passed: CodeQL, Semgrep (SAST + OSS), gitleaks, trivy-fs, dependency-review, SBOM generation, pip-audit, osv-scan, Scorecard, Devin Review, CodeRabbit, coverage-evidence/coverage-source-tree, noema-review, and opencode-review. Full local regression run of `test_strix_quick_gate.sh` (120 cases, including the new red/green-verified `console-model-quality-warning-banner-sanitized` case) also passes with no other-scenario regressions.

Flagging for whoever has merge authority here, consistent with the same one-off-intervention pattern used for prior self-referential CI-pipeline fixes in this repo.

Copy link
Copy Markdown
Contributor Author

Fresh downstream canary for this exact false-fail boundary (2026-08-25 KST): canonical Security Scan owner PR ContextualWisdomLab/.github#897 is still open at exact head 6406b4663b216e4a85fbf96a7cbd33bf9c0a990e on protected main@613a33e0cb1c6db9790fae99f6253445712ac37a. Its current Strix run/job 32710533347/97549503711 reaches a complete fallback scan with 0 vulnerabilities, then rejects that completed result with Strix run emitted provider infrastructure or failure-signal output; failing closed. Current protected-main scripts/ci/strix_quick_gate.sh still uses the broad Fatal|Denied|Warn|Warning|Timeout report matcher while sanitize_known_strix_report_warnings() does not remove the benign MODEL QUALITY WARNING banner. This is therefore not only the TEPP/fast-mlsirm reproduction in this PR body; it is directly blocking #897, which is the canonical Security Scan exact-head repair needed by ContextualWisdomLab/scopeweave#1222.

Acceptance after this repair integrates: rerun unchanged/current #897. A complete authoritative fallback that reports zero findings must no longer be rejected solely because of the model-quality disclaimer, while genuine provider/incomplete-scan signals and real vulnerability findings must remain fail-closed. Once #897 is green on its unchanged exact head and its exact-head binding fix integrates, regenerate ScopeWeave #523/#553 Security evidence against their then-current contributor heads. Later fallback/provider-routing errors (for example direct-provider 404/rate-limit paths) remain separate owner lanes; this comment is specifically the reproduced benign-warning false-failure boundary.

Copy link
Copy Markdown
Contributor Author

Second live canary from the other canonical ScopeWeave #1222 owner path: ContextualWisdomLab/.github#941 is currently at exact head f5d3224404a9dcd04a1b4e1650c51bc256876586 on protected main@613a33e0cb1c6db9790fae99f6253445712ac37a; current Strix run/job 32718660538/97549126961 checks out that exact PR head correctly. In that job, the NVIDIA fallback nvidia_nim/nvidia/llama-3.3-nemotron-super-49b-v1.5 reaches Penetration test completed and Vulnerabilities 0 (No exploitable vulnerabilities detected), but the trusted gate immediately rejects the attempt with Strix run emitted provider infrastructure or failure-signal output; failing closed. The same attempt prints the benign MODEL QUALITY WARNING banner handled by this PR. The chain then unnecessarily falls through to openai-direct/gpt-5.6-luna, which separately returns 404.

This matters directly to ScopeWeave because #941 is the canonical SAST Semgrep owner needed to repair the synthetic-checkout defect tracked by .github#1222. Acceptance for this PR therefore includes rerunning unchanged/current #941 after integration: the completed zero-vulnerability NVIDIA fallback must be accepted rather than rejected solely by the model-quality disclaimer, so a clean completed attempt does not fall through to a later broken provider. Genuine rate-limit/incomplete-scan/provider failures and real vulnerability findings must remain fail-closed; the direct-OpenAI 404 remains a separate owner lane if it is reached for a legitimately incomplete earlier attempt.

Copy link
Copy Markdown
Contributor Author

Current-head repair note after verifying the two unresolved Devin findings against 7c6d80c27b7be017d46560c7b4cc0f7fa2714288 (base/protected main still 613a33e0cb1c6db9790fae99f6253445712ac37a; current hosted workflows are green): both findings are valid and should be fixed before treating this owner PR as review-ready.

  1. Fail-closed preservation bug in the banner matcher. Both new Python regexes begin at the first ╭─...╮ and allow arbitrary intervening lines before MODEL QUALITY WARNING. If another box precedes the model-quality box, the substitution can consume that preceding box and text between it and the warning box. That can erase a real Fatal/Denied/Warning/Timeout signal before deterministic fail-closed classification. Smallest robust remedy: parse/remove one complete box at a time (opening through its matching first closing ) and delete the block only when that same block contains MODEL QUALITY WARNING; do not use a cross-box wildcard. RED acceptance: prepend a separate box containing a genuine provider-failure signal, then the benign model-quality box, followed by an otherwise clean zero-finding result. Sanitization must remove only the benign box and the gate must still fail on the preserved genuine signal. Add the equivalent structured-report regression as well as the console regression.

  2. Audit-artifact fidelity. preserve_attempt_log archives the raw transcript, but sanitize_strix_console_log mutates $STRIX_LOG in place; the cleanup trap later publishes that mutated file as gate-last-attempt.log. Keep the detector input sanitized without destroying the raw last-attempt artifact (for example, classify a sanitized temporary/in-memory copy, or publish the corresponding raw preserved attempt). RED acceptance: successful non-frontier-model attempt must leave gate-last-attempt.log byte-faithful to the raw console including the disclaimer, while deterministic classification ignores only the benign banner and gate-attempts/ remains raw.

After those regressions are GREEN, rerun the full Strix gate suite and then the live #897/#941 canaries. This preserves the intended false-fail repair without creating a fail-open erasure path or weakening evidence provenance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant