fix fail closed

[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	Passed	Critical security improvement: switching from fail-open to fail-closed on API errors eliminates a significant trust bypass vector.
Priya	Open Source Contributor	yes	Concern	Behavior change from fail-open to fail-closed is a breaking change for existing users but is not documented in a CHANGELOG, README, or migration note visible in the diff.
Marcus	Design-Conscious Developer	no	Passed	No UI changes in this diff; no design concerns to raise.
Sarah	Non-Technical Decision-Maker	no	Concern	The product now silently blocks work when its own backend is down, which could confuse non-technical team members who see their tools stop working with no clear remediation step.
The Team	Acquisition Due Diligence	yes	Concern	The security posture improvement is sound, but the absence of tests for the new exit-code paths and no visible CHANGELOG entry are tech-debt signals that would concern a due-diligence review.
Alex	Accessibility Advocate	no	Passed	No UI or DOM changes in this diff; no accessibility concerns.
Yuki	International User	yes	Concern	Error messages are clearer than before but still lack actionable next steps, and raw exception text appended inline may be confusing to non-native English readers.

Concerns

Jordan (Security Auditor)

• plugins/multicorn-shield/hooks/scripts/pre-tool-use.js:487 - Error messages now include the raw exception message (${msg}) which could leak internal network topology, service URLs, or credential fragments into stderr logs. Consider sanitizing or truncating the message before surfacing it.
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.js - The fail-open carve-out for 'no config file, no API key' must be verified to be airtight. If an attacker can delete or corrupt the config file at runtime, they could force fail-open behavior. Confirm config is loaded and validated once at startup and the result is immutable for the lifecycle of the process.

Priya (Open Source Contributor)

• plugins/multicorn-shield/hooks/scripts/pre-tool-use.js - No CHANGELOG or README update visible in the diff. New contributors and existing users won't know that tool calls now block instead of pass when Shield is unreachable — this is a significant behavior change that needs prominent documentation.
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.js - No new tests appear to be added covering the fail-closed code paths (exit code 2 on API error, unexpected response, ambiguous status, non-201 HTTP). Without tests, a future refactor could silently revert to fail-open.

Sarah (Non-Technical Decision-Maker)

• plugins/multicorn-shield/hooks/scripts/pre-tool-use.js:487 - The error message 'Action blocked: Shield API unreachable, cannot verify permissions' tells users what happened but not what to do. Non-developers will not know whether to retry, contact an admin, or check a status page. Add a short actionable hint, e.g. 'Contact your administrator or check Shield service status.'

The Team (Acquisition Due Diligence)

• plugins/multicorn-shield/hooks/scripts/pre-tool-use.js - All four fail-closed branches (API unreachable, unexpected response, ambiguous status, non-201 HTTP) changed exit codes but no test coverage is visible in the diff. This is a critical control path; missing tests are a reliability and auditability red flag.
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.js - The comment in the header now describes two distinct modes (fail-closed on API errors after config load; fail-open if unconfigured). The boundary between these modes should be enforced with a unit test and clearly documented, otherwise the dual-mode logic is a latent correctness risk.
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.js - No CHANGELOG or semver bump visible. A fail-open → fail-closed switch is a breaking behavioral change and should be a major or at minimum minor version bump with a migration guide.

Yuki (International User)

• plugins/multicorn-shield/hooks/scripts/pre-tool-use.js:487 - 'cannot verify permissions' is abstract. A non-native English reader may not know what to do next. Suggest adding a concrete action such as 'Check that the Shield service is running and reachable, then retry.'
• plugins/multicorn-shield/hooks/scripts/pre-tool-use.js - Appending raw exception messages (${msg}) inline after a period creates inconsistent sentence structure and may produce machine-generated text that is hard to parse. Consider a labeled format: 'Detail: ' on a separate line for clarity.

Open-Source Readiness Checklist

Code Quality

• All functions have clear, descriptive names — No new functions introduced; existing structure unchanged.
• No hardcoded secrets, API keys, internal URLs, or employee names in code or comments — No secrets or internal URLs visible in the diff.
• No // TODO without a public issue reference — No TODO comments introduced in the diff.
• No commented-out code blocks — No commented-out code blocks in the diff.
• No debug logging (console.log, println) left in — Uses process.stderr.write for structured error messages, not debug logging.
• [~] All any types eliminated (TypeScript) — File is plain JS with JSDoc; no TypeScript strict typing to evaluate beyond existing JSDoc cast.
• Error handling is complete — no swallowed exceptions, no empty catch blocks — All catch blocks now write to stderr and exit with code 2 (fail-closed) instead of silently allowing.
• No Atlassian-internal references, no proprietary patterns or terminology — No Atlassian-internal references visible in the diff.

Testing

• [~] All new code has tests — No test files included in the diff; cannot confirm coverage from diff alone.
• [~] Coverage meets or exceeds repo minimum — Cannot determine from diff alone.
• [~] Tests pass locally and in CI — CI results not visible in the diff.
• Edge cases and error paths are tested — The diff changes all error/ambiguous paths from fail-open to fail-closed (exit 2). These are significant behavioral changes that should have corresponding test updates, but no test changes are present in the diff.
• [~] No flaky tests — Cannot determine from diff alone.

Security

• No secrets in code, comments, config files, or git history — No secrets introduced.
• All user input is validated — No new user input paths introduced.
• [~] Dependencies audited — no known vulnerabilities — No dependency changes in the diff.
• [~] HTTPS enforced for all external communication — Network call logic not changed in this diff.
• API keys/tokens never logged — Error messages log only the exception message, not tokens or keys.

Documentation

• README.md is accurate and up to date — The fail-open vs fail-closed behavior is a significant behavioral change. README/docs should reflect the new default behavior, but no documentation update is visible in the diff.
• [~] CONTRIBUTING.md is accurate and up to date — No CONTRIBUTING.md changes in diff; unclear if update needed.
• CHANGELOG.md updated with this change — No CHANGELOG update visible in the diff despite a breaking behavioral change (fail-open → fail-closed).
• New public APIs have JSDoc/KDoc with examples — No new public APIs introduced.
• Any new config options are documented — No new config options introduced.
• Architecture decisions documented in ADR if significant — Switching from fail-open to fail-closed is a significant security architecture decision that warrants an ADR, but none is visible in the diff.

Open Source Hygiene

• [~] Licence header present in source files (if required by licence) — Cannot determine licence header requirement from diff alone.
• [~] CODE_OF_CONDUCT.md present — Not determinable from diff alone.
• [~] Issue templates are current — Not determinable from diff alone.
• [~] PR template is current — Not determinable from diff alone.
• No internal company references or links — No internal references visible in the diff.
• [~] Package name and description are correct in package.json — No package.json changes in the diff.
• [~] 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.