refactor: sync opencode config schemas#102
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds many new Effect-based config schemas and loaders, restructures config exports, enhances Effect→Zod translation, implements config dependency detection/install gating in the tool registry, expands TUI/server/util exports, adds extensive tests, and bumps several package versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as Config Loader
participant FS as Filesystem
participant Parser as Frontmatter Parser
participant Validator as Effect/Zod Validator
participant Bus as Event Bus
participant Log as Logger
Loader->>FS: scan `{agent,agents}/**/*.md`
FS-->>Loader: file list
loop per file
Loader->>FS: read file content
FS-->>Loader: content
Loader->>Parser: parse frontmatter + prompt
alt frontmatter parse fails
Parser-->>Loader: parse error
Loader->>Log: log parse error
Loader->>Bus: publish Session.Event.Error
else
Parser-->>Loader: frontmatter + prompt
Loader->>Validator: Info.safeParse(...)
alt validation fails
Validator-->>Loader: validation error
Loader->>Log: log validation error
Loader->>Bus: publish Session.Event.Error
else
Validator-->>Loader: validated config
Loader-->>Loader: accumulate result
end
end
end
Loader-->>Loader: return Record<string, Info>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request refactors the configuration system to use effect/Schema and enhances the effect-zod bridge to support transforms, defaults, and preprocessing. It introduces new configuration modules for agents, commands, keybinds, and providers, while updating the SDK to include experimental workspace and console management endpoints. Feedback highlights opportunities to improve robustness by handling dependency installation and validation errors more gracefully, as well as suggestions to refine type definitions and idiomatic usage in the agent configuration logic.
a008583 to
3acc639
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a008583a70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Astro-Han
left a comment
There was a problem hiding this comment.
Found two config validation regressions that should be fixed before merge.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Astro-Han
left a comment
There was a problem hiding this comment.
Migrated the remaining actionable top-level review notes into inline comments.
✅ Actions performedReview triggered.
|
Astro-Han
left a comment
There was a problem hiding this comment.
Nitpick review. All specific observations are inline comments on the affected files.
Summary: 10 P1 suggestions (error handling, priority issues, API inconsistencies, etc.), 4 P2 notes (temporary hacks, type contracts, OpenAPI changes, etc.). All are minor details that do not block merge.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/config/config.test.ts`:
- Around line 827-861: The test "duplicate derived command names keep the first
command" relies on glob brace expansion order so it can be flaky; make it
deterministic by ensuring the loader or test enforces a stable precedence
between ".opencode/command" and ".opencode/commands": either change load() (the
duplicate-resolution logic) to explicitly prefer entries from the singular
directory over the plural one when building config.command, or in the test
sort/normalize the discovered file list before asserting (e.g., ensure files
from singularDir are processed first) so expect(config.command?.["dup"]) always
reflects the singular file; update the unique symbols load(), config.command,
and the tmpdir init that creates singularDir and pluralDir accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4da4666e-26b9-4ee6-a21d-8fc6399dd6da
📒 Files selected for processing (2)
packages/opencode/src/config/command.tspackages/opencode/test/config/config.test.ts
Astro-Han
left a comment
There was a problem hiding this comment.
Summary
This PR brings in the OpenCode v1.14.19 config schema sync while preserving PawWork compatibility. Overall well-structured for a base sync PR. A few observations:
1. Config namespace export style change
The index.ts export style changed from:
// Before (dev)
import { Config } from "./config"
export { Config }To:
// After (PR)
export * as Config from "./config"This changes Config from a direct value/namespace import to a barrel re-export. While TypeScript consumers using import { Config } from "@/config/config" directly should remain unaffected, callers using import { Config } from "@/config" now receive a namespace wrapper rather than the original Config namespace.
2. Self-referencing barrel exports pattern
Each config module file now starts with:
export * as ConfigAgent from "./agent"
// ... rest of module implementationThis pattern allows internal code to reference ConfigAgent.Info before the actual export is defined. While TypeScript handles this correctly, the ordering is conceptually unusual — consider documenting why this is intentional if it's a deliberate pattern.
3. PawWork compatibility ✓
PROJECT_CONFIG_NAMES = ["opencode", "pawwork"]GLOBAL_CONFIG_LOAD_FILESandGLOBAL_CONFIG_UPDATE_FILESincludepawwork.json/pawwork.jsoncglobalConfigFile()defaults topawwork.jsonneedsConfigDependenciesandusesConfigDependenciesretainedConsoleStateEffect Schema migration with.zodcompatibility
4. Test coverage
New tests for pawwork.json config file loading and updates cover the PawWork-specific paths.
CI passes, PawWork compatibility preserved. Ready to merge.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/opencode/src/config/keybinds.ts (1)
125-127:⚠️ Potential issue | 🟠 MajorMake the exported keybind schema strict.
This wrapper only checks
instanceof z.ZodObject; it never calls.strict(). For a fixed-key config surface, typos likeledaerwill be silently stripped instead of rejected. Mirroring theServer.zodpattern here would preserve typo detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/config/keybinds.ts` around lines 125 - 127, The exported Keybinds schema currently casts KeybindsZod to a ZodObject without enforcing strictness, so unknown keys are stripped instead of rejected; change the validation to ensure KeybindsZod is a z.ZodObject and call .strict() (e.g., derive a strictZod = KeybindsZod.strict() or verify KeybindsZod.strict() before casting) and export that strict object as Keybinds (reference symbols: KeybindsZod, Keybinds, KeybindsSchema, z.ZodObject, .strict()) so typos in the fixed key surface are rejected.packages/opencode/src/config/agent.ts (1)
151-161:⚠️ Potential issue | 🟠 MajorDetect duplicate derived names before writing to
result.These loaders still do last-write-wins assignment on
result[config.name].agent/foo.mdvsagents/foo.mdandmode/foo.mdvsmodes/foo.mdcan collapse to the same derived name and silently shadow each other based on scan order. Reuse the collision reporting/source-tracking flow frompackages/opencode/src/config/command.tshere as well.Also applies to: 189-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/config/agent.ts` around lines 151 - 161, The loader currently derives a name via configEntryNameFromPath and unconditionally assigns parsed.data into result[config.name], causing silent shadowing for paths like agent/foo.md vs agents/foo.md; before assigning, check for an existing entry in result with the same config.name and, if present, invoke the same collision-reporting/source-tracking flow used in the command loader (reuse the helper logic from command.ts) to record both sources and surface an error or warning; apply the same change to the other block around lines 189-199 so all derived-name collisions are detected and reported instead of doing last-write-wins.packages/opencode/src/config/permission.ts (1)
26-35:⚠️ Potential issue | 🟠 MajorKeep
__originalKeysout of the user-visible key space.This preprocess step reserves a real config key inside a schema that otherwise accepts arbitrary string permission names. A tool/permission literally named
__originalKeyswill now be rejected or mangled by the order-preservation workaround.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/config/permission.ts` around lines 26 - 35, The current permissionPreprocess injects a visible "__originalKeys" property into the value object which collides with user permission names; instead, set the original-keys metadata as a non-enumerable property on the same object (use Object.defineProperty(val, "__originalKeys", { value: globalThis.Object.keys(val), enumerable: false, configurable: true })) so Object.keys/Schema.StructWithRest won't see it, and leave the rest of the object intact; update permissionPreprocess (and any code expecting a copied object) to rely on the non-enumerable "__originalKeys" property while preserving ObjectShape usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/config/variable.ts`:
- Around line 35-45: The env-substitution pass in
packages/opencode/src/config/variable.ts unconditionally expands {env:...}
tokens inside input.text (inside the input.text.replace callback) even when
those tokens are commented; update the replacement callback to first detect
whether the matched token is inside a comment using the same comment-guard logic
used for the file-token pass (the check that skips tokens when the line starts
with '//' or when a token is in a commented region) and, if it is commented,
return the original token (or empty per existing file-token behavior) instead of
expanding or throwing; apply the same guarded behavior to the other
env-substitution block referenced around lines 60-66 so both spots use a shared
helper (e.g., isTokenCommented or reuse the file-token guard) to decide whether
to skip expansion and erroring for commented tokens.
---
Duplicate comments:
In `@packages/opencode/src/config/agent.ts`:
- Around line 151-161: The loader currently derives a name via
configEntryNameFromPath and unconditionally assigns parsed.data into
result[config.name], causing silent shadowing for paths like agent/foo.md vs
agents/foo.md; before assigning, check for an existing entry in result with the
same config.name and, if present, invoke the same
collision-reporting/source-tracking flow used in the command loader (reuse the
helper logic from command.ts) to record both sources and surface an error or
warning; apply the same change to the other block around lines 189-199 so all
derived-name collisions are detected and reported instead of doing
last-write-wins.
In `@packages/opencode/src/config/keybinds.ts`:
- Around line 125-127: The exported Keybinds schema currently casts KeybindsZod
to a ZodObject without enforcing strictness, so unknown keys are stripped
instead of rejected; change the validation to ensure KeybindsZod is a
z.ZodObject and call .strict() (e.g., derive a strictZod = KeybindsZod.strict()
or verify KeybindsZod.strict() before casting) and export that strict object as
Keybinds (reference symbols: KeybindsZod, Keybinds, KeybindsSchema, z.ZodObject,
.strict()) so typos in the fixed key surface are rejected.
In `@packages/opencode/src/config/permission.ts`:
- Around line 26-35: The current permissionPreprocess injects a visible
"__originalKeys" property into the value object which collides with user
permission names; instead, set the original-keys metadata as a non-enumerable
property on the same object (use Object.defineProperty(val, "__originalKeys", {
value: globalThis.Object.keys(val), enumerable: false, configurable: true })) so
Object.keys/Schema.StructWithRest won't see it, and leave the rest of the object
intact; update permissionPreprocess (and any code expecting a copied object) to
rely on the non-enumerable "__originalKeys" property while preserving
ObjectShape usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 09382baf-fc52-459c-8394-13244af96a54
⛔ Files ignored due to path filters (1)
packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (13)
packages/opencode/src/config/agent.tspackages/opencode/src/config/command.tspackages/opencode/src/config/entry-name.tspackages/opencode/src/config/keybinds.tspackages/opencode/src/config/managed.tspackages/opencode/src/config/permission.tspackages/opencode/src/config/server.tspackages/opencode/src/config/variable.tspackages/opencode/src/util/effect-zod.tspackages/opencode/test/config/config.test.tspackages/opencode/test/server/middleware.test.tspackages/opencode/test/util/effect-zod.test.tspackages/sdk/openapi.json
Summary
packages/opencodeon product version0.2.4.1.4.11to1.14.19.pawwork.jsonaliases, config dependency bootstrap, console org state, config error serialization, and local tool dependency loading.Why
This is the Base PR for the OpenCode sync cycle. It brings over the schema and generated surfaces that later slices can build on without mixing provider, TUI, desktop, or runtime feature decisions into this PR.
Compatibility Notes
@/confignow exposes config modules through namespace barrels such asConfig,ConfigAgent,ConfigPermission, andConfigError. Direct imports from the underlying files remain the safest internal path when a caller needs a specific schema or error class.parseManagedPlist()now returns sanitized JSON text for the caller to parse through the normal config parser. MalformedplutilJSON is propagated instead of becoming an empty config.ConfigPermission.Action; config errors now live underConfigError.JsonErrorandConfigError.InvalidError.integer,exclusiveMinimum: 0.Related Issue
Part of #92.
How To Verify
bun turbo typecheck bun --cwd packages/opencode test test/config/config.test.ts test/tool/registry.test.ts test/util/effect-zod.test.ts test/server/middleware.test.ts bun turbo test:ciLatest local results for the review-fix batch:
bun turbo typecheck: 8 successful tasksbun --cwd packages/opencode test test/config/config.test.ts test/server/middleware.test.ts test/util/effect-zod.test.ts: 170 pass, 0 failbun packages/sdk/js/script/build.ts: completed and regenerated SDK v2 typesgit diff --check: cleanEarlier full local result for this PR:
bun turbo test:ci: 2098 pass, 11 skip, 1 todo, 0 failScreenshots or Recordings
No visible UI changes.
Checklist
devbranchSummary by CodeRabbit