Skip to content

fix: remove unreachable internal-only branches - #2440

Merged
jatmn merged 1 commit into
Twigpine:mainfrom
jatmn:fix/code-quality-small-1-pMzxPrcW
Oct 27, 2026
Merged

jatmn merged 1 commit into
Twigpine:mainfrom
jatmn:fix/code-quality-small-1-pMzxPrcW

Conversation

@jatmn

@jatmn jatmn commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Disabled internal features left callers with impossible capability reads, rate-limit headers, verification text and deep-search work. Remove those unreachable branches and the updater's false || false expression, preserving the existing external-build behavior.

Part 1/4 of the requested small-category code-quality cleanup. Addresses findings 74, 75, 99–103, 105 and 411 (9 findings) in the quality report. Deep-search helper functions and their sole constant are removed with their unreachable caller.

Impact

Existing model limits, rate-limit handling, plan handoff text, resume picker and update behavior are unchanged. Removes misleading internal-only code without restoring disabled features.

Testing

  • Followed CONTRIBUTING.md Validation and reviewed AGENTS.md.
  • Bun 1.4.4, Node 26.12.1, Linux. Locked dependencies installed with bun install --frozen-lockfile.
  • bun run check:static, bun run typecheck:fast, bun run typecheck:type-tests, node bin/openclaude --version, and NODE_DISABLE_COMPILE_CACHE=1 node bin/openclaude --version: passed.
  • git diff --check and bun run security:pr-scan -- --base upstream/main --head HEAD: passed after fetching canonical upstream main.
  • Initial independent branch from a91f736ef1c257d00dd4ced04f86e21401289f40; no prior PR validation reused.
  • Focused local tests selected for the affected behavior; full-suite and supported-Node-matrix coverage will run in CI. Website/extension-package checks are inapplicable: no site inputs or extension package code changed.
  • All applicable CI checks passed on the reviewed head, including the full unit shards, CodeQL and supported-Node launchers. CodeRabbit approved this revision. Kept as a draft as requested.

Focused validation: bun run test src/utils/context.test.ts src/components/LogSelector.resumeBranches.test.ts src/components/permissions/ExitPlanModePermissionRequest/ExitPlanModePermissionRequest.test.ts src/components/permissions/ExitPlanModePermissionRequest/ExitPlanModePermissionRequest.render.test.tsx src/components/AutoUpdater.test.ts src/components/NativeAutoUpdater.test.ts: 109 passed.

Notes

No provider behavior or visible UI changes; existing rendered picker/plan tests cover the affected consumers, so no new screenshot is applicable. Related open PR #2430 changes updater services/build stubs, not the removed no-effect component expression.

Reviewed, locally validated and pushed commit: eb7a22ae51499e1012da4475fd8638a6e83bb4ed.

Summary by CodeRabbit

  • Behavior Changes
    • Deep-search results are cleared and the searching indicator resets when the debounced query changes. Deep-search result matching and relevance-based ordering are no longer performed.
    • After accepting a plan with a clear context, the initial implementation message no longer includes a verification-tool instruction.
    • Context-window and output-token limits are now determined by integration runtime limits and existing model-specific rules, rather than model capability metadata.

@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between a91f736 and eb7a22a.

Files selected for processing: 5
  • src/components/LogSelector.tsx
  • src/components/PackageManagerAutoUpdater.tsx
  • src/components/permissions/ExitPlanModePermissionRequest/ExitPlanModePermissionRequest.tsx
  • src/services/rateLimitMocking.ts
  • src/utils/context.ts
Files with no reviewable changes: 2
  • src/components/PackageManagerAutoUpdater.tsx
  • src/utils/context.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Recent review details
Context from checks skipped due to timeout: 3
  • GitHub Check: CodeRabbit
  • GitHub Check: unit-tests (5)
  • GitHub Check: CodeQL (javascript-typescript)
Additional context used
Path-based instructions: 3
Review permission prompts, auto-allow logic, sandbox behavior, SDK permission schemas, shell/PowerShell execution, and background execution paths as security-sensitive.

CodeRabbit configuration file

Files:

  • src/components/permissions/ExitPlanModePermissionRequest/ExitPlanModePermissionRequest.tsx
Apply the OpenClaude maintainer review rubric from AGENTS.md.

CodeRabbit configuration file

Files:

  • src/components/permissions/ExitPlanModePermissionRequest/ExitPlanModePermissionRequest.tsx
  • src/components/LogSelector.tsx
  • src/services/rateLimitMocking.ts
Source excerpt: No hidden policy change: Verify that product, trust-model, routing-default, telemetry/network, and permission-policy changes are not hidden inside unrelated cleanup.

CodeRabbit inference engine (Custom checks)

Files:

  • src/components/permissions/ExitPlanModePermissionRequest/ExitPlanModePermissionRequest.tsx
  • src/components/LogSelector.tsx
  • src/services/rateLimitMocking.ts
Learnings: 1
Common learnings
Learnt from: CR
Repo: Twigpine/openclaude

Timestamp: 2026-10-27T13:04:07.299Z
Learning: Source excerpt:
# AGENTS.md - AI Agent Coding Guide

## Validation

`bun run typecheck:fast` runs Bun's builtin type checker locally and is the CI typecheck; use the wrapper because the repository's `check` script shadows `bun check`. `bun run typecheck:type-tests` also runs in CI for focused TypeScript assertions. `bun run typecheck` (tsc) is available as an additional local check. Keep the `typescript` devDependency for ESLint, `astrojs/check`, and the focused type tests.
Additional comments: 2
src/components/permissions/ExitPlanModePermissionRequest/ExitPlanModePermissionRequest.tsx: 1

479-479: 🔒 Security & Privacy

No executable verification path remains in this checkout.

VerifyPlanExecutionTool exports null, so src/tools.ts does not register it even when CLAUDE_CODE_VERIFY_PLAN=true. The removed instruction therefore cannot disable the claimed verification workflow.

src/services/rateLimitMocking.ts: 1

42-42: LGTM!

Walkthrough

The deep-search effect now clears its results and searching state when its dependencies change. Capability-based context and output-token limits, fast-mode mock rate-limit handling, a plan-verification instruction, and an updater no-op expression are removed.

Changes: Log selector deep search

Layer / File(s) Summary
Deep-search effect and helpers
src/components/LogSelector.tsx
The effect clears deep-search state when its dependencies change. The deferred Fuse search and its result-ordering helpers are removed.

Mock rate-limit checks

Layer / File(s) Summary
Fast-mode mock branch removal
src/services/rateLimitMocking.ts
The fast-mode-specific mock branch and its imports are removed. The optional parameter remains with an underscore-prefixed name; the existing unified-status and overage-status checks remain.

Context and output token limits

Layer / File(s) Summary
Context-window and output-token resolution
src/utils/context.ts
Capability metadata no longer sets context-window or maximum output-token limits. Resolution continues through the existing paths.

Clear-context plan acceptance

Layer / File(s) Summary
Plan acceptance message
src/components/permissions/ExitPlanModePermissionRequest/ExitPlanModePermissionRequest.tsx
The verification instruction and its tool-name import are removed. The clear-context implementation message retains the plan, transcript, team, and feedback content.

Updater callback cleanup

Layer / File(s) Summary
Remove no-op expression
src/components/PackageManagerAutoUpdater.tsx
The standalone `false

Priority: Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Possibly related PRs

  • Twigpine/openclaude#2438: Removes the same no-op expression from PackageManagerAutoUpdater.tsx, along with an unrelated MCP expression.
Merge Risk: Minimal · up to eb7a2

No behavior regression was established in the reviewed changes; the PR is mergeable after normal checks.

Pre-merge checks: Passed 6 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 6 passed
Check name Status Explanation
Title check Passed The title "fix: remove unreachable internal-only branches" matches the main change: the diff removes unreachable internal-only branches across context.ts, LogSelector.tsx, rateLimitMocking.ts, ExitPla…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Risk Surface Disclosed Passed The PR touches permissions and background/update behavior. The description identifies the affected risk surfaces: plan handoff text, rate-limit handling, resume picker, and update behavior. It states …
No Hidden Policy Change Passed PASS — The diff removes dead or external-build no-op paths and does not introduce a product, trust-model, routing-default, telemetry/network, or permission-policy decision. The plan handoff still uses…
Description check Passed The description includes the required Summary, Impact, Testing, and Notes sections. It explains the changes and rationale, documents user and maintainer impact, lists validation commands and results, …
Full details: Title check

Explanation

The title "fix: remove unreachable internal-only branches" matches the main change: the diff removes unreachable internal-only branches across context.ts, LogSelector.tsx, rateLimitMocking.ts, ExitPlanModePermissionRequest.tsx, and the updater's no-op expression. It is concise, follows a conventional prefix, and is scoped to the actual diff. It is slightly broad because it does not name the affected areas, but it is not vague or misleading.

Full details: Risk Surface Disclosed

Explanation

The PR touches permissions and background/update behavior. The description identifies the affected risk surfaces: plan handoff text, rate-limit handling, resume picker, and update behavior. It states that existing behavior remains unchanged, reports focused tests passing, and notes no provider behavior or visible UI changes. This discloses the impact and indicates no blocker.

Full details: No Hidden Policy Change

Explanation

PASS — The diff removes dead or external-build no-op paths and does not introduce a product, trust-model, routing-default, telemetry/network, or permission-policy decision. The plan handoff still uses the existing mode and permission-update logic; only the unreachable undefined === 'true' verification text is removed. The model-capability module returns undefined, so removing its reads does not change context or output limits; runtime-route, beta, experiment, Ant, and model-name branches remain. The rate-limit mock module returns no-op values, while normal status/overage/Opus handling and the caller remain. The updater body is unchanged apart from deleting false || false, and the deep-search removal deletes an unconditional dead branch and its helpers.

Full details: Description check

Explanation

The description includes the required Summary, Impact, Testing, and Notes sections. It explains the changes and rationale, documents user and maintainer impact, lists validation commands and results, identifies focused tests and inapplicable checks, and records CI and CodeRabbit status. No required information is materially missing.

Full details: Docstring Coverage

Explanation

Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 3 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@jatmn jatmn self-assigned this Oct 27, 2026
@jatmn
jatmn marked this pull request as ready for review October 27, 2026 13:52
@jatmn
jatmn merged commit 94ebcff into Twigpine:main Oct 27, 2026
28 checks passed
@jatmn
jatmn deleted the fix/code-quality-small-1-pMzxPrcW branch October 27, 2026 13:52
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.

1 participant