feat: harden deployment and worker operations - #351
Conversation
mira-2026
added
priority: high
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds persistent worker-claim pausing, managed systemd reconciliation with rollback, production cache-refresh metrics snapshots, strict Bun revision checks, database migration 8, production bootstrap automation, and nullable Moltbook avatar handling across backend, frontend, tests, and documentation. ChangesWorker claims
Managed dashboard units
Cache refresh metrics
Bun revision identity
Production bootstrap
Moltbook contracts
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (1)
backend/src/managedBunRuntime.ts (1)
118-135: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire a revision suffix before runtime identity checks.
bun --revisioncan report onlypackage_json_versionwhen the build omits git metadata, and these validators still accept plainx.y.z. A version-only manifest and version-only executable would therefore pass path validation despite both runtime checks saying2.0.0. Require the+...identifier for--revision, manifest entries, equality matching, and the current-process identity before caching.🤖 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 `@backend/src/managedBunRuntime.ts` around lines 118 - 135, Require a non-empty +revision suffix for runtime identities: update readBunRevisionIdentity and the related validators in backend/src/managedBunRuntime.ts (lines 118-135 and 167-186) so --revision results, manifest entries, equality checks, and current-process identity reject plain versions before caching; apply the corresponding requirement in backend/src/services/pullRequests.ts (lines 2120-2127) and scripts/runManagedDashboardRelease.sh (lines 54-80), and update backend/test/managedBunRuntime.test.ts (lines 89-99 and 257-285) to cover version-only rejection and valid revision-suffixed identities.
🧹 Nitpick comments (4)
backend/test/managedDashboardSystemd.test.ts (1)
99-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHard-coded call count couples the test to exactly two managed units.
MANAGED_DASHBOARD_UNIT_NAMES.length + 1(onedaemon-reloadplus oneshowper unit) expresses the invariant and survives adding a third unit.♻️ Proposed tweak
- expect(calls).toHaveLength(3); + expect(calls).toHaveLength(MANAGED_DASHBOARD_UNIT_NAMES.length + 1);Also applies to: 107-107
🤖 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 `@backend/test/managedDashboardSystemd.test.ts` at line 99, Update the call-count assertions in the managed dashboard systemd test to use MANAGED_DASHBOARD_UNIT_NAMES.length + 1 instead of the hard-coded value 3, preserving the invariant of one daemon-reload call plus one show call per managed unit at both affected assertions.backend/src/managedDashboardSystemd.ts (1)
139-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated systemd property parsing.
This
key=valueline parser is byte-for-byte identical to the one inbackend/src/releaseDeployment.ts(Lines 281-291). Extracting a sharedparseSystemdProperties(stdout)helper keeps the two verification paths from drifting.🤖 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 `@backend/src/managedDashboardSystemd.ts` around lines 139 - 149, Extract the duplicated key=value parsing logic into a shared parseSystemdProperties(stdout) helper, then update the property-building code in managedDashboardSystemd and releaseDeployment to use it. Preserve the existing handling of blank lines, missing separators, and values containing additional equals signs.frontend/src/test/pageBehavior.test.tsx (1)
1511-1511: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep the claims-enabled mock contract-complete.
Because this mock now handles
?include=claims, return at leastclaimsPaused: false(and the contract’sclaimsPausedAtrepresentation). Otherwise the page test can pass while the Jobs UI never exercises the new pause-state fields.🤖 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 `@frontend/src/test/pageBehavior.test.tsx` at line 1511, Update the mock branch for “/api/job-executions?include=claims” to return the complete claims-enabled response, including claimsPaused set to false and the contract-required claimsPausedAt representation, so the page test exercises the Jobs UI pause-state fields.backend/src/services/cacheRefreshMetrics.ts (1)
92-143: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant directory validation on every metrics write.
writeSnapshot()callsensurePrivateRuntimeDirectory()(mkdir + lstat + realpath + chmod, 4 syscalls) on everypublishSnapshot()call, andpublishSnapshot()runs on every single request/coalesced/started/finished event fromrefreshCacheProducer. The directory only needs validating once per session (it doesn't change afterward), so this repeats avoidable synchronous syscalls on what can be a fairly hot path.Consider validating the directory once when the session starts (or lazily on first publish) and caching that result for the session's lifetime.
♻️ Proposed refactor sketch
let activeSession: | { instanceId: string; snapshotPath: string | undefined; startedAt: string; + directoryValidated: boolean; } | undefined; function writeSnapshot( snapshotPath: string, - snapshot: CacheRefreshMetricsSnapshot + snapshot: CacheRefreshMetricsSnapshot, + ensureDirectory: () => void ): void { const directoryPath = path.dirname(snapshotPath); - ensurePrivateRuntimeDirectory(directoryPath); + ensureDirectory(); ... } function publishSnapshot(): void { if (!activeSession?.snapshotPath) return; + if (!activeSession.directoryValidated) { + ensurePrivateRuntimeDirectory(path.dirname(activeSession.snapshotPath)); + activeSession.directoryValidated = true; + } writeSnapshot(activeSession.snapshotPath, { ... }); }🤖 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 `@backend/src/services/cacheRefreshMetrics.ts` around lines 92 - 143, Move the ensurePrivateRuntimeDirectory call out of writeSnapshot and validate the snapshot directory once per active session, either during session initialization or the first publish. Cache the validated directory state for that session, reuse it in subsequent writeSnapshot calls, and reset the cache when the session ends or changes.
🤖 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 `@backend/test/releaseManager.test.ts`:
- Around line 983-994: Await the rejection assertion in the releaseManager test
before checking preparationCalls, and await the writeReleaseManifest rejection
assertion in releaseManifest.test.ts; update both affected
sites—backend/test/releaseManager.test.ts:983-994 and
backend/test/releaseManifest.test.ts:317-318—to ensure failures are reported by
the tests and subsequent assertions run after the rejected operations complete.
In `@backend/test/routeAndServiceBehavior.test.ts`:
- Around line 1363-1391: Restructure the pause-state test around the PATCH call
that sets paused to true so the try/finally begins before that operation and its
status/body assertions, ensuring cleanup always runs. In the finally block,
capture the paused:false response, assert status 200, and verify its JSON state
has paused false.
---
Outside diff comments:
In `@backend/src/managedBunRuntime.ts`:
- Around line 118-135: Require a non-empty +revision suffix for runtime
identities: update readBunRevisionIdentity and the related validators in
backend/src/managedBunRuntime.ts (lines 118-135 and 167-186) so --revision
results, manifest entries, equality checks, and current-process identity reject
plain versions before caching; apply the corresponding requirement in
backend/src/services/pullRequests.ts (lines 2120-2127) and
scripts/runManagedDashboardRelease.sh (lines 54-80), and update
backend/test/managedBunRuntime.test.ts (lines 89-99 and 257-285) to cover
version-only rejection and valid revision-suffixed identities.
---
Nitpick comments:
In `@backend/src/managedDashboardSystemd.ts`:
- Around line 139-149: Extract the duplicated key=value parsing logic into a
shared parseSystemdProperties(stdout) helper, then update the property-building
code in managedDashboardSystemd and releaseDeployment to use it. Preserve the
existing handling of blank lines, missing separators, and values containing
additional equals signs.
In `@backend/src/services/cacheRefreshMetrics.ts`:
- Around line 92-143: Move the ensurePrivateRuntimeDirectory call out of
writeSnapshot and validate the snapshot directory once per active session,
either during session initialization or the first publish. Cache the validated
directory state for that session, reuse it in subsequent writeSnapshot calls,
and reset the cache when the session ends or changes.
In `@backend/test/managedDashboardSystemd.test.ts`:
- Line 99: Update the call-count assertions in the managed dashboard systemd
test to use MANAGED_DASHBOARD_UNIT_NAMES.length + 1 instead of the hard-coded
value 3, preserving the invariant of one daemon-reload call plus one show call
per managed unit at both affected assertions.
In `@frontend/src/test/pageBehavior.test.tsx`:
- Line 1511: Update the mock branch for “/api/job-executions?include=claims” to
return the complete claims-enabled response, including claimsPaused set to false
and the contract-required claimsPausedAt representation, so the page test
exercises the Jobs UI pause-state fields.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19cc5f27-1ab0-4ce0-82db-5b9ab76c0a1e
📒 Files selected for processing (45)
backend/src/databaseMigrations/0008WorkerControl.tsbackend/src/databaseMigrations/index.tsbackend/src/databaseSchemaCompatibility.tsbackend/src/managedBunRuntime.tsbackend/src/managedDashboardSystemd.tsbackend/src/managedDashboardUnitPolicy.tsbackend/src/observability.tsbackend/src/releaseDeployment.tsbackend/src/releaseLifecycle.tsbackend/src/releaseManager.tsbackend/src/releaseManifest.tsbackend/src/routes/jobExecutionRoutes.tsbackend/src/services/cacheRefresh.tsbackend/src/services/cacheRefreshMetrics.tsbackend/src/services/jobExecutionQueue.tsbackend/src/services/jobWorker.tsbackend/src/services/jobWorkerControl.tsbackend/src/services/pullRequests.tsbackend/src/services/scheduledJobs.tsbackend/test/cacheRefreshMetrics.test.tsbackend/test/databaseLifecycle.test.tsbackend/test/httpApiBehavior.test.tsbackend/test/jobExecutionQueue.test.tsbackend/test/managedBunRuntime.test.tsbackend/test/managedDashboardSystemd.test.tsbackend/test/multiFactorAuth.test.tsbackend/test/releaseManager.test.tsbackend/test/releaseManifest.test.tsbackend/test/routeAndServiceBehavior.test.tsbackend/test/serviceBehavior.test.tscontracts/jobs.tscontracts/moltbook.tsdocs/api/endpoints.mddocs/architecture/database.mddocs/operations/scheduler-cache-backups.mddocs/setup/new-vps.mddocs/setup/production-deploy.mdfrontend/src/components/features/jobs/JobExecutionQueueCard.tsxfrontend/src/hooks/index.tsfrontend/src/hooks/useJobExecutions.tsfrontend/src/test/componentBehavior.test.tsxfrontend/src/test/contracts.test.tsfrontend/src/test/frontendBehavior.test.tsxfrontend/src/test/pageBehavior.test.tsxscripts/runManagedDashboardRelease.sh
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Dashboard checks / 0_backend-checks.txt: feat: harden deployment and worker operations
Conclusion: failure
##[group]test/databaseOverview.test.ts:
227 | maxWait: 9,
228 | avgQueryTime: 20,
229 | avgTransactionTime: 10,
230 | },
231 | });
232 | expect(overview.sqlite).toMatchObject({
^
error: expect(received).toMatchObject(expected)
{
"attention": [
"No verified SQLite backup exists",
"SQLite maintenance job is not registered",
],
"backup": {
"count": 0,
"current": false,
+ "latest": undefined,
+ "latestAgeHours": undefined,
"reviewAgeHours": 48,
},
+ "databaseBytes": 4096,
+ "fileName": "dashboard.db",
"foreignKeysEnabled": true,
+ "freeBytes": 0,
+ "freePages": 0,
+ "freePercent": 0,
"journalMode": "wal",
+ "lastMaintenance": undefined,
"migrations": {
- "applied": 7,
+ "applied": 8,
"current": true,
- "latest": 7,
+ "latest": 8,
},
+ "pageCount": 106,
+ "pageSize": 4096,
"permissions": {
+ "dataDirectory": "0700",
+ "database": "0600",
"secure": true,
+ "shm": "0600",
+ "wal": "0600",
},
+ "shmBytes": 32768,
"status": "review",
+ "storageBytes": 473616,
"walAutoCheckpointPages": 1000,
+ "walBytes": 436752,
}
- Expected - 2
+ Received + 19
at <anonymous> (/home/runner/work/Mira-Dashboard/Mira-Dashboard/backend/test/databaseOverview.test.ts:232:37)
##[error] {
GitHub Actions: Dashboard checks / backend-checks: feat: harden deployment and worker operations
Conclusion: failure
##[group]test/databaseOverview.test.ts:
227 | maxWait: 9,
228 | avgQueryTime: 20,
229 | avgTransactionTime: 10,
230 | },
231 | });
232 | expect(overview.sqlite).toMatchObject({
^
error: expect(received).toMatchObject(expected)
{
"attention": [
"No verified SQLite backup exists",
"SQLite maintenance job is not registered",
],
"backup": {
"count": 0,
"current": false,
+ "latest": undefined,
+ "latestAgeHours": undefined,
"reviewAgeHours": 48,
},
+ "databaseBytes": 4096,
+ "fileName": "dashboard.db",
"foreignKeysEnabled": true,
+ "freeBytes": 0,
+ "freePages": 0,
+ "freePercent": 0,
"journalMode": "wal",
+ "lastMaintenance": undefined,
"migrations": {
- "applied": 7,
+ "applied": 8,
"current": true,
- "latest": 7,
+ "latest": 8,
},
+ "pageCount": 106,
+ "pageSize": 4096,
"permissions": {
+ "dataDirectory": "0700",
+ "database": "0600",
"secure": true,
+ "shm": "0600",
+ "wal": "0600",
},
+ "shmBytes": 32768,
"status": "review",
+ "storageBytes": 473616,
"walAutoCheckpointPages": 1000,
+ "walBytes": 436752,
}
- Expected - 2
+ Received + 19
at <anonymous> (/home/runner/work/Mira-Dashboard/Mira-Dashboard/backend/test/databaseOverview.test.ts:232:37)
##[error] {
🧰 Additional context used
🪛 ast-grep (0.45.0)
backend/src/services/cacheRefreshMetrics.ts
[warning] 118-122: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(temporaryPath, ${JSON.stringify(snapshot)}\n, {
encoding: "utf8",
flag: "wx",
mode: 0o600,
})
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 173-173: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(descriptor, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
backend/src/managedBunRuntime.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (45)
backend/test/serviceBehavior.test.ts (1)
1955-1955: LGTM!Also applies to: 1982-2002, 3156-3156
contracts/moltbook.ts (3)
28-28: LGTM!
66-66: LGTM!
168-175: LGTM!frontend/src/test/frontendBehavior.test.tsx (2)
4078-4078: LGTM!
4192-4192: LGTM!backend/src/managedDashboardUnitPolicy.ts (1)
1-24: LGTM!backend/src/managedDashboardSystemd.ts (1)
45-138: LGTM!Also applies to: 162-288
backend/src/releaseDeployment.ts (1)
16-21: LGTM!Also applies to: 33-36
backend/src/releaseManifest.ts (1)
22-22: LGTM!Also applies to: 37-41, 286-313, 634-646
backend/src/releaseLifecycle.ts (1)
12-12: LGTM!Also applies to: 157-160
backend/src/releaseManager.ts (1)
85-92: LGTM!Also applies to: 1140-1164, 1378-1401, 1466-1481, 1529-1531, 1564-1578
backend/test/managedDashboardSystemd.test.ts (1)
1-98: LGTM!Also applies to: 110-255
backend/test/releaseManifest.test.ts (1)
275-316: LGTM!backend/test/releaseManager.test.ts (1)
27-27: LGTM!Also applies to: 62-73, 145-145, 665-673, 747-767, 779-804, 814-819, 834-850, 862-871, 881-881, 1134-1134, 1150-1167, 1201-1201, 1211-1232, 1271-1276
docs/setup/production-deploy.md (1)
89-99: LGTM!Also applies to: 138-155, 243-243
backend/src/databaseMigrations/0008WorkerControl.ts (1)
1-16: LGTM!backend/src/databaseMigrations/index.ts (1)
8-8: LGTM!Also applies to: 19-19
backend/src/databaseSchemaCompatibility.ts (1)
11-12: LGTM!backend/test/multiFactorAuth.test.ts (1)
613-619: LGTM!backend/test/routeAndServiceBehavior.test.ts (1)
1352-1361: LGTM!backend/test/databaseLifecycle.test.ts (1)
117-129: LGTM!Also applies to: 262-273, 305-316, 386-388, 414-416, 519-519, 545-545, 599-615, 822-823
docs/architecture/database.md (1)
108-108: LGTM!docs/operations/scheduler-cache-backups.md (1)
21-21: LGTM!Also applies to: 46-59, 135-142
docs/api/endpoints.md (1)
144-159: LGTM!Also applies to: 177-180
docs/setup/new-vps.md (1)
250-254: LGTM!backend/src/services/jobWorkerControl.ts (1)
1-57: LGTM!contracts/jobs.ts (1)
99-100: LGTM!Also applies to: 125-138, 223-227, 302-311
backend/src/services/jobExecutionQueue.ts (1)
16-16: LGTM!Also applies to: 418-418, 462-463, 652-655
backend/src/services/scheduledJobs.ts (1)
32-32: LGTM!Also applies to: 1515-1517
backend/src/routes/jobExecutionRoutes.ts (2)
61-90: LGTM!
102-118: 🔒 Security & PrivacyNo change needed.
PATCH /api/job-executions/claimsis covered by global request-policy MFA enforcement and the existing MFA test coverage.frontend/src/components/features/jobs/JobExecutionQueueCard.tsx (1)
1-9: LGTM!Also applies to: 43-45, 68-81, 92-111, 128-141
frontend/src/hooks/useJobExecutions.ts (1)
11-14: LGTM!Also applies to: 47-48, 75-88
frontend/src/hooks/index.ts (1)
83-88: LGTM!frontend/src/test/componentBehavior.test.tsx (1)
3420-3420: LGTM!Also applies to: 3447-3452, 3466-3478, 3511-3530
frontend/src/test/contracts.test.ts (1)
18-30: LGTM!Also applies to: 92-102
backend/test/httpApiBehavior.test.ts (1)
1804-1807: LGTM!Also applies to: 1817-1841
backend/test/jobExecutionQueue.test.ts (1)
27-27: LGTM!Also applies to: 49-49, 607-635
backend/src/observability.ts (1)
12-12: LGTM!Also applies to: 71-81
backend/src/services/cacheRefresh.ts (1)
38-43: LGTM!Also applies to: 1907-1913, 2152-2152, 2167-2167, 2184-2203
backend/src/services/cacheRefreshMetrics.ts (2)
158-203: Solid hardening of the snapshot read path.
O_NOFOLLOW,nlink !== 1, size bound, and strict valibot schema validation together provide good defense-in-depth against a tampered or malicious snapshot file.
55-58: 🩺 Stability & AvailabilityVerify
/run/user/<uid>availability for non-interactive systemd services.The production fallback path derives the runtime root as
/run/user/${uid}whenXDG_RUNTIME_DIRisn't set. This directory is typically created bypam_systemd/logind for login sessions, or via a unit'sRuntimeDirectory=/PAMName=loginsetting for services. If the worker/web systemd units don't guarantee this directory exists,mkdirSyncunder/runmay fail (parent likely root-owned, mode 0755), silently disabling the metrics feature (caught by the try/catch call sites, so no crash, just no metrics).Please confirm the managed systemd unit files (from the "Managed dashboard units" cohort) set
XDG_RUNTIME_DIRor otherwise guarantee/run/user/<uid>exists for the worker/web services.Also applies to: 67-76
backend/src/services/jobWorker.ts (1)
8-11: LGTM!Also applies to: 94-109, 119-119, 130-130, 153-153
backend/test/cacheRefreshMetrics.test.ts (1)
1-176: LGTM!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e61cffe2b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/src/services/cacheRefreshMetrics.ts (1)
233-246: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake snapshot ownership verification atomic with deletion.
The instance-ID check at Line 237 is not atomic with
unlinkSync()at Line 240. During a worker restart, a new worker can publish a replacement snapshot after the old worker reads instance ID A but before it unlinks the path, causing the old worker to delete the new worker’s snapshot and temporarily remove production metrics.Serialize publication and cleanup with the same inter-process lock, or redesign cleanup around instance-qualified snapshot paths and an atomic ownership mechanism.
🤖 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 `@backend/src/services/cacheRefreshMetrics.ts` around lines 233 - 246, The stopCacheRefreshMetricsSession cleanup must make snapshot ownership verification atomic with deletion to prevent an old worker removing a replacement snapshot. Serialize the read/check and fs.unlinkSync operation using the same inter-process lock as snapshot publication, or redesign the flow to use instance-qualified paths with atomic ownership; preserve cleanup of only the session-owned snapshot.backend/src/services/pullRequests.ts (1)
2126-2127: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse bounded, explicit runtime probes in both release paths.
Both release validators execute Bun synchronously without a timeout, allowing a broken runtime to block operations. A failing probe also needs explicit handling so callers receive the intended validation failure instead of an uncontrolled subprocess status.
backend/src/services/pullRequests.ts#L2126-L2127: wrap--revisionin bounded execution and return failure explicitly.scripts/runManagedDashboardRelease.sh#L78-L80: use bounded execution inside aniffailure branch and exit78.🤖 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 `@backend/src/services/pullRequests.ts` around lines 2126 - 2127, Update the release validators at backend/src/services/pullRequests.ts:2126-2127 and scripts/runManagedDashboardRelease.sh:78-80 to run the Bun --revision probe with bounded execution. In the pullRequests validator, explicitly return failure when the probe fails; in runManagedDashboardRelease.sh, place the bounded probe in an if failure branch and exit with status 78.
🧹 Nitpick comments (2)
scripts/productionBootstrap.ts (2)
214-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
systemctl is-enabledexits non-zero for disabled units, so Lines 220-222 never report the intended message.
commandRunnerthrows on any non-zero exit, so a disabled unit surfaces as a genericsystemctl … failed with exit code 1instead of<name> was not persistently enabled. Tolerate the non-zero exit here and rely on the stdout comparison for the diagnostic.🤖 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 `@scripts/productionBootstrap.ts` around lines 214 - 222, The MANAGED_DASHBOARD_UNIT_NAMES validation loop currently lets commandRunner throw before producing the intended disabled-unit diagnostic. Update the commandRunner invocation in this loop to tolerate non-zero exits, then retain the stdout.trim() comparison and throw `${name} was not persistently enabled` when the result is not “enabled”.
349-358: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed 2s stabilization sleep with bounded polling.
A single check after a hardcoded delay makes bootstrap flaky: a unit still in
activating(or a slower host) fails the run even though it would settle shortly after, and a unit that crash-loops after 2s still passes. PollverifyEnabledServicesuntil a deadline instead.♻️ Sketch
- if (stabilizationMs > 0) { - await Bun.sleep(stabilizationMs); - } - const services = await verifyEnabledServices(commandRunner); + const deadline = Date.now() + stabilizationMs; + let services: ProductionBootstrapResult["services"]; + while (true) { + try { + services = await verifyEnabledServices(commandRunner); + break; + } catch (error) { + if (Date.now() >= deadline) { + throw error; + } + await Bun.sleep(250); + } + }Note this changes
serviceStabilizationMssemantics from "delay" to "budget"; the existing non-negative validation still applies.🤖 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 `@scripts/productionBootstrap.ts` around lines 349 - 358, Replace the fixed stabilization sleep in the dashboard service startup flow with bounded polling: repeatedly call verifyEnabledServices(commandRunner) until all services are settled and healthy, or until the serviceStabilizationMs budget expires. Preserve the existing non-negative validation, ensure the final verification result is used, and retain the timeout failure behavior for services that remain activating or become unhealthy.
🤖 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 `@scripts/bootstrapProduction.sh`:
- Around line 21-30: Update the linger re-check in the bootstrap flow after
enable-linger to tolerate loginctl failure, mirroring the existing guarded
assignment to linger_state. Ensure failures reach the subsequent “systemd linger
was not enabled” validation and actionable error message instead of aborting
under set -e.
- Around line 8-18: Normalize MIRA_DASHBOARD_PROJECT_ROOT using the same
trimmed, physical-path semantics as resolveDashboardProjectPaths before
constructing expected_checkout. Resolve symlinks, remove trailing slashes, and
canonicalize .. segments, then compare the resulting production/checkout path
with repository_root while preserving the existing validation behavior.
In `@scripts/productionBootstrap.ts`:
- Around line 145-157: Update initializeProductionBootstrapDatabase to validate
the PRAGMA quick_check result by scanning its returned row values
case-insensitively for the value "ok", rather than accessing
quickCheck.quick_check. Preserve the existing error and database.close behavior
when validation fails.
---
Outside diff comments:
In `@backend/src/services/cacheRefreshMetrics.ts`:
- Around line 233-246: The stopCacheRefreshMetricsSession cleanup must make
snapshot ownership verification atomic with deletion to prevent an old worker
removing a replacement snapshot. Serialize the read/check and fs.unlinkSync
operation using the same inter-process lock as snapshot publication, or redesign
the flow to use instance-qualified paths with atomic ownership; preserve cleanup
of only the session-owned snapshot.
In `@backend/src/services/pullRequests.ts`:
- Around line 2126-2127: Update the release validators at
backend/src/services/pullRequests.ts:2126-2127 and
scripts/runManagedDashboardRelease.sh:78-80 to run the Bun --revision probe with
bounded execution. In the pullRequests validator, explicitly return failure when
the probe fails; in runManagedDashboardRelease.sh, place the bounded probe in an
if failure branch and exit with status 78.
---
Nitpick comments:
In `@scripts/productionBootstrap.ts`:
- Around line 214-222: The MANAGED_DASHBOARD_UNIT_NAMES validation loop
currently lets commandRunner throw before producing the intended disabled-unit
diagnostic. Update the commandRunner invocation in this loop to tolerate
non-zero exits, then retain the stdout.trim() comparison and throw `${name} was
not persistently enabled` when the result is not “enabled”.
- Around line 349-358: Replace the fixed stabilization sleep in the dashboard
service startup flow with bounded polling: repeatedly call
verifyEnabledServices(commandRunner) until all services are settled and healthy,
or until the serviceStabilizationMs budget expires. Preserve the existing
non-negative validation, ensure the final verification result is used, and
retain the timeout failure behavior for services that remain activating or
become unhealthy.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bfee481e-f1cb-4c91-84f1-3df0ba2658a5
📒 Files selected for processing (26)
backend/src/lib/systemdProperties.tsbackend/src/managedBunRuntime.tsbackend/src/managedDashboardSystemd.tsbackend/src/releaseDeployment.tsbackend/src/services/cacheRefreshMetrics.tsbackend/src/services/pullRequestPreviewHost.tsbackend/src/services/pullRequests.tsbackend/src/services/scheduledJobs.tsbackend/test/databaseOverview.test.tsbackend/test/jobExecutionQueue.test.tsbackend/test/managedBunRuntime.test.tsbackend/test/managedDashboardSystemd.test.tsbackend/test/productionBootstrap.test.tsbackend/test/releaseManager.test.tsbackend/test/releaseManifest.test.tsbackend/test/routeAndServiceBehavior.test.tsbackend/test/support/rejections.tsdocs/operations/scheduler-cache-backups.mddocs/setup/new-vps.mddocs/setup/production-deploy.mdfrontend/src/test/componentBehavior.test.tsxfrontend/src/test/pageBehavior.test.tsxpackage.jsonscripts/bootstrapProduction.shscripts/productionBootstrap.tsscripts/runManagedDashboardRelease.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/setup/new-vps.md
- docs/setup/production-deploy.md
- docs/operations/scheduler-cache-backups.md
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.45.0)
backend/src/managedBunRuntime.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (27)
backend/src/services/scheduledJobs.ts (1)
16-16: LGTM!Also applies to: 1290-1291, 1513-1515
frontend/src/test/componentBehavior.test.tsx (1)
3421-3426: LGTM!Also applies to: 3470-3481, 3526-3552
frontend/src/test/pageBehavior.test.tsx (1)
1511-1524: LGTM!backend/test/jobExecutionQueue.test.ts (1)
27-27: LGTM!Also applies to: 49-49, 607-635, 637-659
backend/test/routeAndServiceBehavior.test.ts (1)
1352-1361: LGTM!Also applies to: 1364-1373, 1385-1397
backend/src/services/cacheRefreshMetrics.ts (1)
1-20: LGTM!Also applies to: 22-48, 49-77, 79-105, 108-132, 134-147, 149-207, 209-227, 248-266, 268-285, 287-304, 306-310
backend/test/databaseOverview.test.ts (1)
240-240: LGTM!package.json (1)
17-17: LGTM!scripts/bootstrapProduction.sh (1)
32-43: LGTM!scripts/productionBootstrap.ts (2)
90-143: LGTM!Also applies to: 159-208, 266-348, 368-385
240-248: 🎯 Functional CorrectnessNo change needed.
The managed units are both long-running
Type=simpleservices, soSubState === "running"is expected for the health check.backend/src/managedBunRuntime.ts (2)
10-11: LGTM!Also applies to: 29-30, 44-46, 146-147, 165-177, 185-189
121-125: 🔒 Security & PrivacyNo executable-path trust issue here.
The path used for the identity check originates from
resolveDashboardReleaseBuildBunExecutable(), and installable managed binaries are constructed throughmanagedBunRuntimeExecutablePath()and validated before execution.backend/src/services/pullRequests.ts (1)
2121-2121: LGTM!scripts/runManagedDashboardRelease.sh (1)
58-58: LGTM!backend/test/managedBunRuntime.test.ts (1)
45-47: LGTM!Also applies to: 63-67, 91-101, 116-120, 130-136, 262-290
backend/src/managedDashboardSystemd.ts (1)
9-9: LGTM!Also applies to: 124-160
backend/src/releaseDeployment.ts (1)
9-9: LGTM!Also applies to: 282-282
backend/src/lib/systemdProperties.ts (1)
1-20: LGTM!backend/src/services/pullRequestPreviewHost.ts (1)
36-36: LGTM!Also applies to: 1296-1303
backend/test/productionBootstrap.test.ts (2)
85-303: LGTM!
66-83: 🗄️ Data Integrity & IntegrationNo change needed for the database initialization call.
initializeProductionBootstrapDatabase()uses the shared test database path guard, so it does not default to the production SQLite file when run in the test environment.> Likely an incorrect or invalid review comment.backend/test/releaseManager.test.ts (2)
707-744: 🎯 Functional Correctness | ⚡ Quick winUn-awaited
rejects.toThrowassertion foractivateDashboardRelease.Line 717-722's
expect(activateDashboardRelease(...)).rejects.toThrow(...)is missingawait, so the assertion isn't verified before the test proceeds — the same anti-pattern already fixed elsewhere in this file (see lines 986-1007) and inreleaseManifest.test.ts. A failed expectation here would only surface as an unhandled rejection rather than a test failure.🐛 Proposed fix
- expect( + await expect( activateDashboardRelease(FIRST_COMMIT, runtimeRoot, { ...SCHEMA_6_OPTIONS, hasRuntime: () => false, }) ).rejects.toThrow("requires unavailable managed Bun runtime 0.0.0+missing");
55-55: LGTM!Also applies to: 986-1008, 1169-1181
backend/test/support/rejections.ts (1)
1-16: LGTM!backend/test/managedDashboardSystemd.test.ts (1)
21-21: LGTM!Also applies to: 91-99
backend/test/releaseManifest.test.ts (1)
31-31: LGTM!Also applies to: 318-322, 582-582
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c7ceb6ccc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
2 participants
Summary
bun run deploy:bootstrapas the complete first managed activation on a fresh VPS, including linger, SQLite, exact Bun runtime, units, and service enable/restart;Behavior and regression coverage
0644, runsystemctl --user daemon-reload, and reject unexpected fragment paths or drop-ins. Preparation failures and failed release-link transitions restore the previous unit contents and modes.GET /api/job-executionsresponses remain unchanged; the UI opts into claim state with?include=claims.nullavatar URLs and feed display data normalizes them to an absent avatar.+revisionsuffix in manifests, cache validation, the managed launcher, and the deployment guardian; executable probes are time-bounded.Review follow-up
Verification
bun run lintbun run format:checkbun run build:frontendbun run build:backendgit diff --checkRisk checklist
.envfiles, database dumps, or runtime state committedVisible UI change: the Execution queue card adds a worker status badge, Pause/Resume button, and warning while new claims are paused.
Deployment / operations
The first rollout is intentionally manual: the currently running pre-bundle stager cannot parse the candidate's new systemd artifacts and therefore cannot complete normal staging. Preserve the installed unit files and cutover database snapshot, stage and cache the candidate runtime with the merged code, and use the previous release lifecycle for recovery if cutover fails. The immediately previous pre-bundle slot remains manual-only until a second bundled release rotates it out.
On a fresh VPS, configure the normal external prerequisites and secrets, clone into the documented production checkout, then run
bun run deploy:bootstrap. No separate SQLite, lifecycle, systemd-install,daemon-reload, enable, start, or restart commands are required.Notes for reviewers
Please focus on the preparation hook's placement inside the release transition lock, the fresh-host bootstrap boundary, the claim pause check under SQLite's
BEGIN IMMEDIATE, and the owner-only cache metrics snapshot lifecycle.