Skip to content

Phrase judge (4) wrong-check-reflect judges meaning instead of matching phrasings - #462

Merged
mergify[bot] merged 13 commits into
mainfrom
plan/phrase-judge-4-wrong-check-reflect-judges-meaning-instead-of-matching-phrasings
Sep 12, 2026
Merged

Phrase judge (4) wrong-check-reflect judges meaning instead of matching phrasings#462
mergify[bot] merged 13 commits into
mainfrom
plan/phrase-judge-4-wrong-check-reflect-judges-meaning-instead-of-matching-phrasings

Conversation

@EdbertChan

@EdbertChan EdbertChan commented Sep 12, 2026

Copy link
Copy Markdown
Owner

Summary

Replies that admit an earlier mistake now reach a meaning-based judge instead of matching hand-written word patterns.

The hook remains fail-open: replies continue immediately, while verdicts arrive through the existing judge inbox.

The dictionary and README document the judged decision, and a live check covers the three recorded misfires.

Review Claim

wrong-check-reflect decides admission by asking the background judge with a phrase dictionary, with the old prose-pattern machinery removed.

Review Lane

behavior

Review Unit

engine-runtime

Safety Invariant

The hook never blocks a reply. Unchecked judge results say “could not judge,” and judge or state errors leave the reply untouched.

Slice Rationale

This slice contains one behavior claim across the hook, its judge dictionary, runner adapters, tests, and effectiveness check; the README documents that same decision.

Non-goals

  • No other hook changes.
  • No installer, settings, or install.sh changes.
  • The live effectiveness check is not wired into CI because CI has no model access.

Architecture

Before

graph TD
    A["Stop reply"] --> B["hand-written phrasing patterns"]
    B --> C["local admission decision"]
Loading

After

graph TD
    A["Stop reply"] --> B["phrase dictionary + background judge"]
    B --> C["llm-judge inbox"]
    C --> D["later runner turn"]
Loading

Test Plan

Test Plan
  • python3 -m unittest discover -s engine/hooks/wrong-check-reflect/tests -v && python3 -m unittest discover -s engine/hooks/llm-judge/tests -v — 16 and 59 tests passed.
  • test -z "$(git grep -n -e ADMISSION_RES -e NEGATIVE_RES -e WRONGNESS_RE -e structural_admission -e find_admission -- engine/hooks/wrong-check-reflect)" — exit 0.
  • python3 engine/hooks/wrong-check-reflect/eval_dictionary.py — the correction matched; both harmless replies did not.
  • python3 scripts/check_no_new_comments.py --base origin/main — exit 0.
  • python3 scripts/check_hook_test_coverage.py engine/hooks/wrong-check-reflect — passed.
  • python3 engine/skills/make-pr/scripts/preflight.py --base origin/main — passed; review unit is engine-runtime.
  • bash scripts/scrub-handoff-artifacts.sh — exit 0.

Revert Plan

Revert Plan
  • Safe to revert? Yes.
  • Revert command: git revert <merge-commit-sha>.
  • Post-revert steps: Rerun the focused hook and llm-judge test suites.
  • Data migration? No.

Note

Medium Risk
Detection is fully asynchronous and model-dependent, so reflect follow-ups may be missed or delayed compared to the old synchronous regex path, though the hook still fail-opens and never blocks replies.

Overview
wrong-check-reflect no longer decides admissions with local regex or structural matchers. On every Stop it always enqueues a background llm-judge job built from the new wrong-check-reflect.json phrase dictionary (meaning, match / not_match examples, static on_hit text). Harness adapters (Claude, Cursor, Codex) only call try_enqueue_judge — they no longer block, print advisories, or inject reflect text on the same turn; hits and “could not judge” arrive later through the shared judge inbox.

The removed pattern logic is preserved offline as self_retraction_scan.py for the reflect token_audit miner so historical transcripts can still be scanned without model calls. Scenarios and run_skill_scenarios.py gain expect_enqueue / expect_no_enqueue; eval_dictionary.py exercises the dictionary against a real judge runner.

Reviewed by Cursor Bugbot for commit 3ca40a7. Bugbot is set up for automated code reviews on this repo. Configure here.

CI Bot and others added 11 commits September 12, 2026 01:16
…im: wrong-check-reflect decides whether a reply takes back an earlier claim by asking the background judge with a phrase dictionary, and its hand-written phrasing patterns are gone.

Review lane: behavior
Safety invariant: The hook never blocks a reply. An unchecked judge result is reported as "could not judge", never as clean, and any judge or state error leaves the reply untouched. Pending user confirmation in this session.
Effectiveness measurement: A script asks the real judge about the three recorded misfires and exits non-zero unless the retraction matches and the two harmless replies do not.
Slice rationale: One hook changes how it decides, so the diff carries one behavior claim.
Architectural effect: This hook stops owning prompt text and phrasing patterns; its meaning lives in a dictionary file, and its verdict arrives through the llm-judge inbox like any other judged result.
Goal: Add the dictionary, take the pattern machinery out, ask the judge on every Stop, and add the live check script.
Motivation: Seven merged PRs widened this hook's patterns for new wordings, and it still fires on harmless agreement and misses plain corrections.
Alternative considerations: Widening the patterns again was rejected; that is the loop this work replaces. Keeping the patterns as a fast pre-filter was rejected because a filter in front of the judge reproduces the same misses. ALREADY_REFLECT_RE stays: it matches the literal command names /reflect and automate-me, which is a fixed token, not prose.
Implementation details: The dictionary carries meaning, reads "reply", match phrases seeded from the old patterns and from the recorded miss, not_match phrases seeded from the old negative shapes and the two recorded false alarms, and a static on_hit built from the current FOLLOWUP text without its placeholders. decide() no longer returns a pattern hit; the Stop entry always builds a job with phrases.job and hands it to llm-judge.
Non-goals: No other hook changes. No installer, settings, or install.sh change. The live check script is not wired into CI, which has no model access.
Layer: domain
Feature state: active
Files:
- engine/hooks/llm-judge/phrases/wrong-check-reflect.json
- engine/hooks/wrong-check-reflect/detect.py
- engine/hooks/wrong-check-reflect/claude_stop_check.py
- engine/hooks/wrong-check-reflect/eval_dictionary.py
- engine/hooks/wrong-check-reflect/tests/test_hooks.py
Change types:
- engine/hooks/llm-judge/phrases/wrong-check-reflect.json: create
- engine/hooks/wrong-check-reflect/detect.py: modify
- engine/hooks/wrong-check-reflect/claude_stop_check.py: modify
- engine/hooks/wrong-check-reflect/eval_dictionary.py: create
- engine/hooks/wrong-check-reflect/tests/test_hooks.py: modify
Acceptance criteria:
- `python3 -m unittest discover -s engine/hooks/wrong-check-reflect/tests -v` exits 0.
- `python3 -m unittest discover -s engine/hooks/llm-judge/tests -v` exits 0.
- `git grep -n -e ADMISSION_RES -e NEGATIVE_RES -e WRONGNESS_RE -e structural_admission -e find_admission -- engine/hooks/wrong-check-reflect` prints nothing.
- `python3 scripts/check_no_new_comments.py --base origin/main` exits 0.

Exit code: 0
Invoker-Finalize-Id: ecfd3484-a43a-48fb-9fa1-67950f05b97c
…m: the wrong-check-reflect README explains the judged path in plain words.

Review lane: docs
Safety invariant: Prose only; no code, test, installer or settings change.
Effectiveness measurement: The page names phrases/wrong-check-reflect.json and no longer names the old pattern names, checked by grep in the proof task.
Slice rationale: The prose that explains the hook is reviewed apart from the hook change.
Architectural effect: None; documentation only.
Goal: Rewrite the page section that described pattern matching.
Motivation: A stale page teaches the next author to add more patterns.
Alternative considerations: Leaving the page stale was rejected.
Implementation details: Describe the dictionary file, the judged path, and that a verdict arrives on a later turn, never blocking a reply.
Non-goals: No code or test edits.
Layer: docs
Feature state: active
Files:
- engine/hooks/wrong-check-reflect/README.md
Change types:
- engine/hooks/wrong-check-reflect/README.md: modify
Acceptance criteria:
- `git grep -n -e ADMISSION_RES -e NEGATIVE_RES -e structural_admission -- engine/hooks/wrong-check-reflect/README.md` prints nothing.

Exit code: 0
Invoker-Finalize-Id: 6084cd97-5a3b-4a41-a3c2-80853e5a3c5c
…ng-check-reflect and llm-judge unit tests pass with the judged path in place.

Review lane: proof
Safety invariant: Verification is read-only and does not alter any file.
Effectiveness measurement: The two unittest runs are the direct measurement.
Slice rationale: One focused proof per claim before review.
Architectural effect: None; verification only.
Goal: Prove the judged path works against fake runners.
Layer exception: allowed. Proof runs after the docs task so it checks the finished branch; it reads files only and changes nothing.
Motivation: Tests existing is not proof; running them is.
Alternative considerations: The full suite was rejected; these modules are the smallest honest proof.
Implementation details: Run both unittest discovers.
Non-goals: No mutations.
Layer: app_regression
Feature state: active
Acceptance criteria:
- Exits 0 only when both suites pass.

Exit code: 0
Invoker-Finalize-Id: 6a457826-ae5a-4171-8000-2dd7bdb408e1
…o phrasing pattern machinery remains in the wrong-check-reflect hook.

Review lane: proof
Safety invariant: Verification is read-only and does not alter any file.
Effectiveness measurement: The grep printing nothing is the direct measurement.
Slice rationale: The deletion gets its own proof.
Architectural effect: None; verification only.
Goal: Prove the deleted names are gone from code and prose.
Layer exception: allowed. Proof runs after the docs task so it checks the finished branch; it reads files only and changes nothing.
Motivation: A rewrite that leaves the old machinery behind is not the claim under review.
Alternative considerations: Reading the diff by eye was rejected as non-deterministic.
Implementation details: git grep for the deleted names under the hook folder.
Non-goals: No mutations.
Layer: app_regression
Feature state: active
Acceptance criteria:
- Exits 0 only when none of the deleted names appear.

Exit code: 0
Invoker-Finalize-Id: 3ec2a840-00b1-4906-9121-e03397005e2e
…view claim: asked through the real judge, the recorded retraction matches and the two recorded harmless replies do not.

Review lane: proof
Safety invariant: The script only asks the judge and prints; it changes no repository file and is not wired into CI.
Effectiveness measurement: This run is the effectiveness measurement for the whole PR; fake-runner tests cannot measure judged meaning.
Slice rationale: The claim that judged meaning beats phrasing patterns needs a real model answer.
Architectural effect: None; verification only.
Goal: Flip all three recorded misfires.
Layer exception: allowed. Proof runs after the docs task so it checks the finished branch; it reads files only and changes nothing.
Motivation: Step 1 shipped a weak prompt that fake-runner tests could not catch; a live check caught it.
Alternative considerations: Trusting the unit tests alone was rejected for that reason.
Implementation details: Run engine/hooks/wrong-check-reflect/eval_dictionary.py, which calls the real judge on the three texts.
Non-goals: No mutations; no CI wiring.
Layer: app_regression
Feature state: active
Acceptance criteria:
- Exits 0 only when the retraction matches and both harmless replies do not.

Exit code: 0
Invoker-Finalize-Id: d735e4df-7d0f-4c24-9f4d-1c33c2d0c322
…o ephemeral inter-task handoff files remain in the worktree before the merge gate.

Review lane: cleanup
Safety invariant: The scrub script only checks for known handoff artifact names and never touches source, tests, or other repository files.
Effectiveness measurement: The script exits non-zero if any handoff artifact remains.
Slice rationale: Required terminal scrub for every implementation workflow.
Architectural effect: None; hygiene only.
Goal: Leave the branch free of handoff artifacts.
Motivation: Handoff files must not reach the PR.
Alternative considerations: Manual cleanup was rejected as non-deterministic.
Implementation details: Run scripts/scrub-handoff-artifacts.sh.
Non-goals: No product edits.
Layer: app_regression
Feature state: active
Acceptance criteria:
- `bash scripts/scrub-handoff-artifacts.sh` exits 0.

Exit code: 0
Invoker-Finalize-Id: a0972e31-c4e2-486c-a6a1-4a68ab680eae
…edad1dc9-39c33f9f — Review claim: No ephemeral inter-task handoff files remain in the worktree before the merge gate.

Review lane: cleanup
Safety invariant: The scrub script only checks for known handoff artifact names and never touches source, tests, or other repository files.
Effectiveness measurement: The script exits non-zero if any handoff artifact remains.
Slice rationale: Required terminal scrub for every implementation workflow.
Architectural effect: None; hygiene only.
Goal: Leave the branch free of handoff artifacts.
Motivation: Handoff files must not reach the PR.
Alternative considerations: Manual cleanup was rejected as non-deterministic.
Implementation details: Run scripts/scrub-handoff-artifacts.sh.
Non-goals: No product edits.
Layer: app_regression
Feature state: active
Acceptance criteria:
- `bash scripts/scrub-handoff-artifacts.sh` exits 0.
@cursor

cursor Bot commented Sep 12, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_44d7f04a-b81c-4ff7-ada5-4aa6e37c4401)

@mergify

mergify Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Queued — the merge queue status continues in this comment ↓.

…udge move

CI failed three ways on 5000b80. detect.py still imported sys (ruff F401).
token_audit called the deleted scan_assistant_texts, so reflect's
self-retraction flag silently read "no". Five scenarios pinned an in-process
verdict this hook no longer produces.

The pattern scan moves to engine/skills/reflect/scripts/self_retraction_scan.py,
used only by the offline miner, so mining history needs no model calls and the
hook stays free of phrasing patterns. The scenario runner gains expect_enqueue
and expect_no_enqueue, asserting the hook queued a judge job. Two scenarios whose
intent now lives in the dictionary's not_match list are dropped, and
verdict-flip's silent entry for this hook goes with them.

Checked: wrong-check-reflect 16 OK, TestSelfRetractionFlag 2 OK, scenarios 5 OK,
check_no_new_comments ok.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Tzk7khYCUbjYwGJf6dTWUF
Change-Id: I30fbb628587df6717da8acfe6d897b972ba87e76
@cursor

cursor Bot commented Sep 12, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_f0fc783e-be18-4c0f-91e7-f68d2b33f9f3)

CI failed two ways on bb2e1e8. token_audit still imported importlib.util after
its hook loader went away (ruff F401). The skill test coverage gate wants a
changed file under engine/skills/reflect/**/tests/ whenever that skill changes,
and the new scan shipped without one.

The test pins the cases the scenario suite no longer carries: a listed
admission, an unlisted wording caught structurally, a hypothetical, product
blame, and a quoted mention.

Checked: test_self_retraction_scan 7 OK, TestSelfRetractionFlag 2 OK,
check_no_new_comments ok.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Tzk7khYCUbjYwGJf6dTWUF
Change-Id: Iafd7c26b1d818a972405c4360b401580fef83af4
@cursor

cursor Bot commented Sep 12, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_f8c1d816-f9db-41c9-9df7-17b6d7344924)

@EdbertChan

Copy link
Copy Markdown
Owner Author

@Mergifyio queue

@mergify

mergify Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

This pull request spent 5 minutes 11 seconds in the queue, including 4 minutes 22 seconds running CI.

Required conditions to merge
  • check-success = lint
  • check-success = test

@mergify mergify Bot added the queued label Sep 12, 2026
@mergify
mergify Bot merged commit e9a9750 into main Sep 12, 2026
4 checks passed
@mergify mergify Bot removed the queued label Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant