fix(workboard): isolate stale lifecycle bulk patches#90067
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 3, 2026, 8:35 PM ET / 00:35 UTC. Summary PR surface: Source +14, Tests +43. Total +57 across 4 files. Reproducibility: yes. source-reproducible: current main reuses one bulk patch object across card ids while updateCard mutates that object for stale lifecycle writes. I did not execute the failing test path in this read-only review. 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:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Merge the focused patch after maintainer review confirms current-head validation, keeping lifecycle status precedence in WorkboardStore and migrate-hermes assertions aligned with the current auth-profile shape. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: current main reuses one bulk patch object across card ids while updateCard mutates that object for stale lifecycle writes. I did not execute the failing test path in this read-only review. Is this the best way to solve the issue? Yes, this is the narrow owner-boundary fix: local effectivePatch normalization prevents updateCard from mutating caller-owned input for both bulk reuse and direct callers, without adding a second API path. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 58f7d7e5f8ff. Label changesLabel justifications:
Evidence reviewedPR surface: Source +14, Tests +43. Total +57 across 4 files. View PR surface stats
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
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in the Workboard extension store where stale lifecycle-status suppression mutated the caller-provided patch object during updateCard, causing subsequent cards in a bulkUpdate to lose their intended status/provenance updates. The change introduces an effectivePatch derived from the original patch when stale lifecycle writes must be ignored, ensuring bulk updates remain isolated per-card without side effects.
Changes:
- Prevents
updateCardfrom mutating the inputpatchby applying stale-lifecycle normalization to a derivedeffectivePatch. - Ensures stale lifecycle status writes can be dropped while still allowing other (non-status) fields to apply.
- Adds a regression test proving a shared bulk patch remains unchanged and later cards still receive the lifecycle status/provenance update.
Best-fix verdict: best (fixes the root cause—unexpected mutation of an input object—at the point of mutation).
Alternatives considered:
- Clone the patch inside
bulkUpdatefor each id (would avoid the symptom but leavesupdateCardimpure and vulnerable to other call sites). - Freeze patches defensively (would turn the bug into runtime errors rather than preventing it).
Code read:extensions/workboard/src/store.ts(updateCard,bulkUpdate, stale-lifecycle helpers),extensions/workboard/src/store.test.ts(new regression coverage).
Remaining uncertainty: None material from the reviewed diff/context; the change is localized and covered by a targeted regression test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| extensions/workboard/src/store.ts | Introduces effectivePatch to avoid mutating caller patches while suppressing stale lifecycle status writes (fixes bulk update isolation). |
| extensions/workboard/src/store.test.ts | Adds regression test ensuring stale handling in bulkUpdate doesn’t strip later card updates and doesn’t mutate the shared patch object. |
7617f3e to
c8af37a
Compare
c8af37a to
5ee8b3d
Compare
Summary
effectivePatchso bulk updates do not let one stale card strip the status/provenance update for later cards.migrate-hermestest-only type narrowing because latestmainexposed that unrelatedcheck-test-typesfailure while refreshing this PR.Refs #89600
Refs #88592
Verification
node scripts/run-vitest.mjs ui/src/ui/controllers/workboard.test.ts ui/src/ui/views/workboard.test.ts extensions/workboard/src/store.test.ts extensions/workboard/src/gateway.test.ts --reporter=verbosenode scripts/run-vitest.mjs --config test/vitest/vitest.ui-e2e.config.ts --configLoader runner ui/src/ui/e2e/workboard-status-persistence.e2e.test.ts ui/src/ui/e2e/workboard.e2e.test.ts --reporter=verbosenode scripts/run-vitest.mjs extensions/migrate-hermes/secrets.test.ts src/infra/npm-managed-root.test.ts --reporter=verbosenode scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core-test.tsbuildinfonode scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.extensions.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/extensions-test.tsbuildinfogit diff --name-only origin/main...HEAD | xargs node scripts/run-oxlint.mjsgit diff --check origin/main...HEAD.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main --no-web-search