feat: add safe Relay setup and agent configuration - #48
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR adds safe installed setup for Codex and Claude Code. It introduces configuration planning, atomic writes, ownership metadata, transaction recovery, operational CLI commands, package validation, documentation, fixtures, and workflow tests. ChangesInstalled setup workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant RelayCLI
participant SetupPlanner
participant ClientConfig
participant OwnershipStore
Operator->>RelayCLI: relay setup --client --config-file
RelayCLI->>SetupPlanner: parse and plan change
SetupPlanner->>ClientConfig: inspect configuration
SetupPlanner->>OwnershipStore: read ownership metadata
RelayCLI->>ClientConfig: atomically apply reviewed change
RelayCLI->>OwnershipStore: persist locked ownership update
RelayCLI-->>Operator: return JSON result
🚥 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 verdict: changes required
The overall decomposition is good and the client-file safety path is substantially aligned with issue #41, but I found two blocking correctness problems.
High — ownership metadata has a lost-update race
applyIntegrationChange() reads the ownership file, merges its own record in memory, and calls ownershipStore.write(). The store then performs an unconditional atomic replacement. Two concurrent setup operations for different clients can both read the same old metadata and each write a valid replacement; the last writer silently deletes the other integration record while both client files retain Relay entries.
That leaves one configured entry stranded as an apparently unowned matching entry, so later setup/disable/remove fails closed and cannot manage it. This is especially plausible for Relay because multiple local agents/processes may invoke setup concurrently.
Please add optimistic concurrency or a short exclusive metadata lock around the read-modify-write transaction. A practical shape is readWithFingerprint() plus write(next, expectedFingerprint), with the fingerprint rechecked immediately before replacement and a conflict returned when it changed. Add a deterministic two-writer test proving records cannot be lost.
High — CI is failing on the supported Linux runner
The new parser test uses C:/tmp/codex.toml as the successful absolute-path case. node:path.isAbsolute() uses host semantics, so that path is relative on Linux. CI run 122 fails in operational-commands.test.ts with --config-file must be an absolute path; the gate reports 615 passed and 1 failed.
Use a host-native absolute path for this parser test, or explicitly test platform-specific path parsing through an injected path strategy. The production parser currently follows host semantics, which is appropriate for the running client; the test must not assume Windows while executing on Linux.
Medium — failed metadata persistence does not restore an originally absent file
For a new config path, backupAndAtomicWrite() represents the original as an empty buffer. If the client file is created successfully but metadata persistence then fails, restoreFile() restores the empty backup as an empty file. The pre-operation state was “file absent,” not “empty file present.” Track whether the target existed and delete the newly created target during rollback when it did not.
After these fixes, rerun pnpm verify on CI and add focused regression tests for metadata concurrency and absent-file rollback.
Luna remediation planI added an execution-ready Superpowers remediation plan directly to this PR branch:
Commit: Luna should execute it task-by-task with The plan locks the implementation decisions rather than leaving them to the agent:
Do not merge while the PR is draft or CI is red. The manual real-client restart/backup check remains the final human acceptance gate. |
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Re-review result: the three previously blocking findings are correctly addressed.
- Ownership metadata updates now use a cross-process exclusive lock around the complete read-modify-write cycle, and concurrent Codex/Claude regression coverage verifies both records survive.
- Rollback now distinguishes an originally absent target from an existing file, removes newly created files on metadata failure, restores existing bytes/mode, and retains backups.
- The parser test now uses a host-native absolute path, and CI run 124 passes.
One remaining documentation correctness issue should be fixed before merge:
Medium — docs/setup-and-configuration.md contradicts the implemented and locked CLI contract. The document lists relay config disable/remove ... without --apply and states that without --apply these commands return a preview. The parser intentionally requires --apply for disable/remove and returns exit code 2 when it is omitted; the authoritative issue #41 plan also locks this behavior. Update the command examples and prose so only setup is preview-first, while disable/remove require --apply and users inspect ownership through relay config integrations first.
After that documentation correction and a green CI rerun, I consider the PR merge-ready subject to the already documented manual real-client acceptance gate.
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Final re-review complete. The documentation now matches the accepted CLI contract: setup is preview-first, while config disable/remove require --apply and omission is documented as exit code 2. The three earlier correctness blockers remain resolved, and CI run 125 passed on the latest head. No further code or documentation issues found in this review. PR is merge-ready after the planned manual real-client acceptance check.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (10)
src/interfaces/production-dependencies.ts (1)
22-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the ownership metadata path into one helper.
join(runtimePaths.configRoot, 'config.json')appears here and twice insrc/interfaces/cli/run-operational-command.ts(the fallbackcreateOwnershipStorecall and theconfig pathsoutput). The three sites must agree. If one changes,relay config pathsreports a path that the ownership store does not use.Export a single
resolveOwnershipMetadataPath(runtimePaths)helper next toresolveRuntimePathsand call it from all three sites.🤖 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/production-dependencies.ts` around lines 22 - 25, Extract the shared ownership metadata path calculation into an exported resolveOwnershipMetadataPath(runtimePaths) helper beside resolveRuntimePaths, then replace the direct join(runtimePaths.configRoot, 'config.json') usage in production dependencies and both operational-command sites, including the fallback ownership store and config-paths output.scripts/validate-repository-assets.ts (1)
403-408: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis existence check is already performed.
validateRepositoryAssetsbuildsrequiredPathsfrom...requiredDistributionAssetsand fails on any missing entry. Lines 40-60 add every setup fixture torequiredDistributionAssets, so this loop re-checks paths that are already checked, with a second error message for the same failure. Remove the loop.♻️ Proposed fix
- - for (const path of requiredDistributionAssets.filter((asset) => - asset.startsWith('tests/fixtures/setup/'), - )) { - if (!existsSync(join(rootDir, path))) fail(`Setup fixture is missing: ${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 `@scripts/validate-repository-assets.ts` around lines 403 - 408, Remove the setup-fixture iteration that filters requiredDistributionAssets and calls existsSync/fail, since validateRepositoryAssets already validates every entry through requiredPaths. Keep the existing requiredDistributionAssets validation unchanged and eliminate only this duplicate check and its error path.src/distribution/setup/apply-integration-change.ts (1)
37-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated ownership literals risk drifting from the exact-entry contract.
entryId: 'relay',command: 'relay' as const, andargs: ['mcp'] as constare hardcoded here (andentryId: 'relay'is repeated at Line 21 and Line 90). The documentation states Relay proves ownership by matching "the exactrelayentry, therelaycommand,['mcp']arguments" against what the adapters actually write. If a client adapter or snippet template ever changes its generated command/args independently of this file, ownership records would silently mismatch the file content, weakening the exact-matching guarantee this PR is built around.Extract these values into a single shared constant (for example, exported from
snippets.tsorsetup-types.ts) and import it here and wherever adapters generate the entry, so the ownership record and the generated content cannot diverge.🤖 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/setup/apply-integration-change.ts` around lines 37 - 47, Extract the canonical Relay entry values—entryId, command, and args—into one shared exported constant, then update applyIntegrationChange and the adapter or snippet-generation code to reuse it everywhere, including the repeated entryId references. Remove the duplicated literals while preserving the existing exact values so ownership records and generated content remain synchronized.scripts/validate-agent-integration-assets.ts (2)
279-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant re-parse of the three template assets.
Line 278 calls
validateTemplateShape(rootDir). That function already reads and parsesgeneric-mcp/server-config.json.example,claude-code/.mcp.json.example, andcodex/config.toml.example. Lines 279-281 parse the same three files again and discard the results. The parse-validity check is already covered, so these lines add no assertion.♻️ Proposed removal
validateTemplateShape(rootDir); - JSON.parse(readFileSync(join(integrationRoot, 'generic-mcp/server-config.json.example'), 'utf8')); - JSON.parse(readFileSync(join(integrationRoot, 'claude-code/.mcp.json.example'), 'utf8')); - parse(readFileSync(join(integrationRoot, 'codex/config.toml.example'), 'utf8')); }🤖 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/validate-agent-integration-assets.ts` around lines 279 - 281, Remove the redundant JSON.parse and parse calls immediately after validateTemplateShape(rootDir) in the validation flow. Keep validateTemplateShape responsible for reading and parsing the three template assets, and leave the remaining validation behavior unchanged.
173-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract one shared check for the
command/argscontract.The same rule is now expressed three times: the JSON branch at lines 179-183 and the TOML branch at lines 197-204. The two branches also use different comparison styles for
args. The JSON branch usesJSON.stringifyequality. The TOML branch usesArray.isArrayplus length plus index. Both must stay in sync when the installed command changes.Line 200 also applies optional chaining to
codexServer?.commandafter line 198 already rejectsundefined. Remove the redundant?..♻️ Proposed helper extraction
+function invokesInstalledRelayMcp(server: Record<string, unknown>): boolean { + return ( + JSON.stringify(Object.keys(server).sort()) === JSON.stringify(['args', 'command']) && + server.command === 'relay' && + JSON.stringify(server.args) === JSON.stringify(['mcp']) + ); +}- if ( - JSON.stringify(Object.keys(server).sort()) !== JSON.stringify(['args', 'command']) || - server.command !== 'relay' || - JSON.stringify(server.args) !== JSON.stringify(['mcp']) - ) + if (!invokesInstalledRelayMcp(server as Record<string, unknown>)) fail(`${path} must use separate command and arguments for the installed relay mcp command.`);- if ( - codexServer === undefined || - JSON.stringify(Object.keys(codexServer).sort()) !== JSON.stringify(['args', 'command']) || - codexServer?.command !== 'relay' || - !Array.isArray(codexServer.args) || - codexServer.args.length !== 1 || - codexServer.args[0] !== 'mcp' - ) { + if (codexServer === undefined || !invokesInstalledRelayMcp(codexServer)) { fail('integrations/codex/config.toml.example must invoke the installed relay mcp command.'); }🤖 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/validate-agent-integration-assets.ts` around lines 173 - 206, Extract a shared helper near the validation logic to validate the installed MCP server command/args contract, requiring exactly the command and args keys, command “relay”, and args equivalent to [“mcp”]. Replace both the JSON server check and TOML codexServer check with this helper, using one consistent args comparison style, and change codexServer?.command to codexServer.command after its undefined guard.tests/unit/interfaces/cli/operational-commands.test.ts (1)
22-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd positive-path assertions for
config disableandconfig remove.The tests cover the failure case for
disablewithout--applyat line 27. No test asserts the parsed result for a validdisableorremovecommand. The action-to-kind mapping insrc/interfaces/cli/parse-operational-command.tslines 49-54 is therefore untested. A swap ofconfig-disableandconfig-removepasses this suite. The coding guidelines require coverage of normal flows in the layer where they belong.💚 Proposed additional assertions
it('parses snippet and inspection commands', () => { + expect( + parseOperationalCommand([ + 'config', + 'disable', + '--client', + 'codex', + '--config-file', + absoluteConfigPath, + '--apply', + ]), + ).toEqual({ + kind: 'config-disable', + client: 'codex', + configFile: absoluteConfigPath, + apply: true, + }); + expect( + parseOperationalCommand([ + 'config', + 'remove', + '--client', + 'claude-code', + '--config-file', + absoluteConfigPath, + '--apply', + ]), + ).toEqual({ + kind: 'config-remove', + client: 'claude-code', + configFile: absoluteConfigPath, + apply: true, + }); expect(parseOperationalCommand(['config', 'paths'])).toEqual({ kind: 'config-paths' });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/operational-commands.test.ts` around lines 22 - 50, Add positive-path assertions in the “parses snippet and inspection commands” test for valid `config disable` and `config remove` invocations, including their expected `config-disable` and `config-remove` kinds and parsed client/config-file values. Use the existing absoluteConfigPath fixture and include `--apply` so both action-to-kind mappings in parseOperationalCommand are directly verified.Source: Coding guidelines
src/interfaces/cli/operational-output.ts (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve diagnostics for unexpected internal errors.
Line 41 replaces the original error with a fixed message. No stack trace and no cause reach stdout, stderr, or any log. A user who hits an unexpected failure has no information to report. Emit the detail on stderr, or attach it behind a debug flag, while keeping the stdout envelope generic.
🤖 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/cli/operational-output.ts` at line 41, Update the unexpected internal-error handling in the operational output builder to keep the returned stdout envelope generic while emitting the original error details, including its cause or stack when available, to stderr or through the existing debug logging path. Preserve the INTERNAL_ERROR code and exitCode values.tests/unit/interfaces/cli/run-operational-command.test.ts (1)
13-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared dependency literal and assert the envelope before reading nested fields.
Both tests duplicate the same
runtimePaths,openRuntime,applicationVersion, and stream stubs. Extract acreateDependencies(root, output)helper.Lines 39 and 63 use
JSON.parse(output[0] ?? '{}'). If no output is written, the parse succeeds and the following member access throws aTypeError. The failure message then hides the real cause. Assertoutputhas one entry first, then read the parsed fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/interfaces/cli/run-operational-command.test.ts` around lines 13 - 64, Extract the duplicated runtimePaths, openRuntime, applicationVersion, stdout, and stderr setup into a shared createDependencies(root, output) helper, then use it in both tests. Before parsing output in each test, assert that output contains exactly one entry; only afterward parse output[0] and inspect the nested data fields.src/interfaces/cli/run-operational-command.ts (1)
97-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the cast and the non-null assertion with union narrowing.
command.client as MutableIntegrationClientandcommand.configFile!discard the type guarantees ofOperationalCommand. The invariant holds today only becauseparseSetupinsrc/interfaces/cli/parse-operational-command.tsalways setsconfigFileforcodexandclaude-code. A future parser change breaks this silently at runtime instead of at compile time. Add an explicit guard.♻️ Proposed narrowing
- const client = command.client as MutableIntegrationClient; - const configPath = command.configFile!; + const { client, configFile } = command; + if (client === undefined || client === 'generic-mcp' || configFile === undefined) + throw new CliUsageError('--client and --config-file are required for this operation.'); + const configPath = configFile;🤖 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/cli/run-operational-command.ts` around lines 97 - 98, In the operational command handling around client and configPath, remove the MutableIntegrationClient cast and configFile non-null assertion. Add an explicit guard that narrows the command union to the variants guaranteeing both a compatible client and configFile before accessing them, and handle or reject other variants explicitly so future parser changes fail safely and compile-time guarantees are preserved.scripts/package/smoke-installed-package.ts (1)
292-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParse the rerun envelope instead of matching a raw substring.
Line 299 checks
rerun.stdout.includes('"changed":false'). The check depends on the exact serialization produced byJSON.stringify. A formatting change, a key reorder, or added whitespace makes the assertion pass or fail for the wrong reason. Theenvelopehelper at line 126 already parses and validates single-line JSON output.♻️ Proposed change
- if (rerun.status !== 0 || !rerun.stdout.includes('"changed":false')) + const rerunData = rerun.status === 0 ? (envelope(rerun).data as { changed?: boolean }) : undefined; + if (rerun.status !== 0 || rerunData?.changed !== false) throw new Error('Installed setup rerun was not idempotent.');🤖 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/smoke-installed-package.ts` around lines 292 - 300, Update the rerun validation in the smoke test to use the existing envelope helper for parsing and validating rerun.stdout, then assert the parsed changed field is false instead of matching a raw JSON substring. Preserve the nonzero-status failure check and the existing idempotency error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/package/smoke-installed-package.ts`:
- Around line 241-248: Update the setupEnvironment used by the installed-package
smoke test and the related runCli setup invocation to override platform-specific
configuration roots on Linux and macOS as well as Windows, using temporaryRoot
for the XDG and HOME-based paths consumed by getPlatformDefaultPaths(). Keep all
smoke-test data isolated from the real user directories while preserving the
existing setup validation.
In `@scripts/validate-repository-assets.ts`:
- Around line 132-143: Update the asset validation flow around normalizedPath
and parseJsonc so only the intended Claude JSONC fixtures are parsed with JSONC
support; validate all other .json assets with JSON.parse to reject comments
consistently with contract parsing. Remove the formatted.json exception from the
skip list, retaining only the intentionally malformed setup fixtures.
In `@src/distribution/setup/apply-integration-change.ts`:
- Around line 76-81: Update the restore-failure branch in the outer catch of the
ownershipStore.update flow to preserve both the original metadata failure and
the subsequent restoreError when constructing SetupStorageError. Keep the
existing restore context and backup-path message, but pass or append both error
details so neither cause is discarded.
- Around line 26-35: Update the no-rewrite branch around clientChanged and
before persisting nextRecord to reread input.plan.configPath, recompute its
fingerprint, and compare it with input.plan.beforeFingerprint. Throw
SetupConflictError on any mismatch, while preserving the existing
backupAndAtomicWrite path when clientChanged is true.
In `@src/distribution/setup/backup-and-atomic-write.ts`:
- Around line 26-34: Update the backup flow around readOriginalFile and
createExclusiveBackup to create and return a backup path only when
original.existed is true; otherwise leave backupPath undefined. Propagate the
optional BackupAndAtomicWriteResult.backupPath through applyIntegrationChange so
lastBackupPath is not recorded for targets that did not previously exist, while
preserving restoreOriginalFile’s existing originalExisted === false behavior.
- Around line 50-58: Add an exclusion lock keyed by input.plan.configPath around
the complete client-config replacement workflow, including backup, write,
validation, replacement, and the subsequent ownershipStore.update in
applyIntegrationChange. Ensure concurrent setup operations targeting the same
config path are serialized, while preserving the existing fingerprint conflict
check and cleanup behavior.
In `@src/distribution/setup/clients/claude-json-adapter.ts`:
- Around line 14-21: Add an empty Claude JSON fixture representing a
configuration with no mcpServers key, and update claude-json-adapter.test.ts to
cover the 'absent' path with that fixture alongside the existing unrelated.json
case. Keep the fixture valid JSON and preserve current adapter behavior.
In `@src/distribution/setup/clients/codex-toml-adapter.ts`:
- Line 1: Update the TOML parser dependency used by the Codex configuration
adapter and its import in parseDocument to a version supporting TOML 1.0+
syntax, while preserving the existing malformed-configuration error behavior for
genuinely invalid input.
In `@src/distribution/setup/initialize-relay.ts`:
- Around line 27-28: Update the directory tracking in the initialization flow
around dependencies.mkdir so that, whenever mkdir reports creation, it pushes
the requested directory rather than the returned ancestor path. Add a test
covering dataRoot and configRoot with an absent shared parent, verifying
createdDirectories and the resulting CLI output report both requested roots.
In `@src/distribution/setup/ownership-store.ts`:
- Around line 115-133: Update withOwnershipLock’s acquisition retry path to
inspect an existing lock via the lock payload and invoke tryBreakStaleLock when
acquiredAt exceeds staleAfterMs, then retry opening the lock after unlinking
stale files. Preserve active-lock conflict behavior, and include the lock path
in SetupConflictError so users can locate and remove an unrecoverable lock.
- Around line 34-43: Update the SetupStorageError construction in the ownership
metadata parsing catch block to pass the caught error directly as the
constructor’s second argument, matching the existing storage read/write patterns
and preserving the actual cause.
In `@src/interfaces/cli/operational-output.ts`:
- Around line 35-41: The error mapping currently classifies every RelayError as
VALIDATION_ERROR, including infrastructure failures. Update the error
classification condition to exclude the base RelayError while preserving
existing CliUsageError, SetupUsageError, and explicitly handled
SetupStorageError behavior; let other RelayError instances fall through to
INTERNAL_ERROR.
In `@tests/fixtures/agent-integrations/valid/integrations/generic-mcp/README.md`:
- Line 1: Update the validation guidance in the README to explicitly require an
isolated, non-empty absolute path for RELAY_DB_PATH, while preserving the
existing allowance for omission outside validation use and the instruction that
the SQLite database remains untouched.
In `@tests/fixtures/setup/metadata/disabled.json`:
- Around line 5-12: Replace the static Windows paths in
tests/fixtures/setup/metadata/disabled.json#L5-L12 and
tests/fixtures/setup/metadata/enabled.json#L5-L12 with test-generated
host-native paths using join(tmpdir(), ...), and update the consuming tests to
construct these records. In tests/fixtures/setup/metadata/duplicate.json#L5-L22,
generate two host-native equivalent paths, retaining a "." segment so duplicate
detection remains covered.
In `@tests/fixtures/setup/metadata/wrong-command.json`:
- Line 6: Update the configPath value in the wrong-command fixture to use a
POSIX absolute path or a host-aware platform-native fixture, matching the
established formatted.toml fixture pattern. Preserve the fixture’s invalid
metadata and wrong-command behavior while ensuring ownership-store absolute-path
validation succeeds on Linux and macOS.
In `@tests/unit/distribution/setup/backup-and-atomic-write.test.ts`:
- Around line 62-79: Add a test alongside the existing restore test for a
pre-existing target file, using known content and permissions. Exercise
backupAndAtomicWrite and restoreOriginalFile, then assert the restored file’s
exact bytes and that stat.mode & 0o777 matches the original mode, covering the
existing-file rollback path.
In `@tests/unit/distribution/setup/plan-integration-change.test.ts`:
- Around line 13-62: Extend the planIntegrationChange test suite with focused
cases for a missing configuration file, successful disable, successful remove,
and setup transitioning a disabled Relay entry to enabled. Assert each plan’s
operation and changed values, and verify the resulting configuration content
where the transition writes it, while preserving the existing validation tests.
---
Nitpick comments:
In `@scripts/package/smoke-installed-package.ts`:
- Around line 292-300: Update the rerun validation in the smoke test to use the
existing envelope helper for parsing and validating rerun.stdout, then assert
the parsed changed field is false instead of matching a raw JSON substring.
Preserve the nonzero-status failure check and the existing idempotency error
behavior.
In `@scripts/validate-agent-integration-assets.ts`:
- Around line 279-281: Remove the redundant JSON.parse and parse calls
immediately after validateTemplateShape(rootDir) in the validation flow. Keep
validateTemplateShape responsible for reading and parsing the three template
assets, and leave the remaining validation behavior unchanged.
- Around line 173-206: Extract a shared helper near the validation logic to
validate the installed MCP server command/args contract, requiring exactly the
command and args keys, command “relay”, and args equivalent to [“mcp”]. Replace
both the JSON server check and TOML codexServer check with this helper, using
one consistent args comparison style, and change codexServer?.command to
codexServer.command after its undefined guard.
In `@scripts/validate-repository-assets.ts`:
- Around line 403-408: Remove the setup-fixture iteration that filters
requiredDistributionAssets and calls existsSync/fail, since
validateRepositoryAssets already validates every entry through requiredPaths.
Keep the existing requiredDistributionAssets validation unchanged and eliminate
only this duplicate check and its error path.
In `@src/distribution/setup/apply-integration-change.ts`:
- Around line 37-47: Extract the canonical Relay entry values—entryId, command,
and args—into one shared exported constant, then update applyIntegrationChange
and the adapter or snippet-generation code to reuse it everywhere, including the
repeated entryId references. Remove the duplicated literals while preserving the
existing exact values so ownership records and generated content remain
synchronized.
In `@src/interfaces/cli/operational-output.ts`:
- Line 41: Update the unexpected internal-error handling in the operational
output builder to keep the returned stdout envelope generic while emitting the
original error details, including its cause or stack when available, to stderr
or through the existing debug logging path. Preserve the INTERNAL_ERROR code and
exitCode values.
In `@src/interfaces/cli/run-operational-command.ts`:
- Around line 97-98: In the operational command handling around client and
configPath, remove the MutableIntegrationClient cast and configFile non-null
assertion. Add an explicit guard that narrows the command union to the variants
guaranteeing both a compatible client and configFile before accessing them, and
handle or reject other variants explicitly so future parser changes fail safely
and compile-time guarantees are preserved.
In `@src/interfaces/production-dependencies.ts`:
- Around line 22-25: Extract the shared ownership metadata path calculation into
an exported resolveOwnershipMetadataPath(runtimePaths) helper beside
resolveRuntimePaths, then replace the direct join(runtimePaths.configRoot,
'config.json') usage in production dependencies and both operational-command
sites, including the fallback ownership store and config-paths output.
In `@tests/unit/interfaces/cli/operational-commands.test.ts`:
- Around line 22-50: Add positive-path assertions in the “parses snippet and
inspection commands” test for valid `config disable` and `config remove`
invocations, including their expected `config-disable` and `config-remove` kinds
and parsed client/config-file values. Use the existing absoluteConfigPath
fixture and include `--apply` so both action-to-kind mappings in
parseOperationalCommand are directly verified.
In `@tests/unit/interfaces/cli/run-operational-command.test.ts`:
- Around line 13-64: Extract the duplicated runtimePaths, openRuntime,
applicationVersion, stdout, and stderr setup into a shared
createDependencies(root, output) helper, then use it in both tests. Before
parsing output in each test, assert that output contains exactly one entry; only
afterward parse output[0] and inspect the nested data fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f8152e7-dbcc-4227-a709-61c81a11a3fa
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (74)
.prettierignoreREADME.mddocs/agent-integration.mddocs/distribution/npm-package.mddocs/setup-and-configuration.mddocs/superpowers/plans/2026-08-02-issue-41-safe-setup-and-agent-configuration.mddocs/superpowers/plans/2026-08-02-pr-48-review-remediation.mddocs/troubleshooting-agent-integration.mdintegrations/claude-code/.mcp.json.exampleintegrations/claude-code/README.mdintegrations/codex/README.mdintegrations/codex/config.toml.exampleintegrations/generic-mcp/README.mdintegrations/generic-mcp/server-config.json.examplepackage.jsonscripts/package/smoke-installed-package.tsscripts/validate-agent-integration-assets.tsscripts/validate-repository-assets.tssrc/distribution/setup/apply-integration-change.tssrc/distribution/setup/backup-and-atomic-write.tssrc/distribution/setup/clients/claude-json-adapter.tssrc/distribution/setup/clients/client-adapter.tssrc/distribution/setup/clients/codex-toml-adapter.tssrc/distribution/setup/initialize-relay.tssrc/distribution/setup/ownership-store.tssrc/distribution/setup/plan-integration-change.tssrc/distribution/setup/setup-errors.tssrc/distribution/setup/setup-types.tssrc/distribution/setup/snippets.tssrc/interfaces/cli/main.tssrc/interfaces/cli/operational-output.tssrc/interfaces/cli/parse-operational-command.tssrc/interfaces/cli/run-operational-command.tssrc/interfaces/cli/run-relay.tssrc/interfaces/production-dependencies.tstests/fixtures/agent-integrations/valid/integrations/claude-code/.mcp.json.exampletests/fixtures/agent-integrations/valid/integrations/codex/config.toml.exampletests/fixtures/agent-integrations/valid/integrations/generic-mcp/README.mdtests/fixtures/agent-integrations/valid/integrations/generic-mcp/server-config.json.exampletests/fixtures/setup/claude-code/conflicting.jsontests/fixtures/setup/claude-code/crlf.jsontests/fixtures/setup/claude-code/empty.jsontests/fixtures/setup/claude-code/formatted.jsontests/fixtures/setup/claude-code/malformed.jsontests/fixtures/setup/claude-code/matching.jsontests/fixtures/setup/claude-code/unrelated.jsontests/fixtures/setup/codex/conflicting.tomltests/fixtures/setup/codex/crlf.tomltests/fixtures/setup/codex/empty.tomltests/fixtures/setup/codex/formatted.tomltests/fixtures/setup/codex/malformed.tomltests/fixtures/setup/codex/matching.tomltests/fixtures/setup/codex/unrelated.tomltests/fixtures/setup/metadata/disabled.jsontests/fixtures/setup/metadata/duplicate.jsontests/fixtures/setup/metadata/empty.jsontests/fixtures/setup/metadata/enabled.jsontests/fixtures/setup/metadata/malformed.jsontests/fixtures/setup/metadata/unsupported-schema.jsontests/fixtures/setup/metadata/wrong-command.jsontests/integration/setup-workflow.test.tstests/unit/distribution/setup/apply-integration-change.test.tstests/unit/distribution/setup/backup-and-atomic-write.test.tstests/unit/distribution/setup/claude-json-adapter.test.tstests/unit/distribution/setup/codex-toml-adapter.test.tstests/unit/distribution/setup/initialize-relay.test.tstests/unit/distribution/setup/ownership-store.test.tstests/unit/distribution/setup/plan-integration-change.test.tstests/unit/distribution/setup/setup-types.test.tstests/unit/distribution/setup/snippets.test.tstests/unit/interfaces/cli/operational-commands.test.tstests/unit/interfaces/cli/run-operational-command.test.tstests/unit/scripts/validate-agent-integration-assets.test.tstsup.config.ts
| "client": "claude-code", | ||
| "configPath": "C:/tmp/claude.json", | ||
| "entryId": "relay", | ||
| "command": "relay", | ||
| "args": ["mcp"], | ||
| "status": "disabled", | ||
| "applicationVersion": "0.1.0", | ||
| "lastSuccessfulSetupAt": "2026-08-02T00:00:00.000Z" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use host-native absolute paths for valid ownership fixtures.
These fixtures use C:/tmp/... paths. Those paths are not host-native absolute paths on POSIX runners. The tests can validate a different normalization result, or reject records intended to be valid.
Construct these fixture records in the consuming tests with join(tmpdir(), ...). Preserve the duplicate-path case by creating a host-native equivalent with a . path segment.
tests/fixtures/setup/metadata/disabled.json#L5-L12: replace the static Windows path with test-generated host-native data.tests/fixtures/setup/metadata/duplicate.json#L5-L22: generate two host-native equivalent paths for duplicate detection.tests/fixtures/setup/metadata/enabled.json#L5-L12: replace the static Windows path with test-generated host-native data.
📍 Affects 3 files
tests/fixtures/setup/metadata/disabled.json#L5-L12(this comment)tests/fixtures/setup/metadata/duplicate.json#L5-L22tests/fixtures/setup/metadata/enabled.json#L5-L12
🤖 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/setup/metadata/disabled.json` around lines 5 - 12, Replace the
static Windows paths in tests/fixtures/setup/metadata/disabled.json#L5-L12 and
tests/fixtures/setup/metadata/enabled.json#L5-L12 with test-generated
host-native paths using join(tmpdir(), ...), and update the consuming tests to
construct these records. In tests/fixtures/setup/metadata/duplicate.json#L5-L22,
generate two host-native equivalent paths, retaining a "." segment so duplicate
detection remains covered.
| "integrations": [ | ||
| { | ||
| "client": "codex", | ||
| "configPath": "C:/tmp/codex.toml", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find consumers of the wrong-command.json fixture and inspect how configPath is used.
set -euo pipefail
rg -n "wrong-command" -C 8 tests/unit/distribution/setup/ownership-store.test.ts 2>/dev/null || \
rg -rn "wrong-command" -C 8 testsRepository: krishna916/relay
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg 'wrong-command\.json|ownership-store|metadata|setup' || true
echo
echo "== search configPath and ownership usages =="
rg -n "configPath|wrong-command|Ownership|ownership-store" -S . --glob '!node_modules' | head -200Repository: krishna916/relay
Length of output: 26220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ownership-store.ts validation/read/update slices =="
sed -n '1,130p' src/distribution/setup/ownership-store.ts
sed -n '175,230p' src/distribution/setup/ownership-store.ts
echo
echo "== setup-types.ts =="
sed -n '1,80p' src/distribution/setup/setup-types.ts
echo
echo "== plan-integration-change.ts slices =="
sed -n '1,90p' src/distribution/setup/plan-integration-change.ts
sed -n '100,125p' src/distribution/setup/plan-integration-change.ts
echo
echo "== tests/fixtures/setup/metadata/wrong-command.json =="
cat -n tests/fixtures/setup/metadata/wrong-command.json
echo
echo "== metadata fixture references in validate script =="
sed -n '45,80p' scripts/validate-repository-assets.ts
sed -n '270,315p' scripts/validate-repository-assets.ts
echo
echo "== Node path behavior for sample paths (non-modifying probe) =="
node - <<'JS'
const path = require('node:path');
const samples = [
'C:/tmp/codex.toml',
'/tmp/codex.toml',
'C:/tmp/./codex.toml',
];
for (const sample of samples) {
const resolved = path.resolve(sample);
const normalized = path.normalize(sample);
console.log(JSON.stringify({
sample,
type: path.isAbsolute(sample) ? 'absolute' : 'relative',
resolved,
normalized,
dir: path.dirname(resolved),
existsSync: require('node:fs').existsSync(resolved),
}));
}
JSRepository: krishna916/relay
Length of output: 16988
Use a POSIX absolute path here or make this fixture host-aware.
C:/tmp/codex.toml is accepted by ownership-store.ts only on Windows; on Linux/macOS it is a POSIX relative path that resolves under the current working directory and fails the ownership-record absolute-path check before matching the wrong command. Since this fixture must load with invalid metadata but not an invalid absolute path, use tests/fixtures/setup/codex/formatted.toml-style platform-native input or host-native fixtures for this test.
🤖 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/setup/metadata/wrong-command.json` at line 6, Update the
configPath value in the wrong-command fixture to use a POSIX absolute path or a
host-aware platform-native fixture, matching the established formatted.toml
fixture pattern. Preserve the fixture’s invalid metadata and wrong-command
behavior while ensuring ownership-store absolute-path validation succeeds on
Linux and macOS.
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Re-review of latest commit c45390a after the broad CodeRabbit remediation.
Verdict: one blocking operational-safety issue remains
High — crashed client-config operations can leave a permanent lock and an unrecoverable ownership mismatch
applyIntegrationChange() now acquires <configPath>.relay-lock around the complete client write and metadata update, which correctly serializes concurrent writers. However, withExclusiveFileLock() only removes the lock in normal cleanup and never inspects or recovers an abandoned lock.
Failure sequence:
- Relay acquires
<configPath>.relay-lock. - It replaces the Codex/Claude config successfully.
- The process is killed before ownership metadata is persisted or before lock cleanup.
- The lock remains permanently.
- Every later setup/disable/remove operation for that config fails after the retry window.
- If the user manually removes the lock and retries setup, the client contains a matching Relay entry but ownership metadata may be absent, so planning fails closed as an unowned matching entry.
This is more than a temporary availability issue: the command may have changed the client file while reporting no success, and normal Relay commands cannot reconcile the state afterward.
Required correction should preserve the accepted fail-closed behavior. Do not blindly delete active locks. Add an explicit recovery design, for example:
- inspect lock metadata and safely break only provably stale locks, plus reconcile using the exact entry and retained backup; or
- keep manual stale-lock removal but document and implement a deterministic recovery command/workflow that restores the retained backup or adopts only an exact Relay entry after explicit operator confirmation.
At minimum, add an integration test that simulates an abandoned client lock after the client file is changed but before metadata persistence, and prove the documented recovery path restores a usable state.
Other review notes
- The same-config concurrency lock, no-rewrite fingerprint check, optional backup behavior, error-cause preservation, host-isolated package smoke, JSON/JSONC validation split, initialization reporting, and lifecycle tests look correct.
- The
js-toml1.2.1 replacement supports TOML 1.1 and is above the patched 1.0.2 security floor; package audit and CI pass. - CI run 126 passed.
- CodeRabbit is still processing the latest commit, and several threads remain unresolved, though the static Windows-path fixture comments do not affect the production implementation.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/distribution/setup/clients/codex-toml-adapter.ts (1)
74-88: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve comments outside the removed table.
The section boundary falls back to the end of the file. When Relay is the last table,
removeRelayEntrydeletes trailing comments. When another table follows, it also deletes comments between the tables. This silently removes user content duringconfig remove.Preserve comment-only content outside the Relay table. Add EOF and inter-table comment cases.
🤖 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/setup/clients/codex-toml-adapter.ts` around lines 74 - 88, The section range calculated by singleRelayHeader must exclude comments outside the Relay table. Adjust the boundary logic used by removeRelayEntry so trailing EOF comments and comment-only content between Relay and the next table remain preserved, while the Relay table itself is still removed. Add coverage for both EOF and inter-table comment cases.
🧹 Nitpick comments (3)
src/distribution/setup/ownership-store.ts (1)
113-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the argument check from
RELAY_ARGS.The length check hardcodes
1and compares onlyargs[0]. IfRELAY_ARGSgains an element, this validation rejects valid records and ignores the extra argument. Compare against the constant instead.♻️ Proposed refactor
- !Array.isArray(value.args) || - value.args.length !== 1 || - value.args[0] !== RELAY_ARGS[0] || + !Array.isArray(value.args) || + value.args.length !== RELAY_ARGS.length || + RELAY_ARGS.some((arg, index) => value.args[index] !== arg) ||🤖 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/setup/ownership-store.ts` around lines 113 - 115, Update the argument validation in the ownership record check to compare value.args directly with RELAY_ARGS, including matching length and every element. Remove the hardcoded length and single-index comparison while preserving the existing array validation.tests/unit/distribution/setup/apply-integration-change.test.ts (1)
331-334: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the error type directly instead of matching
constructor.
toMatchObjectcomparesconstructorby reference through the prototype chain. The assertion works, but it is indirect and it breaks silently if the error class is wrapped. Capture the rejection, then assert the instance and the message.♻️ Proposed refactor
- ).rejects.toMatchObject({ - constructor: SetupConflictError, - message: expect.stringMatching(/relay-lock/i), - }); + ).rejects.toThrowError( + expect.objectContaining({ message: expect.stringMatching(/relay-lock/i) }), + );Alternatively, capture the error with
.catch()and asserttoBeInstanceOf(SetupConflictError)plus the message, as the test at lines 287-298 already does.🤖 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/setup/apply-integration-change.test.ts` around lines 331 - 334, Update the rejection assertion in the integration-change test to capture the rejected error, then assert it with toBeInstanceOf(SetupConflictError) and a separate message expectation matching /relay-lock/i. Follow the existing pattern used by the nearby test rather than matching the constructor through toMatchObject.scripts/package/smoke-installed-package.ts (1)
308-318: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert lifecycle effects, not only exit status.
This loop only checks
result.status. A packaged CLI that exits successfully without disabling, re-enabling, or removing the entry would pass the smoke test.After each action, inspect the Codex file and ownership metadata. Verify disabled, enabled, and removed states.
🤖 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/smoke-installed-package.ts` around lines 308 - 318, The action loop in the smoke test currently validates only process status; update it to inspect the Codex configuration file and ownership metadata after each action. Assert that config disable produces the disabled state, setup restores the enabled state, and config remove removes the entry and its ownership metadata, while preserving the existing nonzero-status failure handling.
🤖 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/setup/clients/codex-toml-adapter.ts`:
- Around line 7-9: Update the header matching used by inspect and
removeRelayEntry to recognize whitespace around the table path components and
quoted relay table names, consistent with the parser’s accepted TOML forms.
Adjust headerPattern or the shared matching logic without changing unrelated
parsing behavior, and add regression cases covering spaced headers.
In `@src/distribution/setup/file-lock.ts`:
- Around line 33-46: The lock acquisition loop around open and
SetupConflictError must detect and recover stale lock files. Read the existing
lock payload’s acquiredAt and, when its age exceeds a bounded threshold, unlink
the lock before retrying; otherwise preserve the current retry behavior and
final SetupConflictError for active locks. Reuse the lock payload format written
by the setup lock implementation rather than introducing a separate marker.
- Around line 67-81: Update the cleanup/error propagation in the lock-release
flow so an action failure remains the primary thrown error when both the action
and lock cleanup fail. Preserve the existing cleanup error reporting when the
action succeeds, and when both fail, attach or aggregate the cleanup failure
without replacing actionError, including errors from handle.close and unlink.
In `@tests/unit/distribution/setup/apply-integration-change.test.ts`:
- Around line 313-330: Update the applyIntegrationChange invocation in the
lock-contention test to configure the directly acquired configuration lock,
using its lock options input for retry delay, maximum attempts, and sleep.
Remove the inert lockRetryDelayMs, lockMaxAttempts, and sleep settings from
createOwnershipStore unless they are also needed elsewhere, and ensure the
test’s existing lockPath contention uses the tunable settings.
In `@tests/unit/interfaces/cli/run-operational-command.test.ts`:
- Line 43: Update the test assertions around the setup command to verify the
exact configuration paths: assert that the setup command’s actual configFile
remains absent rather than checking the ownership metadata path, and change the
path assertion to equal join(root, 'config', 'config.json') so it validates
runtimePaths.configRoot.
---
Outside diff comments:
In `@src/distribution/setup/clients/codex-toml-adapter.ts`:
- Around line 74-88: The section range calculated by singleRelayHeader must
exclude comments outside the Relay table. Adjust the boundary logic used by
removeRelayEntry so trailing EOF comments and comment-only content between Relay
and the next table remain preserved, while the Relay table itself is still
removed. Add coverage for both EOF and inter-table comment cases.
---
Nitpick comments:
In `@scripts/package/smoke-installed-package.ts`:
- Around line 308-318: The action loop in the smoke test currently validates
only process status; update it to inspect the Codex configuration file and
ownership metadata after each action. Assert that config disable produces the
disabled state, setup restores the enabled state, and config remove removes the
entry and its ownership metadata, while preserving the existing nonzero-status
failure handling.
In `@src/distribution/setup/ownership-store.ts`:
- Around line 113-115: Update the argument validation in the ownership record
check to compare value.args directly with RELAY_ARGS, including matching length
and every element. Remove the hardcoded length and single-index comparison while
preserving the existing array validation.
In `@tests/unit/distribution/setup/apply-integration-change.test.ts`:
- Around line 331-334: Update the rejection assertion in the integration-change
test to capture the rejected error, then assert it with
toBeInstanceOf(SetupConflictError) and a separate message expectation matching
/relay-lock/i. Follow the existing pattern used by the nearby test rather than
matching the constructor through toMatchObject.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b4392291-d2fb-4adc-a0aa-2674b00b649f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
package.jsonscripts/package/smoke-installed-package.tsscripts/validate-agent-integration-assets.tsscripts/validate-repository-assets.tssrc/distribution/setup/apply-integration-change.tssrc/distribution/setup/backup-and-atomic-write.tssrc/distribution/setup/clients/claude-json-adapter.tssrc/distribution/setup/clients/codex-toml-adapter.tssrc/distribution/setup/file-lock.tssrc/distribution/setup/initialize-relay.tssrc/distribution/setup/ownership-store.tssrc/distribution/setup/plan-integration-change.tssrc/distribution/setup/relay-entry.tssrc/distribution/setup/snippets.tssrc/interfaces/cli/operational-output.tssrc/interfaces/cli/run-operational-command.tssrc/interfaces/production-dependencies.tstests/fixtures/agent-integrations/valid/integrations/generic-mcp/README.mdtests/fixtures/setup/claude-code/no-mcp-servers.jsontests/integration/cli.test.tstests/integration/setup-workflow.test.tstests/unit/contracts/distribution-contract.test.tstests/unit/distribution/setup/apply-integration-change.test.tstests/unit/distribution/setup/backup-and-atomic-write.test.tstests/unit/distribution/setup/claude-json-adapter.test.tstests/unit/distribution/setup/initialize-relay.test.tstests/unit/distribution/setup/ownership-store.test.tstests/unit/distribution/setup/plan-integration-change.test.tstests/unit/distribution/setup/snippets.test.tstests/unit/interfaces/cli/operational-commands.test.tstests/unit/interfaces/cli/operational-output.test.tstests/unit/interfaces/cli/run-operational-command.test.tstests/unit/scripts/validate-agent-integration-assets.test.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- tests/fixtures/agent-integrations/valid/integrations/generic-mcp/README.md
- tests/unit/scripts/validate-agent-integration-assets.test.ts
- src/distribution/setup/snippets.ts
- src/distribution/setup/clients/claude-json-adapter.ts
- tests/unit/distribution/setup/snippets.test.ts
- tests/unit/interfaces/cli/operational-commands.test.ts
- scripts/validate-repository-assets.ts
- tests/unit/distribution/setup/initialize-relay.test.ts
- tests/integration/setup-workflow.test.ts
- tests/unit/distribution/setup/ownership-store.test.ts
- scripts/validate-agent-integration-assets.ts
- src/interfaces/cli/operational-output.ts
- tests/unit/distribution/setup/claude-json-adapter.test.ts
- src/distribution/setup/apply-integration-change.ts
- src/distribution/setup/plan-integration-change.ts
- src/distribution/setup/backup-and-atomic-write.ts
- src/interfaces/cli/run-operational-command.ts
| const headerPattern = new RegExp( | ||
| `^\\s*\\[mcp_servers\\.${RELAY_ENTRY_ID}\\][^\\r\\n]*(?:\\r?\\n|$)`, | ||
| 'gm', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'codex-toml-adapter\.ts$' . || true
echo "== file outline =="
FILE="$(fd 'codex-toml-adapter\.ts$' . | head -n1 || true)"
if [ -n "${FILE:-}" ]; then
ast-grep outline "$FILE" --view expanded || true
echo "== file line count =="
wc -l "$FILE"
echo "== relevant content =="
cat -n "$FILE"
fi
echo "== search related symbols =="
rg -n "RELAY_ENTRY_ID|mcp_servers|singleRelayHeader|headerPattern|removeRelay|parseDocument|inspect" . --glob '*.ts' --glob '*.tsx' || trueRepository: krishna916/relay
Length of output: 14745
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral probe for the exact regex semantics used in this adapter.
node - <<'JS'
const RELAY_ENTRY_ID = "relay";
const headerPattern = new RegExp(
`^\\s*\\[mcp_servers\\.${RELAY_ENTRY_ID}\\][^\\r\\n]*(?:\\r?\\n|$)`,
'gm',
);
const headerRemoverPattern = /(^|\n)\s*\[mcp_servers\.relay\][^\n]*(\n|$)/s;
const samples = [
"[mcp_servers.relay]\nkey = 'val'",
"\n[mcp_servers.relay]\nkey = 'val'",
"[ mcp_servers . relay ]\nkey = 'val'",
'[mcp_servers."relay"]\nkey = "val"',
'[ mcp_servers . "relay" ]\nkey = "val"',
`[mcp_servers.relay]`;
];
console.log("headerPattern matches each sample:", samples.map(s => [{text: JSON.stringify(s).slice(0, 80), found: !!headerPattern.test(s)}]).flat());
console.log("headerRemoverPattern matches each sample:", samples.map(s => ({text: JSON.stringify(s).slice(0, 80), found: !!(s.match(headerRemoverPattern))})));
JSRepository: krishna916/relay
Length of output: 762
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== codex unit tests =="
FILE="$(fd 'codex-toml-adapter\.test\.ts$' . | head -n1 || true)"
if [ -n "${FILE:-}" ]; then
wc -l "$FILE"
cat -n "$FILE"
fi
echo "== package parser dependency =="
if [ -f package.json ]; then
jq -r '.dependencies["js-toml"], .devDependencies["js-toml"]' package.json
fi
echo "== node string-array behavior probe =="
node - <<'JS'
const RELAY_ENTRY_ID = "relay";
const headerPattern = new RegExp(
`^\\s*\\[mcp_servers\\.${RELAY_ENTRY_ID}\\][^\\r\\n]*(?:\\r?\\n|$)`,
'gm',
);
const headerRemoverPattern = /(^|\n)\s*\[mcp_servers\.relay\][^\n]*(\n|$)/s;
const samples = [
"[mcp_servers.relay]\nkey = 'val'",
"\n[mcp_servers.relay]\nkey = 'val'",
"[ mcp_servers . relay ]\nkey = 'val'",
'[mcp_servers."relay"]\nkey = "val"',
'[ mcp_servers . "relay" ]\nkey = "val"',
];
console.log("headerPattern matches each sample:", samples.map(s => [JSON.stringify(s).slice(0, 80), !!headerPattern.test(s)]).flat());
console.log("headerRemoverPattern matches each sample:", samples.map(s => [JSON.stringify(s).slice(0, 80), !!(s.match(headerRemoverPattern))]));
JS
echo "== behavioral probe using js-toml if installed =="
node - <<'JS'
try {
const { load } = require('js-toml');
const samples = [
"[mcp_servers.relay]\ncommand = 'relay'\nargs = ['mcp']",
"[ mcp_servers . relay ]\ncommand = 'relay'\nargs = ['mcp']",
'[mcp_servers."relay"]\ncommand = "relay"\nargs = ["mcp"]',
'[ mcp_servers . "relay" ]\ncommand = "relay"\nargs = ["mcp"]',
];
for (const content of samples) {
try {
const value = load(content);
console.log("js-toml parses", JSON.stringify(content.split("\\n")[0]).slice(0, 60), ":", JSON.stringify(value.mcp_servers?.relay));
} catch (error) {
console.log("js-toml rejects", JSON.stringify(content.split("\\n")[0]).slice(0, 60), ":", error.message);
}
}
} catch (error) {
console.log("js-toml require failed:", error && error.code === 'MODULE_NOT_FOUND' ? 'module-not-found' : error.message || String(error));
}
JSRepository: krishna916/relay
Length of output: 3087
🌐 Web query:
TOML 1.1.0 bare key segments whitespace brackets quoted strings key segments allowed characters
💡 Result:
In TOML 1.1.0, keys are categorized as bare, quoted, or dotted [1][2]. Their specifications are as follows: Bare Keys: Bare keys may only contain ASCII letters (A-Z, a-z), ASCII digits (0-9), underscores (_), and dashes (-) [1][3]. They must be non-empty [1]. Although they may consist solely of ASCII digits (e.g., 1234), they are always interpreted as strings [1][2]. Quoted Keys: Quoted keys use the same syntax rules as basic strings or literal strings [1][2]. They allow for a much broader range of characters than bare keys, including Unicode characters, spaces, and special symbols [1][2]. They cannot use multi-line strings [1][3]. An empty quoted key is permitted but discouraged [1]. Dotted Keys: Dotted keys are a sequence of bare or quoted keys joined by a dot (.) [1][2]. They are used to group properties and implicitly define nested tables [1][4]. Whitespace and Brackets: Whitespace (spaces and tabs) around keys, around the equals sign, and around the dot separators in dotted keys is ignored [1][5]. While allowed, the use of extraneous whitespace is discouraged [1][3]. Regarding brackets, while curly braces ({}) are used to define inline tables, they are not part of the key name itself; however, keys within inline tables follow the same rules as standard keys [4]. Top results: [1], [5], [2]
Citations:
- 1: https://toml.io/en/v1.1.0
- 2: https://deepwiki.com/toml-lang/toml/2.2-keys-and-key-value-pairs
- 3: https://raw.githubusercontent.com/toml-lang/toml.io/main/specs/en/v1.1.0.md
- 4: https://github.com/edadma/toml/blob/a980e587233b4f4f32af856253f7a0d62a2f1672/v1.1.0.md
- 5: https://github.com/toml-lang/toml/blob/1.1.0/toml.abnf
Accept TOML table headers with whitespace.
The parser accepts [mcp_servers.relay], [ mcp_servers . relay ], and quoted relay table names, but inspect and removeRelayEntry only recognize the exact bare spelling. Add matching removal logic and regression cases for spaced headers.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 6-9: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^\\s*\\[mcp_servers\\.${RELAY_ENTRY_ID}\\][^\\r\\n]*(?:\\r?\\n|$),
'gm',
)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 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/setup/clients/codex-toml-adapter.ts` around lines 7 - 9,
Update the header matching used by inspect and removeRelayEntry to recognize
whitespace around the table path components and quoted relay table names,
consistent with the parser’s accepted TOML forms. Adjust headerPattern or the
shared matching logic without changing unrelated parsing behavior, and add
regression cases covering spaced headers.
| while (handle === undefined) { | ||
| try { | ||
| handle = await open(lockPath, 'wx', 0o600); | ||
| } catch (error) { | ||
| if (!isExists(error)) | ||
| throw new SetupStorageError(`Setup lock could not be opened at ${lockPath}.`, error); | ||
| attempts += 1; | ||
| if (attempts >= maxAttempts) | ||
| throw new SetupConflictError( | ||
| `Another Relay configuration operation is in progress. Retry after it completes. Lock: ${lockPath}`, | ||
| ); | ||
| await sleep(retryDelayMs); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
A crashed process still leaves a permanent lock file.
The lock payload records pid and acquiredAt at line 54, but no code reads it. If the holder is killed, the lock file survives. Every later relay setup, relay config disable, and relay config remove then fails with SetupConflictError until the user removes the file manually. The message now names the lock path, which helps recovery, but automatic stale-lock breaking is still missing. Read acquiredAt, treat the lock as stale after a bounded age, and unlink it before the final conflict.
🤖 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/setup/file-lock.ts` around lines 33 - 46, The lock
acquisition loop around open and SetupConflictError must detect and recover
stale lock files. Read the existing lock payload’s acquiredAt and, when its age
exceeds a bounded threshold, unlink the lock before retrying; otherwise preserve
the current retry behavior and final SetupConflictError for active locks. Reuse
the lock payload format written by the setup lock implementation rather than
introducing a separate marker.
| createDependencies(root, output), | ||
| ); | ||
| expect(code).toBe(0); | ||
| expect(existsSync(join(root, 'config', 'config.json'))).toBe(false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the actual configuration paths.
Line 43 checks join(root, 'config', 'config.json'). This is the ownership metadata path, not the mutable client configuration path. A preview could write the client configuration and still pass this test. Assert that the exact configFile used by the setup command remains absent.
Line 55 only checks that the path contains config.json. Assert toBe(join(root, 'config', 'config.json')) so the test verifies runtimePaths.configRoot.
As per coding guidelines, tests must cover normal flows, validation failures, and lifecycle boundaries in the layer where they belong.
Also applies to: 55-55
🤖 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/run-operational-command.test.ts` at line 43, Update
the test assertions around the setup command to verify the exact configuration
paths: assert that the setup command’s actual configFile remains absent rather
than checking the ownership metadata path, and change the path assertion to
equal join(root, 'config', 'config.json') so it validates
runtimePaths.configRoot.
Source: Coding guidelines
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Review verdict: one blocking recovery issue remains
The interrupted-transaction journal, recovery-before-replan orchestration, lock guidance, rollback coverage, and CI are all substantial improvements. CI run 127 passed.
High — client-written recovery can overwrite or silently accept a post-crash third-party edit
In recoverIntegrationTransaction(), the client-written path reads ownership and then either deletes the journal when ownership matches or restores the backup when ownership does not match. It never first verifies that the current client file still has journal.nextFingerprint.
This creates two unsafe cases:
- After Relay writes the client config and crashes, another tool/user changes that config. If ownership happens to reflect the intended state, recovery deletes the journal and reports
completedeven though the on-disk entry may no longer match the transaction. - If ownership does not reflect the intended state, recovery restores the old backup over the newer third-party edit, violating the fail-closed/no-unknown-overwrite contract.
Before any client-written completion or rollback decision, read the current fingerprint and require it to equal nextFingerprint. If it differs, throw SetupConflictError, retain the journal and backup, and do not modify ownership or the client file.
Add regression tests for both ownership-matching and ownership-missing cases where the current file has a third fingerprint. Both must conflict and preserve the file, journal, backup, and ownership unchanged.
Until this is fixed, I would not merge the PR.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/distribution/setup/integration-transaction-journal.ts (1)
52-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
applicationVersioninput.
recoverIntegrationTransactiondeclaresapplicationVersion, but no code in the function reads it. Both callers pass it. Remove the field, or read it to record the recovering version.As per coding guidelines, "Avoid
any, unhandled promises, and unused arguments; prefix intentionally unused arguments with_."🤖 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/setup/integration-transaction-journal.ts` around lines 52 - 58, Remove the unused applicationVersion field from recoverIntegrationTransaction’s input type and update both callers to stop passing it. Do not add replacement handling unless the recovery logic is explicitly changed to use the version.Source: Coding guidelines
src/interfaces/cli/run-operational-command.ts (1)
135-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface the recovery outcome.
recoverIntegrationTransactionreturns'none' | 'rolled-back' | 'completed', and the CLI discards it. When Relay rolls back or completes an interrupted transaction, the user sees only the new setup result. Include the recovery outcome in the success payload so operators can see that a repair occurred.🤖 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/cli/run-operational-command.ts` around lines 135 - 141, Capture the return value from recoverIntegrationTransaction in the operational command flow and include it in the success payload, preserving the existing setup result while exposing whether recovery was 'none', 'rolled-back', or 'completed'.tests/unit/distribution/setup/integration-transaction-journal.test.ts (1)
355-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuild the backup path from
root.
createCase()returnsroot.join(journalPath, '..', 'codex.toml.relay-backup')resolves to the same directory, but it reads as a path inside a file. Usejoin(root, 'codex.toml.relay-backup')here and at line 416.🤖 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/setup/integration-transaction-journal.test.ts` around lines 355 - 363, Update the backup path construction in the test cases around the existing-client and line-416 scenarios to use createCase()’s root value with join(root, 'codex.toml.relay-backup') instead of deriving it from journalPath. Apply the same change in both locations.src/distribution/setup/apply-integration-change.ts (1)
108-123: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDelete the journal when no rollback is possible.
Line 110 returns early when
backup === undefined. The no-content-change branch (lines 60-64) never setsbackup, so a failure duringownershipStore.updateleaves the journal file on disk with phaseclient-written. Nothing on the client file needs restoration in that branch, becausebeforeFingerprint === nextFingerprint. Recovery does clean this up on the next run, but the leftover journal makes the state look interrupted until then. Consider deleting the journal in that branch before rethrowing.♻️ Suggested change
if (ownershipPersisted) throw error; - if (journal === undefined || backup === undefined) throw error; + if (journal === undefined) throw error; + if (backup === undefined) { + await deleteIntegrationTransactionJournal(journalPath).catch(() => undefined); + throw error; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/distribution/setup/apply-integration-change.ts` around lines 108 - 123, Update the catch path in applyIntegrationChange so that when backup is undefined, it deletes the existing integration transaction journal before rethrowing the error. Preserve the current behavior for ownershipPersisted and rollback-capable cases, and target the no-content-change path where beforeFingerprint equals nextFingerprint.
🤖 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/setup/file-lock.ts`:
- Around line 89-95: Update formatConflictMessage so the recoveryJournalPath
branch's journal text ends with a period after “Preserve the journal and
backup,” ensuring the composed conflict message separates it from the following
“If no Relay process is active” sentence.
---
Nitpick comments:
In `@src/distribution/setup/apply-integration-change.ts`:
- Around line 108-123: Update the catch path in applyIntegrationChange so that
when backup is undefined, it deletes the existing integration transaction
journal before rethrowing the error. Preserve the current behavior for
ownershipPersisted and rollback-capable cases, and target the no-content-change
path where beforeFingerprint equals nextFingerprint.
In `@src/distribution/setup/integration-transaction-journal.ts`:
- Around line 52-58: Remove the unused applicationVersion field from
recoverIntegrationTransaction’s input type and update both callers to stop
passing it. Do not add replacement handling unless the recovery logic is
explicitly changed to use the version.
In `@src/interfaces/cli/run-operational-command.ts`:
- Around line 135-141: Capture the return value from
recoverIntegrationTransaction in the operational command flow and include it in
the success payload, preserving the existing setup result while exposing whether
recovery was 'none', 'rolled-back', or 'completed'.
In `@tests/unit/distribution/setup/integration-transaction-journal.test.ts`:
- Around line 355-363: Update the backup path construction in the test cases
around the existing-client and line-416 scenarios to use createCase()’s root
value with join(root, 'codex.toml.relay-backup') instead of deriving it from
journalPath. Apply the same change in both locations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a5fbae1b-a28d-4bb9-9e74-3e6254e89df0
📒 Files selected for processing (10)
src/distribution/setup/apply-integration-change.tssrc/distribution/setup/backup-and-atomic-write.tssrc/distribution/setup/file-lock.tssrc/distribution/setup/integration-transaction-journal.tssrc/interfaces/cli/run-operational-command.tstests/integration/setup-workflow.test.tstests/unit/distribution/setup/apply-integration-change.test.tstests/unit/distribution/setup/integration-transaction-journal.test.tstests/unit/distribution/setup/ownership-store.test.tstests/unit/interfaces/cli/run-operational-command.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/distribution/setup/ownership-store.test.ts
- tests/unit/distribution/setup/apply-integration-change.test.ts
- src/distribution/setup/backup-and-atomic-write.ts
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Final re-review of the crash-recovery fix: the remaining blocker is resolved. recoverIntegrationTransaction() now fingerprints the current target before ownership completion or rollback decisions, requires currentFingerprint === nextFingerprint, and fails closed while preserving the external file, journal, backup, and ownership on mismatch. The requested ownership-present, ownership-missing, and no-content-change regression cases are covered. CI run 129 is green. No additional correctness issues found in this delta. PR remains subject to the planned real Codex/Claude manual acceptance check.
1 participant
Summary
Implements Issue #41 and the authoritative plan at
docs/superpowers/plans/2026-08-02-issue-41-safe-setup-and-agent-configuration.md.relay setupinitialization with migration and task-data retention.config paths,config integrations,config snippet, disable, and remove commands.docs/superpowers/plans/2026-08-02-pr-48-review-remediation.md:Validation
pnpm.cmd verify— passed: 63 test files passed, 622 tests passed, 5 skipped; coverage 88.71% statements, 80.83% branches, 88.99% functions, 91.03% lines; formatting, lint, typecheck, build, assets, and audit stages passed.RELAY_RUN_PACKAGE_SMOKE=1 pnpm.cmd verify:package— passed installed-package setup/config smoke, including preview/apply, idempotency, disable/re-enable/remove, MCP/UI startup, and task-data retention.config integrations --output jsonreturned 2 ownership records.Summary by CodeRabbit
relay setupand configuration commands for Codex and Claude Code integrations.--apply, disable, removal, snippets, and path inspection.relay mcpcommand.