Repository navigation
test(security): merge twin security tests and give weak ones real checks - #24088
Open
iamwhatever wants to merge 1 commit into
Open
iamwhatever wants to merge 1 commit into
iamwhatever wants to merge 1 commit into
Conversation
Contributor
Author
Old → new test-id map475 ids changed; every one of the 12,061 collected ids maps to exactly one id on the branch. Ids not listed are unchanged.
|
Contributor
Author
Old → new test-id map (continued)
|
Contributor
Author
Old → new test-id map (continued)
|
Contributor
UX Review (Opus 5.5) — ⏭️ skippedRevision |
Contributor
First Principles Review (Opus 5.5) — ⏭️ skippedRevision |
github-actions
Bot
added
the
readiness: checking
Contributor
Design Review (Opus 5.5) — ✅ PASSDesign-level review of Design-Verdict: PASS A test-only consolidation with no src change. Every removed test name is either kept as a parametrize [DESIGN-REVIEWED] b1dcbdb Description read[DESCRIPTION-READ] 2c2c7e9b29673d360747ed606187dee125d1334bb4da835cc7c61a3bc951d461 That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
Contributor
GPT 6.1 Review — ✅ no blocking findingsGPT 6.1 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Contributor
Opus 5.5 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Phase 12 of the test-suite cleanup, over the sandbox, security, policy, exfil, denied-commands, rm-floor and governance tests. Merge-only: no case, assert, platform or skip/xfail is dropped or loosened. Same-body tests in one class (and one cross-file pair of governance list-key tests) become one parametrized test whose ids lead with the old test name. Tests that checked nothing, or only that a mock was called, now also check the wrapped argv, the cleanup path, the returned alias set, the retry schedule or the exfil denial reason. Review-round and ticket narration in comments is cut or rewritten as the reason the rule exists; threat-model and attack-shape comments are kept.
Contributor
Author
|
Disposition, SAST (Semgrep) |
iamwhatever
force-pushed
the
test-cleanup/security
branch
from
October 25, 2026 19:57
c9753e8 to
b1dcbdb
Compare
bolichen97
reviewed
Oct 25, 2026
bolichen97
left a comment
bolichen97
left a comment
Contributor
There was a problem hiding this comment.
Reviewed at head b1dcbdb. Test-only cleanup of the sandbox, security, exfil and governance test area: twin test bodies folded into parametrize with the old name kept in every id, about 25 weak tests given a real outcome check, and review-history comments rewritten as the reason a rule exists.
What I checked:
- No
src/change.git diff --statagainst main shows 44 files undertest/only, one new file (test/test_governance_list_keys.py). - Removed assertions in the denied-commands kill, mint and mentions files: every removed
_denied_by(...) is not None/== ruleline sits in a deleted twin whose cases moved into a surviving test with the same body, including the same.format(...)and.replace("PKILL", ...)transforms. I spot checked the xargs, herestring, separator-in-substitution, exec-capable-exclusion and interpreter-binding groups. - Exfil refusals now assert the reason prefix. The prefix matches every refusal string in the
exfil_bash.pysecurity module, so this is a stricter check, not a looser one. - The governance list-key tests moved verbatim into
test/test_governance_list_keys.pyand now run both keys from one place; the call-site tests stay in their own files. - The strengthened wrap_argv, seal, retry and boot tests only add assertions; no existing assertion was relaxed and no skip, xfail or platform mark was added in the diff.
Security checklist (no src/ change, so each answer concerns runtime behavior): auth or authz NO, new external input parsing NO, trust boundary NO, sandbox or confinement NO (tests of it change, the code does not), safety override or approval gate NO, secret handling NO.
Looks good to ship. I am leaving this as a comment rather than an approval for two maintainer checks: the PR touches 44 files, above the size where one reviewer approves alone, and several required checks were still running at review time with readiness not yet passed.
Minor, not blocking: in test/test_exfil_carveouts.py the two test_curl_file_body_denied cases run the identical command. Keeping both ids preserves the old names, which is fine; a later pass could drop one.
Contributor
Author
|
Disposition, review note on |
github-actions
Bot
added
readiness: passed
This branch has not been deployed
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.
Problem / Motivation
Goal: Fold the copied security, sandbox and governance tests into parametrized tests and give the tests that checked nothing a real outcome check, without dropping or loosening a single security case.
Phase 12, the last area of the test-suite cleanup:
test/test_sandbox*.py,test/test_security*.py,test/test_policy*.py,test/test_exfil*.py,test/test_denied*.py,test/test_rm_floor*.pyandtest/test_governance*.py(152 files;test/test_permission*.pymatches nothing on main).scripts/find_duplicate_test_bodies.pyfound 41 groups of identical test bodies here. About 25 tests checked nothing, or only that a mock was called, and the exfil carve-out tests only checked that some refusal came back. Many comments carried review-round and ticket bookkeeping (round-17:,(pre-push review, measured),security-review bdf0d7e5) instead of the reason a rule exists.Why it matters
A security test that cannot fail gives false comfort on exactly the code where a silent regression costs the most. 9 bugs were planted in the code these tests cover: the strengthened tests catch all 9, the same tests on main catch 1.
Not a goal
src/change.What changed (motivation → approach → change)
Same method as #23352 ... #23977.
Twins. Only true copies were merged: same body, same class (or, for the governance list keys, the same class in two files), one
@pytest.mark.parametrizeand no other decorator on every member. Each merged test parametrizes("origin", ...), whereoriginis the old test name, so an old idtest_x[case]becomesmerged[test_x-case]. Every old in-body comment moved above its own block of cases.test_denied_commands_security_kill.py,_mint.py,_mentions.py: 19 within-class groups (for example the nine by-name kill shapes are nowtest_kill_spellings_that_name_the_cli_are_kills).test_governance_list_otlp_hosts.py+test_governance_list_share_sites.py: the four identicalTestListKeystests now run both cases from the newtest_governance_list_keys.py; the OTLP-only host-glob test stays where it was.test_governance_self_protection.py,test_exfil_carveouts.py(two tests ran the identical input; both ids kept),test_security_token_params.py.Weak tests strengthened.
wrap_argvtests (test_sandbox_argv.py,test_sandbox_argv_profile.py,test_sandbox_governance_mask_model_loaders.py): now check the returned argv, the argv handed to the backend, the cleanup path anddetect_backend's answer, not only that a mock ran.test_sandbox_absent_ceiling_seal_*.py): "does not raise" tests now check the returned alias set is(), the reconciler's removed list, and that tolerated files are left intact.test_sandbox_interpreter_enoent_retry.py): check the returned process and the exact backoff schedule.test_sandbox_argv_spawn.pycontrol test now asserts the poisoned launcher exits non-zero when it is not skipped (it passed with no check before).test_governance_boot.py::test_profile_within_ceiling_bootschecks the booted ceiling carriessandbox.min_level = cc.Blocked: command matches data-exfiltration pattern.Comments.
round-N:prefixes,(pre-push review, measured)→(measured),security-review <sha>tags, "the fix", "on main", "the issue's" and review-bot stories were cut or rewritten as one line on why the rule exists. Every threat-model and attack-shape comment is kept.Backwards compatibility
Compatible: test-only change.
Removes nothing: no test or case is deleted; every old test id maps to exactly one new id.
Tests
Proof: not provable -- test-only change
No case or platform dropped
origin/main@0b9a470630and on this branch: 12,061 and 12,061, no duplicates. 475 ids changed; every old id maps to exactly one new id and no new id is unmapped. The map is in the PR comments below.parametrize:skipif,xfail,usefixtures, platform marks, xdist groups) were dumped on both sides and compared through the id map: 12,061 compared, 0 differences.test/windows-expected-failures.txtortest/windows-collect-ignore.txt.-n 4: 3434 passed, 5 skipped, each time.Coverage
src/kiro_crewline + branch coverage of the 152 area files,-n 4, two runs per side, union of covered lines per side compared:Union base vs union head: 0 lines lost, 0 branches lost.
Planted bugs
One bug at a time in
src/, then the strengthened tests were run on this branch and the same tests on main:mode=offanswers from the backend cacheLeft alone on purpose
These same-body groups remain because merging them would cross classes or files that document different mechanisms, or the copies differ in mocks or fixtures:
test_sandbox_probe.py(two tests with different mocks),test_security_bash_gate.pyandtest_exfil_outbound_sends_allowed.py(different contract classes),test_security_oauth_urls.py(different fixture),test_security_posture_facade.py/test_security_split_patch_reach.py(each derives its own file).test_denied_commands_security_kill.py,_mint.py,_mentions.py,_cost.py,_rows.pyand_ssh_self.py, or span the argv-window and de-quoted-view classes inside_kill.py.Remaining history markers for a later pass: mainly
test_security_conductor_*.py(where "review" and "fix" are the feature under test),test_governance_profiles.py,test_sandbox_mount_source_sweep.pyandtest_security_path_resolve_bounded.py.Manual verification
N/A — test-only change; gates and the checks above cover it. Ran locally: black gate, isort, flake8, comment-history and brand gates against the merge-base, file-line cap, subprocess-encoding gate,
test_flake_pattern_ratchet.pyandtest_wait_discipline_ratchet.py.Related Issues
Follows #23352 ... #23977.
Checklist
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement
Contributions are accepted under the project's Apache-2.0 license.