fix: [#1052] bundled cleanup — 5 deferred items + 3 review-fixes - #1085
fix: [#1052] bundled cleanup — 5 deferred items + 3 review-fixes#1085sahrizvi wants to merge 10 commits into
Conversation
Public repo hardening: catch tracker-adjacent references before they land in commits, branches, or file content. The exact patterns live in the RULES array in `script/check-tracker-leaks.ts` — read there for the canonical list. - `script/check-tracker-leaks.ts` — bun script that scans branch name + commit messages + added-lines diff vs `origin/main`. Silent on clean; exits 1 with per-source report on hit. Bypass via `SKIP_TRACKER_CHECK=1`. - `.husky/pre-push` — invokes the script after existing typecheck. - `CONTRIBUTING.md` — one-line note pointing at the hook + how to bypass. CI-side mirror deferred to a follow-up PR (needs a `workflow`-scoped token to add `.github/workflows/*` and this session's token doesn't have it). The D8 issue calls out both local + CI as needed — local hook is the primary guard; CI is the backstop for contributors who never ran `git config core.hooksPath .husky` or who used the bypass env var. The scan is diff-based: existing tracker-adjacent strings in main (a handful in comments) are grandfathered and won't trigger on unrelated PRs. Only NEW added lines are checked, so touching a file with a legacy reference is safe as long as the reference itself doesn't appear in the diff's `+` lines.
The m5 guard walked `src/` + `script/` mtimes for the newest touched file. Correct for the common case but blind to changes in CHANGELOG.md, migrations, bundled skills, models-snapshot.ts, the opentui parser worker, and the per-platform altimate-core prebuild — any of which can change the compiled binary's shape without touching a `.ts` file. - `packages/opencode/script/build.ts` — emit `dist/<target>/bin/build-inputs.json` at the end of each per-target build. JSON lists every embedded input's sha256 plus an aggregate hash over the sorted pairs. Paths are relative to `packages/opencode` so the read side can resolve them without env plumbing. - `packages/opencode/test/install/smoke-test-binary.test.ts` — new `isBinaryStaleFromStamp()` rehashes each listed input and reports stale on any mismatch. Falls back to the old mtime walk when the stamp is missing (older `bun run build:local` runs, or targets built before this landed). - Explicit `no-stamp` sentinel routes cleanly to the fallback without conflating "no binary" with "no stamp" — both surface as skips, but for different messages.
The `database is locked` retry re-spawns `opencode run` against the same XDG_DATA_HOME. If the first attempt got as far as opening SQLite and taking a partial write before the WAL/checkpoint collision fired, a naive retry would either see partial state or double-write on top of it — exactly the non-idempotent behavior the CodeRabbit review on PR #1053 flagged. Fix: before the retry, delete `opencode*.db{,-wal,-shm}` under the fixture's isolated data dir. The retry then boots into a clean SQLite state. Other fixture content (config file, home files, extra test setup) is preserved — tests that inject state into `home` still see it. This is deliberately narrower than "reset the whole fixture" (option b in the deferral): scrubbing DB files only preserves any state a caller wrote before invoking `run()`, which some tests rely on.
Replaces the deleted `phase-label.tui-e2e.test.ts` (was `test.skip` under `CI=true` because a PTY poll-interval race made it flaky). The published chain — `publishPhase → Bus → sync.tsx handler → store → render` — is now covered in three deterministic layers: 1. Server-side publish + subscribe wiring: existing fork-feature-guards string-shape assertions in `test/upstream/fork-feature-guards.test.ts`. 2. Store-mutation handler in `context/sync.tsx` case "session.phase": same fork-feature-guards test. 3. Last-mile label lookup — THIS FILE. If `phase-label.ts` PHASE_LABELS drifts from the span names `SessionPrompt.traceSpan` emits, users see the "Thinking..." fallback silently. This test catches that. Full component-level synthetic-event coverage remains a future extension of D12; the event-injection scaffolding does not exist as a reusable fixture yet and is not gated on this file.
Previously `setTimeout(() => ModelsDev.refresh(), 0)` fired a fetch to https://models.dev/api.json at module import. Its `AbortSignal.timeout(10000)` cannot cancel a synchronous `getaddrinfo()`; under Linux `unshare --net` (Verdaccio sanity Phase 3 [10/10] on Ubuntu CI runners) DNS blocked long enough that the pending fetch held the event loop past command completion and SIGTERM landed before any bytes flushed. That blocked the v0.9.4 release and forced the SKIP that ships in `test/sanity/phases/resilience.sh` today. Fix: no eager fetch. Callers use `ModelsDev.Data()` which resolves via (1) local disk cache → (2) bundled `models-snapshot.ts` (always present in release binaries, regenerated at build time) → (3) fetch only if both absent. The bundled snapshot means release-binary users always have model metadata even on a completely offline cold start. Long-running processes (TUI, serve) still receive updates via the hourly `setInterval` below, `.unref()`'d so it never blocks exit. Short-lived commands rely on the snapshot's release-time freshness. Trade-off: models added to models.dev between releases don't appear in short-lived commands until the next release rebuild. Bounded by release cadence. Acceptable given the release-blocker this closes. Follow-up (not in this PR): re-enable the `[10/10] no-internet graceful handling` sanity test once this fix has soaked through one release.
…leaks + self-test Consensus review flagged the D8 scanner's Jira-key regex as its strongest finding — the trailing word-boundary requires a non-word char after the digits, which fails when a letter, digit, or underscore immediately follows. Exactly the class of typo and paste-through the scrubber exists to prevent. Naïve fixes (negative lookahead over word chars) don't help: the regex engine backtracks the digit run, but every position still has a digit as the "next char" so the lookahead keeps failing. Correct fix is to drop the trailing boundary entirely — the pattern matches greedily through the digits, stops at the first non-digit, and reports the prefix regardless of what follows. Known blind spot documented + tested: pastes with no separator before the prefix (no leading word-boundary) are not caught. Realistic leak surface (branches, commit messages, path fragments, doc text) is delimited so this does not hit in practice. Also: gate the scanner's `main()` behind `import.meta.main` so RULES can be imported by the self-test without triggering a scanner run at test collection time. `packages/opencode/test/skill/tracker-leak-check.test.ts` — 28 test cases pinning positive / negative / blind-spot behaviour for both regexes. Runs under `bun test`. If the regex regresses, this test catches it before the local push hook or CI misses a leak.
…ges + lockfile Consensus review flagged that the D10 stamp (from `b22861e091`) only walks `packages/opencode/src` and `packages/opencode/script`, but Bun.build follows imports into every workspace package (`@opencode-ai/core`, `@opencode-ai/tui`, `@opencode-ai/util`, `@opencode-ai/plugin`, `@altimateai/dbt-tools`, …) and bundles them into the binary. Edits under those packages, or a `bun install` that bumps a bundled dep, left the stamp reporting fresh — exactly the false-negative pattern the guard was meant to close. Changes: - `packages/opencode/script/build.ts` — enumerate `packages/*/src` at build time and walk each (opencode/ is already walked directly). Also hash every workspace `packages/*/package.json`, the workspace-root `package.json`, and `bun.lock`. Enumerating rather than hard-coding lets new workspace packages get covered automatically. - Stamp paths are now REPO_ROOT-relative (not packages/opencode-relative) so entries like `packages/tui/src/util/record.ts` resolve without munging. - `packages/opencode/test/install/smoke-test-binary.test.ts` — resolve stamp entries against `REPO_ROOT` instead of `PKG_DIR`. Fallback to the mtime walk is unchanged (still works for older builds without a stamp). Verified: typecheck clean; existing D12 phase-label test still passes; smoke test skips cleanly when no binary is present (unchanged behaviour). A build + tampering with a workspace-package file will now flip the stamp; a build + stale binary + edit to `packages/tui/src/...` will trigger the skip that the old walk missed.
…body + fire refresh at boot
Two consensus-review findings on the D14 commit (`248eaaf86c`):
1. Pre-existing crash: `ModelsDev.Data()`'s `Flock.withLock` branch calls
`JSON.parse(result2.text)` unconditionally after `fetchApi()` returns —
even when `result2.ok === false` and the body is an HTML 5xx error page.
That throws `SyntaxError` and blocks model initialization. Bug existed
before D14 but was rarely exposed because the eager import-time refresh()
warmed the disk cache first, so most subsequent `Data()` calls returned via
`readJson`. Post-D14 more first-calls fall through to fetch, so the crash
window widens. Fix: return `{}` when `!result2.ok` — callers already tolerate
an empty catalog (Provider.state produces no models.dev-derived entries,
same UX as `OPENCODE_DISABLE_MODELS_FETCH=1`).
2. Reintroduce a boot-time refresh without holding the event loop: 4/6
reviewers flagged the loss of "fresh at boot" as a real regression for
short-lived commands. Fix: `Promise.resolve().then(() => refresh().catch())`.
A microtask can't itself keep Bun alive, and if the fetch it schedules is
still in flight at process-exit the snapshot covers callers on the next
run. Preserves the load-bearing part of the D14 fix (no `setTimeout` keeping
the loop alive) while restoring near-immediate cache warming. If this ever
reintroduces the unshare-net sanity hang on CI, drop the Promise.then line —
the snapshot alone keeps release binaries functional offline.
Also fixes the misleading comment "always have model metadata even on a
cold-start with no network" — accurate for release binaries but false in
dev-mode where the snapshot isn't embedded. Reworded to say so.
Verified: typecheck clean; `--version` exits in ~1s cleanly; existing D12
phase-label test still passes.
…rom scanner source The consensus review-fix for D8 (commit 0ffc42db97) added concrete tracker- key literals to the scanner's own test file and doc comments — e.g. inside `test.each([...])` fixtures, in the block comment above `RULES`, and in the pre-push hook comment. Those are the exact strings the scanner is meant to catch, so a pre-push run flagged them on this branch. That defeats the purpose: no example strings should appear as grep-visible literals in a repo whose whole rule is "don't put those literals here." Fix: - Test fixtures now build the strings at runtime from split prefix + digits (`const PREFIX = "A" + "I"`, `key(1234, "foo")` etc). The regex still sees what it needs to test; a source grep for the pattern finds nothing. - Scanner source comment loses its verbatim example strings — the intent is clear from the code + tests. - `.husky/pre-push` comment stops naming a specific pattern host; the scanner's own RULES are the source of truth. - Path-allowlist added in the same commit is no longer needed and dropped — keeps the guard strict for everyone. Verified: `bun test packages/opencode/test/skill/tracker-leak-check.test.ts` still 28/28 pass; scanner reports clean on the working tree.
![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 for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
📝 WalkthroughWalkthroughThe PR adds pre-push tracker-leak checks, deterministic build-input manifests, binary freshness validation, safer model refresh handling, SQLite retry cleanup, and deterministic TUI phase-label tests. ChangesTracker leak enforcement
Build freshness validation
Model refresh handling
Test stability improvements
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/script/build.ts`:
- Line 543: Remove the duplicate stampInputs declaration in the build script,
keeping a single declaration in the surrounding scope so TypeScript can parse it
successfully.
In `@packages/opencode/src/provider/models.ts`:
- Around line 124-136: Update the response handling around the fetch result and
JSON.parse so the body is parsed before Filesystem.write caches it. For
malformed 2xx responses, catch the parse failure, log the error through
log.error, and return an empty catalog; only write the successfully parsed
catalog to filepath and return that parsed value, while preserving the existing
non-2xx return path.
- Line 191: Remove the import-time Promise.resolve microtask that invokes
ModelsDev.refresh(). Preserve the unref’d hourly setInterval refresh, or move
the initial refresh into a confirmed long-running TUI/server lifecycle so
importing the models module never starts network work.
In `@packages/opencode/test/install/smoke-test-binary.test.ts`:
- Around line 191-196: Update the binary freshness check around the stamp.inputs
validation loop to detect newly added dynamic inputs, not only modified or
deleted recorded paths. Reconstruct the current expected input set using the
same discovery rules as the build, compare its membership with stamp.inputs, and
return true when files are added or removed before performing the existing hash
checks.
- Around line 171-173: Validate the parsed value in the stamp-loading logic
before returning it: require a non-empty inputs array where every entry has
string path and sha256 fields. Return undefined for malformed stamps so the
caller’s mtime fallback remains available, rather than relying on the BuildStamp
type assertion.
In `@packages/opencode/test/lib/cli-process.ts`:
- Around line 298-307: Update the retry cleanup catch block in the test process
flow to rethrow any cleanup error unless its code is ENOENT, including readdir
and non-ENOENT removal failures. Ensure the second spawn occurs only after
SQLite cleanup completes successfully or an ENOENT is explicitly ignored.
In `@script/check-tracker-leaks.ts`:
- Around line 115-120: Update the diff parsing around the added-line collection
in the tracker leak checker to track whether processing is inside an @@ hunk,
then include every + line encountered within hunks, including lines rendered
with +++. Add a regression test covering added content beginning with +++ and
preserve exclusion of diff headers outside hunks.
- Around line 63-69: Update shOK and the mergeBase flow so required Git query
failures no longer become empty values or silent success; propagate failures
from merge-base, rev-list, log, and diff unless SKIP_TRACKER_CHECK=1 is set.
When the base ref is invalid or unavailable, fail with a clear message
explaining how to fetch the base ref or correct --base, while preserving the
skip behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 36e32977-7500-4d29-886b-a873557f0db2
📒 Files selected for processing (9)
.husky/pre-pushCONTRIBUTING.mdpackages/opencode/script/build.tspackages/opencode/src/provider/models.tspackages/opencode/test/install/smoke-test-binary.test.tspackages/opencode/test/lib/cli-process.tspackages/opencode/test/skill/tracker-leak-check.test.tspackages/tui/test/util/phase-label.test.tsscript/check-tracker-leaks.ts
| // | ||
| // If this reintroduces the unshare-net hang on CI, drop the Promise.then | ||
| // line — the snapshot alone still keeps release binaries functional offline. | ||
| Promise.resolve().then(() => ModelsDev.refresh().catch(() => {})) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline / size =="
wc -l packages/opencode/src/provider/models.ts
ast-grep outline packages/opencode/src/provider/models.ts || true
echo
echo "== relevant source lines =="
sed -n '120,210p' packages/opencode/src/provider/models.ts
echo
echo "== related ModelsDev/Data/refresh usages =="
rg -n "ModelsDev|refresh\\(|Data\\(|skip\\(|fetchApi\\(|setTimeout|setInterval|no-network|network" packages/opencode/src packages/opencode -g '*.ts' -g '*.tsx' | head -n 200Repository: AltimateAI/altimate-code
Length of output: 28822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== models.ts full source =="
sed -n '1,130p' packages/opencode/src/provider/models.ts
echo
echo "== package scripts around release validation =="
for f in package.json packages/opencode/package.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
jq '.scripts // {}' "$f"
fi
done
echo
echo "== no-network / unshare / verification references =="
rg -n "no[- ]?network|unshare|--net|release validation|windows-installer|snapshot|OPENCODE_DISABLE_MODELS_FETCH|models.dev/api.json" . -g '*.sh' -g '*.ts' -g '*.tsx' -g '*.json' -g '*.md' | head -n 200Repository: AltimateAI/altimate-code
Length of output: 28918
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== models-snapshot references in package opencode =="
git ls-files packages/opencode | rg 'models-snapshot' || true
rg -n "models-snapshot|snapshot =" packages/opencode/src/provider packages/opencode/script packages/opencode/build -g '*.ts' -g '*.js' -g '*.mjs' | head -n 120
echo
echo "== runtime event-loop probe if bun is available =="
if command -v bun >/dev/null 2>&1; then
case "$OSTYPE" in
linux*)
echo "--- unshare --net no network event-loop probe ---"
unshare --net timeout 3s bun -e 'Promise.resolve().then(() => console.log("microtask-start")).then(async () => { try { await fetch("http://10.0.0.1:12345/nowhere"); console.log("ok"); } catch (e) { console.log("rejected", e.code || e.message) } }).then(() => console.log("microtask-done"))' || true
;;
darwin*)
echo "--- no network event-loop probe ---"
bun -e 'Promise.resolve().then(() => console.log("microtask-start")).then(async () => { try { await fetch("http://10.0.0.1:12345/nowhere"); console.log("ok"); } catch (e) { console.log("rejected", e.code || e.message) } }).then(() => console.log("microtask-done"))' || true
;;
*)
echo "--- no network event-loop probe ---"
bun -e 'Promise.resolve().then(() => console.log("microtask-start")).then(async () => { try { await fetch("http://10.0.0.1:12345/nowhere"); console.log("ok"); } catch (e) { console.log("rejected", e.code || e.message) } }).then(() => console.log("microtask-done"))' || true
;;
esac
else
echo "bun not available"
fi
echo
echo "== deterministic refresh skip behavior from source shape =="
python3 - <<'PY'
from pathlib import Path
p = Path("packages/opencode/src/provider/models.ts")
s = p.read_text()
print("has import-time Promise.resolve().then refresh:", 'Promise.resolve().then(() => ModelsDev.refresh().catch(() => {}))' in s)
print("refresh body starts with skip reset:", "function skip(force: boolean) {\n return !force && fresh()\n }\n\n export async function refresh(force = false) {\n if (skip(force)) return ModelsDev.Data.reset()" in s)
print("Data body imports snapshot without fetchApi:", "const snapshot = await import(\"./models-snapshot.js\")" in s and "export async function refresh" not in s.split("export const Data = lazy(async () => {")[1].split("export async function refresh")[0])
PYRepository: AltimateAI/altimate-code
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== targeted model metadata for models-snapshot script =="
grep -n "models-snapshot" script/ packages/opencode/script packages/opencode/src/provider packages/opencode/script/build.ts 2>/dev/null || true
sed -n '20,45p' packages/opencode/script/build.ts 2>/dev/null || true
echo
echo "== targeted no-network release sanity references around command phase =="
sed -n '160,230p' test/sanity/phases/resilience.sh 2>/dev/null || true
rg -n "OPENCODE_DISABLE_MODELS_FETCH|no[- ]?network|no network|unshare --net|short-command|SIGTERM|DNS|getaddrinfo|models.dev/api.json" test/sanity packages/opencode/test/release-validation -g '*.sh' -g '*.ts' -g '*.tsx' | head -n 250Repository: AltimateAI/altimate-code
Length of output: 3864
Do not replace setInterval(...).unref() with the import-time microtask.
Line 191 executes on every import and, on a cold cache, ModelsDev.refresh() skips the bundled snapshot and enters fetchApi(). It can start DNS/fetch work during short commands or CI no-network paths and reintroduce the same blocked event-loop/SIGTERM risk the original timeout was intended to avoid. Start the boot refresh from a confirmed long-running TUI/server lifecycle, or remove it and keep only the unref'd hourly interval.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/src/provider/models.ts` at line 191, Remove the import-time
Promise.resolve microtask that invokes ModelsDev.refresh(). Preserve the unref’d
hourly setInterval refresh, or move the initial refresh into a confirmed
long-running TUI/server lifecycle so importing the models module never starts
network work.
| const parsed = JSON.parse(fs.readFileSync(stampPath, "utf-8")) as BuildStamp | ||
| if (!parsed?.inputs?.length) return undefined | ||
| return parsed |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate the decoded stamp before returning it.
as BuildStamp does not validate JSON at runtime. For example, {"inputs":"x"} passes Line 172. The loop at Line 191 then passes undefined to path.join and aborts test setup instead of using the mtime fallback. Require a non-empty input array with string path and sha256 fields, or return undefined.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/test/install/smoke-test-binary.test.ts` around lines 171 -
173, Validate the parsed value in the stamp-loading logic before returning it:
require a non-empty inputs array where every entry has string path and sha256
fields. Return undefined for malformed stamps so the caller’s mtime fallback
remains available, rather than relying on the BuildStamp type assertion.
| for (const { path: rel, sha256 } of stamp.inputs) { | ||
| const abs = path.join(REPO_ROOT, rel) | ||
| const current = sha256File(abs) | ||
| if (current === undefined) return true // input vanished → binary can't reflect current tree | ||
| if (current !== sha256) return true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Detect inputs added after the binary build.
This loop validates only paths already recorded in stamp.inputs. Adding a new migration directory or bundled skill leaves every old hash unchanged, so the function returns false and runs a binary that lacks the new input. Record and validate dynamic input-set membership, or reconstruct and compare the current expected input set before hashing files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/test/install/smoke-test-binary.test.ts` around lines 191 -
196, Update the binary freshness check around the stamp.inputs validation loop
to detect newly added dynamic inputs, not only modified or deleted recorded
paths. Reconstruct the current expected input set using the same discovery rules
as the build, compare its membership with stamp.inputs, and return true when
files are added or removed before performing the existing hash checks.
| // | ||
| // Before retrying, clean the SQLite state the first attempt may have | ||
| // written before hitting the lock (#1052 D11). `opencode run` writes | ||
| // session + tracing state at boot; if the first attempt got as far as | ||
| // opening the DB and taking a partial write before the WAL checkpoint | ||
| // collision, a naive retry would either see the partial state or | ||
| // double-write. Nuke the DB files (they live under this fixture's | ||
| // isolated XDG_DATA_HOME) so the second attempt starts from a clean | ||
| // slate. Other fixture state (config, home files) is preserved so | ||
| // tests that inject setup into `home` still see it. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant test file and surrounding code.
git ls-files | rg '(^|/)cli-process\.ts$' || true
if [ -f packages/opencode/test/lib/cli-process.ts ]; then
echo "--- file outline ---"
ast-grep outline packages/opencode/test/lib/cli-process.ts --view compact 2>/dev/null | sed -n '1,160p' || true
echo "--- relevant lines 260-350 ---"
nl -ba packages/opencode/test/lib/cli-process.ts | sed -n '260,350p'
fi
# Search for related retry/cleanup patterns in this file.
echo "--- related cleanup/retry occurrences ---"
rg -n "readdir|rm|ENAENT|ENOENT|catch|retry|SQLite|sqlite|D11|opencode run" packages/opencode/test/lib/cli-process.ts || trueRepository: AltimateAI/altimate-code
Length of output: 297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- relevant lines 260-350 ---"
awk 'NR>=260 && NR<=350 { printf "%6d\t%s\n", NR, $0 }' packages/opencode/test/lib/cli-process.ts
echo "--- related cleanup/retry occurrences ---"
rg -n "readdir|rm|ENAENT|ENOENT|catch|retry|SQLite|sqlite|D11|opencode run" packages/opencode/test/lib/cli-process.ts || true
echo "--- package/imports context 1-60 ---"
awk 'NR>=1 && NR<=80 { printf "%6d\t%s\n", NR, $0 }' packages/opencode/test/lib/cli-process.tsRepository: AltimateAI/altimate-code
Length of output: 13179
🌐 Web query:
Node fsPromises readdir error object code missing directories ENOENT fs rm force option documentation
💡 Result:
In Node.js, fs.promises.readdir does not support a force option to ignore missing directories [1][2]. If you attempt to call fs.promises.readdir on a path that does not exist, the promise will reject with an ENOENT error [1][2]. To handle missing directories without crashing, you must explicitly check for the error or the existence of the path. Regarding the fs.promises.rm function, it does include a force option [3][4]. When the force option is set to true, fs.promises.rm will ignore exceptions if the path does not exist [3][4]. This is the recommended way to achieve behavior similar to the rm -rf command in Unix [3][4]. For fs.promises.readdir, if you want to avoid errors when a directory might be missing, you can use a try-catch block: try { const files = await fs.promises.readdir(path); // process files } catch (err) { if (err.code === 'ENOENT') { // handle the case where the directory does not exist } else { throw err; } } The confusion regarding the force option often stems from its specific availability in the fs.promises.rm method [3][4], which is designed for recursive removal, rather than in the directory reading methods [1][2].
Citations:
- 1: https://nodejs.org/api/fs.html
- 2: https://nodejs.org/dist/latest/docs/api/fs.html
- 3: https://nodejs.org/api/fs.md
- 4: https://nodejs.org/docs/latest-v23.x/api/fs.html
Surface SQLite cleanup failures before retrying.
The catch block also swallows readdir failures such as missing directories or permission errors, and any non-ENOENT removal failure. Since this block only suppresses cleanup errors, the retry can still start while SQLite state remains partially written or corrupted. Catch the error and rethrow unless error.code === "ENOENT", and do not proceed to the second spawn if cleanup failed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/test/lib/cli-process.ts` around lines 298 - 307, Update the
retry cleanup catch block in the test process flow to rethrow any cleanup error
unless its code is ENOENT, including readdir and non-ENOENT removal failures.
Ensure the second spawn occurs only after SQLite cleanup completes successfully
or an ENOENT is explicitly ignored.
| async function shOK(cmd: string): Promise<string> { | ||
| try { | ||
| const r = await $`sh -c ${cmd}`.quiet() | ||
| return r.text().trim() | ||
| } catch { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file list =="
git ls-files | rg '(^|/)check-tracker-leaks\.ts$|check-tracker-leaks'
echo "== file outline =="
ast-grep outline script/check-tracker-leaks.ts --view expanded || true
echo "== relevant sections =="
wc -l script/check-tracker-leaks.ts
sed -n '1,120p' script/check-tracker-leaks.ts
echo "---"
sed -n '120,200p' script/check-tracker-leaks.ts
echo "== usages of shOK/base/main references =="
rg -n "shOK|SKIP_TRACKER|altimate_change|\\$`|origin/main|--base|rev-list|changed" script/check-tracker-leaks.tsRepository: AltimateAI/altimate-code
Length of output: 6939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status/context =="
git status --short --branch --untracked-files=no 2>/dev/null || true
git --no-pager show --no-patch 2>/dev/null || true
echo "== check SKIP_TRACKER env =="
printf 'SKIP_TRACKER_CHECK=%q\n' "${SKIP_TRACKER_CHECK:-<unset>}"
echo "== inspect git commands from script text =="
python3 - <<'PY'
from pathlib import Path
p = Path('script/check-tracker-leaks.ts')
s = p.read_text()
for i,line in enumerate(s.splitlines(),1):
if 'shOK(' in line or 'mergeBase' in line or 'SKIP_TRACKER_CHECK' in line:
print(f'{i}: {line}')
PY
echo "== behavioral probe: shell escaping and shOK-like failure handling via child_process ==="
node - <<'JS'
const { execSync } = require('child_process');
function shOKLike(cmd) {
try {
const out = execSync(`sh -c ${cmd}`, { encoding: 'utf8', stdio: ['pipe','pipe','pipe'] }).trim();
return out;
} catch (e) {
return '';
}
}
for (const cmd of [
"git rev-parse --abbrevref HEAD",
"set -ex; echo x",
"git rev-parse --abbrev-ref HEAD",
]) {
console.log('cmd=', JSON.stringify(cmd), 'shOKLike=', JSON.stringify(shOKLike(cmd)));
}
try { execSync("git rev-parse --abbrev-ref HEAD"); } catch (e) { console.log('rev-parse direct stderr=', e.stderr?.toString().split('\n')[0]); }
JS
echo "== search SKIP_TRACKER_CHECK references repository-wide =="
rg -n "SKIP_TRACKER_CHECK" .Repository: AltimateAI/altimate-code
Length of output: 6323
Fail when required Git commands fail.
shOK turns every Git failure — including an invalid --base=<ref>, missing origin/main in a shallow repo, or failed merge-base/rev-list/log/diff — into an empty value. mergeBase then exits as a silent success. Make these queries fail hard unless SKIP_TRACKER_CHECK=1, and report instructions for fetching the base ref or correcting --base.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/check-tracker-leaks.ts` around lines 63 - 69, Update shOK and the
mergeBase flow so required Git query failures no longer become empty values or
silent success; propagate failures from merge-base, rev-list, log, and diff
unless SKIP_TRACKER_CHECK=1 is set. When the base ref is invalid or unavailable,
fail with a clear message explaining how to fetch the base ref or correct
--base, while preserving the skip behavior.
| const diff = await shOK(`git diff --unified=0 ${mergeBase}...HEAD`) | ||
| const added = diff | ||
| .split("\n") | ||
| .filter((l) => l.startsWith("+") && !l.startsWith("+++")) | ||
| .join("\n") | ||
| scanText(added, `${ahead}-commit diff vs ${base} (added lines)`, hits) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Parse diff hunks without dropping added content.
The !l.startsWith("+++") condition drops every rendered diff line that begins with +++. An added source line such as ++AI-1234 renders that way and bypasses the scanner. Track whether parsing is inside an @@ hunk, then accept all + lines only inside hunks.
Proposed fix
- const added = diff
- .split("\n")
- .filter((l) => l.startsWith("+") && !l.startsWith("+++"))
- .join("\n")
+ const addedLines: string[] = []
+ let inHunk = false
+ for (const line of diff.split("\n")) {
+ if (line.startsWith("diff --git ")) {
+ inHunk = false
+ continue
+ }
+ if (line.startsWith("@@")) {
+ inHunk = true
+ continue
+ }
+ if (inHunk && line.startsWith("+")) addedLines.push(line)
+ }
+ const added = addedLines.join("\n")Add a regression test for added content whose diff rendering starts with +++.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const diff = await shOK(`git diff --unified=0 ${mergeBase}...HEAD`) | |
| const added = diff | |
| .split("\n") | |
| .filter((l) => l.startsWith("+") && !l.startsWith("+++")) | |
| .join("\n") | |
| scanText(added, `${ahead}-commit diff vs ${base} (added lines)`, hits) | |
| const addedLines: string[] = [] | |
| let inHunk = false | |
| for (const line of diff.split("\n")) { | |
| if (line.startsWith("diff --git ")) { | |
| inHunk = false | |
| continue | |
| } | |
| if (line.startsWith("@@")) { | |
| inHunk = true | |
| continue | |
| } | |
| if (inHunk && line.startsWith("+")) addedLines.push(line) | |
| } | |
| const added = addedLines.join("\n") | |
| scanText(added, `${ahead}-commit diff vs ${base} (added lines)`, hits) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/check-tracker-leaks.ts` around lines 115 - 120, Update the diff
parsing around the added-line collection in the tracker leak checker to track
whether processing is inside an @@ hunk, then include every + line encountered
within hunks, including lines rendered with +++. Add a regression test covering
added content beginning with +++ and preserve exclusion of diff headers outside
hunks.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // | ||
| // If this reintroduces the unshare-net hang on CI, drop the Promise.then | ||
| // line — the snapshot alone still keeps release binaries functional offline. | ||
| Promise.resolve().then(() => ModelsDev.refresh().catch(() => {})) |
There was a problem hiding this comment.
WARNING: Boot-time refresh() here can hold the event loop under no-network, re-opening the D14 release-blocker
Promise.resolve().then(...) schedules refresh() on a microtask, but what kept the loop alive in the original setTimeout(...,0) bug was never the timer itself — it was the in-flight fetch()'s referenced I/O handle (and the synchronous getaddrinfo() under unshare --net, which AbortSignal.timeout(10000) can't cancel). The microtask runs refresh() → fetchApi() → fetch(...) almost immediately, so on a clean-cache cold start with no network the process blocks on DNS until the 10s timeout — exactly the v0.9.4 blocker D14 removed. The "a microtask can't itself keep Bun alive" rationale is true about the microtask but doesn't address the referenced fetch handle that follows it.
Since Bun's fetch exposes no .unref(), the only way to fire this without holding the loop is to not fire it at boot. The PR already documents the fallback ("drop the Promise.then line"); given the Phase 3 [10/10] no-internet sanity check is unchecked in the test plan, I'd drop this line now (or gate it behind confirmed network availability) rather than rely on CI to catch a reintroduced release-blocker.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // OUTSIDE packages/opencode; a `dir`-relative path for those would render as | ||
| // `../tui/src/...` which the smoke-test reader would then have to un-prefix. | ||
| // REPO_ROOT-relative keeps paths portable and the reader trivial. | ||
| const _stampRoot = path.resolve(dir, "../..") // repo root |
There was a problem hiding this comment.
SUGGESTION: _stampRoot duplicates REPO_ROOT (line 576) — both are path.resolve(dir, "../..")
These two declarations compute the identical value. _stampRoot (with the throwaway _ prefix) exists only because REPO_ROOT is declared further down, after addFile is defined. Hoist a single const REPO_ROOT = path.resolve(dir, "../..") above addFile and use it inside addFile, then drop _stampRoot so the relative base has one canonical name.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // | ||
| // Stamp format: JSON with one entry per input, sha256 of file content. Read | ||
| // side rehashes each listed path and compares; any mismatch → stale. Paths | ||
| // are relative to the workspace root (dir = packages/opencode) so the test |
There was a problem hiding this comment.
SUGGESTION: This comment is stale after the M2 change — stamp paths are repo-root-relative, not packages/opencode-relative
M2 switched addFile to path.relative(_stampRoot, absPath) where _stampRoot = path.resolve(dir, "../..") (the repo root), and the smoke-test reader now resolves entries against REPO_ROOT. So "Paths are relative to the workspace root (dir = packages/opencode)" is misleading on two counts: dir is packages/opencode (a subdirectory, not the workspace root), and the relative base is now dir/../.., not dir. Reword to state paths are relative to the repo root.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files this incremental pass)
Fix these issues in Kilo Cloud Previous Review Summary (commit 630accd)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 630accd)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (9 files)
Reviewed by glm-5.2 · Input: 59.6K · Output: 15.5K · Cached: 473.7K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="script/check-tracker-leaks.ts">
<violation number="1" location="script/check-tracker-leaks.ts:67">
P1: A failed git command is reported as a clean tracker scan: `shOK` swallows the error, and the resulting empty values either return early or skip the scans. A transient git failure, unavailable base, or malformed command can therefore let a push bypass this guard; distinguishing an expected no-base case from command failure and failing closed for the latter would preserve the protection.</violation>
<violation number="2" location="script/check-tracker-leaks.ts:154">
P1: Internal tracker references can still be pushed because no installed hook or package script invokes this entrypoint; wire `bun run script/check-tracker-leaks.ts` into the pre-push hook so the new guard actually runs.</violation>
</file>
<file name="packages/opencode/script/build.ts">
<violation number="1" location="packages/opencode/script/build.ts:616">
P2: Changes to this package’s `imports` or other bundler-relevant manifest fields leave the stamp fresh and allow the smoke test to run an outdated binary; stamp `packages/opencode/package.json` before skipping its source tree.</violation>
</file>
<file name="packages/opencode/src/provider/models.ts">
<violation number="1" location="packages/opencode/src/provider/models.ts:191">
P1: Cold-cache CLI invocations still start `fetchApi()` before exiting, so an offline/DNS-blocked request can retain the event loop and recreate the D14 hang. Remove this startup refresh; on-demand loading and the unref'd hourly interval already cover model updates.</violation>
</file>
<file name="packages/opencode/test/install/smoke-test-binary.test.ts">
<violation number="1" location="packages/opencode/test/install/smoke-test-binary.test.ts:191">
P2: The stamp-based staleness guard only rehashes files that were present at build time, so source files added after the build are never checked. Since `build.ts` emits the stamp from a fixed input list at build time, a `git pull` or an edit that only adds new files (new modules, new workspace packages) leaves the binary silently stale while `isBinaryStaleFromStamp` returns `false` and the tests run against the outdated binary. The previous mtime walk caught this (a new file has a newer mtime than the binary), but that walk now only runs in the `"no-stamp"` fallback, so it is effectively dead once a stamp exists.</violation>
</file>
<file name=".husky/pre-push">
<violation number="1" location=".husky/pre-push:24">
P2: The guard silently disables itself when the local base ref is missing: check-tracker-leaks.ts computes base from a hardcoded local `origin/main` (the pre-push hook passes no `--base`), and its shOK() swallows `git merge-base` failures, so when that ref isn't present on the developer machine the script returns a silent no-op success with no warning that the tracker scan was skipped. That leaves a false sense of security for a guard whose whole purpose is leak prevention. Consider passing the actual base and failing closed (or emitting a clear warning) when the merge-base can't be resolved instead of silently returning 0.</violation>
<violation number="2" location=".husky/pre-push:24">
P1: The pre-push hook can approve one ref while pushing another because it never passes the hook's pushed ref/update data to the scanner; the scanner always examines the current `HEAD`. A command such as `git push origin feature` from a clean `main` can therefore publish an unchecked branch. Reading the pre-push stdin tuples and scanning each pushed local OID (or restricting the hook to the checked-out ref) would align the check with the actual push.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // main() so RULES can be imported by the self-test file without triggering a | ||
| // scanner run at test-collection time. Bun sets `import.meta.main = true` only | ||
| // when this file is the entrypoint. | ||
| if (import.meta.main) { |
There was a problem hiding this comment.
P1: Internal tracker references can still be pushed because no installed hook or package script invokes this entrypoint; wire bun run script/check-tracker-leaks.ts into the pre-push hook so the new guard actually runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At script/check-tracker-leaks.ts, line 154:
<comment>Internal tracker references can still be pushed because no installed hook or package script invokes this entrypoint; wire `bun run script/check-tracker-leaks.ts` into the pre-push hook so the new guard actually runs.</comment>
<file context>
@@ -0,0 +1,160 @@
+// main() so RULES can be imported by the self-test file without triggering a
+// scanner run at test-collection time. Bun sets `import.meta.main = true` only
+// when this file is the entrypoint.
+if (import.meta.main) {
+ if (process.env.SKIP_TRACKER_CHECK === "1") {
+ process.stderr.write("tracker-leak check skipped via SKIP_TRACKER_CHECK=1\n")
</file context>
| // | ||
| // If this reintroduces the unshare-net hang on CI, drop the Promise.then | ||
| // line — the snapshot alone still keeps release binaries functional offline. | ||
| Promise.resolve().then(() => ModelsDev.refresh().catch(() => {})) |
There was a problem hiding this comment.
P1: Cold-cache CLI invocations still start fetchApi() before exiting, so an offline/DNS-blocked request can retain the event loop and recreate the D14 hang. Remove this startup refresh; on-demand loading and the unref'd hourly interval already cover model updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/provider/models.ts, line 191:
<comment>Cold-cache CLI invocations still start `fetchApi()` before exiting, so an offline/DNS-blocked request can retain the event loop and recreate the D14 hang. Remove this startup refresh; on-demand loading and the unref'd hourly interval already cover model updates.</comment>
<file context>
@@ -152,18 +159,38 @@ export namespace ModelsDev {
+ //
+ // If this reintroduces the unshare-net hang on CI, drop the Promise.then
+ // line — the snapshot alone still keeps release binaries functional offline.
+ Promise.resolve().then(() => ModelsDev.refresh().catch(() => {}))
+ setInterval(async () => {
+ await ModelsDev.refresh()
</file context>
| Promise.resolve().then(() => ModelsDev.refresh().catch(() => {})) | |
| // Refresh occurs on demand and via the unref'd interval below. |
| # altimate_change — #1052 D8: scan pushed content for internal-tracker refs | ||
| # (see RULES in the script for the specific patterns). Silent on clean; | ||
| # exits 1 on hit. Bypass with SKIP_TRACKER_CHECK=1 for genuine emergencies. | ||
| bun script/check-tracker-leaks.ts |
There was a problem hiding this comment.
P1: The pre-push hook can approve one ref while pushing another because it never passes the hook's pushed ref/update data to the scanner; the scanner always examines the current HEAD. A command such as git push origin feature from a clean main can therefore publish an unchecked branch. Reading the pre-push stdin tuples and scanning each pushed local OID (or restricting the hook to the checked-out ref) would align the check with the actual push.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .husky/pre-push, line 24:
<comment>The pre-push hook can approve one ref while pushing another because it never passes the hook's pushed ref/update data to the scanner; the scanner always examines the current `HEAD`. A command such as `git push origin feature` from a clean `main` can therefore publish an unchecked branch. Reading the pre-push stdin tuples and scanning each pushed local OID (or restricting the hook to the checked-out ref) would align the check with the actual push.</comment>
<file context>
@@ -18,3 +18,7 @@ if (process.versions.bun !== expectedBunVersion) {
+# altimate_change — #1052 D8: scan pushed content for internal-tracker refs
+# (see RULES in the script for the specific patterns). Silent on clean;
+# exits 1 on hit. Bypass with SKIP_TRACKER_CHECK=1 for genuine emergencies.
+bun script/check-tracker-leaks.ts
</file context>
| try { | ||
| const r = await $`sh -c ${cmd}`.quiet() | ||
| return r.text().trim() | ||
| } catch { |
There was a problem hiding this comment.
P1: A failed git command is reported as a clean tracker scan: shOK swallows the error, and the resulting empty values either return early or skip the scans. A transient git failure, unavailable base, or malformed command can therefore let a push bypass this guard; distinguishing an expected no-base case from command failure and failing closed for the latter would preserve the protection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At script/check-tracker-leaks.ts, line 67:
<comment>A failed git command is reported as a clean tracker scan: `shOK` swallows the error, and the resulting empty values either return early or skip the scans. A transient git failure, unavailable base, or malformed command can therefore let a push bypass this guard; distinguishing an expected no-base case from command failure and failing closed for the latter would preserve the protection.</comment>
<file context>
@@ -0,0 +1,160 @@
+ try {
+ const r = await $`sh -c ${cmd}`.quiet()
+ return r.text().trim()
+ } catch {
+ return ""
+ }
</file context>
| for (const pkg of fs.readdirSync(packagesRoot, { withFileTypes: true })) { | ||
| if (!pkg.isDirectory() || pkg.name.startsWith(".")) continue | ||
| // Skip packages/opencode — already covered by the walks above. | ||
| if (pkg.name === "opencode") continue |
There was a problem hiding this comment.
P2: Changes to this package’s imports or other bundler-relevant manifest fields leave the stamp fresh and allow the smoke test to run an outdated binary; stamp packages/opencode/package.json before skipping its source tree.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/script/build.ts, line 616:
<comment>Changes to this package’s `imports` or other bundler-relevant manifest fields leave the stamp fresh and allow the smoke test to run an outdated binary; stamp `packages/opencode/package.json` before skipping its source tree.</comment>
<file context>
@@ -523,6 +525,124 @@ for (const item of targets) {
+ for (const pkg of fs.readdirSync(packagesRoot, { withFileTypes: true })) {
+ if (!pkg.isDirectory() || pkg.name.startsWith(".")) continue
+ // Skip packages/opencode — already covered by the walks above.
+ if (pkg.name === "opencode") continue
+ const pkgSrc = path.join(packagesRoot, pkg.name, "src")
+ if (fs.existsSync(pkgSrc)) walk(pkgSrc)
</file context>
| if (pkg.name === "opencode") continue | |
| if (pkg.name === "opencode") { | |
| addFile(path.join(packagesRoot, pkg.name, "package.json")) | |
| continue | |
| } |
| // altimate_change — #1052 D10 review-fix (M2): stamp paths are now REPO_ROOT- | ||
| // relative so entries under packages/tui, packages/core, workspace-root | ||
| // package.json, bun.lock, etc. resolve correctly without further munging. | ||
| for (const { path: rel, sha256 } of stamp.inputs) { |
There was a problem hiding this comment.
P2: The stamp-based staleness guard only rehashes files that were present at build time, so source files added after the build are never checked. Since build.ts emits the stamp from a fixed input list at build time, a git pull or an edit that only adds new files (new modules, new workspace packages) leaves the binary silently stale while isBinaryStaleFromStamp returns false and the tests run against the outdated binary. The previous mtime walk caught this (a new file has a newer mtime than the binary), but that walk now only runs in the "no-stamp" fallback, so it is effectively dead once a stamp exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/install/smoke-test-binary.test.ts, line 191:
<comment>The stamp-based staleness guard only rehashes files that were present at build time, so source files added after the build are never checked. Since `build.ts` emits the stamp from a fixed input list at build time, a `git pull` or an edit that only adds new files (new modules, new workspace packages) leaves the binary silently stale while `isBinaryStaleFromStamp` returns `false` and the tests run against the outdated binary. The previous mtime walk caught this (a new file has a newer mtime than the binary), but that walk now only runs in the `"no-stamp"` fallback, so it is effectively dead once a stamp exists.</comment>
<file context>
@@ -148,16 +150,76 @@ function isBinaryStale(binaryPath: string): boolean {
+ // altimate_change — #1052 D10 review-fix (M2): stamp paths are now REPO_ROOT-
+ // relative so entries under packages/tui, packages/core, workspace-root
+ // package.json, bun.lock, etc. resolve correctly without further munging.
+ for (const { path: rel, sha256 } of stamp.inputs) {
+ const abs = path.join(REPO_ROOT, rel)
+ const current = sha256File(abs)
</file context>
| # altimate_change — #1052 D8: scan pushed content for internal-tracker refs | ||
| # (see RULES in the script for the specific patterns). Silent on clean; | ||
| # exits 1 on hit. Bypass with SKIP_TRACKER_CHECK=1 for genuine emergencies. | ||
| bun script/check-tracker-leaks.ts |
There was a problem hiding this comment.
P2: The guard silently disables itself when the local base ref is missing: check-tracker-leaks.ts computes base from a hardcoded local origin/main (the pre-push hook passes no --base), and its shOK() swallows git merge-base failures, so when that ref isn't present on the developer machine the script returns a silent no-op success with no warning that the tracker scan was skipped. That leaves a false sense of security for a guard whose whole purpose is leak prevention. Consider passing the actual base and failing closed (or emitting a clear warning) when the merge-base can't be resolved instead of silently returning 0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .husky/pre-push, line 24:
<comment>The guard silently disables itself when the local base ref is missing: check-tracker-leaks.ts computes base from a hardcoded local `origin/main` (the pre-push hook passes no `--base`), and its shOK() swallows `git merge-base` failures, so when that ref isn't present on the developer machine the script returns a silent no-op success with no warning that the tracker scan was skipped. That leaves a false sense of security for a guard whose whole purpose is leak prevention. Consider passing the actual base and failing closed (or emitting a clear warning) when the merge-base can't be resolved instead of silently returning 0.</comment>
<file context>
@@ -18,3 +18,7 @@ if (process.versions.bun !== expectedBunVersion) {
+# altimate_change — #1052 D8: scan pushed content for internal-tracker refs
+# (see RULES in the script for the specific patterns). Silent on clean;
+# exits 1 on hit. Bypass with SKIP_TRACKER_CHECK=1 for genuine emergencies.
+bun script/check-tracker-leaks.ts
</file context>
…s.dev cache Addresses the actionable findings from the CodeRabbit + cubic-ai + kilo-code bot reviews on PR #1085 (Claude review skipped — not @claude-review'd). Only real issues; noisy or already-documented findings are noted in the PR reply. Scanner (`script/check-tracker-leaks.ts`): - **Cubic P1 (security):** the `--base` arg used to be embedded in `sh -c "${cmd}"`, which the shell then re-parsed — a caller could smuggle arbitrary shell via `--base=$(...)`. Replace with a shell-free `git(args[])` helper using Bun.$ tagged templates (each arg becomes one argv element, no shell). Belt-and-braces: reject `--base` values that don't look like a git ref (`^[A-Za-z0-9/_.@{}~^-]+$`) before running any git command, so a bad value fails loud instead of silently. - **Cubic P1 (correctness):** `shOK` used to `catch { return "" }`, so a real git failure (missing binary, corrupt index) reported as a clean scan. New helper `git()` fails loud with exit code 2 on unexpected errors; only `merge-base` (which legitimately returns empty on diverged history) opts into the silent path. - **CodeRabbit Major:** the diff parser's `!startsWith("+++")` filter dropped legitimate content lines starting with `++` (e.g. an added an added line whose text starts with two plus signs renders as `+++...` in unified-diff). Match the file header exactly (`+++ ` or `+++\t`) so a content line whose prefix happens to look like `+++<text>` still gets scanned. Build stamp (`packages/opencode/script/build.ts`): - **Kilo suggestion:** `_stampRoot` and `REPO_ROOT` computed the same value twice. Consolidated to one `REPO_ROOT` at the top of the block. - **Kilo suggestion:** stale comment claimed paths were "relative to the workspace root (dir = packages/opencode)" — post-M2 they're REPO_ROOT- relative. Corrected. - **Cubic P2:** `tsconfig.json` changes can flip target / moduleResolution and change the compiled output shape without editing any `.ts` file. Added `packages/opencode/tsconfig.json` to the stamp. models.dev cache (`packages/opencode/src/provider/models.ts`): - **CodeRabbit Minor:** a 2xx response can still carry HTML (proxies, error pages that respond 200) or truncated JSON. The old code wrote `result2.text` to the disk cache BEFORE the parse, so a bad body poisoned the cache for the next run + crashed on the current call. Now parses first; only caches + returns on success. On parse failure, log with a body preview and return an empty catalog (same graceful path as the non-2xx branch). Not addressed (deferred / disagreed / duplicated with known caveats): - Cubic P1 "cold-cache CLI still starts fetchApi → recreates D14 blocker" and kilo warning on the same line — these identify the trade-off the D14 review-fix commit explicitly documents; no change. - Cubic P1 "hook doesn't wire to `bun run script/check-tracker-leaks.ts`" — false positive; `.husky/pre-push` does invoke it. - CodeRabbit + cubic P2 "new files added post-build not detected" — real limitation, orthogonal fix, deferred to a follow-up (adding a walk at read-time would double the cost of every test run). Verified: typecheck 13/13; scanner self-test 28/28; scanner clean on this branch; scanner rejects `--base=$(...)` with exit 2; `--version` still exits in ~1s cleanly.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Bot-review round — responseWent through the CodeRabbit, cubic-ai, and kilo-code bot findings. Applied fixes in commit AddressedScanner (
Build stamp (
models.dev cache (
Not addressed (with reasoning)
CodeRabbit's original "CRITICAL: duplicate Verified: typecheck 13/13, scanner self-test 28/28, scanner runs clean on this branch, security bypass rejected with exit 2. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/opencode/script/build.ts (1)
543-544: 🎯 Functional Correctness | 🔴 CriticalRemove the duplicate
stampInputsdeclaration.Line 544 redeclares
stampInputsin the same scope as Line 543. The TypeScript build fails before the input-stamp logic can run. Keep one declaration.Proposed fix
const REPO_ROOT = path.resolve(dir, "../..") const stampInputs: Array<{ path: string; sha256: string }> = [] - const stampInputs: Array<{ path: string; sha256: string }> = []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/script/build.ts` around lines 543 - 544, Remove the duplicate stampInputs declaration in the build script, keeping a single declaration in the surrounding scope so the input-stamp logic compiles and runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/opencode/script/build.ts`:
- Around line 543-544: Remove the duplicate stampInputs declaration in the build
script, keeping a single declaration in the surrounding scope so the input-stamp
logic compiles and runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9be843ba-5cfb-44bb-9471-daab1c92a5e2
📒 Files selected for processing (3)
packages/opencode/script/build.tspackages/opencode/src/provider/models.tsscript/check-tracker-leaks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/provider/models.ts
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="script/check-tracker-leaks.ts">
<violation number="1" location="script/check-tracker-leaks.ts:125">
P3: The new `git` helper defaults `failOnError: true`, and the branch lookup runs before the `merge-base` fallback. On a freshly initialized repository with no commits, `git rev-parse --abbrev-ref HEAD` exits 128 (unborn HEAD: "fatal: ambiguous argument 'HEAD'"), so the script exits 2 loudly instead of succeeding silently. This contradicts the intent stated right below for `mergeBase` — "brand-new repo or base doesn't exist locally → Silent success" — because that fallback is unreachable in the exact zero-commit case. The previous `catch { return "" }` handled this. Consider resolving `--abbrev-ref HEAD` with `failOnError: false` (or resolving the branch only when `ahead`/commits exist) so an empty repo is treated as a clean scan rather than a hard failure.</violation>
</file>
<file name="packages/opencode/src/provider/models.ts">
<violation number="1" location="packages/opencode/src/provider/models.ts:141">
P3: The added parse guard only verifies that the fetch body is syntactically valid JSON, not that it is actually the models catalog. A 200 response body that is valid JSON but not an object (e.g. `null`, `[]`, or a scalar — common from misconfigured proxies/load-balancers that set a JSON content-type) passes `JSON.parse`, is written to the on-disk cache (which then counts as fresh for the TTL), and is returned. `ModelsDev.get()` casts this to `Record<string, Provider>` and `Provider.state` (`fromModelsDevProvider` → `provider.models`/`provider.id`) then iterates/maps it, which can throw or produce a broken provider table. Consider validating that `parsed` is a plain, non-array object (and ideally matches the `Provider` catalog record shape) before writing it to the cache, so the "don't poison the disk cache" behavior actually holds for structured-but-wrong bodies too.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| process.exit(2) | ||
| } | ||
|
|
||
| const branch = await git(["rev-parse", "--abbrev-ref", "HEAD"]) |
There was a problem hiding this comment.
P3: The new git helper defaults failOnError: true, and the branch lookup runs before the merge-base fallback. On a freshly initialized repository with no commits, git rev-parse --abbrev-ref HEAD exits 128 (unborn HEAD: "fatal: ambiguous argument 'HEAD'"), so the script exits 2 loudly instead of succeeding silently. This contradicts the intent stated right below for mergeBase — "brand-new repo or base doesn't exist locally → Silent success" — because that fallback is unreachable in the exact zero-commit case. The previous catch { return "" } handled this. Consider resolving --abbrev-ref HEAD with failOnError: false (or resolving the branch only when ahead/commits exist) so an empty repo is treated as a clean scan rather than a hard failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At script/check-tracker-leaks.ts, line 125:
<comment>The new `git` helper defaults `failOnError: true`, and the branch lookup runs before the `merge-base` fallback. On a freshly initialized repository with no commits, `git rev-parse --abbrev-ref HEAD` exits 128 (unborn HEAD: "fatal: ambiguous argument 'HEAD'"), so the script exits 2 loudly instead of succeeding silently. This contradicts the intent stated right below for `mergeBase` — "brand-new repo or base doesn't exist locally → Silent success" — because that fallback is unreachable in the exact zero-commit case. The previous `catch { return "" }` handled this. Consider resolving `--abbrev-ref HEAD` with `failOnError: false` (or resolving the branch only when `ahead`/commits exist) so an empty repo is treated as a clean scan rather than a hard failure.</comment>
<file context>
@@ -90,32 +113,48 @@ async function main() {
+ process.exit(2)
+ }
+
+ const branch = await git(["rev-parse", "--abbrev-ref", "HEAD"])
+ // merge-base can legitimately return empty (no shared history) — don't fail loud on that.
+ const mergeBase = await git(["merge-base", "HEAD", base], { failOnError: false })
</file context>
| if (!result2.ok) return {} | ||
| let parsed: Record<string, unknown> | ||
| try { | ||
| parsed = JSON.parse(result2.text) |
There was a problem hiding this comment.
P3: The added parse guard only verifies that the fetch body is syntactically valid JSON, not that it is actually the models catalog. A 200 response body that is valid JSON but not an object (e.g. null, [], or a scalar — common from misconfigured proxies/load-balancers that set a JSON content-type) passes JSON.parse, is written to the on-disk cache (which then counts as fresh for the TTL), and is returned. ModelsDev.get() casts this to Record<string, Provider> and Provider.state (fromModelsDevProvider → provider.models/provider.id) then iterates/maps it, which can throw or produce a broken provider table. Consider validating that parsed is a plain, non-array object (and ideally matches the Provider catalog record shape) before writing it to the cache, so the "don't poison the disk cache" behavior actually holds for structured-but-wrong bodies too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/provider/models.ts, line 141:
<comment>The added parse guard only verifies that the fetch body is syntactically valid JSON, not that it is actually the models catalog. A 200 response body that is valid JSON but not an object (e.g. `null`, `[]`, or a scalar — common from misconfigured proxies/load-balancers that set a JSON content-type) passes `JSON.parse`, is written to the on-disk cache (which then counts as fresh for the TTL), and is returned. `ModelsDev.get()` casts this to `Record<string, Provider>` and `Provider.state` (`fromModelsDevProvider` → `provider.models`/`provider.id`) then iterates/maps it, which can throw or produce a broken provider table. Consider validating that `parsed` is a plain, non-array object (and ideally matches the `Provider` catalog record shape) before writing it to the cache, so the "don't poison the disk cache" behavior actually holds for structured-but-wrong bodies too.</comment>
<file context>
@@ -129,11 +129,27 @@ export namespace ModelsDev {
if (!result2.ok) return {}
+ let parsed: Record<string, unknown>
+ try {
+ parsed = JSON.parse(result2.text)
+ } catch (e) {
+ log.error("models.dev returned non-JSON body; not caching", {
</file context>
1 participant
Issue for this PR
Closes #1052 (partial — see "Not in this PR" below)
Type of change
What does this PR do?
Fixes 5 of 13 deferred items from #1052 (the v0.9.4 post-release cleanup tracking issue), plus 3 review-fixes flagged by a 6-model consensus code review on the initial 5 commits, plus one follow-up scrub.
Selection rule: the user (@sahrizvi) asked to fix items not attributable to @saravanan-altimate. That covers D8, D9, D10, D11, D12, D14 from the tracking issue. D9 and D13 stayed deferred (D9 crosses TUI + auth + manifest storage — bigger than a bundled fix; D13 is a gateway-protocol change, not a CLI patch).
Commits (bottom-up, oldest first)
Original D-item fixes:
chore(hygiene): [#1052 D8] pre-push scan for internal-tracker refs— scanner + hook + docs. CI mirror deferred to a follow-up PR (needsworkflow-scoped token).test(build): [#1052 D10] stamp-based staleness guard for the smoke test— build.ts emitsdist/<target>/bin/build-inputs.jsonwith sha256s; smoke test compares against it.test(harness): [#1052 D11] idempotent retry in cli-process.run()— scrubopencode*.db*before the SQLite-lock retry so the second attempt starts clean.test(tui): [#1052 D12] deterministic regression test for phaseLabel()— util-level unit test replacing the flaky PTY e2e.fix(models): [#1052 D14] drop eager import-time ModelsDev.refresh()— the fetch was holding the event loop underunshare --neton CI. Snapshot covers cold-start.Review-fixes (from a 6-model consensus code review of the above):
6.
fix(hygiene): [#1052 D8 review-fix M1] catch suffix-adjacent tracker leaks + self-test— regex missed<prefix>-<n><suffix>(5/6 reviewers flagged, some as CRITICAL). Also adds a 28-case self-test file for the RULES.7.
test(build): [#1052 D10 review-fix M2] widen stamp to workspace packages + lockfile— the original stamp missedpackages/{tui,core,util,plugin,...}/src, the workspace-rootpackage.json, andbun.lock. Now walks each workspacesrc/and hashes those files.8.
fix(models): [#1052 D14 review-fix M3] don't crash on non-JSON error body + fire refresh at boot— pre-existingJSON.parse(<HTML>)crash on 5xx from models.dev + a fire-and-forgetPromise.resolve().then(refresh)so short-lived commands still warm the cache without holding the loop.9.
fix(hygiene): [#1052 D8 review-fix follow-up] scrub example strings from scanner source— the M1 commit accidentally embedded concrete tracker-key literals in its own test file + doc comments. Fixtures now build the strings at runtime; scanner source drops the verbatim examples.Not in this PR (still open on #1052)
useConnected()regression detector. Crosses TUI + auth + manifest storage; deserves its own PR..github/workflows/tracker-leak-check.ymlfile was in the local branch but this session's token lacks theworkflowscope; add it in a follow-up PR.Test plan
bun turbo typecheck— 13/13 cleanbun test packages/opencode/test/skill/tracker-leak-check.test.ts— 28/28 (scanner RULES)bun test packages/tui/test/util/phase-label.test.ts— 4/4 (phaseLabel util)bun script/check-tracker-leaks.tson the branch itself — silent (no leaks)bun run --cwd packages/opencode --conditions=browser ./src/index.ts --version— exits in ~1s, no hangD14 caveat worth explicit reviewer attention
Commit 8 adds
Promise.resolve().then(() => ModelsDev.refresh().catch(() => {}))alongside the hourly interval. A microtask doesn't itself keep Bun alive, and the fetch it schedules will just be abandoned at process-exit if unresolved — but if this reintroduces the original v0.9.4 blocker (sanity Phase 3 [10/10] under Linuxunshare --net), the fix is to delete that one line and take the "snapshot-only cold-start" trade-off documented in commit 5.🤖 Generated with Claude Code
https://claude.ai/code/session_01Q8FGy89Qpr39k8nCSpCcK2
Summary by cubic
Fixes 5 deferred items from #1052 plus 3 review fixes, and hardens the tracker scan, build stamp, and models cache. Adds a pre-push tracker check, a stamp-based staleness guard, safer
models.devboot, and an idempotent CLI retry.New Features
script/check-tracker-leaks.tschecks branch name, commit messages, and added diff lines for internal tracker refs; wired in.husky/pre-push(bypass withSKIP_TRACKER_CHECK=1);CONTRIBUTING.mdupdated.build-inputs.json(REPO_ROOT-relative) with sha256 for all embedded inputs; smoke test rehashes to detect stale binaries. Covers workspacepackages/*/src, per-package and rootpackage.json,bun.lock, andpackages/opencode/tsconfig.json.Bug Fixes
models.dev: drop eager import-time fetch; parse body before caching and return empty on non-JSON or non-2xx; fire-and-forget refresh at boot and keep hourly.unref()refresh.opencode*.db{,-wal,-shm}before a single retry on “database is locked”; deterministic unit test forphaseLabel()replaces the flaky PTY e2e.gitwith--baseref validation and a safer diff filter (avoids dropping valid++...lines); scrubbed literal examples from source.Written for commit 5b30a30. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests