feat(deploy): harden deferred-cleanup queue — resolve-by-address drain, queue-aware stale-facet invariant, safe-to-prune report [EXSC-723] - #2157
Conversation
…n, queue-aware stale-facet invariant, safe-to-prune report [EXSC-723] Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughThe change adds address-based recovery for pruned deployment-log entries, a queue-aware stale-facet health invariant, environment-filtered parked-task queries, and reconciliation reporting for entries safe to prune. ChangesDeferred cleanup safeguards
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
script/deploy/safe/reconcile-parked-tasks.test.ts (1)
193-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse descriptive result variable names.
Rename repeated
rvariables toprunableEntriesso these tests follow the TypeScript naming guideline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/reconcile-parked-tasks.test.ts` around lines 193 - 255, Rename each repeated result variable `r` within the `computeSafeToPrune` tests to `prunableEntries`, updating the corresponding assertions while preserving all test behavior.Source: Coding guidelines
script/deploy/healthCheckInvariants.test.ts (1)
231-305: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftLGTM on the added
computeStaleRegisteredFacetsunit tests — verified against the implementation, all correct.Consider also covering
no-stale-registered-facets'srun()behavior (mocking the dynamic./safe/parked-tasksimport) for the queue-unreachable fallback and environment-scoped queries — this is exactly the code path with the environment-filter gap flagged inhealthCheckInvariants.ts, and it's currently untested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/healthCheckInvariants.test.ts` around lines 231 - 305, The tests cover computeStaleRegisteredFacets but not the no-stale-registered-facets invariant's run() behavior. Add run() tests that mock the dynamic ./safe/parked-tasks import, covering the queue-unreachable fallback and verifying environment-scoped queries use the correct production scope, including the environment-filter gap in healthCheckInvariants.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/deploy/healthCheckInvariants.ts`:
- Around line 1698-1705: The missing targetFacets condition in the stale-facet
check should record a non-fatal warning in IHealthCheckNetworkResult.warnings
instead of calling consola.info and returning silently. Update the surrounding
health-check result flow to append an actionable warning for the missing
targetState[network].production.LiFiDiamond, while preserving the check’s
non-failing skip behavior.
- Around line 1720-1741: The parked-task lookup used to build
openParkedFacetNames must be scoped to ctx.environment. Extend
IListParkedTasksFilter and listParkedTasks to accept an optional environment:
EnvironmentEnum filter, apply it to queued/proposed task queries, and pass
ctx.environment from the stale-facet check so tasks from other environments
cannot mask the required warning.
---
Nitpick comments:
In `@script/deploy/healthCheckInvariants.test.ts`:
- Around line 231-305: The tests cover computeStaleRegisteredFacets but not the
no-stale-registered-facets invariant's run() behavior. Add run() tests that mock
the dynamic ./safe/parked-tasks import, covering the queue-unreachable fallback
and verifying environment-scoped queries use the correct production scope,
including the environment-filter gap in healthCheckInvariants.ts.
In `@script/deploy/safe/reconcile-parked-tasks.test.ts`:
- Around line 193-255: Rename each repeated result variable `r` within the
`computeSafeToPrune` tests to `prunableEntries`, updating the corresponding
assertions while preserving all test behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 73ccab99-78e7-4b5f-b41f-df8fbe3f31d4
📒 Files selected for processing (9)
.agents/commands/deprecate-contract.mddocs/DeferredDiamondCleanupQueue.mdscript/deploy/healthCheckInvariants.test.tsscript/deploy/healthCheckInvariants.tsscript/deploy/safe/diamondRemovalDiff.test.tsscript/deploy/safe/diamondRemovalDiff.tsscript/deploy/safe/drain-parked-tasks.tsscript/deploy/safe/reconcile-parked-tasks.test.tsscript/deploy/safe/reconcile-parked-tasks.ts
An open staging task for a same-named facet could otherwise mask a production stale facet from the no-stale-registered-facets warning, and the drain's listQueued could pick up cross-environment rows despite minting production-only removals. Theoretical in v1 (enqueue rejects non-production), enforced at the query now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/deploy/healthCheckInvariants.ts (1)
377-382: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve ambiguous deploy-log address mappings.
Object.fromEntriesoverwrites earlier names when multiple deploy-log entries share the same lowercased address. This can hide a stale facet when the surviving name is expected, or report the wrong facet depending on insertion order. Treat duplicate address mappings as ambiguous/unresolved instead of selecting one arbitrarily, and add a regression test for two names mapping to the same address.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/healthCheckInvariants.ts` around lines 377 - 382, Update the nameByAddress construction in the deployedContracts mapping to detect duplicate lowercased addresses and mark them ambiguous/unresolved instead of allowing Object.fromEntries to overwrite an earlier name. Ensure downstream health-check logic does not select either name for an ambiguous address, and add a regression test covering two deploy-log names sharing one address.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@script/deploy/healthCheckInvariants.ts`:
- Around line 377-382: Update the nameByAddress construction in the
deployedContracts mapping to detect duplicate lowercased addresses and mark them
ambiguous/unresolved instead of allowing Object.fromEntries to overwrite an
earlier name. Ensure downstream health-check logic does not select either name
for an ambiguous address, and add a regression test covering two deploy-log
names sharing one address.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d72a70c-f4f2-4e6f-9d5a-849a4722ca52
📒 Files selected for processing (4)
script/deploy/healthCheckInvariants.tsscript/deploy/safe/drain-parked-tasks.tsscript/deploy/safe/parked-tasks.test.tsscript/deploy/safe/parked-tasks.ts
1 participant
Which Linear task belongs to this PR?
EXSC-723
Why did I implement it this way?
Backfilling the #2046 deprecations into the deferred-cleanup queue (112 tasks parked 2026-07-30) exposed three gaps in the parked-removal lifecycle. This PR closes them without touching governance (
ts-only; the minted removal proposal is byte-for-byte unchanged):Drain resolves a pruned deploy-log entry by stored address (
diffNamedFacetshint path, spec §14 Q5). "Remove every selector currently routed to the parked task's storedfacetAddress" is never stale by construction — selectors still come from the live loupe at drain time, so this does NOT reintroduce the frozen-calldata staleness class the design rejected. Resolution happens only when the hint is unambiguous: an address the log maps to a different name, or one claimed by two tasks, stays inprunedButRouted(keep + alert) exactly as before. The deploy log stops being load-bearing for the drain.Queue-aware stale-facet health-check invariant (
no-stale-registered-facets, warning, production-only, EVM + Tron). Today a deprecated-but-still-registered facet is invisible:facets-registeredonly flags missing facets,no-unexpected-facetsonly flags unknown addresses. The new invariant warns on any on-chain facet whose deploy-log name is absent from_targetState.jsonand has no open parked task — open tasks are expected-pending (no alarm flood for the 112 queued removals). Concrete instance it would have caught: GenericSwapFacet on tron (EXSC-724), which fell out of the queue because the enqueue CLI rejects base58. Mongo is only touched when the cheap pre-pass finds something stale; an unreachable queue degrades to a visible "coverage skipped" warning, never false alarms.MONGODB_URIis already in the health-check workflow env.Reconcile reports safe-to-prune deploy-log entries. Nothing pruned
deployments/*.jsonafter a removal executed. The weekly reconcile job now names entries whose parked work is fully terminal (≥1 executed/superseded, none open, entry still in the log; cancelled-only groups are never safe). Report-only — pruning stays a reviewed PR.Pure decisions (
computeStaleRegisteredFacets,computeSafeToPrune,formatSafeToPruneReport, thediffNamedFacetshint resolution) are fully unit-tested; live wiring follows the existing unit-test-exempt adapter convention. Executable check:reconcile-parked-tasks.ts --network arbitrumdry-run against the live queue (5 open tasks seen, allkeep, prune report empty as expected).Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)