prompts: precondition substitution + verdict gate + prefer-zero carve-out#3
Conversation
…-out (council: catch nil-violates-downstream-guard bugs)
![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: e546af006b
ℹ️ 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".
| - Prefer ZERO findings over weak ones. If a strict senior reviewer would not ask the author to fix it before merge, drop it. | ||
| - Prefer ZERO findings over weak ones. If a strict senior reviewer would not ask the author to fix it before merge, drop it. **CARVE-OUT:** a precondition substitution you have actually performed (see "Call sites & preconditions") that yields a wrong/missing behavior is a STRONG finding — prefer-zero does NOT suppress it. This bug class looks speculative until you substitute; never rationalize a performed wrong-effect substitution away as "too weak." | ||
| - Precondition findings: report ONLY when a downstream guard would REJECT the new value on a live path, producing a concrete wrong/missing behavior (mis-route, skipped validation, wrong classification, dropped write, blocked submit). "A nil/empty value reaches a guard" with no wrong effect is NOT a finding — do not pad. Scope to predicates/validations that branch on the changed field, 1 layer; do not chase transitive readers. | ||
| - Verdict gate on new sentinel values: you may return PASS on a diff that adds or changes a nil/empty/zero/default value ONLY if, for each named downstream predicate that reads it, your output records the guard expression + file:line + "passes" or "rejects". If you did not open a guard for such a value, you cannot conclude pass — raise it as a finding or list it under residual_risks with blocks=true. |
There was a problem hiding this comment.
When a diff adds a nil/empty/zero/default value and the reviewer has not opened a downstream guard, this new rule allows the model to “raise it as a finding” despite the preceding rule requiring precondition findings only after a live guard rejects the value. In this repo, any returned P0/P1/P2 finding becomes changes_requested via src/core/verdict.ts, so an inspection gap can now become a blocking false positive instead of a needs_human residual risk. Please route unverified guard gaps only to residual_risks with blocks=true, not to findings.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Needlefish PR Review
Verdict: ⛔ changes_requested
Base: ea0fb68 → Head: e546af0
changes requested: the new verdict-gate wording can turn an unverified inspection gap into a blocking false-positive finding.
Findings
P2: Route unverified guard gaps to residual risks
prompts/review.md:13
Why this breaks: The new rule allows an unopened downstream guard to be raised as a finding, even though the preceding rule says precondition findings are allowed only after a live guard rejects the value. Since src/core/verdict.ts maps any P0/P1/P2 finding to changes_requested, an inspection gap can now block a PR as a false positive instead of producing a needs_human residual risk.
Suggested fix: Remove the finding option here and require unverified guard gaps to be listed only under residual_risks with blocks=true.
Validation: Use a synthetic diff that adds a nil/default value without an opened downstream guard; confirm the review JSON has findings=[] and residual_risks[{blocks:true}], producing needs_human rather than changes_requested.
Checked
- Read the repository instructions reference /home/termtek/.codex/RTK.md; no tracked AGENTS.md exists in the repo.
- Reviewed the full diff for prompts/critic.md and prompts/review.md.
- Verified README.md documents P0/P1/P2 findings as changes_requested and blocking residual risks as needs_human.
- Verified src/core/verdict.ts derives changes_requested from any P0/P1/P2 finding before considering residual risks.
- Checked prompt line numbers with nl -ba for changed-line anchoring.
| - Prefer ZERO findings over weak ones. If a strict senior reviewer would not ask the author to fix it before merge, drop it. | ||
| - Prefer ZERO findings over weak ones. If a strict senior reviewer would not ask the author to fix it before merge, drop it. **CARVE-OUT:** a precondition substitution you have actually performed (see "Call sites & preconditions") that yields a wrong/missing behavior is a STRONG finding — prefer-zero does NOT suppress it. This bug class looks speculative until you substitute; never rationalize a performed wrong-effect substitution away as "too weak." | ||
| - Precondition findings: report ONLY when a downstream guard would REJECT the new value on a live path, producing a concrete wrong/missing behavior (mis-route, skipped validation, wrong classification, dropped write, blocked submit). "A nil/empty value reaches a guard" with no wrong effect is NOT a finding — do not pad. Scope to predicates/validations that branch on the changed field, 1 layer; do not chase transitive readers. | ||
| - Verdict gate on new sentinel values: you may return PASS on a diff that adds or changes a nil/empty/zero/default value ONLY if, for each named downstream predicate that reads it, your output records the guard expression + file:line + "passes" or "rejects". If you did not open a guard for such a value, you cannot conclude pass — raise it as a finding or list it under residual_risks with blocks=true. |
There was a problem hiding this comment.
P2 (bug): Route unverified guard gaps to residual risks
The new rule allows an unopened downstream guard to be raised as a finding, even though the preceding rule says precondition findings are allowed only after a live guard rejects the value. Since src/core/verdict.ts maps any P0/P1/P2 finding to changes_requested, an inspection gap can now block a PR as a false positive instead of producing a needs_human residual risk.
Suggested fix: Remove the finding option here and require unverified guard gaps to be listed only under residual_risks with blocks=true.
…a finding — self-review catch
There was a problem hiding this comment.
Needlefish PR Review
Verdict: ✅ pass
Base: ea0fb68 → Head: 9f67f9d
PASS: prompt-only changes tighten precondition review rules and explicitly route unverified guard gaps to blocking residual risk rather than findings.
Findings
No actionable findings. Prefer this over padding weak ones.
Checked
- Read repository policy from /home/termtek/.codex/RTK.md; it requires rtk-prefixed shell commands.
- Reviewed changed docs-surface files prompts/review.md and prompts/critic.md against the supplied diff.
- Verified prompts/review.md line 13 says unopened guard gaps go to residual_risks with blocks=true and never to findings.
- Verified src/core/verdict.ts maps P0/P1/P2 findings to changes_requested, but blocking residual risks to needs_human.
- Verified src/shared/schema.ts accepts checked entries as strings, so guard-verdict ledger text can be recorded without schema changes.
- Verified prompts/critic.md line 7 requires re-opening cited guards and discarding guard-rejection findings with no verified rejection or wrong effect.
Residual Risk
- Did not run an end-to-end Codex review prompt execution; the inspected change is prompt text only and no concrete parser/runtime incompatibility was found.
1 participant
Council-driven improvement to catch cross-file bugs where a new/changed value (nil/empty/default) silently violates a downstream consumer's guard.
Changes (review.md + critic.md):
Validation (local, on Saiens real code):