fix(agent): string model configs inherit default fallbacks for cron sessions#91373
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 8, 2026, 8:53 AM ET / 12:53 UTC. Summary PR surface: Source +26, Tests +99, Other +93. Total +218 across 5 files. Reproducibility: yes. Source inspection shows current main returns Review metrics: none identified. 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 findings
Review detailsBest possible solution: Land one cron-scoped fix that resolves the selected agent config for both explicit and implicit default-agent cron runs, gates inheritance to default/agent model sources only, and refreshes real proof from the corrected runtime path. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main returns Is this the best way to solve the issue? No. The PR is in the right owner boundary, but this implementation is incomplete for implicit default-agent jobs and stored session overrides; the safer fix is the cron-scoped signal shape covered by full runtime forwarding tests. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 2858c629bd0f. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +26, Tests +99, Other +93. Total +218 across 5 files. View PR surface stats
Acceptance criteria:
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
|
|
Addressed the P1 finding from ClawSweeper review.
Tests:
Real behavior proof script re-run and still passes. Commit: Ready for another look. |
|
Addressed the P1 finding from the latest ClawSweeper review:
Tests:
Commit: @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
… cron fallback resolution Compute the isDefaultPrimaryShorthand decision from raw runtime config before buildCronAgentDefaultsConfig rewrites defaults, and pass the explicit boolean into both preflight (resolveCronPreflightCandidates) and execution (resolveCronFallbacksOverride via executeCronRun). This prevents differing string agent models from accidentally inheriting global default fallbacks after defaults are rewritten in the cron path. - run.ts: compute signal before cfgWithAgentDefaults, thread through context - run-executor.ts: forward signal into resolveCronFallbacksOverride - run-fallback-policy.ts: use explicit signal instead of comparing rewritten configs - run-fallback-policy.test.ts: add rewritten-defaults strict regression test Refs: ClawSweeper P1 finding on PR openclaw#91373 Co-Authored-By: Claude Opus 4.7 <[email protected]>
d5111b6 to
5d14b9b
Compare
|
@clawsweeper re-review Addressed the P1 finding from the latest review:
Tests:RUN v4.1.7 /home/0668000666/0668000666/AI/OpenClaw/openclaw Test Files 1 passed (1)
|
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Closing in favor of the stronger sibling PR #91379. ClawSweeper's latest re-review identified two remaining P1 gaps in this implementation:
Rather than repairing this branch while competing with four other open PRs for #91362, it is more efficient to let #91379 (which already has fuller runtime coverage and the correct raw-config signal) proceed as the canonical fix. Thanks for the review feedback. |
Summary
Fixes #91362.
Keep shared
resolveSelectedModelFallbacksOverridestrict for string model configs (returns[]) and teach the isolated cron resolver to fall through toagents.defaults.model.fallbacksonly for the reported cron case.Changes
src/cron/isolated-agent/run-fallback-policy.ts:resolveCronFallbacksOverridenow returnsundefined(instead of an explicit empty override) when the agent model is a plain string and there is no payload or subagent fallback override. This letsresolveModelCandidateChainwalk the configured default fallbacks during cron preflight.src/agents/agent-scope.tsso non-cron paths (agent runs, subagent selection) keep their existing strict behavior.Verification
pnpm test src/agents/agent-scope.test.ts— 44 passedpnpm test src/cron/isolated-agent/run-fallback-policy.test.ts— 14 passednpx oxlinton changed files — cleanReal behavior proof
Behavior or issue addressed: Isolated cron sessions with a string agent model config (e.g.
"openai/gpt-5.4") were getting an empty fallback chain even whenagents.defaults.model.fallbackswas configured.Real environment tested: Local OpenClaw source checkout at commit
$(git rev-parse --short HEAD)on Node$(node --version), running the actual preflight resolver code (not a unit-test wrapper).Exact steps or command run after this patch:
pnpm exec tsx scripts/repro/91373-cron-fallback-proof.mjsScript content:
scripts/repro/91373-cron-fallback-proof.mjsEvidence after fix:
Observed result after fix: With the agent model set to a plain string
"openai/gpt-5.4"and default fallbacks configured,resolveCronPreflightCandidatesreturns a 3-entry candidate chain that walks through the defaults. The shared helper still returns[]for string models outside the cron path, preserving strict behavior for agent/subagent runs.What was not tested: A live end-to-end cron scheduler run against real providers; this proof exercises the actual preflight resolver code path that the scheduler uses.