Skip to content

Allow per-provider/model system prompt overrides - #483

Merged
m-aebrer merged 6 commits into
masterfrom
feature/issue-482-model-system-prompt-overrides
Aug 20, 2026
Merged

Allow per-provider/model system prompt overrides#483
m-aebrer merged 6 commits into
masterfrom
feature/issue-482-model-system-prompt-overrides

Conversation

@m-aebrer

Copy link
Copy Markdown
Collaborator

Closes #482

Add persistent provider/model-specific settings that can replace or append to the system prompt for built-in and custom models.

Implementation plan posted as a comment below.

@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Implementation Plan

Problem analysis

Dreb currently supports session-wide prompt replacement and append sources, while modelSettings controls only thinking display by bare model ID. The active AgentSession already has the exact provider/model identity whenever it builds or rebuilds the system prompt, so the feature can remain in the coding-agent settings/session layer without adding coding-harness prompt metadata to the generic @dreb/ai Model type.

Use provider-aware entries in the existing settings.json modelSettings, keyed by exact canonical provider/model:

{
  "modelSettings": {
    "openai/gpt-5.6-sol": {
      "appendSystemPrompt": "Model-specific instructions"
    },
    "ollama/qwen-local": {
      "systemPrompt": "Replacement prompt"
    }
  }
}

Existing bare model-ID entries for thinkingDisplay remain backward compatible. Prompt settings use exact canonical keys so two providers exposing the same model ID cannot affect each other.

Deliverables

  1. Extend model-specific settings with replacement and append prompt fields plus exact provider/model lookup.
  2. Resolve the active model's prompt settings during AgentSession system-prompt rebuilding.
  3. Apply the setting on initial startup, session restore, explicit model switches, model cycling, and resource reloads.
  4. Preserve explicit session replacement precedence: CLI flags, prompt files, and SDK/resource-loader replacement remain stronger than persistent model replacement. Compose model-specific append text deterministically after existing append sources.
  5. Reject a canonical model entry that configures both replacement and append behavior, rather than choosing silently.
  6. Document configuration, precedence, model switching, and built-in/custom model examples across the public documentation layers.

Acceptance criteria

  • A setting can target one exact provider/model pair and either replace or append to its system prompt.
  • Append mode preserves the selected base prompt and adds the configured text.
  • Replacement mode substitutes for the built-in prompt when no explicit session replacement is active.
  • Built-in and custom/local models use the same canonical lookup.
  • Switching away removes the prior model's instructions; switching to another configured model applies only its instructions.
  • Global/project settings precedence remains property-level and existing bare-ID thinking-display settings continue to work.
  • Contradictory replacement-plus-append configuration fails loudly.

Files to modify

  • packages/coding-agent/src/core/settings-manager.ts — add prompt setting types and provider/model-aware resolution while preserving legacy thinking-display behavior.
  • packages/coding-agent/src/core/agent-session.ts — compose active-model replacement/append settings into the existing prompt rebuild path.
  • packages/coding-agent/test/settings-manager.test.ts — cover canonical lookup, provider isolation, global/project merging, backward compatibility, and invalid conflicting settings.
  • packages/coding-agent/test/agent-session-model-switch-thinking.test.ts — cover initial application, custom model identities, switching/cycling, precedence, append order, and stale-instruction removal.
  • README.md — update the public model/customization overview.
  • packages/coding-agent/README.md — expand the System Prompt section with provider/model-specific configuration.
  • packages/coding-agent/docs/settings.md — document fields, canonical keys, precedence, validation, and examples.
  • packages/coding-agent/docs/models.md — cross-link custom/local models to their model-specific prompt settings.

No new files are expected.

Testing approach

  • Extend settings-manager.test.ts to verify:
    • exact canonical provider/model matching;
    • no leakage between providers sharing a model ID;
    • global/project per-property precedence;
    • unchanged bare-ID thinkingDisplay reads and writes;
    • loud rejection of conflicting prompt fields.
  • Extend agent-session-model-switch-thinking.test.ts to verify:
    • append and replacement behavior on the initial model;
    • arbitrary custom/local provider/model identities;
    • prompt rebuilding on setModel() and cycling;
    • removal of stale instructions after switching;
    • explicit session replacement precedence;
    • deterministic composition with existing append sources.
  • Run focused Vitest files and Biome checks for touched source/test files.
  • Run the complete npm test suite.
  • Run npm run build, then npm run verify-workspace-links.
  • After the required build, manually exercise the compiled dreb -p binary with a model-specific append configured.

Risks and open questions

  • Model IDs containing slashes: treat the full provider/modelId string as an opaque exact key; do not split and reconstruct model IDs.
  • Prompt precedence: explicit per-session replacement remains highest precedence to avoid changing current CLI, file, and SDK semantics. Model-specific append still composes with the resulting prompt.
  • Settings reload timing: use the existing settings/resource reload lifecycle; document that external edits become active through the normal reload path or a prompt rebuild/model switch after settings reload.
  • Project trust: project settings already participate in prompt construction alongside project prompt/context resources, so this uses the existing trust boundary rather than introducing a new one.
  • Custom-model reloads: models.json remains responsible only for model registration. Once registered, custom and built-in models follow the same canonical settings lookup.

Plan created by mach6

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown

Vitest coverage

Metric Covered Total Coverage
Statements 40175 56201 71.48%
Branches 21624 35222 61.39%
Functions 8529 11791 72.33%
Lines 29061 40432 71.87%

View full coverage run

@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Progress Update

Implemented persistent, exact provider/model system-prompt settings:

  • Added systemPrompt replacement and appendSystemPrompt append modes under modelSettings.
  • Kept bare model-ID thinking-display settings backward compatible while isolating prompt instructions by provider.
  • Applied active-model prompts during startup, model switching/cycling, session restore, and /reload.
  • Preserved explicit CLI/file/SDK replacement precedence and deterministic append ordering.
  • Added loud validation for conflicting, empty, or malformed prompt settings.
  • Added regression coverage for provider isolation, global/project precedence, switching, restore, reload, explicit prompt precedence, slash-containing custom model IDs, and invalid configuration.
  • Updated the root README, package README, settings docs, and custom-model docs.

Verification completed successfully: focused tests, full test suite, npm run check, full build, workspace-link verification, and compiled dreb -p QA.

Commit: f6ce815


Progress tracked by mach6

@m-aebrer
m-aebrer marked this pull request as ready for review August 20, 2026 14:50
@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Unverified Review Candidates — Pending Assessment

Review round: 1
Reviewed commit: f6ce815

These are unverified candidates. Severity reflects reviewer confidence; do not treat any item as a merge blocker until the assessment comment is posted.

Critical

None.

Important

  1. Model-switch validation can leave partially committed state. In packages/coding-agent/src/core/agent-session.ts:2149-2167, with equivalent ordering in both cycle paths, the live model, session history, and persisted default are changed before _rebuildSystemPrompt() validates the target model's prompt settings. A malformed replace/append entry can therefore throw after those mutations, leaving the new model paired with the old prompt and persisting the malformed target as the next startup default. Confidence: 88.

  2. Direct setModel() prompt-override behavior lacks regression coverage. packages/coding-agent/test/agent-session-model-switch-thinking.test.ts:284-300 checks identity rebuilding only; prompt replacement, append selection, and stale-instruction removal are exercised through cycling instead. Because setModel() has its own mutation/rebuild path, a regression there could escape current tests. Confidence: 94.

  3. Replacement/append assertions do not fully prove base-prompt semantics. In packages/coding-agent/test/agent-session-model-switch-thinking.test.ts:319-405, replacement is only asserted at the beginning of the prompt, without proving a recognizable built-in base is absent; append ordering is checked against loader append text, without directly proving the selected base remains. A prepend-instead-of-replace or dropped-base regression could satisfy the current assertions. Confidence: 92.

  4. Cross-scope property merging and merge-created mode conflicts are not directly covered. packages/coding-agent/test/settings-manager.test.ts:931-959 tests same-field override, not preservation of distinct properties at one canonical key or a global/project split that merges systemPrompt and appendSystemPrompt into the required loud conflict. Confidence: 90.

Suggestions

  1. Cross-scope conflict errors do not identify the contributing scopes. packages/coding-agent/src/core/settings-manager.ts:1379-1411 property-merges global and project entries, so individually valid fields can combine into an invalid dual-mode entry. The resulting message names the model key but not that replacement and append came from different settings scopes, making recovery less actionable. Confidence: 82.

  2. ModelPromptSettings duplicates part of ModelSpecificSettings. The exported interface in packages/coding-agent/src/core/settings-manager.ts:99-102 repeats the two prompt fields and could instead be a Pick<ModelSpecificSettings, ...> alias or an unexported return shape, reducing public type duplication. Confidence: 83.

  3. The resume test duplicates session construction already supported by createSession. packages/coding-agent/test/agent-session-model-switch-thinking.test.ts:249-267 can use the expanded factory with settingsManager and initialModel, removing setup redundancy without changing behavior. Confidence: 99.

Strengths

  • Exact canonical provider/model prompt lookup prevents instructions from leaking across providers while preserving legacy bare-ID thinking-display settings.
  • Replacement precedence and deterministic append ordering are centralized in _rebuildSystemPrompt().
  • Empty, non-string, and contradictory prompt settings fail loudly with model-specific errors.
  • Startup, cycling, restore, reload, custom slash-containing model IDs, explicit session precedence, and provider isolation receive meaningful coverage.
  • All authoritative issue criteria and approved plan deliverables are implemented and documented across the required public docs.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier


Reviewed by mach6

@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Review Assessment

#483 (comment)

Classifications

Finding Classification Reasoning
finding 1 merge blocker Factual: All three model-switch paths mutate the live model, append session history, and persist the target as default before prompt validation can throw. Scope: The new validation makes the previously harmless ordering a PR-introduced partial-commit regression in the feature's explicit contradictory/malformed-configuration path. Practical: A user who accidentally configures both modes and selects that model receives an error but is left on the new model with the old model's prompt; the invalid target is also persisted, so the next launch fails during construction until settings are hand-edited. Validate/build for the target before committing switch state.
finding 2 discarded observation Factual: Direct override-flavored assertions are absent, but an existing setModel() test already proves prompt rebuilding and stale model-identity removal. Scope: No per-path permutation is required. Practical: Shared _rebuildSystemPrompt() behavior is covered through startup, cycle, restore, and reload, so this does not expose a credible untested implementation defect.
finding 3 nitpick Factual: The assertions could more directly prove built-in-base absence/presence. Scope: Current implementation satisfies replacement and append requirements. Practical: This is future regression hardening, not evidence of a current defect.
finding 4 nitpick Factual: Distinct-property merging and a conflict assembled across scopes are not tested directly. Scope: Both behaviors are implemented and documented as planned. Practical: No current failure was found; extra tests would harden established merge behavior.
finding 5 nitpick Factual: The conflict error names the canonical key and fields, not their source scopes. Scope: The requirement is loud rejection, which is met. Practical: Users have two settings scopes to inspect and enough information to locate the fields, so the omission does not materially block recovery.
finding 6 nitpick Factual: ModelPromptSettings repeats a two-field subset of ModelSpecificSettings. Scope: Type factoring is outside acceptance behavior. Practical: The narrow type is harmless; changing it is stylistic.
finding 7 nitpick Factual: The expanded createSession factory can replace the manual setup in the resume test. Scope: Test deduplication is not required by the feature. Practical: The duplication is small and has no correctness effect.

Action Plan

  1. Make setModel(), _cycleScopedModel(), and _cycleAvailableModel() validate the target model's prompt settings before mutating the live agent, appending session history, or persisting the default. Add regression coverage proving a malformed target rejects the switch without changing the active model, prompt, session model history, or persisted default.

Assessment by mach6

@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Progress Update

Fixed review finding 1 by making malformed target-model prompt settings fail before any model-switch state is committed:

  • Validate target prompt settings before mutating the live model in direct selection, scoped cycling, and available-model cycling.
  • Preserve the active model, system prompt, session history, and persisted default when validation fails.
  • Added parameterized regression coverage for all three switch paths.

Verification passed: focused tests, the full coding-agent suite, repository checks, build, workspace-link verification, and the commit hook's full non-live suite (5,805 passed).

Commit: 0c6697f


Progress tracked by mach6

@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Unverified Review Candidates — Pending Assessment

Review round: 2
Reviewed commit: 0c6697f

These are unverified candidates. Severity reflects reviewer confidence; do not treat any item as a merge blocker until the assessment comment is posted.

Critical

None.

Important

  1. Session restore can partially commit malformed model prompt settings and leave the agent disconnected. In packages/coding-agent/src/core/agent-session.ts:3608-3668, switchSession() disconnects the agent, switches the session file, replaces messages, and restores the model before _rebuildSystemPrompt() validates the restored model's prompt configuration. A contradictory, empty, or non-string prompt setting can therefore throw after those mutations and before _reconnectToAgent(), leaving mismatched session/model/prompt state and disconnected event handling. The three direct/cycle switch paths now validate before mutation, but restore does not. Confidence: 84.

  2. Replacement and append tests do not prove built-in base-prompt semantics. In packages/coding-agent/test/agent-session-model-switch-thinking.test.ts:364-427, replacement is asserted only at the prompt start without proving recognizable built-in prompt content is absent, while append ordering is checked without proving the built-in base remains. A prepend-instead-of-replace or dropped-base regression could satisfy the current assertions while materially changing model behavior. Confidence: 96.

  3. Explicit session replacement combined with model-specific append is untested. The tests at packages/coding-agent/test/agent-session-model-switch-thinking.test.ts:382-427 separately cover explicit replacement versus model replacement and loader append versus model append, but not the required composition where an explicit CLI/file/SDK replacement remains the base and appendSystemPrompt still follows it. An early-return regression could silently drop persistent model instructions for users with explicit replacement prompts. Confidence: 94.

  4. Cross-scope property merging and merge-created conflicts are not proved. packages/coding-agent/test/settings-manager.test.ts:931-959 sets the same append property globally and per project, so it would pass if the project entry replaced the whole global entry. It does not cover a global systemPrompt plus project appendSystemPrompt merging into the required loud conflict, leaving documented property-level merge behavior vulnerable to silent wholesale override regressions. Confidence: 92.

Suggestions

None.

Strengths

  • The prior round's merge blocker is fixed for setModel(), scoped cycling, and available-model cycling: validation now precedes every state mutation, and the parameterized regression test verifies the active model, prompt, session entries, and persisted default remain unchanged.
  • Exact canonical provider/model lookup prevents prompt leakage across providers while retaining legacy bare-ID thinking-display behavior.
  • Prompt composition correctly gives explicit session replacement precedence and places model-specific append text after existing append sources.
  • Built-in/custom model identities, slash-containing model IDs, startup, cycling, restore, reload, provider isolation, and malformed setting rejection receive meaningful coverage.
  • All issue criteria, approved plan deliverables, and required public documentation layers are implemented.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker; simplifier dispatch timed out before child spawn


Reviewed by mach6

@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Review Assessment

#483 (comment)

Finding 5 was added during assessment as an explicit human-approved scope update after the unverified-candidates comment exposed the workflow defect in this review.

Classifications

Finding Classification Reasoning
finding 1 merge blocker Factual: switchSession() disconnects, changes the session file, replaces messages, and restores the model before _rebuildSystemPrompt() can throw on malformed prompt settings; the throw skips _reconnectToAgent(). Scope: Session restore is an explicit approved-plan application path, and this PR introduces the new validation throw there. Practical: A user resuming a session whose saved model has contradictory, empty, or non-string prompt settings is left with a half-switched, disconnected live session rather than a clean rejection. Resolve and validate the restored model before disconnecting or mutating the current session, and cover atomic rejection.
finding 2 nitpick Factual: The assertions do not directly prove built-in base absence in replacement mode or retention in append mode. Scope: Stronger semantic assertions would improve regression hardening. Practical: Current source composition is correct, and no present defect or likely material failure was found.
finding 3 useful follow-up Factual: No test combines an explicit CLI/file/SDK replacement base with model-specific append text. Scope: That composition is part of the documented precedence behavior. Practical: Current code correctly retains the model append, so this is valuable coverage rather than a shipping defect.
finding 4 useful follow-up Factual: The scope-merge test uses the same property in both scopes and therefore does not prove distinct-property merging or merge-created conflict rejection. Scope: Property-level precedence is an explicit plan criterion. Practical: Current { ...global, ...project } implementation is correct; additional tests would guard it but do not repair a current failure.
finding 5 merge blocker Factual: packages/coding-agent/skills/mach6-review/SKILL.md says “Never run simplifier serially after the others,” which prevented retry after a dispatch failure in this review, and instructs round 3+ to review only the latest delta while rejecting unchanged-code findings. packages/coding-agent/docs/mach6.md repeats the delta-only policy. Scope: The user explicitly approved removing the retry prohibition and required every round to consider all PR changes together unless the user specifies otherwise. Practical: The current retry rule already caused one required specialist to be omitted, while delta-only later rounds can miss unresolved whole-PR defects and interactions merely because their lines did not change after the previous review. Update the skill and public mach6 documentation so every round reviews the full PR, with later deltas used only as supplemental context, and remove the simplifier serial-retry prohibition.

Action Plan

  1. Make malformed restored-model prompt settings reject before switchSession() disconnects or mutates the active session, and add regression coverage proving the current session remains connected and unchanged.
  2. Remove the simplifier serial-retry prohibition and replace all round-3+ delta-only/unchanged-code-rejection instructions with full-PR review in every round unless the user explicitly requests a narrower target; retain the latest delta only as supplemental context for verifying fixes and interactions. Update packages/coding-agent/docs/mach6.md consistently.

Assessment by mach6

@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Progress Update

Fixed review findings 1 and 5:

  • Preflight the saved model and validate its prompt settings before session restore disconnects or mutates the active session.
  • Added regression coverage proving malformed restore settings preserve the active model, prompt, messages, session identity/history, and event subscription.
  • Changed mach6 review so every round reviews the full PR, with later deltas used only as supplemental fix-verification context.
  • Run simplifier in every review round and explicitly retry any specialist that fails dispatch arbitration or execution.
  • Updated mach6 documentation and built-in skill contract tests.

Verification passed: focused tests, full non-live workspace suite (5,806 passed), repository checks, build, and workspace-link verification.

Commit: e1afa63


Progress tracked by mach6

@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Unverified Review Candidates — Pending Assessment

Review round: 3
Reviewed commit: e1afa63

These are unverified candidates. Severity reflects reviewer confidence; do not treat any item as a merge blocker until the assessment comment is posted.

Critical

None.

Important

  1. The interactive model selector persists a malformed target before setModel() can reject it. packages/coding-agent/src/modes/interactive/components/model-selector.ts:319-322 calls setDefaultModelAndProvider() before invoking the async callback that reaches AgentSession.setModel(). If the selected model's canonical prompt settings are contradictory, empty, or non-string, the session-level validation rejects without changing the active model, but the selector has already saved the invalid target as the startup default. A later launch can then fail during prompt construction until settings are manually repaired. Confidence: 88.

  2. The atomic-restore regression test does not behaviorally prove preservation of a populated conversation or live event subscription. packages/coding-agent/test/agent-session-model-switch-thinking.test.ts:283-319 captures an unseeded session's empty messages and entries, so destructive replacement with empty state would still satisfy those assertions. It also compares the private _unsubscribeAgent callback by identity rather than emitting an event and proving the session still processes it; invoking that callback without clearing the field would evade the check. Seed distinctive active messages/history and verify a post-rejection agent event is still processed. Confidence: 96.

Suggestions

  1. One resume test duplicates setup already supported by createSession(). packages/coding-agent/test/agent-session-model-switch-thinking.test.ts:324-350 manually constructs settings, session manager, auth storage, agent, registry, and session even though the expanded createSession({ settingsManager, initialModel }) helper supplies the same setup and is used by the adjacent restore test. Confidence: 88.

Strengths

  • The prior restore blocker is fixed in source: saved-model resolution and prompt validation now occur before disconnecting or mutating the active session.
  • The mach6 policy fix is complete and consistent across the skill, public documentation, and contract tests: every round reviews the full PR, later deltas are supplemental, and failed specialists are retried.
  • Exact canonical provider/model lookup prevents cross-provider prompt leakage while preserving legacy bare-ID thinking-display settings.
  • Prompt replacement precedence and append ordering are centralized, deterministic, and applied across startup, switching, cycling, restore, and reload.
  • All authoritative issue criteria and planned documentation deliverables are implemented.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier


Reviewed by mach6

@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Review Assessment

#483 (comment)

Classifications

Finding Classification Reasoning
finding 1 merge blocker Factual: ModelSelectorComponent.handleSelect() durably saves the target before its callback reaches AgentSession.setModel(), while setModel() validates and persists only after validation. A malformed prompt entry therefore rejects the live switch but leaves the invalid model as the default. Scope: Explicit model switching is an approved feature path, and the PR-introduced validation makes this a reachable partial-commit regression. It also preserves the same invalid-default consequence that the first review required the session switch paths to eliminate. Practical: A user configures contradictory, empty, or non-string prompt settings, selects that model through /model, sees the switch reject, then launches dreb later. Startup resolves the persisted target and throws during system-prompt construction before the TUI appears, requiring manual settings repair or an explicit model override. The selector write is redundant on success because setModel() already persists after validation; removing it closes the failure path without weakening a safeguard. Both the independent assessor and developer's advocate found this material.
finding 2 nitpick Factual: The test compares empty messages and uses private callback identity rather than behavioral event-flow evidence. Scope: Stronger assertions would improve regression hardening. Practical: The existing test still has multiple non-vacuous checks—model, prompt, session file and IDs, target entries, and subscription identity—that fail for the original partial-restore implementation. No important current defect or credible unguarded mutation sequence was established.
finding 3 nitpick Factual: The manual resume-test setup can be replaced by the expanded createSession() helper. Scope: Test deduplication is not part of the feature's acceptance behavior. Practical: The duplication is small and has no correctness or user-facing consequence; this is the same test-hygiene observation previously assessed as a nitpick.

Action Plan

  1. Remove the eager setDefaultModelAndProvider() call from ModelSelectorComponent.handleSelect() so AgentSession.setModel() is the single persistence point after prompt validation. Add regression coverage proving a malformed selection leaves the persisted default unchanged.

Assessment by mach6

@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Progress Update

Fixed review finding 1 by making model-selector persistence atomic:

  • Removed the selector's eager default-model write; AgentSession.setModel() is now the single persistence point after prompt validation succeeds.
  • Removed the selector's now-unused settings-manager dependency.
  • Added regression coverage proving a malformed selected model leaves the persisted default unchanged.

Verification passed: focused model-selector and model-switch tests, repository checks, full non-live suite (5,807 passed), build, workspace-link verification, and the commit hook's full non-live suite. A transient dashboard browser-test timeout during the first commit attempt passed on focused rerun and on the successful commit-hook rerun.

Commit: 1132e5e


Progress tracked by mach6

@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Unverified Review Candidates — Pending Assessment

Review round: 4
Reviewed commit: 1132e5e

These are unverified candidates. Severity reflects reviewer confidence; do not treat any item as a merge blocker until the assessment comment is posted.

Critical

None.

Important

  1. /reload can tear down the active runtime before validating newly loaded prompt settings. In packages/coding-agent/src/core/agent-session.ts:3315-3334, reload emits session_shutdown, reloads settings, resets API providers, and reloads resources before _buildRuntime() reaches _rebuildSystemPrompt() and validates the current model's canonical prompt entry. If an external settings edit adds contradictory, empty, or non-string prompt settings for the active model, the rebuild throws after teardown and before the new extension runtime receives session_start or resource extension, leaving a half-reloaded session despite only reporting the configuration error. Confidence: 85.

  2. Explicit session replacement combined with model-specific append is not exercised. packages/coding-agent/test/agent-session-model-switch-thinking.test.ts:422-465 separately covers explicit replacement versus model replacement and loader append versus model append, but not an explicit CLI/file/SDK replacement serving as the base while appendSystemPrompt still follows it. A regression that skips model append whenever an explicit replacement exists would silently lose persistent model instructions while passing current tests. Confidence: 96.

  3. Cross-scope property merging and merge-created conflicts are not proved. packages/coding-agent/test/settings-manager.test.ts:931-959 configures the same append property globally and per project, so it would still pass if project entries replaced whole global entries. It does not prove that a global systemPrompt plus project appendSystemPrompt merges and fails loudly, or that a compatible global property survives a project prompt property. Confidence: 97.

  4. Core replacement/append tests do not directly prove built-in base-prompt semantics. packages/coding-agent/test/agent-session-model-switch-thinking.test.ts:402-465 proves replacement text starts the prompt and append ordering is correct, but does not assert a recognizable built-in base marker is absent for replacement and retained for append. Prepending a “replacement” while retaining the built-in base, or constructing append mode without the normal base, could satisfy the current assertions while violating the primary contract. Confidence: 93.

Suggestions

None.

Strengths

  • The previous selector-persistence blocker is fixed: the selector only invokes its callback, while AgentSession.setModel() validates before persisting; the regression test proves malformed selection leaves the default unchanged.
  • Exact canonical provider/model prompt lookup prevents cross-provider leakage and supports slash-containing custom model IDs while preserving legacy bare-ID thinking settings.
  • Prompt composition is centralized and gives explicit session replacement precedence while placing model append text after loader append sources.
  • Direct switching, both cycling paths, and session restore preflight malformed settings before committing model/session state.
  • Full issue scope, approved plan deliverables, subsequent review-policy scope updates, and public documentation are implemented.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier (successful after dispatch-arbitration retries)


Reviewed by mach6

@m-aebrer

Copy link
Copy Markdown
Collaborator Author

Review Assessment

#483 (comment)

Classifications

Finding Classification Reasoning
finding 1 useful follow-up Factual: The verified chain is reload() → settings/resource teardown and reload → _buildRuntime()_refreshToolRegistry()setActiveToolsByName()_rebuildSystemPrompt(), where malformed current-model settings can throw after session_shutdown and before the new runner receives session_start or extension resources. Scope: /reload applying external prompt edits and loud malformed-config rejection are planned behavior, but reload atomicity was not an explicit criterion; this is nevertheless a PR-introduced consistency gap worth closing. Practical: A user who edits the active model's entry incorrectly and runs /reload can temporarily leave extensions half-initialized. The failure is loud and exact, does not persist a new default or disconnect agent events, and is recoverable by fixing the file and re-running /reload. Both the independent assessor and developer's advocate found the impact bounded rather than merge-blocking. Preflight the active model before teardown as a follow-up.
finding 2 useful follow-up Factual: No test combines an explicit session replacement base with model appendSystemPrompt. Scope: That composition is documented and part of the approved precedence behavior. Practical: Current code computes replacement selection and append composition independently and correctly, with no early return; this is valuable regression coverage, not a present defect.
finding 3 useful follow-up Factual: The existing scope test proves only same-property project override, not distinct-property preservation or a conflict assembled across scopes. Scope: Property-level global/project precedence is an explicit criterion. Practical: Current { ...global, ...project } merging and merged-shape validation are correct and loud; additional tests would protect against a future wholesale-entry replacement regression but do not repair current behavior.
finding 4 nitpick Factual: Tests do not explicitly assert a built-in marker is absent in replacement mode and retained in append mode, although the replacement assertion is start-anchored. Scope: Stronger semantic assertions would harden the primary contract. Practical: The unchanged buildSystemPrompt replacement branch and current PR wiring implement the correct behavior; no reachable current defect was found.

Action Plan

No merge blockers.


Assessment by mach6

@m-aebrer
m-aebrer merged commit 3dd3b6f into master Aug 20, 2026
3 checks passed
@m-aebrer
m-aebrer deleted the feature/issue-482-model-system-prompt-overrides branch August 20, 2026 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow per-provider/model system prompt overrides

1 participant