feat(plugin): dynamic per-model system prompt tuning - #1
Conversation
- Fix .husky/pre-commit fail-fast (set -e via bunx, chain aborts) - Fix dead rules: settled-reading now tier-disjoint from reasoning-aim, no-filler-verification moved to verification concern (separate slot) - Fix spine warnings: Maeda comment for axis-only families, Hoare word-boundary tier tokens and minimax gate comment, Carmack extraction of matchesFamily and haiku fallback - Ground latest: bump typescript ^5 -> ^7 (7.0.2, GA 2026-07-08) - Tests: add qwen/minimax worked rows, real-id fixture, settings precedence and before_agent_start hook coverage (63 tests)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe plugin now supports injectable configuration roots. Model-family resolution uses shared matching and axis construction. Rule concerns and selection tests were expanded. Hook and settings behavior gained isolated coverage. The pre-commit hook now stops on failures, and TypeScript targets version 7. ChangesModel and prompt behavior
Repository tooling
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
|
Simplifying code... This may take up to 20 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
test/hook.test.ts-159-160 (1)
159-160: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail when the hook is not registered.
These tests return successfully when
loadPlugin()returns no handler. A registration regression then makes the behavior assertions meaningless.Assert that
handlerexists before each guard.Also applies to: 174-175
🤖 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 `@test/hook.test.ts` around lines 159 - 160, Update the tests around loadPlugin in test/hook.test.ts so they assert that the returned handler exists instead of silently returning when it is absent. Apply this to both guarded cases near lines 159-160 and 174-175, preserving the subsequent behavior assertions.test/hook.test.ts-201-205 (1)
201-205: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove the first temporary home before creating the second.
The second
freshHome()call overwritestmpHome.afterEachremoves only the second directory. The firstmt-hook-*directory remains on disk.Clean the existing
tmpHomeinfreshHome()before assigning a new path.🤖 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 `@test/hook.test.ts` around lines 201 - 205, Update freshHome() to remove the currently assigned tmpHome directory before generating and assigning a new temporary home path, ensuring repeated calls clean up the prior mt-hook-* directory while preserving the existing setup behavior.
🤖 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 `@test/hook.test.ts`:
- Around line 17-20: Remove the top-level partial node:os mock from
test/hook.test.ts (lines 17-20) and test/settings.test.ts (lines 14-17). Update
the affected tests to inject the home-directory dependency directly instead,
preserving their existing homedir behavior without replacing the shared node:os
module.
---
Other comments:
In `@test/hook.test.ts`:
- Around line 159-160: Update the tests around loadPlugin in test/hook.test.ts
so they assert that the returned handler exists instead of silently returning
when it is absent. Apply this to both guarded cases near lines 159-160 and
174-175, preserving the subsequent behavior assertions.
- Around line 201-205: Update freshHome() to remove the currently assigned
tmpHome directory before generating and assigning a new temporary home path,
ensuring repeated calls clean up the prior mt-hook-* directory while preserving
the existing setup behavior.
🪄 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: QUIET
Plan: Pro Plus
Run ID: 03733ac5-4fa8-4a70-91fb-14516cc37bcd
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.husky/pre-commitpackage.jsonsrc/index.tssrc/model-axis.tssrc/rules.tstest/hook.test.tstest/model-axis.test.tstest/render.test.tstest/rules.test.tstest/settings.test.ts
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.45.0)
test/hook.test.ts
[error] 77-80: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const k of ENV_KEYS) {
savedEnv[k] = process.env[k];
delete process.env[k];
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').
(prototype-pollution-recursive-merge-typescript)
[error] 84-87: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const k of ENV_KEYS) {
if (savedEnv[k] === undefined) delete process.env[k];
else process.env[k] = savedEnv[k];
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').
(prototype-pollution-recursive-merge-typescript)
test/settings.test.ts
[error] 47-50: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const k of ENV_KEYS) {
savedEnv[k] = process.env[k];
delete process.env[k];
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').
(prototype-pollution-recursive-merge-typescript)
[error] 54-57: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const k of ENV_KEYS) {
if (savedEnv[k] === undefined) delete process.env[k];
else process.env[k] = savedEnv[k];
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').
(prototype-pollution-recursive-merge-typescript)
src/model-axis.ts
[warning] 124-124: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((^|[-._/])${t}($|[-._/]))
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 127-127: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((^|[-._/])${t}($|[-._/]))
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🪛 OpenGrep (1.26.0)
src/model-axis.ts
[ERROR] 216-216: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔍 Remote MCP Context7, Grep, Valyu
Additional review context
- TypeScript 7.0 is stable and published via the
typescriptnpm package. However, it does not ship the programmatic compiler API; the official guidance recommends@typescript/typescript6for tools such astypescript-eslintthat import TypeScript directly. Verify the lint/test toolchain does not depend on that API. - TypeScript 7 introduces configuration changes and hard errors, including defaults for
strict,module,rootDir, andtypes, plus removal of legacy options such asbaseUrlandmoduleResolution: node. Confirm the repository’stsconfigexplicitly remains compatible. - TypeScript 6’s package still exposes the JavaScript compiler API, providing a concrete compatibility distinction between the old and new major versions.
- Repository searches did not locate this exact project or its model-tuning implementation, so model-regex and hook-behavior claims could not be independently validated.
🔇 Additional comments (9)
src/model-axis.ts (1)
9-12: LGTM!Also applies to: 78-80, 118-136, 192-229, 249-249, 271-278, 293-293, 310-318, 328-328, 348-348, 359-360
test/model-axis.test.ts (1)
174-201: LGTM!test/render.test.ts (2)
2-2: LGTM!Also applies to: 19-27
120-120: 🎯 Functional CorrectnessKeep the single
fixturesdeclaration.
test/render.test.tscontains only one declaration in this scope, so this compile-error claim is incorrect.> Likely an incorrect or invalid review comment.src/rules.ts (1)
3-9: LGTM!Also applies to: 20-41, 88-88
src/index.ts (1)
28-28: LGTM!test/rules.test.ts (1)
3-3: LGTM!Also applies to: 49-80
.husky/pre-commit (1)
1-1: LGTM!package.json (1)
52-52: 🩺 Stability & AvailabilityDo not block TypeScript 7 on compiler API compatibility.
The project invokes TypeScript only through
tsc.lint-stagedandbun testdo not consume the TypeScript compiler API. The lockfile resolvestypescript@7.0.2.> Likely an incorrect or invalid review comment.
|
@coderabbitai ultrareview |
|
|
The two new test files replaced node:os in the shared module registry with a partial mock (homedir + tmpdir only) and never restored it. `bun test` runs one registry for the whole suite, so any later file reaching for os.platform(), os.EOL, or os.cpus() would have received undefined -- a landmine whose blast radius depended on file ordering. readTuningSettings and the plugin entry now take the config root as a parameter, so tests point at a temp directory instead of mutating a global. omp's loader still calls the factory with `pi` alone. Two vacuous tests fixed while here: `if (!handler) return;` let a registration regression pass silently, so loadHandler throws instead. freshHome now removes its predecessor -- the second call in the weight-override test orphaned the first mt-hook-* directory past afterEach.
|
✅ Created PR with simplified code: #2 |
📝 CodeRabbit Chat: Simplify code structure
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/model-axis.ts (1)
84-107: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve semantic minor versions.
versionFromDigits("2", "10")returns2.1. It then compares below2.9and renders as2.1, although version2.10is newer than2.9.Use a structured major/minor representation, or a comparison-safe encoding. Update
FRONTIER_VERSION,GENERATION_STEP,deriveWeight, and profile rendering with the same representation.🤖 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 `@src/model-axis.ts` around lines 84 - 107, Replace the decimal encoding in versionFromDigits with a comparison-safe major/minor representation that preserves values such as 2.10 as newer than 2.9. Update FRONTIER_VERSION, GENERATION_STEP, deriveWeight, and profile rendering to consume and display that same representation consistently, preserving correct ordering and semantic minor-version output.
🟡 Other comments (1)
test/hook.test.ts-67-70 (1)
67-70: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest the injected configuration-root path.
loadPluginalways callsdynamicSystemPromptPlugin(pi). The hook suite cannot verify thatconfigHomereachesreadTuningSettings.Accept an optional
configHomeparameter inloadPlugin. Pass it todynamicSystemPromptPlugin. Add a hook test that writes a lockfile under the temporary root and callsloadPlugin(home).Based on learnings, keep configuration-root injection explicit and test it without mocking
node:os.Proposed fix
-async function loadPlugin(): Promise<CapturedHandler | undefined> { +async function loadPlugin( + configHome?: string, +): Promise<CapturedHandler | undefined> { const { pi, getHandler } = createStubPi(); - await dynamicSystemPromptPlugin(pi); + await dynamicSystemPromptPlugin(pi, configHome); return getHandler(); }🤖 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 `@test/hook.test.ts` around lines 67 - 70, Update loadPlugin to accept an optional configHome argument and forward it to dynamicSystemPromptPlugin, keeping configuration-root injection explicit. Add a hook test that creates a lockfile beneath a temporary root and invokes loadPlugin with that root to verify the path reaches readTuningSettings; do not mock node:os.Source: Learnings
🤖 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.
Outside diff comments:
In `@src/model-axis.ts`:
- Around line 84-107: Replace the decimal encoding in versionFromDigits with a
comparison-safe major/minor representation that preserves values such as 2.10 as
newer than 2.9. Update FRONTIER_VERSION, GENERATION_STEP, deriveWeight, and
profile rendering to consume and display that same representation consistently,
preserving correct ordering and semantic minor-version output.
---
Other comments:
In `@test/hook.test.ts`:
- Around line 67-70: Update loadPlugin to accept an optional configHome argument
and forward it to dynamicSystemPromptPlugin, keeping configuration-root
injection explicit. Add a hook test that creates a lockfile beneath a temporary
root and invokes loadPlugin with that root to verify the path reaches
readTuningSettings; do not mock node:os.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: ea609394-e006-4347-83a3-077e0b653dd7
📒 Files selected for processing (6)
src/index.tssrc/model-axis.tssrc/rules.tssrc/settings.tstest/hook.test.tstest/settings.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Run typechecking withbun x tsc --noEmit -p tsconfig.json; this is especially important forpi-catalogimport paths.
Verify the three-axis resolver against the live catalog with abun -esweep covering 2646 IDs, with no null version among classified models.
Do not add runtime dependencies; every import must be a Node builtin or anomppackage resolved from the host.
Preserve append-only behavior: never replaceomp's system prompt; only append## Model tuningviabefore_agent_start.
Before appending, checkevent.systemPrompt.some(entry => entry.includes(TUNING_HEADER))so tuning content does not accumulate across turns.
Resolve model identity through@oh-my-pi/pi-catalog/identityusingbareModelId, the provider parsers, and family predicates; never re-implement parsers.
Normalize model IDs by lowercasingbareModelId(id)and stripping:thinking,:free,-cheaper,@default,[1m],-high, and equivalent suffixes before classification.
DeriveweightfromFRONTIER_VERSIONandGENERATION_STEP, apply thesmalltier penalty, and ensureunknownmodels are assignedheavy.
Keep one rule perconcern; sort rules bypriorityand thenid, and trim from the end toCHAR_BUDGET[weight].
Never include the model name, version, or profile string in the rendered## Model tuningblock.
Read settings from~/.omp/plugins/omp-plugins.lock.jsonunderomp-plugin-dynamic-system-prompt, falling back toOMP_MODEL_TUNING*environment variables and manifest defaults; never throw when the lockfile is missing.
Files:
src/settings.tssrc/index.tstest/hook.test.tssrc/model-axis.tstest/settings.test.tssrc/rules.ts
**/test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run unit tests with
bun test ./testand ensure all four test files pass.
Files:
test/hook.test.tstest/settings.test.ts
🧠 Learnings (1)
📚 Learning: 2026-08-07T15:53:07.838Z
Learnt from: metaphorics
Repo: metaphorics/omp-plugin-dynamic-system-prompt PR: 1
File: test/hook.test.ts:0-0
Timestamp: 2026-08-07T15:53:07.838Z
Learning: In this Bun TypeScript plugin, keep configuration-root injection explicit: `readTuningSettings(home = homedir())` should accept an injectable config-root path, and `dynamicSystemPromptPlugin(pi, configHome?)` should pass it through while production continues calling `dynamicSystemPromptPlugin(pi)` via the unchanged `ExtensionFactory` interface. In `test/hook.test.ts` and `test/settings.test.ts`, avoid partial `mock.module("node:os", ...)` replacements because Bun test files may share a module registry; use injected paths instead.
Applied to files:
test/hook.test.tstest/settings.test.ts
🪛 ast-grep (0.45.0)
src/model-axis.ts
[warning] 133-133: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((^|[-._/])${t}($|[-._/]))
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🔍 Remote MCP Grep, Sequential Thinking, Valyu
Additional review context
-
TypeScript 7 is available as the standard
typescriptnpm package and uses the usualtsccommand. However, it does not provide the prior programmatic compiler API; tools such astypescript-eslintmay need the TypeScript 6 compatibility package until the TypeScript 7.1 API is available. Verify all lint, test, bundler, and code-generation dependencies before accepting the upgrade. -
TypeScript 7 adopts stricter defaults, including
strict: true,module: "esnext",rootDir: "./", andtypes: []. It also turns several options into hard errors, includingbaseUrl,moduleResolution: "node"/"node10"/"classic", andtarget: "es5". The repository’stsconfigand inherited configs should be checked explicitly. -
TypeScript 7’s parallel checker defaults can increase memory usage; CI behavior should be validated rather than relying only on the reported test count.
-
A GitHub search found unrelated projects already declaring
"typescript": "^7", confirming the version range is in active use, but it did not identify this repository or validate this PR’s toolchain compatibility. -
Sequential review prioritization identified the TypeScript upgrade and its surrounding configuration/toolchain as the highest-risk area.
🔇 Additional comments (6)
src/model-axis.ts (2)
3-12: 🎯 Functional CorrectnessValidate the TypeScript 7 and catalog contract.
Run
bun x tsc --noEmit -p tsconfig.json. Runbun test ./testand ensure the required test files pass. Run the requiredbun -esweep across all 2,646 catalog IDs, and ensure every recognized family resolves with a non-null version.This change imports catalog predicates and changes resolver paths used by the prompt hook. TypeScript 7 and catalog export compatibility must be confirmed before merge. As per coding guidelines, “Run typechecking with
bun x tsc --noEmit -p tsconfig.json” and verify the three-axis resolver against the live catalog.Also applies to: 245-360
Sources: Coding guidelines, MCP tools
133-141: LGTM!src/rules.ts (1)
1-1: LGTM!Also applies to: 26-50, 59-59, 68-68, 85-85
src/settings.ts (1)
53-60: LGTM!test/settings.test.ts (1)
1-10: LGTM!Also applies to: 21-26, 61-146
src/index.ts (1)
10-14: 📐 Maintainability & Code QualityRun the required Bun validation suite. Run typechecking,
bun test ./test, and thebun -esweep across all 2,646 live catalog IDs. The current environment lacks Bun andnode_modules; globaltscstops becausebun-typesis missing.
1 participant
Summary
ompnow ships a singlebefore_agent_starthook that appends a capability-scaled## Model tuningblock after the base prompt instead of replacing it. Small/flagship models on newer generations get a light block (≤700 chars), mid-tier or one-generation-back get medium (≤1600), everything unrecognised gets heavy (≤3000). The block shrinks as the model gets newer/larger, matching the observed prior that over-prescriptive scaffolding degrades output on frontier models.Design
src/model-axis.ts) — reuses@oh-my-pi/pi-catalog/identityparsers for openai/anthropic/gemini/glm and adds version+tier regexes for kimi/deepseek/grok/qwen/minimax; normalises:thinking/:free/effort suffixes before matching. OneModelAxis {family,version,tier,weight,profile}shape;unknown→heavy.src/rules.ts) — 9 directives, one perconcern(deliberation/ambiguity/scope/context-budget/aesthetics/verification), filtered byminWeight ≤ weightandapplies(axis), sorted byprioritythenid, deduped to first per concern, budget-trimmed from the end. Wording pairs positive routing with negative guard (senpichanges.mdlesson) and never names the model to itself.src/render.ts) —selectRules/renderTuningBlockreturnsnullwhen empty (no bare heading) and otherwise## Model tuning+ blank-line-joined directives.CHAR_BUDGET[weight]enforces the size dial.src/settings.ts) — reads~/.omp/plugins/omp-plugins.lock.json→OMP_MODEL_TUNING*env → manifest defaults; never throws on missing lockfile (morph shape). Header (src/index.ts:29) is gated onhasUIandlastProfilememo.Verification
bun x tsc --noEmitcleanbun test ./test— 63 tests across 6 files (28 worked rows + qwen/minimax, real-id fixture, settings precedence, hook idempotency)bun -eover 2646 ids: 1284 classified, 0 null-versions among classified, every family spans light+heavy (minimax exception documented)deepseek-v4-proappends once,claude-opus-5returnsundefined(no light rule), second turn withTUNING_HEADERpresent returnsundefinedNotes
^7(7.0.2 GA 2026-07-08) perground-latest(was^5);skipLibCheck:trueretained —falsebreaks upstreampi-catalog/pi-coding-agenttypes..references/added to.gitignore(research material, not plugin source).Summary by cubic
Adds dynamic, per-model system prompt tuning that appends a scaled
## Model tuningblock after the base prompt. Resolver and rules were tightened to avoid dead rules and false matches; settings accept an injected config root for safe tests without changing runtime behavior.Bug Fixes
settled-readingis tier-disjoint fromreasoning-aim;no-filler-verificationmoved to a separateverificationconcern.configHome;readTuningSettings(home)reads from that root. Production path unchanged.set -e.Dependencies
typescriptto^7(keepsskipLibCheck: true).Written for commit 5f9fd7e. Summary will update on new commits.