feat(runtime): add resilient recovery control plane - #13
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change keeps the control plane available during Runtime and configuration failures. It adds deferred activation, Config Recovery, Runtime Data inspection and deletion, serialized mutations, new protocol contracts, and Settings recovery workflows. ChangesRuntime Control Plane Recovery
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/features/SettingsDialog.tsx (1)
47-57: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset
savedWhileRuntimeUnavailablewhen the snapshot changes.
saveclears the flag at Line 82 and the Reload button clears it at Line 132. The snapshot effect does not. Any other caller ofonReload— for exampleSettingsSecurityPanelthroughonConfigChanged— replaces the snapshot while the "Configuration saved. Retry Runtime to use the saved configuration." notice stays on screen. The notice then refers to a save that is no longer the latest change.Clear the flag with the other per-snapshot state.
🔧 Proposed fix
useEffect(() => { setDraft(cloneConfig(snapshot.config)); setErrors({}); setJsonErrors({}); setSaveError(undefined); + setSavedWhileRuntimeUnavailable(false); setRestartRequiredSections(snapshot.restartRequiredSections); setJsonResetVersion((current) => current + 1); }, [snapshot]);🤖 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 `@apps/web/src/components/features/SettingsDialog.tsx` around lines 47 - 57, Update the snapshot-change useEffect in the SettingsDialog component to reset savedWhileRuntimeUnavailable alongside the other per-snapshot state, ensuring the stale runtime-unavailable notice is cleared whenever snapshot changes.
🧹 Nitpick comments (12)
packages/agent-core/src/config/server-config-service.ts (1)
325-327: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOverlapping removal paths abort a valid selection.
dedupeRemovalTargetsremoves only exact duplicate paths. The semantic phase can emit both an ancestor and a descendant target for the same provider, for example["provider", id]from the options check at Line 690 and["provider", id, "options", "x"]fromvalidateSecretValuePlacement. If the user selects both items,deleteConfigPathdeletes the parent first, then throwsInvalidConfigRemovalErrorfor the child because the key no longer exists. The file is restored, so no data is lost, but the user cannot apply the full plan.Delete shallow paths last, or skip a path whose ancestor was already removed.
♻️ Suggested handling for nested selections
- for (const item of selected as InvalidConfigRemovalItem[]) { - deleteConfigPath(candidate, item.path); - } + const removals = (selected as InvalidConfigRemovalItem[]) + .map((item) => item.path) + .sort((left, right) => right.length - left.length); + const removed: readonly string[][] = []; + for (const path of removals) { + // Skip a path already removed together with its ancestor. + if (removed.some((done) => done.length < path.length + && done.every((segment, index) => path[index] === segment))) continue; + deleteConfigPath(candidate, path); + (removed as string[][]).push([...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 `@packages/agent-core/src/config/server-config-service.ts` around lines 325 - 327, Update the removal loop over selected InvalidConfigRemovalItem entries to handle overlapping paths safely: when an ancestor path is selected, skip its descendant targets or order removals so descendants are processed before ancestors, preventing deleteConfigPath from throwing after a parent is removed. Preserve exact-duplicate deduplication and ensure valid combined selections apply successfully.apps/web/src/components/features/SettingsConfigRecoveryPanel.interaction.tsx (1)
161-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
inputTextdepends on React internals.The helper reads
_valueTrackerand searches for a__reactProps$key, then callsonChangedirectly. React 19 does not guarantee either name, so a React upgrade breaks this helper and both tests in this file.Dispatch a real
inputevent after the native value setter. React's delegated event system then produces the synthetic change event.♻️ Suggested helper without internal access
async function inputText(value: string, root: ParentNode): Promise<void> { const input = root.querySelector<HTMLInputElement>('input[type="text"]'); if (!input) throw new Error("Missing confirmation input"); await act(async () => { - const previous = input.value; const setter = Object.getOwnPropertyDescriptor(dom.window.HTMLInputElement.prototype, "value")!.set!; setter.call(input, value); - (input as unknown as { _valueTracker?: { setValue(value: string): void } })._valueTracker?.setValue(previous); - const propsKey = Object.keys(input).find((key) => key.startsWith("__reactProps$")); - const props = propsKey - ? (input as unknown as Record<string, { onChange?: (event: { target: HTMLInputElement }) => void }>)[propsKey] - : undefined; - if (!props?.onChange) throw new Error("Missing confirmation input change handler"); - props.onChange({ target: input }); + input.dispatchEvent(new dom.window.Event("input", { bubbles: true })); await new Promise((resolve) => setTimeout(resolve, 0)); }); }🤖 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 `@apps/web/src/components/features/SettingsConfigRecoveryPanel.interaction.tsx` around lines 161 - 177, Update inputText to stop accessing React internals such as _valueTracker and __reactProps$ or invoking onChange directly. After setting the input value with the native HTMLInputElement setter, dispatch a real bubbling input event so React’s delegated event system produces the synthetic change event, while preserving the existing act and async behavior.packages/agent-core/src/config/server-config-service.test.ts (1)
717-741: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for nested removal selections and for an externally repaired Config.
Two uncovered paths remain:
- A plan that contains both an ancestor path and a descendant path for the same provider.
deleteConfigPathcurrently rejects the descendant after the ancestor is removed. See the comment onserver-config-service.tsLines 325-327.removeInvalidConfigItemswhen the file becomes valid on disk after the plan is built. That reaches theConfigRecoveryConflictErrorbranch atserver-config-service.tsLine 274, which no test exercises. Line 843-880 covers only the changed-but-still-invalid case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/config/server-config-service.test.ts` around lines 717 - 741, Extend server-config-service tests to cover nested removal selections where a plan includes both an ancestor and descendant path for the same provider, verifying removeInvalidConfigItems handles the selection without rejecting the descendant after ancestor removal. Add coverage for externally repaired configuration by building a plan, making the file valid on disk, then asserting removeInvalidConfigItems raises ConfigRecoveryConflictError.packages/agent-core/src/__arch__/automation-boundaries.test.ts (1)
56-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese ordering assertions pass when a needle is missing.
indexOfreturns-1for an absent string, and-1is less than any real index. If a future refactor renames or removesconst runtimeApp = createRuntimeApp(runtime,startServer(host.app, orawait ArchCodeServerHost.create, the ordering assertions still pass and the architecture guard stops protecting the boundary. Assert each index is not-1first.♻️ Suggested guard
+ const indexOfRequired = (source: string, needle: string): number => { + const index = source.indexOf(needle); + expect(index).toBeGreaterThanOrEqual(0); + return index; + };Then replace each
x.indexOf(needle)in these comparisons withindexOfRequired(x, needle).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/__arch__/automation-boundaries.test.ts` around lines 56 - 66, Update the ordering assertions in automation-boundaries.test.ts to validate that each searched string exists before comparing positions. Use the existing or introduce the suggested indexOfRequired helper for the needles in the runtimeApp, ArchCodeServerHost.create, and startServer(host.app) checks, then perform the ordering comparisons with its validated results.apps/web/src/components/features/SettingsRuntimeDataPanel.interaction.tsx (1)
68-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the cleanup-blocked and inspection-failure states.
All four tests start from
recoveryAllowed: trueand a successful inspection. Two documented states stay untested:runtime.state === "error"withrecoveryAllowed: false, where the panel must clear the selection, close the confirmation, and block deletion; and a failedinspectRuntimeDatacall, where the panel must show the inspection error. Both are the exact guards that prevent a destructive request in a broken Runtime.🤖 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 `@apps/web/src/components/features/SettingsRuntimeDataPanel.interaction.tsx` around lines 68 - 178, Extend the “Settings Runtime Data interactions” suite with coverage for a runtime error whose recoveryAllowed is false, asserting the selection and confirmation close and no DELETE request is sent, and for an inspectRuntimeData failure, asserting the inspection error is displayed and deletion is blocked. Reuse the existing renderPanel, fetch-mocking, and request-tracking patterns without changing the current successful-deletion tests.packages/agent-core/src/runtime-data/service.ts (1)
344-361: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRuntime Data inspection has no resource bounds. The service is specified to produce a bounded inspection DTO, and
MAX_SCHEMA_ISSUES_PER_FILEbounds only schema issues. Two other inputs grow with on-disk state, andinspect()runs for every registered project concurrently.
packages/agent-core/src/runtime-data/service.ts#L344-L361: reject a file above a maximum byte size beforehandle.readFileloads it fully into memory;lstatForInspectionalready providesstat.size.packages/agent-core/src/runtime-data/service.ts#L255-L297: add a maximum issue count inaddIssueand stop collecting entries after that limit, so one corrupted tree cannot grow the response without bound.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/runtime-data/service.ts` around lines 344 - 361, Bound runtime-data inspection in packages/agent-core/src/runtime-data/service.ts at lines 344-361 by checking the size from lstatForInspection before handle.readFile and treating oversized files as unreadable; also update addIssue at lines 255-297 to stop collecting issues once a defined maximum issue count is reached, while preserving existing issue handling below the limit.apps/web/src/components/features/SettingsDialog.interaction.tsx (1)
632-702: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the Runtime data delete paths.
The new Runtime Data tests cover inspection, disabled controls, and Runtime retry. No test exercises
confirmDelete. Both branches of that flow carry risk:
- A successful delete must clear the selection, close the dialog, and refresh inspection plus Runtime status.
- A rejected delete currently leaves the confirmation dialog open while the error renders behind it (see the finding in
apps/web/src/components/features/SettingsRuntimeDataPanel.tsx).Add one test per branch so the dialog-close behaviour is pinned.
🤖 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 `@apps/web/src/components/features/SettingsDialog.interaction.tsx` around lines 632 - 702, Add two tests for confirmDelete in SettingsRuntimeDataPanel: verify a successful deletion clears selected projects, closes the confirmation dialog, and refreshes inspection and Runtime status; verify a rejected deletion also closes the confirmation dialog while displaying the error. Reuse the existing Runtime Data test setup and mock delete requests to cover both branches.apps/web/src/components/features/SettingsDialog.tsx (1)
213-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
IndependentSettingsWorkspaceomitsinvalidProfileCount.
SettingsBodypassesinvalidProfileCounttoSettingsSidebarat Line 119 so the Profiles entry shows the attention indicator.IndependentSettingsWorkspacerenders the same sidebar without it, so the indicator disappears while the user views Updates or Runtime Data. No config snapshot is loaded in this workspace, so the count is not available here. Confirm this loss of the indicator is intended.🤖 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 `@apps/web/src/components/features/SettingsDialog.tsx` around lines 213 - 214, The SettingsSidebar component rendered in IndependentSettingsWorkspace is missing the invalidProfileCount prop that SettingsBody passes at line 119 to display the Profiles attention indicator. Either add invalidProfileCount as a parameter to the IndependentSettingsWorkspace function signature and pass it to SettingsSidebar, or explicitly pass a default value (such as 0) to SettingsSidebar to intentionally suppress the indicator in this workspace context.apps/server/src/setup-grant.test.ts (1)
2-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename this test file to
terminal-grant.test.ts.The unit under test is now
terminal-grant.ts. The coding guidelines require the test file to be colocated and named<name>.test.tsfor the file under test. Renameapps/server/src/setup-grant.test.tstoapps/server/src/terminal-grant.test.ts. Also update the test titles at lines 5 and 16, which still describe setup-only behavior even though the grant now serves Config Recovery.As per coding guidelines: "测试文件应与被测文件 colocate，命名为
<name>.test.ts".🤖 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 `@apps/server/src/setup-grant.test.ts` around lines 2 - 4, Rename the test file associated with TerminalGrant from setup-grant.test.ts to terminal-grant.test.ts, and update the test titles in the TerminalGrant describe block to reflect Config Recovery grant behavior rather than setup-only behavior. Keep the existing test coverage and implementation unchanged.Source: Coding guidelines
apps/web/src/api/config-recovery.ts (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOne narrow
apiFetchbody type forces three double casts. Every new recovery helper force-casts a typed protocol request throughas unknown as Record<string, unknown>because thebodyparameter ofapiFetchdoes not accept readonly protocol object types. Widenbodyinapps/web/src/api/client.tsto a JSON-serializable type, then remove the casts.
apps/web/src/api/config-recovery.ts#L33-L33: passbody(aResetInvalidConfigRequest) without a cast.apps/web/src/api/config-recovery.ts#L51-L51: passbody(aRemoveInvalidConfigItemsRequest) without a cast.apps/web/src/api/runtime-data.ts#L18-L18: passrequest(aRuntimeDataDeleteRequest) without a cast.🤖 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 `@apps/web/src/api/config-recovery.ts` at line 33, Widen the body parameter of apiFetch in apps/web/src/api/client.ts to accept JSON-serializable readonly protocol objects, then remove the double casts at apps/web/src/api/config-recovery.ts:33 and :51 and pass the typed body values directly; likewise pass request directly at apps/web/src/api/runtime-data.ts:18. Preserve the existing request behavior.apps/web/src/api/runtime-data.test.ts (1)
11-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe assertion does not prove the claim in the test name.
expect.objectContainingignores extra properties, so this test passes even ifinspectRuntimeDatasends a body. Assert the absence directly.💚 Proposed assertion
await expect(inspectRuntimeData()).resolves.toEqual({ projects: [] }); expect(fetch).toHaveBeenCalledWith("/api/runtime-data", expect.objectContaining({ credentials: "same-origin", })); + const init = (fetch as unknown as ReturnType<typeof mock>).mock.calls[0]![1] as RequestInit; + expect(init.body).toBeUndefined(); + expect(init.method).toBeUndefined();🤖 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 `@apps/web/src/api/runtime-data.test.ts` around lines 11 - 18, Update the fetch assertion in the “inspects Runtime data without a request body” test to explicitly verify that the request has no body, rather than only matching credentials with expect.objectContaining. Keep the existing endpoint and credentials checks, and assert the body’s absence directly.apps/server/src/server-host.test.ts (1)
1744-1753: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRaise the
waitForRuntimeStatewait budget.The helper waits at most 100 iterations of
Bun.sleep(1), so roughly 100 ms plus scheduling. The tests at lines 1513-1614 and 1616-1740 use the realRuntimeDataService, a realProjectRegistry, and real filesystem work before the Runtime reacheserror. On a loaded CI machine that budget can expire and throwRuntime did not reach error. The repository guidelines forbid retry-based flaky-test mitigation, so the wait must be generous in the helper itself.♻️ Proposed deadline-based wait
async function waitForRuntimeState( host: ArchCodeServerHost, state: "activating" | "ready" | "error", ): Promise<void> { - for (let attempt = 0; attempt < 100; attempt += 1) { + const deadline = Date.now() + 5_000; + while (Date.now() < deadline) { if (host.getRuntimeStatus().state === state) return; await Bun.sleep(1); } throw new Error(`Runtime did not reach ${state}`); }🤖 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 `@apps/server/src/server-host.test.ts` around lines 1744 - 1753, Increase the wait budget in waitForRuntimeState so tests using real runtime, registry, and filesystem work have sufficient time to reach the requested state on loaded CI machines. Prefer a generous deadline-based wait over the current fixed 100-iteration retry limit, while preserving the existing state check and timeout error behavior.Source: Coding guidelines
🤖 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 `@apps/server/src/server-host.ts`:
- Around line 246-261: Remove the unguarded runtimeDataService.inspect() call
from the all-deleted branch in runHostMutation, while preserving the subsequent
activateRuntimeFromCurrentConfig retry and successful deletion response. Do not
allow post-delete inspection failures to reject the mutation or prevent runtime
activation.
In `@apps/web/src/components/bootstrap/BootstrapGate.test.tsx`:
- Around line 152-157: Update the Config Recovery response fixture in the
request handler checking “/api/config-recovery” to include the required
removableItems field with an empty array, matching the ConfigRecoveryStatus
contract and server response.
In `@apps/web/src/components/features/SettingsConfigRecoveryPanel.tsx`:
- Around line 144-218: The SettingsConfigRecoveryPanel renders no recovery
actions when getConfigRecoveryStatus fails and recovery is undefined. Update the
recovery/error rendering around the recovery conditional to provide an available
retry control in that failure state, reusing the existing retry or
status-loading handler and matching the inline retry behavior of
SettingsRuntimeDataPanel; preserve the current recovery details for successful
loads.
In `@apps/web/src/components/features/SettingsRuntimeDataPanel.tsx`:
- Around line 117-123: Update the deleteRuntimeData catch path to close the
confirmation dialog via setConfirmOpen(false) before the finally block runs,
matching the success path so the actionError alert is visible and
statusHeadingRef focus remains outside only after the modal closes.
In `@packages/agent-core/src/config/server-config-service.ts`:
- Around line 1389-1419: Update restoreClaimedConfig so every cleanup unlink of
claimedPath is best-effort: catch and suppress unlink failures while preserving
the original restore error or return behavior. When handle.writeFile or
handle.sync fails, ensure configDiscardError includes claimedPath in its message
so the retained config can be located. Keep the claimed file available when
restoration fails before cleanup.
In `@packages/agent-core/src/runtime-data/service.test.ts`:
- Around line 425-439: Update the chmod-based failure setup in the delete test
around service.delete to skip the test when the effective UID is 0, or otherwise
use a failure injection that remains effective for root. Preserve the existing
assertions for non-root execution and ensure cleanup still restores the
directory permissions.
---
Outside diff comments:
In `@apps/web/src/components/features/SettingsDialog.tsx`:
- Around line 47-57: Update the snapshot-change useEffect in the SettingsDialog
component to reset savedWhileRuntimeUnavailable alongside the other per-snapshot
state, ensuring the stale runtime-unavailable notice is cleared whenever
snapshot changes.
---
Nitpick comments:
In `@apps/server/src/server-host.test.ts`:
- Around line 1744-1753: Increase the wait budget in waitForRuntimeState so
tests using real runtime, registry, and filesystem work have sufficient time to
reach the requested state on loaded CI machines. Prefer a generous
deadline-based wait over the current fixed 100-iteration retry limit, while
preserving the existing state check and timeout error behavior.
In `@apps/server/src/setup-grant.test.ts`:
- Around line 2-4: Rename the test file associated with TerminalGrant from
setup-grant.test.ts to terminal-grant.test.ts, and update the test titles in the
TerminalGrant describe block to reflect Config Recovery grant behavior rather
than setup-only behavior. Keep the existing test coverage and implementation
unchanged.
In `@apps/web/src/api/config-recovery.ts`:
- Line 33: Widen the body parameter of apiFetch in apps/web/src/api/client.ts to
accept JSON-serializable readonly protocol objects, then remove the double casts
at apps/web/src/api/config-recovery.ts:33 and :51 and pass the typed body values
directly; likewise pass request directly at apps/web/src/api/runtime-data.ts:18.
Preserve the existing request behavior.
In `@apps/web/src/api/runtime-data.test.ts`:
- Around line 11-18: Update the fetch assertion in the “inspects Runtime data
without a request body” test to explicitly verify that the request has no body,
rather than only matching credentials with expect.objectContaining. Keep the
existing endpoint and credentials checks, and assert the body’s absence
directly.
In
`@apps/web/src/components/features/SettingsConfigRecoveryPanel.interaction.tsx`:
- Around line 161-177: Update inputText to stop accessing React internals such
as _valueTracker and __reactProps$ or invoking onChange directly. After setting
the input value with the native HTMLInputElement setter, dispatch a real
bubbling input event so React’s delegated event system produces the synthetic
change event, while preserving the existing act and async behavior.
In `@apps/web/src/components/features/SettingsDialog.interaction.tsx`:
- Around line 632-702: Add two tests for confirmDelete in
SettingsRuntimeDataPanel: verify a successful deletion clears selected projects,
closes the confirmation dialog, and refreshes inspection and Runtime status;
verify a rejected deletion also closes the confirmation dialog while displaying
the error. Reuse the existing Runtime Data test setup and mock delete requests
to cover both branches.
In `@apps/web/src/components/features/SettingsDialog.tsx`:
- Around line 213-214: The SettingsSidebar component rendered in
IndependentSettingsWorkspace is missing the invalidProfileCount prop that
SettingsBody passes at line 119 to display the Profiles attention indicator.
Either add invalidProfileCount as a parameter to the
IndependentSettingsWorkspace function signature and pass it to SettingsSidebar,
or explicitly pass a default value (such as 0) to SettingsSidebar to
intentionally suppress the indicator in this workspace context.
In `@apps/web/src/components/features/SettingsRuntimeDataPanel.interaction.tsx`:
- Around line 68-178: Extend the “Settings Runtime Data interactions” suite with
coverage for a runtime error whose recoveryAllowed is false, asserting the
selection and confirmation close and no DELETE request is sent, and for an
inspectRuntimeData failure, asserting the inspection error is displayed and
deletion is blocked. Reuse the existing renderPanel, fetch-mocking, and
request-tracking patterns without changing the current successful-deletion
tests.
In `@packages/agent-core/src/__arch__/automation-boundaries.test.ts`:
- Around line 56-66: Update the ordering assertions in
automation-boundaries.test.ts to validate that each searched string exists
before comparing positions. Use the existing or introduce the suggested
indexOfRequired helper for the needles in the runtimeApp,
ArchCodeServerHost.create, and startServer(host.app) checks, then perform the
ordering comparisons with its validated results.
In `@packages/agent-core/src/config/server-config-service.test.ts`:
- Around line 717-741: Extend server-config-service tests to cover nested
removal selections where a plan includes both an ancestor and descendant path
for the same provider, verifying removeInvalidConfigItems handles the selection
without rejecting the descendant after ancestor removal. Add coverage for
externally repaired configuration by building a plan, making the file valid on
disk, then asserting removeInvalidConfigItems raises
ConfigRecoveryConflictError.
In `@packages/agent-core/src/config/server-config-service.ts`:
- Around line 325-327: Update the removal loop over selected
InvalidConfigRemovalItem entries to handle overlapping paths safely: when an
ancestor path is selected, skip its descendant targets or order removals so
descendants are processed before ancestors, preventing deleteConfigPath from
throwing after a parent is removed. Preserve exact-duplicate deduplication and
ensure valid combined selections apply successfully.
In `@packages/agent-core/src/runtime-data/service.ts`:
- Around line 344-361: Bound runtime-data inspection in
packages/agent-core/src/runtime-data/service.ts at lines 344-361 by checking the
size from lstatForInspection before handle.readFile and treating oversized files
as unreadable; also update addIssue at lines 255-297 to stop collecting issues
once a defined maximum issue count is reached, while preserving existing issue
handling below the limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 468e6be4-1bd7-4df8-b6ec-009ae193b484
📒 Files selected for processing (55)
apps/server/src/app.test.tsapps/server/src/app.tsapps/server/src/boot.tsapps/server/src/errors.tsapps/server/src/main.tsapps/server/src/routes/config-recovery.test.tsapps/server/src/routes/config-recovery.tsapps/server/src/routes/runtime-control.test.tsapps/server/src/routes/runtime-control.tsapps/server/src/server-host.test.tsapps/server/src/server-host.tsapps/server/src/setup-grant.test.tsapps/server/src/terminal-grant.tsapps/web/package.jsonapps/web/src/api/config-recovery.test.tsapps/web/src/api/config-recovery.tsapps/web/src/api/runtime-data.test.tsapps/web/src/api/runtime-data.tsapps/web/src/api/update.tsapps/web/src/components/bootstrap/BootstrapGate.test.tsxapps/web/src/components/bootstrap/BootstrapGate.tsxapps/web/src/components/features/ConfigRecoverySettings.tsxapps/web/src/components/features/SettingsConfigRecoveryPanel.interaction.tsxapps/web/src/components/features/SettingsConfigRecoveryPanel.tsxapps/web/src/components/features/SettingsDialog.interaction.tsxapps/web/src/components/features/SettingsDialog.test.tsxapps/web/src/components/features/SettingsDialog.tsxapps/web/src/components/features/SettingsRuntimeDataPanel.interaction.tsxapps/web/src/components/features/SettingsRuntimeDataPanel.tsxapps/web/src/components/features/SettingsUpdatesPanel.tsxapps/web/src/components/features/settings-helpers.tsapps/web/src/components/features/settings-panels.tsxapps/web/src/main.tsxdesign-system/pages/settings.mddocs/goals/runtime-control-plane-recovery-plan-goal.mddocs/goals/runtime-control-plane-recovery-progress.mdpackages/agent-core/src/__arch__/automation-boundaries.test.tspackages/agent-core/src/__arch__/runtime-data-boundaries.test.tspackages/agent-core/src/config/index.tspackages/agent-core/src/config/server-config-service.test.tspackages/agent-core/src/config/server-config-service.tspackages/agent-core/src/index.tspackages/agent-core/src/lead-architecture-flows.integration.test.tspackages/agent-core/src/main.test.tspackages/agent-core/src/runtime-automations.integration.test.tspackages/agent-core/src/runtime-automations.test.tspackages/agent-core/src/runtime-data/index.tspackages/agent-core/src/runtime-data/service.test.tspackages/agent-core/src/runtime-data/service.tspackages/agent-core/src/runtime-mcp.test.tspackages/agent-core/src/runtime.tspackages/agent-core/src/tool-output/artifact-lifecycle.test.tspackages/protocol/src/bootstrap.tspackages/protocol/src/index.tspackages/protocol/src/runtime-data.ts
💤 Files with no reviewable changes (2)
- apps/server/src/app.ts
- apps/server/src/app.test.ts
Owner
Author
|
Review triage for head 624b37f: Fixed:
Not changed after validation:
Validation: full test suite, typecheck, production build, binary version check, and diff checks pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/components/features/SettingsRuntimeDataPanel.tsx (1)
85-88: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSeparate Runtime status refresh failures from recovery-operation failures.
If
onRefreshRuntime()rejects afterretryRuntime()succeeds, the catch treats the refresh as a retry failure and invokesonRefreshRuntime()again. A second rejection can escape the void event handler. If the delete request succeeds but the final refresh rejects, the panel reports “Unable to delete Runtime data.” even though deletion completed.
apps/web/src/components/features/SettingsRuntimeDataPanel.tsx#L85-L88: Refresh once in a separate error path after retry. Preserve the retry result.apps/web/src/components/features/SettingsRuntimeDataPanel.tsx#L116-L122: Report status-refresh failure separately. Do not overwrite a successful deletion result.🤖 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 `@apps/web/src/components/features/SettingsRuntimeDataPanel.tsx` around lines 85 - 88, Separate recovery-operation errors from status-refresh errors in the retry and delete handlers. Around the retryRuntime flow at apps/web/src/components/features/SettingsRuntimeDataPanel.tsx#L85-L88, preserve a successful retry result and perform the single onRefreshRuntime call in its own guarded error path. Around the delete flow at apps/web/src/components/features/SettingsRuntimeDataPanel.tsx#L116-L122, report a final refresh failure separately without replacing the successful deletion result.packages/agent-core/src/runtime-data/service.ts (1)
263-319: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift为 Runtime Data inspection 增加总预算和有界并发。
MAX_RUNTIME_DATA_ISSUES_PER_PROJECT只限制返回的 issue 数量。MAX_INSPECTED_JSON_FILE_BYTES只限制单个 JSON 文件。inspect()仍使用Promise.all检查所有注册项目。scanRuntimeTree()无界遍历目录项；#inspectSessions()随后再次枚举 sessions，并为每个 session 读取 JSON 文件。大量项目或深 Runtime tree 会持续消耗控制面的 I/O、CPU 和内存。
- 为
scanRuntimeTree()增加目录项、累计字节数、深度和时间预算。- 复用
#inspectSessions()的 session enumeration，或避免第二次遍历。- 在继续解析已知 JSON 文件前传播截断状态。
- 使用有界项目并发，替代
Promise.all。- 如果新增截断状态，同步
packages/protocol/src/runtime-data.ts、Web UI mapping，并增加大目录树、多 session 和多项目测试。测试必须断言达到预算后停止工作，而不只是断言 issue 数量不超过 100。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/runtime-data/service.ts` around lines 263 - 319, Add budget enforcement to prevent unbounded resource consumption during Runtime Data inspection. In scanRuntimeTree(), add counters and limits for directory items count, cumulative bytes, depth, and elapsed time, returning early when any budget is exceeded. In the RuntimeDataStats type (currently tracking fileCount and totalBytes), add a truncated flag to signal when limits were reached. Propagate this truncation status through inspectSessions() to avoid parsing JSON files after budgets are exhausted, and update the flow to reuse the session enumeration from scanRuntimeTree() rather than enumerating sessions twice. In the inspect() method, replace Promise.all with bounded concurrency control when iterating projects instead of launching all inspection tasks simultaneously. Define budget constants (for directory items, cumulative bytes, depth, and time limits) alongside existing MAX_RUNTIME_DATA_ISSUES_PER_PROJECT and MAX_INSPECTED_JSON_FILE_BYTES. Sync the truncation status field to packages/protocol/src/runtime-data.ts and Web UI mappings, and add tests verifying that work stops after reaching budgets (not just that issue counts stay under 100) for scenarios with large directory trees, multiple sessions, and multiple projects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/agent-core/src/runtime-data/service.ts`:
- Around line 352-355: 在 packages/agent-core/src/runtime-data/service.ts 的
inspectJsonFile() 及其预检查、删除前复查路径中，将超出 64 MiB 的问题从 unreadable 独立为
inspection_limit；更新 packages/protocol/src/runtime-data.ts 和 UI 的 issueReason()
以支持该原因。仅有 inspection_limit 时仍须执行 assertSafeDeletionTarget() 并允许删除；补充测试验证超过 64
MiB 的 JSON 文件可被删除。涉及
service.ts:352-355、service.ts:116-123、service.ts:130-132，三个位置均需按上述逻辑更新。
---
Outside diff comments:
In `@apps/web/src/components/features/SettingsRuntimeDataPanel.tsx`:
- Around line 85-88: Separate recovery-operation errors from status-refresh
errors in the retry and delete handlers. Around the retryRuntime flow at
apps/web/src/components/features/SettingsRuntimeDataPanel.tsx#L85-L88, preserve
a successful retry result and perform the single onRefreshRuntime call in its
own guarded error path. Around the delete flow at
apps/web/src/components/features/SettingsRuntimeDataPanel.tsx#L116-L122, report
a final refresh failure separately without replacing the successful deletion
result.
In `@packages/agent-core/src/runtime-data/service.ts`:
- Around line 263-319: Add budget enforcement to prevent unbounded resource
consumption during Runtime Data inspection. In scanRuntimeTree(), add counters
and limits for directory items count, cumulative bytes, depth, and elapsed time,
returning early when any budget is exceeded. In the RuntimeDataStats type
(currently tracking fileCount and totalBytes), add a truncated flag to signal
when limits were reached. Propagate this truncation status through
inspectSessions() to avoid parsing JSON files after budgets are exhausted, and
update the flow to reuse the session enumeration from scanRuntimeTree() rather
than enumerating sessions twice. In the inspect() method, replace Promise.all
with bounded concurrency control when iterating projects instead of launching
all inspection tasks simultaneously. Define budget constants (for directory
items, cumulative bytes, depth, and time limits) alongside existing
MAX_RUNTIME_DATA_ISSUES_PER_PROJECT and MAX_INSPECTED_JSON_FILE_BYTES. Sync the
truncation status field to packages/protocol/src/runtime-data.ts and Web UI
mappings, and add tests verifying that work stops after reaching budgets (not
just that issue counts stay under 100) for scenarios with large directory trees,
multiple sessions, and multiple projects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f446d93e-8628-4b23-ab93-bf8c419bad4a
📒 Files selected for processing (15)
apps/server/src/server-host.test.tsapps/server/src/server-host.tsapps/server/src/terminal-grant.test.tsapps/web/src/components/bootstrap/BootstrapGate.test.tsxapps/web/src/components/features/SettingsConfigRecoveryPanel.interaction.tsxapps/web/src/components/features/SettingsConfigRecoveryPanel.tsxapps/web/src/components/features/SettingsDialog.interaction.tsxapps/web/src/components/features/SettingsDialog.tsxapps/web/src/components/features/SettingsRuntimeDataPanel.interaction.tsxapps/web/src/components/features/SettingsRuntimeDataPanel.tsxpackages/agent-core/src/__arch__/automation-boundaries.test.tspackages/agent-core/src/config/server-config-service.test.tspackages/agent-core/src/config/server-config-service.tspackages/agent-core/src/runtime-data/service.test.tspackages/agent-core/src/runtime-data/service.ts
💤 Files with no reviewable changes (1)
- apps/server/src/server-host.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/agent-core/src/arch/automation-boundaries.test.ts
- apps/web/src/components/bootstrap/BootstrapGate.test.tsx
- apps/web/src/components/features/SettingsRuntimeDataPanel.interaction.tsx
- packages/agent-core/src/runtime-data/service.test.ts
- apps/web/src/components/features/SettingsDialog.tsx
- packages/agent-core/src/config/server-config-service.ts
- apps/server/src/server-host.test.ts
Owner
Author
|
Second review triage for head 5d0225d: Fixed:
Not folded into this PR:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
Previously, an invalid persisted Runtime record or strict Config error could prevent the Runtime-backed server from becoming usable, leaving users without an in-product recovery path. Config recovery was also too destructive when only one bounded item was invalid.
User impact
Users can now open Settings during recovery, inspect affected Runtime data, remove only selected invalid Config items, preserve healthy providers/models/profiles/MCP entries and secrets, and retry activation without restarting the process. Failed or stale recovery attempts leave the original Config unchanged.
Architecture
Validation
bun run typecheckbun run testbun run build./dist/archcode --version→archcode 0.0.8git diff --check origin/main...HEADSummary by CodeRabbit