sanitizeSessionHistory uses stale original model identity on fallback

model.api, modelId, and provider are closed over from the outer scope (lines 265-266, 282) and always refer to the original (primary) model. When a fallback candidate from a different provider is being used (e.g., falling back from anthropic/claude-sonnet-4-6 to openai/gpt-4o-mini), sanitizeSessionHistory still receives the original provider's API type and identifiers. Because this function applies provider-specific message sanitization logic (e.g., filtering tool-result blocks that are illegal for a given model API), using the wrong modelApi/provider/modelId can produce incorrectly sanitized messages that the fallback model then rejects or silently mishandles.

The current candidate's values should be passed instead:

This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/compact.ts
Line: 663-673

Comment:
`sanitizeSessionHistory` uses stale original model identity on fallback

`model.api`, `modelId`, and `provider` are closed over from the outer scope (lines 265-266, 282) and always refer to the *original* (primary) model. When a fallback candidate from a different provider is being used (e.g., falling back from `anthropic/claude-sonnet-4-6` to `openai/gpt-4o-mini`), `sanitizeSessionHistory` still receives the original provider's API type and identifiers. Because this function applies provider-specific message sanitization logic (e.g., filtering tool-result blocks that are illegal for a given model API), using the wrong `modelApi`/`provider`/`modelId` can produce incorrectly sanitized messages that the fallback model then rejects or silently mishandles.

The current candidate's values should be passed instead:

```suggestion
            const prior = await sanitizeSessionHistory({
              messages: session.messages,
              modelApi: currentModel.api,
              modelId: cand.model,
              provider: cand.provider,
              allowedToolNames,
              config: params.config,
              sessionManager,
              sessionId: params.sessionId,
              policy: transcriptPolicy,
            });
```

How can I resolve this? If you propose a fix, please make it concise.

Fixed in commit fcc5f17 — sanitizeSessionHistory now uses currentModel.api, cand.model, and cand.provider so sanitization uses the active fallback's provider rules.

Diagnostic log reports original model instead of active fallback candidate

The provider and modelId logged on line 731 are the original session values (lines 265–266) and remain unchanged when a fallback model is active. When a fallback fires (line 576–840), the log line will incorrectly attribute the compaction run to the primary model, making it difficult to correlate actual execution with logged diagnostics.

Replace provider with cand.provider and modelId with cand.model in the diagnostic log template on line 731 to reflect the model actually being used.

Note: The same issue appears on line 785 in the end diagnostic log—update that as well.

This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/compact.ts
Line: 729-735

Comment:
Diagnostic log reports original model instead of active fallback candidate

The `provider` and `modelId` logged on line 731 are the original session values (lines 265–266) and remain unchanged when a fallback model is active. When a fallback fires (line 576–840), the log line will incorrectly attribute the compaction run to the primary model, making it difficult to correlate actual execution with logged diagnostics.

Replace `provider` with `cand.provider` and `modelId` with `cand.model` in the diagnostic log template on line 731 to reflect the model actually being used.

Note: The same issue appears on line 785 in the end diagnostic log—update that as well.

How can I resolve this? If you propose a fix, please make it concise.

Fixed in commit fcc5f17 — both [compaction-diag] start and end log lines inside the outer loop now use cand.provider/cand.model. The fail() log before the loop intentionally keeps provider/modelId since no candidate is active at that point.

Silent failure when explicit fallbackModel string can't be resolved

If a user sets fallbackModel to a string that resolveModelRefFromString can't parse (e.g. a bare model name like "claude-haiku-4-5" that isn't in the alias index, or a typo like "anthropic/claude-haiku4-5"), resolved is null and the function silently returns []. The user gets no fallback and no diagnostic that their configuration is being ignored — they'll only notice when they hit a real quota event and nothing retries.

Consider emitting a log.warn here, or alternatively inside compact.ts after building allCandidates, when fallbackModel is configured but the candidates list ends up empty:

// Explicit "provider/model" string
const resolved = resolveModelRefFromString({
  raw: fallbackModel,
  defaultProvider,
  aliasIndex,
});
if (!resolved) {
  // Optionally: consumers should warn, but a note here aids debugging
  return [];
}
return !isCurrent(resolved.ref) ? [resolved.ref] : [];

At minimum, the test suite would benefit from an additional case — e.g. fallbackModel: "not-a-valid-model-string" — to document and pin this silent-no-op behaviour.

This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/compaction-fallback.ts
Line: 56-62

Comment:
**Silent failure when explicit `fallbackModel` string can't be resolved**

If a user sets `fallbackModel` to a string that `resolveModelRefFromString` can't parse (e.g. a bare model name like `"claude-haiku-4-5"` that isn't in the alias index, or a typo like `"anthropic/claude-haiku4-5"`), `resolved` is `null` and the function silently returns `[]`. The user gets no fallback and no diagnostic that their configuration is being ignored — they'll only notice when they hit a real quota event and nothing retries.

Consider emitting a `log.warn` here, or alternatively inside `compact.ts` after building `allCandidates`, when `fallbackModel` is configured but the candidates list ends up empty:

```ts
// Explicit "provider/model" string
const resolved = resolveModelRefFromString({
  raw: fallbackModel,
  defaultProvider,
  aliasIndex,
});
if (!resolved) {
  // Optionally: consumers should warn, but a note here aids debugging
  return [];
}
return !isCurrent(resolved.ref) ? [resolved.ref] : [];
```

At minimum, the test suite would benefit from an additional case — e.g. `fallbackModel: "not-a-valid-model-string"` — to document and pin this silent-no-op behaviour.

How can I resolve this? If you propose a fix, please make it concise.

Fixed in commit fce6f25 — added an optional warn callback to resolveCompactionFallbackCandidates. When an explicit string can't be resolved, it calls warn() with a message explaining the expected format. The call site in compact.ts passes log.warn so the misconfiguration is visible in the gateway log at startup.

Missing .describe() on fallbackModel schema field

The sibling thinking field (lines 87–100) includes a .describe(...) string that documents its semantics and allowed values. fallbackModel is validated as a plain z.string().optional() with no description, which means generated JSON Schema / config-validation output won't explain the three accepted values ("off", "fallback", or "provider/model"). For consistency and discoverability, add a .describe():

This is a comment left during a code review.
Path: src/config/zod-schema.agent-defaults.ts
Line: 109

Comment:
**Missing `.describe()` on `fallbackModel` schema field**

The sibling `thinking` field (lines 87–100) includes a `.describe(...)` string that documents its semantics and allowed values. `fallbackModel` is validated as a plain `z.string().optional()` with no description, which means generated JSON Schema / config-validation output won't explain the three accepted values (`"off"`, `"fallback"`, or `"provider/model"`). For consistency and discoverability, add a `.describe()`:

```suggestion
        fallbackModel: z.string().optional().describe(
          '"off" (default) disables fallback. "fallback" uses the agents.defaults.model.fallbacks chain. An explicit "provider/model" string targets a specific model.',
        ),
```

How can I resolve this? If you propose a fix, please make it concise.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Fixed in commit fce6f25 — added .describe() to fallbackModel documenting the three accepted values ("off", "fallback", "provider/model") and their semantics, consistent with the sibling thinking field.

Preserve authProfileId when resolving fallback model credentials

The primary compaction model resolves credentials with profileId: params.authProfileId, but the fallback path drops profileId when calling getApiKeyForModel. In sessions that rely on an auth-profile override (for example, provider-scoped profile keys), fallback candidates can be skipped or fail auth even though valid profile credentials exist, so the new quota fallback behavior silently does not work in those runs.

Useful? React with 👍 / 👎.

Accept explicit model refs in fallbackModel config schema

This schema only allows "off" | "fallback", but resolveCompactionFallbackCandidates includes an explicit "provider/model" branch (and tests for it), so users configuring an explicit fallback model will hit config validation errors instead of getting fallback behavior. That makes one supported fallback mode unreachable through normal validated config loading.

Useful? React with 👍 / 👎.

The explicit "provider/model" string handling at lines 58–70 is unreachable dead code. The TypeScript type (line 298 of types.agent-defaults.ts) and Zod schema (lines 104–105 of zod-schema.agent-defaults.ts) have been narrowed to only allow "off" | "fallback". Any explicit string like "openai/gpt-4o-mini" will be rejected at config-validation time and never reach this function.

The commit f077c9b5f ("narrow fallbackModel to off/fallback") narrowed the schema but did not remove this dead code path. The function documentation (lines 12–17) still mentions returning candidates "for 'fallback' or an explicit string", which is now misleading.

Either remove this dead branch (and the corresponding tests at compaction-fallback.test.ts:76–92), or widen the type/schema to actually support explicit strings if the feature is intended for future use.

This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/compaction-fallback.ts
Line: 58-70

Comment:
The explicit `"provider/model"` string handling at lines 58–70 is unreachable dead code. The TypeScript type (line 298 of `types.agent-defaults.ts`) and Zod schema (lines 104–105 of `zod-schema.agent-defaults.ts`) have been narrowed to only allow `"off" | "fallback"`. Any explicit string like `"openai/gpt-4o-mini"` will be rejected at config-validation time and never reach this function.

The commit `f077c9b5f` ("narrow fallbackModel to off/fallback") narrowed the schema but did not remove this dead code path. The function documentation (lines 12–17) still mentions returning candidates "for 'fallback' or an explicit string", which is now misleading. 

Either remove this dead branch (and the corresponding tests at compaction-fallback.test.ts:76–92), or widen the type/schema to actually support explicit strings if the feature is intended for future use.

How can I resolve this? If you propose a fix, please make it concise.

These test cases exercise an explicit "provider/model" string path that cannot be reached in production due to schema validation constraints. The Zod schema only allows "off" or "fallback", rejecting any explicit string before it reaches resolveCompactionFallbackCandidates().

These tests use as unknown as OpenClawConfig to bypass the type system and inject values the schema forbids. They pass at the unit level but give false confidence — there is no production path where a user's explicit string config survives validation and executes this code.

Remove these tests if the explicit-string option is not intended to be exposed, or widen the type/schema to make the feature actually available.

This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/compaction-fallback.test.ts
Line: 76-92

Comment:
These test cases exercise an explicit `"provider/model"` string path that cannot be reached in production due to schema validation constraints. The Zod schema only allows `"off"` or `"fallback"`, rejecting any explicit string before it reaches `resolveCompactionFallbackCandidates()`.

These tests use `as unknown as OpenClawConfig` to bypass the type system and inject values the schema forbids. They pass at the unit level but give false confidence — there is no production path where a user's explicit string config survives validation and executes this code.

Remove these tests if the explicit-string option is not intended to be exposed, or widen the type/schema to make the feature actually available.

How can I resolve this? If you propose a fix, please make it concise.

Exchange Copilot token before applying fallback credentials

In the fallback-candidate auth path, nextKeyInfo.apiKey is written directly to runtime auth storage, but GitHub Copilot requires exchanging the GitHub token for a short-lived Copilot API token first (the primary path in this same function does that). If a quota error triggers fallback to a github-copilot/* model, compaction will retry with the wrong credential type and fail authentication instead of recovering.

Useful? React with 👍 / 👎.

Rebuild provider-sanitized tools for each fallback candidate

The fallback loop reuses the same builtInTools/customTools for every candidate, but those tools are prepared once using the original provider. When fallback switches providers (notably to google-gemini-cli), Gemini-specific schema sanitization is not re-applied, so cross-provider fallback can fail with tool-schema validation errors instead of succeeding after a quota/rate-limit retry.

Useful? React with 👍 / 👎.

Increase lock budget for nested compaction retry loops

This new outer candidateIdx loop plus inner thinkAttempt loop allows a single compaction request to run multiple full compactWithSafetyTimeout calls (300s each), but the lock budget is still computed from a single timeout window (resolveSessionLockMaxHoldFromTimeout), so long fallback/thinking-retry runs can exceed the hold limit and trigger watchdog force-release while this function is still mutating the same session via SessionManager. In sessions that hit repeated timeouts or multiple quota fallbacks, that can permit concurrent writers on the same transcript and corrupt ordering/state; the lock hold time should be scaled to worst-case retries (or refreshed between attempts).

Useful? React with 👍 / 👎.