feat: add atomic release operations and trusted PR dev - #335
Conversation
mira-2026
added
priority: high
📝 WalkthroughWalkthroughAdds managed release status and rollback, trusted pull-request previews, isolated development tooling, development-safe request controls, frontend integrations, expanded tests, and updated CI and operational documentation. ChangesDashboard development and preview infrastructure
Managed release rollback
Safety, verification, and documentation
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: 42a96cf906
ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/services/pullRequests.ts (1)
498-537: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHardcoded "Deploy" wording in the release-lock recovery path.
Line 512 writes
note: "Deploy execution ended before build completion"unconditionally when a stuckactiveJobin"building"status is force-failed here — butactiveJobcan just as easily be a rollback job (this same function is now shared between deploy and rollback release actions, per the PR's stated goal of treating both as "release actions"). The adjacentcleanupQueuedDeploymentCancellation/cleanupExpiredDeploymentExecutionhelpers already differentiate deploy vs rollback note text viaexecution.actionKey; this branch was missed, so an operator recovering from a stuck rollback will see a misleading "Deploy execution ended..." message in the job history/UI.🐛 Proposed fix
if (lockExecution && activeJob?.status === "building") { writeDeploymentJob({ ...activeJob, - note: "Deploy execution ended before build completion", + note: + lockExecution && activeJob.commit === undefined + ? "Release execution ended before build completion" + : "Release execution ended before build completion", status: "failed", updatedAt: dateToISOString(new Date()), });Since
activeJobalone doesn't carry the action type here, prefer readingreadDeploymentLockExecution(activeJobId)?.actionKey(already fetched aslockExecution, though its type here isDeploymentLockExecutionRowwhich may need anactionKeyfield) to pick deploy- vs rollback-specific wording, mirroring the pattern used a few lines above incleanupTerminatedDeploymentExecution.🤖 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 498 - 537, Update ensureNoActiveDeployment to choose the recovery note from lockExecution.actionKey instead of hardcoding “Deploy” wording. Add or expose actionKey on DeploymentLockExecutionRow if needed, and use the existing deploy-versus-rollback wording pattern from cleanupTerminatedDeploymentExecution while preserving the current failure and lock-cleanup behavior.
🧹 Nitpick comments (3)
docs/setup/production-deploy.md (1)
110-177: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHost-local fallback bypasses the exclusive deployment lock.
The fallback script mutates release symlinks and restarts services directly via
$DATABASE_PATH/releaseLifecycle.js, without checking or acquiringdeployment_lockthe way the UI-driven path does viaacquireDeploymentLock. The doc only asks the operator to manually confirm nothing is running; a concurrent UI/worker-driven rollback or deploy could race with this script and corrupt thecurrent/previoussymlinks.Consider adding an automated guard (e.g., query
deployment_lock/job_executionsfor an activedashboard.deploy/dashboard.rollbackrow and abort if found) before proceeding, rather than relying solely on operator discipline.💡 Suggested guard to add before the rollback commands
+ACTIVE_LOCK="$( + sqlite3 "$DATABASE_PATH" "SELECT job_id FROM deployment_lock WHERE id = 1;" +)" +if [[ -n "$ACTIVE_LOCK" ]]; then + echo "A deployment or rollback action is already in progress ($ACTIVE_LOCK); aborting." >&2 + exit 1 +fi + env MIRA_DASHBOARD_DB_PATH="$DATABASE_PATH" \ MIRA_DASHBOARD_RELEASES_ROOT="$RELEASES_ROOT" \ NODE_ENV=production \ bun "$CURRENT_LIFECYCLE" rollback🤖 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 `@docs/setup/production-deploy.md` around lines 110 - 177, Update the host-local fallback before the first rollback command to automatically inspect the SQLite state for an active deployment or rollback, including deployment_lock and running dashboard.deploy/dashboard.rollback job executions, and abort if any are present. Reuse the existing DATABASE_PATH and lifecycle environment configuration, and ensure the guard runs before any release symlink mutation or service restart.backend/src/services/pullRequests.ts (1)
1750-1841: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRollback's systemd unit is still named
mira-dashboard-deploy-*.
scheduleReleaseRollbackschedules its detached script under--unit=mira-dashboard-deploy-${job.id}(line 1833), identical to the deploy cutover's unit name pattern. Sincejob.idis always unique there's no functional collision, but this makes it harder to distinguish an in-flight rollback from a deploy when inspectingsystemctl --user list-unitsor unit logs during an incident — exactly the scenario where quick triage matters most for this feature.♻️ Proposed fix
return runCommand( "systemd-run", [ "--user", "--collect", - `--unit=mira-dashboard-deploy-${job.id}`, + `--unit=mira-dashboard-rollback-${job.id}`, "--description=Mira Dashboard atomic release rollback",🤖 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 1750 - 1841, Update the systemd unit name constructed in scheduleReleaseRollback to use a rollback-specific pattern, such as a name containing “rollback” instead of “deploy,” while preserving the existing job.id suffix and all other scheduling behavior.src/components/features/pullRequests/ProductionReleasesCard.tsx (1)
42-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCommit link hardcodes the GitHub org/repo instead of reusing a backend-supplied URL.
DeploymentJob.commitUrl(used inRecentDeploysCardinPullRequests.tsx) is already supplied by the backend for deployment commit links. Here the release-slot commit link is instead built client-side from a hardcodedrajohan/Mira-Dashboardpath. Consider exposing acommitUrlonDashboardReleaseSummary(mirroringDeploymentJob) for consistency and to avoid duplicating/hardcoding the repo path on the frontend.🤖 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 `@src/components/features/pullRequests/ProductionReleasesCard.tsx` around lines 42 - 49, The release commit link in ProductionReleasesCard should use a backend-supplied URL instead of constructing a hardcoded GitHub path. Add and populate commitUrl on DashboardReleaseSummary, following the existing DeploymentJob.commitUrl pattern, then bind the anchor’s href to release.commitUrl while preserving the current link behavior and styling.
🤖 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.
Outside diff comments:
In `@backend/src/services/pullRequests.ts`:
- Around line 498-537: Update ensureNoActiveDeployment to choose the recovery
note from lockExecution.actionKey instead of hardcoding “Deploy” wording. Add or
expose actionKey on DeploymentLockExecutionRow if needed, and use the existing
deploy-versus-rollback wording pattern from cleanupTerminatedDeploymentExecution
while preserving the current failure and lock-cleanup behavior.
---
Nitpick comments:
In `@backend/src/services/pullRequests.ts`:
- Around line 1750-1841: Update the systemd unit name constructed in
scheduleReleaseRollback to use a rollback-specific pattern, such as a name
containing “rollback” instead of “deploy,” while preserving the existing job.id
suffix and all other scheduling behavior.
In `@docs/setup/production-deploy.md`:
- Around line 110-177: Update the host-local fallback before the first rollback
command to automatically inspect the SQLite state for an active deployment or
rollback, including deployment_lock and running
dashboard.deploy/dashboard.rollback job executions, and abort if any are
present. Reuse the existing DATABASE_PATH and lifecycle environment
configuration, and ensure the guard runs before any release symlink mutation or
service restart.
In `@src/components/features/pullRequests/ProductionReleasesCard.tsx`:
- Around line 42-49: The release commit link in ProductionReleasesCard should
use a backend-supplied URL instead of constructing a hardcoded GitHub path. Add
and populate commitUrl on DashboardReleaseSummary, following the existing
DeploymentJob.commitUrl pattern, then bind the anchor’s href to
release.commitUrl while preserving the current link behavior and styling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 06df07ae-ecb3-42fd-ada1-1bf650abae06
📒 Files selected for processing (14)
backend/src/routes/pullRequestRoutes.tsbackend/src/services/pullRequests.tsbackend/test/multiFactorAuth.test.tsbackend/test/serviceBehavior.test.tsdocs/setup/production-deploy.mdsrc/components/features/chat/ChatHeader.tsxsrc/components/features/pullRequests/ProductionReleasesCard.tsxsrc/components/layout/AppHeader.tsxsrc/hooks/index.tssrc/hooks/usePullRequests.tssrc/pages/PullRequests.tsxsrc/test/chatHeader.test.tsxsrc/test/frontendBehavior.test.tsxsrc/test/pageBehavior.test.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Analyze JavaScript and TypeScript
- GitHub Check: backend-checks
- GitHub Check: frontend-checks
🔇 Additional comments (27)
src/components/features/chat/ChatHeader.tsx (1)
87-95: LGTM!Also applies to: 120-120
src/test/chatHeader.test.tsx (1)
65-68: LGTM!src/components/layout/AppHeader.tsx (4)
2-8: LGTM!
62-80: LGTM!
107-179: LGTM!
227-227: LGTM!Also applies to: 236-236
backend/test/multiFactorAuth.test.ts (1)
605-612: LGTM!backend/test/serviceBehavior.test.ts (5)
1596-1717: LGTM!
1719-1847: LGTM!
1878-1878: LGTM!Also applies to: 1904-1904, 2041-2041
2446-2447: LGTM!Also applies to: 2475-2476, 2743-2744, 2837-2838
69-92: 🎯 Functional CorrectnessNo change needed.
The remaining
--short HEADhandlers are in locally scoped inline fakegitscripts whose callers supply their own fullrev-parse HEADresponses as well.docs/setup/production-deploy.md (1)
89-89: LGTM!src/test/frontendBehavior.test.tsx (4)
150-156: LGTM!
902-909: LGTM!
3129-3156: LGTM!Also applies to: 3306-3310
4623-4634: LGTM!Also applies to: 4676-4680
src/test/pageBehavior.test.tsx (3)
1736-1763: LGTM!
1804-1816: LGTM!
2791-2792: LGTM!Also applies to: 2820-2830
backend/src/services/pullRequests.ts (1)
20-20: LGTM!Also applies to: 185-221, 366-388, 401-462, 465-495, 540-565, 602-640, 1391-1434, 1979-2062, 2064-2122, 2217-2277, 2539-2590
backend/src/routes/pullRequestRoutes.ts (2)
4-8: LGTM!
122-142: 🔒 Security & PrivacyNo change needed.
/api/pull-requests/releases/rollbackis covered by the centralized request-policy MFA checks viarequiresRecentMfa(), which gates all/api/pull-requests/mutations.src/hooks/usePullRequests.ts (1)
50-71: LGTM!Also applies to: 80-80, 106-110, 132-132, 160-167, 218-227, 258-267, 283-285, 364-367, 371-389
src/hooks/index.ts (1)
112-113: LGTM!Also applies to: 125-131
src/components/features/pullRequests/ProductionReleasesCard.tsx (1)
1-41: LGTM!Also applies to: 50-193
src/pages/PullRequests.tsx (1)
15-15: LGTM!Also applies to: 24-24, 33-39, 51-56, 409-411, 422-422, 431-434, 545-548, 610-617, 626-627, 686-694, 883-893, 1036-1039
mira-2026
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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 (1)
docs/setup/production-deploy.md (1)
147-203: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake fallback target selection part of the exclusive transition.
Line 163 captures
TARGET_SHAbefore the rollback command obtains its lock. A deploy/rollback can transition releases after Line 189, causing the unparameterized command at Line 193 to roll back a different pair; the recovery rollback can then undo that concurrent action. Add a guarded lifecycle operation that validates the expected current and target commits atomically under the release lock, and use it here.🤖 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 `@docs/setup/production-deploy.md` around lines 147 - 203, Make TARGET_SHA selection and rollback one exclusive lifecycle transition instead of reading it before the lock. Update the lifecycle operation invoked by this deployment flow to accept and validate the expected current and target commits under the release lock, then perform the rollback only when they still match. Replace the unparameterized rollback calls in the main and recovery paths with this guarded operation, preserving recovery of CURRENT_SHA after readiness failure.
🧹 Nitpick comments (11)
backend/src/services/pullRequestPreviewHost.ts (1)
423-442: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCorrupt preview state permanently breaks the status endpoint.
readPreviewRecordthrows on any unparsable/invalidactive-preview.json, andgetPullRequestPreviewStatuspropagates it, soGET /api/pull-requests/preview(and every start/stop attempt) fails until someone manually deletes the file on the host. Consider treating an unreadable record as "no active preview" (log + quarantine the file) so the slot remains recoverable from the UI.🤖 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/pullRequestPreviewHost.ts` around lines 423 - 442, Update readPreviewRecord and its getPullRequestPreviewStatus callers to recover from invalid or unreadable preview state by logging the error, quarantining the state file, and returning undefined so the endpoint and start/stop flows treat it as no active preview. Preserve existing handling for missing files and valid records.src/components/layout/AppHeader.tsx (1)
154-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant label in the Worker row.
The row already renders
Workeron the left, so the value column reads "Worker Worker offline ○". WebSocket/Backend rows use a bareOnline/Offline; consider a status-only string here for consistency.♻️ Suggested change
- {workerStatus.label} {workerStatus.symbol} + {workerStatus.text} {workerStatus.symbol}with
workerStatusexposingtext: "Online" | "Offline" | "Unavailable"alongside the existinglabelused for the trigger.🤖 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 `@src/components/layout/AppHeader.tsx` around lines 154 - 167, Update the Worker status value in the AppHeader status row to render only the status text and symbol, avoiding the redundant “Worker” label; add or reuse a workerStatus text value such as Online, Offline, or Unavailable while preserving workerStatus.label for the trigger and the existing state-based styling.backend/src/services/pullRequests.ts (2)
1902-1913: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
FULL_COMMIT_SHA_PATTERNinstead of re-declaring the literal.
FULL_COMMIT_SHA_PATTERN(Line 81) already encodes this rule, but the rollback and cutover schedulers inline/^[\da-f]{40}$/uin five places. Swapping them keeps the SHA contract in one spot.🤖 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 1902 - 1913, Replace every inline `/^[\da-f]{40}$/u` check in the rollback and cutover scheduler logic, including the validations around `job.commit` and `originalCommit`, with the existing `FULL_COMMIT_SHA_PATTERN` symbol. Preserve the current validation conditions and error behavior while reusing that shared pattern in all five occurrences.
863-897: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueNo negative caching on the public PR fallback.
Only successful responses populate
publicPullRequestCache, so a failing or rate-limited GitHub API is re-hit on every dashboard poll. Given the unauthenticated 60 req/hour budget this can wedge the dev fallback for a long window; caching a short-lived failure (or the last good value) would fail softer.🤖 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 863 - 897, Update listPublicDashboardPullRequests and publicPullRequestCache to retain a short-lived fallback after GitHub fetch, parsing, or validation failures instead of retrying on every poll. Cache the last successful pull requests or a short-lived failure/empty result with an appropriate expiration, while preserving normal cache returns and successful response handling.backend/test/releaseManager.test.ts (1)
840-860: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest passes even if the transition lock wait regresses.
Nothing asserts the activation was still pending while the shared lock was held, so a build where
transitionLockWaitMsis ignored (or the lock isn't taken) would still satisfy this test. Adding a pre-release check on the pending promise would make it a real regression guard. Also, the descriptor leaks if the assertion throws — atry/finallyaroundcloseSyncwould keep the temp root clean.🤖 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/releaseManager.test.ts` around lines 840 - 860, Add an explicit pre-release assertion in the test around runReleaseLifecycleCommand to verify activation remains pending while the shared lock descriptor from holdTransitionLock is open, then release the descriptor in a try/finally so cleanup occurs even if the assertion or final activation expectation fails.backend/src/releaseLifecycle.ts (1)
73-79: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
statusstill acquires its shared lock with a zero wait.
activate/rollbacknow wait up to 30s, butreadDashboardReleaseStatetakes no options, sostatusfails immediately when a transition holds the exclusive lock. If status is polled by the dashboard during a rollback, that surfaces as a transient error rather than a short wait.🤖 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/releaseLifecycle.ts` around lines 73 - 79, Update the status branch in the release lifecycle handler to read state with the same 30-second lock wait used by activate and rollback. Extend or reuse readDashboardReleaseState’s options so status passes the wait configuration while preserving its no-commit-SHA validation and existing state handling.backend/src/services/pullRequestPreviews.ts (1)
126-141: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffLong-lived request blocking on the preview start job.
prepareAndStartPullRequestPreviewawaits the queued execution for up to 15 minutes on the HTTP request thread. Any intermediary (Tailscale Serve, browser, fetch) will typically time out well before then, leaving the caller without a result even though the job continues. Since the frontend already pollsGET /api/pull-requests/preview, consider returning the queued/starting status immediately and letting the poll drive the UI.🤖 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/pullRequestPreviews.ts` around lines 126 - 141, Update prepareAndStartPullRequestPreview to return the queued/starting PullRequestPreviewStatus immediately after enqueueJobExecution, rather than awaiting waitForJobExecution and previewFromExecution. Preserve the existing job enqueue configuration and rely on the frontend’s GET /api/pull-requests/preview polling for completion.scripts/developmentTailscale.ts (2)
166-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCleanup failure in
finallymasks the stack's exit code/error.If
disableDevelopmentServethrows (tailscale down, sudo denied), the originalrunDevelopmentStackresult or error is replaced by the teardown error and the serve route stays configured with no clear signal. Log and swallow instead.♻️ Proposed fix
} finally { if (route.didCreate) { - await disableDevelopmentServe(port); + try { + await disableDevelopmentServe(port); + } catch (error) { + console.error( + `Failed to remove Tailscale Serve route on port ${port}: ${ + error instanceof Error ? error.message : String(error) + }` + ); + } } }🤖 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/developmentTailscale.ts` around lines 166 - 172, Update the finally cleanup around runDevelopmentStack so failures from disableDevelopmentServe do not replace its exit code or error. When route.didCreate is true, catch teardown errors, log them with the existing logging mechanism, and swallow them while preserving the original stack result or exception.
28-45: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd a timeout to
commandOutputspawns.
commandOutputis used fortailscale status/serve/statusandsudo tailscale serve ..., so a wedged process can block the dev entrypoint indefinitely. Pass atimeoutoption toBun.spawn, e.g.15_000ms, and add a timeout-failure branch toprocess_.exited.🤖 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/developmentTailscale.ts` around lines 28 - 45, Update commandOutput to pass a 15_000 ms timeout option to Bun.spawn, and handle the resulting timeout failure in process_.exited alongside nonzero exit codes. Preserve the existing stderr/stdout capture and error reporting for ordinary command failures.scripts/developmentFrontend.ts (1)
10-14: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerive the default cookie namespace from
portinstead of hardcoding5173.
developmentBackendEnvironmentbuilds the backend namespace asmira_dashboard_dev_${frontendPort}. If someone runs this proxy withPORT=4173but withoutMIRA_DASHBOARD_DEV_COOKIE_NAMESPACE,developmentCookieHeadersilently filters out every real cookie and the session never reaches the backend.♻️ Proposed fix
const cookieNamespace = - process.env.MIRA_DASHBOARD_DEV_COOKIE_NAMESPACE || "mira_dashboard_dev_5173"; + process.env.MIRA_DASHBOARD_DEV_COOKIE_NAMESPACE || `mira_dashboard_dev_${port}`;🤖 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/developmentFrontend.ts` around lines 10 - 14, Update the default value in the cookieNamespace initialization to derive from the parsed port variable, matching the backend namespace format `mira_dashboard_dev_${port}` while preserving the explicit MIRA_DASHBOARD_DEV_COOKIE_NAMESPACE override.backend/src/requestPolicy.ts (1)
499-519: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThird copy of the audit-then-403-deny pattern.
This block duplicates the same audit-write + 503-fallback + 403-JSON shape already used at Lines 463-480 (automation scope denial) and Lines 527-551 (MFA denial). Consider extracting a small helper (e.g.
denyWithAudit(actor, request, requestIdentifier, routePath, automationScope, persistAuditEvent, body)) to avoid the three call sites drifting out of sync.♻️ Example extraction
+function denyWithAudit( + actor: AuditActor, + request: Request, + requestIdentifier: string, + routePath: string, + automationScope: AutomationScope | undefined, + persistAuditEvent: typeof writeAuditEvent, + body: Record<string, unknown> +): Response | undefined { + const didRecordDenial = didWriteRequestAudit( + actor, + "denied", + request, + requestIdentifier, + routePath, + 403, + automationScope, + persistAuditEvent + ); + if (!didRecordDenial) { + return json({ error: "Audit trail unavailable" }, { status: 503 }); + } + return json(body, { status: 403 }); +}🤖 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/requestPolicy.ts` around lines 499 - 519, Extract the repeated audit-then-deny behavior from the current block and the existing automation-scope and MFA denial paths into a shared helper near the relevant request-policy logic, such as denyWithAudit. Have it perform didWriteRequestAudit, return the 503 audit-unavailable response on failure, and otherwise return the supplied 403 JSON body; update all three call sites to use it while preserving their existing response messages and parameters.
🤖 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/development/developmentOpenClaw.ts`:
- Around line 36-37: Update SENSITIVE_AGENT_CONFIG_KEY so the sensitive-key
keyword alternatives match both singular and plural forms, including keys such
as tokens, apiKeys, and passwords, while preserving separator and end-of-key
boundaries and existing matches like secrets.
In `@backend/src/development/developmentStack.ts`:
- Around line 352-357: Move the `try` in `scrubDevelopmentDatabase` to wrap
`Database` creation and all PRAGMA/`BEGIN IMMEDIATE` setup, with the existing
`finally` closing the handle whenever it is successfully created; ensure setup
errors cannot bypass `database.close()` before the caller removes the staging
file.
In `@backend/src/routes/pullRequestRoutes.ts`:
- Around line 165-173: Update the POST handler’s rollback body validation before
accessing body.targetCommit so a null or non-object JSON body returns the same
400 “Rollback target commit is required” response. Preserve the existing string
validation and valid-body behavior.
---
Outside diff comments:
In `@docs/setup/production-deploy.md`:
- Around line 147-203: Make TARGET_SHA selection and rollback one exclusive
lifecycle transition instead of reading it before the lock. Update the lifecycle
operation invoked by this deployment flow to accept and validate the expected
current and target commits under the release lock, then perform the rollback
only when they still match. Replace the unparameterized rollback calls in the
main and recovery paths with this guarded operation, preserving recovery of
CURRENT_SHA after readiness failure.
---
Nitpick comments:
In `@backend/src/releaseLifecycle.ts`:
- Around line 73-79: Update the status branch in the release lifecycle handler
to read state with the same 30-second lock wait used by activate and rollback.
Extend or reuse readDashboardReleaseState’s options so status passes the wait
configuration while preserving its no-commit-SHA validation and existing state
handling.
In `@backend/src/requestPolicy.ts`:
- Around line 499-519: Extract the repeated audit-then-deny behavior from the
current block and the existing automation-scope and MFA denial paths into a
shared helper near the relevant request-policy logic, such as denyWithAudit.
Have it perform didWriteRequestAudit, return the 503 audit-unavailable response
on failure, and otherwise return the supplied 403 JSON body; update all three
call sites to use it while preserving their existing response messages and
parameters.
In `@backend/src/services/pullRequestPreviewHost.ts`:
- Around line 423-442: Update readPreviewRecord and its
getPullRequestPreviewStatus callers to recover from invalid or unreadable
preview state by logging the error, quarantining the state file, and returning
undefined so the endpoint and start/stop flows treat it as no active preview.
Preserve existing handling for missing files and valid records.
In `@backend/src/services/pullRequestPreviews.ts`:
- Around line 126-141: Update prepareAndStartPullRequestPreview to return the
queued/starting PullRequestPreviewStatus immediately after enqueueJobExecution,
rather than awaiting waitForJobExecution and previewFromExecution. Preserve the
existing job enqueue configuration and rely on the frontend’s GET
/api/pull-requests/preview polling for completion.
In `@backend/src/services/pullRequests.ts`:
- Around line 1902-1913: Replace every inline `/^[\da-f]{40}$/u` check in the
rollback and cutover scheduler logic, including the validations around
`job.commit` and `originalCommit`, with the existing `FULL_COMMIT_SHA_PATTERN`
symbol. Preserve the current validation conditions and error behavior while
reusing that shared pattern in all five occurrences.
- Around line 863-897: Update listPublicDashboardPullRequests and
publicPullRequestCache to retain a short-lived fallback after GitHub fetch,
parsing, or validation failures instead of retrying on every poll. Cache the
last successful pull requests or a short-lived failure/empty result with an
appropriate expiration, while preserving normal cache returns and successful
response handling.
In `@backend/test/releaseManager.test.ts`:
- Around line 840-860: Add an explicit pre-release assertion in the test around
runReleaseLifecycleCommand to verify activation remains pending while the shared
lock descriptor from holdTransitionLock is open, then release the descriptor in
a try/finally so cleanup occurs even if the assertion or final activation
expectation fails.
In `@scripts/developmentFrontend.ts`:
- Around line 10-14: Update the default value in the cookieNamespace
initialization to derive from the parsed port variable, matching the backend
namespace format `mira_dashboard_dev_${port}` while preserving the explicit
MIRA_DASHBOARD_DEV_COOKIE_NAMESPACE override.
In `@scripts/developmentTailscale.ts`:
- Around line 166-172: Update the finally cleanup around runDevelopmentStack so
failures from disableDevelopmentServe do not replace its exit code or error.
When route.didCreate is true, catch teardown errors, log them with the existing
logging mechanism, and swallow them while preserving the original stack result
or exception.
- Around line 28-45: Update commandOutput to pass a 15_000 ms timeout option to
Bun.spawn, and handle the resulting timeout failure in process_.exited alongside
nonzero exit codes. Preserve the existing stderr/stdout capture and error
reporting for ordinary command failures.
In `@src/components/layout/AppHeader.tsx`:
- Around line 154-167: Update the Worker status value in the AppHeader status
row to render only the status text and symbol, avoiding the redundant “Worker”
label; add or reuse a workerStatus text value such as Online, Offline, or
Unavailable while preserving workerStatus.label for the trigger and the existing
state-based styling.
🪄 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: 72a30c69-a39f-4b7f-9fcc-f28876926f66
⛔ Files ignored due to path filters (2)
public/vite.svgis excluded by!**/*.svgand included by**/*src/assets/react.svgis excluded by!**/*.svgand included by**/*
📒 Files selected for processing (51)
.github/CONTRIBUTING.md.github/pull_request_template.md.github/workflows/dashboard-checks.ymlREADME.mdbackend/package.jsonbackend/src/development/developmentOpenClaw.tsbackend/src/development/developmentStack.tsbackend/src/http.tsbackend/src/lib/values.tsbackend/src/releaseLifecycle.tsbackend/src/releaseManager.tsbackend/src/requestPolicy.tsbackend/src/routes/pullRequestRoutes.tsbackend/src/server.tsbackend/src/services/jobWorker.tsbackend/src/services/pullRequestPreviewHost.tsbackend/src/services/pullRequestPreviews.tsbackend/src/services/pullRequests.tsbackend/test/developmentStack.test.tsbackend/test/pullRequestPreview.test.tsbackend/test/releaseManager.test.tsbackend/test/serverStartupPolicy.test.tsbackend/test/serviceBehavior.test.tsbackend/test/utilityBehavior.test.tsdocs/api/endpoints.mddocs/architecture/frontend-feature-map.mddocs/architecture/overview.mddocs/development/local-dev.mddocs/development/testing-and-prs.mddocs/operations/scheduler-cache-backups.mddocs/operations/troubleshooting.mddocs/setup/production-deploy.mddocs/setup/secrets-and-env.mdeslint.config.jspackage.jsonscripts/developmentFrontend.tsscripts/developmentStack.tsscripts/developmentTailscale.tssrc/components/features/pullRequests/ProductionReleasesCard.tsxsrc/components/features/pullRequests/PullRequestPreviewCard.tsxsrc/components/layout/AppHeader.tsxsrc/components/ui/Dropdown.tsxsrc/hooks/index.tssrc/hooks/usePullRequests.tssrc/lib/developmentProxyHeaders.tssrc/pages/PullRequests.tsxsrc/test/developmentProxyHeaders.test.tssrc/test/developmentTailscale.test.tssrc/test/frontendBehavior.test.tsxsrc/test/pageBehavior.test.tsxsrc/test/pullRequestPreviewCard.test.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/hooks/index.ts
- src/test/pageBehavior.test.tsx
- src/test/frontendBehavior.test.tsx
- src/components/features/pullRequests/ProductionReleasesCard.tsx
📜 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/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)
backend/test/developmentStack.test.ts
[error] 306-306: Avoid SQL injection
Context: snapshot.query(SELECT COUNT(*) AS count FROM ${tableName})
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-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)
🪛 GitHub Check: CodeQL
backend/src/development/developmentOpenClaw.ts
[failure] 132-132: Potential file system race condition
The file may have changed since it was checked.
🪛 LanguageTool
docs/development/local-dev.md
[grammar] ~87-~87: Ensure spelling is correct
Context: ...space paths at the snapshot. All state roots are owner-only and untracked. Refresh t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 OpenGrep (1.25.0)
backend/test/developmentStack.test.ts
[ERROR] 307-307: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.
(coderabbit.sql-injection.raw-query-concat-js)
🔇 Additional comments (38)
backend/src/services/pullRequestPreviewHost.ts (2)
222-321: LGTM!
1189-1254: LGTM!src/components/layout/AppHeader.tsx (1)
168-189: LGTM!backend/src/services/pullRequestPreviews.ts (1)
41-56: LGTM!Also applies to: 70-118, 144-189
backend/src/services/pullRequests.ts (1)
72-85: LGTM!Also applies to: 177-189, 311-314, 368-411, 521-563, 628-642, 706-728, 792-861, 1133-1139, 1955-1986, 2210-2266, 2362-2437
backend/src/routes/pullRequestRoutes.ts (2)
3-7: LGTM!Also applies to: 106-133, 192-200
164-182: 🔒 Security & PrivacyRecent MFA coverage is already in place.
/api/pull-requests/releases/rollbackis routed throughroutes.tsand covered by the request policy prefix/api/pull-requests/, so no local handler-level step-up check is required.backend/src/releaseLifecycle.ts (1)
14-14: LGTM!Also applies to: 48-70
backend/src/releaseManager.ts (1)
76-76: LGTM!Also applies to: 1217-1296, 1298-1367
backend/test/serverStartupPolicy.test.ts (1)
873-873: LGTM!Also applies to: 892-922, 951-1020, 1046-1100
backend/test/serviceBehavior.test.ts (1)
69-76: LGTM!Also applies to: 1658-1660, 1701-1758, 1772-1816, 1993-2064
backend/test/releaseManager.test.ts (1)
93-113: LGTM!src/test/developmentTailscale.test.ts (1)
1-60: LGTM!src/test/developmentProxyHeaders.test.ts (1)
3-29: LGTM!src/components/ui/Dropdown.tsx (1)
25-25: LGTM!Also applies to: 38-38, 61-62
src/components/features/pullRequests/PullRequestPreviewCard.tsx (1)
8-121: LGTM!src/pages/PullRequests.tsx (1)
52-90: LGTM!Also applies to: 414-470, 645-854, 940-1051, 1187-1210
src/test/pullRequestPreviewCard.test.tsx (1)
7-82: LGTM!src/hooks/usePullRequests.ts (1)
51-104: LGTM!Also applies to: 134-199, 252-329, 435-483
docs/operations/scheduler-cache-backups.md (1)
6-9: LGTM!docs/operations/troubleshooting.md (1)
116-130: LGTM!docs/setup/secrets-and-env.md (1)
55-66: LGTM!Also applies to: 149-175
eslint.config.js (1)
36-36: LGTM!package.json (1)
40-40: 📐 Maintainability & Code QualityNo change needed for the frontend test script.
bunfig.tomlsets the root test discovery tosrc, sotest:frontenddoes not includebackend/test/**/*.test.*files.> Likely an incorrect or invalid review comment.backend/src/development/developmentOpenClaw.ts (1)
47-117: LGTM!Also applies to: 141-228
backend/src/development/developmentStack.ts (1)
88-199: LGTM!Also applies to: 202-303, 442-549, 551-680, 683-728, 731-775, 779-853
scripts/developmentFrontend.ts (1)
42-50: LGTM!Also applies to: 76-79
scripts/developmentStack.ts (1)
1-60: LGTM!scripts/developmentTailscale.ts (1)
57-97: LGTM!Also applies to: 99-165
backend/test/developmentStack.test.ts (1)
30-150: LGTM!Also applies to: 152-438, 440-513
backend/package.json (1)
9-20: 📐 Maintainability & Code QualityNo change needed. The package.json scripts and CI/manual command references use scoping routes from
package.json(build,deploy:prepare,test,test:backend,format,lint:backend) rather than direct unscoped backend script names.backend/src/http.ts (1)
5-26: LGTM!backend/src/lib/values.ts (1)
23-45: LGTM!src/lib/developmentProxyHeaders.ts (1)
3-16: LGTM!backend/test/utilityBehavior.test.ts (1)
13-13: LGTM!Also applies to: 33-59, 91-124, 464-468, 478-568, 671-720, 859-880
backend/src/server.ts (1)
18-22: LGTM!Also applies to: 79-101, 124-127, 162-169, 195-195
backend/src/requestPolicy.ts (1)
106-125: LGTM!Also applies to: 161-192
backend/src/services/jobWorker.ts (1)
12-12: LGTM!Also applies to: 31-39, 55-74
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f1327df74
ℹ️ 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".
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/src/services/pullRequestPreviewHost.ts (1)
437-458: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winQuarantine triggers on transient read failures, not just corruption.
The
catchwrapsreadFileSynctoo, so anEIO/EACCESblip renames away a valid state file and the preview is reportedstoppedwhile the unit is still running. Also, the size guard runs after the whole file is read. ConsiderstatSyncfor the size check and quarantining only on parse/validation failures.♻️ Suggested shape
- try { - const content = readFileSync(config.stateFile, "utf8"); - if (Buffer.byteLength(content) > 256 * 1024) { - throw new Error("Dashboard preview state is too large"); - } - return previewRecordFromJson(JSON.parse(content) as unknown); - } catch (error) { + if (statSync(config.stateFile).size > 256 * 1024) { + throw new Error("Dashboard preview state is too large"); + } + const content = readFileSync(config.stateFile, "utf8"); + try { + return previewRecordFromJson(JSON.parse(content) as unknown); + } catch (error) {🤖 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/pullRequestPreviewHost.ts` around lines 437 - 458, Update the state-loading flow around readFileSync and previewRecordFromJson so transient read failures such as EIO or EACCES return without quarantining the state file. Use statSync to enforce the 256 KiB limit before reading, and restrict the quarantine catch to JSON parsing or previewRecordFromJson validation failures, preserving the existing quarantine behavior for genuinely invalid state.
🤖 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 76-77: Validate transitionLockWaitMs before calculating the
contention deadline: reject values that are non-finite or negative, while
preserving valid finite non-negative delays. Apply this validation in the
release-manager transition lock flow where the option is consumed, before the
retry loop or deadline calculation.
In `@backend/src/services/pullRequestPreviewHost.ts`:
- Around line 744-761: Mark the Tailscale Serve route as owned immediately after
the enable command succeeds, before verification in enableTailscaleServe and
before any cleanup can fail. Propagate this attempted-ownership state to the
caller so the outer failure path invokes disableOwnedTailscaleServe with
ownership enabled and persists ownsTailscaleServe: true when the route may still
exist.
---
Nitpick comments:
In `@backend/src/services/pullRequestPreviewHost.ts`:
- Around line 437-458: Update the state-loading flow around readFileSync and
previewRecordFromJson so transient read failures such as EIO or EACCES return
without quarantining the state file. Use statSync to enforce the 256 KiB limit
before reading, and restrict the quarantine catch to JSON parsing or
previewRecordFromJson validation failures, preserving the existing quarantine
behavior for genuinely invalid state.
🪄 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: 64c59a36-c7be-4b91-9cb6-22f8013bba0d
⛔ Files ignored due to path filters (2)
backend/bun.lockis excluded by!**/*.lockand included by**/*bun.lockis excluded by!**/*.lockand included by**/*
📒 Files selected for processing (37)
.github/dependabot.yml.gitignore.prettierignoreREADME.mdbackend/config/log-rotation.jsonbackend/eslint.config.jsbackend/package.jsonbackend/src/development/developmentOpenClaw.tsbackend/src/development/developmentStack.tsbackend/src/releaseLifecycle.tsbackend/src/releaseManager.tsbackend/src/requestPolicy.tsbackend/src/routes/pullRequestRoutes.tsbackend/src/services/pullRequestPreviewHost.tsbackend/src/services/pullRequestPreviewPolicy.tsbackend/src/services/pullRequestPreviews.tsbackend/src/services/pullRequests.tsbackend/test/developmentStack.test.tsbackend/test/pullRequestPreview.test.tsbackend/test/releaseManager.test.tsbackend/test/serviceBehavior.test.tsbackend/test/utilityBehavior.test.tsbackend/tsconfig.jsondocs/api/endpoints.mddocs/development/local-dev.mddocs/setup/production-deploy.mddocs/setup/secrets-and-env.mdeslint.config.jspackage.jsonscripts/developmentFrontend.tsscripts/developmentTailscale.tssrc/components/features/pullRequests/PullRequestPreviewCard.tsxsrc/components/layout/AppHeader.tsxsrc/hooks/usePullRequests.tssrc/pages/PullRequests.tsxsrc/test/frontendBehavior.test.tsxsrc/test/pullRequestPreviewCard.test.tsx
🚧 Files skipped from review as they are similar to previous changes (24)
- backend/package.json
- README.md
- eslint.config.js
- src/test/pullRequestPreviewCard.test.tsx
- docs/api/endpoints.md
- scripts/developmentFrontend.ts
- docs/setup/secrets-and-env.md
- src/components/layout/AppHeader.tsx
- package.json
- backend/src/routes/pullRequestRoutes.ts
- backend/test/utilityBehavior.test.ts
- docs/development/local-dev.md
- backend/src/development/developmentOpenClaw.ts
- backend/src/requestPolicy.ts
- scripts/developmentTailscale.ts
- docs/setup/production-deploy.md
- src/pages/PullRequests.tsx
- src/test/frontendBehavior.test.tsx
- backend/src/development/developmentStack.ts
- src/hooks/usePullRequests.ts
- backend/test/pullRequestPreview.test.ts
- backend/test/serviceBehavior.test.ts
- backend/src/services/pullRequestPreviews.ts
- 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)
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)
🔇 Additional comments (17)
backend/test/releaseManager.test.ts (1)
429-441: LGTM!Also applies to: 476-485, 869-889, 891-919
backend/test/developmentStack.test.ts (1)
17-17: LGTM!Also applies to: 254-259, 278-278, 298-302, 424-497
backend/config/log-rotation.json (1)
2-56: LGTM!backend/eslint.config.js (1)
22-28: LGTM!.github/dependabot.yml (1)
4-92: LGTM!.gitignore (1)
13-13: LGTM!.prettierignore (1)
1-8: LGTM!backend/src/releaseManager.ts (4)
79-86: LGTM!
1207-1223: LGTM!
1232-1311: LGTM!
1313-1401: LGTM!backend/tsconfig.json (1)
11-12: LGTM!backend/src/services/pullRequestPreviewHost.ts (2)
130-134: LGTM!Also applies to: 268-269, 682-713, 1106-1149, 1173-1183, 1194-1230, 1266-1297
316-319: 📐 Maintainability & Code QualityNo change needed. The preview stack forwards
sourceWebAuthnRpIdasMIRA_DASHBOARD_DEV_SOURCE_WEBAUTHN_RP_ID, and the development stack prefers that env var withMIRA_DASHBOARD_WEBAUTHN_RP_IDas the fallback.backend/src/services/pullRequestPreviewPolicy.ts (1)
1-27: LGTM!src/components/features/pullRequests/PullRequestPreviewCard.tsx (1)
1-6: LGTM!Also applies to: 48-57, 79-89
backend/src/releaseLifecycle.ts (1)
38-39: LGTM!Also applies to: 53-57, 70-107
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42bbbed0a5
ℹ️ 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: 98eec534ab
ℹ️ 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
Mira-Dashboard/backend/src/services/pullRequests.ts
Lines 2092 to 2096 in 19a5a9d
Use the guarded lifecycle during orphan rollback recovery
Fresh evidence after fixing the normal cutover path is this orphan-recovery branch: on the first deployment from a pre-upgrade release, if the candidate is already current, recovery deliberately selects the previous release's old lifecycle binary and later invokes it as rollback <candidate> <previous>. That binary only accepts argument-free rollback, so an interrupted cutover whose candidate fails readiness cannot restore the previous release; select a lifecycle implementation known to support guarded rollback arguments.
ℹ️ 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 (2)
backend/src/routes/metricsRoutes.ts (1)
106-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset byte counters before the
/proc/net/devfallback to avoid double-counting.If the primary
/sys/class/netloop partially succeeds (adds bytes for some interfaces) before throwing on a later interface,downloadBytes/uploadBytesretain those partial sums when the code falls through to the/proc/net/devpath, which then adds its own totals on top — inflating the reported network throughput for that sample.🐛 Proposed fix
} catch (sysError) { try { + downloadBytes = 0; + uploadBytes = 0; const networkDeviceText = await Bun.file("/proc/net/dev").text();Also applies to: 133-168
🤖 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/routes/metricsRoutes.ts` around lines 106 - 107, Reset downloadBytes and uploadBytes immediately before entering the /proc/net/dev fallback after the primary /sys/class/net collection fails, so fallback totals replace any partial sums rather than accumulating on top of them. Update the fallback flow associated with these counters while preserving the existing successful primary-path behavior.backend/src/services/pullRequestPreviewHost.ts (1)
1599-1627: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA "starting" record is stamped before the slowest startup phase, so status reads can tear down an in-flight start.
timestampis captured at Line 1571 but the record is only written at Line 1627, afterensurePreviewWorktreeandinstallPreviewDependencies. A coldbun installeasily exceeds the 75 sPREVIEW_START_RECONCILIATION_GRACE_MS, so the record is already stale when persisted, and the nextgetPullRequestPreviewStatuscall runscleanupPreviewResources— stopping both units and deleting the gateway credentials — while the start is still running.
backend/src/services/pullRequestPreviewHost.ts#L1599-L1627: setstartingRecord.updatedAt = new Date().toISOString()immediately beforewritePreviewRecord(or construct the record at that point) so the grace window starts when the record is written.backend/src/services/pullRequestPreviewHost.ts#L1328-L1356: refresh the record'supdatedAtbetween long startup phases (proxy readiness, sandbox start, frontend readiness) soisRecentStartuptracks real progress rather than a single fixed instant.🤖 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/pullRequestPreviewHost.ts` around lines 1599 - 1627, The starting record uses a stale timestamp during long preview startup. In backend/src/services/pullRequestPreviewHost.ts:1599-1627, refresh startingRecord.updatedAt immediately before writePreviewRecord; in backend/src/services/pullRequestPreviewHost.ts:1328-1356, refresh the record timestamp between proxy readiness, sandbox startup, and frontend readiness phases so isRecentStartup reflects ongoing progress.
🧹 Nitpick comments (7)
backend/test/databaseOverview.test.ts (1)
476-478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an independent oracle for
storageBytes.
4096 + overview.sqlite.storageBytesreuses the value being validated, so the test can pass even if the SQLite metric is stale or incorrect. Control the SQLite fixture/mock and assert its expected byte count before checking the aggregate.🤖 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/databaseOverview.test.ts` around lines 476 - 478, Update the test around the aggregate size assertion to configure the SQLite fixture or mock with a known storage byte count, then assert that expected value through overview.sqlite.storageBytes before validating totalManagedDatabaseSizeBytes. Replace the self-referential 4096 + overview.sqlite.storageBytes expression with an independent expected total derived from the controlled fixture value.backend/test/pullRequestPreviewGatewayProxy.test.ts (2)
283-290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
expectinsidefinallycan mask the real failure.If an assertion in the
tryblock fails, Line 288 may throw first and replace the original error. Move thestopassertion to the end of thetryblock and keepfinallyto cleanup only.🤖 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/pullRequestPreviewGatewayProxy.test.ts` around lines 283 - 290, Move the stop-call assertion from the finally block to the end of the associated try block, after the proxy behavior assertions. Keep finally limited to stopping the proxy, closing sockets, and removing the temporary root so cleanup cannot mask the original test failure.
28-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnbounded waits turn regressions into suite hangs.
next()andclosednever reject on their own, so a dropped frame surfaces as a test-runner timeout with no indication of which await stalled — and here it happens inside a spawned child, so the parent only sees a truncated tail. Racing each waiter against a short timer with a descriptive message would make failures diagnosable.🤖 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/pullRequestPreviewGatewayProxy.test.ts` around lines 28 - 72, The websocketHarness waits in next() and closed can hang indefinitely when messages or closure never arrive. Add short timeout races for each pending next() waiter and for the closed promise, rejecting with descriptive errors that identify whether a message or socket close was awaited, while preserving successful message delivery and closure behavior.backend/src/pullRequestPreviewGatewayProxy.ts (2)
513-539: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStartup depends on a token file the host deletes post-readiness.
optionsFromEnvironmentreadsMIRA_DASHBOARD_PREVIEW_GATEWAY_UPSTREAM_TOKEN_FILEat boot, andpullRequestPreviewHost.tsremoves that file immediately after the proxy reports ready (Line 1642 there). That's safe only because the transient unit is started withoutRestart=; adding a restart policy later would make the proxy unrecoverable. Worth a comment in the unit definition or here noting the one-shot read contract.🤖 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/pullRequestPreviewGatewayProxy.ts` around lines 513 - 539, Add a concise comment near optionsFromEnvironment or the relevant unit definition documenting that the upstream token file is read only during startup and may be deleted after readiness, so the service must not rely on restart-based recovery. Keep the existing token-loading behavior unchanged.
251-287: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
challengeNonceis issued but never verified.
openbroadcastsconnect.challengewith the nonce, yetisClientAuthenticatedonly compares the bare token — the nonce plays no part in the handshake. Either bind the client'sconnectframe to the nonce, or dropchallengeNonceso the handshake doesn't look stronger than it is.🤖 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/pullRequestPreviewGatewayProxy.ts` around lines 251 - 287, Update the authentication handshake spanning the challenge broadcast and isClientAuthenticated so the nonce issued in connect.challenge is verified as part of the client’s connect request, binding authentication to that challenge. Reuse the existing challengeNonce state and request authentication parsing, reject missing or mismatched nonces, and preserve the current token, upstream, and successful hello-ok checks.backend/src/releaseManifest.ts (1)
445-451: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDistinguish the build-time inventory error from the parse-time one.
Both this check and the one at Line 625 throw the identical
"Release manifest artifact inventory is invalid". Naming the missing artifact (or at least the phase) makes a failed release build self-diagnosing.♻️ Proposed tweak
- if ( - CURRENT_BUILD_REQUIRED_RELEASE_ARTIFACTS.some( - (requiredPath) => !artifactPaths.includes(requiredPath) - ) - ) { - throw new TypeError("Release manifest artifact inventory is invalid"); - } + const missingArtifacts = CURRENT_BUILD_REQUIRED_RELEASE_ARTIFACTS.filter( + (requiredPath) => !artifactPaths.includes(requiredPath) + ); + if (missingArtifacts.length > 0) { + throw new TypeError( + `Release build is missing required artifacts: ${missingArtifacts.join(", ")}` + ); + }🤖 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/releaseManifest.ts` around lines 445 - 451, Update the validation around CURRENT_BUILD_REQUIRED_RELEASE_ARTIFACTS to throw a build-time-specific error instead of the generic artifact inventory message, including the missing artifact when practical. Keep the parse-time validation near the separate check unchanged.backend/src/development/developmentGatewayPolicy.ts (1)
10-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
DEVELOPMENT_GATEWAY_PROXY_METHODSfrom the browser allowlist.
DEVELOPMENT_ALLOWED_GATEWAY_METHODSallowssubscribe/unsubscribe, but the proxy allowlist duplicates methods instead of using them, and its current contents are actually more permissive (sessions.subscribe) while rejectingsubscribe/unsubscribeif a browser Gateway call later uses those verbs.♻️ Derive the proxy set from the browser set
const DEVELOPMENT_GATEWAY_PROXY_METHODS = new Set([ - "chat.abort", - "chat.history", - "chat.send", - "config.get", - "cron.list", - "models.list", - "sessions.list", - "sessions.patch", + ...DEVELOPMENT_ALLOWED_GATEWAY_METHODS, "sessions.subscribe", ]);🤖 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/development/developmentGatewayPolicy.ts` around lines 10 - 29, Update DEVELOPMENT_GATEWAY_PROXY_METHODS to derive from DEVELOPMENT_ALLOWED_GATEWAY_METHODS instead of maintaining a separate method list. Preserve the browser allowlist as the single source of truth so subscribe and unsubscribe remain supported and no additional methods such as sessions.subscribe are permitted.
🤖 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/development/developmentStack.ts`:
- Around line 146-159: Update the log append flow around the existing
existsSync/statSync guard and appendFileSync call to serialize the complete log
line before checking the cap. Compute its UTF-8 byte length with
Buffer.byteLength, and append only when the current file size plus the pending
line size is less than or equal to MAX_DEVELOPMENT_LOG_BYTES; otherwise return
without writing.
In `@backend/src/services/databaseOverview.ts`:
- Around line 631-643: Update isDatabaseOverviewSnapshot to also require
candidate.bloatEstimates to be an array before accepting the cached snapshot,
preserving the existing validation checks and ensuring the returned snapshot
satisfies the declared BloatEstimateRow[] field contract.
---
Outside diff comments:
In `@backend/src/routes/metricsRoutes.ts`:
- Around line 106-107: Reset downloadBytes and uploadBytes immediately before
entering the /proc/net/dev fallback after the primary /sys/class/net collection
fails, so fallback totals replace any partial sums rather than accumulating on
top of them. Update the fallback flow associated with these counters while
preserving the existing successful primary-path behavior.
In `@backend/src/services/pullRequestPreviewHost.ts`:
- Around line 1599-1627: The starting record uses a stale timestamp during long
preview startup. In backend/src/services/pullRequestPreviewHost.ts:1599-1627,
refresh startingRecord.updatedAt immediately before writePreviewRecord; in
backend/src/services/pullRequestPreviewHost.ts:1328-1356, refresh the record
timestamp between proxy readiness, sandbox startup, and frontend readiness
phases so isRecentStartup reflects ongoing progress.
---
Nitpick comments:
In `@backend/src/development/developmentGatewayPolicy.ts`:
- Around line 10-29: Update DEVELOPMENT_GATEWAY_PROXY_METHODS to derive from
DEVELOPMENT_ALLOWED_GATEWAY_METHODS instead of maintaining a separate method
list. Preserve the browser allowlist as the single source of truth so subscribe
and unsubscribe remain supported and no additional methods such as
sessions.subscribe are permitted.
In `@backend/src/pullRequestPreviewGatewayProxy.ts`:
- Around line 513-539: Add a concise comment near optionsFromEnvironment or the
relevant unit definition documenting that the upstream token file is read only
during startup and may be deleted after readiness, so the service must not rely
on restart-based recovery. Keep the existing token-loading behavior unchanged.
- Around line 251-287: Update the authentication handshake spanning the
challenge broadcast and isClientAuthenticated so the nonce issued in
connect.challenge is verified as part of the client’s connect request, binding
authentication to that challenge. Reuse the existing challengeNonce state and
request authentication parsing, reject missing or mismatched nonces, and
preserve the current token, upstream, and successful hello-ok checks.
In `@backend/src/releaseManifest.ts`:
- Around line 445-451: Update the validation around
CURRENT_BUILD_REQUIRED_RELEASE_ARTIFACTS to throw a build-time-specific error
instead of the generic artifact inventory message, including the missing
artifact when practical. Keep the parse-time validation near the separate check
unchanged.
In `@backend/test/databaseOverview.test.ts`:
- Around line 476-478: Update the test around the aggregate size assertion to
configure the SQLite fixture or mock with a known storage byte count, then
assert that expected value through overview.sqlite.storageBytes before
validating totalManagedDatabaseSizeBytes. Replace the self-referential 4096 +
overview.sqlite.storageBytes expression with an independent expected total
derived from the controlled fixture value.
In `@backend/test/pullRequestPreviewGatewayProxy.test.ts`:
- Around line 283-290: Move the stop-call assertion from the finally block to
the end of the associated try block, after the proxy behavior assertions. Keep
finally limited to stopping the proxy, closing sockets, and removing the
temporary root so cleanup cannot mask the original test failure.
- Around line 28-72: The websocketHarness waits in next() and closed can hang
indefinitely when messages or closure never arrive. Add short timeout races for
each pending next() waiter and for the closed promise, rejecting with
descriptive errors that identify whether a message or socket close was awaited,
while preserving successful message delivery and closure 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: e460e3b8-aa7a-4de9-be30-a3ef0e0a0cd6
📒 Files selected for processing (36)
backend/scripts/build.tsbackend/src/development/developmentGatewayPolicy.tsbackend/src/development/developmentStack.tsbackend/src/lib/logRoots.tsbackend/src/pullRequestPreviewGatewayProxy.tsbackend/src/releaseManifest.tsbackend/src/requestPolicy.tsbackend/src/routes/logRoutes.tsbackend/src/routes/metricsRoutes.tsbackend/src/services/cacheRefresh.tsbackend/src/services/databaseOverview.tsbackend/src/services/logStreams.tsbackend/src/services/pullRequestPreviewHost.tsbackend/src/services/pullRequestPreviews.tsbackend/src/services/pullRequests.tsbackend/test/databaseOverview.test.tsbackend/test/developmentStack.test.tsbackend/test/pullRequestPreview.test.tsbackend/test/pullRequestPreviewGatewayProxy.test.tsbackend/test/releaseManager.test.tsbackend/test/releaseManifest.test.tsbackend/test/routeAndServiceBehavior.test.tsbackend/test/serviceBehavior.test.tsbackend/test/support/releaseFixture.tsbackend/test/utilityBehavior.test.tsdocs/api/endpoints.mddocs/development/local-dev.mddocs/setup/secrets-and-env.mdscripts/checkTestOutput.tssrc/components/layout/AppHeader.tsxsrc/hooks/useDatabase.tssrc/hooks/useLogs.tssrc/pages/Database.tsxsrc/pages/Logs.tsxsrc/test/frontendBehavior.test.tsxsrc/test/pageBehavior.test.tsx
🚧 Files skipped from review as they are similar to previous changes (13)
- docs/api/endpoints.md
- docs/setup/secrets-and-env.md
- src/components/layout/AppHeader.tsx
- src/test/pageBehavior.test.tsx
- docs/development/local-dev.md
- backend/src/requestPolicy.ts
- backend/test/utilityBehavior.test.ts
- backend/test/pullRequestPreview.test.ts
- backend/src/services/pullRequestPreviews.ts
- backend/test/developmentStack.test.ts
- backend/test/serviceBehavior.test.ts
- src/test/frontendBehavior.test.tsx
- 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/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)
🪛 GitHub Check: CodeQL
backend/src/development/developmentStack.ts
[failure] 149-159: Potential file system race condition
The file may have changed since it was checked.
🔇 Additional comments (37)
backend/test/releaseManager.test.ts (1)
93-103: LGTM!Also applies to: 182-182, 430-442, 477-486, 858-933
backend/test/releaseManifest.test.ts (1)
83-86: LGTM!Also applies to: 207-207, 239-252, 350-350
backend/test/databaseOverview.test.ts (1)
447-475: LGTM!Also applies to: 479-480
scripts/checkTestOutput.ts (1)
15-16: LGTM!backend/test/routeAndServiceBehavior.test.ts (3)
3121-3124: LGTM!
3133-3146: LGTM!
5096-5109: LGTM!backend/test/support/releaseFixture.ts (1)
54-54: LGTM!backend/src/development/developmentStack.ts (1)
3-3: LGTM!Also applies to: 15-15, 25-25, 41-42, 91-145, 160-184, 868-905, 937-937, 953-965, 998-1002
backend/src/lib/logRoots.ts (1)
6-20: LGTM!backend/src/services/logStreams.ts (1)
298-314: LGTM!backend/src/routes/logRoutes.ts (1)
36-44: LGTM!Also applies to: 153-178
backend/src/routes/metricsRoutes.ts (1)
236-243: LGTM!src/hooks/useLogs.ts (1)
8-11: LGTM!Also applies to: 50-65, 89-99
src/pages/Database.tsx (1)
54-59: LGTM!src/pages/Logs.tsx (1)
159-163: LGTM!Also applies to: 170-203, 217-237, 556-565
backend/src/services/cacheRefresh.ts (4)
14-18: LGTM!Also applies to: 31-31
2348-2348: LGTM!Also applies to: 2426-2438
2447-2468: LGTM!
1864-1892: 🗄️ Data Integrity & IntegrationNo change needed for
CacheEntryRow.data.
CacheEntryRow.datais typed as a raw JSON string, soparseJsonField<unknown>(previousEntry?.data || "")is using the correct field type.> Likely an incorrect or invalid review comment.backend/src/services/pullRequestPreviewHost.ts (8)
32-48: LGTM!Also applies to: 91-96
250-262: LGTM!Also applies to: 292-351
407-414: LGTM!Also applies to: 424-479
919-942: LGTM!Also applies to: 952-999
1030-1031: LGTM!Also applies to: 1051-1055, 1111-1111
1164-1166: LGTM!Also applies to: 1180-1226
1392-1472: LGTM!
1612-1618: LGTM!Also applies to: 1640-1646, 1671-1686, 1712-1738
backend/src/pullRequestPreviewGatewayProxy.ts (4)
27-31: LGTM!Also applies to: 85-136
138-194: LGTM!
196-249: LGTM!
289-368: LGTM!Also applies to: 370-402, 481-511
backend/scripts/build.ts (1)
24-24: LGTM!backend/src/releaseManifest.ts (1)
29-43: LGTM!Also applies to: 621-621
backend/test/pullRequestPreviewGatewayProxy.test.ts (1)
74-282: LGTM!backend/src/development/developmentGatewayPolicy.ts (1)
31-47: LGTM!Also applies to: 64-71
src/hooks/useDatabase.ts (1)
6-7: LGTM!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e713b44f1f
ℹ️ 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: 88f539ce78
ℹ️ 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.
🧹 Nitpick comments (2)
src/lib/developmentProxyHeaders.ts (1)
3-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider validating
publicOriginonce at startup.
new URL(...)plus the scheme check runs on every proxied request/upgrade, and a malformed configured origin fails per-request rather than at boot. Parsing/validating once when the dev server starts gives a clearer failure and avoids repeated work on the request 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 `@src/lib/developmentProxyHeaders.ts` around lines 3 - 13, Move parsing and HTTP/HTTPS validation of the configured publicOrigin out of developmentForwardedProtocol and into the development server startup path, so malformed configuration fails during boot. Reuse the validated origin or protocol for each request/upgrade, while preserving requestUrl fallback behavior when publicOrigin is unset.backend/test/pullRequestPreviewGatewayProxy.test.ts (1)
262-276: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded
128duplicatesMAX_CLIENT_PENDING_REQUESTS.If the proxy's limit changes, this stops exercising the boundary (or fails opaquely). Consider exporting the constant from
backend/src/pullRequestPreviewGatewayProxy.tsand using it here.🤖 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/pullRequestPreviewGatewayProxy.test.ts` around lines 262 - 276, Replace the hardcoded pending-request value in the test around handleMessage with the exported MAX_CLIENT_PENDING_REQUESTS constant from pullRequestPreviewGatewayProxy, exporting that constant from the implementation if necessary. Preserve the test’s boundary assertion and reset 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.
Nitpick comments:
In `@backend/test/pullRequestPreviewGatewayProxy.test.ts`:
- Around line 262-276: Replace the hardcoded pending-request value in the test
around handleMessage with the exported MAX_CLIENT_PENDING_REQUESTS constant from
pullRequestPreviewGatewayProxy, exporting that constant from the implementation
if necessary. Preserve the test’s boundary assertion and reset behavior.
In `@src/lib/developmentProxyHeaders.ts`:
- Around line 3-13: Move parsing and HTTP/HTTPS validation of the configured
publicOrigin out of developmentForwardedProtocol and into the development server
startup path, so malformed configuration fails during boot. Reuse the validated
origin or protocol for each request/upgrade, while preserving requestUrl
fallback behavior when publicOrigin is unset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 197f74de-19a4-472d-b480-48e38ce91cb2
📒 Files selected for processing (21)
backend/src/development/developmentOpenClaw.tsbackend/src/development/developmentStack.tsbackend/src/pullRequestPreviewGatewayProxy.tsbackend/src/requestPolicy.tsbackend/src/routes/taskRoutes.tsbackend/src/services/databaseOverview.tsbackend/src/services/pullRequestPreviewHost.tsbackend/src/services/pullRequestPreviews.tsbackend/src/services/pullRequests.tsbackend/test/databaseOverview.test.tsbackend/test/developmentStack.test.tsbackend/test/pullRequestPreview.test.tsbackend/test/pullRequestPreviewGatewayProxy.test.tsbackend/test/routeAndServiceBehavior.test.tsbackend/test/serviceBehavior.test.tsbackend/test/utilityBehavior.test.tsscripts/developmentFrontend.tsscripts/developmentTailscale.tssrc/lib/developmentProxyHeaders.tssrc/test/developmentProxyHeaders.test.tssrc/test/developmentTailscale.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- backend/test/databaseOverview.test.ts
- scripts/developmentFrontend.ts
- backend/test/developmentStack.test.ts
- backend/src/pullRequestPreviewGatewayProxy.ts
- backend/src/services/databaseOverview.ts
- scripts/developmentTailscale.ts
- backend/test/pullRequestPreview.test.ts
- backend/src/development/developmentOpenClaw.ts
- backend/test/utilityBehavior.test.ts
- backend/src/development/developmentStack.ts
- backend/src/services/pullRequestPreviews.ts
- backend/src/services/pullRequestPreviewHost.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 comments (10)
backend/src/requestPolicy.ts (1)
172-177: LGTM!backend/src/routes/taskRoutes.ts (2)
11-11: LGTM!
319-332: LGTM!backend/test/pullRequestPreviewGatewayProxy.test.ts (2)
13-28: LGTM!
294-351: LGTM!backend/test/serviceBehavior.test.ts (1)
1918-1975: LGTM!src/test/developmentProxyHeaders.test.ts (1)
3-29: LGTM!src/test/developmentTailscale.test.ts (2)
1-108: LGTM!
110-155: LGTM!backend/test/routeAndServiceBehavior.test.ts (1)
1029-1050: 📐 Maintainability & Code QualityNo change needed.
The length assertion can fail because
notifyMirareturns immediately when dev safe mode is enabled, while the mockedsendSessionMessageappends synchronously. The inline restore is redundant only in thatrememberEnvironmentalready scheduled cleanup, but it does not introduce a correctness issue.> Likely an incorrect or invalid review comment.
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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 |
|
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". |
3 participants
Summary
currentandpreviousrelease status, guarded exact-SHA rollback, shared/exclusive lifecycle locking, and automatic restoration when rollback readiness failsmainBehavior and regression coverage
Release and rollback behavior
The Dashboard reports immutable managed release slots and queues deploy/rollback work through the exclusive job lifecycle. Rollback confirmation binds the full current and target commit SHAs at the API, worker, and lifecycle boundaries. If either slot changes before the release lock is acquired, the rollback fails instead of switching a different release.
Lifecycle status reads share the release lock and wait through active transitions. A rollback swaps the managed symlinks, restarts web and worker, requires commit-bound readiness, and restores the original release automatically if the rollback target does not become ready. Orphan recovery selects the guarded lifecycle from the persisted action context: deploy recovery uses the candidate lifecycle, while manual rollback recovery uses the original/current activation lifecycle that scheduled the rollback.
Development and PR-dev behavior
bun run devstarts the complete local stack from the repository. It uses isolated Dashboard state and workspace data, retains the production login and elevated-auth intervals, runs the isolated scheduler/worker profile, and connects to the live Gateway so agents, sessions, chat, files, cron metadata, and redacted settings remain representative. Synthetic rotating dev logs exercise the complete Logs UI without mounting production logs.Copied WebAuthn credentials are retained only when the source and dev RP IDs match. Otherwise incompatible credentials are removed and MFA is disabled in the isolated snapshot so local password login and factor enrollment remain possible.
The Pull requests page can start or stop one trusted PR in dev:
0600proxy credential; the production token is never mounted into the sandbox, sent to the browser, or included in a unit commandconfig.get, filters unrelated events, and rejects config writes, cron mutations, destructive session calls, and other host capabilitiesA pre-existing Serve route that is not owned by the managed preview is rejected with a conflict.
Review findings included
bloatEstimatesO_NOFOLLOWdescriptornullrollback request bodyVerification
bun run build:frontendbun run build:backendbun run lint:frontendbun run lint:backendbun run format:check:5173route are stoppedcb92bc20af4b8fc1; both reported nitpicks are fixed incb92bc20Review rate limitedafter those fixesRisk checklist
Deployment/operations
:5173route are stopped.preview-pr-335worktree was moved to the system trash after the final reviewed head was confirmed; the canonical branch worktree remains until merge.Notes for reviewers
Dependency and tooling notes
@microlink/react-json-view, TanStack DB packages,lucide-react, ESLint, andglobals.typescript-eslintcompiler API; the native TypeScript 7 build package remains separate.bun run test:changedselects frontend and backend tests affected by the Git working-tree diff. Full tests and coverage remain the before-push/CI gates.Reviewer focus
Please focus on the release lock/rollback recovery contract and the trusted PR-dev boundary: exact-SHA selection, host proxy capability policy, isolated job profile, sandbox invocation, config redaction, token handling, readiness, resource limits, and cleanup ownership.