Complete the SECURITY_REVIEW chain with pure deterministic handlers - #9
Merged
Conversation
Implement the five deferred SECURITY_REVIEW handlers. Unlike the other chains this one has no agent stage: DiffCaptureHandler captures the working-tree diff and the scan handlers review the changed files. All steps are pure/read-only and deterministic; no faked side effects. - backend/app/handlers/security.py: SecretScan (regex over changed-file content; FAIL with locations), DependencyVulnerability (caller-supplied advisories; FAIL on non-waived HIGH/CRITICAL, SKIP if no feed), AuthChangeRisk (auth- affecting changes must be acknowledged via security_ack or FAIL), InputValidationRisk (dangerous sinks eval/exec/os.system/shell=True/...; FAIL with locations), SecurityVerifier (independent verifier: diff required, tests NOT_APPLICABLE, never downgrades a gate FAIL) - backend/app/handlers/base.py: register the security module - backend/tests/test_security_review_chain.py: clean diff -> gated PR, secret FAIL, dangerous-sink FAIL, unacknowledged/acknowledged auth change, blocking vulnerability FAIL, independent verifier, registration - backend/tests/test_documentation_chain.py: the previously-deferred handlers are now all implemented (no chain handler remains deferred) 216 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JQW4FXw46KBvMCVc2EduXT
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d3090c257
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address the P2 review on PR #9: the SECURITY_REVIEW chain is implementation-mode but has no test runner, so EvidenceGateHandler required a TEST artifact unless the caller set metadata.tests_not_applicable — a hidden requirement that made an otherwise valid security review fail before it could be verified or gated. Add SecurityReviewTestsNotApplicableHandler, ordered before EvidenceGateHandler in security_review_chain. It: - applies only to TaskType.SECURITY_REVIEW (no-op SKIP otherwise, so chains with a real TestRunnerHandler keep their full (DIFF, TEST) requirement) - declares tests NOT_APPLICABLE (sets metadata the evidence gate reads) with an explicit reason, recorded as a hashed artifact security_review_tests_not_applicable.json - preserves the no-merge / no-deploy / no-self-certification / evidence-ledger rules (only the evidence gate's TEST requirement is affected, and only for security reviews) Tests now drive SECURITY_REVIEW without any caller-supplied test flag, proving the chain reaches SecurityVerifierHandler and passes the evidence gate; plus a scoping test that the handler is a no-op on non-security task types. 218 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JQW4FXw46KBvMCVc2EduXT
The workflow failed on every PR for two reasons unrelated to the code under review: 1. `git diff --check origin/main...HEAD` errored with "unknown revision origin/main" because actions/checkout@v4 uses a shallow checkout and never fetches the base. Fix: checkout with fetch-depth: 0 and explicitly fetch the PR base ref, then run the diff check against that base ref. 2. `python -m app.harness.verify_pr` referenced a module that does not exist anywhere in the repo. Fix: remove the stale step; verification is already covered by the existing, verified pytest and diff-check steps. Workflow-only change; no runtime code added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JQW4FXw46KBvMCVc2EduXT
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
2 participants
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.
What
Completes the last deferred chain,
SECURITY_REVIEW, by implementing its five handlers as pure / read-only, deterministic steps. With this merged, every handler referenced by every registered chain resolves — no chain handler remains deferred.Unlike the implementation-style chains, SECURITY_REVIEW has no agent stage:
DiffCaptureHandlercaptures the working-tree diff via the git runner, and the scan handlers review the changed files. Core law upheld: no faked side effects; the diff is reviewed, not generated; only an independent verifier certifies; the PR is GATED, never auto-merged.Handlers (
backend/app/handlers/security.py)SecretScanHandler— regex scan of changed-file content for hardcoded secrets (private-key headers, AWS access-key IDs,api_key/secret/password/token = "…"). FAILs withfile:linelocations.DependencyVulnerabilityHandler— evaluates caller-supplied advisories (no live CVE feed); FAILs on any non-waived HIGH/CRITICAL; SKIPs with a reason when no feed is provided.AuthChangeRiskHandler— flags auth-affecting changes; such a change must be explicitly acknowledged viametadata.security_ack.auth_change_reviewed, otherwise it FAILs.InputValidationRiskHandler— flags dangerous injection sinks (eval/exec/os.system/shell=True/pickle.loads/yaml.loadwithout SafeLoader). FAILs with locations.SecurityVerifierHandler(VERIFIER) — independent verifier: composes no-self-certification + evidence (DIFF required) + scope gates; tests NOT_APPLICABLE; never downgrades a gate FAIL.Other changes
base.py: registered thesecuritymodule.tests/test_documentation_chain.py: the previously-deferred handlers are now all implemented, so the assertion flips from "deferred" to "registered".Tests
backend/tests/test_security_review_chain.py(8 tests): clean diff → gated PR, hardcoded-secret FAIL, dangerous-sink FAIL, unacknowledged vs acknowledged auth change, blocking-vulnerability FAIL, independent verifier, registration. Tests drive a real git working tree (this chain captures viaGitRunner).Status
This is the final secondary chain. All seven registered chains (AI_READINESS_AUDIT, IMPLEMENTATION, DOCUMENTATION_UPDATE, BUG_FIX, CI_FAILURE_REPAIR, DEPENDENCY_UPDATE, SECURITY_REVIEW) now route fully green deterministically.
🤖 Generated with Claude Code
Generated by Claude Code