github / github/gh-aw

no-string-fallback-for-non-string-message: false positive on type-narrowed catch-param alias (safeoutputs_cli.cjs:48)

Open
#60,757 0 comments 0 reactions 0 assignees View on GitHub
cookie eslint eslint-factory
Dominant language
Go
Stars
5.1k
Forks
541
Avg merge
5h 48m
Merged PRs (30d)
773

Description

### Summary

`no-string-fallback-for-non-string-message` flags a live, correct call site as if it were the "stringifies the wrong container" bug it targets, because its structural name-comparison doesn't recognize that the alternate's identifier is a locally-declared, type-narrowed alias of the tested chain's root object — not a genuinely different value.

### Live occurrence

`actions/setup/js/safeoutputs_cli.cjs:45-48` (`runSafeOutputsCLI`'s catch block):

```js
} catch (error) {
const err = /** @type {{message?: string, stderr?: string | Buffer}} */ error ?? {};
const stderr = typeof err.stderr === "string" ? err.stderr.trim() : Buffer.isBuffer(err.stderr) ? err.stderr.toString("utf8").trim() : "";
const message = typeof err.message === "string" ? err.message : String(error);
...
```

The rule's `getTypeofMessageCheckChainKey`/`memberChainKey` machinery resolves the tested chain to `"err.message"` and the alternate's argument to `"error"`. Since `"error" !== "err.message"` and `"error" !== "err"` (the container-guard exemption at lines 99-104 of `no-string-fallback-for-non-string-message.ts` also doesn't match, since there's no `typeof error === "object"` guard in the test), this trips `stringifiesContainerInsteadOfMessage`.

But `err` is declared as `const err = /** @type {...} */ error ?? {}` — a type-narrowing alias of the *same* catch-clause binding, not an unrelated container:
- When `error` is truthy (the overwhelmingly common case for a `childProcess.execFileSync` throw), `err === error` by reference, so `String(error)` and `String(err)` produce **identical** output — there is no information loss.
- When `error` is falsy (`null`/`undefined`), `err` becomes `{}` while `error` keeps the original falsy value. `String(error)` then yields `"null"`/`"undefined"` (informative), whereas `String(err)` would yield `"[object Object]"` (the exact failure mode this rule exists to prevent) — i.e. in the one case where the two diverge, the code as written is *more* correct than the rule's suggested fix.

So this is a false positive: the pattern `const alias = /** @type {...} */ original ?? fallback` followed by `typeof alias.message === "string" ? alias.message : String(original)` is not the "stringifies a different container" bug — it's an intentional, more-robust choice — but the rule can't distinguish it from a genuine mismatched-container bug because it only compares identifier/member-chain text, not aliasing relationships.

Confirmed no `eslint-disable` suppression exists near this line, and the rule's `.test.ts` has no case covering this alias shape — this is a live, currently-unaddressed defect in the rule as shipped.

### Suggested fix

When the alternate's chain key doesn't match the tested container, check whether the alternate identifier's declarator initializes it as ` ?? ` (or similar narrowing pattern) — if so, treat it as referring to the same underlying value and skip the report. Alternatively (simpler and sufficient to close this gap), special-case: if the alternate is a bare identifier `X` whose single declarator is `const X = Y ?? ` where `Y` is the tested chain's root object, exempt it.

### Acceptance criteria

- [ ] `const err = error ?? {}; ... typeof err.message === "string" ? err.message : String(error)` (and the `ternary-chain`/optional-chaining variants) no longer trigger `stringifiesContainerInsteadOfMessage`.
- [ ] Existing invalid-case tests (genuinely different, unrelated containers) still report.
- [ ] A regression test is added mirroring the exact `safeoutputs_cli.cjs:45-48` shape.

**References:**
- `eslint-factory/src/rules/no-string-fallback-for-non-string-message.ts`
- `actions/setup/js/safeoutputs_cli.cjs:45-48`

> [!WARNING]
>
> Firewall blocked 1 domain
>
> The following domain was blocked by the firewall during workflow execution:
>
> - `api.anthropic.com`
>
> To allow these domains, add them to the `network.allowed` list in your workflow frontmatter:
>
> ```yaml
> network:
> allowed:
> - defaults
> - "api.anthropic.com"
> ```
>
> See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information.
>
>

> Generated by [🤖 ESLint Refiner](https://github.com/github/gh-aw/actions/runs/34809465962) · claude · agent · 357.2 AIC · ⌖ 7.35 AIC · ⊞ 5.8K · [◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw+is%3Aissue+%22gh-aw-workflow-call-id%3A+github%2Fgh-aw%2Feslint-refiner%22&type=issues)
> - [x] expires on Sep 20, 2026, 9:37 PM UTC-08:00

Contributor guide

Open the contributing guide

Research direction

Start with eslint-factory/src/rules/no-string-fallback-for-non-string-message.ts, especially getTypeofMessageCheckChainKey and memberChainKey, then inspect the rule's .test.ts cases. Reproduce the reported shape from actions/setup/js/safeoutputs_cli.cjs:45-48 and compare it with an unrelated-container case. Done means the alias cases, including the listed chain variants, are accepted while invalid cases still report and a regression test covers the live shape.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, typescript
Domain
testing-qa, tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.