fix(safeguard): resolve compaction provider/model before registering runtime#73704
fix(safeguard): resolve compaction provider/model before registering runtime#73704joeykrug wants to merge 30 commits into
Conversation
Greptile SummaryThis PR fixes the safeguard auto-compaction path so that
Confidence Score: 3/5Safe to merge for correctly configured setups, but misconfigured compaction model overrides can silently cancel compaction in the compact.ts path rather than falling back gracefully. One P1 finding: when the configured compaction model override is not found in the model registry, resolveSafeguardRuntimeTarget returns model: undefined with no warning. In the compact.ts workflow (ctx.model = undefined), this causes compaction to cancel entirely — a regression from the pre-PR behavior of using the session model. src/agents/pi-embedded-runner/extensions.ts — resolveSafeguardRuntimeTarget needs a warning log (and ideally a fallback) when the registry lookup fails for a non-empty override. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/pi-embedded-runner/extensions.ts
Line: 165-169
Comment:
**Silent `undefined` runtime model when registry lookup misses**
When a compaction model override is configured and the provider/model combination differs from the session model, the function falls through to `params.modelRegistry?.find(provider, modelId)`. If `modelRegistry` is absent or the model isn't registered (e.g. a typo in `agents.defaults.compaction.model`, or a freshly-added model that hasn't been imported yet), the lookup returns `null`/`undefined`, and `model: undefined` is silently stored as `runtime.model`.
Downstream in `compaction-safeguard.ts:915`, `runtime?.model ?? ctx.model` evaluates to `ctx.model`. In the `compact.ts` path `ctx.model` is always `undefined`, so compaction will cancel with `{ cancel: true }` and the only log emitted is the generic "could not resolve a summarization model" message — giving the user no indication that their configured override was silently dropped.
Consider logging a warning here when the registry lookup fails and there is an explicit override:
```typescript
const model = params.modelRegistry?.find(provider, modelId) as
| ProviderRuntimeModel
| null
| undefined;
if (model == null) {
log.warn(
`[compaction-safeguard] Configured compaction model "${provider}/${modelId}" was not found in the model registry. Falling back to session model.`,
);
}
return { provider, modelId, model: model ?? undefined };
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(safeguard): resolve compaction provi..." | Re-trigger Greptile Re-review progress:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Codex review: needs real behavior proof before merge. Reviewed July 12, 2026, 10:00 PM ET / July 13, 2026, 02:00 UTC. Summary PR surface: Source +495, Tests +506. Total +1001 across 9 files. Reproducibility: yes. from source: current Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Use one shared route-aware preparation path for manual and safeguard compaction, retain the isolated target registry and regression coverage, and require redacted live Gateway proof that an automatic cross-provider safeguard compaction uses the configured target without altering the active session credential. Do we have a high-confidence way to reproduce the issue? Yes from source: current Is this the best way to solve the issue? Yes, the proposed pre-registration preparation and isolated model/auth tuple address the owner boundary more safely than changing model precedence alone. The implementation still needs real provider-backed automatic-compaction proof before merge. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 58e84401903b. Label changesLabel justifications:
Evidence reviewedPR surface: Source +495, Tests +506. Total +1001 across 9 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (7 earlier review cycles)
|
1736c23 to
d12930f
Compare
… registry Greptile P1 feedback on PR openclaw#73704: when agents.defaults.compaction.model is configured with a provider/modelId that isn't registered, resolveSafeguardRuntimeTarget silently returns model: undefined. In compact.ts paths where ctx.model is also undefined, this causes compaction to silently cancel with only a generic log. Now we log a clear warning naming the failed provider/modelId, so misconfigured overrides surface instead of being dropped silently.
… registry Greptile P1 feedback on PR openclaw#73704: when agents.defaults.compaction.model is configured with a provider/modelId that isn't registered, resolveSafeguardRuntimeTarget silently returns model: undefined. In compact.ts paths where ctx.model is also undefined, this causes compaction to silently cancel with only a generic log. Now we log a clear warning naming the failed provider/modelId, so misconfigured overrides surface instead of being dropped silently.
0e82fa2 to
4b2bdc5
Compare
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
…runtime Fixes openclaw#57901. Previously `buildEmbeddedExtensionFactories` populated the safeguard runtime with the active session model and the safeguard hook preferred ctx.model over runtime?.model, so users configuring `agents.defaults.compaction.model` saw no effect for safeguard auto-compaction (only manual /compact and context-engine paths honored it). This change shares the compaction target resolution between the explicit compaction runtime context and the safeguard runtime, then swaps the precedence in the safeguard hook so the configured compaction model wins when present, falling back to the session model when no override is set. Adds a regression where `ctx.model` is the session model and `agents.defaults.compaction.model` is different — confirms the configured model is used. Co-authored-by: Joey Krug <[email protected]>
… registry Greptile P1 feedback on PR openclaw#73704: when agents.defaults.compaction.model is configured with a provider/modelId that isn't registered, resolveSafeguardRuntimeTarget silently returns model: undefined. In compact.ts paths where ctx.model is also undefined, this causes compaction to silently cancel with only a generic log. Now we log a clear warning naming the failed provider/modelId, so misconfigured overrides surface instead of being dropped silently.
4b2bdc5 to
cc66444
Compare
openclaw-barnacle
Bot
added
proof: supplied
openclaw-barnacle
Bot
added
triage: needs-real-behavior-proof
|
Any update? |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
clawsweeper
Bot
added
the
status: ⏳ waiting on author
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
added
the
proof: sufficient
…odelRegistry type
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
removed
the
rating: 🦐 gold shrimp
Dependency graph guard clearedThis PR no longer has blocked dependency graph changes. A future dependency graph change requires a fresh
|
# Conflicts: # extensions/slack/src/monitor/send.runtime.ts # src/agents/embedded-agent-runner/extensions.ts # src/agents/embedded-agent-runner/run/attempt.ts # ui/src/ui/views/workboard.ts
Resolve the configured safeguard compaction target before runtime registration, preserve native-harness model locks, and warn while retaining the active session model when an override cannot be resolved. Trim PR-only proof and unrelated collateral files while preserving the synchronized main tree.
|
@clawsweeper re-review Pushed the synchronized refresh in
Exact-cutoff validation: production and test TypeScript projects passed; 4 focused Vitest files / 154 tests passed; six-file formatting, targeted oxlint, and |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
CI triage for exact head
A maintainer or automation identity with Actions write permission will need to click Re-run failed jobs to clear the stale TUI timeout; no compaction-path code change is indicated by the failure. |
The prior exact-head CI run had one unrelated TUI PTY session-adoption timeout. The same test passes locally on the synchronized tree (8/8); this empty same-tree commit retriggers Actions without changing the patch.
|
Exact-head CI retrigger is now This is an empty same-tree commit: tree The PR body now has the required @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review Addressed the exact-head P1 in
Exact-head local validation: core and core-test TypeScript projects passed; 8 compaction-focused files / 464 tests passed; 12 run-attempt support files / 190 tests passed; targeted formatting/lint and |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Reconcile safeguard compaction target resolution with the route-aware runtime auth planner. Preserve the prepared model route, runtime policy, transport overrides, and credential tuple in an isolated target registry so compaction overrides cannot mutate active-session auth.
|
@clawsweeper re-review Exact head The prepared safeguard target now carries one model/route/profile/key tuple in a dedicated target Exact local tree |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@joeykrug thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward. Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive. Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it. |
2 participants
Fixes #57901.
What Problem This Solves
Normal safeguard auto-compaction still registers the active session model even when
agents.defaults.compaction.modelselects a different summarization target. Main's new route-aware auth planner fixed explicit compaction, but normal safeguard registration still ignored the override.A safeguard override also needs its model route and credential to remain one atomic tuple. Binding a target credential into the active attempt registry can corrupt same-provider sessions before compaction starts.
Summary
This refresh is merged with main at
266d49cddb977c45d40f13d27045214db29bc756and adapts the safeguard override to the current route-aware runtime planner.prepareAgentRuntimeAuth→materializePreparedRuntimeModel→resolvePreparedRuntimeAuthAttempts→resolvePreparedRuntimeModelAuth.AuthStorage/ModelRegistry; never mutate the active attempt's auth registry.Safety Invariants
buildEmbeddedExtensionFactoriesremains synchronous by receiving a precomputed target.Evidence
Validated on exact local tree
b38eded0065419c75b3b1f369b96d7359c0cb239:9f2ac5d5ac9e17d20e9ec7be684f82157b4b3327ecfc4491ece04b29ccb48c42ecb6a40d6e0d8a37and main266d49cddb977c45d40f13d27045214db29bc756oxfmt --check, type-awareoxlint --deny-warnings, andgit diff --check— passedA direct compact-hooks subset currently reproduces 9/102 failures on an unmodified
266d49cworktree as well as the merge tree; the conflicted compact implementation and harness are byte-identical to main. No unrelated main fix is included here. Fresh exact-head GitHub CI is running.Testing Scope
No live provider API call was made. Exact-head automated coverage proves route/auth tuple integrity and active-registry isolation. A real provider-backed automatic cross-provider safeguard compaction remains an external maintainer/environment proof gate.