Sr 7

[rachellerathbone]

What

Brief description of changes

Why

Why this change was needed

How

Brief technical approach

Testing

How to verify the changes

[github-actions]

multicorn-ops review

Persona	Role	Primary	Status	Summary
Jordan	Security Auditor	yes	Concern	API key forwarding via user-controlled base_url and error messages leaking internal details warrant scrutiny.
Priya	Open Source Contributor	yes	Concern	Deleting the only existing extension test without a clear replacement leaves a coverage gap that will confuse new contributors.
Marcus	Design-Conscious Developer	yes	Passed	No UI or frontend rendering changes are present in this diff; only backend routing and config schema changes.
Sarah	Non-Technical Decision-Maker	yes	Concern	The new 'API base URL' config field and changelog text use developer jargon that will confuse a non-technical team admin.
The Team	Acquisition Due Diligence	yes	Concern	Deleting tests for removed code is acceptable, but the user-controlled base_url without validation and the dual-unreleased-version changelog signal process gaps.
Alex	Accessibility Advocate	yes	Passed	No HTML, React, or UI component changes are present; accessibility impact is negligible for this diff.
Yuki	International User	yes	Concern	Several user-facing strings contain idiomatic phrasing and unexplained jargon that may be hard for non-native English speakers to parse.

Concerns

Jordan (Security Auditor)

• manifest.json:30 - MULTICORN_BASE_URL is now user-supplied from user_config.base_url with no validation or allowlist. A malicious or misconfigured value could redirect API key traffic (including Bearer tokens) to an attacker-controlled endpoint.
• manifest.json:44 - The base_url field has no format validation (URL scheme, host, etc.) in the manifest schema. There should be a pattern or validation constraint to prevent arbitrary redirects.
• src/extension/__tests__/proxy-client.test.ts:295 - Test confirms that raw error text from Shield (e.g. 'Action blocked by Multicorn Shield: no access') is surfaced verbatim to the caller. Verify that upstream error messages cannot leak sensitive internal state or stack traces in production responses.
• src/extension/__tests__/proxy-client.test.ts:314 - HTTP 502 body ('bad gateway') is included in the isError content. If the proxy returns a verbose error body, it will be forwarded to the LLM/user. Ensure error body truncation or sanitisation is applied in the real implementation.
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.cjs:107 - The platform value 'claude-code' is appended to a consent URL as a query parameter. If platform is ever derived from user input rather than a hardcoded string, this becomes an open-redirect or parameter-injection risk. Confirm it is always hardcoded.
• pnpm-lock.yaml:1826 - path-to-regexp is bumped from 8.3.0 to 8.4.0 via an override. Confirm the integrity hash (sha512) has been independently verified; the diff shows a hash change and overrides bypass normal resolution, making supply-chain verification critical.

Priya (Open Source Contributor)

• src/extension/__tests__/child-manager.test.ts - The child-manager test file is deleted entirely. The PR description says child processes are no longer spawned, but there is no replacement test that validates the new routing path end-to-end. A contributor cloning this repo cannot verify the old behaviour is preserved.
• CHANGELOG.md:10 - Two changelog sections exist: '[Unreleased]' and '[0.2.2] - Unreleased'. Having two unreleased sections is confusing; a contributor following Keep-a-Changelog conventions will not know where to add entries.
• src/extension/__tests__/proxy-client.test.ts - New proxy-client tests mock global fetch directly. There is no fixture or helper abstraction, making tests harder to maintain. A CONTRIBUTING-friendly pattern would extract a shared mock factory.

Sarah (Non-Technical Decision-Maker)

• manifest.json:44 - The title 'API base URL (optional)' and description 'Defaults to https://api.multicorn.ai when empty. Use this to point at a self-hosted Shield API.' will appear in the Claude Desktop settings UI. A non-technical admin will not understand what a 'base URL' or 'self-hosted API' means. Consider 'Custom server address (leave blank for default)'.
• CHANGELOG.md:10 - Changelog copy references 'proxy configs', 'MCP processes', 'sandbox limits', and 'hosted proxy' without explanation. If this changelog is customer-facing, non-technical readers will not understand what changed or why they should care.

The Team (Acquisition Due Diligence)

• manifest.json:30 - Allowing user_config.base_url to override the API endpoint with no schema-level URL validation is an architectural risk: it breaks the security boundary between user configuration and credential routing.
• CHANGELOG.md - Two '[Unreleased]' blocks exist simultaneously ('[Unreleased]' and '[0.2.2] - Unreleased'). This suggests the release process is manual and error-prone, a signal of process debt for acquirers.
• pnpm-lock.yaml - path-to-regexp override jumps a patch version (8.3.0 -> 8.4.0). Overrides in pnpm bypass normal peer resolution; without a documented reason (CVE fix, etc.) this adds unexplained dependency debt.
• src/extension/__tests__/proxy-client.test.ts - 411 lines of new tests are good signal. However, all tests stub global fetch inline with no shared harness, suggesting no established test infrastructure pattern for HTTP-layer code.

Yuki (International User)

• manifest.json:44 - 'Use this to point at a self-hosted Shield API' — 'point at' is an English idiom. Prefer 'Enter the URL of your self-hosted Shield API server'.
• CHANGELOG.md:13 - 'avoids sandbox limits in Claude Desktop' — 'sandbox limits' is unexplained jargon. Non-native readers cannot act on or understand this without further context.
• src/extension/__tests__/proxy-client.test.ts:163 - The test description 'runs initialize, notification, tools/list, and tools/call over HTTP' is clear, but the error text 'Action blocked by Multicorn Shield: agent does not have write access to Gmail. Configure permissions at https://app.example' mixes product names and a placeholder URL. Real error messages should use consistent, actionable language and real URLs so developers can copy-paste to resolve issues.

Open-Source Readiness Checklist

Code Quality

• All functions have clear, descriptive names — Visible functions and classes (fetchProxyConfigs, buildProxyToolRouter, parseToolsListResult, resultSuggestsConsentNeeded, ProxySession, main, shutdown) have clear, descriptive names.
• No hardcoded secrets, API keys, internal URLs, or employee names in code or comments — The default URL 'https://api.multicorn.ai' and 'https://app.multicorn.ai' appear hardcoded in manifest.json description and CHANGELOG. While the manifest.json change moves MULTICORN_BASE_URL to user_config (good), the fallback default is still baked into description strings and test fixtures use 'https://api.multicorn.ai' and 'https://api.multicorn.ai/api/v1/proxy/config'.
• [~] No // TODO without a public issue reference — Diff is truncated; no TODOs visible in the shown portion.
• No commented-out code blocks — No commented-out code blocks observed in the diff.
• [~] No debug logging (console.log, println) left in — The new proxy-client source file is not shown (truncated); cannot fully verify. Visible test and hook files do not show debug logging.
• All any types eliminated (TypeScript) — In proxy-client.test.ts, cast expressions like 'as { headers?: Record<string, string> } | undefined' suggest the underlying implementation may use loosely typed responses. Cannot confirm all any is eliminated since proxy-client.ts itself is truncated.
• Error handling is complete — no swallowed exceptions, no empty catch blocks — Tests explicitly cover error paths (network failure, HTTP 4xx/5xx, malformed JSON). The test for ProxyConfigFetchError and isError returns indicate errors are surfaced rather than swallowed.
• No Atlassian-internal references, no proprietary patterns or terminology — No Atlassian-specific references found in the diff.

Testing

• All new code has tests — proxy-client.test.ts is added and covers the new proxy-client module well. However, the diff deletes child-manager.test.ts and the child-manager source changes are not visible; it's unclear whether the deleted child-manager is tested elsewhere or removed entirely. The proxy-client.ts source itself is also not shown in the diff, so completeness of test coverage cannot be fully confirmed.
• [~] Coverage meets or exceeds repo minimum — Coverage metrics are not available 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 — Tests cover 401, 429, 502, 503, malformed JSON, invalid rows, duplicate tool names, notification failures, and network errors — good edge case coverage.
• [~] No flaky tests — Cannot determine flakiness from static diff review.

Security

• No secrets in code, comments, config files, or git history — No secrets or API keys are hardcoded. The manifest correctly uses ${user_config.api_key} and ${user_config.base_url}.
• [~] All user input is validated — proxy-client.ts source is truncated; cannot verify input validation for proxy_url, server_name, etc. beyond what tests imply (invalid rows are skipped).
• Dependencies audited — no known vulnerabilities — path-to-regexp is bumped from 8.3.0 to 8.4.0 via an override, which is consistent with patching a known vulnerability in that package.
• HTTPS enforced for all external communication — All URLs in tests and config use https://. The default base URL is https://api.multicorn.ai.
• [~] API keys/tokens never logged — proxy-client.ts source is truncated; cannot confirm keys are never logged within the implementation.

Documentation

• [~] README.md is accurate and up to date — README.md is not included in the diff.
• [~] CONTRIBUTING.md is accurate and up to date — CONTRIBUTING.md is not included in the diff.
• CHANGELOG.md updated with this change — CHANGELOG.md is updated with entries describing the new hosted proxy config loading, removal of child MCP spawning, and the new base_url config option.
• New public APIs have JSDoc/KDoc with examples — New exports (fetchProxyConfigs, ProxySession, buildProxyToolRouter, parseToolsListResult, resultSuggestsConsentNeeded) are visible only in the test file; the source file is truncated. No JSDoc is visible for these public APIs.
• Any new config options are documented — The new base_url config option is documented in manifest.json with a title and description, and mentioned in CHANGELOG.md.
• Architecture decisions documented in ADR if significant — Switching from spawning child MCP processes to a hosted proxy over HTTP is a significant architectural change. No ADR is referenced or added in the diff.

Open Source Hygiene

• [~] Licence header present in source files (if required by licence) — New source file proxy-client.ts is truncated; cannot verify licence header. The test file shown has no licence header, but whether the repo requires them per-file is unknown.
• [~] CODE_OF_CONDUCT.md present — Not modified or referenced in the diff; presence in repo cannot be determined.
• [~] Issue templates are current — Not modified or referenced in the diff.
• [~] PR template is current — Not modified or referenced in the diff.
• No internal company references or links — References are to public-facing domains (multicorn.ai, app.multicorn.ai) with no internal company URLs or tooling references.
• [~] Package name and description are correct in package.json — package.json name/description fields are not shown in the diff portion; only overrides were changed.
• [~] Repository topics/tags are set on GitHub — Cannot be determined from the diff.

---

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