feat: support scoped model config overlays - #355
Conversation
|
Review: this reproduces the scoped two-profile model-selection workflow accepted by the maintainer, not arbitrary full-config replacement. Full replacement would conflict with shared embedding/storage/compaction/memory state. I found one issue: unsupported top-level or non-OpenCode overlay keys were silently ignored. I am tightening warnings and adding regression coverage. |
|
Follow-up review and fix: unsupported top-level fields and non-OpenCode harness fields in MAGIC_CONTEXT_CONFIG are now reported explicitly instead of being silently ignored. Regression coverage verifies model fields apply while embedding and memory settings remain unchanged. Focused suite: 53 passed, 0 failed; git diff --check passed. Commit: 24f3dcf. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/config/index.ts">
<violation number="1" location="packages/plugin/src/config/index.ts:74">
P2: Keys in `MAGIC_CONTEXT_CONFIG` that are not inside one of the three agents' `opencode` blocks are silently dropped with no warning, which contradicts the PR's stated behavior of emitting a config warning for any unsupported key. The loop only iterates `MODEL_OVERLAY_AGENTS`, so a top-level key such as `embedding`, `memory`, or `storage`, or a `pi`-harness block (`historian.pi.model`), or any key directly under an agent's `opencode` that isn't one of the three model fields never reaches `ignored`. The regression test itself includes `embedding: { provider: off }` in the overlay and only asserts it isn't applied — it never asserts a warning, so the silent-drop path is untested. A user who puts durable settings in the overlay (or a Pi-only override) gets no feedback that they were ignored, which is exactly the "ignored key" risk the warning was meant to surface. Consider iterating all remaining overlay keys (outside the supported agent/field set) and adding them to the `ignored` list.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ignored.push(key); | ||
| } | ||
| } | ||
| for (const agent of MODEL_OVERLAY_AGENTS) { |
There was a problem hiding this comment.
P2: Keys in MAGIC_CONTEXT_CONFIG that are not inside one of the three agents' opencode blocks are silently dropped with no warning, which contradicts the PR's stated behavior of emitting a config warning for any unsupported key. The loop only iterates MODEL_OVERLAY_AGENTS, so a top-level key such as embedding, memory, or storage, or a pi-harness block (historian.pi.model), or any key directly under an agent's opencode that isn't one of the three model fields never reaches ignored. The regression test itself includes embedding: { provider: off } in the overlay and only asserts it isn't applied — it never asserts a warning, so the silent-drop path is untested. A user who puts durable settings in the overlay (or a Pi-only override) gets no feedback that they were ignored, which is exactly the "ignored key" risk the warning was meant to surface. Consider iterating all remaining overlay keys (outside the supported agent/field set) and adding them to the ignored list.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/config/index.ts, line 74:
<comment>Keys in `MAGIC_CONTEXT_CONFIG` that are not inside one of the three agents' `opencode` blocks are silently dropped with no warning, which contradicts the PR's stated behavior of emitting a config warning for any unsupported key. The loop only iterates `MODEL_OVERLAY_AGENTS`, so a top-level key such as `embedding`, `memory`, or `storage`, or a `pi`-harness block (`historian.pi.model`), or any key directly under an agent's `opencode` that isn't one of the three model fields never reaches `ignored`. The regression test itself includes `embedding: { provider: off }` in the overlay and only asserts it isn't applied — it never asserts a warning, so the silent-drop path is untested. A user who puts durable settings in the overlay (or a Pi-only override) gets no feedback that they were ignored, which is exactly the "ignored key" risk the warning was meant to surface. Consider iterating all remaining overlay keys (outside the supported agent/field set) and adding them to the `ignored` list.</comment>
<file context>
@@ -55,6 +56,58 @@ function getProjectConfigBasePath(directory: string): string {
+
+ const overlay: Record<string, unknown> = {};
+ const ignored: string[] = [];
+ for (const agent of MODEL_OVERLAY_AGENTS) {
+ const source = loaded.config[agent];
+ if (!source || typeof source !== "object" || Array.isArray(source)) {
</file context>
There was a problem hiding this comment.
The top-level/non-OpenCode warning path was added in commit 24f3dcf and is covered by the existing regression assertions for embedding and memory. The current branch now also adds flat sidekick coverage.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/config/index.ts">
<violation number="1" location="packages/plugin/src/config/index.ts:87">
P3: For a flat sidekick overlay, `harness` is `sourceRecord`, so the existing second loop (`for (const [key, value] of Object.entries(harness))`) iterates the exact same keys the new first loop just scanned. Any unsupported sidekick key (e.g. `system_prompt`) is pushed to `ignored` twice — once as `sidekick.system_prompt` (first loop) and again as `sidekick.opencode.system_prompt` (second loop, which still uses the historian/dreamer `.opencode.` label). Users see a duplicate, wrongly-labeled entry in the `[model overlay] Ignoring unsupported keys` warning. Skip the first-loop push for flat sidekick and label the second-loop push correctly using `isFlatSidekick`.</violation>
<violation number="2" location="packages/plugin/src/config/index.ts:106">
P2: When a flat sidekick overlay contains an invalid model field, it erases the corresponding valid base setting before recovery runs. Validate overlay fields before merging, or retain the base value when an overlay field fails validation.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
| } | ||
| if (Object.keys(selected).length > 0) { | ||
| overlay[agent] = isFlatSidekick ? selected : { opencode: selected }; |
There was a problem hiding this comment.
P2: When a flat sidekick overlay contains an invalid model field, it erases the corresponding valid base setting before recovery runs. Validate overlay fields before merging, or retain the base value when an overlay field fails validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/config/index.ts, line 106:
<comment>When a flat sidekick overlay contains an invalid model field, it erases the corresponding valid base setting before recovery runs. Validate overlay fields before merging, or retain the base value when an overlay field fails validation.</comment>
<file context>
@@ -99,7 +103,7 @@ function loadModelOverlay(): Record<string, unknown> | null {
}
if (Object.keys(selected).length > 0) {
- overlay[agent] = { opencode: selected };
+ overlay[agent] = isFlatSidekick ? selected : { opencode: selected };
}
}
</file context>
| } | ||
| const sourceRecord = source as Record<string, unknown>; | ||
| const isFlatSidekick = agent === "sidekick"; | ||
| const harness = isFlatSidekick ? sourceRecord : sourceRecord.opencode; |
There was a problem hiding this comment.
P3: For a flat sidekick overlay, harness is sourceRecord, so the existing second loop (for (const [key, value] of Object.entries(harness))) iterates the exact same keys the new first loop just scanned. Any unsupported sidekick key (e.g. system_prompt) is pushed to ignored twice — once as sidekick.system_prompt (first loop) and again as sidekick.opencode.system_prompt (second loop, which still uses the historian/dreamer .opencode. label). Users see a duplicate, wrongly-labeled entry in the [model overlay] Ignoring unsupported keys warning. Skip the first-loop push for flat sidekick and label the second-loop push correctly using isFlatSidekick.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/config/index.ts, line 87:
<comment>For a flat sidekick overlay, `harness` is `sourceRecord`, so the existing second loop (`for (const [key, value] of Object.entries(harness))`) iterates the exact same keys the new first loop just scanned. Any unsupported sidekick key (e.g. `system_prompt`) is pushed to `ignored` twice — once as `sidekick.system_prompt` (first loop) and again as `sidekick.opencode.system_prompt` (second loop, which still uses the historian/dreamer `.opencode.` label). Users see a duplicate, wrongly-labeled entry in the `[model overlay] Ignoring unsupported keys` warning. Skip the first-loop push for flat sidekick and label the second-loop push correctly using `isFlatSidekick`.</comment>
<file context>
@@ -82,12 +82,16 @@ function loadModelOverlay(): Record<string, unknown> | null {
- if (key !== "opencode") ignored.push(`${agent}.${key}`);
+ const sourceRecord = source as Record<string, unknown>;
+ const isFlatSidekick = agent === "sidekick";
+ const harness = isFlatSidekick ? sourceRecord : sourceRecord.opencode;
+ for (const key of Object.keys(sourceRecord)) {
+ if (isFlatSidekick ? !(MODEL_OVERLAY_FIELDS as readonly string[]).includes(key) : key !== "opencode") {
</file context>
|
Status update, honestly framed: your implementation announcement on #354 was legitimate — you followed the words of our comment more carefully than we followed the thread, and the hours of silence before your PR were our miss. Since then we've done the deeper surface walk that should have preceded that comment, and posted the resulting concerns on #354 (process-vs-profile scoping, doctor/dashboard visibility, a direnv-shaped trust edge, loader proliferation, cross-process config sync). The env-var mechanism itself is now an open design question there, so this PR is on hold pending that conversation rather than in review. Two things so your work isn't wasted: your historian/dreamer reading is already per-harness-aware, which is exactly right against the shape landing in the next release; and if the design conversation lands on proceeding (in env-var form or the profile-block alternative sketched on #354), we have concrete review feedback ready — the known items are the sidekick block (sidekick is flat in the current schema, so those overlay entries are silently discarded), unvalidated overlay values overwriting valid config, and Pi-side coverage. Thanks for the speed and the engagement either way. |
|
Thanks for the detailed status update and for clarifying the current direction. I agree that the PR should remain on hold while #354 settles the configuration mechanism and the process/profile semantics. I have updated the PR description to reflect that status and to record the remaining review questions: invalid overlay values must not erase valid base settings, and flat sidekick warnings need correct de-duplication/labels. I will align any follow-up implementation with the final design from #354 rather than making those changes against a potentially obsolete env-var contract. |
f457364 to
e985c28
Compare
1 participant
Summary
Closes #354.
This PR adds an optional
MAGIC_CONTEXT_CONFIGJSONC overlay for per-run model selection across historian, dreamer, and sidekick.model,fallback_models, andvariantoverrides only.opencodeshape for historian and dreamer, and the flat model shape for sidekick.Existing behavior is unchanged when
MAGIC_CONTEXT_CONFIGis unset.Verification
bun test packages/plugin/src/config/index.test.ts— passed (53 tests, 0 failures).git diff --check— passed.Status / Design Note
The implementation follows the scoped two-profile model-selection workflow discussed for #354. The PR is currently on hold pending the maintainer's design discussion about the configuration mechanism and profile/process behavior.
The latest automated review also identified follow-up coverage/robustness questions around invalid supported overlay values replacing valid base settings, and duplicate/mislabelled warnings for unsupported flat sidekick fields. These should be resolved against the final direction agreed in #354 before merging.
Greptile Summary
This PR adds a
MAGIC_CONTEXT_CONFIGJSONC overlay that limits per-run overrides to historian, dreamer, and sidekick model-selection fields while preserving durable configuration.Confidence Score: 4/5
The PR is not yet safe to merge because invalid supported overlay values can erase valid base model selections.
The overlay is merged over the valid base configuration before validation, and nested recovery removes invalid replacement fields from that merged object rather than restoring the values they displaced.
Files Needing Attention: packages/plugin/src/config/index.ts
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR U[User configuration] --> M[Merge user and project configuration] P[Project configuration] --> M E[MAGIC_CONTEXT_CONFIG] --> F[Filter to supported model fields] F --> M M --> V[Schema validation and recovery] V --> R[Runtime configuration]Reviews (3): Last reviewed commit: "fix: support flat sidekick model overlay..." | Re-trigger Greptile
Context used: