Fix CLI config fallbacks for Optique 1.0#728
Conversation
Use optional parsers for tunnel service configuration so missing configuration values do not make commands fail during Optique's two-pass parsing. Move other config-backed defaults into bindConfig() directly to avoid relying on wrapper defaults around configuration bindings. Add regression coverage for tunnel, inbox, and relay command parsing when no tunnel service is configured. Assisted-by: Codex:gpt-5.5
Update the Optique packages to 1.0.2 across the Deno and pnpm workspace manifests and refresh both lockfiles. Add an explicit @standard-schema/spec dependency for the CLI package to satisfy @optique/config's peer dependency under Node.js and Bun installs. Assisted-by: Codex:gpt-5.5
dahlia
added
type/bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdates dependency pins (JSR and npm) including adding Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
issues-auto-labeler
Bot
added
breaking change
dahlia
removed
breaking change
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the @optique suite of packages (config, core, run) to version 1.0.2 and adds @standard-schema/spec to the project dependencies. It refactors the CLI's option handling for recursion depth and tunneling to improve how default values and optional configurations are bound. Furthermore, it introduces new unit tests to ensure the CLI correctly handles scenarios where tunnel services are omitted. I have no feedback to provide.
There was a problem hiding this comment.
Pull request overview
Fixes Fedify CLI option/config fallback behavior after upgrading to Optique 1.0, preventing “Missing required configuration value” errors when positional args are provided and config-backed options are intentionally omitted.
Changes:
- Adjusted CLI option compositions to align with Optique 1.0’s intended
bindConfig()+optional()semantics (notably tunnel service and tunnel enable/disable). - Updated lookup recursion depth config binding so defaults are owned by the config-binding layer.
- Added regression tests for
tunnel,inbox, andrelayrunners when tunnel service config is omitted, and updated Optique-related dependencies/locks (including@standard-schema/spec).
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Bumps Optique catalog versions and adds @standard-schema/spec to satisfy peer deps. |
| pnpm-lock.yaml | Refreshes lockfile for Optique 1.0.2 and @standard-schema/spec resolution. |
| packages/cli/src/tunnel.test.ts | Adds regression tests ensuring runners parse correctly with omitted tunnel service config. |
| packages/cli/src/options.ts | Fixes tunnel-related option/config fallback modeling using optional(bindConfig(...)) and config-owned defaults. |
| packages/cli/src/lookup.ts | Updates recursion depth config binding/default handling for Optique 1.0 behavior. |
| packages/cli/package.json | Adds explicit @standard-schema/spec dependency metadata for Node/Bun installs. |
| deno.lock | Updates locked JSR Optique versions to 1.0.2 and refreshes integrity entries. |
| deno.json | Updates Optique import mappings to ^1.0.2. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
packages/cli/src/options.ts:90
- In
createTunnelOption,keyalways returns a boolean (it treats an absent config value asfalseand then negates), anddefault: trueis also specified. This makes thedefaultoption effectively unreachable and duplicates the source of truth for the default behavior. Consider returningundefinedfromkeywhen the config field is absent and keepingdefault: trueas the single fallback mechanism.
{
context: configContext,
key: (config: Config) => !(config[section]?.noTunnel ?? false),
default: true,
},
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Keep config-bound fallback values in one place by returning raw config values from key callbacks and relying on bindConfig's default option for missing values. This avoids drift between duplicated fallback expressions for lookup recursion depth and tunnel defaults. Addresses fedify-dev#728 (comment) Addresses fedify-dev#728 (review) Assisted-by: Codex:gpt-5.5
|
Also addressed the |
|
@codex review |
|
/gemini review |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Code Review
This pull request updates the @optique dependencies to version 1.0.2 and adds @standard-schema/spec to the project. The CLI configuration logic was refactored to use the default property within bindConfig and the optional utility, replacing several instances of withDefault. Additionally, the tunnel option mapping was updated to explicitly handle nullish values, ensuring the configuration fallback mechanism functions correctly. New test cases were added to verify that the tunnel, inbox, and relay runners handle omitted tunnel services as expected. I have no feedback to provide.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deno.json (1)
38-79: 🧹 Nitpick | 🔵 TrivialConsider adding
@standard-schema/spectodeno.jsonimports for consistency with JSR packages.
@standard-schema/specwas added topackages/cli/package.jsonas a dependency for@optique/config's peer requirements, but no corresponding entry exists indeno.json. The codebase has no direct imports of@standard-schema/spec, so Deno resolves it transitively through@optique/config's JSR metadata. However, for consistency with the convention of explicitly declaring dependencies in both configuration files, consider adding:"@standard-schema/spec": "jsr:`@standard-schema/spec`@^1.1.0",The JSR package exists and version 1.1.0 is available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deno.json` around lines 38 - 79, The deno.json imports are missing an explicit JSR entry for the peer dependency "@standard-schema/spec" (which causes implicit/transitive resolution via `@optique/config`); add an imports entry for "@standard-schema/spec" in the existing "imports" object using the JSR spec syntax, e.g. add "@standard-schema/spec": "jsr:`@standard-schema/spec`@^1.1.0" to the "imports" map so Deno resolves the package explicitly and matches the package.json dependency.
🤖 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/cli/src/options.ts`:
- Around line 88-91: Hoist the repeated indexed access by assigning
config[section] to a local (e.g., const sect = config[section]) and then use
that local in the arrow for key to perform the null check and negation (replace
both config[section] uses with sect and use optional chaining on the check as
needed), so the expression becomes: const sect = config[section]; sect?.noTunnel
== null ? undefined : !sect.noTunnel — this ensures safe narrowing and removes
the redundant lookup for functions referencing section and config.
---
Outside diff comments:
In `@deno.json`:
- Around line 38-79: The deno.json imports are missing an explicit JSR entry for
the peer dependency "@standard-schema/spec" (which causes implicit/transitive
resolution via `@optique/config`); add an imports entry for
"@standard-schema/spec" in the existing "imports" object using the JSR spec
syntax, e.g. add "@standard-schema/spec": "jsr:`@standard-schema/spec`@^1.1.0" to
the "imports" map so Deno resolves the package explicitly and matches the
package.json dependency.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7207b2a1-e132-4319-a3d8-d8f42d4dba22
⛔ Files ignored due to path filters (2)
deno.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
deno.jsonpackages/cli/package.jsonpackages/cli/src/lookup.tspackages/cli/src/options.tspackages/cli/src/tunnel.test.tspnpm-workspace.yaml
Hoist the section config lookup before checking noTunnel so the nullish check and boolean negation share the same narrowed value. This removes repeated indexed access in the config fallback callback. Addresses fedify-dev#728 (comment) Assisted-by: Codex:gpt-5.5
Add @standard-schema/spec to the Deno import map as well as the npm package metadata so the peer dependency introduced for @optique/config is declared consistently across runtimes. Addresses fedify-dev#728 (review) Assisted-by: Codex:gpt-5.5
|
Addressed the |
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the @optique suite of dependencies to version 1.0.2 and introduces @standard-schema/spec version 1.1.0 across the workspace. In the CLI package, the configuration binding logic for recurse-depth and tunnel options was refactored to use the built-in default property within bindConfig and the optional utility, replacing the previous withDefault wrappers. Additionally, new test cases were added to packages/cli/src/tunnel.test.ts to verify that the CLI runners correctly handle omitted tunnel service configurations. I have no feedback to provide.
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
This fixes Fedify CLI option/config fallback handling after the Optique 1.0 upgrade. The
tunnel,inbox, andrelaycommands could fail withError: Missing required configuration value.even when required positional arguments were provided, because optional config-bound options were modeled withwithDefault(..., undefined)or nestedbindConfig(withDefault(...))patterns.Changes
optional(bindConfig(...)).bindConfig()owns the default value directly.tunnel,inbox, andrelayrunners with omitted tunnel service config.@standard-schema/spec, which satisfies@optique/config's peer dependency in Node.js and Bun installs.Rationale
Optique's config file support documentation describes
bindConfig()as the layer that handles fallback from CLI arguments to config file values to adefaultvalue, and then to an error if none of those sources produce a value. Optique's modifying combinators documentation describesoptional()as the way to represent a value that may be genuinely absent.The previous Fedify CLI patterns mixed those concepts:
withDefault(..., undefined)was used to express optional absence, andwithDefault()was also nested around or inside config-bound parsers. With Optique 1.0's config parsing model, those compositions can leavebindConfig()with no config value and no real default, which surfaces asError: Missing required configuration value.The fix follows the documented shape more directly: use
optional(bindConfig(...))when absence is meaningful, and put concrete defaults such astrueor20in thebindConfig()options when config fallback should have a real default value.Verification
deno test --allow-allfor the tunnel and lookup CLI tests.deno checkagainst the CLI source files.deno lintanddeno fmt --checkon the touched CLI source and test files.pnpm install,pnpm install --lockfile-only --frozen-lockfile --ignore-scripts, and the repository pre-commitmise run check.