Skip to content

Cl#71

Merged
rachellerathbone merged 48 commits intomainfrom
cl
Mar 22, 2026
Merged

Cl#71
rachellerathbone merged 48 commits intomainfrom
cl

Conversation

@rachellerathbone
Copy link
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

multicorn-ops review

Persona Role Primary Status Summary
Jordan Security Auditor yes Concern Several security-relevant patterns in the changelog warrant deeper inspection before approving for a regulated environment.
Priya Open Source Contributor yes Concern The changelog is detailed but the diff contains only metadata changes; no tests, implementation, or docs are visible, making it impossible to evaluate contribution quality.
Marcus Design-Conscious Developer no Passed No UI files changed in this diff; only changelog and version metadata are touched.
Sarah Non-Technical Decision-Maker no Concern The changelog contains developer-internal jargon that would confuse a non-technical evaluator trying to understand what changed.
The Team Acquisition Due Diligence yes Concern A diff consisting solely of a changelog and version bump for a claimed 0.2.0 release is a significant OSS health and due-diligence red flag.
Alex Accessibility Advocate no Passed No UI or HTML changes in this diff; accessibility cannot be evaluated.
Yuki International User yes Concern Several changelog entries use idioms and unexplained abbreviations that would be unclear to a non-native English reader relying on docs.

Concerns

Jordan (Security Auditor)

  • CHANGELOG.md:22 - Consent marker file is described as preventing repeated browser opens, but if this file is world-readable or predictably named, an attacker could pre-create it to suppress consent prompts entirely. The storage path and permissions of this marker file must be documented and enforced.
  • CHANGELOG.md:30 - Agent name validation regex /^[a-zA-Z0-9_-]+$/ is mentioned but applied only before use in config files. If agent names are also interpolated into shell commands, API paths, or log entries elsewhere, this single boundary check may be insufficient. Need to confirm the validation is enforced at every injection point, not just config writing.
  • CHANGELOG.md:38 - The Claude Desktop wizard auto-writes claude_desktop_config.json and merges config without clobbering existing entries. Auto-writing config files to well-known OS paths is a privilege-escalation risk if the process runs with elevated rights or if a symlink attack is possible. The diff shows no evidence of atomic writes or permission checks.
  • CHANGELOG.md:42 - localhost:8080 API base URL is hardcoded and mapped to localhost:5173 dashboard URL for consent and approvals links. Hardcoded localhost ports in a shipped plugin create a DNS rebinding or SSRF surface if the plugin can be loaded in contexts where those ports are bound by a malicious service. This should be configurable and validated.
  • CHANGELOG.md:24 - PostToolUse hook logs completed tool calls to the Shield audit trail. The diff gives no indication of whether tool call arguments (which may contain secrets, PII, or credentials) are sanitised before logging. In a regulated environment this is a data-leakage risk.

Priya (Open Source Contributor)

  • CHANGELOG.md:9 - The 0.2.0 entry lists 12+ behavioural additions and fixes but the diff only shows CHANGELOG.md and a version bump in package.json. No implementation files, tests, or README updates are included, making it impossible to review or learn from the actual changes as a new contributor.
  • package.json:4 - Version bumped from 0.1.16 to 0.2.0 (semver minor bump) but the changelog describes changes that include breaking behaviour (auto-writing config files, replacing tool mapping strategy). A major version bump or at minimum a clear migration note would help contributors understand the stability contract.

Sarah (Non-Technical Decision-Maker)

  • CHANGELOG.md:22 - Terms like 'PreToolUse hook', 'PostToolUse hook', 'MCP proxy', 'consent marker file', and 'extractServiceFromToolName' are implementation jargon with no plain-English explanation. A non-technical decision-maker reading release notes to evaluate trust in the product will be lost immediately.

The Team (Acquisition Due Diligence)

  • CHANGELOG.md:9 - A 0.2.0 release with 12+ features and fixes should be accompanied by implementation diffs, test additions, and updated docs. A PR that only updates CHANGELOG and package.json suggests either the implementation landed in unreviewed commits or the PR process is not being used as a quality gate, both of which are OSS health concerns.
  • CHANGELOG.md:40 - The MCP proxy tool mapping was wholesale replaced (underscore-split heuristic → explicit lookup table). Replacing a heuristic with a hardcoded table is brittle at scale and will require manual maintenance for every new MCP server. There is no mention of a test suite covering the mapping table, which is a tech-debt signal.
  • CHANGELOG.md:36 - The previous release (0.1.16) was one day before 0.2.0 (2026-03-21 vs 2026-03-22). Rapid sequential releases with large feature sets suggest insufficient stabilisation time and potential quality control gaps.
  • package.json:4 - No lock-file changes are visible in the diff. If new dependencies were introduced for the Claude Code plugin or MCP proxy, their absence here means dependency hygiene cannot be evaluated in this PR.

Yuki (International User)

  • CHANGELOG.md:38 - 'falls back to manual on invalid JSON or user skip' — 'user skip' is informal shorthand. An actionable description such as 'if the user chooses to skip this step' would be clearer and more copy-pasteable for documentation purposes.
  • CHANGELOG.md:42 - 'instead of garbage service names' is slang/idiom. Replace with plain language such as 'instead of incorrect or unrecognised service names' to avoid confusion for international readers.
  • CHANGELOG.md:30 - 'connected checkmark' — the word 'checkmark' may not be universally understood. 'connected status indicator (✓)' with an explicit example character would be clearer in docs and SDK references.

Open-Source Readiness Checklist

Code Quality

  • [~] All functions have clear, descriptive names — No function code is present in this diff; only CHANGELOG.md and package.json are modified.
  • No hardcoded secrets, API keys, internal URLs, or employee names in code or comments — CHANGELOG mentions 'localhost:8080 API base URL' and 'localhost:5173 dashboard URL' hardcoded in the hook code. While this is a changelog description rather than code, it signals hardcoded localhost URLs in the actual implementation.
  • [~] No // TODO without a public issue reference — No source code files are included in this diff.
  • [~] No commented-out code blocks — No source code files are included in this diff.
  • [~] No debug logging (console.log, println) left in — No source code files are included in this diff.
  • [~] All any types eliminated (TypeScript) — No TypeScript source files are included in this diff.
  • [~] Error handling is complete — no swallowed exceptions, no empty catch blocks — No source code files are included in this diff.
  • No Atlassian-internal references, no proprietary patterns or terminology — No Atlassian-internal references detected in the diff.

Testing

  • All new code has tests — Significant new functionality is described in the changelog (PreToolUse/PostToolUse hooks, consent flow, CLI wizard, tool name mapper, agent name validation, etc.) but no test files are included in this diff.
  • [~] Coverage meets or exceeds repo minimum — Cannot determine coverage from this diff.
  • [~] Tests pass locally and in CI — CI results are not visible in this diff.
  • [~] Edge cases and error paths are tested — No test files are included in this diff.
  • [~] No flaky tests — No test files are included in this diff.

Security

  • [~] No secrets in code, comments, config files, or git history — No source or config files included; changelog descriptions do not reveal secrets.
  • All user input is validated — Changelog mentions agent name validation was added (/^[a-zA-Z0-9_-]+$/), implying it was missing before; other user inputs (MCP server command, config merge) may not be validated — cannot confirm from diff alone.
  • [~] Dependencies audited — no known vulnerabilities — No dependency changes visible in this diff beyond a version bump.
  • HTTPS enforced for all external communication — CHANGELOG explicitly mentions localhost:8080 and localhost:5173 URLs hardcoded in the hook, which are HTTP, not HTTPS. While localhost traffic may be intentional, this warrants review.
  • [~] API keys/tokens never logged — No source code included to inspect logging behaviour.

Documentation

  • [~] README.md is accurate and up to date — README.md is not included in this diff; cannot verify accuracy.
  • [~] CONTRIBUTING.md is accurate and up to date — CONTRIBUTING.md is not included in this diff.
  • CHANGELOG.md updated with this change — CHANGELOG.md is clearly updated with entries for [0.2.0] and [0.1.16] describing all changes.
  • [~] New public APIs have JSDoc/KDoc with examples — No source code files included to check for JSDoc.
  • Any new config options are documented — New config options (claude_desktop_config.json format, plugin.json structure, marketplace.json fields) are described in the changelog but it is unclear if they are documented in README or dedicated docs.
  • Architecture decisions documented in ADR if significant — Significant architectural changes (PreToolUse/PostToolUse hooks, consent flow, tool name mapper refactor) are described but no ADR is referenced or included.

Open Source Hygiene

  • [~] Licence header present in source files (if required by licence) — No source files included in this diff to check for licence headers.
  • [~] CODE_OF_CONDUCT.md present — Cannot determine presence from this diff alone.
  • [~] Issue templates are current — Cannot determine from this diff.
  • [~] PR template is current — Cannot determine from this diff.
  • No internal company references or links — No internal company references detected in the diff.
  • Package name and description are correct in package.json — package.json name is 'multicorn-shield' and description is clear and appropriate; version bumped to 0.2.0.
  • [~] Repository topics/tags are set on GitHub — Cannot determine GitHub repository topics from a diff.

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

@rachellerathbone rachellerathbone merged commit df4fd41 into main Mar 22, 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