Match Fish completions against the full command path - #6959
Conversation
fubhy
added
the
audit
🦋 Changeset detectedLatest commit: 4825ff4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
This PR currently ships only the failing regression test. Before merging, the implementation fix and a way to run the Fish test in CI must be included.
Reviewed changes
The PR adds a single Fish integration test to packages/effect/test/unstable/cli/completions/completions.test.ts that reproduces a bug where nested completions are gated only by the immediate parent subcommand name, so a child subcommand reused under different parents leaks completions from the wrong branch.
- Added a test that shells out to
fishto asserttool alpha common --only offers--alpha-only. - Added an
execFileSyncimport fromnode:child_process.
⚠️ Implementation fix is required before merge
The PR title and description say the implementation fix should land in this branch. In packages/effect/src/unstable/cli/internal/completions/fish.ts, subcommandCondition currently only checks parentPath[parentPath.length - 1]. For branches that share a child subcommand name, completion conditions need to match the full command path (all ancestors) rather than only the direct parent. Without that fix, this PR leaves CI intentionally red.
🚨 The test depends on the fish binary, which is not installed in CI
The new test calls execFileSync("fish", ...). On this runner — and in the current GHA setup, where .github/actions/setup/action.yaml only installs Node/pnpm/deno/bun — the binary is absent, so the test fails with spawnSync fish ENOENT before it can exercise the intended bug.
Suggested options:
- Convert this to a generated-script assertion (for example, assert that the generated
-ncondition for the duplicatedcommonpath includes all ancestors). This keeps the regression deterministic and avoids a new runtime dependency. - If runtime Fish verification is intentional, install
fishin CI and/or guard the test so it is skipped when the binary is unavailable.
ℹ️ Nitpicks
- Description/file mismatch: the PR body references
packages/effect/test/unstable/cli/FishCommandPath.audit.test.ts, but the diff editspackages/effect/test/unstable/cli/completions/completions.test.ts. - Formatting:
pnpm dprint checkflags the new import order and the long inline alpha subcommand object; runpnpm dprint fmt.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior review, the PR added the missing implementation fix and reworked the regression test to avoid a CI runtime dependency.
- Fixed
subcommandConditioninfish.tsto match the full parent path (parentPath.map(...).join("; and ")) instead of only the immediate parent, so duplicate nested subcommand names under different ancestors no longer share completion conditions. - Replaced the runtime Fish test with generated-script assertions in
completions.test.ts, verifying that flags underalpha commonandbeta commonare gated by__fish_seen_subcommand_from alpha; and __fish_seen_subcommand_from commonand__fish_seen_subcommand_from beta; and __fish_seen_subcommand_from commonrespectively. - Added
.changeset/fix-fish-command-path.mdas a patch changeset foreffect. - Removed the
execFileSyncimport and the dependency on thefishbinary;pnpm dprint checknow passes.
Both prior Pullfrog review threads are resolved by 4825ff4.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
2 participants
Summary
Match nested Fish completion rules against every command in the active parent path. This prevents commands that reuse a nested subcommand name under different parents from activating completions from unrelated branches.
The regression test remains in the main completion test file and asserts the generated Fish predicates directly, so it does not require the
fishbinary in CI.Closes EFF-405
Validation
pnpm test --run packages/effect/test/unstable/cli/completions/completions.test.ts pnpm lint-fix pnpm checkAudit provenance
unstable-ai-cli-fish-nested-command-pathc9b56ab507f224426ee8388dc450da447ec4715f