Skip to content

refactor(core): move shell tool config into state - #43476

Open
nexxeln wants to merge 1 commit into
v2from
shell-tool-policy
Open

refactor(core): move shell tool config into state#43476
nexxeln wants to merge 1 commit into
v2from
shell-tool-policy

Conversation

@nexxeln

@nexxeln nexxeln commented Aug 19, 2026

Copy link
Copy Markdown
Member

summary

  • expose effective truncation limits from ToolOutput
  • project portable scanner config into reloadable ShellPolicy state
  • remove direct Config reads from ShellTool while preserving live reload behavior

testing

  • bun typecheck from packages/core
  • bun test test/config/shell.test.ts test/config/tool-output.test.ts
  • bun test test/tool-shell.test.ts
  • bun test test/location-layer.test.ts
  • bun test test/workerd.test.ts from packages/server
  • focused config and shell policy tests with --rerun-each 5
  • pre-push monorepo typecheck

@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

  1. packages/core/src/tool/plugin/shell.ts:167 — portable_shell_scanner used to be read directly from config at every guarded invocation, independent of which plugins are enabled; it now flows only through ConfigShellPlugin, so disabling that internal plugin (-opencode.shell-config) silently reverts the scanner to false even when the experimental flag is set in config — if that's intended, fine, but it's an observable behavior change worth noting; otherwise wire the policy transform into a always-on path.

  2. packages/core/src/config/plugin/shell.ts:14 — the refresh chain is wired after loaded.entries = yield* config.entries() and the subscription starts inside the same effect; a config.updated firing between the initial read and Stream.subscribe is lost, leaving stale portableScanner/shell selection until the next update — subscribe first or do one post-subscription reload, as noted for the variant plugin too (shared helper candidate).

  3. packages/core/test/tool-shell.test.ts:548/:629 — two behavior tests flipped from it.live to productionIt.live; that excludes them from the default CI run precisely where the config-to-state plumbing they cover just changed — please confirm they still execute somewhere (nightly/local profile) or add slimmer always-on equivalents asserting the new ShellPolicy/ToolOutput.limits() paths.

  4. packages/core/config/plugin/shell.ts:17 — Effect.all([a, b], { concurrency: "unbounded" }) for exactly two synchronous-ish reloads adds noise without benefit; plain Effect.all([shell.reload(), policy.reload()], { discard: true }) (sequential) is clearer and avoids interleaved state writes.

  5. Nit — packages/core/src/shell/policy.ts:9 — Data.portableScanner: boolean + single-method Draft is fine, but consider naming the draft method configurePortableScanner (or accepting an object) so future policy knobs don't turn configure(x) into a positional-boolean archaeology dig.

Overall: solid refactor — pulling config parsing out of the per-invocation shell path and the truncation loop into event-refreshed state is the right shape, and the polling-based reload tests verify the wiring end to end. Item 1 deserves an explicit decision; the rest is polish. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants