feat(safe): fold parked facet removals into the primary proposal

[0xDEnYO]

Which Linear task belongs to this PR?

Fixes EXSC-721

Why did I implement it this way?

The deferred diamond-cleanup drain was minting a second Safe proposal for parked facet removals, so a rollout that already signed one proposal had to sign two — a full extra sign → timelock schedule → execute cycle. That defeated the queue's whole purpose (removals riding along "at ~zero marginal signing cost"): a separate Safe proposal is never zero-cost.

This folds the removals into the primary proposal's timelock scheduleBatch instead — one diamondCut Remove element per parked facet, appended to the primary's inner calls (N parked facets → the primary's call(s) + N removal elements). One Safe transaction, one signature. Each removal still carries its origin-PR link (parkedTaskRefs on the primary proposal), so confirm-safe-tx / list-pending-proposals / Slack show every folded cleanup inline for the reviewer.

Mechanics. The drain is restructured into two phases:

• prepareDrainNetwork — partition against the live loupe (gone → supersede, pruned-but-routed → keep + alert, protected → cancel + alert, removable → claim) and build one Remove call per claimed facet. Self-reverts every claim it made if preparation throws.
• proposeWithDrain — feeds those calls into _runPropose (which appends them to the scheduleBatch before signing and returns { safeTxHash, stored }), then links each claimed task to the primary's safeTxHash, or reverts them if the primary throws or was a duplicate.

Best-effort is preserved: a flag-off / non-timelock / direct-send / prep-failure path proposes the primary alone. The fold is gated to the timelock path because removals need a scheduleBatch to batch into. The fold happens purely in TypeScript at the runPropose layer (the primary's calldata is already TS there) — no removal logic threads into the Solidity deploy scripts, so the "language boundary" concern that originally motivated a separate proposal never applied to a shared scheduleBatch.

Design reversal — please note. This reverses the Q4 decision ("one extra proposal per network in the same session") that was co-signed by Goran and Daniel. @Goran — flagging you since you co-owned that call. Docs updated in both DeferredDiamondCleanupQueue.md §6 and FacetRemovalReconciliation.md §4.

Accepted tradeoff. Folding into one timelock op couples the removals to the upgrade: a reviewer who objects to a folded removal must reject the whole proposal (upgrade included), not just the removal. Acceptable because (a) each removal carries its origin-PR link for review, and (b) the DRAIN_PARKED_TASKS-off default keeps emergency / break-glass proposals a single clean upgrade with nothing folded in.

No Solidity changed; no new env var. tsc, eslint, and the full script/deploy/safe Bun suite (348 tests, incl. 27 rewritten drain tests) pass.

Checklist before requesting a review

• I have performed a self-review of my code
• This pull request is as small as possible and only tackles one problem
• I have added tests that cover the functionality / test the bug
• For new facets: I have checked all points from this list: https://www.notion.so/lifi/New-Facet-Contract-Checklist-157f0ff14ac78095a2b8f999d655622e
• I have updated any required documentation

Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)

• I have checked that any arbitrary calls to external contracts are validated and or restricted
• I have checked that any privileged calls (i.e. storage modifications) are validated and or restricted
• I have ensured that any new contracts have had AT A MINIMUM 1 preliminary audit conducted on by <company/auditor>

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Parked facet removals now flow through proposeWithDrain, which prepares and folds diamondCut(Remove) calls into the primary timelock proposal, records origin PR links, and revalidates removals before batch execution.

Changes

Parked removal proposal flow

Layer / File(s)	Summary
Removal preparation and orchestration script/deploy/safe/drain-parked-tasks.ts	prepareDrainNetwork classifies and claims tasks, builds removal calls, while proposeWithDrain coordinates eligibility, proposal creation, linking, rollback, and resilience handling.
Primary Safe transaction integration script/deploy/safe/propose-to-safe.ts, script/deploy/safe/parked-tasks.ts	The primary proposal accepts extra timelock calls and parkedTaskRefs, persists metadata, and returns structured stored or duplicate results.
Execution-time removal validation script/deploy/safe/diamondRemovalDiff.ts, script/deploy/safe/execute-pending-timelock-tx.ts	Remove payloads and parked-task identities reconstruct the propose-time snapshot; stale, mismatched, or unrecoverable removals fail the entire batch before executeBatch.
Validation and operator documentation script/deploy/safe/*test.ts, docs/*, .agents/commands/multisig-rollout.md	Tests cover preparation, folding, rollback, duplicates, failure resilience, snapshot reconstruction, and parked-task lookup; documentation describes folded batching, atomicity, and proposal metadata.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• lifinance/contracts#2047: Adds the facet-removal reconciliation flow extended by this change.
• lifinance/contracts#2051: Adds parked-task persistence operations used by this flow.
• lifinance/contracts#2076: Adds the deferred-cleanup drain behavior refactored here.

Suggested labels: QA AI Reviewing

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 76.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: folding parked facet removals into the primary proposal.
Description check	✅ Passed	The description matches the template with task, rationale, and checklists, and it covers the implementation and testing approach.

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🛠️ Fix failing CI checks 💡

• Fix failing CI checks

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch drain-merge-into-primary-proposal

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

script/deploy/safe/drain-parked-tasks.test.ts (1)

449-458: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider reusing the exported ref type instead of re-declaring its shape.

{ facet: string; prUrl: string }[] duplicates IParkedTaskRef from the module under test; importing it keeps the spy in lockstep if the ref shape changes.

🤖 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/drain-parked-tasks.test.ts` around lines 449 - 458, Update
the IPrimarySpy interface to import and reuse the exported IParkedTaskRef type
from the module under test for both the fn parameter and received refs, removing
the duplicated inline `{ facet: string; prUrl: string }[]` shape while
preserving the existing optionality.

🤖 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/safe/drain-parked-tasks.ts`:
- Around line 238-248: Update the rollback loops in prepareDrainNetwork’s catch,
proposeWithDrain’s proposePrimary catch, and finalizeClaimed’s duplicate branch
to isolate each deps.revert()/queue.deps.revert() call with per-task error
handling. Continue attempting all claimed tasks when one revert fails, while
preserving and rethrowing the original triggering error; report individual
revert failures through the existing alert/error mechanism without replacing the
primary failure.

---

Nitpick comments:
In `@script/deploy/safe/drain-parked-tasks.test.ts`:
- Around line 449-458: Update the IPrimarySpy interface to import and reuse the
exported IParkedTaskRef type from the module under test for both the fn
parameter and received refs, removing the duplicated inline `{ facet: string;
prUrl: string }[]` shape while preserving the existing optionality.

🪄 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: 27225f2f-d25c-43bc-a277-2b3825e29341

📥 Commits

Reviewing files that changed from the base of the PR and between fb5edec and d6c8380.

📒 Files selected for processing (5)

• docs/DeferredDiamondCleanupQueue.md
• docs/FacetRemovalReconciliation.md
• script/deploy/safe/drain-parked-tasks.test.ts
• script/deploy/safe/drain-parked-tasks.ts
• script/deploy/safe/propose-to-safe.ts

[0xDEnYO]

Pre-review gate summary (auto-fix trial)

Ran an in-session adversarial review (rule-adherence, bug + out-of-diff collaborators, git-history/prior-PR-comments, and a falsification pass), plus a re-gate of the fix commits. No critical bugs — the new eligibility/timelock guards and the diamondAddress assertion were all proven to fire correctly, and the fold produces a valid multi-element scheduleBatch.

Auto-fixed (already pushed):

• HIGH — the drain's finalizeClaimed wasn't best-effort: a Mongo error while linking a claimed task after the primary proposal was already stored crashed runPropose (non-zero exit) and stranded the task in proposed. Now wrapped best-effort with per-key resilient link/revert; a failed link leaves the task proposed for reconcile/loupe self-heal (never reverted — its removal already rode the stored proposal). Added a test.
• Operator-doc drift in .agents/commands/multisig-rollout.md (Phase 3.5/Phase 4): removed the stale "expect two or three proposals per network / separate removal proposal" guidance — the drain now folds in and adds no extra proposal.
• Hardened queue.close() best-effort; corrected a doc error-name (FunctionDoesNotExist); @throws tags + comment/header tidy-ups.

Escalated — needs a governance call (does NOT block, but @goran should weigh in):

• Execution-time atomicity (TOCTOU). Because the removals now share the primary cut's scheduleBatch, execution is atomic: if a folded facet is removed by another path during the timelock delay, its diamondCut Remove reverts (FunctionDoesNotExist) and the whole batch — including the primary rollout cut — reverts. Low probability (parked removals target already-deprecated facets), but strictly larger exposure than the old separate-proposal design. Documented as an accepted tradeoff in docs/DeferredDiamondCleanupQueue.md §6; the DRAIN_PARKED_TASKS-off default is the escape hatch. Mitigation if it ever bites: a separate scheduleBatch/MultiSend (one signature, two ops).

This PR also reverses the Q4 "one extra proposal per network" decision that @goran co-signed — docs updated in both DeferredDiamondCleanupQueue.md §6 and FacetRemovalReconciliation.md §4.

[0xDEnYO]

Fixed in aa82001 — routed prepareDrainNetwork's catch through the same revertQuietly helper. Note: the other two rollback paths you flagged (proposeWithDrain's proposePrimary catch, lines ~360-365, and finalizeClaimed's duplicate branch, ~391-398) were already converted to revertQuietly in an earlier commit (30908fd), so all three are now per-key isolated and preserve the original triggering error.

[coderabbitai]

Actionable comments posted: 1

🤖 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/safe/drain-parked-tasks.ts`:
- Around line 9-10: Clarify the best-effort guarantee: in
script/deploy/safe/drain-parked-tasks.ts lines 9-10, state that drain-only
failures are isolated only before proposal storage, while folded
diamondCut(Remove) operations execute atomically within the primary timelock
scheduleBatch and any on-chain removal failure can block the rollout. Add the
same execution-time atomicity warning beside the best-effort guidance in
.agents/commands/multisig-rollout.md lines 126-127.

🪄 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: a7f00460-2cae-4491-8143-c82a1a761046

📥 Commits

Reviewing files that changed from the base of the PR and between d6c8380 and aa82001.

📒 Files selected for processing (5)

• .agents/commands/multisig-rollout.md
• docs/DeferredDiamondCleanupQueue.md
• script/deploy/safe/drain-parked-tasks.test.ts
• script/deploy/safe/drain-parked-tasks.ts
• script/deploy/safe/propose-to-safe.ts

🚧 Files skipped from review as they are similar to previous changes (3)

• docs/DeferredDiamondCleanupQueue.md
• script/deploy/safe/propose-to-safe.ts
• script/deploy/safe/drain-parked-tasks.test.ts

[0xDEnYO]

Done in the latest push — qualified the best-effort guarantee in both the drain-parked-tasks.ts module header and the multisig-rollout.md Phase 3.5 note: it holds only until the proposal is stored; once folded, the removals execute atomically in the primary scheduleBatch, so an on-chain removal revert during the timelock window reverts the primary cut too. This matches the §6 TOCTOU tradeoff already documented.

[0xDEnYO]

Pre-review gate — residual findings (auto-fix trial, round 2 on 101ebe2/a004ffe74)

Adversarial review of the fold-into-primary change (rules, bugs incl. out-of-diff collaborators, git-history, prior-PR comments, comment-vs-code drift, falsification). The fold logic itself was proven executable on real repo data shapes: partition → claim → one diamondCut Remove per facet → single scheduleBatch (primary first, order preserved), plus negative controls for every gate/rollback path (8/8 probes pass; suite now 352 tests).

Escalated — needs your decision

E1. Non-owner process.exit(1) strands claimed parked tasks — propose-to-safe.ts:161-167. New exposure in this PR: prepareDrainNetwork flips tasks queued → proposed before _runPropose runs; the not-an-owner path then exits the process, bypassing proposeWithDrain's revert entirely. Tasks are left proposed with no safeTxHash and no proposal; reconcile-parked-tasks keeps them (present facet, no proposal status) until the 60d TTL alert. Note the new @throws JSDoc on _runPropose claims it throws on non-owner — it doesn't, it exits. Two valid fixes (throw instead of exit — changes behavior for every caller; or check ownership before the drain claims), so escalated per the governance-flow floor.

E2. The §4 "Mandatory pre-execute re-validation" contract is structurally unsatisfiable under the fold, and its silent failure mode is undocumented. revalidateRemovalsOnChain (shipped for exactly this in #2047) still has zero non-test callers, and diamondRemovalDiff.ts:686-688 still says the drain/execute consumer MUST call it before executing. Under the fold the removal shares the primary's immutable scheduleBatch, so "abort just the removal" is impossible. The new TOCTOU paragraphs cover only the revert case (facet already gone). They omit the case §4 was written for: a selector re-pointed to a new live facet during the 48h delay makes the folded Remove (facetAddress=0) succeed and silently delete the live selector. Options: a staleness check + abort in execute-pending-timelock-tx (possibly #2157 territory), or amend §4 + the docstring to the accepted-risk position. Docs are currently self-contradictory.

Lower-confidence — human judgment (60–79)

• (65) stored: false duplicate → revertToQueued can double-fold: with the time-salted intentHash, the realistic stored:false is a Mongo retry after a persisted first insert — the stored proposal already carries the removals, and re-queuing re-folds them into a future batch that then reverts at timelock execution. Same for post-store _runPropose failures (!result.acknowledged; mongoClient.close() throwing in finally — the drain guards its own close, the primary doesn't). Consider leave-proposed-and-alert on post-sign paths.
• (75) Fact 2 says the drain surfaces unresolved[] for investigation; prepareDrainNetwork ignores that bucket — such a task silently stays queued with no alert.
• (75) Spec §6 visibility item 3 promises origin-PR URLs in the rollout Slack thread; no Slack-path consumer of parkedTaskRefs exists (the runbook's claim was corrected in this push; the spec directive remains unimplemented).
• (60) Claims now span the interactive Ledger window; a kill mid-signing strands proposed/no-hash tasks (reconcile keeps them until TTL).
• (60) drain-parked-tasks.ts module header exceeds the 1–3-sentence rule (200-typescript) — left as-is: the TOCTOU qualification was added at CodeRabbit's request.
• (60) The _runPropose extras-without-timelock guard has no test (module is untestable — top-level runMain); verified by reading + probe replication.
• Fact-ledger line refs to propose-to-safe.ts (:58/:257/:356/:375) are stale after this PR; left alone since the provenance note pins them to an origin/main snapshot.

Auto-fixed in this push

• drain-parked-tasks.test.ts: +3 tests covering the previously-uncovered error branches (queue-close throw, revert-throw during primary failure, post-store bookkeeping throw) — only the live adapter remains uncovered, as the header claims.
• Stale two-proposal-era comments aligned with the fold design: parked-tasks.ts (header "no safeTxHash writer is exposed yet", setSafeTxHash/revertToQueued "minted proposal", phantom §6.3 refs), safe-utils.ts ("drain-minted/created" ×3), list-pending-proposals.ts, confirm-safe-tx.ts, multisig-rollout.md (removed the not-yet-real "Phase 8 Slack post" surfacing claim), one-char diagram alignment in the spec.

Escalated items need Daniel's decision before this PR is ready for human review.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

script/deploy/safe/diamondRemovalDiff.test.ts (1)

556-619: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider covering the "cuts empty, parked non-empty" mismatch branch.

buildRemovalSnapshotFromPayloads also returns { kind: 'mismatch', reason: 'parked tasks present ... but no Remove diamondCut payloads found' } when removeCuts.length === 0 and parked.length > 0 — a distinct mismatch reason from the one already tested (parked > cuts). This branch isn't exercised in this suite.

♻️ Suggested additional test

+  it('signals mismatch when parked tasks exist but no Remove cuts are found', () => {
+    const built = buildRemovalSnapshotFromPayloads(
+      [],
+      [{ facetName: 'A', facetAddress: addr(2) }]
+    )
+    expect(built.kind).toBe('mismatch')
+  })

🤖 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/diamondRemovalDiff.test.ts` around lines 556 - 619, Add a
test in the buildRemovalSnapshotFromPayloads suite covering non-empty parked
identities with no Remove diamondCut payloads. Assert the result is kind
"mismatch" and validates the branch-specific reason indicating parked tasks
exist without any Remove payloads, distinct from the existing
parked-greater-than-cuts mismatch test.

🤖 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/safe/diamondRemovalDiff.ts`:
- Around line 753-759: Remove the local DIAMOND_CUT_ABI definition in
diamondRemovalDiff.ts and import and reuse the shared DIAMOND_CUT_ABI exported
by script/deploy/shared/constants.ts. Keep the existing decodeFunctionData usage
unchanged.

---

Nitpick comments:
In `@script/deploy/safe/diamondRemovalDiff.test.ts`:
- Around line 556-619: Add a test in the buildRemovalSnapshotFromPayloads suite
covering non-empty parked identities with no Remove diamondCut payloads. Assert
the result is kind "mismatch" and validates the branch-specific reason
indicating parked tasks exist without any Remove payloads, distinct from the
existing parked-greater-than-cuts mismatch test.

🪄 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: fceced16-db15-4549-b407-78d8605b6d32

📥 Commits

Reviewing files that changed from the base of the PR and between aa82001 and 8cfba17.

📒 Files selected for processing (14)

• .agents/commands/multisig-rollout.md
• docs/DeferredDiamondCleanupQueue.md
• docs/FacetRemovalReconciliation.md
• script/deploy/safe/confirm-safe-tx.ts
• script/deploy/safe/diamondRemovalDiff.test.ts
• script/deploy/safe/diamondRemovalDiff.ts
• script/deploy/safe/drain-parked-tasks.test.ts
• script/deploy/safe/drain-parked-tasks.ts
• script/deploy/safe/execute-pending-timelock-tx.ts
• script/deploy/safe/list-pending-proposals.ts
• script/deploy/safe/parked-tasks.test.ts
• script/deploy/safe/parked-tasks.ts
• script/deploy/safe/propose-to-safe.ts
• script/deploy/safe/safe-utils.ts

🚧 Files skipped from review as they are similar to previous changes (4)

• docs/FacetRemovalReconciliation.md
• .agents/commands/multisig-rollout.md
• script/deploy/safe/drain-parked-tasks.ts
• docs/DeferredDiamondCleanupQueue.md

[0xDEnYO]

Pre-review gate — residual findings (auto-fix trial, round 3 on 8cfba17 / follow-up)

Adversarial review of the E1/E2 fix commits (non-owner throw + pre-execute revalidateRemovalsOnChain wiring). E1 verified clean end-to-end (throw → proposeWithDrain revert → citty runMain exit 1). E2 happy-path wiring proven on real buildDiamondCutRemoveCalldata shapes + trailing-N zip; stale/mismatch abort paths refuse the whole batch.

Escalated — needs your decision

E3. unvalidated warn+proceed can skip the guard for real folded removals. When Remove cuts exist but listParkedTasksBySafeTxHash returns [] (link never stamped: swallowed finalizeClaimed / setSafeTxHash null / crash between store and link), the guard treats the batch as legacy cleanUpProdDiamond and proceeds without revalidation — reopening the silent live-selector delete. Options: (a) abort whenever Remove cuts are present and parked rows are missing (closes the hole; also blocks legacy cleanup until it parks or carries a snapshot), or (b) keep warn+proceed for intentional legacy and accept the unlink TOCTOU (document + maybe alert harder / fix link reliability separately).

E4. Trailing-N zip order is fail-safe but fragile. Snapshot zip trusts proposedAt (+ taskKey tiebreak) equals drain append order. Same-ms proposedAt ties / unsorted listQueued can mis-label cuts → false re-pointed stale → abort whole batch (not silent delete). Options: (a) stamp an explicit appendIndex (or store selectors) at claim time for order-independent zip, or (b) accept rare false aborts + cancel/re-propose.

Lower-confidence — human judgment (60–79)

• (70) No unit tests for revalidateFoldedRemovalsOrAbort orchestration (~200 LOC) — pure helpers covered; wiring regressions would be silent. Extract+inject deps to test, or accept script-private glue.
• (65) Duplicate diamondCut ABI parse vs private ABI_DIAMOND_CUT in safe-decode-utils (export-and-reuse vs leave).
• (60) Operator-facing TOCTOU comments in drain-parked-tasks.ts header / multisig-rollout.md still omit the re-point + pre-execute abort story (docs §6 updated; those two lag).
• (60) Spec still claims Phase 8 Slack parkedTaskRefs surfacing (round-2 residual; unimplemented).
• Round-2 leftovers still open: Mongo stored:false double-fold; unresolved[] ignored; claims spanning Ledger window.

Auto-fixed in this round

• E1: _runPropose throws on non-owner (first fix commit).
• E2: wire revalidateRemovalsOnChain into executeOperation + docs §4/§6 + FacetRemovalReconciliation §5.1.
• Transient Mongo/loupe failures: refuse execute but leave queue row queued (only mark failed on durable stale/mismatch).
• Dry-run: notifyFailure gated behind !isDryRun.
• Loupe read uses runner publicClient so --rpcUrl is honoured.
• RemovalSnapshotBuild rename, JSDoc on new exports, taskKey sort tiebreak, mismatch test for parked-without-Removes.

Escalated items need your decision before this PR is ready for the #dev-sc-review ping (Q4 reversal + E2 tradeoff still to flag to Goran once clear).

[coderabbitai]

🧹 Nitpick comments (1)

script/deploy/safe/execute-pending-timelock-tx.ts (1)

1136-1139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move FACETS_LOUPE_ABI to the shared loupe ABI export.

diamondRemovalDiff.ts already defines the same facets() ABI as FACETS_LOUPE_ABI, but keeps it unexported for local use. Export that shared constant and import it here to avoid maintaining two identical parseAbi(...) declarations.

🤖 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/execute-pending-timelock-tx.ts` around lines 1136 - 1139,
Export the existing facets() ABI constant from diamondRemovalDiff.ts, then
import and reuse that shared symbol in execute-pending-timelock-tx.ts. Remove
the local FACETS_LOUPE_ABI parseAbi declaration while preserving the existing
pre-execute revalidation behavior.

Source: Path instructions

🤖 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.

Nitpick comments:
In `@script/deploy/safe/execute-pending-timelock-tx.ts`:
- Around line 1136-1139: Export the existing facets() ABI constant from
diamondRemovalDiff.ts, then import and reuse that shared symbol in
execute-pending-timelock-tx.ts. Remove the local FACETS_LOUPE_ABI parseAbi
declaration while preserving the existing pre-execute revalidation behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0aee76a7-596c-438d-b622-8b61de2f96a8

📥 Commits

Reviewing files that changed from the base of the PR and between 8cfba17 and f832701.

📒 Files selected for processing (4)

• script/deploy/safe/diamondRemovalDiff.test.ts
• script/deploy/safe/diamondRemovalDiff.ts
• script/deploy/safe/execute-pending-timelock-tx.ts
• script/deploy/safe/parked-tasks.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• script/deploy/safe/diamondRemovalDiff.test.ts

[coderabbitai]

Actionable comments posted: 1

🤖 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/safe/execute-pending-timelock-tx.ts`:
- Around line 1208-1222: Update the operation validation flow before the early
return around the safeTxHash check: decode and inspect payloads first, and
permit missing safeTxHash only when built.removeCutCount is zero. When a
hashless operation contains Remove cuts, mark it failed via
markTimelockOpFailed, alert the failure, and return 'failed'; preserve normal
processing for operations with a safeTxHash.

🪄 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: a43471a6-89d6-4eb9-ba83-ea67a850a7e5

📥 Commits

Reviewing files that changed from the base of the PR and between f832701 and f39fdaf.

📒 Files selected for processing (4)

• docs/DeferredDiamondCleanupQueue.md
• script/deploy/safe/diamondRemovalDiff.test.ts
• script/deploy/safe/diamondRemovalDiff.ts
• script/deploy/safe/execute-pending-timelock-tx.ts

🚧 Files skipped from review as they are similar to previous changes (3)

• script/deploy/safe/diamondRemovalDiff.test.ts
• script/deploy/safe/diamondRemovalDiff.ts
• docs/DeferredDiamondCleanupQueue.md

[0xDEnYO]

Pre-review gate — residual findings (round 4 on f39fdaf / 923fec7)

E3=a / E4=b applied. Focused gate on the fail-closed unvalidated path: CLEAN.

Auto-fixed

• Remove cuts with no parked rows for the Safe tx hash → abort + mark failed (closes unlink / legacy-cleanup hole).
• Docs §4/§6 updated (fail closed + accepted rare same-ms zip false abort).
• CodeRabbit: reuse exported ABI_DIAMOND_CUT from safe-decode-utils (no duplicate parseAbi).
• Linear section: Fixes [EXSC-721](...).

No further escalations from this round. Lower-confidence leftovers (orchestration unit tests, Slack parkedTaskRefs surfacing, Mongo stored:false double-fold, unresolved[] alert) remain follow-ups — not blocking peer review.