Skip to content

fix: correctly handle zero exit code to avoid false positives on "failed" in hasCommandFailed - #1340

Open
awhite0030 wants to merge 5 commits into
Nano-Collective:mainfrom
awhite0030:jules-14333065988853829781-72750a9d
Open

awhite0030 wants to merge 5 commits into
Nano-Collective:mainfrom
awhite0030:jules-14333065988853829781-72750a9d

Conversation

@awhite0030

@awhite0030 awhite0030 commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Description

Root cause: hasCommandFailed incorrectly evaluates to a failure when exitCode === 0 if the command output contains substring regexes from Strategy 2 like failed or cannot (e.g., "0 failed" in an npm install log).
Fix: Added explicit checks inside the exit code 0 block. It will now only trigger a failure if it finds critical explicit strings like error: or fatal, but explicitly ignores patterns prone to false positives such as failed or cannot. Otherwise, it returns false.
Validation:

$ pnpm test:ava source/commands/update.spec.tsx
  27 tests passed
$ pnpm run build && pnpm test:format && pnpm test:lint && pnpm test:types && pnpm test:knip
(all passed cleanly)

Fixes #1304

Fixes #1304

Type of Change

  • Bug fix
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Docs-only or internal chores need no changeset (or run pnpm changeset --empty to note that intentionally).

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Checklist

  • If this was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md)

actions-user and others added 5 commits September 12, 2026 02:32
…ed in update command

Ensure that hasCommandFailed correctly ignores benign strings like 'failed' when the command successfully exits with 0. However, it still catches true explicit failures if the output contains messages like 'error:' or 'fatal' despite the 0 exit code.
@github-actions

Copy link
Copy Markdown
Contributor

nc-review: needs work — 1 blocking, 2 important, 1 nit

@awhite0030 — there is a blocking item below.

Fixes a real bug in hasCommandFailed (false-positive failure when exit code is 0 but output contains 'failed'/'cannot' substrings), and the test added covers the regression. However, the PR mixes in five auto-generated badge SVG rewrites — including stars.svg pointing to the contributor's own fork — that have nothing to do with the bug and one of which would be visually wrong if merged. The same bug is being fixed by open PR #1331, so this is a duplicate of work in flight.

Possible duplicate of #1331 — worth checking before going further.

🔴 blocking · duplicate · source/commands/update.tsx:26

Open PR #1331 (fix(commands): don't report /update failure when the command exits 0) fixes the exact same bug in the exact same function for the exact same issue (#1304). Both diffs modify hasCommandFailed in source/commands/update.tsx and add a near-identical AVA test. Before this PR can merge, the two need to be reconciled — pick one approach, or the later one to merge will collide with the earlier one. Coordinate with @niukanen1 in the issue thread.

🟠 important · scope · badges/stars.svg:1

The badge rewrites are unrelated to the bug fix and are produced by the Update Status Badges workflow on a daily schedule. More importantly, stars.svg was rewritten to point at the contributor's fork (github.com/awhite0030/nanocoder) and shows STARS: 0. Merging this would replace the canonical github.com/Nano-Collective/nanocoder badge in README.md with a broken fork URL. The other four badge SVGs (coverage, forks, repo-size, npm-downloads) carry stale local stats for the same reason. Drop all five badge changes from this PR and let CI regenerate them on the canonical repo.

🟠 important · changeset · .changeset/fix-has-command-failed-exit-0.md

The changeset filename and contents are not visible on disk (PR-only file), so this is a check against the diff context. The filename pattern matches other accepted changesets and the package name must be @nanocollective/nanocoder to resolve against the workspace — a wrong name passes the file-presence check and breaks release-prepare. Confirm the changeset body names that package explicitly and credits the linked issue.

⚪ nit · tests · source/commands/update.spec.tsx:158

The new test ('detects success via exit code 0 even with false positive words') covers the regression, and the pre-existing 'detects an error before a later "0 errors" summary' test at line 188 covers the new Strategy-1-with-error branch. Coverage is adequate. Minor: the new test's title would read more clearly as 'detects success via exit code 0 even when output contains the word "failed"' — the current wording is vague about what the false positive is.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:needs-work nc-review found blocking findings label Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:needs-work nc-review found blocking findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] hasCommandFailed in /update reports failure on exit code 0 if stdout/stderr contains words like "failed"

2 participants