fix: high-severity bug batch from comprehensive review by gfargo · Pull Request #1646 · gfargo/coco

gfargo · 2026-07-12T12:23:40Z

What

Six atomic fixes for the high-severity findings from a comprehensive codebase review (issues filed as #1577–#1644), plus the 16 medium-severity fixes and the resolution of this PR's own review feedback. Each is a standalone commit; normally these would be separate PRs, but they were fixed together as a batch since they were triaged together.

Closes #1578
Closes #1584
Closes #1592
Closes #1580
Closes #1577
Closes #1579
Closes #1583
Closes #1586
Closes #1590
Closes #1595
Closes #1599
Closes #1604
Closes #1608
Closes #1616
Closes #1623
Closes #1597
Closes #1603
Closes #1609
Closes #1629
Closes #1582
Closes #1588
Closes #1593

How

fix(workstation): bisect surface copy says g marks good, but the binding is y #1578 — bisect panel copy said g marks good; the binding is y (moved off g in fix(workstation): bisect view — bare g mutates bisect state where muscle memory expects the chord prefix #1352 since g is the chord prefix). Fixed the three stale strings, not the binding.
fix(config): OPENAI_API_KEY / ANTHROPIC_API_KEY are never read by the env loader — every UX surface tells users to set them #1584 — env loader never read OPENAI_API_KEY/ANTHROPIC_API_KEY even though init/doctor/error messages all tell users to set them. Added both, kept OPEN_AI_KEY as a deprecated alias (OPENAI_API_KEY wins if both set).
fix(llm): tokenizer bootstrap crashes every LLM command for model ids unknown to tiktoken (custom baseURL models, Azure deployments, new OpenAI ids) #1592 — encoding_for_model() threw for any model id outside tiktoken's compiled-in map (custom baseURL models, Azure deployment names, newer OpenAI ids), crashing every LLM command before any work happened. Added a try/catch fallback to o200k_base/cl100k_base.
fix(pr create): 'create' is parsed as an arbitrary positional — any coco pr <word> creates a PR, and coco pr fails with a cryptic yargs error #1580 — pr create's command string parsed create as an unconstrained positional, so coco pr close/list/anything silently ran PR creation with no confirmation. Switched to a required pr <action> positional with choices: ['create'] validation.
fix(commit --split): plan is invisible in stdout mode — quiet logger mutes the plan, then the apply prompt asks to commit unseen changes (hangs in CI) #1577 — commit --split's default plan was printed through the quiet logger (muted in non-interactive mode), then blocked on a confirm prompt for a plan the user never saw — hangs in CI. Non-interactive now degrades to a plan preview with no prompt; interactive writes the plan straight to stdout so it can't be hidden by an independently-set --quiet.
fix(workstation): staging diff goes permanently blank after hunk-level stage/revert when the file's porcelain status doesn't change #1579 — hunk-level stage/revert cleared the cached worktree diff expecting a reload, but the hydration effects only re-fire on the file's own two-letter status — unchanged when reverting one of several unstaged hunks, or staging the 2nd+ hunk of an already-MM file. Added worktreeDiffRefreshToken, bumped centrally inside refreshWorktreeContext, mirroring the existing prDiffRefreshToken pattern.
fix(commit): generation failure exits 0 — "Agent failed to return content" calls commandExit(0), masking failures in CI #1583 — commit generation failure exited 0.
fix(recap): empty-timeframe path throws NO_CHANGES_DETECTED — friendly message is followed by "Failed to execute command" and exit 1 #1586 — recap's clean-repo path threw a scary NO_CHANGES_DETECTED error instead of a plain message.
fix(changelog): --range without a colon is silently ignored, and --range is missing from the mutual-exclusion check #1590 — changelog --range was silently ignored unless it contained a literal :; now accepts git-native ../... syntax too.
fix(commit): --noDiff summarizes unstaged and untracked files as "Staged files" — message describes changes that won't be committed #1595 — commit --noDiff described unstaged files as staged in the summary.
fix(review): non-numeric --severity coerces to NaN and silently disables the CI gate #1599 — review --severity accepted non-numeric/out-of-range values and silently disabled the CI gate.
fix(pr create): branch with no commits ahead surfaces as red "Command exited with code 0" error #1604 — pr create showed a misleading "exited code 0" error on an empty-changelog branch instead of a human message.
fix(recap): default 'current' timeframe never short-circuits on a clean repo — burns an LLM call and fabricates a recap #1608 — recap on a clean repo still burned an LLM call before discovering there was nothing to summarize.
fix(config): project config lookup is cwd-relative — .coco.json at the repo root is ignored when running from a subdirectory #1616 — project config lookup was cwd-relative instead of git-root-relative.
fix(config): .env written by coco init uses variable names the loader never reads — and nothing loads .env at all #1623 — init offered .env as a config file target but the writer for it was dead code — removed the option and the dead code.
fix(git): porcelain status parser keeps C-style quoting — staging/unstaging/diffing non-ASCII filenames fails #1597 — the porcelain status parser broke on non-ASCII filenames (v1 text format's C-style quoting); switched to NUL-separated -z parsing.
fix(git): getDiff builds diff pathspecs without -- — a file named like a ref yields the wrong diff for the LLM #1603 — getDiff was missing a -- separator before pathspecs, so a file literally named main or HEAD produced an "ambiguous argument" error.
fix(forge): GitHub Enterprise remotes — gh repo view queries github.com, and PR/issue surfaces reject GHE remotes outright #1609 — GitHub Enterprise remotes were probed against github.com instead of their own host.
fix(llm): OpenAI provider never passes maxConcurrency — service.maxConcurrent is ignored and the summarize fan-out runs unthrottled #1629 — the OpenAI provider never passed maxConcurrency, so service.maxConcurrent was silently ignored.
fix(workstation): force-push/force-delete recovery prompts and batch-mark freezes skip the frameChanged guard and can target the wrong repo frame #1582 — force-push/force-delete recovery confirmations and the delete-branch/drop-stash batch-mark freeze skipped the frameChanged guard fix(workstation): async choice prompts land invisible under the help overlay and aren't frame-scoped #1429 established for recovery prompts, so a repo-frame race could target the wrong repo.
fix(workstation): selectedWorktreeFileIndex resolves unclamped — status cursor and file actions vanish when the visible list shrinks #1588 — selectedWorktreeFileIndex resolved unclamped, so the cursor and file actions vanished when the visible list shrank.
fix(workstation): AI action hooks lack the defensive catch — a thrown workflow error becomes an unhandled rejection and strands loading state #1593 — four AI action hooks (changelog, commit-split, PR create, conflict resolution) lacked the defensive catch useAiCommitDraftActions already had, so an unexpected throw could become an unhandled rejection and permanently strand the loading state.

Review feedback addressed

prCreate/config.ts now matches the cache <subcommand> pattern (required positional + native choices) instead of a hand-rolled .check().
tokenizer.ts's fallback no longer tries to positively match "newest" OpenAI ids by regex; defaults to o200k_base and only falls back further for ids that look pre-o200k.
worktreeDiffRefreshToken is now bumped centrally inside refreshWorktreeContext, not at each individual call site.
env.ts's three hand-synced places for provider-scoped API-key env vars collapsed into one lookup table.

Validation

npm test — full jest suite passes locally
New/updated behavior has test coverage throughout
schema.json regenerated — not needed, no config schema changes
Docs updated — not needed, no user-facing doc changes
Commits follow Conventional Commits (one per fix)

Notes

Remaining low-severity findings from the same review are tracked as separate open issues and will follow in subsequent PRs.

gfargo

Ran a high-effort review (8 finder angles: line-by-line scan, removed-behavior audit, cross-file trace, reuse/duplication, simplification, efficiency, altitude, conventions) against this diff, plus manual tracing of the more consequential candidates.

No blocking issues. Full test suite (334 suites / 4809 tests), tsc --noEmit, and eslint were all re-verified clean on this branch. The six fixes are each correct, well-scoped, and covered by new tests; cross-file tracing confirmed no call site is broken by any of the six changes.

Six non-blocking findings, all polish/robustness — left as inline comments:

worktreeDiffRefreshToken is bumped ad hoc at 4 sites instead of centrally inside refreshWorktreeContext (unlike the sibling prDiffRefreshToken, which is centralized). Traced every other caller — none currently reproduce #1579, but the safety is implicit, not enforced.
toggleSelectedFileStage's exemption from the token bump relies on the same unenforced invariant, localized to this file.
prCreate's 'pr [action]' + hand-rolled .check() diverges from the sibling cache/config.ts pattern (required positional + native choices) for the same underlying yargs footgun.
tokenizer.ts's fallback regex doesn't actually cover its own motivating cases (Azure custom deployment names, gpt-4.1/gpt-4.5) — still strictly better than crashing, just an approximation gap worth a follow-up.
env.ts's env-key handling is now duplicated across 3 places that must stay in sync by hand; this PR extended that pattern rather than collapsing it.
Minor dep-array inconsistency in useWorktreeStageActions.ts — cosmetic only (setState setters are referentially stable), not a functional bug.

None of these block merging; happy to see them addressed here or as quick follow-ups.

Resolves all five non-blocking review comments from the PR #1646 code review so the batch can merge cleanly: - prCreate/config.ts: switch `pr [action]` + a hand-rolled `.check()` to a required `pr <action>` positional with native yargs `choices`, matching the `cache <subcommand>` pattern already used elsewhere for the same "verb + required-choice positional" shape. - tokenizer.ts: the unrecognized-model fallback no longer tries to positively match "newest" OpenAI ids by regex (which missed Azure custom deployment aliases and ids newer than the pinned tiktoken release, e.g. gpt-4.1/gpt-4.5). Default to o200k_base — the more common recent encoding — and only fall back further to cl100k_base for ids that look like pre-o200k OpenAI models. - useWorktreeStageActions.ts / useContextRefresh.ts: move the `worktreeDiffRefreshToken` bump out of four individual stage/revert call sites and into `refreshWorktreeContext` itself, mirroring how `prDiffRefreshToken` is already bumped centrally inside `refreshContext`. Every current and future `refreshWorktreeContext` caller now gets the reload signal automatically instead of each call site having to remember it. - env.ts: collapse the three hand-synced places for provider-scoped API-key env vars (the `envKeys` array, a boolean OR-chain, and the `handleServiceEnvVar` switch) into one `PROVIDER_API_KEY_ENV_VARS` lookup table used for both membership-testing and provider routing.

The panel copy was never updated when #1352 rebound mark-good from g to y (g stays free as the chord prefix). Fixes #1578.

…onment The env loader had no handling for OPENAI_API_KEY and no Anthropic key variable at all, while init, doctor, the missing-key screen, and the error formatter all instruct users to set exactly those variables. OPEN_AI_KEY is kept as a deprecated alias; OPENAI_API_KEY wins when both are set. Fixes #1584.

encoding_for_model() throws for any model id outside tiktoken's compiled-in map, which killed every LLM command before any work happened for custom baseURL models, Azure deployment names, and OpenAI ids newer than the pinned tiktoken release. Falls back to o200k_base or cl100k_base by model-id shape instead. Fixes #1592.

'pr create' registered 'create' as an unconstrained required positional, not a literal subcommand, so 'coco pr close'/'list'/'view' (or any typo) silently ran PR creation with no confirmation. 'pr [action]' now validates against choices: ['create'] and a bare 'pr' names the valid action instead of yargs' generic arity error. Fixes #1580.

In non-interactive mode the plan was printed via the muted logger, then an inquirer confirm asked to apply commits the user never saw — hanging forever on an open, silent stdin. The default path now degrades to a plan preview (no prompt) when non-interactive, and writes the plan directly to stdout in the interactive path so it's never hidden behind an independently-muted logger. Fixes #1577.

…tation Hunk-level stage/revert cleared the cached diff/hunks expecting rehydration, but the loaders only re-fire on the file's own indexStatus/worktreeStatus — unchanged when reverting one of several unstaged hunks, or staging the 2nd+ hunk of an already-MM file. The pane went permanently blank with every further action reporting "no hunk selected". Adds worktreeDiffRefreshToken, bumped by the four hunk/line-level handlers and threaded into both diff-hydration effects, mirroring the prDiffRefreshToken pattern (OSS-452). Fixes #1579.

commandExit(0) on an empty LLM response masked hard failures as success in CI. Also routes the failure message through logger.error so it's visible even in quiet/non-interactive mode. Fixes #1583.

…ircuit on a clean repo noResult() threw NO_CHANGES_DETECTED, making the handler's commandExit(0) unreachable and escaping to the generic error formatter with exit 1. Removed the throw so the handler's own exit runs, matching changelog's "no commits" behavior (#1586). Separately, the default 'current' timeframe factory always returned non-empty template-prefixed strings even when every section was empty, so a completely clean repo never reached noResult and burned an LLM call on fabricated content. Added the same empty-change-set guard review already has (#1608).

…exclusivity check --range only matched coco's <from>:<to> form; a bare HEAD~5..HEAD was silently dropped and fell through to current-branch mode with no warning. Now accepts both : and ../... separators, and rejects unparseable values instead of ignoring them. Also added --range to the mutual-exclusion check so combining it with --branch/--tag errors consistently instead of silently favoring range. Fixes #1590.

The --noDiff factory mapped every git-status entry, including unstaged and untracked files, into the "Staged files" summary fed to the model — describing changes that wouldn't be part of the commit. Reuses getChanges().staged (the same source the regular diff path uses), which also restores ignoredFiles/ignoredExtensions filtering that the raw git-status mapping bypassed. Fixes #1595.

… silently disabling the CI gate yargs coerces an unparseable --severity value to NaN, and typeof NaN === 'number' — so a typo (or any non-integer/out-of-range value) passed the old guard and every severity comparison silently evaluated false, disabling the CI gate with no signal. The builder now validates severity is an integer between 1 and 10 up front; the handler's own gate additionally uses Number.isFinite as a backstop. Fixes #1599.

…ntinel text When the current branch has no commits ahead of base, changelog's clean -exit sentinel (CommandExitError(0), default message "Command exited with code 0") was relayed verbatim as the PR-body generation failure, so users saw a red error that literally described the exit code instead of an explanation. runPullRequestBodyWorkflow now special-cases a zero-code CommandExitError and returns the existing curated message. Fixes #1604.

loadProjectJsonConfig tested bare relative filenames, resolving against process.cwd() — while coco init writes .coco.json at the repo root. Running any command from a subdirectory silently dropped the entire project config with no warning. Extracted the git-toplevel resolution (with cwd fallback) that diffSummaryCache.ts already used for the same subdirectory-invocation problem (#1463) into a shared resolveGitRepoRoot utility, and reuse it here so project config loading finds the same file coco init wrote regardless of invocation directory. Fixes #1616.

Choosing ".env" at coco init produced a file coco could never read back: the writer emitted un-prefixed flattened names while the env loader only reads COCO_*-prefixed names, and nothing loaded a .env file into process.env in the first place. Init reported success while every chosen setting silently never applied. Removed the option (and the now-dead appendToEnvFile/flattenObject writer) rather than building out a new .env-loading + trust-boundary code path — .coco.json already works and is the recommended choice. Fixes #1623.

git status --porcelain (v1, without -z) C-quotes "unusual" paths under the default core.quotePath=true — any non-ASCII filename, or one containing a quote/tab, round-tripped its literal quotes and octal escapes into the subsequent git add/restore/diff pathspec, which then failed to match. -z disables quoting entirely and reports rename/copy origin paths as a separate trailing NUL-terminated field instead of an in-line " -> " separator, which also removes the ambiguity of a filename containing that literal substring. Applied the same fix to operationData.ts's conflict-file parser, which duplicated the identical v1 text-parsing logic (and the same bug) for merge-conflict detection. Fixes #1597.

getDiff's default-path git.diff() calls passed the file path as a bare argument, without the -- separator used everywhere else in the codebase. A tracked file literally named like a ref (a branch, HEAD, a tag) was resolved as a revision instead of a path, silently diffing the wrong thing and feeding it to the commit-message/review LLM with no error surfaced. Fixes #1603.

…lt-branch lookups Two halves of the same host-scoping gap: getDefaultBranch queried gh's default host (github.com) with a bare owner/name slug regardless of which host the remote actually lives on, and the PR/issue list surfaces gated on a github.com-only remote parser — rejecting a GitHub Enterprise remote outright even though the provider layer already classifies it correctly and probes auth against the right host. getDefaultBranch now uses the full host-qualified URL when the repository isn't on github.com. issuesListData/pullRequestListData/ pullRequestData now resolve the repository via the same host-aware provider detection getProviderOverview already uses (heuristic + forgeHosts overrides), and pass the resolved host into getGhStatus instead of defaulting to github.com. Fixes #1609.

The OpenAI provider's config object omitted maxConcurrency, so service.maxConcurrent was silently ignored and the summarize fan-out ran unthrottled while every other provider honored the setting. Fixes #1629.

…es against repo-frame races force-delete-branch, force-push-current-branch/force-push-selected-branch, and the delete-branch/drop-stash setMarks freeze were the only post-await dispatch sites in runWorkflowAction missing the frameChanged guard #1429 established for recovery prompts. A repo-frame push/pop that races one of these slow workflows could re-target the follow-up confirm or mark freeze at the wrong repo frame. Fixes #1582.

selectedWorktreeFile resolved by raw index into the flattened status list with no clamp, and nothing rectifies the index when a refresh (revert, external stage) shrinks the visible list — the cursor and row highlight vanished and file actions no-op'd even with files still on screen. Clamp at every resolution site (buildStatusSurfaceData's pure core and memo, the status renderer's cursor row, and useInputHandler's worktreeSelectedPath) so highlight, input target, and action target keep agreeing, matching the clamp pattern every other list selector already uses. Fixes #1588.

…g them startChangelogView, startCommitSplit, applyCommitSplit, startCreatePullRequest, and startConflictResolution awaited their workflow with no catch (or, for applyCommitSplit, no try at all) around the awaited call. Any unexpected throw — as opposed to the workflow's own `{ ok: false }` result — would escape as an unhandled promise rejection and permanently strand the surface's loading state, with no way to retry short of quitting. useAiCommitDraftActions already carries this defensive catch for itself; mirror it in the four sibling hooks so an escaped throw surfaces an error and clears loading instead. Fixes #1593.

Resolves all five non-blocking review comments from the PR #1646 code review so the batch can merge cleanly: - prCreate/config.ts: switch `pr [action]` + a hand-rolled `.check()` to a required `pr <action>` positional with native yargs `choices`, matching the `cache <subcommand>` pattern already used elsewhere for the same "verb + required-choice positional" shape. - tokenizer.ts: the unrecognized-model fallback no longer tries to positively match "newest" OpenAI ids by regex (which missed Azure custom deployment aliases and ids newer than the pinned tiktoken release, e.g. gpt-4.1/gpt-4.5). Default to o200k_base — the more common recent encoding — and only fall back further to cl100k_base for ids that look like pre-o200k OpenAI models. - useWorktreeStageActions.ts / useContextRefresh.ts: move the `worktreeDiffRefreshToken` bump out of four individual stage/revert call sites and into `refreshWorktreeContext` itself, mirroring how `prDiffRefreshToken` is already bumped centrally inside `refreshContext`. Every current and future `refreshWorktreeContext` caller now gets the reload signal automatically instead of each call site having to remember it. - env.ts: collapse the three hand-synced places for provider-scoped API-key env vars (the `envKeys` array, a boolean OR-chain, and the `handleServiceEnvVar` switch) into one `PROVIDER_API_KEY_ENV_VARS` lookup table used for both membership-testing and provider routing.

gfargo commented Jul 12, 2026

View reviewed changes

Comment thread src/commands/prCreate/config.ts

Comment thread src/lib/utils/tokenizer.ts

Comment thread src/workstation/runtime/hooks/useWorktreeStageActions.ts Outdated

Comment thread src/workstation/runtime/hooks/useWorktreeStageActions.ts

Comment thread src/lib/config/services/env.ts Outdated

gfargo added 22 commits July 12, 2026 18:57

fix(workstation): bisect surface copy says g marks good, binding is y

7da4213

The panel copy was never updated when #1352 rebound mark-good from g to y (g stays free as the chord prefix). Fixes #1578.

fix(commit): agent-returned-no-content exits 1, not 0

3d51b2c

commandExit(0) on an empty LLM response masked hard failures as success in CI. Also routes the failure message through logger.error so it's visible even in quiet/non-interactive mode. Fixes #1583.

fix(llm): pass maxConcurrency to the OpenAI provider

a60edae

The OpenAI provider's config object omitted maxConcurrency, so service.maxConcurrent was silently ignored and the summarize fan-out ran unthrottled while every other provider honored the setting. Fixes #1629.

gfargo force-pushed the claude/code-review-github-issues-t1wry1 branch from f00a3ae to 587c49d Compare July 12, 2026 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: high-severity bug batch from comprehensive review#1646

fix: high-severity bug batch from comprehensive review#1646
gfargo wants to merge 22 commits into
mainfrom
claude/code-review-github-issues-t1wry1

gfargo commented Jul 12, 2026 •

edited

Loading

Uh oh!

gfargo left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

gfargo commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Review feedback addressed

Validation

Notes

Uh oh!

gfargo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

gfargo commented Jul 12, 2026 •

edited

Loading

gfargo left a comment •

edited

Loading