sync
This commit is contained in:
parent
d97523adbd
commit
137992ca85
5 changed files with 26 additions and 145 deletions
|
|
@ -1,136 +0,0 @@
|
||||||
# Bun shell migration plan
|
|
||||||
|
|
||||||
Practical phased replacement of Bun `$` calls.
|
|
||||||
|
|
||||||
## Goal
|
|
||||||
|
|
||||||
Replace runtime Bun shell template-tag usage in `packages/opencode/src` with a unified `Process` API in `util/process.ts`.
|
|
||||||
|
|
||||||
Keep behavior stable while improving safety, testability, and observability.
|
|
||||||
|
|
||||||
Current baseline from audit:
|
|
||||||
|
|
||||||
- 143 runtime command invocations across 17 files
|
|
||||||
- 84 are git commands
|
|
||||||
- Largest hotspots:
|
|
||||||
- `src/cli/cmd/github.ts` (33)
|
|
||||||
- `src/worktree/index.ts` (22)
|
|
||||||
- `src/lsp/server.ts` (21)
|
|
||||||
- `src/installation/index.ts` (20)
|
|
||||||
- `src/snapshot/index.ts` (18)
|
|
||||||
|
|
||||||
## Decisions
|
|
||||||
|
|
||||||
- Extend `src/util/process.ts` (do not create a separate exec module).
|
|
||||||
- Proceed with phased migration for both git and non-git paths.
|
|
||||||
- Keep plugin `$` compatibility in 1.x and remove in 2.0.
|
|
||||||
|
|
||||||
## Non-goals
|
|
||||||
|
|
||||||
- Do not remove plugin `$` compatibility in this effort.
|
|
||||||
- Do not redesign command semantics beyond what is needed to preserve behavior.
|
|
||||||
|
|
||||||
## Constraints
|
|
||||||
|
|
||||||
- Keep migration phased, not big-bang.
|
|
||||||
- Minimize behavioral drift.
|
|
||||||
- Keep these explicit shell-only exceptions:
|
|
||||||
- `src/session/prompt.ts` raw command execution
|
|
||||||
- worktree start scripts in `src/worktree/index.ts`
|
|
||||||
|
|
||||||
## Process API proposal (`src/util/process.ts`)
|
|
||||||
|
|
||||||
Add higher-level wrappers on top of current spawn support.
|
|
||||||
|
|
||||||
Core methods:
|
|
||||||
|
|
||||||
- `Process.run(cmd, opts)`
|
|
||||||
- `Process.text(cmd, opts)`
|
|
||||||
- `Process.lines(cmd, opts)`
|
|
||||||
- `Process.status(cmd, opts)`
|
|
||||||
- `Process.shell(command, opts)` for intentional shell execution
|
|
||||||
|
|
||||||
Git helpers:
|
|
||||||
|
|
||||||
- `Process.git(args, opts)`
|
|
||||||
- `Process.gitText(args, opts)`
|
|
||||||
|
|
||||||
Shared options:
|
|
||||||
|
|
||||||
- `cwd`, `env`, `stdin`, `stdout`, `stderr`, `abort`, `timeout`, `kill`
|
|
||||||
- `allowFailure` / non-throw mode
|
|
||||||
- optional redaction + trace metadata
|
|
||||||
|
|
||||||
Standard result shape:
|
|
||||||
|
|
||||||
- `code`, `stdout`, `stderr`, `duration_ms`, `cmd`
|
|
||||||
- helpers like `text()` and `arrayBuffer()` where useful
|
|
||||||
|
|
||||||
## Phased rollout
|
|
||||||
|
|
||||||
### Phase 0: Foundation
|
|
||||||
|
|
||||||
- Implement Process wrappers in `src/util/process.ts`.
|
|
||||||
- Refactor `src/util/git.ts` to use Process only.
|
|
||||||
- Add tests for exit handling, timeout, abort, and output capture.
|
|
||||||
|
|
||||||
### Phase 1: High-impact hotspots
|
|
||||||
|
|
||||||
Migrate these first:
|
|
||||||
|
|
||||||
- `src/cli/cmd/github.ts`
|
|
||||||
- `src/worktree/index.ts`
|
|
||||||
- `src/lsp/server.ts`
|
|
||||||
- `src/installation/index.ts`
|
|
||||||
- `src/snapshot/index.ts`
|
|
||||||
|
|
||||||
Within each file, migrate git paths first where applicable.
|
|
||||||
|
|
||||||
### Phase 2: Remaining git-heavy files
|
|
||||||
|
|
||||||
Migrate git-centric call sites to `Process.git*` helpers:
|
|
||||||
|
|
||||||
- `../core/src/filesystem.ts`
|
|
||||||
- `src/project/vcs.ts`
|
|
||||||
- `../core/src/filesystem/watcher.ts`
|
|
||||||
- `src/storage/storage.ts`
|
|
||||||
- `src/cli/cmd/pr.ts`
|
|
||||||
|
|
||||||
### Phase 3: Remaining non-git files
|
|
||||||
|
|
||||||
Migrate residual non-git usages:
|
|
||||||
|
|
||||||
- `src/cli/cmd/tui/util/clipboard.ts`
|
|
||||||
- `src/util/archive.ts`
|
|
||||||
- `../core/src/filesystem/ripgrep.ts`
|
|
||||||
- `src/tool/bash.ts`
|
|
||||||
- `src/cli/cmd/uninstall.ts`
|
|
||||||
|
|
||||||
### Phase 4: Stabilize
|
|
||||||
|
|
||||||
- Remove dead wrappers and one-off patterns.
|
|
||||||
- Keep plugin `$` compatibility isolated and documented as temporary.
|
|
||||||
- Create linked 2.0 task for plugin `$` removal.
|
|
||||||
|
|
||||||
## Validation strategy
|
|
||||||
|
|
||||||
- Unit tests for new `Process` methods and options.
|
|
||||||
- Integration tests on hotspot modules.
|
|
||||||
- Smoke tests for install, snapshot, worktree, and GitHub flows.
|
|
||||||
- Regression checks for output parsing behavior.
|
|
||||||
|
|
||||||
## Risk mitigation
|
|
||||||
|
|
||||||
- File-by-file PRs with small diffs.
|
|
||||||
- Preserve behavior first, simplify second.
|
|
||||||
- Keep shell-only exceptions explicit and documented.
|
|
||||||
- Add consistent error shaping and logging at Process layer.
|
|
||||||
|
|
||||||
## Definition of done
|
|
||||||
|
|
||||||
- Runtime Bun `$` usage in `packages/opencode/src` is removed except:
|
|
||||||
- approved shell-only exceptions
|
|
||||||
- temporary plugin compatibility path (1.x)
|
|
||||||
- Git paths use `Process.git*` consistently.
|
|
||||||
- CI and targeted smoke tests pass.
|
|
||||||
- 2.0 issue exists for plugin `$` removal.
|
|
||||||
|
|
@ -1,4 +1,5 @@
|
||||||
import { NamedError } from "@opencode-ai/core/util/error"
|
import { NamedError } from "@opencode-ai/core/util/error"
|
||||||
|
import { ConfigErrorV1 } from "@opencode-ai/core/v1/config/error"
|
||||||
import { Cause, Effect } from "effect"
|
import { Cause, Effect } from "effect"
|
||||||
import { HttpRouter, HttpServerError, HttpServerRespondable, HttpServerResponse } from "effect/unstable/http"
|
import { HttpRouter, HttpServerError, HttpServerRespondable, HttpServerResponse } from "effect/unstable/http"
|
||||||
|
|
||||||
|
|
@ -15,6 +16,15 @@ export const errorLayer = HttpRouter.middleware<{ handles: unknown }>()((effect)
|
||||||
if (!defect) return Effect.failCause(cause)
|
if (!defect) return Effect.failCause(cause)
|
||||||
|
|
||||||
const error = defect.defect
|
const error = defect.defect
|
||||||
|
if (
|
||||||
|
ConfigErrorV1.JsonError.isInstance(error) ||
|
||||||
|
ConfigErrorV1.InvalidError.isInstance(error) ||
|
||||||
|
ConfigErrorV1.FrontmatterError.isInstance(error) ||
|
||||||
|
ConfigErrorV1.DirectoryTypoError.isInstance(error)
|
||||||
|
) {
|
||||||
|
return Effect.succeed(HttpServerResponse.jsonUnsafe(error.toObject(), { status: 400 }))
|
||||||
|
}
|
||||||
|
|
||||||
const ref = `err_${crypto.randomUUID().slice(0, 8)}`
|
const ref = `err_${crypto.randomUUID().slice(0, 8)}`
|
||||||
|
|
||||||
return Effect.logError("failed", { ref, error, cause: Cause.pretty(cause) }).pipe(
|
return Effect.logError("failed", { ref, error, cause: Cause.pretty(cause) }).pipe(
|
||||||
|
|
|
||||||
|
|
@ -264,7 +264,7 @@ export function createRoutes(
|
||||||
]),
|
]),
|
||||||
Layer.provide(Layer.succeed(CorsConfig)(corsOptions)),
|
Layer.provide(Layer.succeed(CorsConfig)(corsOptions)),
|
||||||
Layer.provide(InstanceLayer.layer),
|
Layer.provide(InstanceLayer.layer),
|
||||||
Layer.provide(Observability.layer),
|
Layer.provideMerge(Observability.layer),
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -53,7 +53,7 @@ describe("HttpApi error middleware", () => {
|
||||||
}),
|
}),
|
||||||
)
|
)
|
||||||
|
|
||||||
it.live("does not expose config defects from generic middleware", () =>
|
it.live("returns invalid config defects as structured client errors", () =>
|
||||||
Effect.gen(function* () {
|
Effect.gen(function* () {
|
||||||
const configError = new ConfigErrorV1.InvalidError({
|
const configError = new ConfigErrorV1.InvalidError({
|
||||||
path: "/tmp/opencode.json",
|
path: "/tmp/opencode.json",
|
||||||
|
|
@ -70,11 +70,16 @@ describe("HttpApi error middleware", () => {
|
||||||
const body = yield* response.json
|
const body = yield* response.json
|
||||||
const serialized = JSON.stringify(body)
|
const serialized = JSON.stringify(body)
|
||||||
|
|
||||||
expect(response.status).toBe(500)
|
expect(response.status).toBe(400)
|
||||||
expectUnknownErrorBody(body)
|
expect(body).toMatchObject({
|
||||||
expect(serialized).not.toContain("/tmp/opencode.json")
|
name: "ConfigInvalidError",
|
||||||
expect(serialized).not.toContain("provider")
|
data: {
|
||||||
expect(serialized).not.toContain("anthropic")
|
path: "/tmp/opencode.json",
|
||||||
|
issues: [{ message: "Expected object", path: ["provider", "anthropic", "options"] }],
|
||||||
|
},
|
||||||
|
})
|
||||||
|
expect(serialized).toContain("/tmp/opencode.json")
|
||||||
|
expect(serialized).toContain("anthropic")
|
||||||
}),
|
}),
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,8 @@
|
||||||
import type { CliRenderer } from "@opentui/core"
|
import type { CliRenderer } from "@opentui/core"
|
||||||
|
|
||||||
export function destroyRenderer(renderer: Pick<CliRenderer, "isDestroyed" | "setTerminalTitle" | "destroy">) {
|
export function destroyRenderer(renderer: Pick<CliRenderer, "isDestroyed" | "setTerminalTitle" | "destroy">) {
|
||||||
renderer.setTerminalTitle("")
|
if (!renderer.isDestroyed) {
|
||||||
if (!renderer.isDestroyed) renderer.destroy()
|
renderer.setTerminalTitle("")
|
||||||
|
renderer.destroy()
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue