feat: add Relay doctor diagnostics - #49
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds ChangesRelay doctor diagnostics
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant runRelay
participant runDoctorCommand
participant runDoctor
participant DoctorChecks
participant DoctorOutput
Operator->>runRelay: invoke doctor
runRelay->>runDoctorCommand: pass doctor arguments
runDoctorCommand->>runDoctor: execute ordered checks
runDoctor->>DoctorChecks: run runtime, asset, database, MCP, and UI checks
DoctorChecks-->>runDoctor: return diagnostic results
runDoctor-->>runDoctorCommand: return versioned report
runDoctorCommand->>DoctorOutput: write human or JSON report
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Review summary
The overall implementation is strong: the diagnostic contract is explicit, database checks avoid the migrating runtime, output is schema-versioned and sanitized, installed-tarball coverage is substantial, and MCP/UI probes use isolated databases.
I found one High / merge-blocking lifecycle issue.
High: Ctrl+C / SIGTERM does not terminate relay doctor
installDoctorSignalHandlers() installs handlers that only call void cleanupDoctorChildren(). Installing the handler suppresses Node's default signal termination, but the handler neither awaits cleanup nor aborts runDoctor(). Consequently, after SIGINT/SIGTERM the command can continue executing the current check, proceed to later checks, start new MCP/UI children, and eventually print a normal report. This does not satisfy the issue's requirement to clean up on signal and is surprising operational behavior for Ctrl+C.
There is a related cleanup gap: temporary-root cleanup is performed in each check's finally, but roots are not registered with the signal cleanup coordinator. A correct immediate-exit implementation therefore also needs to ensure those cleanups are awaited before termination.
Please introduce a coordinated abort path that:
- records that termination was requested and prevents subsequent checks from starting;
- awaits registered child and temporary-resource cleanup;
- terminates with conventional signal semantics or an explicit non-zero interrupted exit code (for example 130 for SIGINT and 143 for SIGTERM);
- does not emit a successful/complete doctor report after interruption;
- has a process-level test that spawns installed/built
relay doctor, sends SIGINT and SIGTERM during a probe, and verifies timely command exit, descendant termination, and temporary-root removal.
Once this is fixed, I would re-review the updated head.
Luna remediation planThe execution-ready Superpowers remediation plan is committed directly to this PR branch:
Commit: The plan locks the design so Luna does not invent lifecycle behavior:
Luna should execute the plan task-by-task using |
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Re-review result
The previous High / merge-blocking signal-handling finding is resolved.
Verified on the updated head:
runDoctorCommand()owns a command-scopedAbortControllerand returns130for SIGINT /143for SIGTERM.- interrupted runs suppress completed human and JSON reports;
runDoctor()checks cancellation before and after each check, so no later check starts after interruption;- the first signal wins and starts one coordinated cleanup workflow;
- child termination and temporary-root cleanup are registered through once-only, race-safe cleanup entries;
- MCP and UI temporary roots are registered immediately after creation;
- cleanup is awaited before command completion and signal handlers are removed in
finally; - command-level tests cover exit codes, awaited cleanup, output suppression, and handler removal;
- installed-package smoke coverage verifies SIGINT and SIGTERM, descendant termination, root removal, prevention of UI startup, unchanged configured database/client/ownership state, and sanitized output.
I found no remaining merge-blocking correctness issue in the remediation. The original review thread has been resolved. GitHub does not permit approving a pull request owned by the authenticated account, so this is recorded as a clean COMMENT re-review.
Luna CI remediation planI added a narrow execution-ready Superpowers plan to the PR branch:
Commit: The plan locks the root cause and fix so Luna does not change production behavior:
Luna should execute the plan with |
|
I added an execution-ready dependency audit remediation plan for Luna:
Plan commit: Locked policy:
Luna should execute the plan using |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/package/stage-package-assets.ts (1)
28-34: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate compatibility metadata during staging.
stagePackageAssetsonly requiresassets/compatibility.jsonto exist, so truncated manifests can pass packaging and only fail later ascompatibility.assets.invalid. Validate the compatibility manifest fields and relationship invariants during package staging, includingschemaVersion,minimumPackageVersion, contract/schema version fields, migration count against staged SQL migrations, skill metadata version withSKILL.md, and template metadata. Add a staging failure test for an unsupported or inconsistent compatibility manifest.🤖 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/package/stage-package-assets.ts` around lines 28 - 34, Extend stagePackageAssets to parse and validate assets/compatibility.json rather than only checking existence. Validate schemaVersion, minimumPackageVersion, contract/schema version fields, migration count against staged SQL migrations, skill metadata against SKILL.md, and template metadata using the existing compatibility validation rules; reject unsupported or inconsistent manifests during staging. Add a staging failure test covering an invalid compatibility manifest.
🧹 Nitpick comments (24)
tests/integration/packaged-assets.test.ts (1)
23-24: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd the missing-file failure test.
This change covers only the successful staging path. It does not verify that
stagePackageAssetsrejects a package root withoutassets/compatibility.json. Add a separate test that omits the file and asserts thePackage asset is missing after builderror.As per coding guidelines:
**/*.{test.ts,test.tsx}: Cover normal flows, validation failures, and lifecycle boundaries in the layer where they belong.🤖 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 `@tests/integration/packaged-assets.test.ts` around lines 23 - 24, Add a separate integration test for stagePackageAssets that creates a package root with the assets directory but omits compatibility.json, then assert it rejects with the exact “Package asset is missing after build” error while preserving the existing successful staging test.Source: Coding guidelines
tests/unit/distribution/doctor/check-integrations.test.ts (2)
103-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRun the check once and reuse the result.
generic.run()executes twice. Capture the result, then assert both the shape and the absence of template content.♻️ Proposed change
- await expect(generic.run()).resolves.toMatchObject({ + const result = await generic.run(); + expect(result).toMatchObject({ status: 'failure', code: 'integrations.generic-mcp.template-invalid', }); - const result = await generic.run(); expect(JSON.stringify(result)).not.toContain('secret');🤖 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 `@tests/unit/distribution/doctor/check-integrations.test.ts` around lines 103 - 108, Update the test around generic.run() to execute it once, store the returned result, and reuse that result for both the status/code assertions and the JSON secret-content assertion.
55-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the remaining native-client branches.
createNativeClientCheckhas five outcomes. The tests covernot-configuredandentry-conflict. Three remain untested:
ownership-invalid, whenownershipStore.read()rejects.disabled, when every owned record hasstatus !== 'enabled'.file-unreadable, whenaccessorreadFilerejects.The
accessstub at Line 79 always resolves, so the read-failure branch never runs.The coding guidelines require tests to cover validation failures and lifecycle boundaries in the layer where they belong.
🤖 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 `@tests/unit/distribution/doctor/check-integrations.test.ts` around lines 55 - 86, Add unit-test cases covering the remaining createNativeClientCheck outcomes: ownership-invalid when ownershipStore.read() rejects, disabled when all owned records are not enabled, and file-unreadable when access or readFile rejects. Extend the existing createIntegrationChecks fixtures and assertions to verify each failure code and message, and configure access/readFile rejection so the unreadable-file branch is exercised.Source: Coding guidelines
src/database/migration.ts (1)
57-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccumulate
MigrationManifestEntryinstead of placeholderMigrationFilevalues.
loadMigrationManifestbuildsMigrationFileobjects withsql: ''andchecksum: '', then strips both fields in a final.map(). The empty checksum is a valid-looking value that never matches a real ledger row. If a later change returnsfilesdirectly, the empty checksum reachesinspectDatabaseReadOnlyand marks every applied migration as unknown.Type the accumulator as
MigrationManifestEntry[]. The placeholders and the projection then disappear.♻️ Proposed refactor
export function loadMigrationManifest(migrationsDir: string): readonly MigrationManifestEntry[] { const entries = readdirSync(migrationsDir, { withFileTypes: true }); - const files: MigrationFile[] = []; + const files: MigrationManifestEntry[] = [];- files.push({ version, name, filename: entry.name, sql: '', checksum: '' }); + files.push({ version, name, filename: entry.name }); } - return files - .sort((a, b) => a.version - b.version) - .map(({ version, name, filename }) => ({ version, name, filename })); + return files.sort((a, b) => a.version - b.version); }🤖 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/database/migration.ts` around lines 57 - 62, Update loadMigrationManifest to accumulate MigrationManifestEntry objects rather than MigrationFile placeholders with empty sql and checksum values. Type the files accumulator as MigrationManifestEntry[], remove the final projection that strips placeholder fields, and retain the existing version sorting and manifest entry data.src/distribution/doctor/doctor-types.ts (1)
54-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnforce exhaustiveness between
DoctorCheckIdandDOCTOR_CHECK_ORDER.
satisfies readonly DoctorCheckId[]proves each entry is a valid id. It does not prove that every id appears, and it does not prevent duplicates. If a later change adds an id to the union but not to the array, the check silently never runs, andassertCheckOrderstill passes because it compares against the array itself.Add a compile-time exhaustiveness assertion.
♻️ Proposed compile-time exhaustiveness guard
] as const satisfies readonly DoctorCheckId[]; + +// Fails to compile if an id is missing from DOCTOR_CHECK_ORDER. +type MissingCheckId = Exclude<DoctorCheckId, (typeof DOCTOR_CHECK_ORDER)[number]>; +const _exhaustiveCheckOrder: MissingCheckId extends never ? true : never = true; +void _exhaustiveCheckOrder;🤖 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/distribution/doctor/doctor-types.ts` around lines 54 - 69, Add a compile-time exhaustiveness assertion adjacent to DOCTOR_CHECK_ORDER that verifies the array contains every DoctorCheckId, while preserving the existing valid-id constraint. Ensure the assertion also detects duplicate entries so newly added or repeated check IDs cannot pass compilation unnoticed.tests/unit/distribution/doctor/check-database.test.ts (2)
97-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test asserts its own stub value, not production behavior.
openProbeis supplied by the caller. The stub at Line 101 setsopenedPath = ':memory:', and Line 108 asserts that same literal.createNativeAddonChecknever selects the probe path, so the assertion cannot fail and the test title is not verified.The in-memory isolation is decided in
src/interfaces/production-dependencies.ts. Assert it there. Keep this test focused on the healthy result and onclose()being called.💚 Proposed replacement assertion
it('loads the native addon through an isolated in-memory probe', async () => { - let openedPath: string | undefined; + let closed = false; const result = await createNativeAddonCheck({ openProbe: () => { - openedPath = ':memory:'; - return { close: () => undefined } as never; + return { + close: () => { + closed = true; + }, + } as never; }, nodeAbi: '137', packageVersion: '13.0.1', }).run(); expect(result.status).toBe('healthy'); - expect(openedPath).toBe(':memory:'); + expect(closed).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/distribution/doctor/check-database.test.ts` around lines 97 - 109, Update the test around createNativeAddonCheck to stop asserting the caller-controlled ':memory:' stub value. Focus it on the healthy result and verify the probe handle’s close() callback is invoked; move coverage of the in-memory probe selection to production-dependencies.ts.
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the non-creation assertion out of
finally.Line 25 asserts inside
finally. If the assertion at Line 23 fails, this assertion runs and can throw first, which replaces the original failure message. Cleanup and assertion also belong in separate blocks.Assert before the
finally, and useexistsSyncto state the intent directly.💚 Proposed test restructure
expect(result).toMatchObject({ status: 'warning', code: 'database.missing' }); + expect(existsSync(databasePath)).toBe(false); + expect(existsSync(join(root, 'data'))).toBe(false); } finally { - expect(() => new Database(databasePath)).toThrow(); rmSync(root, { recursive: true, force: true }); }Add
existsSyncto thenode:fsimport.🤖 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 `@tests/unit/distribution/doctor/check-database.test.ts` around lines 24 - 27, Move the Database non-creation assertion out of the finally block and perform it before cleanup, using existsSync(databasePath) to assert that the database file was not created. Add existsSync to the node:fs imports, and leave finally responsible only for removing root.tests/unit/distribution/doctor/run-doctor.test.ts (1)
59-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for result sanitization.
This test covers the thrown-error path. Two validation paths in
run-doctor.tsremain untested:
sanitizeResultrejects an invalidstatus, or a non-stringcodeormessage.sanitizeDetailsdrops keys that do not match/^[a-z][a-zA-Z0-9]*$/and sorts the remaining keys.Add cases for both. The key filter is the part of the contract that keeps unexpected fields out of the JSON report, so a regression there is silent.
The coding guidelines require tests to cover validation failures in the layer where they belong.
🤖 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 `@tests/unit/distribution/doctor/run-doctor.test.ts` around lines 59 - 89, Add unit-test cases in run-doctor coverage for checks returning invalid status, non-string code, or non-string message, asserting they become sanitized internal-error results. Add a separate case with details containing valid and invalid keys, asserting sanitizeDetails removes keys failing /^[a-z][a-zA-Z0-9]*$/ and sorts the retained keys in the report. Keep these tests focused on runDoctor’s result-sanitization paths.Source: Coding guidelines
src/distribution/doctor/check-integrations.ts (1)
88-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBroad catch blocks map several distinct causes to one stable check code. Both integration checks wrap filesystem access, reading, and content validation in a single
try, so the reported code cannot tell the user which step failed. Doctor codes are part of the documented, stable output contract, so each cause needs its own code.
src/distribution/doctor/check-integrations.ts#L88-L111: return the invalid-shape result directly instead of throwingnew Error('invalid template'), and narrow the catch to theaccess,readFile, andJSON.parsefailures under a separate code such asintegrations.generic-mcp.template-unreadable.src/distribution/doctor/check-integrations.ts#L58-L77: moveadapter.parse(content)out of the readtry, and report a parse failure under a separate code such asintegrations.${client}.config-unparsable.If you add codes, document them in
docs/doctor.mdalongside the existing ones.🤖 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/distribution/doctor/check-integrations.ts` around lines 88 - 111, In src/distribution/doctor/check-integrations.ts:88-111, update createGenericCheck so invalid isGenericRelayEntry results return the invalid-shape failure directly, while access, readFile, and JSON.parse failures are caught separately with integrations.generic-mcp.template-unreadable; in src/distribution/doctor/check-integrations.ts:58-77, move adapter.parse(content) outside the read try block and report failures with integrations.${client}.config-unparsable. Document both new stable codes in docs/doctor.md.src/distribution/doctor/run-doctor.ts (1)
21-44: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCancel doctor checks when
input.signalarrives.
installDoctorSignalHandlers()aborts the doctor controller onSIGINT/SIGTERM, andrunDoctor()checks the signal between checks. However, eachcheck.run()is awaited without a signal, somcp.handshakecan still run for its 5s timeout andui.loopbackcan still run for its UI timeout while interruption is pending.Pass the signal into
DoctorCheck.run()so checks can route it to child-process probes, MCP connections, and HTTP fetches duringrunDoctor().🤖 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/distribution/doctor/run-doctor.ts` around lines 21 - 44, Update the DoctorCheck.run contract and the runDoctor loop to pass input.signal into every check execution. Propagate that signal through each check implementation to child-process probes, MCP connections, and HTTP fetches so in-flight checks cancel promptly, while preserving the existing interruption handling and result collection behavior.tests/unit/interfaces/cli/doctor-command.test.ts (1)
100-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest the warning-only exit path.
The test name states that warning-only reports return
0, but this fixture creates only healthy and failure results. Add a warning result and assert thatrunDoctorCommand()returns0. This locks the documented exit contract against future changes.As per coding guidelines, cover normal flows, validation failures, and lifecycle boundaries in the layer where they belong.
🤖 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 `@tests/unit/interfaces/cli/doctor-command.test.ts` around lines 100 - 117, Add a warning-status check fixture in the doctor command test and assert that runDoctorCommand() returns 0 for warning-only results, while preserving the existing validation-error and failure assertions. Use the existing createChecks dependency setup and DOCTOR_CHECK_ORDER symbols.Source: Coding guidelines
tests/unit/distribution/doctor/check-compatibility.test.ts (1)
56-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the version and migration-count branches.
The malformed-manifest test fails at
isManifest, so it returns before the comparison block at check-compatibility.ts lines 39-47. Those comparisons carry the compatibility semantics, and no test reaches them.Add two cases: an
applicationVersionbelowminimumPackageVersion, and amigrationCountthat disagrees with the migration directory.As per coding guidelines: "Cover normal flows, validation failures, and lifecycle boundaries in the layer where they belong."💚 Proposed tests
it('fails a malformed or newer compatibility manifest safely', async () => {it('fails when the installed version is below the minimum', async () => { const root = fixture(); try { await expect( createCompatibilityCheck({ applicationVersion: '0.0.9', migrationsDir: join(root, 'assets', 'migrations'), skillsDir: join(root, 'skills'), integrationsDir: join(root, 'integrations'), }).run(), ).resolves.toMatchObject({ status: 'failure', code: 'compatibility.assets.invalid' }); } finally { rmSync(root, { recursive: true, force: true }); } }); it('fails when the manifest migration count disagrees with the packaged migrations', async () => { const root = fixture(); try { writeFileSync( join(root, 'assets', 'migrations', '0002_extra.sql'), 'CREATE TABLE extra (id INTEGER);', ); await expect( createCompatibilityCheck({ applicationVersion: '0.1.0', migrationsDir: join(root, 'assets', 'migrations'), skillsDir: join(root, 'skills'), integrationsDir: join(root, 'integrations'), }).run(), ).resolves.toMatchObject({ status: 'failure', code: 'compatibility.assets.invalid' }); } finally { rmSync(root, { recursive: true, force: true }); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/distribution/doctor/check-compatibility.test.ts` around lines 56 - 74, Extend the compatibility-check tests with separate cases for an applicationVersion below minimumPackageVersion and a migrationCount mismatch caused by adding an extra migration file under migrationsDir. Run createCompatibilityCheck(...).run() in each case and assert compatibility.assets.invalid, preserving fixture cleanup with the existing try/finally pattern.Source: Coding guidelines
src/distribution/doctor/check-compatibility.ts (1)
27-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass the compatibility manifest path explicitly.
The check derives the manifest path with
dirname(input.migrationsDir). This encodes theassets/migrationslayout ofresolvePackageAssets(src/distribution/package-assets.ts, lines 33-43) as an implicit contract. If the asset layout changes, this check fails withcompatibility.assets.invalidinstead of a build error.Add an explicit input field and pass it from
createDoctorDependencies.♻️ Proposed change
export function createCompatibilityCheck(input: { readonly applicationVersion: string; + readonly compatibilityManifestPath: string; readonly migrationsDir: string; readonly skillsDir: string; readonly integrationsDir: string; }): DoctorCheck { @@ - const manifest = JSON.parse( - readFileSync(join(dirname(input.migrationsDir), 'compatibility.json'), 'utf8'), - ) as CompatibilityManifest; + const manifest = JSON.parse( + readFileSync(input.compatibilityManifestPath, 'utf8'), + ) as CompatibilityManifest;🤖 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/distribution/doctor/check-compatibility.ts` around lines 27 - 31, Update the doctor compatibility check input to include an explicit compatibility manifest path, and use that field when reading the manifest instead of deriving it from input.migrationsDir. Update createDoctorDependencies to provide the resolved manifest path while preserving manifest validation and migration loading.tests/unit/distribution/doctor/check-package-assets.test.ts (1)
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuild the asset fixtures with host-native paths.
The fixture hardcodes POSIX separators. The verdicts still hold on Windows, because every literal shares one root and
relativeproduces a relative result in both cases. The PR remediation plan for cross-platform tests states that fixtures must use host-nativeresolve()andjoin(). This file does not follow that convention.Align the fixture with the convention so later assertions on
detailspaths stay platform-correct.♻️ Proposed change
import { describe, expect, it } from 'vitest'; +import { join, resolve } from 'node:path'; import { createPackageAssetsCheck } from '../../../../src/distribution/doctor/check-package-assets.js'; @@ -const assets: PackageAssets = { - packageRoot: '/tmp/relay-package', - migrationsDir: '/tmp/relay-package/assets/migrations', - webRoot: '/tmp/relay-package/dist/web', - skillsDir: '/tmp/relay-package/skills', - integrationsDir: '/tmp/relay-package/integrations', -}; +const packageRoot = resolve('/tmp/relay-package'); +const assets: PackageAssets = { + packageRoot, + migrationsDir: join(packageRoot, 'assets', 'migrations'), + webRoot: join(packageRoot, 'dist', 'web'), + skillsDir: join(packageRoot, 'skills'), + integrationsDir: join(packageRoot, 'integrations'), +}; +const executablePath = join(packageRoot, 'dist', 'cli', 'main.js'); +const outsidePath = join(resolve('/tmp/outside'), 'web');Then replace the remaining literals with
executablePathandoutsidePath.🤖 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 `@tests/unit/distribution/doctor/check-package-assets.test.ts` around lines 7 - 13, Update the asset fixture around assets to build paths with host-native resolve() and join() rather than hardcoded POSIX separators, reusing the existing executablePath and outsidePath symbols for the remaining path literals so details assertions remain platform-correct.src/interfaces/mcp/main.ts (1)
10-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest instrumentation is embedded in the MCP production entry point.
This block ships in the published package and runs on every MCP start. The
RELAY_RUN_PACKAGE_SMOKE === '1'gate keeps it active in an installed package, outsideNODE_ENV=test.The equivalent doctor probes stay inside
src/distribution/doctor/check-mcp.tsandcheck-ui.ts. Move this readiness signal out of the delivery adapter. One option is a separate probe entry module that the signal tests spawn, which keepsrunMcpServerfree of test state.If the smoke test requires the marker from the real entry point, extract the block into a named helper such as
writeDoctorProbeMarker()in a dedicated module, and keeprunMcpServerlimited to one guarded call.🤖 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/interfaces/mcp/main.ts` around lines 10 - 16, Remove the inline test instrumentation from the MCP production entry point and relocate it to a dedicated probe module, following the existing doctor probe structure. Expose a named helper such as writeDoctorProbeMarker() that owns the environment checks and marker write, then keep runMcpServer limited to a single guarded helper call so production startup contains no embedded test state.src/distribution/doctor/check-mcp.ts (3)
49-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe stderr handler discards output and counts bytes that nothing reads.
stderrBytesis only written. The handler never stores the chunks, so no stderr is captured and the counter has no effect. Either drop the listener, or capture bounded stderr and use it. If you keep a bound, reuseDOCTOR_MAX_CAPTURE_BYTESfrom./child-process-probe.jsinstead of the literal32_768.♻️ Proposed removal of the dead counter
- let stderrBytes = 0; - transport.stderr?.on('data', (chunk: Buffer | string) => { - const remaining = Math.max(0, 32_768 - stderrBytes); - stderrBytes += Math.min(remaining, Buffer.byteLength(chunk)); - }); + transport.stderr?.resume();🤖 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/distribution/doctor/check-mcp.ts` around lines 49 - 53, Remove the unused stderr data listener and stderrBytes counter from the transport setup, since they neither capture nor consume output. If stderr is needed for diagnostics, instead capture it with a bounded buffer using DOCTOR_MAX_CAPTURE_BYTES from child-process-probe.js and use the captured value.
112-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
node:pathjoinfor the isolated database path.
joinDatabasePathbuilds the path with a hard-coded forward slash.src/distribution/doctor/check-ui.tsLine 41 builds the samerelay.dbpath withjoin(root.path, 'relay.db'). Use one mechanism for one concept.♻️ Proposed change
+import { join } from 'node:path';- env: { ...process.env, RELAY_DB_PATH: joinDatabasePath(root.path) }, + env: { ...process.env, RELAY_DB_PATH: join(root.path, 'relay.db') },-function joinDatabasePath(root: string): string { - return `${root.replace(/[\\/]$/, '')}/relay.db`; -} -🤖 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/distribution/doctor/check-mcp.ts` around lines 112 - 114, Update joinDatabasePath to use node:path’s join function with the root directory and “relay.db”, matching the existing path construction used by check-ui.ts. Remove the manual trailing-separator removal and hard-coded slash while preserving the same database filename.
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the exported timeout constant.
child-process-probe.tsexportsDOCTOR_MCP_TIMEOUT_MS = 5_000. The check duplicates the literal5_000twice.tests/unit/distribution/doctor/child-process-probe.test.tstreats these constants as locked values, so a future change to the constant will silently not apply here.♻️ Proposed change
-import { registerDoctorCleanup, registerDoctorTemporaryRoot } from './child-process-probe.js'; +import { + DOCTOR_MCP_TIMEOUT_MS, + registerDoctorCleanup, + registerDoctorTemporaryRoot, +} from './child-process-probe.js';- await withTimeout(client.connect(transport), 5_000); + await withTimeout(client.connect(transport), DOCTOR_MCP_TIMEOUT_MS);- const tools = (await withTimeout(client.listTools(), 5_000)).tools.map((tool) => tool.name); + const tools = (await withTimeout(client.listTools(), DOCTOR_MCP_TIMEOUT_MS)).tools.map( + (tool) => tool.name, + );Also applies to: 69-69
🤖 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/distribution/doctor/check-mcp.ts` at line 58, Update the timeout arguments in the MCP check flow around client.connect and the second duplicated timeout use to import and reuse the exported DOCTOR_MCP_TIMEOUT_MS constant from child-process-probe.ts instead of hardcoding 5_000.tests/fixtures/doctor/process/ui-ready-child.mjs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplain the split scheme literal.
${'http' + '://'}produceshttp://. The concatenation hides the literal from a scanner or lint rule, but the reason is not visible in the file. Add a one-line comment that states why the literal is split, so a later reader does not "simplify" it back and break the check.🤖 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 `@tests/fixtures/doctor/process/ui-ready-child.mjs` at line 1, Add a one-line comment immediately above the process.stderr.write call explaining that the HTTP scheme is intentionally split to avoid scanner or lint-rule detection; leave the existing output behavior unchanged.tests/unit/distribution/doctor/check-ui.test.ts (1)
25-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
ui.non-loopbackandui.health-invalidbranches.The two timeout tests are correct. The file does not cover two validation-failure branches of
createUiLoopbackCheck:
ui.non-loopbackatsrc/distribution/doctor/check-ui.tsLines 67-73. Add a fixture that reports a non-loopback readiness URL.ui.health-invalidat Lines 100-106. Return a well-formed JSON body with an unexpectednameorstatus.The coding guidelines require tests to cover validation failures in the layer where they belong.
Do you want me to generate these two tests?
As per coding guidelines: "Cover normal flows, validation failures, and lifecycle boundaries in the layer where they belong."
🤖 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 `@tests/unit/distribution/doctor/check-ui.test.ts` around lines 25 - 63, Add tests in the existing check-ui test suite for the two uncovered validation branches of createUiLoopbackCheck: use a readiness fixture reporting a non-loopback URL and assert code ui.non-loopback, then mock a successful health response with valid JSON containing an unexpected name or status and assert code ui.health-invalid. Reuse the existing command, temporaryRootFactory, and run setup from the timeout tests.Source: Coding guidelines
tests/unit/distribution/doctor/child-process-probe.test.ts (1)
152-153: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the truncated content, not only its length.
toBeLessThanOrEqual(4)also passes whenstdoutis empty. An empty capture would hide a regression in thecapturehelper atsrc/distribution/doctor/child-process-probe.tsLines 79-83.hanging-child.mjswriteshanging-output, so the bounded result is deterministic.💚 Proposed fix
expect(result.timedOut).toBe(true); - expect(result.stdout.length).toBeLessThanOrEqual(4); + expect(result.stdout).toBe('hang');🤖 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 `@tests/unit/distribution/doctor/child-process-probe.test.ts` around lines 152 - 153, Update the timeout assertion in the child-process probe test to verify the expected truncated stdout content produced by hanging-child.mjs, rather than only checking stdout.length. Keep the timedOut assertion and ensure the expectation would fail if capture returned an empty string.src/distribution/doctor/check-ui.ts (1)
125-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTerminate only this check's child instead of calling the global cleanup.
cleanupDoctorChildren()runs every registered cleanup in the process, including children and temporary roots owned by other checks.src/distribution/doctor/check-mcp.tsLines 94-100 does the opposite: it closes only its own transport and root.No present bug exists, because
createChecks()insrc/interfaces/production-dependencies.tsorders the MCP check before the UI check and each check cleans its own root. The coupling breaks if check execution ever overlaps. Expose the child fromrunChildProcessProbeand terminate that child 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 `@src/distribution/doctor/check-ui.ts` around lines 125 - 129, Update the UI check’s cleanup flow around runChildProcessProbe to expose and retain this check’s child process, then terminate only that child in the finally block instead of calling the global cleanupDoctorChildren(). Preserve the existing probe error handling and registeredRoot.cleanup() behavior, following the per-check cleanup pattern used by the MCP check.src/distribution/doctor/child-process-probe.ts (2)
219-261: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueWindows branch runs
taskkilleven after the child exited.Line 242 computes
exited, but the win32 branch at Line 243 ignores it. If the child already exited after SIGTERM, the code still spawnstaskkill. Return early whenexitedis true.♻️ Proposed early return
const exited = await waitForExit(child, 500); + if (exited) return; if (process.platform === 'win32' && pid !== undefined) { await taskkill(pid); await waitForExit(child, 500); return; } - if (exited) return;🤖 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/distribution/doctor/child-process-probe.ts` around lines 219 - 261, Update terminateChildInternal so the Windows-specific taskkill branch checks exited and returns immediately when waitForExit confirms the child has exited. Only invoke taskkill and the subsequent waitForExit when exited is false; preserve the existing behavior for non-Windows platforms.
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
activeChildrenregistry.
activeChildrenis only declared, added to, and deleted from.runChildProcessProbe()usesregisterDoctorCleanup()for termination, andactiveCleanupsbacks the cleanup paths that run on exit or abort. RemoveactiveChildrenand its five mutation sites.Also applies to: 71, 96, 107, 111, 134
🤖 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/distribution/doctor/child-process-probe.ts` at line 39, Remove the unused activeChildren registry from child-process probe logic, including its declaration and all five add/delete mutation sites near runChildProcessProbe. Preserve registerDoctorCleanup() and activeCleanups cleanup behavior unchanged.
🤖 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 `@docs/superpowers/plans/2026-08-06-pr-49-dependency-audit-remediation.md`:
- Around line 79-88: Update Step 3’s pnpm audit baseline capture instructions to
resolve the conflict with the document-wide || true prohibition: either
explicitly scope that prohibition to release-gate commands or describe an
alternative method that captures the expected non-zero audit result without
relying on || true. Preserve the requirement to write the JSON baseline to
.artifacts/audit/before.json.
In `@scripts/package/smoke-installed-package.ts`:
- Around line 200-203: Update verifyInstalledDoctorSignals so the
RELAY_DOCTOR_TEST_CHILD_MARKER PID is captured and cleaned up on the timeout and
finally paths, not only after waitForDoctorExit resolves. Ensure every failure
kills and awaits waitForProcessExit(childPid) before caseRoot is removed, while
preserving the existing successful exit flow.
In `@scripts/validate-repository-assets.ts`:
- Around line 193-198: Update validateDoctorFixtures so its disallowed-path
validation rejects UNC paths and absolute POSIX paths such as /tmp, while
preserving drive-letter and existing sensitive-root detection. Prefer path-aware
checks using path.posix and path.win32, or expand the matcher accordingly, and
add regression fixtures covering these cross-platform absolute path forms.
In `@src/distribution/doctor/check-database.ts`:
- Around line 34-36: Update the database inspection flow around the
migration-ledger query to first verify whether _relay_migrations exists. When
the database file exists but the ledger table is absent, return exists: true
with appliedMigrations empty and pendingMigrations containing all available
migrations; preserve the existing query and error handling for databases where
the ledger table is present.
In `@src/distribution/doctor/check-paths.ts`:
- Around line 103-111: Replace the boolean result from directoryState with an
observed directory state that distinguishes absence, readability, writability,
and whether the path is a directory. Update each caller in the required-root,
cache, and database-parent checks to use those fields when populating exists,
readable, and writable, preserving the existing diagnostics while reporting
actual permissions and directory status.
In `@src/distribution/doctor/check-ui.ts`:
- Line 34: Update the UI child startup flow around findFreePort so
RELAY_HTTP_PORT is set to 0 rather than using a preallocated port. Remove the
findFreePort call and preserve the existing stderr parsing that reads the UI’s
actual bound URL.
In `@src/interfaces/production-dependencies.ts`:
- Line 78: Update the argument passed to resolvePackageAssets so pathToFileURL
is applied only to process.argv[1] when present, while the undefined fallback
uses import.meta.url directly. Preserve resolvePackageAssets’s support for both
filesystem paths converted to file URLs and the existing file URL fallback.
In `@tests/unit/distribution/doctor/check-mcp.test.ts`:
- Around line 21-26: The spawn-failure fixtures use a Windows-only temporary
root, masking the intended missing-command failure on other hosts. In
tests/unit/distribution/doctor/check-mcp.test.ts lines 21-26, update the
temporaryRootFactory fixture to use host-native join(tmpdir(),
'relay-doctor-mcp-test') and create that directory before the run; in
tests/unit/distribution/doctor/check-ui.test.ts lines 12-17, replace the Windows
path with process.cwd(), matching the other tests in that file.
In `@tests/unit/distribution/doctor/check-paths.test.ts`:
- Line 54: Update the assertion for accessed paths to use a content-aware array
matcher, such as checking that no element satisfies the probe substring
condition, instead of passing expect.stringContaining to toContain. Ensure the
test fails when accessed contains a path including “probe”.
In `@tests/unit/distribution/doctor/child-process-probe.test.ts`:
- Around line 172-184: Validate childPid immediately after parsing result.stdout
in the “terminates a spawned grandchild with the timed-out parent” test,
asserting it is a positive integer before calling waitForProcessExit. Keep the
existing timeout assertion and process-exit verification unchanged.
In `@tests/unit/scripts/mcpb/stage-linux-mcpb.test.ts`:
- Around line 40-41: The staged lockfile fixture in the test setup must include
both declared overrides: the existing tmp override and brace-expansion@>=4.0.0
<5.0.9. Regenerate or update the fixture lockfile using the repository’s pnpm
version so it matches the source manifest before stageLinuxMcpb runs frozen
installation.
---
Outside diff comments:
In `@scripts/package/stage-package-assets.ts`:
- Around line 28-34: Extend stagePackageAssets to parse and validate
assets/compatibility.json rather than only checking existence. Validate
schemaVersion, minimumPackageVersion, contract/schema version fields, migration
count against staged SQL migrations, skill metadata against SKILL.md, and
template metadata using the existing compatibility validation rules; reject
unsupported or inconsistent manifests during staging. Add a staging failure test
covering an invalid compatibility manifest.
---
Nitpick comments:
In `@src/database/migration.ts`:
- Around line 57-62: Update loadMigrationManifest to accumulate
MigrationManifestEntry objects rather than MigrationFile placeholders with empty
sql and checksum values. Type the files accumulator as MigrationManifestEntry[],
remove the final projection that strips placeholder fields, and retain the
existing version sorting and manifest entry data.
In `@src/distribution/doctor/check-compatibility.ts`:
- Around line 27-31: Update the doctor compatibility check input to include an
explicit compatibility manifest path, and use that field when reading the
manifest instead of deriving it from input.migrationsDir. Update
createDoctorDependencies to provide the resolved manifest path while preserving
manifest validation and migration loading.
In `@src/distribution/doctor/check-integrations.ts`:
- Around line 88-111: In src/distribution/doctor/check-integrations.ts:88-111,
update createGenericCheck so invalid isGenericRelayEntry results return the
invalid-shape failure directly, while access, readFile, and JSON.parse failures
are caught separately with integrations.generic-mcp.template-unreadable; in
src/distribution/doctor/check-integrations.ts:58-77, move adapter.parse(content)
outside the read try block and report failures with
integrations.${client}.config-unparsable. Document both new stable codes in
docs/doctor.md.
In `@src/distribution/doctor/check-mcp.ts`:
- Around line 49-53: Remove the unused stderr data listener and stderrBytes
counter from the transport setup, since they neither capture nor consume output.
If stderr is needed for diagnostics, instead capture it with a bounded buffer
using DOCTOR_MAX_CAPTURE_BYTES from child-process-probe.js and use the captured
value.
- Around line 112-114: Update joinDatabasePath to use node:path’s join function
with the root directory and “relay.db”, matching the existing path construction
used by check-ui.ts. Remove the manual trailing-separator removal and hard-coded
slash while preserving the same database filename.
- Line 58: Update the timeout arguments in the MCP check flow around
client.connect and the second duplicated timeout use to import and reuse the
exported DOCTOR_MCP_TIMEOUT_MS constant from child-process-probe.ts instead of
hardcoding 5_000.
In `@src/distribution/doctor/check-ui.ts`:
- Around line 125-129: Update the UI check’s cleanup flow around
runChildProcessProbe to expose and retain this check’s child process, then
terminate only that child in the finally block instead of calling the global
cleanupDoctorChildren(). Preserve the existing probe error handling and
registeredRoot.cleanup() behavior, following the per-check cleanup pattern used
by the MCP check.
In `@src/distribution/doctor/child-process-probe.ts`:
- Around line 219-261: Update terminateChildInternal so the Windows-specific
taskkill branch checks exited and returns immediately when waitForExit confirms
the child has exited. Only invoke taskkill and the subsequent waitForExit when
exited is false; preserve the existing behavior for non-Windows platforms.
- Line 39: Remove the unused activeChildren registry from child-process probe
logic, including its declaration and all five add/delete mutation sites near
runChildProcessProbe. Preserve registerDoctorCleanup() and activeCleanups
cleanup behavior unchanged.
In `@src/distribution/doctor/doctor-types.ts`:
- Around line 54-69: Add a compile-time exhaustiveness assertion adjacent to
DOCTOR_CHECK_ORDER that verifies the array contains every DoctorCheckId, while
preserving the existing valid-id constraint. Ensure the assertion also detects
duplicate entries so newly added or repeated check IDs cannot pass compilation
unnoticed.
In `@src/distribution/doctor/run-doctor.ts`:
- Around line 21-44: Update the DoctorCheck.run contract and the runDoctor loop
to pass input.signal into every check execution. Propagate that signal through
each check implementation to child-process probes, MCP connections, and HTTP
fetches so in-flight checks cancel promptly, while preserving the existing
interruption handling and result collection behavior.
In `@src/interfaces/mcp/main.ts`:
- Around line 10-16: Remove the inline test instrumentation from the MCP
production entry point and relocate it to a dedicated probe module, following
the existing doctor probe structure. Expose a named helper such as
writeDoctorProbeMarker() that owns the environment checks and marker write, then
keep runMcpServer limited to a single guarded helper call so production startup
contains no embedded test state.
In `@tests/fixtures/doctor/process/ui-ready-child.mjs`:
- Line 1: Add a one-line comment immediately above the process.stderr.write call
explaining that the HTTP scheme is intentionally split to avoid scanner or
lint-rule detection; leave the existing output behavior unchanged.
In `@tests/integration/packaged-assets.test.ts`:
- Around line 23-24: Add a separate integration test for stagePackageAssets that
creates a package root with the assets directory but omits compatibility.json,
then assert it rejects with the exact “Package asset is missing after build”
error while preserving the existing successful staging test.
In `@tests/unit/distribution/doctor/check-compatibility.test.ts`:
- Around line 56-74: Extend the compatibility-check tests with separate cases
for an applicationVersion below minimumPackageVersion and a migrationCount
mismatch caused by adding an extra migration file under migrationsDir. Run
createCompatibilityCheck(...).run() in each case and assert
compatibility.assets.invalid, preserving fixture cleanup with the existing
try/finally pattern.
In `@tests/unit/distribution/doctor/check-database.test.ts`:
- Around line 97-109: Update the test around createNativeAddonCheck to stop
asserting the caller-controlled ':memory:' stub value. Focus it on the healthy
result and verify the probe handle’s close() callback is invoked; move coverage
of the in-memory probe selection to production-dependencies.ts.
- Around line 24-27: Move the Database non-creation assertion out of the finally
block and perform it before cleanup, using existsSync(databasePath) to assert
that the database file was not created. Add existsSync to the node:fs imports,
and leave finally responsible only for removing root.
In `@tests/unit/distribution/doctor/check-integrations.test.ts`:
- Around line 103-108: Update the test around generic.run() to execute it once,
store the returned result, and reuse that result for both the status/code
assertions and the JSON secret-content assertion.
- Around line 55-86: Add unit-test cases covering the remaining
createNativeClientCheck outcomes: ownership-invalid when ownershipStore.read()
rejects, disabled when all owned records are not enabled, and file-unreadable
when access or readFile rejects. Extend the existing createIntegrationChecks
fixtures and assertions to verify each failure code and message, and configure
access/readFile rejection so the unreadable-file branch is exercised.
In `@tests/unit/distribution/doctor/check-package-assets.test.ts`:
- Around line 7-13: Update the asset fixture around assets to build paths with
host-native resolve() and join() rather than hardcoded POSIX separators, reusing
the existing executablePath and outsidePath symbols for the remaining path
literals so details assertions remain platform-correct.
In `@tests/unit/distribution/doctor/check-ui.test.ts`:
- Around line 25-63: Add tests in the existing check-ui test suite for the two
uncovered validation branches of createUiLoopbackCheck: use a readiness fixture
reporting a non-loopback URL and assert code ui.non-loopback, then mock a
successful health response with valid JSON containing an unexpected name or
status and assert code ui.health-invalid. Reuse the existing command,
temporaryRootFactory, and run setup from the timeout tests.
In `@tests/unit/distribution/doctor/child-process-probe.test.ts`:
- Around line 152-153: Update the timeout assertion in the child-process probe
test to verify the expected truncated stdout content produced by
hanging-child.mjs, rather than only checking stdout.length. Keep the timedOut
assertion and ensure the expectation would fail if capture returned an empty
string.
In `@tests/unit/distribution/doctor/run-doctor.test.ts`:
- Around line 59-89: Add unit-test cases in run-doctor coverage for checks
returning invalid status, non-string code, or non-string message, asserting they
become sanitized internal-error results. Add a separate case with details
containing valid and invalid keys, asserting sanitizeDetails removes keys
failing /^[a-z][a-zA-Z0-9]*$/ and sorts the retained keys in the report. Keep
these tests focused on runDoctor’s result-sanitization paths.
In `@tests/unit/interfaces/cli/doctor-command.test.ts`:
- Around line 100-117: Add a warning-status check fixture in the doctor command
test and assert that runDoctorCommand() returns 0 for warning-only results,
while preserving the existing validation-error and failure assertions. Use the
existing createChecks dependency setup and DOCTOR_CHECK_ORDER symbols.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa6cecc8-559f-4a27-b4f5-5d8faa175f7d
⛔ Files ignored due to path filters (2)
integrations/claude-desktop/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (59)
README.mdassets/compatibility.jsondocs/doctor.mddocs/setup-and-configuration.mddocs/superpowers/plans/2026-08-04-issue-42-relay-doctor-diagnostics.mddocs/superpowers/plans/2026-08-04-pr-49-doctor-signal-remediation.mddocs/superpowers/plans/2026-08-05-pr-49-cross-platform-path-test-remediation.mddocs/superpowers/plans/2026-08-06-pr-49-dependency-audit-remediation.mdintegrations/claude-desktop/package.jsonpackage.jsonscripts/package/package-files.tsscripts/package/smoke-installed-package.tsscripts/package/stage-package-assets.tsscripts/package/verify-package-metadata.tsscripts/validate-mcpb-assets.tsscripts/validate-repository-assets.tssrc/database/migration.tssrc/distribution/doctor/check-compatibility.tssrc/distribution/doctor/check-database.tssrc/distribution/doctor/check-integrations.tssrc/distribution/doctor/check-mcp.tssrc/distribution/doctor/check-package-assets.tssrc/distribution/doctor/check-paths.tssrc/distribution/doctor/check-runtime.tssrc/distribution/doctor/check-ui.tssrc/distribution/doctor/child-process-probe.tssrc/distribution/doctor/doctor-interruption.tssrc/distribution/doctor/doctor-types.tssrc/distribution/doctor/run-doctor.tssrc/interfaces/cli/doctor-output.tssrc/interfaces/cli/main.tssrc/interfaces/cli/parse-doctor-command.tssrc/interfaces/cli/run-doctor-command.tssrc/interfaces/cli/run-relay.tssrc/interfaces/mcp/main.tssrc/interfaces/production-dependencies.tstests/fixtures/doctor/README.mdtests/fixtures/doctor/process/hanging-child.mjstests/fixtures/doctor/process/healthy-child.mjstests/fixtures/doctor/process/spawn-grandchild.mjstests/fixtures/doctor/process/ui-ready-child.mjstests/integration/database-path-parity.test.tstests/integration/doctor-installed-package.test.tstests/integration/installed-package.test.tstests/integration/packaged-assets.test.tstests/unit/distribution/doctor/check-compatibility.test.tstests/unit/distribution/doctor/check-database.test.tstests/unit/distribution/doctor/check-integrations.test.tstests/unit/distribution/doctor/check-mcp.test.tstests/unit/distribution/doctor/check-package-assets.test.tstests/unit/distribution/doctor/check-paths.test.tstests/unit/distribution/doctor/check-runtime.test.tstests/unit/distribution/doctor/check-ui.test.tstests/unit/distribution/doctor/child-process-probe.test.tstests/unit/distribution/doctor/run-doctor.test.tstests/unit/interfaces/cli/doctor-command.test.tstests/unit/interfaces/cli/run-relay.test.tstests/unit/scripts/mcpb/model.test.tstests/unit/scripts/mcpb/stage-linux-mcpb.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/distribution/doctor/check-ui.ts`:
- Line 79: Update the fetch and response-body flow in the UI health check so the
abort listener remains registered through readHealthBody() and is removed only
after body processing completes. In the outer catch, detect an aborted signal
and rethrow signal.reason instead of converting cancellation into
ui.start-failed; preserve existing handling for non-cancellation errors.
In `@src/interfaces/http/create-http-server.ts`:
- Around line 80-82: Update the RELAY_HTTP_PORT validation around parsed so
envPort must match the complete non-negative decimal-integer format before
conversion; reject values with trailing characters, exponent notation, or
hexadecimal prefixes such as 123abc, 1e3, and 0x10, while preserving the
existing 0–65535 range validation.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 24663b7b-7f09-4a49-ba08-b003d8db6ec9
📒 Files selected for processing (35)
docs/doctor.mddocs/superpowers/plans/2026-08-06-pr-49-dependency-audit-remediation.mdscripts/package/smoke-installed-package.tsscripts/package/stage-package-assets.tsscripts/validate-repository-assets.tssrc/database/migration.tssrc/distribution/doctor/check-compatibility.tssrc/distribution/doctor/check-database.tssrc/distribution/doctor/check-integrations.tssrc/distribution/doctor/check-mcp.tssrc/distribution/doctor/check-paths.tssrc/distribution/doctor/check-ui.tssrc/distribution/doctor/child-process-probe.tssrc/distribution/doctor/doctor-types.tssrc/distribution/doctor/run-doctor.tssrc/interfaces/http/create-http-server.tssrc/interfaces/mcp/doctor-probe-marker.tssrc/interfaces/mcp/main.tssrc/interfaces/production-dependencies.tstests/fixtures/doctor/process/ui-non-loopback-child.mjstests/fixtures/doctor/process/ui-ready-child.mjstests/integration/packaged-assets.test.tstests/unit/distribution/doctor/check-compatibility.test.tstests/unit/distribution/doctor/check-database.test.tstests/unit/distribution/doctor/check-integrations.test.tstests/unit/distribution/doctor/check-mcp.test.tstests/unit/distribution/doctor/check-package-assets.test.tstests/unit/distribution/doctor/check-paths.test.tstests/unit/distribution/doctor/check-ui.test.tstests/unit/distribution/doctor/child-process-probe.test.tstests/unit/distribution/doctor/run-doctor.test.tstests/unit/interfaces/cli/doctor-command.test.tstests/unit/interfaces/http/create-http-server.test.tstests/unit/scripts/mcpb/stage-linux-mcpb.test.tstests/unit/scripts/validate-repository-assets.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- tests/unit/distribution/doctor/check-mcp.test.ts
- src/distribution/doctor/check-mcp.ts
- tests/unit/distribution/doctor/check-package-assets.test.ts
- docs/doctor.md
- src/distribution/doctor/run-doctor.ts
- src/distribution/doctor/check-database.ts
- tests/unit/distribution/doctor/child-process-probe.test.ts
- tests/unit/distribution/doctor/check-paths.test.ts
- src/distribution/doctor/check-integrations.ts
- src/interfaces/mcp/main.ts
- docs/superpowers/plans/2026-08-06-pr-49-dependency-audit-remediation.md
- scripts/validate-repository-assets.ts
- tests/unit/scripts/mcpb/stage-linux-mcpb.test.ts
- tests/fixtures/doctor/process/ui-ready-child.mjs
- src/distribution/doctor/check-paths.ts
|
@CodeRabbit review |
✅ Action performedReview finished.
|
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Fresh re-review after the large CodeRabbit remediation pass (1cebb25 + 57a0ad3). I re-checked the behavior-changing doctor code rather than relying on the prior review.
No merge-blocking correctness findings remain. In particular, the current head preserves the interruption contract through MCP/UI probes, keeps UI health cancellation active through response-body parsing, uses ephemeral port 0 without the prior allocation race, reports path access state accurately, handles databases without _relay_migrations as pre-migration state, validates compatibility assets during packaging, and retains the non-destructive/read-only boundaries. All CodeRabbit inline threads are resolved, and CI run 31120819744 is green across the verification gate and Linux MCPB staging/integration/validation/packaging steps.
Non-blocking cleanup: the PR description is now stale where it says six audit advisories remain and are unchanged. The dependency remediation is present (@modelcontextprotocol/sdk ^1.30.0 plus the scoped brace-expansion override) and the current pnpm audit --audit-level high gate passes, so that paragraph should be updated before merge to keep the PR record accurate.
1 participant
Summary
Implements issue #42 and the approved doctor diagnostics plan.
relay doctorandrelay doctor --output jsonwith schema-versioned output, fixed 14-check order, stable statuses/codes, and exit semantics.Review remediation
Fresh review findings were addressed for UI request and body hangs, chunk-fragile readiness parsing, MCP cleanup registration, native-addon startup isolation, missing-database false failures, usage parsing order, package-root escape handling, Windows process-tree cleanup ordering, safe bootstrap reporting for invalid paths/malformed package setup, coordinated signal abort/cleanup, retryable temporary-root cleanup, and process-level signal verification.
Validation
pnpm format:checkpnpm lintpnpm typecheck--testTimeout=15000: 77 files passed, 1 skipped; 701 tests passed, 5 skipped.RELAY_DB_PATHemits a sanitized schema-valid JSON failure report rather than a stack trace.The repository audit remains external-environment dependent: the sandbox registry request is blocked by EACCES; the network-enabled audit reports six existing transitive advisories in
fast-uri,ip-address, andbrace-expansion, which are not changed by this issue.Closes #42
Summary by CodeRabbit
New Features
relay doctorcommand with human-readable and JSON reports.Bug Fixes
0is now supported for automatic port assignment.Documentation