Skip to content

fix: bug-hunt pass — 35 fixes across streaming, usage, providers, models and tooling - #157

Merged
ltmoerdani merged 36 commits into
ltmoerdani:mainfrom
Fahad090NP:fix/bug-hunt-round1
Aug 17, 2026
Merged

fix: bug-hunt pass — 35 fixes across streaming, usage, providers, models and tooling#157
ltmoerdani merged 36 commits into
ltmoerdani:mainfrom
Fahad090NP:fix/bug-hunt-round1

Conversation

@Fahad090NP

@Fahad090NP Fahad090NP commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

📝 What does this change?

A bug-hunt pass over the whole codebase — 35 small, atomic, one-fix-per-commit changes (32 files, +505/−128). Every fix was verified against the OpenCode gateway source and VS Code source, and the full lint gate (eslint strict + tsc + prettier + markdownlint + shellcheck + all unit tests) is green.

Highlights (confirmed bugs)

Crashes / dangerous

  • Webview Models tab crashed with ReferenceError: esc is not defined — added the missing HTML-escape helper.
  • Tool-schema sanitizer could infinite-recurse on a cyclic schema and blow the extension-host stack — added a cycle guard (+ tests).

Streaming

  • 5xx Router.Unavailable retry branch was dead (the 5xx body was never read) — now read + classified.
  • Cancellation during 5xx backoff surfaced a misleading stale gateway error — now fails cleanly as "cancelled".
  • Google tool calls had empty ids and per-chunk indices — now get stable synthetic ids.
  • Reasoning could be silently dropped when no progress sink existed — now falls back to the legacy emit path.
  • Reasoning marker data part was counted as phantom tokens in estimates — now treated as internal data.
  • Empty-response diagnostics were missing on the Anthropic/Google transports — added.

Usage / cost

  • Day bucketing used Math.round → afternoon events landed in the next day — now floor (+ regression test).
  • Subscription percent could go negative with a baseline over-correction — now clamped to [0,100].
  • Unknown/missing models tracked as $0 offline — now get a conservative fallback estimate.
  • sqlite3 history read capped at 64MB → large histories silently vanished — raised to 256MB.
  • Profile deletion left serverUsage/everTracked stale keys — now cleared.
  • The active profile was being silently overridden by provider re-resolution ~every 300ms — a user's explicit choice is now respected.
  • formatUsd rendered $-5.00; escapeHtml missed ' — fixed.

Provider / models

  • family was a per-model unique string, breaking VS Code's family selection/grouping — now a real family name.
  • Thinking setting values were unvalidated (a bad reasoning_effort → 400) — now validated against the allowed enums; the picker derives from the same allowlist.
  • temperature and the timeout settings were unclamped — now bounded.
  • Tool schemas lost top-level enums and common keywords (pattern/format/default) — preserved (+ tests).
  • Model-list sync drift (dead ring-2.6-1t, missing minimax-m3 in the Go fallback) — cleaned up.

Agents / vision / misc

  • autoEnableAgentsWindow off never reverted the auto-enabled core settings — now reverts.
  • Autocomplete stale-token race could kill the newest pending suggestion — fixed.
  • Vision proxy: empty descriptions now keep a placeholder; whole-conversation mode reuses the cache.
  • reasoning_content preserved when consecutive assistant messages merge.
  • Diagnostics: one failing vendor no longer aborts the whole report; retry-after durations are capped.

🧪 How did you test it?

  • npm run lint — all 7 steps green, incl. the full unit suite.
  • Pre-commit gate exercised on every one of the 35 commits.
  • Verified against tmp/vscode + tmp/opencode reference sources where behavior depended on VS Code / the gateway.

✅ Checklist

  • npm run compile passes
  • npm run lint passes
  • npm test passes
  • npm run package produces a VSIX (not run — code + tests only)
  • I tested it works (live API verification still recommended for streaming/tool-call paths)
  • I updated docs/CHANGELOG if needed (left for release-time per repo convention)

PEACE BE UPON YOU 🤲

@Fahad090NP

Copy link
Copy Markdown
Contributor Author

Hi @ltmoerdani 👋 — I know this is a big PR, so a few words up front to make it easy.

1. It's 35 commits because they're deliberately one-fix-each and tiny. No mega-diff hiding refactors — every commit is a single, self-contained bug fix with its own message (fix(area): …). You can review commit-by-commit and safely skip what you don't care about; nothing depends on anything else.

2. Scope is strictly corrective, not creative. I did not restructure anything. The largest single file change is the tool-schema cycle guard (~30 lines + tests); everything else is a few lines. No renames, no reorgs, no behavior I invented.

3. Every commit passed the pre-commit gate, and the full npm run lint (eslint strict + tsc + prettier + markdownlint + shellcheck + all unit tests) is green on the merged head. CI runs on this PR too.

4. Honesty about confidence. The description separates verified fixes from the ones that really want a quick live smoke test:

  • Needs a live run before release: the 5xx Router.Unavailable retry path, Google/Gemini tool-call streaming, vision-proxy empty-description path, and the DeepSeek reasoning-marker echo.
  • Verified by reading + unit tests: everything else (the two crash fixes have dedicated tests, plus regression tests for the day-bucket, schema, and utils changes).

5. A few things I deliberately did NOT touch, so you're not surprised they're absent: the contextWindowHook global monkey-patching (required for the feature — kept), the autocomplete Go-only key resolution (documented), and AGENTS.md/docs (separate PR #156).

6. Three "bugs" I found turned out to be false positives — I rejected them and noted why in the PR thread (fragmented tool-call names are intentional + tested; the token-estimate "double count" isn't one; the Qwen vision-proxy thinking path was already guarded). I'd rather say "I checked and it's fine" than churn code for nothing.

If any fix feels wrong or too aggressive, I'm happy to drop or rework that single commit — they're all independent, so carving any out is trivial. Happy to walk through any specific one.

@Fahad090NP

Fahad090NP commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

What these fixes prevent in the real world

1. Crashes that users would actually hit

  • Clicking the "Models" tab in the Usage panel crashed (missing esc() function) — the panel would just break/error.
  • A tool with a circular schema (some agents/MCP tools have these) could crash the whole extension host with a stack overflow — not just the chat, everything in VS Code's extension host.

2. Requests that fail for no good reason

  • During a gateway hiccup (502/503), the retry logic silently didn't retry the Router.Unavailable case, so users saw errors they could have waited through.
  • If the user cancelled during a retry, they got a scary "502 gateway error" instead of a clean "cancelled".
  • A typo in a Thinking setting (e.g. deepseek: "banana") sent a bad reasoning_effortevery request 400s with no clear reason.
  • A temperature above 2 in settings → 400 on models that reject it.

3. Wrong or silent usage/cost data (the worst kind)

  • Afternoon usage counted on the wrong day — the Today/Yesterday/Codebase numbers were subtly wrong.
  • Negative percentages ("-5% used") when a manual usage target over-corrected.
  • Brand-new or unknown models tracked as $0 — looked free, so users' real spend was underreported and surprises came at billing.
  • Heavy CLI users saw an empty Usage panel (the SQLite read silently failed past 64 MB).
  • Phantom tokens in estimates from internal reasoning markers — wrong token/cost figures.
  • Deleting a profile didn't fully erase it — stale meters could come back.
  • With multiple API keys, the "active" account silently jumped around — you could be looking at the wrong account's usage.

4. Broken tool calling / multi-turn reasoning

  • Google/Gemini tool calls had empty IDs → tool calls could get mixed up or lost in agent loops.
  • Tool-call names could duplicate (getWeathergetWeather) if a gateway re-sent the name.
  • Reasoning text could be silently dropped in some paths — breaking DeepSeek-style multi-turn conversations (the recurring 400).

5. Annoying / confusing UX

  • Ghost-text autocomplete sometimes never appeared (a stale request killed the new one).
  • Turning off auto-enable Agents window never reverted the settings the extension had flipped.
  • Vision proxy empty results stripped images with a misleading "proxy unavailable" message.
  • One bad vendor killed the whole Diagnostics report.

6. Money-display mistakes

  • Costs showed as $-5.00 (sign in the wrong place); HTML escaping missed single quotes.

— Every one of these is now fixed or explicitly verified-not-a-bug; the list maps 1:1 to the commits in this PR.

@ltmoerdani

Copy link
Copy Markdown
Owner

Hi @Fahad090NP,

Reviewed this the slow way: checked out the branch, ran the full unit suite (324/324 green), typecheck clean, then spot-checked the riskier claims against main and the live gateway before trusting the writeup. That last step doesn't always survive contact with a 35-commit PR, so credit where it's due: everything I checked held up.

  • esc() missing in the Models tab: confirmed. dashboard.ts on main calls it three times with no definition. Real crash.
  • The 5xx Router.Unavailable retry: also confirmed dead. isTransientServerError needs the body for non-502/503/504 5xx, but consumedErrorBody was only ever set on the 400 path, so the branch could never fire. Nice find.
  • Day bucketing round vs floor: agreed. With day-aligned buckets, an afternoon event lands in tomorrow.
  • Model drift checks out against the live list: minimax-m3 is there, ring-2.6-1t is gone.
  • The active-profile override is the fix I'm happiest about. ensureProfileSync running unconditionally from provideLanguageModelChatInformation has bugged me for a while, and this is a clean way to stop it.

Two small things before merge, both doc-level:

  1. The description lists the getWeathergetWeather name-duplication fix under Streaming, but there's no change for it in the diff and toolCallAccumulator.ts is untouched. You already noted in the thread you rejected it as a false positive, so just drop that line so nobody hunts for a commit that doesn't exist.
  2. The comment in dashboard.ts cites (issue #51), but [BUG] Spamming with notifications: Could not fetch OpenCode Go (Agents) model list #51 is the model-list fetch notification spam. The relevant one is [BUG] When I have two opencode, the usage panel display error #63, the usage panel with two opencode profiles. Mind pointing the comment there? Happy to fix it myself on merge if you'd rather not rebase.

One judgment call I want to flag rather than ask you to change: UNKNOWN_GO_MODEL_PRICE (0.5 in / 2.0 out) plus the "family estimate" entries means unknown models now over-report instead of reading as free. $0 is the worse failure, I agree. But an over-report has its own cost: someone watching their cheap daily driver get priced like a premium model may stop trusting the tracker. Product decision more than a bug fix, and worth getting real prices into the snapshot once they land.

Plan: merge as a merge commit per repo convention (never squash, I want all 35 commits in the history), then live smoke test the streaming paths (5xx retry, Google tool calls, vision placeholder) before the next release. No code changes requested.

Thanks for this. The honesty in the checklist and the rejected false-positives writeup made reviewing a 35-commit PR a lot easier than it usually is.

@Fahad090NP

Copy link
Copy Markdown
Contributor Author

Both review notes are addressed:

  1. Dropped the getWeathergetWeather line from the description — the false-positive note is gone so nobody hunts for a commit that doesn't exist.
  2. Fixed the dashboard.ts comment: it now cites (issue #63) (usage panel with two opencode profiles) instead of [BUG] Spamming with notifications: Could not fetch OpenCode Go (Agents) model list #51 (model-list fetch notification spam). Committed as docs(usage): point active-profile override comment at the right issue (#63) on the branch.

The 5xx retry / Google tool-call / vision placeholder live smoke test plan still stands for after merge. Thanks for the thorough review.

@ltmoerdani

Copy link
Copy Markdown
Owner

Hi @Fahad090NP,

Both confirmed on my end. Pulled the branch again, checked the new commit, and the dashboard.ts comment now points at #63. Grep on the PR description also comes back clean, so the getWeathergetWeather line is gone. Thanks for picking both up that fast.

Nothing else from me. The plan stands: merge as a merge commit (never squash, I want all 36 commits in the history), then live smoke test the streaming paths (5xx retry, Google tool calls, vision placeholder) before the next release.

Nice work on this one. The one-fix-per-commit structure and the honest checklist made it a genuinely easy PR to review.

@ltmoerdani
ltmoerdani merged commit a830618 into ltmoerdani:main Aug 17, 2026
2 checks passed
@Fahad090NP
Fahad090NP deleted the fix/bug-hunt-round1 branch August 17, 2026 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants