feat(codex): add local review evidence checker - #110
Conversation
Why: - Medium/high-risk Codex runs should not claim AUTO_OK unless the Signum AUDIT phase produced concrete local agent-review evidence. - The prompt now defines the artifact contract, but a deterministic checker is needed so missing evidence is caught before packaging or review. What changed: - Add a stdlib-only checker for Codex local review evidence under the active contract root. - Require ready Codex review coverage, a concrete reviews/codex.json artifact, audit artifact references, and proofpack evidence for medium/high AUTO_OK runs. - Document the checker in the Codex skill and cover pass, bypass, and failure cases with shell tests. Testing: - python3 -m py_compile scripts/check_codex_agent_review.py - bash tests/test-codex-agent-review-check.sh - bash tests/test-codex-plugin-metadata.sh - bash tests/test-codex-prompt-evals.sh - bash tests/test-codex-prompt-eval-compare.sh - bash scripts/run-deterministic-tests.sh Risk: - narrow - this adds an opt-in deterministic Codex review evidence gate and prompt reference without changing scanner/catalog/CI behavior. Signed-off-by: Vitaly D. <netmin@pm.me>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0954dad106
ℹ️ 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".
|
|
||
|
|
||
| def _determine_risk(contract: dict[str, Any], audit: dict[str, Any], proofpack: dict[str, Any]) -> str | None: | ||
| for source in (proofpack, audit, contract): |
There was a problem hiding this comment.
Use contract risk as source of truth
_determine_risk currently prefers proofpack/audit_summary over contract.json, so a medium/high contract can be downgraded to low by those mutable artifacts and the checker will skip the hard gate (required: false) even when AUTO_OK is claimed without Codex review evidence. Repro: contract riskLevel=high, audit/proofpack riskLevel=low returns exit 0 and no violations. This defeats the stated purpose of enforcing medium/high AUTO_OK review evidence.
Useful? React with 👍 / 👎.
| SCHEMA_VERSION = "1.0" | ||
| REQUIRED_RISKS = {"medium", "high"} | ||
| READY_STATE = "ready" | ||
| APPROVING_VERDICTS = {"APPROVE", "CONDITIONAL"} |
There was a problem hiding this comment.
Exclude CONDITIONAL from approving verdicts
The gate treats CONDITIONAL as an approving verdict, so medium/high AUTO_OK can pass even when the Codex review explicitly reports a conditional outcome (typically unresolved major issues). Because codex_review.verdict_not_approving is the only verdict check, including CONDITIONAL here weakens the gate and allows inconsistent AUTO_OK decisions to be marked as hard-gate passed.
Useful? React with 👍 / 👎.
| if review.get("reviewerType") != "local_agent": | ||
| violations.append("codex_review.reviewer_type_missing") |
There was a problem hiding this comment.
Validate reviewer ID before passing AUTO_OK gate
The skill contract says medium/high AUTO_OK requires a ready reviewer with a non-empty reviewer ID, but this checker never validates any reviewer identifier field in reviews/codex.json. As a result, an anonymous or blank-identity local review still passes the hard gate, so the deterministic check does not enforce a stated acceptance condition.
Useful? React with 👍 / 👎.
| if contract_error and contract_error != "missing": | ||
| violations.append("contract.invalid_json") |
There was a problem hiding this comment.
Fail when contract.json is missing
Missing contract.json is not recorded as a violation (only invalid JSON is), so the gate can return hardGatePassed: true using only audit_summary.json/proofpack.json values. That makes it possible to bypass medium/high enforcement via stale or downgraded risk metadata when the canonical contract source is absent.
Useful? React with 👍 / 👎.
| if codex_review.get("status") == "present": | ||
| return True |
There was a problem hiding this comment.
Require codex identity in proofpack review evidence
_proofpack_has_codex_review returns true as soon as checks.reviews.codex.status is present, without verifying that the referenced content/path is actually the Codex review artifact. If packing mistakenly points checks.reviews.codex to a non-Codex file, the checker still reports proofpackIncludesCodexReview: true and passes medium/high AUTO_OK, so the proofpack evidence requirement can be satisfied by the wrong artifact.
Useful? React with 👍 / 👎.
|
|
||
| decision = _determine_decision(audit, proofpack) | ||
| risk_level = _determine_risk(contract, audit, proofpack) | ||
| required = decision == "AUTO_OK" and risk_level in REQUIRED_RISKS |
There was a problem hiding this comment.
Reject malformed AUTO_OK decision values
The hard-gate condition is an exact string match (decision == "AUTO_OK"), so malformed variants like "AUTO_OK " or casing differences skip enforcement (required: false) even on medium/high risk runs. In that case the checker can exit successfully without any Codex review evidence, so invalid decision tokens should fail closed instead of bypassing the gate.
Useful? React with 👍 / 👎.
Linked intent
Link the Issue or Discussion this PR implements.
For non-trivial changes, open an Issue or Discussion before code review. Direct PRs are intended only for typo/docs fixes, small test-only changes, clearly scoped bug fixes, or maintainer-approved work.
Problem
The Codex prompt now requires an internal local agent-review artifact for non-trivial medium/high-risk work, but that rule was still easy to miss manually. A deterministic check is needed before a Codex run can safely claim
AUTO_OK.Why now
PR #109 made the Codex review artifact contract explicit. This PR adds the smallest deterministic guard for that contract so future runs can verify the evidence exists instead of relying on prompt memory alone.
Existing options checked
The existing Codex prompt eval catches simulated invariant failures, but it does not validate a real contract artifact root after a run. The proofpack validator checks general proofpack shape, but not the Codex-local review evidence requirement.
Alternatives considered
No-code alternative
The prompt text already documents the rule. That helps but does not prevent missing local review evidence from slipping through.
Why code is needed
A small checker can validate the actual
.signum/contracts/<contractId>/artifact root and fail deterministically when medium/highAUTO_OKlacks Codex local review evidence.Summary
scripts/check_codex_agent_review.py.AUTO_OK, validates ready Codex coverage,reviews/codex.json, audit artifact refs, and proofpack evidence.AUTO_OKruns to bypass this gate.AUTO_OK.Type
Mark all that apply.
libScope
In:
Out:
Risk areas
Mark anything touched in this PR.
commands/signum.mdcommands/init.mdagents/*lib/*lib/schemas/*.github/workflows/*Docs impact
README.mdorQUICKSTART.mdupdatedAGENTS.mdupdateddocs/how-it-works.mdordocs/reference.mdupdateddocs/SECURITY.mdupdatedDocs / rationale:
Validation / proof
Commands run:
Observed:
DCO / authorship
git commit -s) and complies withDCO.mdReviewer notes
This PR intentionally does not wire the checker into global CI or the root command runtime. It adds the deterministic checker and Codex prompt call site first, keeping behavior changes bounded.