Skip to content

fix(agent): string model configs inherit default fallbacks for cron sessions#91373

Closed
wangmiao0668000666 wants to merge 1 commit into
openclaw:mainfrom
wangmiao0668000666:fix/91362-cron-inherit-default-fallbacks
Closed

fix(agent): string model configs inherit default fallbacks for cron sessions#91373
wangmiao0668000666 wants to merge 1 commit into
openclaw:mainfrom
wangmiao0668000666:fix/91362-cron-inherit-default-fallbacks

Conversation

@wangmiao0668000666

@wangmiao0668000666 wangmiao0668000666 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #91362.

Keep shared resolveSelectedModelFallbacksOverride strict for string model configs (returns []) and teach the isolated cron resolver to fall through to agents.defaults.model.fallbacks only for the reported cron case.

Changes

  • src/cron/isolated-agent/run-fallback-policy.ts: resolveCronFallbacksOverride now returns undefined (instead of an explicit empty override) when the agent model is a plain string and there is no payload or subagent fallback override. This lets resolveModelCandidateChain walk the configured default fallbacks during cron preflight.
  • Reverted the broad change to src/agents/agent-scope.ts so non-cron paths (agent runs, subagent selection) keep their existing strict behavior.

Verification

  • pnpm test src/agents/agent-scope.test.ts — 44 passed
  • pnpm test src/cron/isolated-agent/run-fallback-policy.test.ts — 14 passed
  • npx oxlint on changed files — clean

Real 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 when agents.defaults.model.fallbacks was 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.mjs

Script content: scripts/repro/91373-cron-fallback-proof.mjs

Evidence after fix:

=== PR #91373 Cron fallback inheritance proof ===

Config shape:
  agents.defaults.model.fallbacks: ["anthropic/claude-sonnet-4-6", "google/gemini-3-pro"]
  agents.list[0].model: "openai/gpt-5.4" (string, no explicit fallbacks)

resolveCronFallbacksOverride result: undefined (will fall through to defaults)

resolveCronPreflightCandidates result:
  - openai/gpt-5.4
  - anthropic/claude-sonnet-4-6
  - google/gemini-3.1-pro-preview

Verification:
  Primary present:     true
  Anthropic fallback present: true
  Google fallback present:    true
  Total candidates:    3

PASS: fallback chain inherits defaults.

Observed result after fix: With the agent model set to a plain string "openai/gpt-5.4" and default fallbacks configured, resolveCronPreflightCandidates returns 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.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S labels Jun 8, 2026
@openclaw-barnacle openclaw-barnacle Bot added triage: blank-template Candidate: PR template appears mostly untouched. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 8, 2026
@clawsweeper

clawsweeper Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 8, 2026, 8:53 AM ET / 12:53 UTC.

Summary
The branch adds a cron-local shorthand signal so matching string agent model configs can inherit agents.defaults.model.fallbacks, threads it through isolated cron preflight/execution, and adds fallback-policy tests plus a repro script.

PR surface: Source +26, Tests +99, Other +93. Total +218 across 5 files.

Reproducibility: yes. Source inspection shows current main returns [] for string agent model configs, blocking default fallbacks in cron fallback resolution; I did not run tests because this review is constrained to read-only inspection.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Fix default-agent cron inheritance by resolving the selected agent config before computing the raw-config signal.
  • Gate inheritance by the final cron model source so stored session overrides remain strict.
  • Refresh the repro script and PR-body proof after the code changes.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR includes copied live output, but the committed proof script omits the new shorthand argument and would not reproduce that output for the current patch; the contributor needs to rerun corrected real behavior proof with private details redacted. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Merging this revision can leave ordinary default-agent cron jobs unfixed when they omit agentId, which is a common cron shape and matches the reported config pattern.
  • [P1] The new inheritance flag can also make stored cron session model overrides inherit global fallbacks, changing provider/auth/billing behavior for a session-selected model.
  • [P1] The posted real behavior proof is stale because the proof script omits the new shorthand signal, so maintainers do not have reliable after-fix proof for this head.

Maintainer options:

  1. Fix the cron signal before merge (recommended)
    Resolve the selected agent before computing the shorthand signal, gate it by the final model source, add the missing runtime regressions, and refresh the proof output.
  2. Use the stronger sibling PR as canonical
    If maintainers prefer the related open PR's fuller runtime coverage, pause this PR and avoid merging both variants.

Next step before merge

  • [P1] Contributor action is needed because the branch has correctness blockers and the real behavior proof must be rerun from the contributor's corrected setup.

Security
Cleared: No concrete security or supply-chain issue was found; the meaningful safety concern is provider/auth routing compatibility, captured as merge risk.

Review findings

  • [P1] Resolve the default agent before checking shorthand inheritance — src/cron/isolated-agent/run.ts:544-547
  • [P1] Keep session-selected cron models strict — src/cron/isolated-agent/run.ts:670
  • [P2] Pass the shorthand signal in the repro script — scripts/repro/91373-cron-fallback-proof.mjs:47
Review details

Best 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 [] for string agent model configs, blocking default fallbacks in cron fallback resolution; I did not run tests because this review is constrained to read-only inspection.

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:

  • [P1] Resolve the default agent before checking shorthand inheritance — src/cron/isolated-agent/run.ts:544-547
    agentConfigOverride is only populated when the cron input or job has an explicit agentId. For jobs that rely on the default agent, rawAgentModel stays undefined, so the new flag is false and the reported string-agent/default-fallback case still returns the strict empty fallback chain. Resolve the selected agentId config first, then decide whether its raw string model matches the default primary.
    Confidence: 0.91
  • [P1] Keep session-selected cron models strict — src/cron/isolated-agent/run.ts:670
    This flag is computed before resolveCronModelSelection can choose a stored session override. If an existing cron session has modelSource: "session" and the agent string happens to match the default primary, resolveCronFallbacksOverride returns undefined and allows global fallbacks for a session-selected model. Gate inheritance on the final model source so only default/agent shorthand runs inherit fallbacks.
    Confidence: 0.87
  • [P2] Pass the shorthand signal in the repro script — scripts/repro/91373-cron-fallback-proof.mjs:47
    The added proof script calls resolveCronFallbacksOverride and resolveCronPreflightCandidates without isDefaultPrimaryShorthand: true. With this patch, that exercises the strict string-model path and will not produce the PASS output shown in the PR body, so the proof needs to be updated and rerun after the code fix.
    Confidence: 0.95

Overall correctness: patch is incorrect
Overall confidence: 0.89

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 2858c629bd0f.

Label changes

Label changes:

  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR includes copied live output, but the committed proof script omits the new shorthand argument and would not reproduce that output for the current patch; the contributor needs to rerun corrected real behavior proof with private details redacted. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove proof: sufficient: Current real behavior proof status is insufficient, not sufficient.
  • remove status: ⏳ waiting on author: Current PR status label is status: 📣 needs proof.

Label justifications:

  • P2: The PR targets a real isolated-cron fallback bug with limited but meaningful provider-routing impact.
  • merge-risk: 🚨 compatibility: The diff changes cron model fallback inheritance semantics and must preserve strict string model and session override behavior for existing setups.
  • merge-risk: 🚨 auth-provider: Inherited fallbacks can move cron execution to different providers and auth profiles when the primary model is unavailable.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR includes copied live output, but the committed proof script omits the new shorthand argument and would not reproduce that output for the current patch; the contributor needs to rerun corrected real behavior proof with private details redacted. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +26, Tests +99, Other +93. Total +218 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 3 27 1 +26
Tests 1 99 0 +99
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 1 93 0 +93
Total 5 219 1 +218

Acceptance criteria:

  • [P2] node scripts/run-vitest.mjs src/cron/isolated-agent/run-fallback-policy.test.ts src/agents/agent-scope.test.ts src/cron/isolated-agent/run.cron-model-override-forwarding.test.ts.
  • [P1] pnpm tsgo:core:test.
  • [P2] pnpm exec tsx scripts/repro/91373-cron-fallback-proof.mjs.

What I checked:

Likely related people:

  • @849261680: Authored recent current-main isolated cron work and also opened the related same-issue PR with fuller runtime-path coverage. (role: recent area contributor; confidence: high; commits: 4780546c124d, 4562a3850bf0; files: src/cron/isolated-agent/run.ts, src/cron/isolated-agent/run.cron-model-override-forwarding.test.ts)
  • Jason Yao: Current blame for the relevant cron preparation and agent fallback helper code points to this author, so they are a useful routing candidate for fallback contract review. (role: current fallback baseline contributor; confidence: medium; commits: d1cb6cd0b58c; files: src/cron/isolated-agent/run.ts, src/cron/isolated-agent/run-fallback-policy.ts, src/agents/agent-scope.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 8, 2026
@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 8, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 8, 2026
@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts and removed agents Agent runtime and tooling labels Jun 8, 2026
@wangmiao0668000666

Copy link
Copy Markdown
Contributor Author

Addressed the P1 finding from ClawSweeper review.

  • Reverted the broad change to resolveSelectedModelFallbacksOverride — string model configs once again return [] (strict) outside the cron path.
  • Moved the inheritance behavior into resolveCronFallbacksOverride only: when the agent model is a plain string and there is no explicit payload or subagent fallback override, the isolated cron resolver returns undefined, letting the preflight candidate chain walk agents.defaults.model.fallbacks.

Tests:

  • pnpm test src/agents/agent-scope.test.ts — 44 passed
  • pnpm test src/cron/isolated-agent/run-fallback-policy.test.ts — 14 passed

Real behavior proof script re-run and still passes. Commit: 679559c177.

Ready for another look.

@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 8, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 8, 2026
@wangmiao0668000666

Copy link
Copy Markdown
Contributor Author

Addressed the P1 finding from the latest ClawSweeper review:

  • src/cron/isolated-agent/run-fallback-policy.ts now checks that the agent's string model matches the configured default primary (resolvePrimaryStringValue) before returning undefined. Differing string agent models (e.g. "google/gemini-3-pro" when the default primary is "openai/gpt-5.4") fall through to resolveEffectiveModelFallbacks and remain strict.
  • Added regression test keeps differing string agent models strict in cron preflight to lock this behavior.

Tests:

  • pnpm test src/cron/isolated-agent/run-fallback-policy.test.ts — 15 passed
  • pnpm test src/agents/agent-scope.test.ts — 44 passed

Commit: d5111b6542.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 8, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 8, 2026
… 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]>
@wangmiao0668000666
wangmiao0668000666 force-pushed the fix/91362-cron-inherit-default-fallbacks branch from d5111b6 to 5d14b9b Compare June 8, 2026 12:38
@wangmiao0668000666

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Addressed the P1 finding from the latest review:

  • is now computed from raw runtime config in before rewrites defaults.
  • The explicit boolean is threaded through into both preflight () and execution ( via ).
  • This ensures differing string agent models stay strict even after defaults are rewritten, because the signal captures raw-config provenance.
  • Added regression test that simulates the exact rewritten-defaults scenario.

Tests:

RUN v4.1.7 /home/0668000666/0668000666/AI/OpenClaw/openclaw

Test Files 1 passed (1)
Tests 16 passed (16)
Start at 20:38:25
Duration 13.83s (transform 9.28s, setup 1.00s, import 10.59s, tests 1.92s, environment 0ms) — 16 passed

RUN v4.1.7 /home/0668000666/0668000666/AI/OpenClaw/openclaw

Test Files 1 passed (1)
Tests 44 passed (44)
Start at 20:38:49
Duration 1.58s (transform 1.49s, setup 796ms, import 283ms, tests 244ms, environment 0ms) — 44 passed

RUN v4.1.7 /home/0668000666/0668000666/AI/OpenClaw/openclaw

Test Files 1 passed (1)
Tests 10 passed (10)
Start at 20:39:00
Duration 9.53s (transform 3.67s, setup 452ms, import 4.79s, tests 4.03s, environment 0ms) — 10 passed

  • Scope: all 153 workspace projects
    Lockfile is up to date, resolution step is skipped
    Progress: resolved 0, reused 1, downloaded 0, added 0
    Progress: resolved 0, reused 99, downloaded 0, added 0
    Packages: -172

Progress: resolved 0, reused 172, downloaded 0, added 0, done

. preinstall$ node scripts/preinstall-package-manager-warning.mjs
. preinstall: Done
. postinstall$ node scripts/postinstall-bundled-plugins.mjs
. postinstall: Done
. prepare$ node scripts/prepare-git-hooks.mjs
. prepare: Done
Done in 4.9s using pnpm v11.2.2 — pass

Also rebased onto latest main to resolve the merge conflict.

Commit: .

@clawsweeper

clawsweeper Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 8, 2026
@wangmiao0668000666

Copy link
Copy Markdown
Contributor Author

Closing in favor of the stronger sibling PR #91379.

ClawSweeper's latest re-review identified two remaining P1 gaps in this implementation:

  1. Default-agent cron jobs (without explicit agentId) are not covered by the shorthand inheritance signal.
  2. Session-selected cron models may incorrectly inherit global fallbacks.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. scripts Repository scripts size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: blank-template Candidate: PR template appears mostly untouched.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(agent): agents.defaults.model.fallbacks not inherited by isolated cron sessions

1 participant