Repository navigation
fix: stop onboarding injection when the flag is off or unknown (#538) - #539
Conversation
📝 WalkthroughWalkthroughAdjusts onboarding flag fallback behavior to use a false default when the flag value is unknown, and adds an integration test that exercises cold-start and warm-cache flag delivery paths. ChangesOnboarding injection cold-start behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TestScript
participant FlagServer
participant DistIndex as dist/index.js
TestScript->>FlagServer: start delayed JSON response
TestScript->>DistIndex: spawn with DC_FLAG_URL and temp HOME
DistIndex->>TestScript: initialize response
TestScript->>DistIndex: tools/call request
DistIndex-->>TestScript: newline-delimited JSON-RPC results
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/test-onboarding-injection-flag.js (2)
75-83: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDrain (or ignore) the child's
stderrto avoid a potential block/timeout.
stderris opened as a pipe but never consumed. The spawned server is quite chatty (manylogger/debug lines). If enough is written tostderr, the ~64KB OS pipe buffer fills and the child blocks on write, which would surface here as aSCENARIO_TIMEOUT_MSfailure rather than a real assertion result.Either drain it or set it to
'ignore':♻️ Suggested change
env: { ...process.env, HOME: home, USERPROFILE: home, // Windows homedir DC_FLAG_URL: flagUrl, }, - stdio: ['pipe', 'pipe', 'pipe'], + stdio: ['pipe', 'pipe', 'ignore'], });If you need stderr for debugging, drain it instead:
child.stderr.on('data', () => {})(or'inherit').Please confirm whether the server routes its debug output to
stdout(handled by the existing line parser) orstderr; if it's the latter, this is a real hang risk.🤖 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 `@test/test-onboarding-injection-flag.js` around lines 75 - 83, The spawned child process in the onboarding injection test leaves `stderr` as a pipe without consuming it, which can block the server if debug/logger output is written there. Update the `spawn('node', [DIST_INDEX], ...)` setup to either drain `child.stderr` or set it to `'ignore'`/`'inherit'`, and verify whether the server’s debug output from `logger` goes to `stdout` or `stderr` so the test’s existing line parsing remains correct.
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
process.execPathover the bare'node'.Using
process.execPathruns the exact interpreter executing the test, avoidingPATHlookup failures or a version mismatch between the test runner and the spawned server.♻️ Suggested change
- const child = spawn('node', [DIST_INDEX], { + const child = spawn(process.execPath, [DIST_INDEX], {🤖 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 `@test/test-onboarding-injection-flag.js` at line 75, The test spawn in the onboarding injection flag flow should use the same Node interpreter as the test runner instead of relying on a PATH-resolved command. Update the `spawn(...)` call in `test-onboarding-injection-flag.js` to use `process.execPath` in place of the bare `node`, so the child process runs with the exact executable used by the current test environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/test-onboarding-injection-flag.js`:
- Around line 75-83: The spawned child process in the onboarding injection test
leaves `stderr` as a pipe without consuming it, which can block the server if
debug/logger output is written there. Update the `spawn('node', [DIST_INDEX],
...)` setup to either drain `child.stderr` or set it to `'ignore'`/`'inherit'`,
and verify whether the server’s debug output from `logger` goes to `stdout` or
`stderr` so the test’s existing line parsing remains correct.
- Line 75: The test spawn in the onboarding injection flag flow should use the
same Node interpreter as the test runner instead of relying on a PATH-resolved
command. Update the `spawn(...)` call in `test-onboarding-injection-flag.js` to
use `process.execPath` in place of the bare `node`, so the child process runs
with the exact executable used by the current test environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: daa7ceb0-e4c8-4ec6-a162-7a0b4a80e430
📒 Files selected for processing (2)
src/utils/usageTracker.tstest/test-onboarding-injection-flag.js
On a fresh start with no cached flags (like ephemeral Docker MCP Gateway containers), the onboarding decision ran before the flag download finished, and an unknown flag was treated as "on". Result: the onboarding [SYSTEM INSTRUCTION] was appended to tool results even though the remote flag serves onboarding_injection: false. Same root cause as #303, which was closed by a local config override that cannot persist in ephemeral containers. The fix: treat an unknown flag as "off" instead of "on". No waiting anywhere, so zero added latency on startup and tool calls in every network condition. Tradeoff: when the flag is ON, a brand-new user's very first tool call still sees empty flags, so onboarding fires on a later call once flags have loaded (they load within seconds, and the prompt window covers the user's first 10 calls). Considered alternative: wait for the flag download on cold starts (bounded 3-5s worst case) so onboarding fires at the first opportunity — rejected in favor of guaranteed zero delay. Adds a regression test that boots the built server over stdio with a pristine HOME and covers: unreachable flag server and flags(false) arriving after the first call (no injection on first or subsequent calls), flags(true) (onboarding must fire once flags load), and warm cache with flags(false) (honored, unchanged).
16bd2d5 to
bd5ca16
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/test-onboarding-injection-flag.js (1)
190-227: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo scenario exercises the bounded-timeout-exceeded path.
The PR objective states the cold-start wait is bounded to "about 3–5 seconds at worst." The current scenarios cover an instantly-refused connection (
unreachableUrl, port 9) and a 1s-delayed response (RACE_DELAY_MS), but neither simulates a flag server that hangs without responding or erroring — the actual case the bound is meant to protect against. Consider adding a scenario where the flag endpoint never responds (e.g., a server that accepts the connection and never writes/ends the response) to assert the tool call still resolves (without injection) within the expected bound instead of timing out atSCENARIO_TIMEOUT_MS.🤖 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 `@test/test-onboarding-injection-flag.js` around lines 190 - 227, The onboarding-injection test suite does not cover the bounded-timeout-exceeded case; add a scenario in test-onboarding-injection-flag.js that uses runScenario with a flagUrl backed by a server that accepts the request but never responds, so the cold-start wait is exercised under a hanging endpoint. Reuse the existing runScenario, startFlagServer-style helpers, and followUpDelayMs setup to assert the tool call completes without injection within the expected bound instead of reaching SCENARIO_TIMEOUT_MS.
🤖 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/utils/usageTracker.ts`:
- Around line 419-430: `shouldShowOnboarding()` is still reading
`onboarding_injection` before fresh flags are available, so cold starts can
return a false fallback and skip onboarding on the first call. Update
`usageTracker.shouldShowOnboarding()` to wait for
`featureFlagManager.waitForFreshFlags()` before checking the flag, then read
`featureFlagManager.get('onboarding_injection', false)` so the existing 5s cap
is respected while avoiding the premature default.
---
Nitpick comments:
In `@test/test-onboarding-injection-flag.js`:
- Around line 190-227: The onboarding-injection test suite does not cover the
bounded-timeout-exceeded case; add a scenario in
test-onboarding-injection-flag.js that uses runScenario with a flagUrl backed by
a server that accepts the request but never responds, so the cold-start wait is
exercised under a hanging endpoint. Reuse the existing runScenario,
startFlagServer-style helpers, and followUpDelayMs setup to assert the tool call
completes without injection within the expected bound instead of reaching
SCENARIO_TIMEOUT_MS.
🪄 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
Run ID: 9cdb6978-6697-4325-bfe8-d0642046e716
📒 Files selected for processing (2)
src/utils/usageTracker.tstest/test-onboarding-injection-flag.js
| async shouldShowOnboarding(): Promise<boolean> { | ||
| // Check feature flag first (remote kill switch) | ||
| const { featureFlagManager } = await import('./feature-flags.js'); | ||
| const onboardingEnabled = featureFlagManager.get('onboarding_injection', true); | ||
|
|
||
| // Default false: on cold starts (no flag cache yet, e.g. ephemeral Docker | ||
| // containers) this can run before the background flag fetch completes. | ||
| // Treat an unknown flag as "off" so nothing is injected — flags load | ||
| // within seconds, so when the flag is ON onboarding still fires on a | ||
| // later call while the user is new. Deliberately no waiting here to keep | ||
| // tool calls latency-free. | ||
| // See: https://github.com/wonderwhy-er/DesktopCommanderMCP/issues/538 | ||
| const onboardingEnabled = featureFlagManager.get('onboarding_injection', false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any wait/delay/await-fetch logic tied to feature flags
rg -n -C3 'onboarding_injection|awaitFlags|waitFor|fetchPromise|flagsReady' src/utils/feature-flags.ts src/utils/usageTracker.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 2620
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- usageTracker.ts (around shouldShowOnboarding) ---'
sed -n '410,470p' src/utils/usageTracker.ts
echo
echo '--- feature-flags.ts (relevant parts) ---'
sed -n '1,260p' src/utils/feature-flags.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 10198
shouldShowOnboarding() still bypasses the cold-start wait. featureFlagManager.waitForFreshFlags() already has the 5s cap, but this path only reads onboarding_injection with a false fallback, so first calls on uncached starts can skip onboarding until the next invocation.
🤖 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/utils/usageTracker.ts` around lines 419 - 430, `shouldShowOnboarding()`
is still reading `onboarding_injection` before fresh flags are available, so
cold starts can return a false fallback and skip onboarding on the first call.
Update `usageTracker.shouldShowOnboarding()` to wait for
`featureFlagManager.waitForFreshFlags()` before checking the flag, then read
`featureFlagManager.get('onboarding_injection', false)` so the existing 5s cap
is respected while avoiding the premature default.
2 participants
On a fresh start with no cached flags (like ephemeral Docker MCP Gateway containers), the onboarding decision ran before the flag download finished, and an unknown flag was treated as "on".
The fix:
Considered alternative: skip the wait entirely and rely only on the off-by-default fallback. That also fixes the bug and guarantees zero added latency in every case, but when the flag is ON, brand-new users would get the onboarding prompt one tool call later (the first call always sees empty flags), so onboarding would not fire at the first opportunity. We kept the short bounded wait.
Adds a regression test that boots the built server over stdio with a pristine HOME and covers: unreachable flag server, flags arriving after the first call (both must not inject), flag ON arriving late (must still inject), and warm cache (unchanged, no wait).
Summary by CodeRabbit
Bug Fixes
Tests