Skip to content

test(update): isolate campaign RPC ledger state - #139116

Merged
steipete merged 1 commit into
mainfrom
codex/update-campaign-test-isolation
Sep 5, 2026
Merged

test(update): isolate campaign RPC ledger state#139116
steipete merged 1 commit into
mainfrom
codex/update-campaign-test-isolation

Conversation

@steipete

@steipete steipete commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Resolves a problem where full release verification fails the Gateway update-status test after campaign RPC tests have run in the same worker.

Why This Change Was Made

The campaign fixture invokes the real update-run ledger but did not own an isolated state directory. An active campaign-1 row survived into the later status test. Reuse the existing temporary-home lifecycle at the producing fixture, as the other update RPC suites already do, and await cleanup before restoring the environment.

User Impact

No product behavior changes. Release verification retains its strict status assertions without sharing campaign records between tests.

Evidence

  • Original ordered campaign/status reproduction: 35 passed, 1 failed with the same leaked active/last run as full release CI.
  • Same command after repair: all 36 passed. Related update, owner, campaign, effective-channel, and status suites: all 89 passed.
  • Full changed-file gate and independent review through P2 passed.
  • Existing assertions unchanged; no added production seam. Production delta: 0. Test fixture delta: +11/-1.
  • Relevant fixture, ledger, and cleanup owners are byte-identical between the release reproduction and this main-based patch.
node scripts/run-vitest.mjs src/gateway/server-methods/update-run-campaign.test.ts src/gateway/server-methods/update-effective-channel.test.ts --maxWorkers=1 --no-file-parallelism

@clawsweeper

clawsweeper Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: XS maintainer Maintainer-authored PR labels Sep 5, 2026
@steipete
test(update): isolate campaign RPC ledger state
1f1c1ab 
Give each campaign test an owned temporary state home and await cleanup,
matching the other update RPC fixtures. This prevents active campaign rows
from leaking into later status tests in the full release CI shard.
@steipete
steipete force-pushed the codex/update-campaign-test-isolation branch from f27009c to 1f1c1ab Compare September 5, 2026 13:50
@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 9:56 AM ET / 13:56 UTC.

ClawSweeper review

What this changes

Give each update-campaign RPC test its own temporary state directory and await cleanup so ledger records cannot leak into later tests.

Merge readiness

✅ Ready for maintainer review

Keep open: this is a focused repair for a source-supported test-state leak that remains on current main. No blocking correctness findings were found.

Priority: P2
Reviewed head: 1f1c1abc4793098115c5323d3c71e60223b28a82

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused fixture repair using an established lifecycle, with relevant reported validation and no blocking findings.
Proof confidence 🌊 off-meta tidepool Not applicable: This maintainer-authored fixture-only PR changes no production behavior; the reported ordered 36-test run and related 89-test run support the isolation repair.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This maintainer-authored fixture-only PR changes no production behavior; the reported ordered 36-test run and related 89-test run support the isolation repair.
Evidence reviewed 8 items Verified introduced change: The pinned base-to-head diff adds temporary-home acquisition and awaited restoration only; existing assertions and production files are unchanged.
Producer and failing consumer: The fixture invokes the real update.run handler, which creates a ledger row at update.ts:163 and deliberately leaves successful or handed-off runs nonterminal at lines 621–623. update-status.ts:68–69 reads those rows, conflicting with the empty-ledger assertion at update-effective-channel.test.ts:157 when tests share state.
Existing cleanup owner: The helper creates a distinct state directory, captures and restores environment selectors, and awaits cleanup before removing the home. The cleanup owner drains writers and closes the fixture's shared database by its exact path.
Findings None None.
Security None None.

How this fits together

Gateway update RPCs record update progress in a SQLite ledger, which status RPCs subsequently read. These tests exercise that real ledger while mocking package updates and restarts.

flowchart TD
  A[Campaign test] --> B[Temporary state home]
  B --> C[Gateway update RPC]
  C --> D[Update ledger]
  D --> E[Await database cleanup]
  E --> F[Restore previous environment]
  F --> G[Later status test]
Loading

Before merge

None.

Agent review details

Security

None.

PR surface

Tests +10. Total +10 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 1 11 1 +10
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 11 1 +10

Review metrics

Metric Value Why it matters
Production versus test growth Production: 0; tests/test support: +10 net All growth establishes fixture ownership without adding a production seam or weakening assertions.

Technical review

Best possible solution:

Keep isolation at the campaign fixture through the existing temporary-home lifecycle, preserving real ledger coverage and strict status assertions.

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

Yes: the campaign-to-effective-channel sequence shares persistent ledger state on main, and the PR supplies an ordered failing/passing reproduction. This review verified the mechanism without executing tests.

Is this the best way to solve the issue?

Yes: the producing fixture owns the leaked state, and sibling suites already use this lifecycle; mocking the ledger or relaxing status assertions would reduce coverage.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: Test-created ledger rows contaminate subsequent status checks and undermine release verification, with no product-runtime change.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This maintainer-authored fixture-only PR changes no production behavior; the reported ordered 36-test run and related 89-test run support the isolation repair.

Label justifications:

  • P2: Test-created ledger rows contaminate subsequent status checks and undermine release verification, with no product-runtime change.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This maintainer-authored fixture-only PR changes no production behavior; the reported ordered 36-test run and related 89-test run support the isolation repair.

Evidence

What I checked:

  • Verified introduced change: The pinned base-to-head diff adds temporary-home acquisition and awaited restoration only; existing assertions and production files are unchanged. (src/gateway/server-methods/update-run-campaign.test.ts:13, 1f1c1abc4793)
  • Producer and failing consumer: The fixture invokes the real update.run handler, which creates a ledger row at update.ts:163 and deliberately leaves successful or handed-off runs nonterminal at lines 621–623. update-status.ts:68–69 reads those rows, conflicting with the empty-ledger assertion at update-effective-channel.test.ts:157 when tests share state. (src/gateway/server-methods/update.ts:621, 1f1c1abc4793)
  • Existing cleanup owner: The helper creates a distinct state directory, captures and restores environment selectors, and awaits cleanup before removing the home. The cleanup owner drains writers and closes the fixture's shared database by its exact path. (src/test-utils/temp-home.ts:77, 1f1c1abc4793)
  • Sibling coverage and established pattern: The shared update RPC harness already uses the same acquisition/restoration lifecycle. Status tests retain real persisted-history assertions, and temp-home tests cover environment restoration, directory removal, and failed acquisition. (src/gateway/server-methods/update.test-harness.ts:8, 1f1c1abc4793)
  • Current-main necessity: Comparing the campaign fixture, update handlers, ledger, temporary-home helper, and cleanup owner against fetched main shows only this PR's fixture-isolation change; main still lacks the repair. (src/gateway/server-methods/update-run-campaign.test.ts:11, 44378c46bb04)
  • Release boundary: The supplied latest release contains the same unisolated campaign fixture but no update-run-ledger.ts module. This review therefore does not claim a shipped product regression. (src/gateway/server-methods/update-run-campaign.test.ts:11, ad6fe23aecb9)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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
steipete merged commit df78d38 into main Sep 5, 2026
100 checks passed
@steipete
steipete deleted the codex/update-campaign-test-isolation branch September 5, 2026 14:02
@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

@github-actions github-actions Bot mentioned this pull request Sep 5, 2026
Open
This was referenced Sep 5, 2026
Merged
Merged
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 6, 2026
@steipete
test(update): isolate campaign RPC ledger state (openclaw#139116)
d575bf9 
Give each campaign test an owned temporary state home and await cleanup,
matching the other update RPC fixtures. This prevents active campaign rows
from leaking into later status tests in the full release CI shard.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

gateway Gateway runtime maintainer Maintainer-authored PR P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete