fix(USE-001): add standardized error codes to four safe-output handlers

[Copilot]

Four safe-output handlers that interact with GitHub were throwing errors without standardized ERR_* prefixes, failing the USE-001 conformance check.

Changes

• add_reviewer.cjs — import ERR_API; prefix the copilot node ID resolution throw
• apply_samples.cjs — import ERR_VALIDATION, ERR_PARSE, ERR_SYSTEM, ERR_API, ERR_CONFIG; prefix all 10 throw sites with the appropriate category
• hide_comment.cjs — import ERR_VALIDATION, ERR_API; prefix all 4 throw sites
• safeoutputs_cli.cjs — import ERR_VALIDATION, ERR_SYSTEM; prefix both throw sites (local CLI execution failures classified as ERR_SYSTEM)
• hide_comment.test.cjs — update one toBe exact-match assertion to include the new ERR_VALIDATION: prefix

Pattern (consistent with assign_to_agent.cjs)

const { ERR_VALIDATION, ERR_API } = require("./error_codes.cjs");

throw new Error(`${ERR_VALIDATION}: comment_id must be a GraphQL node ID string or a positive numeric REST comment ID`);
throw new Error(`${ERR_API}: Failed to resolve GraphQL node ID for comment_id ${commentId}: comment not found or node_id unavailable`);

[Copilot]

Pull request overview

This PR updates several safe-output handler scripts to use standardized ERR_* error code prefixes in thrown errors, addressing the USE-001 conformance check and improving machine-readable error classification across GitHub-integrated operations.

Changes:

• Added ERR_* imports and prefixed thrown errors in four handler/utility scripts (add_reviewer, apply_samples, hide_comment, safeoutputs_cli).
• Updated a hide_comment unit test to match the new prefixed validation error string.

Show a summary per file

File	Description
actions/setup/js/safeoutputs_cli.cjs	Adds standardized error code prefixes to thrown errors from the safeoutputs CLI wrapper.
actions/setup/js/hide_comment.test.cjs	Updates an assertion to include the ERR_VALIDATION: prefix.
actions/setup/js/hide_comment.cjs	Prefixes validation/API thrown errors while resolving comment node IDs.
actions/setup/js/apply_samples.cjs	Prefixes parse/validation/system/config/API thrown errors in the samples replay driver.
actions/setup/js/add_reviewer.cjs	Prefixes a thrown error when failing to resolve the PR node ID via GraphQL.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 5/5 changed files
• Comments generated: 2

[github-actions]

🧠 Matt Pocock Skills Reviewer is reviewing this pull request using Matt Pocock's engineering skills...

[github-actions]

🔍 Design Decision Gate 🏗️ is checking for design decision records on this pull request...

[github-actions]

REQUEST_CHANGES — two semantic error-code bugs and weak test coverage

Blocking issues: ERR_API is used for MCP subprocess/process failures in apply_samples.cjs where the docstring says it means GitHub

🔎 Code quality review by PR Code Quality Reviewer · ⌖ 14 AIC

Comments that could not be inline-anchored

actions/setup/js/apply_samples.cjs:178

Wrong error code: ERR_API applied to an MCP subprocess I/O failure. ERR_API is documented as "GitHub API call failures", but "MCP server closed stdout" is a system-level process event.

<details>
<summary>💡 Suggested fix</summary>

ERR_SYSTEM is reserved for "System and I/O errors" — a subprocess closing stdout mid-stream fits exactly. update_release.cjs already uses errorMessage.startsWith('ERR_CONFIG:') for structural branching, setting precedent for programmatic code parsing.
…

actions/setup/js/apply_samples.cjs:313

ERR_API misapplied to an MCP protocol-level error, not a GitHub API call. "MCP initialize failed" means the JSON-RPC handshake with the local subprocess failed — no GitHub API was invoked yet.

<details>
<summary>💡 Suggested fix</summary>

ERR_SYSTEM covers process/IPC setup failures. ERR_CONFIG would also be defensible if the cause is a misconfigured server. Neither case is a GitHub API failure.

// was: ERR_API
throw new Error(`${ERR_SYSTEM}: MCP initialize failed: ${JSON.str…

</details>

<details><summary>actions/setup/js/hide_comment.test.cjs:180</summary>

**Hardcoded string literal instead of imported constant makes this assertion brittle.** If `ERR_VALIDATION`&#39;s value ever changes, this `toBe` test will silently pass against the stale string.

&lt;details&gt;
&lt;summary&gt;💡 Suggested fix&lt;/summary&gt;

Import the constant and interpolate it, consistent with how production code is written:

```js
const { ERR_VALIDATION } = require(&#39;./error_codes.cjs&#39;);
// ...
expect(result.error).toBe(`${ERR_VALIDATION}: comment_id must be a GraphQL node ID string or a posit…

</details>

<details><summary>actions/setup/js/hide_comment.test.cjs:116</summary>

**Four `toContain` assertions provide zero signal that error code prefixes were actually added.** `toContain(&#39;comment_id is required&#39;)` passes whether the throw says `&quot;comment_id is required&quot;` or `&quot;ERR_VALIDATION: comment_id is required&quot;` or even `&quot;ERR_API: comment_id is required&quot;`.

&lt;details&gt;
&lt;summary&gt;💡 Suggested fix&lt;/summary&gt;

The tests at lines 116, 158, 169, and 194 (which cover the `ERR_VALIDATION` throws added at hide_comment.cjs lines 57, 57, 57, and 73) should be tightened so they&#39;d fa…

</details>

<details><summary>actions/setup/js/add_reviewer.cjs:217</summary>

**No test covers this branch — the `ERR_API:` prefix is untested.** All graphql mocks in `add_reviewer.test.cjs` return a valid `id`; there is no case where `pullRequestId` is null/undefined.

&lt;details&gt;
&lt;summary&gt;💡 Suggested fix&lt;/summary&gt;

Add a test that mocks graphql to return an empty pullRequest node:

```js
it(&#39;should fail with ERR_API when pullRequest node ID is missing&#39;, async () =&gt; {
  mockGithub.graphql.mockResolvedValueOnce({ repository: { pullRequest: {} } });
  const result = await …

</details>

<details><summary>actions/setup/js/add_reviewer.cjs:217</summary>

**No test covers this branch — the `ERR_API:` prefix is untested.** All graphql mocks in `add_reviewer.test.cjs` return a valid `id`; there is no test where `pullRequestId` is null/undefined.

&lt;details&gt;
&lt;summary&gt;💡 Suggested fix&lt;/summary&gt;

Add a case that mocks graphql to return an empty pullRequest node:

```js
it(&#39;should fail with ERR_API when pullRequest node ID is missing&#39;, async () =&gt; {
  mockGithub.graphql.mockResolvedValueOnce({ repository: { pullRequest: {} } });
  const result = await …

</details>

Dismiss

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Completed a pr-finisher pass. Addressed the actionable review feedback in 27896c1 by switching safeoutputs_cli exec-failure error classification from ERR_API to ERR_SYSTEM, and re-ran local validation (make fmt, make lint, make test-unit, make test, make recompile).