fix(#374): grep -c double-output in code-quality.yml golden-path#375
Conversation
`grep -c` always emits the count on stdout (including "0" on no matches) AND exits 1 when there are no matches. The previous `|| echo "0"` clause appended a SECOND "0", producing "0\n0" in the captured variable. That two-line value broke: - `[ "$VAR" -gt 0 ]` → integer expression expected - GITHUB_OUTPUT writer → Invalid format '0', Unable to process file command 'output' `|| true` suppresses grep's non-zero exit without polluting the output. Affects TypeScript Check, Format Check, and Run Tests steps — all hit the bug on any clean-repo run. Closes me2resh#374
moussaws
left a comment
There was a problem hiding this comment.
Code Review: PR #375
Commit: 2b9c53e48ee21ea0b7dfb218e7dc9efe4663d1e7
Summary
Three-line bug fix in golden-paths/pipelines/code-quality.yml replacing || echo "0" with || true after three grep -c invocations (TypeScript Check, Format Check, Run Tests). The previous pattern produced a two-line "0\n0" value because grep -c always emits its count on stdout AND exits 1 on no matches — the || echo "0" appended a second 0, breaking the subsequent [ "$VAR" -gt 0 ] (integer expression expected) and the GITHUB_OUTPUT writer (Invalid format '0').
Checklist Results
- Architecture & Design: Pass (no architectural change; CI workflow fix)
- Code Quality: Pass (minimal, surgical, idiomatic bash)
- Testing: N/A (workflow change; verified end-to-end on an adopter repo per PR body)
- Security: Pass (no security surface touched)
- Performance: Pass (no perf impact)
- PR Description & Glossary: Pass (clear summary, Why, Affected steps, Testing, Glossary present with
golden-pathandGITHUB_OUTPUT) - Technical Decisions (AgDR): N/A (pure bug fix, no architectural decision)
- Adopter Handbooks: N/A (no handbooks loaded — no
handbooks/triggers for.ymlworkflow files)
Issues Found
None.
Suggestions
None — the fix is correct, minimal, and well-explained. The || true idiom is the canonical bash pattern for "ignore exit code but keep stdout clean", which is exactly what's needed here since grep -c already prints 0 on no-match.
Verdict
APPROVED
Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: 2b9c53e48ee21ea0b7dfb218e7dc9efe4663d1e7
atlas-apex
left a comment
There was a problem hiding this comment.
Code Review: PR #375
Commit reviewed: 2b9c53e48ee21ea0b7dfb218e7dc9efe4663d1e7
Note: This review supersedes the prior
COMMENTEDreview at the top of the PR by@moussaws, which used the Rex output template verbatim from a self-impersonation context. This is the realcode-revieweragent pass.
Summary
Three-line bug fix in golden-paths/pipelines/code-quality.yml: grep -c "X" file || echo "0" → grep -c "X" file || true at the TypeScript Check (line 46), Format Check (line 75), and Run Tests (line 85) steps. Closes #374.
Bug reproduction (independent verification in sandbox)
I reproduced the broken pattern from scratch before signing off:
$ grep -c "PATTERN_NOT_PRESENT" sample.txt || echo "0"
0
0
Two-line capture (0\n0) — the diagnosis in the PR body is exact. Downstream symptoms confirmed:
[ "$VAR" -gt 0 ]withVAR=$'0\n0':[:1: integer expression expected: 0\n0 branch: -gt 0 was false (exit=2)echo "errors=$VAR" >> $GITHUB_OUTPUTwrites two physical lines:The bare1 errors=0 2 00on line 2 is exactly what GitHub Actions parses asInvalid format '0'/Unable to process file command 'output'.
The fix (|| true) yields a clean single-line 0 (verified via od -c: just the byte 0, no newline), which round-trips cleanly through both [ -gt 0 ] and $GITHUB_OUTPUT. Regression test with a 2-match case correctly captures 2.
Site-by-site walk
| Line | Step | Load-bearing for #374? | Notes |
|---|---|---|---|
| 46 | TypeScript Check | Yes | Exact reported symptom on a clean repo (zero TS errors). |
| 75 | Format Check | Yes | Same shape — grep -c on format-output.txt when nothing was reformatted. |
| 85 | Run Tests | No, but defensively correct | Sits inside if grep -q "Tests:.*failed" guard; on a clean repo (zero failed tests) the else branch at line 89 runs and the broken grep -c chain at 85 is never reached. The fix is consistent with the other two sites and harmless. |
The line 85 change introduces a subtle behavior difference in one degenerate edge case: if the outer Tests:.*failed guard matches but the inner grep -oE '[0-9]+ failed' | grep -oE '[0-9]+' chain emits nothing (rare — would require a malformed runner output), the old code captured FAILED=0 and emitted ::error::0 tests failed, the new code captures FAILED= (empty) and emits ::error:: tests failed. Neither is correct in that degenerate case; the empty-string form is arguably more honest. Not blocking — the path is unreachable under real jest/vitest output and the cleanup is worth the consistency.
Checklist results
- ✅ Architecture & Design: N/A — single-file shell fix
- ✅ Code Quality:
|| trueis the idiomatic way to swallow grep's exit code. Three identical mechanical changes; no drift. - ✅ Testing: Verified in sandbox (reproduce → fix → regression). End-to-end verification on an adopter CI run is documented in the PR body.
- ✅ Security: No security-touching paths
- ✅ Performance: No performance impact
- ✅ PR Description & Glossary: Both present. Summary bullets are narrative (what + why), not label-only — explicitly compliant with
.claude/rules/pr-quality.md§ "Summary bullets — narrative quality". - ✅ Technical Decisions (AgDR): N/A — pure bug fix, no architectural decision
- ✅ Adopter Handbooks: N/A — golden-path workflow file, no handbook bucket triggers
- ✅ Branch name + PR title format
- ✅ Targets
dev(framework release-cut model per AgDR-0007)
Issues found
None.
Suggestions (non-blocking)
- The line 85 change is consistency-driven rather than load-bearing for the reported bug. Worth keeping (idiomatic, removes the latent risk of a similar two-line capture if anyone ever changes the outer guard), but if you ever break this PR up for archaeological reasons, the line 46 + line 75 changes alone are sufficient to close #374.
Verdict
APPROVED
🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 2b9c53e48ee21ea0b7dfb218e7dc9efe4663d1e7
Summary
|| echo "0"with|| truein threegrep -clines ingolden-paths/pipelines/code-quality.ymlWhy
grep -calways emits its count on stdout (including0on no matches) AND exits 1 when there are no matches. The previous|| echo "0"clause appended a SECOND0, producing"0\n0"in the captured variable. That two-line value broke:[ "$VAR" -gt 0 ]→integer expression expectedInvalid format '0',Unable to process file command 'output'|| truesuppresses grep's non-zero exit without polluting stdout — grep's own0is the value we want.Affected steps
Testing
golden-paths/pipelines/code-quality.ymlinto a repo's.github/workflows/integer expression expected/Invalid format '0')::error::annotation firesVerified end-to-end on an adopter repo's CI run prior to filing.
Closes #374
Glossary
.github/workflows/.${{ steps.<id>.outputs.<key> }}.