Repository navigation
Close verified Security findings by default - #1433
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security runs close addressed findings by default after the existing verification:
no-security-closeorclose = falseunder[security]keeps the finding open.security-closeoverrides the setting. Fixing remains opt-in. Setup, CLI help, the manual, site and ADR 0015 describe the default.Closes #1432
Evidence
cargo test --test security_run a_merged_fix_closes_the_advisory_by_default_naming_its_pull_request -- --exactfailed: advisory wasdraft, expectedclosed.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.
cargo test --test setup setup_keeps_fixing_opt_in_and_defaults_to_closing_merged_findings -- --exactfailed because closing defaulted to no.After: the same test passes; Enter accepts closing while fixing still requires yes.
cargo test --test command_surfacepasses all 20 tests (one ignored fixture).NEXTEST_TEST_THREADS=8 /usr/bin/time -p scripts/affected-tests mainpassed: 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 --checkandnpm run docs:check --prefix sitepassed.Test seams
thirdshift secureCLI with user config, observing the private finding record, merged pull request and Run notification through the existing GitHub and Harness stand-ins.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 generatedsite/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