fix(dashmate): stop concurrent commands reverting each other's config changes

[shumkov]

Issue being fixed or feature implemented

Closes #4242.

Config's constructor delegates to setOptions(), which marks the config changed — so every config was dirty the moment it was read off disk. BaseCommand.finally() persists the whole config file whenever anything is dirty, so every successful command, including pure readers like dashmate config get and dashmate core cli, rewrote config.json from its own load-time snapshot.

That turns any overlapping pair of commands into a lost update:

1. A reader loads the config at T0.
2. dashmate config set …docker.image X loads, writes, prints success and exits 0 at T1.
3. The reader finishes and writes its T0 snapshot at T2.

The setting silently reverts even though config set reported success. Reported from a real rolling masternode image-pin update, where some nodes kept the old image digest.

Two things made it hard to spot:

• In single-process use the rewrite is byte-identical (verified against a real 58 KB, 4-config config.json), so it never shows up outside real concurrency.
• The worst window is not a CLI command at all — scripts/helper.js reads config once at startup and writes it back on certificate renewal months later. Its guard is even commented "Persist config if it was migrated", and this bug made it fire on every startup.

What was done?

Hydration is no longer a mutation. Config's constructor resets the flag after the initial setOptions(). ConfigFile already did exactly this; Config was just missing it. This alone means read-only commands write nothing — they cannot clobber anyone.

Changing configuration reads and saves as one locked step. Reading when a command starts and saving when it exits means the state written may already be out of date. ConfigFileJsonRepository.update(mutator) takes the lock, reads the current state, applies the change and saves — the state handed to the mutator is read after the lock is held, so there is nothing stale to write:

configFileRepository.update((configFile) => {
  configFile.getConfig(name).set(path, value);
});

Used by config set, create, remove, default and group default. Two commands changing different options now both succeed.

Commands that reconfigure a node hold the lock for their run. setup, reset, group reset and ssl obtain change configuration repeatedly while doing long, partly irreversible work, so a single locked step does not fit. They declare static mutatesConfig = true; BaseCommand.init() takes the lock before reading and releases it in finally(). Reading inside the lock is what makes their state current.

Read-only commands never take the lock, so a node can still be inspected while it is being set up.

Supporting details that matter:

• A command holding the lock reuses it for update()/write() rather than re-acquiring — otherwise the SSL task's mid-command save (obtainZeroSSLCertificateTaskFactory.js:150) would wait on itself.
• Release is idempotent and runs from normal completion, command failure, failure before the command body, and graceful shutdown.
• A lock lost while we believed we held it blocks the next save instead of being ignored — ignoring it is fine when a lock only narrows a window, not when it provides exclusivity.
• Writes are atomic and preserve file mode — config.json holds masternode privateKey, Core RPC password and ZeroSSL apiKey.
• config remove deletes the service directory only after the removal is saved, so a failed save cannot leave a config listed with nothing behind it.
• Service templates render only for configs that actually changed. That is safe because upgrading Dashmate records the new version even when no migration applies, which marks every config changed — so a release that edits a template still reaches every node.

Merging concurrent edits was considered and rejected: in-memory holds the whole config rather than a diff, so "our copy wins" reverts every field we merely loaded — the original bug, reintroduced by the merge.

The helper renews against current configuration. The worst instance of the bug was the helper writing a snapshot it had held since process start, reverting months of changes on certificate renewal. renewCertificate() now takes the lock, reads the current configuration, runs the obtain task against it, then writes and renders before releasing. The lock is deliberately held across issuance — for ZeroSSL that includes HTTP validation and can take minutes. Releasing earlier would mean replaying selected renewal fields afterwards, which is what undoes a provider switch made while the certificate was being issued. The operator-visible cost is documented in docs/config/index.md.

Certificate issuance is testable, and now tested. The ACME directory was hardcoded, so no test could reach it and the renewal path that keeps a node's TLS alive existed only in production. It is now configuration, which also gives an operator somewhere to rehearse: Let's Encrypt production allows only a few failed validations per hour, and the usual cause — an unreachable inbound port 80 — takes several attempts to sort out. Existing configurations are migrated to the directory they were already using.

The accompanying integration test obtains a certificate from Pebble, the server Let's Encrypt tests its own implementation against, over a private Docker network. Everything below the directory URL is the shipped code: the same lego image, the same arguments, a real HTTP-01 challenge, and the files the gateway then loads. Pebble rather than a stub because a node is identified by its external IP, and only Let's Encrypt issues IP certificates over ACME — Pebble implements that extension and ships the same short-lived profile name, so the production arguments run unmodified.

How Has This Been Tested?

259 unit tests and 2 integration tests pass; eslint clean. Behaviours were proven by breaking them, not by assertion:

Behaviour	Proof it is pinned
Hydrated config is clean	✖ expected true to be false before the fix
Read-only command writes nothing	asserts the write does not happen (mtime + inode) — asserting content passes on the buggy code
Concurrent edits both survive	old read-early/write-late loses one: other command edit survived => false; update() → both true
Held lock is reused, not re-taken	removing the guard → Lock file is already being held after 15s
Lost lock blocks the next save	restoring the old no-op handler → the save succeeds and the test fails
Waiting too long reports clearly	✖ before; now an actionable dashmate-level message
Version recorded on upgrade with no migration	✖ expected '98.0.0' to equal '99.0.0'
config remove keeps service files if the save fails	✖ before
The fix itself is pinned	deleting the mutatesConfig save gate, or hoisting the read outside the lock, now fails the suite — both shipped green before BaseCommand had tests
A failed template render is retried	reverting the render-before-persist order → ✖; the version stamp is no longer consumed by a failure
Interrupting a command cannot revert a concurrent writer	nested release no longer drops the outer lease, verified from a second process
Certificate private key is not world-readable	reverting the fix fails the Pebble test with expected 420 to equal 384 — through a real ACME issuance

Lock timings are injectable purely so the paths that only occur after seconds of waiting can be tested in about a second. (proper-lockfile floors stale at 2s and its refresh at 1s, which sets the limit on how fast a lost lock can be noticed.)

E2E — 5/5 passing locally and in CI, packages/dashmate/test/e2e/localNetwork.spec.js: setup → start → restart → stop → reset on a real local network, plus the testnet fullnode and evonode suites. This is the coverage that matters most here, since setup and reset are exactly the commands that hold the lock across their whole run. It also exercises saveCertificateTask for real — the local preset obtains a self-signed certificate through the same code path, and Envoy then loads the pair and serves the network.

Verified against the real CLI on a scratch home directory: a read-only command leaves config.json byte- and mtime-identical; four concurrent config set on different options all survive; a held lock produces the new message and writes nothing; 0600 on config.json survives a write; and a helper renewal preserves changes made after the helper started.

Design and code were reviewed by independent cross-model passes throughout, which caught several defects in earlier iterations — including two concurrency bugs and an error path that would have hidden the message an operator needs.

Behaviour Changes

No operator action is required and no configuration needs editing — platform.gateway.ssl.providerConfigs.letsencrypt.acmeDirectoryUrl is added by a migration that runs on the next command.

A command that changes configuration can now report that another dashmate command is modifying configuration and exit non-zero, after waiting ~15s. Previously it would have silently overwritten that other command's work. Automation that changes configuration while setup, reset, ssl obtain or core reindex is running will see this; retrying once the other command finishes succeeds. Note that core reindex holds the lock for the whole reindex, which can be hours.

The guarantee is between cooperating dashmate versions on a local filesystem: a pre-fix process does not honour the lock, so upgrade all instances and restart the node (which restarts the helper) before relying on it.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have described the behaviour changes in the corresponding section; none are breaking, so the title carries no marker
• I have made corresponding changes to the documentation if needed

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Configuration changes are now safely coordinated when multiple Dashmate commands run concurrently.
  • Long-running setup, reset, group reset, and SSL operations protect configuration throughout execution.
  • Configuration saves are atomic, reducing the risk of partial or lost updates.
  • Newly generated miner addresses and renewed SSL settings are persisted reliably.

• Bug Fixes

  • Prevented concurrent commands from overwriting configuration changes.
  • Fixed template regeneration and change tracking after configuration updates.
  • Improved recovery and error reporting when configuration locks are unavailable or lost.

• Documentation

  • Updated guidance on concurrent commands, configuration locking, and lock recovery behavior.

[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

Walkthrough

Dashmate configuration hydration now preserves clean state, mutations use locked read-modify-save operations with atomic persistence, and templates render from saved state. Commands, certificate and miner workflows, tests, dependencies, and concurrency documentation were updated.

Changes

Dashmate configuration locking

Layer / File(s)	Summary
Configuration persistence core packages/dashmate/src/config/..., .pnp.cjs, packages/dashmate/package.json, packages/dashmate/test/unit/config/...	Repository updates now use inter-process locking, retry and timeout handling, atomic writes, compromise detection, and post-write state tracking.
Command lock lifecycle packages/dashmate/src/oclif/command/BaseCommand.js, packages/dashmate/src/commands/{setup,reset,...}	Commands that reconfigure nodes hold the configuration lock through execution and release it during normal or exceptional shutdown.
Mutation command integration packages/dashmate/src/commands/..., packages/dashmate/src/helper/..., packages/dashmate/src/listr/...	Configuration mutations, renewals, certificate saves, and miner address creation use repository-backed updates and render templates from saved configuration.
Validation and concurrency guidance packages/dashmate/test/unit/..., packages/dashmate/docs/config/index.md	Tests cover hydration, migration, persistence, concurrent locking, command behavior, and rejected updates; documentation describes the locking rules.

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

Possibly related PRs

• dashpay/platform#4214: Both changes modify startGroupNodesTaskFactory in the miner-start flow.

Suggested reviewers: quantumexplorer

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes add clean hydration and inter-process locking so config writes are durable and read-only commands stop rewriting config.json.
Out of Scope Changes check	✅ Passed	The diff stays focused on config-dirty tracking, locking, atomic saves, and related tests/docs for the same concurrency fix.
Docstring Coverage	✅ Passed	Docstring coverage is 93.75% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: preventing concurrent Dashmate commands from reverting configuration changes.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/dashmate/4242-readonly-config-clobber

---

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js (3)

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

Hardcoded format version will drift.

'4.1.0' duplicates the production config format version; when it bumps, these tests seed a file that no longer matches the schema/migration expectations. Prefer importing the same constant/source the repository uses.

🤖 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
`@packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js`
at line 9, Replace the hardcoded CURRENT_FORMAT_VERSION in
ConfigFileJsonRepository.spec.js with the production config format version
constant or source used by ConfigFileJsonRepository, so tests always seed files
using the current schema version.

---

297-300: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Hot busy-wait spins a core for up to 10s on failure.

fs.existsSync in a tight loop pegs a CPU in CI when the child fails to start. Reuse an Atomics.wait-based sleep between polls.

♻️ Suggested tweak

       const deadline = Date.now() + 10000;
       while (!fs.existsSync(lockPath) && Date.now() < deadline) {
-        // busy-wait
+        Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 10);
       }

🤖 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
`@packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js`
around lines 297 - 300, Replace the tight polling loop around lockPath in the
ConfigFileJsonRepository test with an Atomics.wait-based sleep between
fs.existsSync checks, preserving the existing 10-second deadline and success
condition. Ensure the polling still exits promptly when the lock appears while
avoiding continuous CPU usage.

---

273-319: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Child process may leak if an assertion throws.

Any failing expect between Lines 302-316 aborts the test before the exit listener path completes, leaving the spawned child (and the lock dir) around until its own timer fires. A child.kill() in an afterEach/try-finally makes the failure mode deterministic.

🤖 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
`@packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js`
around lines 273 - 319, Ensure the spawned child in the “should wait for a lock
held by another process before writing” test is always cleaned up when an
assertion or repository operation fails. Wrap the synchronous test flow in
try/finally or add equivalent teardown that kills the child and removes any
remaining lock directory, while preserving the existing exit callback and
success assertions.

🤖 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 `@packages/dashmate/docs/config/index.md`:
- Around line 192-197: Add the `text` language identifier to the fenced code
block containing the configuration overwrite warning in the documentation, while
preserving the warning text unchanged.

In `@packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js`:
- Around line 174-198: Update the lock acquisition and release flow around
`#acquireLock` so proper-lockfile uses a non-throwing onCompromised handler and
release failures are caught in finally. Preserve the original operation or
ConfigFileConflictError when release() reports ERELEASED, and prevent
compromised-lock refresh errors from becoming uncaught asynchronous exceptions.

---

Nitpick comments:
In
`@packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js`:
- Line 9: Replace the hardcoded CURRENT_FORMAT_VERSION in
ConfigFileJsonRepository.spec.js with the production config format version
constant or source used by ConfigFileJsonRepository, so tests always seed files
using the current schema version.
- Around line 297-300: Replace the tight polling loop around lockPath in the
ConfigFileJsonRepository test with an Atomics.wait-based sleep between
fs.existsSync checks, preserving the existing 10-second deadline and success
condition. Ensure the polling still exits promptly when the lock appears while
avoiding continuous CPU usage.
- Around line 273-319: Ensure the spawned child in the “should wait for a lock
held by another process before writing” test is always cleaned up when an
assertion or repository operation fails. Wrap the synchronous test flow in
try/finally or add equivalent teardown that kills the child and removes any
remaining lock directory, while preserving the existing exit callback and
success assertions.

🪄 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 Plus

Run ID: d46df355-b215-4071-93e5-830b41f4f559

📥 Commits

Reviewing files that changed from the base of the PR and between ed4116b and b7a53e3.

⛔ Files ignored due to path filters (3)

• .yarn/cache/proper-lockfile-npm-4.1.2-a140a3c928-000a4875f5.zip is excluded by !**/.yarn/**, !**/*.zip
• .yarn/cache/write-file-atomic-npm-5.0.1-52283db6ee-648efddba5.zip is excluded by !**/.yarn/**, !**/*.zip
• yarn.lock is excluded by !**/yarn.lock, !**/*.lock

📒 Files selected for processing (11)

• .pnp.cjs
• packages/dashmate/docs/config/index.md
• packages/dashmate/package.json
• packages/dashmate/scripts/helper.js
• packages/dashmate/src/config/Config.js
• packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js
• packages/dashmate/src/config/errors/ConfigFileConflictError.js
• packages/dashmate/src/oclif/command/BaseCommand.js
• packages/dashmate/src/templates/writeConfigTemplatesFactory.js
• packages/dashmate/test/unit/config/Config.spec.js
• packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js

💤 Files with no reviewable changes (1)

• packages/dashmate/src/templates/writeConfigTemplatesFactory.js

[shumkov]

Dispositions for the three nitpicks from the CodeRabbit review, for the record.

Fixed in 235801568c:

• Hot busy-wait spins a core for up to 10s on failure — valid, and the worse failure mode is on a CI runner where the child never starts. The poll waiting for the other process to take the lock now sleeps between attempts instead of spinning.
• Child process may leak if an assertion throws — valid. The spawned lock holder was only cleaned up on the success path, so a failing assertion left it running and still owning the lock directory after its test was gone. It is now tracked and killed in afterEach, making the failure mode deterministic.

Declining, with reasoning:

• Hardcoded '4.1.0' format version will drift — the concern is that the seeded file stops matching schema/migration expectations when the real version bumps. It cannot: these tests inject an identity migration, so migratedConfigFileData.configFormatVersion === originConfigVersion holds for any value, and the schema constrains the field's type rather than its value. The literal is deliberately arbitrary test data, not a mirror of the production constant — importing the real one would couple the fixture to a value the test does not depend on, and would make the migration-path test (which uses '9.9.9') inconsistent with its sibling. Happy to revisit if a reviewer disagrees.

[thepastaclaw]

⛔ Blockers found — Opus deferred (commit 470bc4c)
Canonical validated blockers: 1

[thepastaclaw]

Preliminary review — Codex only

The compare-and-replace write path prevents stale writers from overwriting config.json, but the new dirty-state lifecycle is not safely integrated with existing SSL and template workflows. Intermediate SSL saves can discard pending render work, completed JSON writes can permanently consume the only template-upgrade signal, and the long-lived helper cannot recover after another process updates the file. These are three blocking 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 (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 3 blocking

1 additional finding(s) omitted (not in diff).

🤖 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 `packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js`:
- [BLOCKING] packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js:194-195: Intermediate SSL saves discard pending template renders
  Clearing every nested `Config` flag here breaks callers that save during a command and rely on `BaseCommand.finally()` to render afterward. The ZeroSSL pipeline updates `enabled`, `provider`, and the certificate ID and writes the `ConfigFile` at `obtainZeroSSLCertificateTaskFactory.js:145-150`; its remaining tasks only write certificate files, so nothing marks the config changed again. `BaseCommand.finally()` consequently sees a clean file and skips rendering, which can leave `config.json` switched from self-signed to ZeroSSL while Envoy still has the old listener configuration. The Let's Encrypt save task re-dirties the selected config, but its intermediate write still clears migration-triggered flags on every other config, suppressing their required first-command-after-upgrade renders. Preserve pending-template state across intermediate persistence, or explicitly render the captured pending configs after these writes.

In `packages/dashmate/src/oclif/command/BaseCommand.js`:
- [BLOCKING] packages/dashmate/src/oclif/command/BaseCommand.js:119-130: A template failure permanently consumes the upgrade marker
  `BaseCommand.finally()` commits the new config format version and clears all dirty flags before rendering any service template. If rendering or writing a template throws, or the process terminates after the config write, `config.json` remains stamped with the current version while one or more generated files remain stale. The next process hydrates every config as clean, and migration no longer marks them because the recorded version already matches, so the failed and unattempted templates are not retried automatically. This defeats the upgrade invariant on which render-only-changed behavior relies. Keep a durable pending-render indication until all affected templates have been written successfully, or provide an equivalent recovery mechanism.

In `packages/dashmate/scripts/helper.js`:
- [BLOCKING] packages/dashmate/scripts/helper.js:93: The helper cannot recover after another process updates config.json
  The helper reads `config.json` only once and retains that repository, `ConfigFile`, and selected `Config` for its entire lifetime. Under the new compare-and-replace contract, any later Dashmate command that changes the shared file invalidates the helper's baseline, including changes to an unrelated config. Certificate renewal then mutates the stale objects and writes through the stale repository, producing `ConfigFileConflictError`. Both renewal schedulers catch failures and schedule subsequent work with the same objects without re-reading, so every future renewal write remains unable to commit. Because the error is swallowed, the `unless-stopped` helper container stays alive rather than restarting and reloading, allowing gateway certificate renewal to remain broken until an external restart. Reload and reselect the current config before renewal mutations, or explicitly abandon and reload the helper state after a conflict.

[coderabbitai]

🧹 Nitpick comments (2)

packages/dashmate/src/commands/config/create.js (1)

21-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document the new injected parameters.

configFileRepository and writeConfigTemplates are resolved by name from the DI container; leaving them out of the JSDoc makes the injection contract easy to break on rename. Same applies to default.js, remove.js, set.js, and group/default.js.

♻️ Proposed doc update

   /**
    * `@param` {Object} args
    * `@param` {Object} flags
    * `@param` {ConfigFile} configFile
+   * `@param` {ConfigFileJsonRepository} configFileRepository
+   * `@param` {writeConfigTemplates} writeConfigTemplates
    * `@return` {Promise<void>}
    */

🤖 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 `@packages/dashmate/src/commands/config/create.js` around lines 21 - 45, Update
the JSDoc for runWithDependencies in create.js to document the injected
configFileRepository and writeConfigTemplates parameters, including their
expected types. Apply the same dependency-parameter documentation to the
corresponding runWithDependencies methods in default.js, remove.js, set.js, and
group/default.js, preserving the existing injection names and behavior.

packages/dashmate/test/unit/commands/config/mutatingCommands.spec.js (1)

114-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the command actually rejects.

.catch(() => {}) swallows the outcome, so the test would still pass if config remove silently succeeded on a missing config and simply skipped the delete. Pin the failure explicitly.

♻️ Proposed test tightening

-      await new ConfigRemoveCommand().runWithDependencies(
-        { config: 'does-not-exist' },
-        flags,
-        loadedConfigFile,
-        { has: () => false },
-        homeDir,
-        configFileRepository,
-      ).catch(() => {});
+      let error;
+      try {
+        await new ConfigRemoveCommand().runWithDependencies(
+          { config: 'does-not-exist' },
+          flags,
+          loadedConfigFile,
+          { has: () => false },
+          homeDir,
+          configFileRepository,
+        );
+      } catch (e) {
+        error = e;
+      }
+
+      expect(error).to.exist();

🤖 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 `@packages/dashmate/test/unit/commands/config/mutatingCommands.spec.js` around
lines 114 - 126, Update the test around ConfigRemoveCommand.runWithDependencies
to explicitly assert that the command rejects when removing the missing
“does-not-exist” configuration. Replace the empty catch that swallows the
outcome with an assertion on the rejected promise, while preserving the existing
checks that the service directory and configuration remain unchanged.

🤖 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 `@packages/dashmate/src/commands/config/create.js`:
- Around line 21-45: Update the JSDoc for runWithDependencies in create.js to
document the injected configFileRepository and writeConfigTemplates parameters,
including their expected types. Apply the same dependency-parameter
documentation to the corresponding runWithDependencies methods in default.js,
remove.js, set.js, and group/default.js, preserving the existing injection names
and behavior.

In `@packages/dashmate/test/unit/commands/config/mutatingCommands.spec.js`:
- Around line 114-126: Update the test around
ConfigRemoveCommand.runWithDependencies to explicitly assert that the command
rejects when removing the missing “does-not-exist” configuration. Replace the
empty catch that swallows the outcome with an assertion on the rejected promise,
while preserving the existing checks that the service directory and
configuration remain unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 18ab5426-a856-46b8-bfa7-fbcbbc673ca0

📥 Commits

Reviewing files that changed from the base of the PR and between e23dbdf and 95611b9.

📒 Files selected for processing (10)

• packages/dashmate/docs/config/index.md
• packages/dashmate/src/commands/config/create.js
• packages/dashmate/src/commands/config/default.js
• packages/dashmate/src/commands/config/remove.js
• packages/dashmate/src/commands/config/set.js
• packages/dashmate/src/commands/group/default.js
• packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js
• packages/dashmate/test/unit/commands/config/mutatingCommands.spec.js
• packages/dashmate/test/unit/commands/config/set.spec.js
• packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js (1)

317-350: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

#compromised is never reset on the standalone lock path — a single compromise permanently disables saves.

#compromised is only cleared in acquire() (line 210). #locked()'s standalone branch (used by every plain update()/write() call, e.g. from scheduleRenewLetsEncryptCertificateFactory/scheduleRenewZeroSslCertificateFactory) reaches #acquireLock() directly and never resets the flag after a fresh, successful acquisition. Once onCompromised fires once (line 332-334) on any instance — including a long-lived singleton used by the helper daemon — every future save via that instance throws "Lost the lock" forever, even though each call re-acquires the lock legitimately, until the process restarts.

🔒 Proposed fix: reset compromise state on every successful acquisition

   `#acquireLock`() {
     const deadline = Date.now() + this.lockAcquireTimeoutMs;
 
     for (;;) {
       try {
-        return lockfile.lockSync(this.configFilePath, {
+        const release = lockfile.lockSync(this.configFilePath, {
           lockfilePath: this.lockFilePath,
           realpath: false,
           stale: this.lockStaleMs,
           onCompromised: () => {
             this.#compromised = true;
           },
         });
+
+        // A fresh, successful acquisition re-establishes exclusivity, so a
+        // compromise recorded during an earlier session must not keep
+        // failing saves made under this new lock.
+        this.#compromised = false;
+
+        return release;
       } catch (e) {

🤖 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 `@packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js` around
lines 317 - 350, Reset `#compromised` after every successful lock acquisition in
`#acquireLock`(), including the standalone path used by `#locked`() for plain
update()/write() calls. Set it only after lockfile.lockSync succeeds, while
preserving the existing compromised callback and error-handling behavior.

packages/dashmate/src/oclif/command/BaseCommand.js (1)

145-183: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Write-before-render ordering loses the upgrade/migration retry marker on template failure (two sites, one root cause). In both places, configFileRepository.write(configFile) commits and clears dirty/version flags before templates are rendered from the (now stale) captured changedConfigs; if rendering throws partway through, the config file is durably marked clean/current while some generated service files remain stale, with no future migration-triggered retry. This matches a previously-raised, apparently still-unaddressed review finding on this pattern.

• packages/dashmate/src/oclif/command/BaseCommand.js#L145-L183: in saveConfigAndStopContainers, render changedConfigs via writeConfigTemplates (or otherwise confirm success) before calling configFileRepository.write(configFile), or retain a separate pending-render marker that survives a partial rendering failure.
• packages/dashmate/src/oclif/command/BaseCommand.js#L73-L85: apply the same reordering/marker fix to the migration-triggered write in init().

🤖 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 `@packages/dashmate/src/oclif/command/BaseCommand.js` around lines 145 - 183,
In packages/dashmate/src/oclif/command/BaseCommand.js lines 145-183, update
saveConfigAndStopContainers to render changedConfigs with writeConfigTemplates
successfully before configFileRepository.write(configFile), or preserve a
pending-render marker across partial failures. Apply the same ordering or marker
fix to the migration-triggered write in init at lines 73-85; both sites must
prevent the configuration from being marked clean/current until all template
rendering completes.

🤖 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 `@packages/dashmate/src/helper/scheduleRenewLetsEncryptCertificateFactory.js`:
- Around line 78-91: The certificate renewal schedulers must update only
renewal-produced fields on freshly loaded configuration, rather than replacing
stale SSL snapshots or persisting stale task inputs. In
packages/dashmate/src/helper/scheduleRenewLetsEncryptCertificateFactory.js:78-91
and
packages/dashmate/src/helper/scheduleRenewZeroSslCertificateFactory.js:82-95,
change the fresh-state callbacks to patch individual renewed fields; in
packages/dashmate/src/listr/tasks/ssl/letsencrypt/obtainLetsEncryptCertificateTaskFactory.js:257-261
and
packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js:151-155,
skip the direct write(configFile) when invoked by the background schedulers so
the scheduler updates own persistence.

In `@packages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.js`:
- Around line 96-101: Remove the subsequent minerConfig.set(...) assignment in
the group-node startup flow after configFileRepository.update. Use the existing
minerAddress value for the downstream logic, leaving minerConfig as the original
snapshot so command-exit persistence cannot overwrite concurrent configuration
changes.

---

Outside diff comments:
In `@packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js`:
- Around line 317-350: Reset `#compromised` after every successful lock
acquisition in `#acquireLock`(), including the standalone path used by `#locked`()
for plain update()/write() calls. Set it only after lockfile.lockSync succeeds,
while preserving the existing compromised callback and error-handling behavior.

In `@packages/dashmate/src/oclif/command/BaseCommand.js`:
- Around line 145-183: In packages/dashmate/src/oclif/command/BaseCommand.js
lines 145-183, update saveConfigAndStopContainers to render changedConfigs with
writeConfigTemplates successfully before configFileRepository.write(configFile),
or preserve a pending-render marker across partial failures. Apply the same
ordering or marker fix to the migration-triggered write in init at lines 73-85;
both sites must prevent the configuration from being marked clean/current until
all template rendering completes.

🪄 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 Plus

Run ID: 8dda59f3-48b6-4f14-a47b-e495ca1ed7c1

📥 Commits

Reviewing files that changed from the base of the PR and between 95611b9 and aae3308.

📒 Files selected for processing (20)

• packages/dashmate/docs/config/index.md
• packages/dashmate/scripts/helper.js
• packages/dashmate/src/commands/config/create.js
• packages/dashmate/src/commands/config/remove.js
• packages/dashmate/src/commands/config/set.js
• packages/dashmate/src/commands/group/reset.js
• packages/dashmate/src/commands/reset.js
• packages/dashmate/src/commands/setup.js
• packages/dashmate/src/commands/ssl/obtain.js
• packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js
• packages/dashmate/src/createDIContainer.js
• packages/dashmate/src/helper/scheduleRenewLetsEncryptCertificateFactory.js
• packages/dashmate/src/helper/scheduleRenewZeroSslCertificateFactory.js
• packages/dashmate/src/listr/tasks/ssl/letsencrypt/obtainLetsEncryptCertificateTaskFactory.js
• packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js
• packages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.js
• packages/dashmate/src/oclif/command/BaseCommand.js
• packages/dashmate/test/unit/commands/config/mutatingCommands.spec.js
• packages/dashmate/test/unit/commands/config/set.spec.js
• packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js

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

• packages/dashmate/scripts/helper.js
• packages/dashmate/docs/config/index.md

[thepastaclaw]

Preliminary review — Codex only

The two previously verified blockers are fixed: migrations render before consuming the format-version marker, and certificate renewal reloads configuration after acquiring the lock. Three in-scope blockers remain: ordinary updates have no durable recovery after a post-save template failure, a command with a compromised lease can overwrite templates before its JSON save is refused, and certificate replacement preserves legacy world-readable private-key permissions.
Source: reviewer backends gpt-5.6-sol (general) and gpt-5.6-sol (security-auditor); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — security-auditor (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 3 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 `packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js`:
- [BLOCKING] packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js:186-190: A failed post-save render leaves the committed configuration unapplied
  `update()` commits `config.json` before invoking `onSaved`, while `config set`, `config create`, and miner-address generation use that callback to write the corresponding service templates. If rendering throws or the process terminates after `#save`, the JSON contains the new value but the generated files remain stale. This PR makes hydration clean, so a later process has neither a dirty flag nor a format-version mismatch that would retry the render; the mismatch can therefore persist indefinitely. The renewal helper has the same save-then-render ordering on its successful changed-config path. Preserve a durable pending-render indication, make the JSON/template update recoverable as one repository operation, or provide another automatic retry mechanism.

In `packages/dashmate/src/oclif/command/BaseCommand.js`:
- [BLOCKING] packages/dashmate/src/oclif/command/BaseCommand.js:173-177: A compromised command renders its stale snapshot before save is refused
  A long-running mutating command renders every changed config before `configFileRepository.write()` checks whether its lease was compromised. If command A loses its lease and command B acquires the stale lock, B can save and render newer state. When A resumes, these lines overwrite B's service templates from A's stale snapshot; only afterward does `#save()` observe `#compromised`, preserve a rescue file, and reject A's JSON write. The result is current JSON paired with stale generated files. Validate the held lease before any rendering side effect, preferably through a repository operation that checks exclusivity both before running the render callback and before saving.

In `packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js`:
- [BLOCKING] packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js:51-54: Renewal preserves legacy world-readable TLS private keys
  Before this PR, `private.key` was created with Node's default `0666` mode, normally producing `0644` under a `0022` umask. The replacement path now copies every permission bit from the existing key, so upgrading and renewing preserves those legacy group/world-readable modes instead of repairing them. Dashmate creates its own directories with default modes as well, so another local account can copy the gateway private key when the user's home path is traversable. Strip all group/world bits when replacing an existing key while preserving stricter owner-only modes such as `0400`; also normalize the ZeroSSL reuse path, which skips writing an existing key entirely.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.58%. Comparing base (7a7ec9f) to head (470bc4c).
⚠️ Report is 18 commits behind head on v4.2-dev.

Additional details and impacted files

@@             Coverage Diff              @@
##           v4.2-dev    #4248      +/-   ##
============================================
- Coverage     87.62%   87.58%   -0.04%     
============================================
  Files          2704     2705       +1     
  Lines        345206   345614     +408     
============================================
+ Hits         302473   302715     +242     
- Misses        42733    42899     +166     

Components	Coverage Δ
dpp	88.86% <ø> (ø)
drive	86.25% <ø> (ø)
drive-abci	89.66% <ø> (+<0.01%)	⬆️
sdk	∅ <ø> (∅)
dapi-client	∅ <ø> (∅)
platform-version	∅ <ø> (∅)
platform-value	92.88% <ø> (ø)
platform-wallet	∅ <ø> (∅)
drive-proof-verifier	48.02% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[thepastaclaw]

Preliminary review — Codex only

The three previously verified blockers are fixed, but three new blocking consistency and concurrency defects remain: migration side effects still run outside the lock, render-before-save can leave generated files ahead of a failed JSON write, and certificate renewal can render stale state after losing its lease. The configurable ACME directory should also reject plaintext schemes.
Source: reviewers gpt-5.6-sol (general), gpt-5.6-sol (security-auditor); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — security-auditor (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 3 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 `packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js`:
- [BLOCKING] packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js:227-233: The unlocked migration probe executes filesystem migrations
  The initial `readResult()` calls `read()`, which invokes `migrateConfigFile()` before `#locked()` is entered. Some historical migrations are not pure: the `0.25.7` and `1.0.0-dev.12` migrations in `packages/dashmate/configs/getConfigFileMigrationsFactory.js` copy TLS files to new directories and remove the source files. A read-only command upgrading an old configuration can therefore move service files while `setup`, `reset`, or another migration process holds the configuration lock. Two upgrade processes can also race through the same copy/remove sequence before either acquires the lock. Probe the raw `configFormatVersion` without running migrations, or acquire the lock before invoking any migration.
- [BLOCKING] packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js:190-194: A failed JSON save leaves pre-rendered service files active
  `beforeSave` writes generated service files before `#save()` makes the source configuration durable. If the subsequent atomic write fails during temporary-file creation, fsync, chmod, or rename, the command returns an error with the old value still in `config.json`, while the generated files already contain the rejected value. `writeServiceConfigs()` also writes files sequentially without rollback, so a rendering failure can leave only part of the generated configuration updated. Because the durable file still has the current format version and hydration produces clean configs, a later command has no marker telling it to regenerate or restore these files. Use a durable pending-render record or a staged, rollback-capable update rather than moving the inconsistency window from after the JSON save to before it.

In `packages/dashmate/src/helper/renewCertificate.js`:
- [BLOCKING] packages/dashmate/src/helper/renewCertificate.js:57-64: A compromised renewal still renders its stale configuration
  Certificate issuance can hold this lease for minutes, but the success path does not verify `configFileRepository.isExclusive()` before rendering. If renewal A loses its lease and command B acquires the stale lock, B can save and render the current configuration. When A completes, `writeConfigTemplates(config)` overwrites B's templates from A's stale snapshot; only the following `write()` notices `#compromised`, writes a rescue file, and rejects the JSON save. This leaves current `config.json` paired with stale generated files. Check exclusivity before the rendering side effect, matching the guard already used by `BaseCommand`.

In `packages/dashmate/src/config/configJsonSchema.js`:
- [SUGGESTION] packages/dashmate/src/config/configJsonSchema.js:749-752: Require HTTPS for configurable ACME directories
  `format: 'uri'` accepts plaintext and unrelated URI schemes, and this value is passed directly to lego as `--server`. With an `http://` directory, an on-path attacker can replace unauthenticated ACME directory and protocol responses, feed arbitrary certificate material to the renewal process, or prevent renewal until the gateway certificate expires. RFC 8555 ACME services use HTTPS, while both the documented Let's Encrypt endpoints and the Pebble integration use HTTPS with an injected CA. Restricting this option to HTTPS preserves the stated production, staging, and testing use cases.

[thepastaclaw]

Preliminary review — Codex only

The locked fresh-read update design addresses the original lost-update bug, and the latest head fixes the unlocked migration probe, stale certificate-renewal rendering, and plaintext ACME directory acceptance. Two blocking consistency failures remain: generated service files can survive a failed or interrupted JSON save, and a failed post-save directory deletion makes config removal partial and impossible to retry normally.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (security-auditor); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — security-auditor (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 2 blocking

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 `packages/dashmate/src/commands/config/remove.js`:
- [BLOCKING] packages/dashmate/src/commands/config/remove.js:47-54: A failed directory deletion commits an unretryable partial removal
  `onSaved` runs after `config.json` has durably removed the config, so an `fs.rmSync()` failure returns an error while leaving the config absent and its directory present. Re-running `config remove` cannot retry the cleanup because `removeConfig(configName)` now throws `ConfigIsNotPresentError`. The name is meanwhile available for recreation, and template rendering only overwrites its known output files rather than removing leftover files such as certificate material. Rename the directory to a unique tombstone while holding the lock, restore it if the JSON save fails, and delete the tombstone after the removal commits; cleanup can then be retried without leaving stale files under a reusable config name.

In `packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js`:
- [BLOCKING] packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js:200-213: A failed JSON save leaves pre-rendered service files active
  (existing thread: https://github.com/dashpay/platform/pull/4248#discussion_r3751139829)
  `beforeSave` modifies live generated files before `#save()` makes the corresponding configuration durable. The recovery only covers save failures followed by a successful re-render of the old state: for `config create`, the old on-disk file does not contain the new config name, so `beforeSave(this.read())` throws and the newly rendered directory remains; a failure partway through `writeServiceConfigs()` occurs before the recovery block; and process termination between rendering and saving leaves no durable marker for the next command. This can leave service files describing rejected or only partially rendered configuration indefinitely. Stage and atomically install the complete rendered output, or persist a pending-render transaction marker that can recover after both ordinary failures and process termination.

[thepastaclaw]

Preliminary review — Codex only

The locked fresh-read update path addresses the original concurrent lost-update bug, but four blocking filesystem-transaction defects remain: uncommitted creations are not cleaned up, removals are not durably recoverable, and internal repository paths collide with accepted config names. Both prior verified findings remain valid at the exact head.
Source: Codex general reviewer backend gpt-5.6-sol; Codex security-auditor reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — security-auditor (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 4 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 `packages/dashmate/src/commands/config/remove.js`:
- [BLOCKING] packages/dashmate/src/commands/config/remove.js:60-70: Interrupted removal strands a live config's private files
  If the process is killed after `fs.renameSync()` but before `#save()`, the durable JSON still lists the config while its entire directory has moved to the tombstone. The generic render-pending recovery cannot reverse that move: it renders the still-listed config into a newly created live directory and clears the marker, leaving TLS keys and other non-generated files stranded in the tombstone. The local `catch` only restores the directory for exceptions handled by the same process. Removal needs a durable transaction record whose recovery restores the tombstone when the config remains listed and deletes it when the removal is durable.
- [BLOCKING] packages/dashmate/src/commands/config/remove.js:50-70: A failed directory deletion commits an unretryable partial removal
  (existing thread: https://github.com/dashpay/platform/pull/4248#discussion_r3752427426)
  If the post-save `fs.rmSync()` fails, `config.json` has already durably removed the config and there is no persistent cleanup record. Re-running `config remove` fails because the config is no longer present. The tombstone is also not outside the config namespace or genuinely unique: for common names, `${configName}.removed-${process.pid}` passes the accepted `[A-Za-z0-9][A-Za-z0-9._-]{0,63}` pattern, so a later `config create` can write into the retained directory and inherit its certificate material; PID reuse can collide with an earlier tombstone as well. Store tombstones beneath a reserved directory, use a unique identifier, and retain durable state so cleanup can be retried after the removal commits.

In `packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js`:
- [BLOCKING] packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js:95-99: Reserved repository paths collide with valid config names
  `config.json.lock` and `config.json.render-pending` are both accepted config names under the current validation rule. Creating `config.json.lock` writes service files inside the directory that `proper-lockfile` created as its lock; release then cannot remove the non-empty directory, and stale-lock reclamation uses the same non-recursive `rmdir`, permanently blocking later writes with `ENOTEMPTY`. An existing config with this name is also broken by upgrading to this version. A config named `config.json.render-pending` is mistaken for a pending marker, and recovery eventually attempts to remove its non-empty service directory as though it were a marker file. Use internal paths beginning with a character that config names cannot use.
- [BLOCKING] packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js:374-384: A failed JSON save leaves pre-rendered service files active
  (existing thread: https://github.com/dashpay/platform/pull/4248#discussion_r3751139829)
  During `config create`, `update()` records the marker and renders the new config into its live directory before `#save()` commits the name to `config.json`. If rendering partially throws, the save fails, or the process terminates before the save, the surviving JSON does not contain that name. `recoverPendingRender()` only renders configs found in the surviving JSON and then clears the marker, so it cannot identify or remove the uncommitted directory. `writeServiceConfigs()` also only overwrites files in the current template set and never removes unknown files. A later creation can therefore reuse the directory and inherit stale service files or certificate material. Record the affected output names in the pending transaction, or stage rendered output and install it only as part of a recoverable commit.

[thepastaclaw]

Preliminary review — Codex only

The latest commit fixes all four previously verified filesystem-transaction defects: interrupted creates and removals are now recoverable, and active repository paths no longer collide with configuration directories. Two blocking regressions remain: the new reserved-name validation prevents previously valid persisted configurations from loading, and long-running commands discard pending configuration after losing their lease instead of invoking the repository's rescue path.
Source: Codex general reviewer backend gpt-5.6-sol; Codex security-auditor reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — security-auditor (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 `packages/dashmate/src/config/resolve-config-directory.js`:
- [BLOCKING] packages/dashmate/src/config/resolve-config-directory.js:22-26: New reserved-name validation makes existing configs unreadable
  The merge-base validation accepted every name matching the path-safe pattern, including `config.json.lock`, `config.json.rescue`, `config.json.render-pending`, and their case or trailing-period variants. `ConfigFileJsonRepository.read()` constructs every persisted entry through `new Config()`, so this new rejection makes the entire file fail to load when any existing entry has one of these names. Commands cannot then load the collection to rename or remove the affected entry. The active lock and render-marker paths are now dot-prefixed and cannot be represented by valid config names; move the rescue path into the same dot-prefixed namespace and preserve compatibility, or migrate existing entries before enforcing the restriction.

In `packages/dashmate/src/oclif/command/BaseCommand.js`:
- [BLOCKING] packages/dashmate/src/oclif/command/BaseCommand.js:181-185: Lost leases discard the configuration that rescue files are meant to preserve
  When a long-running mutating command loses its lease, this guard throws before calling `configFileRepository.write(configFile)`. That correctly prevents stale template rendering, but it also bypasses `ConfigFileJsonRepository.#save()`'s compromised-lock branch, which writes the pending JSON to the mode-0600 `config.json.rescue` file without modifying `config.json`. A completed `setup`, `reset`, reindex, or foreground SSL operation can therefore discard generated keys, operator input, and configuration corresponding to external work already performed. Route the pending `ConfigFile` through `write()` before any rendering; under the compromised held lease, `#save()` creates the rescue file and throws safely.

[thepastaclaw]

Preliminary review — Codex only

The locked fresh-read update path addresses the original lost-update problem, and both blockers verified at the previous head are fixed. One blocking regression remains: cleanup-mode config remove can delete config.json itself; the new Pebble integration test also has a fixed-subnet reliability issue. Source: Codex general reviewer backend gpt-5.6-sol; Codex security-auditor reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — security-auditor (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 `packages/dashmate/src/commands/config/remove.js`:
- [BLOCKING] packages/dashmate/src/commands/config/remove.js:45-50: Removing the reserved config name deletes the entire configuration file
  The cleanup-retry path permits removal when the requested config is absent. As a result, `dashmate config remove config.json` performs a no-op mutation, saves the collection, and then calls `fs.rmSync()` on the repository's own `config.json`, deleting every persisted configuration before reporting success. Case-folded and trailing-period aliases can reach the same file on macOS or Windows. Reject repository-owned paths when no persisted entry requires compatibility cleanup; if a legacy `config.json` entry exists, remove that entry without treating the repository file as its service directory.

In `packages/dashmate/test/integration/ssl/letsencryptPebble.spec.js`:
- [SUGGESTION] packages/dashmate/test/integration/ssl/letsencryptPebble.spec.js:30-34: Fixed Pebble subnet can collide with existing Docker networks
  The unique network name does not make the hardcoded `172.29.0.0/16` IPAM pool unique. Docker rejects creation when that pool overlaps an existing network, so this integration test can fail before Pebble starts on developer machines and reused runners. Let Docker allocate an available subnet and derive the Pebble and lego addresses from the assigned pool, or retry with a verified non-overlapping pool.

[thepastaclaw]

🔴 Blocking: Removing the reserved config name deletes the entire configuration file

The cleanup-retry path permits removal when the requested config is absent. As a result, dashmate config remove config.json performs a no-op mutation, saves the collection, and then calls fs.rmSync() on the repository's own config.json, deleting every persisted configuration before reporting success. Case-folded and trailing-period aliases can reach the same file on macOS or Windows. Reject repository-owned paths when no persisted entry requires compatibility cleanup; if a legacy config.json entry exists, remove that entry without treating the repository file as its service directory.

_{source: ['codex']}

[thepastaclaw]

Resolved in 470bc4c — Removing the reserved config name deletes the entire configuration file no longer present.

_{Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.}

[thepastaclaw]

🟡 Suggestion: Fixed Pebble subnet can collide with existing Docker networks

The unique network name does not make the hardcoded 172.29.0.0/16 IPAM pool unique. Docker rejects creation when that pool overlaps an existing network, so this integration test can fail before Pebble starts on developer machines and reused runners. Let Docker allocate an available subnet and derive the Pebble and lego addresses from the assigned pool, or retry with a verified non-overlapping pool.

_{source: ['codex']}

[thepastaclaw]

Resolved in 470bc4c — Fixed Pebble subnet can collide with existing Docker networks no longer present.

_{Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.}

[thepastaclaw]

Preliminary review — Codex only

The locked fresh-read update path fixes the original lost-update problem, and both previously verified findings are fixed at the current head. One blocking recovery defect remains because compromised saves share a single rescue path; removal of legacy reserved-name aliases also leaves distinct secret-bearing directories behind on case-sensitive filesystems.
Source: Codex general reviewer backend gpt-5.6-sol; Codex security-auditor reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — security-auditor (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 `packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js`:
- [BLOCKING] packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js:591: A later lost lease overwrites an earlier command's rescue
  Every compromised save atomically replaces the same `.config.json.rescue` file. If a completed setup, reset, reindex, or certificate operation loses its lease, that file may be the only copy containing generated keys, operator input, or configuration corresponding to external work already performed. A second command that later loses its lease replaces the first rescue before it is reviewed, permanently discarding unrelated recoverable work. Give each rescue a unique path, as the render-pending records already do, so one compromised lease cannot destroy another command's preserved state.

In `packages/dashmate/src/commands/config/remove.js`:
- [SUGGESTION] packages/dashmate/src/commands/config/remove.js:48-66: Legacy reserved-name aliases retain secret-bearing directories on Linux
  `isConfigNameAvailable()` canonicalizes names case-insensitively and removes trailing periods on every platform, so a persisted legacy config named `CONFIG.JSON` or `config.json.` is classified as repository-owned. On a case-sensitive Linux filesystem, however, those paths are distinct directories rather than aliases of `config.json`. Removal deletes the collection entry but skips `fs.rmSync()`, and a cleanup retry is then rejected because the entry is absent. The retained directory can contain TLS keys and generated files such as `dash.conf` with masternode or spork private keys. Skip deletion only when the resolved service path actually aliases the repository file on the current filesystem; delete distinct legacy directories while retaining the existing protection for true aliases.

[thepastaclaw]

🔴 Blocking: A later lost lease overwrites an earlier command's rescue

Every compromised save atomically replaces the same .config.json.rescue file. If a completed setup, reset, reindex, or certificate operation loses its lease, that file may be the only copy containing generated keys, operator input, or configuration corresponding to external work already performed. A second command that later loses its lease replaces the first rescue before it is reviewed, permanently discarding unrelated recoverable work. Give each rescue a unique path, as the render-pending records already do, so one compromised lease cannot destroy another command's preserved state.

Suggested change

	const rescuePath = path.join(this.homeDirPath, '.config.json.rescue');
	const rescuePath = path.join(this.homeDirPath, `.config.json.rescue-${randomUUID()}`);

_{source: ['codex']}

[thepastaclaw]

🟡 Suggestion: Legacy reserved-name aliases retain secret-bearing directories on Linux

isConfigNameAvailable() canonicalizes names case-insensitively and removes trailing periods on every platform, so a persisted legacy config named CONFIG.JSON or config.json. is classified as repository-owned. On a case-sensitive Linux filesystem, however, those paths are distinct directories rather than aliases of config.json. Removal deletes the collection entry but skips fs.rmSync(), and a cleanup retry is then rejected because the entry is absent. The retained directory can contain TLS keys and generated files such as dash.conf with masternode or spork private keys. Skip deletion only when the resolved service path actually aliases the repository file on the current filesystem; delete distinct legacy directories while retaining the existing protection for true aliases.

_{source: ['codex']}