mirror of
https://github.com/Gitlawb/openclaude.git
synced 2026-08-24 02:34:15 -05:00
main
17
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  CalandGitHub
|
e8026263ca |
fix(tui): proper Unicode/IME input handling for composed sequences (#2018) (#2154)
* fix(tui): proper Unicode/IME input handling for composed sequences (#2018) * test(tui): address CodeRabbit review - hook-level IME coverage, full Unicode marks, astral code points, timeout regression tests |
||
|
CalandGitHub
|
54f963d006 | fix(openai-shim): drop synthetic tool-results marker and guard echoes (#2039) (#2153) | ||
|
CalandGitHub
|
2da3e4c400 |
docs: clarify pre-PR contributor checklist (#1920)
* docs: clarify pre-PR contributor checklist * docs: require contribution guide confirmation |
||
|
3b41cf3adb |
fix(command-semantics): cover remaining linter runner exits (#1700)
* fix(command-semantics): cover remaining linter runner exits * fix(command-semantics): handle env prefixes and PowerShell chains * fix(command-semantics): preserve runner and pipeline failures * fix(command-semantics): narrow setup and runner failure guards * fix(command-semantics): catch real setup failure stderr * fix(command-semantics): preserve setup failures with output * fix(command-semantics): parse inline env split strings * fix(command-semantics): inspect Bash merged failure output * fix(command-semantics): align PowerShell failure parsing * test(web-search): stabilize Brave timeout assertion * fix(command-semantics): cover package scripts and wrapper failures * fix(command-semantics): handle script run forms and PS call operator * fix(command-semantics): cover package prefixes and npm errors * fix(command-semantics): stabilize brave timeout and ps flags * test(websearch): assert brave timeout aborts fetch signal * fix(command-semantics): handle tsc diagnostic exit 1 * fix(command-semantics): preserve script diagnostics * fix(command-semantics): guard silent skipped diagnostics * fix(command-semantics): tighten setup failure guards --------- Co-authored-by: jatmn <the@jat.mn> |
||
|
fb1137275a |
feat: add ultrathink keyword detection and ultracode effort level (#1551) (#1630)
* feat: add ultrathink keyword detection and ultracode effort level Rebased onto current main so the diff contains only these changes — #1780's model-level effort routing now comes from main rather than being duplicated. - ultrathink: a `\bultrathink\b` keyword in a prompt injects a high-effort reminder, gated behind the isUltrathinkEnabled() rollout flag. - ultracode: a new session-only EffortLevel that maps to xhigh (or high) on the wire and grants a standing multi-agent orchestration permission. First-party only, suppressed under a per-agent providerOverride, and gated to xhigh-capable models. Honors CLAUDE_CODE_EFFORT_LEVEL precedence across the API path, the permission attachment, and the display surfaces; rejected from every agent-definition input (markdown/skill/plugin frontmatter, SDK, and JSON). Closes #1551 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(effort): clamp ultracode display availability * fix(effort): report effective effort overrides * test(model): avoid catalog-dependent effort label * fix(model): resolve current effort against session model * fix(spinner): resolve effort suffix against session model --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: jatmn <the@jat.mn> |
||
|
|
4aec353f9c |
fix(grep): relativize content-mode paths correctly on Windows (#1704)
* fix(grep): relativize content-mode paths correctly on Windows Grep output_mode "content" split each line at the first colon to separate path from content, but a Windows absolute path starts with a drive-letter colon (C:), so it split at "C" and reassembled the original absolute path — defeating relativization (count and files_with_matches modes were already correct). Skip a leading drive-letter colon when locating the boundary. Extracts a pure relativizeContentLine helper with cross-platform tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(grep): relativize Windows context rows, not just match rows ripgrep separates the path with `:` on match rows and `-` on context rows (-A/-B/-C). The helper only looked for a boundary colon, so Windows context rows like `C:\...\file.ts-1-before` kept their absolute path. Locate the boundary as the first `:<n>:` (match; unambiguous since paths have no non-drive colon) else the first `-<n>-` (context), falling back to the first colon for line-number-less rows. This also stops date-like `-2024-` runs in filenames from being mistaken for the context boundary. Adds tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(grep): relativize content rows by stripping the known search root Review follow-up: the previous delimiter heuristic chose the first `-<n>-`, which mis-split rows when the cwd or an ancestor directory contained a date-like segment (e.g. C:\Users\proj-2024-01-15\...), and left line-number-disabled context rows (`path-content`) with absolute paths. Strip the known absolute root prefix instead: every ripgrep path under the root starts with `<root><sep>`, so removing it yields the relative path + the original delimiter + content verbatim, independent of the delimiter or whether line numbers are enabled. Paths outside the root stay absolute, matching toRelativePath. Rewrites the tests, including the date-cwd and no-line-number context-row regressions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(grep): compare the root with Windows path semantics (case/slash-insensitive) Review follow-up: stripping the root used a literal startsWith, so when getCwd() and ripgrep spelled the same Windows root with different casing or slash style (e.g. C:\USERS\PROJ vs C:\Users\proj), the prefix did not match and absolute paths leaked. Normalize the comparison for Windows roots (lowercase + treat `/` as `\`) while slicing the original line by prefix length, mirroring toRelativePath's case-insensitive path.win32 behavior. Adds casing/slash regressions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
f5041e4d46 |
fix(format): roll formatFileSize over to the next unit at the 1024 boundary (#1703)
* fix(format): roll formatFileSize over to the next unit at the 1024 boundary formatFileSize selected the unit from the unrounded magnitude (kb < 1024) but displayed the rounded value, so sizes just under a boundary rendered as "1024KB"/"1024MB" instead of "1MB"/"1GB" (e.g. 1048575 bytes -> "1024KB"). Compare the rounded magnitude when choosing the unit so it promotes correctly. Adds format.test.ts covering the boundary bands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(format): assert formatFileSize(1023) renders as raw bytes The sub-KB test labelled "raw bytes" expected formatFileSize(1023) to be "1KB", but the implementation returns "1023 bytes" for values below the 1024-byte threshold, so the focused test (and the smoke-and-tests check) was red. Correct the expectation to "1023 bytes". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
ba85aa6dd0 |
fix(frontmatter): expand nested brace globs in paths: correctly (#1701)
* fix(frontmatter): expand nested brace globs in paths: correctly
expandBraces used the regex `^([^{]*)\{([^}]+)\}(.*)$`, whose `[^}]+` stops
at the first `}`, so nested brace groups were corrupted: `{a,{b,c}}` became
`["a}","b","c}"]` and `src/**/*.{js,{ts,tsx}}` produced stray `}` and broken
globs — silently breaking path-scoped skill / CLAUDE.md activation. Replace
the regex with a depth-aware scan that finds the matching close brace and
splits on top-level commas only, recursing as before. Unbalanced braces fall
back to the literal input. Adds frontmatterParser.test.ts.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(frontmatter): keep an empty brace group `{}` literal in paths
The balance-aware scanner expanded `{}` to a single empty alternative, so
`paths: "{}"` collapsed to [""]; parseSkillPaths and the CLAUDE.md path
parser drop that empty string and treat the file as having no path
restriction (activating everywhere). The previous regex required >=1 inner
char, so `{}` stayed literal. Restore that: treat an empty brace group as
literal while still expanding any later groups in the suffix. Adds tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
|
0c45e16f18 |
feat(config): add compactModel option to use a separate model for compaction (#1445) (#1629)
* feat: add compactModel config option to use a different model for compaction When set and different from mainLoopModel, the forked-agent prompt-cache-sharing path is bypassed (guaranteed cache miss with different models) and the streaming fallback uses compactModel for the API call instead. Closes #1445 * feat(config): expose compactModel in /config TUI and add compactModel test coverage Addresses review feedback on #1629: - Surface compactModel as a managedEnum setting in the /config screen, following the teammateDefaultModel pattern (submenu + ModelPicker). - Add a compact.test.ts case covering the compactModel !== mainLoopModel path: cache-sharing is skipped and the streaming compaction fallback routes model/maxOutputTokensOverride to compactModel. * fix(compact): use compactModel for tool-search check and add no-op guard Addresses round-2 review feedback on #1629: - compact.ts: pass compactModel ?? mainLoopModel to isToolSearchEnabled so the tool-search capability check matches the model actually used for streaming compaction (not always mainLoopModel when a compact model is set) - Config.tsx: mirror the teammateDefaultModel no-op guard — return early when compactModel is unset and the picker confirms null (no-op selection) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(compact): normalize compactModel comparison in no-op guard Compare globalConfig.compactModel ?? null against the picker's selection so re-confirming the current value (including explicit "Default"/null when a model was previously set) is treated as a no-op instead of marking settings dirty. * fix: resolve compactModel alias to full model ID before API calls ModelPicker stores alias strings (e.g. 'sonnet') directly in globalConfig.compactModel. Compact reads that value and must call parseUserSpecifiedModel() to expand it to the canonical model ID before comparing against mainLoopModel or sending to the API. Two read-sites in compactConversation and streamCompactSummary are both fixed. Test updated so the 'skips cache-sharing' case uses the resolved model ID (legacy claude-opus-4-1 remaps to current default) and a new test verifies alias expansion end-to-end. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
|
a7d6580521 |
fix(plugins): prevent ENOENT on Windows marketplace cache finalization (#1500) (#1531)
* fix(plugins): prevent ENOENT on Windows marketplace cache finalization (#1500) * test(plugins): add regression test for Windows marketplace cache finalization (#1500) Add regression tests covering the case-only path comparison bug where fs.rm would destroy source data when temp and final cache paths differ only in case on case-insensitive filesystems (Windows NTFS). Three test cases: 1. Generic mixed-case name (MyMarketplace/mymarketplace) — verifies the samePathCaseInsensitive guard skips rm+rename 2. GitHub-style naming (AgriciDaniel-claude-obsidian) — explicitly models the exact #1500 bug report scenario 3. Already-lowercase name (claude-obsidian) — verifies the string-equality fast path that GitHub sources hit post-fix Also exports loadAndCacheMarketplace from _test for testability. * test: add rename-failure fallback regression test for EXDEV Add a regression test that forces the rename-failure fallback path in loadAndCacheMarketplace. Uses a 'url' source with a mocked axios response so the temp cache path (timestamp-based) truly differs from the final cache path (marketplace.name.toLowerCase()), bypassing the samePathCaseInsensitive guard. The test stubs rename to throw EXDEV and verifies the cp+rm fallback correctly copies data to the final location, cleans up the temporary path, and returns the final cache path. * test: fix brittle temp-cleanup assertion in marketplace fallback test Replace fragile rmSpy.call filter with direct readdirSync(cacheDir) to verify the real temp directory state. Fixes CI failure at line 316. * test: use pre-call snapshot for platform-agnostic fallback assertion Replace brittle readdirSync length check with delta comparison against pre-call directory state. Works on both case-sensitive (Linux) and case-insensitive (Windows) filesystems. * test: isolate marketplace mocks and fix cross-platform temp file assertion - Move axios mock inside describe with beforeAll/afterAll lifecycle - Filter temp_*.json files from directory delta comparison - Add mock isolation to prevent leakage to other test files * test(marketplace): assert renameSpy called and no temp files linger after EXDEV fallback * test(marketplace): add cp spy and isolate axios mock to test lifecycle Address outstanding review gaps for #1531: 1. The rename-failure fallback (cp + rm) is now explicitly verified via a cpSpy on the real fs/promises.cp. The mock.module('fs/promises', ...) registration in beforeAll wraps cp with a call-through spy, so the test exercises the actual filesystem fallback while still asserting the call args (temp source, finalCachePath dest, { recursive: true } option). Previously, the test only proved the final file existed, which could be reached by a different code path or by leftover pre-test state. 2. The axios mock is now isolated to the test lifecycle: the axiosGetSpy is created in beforeEach (with a fresh implementation) instead of at module scope, and the mock.module('axios', ...) factory uses a closure indirection so the spy can be swapped per test. This prevents call counts and implementations from leaking across tests in this suite or into other suites. 3. cpSpy.mockClear() runs in beforeEach to reset .mock.calls / .mock.results between tests while preserving the call-through behavior of the persistent mock.module() registration. Skipped: the P1 about finalCachePath lowercasing the cache directory (marketplaceManager.ts:1712). The current code already preserves cacheDir and lowercases only marketplace.name, so the original review concern was already addressed by an earlier commit. Skipped: temp path detection via rmSpy filter and explicit temp_* cleanup verification. Both are already covered by the existing afterEntries/beforeEntries snapshot comparison (L334-377) and the lingeringTempFiles assertion (L380-385). * Revert "test(marketplace): add cp spy and isolate axios mock to test lifecycle" This reverts commit c57ad55db18edbd235d8af337ac755fe1cb12eaa. * test(marketplace): force real module + drop unused source.name Two pre-existing issues that blocked typecheck and caused 4 marketplace tests to fail in the full suite: 1. mock.module('./marketplaceManager.js', async (original) => ...) called original() to 'force the real module' but original() returns the previously-registered factory's output, which is a stale mock from a prior test file (e.g. officialMarketplaceStartupCheck.test.ts or lspRecommendation.test.ts). The stale mock has its own env-var behavior, so getMarketplacesCacheDir() returned the leaked /tmp/openclaude-marketplaces instead of the test's tempDir. The real module reads process.env.CLAUDE_CODE_PLUGIN_CACHE_DIR live on every call, so the beforeEach override takes effect immediately. Forcing the real module makes the env-var-driven cache path work correctly. The factory now returns undefined so Bun uses the real module regardless of any previously-registered mock. 2. The EXDEV test's url source had a 'name' field that does not exist in the MarketplaceSource.url variant of the schema (only url and optional headers are allowed). The field was unused — the test hardcodes finalCachePath from the marketplace manifest name (lowercased), not from source.name. Drop the field; typecheck goes green. * test(marketplace): use mock.restore() to clear stale marketplaceManager mock Previous attempt used mock.module(path, () => undefined) but Bun rejects that — TypeError: 'mock(module, fn) requires a function that returns an object'. mock.module() with no factory isn't a documented API, so switch to mock.restore() which clears all module mocks. After mock.restore(), the real marketplaceManager.js is used on import. The EXDEV describe's beforeAll re-registers the axios mock (which mock.restore() also cleared), so the suite-specific mock still works. Skipped: spying on cp via setFsImplementation. The FsOperations interface (src/utils/fsOperations.ts L23-72) has no cp method, so adding cp: cpSpy to setFsImplementation would fail typecheck. The rename-failure fallback is still verified via filesystem state assertions: before/after readdirSync snapshot plus the explicit lingeringTempFiles === [] check. * Revert "test(marketplace): use mock.restore() to clear stale marketplaceManager mock" This reverts commit bbbebfb305b5585f34e0a13530a5ea4ce7a9edd7. * test(marketplace): fix mock.module TypeError (factory must return object) 0c917a1 used () => undefined to force the real module, but Bun rejects factories that don't return an object ('mock(module, fn) requires a function that returns an object'). Switch to mock.restore() at the top of the file — clears all module mocks registered by prior test files so the real marketplaceManager.js is used on import. The EXDEV describe's beforeAll re-registers the axios mock (which mock.restore also cleared), so suite-specific mocks still work. The env-var-leak bug (getMarketplacesCacheDir returning the leaked /tmp/openclaude-marketplaces instead of the test's tempDir) is a deeper issue with how process.env is read in Bun's test runner. Pre-existing on origin/main and out of scope for this PR; documented in the PR body. * test(marketplace): drop mock.restore() to let natural module resolution work CodeRabbit round 8 P2: the previous implementation called mock.restore() at file-scope on the assumption it would clear module mocks registered by other test files (lspRecommendation.test.ts, officialMarketplaceStartupCheck.test.ts). Bun's docs confirm mock.restore() does NOT clear mock.module() registrations, so the partial mock from the prior file persisted and the import of _test broke ("Export named '_test' not found"). The reproducer is real: running lspRecommendation.test.ts first then this file fails. But the root cause is a pre-existing bug in lspRecommendation.test.ts (its mock for '../config.js' is missing normalizeMaxMessagesCompactionThreshold, added to config.ts in PR #1605 / commit |
||
|
|
822eff39d1 |
fix(copilot): limit sub-agent concurrency to reduce Premium Request usage (#678) (#1534)
* fix(copilot): limit sub-agent concurrency to reduce Premium Request usage (#678) * fix(copilot): enforce sub-agent concurrency cap at Agent invocation level AgentTool.isConcurrencySafe() now returns false when getCopilotMaxConcurrentSubagents() > 0, preventing the tool scheduler from batching multiple Agent calls together. This ensures at most one sub-agent runs at a time when the cap is active. Previously, AgentTool was always concurrency-safe, allowing the scheduler's runToolsConcurrently to batch multiple Agent calls from a single assistant message — bypassing the documented MAX_SUBAGENTS cap. Add comprehensive copilotOptimization unit tests. * fix(copilot): enforce cap for any positive value and honor OPTIMIZATION_DISABLED - shouldForceSyncSubagentsInCopilotMode: gate on > 0 instead of === 1 so any configured cap (2, 3, ..., 10) forces serial execution - isConcurrencySafe: early-return true when OPTIMIZATION_DISABLED is set - Update log message to reflect any-cap behavior * fix(copilot): align scheduler with launch path, fix mock leak - isConcurrencySafe now uses shouldForceSyncSubagentsInCopilotMode() instead of raw cap check, matching the launch path at line 447 - Add afterAll(mock.restore) to copilotOptimization.test.ts to prevent providers.js mock leaking to AgentTool routing tests * fix(copilot): clarify MAX_SUBAGENTS semantics and fix remediation hint log - Document that only MAX_SUBAGENTS=0 and =1 are enforced; values 2-10 have no runtime effect. - Fix the log remediation hint to depend on the actual cause: MAX=0 suppresses sub-agents entirely (not just forces sync), FORCE_SYNC=1 requires unsetting the flag, and MAX>=1 requires ALLOW_SUBAGENTS=1 to restore parallel execution. * docs(env): document GITHUB_COPILOT_* tuning vars in .env.example The Copilot Premium Request optimization introduces four env vars (GITHUB_COPILOT_MAX_SUBAGENTS, GITHUB_COPILOT_ALLOW_SUBAGENTS, GITHUB_COPILOT_FORCE_SYNC_SUBAGENTS, GITHUB_COPILOT_OPTIMIZATION_DISABLED) that change how sub-agents run for CLAUDE_CODE_USE_GITHUB=1 sessions. Previously these were documented only in source comments, which made them undiscoverable for users affected by the new default. Add them to the GitHub Models section (Option 4) of .env.example with descriptions of each var's effect and default value, addressing the reviewer ask to put the new default behavior in user-facing docs. * fix(copilot): telemetry reflects final async mode; docs in README Address outstanding review gaps for #1534: 1. Telemetry is_async/isAsync now uses the final shouldRunAsync value computed once at the top of the function (was duplicating the partial expression, omitting isCoordinator/forceAsync/assistantForceAsync/ proactiveModule signals that contribute to the launch decision). 2. The shouldSuppressSubagentsInCopilotMode() throw now happens before the event log (so a suppressed-agent error isn't followed by a misleading 'is_async: true' event). 3. isCoordinator, forceAsync, assistantForceAsync are now computed once alongside forceSyncCopilot instead of being declared inline later. 4. README: add GitHub Copilot sub-agent optimization subsection under Provider Notes, with the env var table mirroring the .env.example entry (default behavior, cap semantics, all-opt-out). The doc comment in copilotOptimization.ts L16-29 already explains MAX_SUBAGENTS=0/1 enforcement; the test at L186-191 is consistent with the current implementation (positive cap = synchronous). Skipped: getEffectiveConcurrencyCap() in toolOrchestration.ts (the function no longer exists in the current code; the bot's review was based on an earlier version). * fix(copilot): skip <BackgroundHint /> when forced sync When forceSyncCopilot is true the task can no longer be backgrounded (registerAgentForeground is skipped at L918), but the background hint UI was still rendered once the progress threshold elapsed. That advertises a non-existent affordance on every long-running Copilot sub-agent, which is confusing for users. Gate the hint on the same !forceSyncCopilot condition as the foreground registration. Address the CodeRabbit P2 on round 6. * test(copilot): use spyOn instead of mock.module to avoid partial-mock leak CodeRabbit P2 review on round 7 found the copilotOptimization test registered mock.module('./model/providers.js', () => ({ only 4 exports })) which removed all other exports of providers.ts. Downstream tests in the same CI process (e.g. withRetry, domainCheck, apiPreconnect, agent) that import symbols like isFirstPartyAnthropicBaseUrl would then fail with 'Export named ... not found in module' errors. Switch to spyOn() on the real providers module's getAPIProvider. The real module's other exports remain available, and the spy is torn down via mockRestore() in afterEach. Also drop the cache-busting dynamic-import pattern: the spy persists across the static import, so the test no longer needs a fresh module per test. Also fix README P3: the earlier PowerShell heredoc introduced a TAB (0x09) and Form Feed (0x0C) in place of 't' and 'f' in the new Copilot section, rendering 'tengu_agent_tool_selected' as 'engu_...' and 'false' as 'alse'. Rewrite the line with proper 't' and 'f' characters and add backticks for code formatting (was unformatted plain text). Skipped: P2 scheduler-boundary coverage (CodeRabbit round 6 item). That requires driving multiple Agent tool-use blocks through the scheduler in AgentTool/StreamingToolExecutor, which is a larger change than the current PR's scope. * test(copilot): add FORCE_SYNC overrides ALLOW_SUBAGENTS precedence test CodeRabbit round 9: add a test that pins the precedence between GITHUB_COPILOT_FORCE_SYNC_SUBAGENTS=1 and GITHUB_COPILOT_ALLOW_SUBAGENTS=1. The user explicitly asking for synchronous execution must win over the softer "I'm fine with the cap" opt-out. A future reordering of the checks in shouldForceSyncSubagentsInCopilotMode() would silently allow parallel Copilot sub-agent launches when the user asked for sync; this test locks the precedence. Verified locally: 23/23 pass (was 22/22 before adding this test). * fix(copilot): address jatmn round 11 P2/P3 and add scheduler-boundary coverage This commit addresses the latest human + bot review feedback on #1534 across three findings: 1. **P3: Update GitHub Copilot comment in github.ts to use billing-cycle wording.** The previous comment hard-coded "per month (300 for Copilot Free)" — a calendar quota the runtime doesn't own. Mirror the wording from src/utils/copilotOptimization.ts: "per billing cycle, with the exact quota set by the user's Copilot plan." Same docstring shape across both files now. 2. **P2: Add afterEach cleanup to copilotOptimization.test.ts.** Captured the GITHUB_COPILOT_* env vars at module top-level and restore them in afterEach. Previously only beforeEach deleted them, so the precedence test (which sets FORCE_SYNC=1 + ALLOW_SUBAGENTS=1) left those values in process.env after the file completed. Verified by `bun test src/utils/copilotOptimization.test.ts ../copilot-env-probe.test.ts`: before the fix, the probe test sees FORCE_SYNC=1 leaked. After the fix, the probe sees the original env. This is the round 11 P2 review item from jatmn. 3. **P2: Add scheduler-boundary regression test.** New file src/tools/AgentTool/AgentTool.copilotScheduling.test.ts pins the launch↔scheduler alignment by calling `AgentTool.isConcurrencySafe()` directly under each Copilot flag combination. Seven matrix rows: OPTIMIZATION_DISABLED=1, default cap=1, cap=2, ALLOW_SUBAGENTS=1, FORCE_SYNC=1 alone, FORCE_SYNC=1 + ALLOW_SUBAGENTS=1 (precedence), cap=0 (suppressed). A future reorder of the helpers in copilotOptimization.ts that breaks the precedence would fail FORCE_SYNC + ALLOW_SUBAGENTS, locking the launch/scheduling alignment. This is the round 9 / round 11 P2 review item from CodeRabbit + jatmn that has been deferred across multiple rounds. The test uses spyOn on providers.getAPIProvider to control the provider state, then imports AgentTool via cache-busting (?copilotScheduling=... query string) — the same pattern as AgentTool.routing.test.ts. Per-test timeout of 30s absorbs the ~16s one-time AgentTool module load (subsequent tests are sub-1ms because the module is cached after the first beforeAll import). All three changes are verified locally: - `bun test src/utils/copilotOptimization.test.ts` — 23/23 pass - `bun test src/tools/AgentTool/AgentTool.copilotScheduling.test.ts` — 7/7 pass - `bun test --max-concurrency=1` of both files together — 30/30 pass * test(copilot): move per-test timeout to 3rd arg (bun:test API) * fix(copilot): let FORCE_SYNC override MAX_SUBAGENTS=0 + add scheduler-boundary test Two review findings: 1. FORCE_SYNC vs suppression: shouldSuppressSubagentsInCopilotMode() returned true for MAX_SUBAGENTS=0 before FORCE_SYNC was consulted, so GITHUB_COPILOT_MAX_SUBAGENTS=0 + GITHUB_COPILOT_FORCE_SYNC_SUBAGENTS=1 threw "Sub-agents are disabled" instead of running them synchronously, contradicting the documented behavior. FORCE_SYNC (like ALLOW_SUBAGENTS) now bypasses the =0 suppression; docs clarified accordingly. 2. Scheduler-boundary coverage: the existing tests only called isConcurrencySafe() directly. Added a regression that drives multiple Agent tool-use blocks through the real batching path (partitionToolCalls, now exposed via _test): forced-sync splits them into serial single-block batches, ALLOW_SUBAGENTS coalesces them into one concurrent batch. Catches a future divergence between launch and scheduling policy for multiple Agent blocks in one assistant message. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
db2d093af3 |
fix(session): filter Anthropic-only params from 3P provider requests (#248) (#1533)
* fix(session): filter Anthropic-only params from 3P provider requests (#248)
* test: add regression tests for provider gates (PR #1533)
- betas.test.ts: add isAnthropicProvider tests for mistral, xai, minimax
- compact.test.ts: verify compactConversation skips forked-agent
cache-sharing for non-Anthropic providers and uses it for Anthropic
These tests pin the provider gates added in PR #1533:
- getMergedBetas() returns [] for non-Anthropic (betas.test.ts)
- compactConversation()/streamCompactSummary() skip cache-sharing
when isAnthropicProvider() returns false (compact.test.ts)
* test: fix leaky mock harness and add redacted_thinking coverage
- Complete all missing module exports in compact.test.ts mock harness to
prevent global mock leakage breaking other test files
- Add afterAll cleanup hook for safety
- Add regression test for redacted_thinking blocks stripped before
OpenAI-compatible replay in openaiShim
* test: eliminate mock.module leaks causing 10 CI failures
Replace broad mock.module() stubs for betas/providers/envUtils with
env-var-based provider control. Add mock.restore() safety nets to
betas.test.ts and autoCompact.test.ts. Tests now use real modules
without cross-test-file mock contamination.
* test: clear provider profile env vars and remove aggressive mock.restore
- Snapshot and clear CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED flags
so isAnthropicProvider() returns correct values regardless of
provider profile state from previous tests
- Remove mock.restore() from betas.test.ts and autoCompact.test.ts
that was breaking legitimate mocks in other test files
* test(betas): ensure provider env vars are cleared in afterEach, not just beforeEach
* test(3p): also clear VENICE_API_KEY and MIMO_API_KEY per test
resolveEnvOnlyProviderRouteId (src/integrations/routeMetadata.ts:428) returns
'venice' or 'xiaomi-mimo' when those env vars are set, even when the test
sets CLAUDE_CODE_USE_OPENAI=1 — its 'env-only' check runs BEFORE the
USE_OPENAI branch (L658). Without clearing these, leaked values from
earlier tests in the same process cause isAnthropicProvider() to return
the wrong value, so the new provider-gate assertions fail:
getMergedBetas returns [] for the openai provider
expected: []
received: [claude-code-20250219, interleaved-thinking-2025-05-14,
context-management-2025-06-27, prompt-caching-scope-2026-01-05]
isAnthropicProvider is false for the openai/gemini/mistral/xai/minimax
expected: false received: true
compactConversation provider gate > uses forked-agent cache-sharing
for Anthropic providers
expected: runForkedAgent called received: 0 calls
The previous 'CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED[_ID]' fix was
insufficient because those vars don't gate the env-only routes directly;
VENICE_API_KEY / MIMO_API_KEY do. Add them to the per-test cleanup in
both betas.test.ts (PROVIDER_ENV_KEYS) and compact.test.ts (SAVED_ENV).
Skipped: pre-existing CI failures unrelated to this PR's provider gate
(autoCompactIfNeeded circuit breaker × 7, getEffectiveContextWindowSize
on MiniMax M2, microCompact MCP, /export direct filename, getProjectMemoryPathForSelector).
These fail on origin/main with the same pre-PR failures and are out of
scope for #1533.
* test(3p): use crypto.randomUUID() for message uuids
The Message type's uuid field is now a branded UUID type from node:crypto
(strings must match the 5-segment UUID format). After rebase onto the
latest origin/main, the typecheck failed at L26 and L38 where
\\ est-\\\ no longer satisfies the type.
crypto.randomUUID() produces a valid UUID per call, so each message
gets a unique, type-correct uuid. No behavioral change for the tests
themselves — they only rely on the messages being distinct.
* test(3p): cast assistantMessage.message through 'as never'
The Message type's assistant message field is AssistantMessageContent
<BetaContentBlock>, which requires id, model, usage at the type level.
The compact code paths under test don't read these fields, so the
helper focused on the text content needs to bypass the type check.
Pre-rebase this likely passed via a wider union; after the rebase the
branded type tightened. 'as never' keeps the helper minimal without
having to fabricate realistic id/model/usage values.
* test(betas): add non-Claude GitHub regression test for provider gate
CodeRabbit P2 review on the most recent round: the provider gate tests
cover the GitHub Native Anthropic exception (Claude model) but no
sibling assertion for CLAUDE_CODE_USE_GITHUB=1 with a non-Claude model.
That untested branch is the risky half of the gate — a future broadening
of isGithubNativeAnthropicMode() (e.g. matching on the wrong substring,
or matching on 'claude' too permissively) would silently re-introduce
Anthropic-only beta headers for OpenAI-style models served via GitHub.
Add the inverse case: GitHub provider with OPENAI_MODEL='gpt-4o-mini' must
return [] (the gate strips the headers). Verified locally: betas.test.ts
runs 19/19 (with the new test) and the betas+compact pair runs 21/21
cleanly, confirming no test-isolation regression between the two files.
* docs(compact): update stale "3P default: true" comments
CodeRabbit round 10 nitpick: the comment at compact.ts L434-439 still
claims "3P default: true" and frames the GB flag as a "kill-switch",
but the code is now gated by isAnthropicProvider() so non-Anthropic
3P providers are never on this path in the first place. Update both
the compactConversation() block and the streamCompactSummary() block
(now points at the first block instead of a separate stale sentence)
to describe the actual current behavior: cache-sharing is enabled
only for Anthropic-capable providers when tengu_compact_cache_prefix
is on; non-Anthropic providers remain incompatible and would send
Anthropic-only params they reject.
Verified: 2/2 compact.test.ts still passes; no behavioral change, only
the comment text.
Skipped: rebase to current origin/main (#1533 was rebased earlier in
4c1b4c2; no new conflicts since).
* test(3p): also clear NEARAI_API_KEY per test
Reproduce locally with `bun test --max-concurrency=1` and find that
`providerValidation.test.ts` (which runs alongside the other 3800+ test
files in the same CI process) sets NEARAI_API_KEY for one of its
neearai-detect tests, and the previous cleanup list missed that env
var. So when betas.test.ts and compact.test.ts later run their
`isAnthropicProvider is true for firstParty` (and the other
`isAnthropicProvider is true for *`) tests, hasNearaiEnvOnlyProviderIntent()
returns true on the leaked value, the function classifies the provider
as 'nearai' (not Anthropic), and the gate strips all betas. The test
that sets no env var then sees `getMergedBetas()` return `[]` instead
of the Anthropic list, which is the failure pattern on CI.
resolveEnvOnlyProviderRouteId (src/integrations/routeMetadata.ts:428)
returns 'nearai' (or 'xai', 'minimax', 'venice', 'xiaomi-mimo') for
those env var leak paths, all of which were missed by the prior
profile-env-var fix. The previous CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED
fix was insufficient because that var doesn't gate the env-only routes
directly — only the API keys do. NEARAI_API_KEY is the latest in the
family (xai/minimax/venice/mimo all already in the list).
Add NEARAI_API_KEY to PROVIDER_ENV_KEYS in betas.test.ts and
SAVED_ENV in compact.test.ts, matching the xai/minimax/venice/mimo
additions from 768ab65.
Verified: betas.test.ts (19/19) and compact.test.ts (2/2) still
pass in isolation and as a pair. The full-suite failure should now
resolve since the leak path is covered.
Skipped: tracing the exact leak site. The 3800+ test files can't
all be enumerated; the fix is comprehensive (all known env-only
intent vars are now in the clear list).
* test(3p): pre-warm importFreshBetas in beforeAll to avoid 5s timeout
The first call to importFreshBetas() in this file triggered a 4.3s module
load (growthbook feature-flag init), which exceeded Bun's default 5s test
timeout. On the agent's local Windows machine, that race is close to
deterministic (the openai test, being first in source order, paid the
full cost and reported `(fail) this test timed out after 5000ms`). On CI
(Ubuntu) the import was usually fast enough to pass — but flaky, and
completely unrelated to actual test correctness.
Move the slow import to beforeAll so it happens once before any test
runs and is not charged to the first test's budget. After pre-warming,
every per-test import is sub-second (the 579ms / 8ms timings in the
last run confirm this). All 21 tests in the betas+compact pair now pass
deterministically.
This is the only remaining source-order flake in this file. The actual
test logic (provider-gate assertions, env-var isolation, cache-busting
imports) was already correct.
* test(3p): add FIREWORKS_API_KEY to provider env cleanup
CI smoke run on #1533 still shows the openai/gemini/bedrock tests
failing in `getAPIProvider` even after the pre-warm + clearProviderEnv
fix from
|
||
|
CalandGitHub
|
1e8c1ac8f5 | feat(github): expose all 21 Copilot models with context window metadata (#822) (#1535) | ||
|
14e5a41acd |
feat: extend --fallback-model to interactive REPL sessions (#1346) (#1419)
* feat: thread --fallback-model into REPL interactive sessions (#1346) * doc: remove outdated --print-only note from --fallback-model help text * fix: thread fallbackModel through ResumeConversation to REPL * fix: thread fallbackModel through background session path * test: add regression coverage for fallbackModel prop plumbing * test: rewrite fallback test to read sources as text (no transitive imports) * test: assert specific query paths for fallbackModel plumbing * test: assert specific call sites for fallbackModel plumbing The previous regression test counted fallbackModel tokens in REPL.tsx and used a loose 8KB window after startBackgroundSession(. Either the foreground query({ ... }) call or the background queryParams = { ... } entry could be removed and the test would still pass from the prop type, destructuring, dep array, and the other path's token. Replace the loose assertions with bounded regex/snippet checks anchored to each specific call site: - REPL function signature: function REPL({ ... fallbackModel ... }: Props) - foreground: a complete query({ ... fallbackModel ... }) call - resume: the <REPL ... /> element contains fallbackModel={fallbackModel} - background: queryParams block contains fallbackModel, and startBackgroundSession(...) references queryParams - main.tsx: launchResumeChooser(...) uses ...sessionConfig spread, and the sessionConfig is built with fallbackModel: userSpecifiedFallbackModel The main.tsx call is multi-line with nested objects and other arg lists (e.g. getWorktreePaths(getOriginalCwd())); a balanced-paren scan finds the matching close so the spread assertion sees the full call body. Addresses the jatmn review thread on #1346. * fix(1346): forward --fallback-model through SSH; tighten background test - SSH interactive pre-extract now mirrors --model handling for --fallback-model so the flag survives the remote spawn via _pendingSSH.extraCliArgs (handles both space and =value forms). - Background test no longer relies on a loose /queryParams[\\s\\S]*?fallbackModel/ regex: replaced with a balanced-brace walker that bounds the assertion to the body of the queryParams object literal in REPL.tsx, so a regression that drops fallbackModel from the queryParams block can no longer be masked by a later reference (e.g. useEffect dep array). Addresses round-4 review feedback on #1419. * test(1346): use paren-walking for startBackgroundSession call slice The startBackgroundSession(...) call in REPL.tsx contains nested function calls (getQuerySourceForREPL, getAutoCompactTrackingForSession, setAutoCompactTrackingForSession), so source.indexOf(')') landed on the first inner close-paren instead of the call's matching close. The test only passed because queryParams happened to appear in the outer object before those nested calls. Replaced with the same paren-walking depth-tracking pattern used by the launchResumeChooser test in this file. Bounds the slice to the actual startBackgroundSession call body, so a regression that moves queryParams out of the outer argument list (e.g. into a nested helper) is now caught by the assertion. Addresses CodeRabbit nitpick on #1419. --------- Co-authored-by: adityachaudhary99 <adityachaudhary99@users.noreply.github.com> |
||
|
CalandGitHub
|
1c279577f9 |
feat: add .gitattributes to enforce LF line endings (#1550)
* feat: add .gitattributes to enforce LF line endings Prevents Windows/NTFS contributors with core.autocrlf=true from accidentally converting the entire codebase CRLF→LF, which produces PRs with +600k/-600k diffs across 700+ files. - * text=auto normalizes all text files to LF on checkin - Explicit binary markers for images, fonts, archives, compiled code - No code changes — config-only, zero risk * fix: treat SVGs as text, not binary SVGs are XML-based vector files and should be diffable/mergeable. CodeRabbit review: https://github.com/Gitlawb/openclaude/pull/1550#discussion_r4632644506 * fix: renormalize CRLF TypeScript files to LF The new *.ts text attribute exposed 3 TS files with CRLF in the index. Renormalizing so a fresh Windows checkout stays clean. Fixes jatmn's P2 finding. |
||
|
CalandGitHub
|
7df7cad333 |
fix: show all configured Mistral models and fix model selection priority (#1360) (#1418)
* fix(mistral): show all configured models in model picker (#1360) * fix(mistral): use correct API model names and prepend selected model * chore(mistral): fix comment and normalize model ids for consistency * fix(mistral): remove retired models, fix descriptor ids, add multi-model test * fix(mistral): preserve original delimiter and fix codestral descriptor id * fix(mistral): revert codestral descriptor to match shared model registry * fix(mistral): revert profile mutation; align with documented contract persistActiveProviderProfileModel() is now a no-op that returns the active profile. Runtime model selection is a session-level choice handled by mainLoopModelOverride (set by onChangeAppState before this helper is called); the profile's model list should only change via an explicit provider edit, not as a side-effect of /model. This addresses the Copilot and jatmn review threads that flagged the prior prepend behavior: - contradicted the comment above the function stating session-level switching is owned by mainLoopModelOverride - caused unbounded list growth on rotation (A->B->C->D on a profile starting with A; B produced D; C; B; A; B) - used a separator inferred from a single-character substring of the model field that broke on mixed-separator inputs - the catalog also used inconsistent id naming Catalog ids are normalized to the mistral-* scheme (mistral-devstral, mistral-large, mistral-small, mistral-ministral-3b, mistral-codestral) to match the existing prefix convention. apiName and modelDescriptorId are unchanged so route metadata and descriptor lookups are unaffected. New coverage: - providerProfiles.test.ts: locks the no-op contract for single- and multi-model profiles (semicolon and comma separators) and the in-list pick path that returns the active profile unchanged. Resolves #1360 |
||
|
CalandGitHub
|
ac3ae10936 |
fix(bash): show output for ! shell commands (#1265) (#1395)
* fix(bash): show output for ! shell commands (#1265) Apply the fix from upstream PR #1270: use raw stdout (with escapeXml) for normal ! commands, only use processToolResultBlock when output is persisted or backgrounded. This prevents the model-facing formatter from silently losing stdout. Fixes #1265 * fix(bash): address PR review comments - Remove escapeXml from backgrounded formatter output (trusted XML) - Force bash routing in tests via mock.module to avoid PowerShellTool routing on Windows * chore(bash): clarify formatter output is trusted in both metadata branches * fix(bash): decode XML entities in user-visible bash stdout/stderr display * fix(bash): fix unescapeXml entity order and decode in export path * fix(bash): only unescape stdout/stderr in exports, not bash-input |