Skip to content

Flow updates#93

Merged
rachellerathbone merged 9 commits intomainfrom
flow-updates
Apr 11, 2026
Merged

Flow updates#93
rachellerathbone merged 9 commits intomainfrom
flow-updates

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 yes Concern API key is passed as CLI flag (visible in process list) and logged at debug level; error messages now omit distinguishing detail which could hinder legitimate debugging.
Priya Open Source Contributor yes Concern Good test coverage additions and clear changelog, but the isDirectRun heuristic is fragile and could break contributors running the CLI in non-standard ways.
Marcus Design-Conscious Developer no Passed No UI changes in this diff; CLI and SDK only.
Sarah Non-Technical Decision-Maker no Concern The new error message is clearer than before, but the terminal output still uses developer jargon that would confuse a non-technical team member trying to set up the proxy.
The Team Acquisition Due Diligence yes Concern Solid incremental improvement but the isDirectRun guard, untrimmed env var, and loss of structured error categorisation are tech-debt signals worth noting before acquisition.
Alex Accessibility Advocate no Passed No UI or DOM changes in this diff; accessibility concerns are not applicable.
Yuki International User yes Concern Most messages are clear and actionable, but the SDK error URL is incomplete and one error message bundles too many options without a recommended starting point.

Concerns

Jordan (Security Auditor)

  • bin/multicorn-proxy.ts:339 - API key passed via --api-key CLI flag will be visible in /proc//cmdline, ps output, and shell history on any multi-user or shared system. The env var path is safer; the flag path needs a prominent security warning in docs and help text.
  • bin/multicorn-proxy.ts:337 - logger.debug('Using API key from --api-key flag.') is safe, but confirm the logger never emits the key value itself at any level. The raw apiKey string flows into ProxyConfig and from there into HTTP Authorization headers — trace that path to ensure no accidental debug-log of the header value.
  • bin/multicorn-proxy.ts:345 - process.env['MULTICORN_API_KEY'] is read without sanitisation. A trailing newline (common in .env tooling) would produce an invalid key that silently falls through to the config file rather than warning the user. Trim the value before use.
  • src/multicorn-shield.ts:730 - All validation failures now surface an identical generic message. This is good for not leaking key material, but it removes the ability for an operator to distinguish 'key was undefined' from 'key has wrong prefix' in audit logs. Consider logging the category internally (not to user output) for incident response.
  • src/multicorn-shield.ts:718 - PLACEHOLDER_KEYS is a hardcoded Set with a single entry. If the placeholder changes or new ones are added across SDK versions, old builds silently accept them. Consider a prefix+length check instead of an allowlist of bad values, or at minimum document the maintenance obligation.
  • bin/multicorn-proxy.ts:408 - The isDirectRun heuristic checks import.meta.url against hardcoded filename suffixes (.js/.ts). This is a fragile guard; if the bundled output filename changes, main() will not run in production. More critically, the VITEST env var check means any test that forgets to set VITEST could invoke main() and make real network calls.

Priya (Open Source Contributor)

  • bin/multicorn-proxy.ts:403 - The isDirectRun block uses four different URL-matching heuristics including hardcoded filename strings. This will confuse contributors debugging why main() does or doesn't run, and is likely to break under bundlers or alternative package managers. A standard __filename === process.argv[1] pattern (or a separate entrypoint file) would be more idiomatic and self-explanatory.
  • src/proxy/__tests__/proxy.cli-api-key.test.ts:1 - The test file is truncated in the diff so full coverage cannot be confirmed. Ensure the happy-path test for --api-key appearing before --wrap (not just after) is present, as this is a distinct parser code path.
  • CHANGELOG.md:8 - Minor: the changelog date is 2026-04-11, which is in the future relative to typical review timelines. Confirm this is intentional and not a typo that will confuse contributors checking version history.

Sarah (Non-Technical Decision-Maker)

  • bin/multicorn-proxy.ts:360 - The error 'No API key found. Provide one via the --api-key flag, the MULTICORN_API_KEY environment variable, or run npx multicorn-proxy init...' uses three different configuration concepts in one sentence. A non-developer seeing this would not know where to start. Consider leading with the simplest action: 'Run npx multicorn-proxy init to get started.' and list advanced options below.
  • src/multicorn-shield.ts:722 - Error message 'Invalid Multicorn Shield API key. Get your key at https://app.multicorn.ai/settings' is user-facing but drops the user at a generic settings page with no anchor. The URL should include #api-keys as done elsewhere in this PR to land users in the right place.

The Team (Acquisition Due Diligence)

  • bin/multicorn-proxy.ts:403 - The isDirectRun guard is a code smell indicating the module boundary between 'CLI entrypoint' and 'library' is not cleanly separated. For a project at this stage, the entrypoint should be a thin wrapper (e.g. bin/run.ts) that imports and calls main(), making the guard unnecessary and eliminating the fragile heuristic.
  • bin/multicorn-proxy.ts:345 - No trimming of MULTICORN_API_KEY before use. .env files and some CI systems append newlines; this will produce a silent misconfiguration that is hard to diagnose.
  • src/multicorn-shield.ts:718 - Collapsing all validation errors to a single undifferentiated message removes structured error information. For a library intended for enterprise/regulated environments, callers (including automated pipelines) benefit from being able to programmatically distinguish error categories. Consider an error code or subclass.
  • src/proxy/__tests__/proxy.cli-api-key.test.ts:1 - 476-line test file is truncated in the diff. Cannot confirm coverage of the priority ordering edge cases (e.g. both CLI flag and env var set simultaneously, or env var set to whitespace only). These boundary cases are critical for the stated priority contract.
  • bin/multicorn-proxy.ts:325 - resolveWrapConfig is exported and takes a ProxyLogger but process.exit(1) is called directly inside it. This makes the function hard to unit test without mocking process.exit and couples library logic to process lifecycle. Throw an error instead and let the caller handle exit.

Yuki (International User)

  • src/multicorn-shield.ts:722 - INVALID_KEY_MESSAGE points to 'https://app.multicorn.ai/settings' without the '#api-keys' fragment. A user following this URL lands on a general settings page and must search for the API keys section. Use 'https://app.multicorn.ai/settings#api-keys' for consistency with the rest of this PR and to reduce confusion for non-native English speakers navigating an unfamiliar UI.
  • bin/multicorn-proxy.ts:360 - The 'No API key found' error lists three options without indicating which is recommended for a first-time user. Non-native English readers may not infer that 'npx multicorn-proxy init' is the easiest path. Add a cue like '(recommended for first-time setup)' after that option.
  • CHANGELOG.md:12 - 'Resolves with priority' is slightly ambiguous phrasing for non-native readers. 'Key is read from the first available source in this order:' would be clearer.

Open-Source Readiness Checklist

Code Quality

  • All functions have clear, descriptive names — New functions resolveWrapConfig, parseArgs, deriveAgentName, validateApiKey, resolveWrapAgentName are all clearly named.
  • No hardcoded secrets, API keys, internal URLs, or employee names in code or comments — No real secrets present. URLs reference app.multicorn.ai which appears to be the product's own domain. Placeholder key 'mcs_your_key_here' is explicitly used as a test/doc example and is now rejected by validation. VALID_KEY in tests is a clearly fake test value.
  • [~] No // TODO without a public issue reference — No TODO comments visible in the diff; cannot confirm absence in the full codebase.
  • No commented-out code blocks — No commented-out code blocks observed in the diff.
  • No debug logging (console.log, println) left in — Only logger.debug calls used, no console.log present in the diff.
  • All any types eliminated (TypeScript) — 'config.apiKey as unknown' and '{ apiKey: undefined } as unknown as MulticornShieldConfig' casts appear; validateApiKey now takes 'unknown' intentionally. The as-unknown casts are used to test runtime behavior, which is acceptable, but 'as unknown as MulticornShieldConfig' in tests bypasses type safety.
  • Error handling is complete — no swallowed exceptions, no empty catch blocks — process.exit(1) is used after error writes; resolveWrapConfig properly surfaces errors; no empty catch blocks visible.
  • No Atlassian-internal references, no proprietary patterns or terminology — No Atlassian references found in the diff.

Testing

  • All new code has tests — New file proxy.cli-api-key.test.ts tests parseArgs and resolveWrapConfig. New SDK validation cases are covered in multicorn-shield.test.ts.
  • [~] Coverage meets or exceeds repo minimum — Coverage metrics cannot be determined from the diff alone.
  • [~] Tests pass locally and in CI — CI results not visible in the diff.
  • Edge cases and error paths are tested — Tests cover undefined apiKey, non-string apiKey, placeholder key, empty key, wrong prefix, too short — good edge case coverage.
  • [~] No flaky tests — Cannot determine flakiness from diff alone; tests use vi.fn() mocks which appear deterministic.

Security

  • No secrets in code, comments, config files, or git history — No real secrets detected. Test keys are clearly fake (mcs_testkey123456).
  • All user input is validated — API key input is validated via validateApiKey with type, prefix, length, and placeholder checks before use.
  • [~] Dependencies audited — no known vulnerabilities — No dependency changes in this diff.
  • HTTPS enforced for all external communication — All URLs in the diff use https://. isAllowedShieldApiBaseUrl check is referenced but not shown — assumed to enforce this.
  • API keys/tokens never logged — logger.debug('Using API key from --api-key flag.') and 'Using API key from MULTICORN_API_KEY environment variable.' log messages do not log the key value itself — this is fine. However, there is no explicit check that the key value is not logged elsewhere in resolveWrapConfig. Based on visible code, the key itself is not logged, so this is a marginal pass, but worth noting.

Documentation

  • [~] README.md is accurate and up to date — README.md not included in the diff; cannot verify.
  • [~] CONTRIBUTING.md is accurate and up to date — CONTRIBUTING.md not included in the diff.
  • CHANGELOG.md updated with this change — CHANGELOG.md is updated with a detailed 0.7.0 entry covering all changes in this PR.
  • New public APIs have JSDoc/KDoc with examples — resolveWrapConfig has a JSDoc comment. The CliArgs.apiKey field has an inline doc comment. The constructor JSDoc examples are updated to use a non-placeholder key.
  • Any new config options are documented — New --api-key flag is documented in printHelp() output and in CHANGELOG.md. MULTICORN_API_KEY env var is mentioned in CHANGELOG and error messages.
  • [~] Architecture decisions documented in ADR if significant — No ADR directory visible in the diff; priority resolution logic is a notable design decision but may not rise to ADR level.

Open Source Hygiene

  • [~] Licence header present in source files (if required by licence) — No licence headers visible in the diff; cannot determine if they are required without seeing other source files.
  • [~] CODE_OF_CONDUCT.md present — Not present in the diff; cannot confirm existence.
  • [~] Issue templates are current — Issue templates not included in the diff.
  • [~] PR template is current — PR template not included in the diff.
  • No internal company references or links — All URLs reference multicorn.ai, which is the project's public domain. No internal company links detected.
  • [~] Package name and description are correct in package.json — package.json not included in the diff.
  • [~] Repository topics/tags are set on GitHub — Cannot be verified from a diff.

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

@rachellerathbone rachellerathbone merged commit acf4c87 into main Apr 11, 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