Skip to content

Close verified Security findings by default - #1433

Merged
JacobStephens2 merged 3 commits into
mainfrom
issue-1432
Oct 27, 2026
Merged

JacobStephens2 merged 3 commits into
mainfrom
issue-1432

Conversation

@JacobStephens2

@JacobStephens2 JacobStephens2 commented Oct 27, 2026 •

Copy link
Copy Markdown
Owner

Summary

Security runs close addressed findings by default after the existing verification:

fix merges
  closing allowed? (default yes)
    proof-of-concept passes on merged Base branch?
      close private finding and name the merged PR

no-security-close or close = false under [security] keeps the finding open. security-close overrides the setting. Fixing remains opt-in. Setup, CLI help, the manual, site and ADR 0015 describe the default.

Closes #1432

Evidence

  • Before: cargo test --test security_run a_merged_fix_closes_the_advisory_by_default_naming_its_pull_request -- --exact failed: advisory was draft, expected closed.
    After: the same test passes, including merged-PR attribution and Run notification checks. Coverage also exercises private finding issues, both opt-outs, command precedence and a failing proof-of-concept check.
  • Before: cargo test --test setup setup_keeps_fixing_opt_in_and_defaults_to_closing_merged_findings -- --exact failed because closing defaulted to no.
    After: the same test passes; Enter accepts closing while fixing still requires yes.
  • The existing compact-help test caught an extra help line during full validation; folding the closing default into the original Security line keeps the 65-line limit. cargo test --test command_surface passes all 20 tests (one ignored fixture).
  • Focused checks passed: 123 Security integration tests, 34 Setup integration tests, 16 flag-resolution tests, 37 config tests and 51 Setup module tests. Fixture subprocess entry points remain ignored where applicable.
  • Final NEXTEST_TEST_THREADS=8 /usr/bin/time -p scripts/affected-tests main passed: 51 site browser tests and 3,138 Rust tests, with 69 Rust tests skipped. Wall time: 1,480.69s including compilation and browser checks; nextest execution: 1,410.934s, Rust compilation: 13.76s (nextest 0.9.150, eight workers).
  • cargo clippy --all-targets -- -D warnings, cargo fmt --check, git diff --check and npm run docs:check --prefix site passed.

Test seams

  • The thirdshift secure CLI with user config, observing the private finding record, merged pull request and Run notification through the existing GitHub and Harness stand-ins.
  • The Setup CLI and its terminal prompts, observing the saved User config, plus existing config-completion and flag-resolution interfaces.

Review coverage

Fresh independent Standards and Spec reviews of the final diff against main: 0 findings on each axis. Both reviewed all 11 changed files, including generated site/docs/index.html. Unread changed files: none. Generated-output exclusions: none.

Unaddressed findings

Standards

None.

Spec

None.

Security

None.

Security review outcome

No unaddressed introduced findings.

Merge Danger

Door: two-way

Revert or opt out through the existing flag/config setting. Closing still requires a merged fix and a passing proof-of-concept check; an unsuccessful check leaves the record open.

Blast Radius: Security

Security runs with fixing enabled and no explicit closing preference now attempt verified closure after merging.

Built with codex codex-cli 0.164.0 · gpt-6.1-sol · xhigh

@JacobStephens2
JacobStephens2 merged commit acf9c53 into main Oct 27, 2026
18 checks passed
@JacobStephens2
JacobStephens2 deleted the issue-1432 branch October 27, 2026 12:54
This was referenced Oct 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant