Oracle review on PR #4132 flagged that resetStallStateForPlanChange only
reset state when stalledContinuationReason was already set, so partial
in-progress counts from plan A could carry to plan B and abort the new
plan on its first idle.
Adds activeContinuationPlanPath to SessionState and resets the
no-tool-progress counter + awaiting state whenever the active plan path
changes, even when no stall has occurred yet. The existing stalled-plan
reset still fires when the previously stalled plan path differs from
the new one.
Raise test token counts from 170K/180K to 800K/810K so compaction triggers at 78% of 1M instead of 78% of 200K, matching the actual GA context limit for 4.6 models.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Address Oracle review feedback: refactor 4 aliased mutations via argsObject
in plugin/tool-execute-before.ts and 1 via toolOutput in atlas/tool-execute-before.ts.
Strengthen audit test regex to catch Output.args mutations regardless of the
variable name prefix (toolOutput, argsObject aliases).
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Add replace-tool-args.audit.test.ts that scans src/**/*.ts for direct
output.args property assignments and Object.assign(output.args, ...) outside
the helper. Also fix the 9th mutation site discovered by the audit in
compaction-todo-preserver/hook.ts.
Add replace-tool-args.test.ts with 12 regression tests covering both mutable
and Object.freeze'd output.args scenarios for all hook patterns.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Replace output.args.command and output.args.prompt direct assignments
with replaceToolArgs() in non-interactive-env, prometheus-md-only,
and sisyphus-junior-notepad hooks.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Add 4 regression tests covering the pre-set effort path:
- pre-set effort=max + variant=max + github-copilot Opus -> clamped to high
- pre-set effort=max + variant=high + github-copilot Opus -> clamped to high (cubic violation case from PR #3608)
- pre-set effort=max + non-constrained Opus -> max preserved (no regression)
- pre-set effort=high + github-copilot Opus -> high preserved (don't overwrite valid pre-set)
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Move the pre-set effort handling BEFORE the message.variant !== "max" early-return so that output.options.effort="max" set via session params or model-requirements fallback chains is always clamped to "high" on constrained providers (github-copilot, Anthropic OAuth), even when message.variant is not "max".
This addresses the cubic violation from PR #3608 where the clamp block was gated behind the variant-based return.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Replace output.args.todos = parsed and Object.assign(output.args, result.modifiedInput)
with replaceToolArgs() calls that create a shallow clone instead of
mutating the potentially-frozen args object.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Addresses cubic-dev-ai P1 finding on #4113 (#4113 review).
The original chain `extractErrorName(error)?.toLowerCase().replace(...)`
is semantically safe — JavaScript optional chaining short-circuits the
ENTIRE access chain when the head returns null/undefined, so when
`extractErrorName` returns undefined the whole expression evaluates to
undefined without ever reaching `.replace()`. Verified empirically via
`const x = undefined; x?.toLowerCase().replace(/_/g, "")` returns
undefined with no crash.
Applying the suggested defensive `?.` before `.replace` anyway, since
it is semantically a no-op and explicit chaining at each hop is easier
for static analyzers to reason about.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes#3607.
## Root cause
On Windows, `detectCommandShellType()` fell through to `detectShellType()`
for two common environments and incorrectly returned `"unix"`:
1. **SHELL points at a Unix-shaped path** (e.g. Git Bash sets
`SHELL=/usr/bin/bash` on a fresh Windows install).
The `detectWindowsShellType(process.env.SHELL)` probe didn't recognize
`bash` as a Windows shell, so the function fell through and
`detectShellType()` returned `"unix"`.
2. **MSYSTEM is set but SHELL is not** (Git Bash leaves MSYSTEM permanently
set system-wide even when the active shell is PowerShell).
The fall-through path returned `"unix"` via the MSYSTEM check.
In both cases, the hook then prepended `export KEY=val;` to git commands,
which PowerShell rejects with:
`export : 无法将"export"项识别为 cmdlet...`
OpenCode on Windows runs the bash tool through a Windows shell
(PowerShell by default, cmd as the user-overridable fallback), regardless
of MSYSTEM or a Unix-shaped SHELL set by Git Bash — so the env prefix
must use Windows-compatible syntax.
## Fix
`detectCommandShellType()` now short-circuits on `process.platform === "win32"`:
- If `SHELL` points at a recognized Windows shell (`cmd.exe`, `powershell.exe`,
`pwsh.exe`), return that.
- If `SHELL` and `MSYSTEM` are both unset, fall back to `ComSpec` then to cmd.
- Otherwise, default to PowerShell — matching what OpenCode actually spawns.
`detectShellType()` is unchanged; other callers (including non-Windows
platforms) are unaffected.
## Test changes
Three pre-existing tests encoded the buggy behavior as expected behavior
and have been updated to assert the new PowerShell syntax with a
`(#3607)` marker and a comment explaining why a Unix-shaped SHELL on
win32 must still resolve to PowerShell. WSL is not affected because in
WSL `process.platform === "linux"`, not `"win32"`.
- `src/hooks/non-interactive-env/`: 24 tests pass / 0 fail
- `bunx tsc --noEmit`: clean
## Note on issue thread
The sisyphus-bot triage comment on #3607 framed this as a policy choice
between (A) forcing Windows env-prefix syntax and (B) resolving against
the OpenCode-configured shell. This PR implements option (A) as the
minimal surgical fix; option (B) remains a follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses cubic-dev-ai bot review on #4113 (P2): the RESOURCE_EXHAUSTED
and snake_case insufficient_quota fixtures contained quota-shaped
messages that already matched pre-existing message regexes, so the tests
passed even without the new errorName allow-list entry and the
underscore normalization respectively.
Replace both fixture messages with a generic "Request failed." so the
only path to a `quota_exceeded` classification is via the new code:
- RESOURCE_EXHAUSTED: only the new `errorName?.includes("resourceexhausted")`
match on the normalized name can fire.
- insufficient_quota (snake_case): only the new underscore-stripping
normalization can route the name to `insufficientquota` and match the
existing allow-list entry.
The third new test (Google ResourceExhausted message-only) is unchanged
because its message uniquely matches only the new
`/resource.?exhausted/i` pattern and not any existing quota regex.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes#3937.
Adds three small classification gaps to `classifyErrorType` so that
quota-exhaustion errors from a wider range of providers trigger
configured fallback chains instead of looping retry attempts:
- Normalize error names by stripping `_` and `-` so snake_case /
SCREAMING_SNAKE_CASE provider names (`insufficient_quota`,
`RESOURCE_EXHAUSTED`, `rate_limit_exceeded`) match the existing
alphanumeric `.includes()` checks.
- Add `resourceexhausted` to the quota error-name allow-list to cover
Google Generative AI's gRPC code 8 / `ResourceExhausted` surface.
- Add `/resource.?exhausted/i` to the quota message-pattern list so the
same error surface is caught when the provider only sets a generic
error name but puts the signal in the message.
Three new regression tests in
`quota-error-classifier.regression.test.ts` cover:
- Google `RESOURCE_EXHAUSTED` (gRPC error name + quota-shaped message)
- Google `ResourceExhausted` message form without HTTP status
- OpenAI snake_case `insufficient_quota` error name
No existing tests were touched; the underscore normalization preserves
all existing `.includes()` matches by rewriting the one underscore-bearing
literal (`ai_loadapikeyerror` → `ailoadapikeyerror`) so previously
matched names still resolve.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Guard command.execute.before against injecting when parts already
contain auto-slash-command tags, preventing duplication when both
chat.message and command.execute.before fire for the same slash command.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
YeonGyu-Kim