fix(reviewer): drop the consumer-verb allowlist gate - #42
Conversation
Held-out evaluation showed all 7 fabrication misses died at _has_consumer_verb:
a goal phrased with merge/load/validate/restore/apply/count skipped the entire
check before any path logic ran. An allowlist of English verbs is the wrong
shape for deciding whether a path is an input.
- Removed the gate and _CONSUMER_VERBS entirely.
- Precision is now held by an explicit exemption: a path the goal introduces as
an output/destination, or explicitly authorizes creating/resetting/rotating.
- Split the binding rules: a preposition ('to X', 'into X') must be immediately
followed by the path, while a creation verb may take a short bounded noun
phrase ('create a fresh SQLite database at X'). Without that split, removing
the gate exposed a false positive the gate had been masking.
Measured old vs new. Unseen corpus (26 cases): recall 1/13 both, false
positives 1 -> 0. Seen corpus (22 cases): recall 4/11 -> 6/11, FP 0 both.
Never worse; the honest gain on unseen data is the removed false positive and
the simpler shape, NOT better recall.
Full suite 564 + 14 subtests.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR adjusts the deterministic “fabricated precondition” heuristic in justai/reviewer.py by removing the consumer-verb allowlist gate and replacing it with tighter goal-permission logic for when a goal explicitly authorizes creating/resetting an output path.
Changes:
- Removed the
_has_consumer_verballowlist gate so all goals with concrete file-like paths are checked for fabricated preconditions. - Split “goal permits making this path” logic into (1) tight-binding destination prepositions and (2) bounded-gap creation/reset verbs.
Suppressed comments (1)
justai/reviewer.py:216
- The PR intent is to remove the consumer-verb gate so fabricated-precondition detection also triggers for goals phrased with verbs like merge/load/validate/restore/apply/count. There are existing tests for fabricated-precondition behavior, but none appear to cover these previously-missed verb forms, so a regression could slip in unnoticed. Consider adding at least one test where the goal uses one of these verbs and the plan fabricates an input path (should be rejected).
An earlier version gated the whole check on an allowlist of 'consumer verbs',
which silently skipped every goal phrased with merge/load/validate/restore/
apply/count -- the dominant miss cause in held-out evaluation. The gate is
gone; precision is held by the exemption above plus the direct-object rule.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def _is_goal_output_path(goal_lower: str, path_lower: str) -> bool: | ||
| """The goal introduces this path as an output/destination.""" | ||
| return bool(re.search(_DEST_PREP_RE + re.escape(path_lower), goal_lower)) | ||
|
|
||
|
|
||
| def _is_goal_output_path(goal_lower: str, path_lower: str) -> bool: | ||
| """True if the goal introduces this path as an output/destination.""" | ||
| return bool(re.search(_DEST_CUE_RE + re.escape(path_lower), goal_lower)) | ||
| def _goal_permits_making(goal_lower: str, path_lower: str) -> bool: | ||
| """The goal names this path as an output/destination, or explicitly | ||
| authorizes creating/resetting/rotating it.""" | ||
| if _is_goal_output_path(goal_lower, path_lower): | ||
| return True | ||
| return bool( | ||
| re.search(_MAKE_VERB_RE + r"[^\n]{0,40}?" + re.escape(path_lower), goal_lower) | ||
| ) |
![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: b43be1e788
ℹ️ 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".
| if _is_goal_output_path(goal_lower, path_lower): | ||
| return True | ||
| return bool( | ||
| re.search(_MAKE_VERB_RE + r"[^\n]{0,40}?" + re.escape(path_lower), goal_lower) |
There was a problem hiding this comment.
For a goal such as Create /o/r.json by parsing /d/i.csv, the 40-character wildcard associates create with both paths, so _goal_permits_making incorrectly returns true for the source /d/i.csv. If a task then creates that missing source, _fabricated_preconditions skips it; before this change, the consumer gate ran and the direct destination cue exempted only the output. Stop the match at source clauses such as from/by parsing, or otherwise bind the verb to its actual object.
Useful? React with 👍 / 👎.
| for path in sorted(goal_paths): | ||
| pl = path.lower() | ||
| if _is_goal_output_path(goal_lower, pl): | ||
| if _goal_permits_making(goal_lower, pl): |
There was a problem hiding this comment.
When the goal is Ensure /var/lib/app/state.db exists, the now-unconditional path check treats the database as a pre-existing input because _goal_permits_making does not recognize ensure or set up. A task that creates the database if absent is therefore rejected as fabrication even though it directly satisfies the goal, and the deterministic check overrides an LLM approval. Recognize explicit existence objectives before applying the fabricated-input rule.
Useful? React with 👍 / 👎.
| # a short bounded gap. | ||
| _DEST_PREP_RE = r"(?:\bto|\binto|\bonto|\bas|\boutput|\bsave|\bexport|\bdump|-o)\s+" | ||
| _MAKE_VERB_RE = ( | ||
| r"(?:\bcreat\w*|\bgenerat\w*|\bwrit\w*|\bproduc\w*|\bmak\w*|\bbuild\w*|\bnew\b|" |
There was a problem hiding this comment.
new as a standalone creation authorization
For an input-oriented goal such as Summarize the new report.csv, the adjective new makes _goal_permits_making return true for report.csv, so a task that creates a placeholder input is no longer flagged. Calling an existing input “new” does not authorize the plan to synthesize it; require an actual creation verb or another explicit creation instruction instead.
Useful? React with 👍 / 👎.
|
Closing unmerged — the evidence does not support it. Independent review + my own first-hand check found the change is net negative:
The gate stays for now. Next design (validated on a fresh corpus, not these): invert the polarity — flag on positive evidence of pre-existence ( |
All 7 fabrication misses in the held-out eval died at
_has_consumer_verb— goals phrased with merge/load/validate/restore/apply/count skipped the whole check. Gate removed; precision held by an explicit output/authorized-mutation exemption, with preposition vs creation-verb binding split (a preposition must be immediately followed by the path; a creation verb may take a bounded noun phrase).Old vs new, measured: unseen corpus recall 1/13 both, FP 1 → 0; seen corpus recall 4/11 → 6/11, FP 0 both. Never worse. The honest gain on unseen data is the removed false positive and the simpler shape, not recall.
Full suite 564 + 14.