Skip to content

chore: speed cron model override coverage#106938

Merged
steipete merged 1 commit into
mainfrom
codex/test-speed-cron-model-overrides
Jul 14, 2026
Merged

chore: speed cron model override coverage#106938
steipete merged 1 commit into
mainfrom
codex/test-speed-cron-model-overrides

Conversation

@steipete

Copy link
Copy Markdown
Contributor

What Problem This Solves

The cron model override shard repeated selection and forwarding coverage through a broad temp-home integration suite. That file took 7.58 seconds with warm transforms and 21.32 seconds on a cold Testbox run.

Why This Change Was Made

This removes the overlapping integration file and keeps its distinct contracts at their owner boundaries: Gmail hook precedence in the model-selection suite and default-thinking propagation in the run-forwarding suite. Existing focused tests already cover whitespace, payload/session precedence, invalid refs, catalog reasoning policy, and provider-authoritative stale-catalog behavior.

User Impact

No runtime or user-visible behavior changes. CI executes less duplicate test work while retaining the behavior coverage.

Evidence

  • Baseline Testbox run 29294849108: removed suite 9/9, 21.32 seconds cold; a warm rerun measured 7.58 seconds.
  • Exact-base Testbox run 29295998167: replacement owner suites 58/58, 5.58 seconds cold.
  • pnpm check:changed passed core-test types, formatting, lint, and static guards for the unchanged patch before the final rebase; exact-base focused tests reran after the rebase.
  • Fresh exact-base autoreview: no accepted/actionable findings; correctness 0.86.
  • Test-only change; no docs or changelog required.

@openclaw-barnacle openclaw-barnacle Bot added size: M maintainer Maintainer-authored PR labels Jul 14, 2026
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jul 14, 2026
@clawsweeper

clawsweeper Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 13, 2026, 8:38 PM ET / July 14, 2026, 00:38 UTC.

Summary
Consolidates cron model-selection and thinking-forwarding coverage into focused owner suites while removing the slower overlapping temp-home integration suite.

PR surface: Tests -241. Total -241 across 3 files.

Reproducibility: not applicable. This PR addresses duplicated test execution rather than a runtime defect, and the performance comparison is documented with Testbox runs.

Review metrics: 1 noteworthy metric.

  • Cold test runtime: 21.32s broad suite vs 5.58s focused owner suites. The reported exact-base measurements show a substantial CI-time reduction while retaining the identified distinct contracts.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🌊 off-meta tidepool
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

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

Risk before merge

  • [P1] The branch removes a broad integration suite, so landing should wait for exact-head focused tests and static checks to finish against the final three-way merge result.

Maintainer options:

  1. Decide the mitigation before merge
    Land the focused owner-boundary assertions and remove the duplicate temp-home suite once exact-head CI confirms every distinct contract remains covered.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] No automated repair is needed; complete exact-head CI and normal maintainer landing review for the final merge result.

Security
Cleared: The test-only diff changes no runtime, dependency, workflow, permissions, secrets, package resolution, or supply-chain execution surface.

Review details

Best possible solution:

Land the focused owner-boundary assertions and remove the duplicate temp-home suite once exact-head CI confirms every distinct contract remains covered.

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

Not applicable: this PR addresses duplicated test execution rather than a runtime defect, and the performance comparison is documented with Testbox runs.

Is this the best way to solve the issue?

Yes. Moving distinct assertions to the model-selection and forwarding owner suites, then deleting the overlapping integration harness, is cleaner than retaining duplicate end-to-end-style coverage.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against cae29a079203.

Label changes

Label changes:

  • add P3: This is a low-risk, test-only CI performance cleanup with no intended runtime or user-visible behavior change.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Real user-behavior proof is not applicable to this maintainer-labeled, behavior-neutral test consolidation; exact-base Testbox timing and focused-suite results are the appropriate evidence.

Label justifications:

  • P3: This is a low-risk, test-only CI performance cleanup with no intended runtime or user-visible behavior change.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Real user-behavior proof is not applicable to this maintainer-labeled, behavior-neutral test consolidation; exact-base Testbox timing and focused-suite results are the appropriate evidence.
Evidence reviewed

PR surface:

Tests -241. Total -241 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 3 59 300 -241
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 59 300 -241

What I checked:

Likely related people:

  • tyler6204: Repository contributor guidance assigns cron ownership to Tyler Yust, making this the clearest routing candidate for test-boundary decisions in this subsystem. (role: cron area owner; confidence: high; files: src/cron/isolated-agent/model-selection.ts, src/cron/isolated-agent/run.ts)
  • vincentkoc: Authored merged isolated cron runner refactoring, including commit a81dc15 on the central execution path covered by these tests. (role: isolated-run refactor contributor; confidence: high; commits: a81dc153c61b; files: src/cron/isolated-agent/run.ts, src/cron/isolated-agent/run.types.ts)
  • steipete: Has prior merged cron service and performance work and is connected to the current test-runtime consolidation beyond merely opening this PR. (role: recent adjacent cron and performance contributor; confidence: medium; commits: 7877182b6f59, e3dd80f9d4f4, b6be42230672; files: src/cron/service/ops.ts, src/cron/service-contract.ts, src/cron/isolated-agent.model-overrides.test.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.

@steipete

Copy link
Copy Markdown
Contributor Author

Land-ready proof:

@steipete
steipete merged commit 0934f56 into main Jul 14, 2026
141 of 147 checks passed
@steipete
steipete deleted the codex/test-speed-cron-model-overrides branch July 14, 2026 00:47
@steipete

Copy link
Copy Markdown
Contributor Author

Merged via squash.

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

Labels

maintainer Maintainer-authored PR P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant