Repository navigation
fix(dashmate): load ZeroSSL config in force mode - #4298
thepastaclaw wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Walkthrough
🚥 Pre-merge checks | ✅ 5
✨ Finishing Touches 💡 1
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Final review complete — no blockers (commit e35d964) · triage: normal |
✅ Action performedReviews resumed. |
|
The failing CodeRabbit or PastaClaw review check is unrelated to this PR's code changes. The gate ran when the PR opened, before either reviewer had submitted a formal GitHub review, and its log explicitly says it will rerun automatically when a review is submitted. CodeRabbit has since completed its analysis, and the PastaClaw review is queued separately. The ZeroSSL change is proper as-is; no branch change, rebase, empty commit, or CI retrigger is needed for this failure. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The PR fixes force mode's missing ZeroSSL configuration and path initialization, but the new regeneration path can persist a replacement certificate ID while leaving the previous certificate's key and CSR on disk. If renewal is interrupted and later retried without force, the pipeline can install a certificate bundle that does not match the retained private key.
Source: reviewers codex/general=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
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) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 1 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/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js`:
- [BLOCKING] packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js:75-81: Keep forced renewal artifacts consistent with the persisted certificate ID
Force mode clears only the in-memory presence flags; the previous `private.key`, `csr.pem`, and `bundle.crt` remain on disk. After generating a replacement key and CSR, the create task persists the new certificate ID at lines 180-185 before verification, download, or any artifact save occurs. If a later task fails, a subsequent non-force run loads that new pending certificate while `validateZeroSslCertificate` reports the old key and CSR as present; its pending-certificate branch clears only the bundle flag. Key/CSR generation and save are therefore skipped, while the new bundle is downloaded and written beside the old private key, producing a certificate/key mismatch. Stage the replacement ID, key, CSR, and bundle and commit them together after successful download, or roll back the persisted ID on failure so a retry cannot associate the old artifacts with the replacement certificate. Add a regression test covering a force run that fails after certificate creation followed by a non-force retry.
Note: GitHub does not allow me to request changes on my own PR, so this COMMENT review preserves the verified blocking finding and should still be treated as merge-blocking feedback.
91431c2 to
e910329
Compare
|
Addressed the blocking forced-renewal consistency finding in
Validation: targeted unit spec (8 passing), changed-file ESLint, and |
There was a problem hiding this comment.
Preliminary review — Codex only
The carried-forward blocker, "Keep forced renewal artifacts consistent with the persisted certificate ID," is resolved: the current head delays publishing the replacement ID until the artifacts are ready and rolls back artifacts and configuration when persistence fails. The latest delta introduces a new blocking security regression because the staged replacement private key is installed with the process-default file mode rather than preserving owner-only access.
Source: reviewers codex/general=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
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)
🔴 1 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/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js`:
- [BLOCKING] packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js:338-357: Preserve restrictive permissions when installing the replacement private key
The staged private key is created with Node's default mode (`0666` masked by the process umask), and `renameSync` replaces the destination inode instead of overwriting it. A forced renewal therefore changes an existing owner-only `0600` key to `0644` under the usual `0022` umask. Dashmate also creates its default and nested home directories without an explicitly restrictive mode, so on typical installations other local users can traverse the path and read the gateway TLS private key. Create the staged `private.key` with mode `0600` or explicitly chmod it before or after installation, while leaving the CSR and certificate bundle modes unchanged, and add a regression assertion for the installed key mode.
Note: GitHub does not allow me to request changes on my own PR, so this COMMENT review preserves the verified blocking finding and should still be treated as merge-blocking feedback.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The force-mode configuration initialization now works, and the prior private-key permission issue is fixed by staging the key with mode 0600. One blocker remains: replacing certificate files with renameSync disconnects successful renewals from the gateway's existing file-level bind mounts, so its SIGHUP hot restart continues serving the old certificate.
Source: reviewer codex/general=gpt-5.6-sol; final verifier codex/verifier=gpt-5.6-sol; orchestration-only and not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
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) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 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/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js`:
- [BLOCKING] packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js:358-361: Preserve bind-mounted certificate inodes during renewal
The gateway mounts bundle.crt and private.key as individual files in docker-compose.yml lines 208-209. Replacing either destination with renameSync creates a new inode on the host, while the running container's bind mount remains attached to the old inode. The renewal scheduler then sends SIGHUP to PID 1, whose hot-restart supervisor forks another Envoy process in the same mount namespace, so that process also reads the old certificate. Normal expiration renewal replaces bundle.crt, and force mode replaces both mounted files; either path can publish the new certificate ID and report success while the gateway continues serving the expiring certificate. Overwrite existing destinations in place to preserve their mounted inodes, and use renameSync only for first-time installation.
Canonical verifier result: REQUEST_CHANGES. GitHub does not allow authors to approve or request changes on their own pull requests, so this review was submitted using COMMENT transport. The findings and blocker status above are unchanged.
9955519 to
41dc4bf
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head 41dc4bf, the prior bind-mount blocker is fixed: existing certificate artifacts are overwritten in place, while first-time installations still use rename, and regression coverage confirms the behavior. The targeted ZeroSSL suites pass with 21 tests, and ESLint passes for both changed files.
Source: reviewers codex/general=gpt-5.6-sol and codex/security-auditor=gpt-5.6-sol; final verifier codex/verifier=gpt-5.6-sol; openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
Canonical verifier result: APPROVE. GitHub does not allow authors to approve or request changes on their own pull requests, so this review was submitted using COMMENT transport. The findings and blocker status above are unchanged.
|
Friendly review reminder for @QuantumExplorer and @shumkov on the current head. This PR is mergeable with no failed or pending checks; when you have a chance, could you review the live Tenderdash version handling? Thanks. |
|
@coderabbitai review No review for |
github-actions
Bot
added
waiting-self-review
github-actions
Bot
removed
bot-review-skipped
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
Ready for review — |
github-actions
Bot
added
ready-for-human
Co-Authored-By: Claude Code <noreply@anthropic.com>
41dc4bf to
e35d964
Compare
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The force-mode ZeroSSL fix correctly initializes configuration and paths before bypassing certificate validation, and resets stale artifact state so regeneration can proceed. No blocking correctness or security issues were identified. Two targeted regression-coverage gaps remain: the new missing-configuration errors are untested, and the added force-mode test stops before artifact persistence.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a small, well-contained behavioral fix in dashmate's ZeroSSL certificate generation flow, but it changes key and certificate handling and therefore warrants the default review effort rather than a trivial or low classification. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js`:
- [SUGGESTION] packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js:69-75: Add regression tests for force-mode missing configuration errors
The new force-only branches for a missing ZeroSSL API key and missing external IP are user-facing behavior introduced by this change, but the test suite does not exercise either branch. Add cases that run the task with force enabled and each configuration value absent, then assert the corresponding clear error message. Without these tests, a future refactor could remove the friendly validation and restore the less actionable key-generation failure that this change is intended to prevent.
In `packages/dashmate/test/unit/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.spec.js`:
- [SUGGESTION] packages/dashmate/test/unit/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.spec.js:102-121: Exercise forced artifact replacement through persistence
The force-mode regression test verifies that stale flags are reset far enough to generate a new keypair, CSR, and certificate request, but it deliberately rejects at certificate creation and never reaches the save tasks. Consequently, it would still pass if the force reset of isPrivateKeyFilePresent or isBundleFilePresent were removed. Add a successful force-mode case using a temporary home directory with existing artifact files, then assert that the generated private key, CSR, and downloaded bundle replace the existing contents at the configured paths.
| if (ctx.force && !ctx.apiKey) { | ||
| throw new Error('ZeroSSL API key is not set. Please set it in the config file'); | ||
| } | ||
|
|
||
| if (ctx.force && !ctx.externalIp) { | ||
| throw new Error('External IP is not set. Please set it in the config file'); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Add regression tests for force-mode missing configuration errors
The new force-only branches for a missing ZeroSSL API key and missing external IP are user-facing behavior introduced by this change, but the test suite does not exercise either branch. Add cases that run the task with force enabled and each configuration value absent, then assert the corresponding clear error message. Without these tests, a future refactor could remove the friendly validation and restore the less actionable key-generation failure that this change is intended to prevent.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
| await expect(task.run({ | ||
| expirationDays: 30, | ||
| force: true, | ||
| isCsrFilePresent: true, | ||
| isPrivateKeyFilePresent: true, | ||
| isBundleFilePresent: true, | ||
| certificate: { id: 'old-certificate' }, | ||
| })).to.be.rejectedWith(sentinel); | ||
|
|
||
| expect(validate).to.not.have.been.called(); | ||
| expect(generateKeyPair).to.have.been.calledOnce(); | ||
| expect(generateCsr).to.have.been.calledOnceWithExactly( | ||
| { privateKey: 'private-key' }, | ||
| '1.2.3.4', | ||
| ); | ||
| expect(createCertificate).to.have.been.calledOnceWithExactly( | ||
| 'certificate-request', | ||
| '1.2.3.4', | ||
| 'test-api-key', | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: Exercise forced artifact replacement through persistence
The force-mode regression test verifies that stale flags are reset far enough to generate a new keypair, CSR, and certificate request, but it deliberately rejects at certificate creation and never reaches the save tasks. Consequently, it would still pass if the force reset of isPrivateKeyFilePresent or isBundleFilePresent were removed. Add a successful force-mode case using a temporary home directory with existing artifact files, then assert that the generated private key, CSR, and downloaded bundle replace the existing contents at the configured paths.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, security-auditor)
|
/skip-bots |
|
Ready for review — |
github-actions
Bot
added
ready-for-human
This branch has not been deployed
2 participants
Issue being fixed or feature implemented
Fixes #3803
Fixes #4249
Tracking: https://github.com/thepastaclaw/tracker/issues/1419
dashmate ssl obtain --forceskipped the ZeroSSL validation task that also initialized the task context. As a result, required values such as the API key, external IP, and certificate paths were undefined before key and CSR generation.What was done?
--forcelimited to bypassing existing-certificate validation and resetting prior certificate/artifact state so a fresh keypair, CSR, certificate, and bundle are generated.How Has This Been Tested?
yarn exec mocha --require ./test/bootstrap.js test/unit/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.spec.js test/unit/ssl/zerossl/validateZeroSslCertificateFactory.spec.js— 16 passingyarn exec eslint src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js test/unit/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.spec.jsyarn workspace dashmate lintvia the pre-commit hookBreaking Changes
None.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
PR Hygiene ·
e35d964dashmate(packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js,packages/dashmate/test/unit/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.spec.js) — ktechmidas or shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.