feat(github-bot): UI-driven model selector with inline directive override#619
feat(github-bot): UI-driven model selector with inline directive override#619donnfelker wants to merge 2 commits into
Conversation
…ride Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds inline directive parsing for GitHub bot comments to override session model and reasoning effort settings. Introduces shared model alias resolution, updates configuration schemas across control-plane and integration packages, integrates directive processing into GitHub event handlers, and extends web UI settings for managing defaults and override toggles. ChangesInline Directive Override Feature
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@packages/github-bot/src/inline-directive.test.ts`:
- Line 1: Move the test file inline-directive.test.ts out of the package source
folder into the package's test directory (i.e., relocate it from the src tree
into test/), update any relative import paths inside the file to reflect the new
location, and ensure the package's test discovery (vitest/glob or package.json
test config) still includes test/*.test.ts so this file is picked up; no code
changes to tests themselves are required beyond fixing imports/paths.
In `@packages/github-bot/src/model-resolution.test.ts`:
- Line 1: The test file model-resolution.test.ts is placed under src/ but should
live in the package test folder; move the file from
packages/github-bot/src/model-resolution.test.ts to
packages/github-bot/test/model-resolution.test.ts and update any import paths if
necessary (ensure the vitest imports: describe, it, expect remain valid and any
relative imports inside model-resolution.test.ts still resolve after
relocation).
In `@packages/github-bot/src/model-resolution.ts`:
- Around line 64-66: The current early return when modelFromDirective is true
always sets reasoningEffort: null and prevents the config fallback; change the
logic so that if modelFromDirective is true you only return immediately when the
directive explicitly provided a reasoning value (use that value for
reasoningEffort), otherwise do not override reasoningEffort and allow the
subsequent config fallback to run. Specifically, adjust the branch around
modelFromDirective to check for the presence/definedness of the directive's
reasoning (instead of unconditionally assigning null), returning { model,
reasoningEffort: <directiveReasoning> } only when that reasoning is present.
In `@packages/shared/src/models.ts`:
- Around line 206-223: The fallback in resolveModelAlias passes the original raw
string to normalizeModelId which does case-sensitive prefix checks, so
mixed-case inputs like "GPT-5.4" bypass normalization; update resolveModelAlias
to call normalizeModelId with the already-lowercased variable (use lower instead
of raw) so normalizeModelId sees canonical lowercase prefixes and correctly
normalizes bare "claude-"/"gpt-" ids.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e99d115-f7b6-4a19-a131-76e0f6118ff1
📒 Files selected for processing (17)
docs/MODELS.mdpackages/control-plane/src/db/integration-settings.tspackages/control-plane/src/routes/integration-settings.tspackages/control-plane/test/integration/integration-settings.test.tspackages/github-bot/src/handlers.tspackages/github-bot/src/inline-directive.test.tspackages/github-bot/src/inline-directive.tspackages/github-bot/src/model-resolution.test.tspackages/github-bot/src/model-resolution.tspackages/github-bot/src/utils/integration-config.tspackages/github-bot/test/handlers.test.tspackages/github-bot/test/integration-config.test.tspackages/linear-bot/src/__tests__/pure-functions.test.tspackages/linear-bot/src/model-resolution.tspackages/shared/src/models.tspackages/shared/src/types/integrations.tspackages/web/src/components/settings/integrations/github-integration-settings.tsx
| if (modelFromDirective) { | ||
| return { model, reasoningEffort: null }; | ||
| } |
There was a problem hiding this comment.
Model-only directives currently null out reasoning unintentionally
On Line 64, modelFromDirective triggers reasoningEffort: null even when no directive reasoning was supplied. That skips the Line 69 config fallback and is broader than the comment on Lines 62-63.
Proposed fix
- if (modelFromDirective) {
+ if (modelFromDirective && input.directiveReasoningEffort) {
return { model, reasoningEffort: null };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (modelFromDirective) { | |
| return { model, reasoningEffort: null }; | |
| } | |
| if (modelFromDirective && input.directiveReasoningEffort) { | |
| return { model, reasoningEffort: null }; | |
| } |
🤖 Prompt for 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.
In `@packages/github-bot/src/model-resolution.ts` around lines 64 - 66, The
current early return when modelFromDirective is true always sets
reasoningEffort: null and prevents the config fallback; change the logic so that
if modelFromDirective is true you only return immediately when the directive
explicitly provided a reasoning value (use that value for reasoningEffort),
otherwise do not override reasoningEffort and allow the subsequent config
fallback to run. Specifically, adjust the branch around modelFromDirective to
check for the presence/definedness of the directive's reasoning (instead of
unconditionally assigning null), returning { model, reasoningEffort:
<directiveReasoning> } only when that reasoning is present.
There was a problem hiding this comment.
Declining this change — the current behavior is intentional and is covered by an explicit test in model-resolution.test.ts ("directive model with no directive reasoning: drops config reasoning entirely"). The design choice: when a user supplies a fresh model via inline directive, we deliberately do not carry the previous config reasoning over, because reasoning is tightly coupled to the model and the override is treated as a clean reset. Adopting the suggested fallback would require updating both the test and the surrounding doc comment to flip the documented contract.
There was a problem hiding this comment.
@donnfelker, understood — thank you for the detailed explanation. The "clean reset" contract makes good sense: since reasoning effort is model-specific, inheriting config reasoning when the user explicitly switches models via directive could silently apply an incompatible or unexpected effort level. The existing test case documents this contract clearly, so no change is needed here.
( ´･‿･｀) ( )
✏️ Learnings added
Learnt from: donnfelker
Repo: ColeMurray/background-agents PR: 619
File: packages/github-bot/src/model-resolution.ts:64-66
Timestamp: 2026-05-14T19:35:18.813Z
Learning: In `packages/github-bot/src/model-resolution.ts`, when a user supplies a model via an inline directive (`modelFromDirective = true`) but provides no directive reasoning effort, the resolver intentionally returns `reasoningEffort: null` and does NOT fall back to `configReasoningEffort`. This is by design — a directive model override is treated as a "clean reset" because reasoning effort is tightly coupled to the model. This contract is covered by the test "directive model with no directive reasoning: drops config reasoning entirely" in `packages/github-bot/src/model-resolution.test.ts`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Move github-bot test files to test/ to match the per-package convention documented in CLAUDE.md, and fix a case-sensitivity bug in resolveModelAlias where the raw (non-lowercased) input was passed to normalizeModelId, causing mixed-case bare ids like "GPT-5.4" to skip prefix normalization. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Summary
@<bot> model: <alias> reasoning: <effort>inline directives in PR comments to override the configured model for a single run.MODEL_OPTIONS,MODEL_ALIASES,resolveModelAlias,normalizeModelId) in@open-inspect/sharedso the GitHub and Linear bots share one source of truth for model ids, aliases, and reasoning-effort compatibility.modelandreasoningEffortselection in the control-plane'sintegration-settingsstore, plus a booleanallowInlineDirectiveOverrideflag.packages/github-bot/src/inline-directive.tsand a near-clone of the Linear bot's resolver aspackages/github-bot/src/model-resolution.ts(kept separate so the two bots' policies can drift).docs/MODELS.md.Screenshots
Summary by CodeRabbit
New Features
Documentation