Skip to content

Run the Security review on security.harness when it is set - #932

Merged
JacobStephens2 merged 5 commits into
mainfrom
issue-910
Oct 21, 2026
Merged

JacobStephens2 merged 5 commits into
mainfrom
issue-910

Conversation

@JacobStephens2

@JacobStephens2 JacobStephens2 commented Oct 21, 2026 •

Copy link
Copy Markdown
Owner

Summary

A Security review now runs on the User config's security.harness when that is set, with that Harness's own [harness.<name>] Model and Effort. This applies wherever a review runs: in a Run, on a Spec run's Spec PR, and in a Base fix. Every other session keeps the Command's Harness. The command's harness, model and effort words don't move the review. With security.harness blank or missing, nothing changes.

 Run (harness.default = "codex", security.harness = "claude")
   check Harness: codex
+  check Harness: claude, [harness.claude]   (only when a review will run on it)
   implement            → codex
-  security-review      → codex
+  security-review      → claude · [harness.claude] model · effort
   Repairs / Resumes    → codex
   Built with codex …

How the review's Choice reaches its session:

Asks::of_run → Flags::security_review_harness   (None unless a review is asked, security.harness is set, and it differs from the Command's)
  Run: Choice::check_with_security_review        (before any work)
  Pass: Outside::check_harness(dispatches)       (pickup, Security fix, architect without --plan-only)
  ChildAsks → --security-review-{harness,model,effort}   (child Runs get it already checked)
  Delivery → Sessions::checked_within(…, security_review)
    OnMachine::run_session: Purpose::SecurityReview → the review's Choice and adapter

Where the Harness shows up:

  • Logs: the review's Session log, its .models.json and the Run notification's "Session models" name the Harness it ran on. The Command log adds security-review: on claude · … · high, chosen by security.harness in the User config.
  • Factory skills: both Harnesses' skills are linked into the worktree.
  • Recovery: the Recovery command needs no new word, because a Continuation reads security.harness again.
  • Docs: the glossary's Harness and Security review entries, the guide ([security] comments, the Harness section, a new Security review paragraph), Setup's security.harness comment and the regenerated site docs are updated. New ADR-0021 says a Command may use more than one Harness, starting with the Security review.

Seams under test:

  • the Run's command line, through the Claude Code and Codex fakes (tests/security_review.rs, tests/spec_run.rs, tests/pickup_run.rs, tests/security_run.rs);
  • Asks::of_run's resolution of the review's Harness (src/asks.rs);
  • the child-Run hidden-argument round trip (src/child_run.rs).

Evidence

  • Before: a_runs_security_review_runs_on_the_security_harness_and_its_other_sessions_on_the_commands failed. Two Codex calls, the second being the review (assertion left == right failed: [Object {"argv": … "exec" …}, …]).
    After: it passes. Its Codex calls are the implement session and a CI Repair. The review is the only Claude Code session, with --model claude-opus-5-5 --effort high. .models.json says claude for security-review and codex for implement. The Run notification lists security-review: claude · claude-opus-5-5, and the body says Built with codex.
  • The same split, shown with the fakes:
    • a Spec run's Spec PR (a_spec_runs_security_review_runs_on_the_security_harness_and_its_tickets_on_the_commands);
    • a Base fix (a_base_fix_runs_its_security_review_on_the_security_harness);
    • a harness codex effort max command (a_commands_harness_word_does_not_choose_the_security_reviews_harness);
    • a Continuation from the printed Recovery command, which ends harness codex (a_continuation_from_the_recovery_command_runs_its_security_review_on_the_security_harness_again).
  • A security Harness that can't run:
    • A Run fails before any work with the Security review's Harness claude, chosen by security.harness in the User config, can't run: ….
    • A Pickup run fails the same way before it makes its Claim.
    • Without a review asked, the security Harness is not checked.
  • Before: an audit-only thirdshift secure harness claude with [harness.codex] model = "no-such-model" failed with the Security review's Harness codex … is not in Codex's catalog.
    After: it passes and makes no Codex check (an_audit_only_security_run_does_not_check_the_security_review_harness).
  • Full checks: cargo test gave 2196 passed and 0 failed. cargo clippy --all-targets -- -D warnings and cargo fmt --check are clean. cd site && npm run docs:check passes.

Merge Danger

Door: two-way. Only User configs that set security.harness and turn on Security reviews behave differently, and reverting restores the old routing.

Blast Radius: Security reviews.

  • Configs with security.harness set and reviews on now also check that Harness before work, and fail before any work if it can't run.
  • Child Runs take new hidden arguments. A child is always the same binary as its parent, so no version skew is possible.
  • Command::Run now holds a Box<RunArgs> (clippy large_enum_variant).

Unaddressed findings

Standards

All of these are judgement-call smells. None says behaviour is wrong.

  • Data Clumps in src/child_run.rs (the SESSIONS_* and SECURITY_REVIEW_* triples): declined. Every hidden argument there is its own named constant with its own Reader::take arm and repeat check. The new triple follows that pattern rather than adding a second argument style for one use.
  • pass::LaunchAndGitHub.security_review_harness set only by check_harness, and None in literals: declined. The check settles the Model and Effort on the Harness's names (Choice::check, src/harness.rs), and the dispatched Run must use the settled value. That is why it is stored, as the existing harness: &mut Choice beside it is. The None lines in test literals are the cost of a plain struct field.
  • Sessions::checked_within's sixth parameter, and OnMachine's (Choice, Adapter): declined. Sessions lives below Delivery and knows nothing of ChildAsks. Passing that type down would couple the session module to Run asks.
  • Flags::security_review_harness beside Flags::security_harness: declined. Each name says whose Harness it is (the Security review's, the Security run's), as the glossary distinguishes them.
  • OnMachine::run_session matching purpose twice: declined. The first match picks which Choice, the second which invocation. Merging them would multiply the arms.
  • Test helpers (the inline "review or other" scripts, three argv-inspection styles, "guidance mode" as a bare marker, CODEX_WITH_A_CLAUDE_SECURITY_HARNESS not saying that review is unset): declined.
    • The inline scripts vary their branches: one has no else, another uses a findings review.
    • "guidance mode" and argv windows(2) checks follow the existing tests in both files.
    • Each test that uses the config constant passes security-review on its command line, where it can be seen.

Spec

None. The reviewer's one behaviour finding, a Pass checking the review's Harness when it dispatches nothing, was confirmed with a sharpened version of its test (red before, green after) and fixed. Its three evidence gaps (a Repair on Codex, the Run notification, a Continuation) now have tests.

Security

None. The Security review read the diff and its supporting code (src/session.rs, src/harness.rs, src/child_run.rs, src/pass.rs, src/skills.rs). It found no trust-boundary violation the change introduced:

  • the --security-review-* hidden arguments are accepted only alongside a child Run's --stamp, and carry values from the parent's checked User config;
  • the second Harness's Factory skills are linked through the same symlink-checked CheckedDirectory path and git exclude as the first;
  • the review session gets its own adapter's security-session invocation and refusal interpretation, so it keeps that Harness's security handling.

Review coverage

Neither axis left a changed file unread. site/docs/index.html is generated from docs/guide.md. Standards confirmed it as an exclusion (node generate-docs.mjs --check exits 0, and the changed lines match the guide hunks). Spec read its changed hunks.

Closes #910

🤖 Generated with Claude Code

Built with claude · claude-opus-5-5 · medium

Security review outcome

No unaddressed introduced findings.

JacobStephens2 and others added 5 commits October 21, 2026 06:56
A Run's, a Spec run's and a Base fix's Security review now runs on the
User config's security.harness, with that Harness's own Model and Effort,
whatever the command's harness words ask. Every other session keeps the
Command's Harness. The Run, or the Pass that dispatches it, checks the
security Harness before any work, and child Runs are handed it already
checked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…etup comment and ADR-0021

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…a Run

An audit-only Security run, or architect --plan-only, failed on a check
of a Harness it never used. The Run and the Pass now share one check, and
its failure names what chose the review's Harness. The Run test now shows
a Repair on the Command's Harness and the Run notification's session
models, and a Continuation from the Recovery command reviews on
security.harness again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d Codex calls

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JacobStephens2
JacobStephens2 merged commit 04b5350 into main Oct 21, 2026
18 checks passed
@JacobStephens2
JacobStephens2 deleted the issue-910 branch October 21, 2026 12:00
@JacobStephens2 JacobStephens2 mentioned this pull request Oct 21, 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

Development

Successfully merging this pull request may close these issues.

Run the Security review on security.harness when it is set

1 participant