feat: upstream_fix marker convention, --audit-fixes command, and CI Bun crash resilience#555
Merged
anandgupta42 merged 3 commits intomainfrom Mar 29, 2026
Merged
Conversation
Introduces a strategy for handling bug fixes to upstream code: **`upstream_fix:` convention:** - Tag in marker description distinguishes temporary bug fixes from permanent feature additions - Before each upstream merge, run `--audit-fixes` to review which fixes we're carrying and whether upstream has shipped their own **`--audit-fixes` flag on analyze.ts:** - Lists all `upstream_fix:` markers with file locations and descriptions - Prints merge review checklist **Retagged 3 existing upstream bug fixes:** - `locale.ts` — days/hours duration swap - `command/index.ts` — placeholder lexicographic sort - `home.tsx` — beginner UI race condition **Code review fixes from #546:** - Guard `--downstream` without `--model` (returns error instead of silently running full project build) - Remove unused `condition` field from `Suggestion` interface - Add test for `--downstream` error case Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (CLI)
participant Analyze as analyze.ts
participant Repo as Repository files
participant Parser as Marker Parser
participant Console as Console/Report
Dev->>Analyze: run `analyze.ts --audit-fixes`
Analyze->>Repo: scan files for marker blocks
Repo-->>Analyze: return file hunks
Analyze->>Parser: parse hunks for `upstream_fix:` in start comment
Parser-->>Analyze: list of matching markers (file, range, description)
Analyze->>Console: print "Upstream Bug Fixes We’re Carrying" report
Console-->>Dev: human-readable checklist + exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
script/upstream/analyze.ts (1)
514-517: Minor: JSX comment trailing characters may leak into description.For JSX markers like
{/* altimate_change start — upstream_fix: desc */}, the description extraction may leave a trailing}:desc }This is cosmetic and doesn't affect functionality, but for cleaner output:
Optional fix for cleaner JSX description extraction
- const desc = fix.startComment.replace(/.*upstream_fix:\s*/, "").replace(/\s*\*\/\s*$/, "") + const desc = fix.startComment + .replace(/.*upstream_fix:\s*/, "") + .replace(/\s*\*\/\s*\}?\s*$/, "") + .trim()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/upstream/analyze.ts` around lines 514 - 517, The description extraction for each fix (inside the for loop over fixes) can leave a trailing JSX brace like "}" in desc; update the desc assignment that uses fix.startComment to additionally strip trailing braces and stray comment chars by chaining a final replace such as .replace(/[\s\}]*$/, "") (i.e., modify the desc computation that currently uses fix.startComment.replace(/.*upstream_fix:\s*/, "").replace(/\s*\*\/\s*$/, "") to also remove trailing "}"/whitespace so JSX markers like {/* ... */} don't leak a trailing brace).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@script/upstream/analyze.ts`:
- Around line 514-517: The description extraction for each fix (inside the for
loop over fixes) can leave a trailing JSX brace like "}" in desc; update the
desc assignment that uses fix.startComment to additionally strip trailing braces
and stray comment chars by chaining a final replace such as .replace(/[\s\}]*$/,
"") (i.e., modify the desc computation that currently uses
fix.startComment.replace(/.*upstream_fix:\s*/, "").replace(/\s*\*\/\s*$/, "") to
also remove trailing "}"/whitespace so JSX markers like {/* ... */} don't leak a
trailing brace).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c8cba34f-f6ea-4145-ab15-a11e13864ddc
📒 Files selected for processing (9)
packages/dbt-tools/src/commands/build.tspackages/dbt-tools/test/build.test.tspackages/opencode/src/cli/cmd/tui/routes/home.tsxpackages/opencode/src/command/index.tspackages/opencode/src/skill/followups.tspackages/opencode/src/util/locale.tsscript/upstream/README.mdscript/upstream/analyze.test.tsscript/upstream/analyze.ts
Bun 1.3.x has a known crash (segfault/SIGTERM) during process cleanup after all tests pass successfully. This caused flaky CI failures where 5362 tests pass but the job reports failure due to exit code 143. The fix captures test output and checks the actual pass/fail summary instead of relying on Bun's exit code: - Real test failures (N fail > 0): exit 1 - No test summary at all (Bun crashed before running): exit 1 - All tests pass but Bun crashes during cleanup: emit warning, exit 0 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
anandgupta42
changed the title
The previous approach using `tee` failed because when Bun segfaults, the pipe breaks and tee doesn't flush output to the file. The grep then finds no summary and reports "crashed before producing results" even though all tests passed. Fix: redirect bun output to file directly (`> file 2>&1 || true`), then `cat` it for CI log visibility. Use portable `awk` instead of `grep -oP` for summary extraction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
101-121: Capture and reuse the test command exit code for better CI diagnostics.Line 101 currently swallows the test process status via
|| true, so the step can’t tell a clean pass from a crash-after-pass case. Capture the exit code and emit a warning when tests pass but the command returned non-zero.Suggested patch
- bun test --timeout 30000 > /tmp/test-output.txt 2>&1 || true + BUN_EXIT=0 + bun test --timeout 30000 > /tmp/test-output.txt 2>&1 || BUN_EXIT=$? cat /tmp/test-output.txt @@ - if [ -n "$PASS_COUNT" ] && [ "$PASS_COUNT" -gt 0 ] 2>/dev/null; then + if [ -n "$PASS_COUNT" ] && [ "$PASS_COUNT" -gt 0 ] 2>/dev/null; then + if [ "$BUN_EXIT" -ne 0 ]; then + echo "::warning::Tests passed, but bun exited with status $BUN_EXIT (likely cleanup crash)" + fi exit 0 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 101 - 121, The CI step currently swallows the bun test exit code by using "|| true"; run bun test redirecting output to /tmp/test-output.txt without "|| true", immediately capture its exit status into a variable (e.g., TEST_EXIT_CODE=$?), and then use PASS_COUNT and FAIL_COUNT as already extracted to decide final outcome: if FAIL_COUNT non-zero exit 1 as now; if PASS_COUNT > 0 but TEST_EXIT_CODE != 0 emit a warning message including TEST_EXIT_CODE and then exit non-zero (or at least surface the non-zero status), otherwise exit 0 on clean success. Ensure references to the bun test invocation, /tmp/test-output.txt, TEST_EXIT_CODE, PASS_COUNT, and FAIL_COUNT are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 101-121: The CI step currently swallows the bun test exit code by
using "|| true"; run bun test redirecting output to /tmp/test-output.txt without
"|| true", immediately capture its exit status into a variable (e.g.,
TEST_EXIT_CODE=$?), and then use PASS_COUNT and FAIL_COUNT as already extracted
to decide final outcome: if FAIL_COUNT non-zero exit 1 as now; if PASS_COUNT > 0
but TEST_EXIT_CODE != 0 emit a warning message including TEST_EXIT_CODE and then
exit non-zero (or at least surface the non-zero status), otherwise exit 0 on
clean success. Ensure references to the bun test invocation,
/tmp/test-output.txt, TEST_EXIT_CODE, PASS_COUNT, and FAIL_COUNT are updated
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 45c5557a-e74a-467d-bfc1-4a17303e433c
📒 Files selected for processing (1)
.github/workflows/ci.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Introduces a strategy for handling bug fixes to upstream code, plus code review fixes from #546.
upstream_fix:convention:// altimate_change start — upstream_fix: description) distinguishes temporary bug fixes from permanent feature additions--audit-fixescommand:bun run script/upstream/analyze.ts --audit-fixeslists all upstream bug fixes we're carryingRetagged 3 existing upstream bug fixes:
locale.ts:57— days/hours duration calculation swapcommand/index.ts:57— placeholder lexicographic sort ($10 before $2)home.tsx:41— beginner UI race condition flashCode review fixes from #546:
--downstreamwithout--modelin dbt build (returns error instead of silently running full project build)conditionfield fromSuggestioninterfaceType of change
Issue for this PR
Closes #554
How did you verify your code works?
upstream_fix:recognition)--audit-fixescorrectly lists all 3 upstream bug fixes--downstreamguard)origin/mainChecklist
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Refactor
conditionfield from followups interface.Tests
Style / Chores