Skip to content

fix(ci): fail loudly instead of silently running full suite on shard-lookup failure - #7864

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
tryeverything24:fix-shard-file-lookup-guard-7767
Jul 21, 2026
Merged

fix(ci): fail loudly instead of silently running full suite on shard-lookup failure#7864
JSONbored merged 1 commit into
JSONbored:mainfrom
tryeverything24:fix-shard-file-lookup-guard-7767

Conversation

@tryeverything24

Copy link
Copy Markdown
Contributor

Closes #7767

Problem

.github/workflows/ci.yml's shard-file lookup step read a shard's file list with:

mapfile -t SHARD_FILES < <(node -e "console.log(JSON.parse(require('fs').readFileSync('shard-assignment.json','utf8'))['${{ matrix.shard }}'].join('\n'))")

mapfile -t VAR < <(cmd) runs cmd in a process-substitution subshell whose exit status mapfile does not propagate -- mapfile itself returns 0 regardless of whether the command inside succeeded. If matrix.shard doesn't match a key present in shard-assignment.json (shard-count change, key-format drift) or the file is missing/corrupt, the node -e one-liner throws and exits non-zero, but the step continues anyway with SHARD_FILES silently empty. vitest then receives no file filter, i.e. "run every test file" -- each of the 3 shard jobs would redundantly run the entire suite instead of its slice, defeating the duration-aware sharding, with no error surfaced anywhere.

This is the same subshell-swallow class fixed for compose_file_args in #7765/#7862.

Fix

Capture the node script's output via a checked command-substitution assignment (if ! VAR="$(cmd)"; then exit 1; fi) instead of mapfile < <(cmd), then mapfile the already-captured string with a here-string. A lookup failure now hits if ! ... directly, prints an ::error:: annotation naming the offending shard, and exits 1 -- failing the job loudly instead of silently running everything.

Diff is scoped to exactly this one step in the one else branch (full-suite sharding path); the sibling scoped-selection (--changed=origin/main) branch is untouched.

Verification

This is a workflow-YAML-only change, explicitly not covered by the Codecov patch gate per the issue. Verified manually rather than via vitest:

  • Extracted the exact old idiom into a standalone script and confirmed it reaches the end with SHARD_FILES empty and exit code 0 even when node -e throws (bad matrix.shard key) -- reproducing the silent-degradation bug described in the issue.
  • Extracted the exact new idiom and confirmed it now exits 1 with an ::error:: line for all three failure modes: a matrix.shard key not present in shard-assignment.json, a corrupt (non-JSON) shard-assignment.json, and a missing shard-assignment.json.
  • Confirmed the happy path is unaffected: valid shard keys with both a single-file and a multi-file shard list populate SHARD_FILES identically to before.
  • npm run actionlint: clean.
  • git diff --check against upstream/main: clean (no trailing-whitespace regressions).

Scope

Single file, single step, +10/-1 lines. No other jobs/steps touched.

…lookup failure

The shard-file lookup used `mapfile -t SHARD_FILES < <(node -e ...)`, which
runs the node one-liner in a process-substitution subshell whose exit code
mapfile does not propagate. If matrix.shard doesn't match a key in
shard-assignment.json, or the file is missing/corrupt, the node script
throws, exits non-zero, but mapfile still returns 0 -- leaving SHARD_FILES
empty and silently making vitest run the entire suite instead of its slice.

Capture the node output via a checked command-substitution assignment
instead (same idiom used to fix compose_file_args in JSONbored#7765), so a lookup
failure now fails the step with an explicit ::error:: annotation and a
non-zero exit.

Closes JSONbored#7767
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.40%. Comparing base (fa21f3e) to head (f6ba77c).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7864   +/-   ##
=======================================
  Coverage   91.40%   91.40%           
=======================================
  Files         730      730           
  Lines       74763    74763           
  Branches    22815    22816    +1     
=======================================
  Hits        68335    68335           
  Misses       5385     5385           
  Partials     1043     1043           
Flag Coverage Δ
control-plane 99.45% <ø> (ø)
rees 88.56% <ø> (ø)
shard-1 56.26% <ø> (ø)
shard-2 51.12% <ø> (-0.01%) ⬇️
shard-3 53.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 21, 2026
@loopover-orb

loopover-orb Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-21 15:46:22 UTC

1 file · 1 AI reviewer · no blockers · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review: This PR changes guardrail-protected path(s): .github/workflows/ci.yml (matched .github/workflows/**).

Review summary
This fixes a real bug: `mapfile -t VAR < <(cmd)` swallows the exit status of the process-substitution subshell, so a throwing `node -e` (bad shard key, missing/corrupt shard-assignment.json) left SHARD_FILES empty with exit 0, silently causing vitest to run the full suite instead of its shard slice. The replacement captures output via a checked `$(...)` assignment, fails loudly with an `::error::` annotation and `exit 1` on failure, then mapfiles the captured string via here-string — this correctly propagates the failure while preserving the original line-splitting behavior. The fix is narrowly scoped to the one affected line/branch and mirrors the same idiom already applied for `compose_file_args` in #7765/#7862, and the PR closes the linked issue #7767.

Nits — 5 non-blocking
  • The `console.log` flagged by the external brief is just the pre-existing `node -e` one-liner being preserved/reformatted, not a debug leftover added by this PR.
  • The inline comment block (lines above the fix) is fairly long for a one-step fix; could be trimmed to a single line referencing the issue/PR for brevity.
  • Consider extracting the shard-lookup-and-capture logic into the same shared helper style used for compose_file_args if there's a third occurrence of this idiom, to avoid future copy-paste drift.
  • Verify (as the description states was done manually) that this exact fix was tested against a real bad matrix.shard value in a scratch workflow run, since this path isn't exercised by vitest/codecov gates.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ⚠️ Gate result — Not blocking (Advisory; not blocking this PR.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #7767
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 60 registered-repo PR(s), 25 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor tryeverything24; Gittensor profile; 60 PR(s), 0 issue(s).
Improvement ℹ️ Insufficient signal risk: clean · value: insufficient-signal · LLM: moderate
Linked issue satisfaction

Addressed
The diff replaces the swallow-prone `mapfile -t SHARD_FILES < <(node -e ...)` with a checked command-substitution assignment that exits 1 with an ::error:: annotation if the node one-liner throws, then mapfiles the captured string via here-string, matching one of the two acceptable fixes described in the issue.

Review context
  • Author: tryeverything24
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: JavaScript, TypeScript, Python, HTML, C++, Java, PHP, C#
  • Official Gittensor activity: 60 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 21, 2026
@JSONbored
JSONbored merged commit 283fb8f into JSONbored:main Jul 21, 2026
12 checks passed
@JSONbored JSONbored added the gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. label Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci.yml's shard-file mapfile silently degrades to "run every test" if the shard-lookup one-liner throws

2 participants