Skip to content

Windsurf#95

Merged
rachellerathbone merged 2 commits intomainfrom
windsurf
Apr 12, 2026
Merged

Windsurf#95
rachellerathbone merged 2 commits intomainfrom
windsurf

Conversation

@rachellerathbone
Copy link
Copy Markdown
Contributor

What

Brief description of changes

Why

Why this change was needed

How

Brief technical approach

Testing

How to verify the changes

@github-actions
Copy link
Copy Markdown

multicorn-ops review

Persona Role Primary Status Summary
Jordan Security Auditor no Concern API key is embedded in plain text in config snippets printed to stderr and written to disk; the detection heuristic for Windsurf silently swallows all JSON parse errors.
Priya Open Source Contributor yes Concern Tests are thorough and the CHANGELOG is clear, but the new Windsurf test leaks implementation coupling via magic prompt-selection strings, and there is no unit test for the new getWindsurfConfigPath or isWindsurfConnected helpers in isolation.
Marcus Design-Conscious Developer no Passed No UI changes in this diff; all output is CLI stderr text which is outside Marcus's evaluation scope.
Sarah Non-Technical Decision-Maker no Concern The CLI output mixes technical jargon like 'Cascade panel', 'green status indicator', and 'mcpServers schema' with no plain-English explanation of what these mean for a first-time user.
The Team Acquisition Due Diligence yes Concern The feature is well-structured but the positional integer dispatch pattern for platform selection is fragile technical debt, the Windsurf detection heuristic is too loose for production, and there is no test covering the isWindsurfConnected helper directly.
Alex Accessibility Advocate no Passed No HTML or interactive UI changed; all output is terminal stderr text outside accessibility review scope.
Yuki International User yes Concern Most copy is clear but a few phrases assume prior English idioms or product-specific knowledge that non-native users may not parse quickly.

Concerns

Jordan (Security Auditor)

  • src/proxy/config.ts:877 - The actual API key (apiKey) is inlined into the JSON snippet for Windsurf (usesInlineKey path) and printed to stderr. If stderr is captured in logs or CI output this leaks the credential in plaintext.
  • src/proxy/config.ts:596 - isWindsurfConnected swallows all exceptions including unexpected runtime errors, making it impossible to distinguish a missing file (expected) from a permissions error or corrupted JSON. Silent failure here could cause incorrect detection state.
  • src/proxy/config.ts:591 - Detection of a Multicorn proxy entry relies on the string 'multicorn' appearing in the serverUrl. An attacker-controlled config file with a crafted URL could spoof detection; the check should also verify the domain more strictly.

Priya (Open Source Contributor)

  • src/proxy/__tests__/proxy.edge-cases.test.ts:876 - The test uses Select: '4' to choose Windsurf, relying on a positional menu index. If the menu order changes again all tests break silently in hard-to-diagnose ways. Consider using a named constant or at least a comment explaining why '4' means Windsurf.
  • src/proxy/config.ts:572 - getWindsurfConfigPath and isWindsurfConnected have no dedicated unit tests. Priya cloning the repo for the first time won't know these helpers are tested at all without tracing through the large integration test.
  • src/proxy/__tests__/proxy.edge-cases.test.ts:877 - The readFileMock dispatches on path.includes('.openclaw') but returns ENOENT for all other paths including the Windsurf config path. This means the test never exercises the 'detected locally' branch for Windsurf. A comment explaining this intent (or a second test) would help contributors.

Sarah (Non-Technical Decision-Maker)

  • src/proxy/config.ts:933 - 'Open the Cascade panel and verify the server appears with a green status indicator.' — a non-technical user won't know what the Cascade panel is. Should say something like 'Open the Windsurf AI assistant panel (Cascade) and look for a green dot next to your server name.'
  • src/proxy/config.ts:929 - 'Restart Windsurf (Cmd/Ctrl+Q, then reopen).' — keyboard shortcut alone may confuse Windows/Linux users or those unfamiliar with the quit shortcut. A brief fallback like 'or use File > Quit' would help.

The Team (Acquisition Due Diligence)

  • src/proxy/config.ts:695 - PLATFORM_BY_SELECTION is an integer-keyed record that must stay in sync with PLATFORM_LABELS array order and the hardcoded 'if (selection === 5)' branch. This is a three-way coupling that is fragile under future platform additions and has already required renumbering all tests in this PR.
  • src/proxy/config.ts:591 - isWindsurfConnected detects a Multicorn entry by checking url.includes('multicorn'). This is a substring heuristic with no hostname validation, making it easy to produce false positives. Consider matching against the known proxy domain pattern.
  • src/proxy/config.ts:1069 - The fallback '?? "cursor"' on PLATFORM_BY_SELECTION lookup at line 1069 would silently treat an unknown selection as Cursor. Given the selection is validated to 1–5 this can't happen today, but the silent default is a latent bug.
  • src/proxy/__tests__/proxy.edge-cases.test.ts - No test exercises isWindsurfConnected returning true, meaning the 'detected locally' label for Windsurf has no test coverage.

Yuki (International User)

  • src/proxy/config.ts:929 - 'Cmd/Ctrl+Q, then reopen' — the phrase 'reopen' is ambiguous. Does it mean relaunch from the dock, re-open a file, or re-open the app? Prefer 'quit and restart Windsurf'.
  • src/proxy/config.ts:933 - 'verify the server appears with a green status indicator' — 'status indicator' is an idiom. Suggest 'check that your server name is shown with a green circle (●) meaning it is connected'.
  • CHANGELOG.md:14 - 'gave no guidance to first-time users' — changelog is developer-facing so this is fine, but 'first-time users' in a changelog entry is unusual phrasing; 'new users' is more standard and internationally familiar.

Open-Source Readiness Checklist

Code Quality

  • All functions have clear, descriptive names — New functions getWindsurfConfigPath and isWindsurfConnected are clearly named and descriptive.
  • No hardcoded secrets, API keys, internal URLs, or employee names in code or comments — No secrets or API keys hardcoded. URLs like https://app.multicorn.ai/settings#api-keys and https://hosted.proxy.example/mcp are user-facing docs or test fixtures, not internal.
  • No // TODO without a public issue reference — No TODO comments introduced in the diff.
  • No commented-out code blocks — No commented-out code blocks found in the diff.
  • No debug logging (console.log, println) left in — All output uses process.stderr.write which is intentional CLI output, not debug logging.
  • All any types eliminated (TypeScript) — In isWindsurfConnected, casts like as Record<string, unknown> and as Record<string, unknown> | undefined are used without proper type guards. The cast obj['mcpServers'] as Record<string, unknown> | undefined is applied after the typeof check rather than before, and JSON.parse(raw) as Record<string, unknown> is an unchecked cast that could be any JSON value.
  • Error handling is complete — no swallowed exceptions, no empty catch blocks — The catch block in isWindsurfConnected silently returns false for all errors (including unexpected errors beyond ENOENT), which swallows exceptions broadly.
  • No Atlassian-internal references, no proprietary patterns or terminology — No Atlassian-internal references found.

Testing

  • All new code has tests — A new test 'runInit completes Windsurf platform with proxy URL and Windsurf next steps' covers the Windsurf flow. isWindsurfConnected and getWindsurfConfigPath do not have dedicated unit tests visible in the diff, but the integration-style test exercises them indirectly.
  • [~] Coverage meets or exceeds repo minimum — Cannot determine coverage metrics from the diff alone.
  • [~] Tests pass locally and in CI — CI results are not visible in the diff.
  • Edge cases and error paths are tested — No tests for isWindsurfConnected edge cases: malformed JSON, missing mcpServers key, non-object mcpServers, entries without serverUrl, or entries with serverUrl not containing 'multicorn'.
  • [~] No flaky tests — Cannot determine flakiness from the diff alone.

Security

  • No secrets in code, comments, config files, or git history — No secrets found in the diff.
  • All user input is validated — Selection input is validated (parseInt + range check 1-5). Agent name and URL inputs follow existing patterns.
  • [~] Dependencies audited — no known vulnerabilities — No dependency changes in this diff.
  • HTTPS enforced for all external communication — All URLs in the diff use HTTPS.
  • API keys/tokens never logged — The Bearer token (apiKey) is written to process.stderr in the Windsurf snippet (same as Cursor). This is intentional for user setup but means the key appears in terminal output.

Documentation

  • [~] README.md is accurate and up to date — README.md is not modified in this diff; cannot verify if it documents Windsurf support.
  • [~] CONTRIBUTING.md is accurate and up to date — CONTRIBUTING.md not touched in the diff.
  • CHANGELOG.md updated with this change — CHANGELOG.md has been updated with a [0.8.0] entry describing the Windsurf addition.
  • New public APIs have JSDoc/KDoc with examples — getWindsurfConfigPath and isWindsurfConnected both have JSDoc comments with @returns descriptions.
  • [~] Any new config options are documented — The new 'windsurf' platform value in config is described in inline comments but no external docs are visible in the diff.
  • [~] Architecture decisions documented in ADR if significant — No ADR directory changes visible; adding a new platform is incremental rather than a major architectural change.

Open Source Hygiene

  • [~] Licence header present in source files (if required by licence) — Cannot determine licence header requirements from the diff alone; existing files don't show headers in the diff context.
  • [~] CODE_OF_CONDUCT.md present — Not modified or referenced in the diff.
  • [~] Issue templates are current — Not modified in the diff.
  • [~] PR template is current — Not modified in the diff.
  • No internal company references or links — References to https://app.multicorn.ai are to the public product, not internal systems.
  • [~] Package name and description are correct in package.json — package.json not modified in the diff.
  • [~] Repository topics/tags are set on GitHub — Cannot be determined from a code diff.

Advisory only. Does not block merge. Actions logged to Shield as pr_review and oss_check.

@rachellerathbone rachellerathbone merged commit cc6810e into main Apr 12, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant