Follow ups

[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	Fail-closed fix is good, but raw HTTP response bodies are echoed to stderr, and test-only code injection paths exist in production code.
Priya	Open Source Contributor	yes	Concern	Tests are skipped with describe.skip, so the new coverage doesn't actually run in CI, which is misleading for a security-critical change.
Marcus	Design-Conscious Developer	no	Passed	No UI changes in this diff; error messages are for CLI/stderr only and are outside Marcus's evaluation scope.
Sarah	Non-Technical Decision-Maker	no	Passed	No user-facing dashboard or onboarding copy changed; the stderr messages are developer/operator-facing and reasonably plain.
The Team	Acquisition Due Diligence	yes	Concern	Security fix is architecturally correct but test coverage is skipped, test-only env-var hooks are baked into production code, and there are minor tech-debt signals.
Alex	Accessibility Advocate	no	Passed	No UI or HTML changes; all modifications are CLI stderr output and Node scripts, outside accessibility scope.
Yuki	International User	yes	Concern	Error messages are much improved and actionable, but a few Detail lines mix technical jargon with user instructions in ways that could confuse non-native readers.

Concerns

Jordan (Security Auditor)

• plugins/multicorn-shield/hooks/scripts/pre-tool-use.cjs:549 - bodyText is sliced and written to stderr verbatim. If Shield returns a 500 with internal stack traces, database names, or secret values in the body, those will appear in Claude Code's stderr output. Truncation at 500 chars is not sufficient sanitization — consider stripping the body entirely or whitelisting safe fields.
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.cjs:453 - MULTICORN_SHIELD_PRE_HOOK_TEST_SERIALIZE_FAIL injects a malicious toJSON shim into hookPayload.tool_input at runtime in production code. A compromised environment variable could trigger unexpected behaviour. Test-only escape hatches should never ship in production hooks; use a separate test fixture or mock script instead.
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.cjs:501 - MULTICORN_SHIELD_PRE_HOOK_TEST_THROW causes an unconditional throw mid-flow in production code. Same concern as above — env-var-driven code paths in a security hook are a privilege-escalation risk if an attacker can influence the environment.
• plugins/multicorn-shield/hooks/scripts/post-tool-use.cjs - PostToolUse continues to exit 0 (allow) on all error paths, including audit-log write failures. The CHANGELOG notes this is intentional, but a silent audit gap should at minimum be documented as a known limitation for regulated environments. Consider a configurable fail-closed option for PostToolUse as well.
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.cjs - The Detail line in several error messages echoes the approvalsUrl, which contains the approval ID. Depending on the URL scheme, this could leak internal resource identifiers to stderr consumers. Confirm approval IDs are not sensitive or redact them.

Priya (Open Source Contributor)

• src/hooks/claude-code-pre-tool-use.hook.test.ts:110 - describe.skip means none of the 332 lines of new tests execute in CI. The PR title implies a tested security fix, but CI passes vacuously. Either remove the skip or add a clear TODO explaining why it's skipped and what needs to happen before it's enabled.
• src/hooks/claude-code-pre-tool-use.hook.test.ts:61 - Port 61237 is hardcoded for the ECONNREFUSED test. This could collide on a busy CI host. Use port 0 and a refused connection pattern instead.
• .github/workflows/ci.yml - Timeout bumped from 10 to 15 minutes with no explanation in the PR description. If the new tests were unskipped and caused slowness, that context should be documented so future contributors know why the limit is higher.

The Team (Acquisition Due Diligence)

• src/hooks/claude-code-pre-tool-use.hook.test.ts:110 - describe.skip renders the entire new test suite inert. A security patch with zero enforced test coverage is a due-diligence red flag — acquirers will notice that CI green does not mean tests pass.
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.cjs:453 - Production hook contains two env-var-driven test injection paths (MULTICORN_SHIELD_PRE_HOOK_TEST_SERIALIZE_FAIL, MULTICORN_SHIELD_PRE_HOOK_TEST_THROW). This is a code hygiene and attack-surface concern; test seams should not be in the shipped artifact.
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.cjs - .js renamed to .cjs without explanation in the PR description. The rationale (ESM/CJS interop in Claude Code plugin host) should be documented so the next engineer doesn't revert it accidentally.
• package.json - Version bump to 0.2.1 is consistent with the CHANGELOG, which is good hygiene — no concern, noted positively.

Yuki (International User)

• plugins/multicorn-shield/hooks/scripts/pre-tool-use.cjs:566 - 'Check that your Shield API and plugin versions match' — 'versions match' is ambiguous. Does this mean the plugin package version, the API schema version, or the HTTP protocol version? A non-native English speaker may not know what to check. Suggest: 'Check that the Shield plugin and Shield server are both up to date.'
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.cjs:416 - 'Start the tool call again and complete approval when prompted' — 'when prompted' assumes the user knows what prompt to expect. Consider 'Start the tool call again; approve it in the Shield dashboard when the approval request appears.'
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.cjs - Detail lines that echo raw HTTP body text (e.g. 'body=...') will sometimes contain English error prose from the server. If the server returns a non-English or highly technical message, this could be confusing. Labeling it clearly as 'Server response:' rather than 'body=' would help.

Open-Source Readiness Checklist

Code Quality

• All functions have clear, descriptive names — Function names like runPreToolUse, writeConfig, withActionServer, blockedMessage, handlePendingWithConsentAndPoll are all descriptive.
• No hardcoded secrets, API keys, internal URLs, or employee names in code or comments — Test fixtures use placeholder values like 'test-api-key' and '127.0.0.1' localhost addresses only.
• [~] No // TODO without a public issue reference — No TODO comments visible 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 with structured messages; no console.log calls introduced.
• All any types eliminated (TypeScript) — Explicit JSDoc casts like /** @type {Record<string, unknown>} */ are used; no bare 'any' introduced.
• Error handling is complete — no swallowed exceptions, no empty catch blocks — Previously empty catch blocks (/* fail silent */) and silent .catch(() => {}) have been replaced with proper error logging. All catch paths now log to stderr.
• No Atlassian-internal references, no proprietary patterns or terminology — No Atlassian-internal references found.

Testing

• All new code has tests — The test suite is in describe.skip(...), meaning all new tests are skipped and will not run in CI. The fail-closed behaviour changes are not exercised by any running test.
• Coverage meets or exceeds repo minimum — Because all tests are wrapped in describe.skip, coverage for the new fail-closed paths in pre-tool-use.cjs will be zero at runtime.
• [~] Tests pass locally and in CI — Cannot verify CI results from the diff alone, but the skip decorator means the new tests never execute.
• Edge cases and error paths are tested — Tests for ECONNREFUSED, HTTP 401/403/429, serialization failure, and approval timeout are written but all skipped via describe.skip.
• [~] No flaky tests — Cannot determine flakiness from diff alone, but spawnSync with a real HTTP server on a random port has some risk; fast-poll env var mitigates timing issues.

Security

• No secrets in code, comments, config files, or git history — No real secrets present; test values are clearly placeholders.
• All user input is validated — stdin JSON parsing is guarded; tool_input serialization errors are caught and handled; body text is truncated before logging.
• [~] Dependencies audited — no known vulnerabilities — Cannot assess from the diff alone.
• [~] HTTPS enforced for all external communication — The hook script uses whatever baseUrl is in config; tests use HTTP localhost. Production enforcement cannot be determined from the diff alone.
• API keys/tokens never logged — API keys are not logged in any of the new error messages; only error messages and HTTP status codes are written to stderr.

Documentation

• [~] README.md is accurate and up to date — No README changes in the diff; cannot determine if updates are needed.
• [~] CONTRIBUTING.md is accurate and up to date — Not touched in this diff.
• CHANGELOG.md updated with this change — CHANGELOG.md has a new [0.2.1] entry describing the fail-closed security fix.
• [~] New public APIs have JSDoc/KDoc with examples — No new public APIs introduced; changes are internal hook scripts.
• Any new config options are documented — Two new environment variables MULTICORN_SHIELD_PRE_HOOK_TEST_FAST_POLL and MULTICORN_SHIELD_PRE_HOOK_TEST_SERIALIZE_FAIL and MULTICORN_SHIELD_PRE_HOOK_TEST_THROW are introduced with no documentation in README or CHANGELOG about their existence or purpose.
• [~] Architecture decisions documented in ADR if significant — Fail-closed behaviour change is noted in CHANGELOG; whether a formal ADR is required depends on project conventions not visible in the diff.

Open Source Hygiene

• [~] Licence header present in source files (if required by licence) — Cannot determine if licence headers are required; none are visible in the script files shown.
• [~] CODE_OF_CONDUCT.md present — Not touched in this diff; cannot verify presence.
• [~] Issue templates are current — Not touched in this diff.
• [~] PR template is current — Not touched in this diff.
• No internal company references or links — References are to 'Shield dashboard' generically; no internal URLs or company-specific links visible.
• Package name and description are correct in package.json — Name is 'multicorn-shield', description is present and reasonable, version bumped to 0.2.1 consistently with CHANGELOG.
• [~] Repository topics/tags are set on GitHub — Cannot determine from diff alone.

---

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