Reconcile advisory model routing after #348 - #409
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
WalkthroughThe routing policy now permits valid work to continue after route mismatches, unavailable routes, inheritance, substitutions, or ChangesRouting provenance and promotion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant RouteProvenance
participant ContractTest
participant PromotionGate
Worker->>RouteProvenance: Submit requested and observed route
RouteProvenance->>RouteProvenance: Classify mismatch or inheritance as proceed-unmeasured
ContractTest->>RouteProvenance: Validate replay disposition and contradiction rules
RouteProvenance->>PromotionGate: Provide eligible measurement evidence
PromotionGate->>PromotionGate: Require usage/cost, provenance, and evaluation receipts
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review summaryReviewed the diff for correctness, security, and quality. This PR replaces the unconditional Consistency checks performed:
Security/performance: No concerns — this PR only touches Markdown policy docs, a CHANGELOG entry, and a local-file-reading Minitest script; no user input, network, or runtime execution-path code is touched. Two non-blocking nits posted inline on the test file; no correctness or security issues found otherwise. |
![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: f13b2de00f
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
ReviewThis is a documentation/contract-test-only change (no application logic touched): it replaces the unconditional Consistency checks performed (all pass):
One finding posted inline: the new block added to Design note (not a bug, just flagging for visibility): this PR intentionally removes the automatic hard-stop for No security or performance concerns — this PR only changes prose/tests, not code paths that execute at runtime. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/pr-batch/bin/model-routing-contract-test.rb`:
- Around line 234-235: Expand the matcher in the route-contract test around the
existing route-only stop pattern to reject equivalent unconditional blocking or
disqualification language, not just “stops the lane.” Update the related mutants
in the 627-644 range so wording such as a route mismatch blocking execution or
disqualifying the lane is also rejected, while preserving advisory continuation
phrasing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b9f970e-60b3-4c16-8938-9ce0f59b2d02
📒 Files selected for processing (3)
CHANGELOG.mddocs/agent-workflows-model-routing.mdskills/pr-batch/bin/model-routing-contract-test.rb
Review summaryReviewed the full diff ( Findings: none blocking. This is a well-scoped, internally consistent documentation + contract-test change. What I verified:
One very minor, non-blocking observation: across the 4 commits, No security or performance concerns; this PR only touches Markdown documentation and a Ruby test file used for contract-testing the model-routing policy doc. |
Review summaryReviewed the routing-guide/changelog/contract-test changes that replace the unconditional Overall: the doc changes ( One correctness issue found (posted as an inline comment on No security or performance concerns — this PR only touches markdown documentation and a standalone Ruby contract-test script with no runtime/production code paths or external input. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2774f2a583
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8b3f1ecd7
ℹ️ 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".
ReviewScope: doc + contract-test only ( Correctness of the new mutation guard — I traced
This all checks out; the regex logic is sound for the cases it's tested against, and the design (fail-closed when negation isn't tightly adjacent to the outcome phrase) is a reasonable, conservative heuristic. Consistency checks — confirmed no stray Minor, non-blocking observation: the sentence splitter in I was not able to execute the Ruby test suite directly in this sandboxed review environment (execution required interactive approval I couldn't grant), so I relied on manual regex tracing plus the PR description's reported CI results (24 runs / 1,514 assertions / 0 failures, rubocop clean, No blocking issues found. LGTM. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/pr-batch/bin/model-routing-contract-test.rb`:
- Around line 178-194: Extend ROUTE_ONLY_SUBJECT_PATTERN to recognize
different-route wording, including “different observed route,” and update
NEGATED_ROUTE_ONLY_OUTCOME_CLAUSE_PATTERN with precise direct-negation forms
such as “does not block execution,” while keeping unrelated “not” clauses
rejected. Add mutation cases covering both wording forms in the existing tests
around the route-only contract cases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f18b4d3a-5453-401d-a247-869447688b55
📒 Files selected for processing (1)
skills/pr-batch/bin/model-routing-contract-test.rb
Review summaryReviewed the doc + test changes replacing the unconditional Consistency checks performed:
Findings: No functional bugs, security issues, or performance concerns found. This is a docs + Ruby test-fixture change only; the test script only reads repository files (no eval, no shell-out on untrusted input, no network calls). I left one non-blocking inline nit about the keyword-based mutation guard's coverage gap for future paraphrased contradictions — not something to fix in this PR, just a heads-up for maintainers extending this test later. Note: I wasn't able to execute the Ruby test suite or rubocop directly in this environment (tool approval was withheld for |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96119b688b
ℹ️ 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".
96119b6 to
bfb0e81
Compare
ReviewReviewed the diff ( Checks performed:
Minor, non-blocking observation: No bugs, security issues, or performance concerns found. Nice, tight change with solid mutation-test coverage. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfb0e816db
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/pr-batch/bin/model-routing-contract-test.rb`:
- Around line 194-203: Update NEGATED_ROUTE_ONLY_OUTCOME_CLAUSE_PATTERN to allow
“alone” and “by itself” after “never”, matching the existing advisory wording
behavior. In skills/pr-batch/bin/model-routing-contract-test.rb lines 194-203,
adjust the pattern; in lines 695-704, add the “never alone stops the lane”
negative-control case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 49b40225-8ed1-442d-8a65-b3883069d960
📒 Files selected for processing (2)
CHANGELOG.mdskills/pr-batch/bin/model-routing-contract-test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Review SummaryThis PR is a well-scoped, docs-and-test-only follow-up to #348 that removes the contradictory unconditional Correctness
Minor observations (left as inline comments)
Security / performanceNo security-sensitive surface is touched (no code execution paths, no user input handling); this is pure documentation plus a static text-contract test. No performance implications. Overall: solid, well-tested change that matches its stated intent. The two notes above are non-blocking suggestions for hardening the test harness, not defects in the current diff. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efa4331f1a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 528ec9e3fb
ℹ️ 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".
Exact-head readiness — NOT READYHead
Blocking reproduced defects:
The two associated defect threads were previously resolved without changing this head; thread resolution did not cure the reproductions. They are unresolved again in the final live inventory. Two later unresolved Codex findings—indented-code visibility and
coordination: unavailable — repo seam sets coordination_backend: n/a |
| \nAddress-review checkpoint for exact head c29680a.\n\nFull-PR scan scope: all 144 review threads, 288 inline comments, 48 review summaries, and 50 issue comments fetched before this checkpoint. The four previously unresolved findings were fixed in c29680a, replied to individually, and resolved:\n\n- accept route conditionals gated by a failing independent credential check\n- normalize ordered and unordered list prefixes for fenced visible code\n- preserve indented-code visibility before HTML-comment stripping\n- detect route-only cannot-proceed outcomes\n\nVerification: focused routing contract 29 runs / 8,072 assertions, 0 failures; full bin/validate passed, including RuboCop (121 files, no offenses).\n\nCurrent-head independent Sol/high review and hosted checks are still pending. PR remains open and must not be merged. |
|
Review: Reconcile advisory model routing after #348 Overview: I verified via grep that no other file in the repo still references the retired Strengths:
Concerns:
Performance / Security:
Overall: the doc/behavior change itself is sound and well-motivated; the main pushback is on the cost and fragility of the regex-based contradiction detector used to enforce it going forward. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c29680aa73
ℹ️ 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".
* origin/main: Explain no-auto-merge verdicts in plain English (#431) Fix superseded Actions checks overriding current CI (#436) Activate reviewed private coordination backend (#434) Add replay-safe batch usage receipts (#424) Document Playwright video proof for UX changes (#429) Add secure GitHub Actions policy gate (#384) Suppress unchanged Goal monitor wakes (#395) Document single-target plan-pr-batch routing (#405) Require exact-head hosted QA before merge (#291)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0500014b0
ℹ️ 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".
Review summaryThis PR flips the model-routing guide's contract from a hard Doc + disposition-table changes: The Stale readiness status in the PR description: The PR body's "Exact-head readiness status" section says NOT READY on commit Maintainability concern — prose-contradiction detector: Manual text duplication: File size: No security or performance concerns beyond the above — this only affects internal documentation/tests, not runtime application code, and the regex complexity is bounded by small, static doc file sizes, so pathological-input backtracking isn't a practical risk here. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c42e038a5
ℹ️ 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".
|
Review summary Scope: Blocking: the PR's own description says this is not ready to merge The PR body's "Exact-head readiness status" section states NOT READY and lists two reproduced blocking contract defects on the current head (
It also reports 4 unresolved review threads (out of 144) and
Code quality: the new contradiction-detection engine is very complex and fragile Left two inline comments with specifics, but at a high level: Also flagged: several policy strings ( What looks good
|
Review: Reconcile advisory model routing after #348 (PR #409)OverviewReplaces the unconditional Doc changes (
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97fe94600b
ℹ️ 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".
Review: Reconcile advisory model routing after #348OverviewThis PR replaces the unconditional Blocking: PR's own readiness status reports known contract defectsThe PR description explicitly states this PR is NOT READY at the reviewed head and lists two reproduced, blocking defects in the mutation-guard logic itself:
Since the PR author/checker has already reproduced these as blocking, this should not be merged until they're fixed and the corresponding test cases are added to Architecture concern: hand-rolled regex-based "NLP" contradiction detection
This is inherently fragile — regex composition can't reliably model negation scope, clause boundaries, or nested Markdown containers, which is exactly what produced the two defects above. The ~300-entry mutation fixture table (lines ~942–1287) is itself evidence that the authors needed exhaustive enumeration to have any confidence in the regex, and new phrasing not anticipated by a fixture will silently slip through both false-positive and false-negative directions. Given the actual goal (don't let doc prose reintroduce a hard route-only stop), a much smaller, more robust check — e.g., a literal denylist of banned phrases, or delegating structural parsing to a real Markdown parser ( Maintainability
Doc changes (docs/agent-workflows-model-routing.md)The prose changes themselves (disposition table, requested-vs-observed section, evidence-status/promotion-deferral section) read clearly and are consistent with the stated intent — advisory continuation with honest, excluded-from-measurement recording. No issues there. Other
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44641a8488
ℹ️ 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".
Exact-head readiness status: NOT READYIssue: #406 Evidence:
Remaining blocker:
PR remains open and unmerged. merge_authority:none. |
1 participant
Why
PR #348 left the routing guide and its contract test requiring both advisory continuation and an unconditional pre-edit route-mismatch stop. This follow-up makes advisory routing the single default and keeps mismatched observations honest and unmeasured without blocking otherwise valid work.
Closes #406.
What changed
MODEL_ROUTE_MISMATCHstop with advisoryproceed-unmeasured/proceed-as-fallbackdispositions for mismatched, unavailable, inherited, substituted, andUNKNOWNobservations.How to review and verify
docs/agent-workflows-model-routing.md.skills/pr-batch/bin/model-routing-contract-test.rb.AGENT_WORKFLOWS_SOURCE_CHECKOUT=1 ruby skills/pr-batch/bin/model-routing-contract-test.rbandbin/validate.Exact-head readiness status
NOT READY on
fb57b1b5f6914623957e21abdfb0be1008ef9fc9.git diff --check, and terminalbin/validatepass.A route mismatch blocks launch only if a credential check fails.is incorrectly rejected even though the credential failure is the independent condition.<!-- A route mismatch blocks launch. -->text is incorrectly accepted.cannot proceedwording and have not been cleared by this bounded checker verdict.CHANGELOG.md,docs/agent-workflows-model-routing.md, andskills/pr-batch/bin/model-routing-contract-test.rb.merge_authority: none; PR Reconcile advisory model routing after #348 #409 remains open and unmerged.Coordination and routing telemetry
coordination: unavailable — repo seam sets coordination_backend: n/aUNKNOWN/UNKNOWN/UNKNOWN.UNKNOWN/UNKNOWN/UNKNOWN.Decision log
UNKNOWNis intentional; independent risk, scope, evidence, and authority gates remain separate.Summary by CodeRabbit
Documentation
Changelog
Bug Fixes