Greptile Summary

This PR adds warning logs in two places when a configured model ID cannot be found: at runtime when runWithModelFallback successfully falls back after the primary model returns model_not_found, and at config-resolution time when a malformed model string (e.g. "openai/") cannot be parsed. Fallback behavior is unchanged — this is purely additive observability. The implementation is clean and the gating condition (attempts[0]?.reason === "model_not_found") correctly avoids false positives for cooldown-skip scenarios.

• src/agents/model-fallback.ts: Warning is correctly guarded by i > 0 && attempts[0]?.reason === "model_not_found", ensuring it only fires when the user's primary model was actually rejected with that specific error.
• src/agents/model-selection.ts: Warning is placed after the if (resolved) return guard so it only fires on the unresolvable-string fallback path.
• src/agents/model-selection.test.ts: warnSpy.mockRestore() is missing from the finally block, causing the suppressing mock implementation to leak into subsequent tests (see inline comment).

Confidence Score: 4/5

• Safe to merge; the only issue is a test-isolation bug in model-selection.test.ts that could suppress warnings in later tests.
• The production code changes are minimal, correctly gated, and do not alter any fallback behavior. The single bug found (missing warnSpy.mockRestore() in model-selection.test.ts) is a test-isolation issue, not a production correctness problem, which keeps the score high but not perfect.
• src/agents/model-selection.test.ts — missing warnSpy.mockRestore() in the finally block will leak the suppression mock to subsequent tests.

_{Last reviewed commit: aeb6181}

@steipete — small agents fix (4 files, size:S). When an unrecognised model ID is configured, the current code silently falls back to the default model. This adds a console.warn() so users know the configured model wasn't found.

CI all green, greptile finding (missing mockRestore) already fixed in 963846d.

Fixed in da08ac8. Both warn sites now sanitize model strings via stripAnsi() + control char removal before interpolation (CWE-117).

Uses the existing src/terminal/ansi.ts:stripAnsi utility — no new dependencies.

@steipete -- would appreciate your eyes on this when you get a chance.

Context: Adds a warning log when an unrecognised model ID silently falls back to defaults (closes #37813). Size S, 4 files in src/agents/, +90/-0, all 98 tests green.

CI note: The extensions/diffs test failure is a pre-existing issue from your 44881b02 commit (missing headers in index.test.ts mock). Separate fix submitted in #39041 -- trivial 1-line alignment with http.test.ts.

The "Changes at a glance" table in the description has the exact lines to review.

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🔵 Incomplete log sanitization allows terminal control / log injection via C1 control characters

Description

runWithModelFallback() adds a warning log that includes provider/model values derived from user-controlled sources (CLI args, config values, session model overrides). The new sanitize() helper strips ANSI SGR + OSC-8 and removes C0 controls (U+0000–U+001F) and DEL (U+007F), but does not remove C1 control characters (U+0080–U+009F).

Because many terminals/log viewers treat C1 characters as control codes (e.g. U+009B CSI, U+009D OSC, U+0085 NEL), an attacker who can influence provider/model strings can still:

• inject terminal control sequences (screen clearing, cursor movement, OSC sequences)
• potentially forge/mislead operators by manipulating how the log line renders

Vulnerable code:

const sanitize = (v: string) => {
  let out = stripAnsi(v);
  for (let c = 0; c <= 0x1f; c++) {
    out = out.replaceAll(String.fromCharCode(c), "");
  }
  return out.replaceAll(String.fromCharCode(0x7f), "");
};
log.warn(`Model "${sanitize(attempts[0].provider)}/${sanitize(attempts[0].model)}" ...`);

This leaves C1 controls intact (e.g. \u009b, \u009d), which undermines the intended CWE-117 mitigation.

Recommendation

Use a dedicated sanitizer that removes both C0 and C1 control characters before logging, and consider making separators visible (escaping) instead of deleting.

This repo already has a helper for this purpose: sanitizeTerminalText (src/terminal/safe-text.ts). Prefer reusing it:

import { sanitizeTerminalText } from "../terminal/safe-text.js";

log.warn(
  `Model "${sanitizeTerminalText(attempts[0].provider)}/${sanitizeTerminalText(attempts[0].model)}" not found. ` +
  `Fell back to "${sanitizeTerminalText(candidate.provider)}/${sanitizeTerminalText(candidate.model)}".`,
);

If you keep a local sanitizer, extend it to strip U+007F–U+009F at minimum (and optionally other Unicode separators like U+2028/U+2029 if you want to prevent multi-line rendering across environments).

Analyzed PR: #38631 at commit 4da48cb

_{Last updated on: 2026-03-07T21:12:23Z}

CI Status Update (post-rebase)

After rebasing onto latest main, 5 CI failures surfaced. Analysis:

Fixed in this PR (commit 4da48cb):

• check (TS2741): Two test files (models-config.fills-missing-provider-apikey-from-env-var.test.ts, models-config.providers.normalize-keys.test.ts) were missing baseUrl in ModelProviderConfig objects after upstream made baseUrl required. Added baseUrl: "" to both. All 117 tests pass locally.

Pre-existing on main (not fixed here):

• secrets: detect-secrets baseline drift - already an active firefight with 7+ commits on March 7 from steipete, Vincent Koc, Ayaan Zaidi, and Josh Avant. PR fix CI: add missing mode property and secret scan pragmas in tests #39169 addresses the exact breakage. See also The secrets job appears to be falling back to a full repository scan, #38515, CI secrets PR fast-path can fallback to full scan when base SHA missing in shallow checkout #38481, [Security] Baseline hardening: headers, CI scanning, and local assets #6675.
• checks-windows (shards 1, 4, 6): Windows 8.3 short-path normalisation (RUNNER~1 vs runneradmin) in src/hooks/install.test.ts. Unrelated to our changes.
• check (ui/ type errors): chatStreamSegments / streamSegments type mismatches in ui/src/ui/. Pre-existing on main, unrelated to agent model fallback.

Our PR touches only 4 files in src/agents/ (model-fallback and model-selection) with 90 lines of changes. The unfixed failures all exist on main independent of this PR.

Closing this one. The merge conflicts and CI failures have stacked up, and the extensions test regressions on main make it hard to get a clean baseline here. The underlying idea (warning on unrecognised model IDs) still has merit - I'll revisit it as a smaller, more focused fix once the test landscape on main settles down.