Skip to content

fix(USE-001): standardize error codes across 4 safe-output handlers#46565

Merged
pelikhan merged 4 commits into
mainfrom
copilot/use-001-standardize-error-codes
Jul 19, 2026
Merged

fix(USE-001): standardize error codes across 4 safe-output handlers#46565
pelikhan merged 4 commits into
mainfrom
copilot/use-001-standardize-error-codes

Conversation

Copilot AI commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Four safe-output handlers were throwing bare new Error(...) messages without the standardized ERR_* prefixes defined in error_codes.cjs, causing USE-001 conformance failures.

Changes

Each of the four handlers now imports the appropriate constant from ./error_codes.cjs and prefixes its throws accordingly. Existing { cause: err } chaining and message text are preserved.

  • create_code_scanning_alert.cjs — SARIF file-write failure → ERR_SYSTEM
  • load_experiment_state_from_repo.cjs — directory creation and state file write failures → ERR_SYSTEM
  • safe_outputs_bootstrap.cjscreate-pull-request runtime policy violation → ERR_CONFIG
  • safe_outputs_config.cjs — output directory creation failure → ERR_SYSTEM
// Before
throw new Error(`Failed to create directory ${stateDir}: ${String(err)}`, { cause: err });

// After
const { ERR_SYSTEM } = require("./error_codes.cjs");
throw new Error(`${ERR_SYSTEM}: Failed to create directory ${stateDir}: ${String(err)}`, { cause: err });

scripts/check-safe-outputs-conformance.sh now reports [PASS] USE-001: All handlers use standardized error codes.

Add ERR_SYSTEM/ERR_CONFIG prefixes to all throw new Error() calls in:
- create_code_scanning_alert.cjs (file write I/O)
- load_experiment_state_from_repo.cjs (directory/file I/O)
- safe_outputs_bootstrap.cjs (policy enforcement → ERR_CONFIG)
- safe_outputs_config.cjs (directory creation I/O)

Fixes #46550

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Standardize error codes across four safe-output handlers fix(USE-001): standardize error codes across 4 safe-output handlers Jul 19, 2026
Copilot AI requested a review from pelikhan July 19, 2026 07:25
@pelikhan
pelikhan marked this pull request as ready for review July 19, 2026 07:32
Copilot AI review requested due to automatic review settings July 19, 2026 07:32
@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. Test Quality Sentinel skipped.

@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #46565 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).

@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

PR Code Quality Reviewer completed the code quality review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Standardizes error prefixes in safe-output handlers to improve USE-001 conformance and error classification.

Changes:

  • Adds ERR_SYSTEM to filesystem failure messages.
  • Adds ERR_CONFIG to runtime policy violations.
  • Preserves existing messages and error causes.
Show a summary per file
File Description
actions/setup/js/create_code_scanning_alert.cjs Prefixes SARIF write failures.
actions/setup/js/load_experiment_state_from_repo.cjs Prefixes local state I/O failures.
actions/setup/js/safe_outputs_bootstrap.cjs Prefixes runtime policy errors.
actions/setup/js/safe_outputs_config.cjs Prefixes output-directory failures.

Review details

Tip

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

  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Medium

const fs = require("fs");
const path = require("path");
const { getErrorMessage } = require("./error_helpers.cjs");
const { ERR_SYSTEM } = require("./error_codes.cjs");

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look correct and complete. All four handlers now use standardized error codes from error_codes.cjs with appropriate categories (ERR_SYSTEM for I/O, ERR_CONFIG for policy). Existing { cause: err } chaining and message text preserved throughout.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 14.3 AIC · ⌖ 4.32 AIC · ⊞ 5K

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — requesting a change on the log/throw inconsistency in safe_outputs_bootstrap.cjs.

📋 Key Themes & Highlights

Key Themes

  • Log/throw inconsistency (safe_outputs_bootstrap.cjs): debugError logs the unprefixed message while throw wraps it with ERR_CONFIG:, creating a mismatch between log output and the exception message.
  • No regression tests: The PR fixes a conformance gap but adds no unit tests to lock in the new behaviour. A future refactor could silently revert to bare errors without any test failing.

Positive Highlights

  • ✅ Clean, surgical changes — existing message text and { cause: err } chaining are fully preserved.
  • ✅ Consistent use of shared error_codes.cjs constants rather than string literals.
  • ✅ Correct code classification: ERR_SYSTEM for I/O failures, ERR_CONFIG for policy violations.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 24.4 AIC · ⌖ 4.52 AIC · ⊞ 6.7K
Comment /matt to run again

@@ -68,7 +69,7 @@ function enforceCreatePullRequestRuntimePolicy(config, logger) {

const message = `create-pull-request is disabled by runtime policy: ${policyVarName}=false. ` + `Remove safe-outputs.create-pull-request or set ${policyVarName}=true.`;
logger.debugError(message);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The debugError call logs message without the ERR_CONFIG: prefix, but the thrown error includes it — so logs and the exception message will differ, complicating post-hoc debugging.

💡 Suggested fix

Log the prefixed form so that the debug output is consistent with the thrown error:

const message = `create-pull-request is disabled by runtime policy: ${policyVarName}=false. ` +
  `Remove safe-outputs.create-pull-request or set ${policyVarName}=true.`;
const prefixed = `${ERR_CONFIG}: ${message}`;
logger.debugError(prefixed);
throw new Error(prefixed);

When a consumer catches the error and compares it to a log line, both should carry the same ERR_CONFIG: prefix.

@copilot please address this.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue must be fixed before merging

The other four changes (prefixing ERR_SYSTEM / ERR_CONFIG onto file-system error throws) are mechanically correct and consistent with the convention.

Blocking finding

safe_outputs_bootstrap.cjslogger.debugError(message) is called with the raw message before the ERR_CONFIG prefix is prepended to the thrown error. The prefix is supposed to enable structured-log scraping and alerting; logging the event without it defeats that purpose for this code path. See inline comment.

🔎 Code quality review by PR Code Quality Reviewer · 20.2 AIC · ⌖ 4.46 AIC · ⊞ 5.6K
Comment /review to run again

Comments that could not be inline-anchored

actions/setup/js/safe_outputs_bootstrap.cjs:76

Log/error message inconsistency: logger.debugError logs message without the ERR_CONFIG prefix, while the thrown Error prepends it — callers and monitoring rules parsing log output for ERR_CONFIG will miss this event entirely.

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

Apply the prefix before both the log call and the throw so they remain in sync:

const prefixedMessage = `${ERR_CONFIG}: ${message}`;
logger.debugError(prefixedMessage);
throw new Error(prefixedMessage);
`

</details>

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher skill

…API prefix

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

@copilot run pr-finisher skill

Done. Added ERR_API import and wrapped the non-404 rethrow in fetchFileFromBranch with an ERR_API-prefixed error (preserving the original as cause). Committed in 5ab987d.

Pre-existing unrelated test failure: TestDocGo_AnalyzersMatchREADME in pkg/linters (missing trimleftright from README) fails on the base branch before any changes and is not in scope here.

@pelikhan
pelikhan merged commit bd8bf24 into main Jul 19, 2026
8 checks passed
@pelikhan
pelikhan deleted the copilot/use-001-standardize-error-codes branch July 19, 2026 10:34
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.

[Safe Outputs Conformance] USE-001: Standardize error codes across 4 safe-output handlers

3 participants