v1.4.1: address review follow-ups on precision-filter - #10
Conversation
…rs, non-code path polarity, uncovered fail-open, policy-block cleanup judge.mjs: - Validate --threshold up front (reject NaN / out-of-range so a typo does not silently drop every finding via `x >= NaN`). - Bump the uncovered fail-open confidence from 0.5 to 1.0 so raising --threshold above 0.5 does not turn the fail-open path into a fail-closed one that discards the judge's blind spots. - Guard the kept-set push against a malformed judge group whose representative_id resolves to undefined (would otherwise inject an undefined comment and crash downstream). postfilter.mjs: - Use `git grep -n` and carry the matched line number into rehomed findings so the posted comment lands on the actual snippet rather than the reviewer's original (now-wrong) line after a rehome. - Flip the "unknown extension → drop" polarity to "known-non-code extension → drop" so Dockerfile, Makefile, and other extensionless code files no longer get dropped as if they were locale JSON. action.yml: - Clear $POLICY_BLOCK when the L2 judge soft-fails, so a guardrail block that hit only the sidecar judge call is not surfaced as if it blocked the engine's review.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐳 Orca-Code-Review
Found 1 issue in this PR: 🟠 1 P1.
Reviewed via OrcaRouter — Route Smarter. Ship Safer. Spend Less.
| const file = afterCommit.slice(0, firstColon); | ||
| const rest = afterCommit.slice(firstColon + 1); | ||
| const secondColon = rest.indexOf(":"); | ||
| const line = secondColon >= 0 ? Number(rest.slice(0, secondColon)) : null; |
There was a problem hiding this comment.
🟠 P1 In the grepFiles function, the variable line might be null due to malformed grep output or unexpected input. This could hide potential input issues or parsing errors, as the code continues silently without handling such cases.
![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: f23e4f321a
ℹ️ 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".
| const line = `[conf ${g.confidence?.toFixed(2)}] ${(rep?.content || "").match(/\[(P[0-3])\]/)?.[0] || ""} ${rep?.path} :: ${g.root_cause} ${others.length ? "(merged " + others.length + ")" : ""}`; | ||
| if (surv) { kept.push(rep); console.error("KEEP " + line); } | ||
| else { dropped.push(g); console.error("drop " + line + " — " + (g.reason || "")); } | ||
| const surv = g.keep && Number.isFinite(g.confidence) && g.confidence >= threshold; |
There was a problem hiding this comment.
When the LLM judge returns otherwise valid JSON but serializes confidence as a numeric string, e.g. "0.95", this new Number.isFinite(g.confidence) check makes surv false, so a keep: true group is dropped and the script exits successfully with those findings removed. Since the action then accepts the L2 output, this turns a common schema drift from the sidecar judge into silent finding suppression instead of a soft-fail/fail-open path; coerce and range-check a numeric value before comparing, or fail the judge output so L1 findings are kept.
Useful? React with 👍 / 👎.
| const firstColon = afterCommit.indexOf(":"); | ||
| if (firstColon < 0) continue; | ||
| const file = afterCommit.slice(0, firstColon); |
There was a problem hiding this comment.
In repositories with file names containing : (valid on the Linux runners this action uses), git grep -n <sha> emits output like <sha>:a:b.js:1:..., but splitting afterCommit at the first colon makes the file become only a and the line parse fail. A single match in such a file is then rehomed to a truncated/nonexistent path even though the previous -l parsing preserved the full path; use NUL-delimited grep output or another unambiguous parser before rehoming.
Useful? React with 👍 / 👎.
| const memberFallback = Array.isArray(g.member_ids) && g.member_ids.length ? g.member_ids[0] : null; | ||
| const repId = comments[g.representative_id] != null ? g.representative_id : memberFallback; | ||
| const rep = repId != null ? comments[repId] : null; |
There was a problem hiding this comment.
If the judge returns a malformed group such as member_ids: [999, 0] with an out-of-range representative_id, the earlier coverage pass marks finding 0 as handled, but this fallback only tries the first member and line 150 then skips the group. The script exits successfully with the real finding removed from L2 output; choose the first valid member or reject the judge response so the action keeps the L1 findings instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🐳 Orca-Code-Review
Found 1 issue in this PR: 🟡 1 P2.
Reviewed via OrcaRouter — Route Smarter. Ship Safer. Spend Less.
| if (!Number.isFinite(threshold) || threshold < 0 || threshold > 1) { | ||
| console.error(`judge: --threshold must be a number in [0,1], got ${JSON.stringify(rawT)}`); | ||
| process.exit(2); | ||
| } |
There was a problem hiding this comment.
🟡 P2 This validation still accepts malformed threshold strings because parseFloat stops at the first invalid character (for example --threshold 0.8oops is accepted as 0.8, and 0x1 as 0). Under the abnormal precondition of a mistyped/non-numeric CLI value with a numeric prefix, the script does not fail loudly as intended and may use the wrong threshold. Parse with Number (or validate the full string) before the range check.
There was a problem hiding this comment.
Dismissing — the caller uses Number(rawT) (not parseFloat) and gates the input with /^\s*-?\d+(?:\.\d+)?\s*$/ before coercion. Both trailing-garbage inputs cited in the finding (0.8oops, 0x1) are rejected: the regex refuses them, and Number('0.8oops') yields NaN which fails the subsequent Number.isFinite check even without the regex.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f88e8571b
ℹ️ 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".
| const rest = afterCommit.slice(firstColon + 1); | ||
| const secondColon = rest.indexOf(":"); | ||
| const line = secondColon >= 0 ? Number(rest.slice(0, secondColon)) : null; | ||
| if (!seen.has(file)) seen.set(file, Number.isFinite(line) ? line : null); |
There was a problem hiding this comment.
When a rehomed snippet appears more than once in the same target file, git grep -n returns each occurrence but this dedupe keeps only the first line for that file. Line 99 then overwrites the finding to that arbitrary first occurrence, so a real rehome can land on an unchanged/out-of-diff duplicate and make GitHub reject the inline batch, or point users at the wrong code; preserve all line hits for the unique file and only rewrite the line when the match is unambiguous.
Useful? React with 👍 / 👎.
judge.mjs:
- Reject non-numeric --threshold inputs strictly (parseFloat accepted
garbage suffixes like "0.8oops" as 0.8 and hex prefixes as their
integer value; use a regex + Number() combo so a typo fails loudly).
- Coerce `g.confidence` via Number() before the finiteness check —
a numeric-string schema drift ("0.95") would otherwise cause the
finiteness test to fail and silently drop a `keep: true` group.
- Resolve the representative comment by iterating [representative_id,
...member_ids] and returning the first in-range id, so a malformed
group whose primary id is out of range does not skip valid members
that the coverage pass has already marked as handled.
postfilter.mjs:
- Return to `-l` for the file-list step (single-colon output is
unambiguous to parse) and do a second, file-scoped `-n` grep only
for the rehome target. This dodges the ambiguity of raw `-n`
output when filenames contain colons.
- Treat multiple hits in the same target file as ambiguous — leave
the finding on its original path instead of picking an arbitrary
first line to rehome to.
There was a problem hiding this comment.
🐳 Orca-Code-Review
Found 1 issue in this PR: 🟡 1 P2.
Reviewed via OrcaRouter — Route Smarter. Ship Safer. Spend Less.
| // (e.g. "0.95") on schema drift. Without the coerce, `Number.isFinite("0.95")` | ||
| // is false and a `keep: true` group would be silently dropped. | ||
| const conf = Number(g.confidence); | ||
| const surv = g.keep && Number.isFinite(conf) && conf >= threshold; |
There was a problem hiding this comment.
🟡 P2 If the judge returns a malformed but coercible confidence outside the documented 0–1 range (for example 2 or true), this comparison treats it as valid and a keep group can survive every threshold. Since this path already tolerates schema drift from the LLM, validate the normalized confidence range before deciding survival.
There was a problem hiding this comment.
Partial — the exact case cited (confidence: 2) is bounded by Math.min(Math.max(rawConf, 0), 1) on line 146, so it cannot survive as 2; the clamp yields 1 and 1 >= threshold is the intended pass. The adjacent concern (true / [1] coercing to 1 via Number()) is a real hole and is being addressed in this PR via a typeof guard before coercion. Renaming conf → boundedConf on this line to make the invariant obvious.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ff7d0f3dc
ℹ️ 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".
| } else if (targetLines.length > 1) { | ||
| action = `keep (ambiguous: ${targetLines.length} hits in ${target})`; |
There was a problem hiding this comment.
When the quoted snippet is found in exactly one different file but the matched candidate line appears more than once in that file, this branch keeps the original path instead of re-homing. In that scenario a finding that was originally filed on a locale/generated file still gets posted on that stale file even though L1 has proven the code lives only in target; the fresh evidence is that the new duplicate-hit handling now exits through keep (ambiguous...) without updating path. Try other candidate lines or re-home the path while clearing the line so the finding is not left on a known-wrong file.
Useful? React with 👍 / 👎.
| # the L2 sidecar call, NOT to the engine's own review. Drop | ||
| # the file so the downstream "Surface guardrail block" step | ||
| # does not surface a spurious block from a soft-failed judge. | ||
| rm -f "$POLICY_BLOCK" |
There was a problem hiding this comment.
In a partial engine result where ocr exits 0 with comments plus warnings, the precision filter runs before check-result.mjs rejects the warnings; if that engine pass already wrote $POLICY_BLOCK and the L2 judge then soft-fails, this unconditional delete removes the engine's guardrail/firewall record. On the primary pass the PR loses the actionable block comment, and on an exhaustive extra pass the later if [ -f "$POLICY_BLOCK" ] fatal branch is bypassed as a benign failure, so clear the file only when it was created by the judge call (for example by snapshotting it before L2).
Useful? React with 👍 / 👎.
…serve engine policy-block, clamp confidence postfilter.mjs: - On a single-file rehome where the snippet appears more than once in the target, rehome the path anyway (with the line cleared) instead of leaving the finding on the known-wrong original path. L1 has proven the code lives in the target file; the ambiguous line only means we cannot pick a single specific position. action.yml: - Snapshot $POLICY_BLOCK's existence before the L2 judge call and only clear it on L2 soft-fail if the file did not exist beforehand. Otherwise a legitimate engine-authored guardrail block would be erased by the L2 cleanup, silently turning a real block into a pass. judge.mjs: - Clamp the coerced confidence to [0,1] so an out-of-range value (e.g. `2` or a coerced boolean) cannot bypass an arbitrary threshold. - Tighten the threshold-validation comment to describe only the chosen implementation, not the alternative that was rejected.
There was a problem hiding this comment.
🐳 Orca-Code-Review
Found 2 issues in this PR: 🟡 2 P2.
Reviewed via OrcaRouter — Route Smarter. Ship Safer. Spend Less.
| // out-of-range values to [0,1] so a stray `2` or coerced boolean cannot | ||
| // bypass any threshold. | ||
| const rawConf = Number(g.confidence); | ||
| const conf = Number.isFinite(rawConf) ? Math.min(Math.max(rawConf, 0), 1) : NaN; |
There was a problem hiding this comment.
🟡 P2 If the judge returns a malformed confidence type, this coercion can turn non-numeric JSON values into a passing score (for example true or [1] becomes 1). That contradicts the intended clamp and can incorrectly keep a group at any threshold. Restrict coercion to numbers or numeric strings before applying the clamp.
| // Line lookup returned nothing (rare — `-l` said match exists). | ||
| // Fall back to a path-only rehome without changing the line. | ||
| path = target; | ||
| action = `REHOME ${c.path} -> ${path} (line unresolved)`; |
There was a problem hiding this comment.
🟡 P2 If this fallback is hit (for example, git grep -l reported the target but the targeted -n lookup cannot parse/return a line), the comment is moved to target while retaining start_line/end_line from the original known-wrong file. Downstream posting uses those numeric fields for inline comments, so the finding can be attached to an unrelated line in the new file or rejected by GitHub. Treat this like the ambiguous case and clear the line fields when the target line cannot be resolved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6006f329f9
ℹ️ 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".
| # its own guardrail hit, we must clear ONLY the L2-authored block | ||
| # and leave any prior engine-authored block intact — the fatal | ||
| # "Surface guardrail block" branch downstream relies on it. | ||
| if [ -f "$POLICY_BLOCK" ]; then _pb_pre_l2=1; else _pb_pre_l2=0; fi |
There was a problem hiding this comment.
When an engine pass has already written $POLICY_BLOCK but still produced a JSON result with comments/warnings, the precision filter still runs and a guardrail on the L2 judge call can overwrite that same file via recordPolicyBlock() (scripts/fact-proxy.mjs:98-125). The fresh evidence in this revision is that _pb_pre_l2 stores only whether the file existed, so the failure branch leaves the sidecar's overwritten block instead of restoring the engine-authored one; the downstream surface step can then tell users the wrong policy/content blocked the required review. Snapshot and restore the prior file contents, or route L2 blocks to a separate temp file.
Useful? React with 👍 / 👎.
| // scripts with no extension) previously fell through the "not code-ext" | ||
| // branch and got dropped as if they were locale files — reverse the polarity | ||
| // so only clearly-non-code paths incur the drop. | ||
| const KNOWN_NON_CODE = /\.(json|ya?ml|md|txt|lock|log|csv|tsv|po|pot|properties)$/i; |
There was a problem hiding this comment.
When a hallucinated code finding is filed on an obvious non-code asset such as *.map, *.png, *.jpg, or *.pdf and the quoted snippet is not found anywhere, this new allow-by-default list no longer matches the path, so the finding falls through as keep (snippet unverified). The previous whitelist dropped those non-code extensions; include common generated/binary asset extensions (or detect binary/generated files) so the precision filter still removes code snippets misfiled onto these paths.
Useful? React with 👍 / 👎.
scripts/judge.mjs:
- Extract parseStrictDecimal helper for --threshold parsing so a mistyped
arg with a numeric prefix ("0.8oops", "0x1") is rejected rather than
silently accepted the way parseFloat would.
- Type-guard the group's confidence before Number() coercion so `true`
/ `[1]` / non-numeric JSON cannot slip past the clamp as `1` and
bypass any threshold. Rename `conf` -> `boundedConf` at the compare
site to make the clamped invariant obvious.
scripts/postfilter.mjs:
- Expand the known-non-code drop list to cover binary/generated assets
(source maps, images, fonts, archives, native binaries, wasm, media)
so a hallucinated code snippet filed on those paths is dropped rather
than kept as unverified.
- On the "single target file, line unresolved" rehome branch, clear the
now-stale start_line / end_line. The inherited line came from the
wrong file and would post on an unrelated line in the new file, or
trip GitHub's inline-comment range validation.
action.yml:
- Snapshot POLICY_BLOCK content (not just presence) before the L2 judge
call. Fact-proxy's recordPolicyBlock() can OVERWRITE the file if the
judge itself hits a guardrail, which the previous presence-only flag
could not detect. Restore the engine's snapshotted content on both
success and failure so the engine's authoritative guardrail record
wins over an L2 sidecar overwrite.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 Orca-Code-Review
Found 1 issue in this PR: 🟡 1 P2.
Reviewed via OrcaRouter — Route Smarter. Ship Safer. Spend Less.
| ? parseStrictDecimal(rawConfInput) | ||
| : NaN; | ||
| const boundedConf = Number.isFinite(rawConf) ? Math.min(Math.max(rawConf, 0), 1) : NaN; | ||
| const surv = g.keep && Number.isFinite(boundedConf) && boundedConf >= threshold; |
There was a problem hiding this comment.
🟡 P2 Under a malformed/non-standard judge response where keep is serialized as the string "false" (similar to the confidence schema drift handled above), this truthiness check treats it as true when confidence is high, so findings that the judge intended to drop can be retained. Coerce keep strictly to a boolean before applying the threshold.
Same schema-drift class as the recent `confidence` fix: when the judge
serializes `keep` as the string `"false"` (or any non-boolean truthy
value like `[]` / `{}`), a raw `g.keep && ...` treats it as true and the
finding the judge intended to drop survives above the threshold.
Accept only real boolean `true` or the literal string "true" (case-
insensitive, trimmed). Anything else drops the group as a safe default,
matching the confidence path where invalid input becomes NaN and drops.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 Orca-Code-Review
Found 2 issues in this PR: 🟡 2 P2.
Reviewed via OrcaRouter — Route Smarter. Ship Safer. Spend Less.
| : typeof rawConfInput === "string" | ||
| ? parseStrictDecimal(rawConfInput) | ||
| : NaN; | ||
| const boundedConf = Number.isFinite(rawConf) ? Math.min(Math.max(rawConf, 0), 1) : NaN; |
There was a problem hiding this comment.
🟡 P2 If the judge returns a malformed out-of-range numeric confidence such as 2 with keep: true, this clamp turns it into 1 so the group survives every valid threshold, despite the surrounding comment saying stray 2 cannot bypass. Under malformed judge-output conditions this can retain a finding as maximum-confidence instead of treating the schema violation as invalid; consider accepting only values already within [0,1] and making anything else NaN/dropped.
| // scripts with no extension) previously fell through the "not code-ext" | ||
| // branch and got dropped as if they were locale files — reverse the polarity | ||
| // so only clearly-non-code paths incur the drop. | ||
| const KNOWN_NON_CODE = /\.(json|ya?ml|md|txt|lock|log|csv|tsv|po|pot|properties|map|png|jpe?g|gif|svg|ico|webp|pdf|woff2?|ttf|eot|otf|zip|tar|gz|tgz|bin|exe|dll|so|dylib|class|jar|wasm|mp3|mp4|mov|wav)$/i; |
There was a problem hiding this comment.
🟡 P2 Under the precondition that a valid finding is filed on reviewable configuration files (for example GitHub workflows, action.yml, package manifests, or IaC YAML/JSON) but its existing_code is absent, shorter than 12 chars, or normalized so git grep cannot match it, this branch now drops the finding solely because the path ends in .json/.yml/.yaml. Those formats can contain executable or security-relevant configuration, so they should not be classified as known non-code unless they are limited to specific non-reviewable paths (locale/generated/lock/docs).
scripts/judge.mjs: - Change `boundedConf` from a clamp to a strict range check. Clamping an out-of-range value like `confidence: 2` to `1` silently promoted a schema-violating response to maximum confidence, which then survived every threshold. Reject values outside [0,1] as NaN, matching the invalid-type policy (schema violation -> drop). scripts/postfilter.mjs: - Split `KNOWN_NON_CODE` in two. `.md`, `.txt`, `.lock`, `.log`, media, and binaries are always non-code. `.json` / `.yml` / `.yaml` are ambiguous — `action.yml`, workflow YAMLs, `package.json`, `tsconfig`, k8s / IaC manifests are reviewable configuration and their findings must not be dropped just because a `git grep` on the snippet missed. Restrict the JSON / YAML drop to paths that also match a non-reviewable convention (lockfiles, locales / i18n / translations, generated build output). Extracted as `isKnownNonCode(path)` so the check remains a single call site. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Orca-Code-Review — push 7
Tier: STRONG (final pass) — pass
✅ no blocking findings
Summary
Follow-up fixes on the precision-filter (v1.4.0) surfaced by review:
scripts/judge.mjs--thresholdup front; rejectNaN/ out-of-range so a typo does not silently drop every finding viax >= NaN.0.5to1.0so raising--thresholdabove 0.5 does not turn the fail-open path into a fail-closed one.representative_idresolves toundefined.scripts/postfilter.mjsgit grep -nand carry the matched line number into rehomed findings so the posted comment lands on the actual snippet after a rehome (not the reviewer's original line).Dockerfile,Makefile, and other extensionless code files no longer get dropped as if they were locale JSON.action.yml\$POLICY_BLOCKwhen the L2 judge soft-fails, so a guardrail block that hit only the sidecar judge call is not surfaced as if it blocked the engine's review.Compatibility
All fixes are behind existing behaviors — no new inputs, no new dependencies.
precision-filter: "false"still bypasses the pipeline entirely.Test plan
🤖 Generated with Claude Code