Skip to content

fix(pr971): resolve CodeRabbit review follow-ups#1004

Merged
mswilkison merged 2 commits into
extraction/frost-mirror-2026-05-26from
fix/pr971-coderabbit-followups
Jul 2, 2026
Merged

fix(pr971): resolve CodeRabbit review follow-ups#1004
mswilkison merged 2 commits into
extraction/frost-mirror-2026-05-26from
fix/pr971-coderabbit-followups

Conversation

@mswilkison

@mswilkison mswilkison commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

This PR stacks on top of #971 (extraction/frost-mirror-2026-05-26) and addresses its open CodeRabbit review threads. It is based on the PR #971 head branch, not main.

CodeRabbit follow-ups

  • 1 & 2. CI action hardening. CodeRabbit findings 1 & 2 named ci-pr.yml and nightly-formal-invariants.yml, which do not exist in this repo. The real workflow PR feat(frost): mirror FROST/Schnorr extraction from tBTC monorepo #971 introduces is .github/workflows/services-watchtower.yml, and it had the exact issue those findings describe (unpinned actions, no persist-credentials). Hardened there instead:
    • Added with: persist-credentials: false to both actions/checkout steps.
    • Pinned every uses: to a full 40-hex commit SHA with a # vX.Y.Z comment (major versions unchanged):
      • actions/checkout@v3f43a0e5ff2bd294095638e18286ca9a3d1956744 # v3.6.0
      • dorny/paths-filter@v24512585405083f25c027a35db413c2b3b9006d50 # v2.11.1
      • pnpm/action-setup@v4b906affcce14559ad1aafd4ab0e942779e9f58b1 # v4.3.0 (annotated tag, pinned to peeled commit)
      • actions/setup-node@v33235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1
    • Scope limited to services-watchtower.yml (the workflow this PR introduces); pre-existing repo workflows are intentionally left untouched — repo-wide pinning is a separate effort.
  • 3. docs/rfc/frost-migration/audit-prep-findings-2026-05-25.md — canonical paths. Normalized the scope-summary roots (and the L32 plan-doc reference) to solidity/contracts/..., solidity/test/..., and docs/rfc/frost-migration/.
  • 4. docs/rfc/frost-migration/b1-implementation-plan.md — deploy-script filename. The on-disk script is solidity/deploy/48_deploy_frost_wallet_registry.ts; corrected the stale 46_ mention to 48_ (the other mention was already 48_).
  • 5. services/watchtower/src/FileBackedP2TRWatchtowerChallengeRecordPersistence.ts — validate on save. saveChallengeRecords now maps records through the existing normalizeSerializedRecord validator before JSON.stringify, mirroring the load path so invalid state can't be written. Atomic-write behavior is unchanged.
  • 6. solidity/contracts/bridge/BridgeGovernance.sol — forwarder NatSpec. Bridge.migrateLegacyFraudChallenges is stubbed to always revert MigrateLegacyFraudChallengesNotImplemented(). Updated the forwarder NatSpec to state it is intentionally non-operational and reverts until a future Bridge upgrade lands. Function retained.
  • 7. solidity/contracts/bridge/IBridgeLifecycleRouter.sol — interface docs vs. shipped ABI. The Bridge exposes a single frostLifecycleContext(walletPubKeyHash) view and does not expose separate lifecycleRouter / frostWalletRegistry / walletIDByWalletPubKeyHash getters. Updated the doc to point implementers at frostLifecycleContext(...) and to state those getters are absent. The one-shot setLifecycleRouter note was already present and accurate.
  • 8. services/watchtower/src/P2TRSignatureFraudWatchtowerLoop.ts — listener leak. abortableDelay now stores the handler in const onAbort and removes the abort listener in the timeout path too (signal?.removeEventListener("abort", onAbort)), keeping clearTimeout in the abort path. Fixes the leak of abort listeners across loop iterations.

Validation

  • Watchtower TS changes typecheck cleanly under Node 18 / TypeScript (isolated typecheck against the two changed files and their local deps).
  • Both changed TS files, both changed Markdown files, and services-watchtower.yml pass prettier@2.3.2 (@keep-network/prettier-config-keep, i.e. { semi: false }).
  • services-watchtower.yml parses as valid YAML (ruby -ryaml).
  • Solidity edits are NatSpec/comment-only; no executable code changed.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c9c6f693-e03d-46d2-8cd1-4403837370c6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pr971-coderabbit-followups

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

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

mswilkison and others added 2 commits July 2, 2026 10:47
…wer, docs, NatSpec)

Follow-ups on PR #971 (extraction/frost-mirror-2026-05-26). Per-item status:

1. .github/workflows/ci-pr.yml (SHA-pin actions + persist-credentials):
   SKIPPED — file does not exist in PR #971 or on branch
   extraction/frost-mirror-2026-05-26; nothing to pin.
2. .github/workflows/nightly-formal-invariants.yml (same treatment):
   SKIPPED — file does not exist in PR #971 or on the branch.
3. docs/rfc/frost-migration/audit-prep-findings-2026-05-25.md:
   FIXED — normalized non-canonical repo paths in the scope summary
   (and the L32 plan-doc reference) to the canonical layout
   (solidity/contracts/..., solidity/test/..., docs/rfc/frost-migration/).
4. docs/rfc/frost-migration/b1-implementation-plan.md:
   FIXED — deploy-script filename made consistent with on-disk
   solidity/deploy/48_deploy_frost_wallet_registry.ts (46_ -> 48_).
5. services/watchtower/src/FileBackedP2TRWatchtowerChallengeRecordPersistence.ts:
   FIXED — saveChallengeRecords now maps through the existing
   normalizeSerializedRecord validator before JSON.stringify, mirroring
   the load path; atomic-write behavior preserved.
6. solidity/contracts/bridge/BridgeGovernance.sol:
   FIXED — NatSpec on the migrateLegacyFraudChallenges forwarder now
   states the target Bridge helper is intentionally stubbed and always
   reverts (MigrateLegacyFraudChallengesNotImplemented) until a future
   upgrade; function retained.
7. solidity/contracts/bridge/IBridgeLifecycleRouter.sol:
   FIXED — interface docs aligned with the shipped Bridge ABI: point
   implementers to frostLifecycleContext(walletPubKeyHash); note the
   absence of separate lifecycleRouter/frostWalletRegistry/
   walletIDByWalletPubKeyHash getters. One-shot setLifecycleRouter note
   was already present and accurate.
8. services/watchtower/src/P2TRSignatureFraudWatchtowerLoop.ts:
   FIXED — abortableDelay stores the handler in const onAbort and removes
   the abort listener in the timeout path too, preventing listener leaks
   across loop iterations; clearTimeout kept in the abort path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sistence

Addresses CodeRabbit findings 1 & 2 on the real workflow file introduced
by PR #971 (services-watchtower.yml), rather than the non-existent
ci-pr.yml / nightly-formal-invariants.yml the findings named.

- Add `with: persist-credentials: false` to both `actions/checkout` steps.
- Pin every `uses:` to a full 40-hex commit SHA (major version unchanged):
  - actions/checkout@v3      -> f43a0e5ff2bd294095638e18286ca9a3d1956744 # v3.6.0
  - dorny/paths-filter@v2    -> 4512585405083f25c027a35db413c2b3b9006d50 # v2.11.1
  - pnpm/action-setup@v4     -> b906affcce14559ad1aafd4ab0e942779e9f58b1 # v4.3.0
  - actions/setup-node@v3    -> 3235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1

SHAs resolved via `git ls-remote` against each action repo's tags (the
annotated pnpm/action-setup tag pinned to its peeled commit).

Scope limited to services-watchtower.yml (the workflow this PR
introduces); pre-existing repo workflows are left untouched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mswilkison mswilkison force-pushed the fix/pr971-coderabbit-followups branch from e3976f6 to a60b49b Compare July 2, 2026 14:47
@mswilkison mswilkison merged commit 4b32995 into extraction/frost-mirror-2026-05-26 Jul 2, 2026
51 checks passed
@mswilkison mswilkison deleted the fix/pr971-coderabbit-followups branch July 2, 2026 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant