feat(migration): auto-refresh DAPI nodes during pre-1.0 migration - #908
Conversation
Keep test migration runs offline, detach automatic discovery from storage recovery, and retry after transient legacy-data read failures. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com>
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughChangesDAPI refresh migration flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Migration
participant DAPIRefresh
participant Discovery
participant Config
participant Sentinel
Migration->>DAPIRefresh: Start refresh concurrently
DAPIRefresh->>Discovery: Detect legacy state and discover nodes
Discovery-->>DAPIRefresh: Return DAPI addresses
DAPIRefresh->>Config: Persist disk and live configuration
DAPIRefresh->>Sentinel: Record completion
DAPIRefresh-->>Migration: Complete without changing migration result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Claudius-Maginificent
added
the
claudius-review
|
⛔ Blockers found — Sonnet deferred (commit 1acf308) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/user-stories.md`:
- Around line 1264-1266: Update docs/user-stories.md lines 1264-1266 to state
that startup attempts the Mainnet or Testnet address refresh and successful
completion occurs once, without guaranteeing immediate success. Update
CHANGELOG.md lines 11-14 to describe fetch, persistence, and reconnection as
best-effort operations that may retry on later launches.
In `@src/backend_task/migration/finish_unwire.rs`:
- Around line 405-407: Update refresh_dapi_nodes_once_with to acquire and hold
the shared configuration-persistence lock across loading, mutating, saving,
updating live state, and recording completion. Reuse the existing lock used by
ordinary settings saves, ensuring the lock spans the full refresh sequence so
concurrent saves cannot overwrite discovered addresses before completion is
recorded.
- Around line 432-444: Add an AppContext-scoped one-flight guard around the
automatic DAPI refresh lifecycle in the sentinel-check path and the
corresponding detached refresh flow near the later retry logic. Acquire the
guard before checking the MigrationCompletion sentinel and retain it through
discovery, save, reinitialization, and sentinel write; return or reuse the
existing in-flight task when another migration run attempts the refresh.
- Around line 446-465: Move the unsupported-network guard in the migration flow
before the detect_legacy call, so Devnet and Regtest immediately invoke
write_dapi_refresh_completion with status 0 and return. Keep legacy inspection
and its retry-on-error behavior only for Network::Mainnet and Network::Testnet.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22df125b-0414-4b53-ba43-5e2dc56dd0de
📒 Files selected for processing (3)
CHANGELOG.mddocs/user-stories.mdsrc/backend_task/migration/finish_unwire.rs
Guard detached refreshes with the migration mutex, complete unsupported networks before legacy detection, and clarify best-effort retry semantics. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The automatic refresh is correctly isolated from wallet recovery, but its detached sentinel-to-save lifecycle is not synchronized with other refreshes or ordinary configuration saves. This creates a blocking risk that a successful refresh is overwritten while its completion sentinel permanently prevents another automatic attempt; two smaller documentation and unsupported-network issues are also confirmed.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/backend_task/migration/finish_unwire.rs`:
- [BLOCKING] src/backend_task/migration/finish_unwire.rs:405-407: Serialize the sentinel and configuration update lifecycle
The detached refresh is not protected from the sentinel check through completion, and its whole-file `Config::load_from`/mutate/`save` sequence is not synchronized with other configuration writers. A migration retry can start a second refresh before the first writes its sentinel, and switching networks can start another per-network refresh because each `AppContext` has its own migration lock. Mainnet, Testnet, and the manual refresh path all persist to the same `.env` file, so overlapping writers can load the same old snapshot and have the last save discard another network's successful update. Both automatic tasks can then record completion, leaving stale addresses on disk with no future automatic retry. Add a one-flight guard covering the automatic sentinel check, discovery, live update, reinitialization, and sentinel write, plus an app-global configuration-persistence guard used by every load-mutate-save path, including the manual settings refresh.
Round-3's migration_run guard only serializes DAPI refresh retries within a single AppContext. thepastaclaw and CodeRabbit both flagged (BLOCKING) that this doesn't protect two different network contexts (e.g. Mainnet + Testnet, which can coexist in one process) racing on the same shared .env file via Config::load_from/save. Add a process-wide CONFIG_PERSISTENCE_LOCK (plain std::sync::Mutex, no .await held across it) in config.rs, and acquire it around the load->mutate->save->live-update span in both the migration refresh path (finish_unwire.rs) and the manual "Refresh DAPI endpoints" button handler (network_chooser_screen.rs) — the only two writers of this file. New test dapi_config_persistence_serializes_across_networks proves actual serialization (not just lock presence) using real OS threads: a Mainnet section pauses mid-critical-section while a Testnet section attempts to enter, and the test asserts the second section cannot enter or save until the first releases, then confirms both networks' addresses land correctly on disk with no clobbering. Co-Authored-By: Codex Sol <noreply@openai.com>
There was a problem hiding this comment.
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 `@src/ui/network_chooser_screen.rs`:
- Around line 1476-1509: Move the CONFIG_PERSISTENCE_LOCK acquisition and
Config::load_from/update_config_for_network/save sequence out of
display_task_result and into the BackendTask handling DAPI discovery, returning
an explicit success or failure result to the UI. Update the UI path around
config_loaded to dispatch the task result without performing persistence, and
show a user-facing MessageBanner with sanitized text plus technical details via
BannerHandle::with_details() when loading or saving fails; only show success
after persistence succeeds.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c47023e7-c81b-4eb3-8334-5c1c492489c4
📒 Files selected for processing (5)
CHANGELOG.mddocs/user-stories.mdsrc/backend_task/migration/finish_unwire.rssrc/config.rssrc/ui/network_chooser_screen.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/user-stories.md
- CHANGELOG.md
- src/backend_task/migration/finish_unwire.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The PR fixes the previously reported lost-update race: automatic refresh attempts are serialized, both production configuration writers use the process-wide persistence lock, and completion sentinels are ordered after persistence and live updates. However, the automatic refresh now holds the general migration mutex during network discovery, so a terminal migration can still reject identity removal for up to the 10-second discovery timeout; the new persistence concurrency test also duplicates the locking protocol instead of exercising a production writer.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/backend_task/migration/finish_unwire.rs`:
- [BLOCKING] src/backend_task/migration/finish_unwire.rs:678-680: Do not hold the migration mutex during DAPI discovery
The detached refresh acquires `migration_run` and retains it across the entire refresh, including the network request at line 468, which has a 10-second timeout. The refresh can acquire this mutex after the actual migration has published a terminal status and released its guard, despite the function documentation promising that discovery cannot delay access to recovered identities. During that interval, `delete_local_qualified_identity` fails its `migration_run.try_lock()` with `WalletStorageNotReady`, and the UI tells the user that the storage update is still running even though only the unrelated best-effort endpoint refresh remains. Use a dedicated per-context DAPI-refresh one-flight mutex or state flag to serialize the refresh from sentinel check through completion without blocking identity operations protected by the migration mutex.
- [SUGGESTION] src/backend_task/migration/finish_unwire.rs:2779-2853: Exercise the production persistence path in the concurrency test
The test acquires `CONFIG_PERSISTENCE_LOCK` directly and reimplements the load, mutate, and save sequence in both worker threads. It proves that the mutex serializes those test closures, but it does not verify that either production writer acquires the lock or retains it across the required transaction. The test would continue passing if a production call site removed the guard, released it before saving, or moved its live update outside the protected scope. Extract the locked configuration update into a shared production helper used by both writers, then invoke that helper concurrently from this test with an injected synchronization hook around the post-load/pre-save point.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The automatic DAPI refresh still has two blocking defects: it can silently erase unrelated .env settings, and it holds the migration mutex during network discovery, temporarily blocking identity operations after migration has finished. The persistence race is guarded in production, but its concurrency test duplicates the locking protocol instead of exercising either production writer.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/backend_task/migration/finish_unwire.rs`:
- [BLOCKING] src/backend_task/migration/finish_unwire.rs:503: Preserve unrelated settings when automatically rewriting .env
A successful automatic discovery calls `Config::save`, which atomically replaces the entire `.env` file using only fields represented by the four network `NetworkConfig` sections (`src/config.rs:108-220`). The repository's `.env.example` also stores `MCP_API_KEY` and `MCP_LISTEN`, which `Config` neither models nor emits. Therefore, an upgrade that reaches this automatic migration path silently removes those settings and any other unrecognized entries; values already loaded into the current process may continue working, but MCP authentication or listener configuration disappears on the next launch. Update only the target network's DAPI entry or preserve unrecognized lines when saving.
github-actions
Bot
removed
the
claudius-review
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The exact head retains two blocking defects: automatic DAPI refresh can silently erase unrelated .env settings, and it holds the migration mutex during network discovery, temporarily rejecting identity operations after migration has completed. The focused exact-head tests pass, but the concurrency test still duplicates the locking protocol instead of exercising either production persistence writer.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
* fix: prevent silent no-op when review agents run in background PR dashpay/dash-evo-tool#908 (run 29739736003) showed the review orchestrator spawning grumpy-review's specialist fan-out with run_in_background:true, then ending its own turn ("Sit tight...") before consolidating. This action runs Claude Code as a single one-shot execution with no resumption-on-notification, so the backgrounded agents were orphaned when the process exited: report.json was never written, no findings were posted, and the job still exited 0. - Prompt now explicitly forbids run_in_background/async agent spawning and explains why (no follow-up turn exists to collect the results). - Add a "Verify review report was produced" step that fails the job when report.json is absent, so a silent no-op becomes a visible red X instead of a false-green "success" — and the trigger label stays on the PR for retry. * fix: allow background spawning, require blocking collection instead Per review feedback: banning run_in_background outright would force serialized-looking spawns. The actual requirement is narrower — agents can still run in the background for concurrency, but the orchestrator must act as a foreground monitor and TaskOutput(block=true) every one of them before ending its turn, instead of writing a "sit tight" reply and stopping while results are still outstanding.
Claudius-Maginificent
added
claudius-review
lklimek
added
the
claudius-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claudius the Magnificent — consolidated review of #908
I set three specialists loose on this — security, project consistency, and an adversarial QA reviewer who compiles and actually runs things rather than trusting the diff. They converged, independently, on the same headline. That's rarely a coincidence.
The good news first, because I am nothing if not fair: the best-effort architecture is genuinely well built. Every failure path returns before writing the completion sentinel, discovery is time-bounded, wallet/identity recovery can never be blocked or failed by this pass, the poison-lock recovery is sound, and the full 81-test finish_unwire suite passes. This is careful work.
Now the part where I sigh theatrically. Tally: 0 critical, 0 high, 4 medium, 5 low.
The four MEDIUMs, posted inline:
.envclobber (SEC-001) —Config::saverewrites the whole file from four network blocks and silently deletes everything else, including the storedMCP_API_KEYsecret. This PR fires that automatically and silently for every upgrading Mainnet/Testnet user. My QA reviewer didn't argue about it — he wrote a probe test and watched the key vanish. This is your open blocking thread; it is real; three reviewers agree.migration_runheld across ~10s discovery (SEC-002) — the best-effort refresh borrows the migration mutex it has no business holding, so identity deletion spuriously reports "storage update still running" for up to ten seconds after migration finished.- False-success banner on failed save (CODE-001) — a failed
config.saveis logged and then followed by a cheerful success message. The user is told the change persisted; it did not. - Persistence transaction copy-pasted three ways (CODE-003) — the locked load→mutate→save→live-update span exists in the migration path, the UI path, and re-rolled by hand inside its own concurrency test, which is why that test wouldn't catch a production regression.
The five LOWs (sentinel-on-reinit-failure vs. the docs' "retries until reconnect" wording, blocking disk I/O on the render thread, the hollow concurrency test, unvalidated address CSV, and an over-broad pub) are in the attached report and worth a pass, but none block.
Verdict: not approving — the .env secret-loss and the migration-mutex hold want fixing before this ships. Fix those two and the false-success banner and I'll happily change my tune. The design underneath is sound; it's the persistence seam that needs tightening.
🤖 Consolidated from parallel security / project / QA review by Claudius the Magnificent. Full severity-ranked report generated as an artifact.
📊 View full HTML review report
github-actions
Bot
removed
the
claudius-review
lklimek
added
the
claudius-review
…nestly Addresses the still-open review threads on PR #908: - Config::save previously rewrote .env from only its four modeled network blocks, silently destroying anything else in the file (MCP_API_KEY, MCP_LISTEN, RUST_LOG, operator comments, ...) on every save — including the unattended automatic DAPI refresh on first launch. save() now reads the existing file, preserves every line it doesn't own, and only rewrites modeled keys in place; load() now fails closed (typed error) on a malformed modeled value instead of silently dropping it via .ok(). - Extracted the lock->load->mutate->save->live-update sequence, previously hand-duplicated across the migration path, the manual UI refresh, and the concurrency test, into one shared persist_dapi_addresses() helper (backend_task::dapi_discovery). Both writers and the test now go through it, so a future change to the lock discipline can't silently drift out of sync between copies. - network_chooser_screen's display_task_result no longer shows a success banner when the save actually failed — it now surfaces a proper MessageBanner error with details instead of only tracing::error!-logging and falling through to the happy path. - Documented the migration_run mutex policy as intentional: holding it across the detached DAPI refresh (not just core migration steps) is by design, per maintainer direction. Fixed the spawn_dapi_refresh docstring that previously claimed discovery "cannot delay" identity operations — it does, on purpose, and WalletStorageNotReady's doc comment now says so. Co-Authored-By: Codex Sol <noreply@openai.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Claudius the Magnificent — round two on #908 (commit ca0d15b2)
I dispatched the usual trio — security, project-consistency, and an adversarial QA reviewer who compiles and runs things rather than admiring the diff — against the newest commit. Let me open with credit where it's earned, because I am scrupulously fair even when it pains me.
The prior round's fixes are real and test-backed. The .env secret-clobber is genuinely gone — MCP_API_KEY, MCP_LISTEN, RUST_LOG, comments, and blank lines all round-trip verbatim; save is atomic (temp + fsync + persist), re-applies 0600, and validates before touching the file. The false-success banner is fixed. The triplicated persistence transaction is single-sourced in persist_dapi_addresses, and — I checked — the concurrency test genuinely exercises the production writer rather than re-rolling the protocol. config:: 20/20, migration::finish_unwire 82/82, clippy clean. Good, careful work.
And then the same commit introduced three new things worth your attention. Tally: 0 critical, 0 high, 3 medium, 6 low.
The three MEDIUMs, posted inline:
- Fail-closed load bricks all-network startup (SEC-001) — the hardening swung too far: one malformed line anywhere in
.envnow aborts the entire load, defeats the cross-network fallback loop inapp.rs, and locks the user out at boot with a misleading "check your DAPI addresses" message. Flagged independently by security and QA — QA reproduced it with a probe test. This is exactly the hand-edited-.envupgrade path the PR exists to serve. Blocking. - Off-thread
set_var(CODE-001) — the detached refresh reachesConfig::load_from→dotenvy(std::env::set_var) on a tokio worker thread;CONFIG_PERSISTENCE_LOCKdoesn't cover the other main-threadload_fromcallers. Concurrentsetenv/getenvis UB — the reason Rust 2024 made itunsafe, and the reason your own tests use subprocess isolation. Blocking. - Sentinel marked complete on reinit failure (CODE-002) — a failed
reinitis logged, then the completion sentinel is written anyway, so auto-reconnect never retries. Silently recreates the NET-016 failure the feature set out to prevent.
The six LOWs (line-injection guard at the persist chokepoint, save dropping on-disk modeled secrets absent in-memory, the modeled-field-list triplication, the unenforced lock invariant, the untested Ok(empty) discovery result, and the CHANGELOG overstating "until the app reconnects") are in the attached report — worth a pass, none blocking.
Verdict: not approving. The persistence rework is sound; it's the fail-closed load regression and the off-thread set_var hazard — both born in this very commit — that want fixing first, with CODE-002 close behind. Address those and I shall descend from Olympus to bless the merge.
🤖 Consolidated from parallel security / project / QA review by Claudius the Magnificent. Full severity-ranked HTML report generated as a build artifact.
github-actions
Bot
removed
the
claudius-review
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest commit fixes the five actionable prior issues around lifecycle serialization, unsupported networks, retry documentation, production-path test coverage, and unmodeled .env preservation. Two blocking regressions remain: DAPI persistence round-trips unrelated modeled settings, which can persist process-only secrets and corrupt dotenv-escaped values, while one malformed unused network now prevents every network context from starting. The focused configuration and DAPI persistence tests pass but do not cover these regressions.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/backend_task/dapi_discovery.rs`:
- [BLOCKING] src/backend_task/dapi_discovery.rs:158-166: Limit DAPI persistence to the selected assignment
The automatic refresh loads every network from both the process environment and `.env`, changes one DAPI field, and serializes the complete modeled configuration. This silently copies process-only credentials such as `core_rpc_password` and `wallet_private_key` into persistent plaintext. It also rewrites parsed values without dotenv escaping: a valid `MAINNET_core_rpc_password="abc def"` becomes `MAINNET_core_rpc_password=abc def`, which fails to parse on the next launch, while literal dollar signs or backslashes can be substituted or changed. Update only the selected network's DAPI assignment while preserving all other raw lines and value provenance.
In `src/config.rs`:
- [BLOCKING] src/config.rs:260-267: Keep invalid network settings isolated to that network
Every `load_network_config` error is propagated, so one malformed value in any network block aborts the complete `Config` load. For example, valid Mainnet settings plus `TESTNET_core_rpc_port=not-a-number` makes `AppContext::new` fail for Mainnet as well. The startup fallback then retries every network through the same failing whole-file load and cannot start, even though a usable configuration exists. Preserve per-network parsing isolation during normal loads; write paths can still validate the existing file before modifying it.
#7) Follow-ups from auditing dashpay/dash-evo-tool#908's successful run (29744073162) after the run_in_background fix landed: - TaskOutput can time out mid-block and return status:running instead of a final result; the prompt now requires re-blocking on timeout instead of treating that as "collected". Made explicit that this rule is transitive — the orchestrator must pass it down to any specialist it spawns (e.g. via grumpy-review), since a nested reviewer that itself fans out sub-agents has no way to know it's running headless otherwise. Observed live: a QA sub-agent hit exactly this and only recovered by model discretion, not enforcement. - Add a fast write-access preflight: the underlying claude-code-action rejects non-write-access triggering actors from deep inside its own execution (~40s in, opaque "Action failed with error" message). Check via `gh api .../collaborators/{actor}/permission` before paying that cost. - Install claudius's documented report-rendering Python deps (markdown, jsonschema, jinja2, etc.) via apt — the runner image doesn't have them, which was crashing generate_review_report.py mid-run (`import markdown`) and getting silently retried around. - Surface (rather than swallow) trigger-label removal failures — a silent failure there leaves the label on the PR and re-triggers a full review on the next push.
Run 29744073162 (PR #908, first run after the run_in_background fix landed in lklimek/claudius-review-action) took 30m37s wall-clock, leaving under 10 minutes of headroom against the 40-minute cap. A larger PR would trip the timeout and lose the whole review after real work was already done.
Keep valid network configs usable when another network is malformed. Persist DAPI endpoints as a targeted locked update and retry refresh after reinitialization fails. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Exact head ac8db09 retains two blocking configuration regressions introduced by this PR. DAPI refresh persists the complete merged configuration instead of only the selected assignment, and configuration loading now lets one malformed inactive-network value prevent every valid network context from starting. The 20 configuration tests and the cross-network persistence concurrency test pass, but they do not cover these behaviors.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
lklimek
added
the
claudius-review
|
Pushed fixes for all 4 review findings plus the flaky CI test:
Commit: 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
HEAD matches 1acf308, and the focused DAPI refresh, configuration isolation, targeted persistence, and cross-network concurrency tests pass. Six prior findings are resolved, but the detached network request still occupies the storage-migration mutex after terminal migration status is published, contradicting the PR's original recovery-independence contract and temporarily rejecting unrelated identity operations. The new per-network isolation also drops the underlying parse error from technical logs.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/config.rs`:
- [SUGGESTION] src/config.rs:394-401: Preserve invalid-network parse diagnostics
`load_network_config_or_none` correctly isolates a malformed network so other configurations remain usable, but the `Err(_)` arm discards the typed `ConfigError::InvalidNetworkConfig`. If that network is later selected, `AppContext::new` only observes an absent configuration, leaving technical logs unable to distinguish malformed values from a missing network block or identify the invalid field. Include the error source in the warning while continuing to return `None`.
| match load_network_config(prefix, network, process_entries, file_entries) { | ||
| Ok(config) => config, | ||
| Err(_) => { | ||
| tracing::warn!( | ||
| network, | ||
| "Ignoring invalid network settings while loading other networks" | ||
| ); | ||
| None |
There was a problem hiding this comment.
🟡 Suggestion: Preserve invalid-network parse diagnostics
load_network_config_or_none correctly isolates a malformed network so other configurations remain usable, but the Err(_) arm discards the typed ConfigError::InvalidNetworkConfig. If that network is later selected, AppContext::new only observes an absent configuration, leaving technical logs unable to distinguish malformed values from a missing network block or identify the invalid field. Include the error source in the warning while continuing to return None.
| match load_network_config(prefix, network, process_entries, file_entries) { | |
| Ok(config) => config, | |
| Err(_) => { | |
| tracing::warn!( | |
| network, | |
| "Ignoring invalid network settings while loading other networks" | |
| ); | |
| None | |
| Err(error) => { | |
| tracing::warn!( | |
| network, | |
| ?error, | |
| "Ignoring invalid network settings while loading other networks" | |
| ); | |
| None | |
| } |
source: ['codex']
There was a problem hiding this comment.
Confirming this one is still open at 1acf3085. load_network_config_or_none (src/config.rs:394-403) continues to match Err(_) and drops the typed ConfigError::InvalidNetworkConfig, so the warn! logs only the network name — a later AppContext::new for that network can't tell a malformed value from an absent block, nor name the offending field. Your suggested ?error in the warning is a clean, self-contained improvement and I'd happily see it land. Leaving the thread for you to resolve once applied.
(Related note for whoever picks this up: the sibling MEDIUM I posted about load_from_env_path fail-closing the whole load on a lexer-level malformed line lives one layer up — different failure mode, same file. Worth fixing together.)
There was a problem hiding this comment.
Claudius the Magnificent — re-review of #908 @ 1acf3085
I sent three specialists back in — security, project consistency, and an adversarial QA reviewer who compiles and runs things rather than trusting the diff — to check the fixes from your last push. Credit where it's due: the fixes are real, and I verified them rather than taking your word for it.
Genuinely fixed (confirmed, several by executing probe tests):
- The
.envsecret clobber —MCP_API_KEYnow survives a save; the concurrency test drives the real productionpersist_dapi_addresses_inner, not a hand-rolled copy. - The off-main-thread
env::set_varUB — the refresh path never touchesConfig::load_from. - Per-network parse isolation on load, and the completion sentinel correctly withheld on reinit failure.
The persistence seam underneath — atomic renames, 0600 perms, poison-tolerant lock, honest save-failure reporting, no secret leakage in banners — is well built. I remain impressed against my will.
Now the two things still standing. Tally: 0 critical, 0 high, 2 medium, 6 low, 1 info.
migration_runheld across the ~10s discovery (posted inline). The best-effort refresh still borrows the migration mutex and holds it after migration goes terminal, so identity deletion spuriously reports "storage update still running." My QA reviewer proved it with a passing probe test on this commit, and your own new rustdoc admits the behavior — which is awkward, because it's the exact thing this PR promises never to do. This is the one I'd genuinely like resolved.load_from_env_pathfail-closed regression (posted inline). One malformed.envline now bricks startup for a network where the base branch warned and continued — on precisely the hand-edited-.envpopulation this feature courts.
The six LOWs (env-write CR/LF sanitization, an unlocked dead Config::save, a three-way-duplicated field list, an orphaned whole-file save, a test-only no-op future param, and a test suite that waits out the very race it should assert on) plus the INFO call-tree record are in the attached report — worth a pass, none blocking.
There's also one still-open human thread from @thepastaclaw about preserving invalid-network parse diagnostics — a fair, tiny nitpick I've left for you to action.
Verdict: not approving yet. Fix the migration-mutex hold (and ideally the false-brick load path), and I'll change my tune with theatrical grace. The design is sound; it's the persistence-and-locking seam that wants one more turn.
🤖 Consolidated from parallel security / project / QA review by Claudius the Magnificent. Full severity-ranked HTML report generated as a build artifact.
| { | ||
| let ctx = Arc::clone(app_context); | ||
| tokio::spawn(async move { | ||
| let _refresh_guard = ctx.migration_run.lock().await; |
There was a problem hiding this comment.
🟠 MEDIUM — the best-effort refresh still holds migration_run across the whole ~10s discovery, and it still blocks identity deletion after migration has finished.
I know these threads were marked resolved, so allow me to be the tiresome one who re-opens the case with evidence. This guard is taken here and held across refresh.await — network discovery (try_discover_nodes, 10s timeout, preceded by an un-timed TrustedHttpContextProvider::new), persistence, and reinit_core_client_and_sdk. Because run()'s own guard is released before this detached task can acquire it, the refresh grabs migration_run precisely after migration has gone terminal and the banner has cleared.
Meanwhile delete_local_qualified_identity gates on the same mutex via try_lock() and returns WalletStorageNotReady on contention — it can't tell "migration running" from "a background node-address refresh happens to hold the lock." So a user who watches migration succeed and immediately deletes a duplicate identity gets "The storage update is still running" for up to ~10s, about an operation that has nothing to do with their identity.
This isn't theoretical. My QA reviewer set MigrationState::Ready, parked this future mid-flight, and watched delete_local_qualified_identity return Err(WalletStorageNotReady) — a passing probe test on this exact commit. And the PR's own new rustdoc on run() admits it verbatim. That directly contradicts this PR's stated non-goal: the refresh "must never fail migration or block wallet/identity recovery."
Fix: give the auto-refresh its own dedicated one-flight guard, or narrow migration_run to span only the actual migration passes — run discovery before taking the guard and hold it only around the persist+reinit critical section (which persist_dapi_addresses already serializes via CONFIG_PERSISTENCE_LOCK). Wrap provider construction in a timeout while you're there. If sharing the guard really is intentional, at minimum give WalletStorageNotReady message text that doesn't claim a storage update is running when it isn't, and document the tradeoff in docs/user-stories.md rather than only a rustdoc comment.
| .collect::<Result<Vec<_>, _>>() | ||
| .map_err(|source| ConfigError::InvalidEnvFile { source })?, | ||
| Err(error) if error.kind() == io::ErrorKind::NotFound => Vec::new(), | ||
| Err(source) => return Err(ConfigError::LoadError { source }), | ||
| }; | ||
| match dotenvy::from_path_override(&env_file_path) { | ||
| Ok(()) => tracing::info!("Successfully loaded .env file"), | ||
| Err(error) if error.not_found() => { | ||
| tracing::warn!( | ||
| ?error, | ||
| "No .env file was found. Continuing with environment variables." | ||
| ); | ||
| } | ||
| Err(source) => return Err(ConfigError::InvalidEnvFile { source }), |
There was a problem hiding this comment.
🟠 MEDIUM — load_from_env_path now hard-fails the entire load on a single malformed .env line, where the base branch warned and carried on.
The per-network isolation fix (load_network_config_or_none) is good — but it only rescues typed-value parse errors. A lexer-level malformed line never reaches it: dotenvy::Iter::new(...).collect::<Result<_,_>>() returns InvalidEnvFile here (line 226-227), and the from_path_override arm below (231-239) returns InvalidEnvFile on any non-not-found error too. Both are fatal.
Compare the base branch (04b8212:src/config.rs:250-258), which did if let Err(err) = dotenvy::from_path_override(...) { warn!(...) } and continued. So one stray quote or a hand-edited credential in .env used to be survivable; now it returns None from AppContext::new and the app can't build a context for that network. This path is also reached from the core task and two MCP entry points.
The irony: hand-edited pre-1.0 .env files are exactly the population this feature is meant to smooth the upgrade for — and this makes their startup more brittle, silently, with no mention in the CHANGELOG.
Fix: either confirm the fail-closed intent is deliberate and record it in the PR/CHANGELOG, or warn-and-continue on the whole-file lexer parse the same way per-network value failures are already tolerated, rather than propagating InvalidEnvFile up to AppContext::new.
github-actions
Bot
removed
the
claudius-review
Brings in: shutdown fix (dashpay#905), duplicate-DPNS-name error message (dashpay#915), startup banner clearing (dashpay#916), nav pointer cursor + tooltips and wallet-less masternode indication (dashpay#917), onboarding disconnected- banner suppression (dashpay#907), masternode dialog/nav/passphrase fixes (dashpay#913), DAPI auto-refresh during pre-1.0 migration (dashpay#908), "Add Receiving Address" wiring + its test hardening (dashpay#914, dashpay#920), and a CI timeout bump (dashpay#912). dashpay#906 (shielded re-enable) was already pulled in individually last session, so its squashed commit merged as a no-op. Conflicts (6 files) were rebrand-naming overlaps (dash_evo_tool:: vs orchardpay:: imports) plus one real merge in left_panel.rs, where OrchardPay's green-icon tint had to combine with upstream's new nav tooltip. Also fixed 5 files upstream's auto-merged (non-conflicting) additions left un-rebranded: a stray DASH_EVO_DATA_DIR_LOCK/env-var name in a new app.rs test, and dash_evo_tool:: references in three kittest test files. Added tooltip strings for OrchardPay's own nav entries (OrchardPay, DashPay) so the new every_nav_entry_has_a_tooltip test covers them — upstream's version only knows its own nav items. Fixed the new nav_label_hover_shows_pointer_cursor kittest test: OrchardPay's nav rail carries two more always-visible entries than upstream's, pushing "Settings" below the scrollable list's default-size visible viewport; scroll it into view first, matching what a real user would do. Verified: cargo check (both feature modes), cargo clippy --all-features --all-targets -- -D warnings, cargo fmt --all, cargo test --all-features --workspace (2066 lib + 257 kittest + doc tests, 0 failed), all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Why this PR exists
What was done
FinishUnwiremigration orchestrator (src/backend_task/migration/finish_unwire.rs) that automatically refreshes DAPI node addresses the first time the app detects it's migrating a genuine pre-1.0 install (gated ondetect_legacy_rows, Mainnet/Testnet only — Devnet/Regtest have no discovery endpoint).dapi_discovery::discover_and_format,Configpersistence to.env, liveAppContext.configupdate, andreinit_core_client_and_sdk()— no new discovery/config-write logic.det:migration:dapi_refresh:<network>:v1), independent of the wallet-drain sentinel so it can keep retrying on a later launch even after wallet recovery has already completed.tokio::spawn, not inline-awaited).migration_runguard prevents duplicate/retry races within one network's own refresh, and a process-wideCONFIG_PERSISTENCE_LOCKguards the.envload → mutate → save → live-update span so two different network contexts (e.g. Mainnet and Testnet, which can be live in the same process at once) can never interleave writes to the same shared config file — this same lock also now covers the pre-existing manual "Refresh DAPI endpoints" button's persistence path.docs/user-stories.md(NET-016) andCHANGELOG.mdupdated to describe the new automatic behavior alongside the existing manual button.Testing
cargo test --all-features -- migration::finish_unwire: 81 passed, 0 failed.cargo test --all-features -- migration::v093_upgrade: 7 passed, 0 failed — including the full v0.9.3 upgrade regression harness, confirmed making zero network I/O (test builds use a#[cfg(test)]no-network stub for the discovery call, verified structurally and viastrace).cargo clippy --all-features --all-targets -- -D warnings: clean.cargo +nightly fmt --all: clean.Breaking changes
None. Purely additive; the manual "Refresh DAPI endpoints" button's user-visible behavior is unchanged (only its internal persistence path now shares the same concurrency guard).
Checklist
cargo fmt/cargo clippycleandocs/user-stories.md,CHANGELOG.md)v093_upgrade.rsregression harness instead)Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit