fix(agents): loosen abort settle env typing by vincentkoc · Pull Request #87750 · openclaw/openclaw

vincentkoc · 2026-05-28T18:20:23Z

Summary

narrow resolveEmbeddedAbortSettleTimeoutMs to the two env keys it actually reads
keep the existing process.env default and runtime timeout behavior unchanged
unblock check:changed generated/test type surfaces that reject full NodeJS.ProcessEnv as a required-key Pick
keep live-model-dynamic-candidates.test.ts focused on injected dynamic-model hooks instead of loading the real provider normalization/plugin runtime path

Verification

OPENCLAW_VITEST_FS_MODULE_CACHE_PATH=.artifacts/vitest-cache/abort-settle-env-final-$USER-$$ node scripts/run-vitest.mjs src/agents/embedded-agent-runner/run/attempt.abort-settle-timeout.test.ts
OPENCLAW_VITEST_FS_MODULE_CACHE_PATH=.artifacts/vitest-cache/live-dynamic-mocked-$USER-$$ node scripts/run-vitest.mjs run --config test/vitest/vitest.agents-core.config.ts src/agents/live-model-dynamic-candidates.test.ts --reporter=verbose
./node_modules/oxfmt/bin/oxfmt --check --threads=1 src/agents/live-model-dynamic-candidates.test.ts
node scripts/run-oxlint.mjs src/agents/live-model-dynamic-candidates.test.ts
git diff --check
.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main
AWS Crabbox run_8a485e593c2e, lease cbx_ab53fb060315, slug quick-crab, provider aws: corepack pnpm check:changed passed on amended head a05e30b1c8
clean ancestry rebase to 21e69fdd4f, followed by focused Vitest, format/lint/diff checks, and autoreview

Real behavior proof

Behavior addressed: check:changed failed in generated/test type surfaces because the helper accepted NodeJS.ProcessEnv, making the narrowed keys appear required at the boundary. The agentic-agents CI shard also timed out in live-model-dynamic-candidates.test.ts because a hook-focused unit test paid the real provider normalization/plugin-runtime load cost.

Real environment tested: focused local Codex worktree plus AWS Crabbox Linux runner.

Exact steps or command run after this patch: focused Vitest for the abort settle timeout helper, focused Vitest for live-model-dynamic-candidates.test.ts, local format/lint/diff checks, autoreview, and remote corepack pnpm check:changed.

Evidence after fix: abort-settle focused test passed 1 file / 5 tests in the earlier branch proof; live-model-dynamic-candidates.test.ts passed 1 file / 2 tests in 2.01s after the scoped mock; autoreview reported no actionable findings; AWS Crabbox run_8a485e593c2e exited 0.

Observed result after fix: the prior ProcessEnv assignability failure is gone while the helper still reads the same override and OPENCLAW_TEST_FAST fallback; the dynamic live model unit test no longer spends roughly 78s loading provider normalization/runtime plugins.

What was not tested: no live provider or runtime session smoke; this is a static env typing repair plus a test-only CI timeout fix.

clawsweeper · 2026-05-28T18:22:24Z

Codex review: needs changes before merge. Reviewed May 28, 2026, 3:18 PM ET / 19:18 UTC.

Summary
The PR narrows resolveEmbeddedAbortSettleTimeoutMs to the two env keys it reads and adds a focused agent-model-discovery mock in the dynamic live-model candidate test.

PR surface: Source +6, Tests +4. Total +10 across 2 files.

Reproducibility: yes. the PR body gives focused Vitest and changed-gate commands for the type/test failures, and source inspection confirms the affected helper and test path. I did not rerun those commands because this review is read-only.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

Refresh against current main and leave only one agent-model-discovery mock in src/agents/live-model-dynamic-candidates.test.ts.

Risk before merge

[P1] The current-main three-way merge result duplicates the agent-model-discovery mock in src/agents/live-model-dynamic-candidates.test.ts; this is small but should be cleaned up before merge to avoid stale test scaffolding.

Maintainer options:

Decide the mitigation before merge
Land the env typing fix after refreshing the branch so the dynamic live-model test keeps a single narrow mock and the abort-settle timeout behavior stays unchanged.
Pause or close
Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

[P2] A small mechanical PR-branch cleanup can remove the duplicate test mock after refreshing against current main; no product decision is needed.

Security
Cleared: The PR touches only an agent timeout helper and a unit test mock, with no dependency, workflow, credential, package, or code-execution surface change.

Review findings

[P3] Drop the duplicate discovery mock before merge — src/agents/live-model-dynamic-candidates.test.ts:11-13

Review details

Best possible solution:

Land the env typing fix after refreshing the branch so the dynamic live-model test keeps a single narrow mock and the abort-settle timeout behavior stays unchanged.

Do we have a high-confidence way to reproduce the issue?

Yes, the PR body gives focused Vitest and changed-gate commands for the type/test failures, and source inspection confirms the affected helper and test path. I did not rerun those commands because this review is read-only.

Is this the best way to solve the issue?

Mostly yes: narrowing the helper env type is the narrow maintainable fix and preserves runtime behavior. The test-side mock should be de-duplicated against current main before merge.

Full review comments:

[P3] Drop the duplicate discovery mock before merge — src/agents/live-model-dynamic-candidates.test.ts:11-13
If this merges with current main as simulated, this test ends up with two vi.mock("./agent-model-discovery.js", ...) blocks. Current main already has the mock, so refresh the branch or drop the new copy to keep one source of truth for the test setup.
Confidence: 0.87

Overall correctness: patch is correct
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 2a5a9fd7202d.

Label changes

Label changes:

add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
remove rating: 🦞 diamond lobster: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

P3: This is a small agent CI/type cleanup with a low-risk stale-test cleanup before merge.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): The PR body includes after-fix focused test output plus AWS Crabbox changed-gate proof for the static type/test behavior under review.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix focused test output plus AWS Crabbox changed-gate proof for the static type/test behavior under review.

Evidence reviewed

PR surface:

Source +6, Tests +4. Total +10 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	7	1	+6
Tests	1	4	0	+4
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	11	1	+10

Acceptance criteria:

[P2] node scripts/run-vitest.mjs src/agents/embedded-agent-runner/run/attempt.abort-settle-timeout.test.ts.
[P1] node scripts/run-vitest.mjs run --config test/vitest/vitest.agents-core.config.ts src/agents/live-model-dynamic-candidates.test.ts --reporter=verbose.
[P1] ./node_modules/oxfmt/bin/oxfmt --check --threads=1 src/agents/live-model-dynamic-candidates.test.ts.
[P1] node scripts/run-oxlint.mjs src/agents/live-model-dynamic-candidates.test.ts.
[P1] git diff --check.

What I checked:

Current source uses broad env typing: Current main still types resolveEmbeddedAbortSettleTimeoutMs as NodeJS.ProcessEnv while the function only reads OPENCLAW_EMBEDDED_ABORT_SETTLE_TIMEOUT_MS and OPENCLAW_TEST_FAST. (src/agents/embedded-agent-runner/run/attempt.abort-settle-timeout.ts:3, 2a5a9fd7202d)
PR merge ref keeps runtime behavior: The PR merge ref changes only the parameter type to a two-key optional env shape and leaves the same override and fast-test fallback logic intact. (src/agents/embedded-agent-runner/run/attempt.abort-settle-timeout.ts:3, e5824ae0af5a)
Current-main merge result duplicates a mock: A read-only merge-tree of current main and the PR head leaves two vi.mock("./agent-model-discovery.js", ...) blocks in the same test file. (src/agents/live-model-dynamic-candidates.test.ts:11)
Agent test policy applies: The scoped agents guide asks performance edits to avoid full provider/plugin runtime loads and prefer explicit mock factories, which matches the intended test-slimming direction. (src/agents/AGENTS.md:1)
Feature history provenance: The helper and dynamic live-model candidate test were introduced in commit 5b79ab0901686e385f71bd66175e5483919f9177, with the current-main mock carried by 43e243f4366e51b9a8626d4199da9f0453457c28. (src/agents/live-model-dynamic-candidates.test.ts:5, 43e243f4366e)
Contributor proof: The PR body records focused Vitest, format/lint/diff checks, autoreview, and AWS Crabbox corepack pnpm check:changed passing for the branch proof. (a05e30b1c859)

Likely related people:

Peter Steinberger: Git blame/log show Peter introduced the abort-settle helper and dynamic live-model candidate test, then recently added the current-main test mock that this PR now overlaps. (role: feature-history owner and recent area contributor; confidence: high; commits: 5b79ab090168, 21db3ff11c42, 43e243f4366e; files: src/agents/embedded-agent-runner/run/attempt.abort-settle-timeout.ts, src/agents/live-model-dynamic-candidates.test.ts, src/agents/live-model-dynamic-candidates.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.

vincentkoc self-assigned this May 28, 2026

openclaw-barnacle Bot added agents Agent runtime and tooling size: XS maintainer Maintainer-authored PR labels May 28, 2026

vincentkoc mentioned this pull request May 28, 2026

fix(agents): broaden local model lean profile #87617

Open

vincentkoc mentioned this pull request May 28, 2026

test(agents): add small model live profile #87638

Merged

vincentkoc marked this pull request as ready for review May 28, 2026 18:29

This was referenced May 28, 2026

fix(diagnostics): account stream deltas incrementally #87619

Closed

fix(agents): stream assistant deltas incrementally #87671

Merged

fix(agents): loosen abort settle env typing

a05e30b

vincentkoc force-pushed the fix/abort-settle-env-type branch from 8acccb5 to a05e30b Compare May 28, 2026 19:11

clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels May 28, 2026

vincentkoc merged commit 396a8ef into main May 28, 2026
185 of 190 checks passed

vincentkoc deleted the fix/abort-settle-env-type branch May 28, 2026 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(agents): loosen abort settle env typing#87750

fix(agents): loosen abort settle env typing#87750
vincentkoc merged 1 commit into
mainfrom
fix/abort-settle-env-type

vincentkoc commented May 28, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented May 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

vincentkoc commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Real behavior proof

Uh oh!

clawsweeper Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vincentkoc commented May 28, 2026 •

edited

Loading

clawsweeper Bot commented May 28, 2026 •

edited

Loading