Complete atomic Dashboard release cutover - #334
Conversation
mira-2026
added
priority: high
|
Warning Review limit reached
Next review available in: 59 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThe PR adds immutable dashboard release staging, atomic cutover and rollback, release retention pruning, format-2 manifest enforcement, managed systemd validation, orphaned-cutover reconciliation, health-route changes, configurable log-rotation paths, and related tests and documentation. ChangesManaged release platform
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! |
![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: b81e27c689
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
backend/src/services/logRotation.ts (1)
71-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate absolute/non-root path validator.
This is the same rule as
resolveAbsoluteNonRootPathinbackend/src/releaseDeployment.ts(lines 80-90), minus thetrim()-before-isAbsoluteordering. Two copies of a security-relevant validator will drift; extract one shared helper (e.g. intolib/) and have both call it. Minor nit alongside:path.resolve(configured)is recomputed three times.♻️ Sketch
function resolveLogRotationLockFile(): string { - const configured = - process.env.MIRA_DASHBOARD_LOG_ROTATION_LOCK_FILE?.trim() || DEFAULT_LOCK_FILE; - if ( - configured.includes("\0") || - !path.isAbsolute(configured) || - path.resolve(configured) === path.parse(path.resolve(configured)).root - ) { - throw new TypeError( - "MIRA_DASHBOARD_LOG_ROTATION_LOCK_FILE must be an absolute non-root path" - ); - } - return path.resolve(configured); + return resolveAbsoluteNonRootPath( + process.env.MIRA_DASHBOARD_LOG_ROTATION_LOCK_FILE?.trim() || DEFAULT_LOCK_FILE, + "MIRA_DASHBOARD_LOG_ROTATION_LOCK_FILE" + ); }🤖 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/logRotation.ts` around lines 71 - 86, Extract the absolute, non-root path validation from resolveLogRotationLockFile and resolveAbsoluteNonRootPath into one shared helper, preserving trimming, null-byte rejection, absolute-path enforcement, and non-root rejection. Update both callers to use the shared helper and reuse a single resolved-path computation within it instead of repeatedly calling path.resolve.systemd/mira-dashboard-worker.service (1)
9-18: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winManaged-path contract is hand-duplicated across both unit files with no single source of truth.
assertManagedDashboardUnitPropertiesrequires exact string equality againstmanagedDashboardUnitContract(), yet the same six paths are maintained independently in two unit files and again asDEFAULT_DASHBOARD_*constants inbackend/src/releaseDeployment.ts. A single typo in any copy fails the contract gate and blocks every deployment, and there is no test asserting the shipped unit files satisfy the contract.
systemd/mira-dashboard-worker.service#L9-L18: verifyWorkingDirectoryand all fiveEnvironment=values matchmanagedDashboardUnitContract()exactly, and add a unit test that feeds this file's parsed properties intoassertManagedDashboardUnitProperties("mira-dashboard-worker.service", …).systemd/mira-dashboard.service#L9-L18: apply the identical verification and add the equivalent assertion for"mira-dashboard.service", so drift in either file is caught in CI rather than at deploy time.🤖 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 `@systemd/mira-dashboard-worker.service` around lines 9 - 18, Verify WorkingDirectory and all five Environment values in systemd/mira-dashboard-worker.service lines 9-18 against managedDashboardUnitContract(), then add a unit test parsing that file and passing its properties to assertManagedDashboardUnitProperties("mira-dashboard-worker.service", …). Apply the same verification and equivalent assertion for systemd/mira-dashboard.service lines 9-18 using "mira-dashboard.service", ensuring both shipped unit files are covered by CI.backend/test/releaseDeployment.test.ts (1)
254-307: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConcurrency test can hang instead of failing.
buildsReleasedonly resolves whendeploy:prepareis reached twice. If either staging run fails earlier (or short-circuits via release reuse), the other awaits forever and the test hangs until the suite timeout, with no useful diagnostic. Consider racing the barrier with a bounded timeout.♻️ Bounded barrier
- buildsReady += 1; - if (buildsReady === 2) { - releaseBuilds(); - } - await buildsReleased; + buildsReady += 1; + if (buildsReady === 2) { + releaseBuilds(); + } + await Promise.race([ + buildsReleased, + Bun.sleep(5000).then(() => { + throw new Error("concurrent build barrier timed out"); + }), + ]);🤖 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/releaseDeployment.test.ts` around lines 254 - 307, Make the concurrency barrier in the “accepts a concurrently published copy of the same verified release” test bounded so buildsReleased cannot wait indefinitely if either stageDashboardRelease call exits early or fails. Race the deploy:prepare synchronization wait against a short timeout, while preserving the existing two-build coordination and shared-path assertions.backend/test/serviceBehavior.test.ts (1)
50-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRelease fixture is duplicated across test files.
createDeploymentReleaseFixtureis near-identical tocreateBuiltReleaseinbackend/test/releaseDeployment.test.ts(same directory tree, lockfiles, build-identity payloads, entrypoint list, manifest write). Extracting a shared helper (e.g.backend/test/support/releaseFixture.ts) keeps the artifact contract in one place as the manifest format evolves.🤖 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/serviceBehavior.test.ts` around lines 50 - 109, Extract createDeploymentReleaseFixture and the shared release-artifact setup into a reusable helper under the test support area, then update serviceBehavior.test.ts and releaseDeployment.test.ts to use it. Preserve the existing directory tree, lockfiles, build-identity payloads, entrypoint list, and writeReleaseManifest inputs so both tests continue validating the same release contract from one implementation.backend/src/releaseManager.ts (2)
1257-1262: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRetention ordering falls back to commit SHA when
builtAtties, and the tests bake that in. Sorting compares ISObuiltAtwithlocaleCompareand breaks ties by descending SHA, which is unrelated to recency; the prune test creates all fixtures with the samebuiltAt, so it validates the tie-break rather than newest-first retention.
backend/src/releaseManager.ts#L1257-L1262: comparebuiltAtwith plain</>and pick a meaningful tie-break (e.g. directory birth time) instead of SHA order.backend/test/releaseManager.test.ts#L830-L851: give each managed release fixture a distinctbuiltAtso the retained ordering assertion exercises recency.🤖 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/releaseManager.ts` around lines 1257 - 1262, Update release ordering in backend/src/releaseManager.ts lines 1257-1262 to compare ISO builtAt values with direct chronological < or > comparisons and replace the commitSha tie-breaker with a meaningful recency tie-break such as directory birth time. In backend/test/releaseManager.test.ts lines 830-851, assign distinct builtAt values to each managed release fixture so the retention assertion verifies newest-first ordering; both sites require changes.
1257-1262: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTie-break on equal
builtAtis arbitrary.
builtAtis an ISO string compared withlocaleCompare, then ties fall back to descending commit SHA — a lexicographic SHA has no relation to recency, so two releases built in the same second can be ordered wrongly and the newer one pruned. Plain</>comparison on the ISO strings is also cheaper and locale-independent.🤖 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/releaseManager.ts` around lines 1257 - 1262, Update the sorting callback that creates newestFirst to compare ISO builtAt values with direct lexicographic ordering rather than localeCompare, and replace the commitSha tie-break with a recency-based field or ordering that reliably identifies which equal-timestamp release is newer. Preserve newest-first ordering so pruning retains the newer release.
🤖 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/src/releaseDeployment.ts`:
- Around line 411-418: Update the environment object in the release deployment
flow to explicitly set or remove MIRA_DASHBOARD_RELEASE_ROOT so the detached
build cannot inherit the service’s active-release value; ensure any default
release-root lookup resolves to the worktree being built, alongside the existing
MIRA_DASHBOARD_RELEASES_ROOT handling.
In `@backend/src/releaseManager.ts`:
- Around line 1228-1255: Update pruneDashboardReleases so an unverifiable
non-protected SHA-named release does not abort retention cleanup: catch
validation failures from loadManagedReleaseFromLayout, warn, and treat that
release as prunable or otherwise continue processing. Preserve error propagation
for protected releases, and ensure retired-entry cleanup still proceeds.
In `@backend/src/services/logRotation.ts`:
- Around line 2048-2059: Update elevatedLogRotationEnvironment and the elevated
log-rotation locking flow to preserve and use
MIRA_DASHBOARD_LOG_ROTATION_LOCK_FILE instead of falling back to the module’s
default lock path. Ensure elevated and non-elevated execution share the
configured lock file, and add coverage exercising both paths with distinct
configured lock files to verify no second default lock is created.
In `@backend/src/services/pullRequests.ts`:
- Line 1537: The readiness checks must use the configured effective Dashboard
port instead of hardcoded 3100. In backend/src/services/pullRequests.ts lines
1537-1537, update the readiness URL construction in the relevant cutover path to
use the validated service-port value; in docs/setup/production-deploy.md lines
182-188, define and use the same effective port in ready_for_commit. Ensure both
cutover paths consistently probe the configured port.
In `@docs/setup/production-deploy.md`:
- Around line 83-87: Update the “One-Time Managed Cutover” instructions to
require the managed cutover directly after merging the control checkout, without
deploying through or relying on the old in-place executor. Remove the
prerequisite that the old deployment must build or run this change, while
preserving the idle Jobs queue and known-good bootstrap release requirements.
- Around line 174-197: Update docs/setup/production-deploy.md lines 174-197
around ready_for_commit and "$BOOTSTRAP_SHA" so a failed bootstrap readiness
check enters the documented unit/state recovery flow and exits 1. Update lines
227-250 to explicitly invoke ready_for_commit for the candidate, and on failure
roll back, verify bootstrap readiness with ready_for_commit, then exit nonzero
before cleanup.
---
Nitpick comments:
In `@backend/src/releaseManager.ts`:
- Around line 1257-1262: Update release ordering in
backend/src/releaseManager.ts lines 1257-1262 to compare ISO builtAt values with
direct chronological < or > comparisons and replace the commitSha tie-breaker
with a meaningful recency tie-break such as directory birth time. In
backend/test/releaseManager.test.ts lines 830-851, assign distinct builtAt
values to each managed release fixture so the retention assertion verifies
newest-first ordering; both sites require changes.
- Around line 1257-1262: Update the sorting callback that creates newestFirst to
compare ISO builtAt values with direct lexicographic ordering rather than
localeCompare, and replace the commitSha tie-break with a recency-based field or
ordering that reliably identifies which equal-timestamp release is newer.
Preserve newest-first ordering so pruning retains the newer release.
In `@backend/src/services/logRotation.ts`:
- Around line 71-86: Extract the absolute, non-root path validation from
resolveLogRotationLockFile and resolveAbsoluteNonRootPath into one shared
helper, preserving trimming, null-byte rejection, absolute-path enforcement, and
non-root rejection. Update both callers to use the shared helper and reuse a
single resolved-path computation within it instead of repeatedly calling
path.resolve.
In `@backend/test/releaseDeployment.test.ts`:
- Around line 254-307: Make the concurrency barrier in the “accepts a
concurrently published copy of the same verified release” test bounded so
buildsReleased cannot wait indefinitely if either stageDashboardRelease call
exits early or fails. Race the deploy:prepare synchronization wait against a
short timeout, while preserving the existing two-build coordination and
shared-path assertions.
In `@backend/test/serviceBehavior.test.ts`:
- Around line 50-109: Extract createDeploymentReleaseFixture and the shared
release-artifact setup into a reusable helper under the test support area, then
update serviceBehavior.test.ts and releaseDeployment.test.ts to use it. Preserve
the existing directory tree, lockfiles, build-identity payloads, entrypoint
list, and writeReleaseManifest inputs so both tests continue validating the same
release contract from one implementation.
In `@systemd/mira-dashboard-worker.service`:
- Around line 9-18: Verify WorkingDirectory and all five Environment values in
systemd/mira-dashboard-worker.service lines 9-18 against
managedDashboardUnitContract(), then add a unit test parsing that file and
passing its properties to
assertManagedDashboardUnitProperties("mira-dashboard-worker.service", …). Apply
the same verification and equivalent assertion for
systemd/mira-dashboard.service lines 9-18 using "mira-dashboard.service",
ensuring both shipped unit files are covered by CI.
🪄 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: 6359a8d8-68b5-4ca6-a853-9aa2b0bb2a12
📒 Files selected for processing (28)
backend/src/releaseDeployment.tsbackend/src/releaseLifecycle.tsbackend/src/releaseManager.tsbackend/src/releaseManifest.tsbackend/src/requestPolicy.tsbackend/src/routes.tsbackend/src/services/logRotation.tsbackend/src/services/pullRequests.tsbackend/test/bunNativeServerBehavior.test.tsbackend/test/healthReadiness.test.tsbackend/test/httpApiBehavior.test.tsbackend/test/releaseDeployment.test.tsbackend/test/releaseManager.test.tsbackend/test/releaseManifest.test.tsbackend/test/serviceBehavior.test.tsbackend/test/testDatabaseGuard.test.tsdocs/api/overview.mddocs/architecture/database.mddocs/architecture/gateway-and-chat.mddocs/index.mddocs/operations/runbooks.mddocs/operations/scheduler-cache-backups.mddocs/security/auth-and-trust-boundaries.mddocs/setup/new-vps.mddocs/setup/production-deploy.mddocs/setup/secrets-and-env.mdsystemd/mira-dashboard-worker.servicesystemd/mira-dashboard.service
💤 Files with no reviewable changes (6)
- docs/api/overview.md
- backend/src/routes.ts
- backend/test/httpApiBehavior.test.ts
- backend/src/requestPolicy.ts
- backend/test/bunNativeServerBehavior.test.ts
- backend/test/releaseManifest.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: frontend-checks
- GitHub Check: backend-checks
- GitHub Check: Analyze JavaScript and TypeScript
🧰 Additional context used
🪛 ast-grep (0.44.1)
backend/test/releaseManager.test.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)
backend/src/releaseManager.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)
backend/src/releaseDeployment.ts
[warning] 96-96: Do not use variable for regular expressions
Context: new RegExp(String.raw(?:^|[\s"])${escaped}(?=$|[\s"]), "u")
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
[warning] 103-103: Do not use variable for regular expressions
Context: new RegExp(String.raw(?:^|[\s";])${escaped}(?=$|[\s";]), "u")
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
🔇 Additional comments (30)
backend/src/services/pullRequests.ts (1)
12-23: LGTM!Also applies to: 1464-1485, 1584-1634, 1636-1664
docs/setup/new-vps.md (1)
46-122: LGTM!Also applies to: 261-261
docs/setup/production-deploy.md (1)
3-81: LGTM!Also applies to: 129-172, 252-386
backend/test/testDatabaseGuard.test.ts (1)
147-156: LGTM!docs/architecture/database.md (1)
11-19: LGTM!Also applies to: 74-107, 154-156, 170-174, 200-204
docs/architecture/gateway-and-chat.md (1)
620-621: LGTM!docs/index.md (1)
51-55: LGTM!docs/operations/runbooks.md (1)
63-67: LGTM!Also applies to: 76-77, 87-87, 112-114, 160-167, 191-208, 278-282
docs/operations/scheduler-cache-backups.md (1)
235-250: LGTM!Also applies to: 260-260
docs/security/auth-and-trust-boundaries.md (1)
113-114: LGTM!docs/setup/secrets-and-env.md (1)
26-63: LGTM!backend/src/releaseDeployment.ts (8)
23-107: LGTM!
109-156: LGTM!
158-214: LGTM!
216-304: LGTM!
306-370: LGTM!
541-551: LGTM!
532-535: 🗄️ Data Integrity & IntegrationNo change needed for
retainCountvalidation.
prunePublishedDashboardReleasesdelegates topruneDashboardReleases, which rejects non-safe integers and counts outside2..20, so invalid CLI values like non-numeric, negative, fractional, zero, or empty input fail before prune runs.> Likely an incorrect or invalid review comment.
445-465: 🩺 Stability & AvailabilityNo change needed.
NODE_ENV=productiondoes not makebun installomit devDependencies;bun install --productionor--omit=devis required for production-only installs.backend/src/releaseManifest.ts (4)
29-39: LGTM!Also applies to: 63-71
510-558: LGTM!
574-574: LGTM!Also applies to: 610-612, 629-629
763-765: LGTM!backend/test/healthReadiness.test.ts (1)
22-22: LGTM!Also applies to: 46-46
backend/test/releaseDeployment.test.ts (1)
1-213: LGTM!Also applies to: 309-535
backend/test/serviceBehavior.test.ts (1)
9-9: LGTM!Also applies to: 21-27, 1879-1915, 1926-1939, 1957-1996, 2057-2104
backend/src/releaseManager.ts (2)
32-33: LGTM!Also applies to: 58-62, 407-416, 1211-1227, 1263-1317
645-663: 🩺 Stability & AvailabilityNo change needed.
schema.migrationsis non-optional inDashboardReleaseManifest, generated byparseSchema, and included bycreateReleaseManifest, so this access path does not rely on an absent field.> Likely an incorrect or invalid review comment.backend/src/releaseLifecycle.ts (1)
7-7: LGTM!Also applies to: 74-81
backend/test/releaseManager.test.ts (1)
33-33: LGTM!Also applies to: 52-52, 387-397, 862-873
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdebc02232
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bb231386e
ℹ️ 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.
🧹 Nitpick comments (1)
backend/src/services/logRotation.ts (1)
72-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deduplicating the two preserved-env allowlists.
ELEVATED_LOG_ROTATION_PRESERVED_ENVIRONMENT(sudo--preserve-env) and theallowedarray inelevatedLogRotationEnvironment()(env forwarded to the spawned bun process) share most entries (LANG,NODE_ENV,TZ,MIRA_DASHBOARD_DB_PATH,MIRA_DASHBOARD_LOG_ROTATION_LOCK_FILE), differing only byPATH/HOME. Keeping them as separate literals risks drift if a future variable needs to be added to only one list.♻️ Suggested consolidation
-const ELEVATED_LOG_ROTATION_PRESERVED_ENVIRONMENT = [ - "LANG", - "NODE_ENV", - "TZ", - "MIRA_DASHBOARD_DB_PATH", - "MIRA_DASHBOARD_LOG_ROTATION_LOCK_FILE", -] as const; +const ELEVATED_LOG_ROTATION_FORWARDED_VARIABLES = [ + "LANG", + "NODE_ENV", + "TZ", + "MIRA_DASHBOARD_DB_PATH", + "MIRA_DASHBOARD_LOG_ROTATION_LOCK_FILE", +] as const; +const ELEVATED_LOG_ROTATION_PRESERVED_ENVIRONMENT = ELEVATED_LOG_ROTATION_FORWARDED_VARIABLES;function elevatedLogRotationEnvironment(): NodeJS.ProcessEnv { - const allowed = [ - "PATH", - "HOME", - "LANG", - "NODE_ENV", - "TZ", - "MIRA_DASHBOARD_DB_PATH", - "MIRA_DASHBOARD_LOG_ROTATION_LOCK_FILE", - ]; + const allowed = ["PATH", "HOME", ...ELEVATED_LOG_ROTATION_FORWARDED_VARIABLES];Also applies to: 2053-2062
🤖 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/logRotation.ts` around lines 72 - 78, Deduplicate the shared environment names used by ELEVATED_LOG_ROTATION_PRESERVED_ENVIRONMENT and the allowed list in elevatedLogRotationEnvironment(). Define the common entries once and derive or reuse them in both flows, while retaining PATH and HOME only where required by the spawned bun process.
🤖 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 `@backend/src/services/logRotation.ts`:
- Around line 72-78: Deduplicate the shared environment names used by
ELEVATED_LOG_ROTATION_PRESERVED_ENVIRONMENT and the allowed list in
elevatedLogRotationEnvironment(). Define the common entries once and derive or
reuse them in both flows, while retaining PATH and HOME only where required by
the spawned bun process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c806bbf-8110-4f85-9ff1-9e73ef759c94
📒 Files selected for processing (18)
backend/package.jsonbackend/src/lib/safePath.tsbackend/src/lib/values.tsbackend/src/releaseDeployment.tsbackend/src/releaseManager.tsbackend/src/server.tsbackend/src/services/logRotation.tsbackend/src/services/pullRequests.tsbackend/test/bunNativeServerBehavior.test.tsbackend/test/releaseDeployment.test.tsbackend/test/releaseManager.test.tsbackend/test/serviceBehavior.test.tsbackend/test/support/releaseFixture.tsbackend/test/utilityBehavior.test.tsdocs/operations/runbooks.mddocs/setup/production-deploy.mdsystemd/mira-dashboard-worker.servicesystemd/mira-dashboard.service
🚧 Files skipped from review as they are similar to previous changes (6)
- systemd/mira-dashboard.service
- systemd/mira-dashboard-worker.service
- backend/src/releaseDeployment.ts
- backend/src/services/pullRequests.ts
- docs/operations/runbooks.md
- docs/setup/production-deploy.md
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: frontend-checks
- GitHub Check: backend-checks
🧰 Additional context used
🪛 ast-grep (0.44.1)
backend/src/services/logRotation.ts
[error] 2064-2068: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const key of allowed) {
if (process.env[key] !== undefined) {
environment[key] = process.env[key];
}
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').
(prototype-pollution-recursive-merge-typescript)
backend/src/releaseManager.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)
[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)
backend/test/releaseManager.test.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)
[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 (18)
backend/src/releaseManager.ts (3)
1202-1340: 🩺 Stability & AvailabilityRetention pruning: past critical retention-abort bug is now resolved.
The previously-flagged issue (an unverifiable non-protected release directory aborting
prune 3entirely) is fixed here:loadManagedReleaseFromLayoutfailures for non-protected commits are now caught, recorded inwarnings, and treated as prunable-skip candidates, while protected (current/previous) commit failures still re-throw. Sort/tie-break, TOCTOU re-verification before rename, post-rename inode identity check, and retired-directory cleanup all look correct.
16-16: LGTM!Also applies to: 27-34, 59-64, 322-352, 354-361, 394-396
636-654: 🗄️ Data Integrity & Integrationschema.migrations is already required.
backend/test/releaseManager.test.ts (3)
831-905: 🩺 Stability & AvailabilityGood coverage confirming the retention-abort fix.
This test directly exercises the previously-reported scenario (unverifiable release directory + interrupted retirement leftovers) and asserts pruning still proceeds, preserves current/previous, and surfaces a warning instead of throwing. Matches the fixed behavior in
releaseManager.ts.
114-180: LGTM!
388-399: LGTM!backend/test/releaseDeployment.test.ts (2)
1-548: LGTM!
30-43: 🎯 Functional CorrectnessNo change needed.
The shipped units use unquoted
Environment=KEY=VALUElines, somanagedUnitPropertiesdoes not introduce quoted tokens for this test path.backend/test/support/releaseFixture.ts (2)
1-70: LGTM on the rest of the fixture — comprehensive artifact set (manifest, lockfiles, entrypoints, non-artifact noise file) matches the immutable-release publish contract described in the stack context.
42-50: 🗄️ Data Integrity & IntegrationNo change needed.
backend/src/releaseManifest.tsstill validates componentbuild-identity.jsonasformatVersion: 1, so the fixture matches the enforced build-identity schema.backend/test/bunNativeServerBehavior.test.ts (1)
338-341: LGTM! Matches the updatedstaticResponse404 behavior for the retired/healthroute.Also applies to: 520-520
backend/test/serviceBehavior.test.ts (1)
9-9: LGTM! Deployment/log-rotation test updates are internally consistent withreleaseManager.tsandlogRotation.tscontracts (managed layout, env-preservingsudo/dopplerinvocations, detached-cutover ordering assertions).Also applies to: 21-27, 1819-2032, 5008-5103
backend/test/utilityBehavior.test.ts (1)
22-22: LGTM! Assertions matchresolveDashboardPort/resolveAbsoluteNonRootPathimplementations exactly, including boundary cases.Also applies to: 32-32, 410-413, 451-456
backend/src/services/logRotation.ts (1)
7-7: 🩺 Stability & AvailabilityPrior critical lock-file mismatch is fixed.
The elevated
sudopath now preserves and forwardsMIRA_DASHBOARD_LOG_ROTATION_LOCK_FILEconsistently (--preserve-envallowlist +elevatedLogRotationEnvironment()), andresolveLogRotationLockFile()reads the same env var for both elevated and non-elevated runs. This resolves the previously flagged dual-lock-file race.Also applies to: 72-85, 1488-1503, 1526-1527, 1628-1637, 2042-2042, 2053-2071
backend/src/lib/safePath.ts (1)
41-53: LGTM!backend/src/lib/values.ts (1)
13-22: LGTM!backend/src/server.ts (1)
18-18: LGTM!Also applies to: 80-80, 103-103, 234-236
backend/package.json (1)
12-12: LGTM!
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 337d3568ca
ℹ️ 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: 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/releaseManager.ts (1)
1206-1207: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRequire at least three retained releases.
The release contract requires retaining
current,previous, and one additional verified release, but this validation acceptsretainCount = 2. Since the retention set already starts with the two protected commits, that value can remove the additional rollback fallback.Proposed fix
- if (!Number.isSafeInteger(retainCount) || retainCount < 2 || retainCount > 20) { + if (!Number.isSafeInteger(retainCount) || retainCount < 3 || retainCount > 20) {🤖 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/releaseManager.ts` around lines 1206 - 1207, Update the retainCount validation in the release manager to reject values below 3 while preserving the safe-integer check and maximum of 20. Ensure the resulting error message states the managed release retention range as 3 to 20.
🧹 Nitpick comments (1)
backend/src/services/scheduledJobs.ts (1)
1285-1331: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrepared statements rebuilt on every reconcile pass.
database.prepare(...)inside the transaction closure runs per call (every 5s). Hoisting the two statements to module scope (or relying on cachedquery()) trims avoidable parse work. Optional.🤖 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/scheduledJobs.ts` around lines 1285 - 1331, Hoist or cache the UPDATE and DELETE prepared statements used by failOrphanedCutover in reconcileOrphanedDeploymentCutovers so they are not rebuilt for each reconciliation pass or job. Preserve the transaction, status guard, timestamp updates, and lock cleanup behavior.
🤖 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/src/services/scheduledJobs.ts`:
- Around line 1333-1354: Update hasPendingDeploymentCutover and its
reconciliation flow to provide an escape hatch for restart-scheduled rows that
remain stale when guardian checks cannot complete, such as systemctl failures.
Before returning the pending-row gate, clear or otherwise resolve rows exceeding
a bounded age or reconcile-attempt window, while preserving normal
reconciliation for active cutovers and avoiding indefinite worker claim pauses.
- Around line 1262-1283: Update readDeploymentGuardianState to distinguish a
genuine inactive systemd unit from a missing unit or command failure: inspect
result.exitCode and decoded stderr before mapping the "unknown" stdout state to
"inactive". Only return "inactive" for a confirmed non-error inactive/failed
condition; otherwise return "unknown" so reconcileOrphanedDeploymentCutovers()
defers the cutover.
---
Outside diff comments:
In `@backend/src/releaseManager.ts`:
- Around line 1206-1207: Update the retainCount validation in the release
manager to reject values below 3 while preserving the safe-integer check and
maximum of 20. Ensure the resulting error message states the managed release
retention range as 3 to 20.
---
Nitpick comments:
In `@backend/src/services/scheduledJobs.ts`:
- Around line 1285-1331: Hoist or cache the UPDATE and DELETE prepared
statements used by failOrphanedCutover in reconcileOrphanedDeploymentCutovers so
they are not rebuilt for each reconciliation pass or job. Preserve the
transaction, status guard, timestamp updates, and lock cleanup behavior.
🪄 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: 290a1ec9-4043-42bd-aef3-3d5c53c4ce3f
📒 Files selected for processing (8)
backend/src/releaseDeployment.tsbackend/src/releaseManager.tsbackend/src/services/logRotation.tsbackend/src/services/pullRequests.tsbackend/src/services/scheduledJobs.tsbackend/test/jobExecutionQueue.test.tsbackend/test/releaseDeployment.test.tsbackend/test/serviceBehavior.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/test/releaseDeployment.test.ts
- backend/src/services/logRotation.ts
- backend/src/releaseDeployment.ts
- backend/test/serviceBehavior.test.ts
- backend/src/services/pullRequests.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: frontend-checks
- GitHub Check: backend-checks
🧰 Additional context used
🪛 ast-grep (0.44.1)
backend/src/releaseManager.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)
🔇 Additional comments (3)
backend/src/services/scheduledJobs.ts (1)
36-39: LGTM!Also applies to: 56-56, 66-72, 1259-1260
backend/test/jobExecutionQueue.test.ts (2)
1-1: LGTM!Also applies to: 27-27, 40-40, 51-59, 81-130
131-135: 🎯 Functional CorrectnessNo change needed. The project runs tests with
bun testanddatabase.tsimportsbun:sqlite, soStatement.get()returningnullfor no rows is the active driver behavior.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef9315758c
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efb03a3c6a
ℹ️ 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: 1
🧹 Nitpick comments (3)
backend/src/services/scheduledJobs.ts (2)
1418-1429: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPending gate never clears when no recovery handler is registered.
reconcileOrphanedDeploymentCutovers()falls back toscheduledJobRuntimeState.deploymentCutoverRecoveryHandler; if it isundefined(registration skipped, e.g. a worker-only process that never callsregisterPullRequestExecutionActions), everyrestart-scheduledrow is skipped andexecutorTick()pauses claims forever with no diagnostic. A one-time warning when a recoverable cutover is found without a handler would make that state observable.🤖 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/scheduledJobs.ts` around lines 1418 - 1429, The hasPendingDeploymentCutover flow should detect when recoverable restart-scheduled cutovers exist but scheduledJobRuntimeState.deploymentCutoverRecoveryHandler is undefined. Emit a one-time warning identifying the missing recovery handler and affected cutovers, while preserving normal reconciliation and avoiding repeated warnings on every executorTick.
1276-1322: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNon-empty stderr on a successful
systemctl showdegrades tounknown.
systemctl showcan print benign diagnostics to stderr while exiting 0. Gating onstderr ||therefore biases towardunknown, which defers reconciliation until the 10-minute bound expires and then triggers rollback recovery for a possibly-healthy cutover. Consider keying only onexitCode !== 0plus unparseable output, and logging stderr instead.♻️ Suggested change
- const stderr = new TextDecoder().decode(result.stderr).trim(); - if (stderr || result.exitCode !== 0) { + if (result.exitCode !== 0) { + console.warn( + "[ScheduledJobs] systemctl show failed:", + new TextDecoder().decode(result.stderr).trim() + ); return "unknown"; }🤖 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/scheduledJobs.ts` around lines 1276 - 1322, Update readSystemdUnitState to stop treating non-empty stderr as failure when systemctl exits successfully; gate the unknown result on result.exitCode !== 0 and unparseable or missing required output instead. Preserve stderr for diagnostic logging rather than using it to override a successful state determination.backend/test/jobExecutionQueue.test.ts (1)
136-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpired-
unknownpath is only covered via the throwing reader.The post-bound assertion uses a reader that throws, which reaches the same branch only because
statedefaults to"unknown". Add a case with() => "unknown"at03:11so the intended bounded-unknown expiry is asserted directly and stays covered if the catch-block default ever changes.🤖 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/jobExecutionQueue.test.ts` around lines 136 - 166, Extend the test around reconcileOrphanedDeploymentCutovers with a separate post-bound invocation at 03:11 using a reader that returns "unknown" without throwing, and assert it schedules one recovery. Keep the existing throwing-reader case and warning assertions unchanged so the bounded-unknown expiry path is covered independently of catch-block defaults.
🤖 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/src/releaseManager.ts`:
- Around line 1244-1288: Protect the entire staging write, verification, and
activation sequence in the release creation flow with
withReleaseTransitionLock(layout, "exclusive", ...), starting before stagingPath
is created and ending after it is deleted or renamed to finalPath. Ensure
copyVerifiedRelease and all related staging operations execute under this lock
so pruneDashboardReleases cannot remove an in-progress .staging-* directory;
leave the existing cleanup validation around the pruner unchanged.
---
Nitpick comments:
In `@backend/src/services/scheduledJobs.ts`:
- Around line 1418-1429: The hasPendingDeploymentCutover flow should detect when
recoverable restart-scheduled cutovers exist but
scheduledJobRuntimeState.deploymentCutoverRecoveryHandler is undefined. Emit a
one-time warning identifying the missing recovery handler and affected cutovers,
while preserving normal reconciliation and avoiding repeated warnings on every
executorTick.
- Around line 1276-1322: Update readSystemdUnitState to stop treating non-empty
stderr as failure when systemctl exits successfully; gate the unknown result on
result.exitCode !== 0 and unparseable or missing required output instead.
Preserve stderr for diagnostic logging rather than using it to override a
successful state determination.
In `@backend/test/jobExecutionQueue.test.ts`:
- Around line 136-166: Extend the test around
reconcileOrphanedDeploymentCutovers with a separate post-bound invocation at
03:11 using a reader that returns "unknown" without throwing, and assert it
schedules one recovery. Keep the existing throwing-reader case and warning
assertions unchanged so the bounded-unknown expiry path is covered independently
of catch-block defaults.
🪄 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: bd47f614-ad08-42cf-8563-ec91c7b5267e
📒 Files selected for processing (14)
backend/src/releaseDeployment.tsbackend/src/releaseManager.tsbackend/src/services/jobExecutionQueue.tsbackend/src/services/pullRequests.tsbackend/src/services/scheduledJobs.tsbackend/test/jobExecutionQueue.test.tsbackend/test/releaseDeployment.test.tsbackend/test/releaseManager.test.tsbackend/test/serviceBehavior.test.tsdocs/setup/production-deploy.mddocs/setup/secrets-and-env.mdsrc/pages/PullRequests.tsxsystemd/mira-dashboard-worker.servicesystemd/mira-dashboard.service
🚧 Files skipped from review as they are similar to previous changes (8)
- systemd/mira-dashboard.service
- systemd/mira-dashboard-worker.service
- docs/setup/secrets-and-env.md
- backend/test/releaseDeployment.test.ts
- backend/src/releaseDeployment.ts
- backend/test/serviceBehavior.test.ts
- docs/setup/production-deploy.md
- backend/src/services/pullRequests.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: backend-checks
- GitHub Check: frontend-checks
🧰 Additional context used
🪛 ast-grep (0.44.1)
backend/test/releaseManager.test.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)
backend/src/releaseManager.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)
🪛 OpenGrep (1.25.0)
backend/src/releaseManager.ts
[ERROR] 1244-1244: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (10)
src/pages/PullRequests.tsx (1)
302-302: LGTM!backend/src/services/scheduledJobs.ts (3)
37-41: LGTM!Also applies to: 50-77
1343-1409: LGTM!
1324-1341: 🩺 Stability & AvailabilityNo change needed. The cutover status is written before the
systemd-runguardian is scheduled, so neither side creates an earlierrestart-scheduled/missingwindow to reconcile.backend/test/jobExecutionQueue.test.ts (2)
80-134: LGTM!
167-189: LGTM!backend/src/services/jobExecutionQueue.ts (1)
17-17: LGTM!Also applies to: 435-435, 474-476
backend/test/releaseManager.test.ts (1)
13-13: LGTM!Also applies to: 395-396, 872-887, 906-922
backend/src/releaseManager.ts (2)
35-37: LGTM!
1209-1211: LGTM!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 862b474de7
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/test/jobExecutionQueue.test.ts (2)
252-293: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSetup steps run outside the try/finally, risking a leaked temp dir on failure.
mkdtempSync,writeFileSync,chmodSync, and theprocess.env.PATHmutation all happen before thetryblock. If any of them throws,rmSync(fakeBin, ...)infinallynever runs and the temp directory leaks. Low likelihood, but easy to close by moving these into the guarded region.🧹 Proposed fix to guard setup as well
- const startedAt = "2026-07-26T03:00:00.000Z"; - createRestartScheduledDeployment(startedAt); - const fakeBin = mkdtempSync(path.join(tmpdir(), "mira-systemctl-test-")); - const systemctl = path.join(fakeBin, "systemctl"); - const originalPath = process.env.PATH; - writeFileSync( - systemctl, - String.raw`#!/usr/bin/env bash -printf 'benign diagnostic\n' >&2 -printf 'LoadState=loaded\nActiveState=active\n' -` - ); - chmodSync(systemctl, 0o755); - process.env.PATH = `${fakeBin}${path.delimiter}${originalPath ?? ""}`; - const warning = jest.spyOn(console, "warn").mockImplementation(() => {}); - const recovery = jest.fn(() => true); - try { + const startedAt = "2026-07-26T03:00:00.000Z"; + createRestartScheduledDeployment(startedAt); + const fakeBin = mkdtempSync(path.join(tmpdir(), "mira-systemctl-test-")); + const originalPath = process.env.PATH; + try { + const systemctl = path.join(fakeBin, "systemctl"); + writeFileSync( + systemctl, + String.raw`#!/usr/bin/env bash +printf 'benign diagnostic\n' >&2 +printf 'LoadState=loaded\nActiveState=active\n' +` + ); + chmodSync(systemctl, 0o755); + process.env.PATH = `${fakeBin}${path.delimiter}${originalPath ?? ""}`; + const warning = jest.spyOn(console, "warn").mockImplementation(() => {}); + const recovery = jest.fn(() => true);🤖 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/jobExecutionQueue.test.ts` around lines 252 - 293, Move the temporary-directory setup, fake systemctl creation, permissions update, and PATH mutation into the try/finally guarded by the existing cleanup in the test around reconcileOrphanedDeploymentCutovers. Ensure cleanup safely handles setup failures, including when fakeBin was not created, while preserving the current assertions and environment restoration.
169-194: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an exact 10-minute boundary probe for unknown guardian state.
isDeploymentCutoverReconciliationExpiredexpires when the delta reaches>= 10 * 60 * 1000, but this test only shows no recovery at 1 minute and recovery at 11 minutes. A probe at03:10:00.000Zwould catch an off-by-one such as>vs>=in this reliability-critical reconciliation path.🤖 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/jobExecutionQueue.test.ts` around lines 169 - 194, Add an assertion in the test covering reconcileOrphanedDeploymentCutovers for unknown guardian state at exactly 03:10:00.000Z, verifying recovery is scheduled and called with the expected deployment details. Keep the existing 1-minute and 11-minute checks intact.
🤖 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 295-301: Replace the fixed Bun.sleep delay in the
publishVerifiedDashboardRelease test with a deterministic barrier or test hook
that confirms execution has reached lock acquisition and is waiting for the
transition lock before checking the releases directory. Keep the assertion that
no .staging- directory exists, but ensure it runs only after that
synchronization point.
---
Nitpick comments:
In `@backend/test/jobExecutionQueue.test.ts`:
- Around line 252-293: Move the temporary-directory setup, fake systemctl
creation, permissions update, and PATH mutation into the try/finally guarded by
the existing cleanup in the test around reconcileOrphanedDeploymentCutovers.
Ensure cleanup safely handles setup failures, including when fakeBin was not
created, while preserving the current assertions and environment restoration.
- Around line 169-194: Add an assertion in the test covering
reconcileOrphanedDeploymentCutovers for unknown guardian state at exactly
03:10:00.000Z, verifying recovery is scheduled and called with the expected
deployment details. Keep the existing 1-minute and 11-minute checks intact.
🪄 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: 9fde9069-d486-4798-8043-55e7d93a4b78
📒 Files selected for processing (8)
backend/src/releaseDeployment.tsbackend/src/releaseManager.tsbackend/src/services/pullRequests.tsbackend/src/services/scheduledJobs.tsbackend/test/jobExecutionQueue.test.tsbackend/test/releaseManager.test.tsbackend/test/serviceBehavior.test.tsdocs/setup/production-deploy.md
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/src/services/scheduledJobs.ts
- docs/setup/production-deploy.md
- backend/test/serviceBehavior.test.ts
- backend/src/services/pullRequests.ts
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.1)
backend/src/releaseManager.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)
[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)
backend/test/releaseManager.test.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 (12)
backend/test/releaseManager.test.ts (1)
13-13: LGTM!Also applies to: 34-35, 49-55, 121-122, 176-176, 417-428, 860-939, 941-952
backend/src/releaseDeployment.ts (3)
12-12: LGTM!
291-427: LGTM!
436-462: LGTM!backend/src/releaseManager.ts (5)
39-40: LGTM!Also applies to: 68-68
138-146: LGTM!
970-1039: LGTM!
1041-1148: LGTM!
1335-1523: LGTM!backend/test/jobExecutionQueue.test.ts (3)
1-4: LGTM!Also applies to: 31-31
140-167: LGTM!
44-99: LGTM!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 178686dba2
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c105e62513
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
mira-2026
removed
status: needs-review
2 participants
Summary
current/previousatomically, restart web and worker together, require commit-bound readiness, and automatically roll back on failure/home/ubuntu/projects/mira-dashboard-state/health//api/healthcompatibilityBehavior and regression coverage
Verification
bun run lint:frontendbun run buildbun run test:coverage(465 tests, 94.42% lines)bun run lint:backendbun run buildfrombackend/bun run test:coveragefrombackend/(544 tests, 90.13% lines)deploy:prepare: database initialization, restore-verified backup, schema preflight, build, and format-2 manifest verification (33 artifacts)Risk checklist
.envfiles, database dumps, or runtime state committedWKworker status badge besideWSandBEDeployment / operations
mira-dashboard.serviceandmira-dashboard-worker.serviceThis PR must not be deployed with the pre-cutover executor. After merge, use the documented one-time managed cutover to:
No production cutover is performed by this PR itself.
Notes for reviewers