Skip to content

Commit dbff413

Browse files
anandgupta42claude
andauthored
feat: upstream_fix marker convention, --audit-fixes command, and CI Bun crash resilience (#555)
* feat: add `upstream_fix:` marker convention with `--audit-fixes` command 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> * fix: handle Bun segfault during CI test cleanup 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> * fix: use file redirect instead of tee for Bun crash resilience 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> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 384335d commit dbff413

File tree

10 files changed

+193
-33
lines changed

10 files changed

+193
-33
lines changed

.github/workflows/ci.yml

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,13 +84,45 @@ jobs:
8484
run: bun install
8585

8686
- name: Run tests
87-
run: bun test --timeout 30000
8887
working-directory: packages/opencode
8988
# Cloud E2E tests (Snowflake, BigQuery, Databricks) auto-skip when
9089
# ALTIMATE_CODE_CONN_* env vars are not set. Docker E2E tests auto-skip
9190
# when Docker is not available. No exclusion needed — skipIf handles it.
9291
# --timeout 30000: matches package.json "test" script; prevents 5s default
9392
# from cutting off tests that run bun install or bootstrap git instances.
93+
#
94+
# Bun 1.3.x has a known segfault during process cleanup after all tests
95+
# pass (exit code 143/SIGTERM or 134/SIGABRT). We capture test output and
96+
# check for real failures vs Bun crashes to avoid false CI failures.
97+
shell: bash
98+
run: |
99+
# Redirect bun output to file, then cat it for CI visibility.
100+
# This avoids tee/pipe issues where SIGTERM kills tee before flush.
101+
bun test --timeout 30000 > /tmp/test-output.txt 2>&1 || true
102+
cat /tmp/test-output.txt
103+
104+
# Extract pass/fail counts from Bun test summary (e.g., " 5362 pass")
105+
PASS_COUNT=$(awk '/^ *[0-9]+ pass$/{print $1}' /tmp/test-output.txt || true)
106+
FAIL_COUNT=$(awk '/^ *[0-9]+ fail$/{print $1}' /tmp/test-output.txt || true)
107+
108+
echo ""
109+
echo "--- Test Summary ---"
110+
echo "pass=${PASS_COUNT:-none} fail=${FAIL_COUNT:-none}"
111+
112+
# Real test failures — always fail CI
113+
if [ -n "$FAIL_COUNT" ] && [ "$FAIL_COUNT" != "0" ]; then
114+
echo "::error::$FAIL_COUNT test(s) failed"
115+
exit 1
116+
fi
117+
118+
# Tests passed (we have a pass count and zero/no failures)
119+
if [ -n "$PASS_COUNT" ] && [ "$PASS_COUNT" -gt 0 ] 2>/dev/null; then
120+
exit 0
121+
fi
122+
123+
# No test summary at all — Bun crashed before running tests
124+
echo "::error::No test results found in output — Bun may have crashed before running tests"
125+
exit 1
94126
95127
# ---------------------------------------------------------------------------
96128
# Driver E2E tests — only when driver code changes.

packages/dbt-tools/src/commands/build.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,11 @@ import type { DBTProjectIntegrationAdapter, CommandProcessResult } from "@altima
22

33
export async function build(adapter: DBTProjectIntegrationAdapter, args: string[]) {
44
const model = flag(args, "model")
5-
if (!model) return project(adapter)
65
const downstream = args.includes("--downstream")
6+
if (!model) {
7+
if (downstream) return { error: "--downstream requires --model" }
8+
return project(adapter)
9+
}
710
const result = await adapter.unsafeBuildModelImmediately({
811
plusOperatorLeft: "",
912
modelName: model,

packages/dbt-tools/test/build.test.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,14 @@ describe("build command", () => {
4545
})
4646
})
4747

48+
test("build --downstream without --model returns error", async () => {
49+
const adapter = makeAdapter()
50+
const result = await build(adapter, ["--downstream"])
51+
expect(result).toEqual({ error: "--downstream requires --model" })
52+
expect(adapter.unsafeBuildProjectImmediately).not.toHaveBeenCalled()
53+
expect(adapter.unsafeBuildModelImmediately).not.toHaveBeenCalled()
54+
})
55+
4856
test("build surfaces stderr as error", async () => {
4957
const adapter = makeAdapter({
5058
unsafeBuildProjectImmediately: mock(() =>

packages/opencode/src/cli/cmd/tui/routes/home.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ export function Home() {
3838
return Object.values(sync.data.mcp).filter((x) => x.status === "connected").length
3939
})
4040

41-
// altimate_change start — fix race condition: don't show beginner UI until sessions loaded
41+
// altimate_change start — upstream_fix: race condition shows beginner UI flash before sessions loaded
4242
const isFirstTimeUser = createMemo(() => {
4343
// Don't evaluate until sessions have actually loaded (avoid flash of beginner UI)
4444
// Return undefined to represent "loading" state

packages/opencode/src/command/index.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,9 @@ export namespace Command {
5454
const result: string[] = []
5555
const numbered = template.match(/\$\d+/g)
5656
if (numbered) {
57-
// altimate_change start — fix lexicographic sort of multi-digit placeholders ($10 before $2)
58-
for (const match of [...new Set(numbered)].sort((a, b) => parseInt(a.slice(1), 10) - parseInt(b.slice(1), 10))) result.push(match)
57+
// altimate_change start — upstream_fix: lexicographic sort of multi-digit placeholders ($10 sorted before $2)
58+
for (const match of [...new Set(numbered)].sort((a, b) => parseInt(a.slice(1), 10) - parseInt(b.slice(1), 10)))
59+
result.push(match)
5960
// altimate_change end
6061
}
6162
if (template.includes("$ARGUMENTS")) result.push("$ARGUMENTS")

packages/opencode/src/skill/followups.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ export namespace SkillFollowups {
44
skill: string // skill name to suggest
55
label: string // short display label
66
description: string // why this is a good next step
7-
condition?: string // optional: when this suggestion applies
87
}
98

109
// Map from skill name to follow-up suggestions
@@ -151,7 +150,8 @@ export namespace SkillFollowups {
151150
}
152151

153152
// A special warehouse nudge for users who haven't connected yet
154-
const WAREHOUSE_NUDGE = "**Tip:** Connect a warehouse to validate against real data. Run `/discover` to auto-detect your connections."
153+
const WAREHOUSE_NUDGE =
154+
"**Tip:** Connect a warehouse to validate against real data. Run `/discover` to auto-detect your connections."
155155

156156
export function get(skillName: string): readonly Suggestion[] {
157157
return Object.freeze(FOLLOWUPS[skillName] ?? [])

packages/opencode/src/util/locale.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,10 @@ export namespace Locale {
5454
const minutes = Math.floor((input % 3600000) / 60000)
5555
return `${hours}h ${minutes}m`
5656
}
57+
// altimate_change start — upstream_fix: days/hours calculation were swapped (hours used total, not remainder)
5758
const days = Math.floor(input / 86400000)
5859
const hours = Math.floor((input % 86400000) / 3600000)
60+
// altimate_change end
5961
return `${days}d ${hours}h`
6062
}
6163

script/upstream/README.md

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,44 @@ When we modify upstream files (not fully custom ones), we wrap our changes with
222222

223223
These help during conflict resolution — you can see exactly what we changed vs upstream code. The `analyze.ts` script audits for unclosed marker blocks.
224224

225+
### Upstream Bug Fixes (`upstream_fix:` tag)
226+
227+
When fixing a **bug in upstream code** (not adding a feature), use the `upstream_fix:` tag in the marker description:
228+
229+
```typescript
230+
// altimate_change start — upstream_fix: days/hours calculation were swapped
231+
const days = Math.floor(input / 86400000)
232+
const hours = Math.floor((input % 86400000) / 3600000)
233+
// altimate_change end
234+
```
235+
236+
**Why this matters:** Regular `altimate_change` markers protect features we added — they're permanent. But upstream bug fixes are **temporary**: once upstream ships their own fix, we should drop our marker and accept theirs.
237+
238+
Without the `upstream_fix:` tag:
239+
- If upstream fixes the same bug, the merge creates a conflict (good — forces review)
240+
- But the reviewer doesn't know our change was a bug fix vs a feature, so they may keep both
241+
242+
With the `upstream_fix:` tag:
243+
- Before each merge, run `--audit-fixes` to see all bug fixes we're carrying
244+
- During conflict resolution, reviewers know to check "did upstream fix this?" and can safely drop our version
245+
- After merge, any remaining `upstream_fix:` markers represent bugs upstream hasn't fixed yet
246+
247+
**When to use which:**
248+
249+
| Scenario | Marker |
250+
|----------|--------|
251+
| New feature/custom code | `// altimate_change start — description` |
252+
| Fix bug in upstream code | `// altimate_change start — upstream_fix: description` |
253+
| Branding change | No marker (handled by branding transforms) |
254+
| Code in `keepOurs` files | No marker needed |
255+
256+
**Audit before merging:**
257+
258+
```bash
259+
# List all upstream bug fixes we're carrying
260+
bun run script/upstream/analyze.ts --audit-fixes
261+
```
262+
225263
## File Organization
226264

227265
```

script/upstream/analyze.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,18 @@ describe("parseDiffForMarkerWarnings", () => {
246246
expect(warnings[0].context).toContain("first")
247247
})
248248

249+
test("upstream_fix: tagged markers are recognized as valid markers", () => {
250+
const diff = makeDiff(
251+
`@@ -50,4 +50,6 @@
252+
const existing = true
253+
+// altimate_change start — upstream_fix: days/hours were swapped
254+
+const days = Math.floor(input / 86400000)
255+
+// altimate_change end
256+
const more = true`,
257+
)
258+
expect(parseDiffForMarkerWarnings("file.ts", diff)).toEqual([])
259+
})
260+
249261
test("real-world scenario: upgrade indicator in footer.tsx", () => {
250262
// Simulates the exact diff that leaked: UpgradeIndicator added to
251263
// session footer without markers, adjacent to existing yolo marker block.

0 commit comments

Comments
 (0)