Skip to content

test(completion): isolate ZshInstaller tests from a real Oh My Zsh install - #1400

Merged
clay-good merged 2 commits into
mainfrom
fix/zsh-installer-test-isolation
Jul 22, 2026
Merged

test(completion): isolate ZshInstaller tests from a real Oh My Zsh install#1400
clay-good merged 2 commits into
mainfrom
fix/zsh-installer-test-isolation

Conversation

@clay-good

@clay-good clay-good commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Fixes #1321.

Status: LGTM — test-only change, no runtime code touched, full suite green.

What was wrong: test/core/completions/installers/zsh-installer.test.ts failed 17 of 50 tests on any machine with a real Oh My Zsh install. The tests inject an isolated temp home directory, but isOhMyZshInstalled() checks process.env.ZSH before falling back to that directory (zsh-installer.ts:31-46). Oh My Zsh exports $ZSH, the test runner inherits it, and every standard-Zsh test silently ran down the Oh My Zsh code path. CI containers don't have Oh My Zsh, so CI stayed green while local contributors saw a permanently red suite that could mask real regressions in this file.

How it was fixed:

  • Clear $ZSH in beforeEach and restore it in afterEach — the same save/clear/restore idiom this file already uses for OPENSPEC_NO_AUTO_CONFIG and shell-detection.test.ts uses for SHELL/COMSPEC.
  • Add one explicit test that sets $ZSH and asserts detection succeeds with no .oh-my-zsh directory present. Clearing the variable would otherwise leave that source branch with no coverage anywhere (before this PR it was only ever exercised accidentally, on the machines where the suite failed).

21 lines total, all inside the test file.

Replication / proof: On macOS with Oh My Zsh ($ZSH set):

# before (main)
npx vitest run test/core/completions/installers/zsh-installer.test.ts
#  Tests  17 failed | 33 passed (50)

# after (this branch)
npx vitest run test/core/completions/installers/zsh-installer.test.ts
#  Tests  51 passed (51)

npx vitest run
#  Test Files  105 passed (105)
#       Tests  2045 passed (2045)

Notes: The fix approach was proposed by @stanleykao72 in #1321 (credited as co-author). No behavior change for users; nothing in src/ or the published package is touched.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Improved Zsh installer test isolation by saving the prior ZSH environment value, clearing it for each test, and restoring (or removing) it afterward.
    • Added coverage to confirm isOhMyZshInstalled() returns true when the ZSH environment variable is set, independent of an .oh-my-zsh directory.

…stall

process.env.ZSH (exported by Oh My Zsh) short-circuits
isOhMyZshInstalled() before the fallback check against the injected
test home directory, so 17 of the 50 tests failed on any machine with
Oh My Zsh installed. Clear $ZSH in beforeEach and restore it in
afterEach, matching the save/restore idiom already used for
OPENSPEC_NO_AUTO_CONFIG in this file and for SHELL/COMSPEC in
shell-detection.test.ts.

Fixes #1321

Co-Authored-By: Stanley Kao <stanleykao72@gmail.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@clay-good
clay-good requested a review from TabishB as a code owner July 20, 2026 21:13
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The Zsh installer tests now clear and restore process.env.ZSH around each test and verify that Oh My Zsh detection succeeds when the environment variable is set.

Changes

Zsh installer test isolation

Layer / File(s) Summary
Environment isolation and detection coverage
test/core/completions/installers/zsh-installer.test.ts
Test setup saves and clears process.env.ZSH, restores or removes it after each test, and verifies environment-based Oh My Zsh detection.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related PRs

Suggested reviewers: tabishb

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR matches #1321 by isolating the test suite from host Zsh state and adding coverage for $ZSH-based detection.
Out of Scope Changes check ✅ Passed The changes are scoped to the test file and do not introduce runtime or published-package behavior changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the test-only fix to isolate ZshInstaller tests from host Oh My Zsh installs.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/zsh-installer-test-isolation

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Clearing $ZSH in setup left isOhMyZshInstalled()'s env-var branch with
no coverage anywhere (before, it was only exercised accidentally on
machines with Oh My Zsh). Assert detection succeeds from $ZSH alone,
with no .oh-my-zsh directory present.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the host-environment failure: the base has 17 of 50 focused tests fail with ZSH set, while this exact head passes all 51 and restores the original environment after each test. Clean test-only fix.

@clay-good
clay-good added this pull request to the merge queue Jul 22, 2026
Merged via the queue into main with commit 34d2d67 Jul 22, 2026
14 checks passed
@clay-good
clay-good deleted the fix/zsh-installer-test-isolation branch July 22, 2026 13:25
clay-good added a commit that referenced this pull request Jul 28, 2026
The fpath verification advice still grepped the literal
custom/completions, which a relocated $ZSH_CUSTOM need never contain —
grep the actual directory instead. The installer tests cleared only
$ZSH, so on a machine exporting $ZSH_CUSTOM they would have written
into (and deleted from) the developer's real OMZ custom dir — the same
leakage class #1400 fixed for $ZSH. Clear/restore both, and pin the
custom-location paths with two new tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
birhantprkc pushed a commit to birhantprkc/OpenSpec that referenced this pull request Jul 29, 2026
…-audit follow-ups (Fission-AI#1475)

* fix(archive): make the scenario-drift check fence-aware

parseScenarioBlocks matched #### Scenario: headers on raw lines while the
validator's countScenarios masks fenced code blocks (Fission-AI#1151). The drift
check (Fission-AI#1391) inherited the raw scan, so a fenced scenario example in the
current spec aborted an archive that validate had passed, and a fenced
name in the MODIFIED block counted as keeping a scenario the block had
actually dropped. Build the shared code-fence mask and skip masked lines
in both the header scan and the block-end scan.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(update): tear down the redirected request when the budget expires

The overall request budget was armed inside the first send() and its
callback closed over that hop's request. After a redirect the timer
destroyed the already-dead first request, so a redirect target that
trickled bytes kept resetting its idle timeout and held the socket open
until the body-size cap. Track the in-flight request and have the budget
timer destroy whichever one is open.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore(release): add changesets for user-facing changes missing from the 1.7.0 notes

18 feat/fix commits merged since v1.6.0 without a changeset, so the
pending Version Packages PR would have released them silently: five tool
integrations (ZCode, Hermes, CodeArts, Kimi Code rename, Codex
skills-only), skills.sh distribution, symlinked schema dirs, nested spec
discovery, drift multiplicity, checkbox markers, Windows welcome input,
npx avoidance, doctor store drift, local dates, missing-core-workflows
warning, store-aware main specs, open-questions guidance, and spec
content guidance. Plus changesets for this branch's two fixes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(adapters): escape TOML-active characters in Gemini command files

The gemini adapter interpolated the description into a TOML basic string
and the body into a multiline basic string with no escaping. Every
current template value happens to be safe; the first description with a
double quote or backslash would silently produce invalid TOML for all
Gemini command files. Escape both contexts (Fission-AI#1447 fixed the same class
for the YAML adapters but scoped itself to YAML).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(update): harden install detection and redirect handling

Three follow-ups from the release audit:
- A path segment literally named volta (a user or project directory)
  classified the install as volta-managed and swallowed the upgrade
  offer. The undotted spelling now requires volta's own tools/image
  layout, matching how pnpm and yarn already demand corroboration.
- The Windows npm-ownership fallback checked that the npm prefix exists,
  which is true of any X\node_modules\pkg tree, hand-copied ones
  included. Corroborate with the openspec.cmd shim npm actually writes.
- A https registry redirecting to plain http was followed; a MITM on
  that reply controls the newer-version answer. Refuse the downgrade.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore(cli): export zcodeAdapter from the barrel and sync a completion description

zcode was registered but missing from the adapters barrel (its test
imported the module directly), and the completion registry still carried
the pre-Fission-AI#1062 description for the instructions command.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(parser): strip a UTF-8 BOM before parsing specs and deltas

A BOM-prefixed delta spec (Windows editors, PowerShell Out-File) failed
validate and archive with 'No delta sections found' because the first
line never matched '## ADDED Requirements'. Strip the BOM in both
normalizers, the same way tool detection already does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(cli): reject over-long change names with a validation message

A 300-character change name surfaced two raw ENAMETOOLONG errno dumps
from stat and mkdir. Bound the name at 200 characters in
validateChangeName so the failure is a normal validation error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(archive): finish the early-sync no-op rules for MODIFIED and RENAMED

Two asymmetries left over from the Fission-AI#1376/Fission-AI#1386/Fission-AI#1437 no-op work:

- MODIFIED counted every delta as applied even when the block was
  byte-equal to the main spec, so a fully early-synced change rewrote
  the file (normalization churn), printed '~ N modified', and reported
  specsUpdated: true where its ADDED/REMOVED/RENAMED twins print 'Specs
  already in sync; no files changed.' Count only real replacements.
- RENAMED's already-synced skip (source gone, target present) had no
  near-miss guard: a case/whitespace variant of the source still in the
  spec means a typo'd header, and REMOVED already hard-aborts on that
  signal. Apply the same guard, excluding the target itself so a
  case-only rename still no-ops.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(validate): stop reporting an unreadable specs dir as 'no deltas'

The delta-validation loop swallowed every error as 'if no specs dir,
treat as no deltas', so an EACCES capability folder produced the
misleading 'Change must have at least one delta' while archive let the
same error propagate. Tolerate only ENOENT and ENOTDIR (a stray specs
file); anything else stays loud, matching discoverSpecFiles' documented
fail-loud contract.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(update): say when commands-only delivery leaves a tool with nothing

Under delivery: commands, update removed the skills of adapterless
skills-only tools (Hermes, Kimi Code, Vibe, CodeArts, ForgeCode) without
a word — leaving zero OpenSpec artifacts while the tool's detection dir
kept re-suggesting an init that would also generate nothing. Print the
same per-tool configuration correction init already prints, pointing at
'openspec config set delivery both'.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(completion): honor $ZSH and $ZSH_CUSTOM for Oh My Zsh installs

The installer used a set $ZSH only as an is-installed signal and then
wrote to ~/.oh-my-zsh regardless, so a custom OMZ location got a
freshly created ~/.oh-my-zsh tree that no shell ever loads — and
isInstalled/uninstall looked in the same wrong place. Route every path
through the $ZSH/$ZSH_CUSTOM-aware helpers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(init): make the static welcome screen wait for the Enter it asks for

The static branch printed 'Press Enter to select tools...' and returned
immediately, so the Enter landed in the tool picker and submitted the
pre-selected set sight-unseen. Fission-AI#1462 routed reduced-motion,
OPENSPEC_NO_ANIMATION, --no-animation, NO_COLOR, and narrow-terminal
users onto this path. Wait in a TTY; drop the prompt line when there is
no TTY to wait on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(feedback): keep the manual fallback on every gh failure

Only missing-gh and unauthenticated flows showed the formatted feedback
and pre-filled submission URL; issues-disabled, network, or rate-limit
failures printed gh's stderr and discarded the path to submit what the
user had already typed. Route those through the same manual fallback,
preserving gh's exit code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test(update): model the npm shim in the Windows prefix fixture

The ownership corroboration now checks for the openspec.cmd shim npm
writes beside node_modules; the Homebrew-prefix fixture built the layout
without it, so the test failed on windows-pwsh. Write the shim in the
fixture and pin the inverse: the same shape with nothing npm wrote (a
hand-copied portable tree) is not an npm install.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(update): require volta's full tools/image layout for the undotted spelling

The corroboration used has('tools', 'image'), which is some() — volta
AND (tools OR image) — so /srv/volta/tools/apps/... still classified as
a Volta install and swallowed the upgrade offer. Require both segments,
matching the real %LOCALAPPDATA%\Volta\tools\image layout.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(adapters): escape control characters in Gemini multiline prompts

escapeTomlMultilineBasicString handled backslashes and quote-triples but
not the C0 controls that are as invalid in a multiline basic string as
in a single-line one. Reuse TOML_CONTROL_CHARS, applied last so the
escapes it introduces are not re-doubled.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(completion): finish the $ZSH_CUSTOM support and isolate it in tests

The fpath verification advice still grepped the literal
custom/completions, which a relocated $ZSH_CUSTOM need never contain —
grep the actual directory instead. The installer tests cleared only
$ZSH, so on a machine exporting $ZSH_CUSTOM they would have written
into (and deleted from) the developer's real OMZ custom dir — the same
leakage class Fission-AI#1400 fixed for $ZSH. Clear/restore both, and pin the
custom-location paths with two new tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore(release): correct the hermes and zcode changeset wording

Hermes is skills-only (no command adapter), and zcode's namespaced
commands register /opsx:<id>, not /opsx-* — the release notes must not
reintroduce the invocation-spelling confusion Fission-AI#1471 removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test(feedback): pin the manual fallback on a non-label gh failure

The new reportGhFailure output (formatted feedback + pre-filled URL) had
no coverage; the network-failure test now asserts it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(completion): match fpath entries as literal strings in the OMZ guidance

The verification advice interpolated the completions dir into
grep "<dir>" where regex metacharacters make the check unreliable and
quotes could break the displayed command. Print one fpath entry per
line and match with grep -F on a shell-quoted literal.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(adapters): never emit a bare carriage return in Gemini TOML prompts

A lone CR is illegal in a multiline basic string — Python 3.13 tomllib
rejects the file — and the control-char pass deliberately skipped it on
the assumption it only appears as CRLF. Normalize CRLF to LF and escape
any remaining CR as \r. The escaping guarantee is now parser-backed:
smol-toml (new devDependency) round-trips every hostile body in the
regression matrix (lone CR, CRLF, CR before a quote run, NUL/VT/FF,
trailing backslash, four- and five-quote runs), and the same outputs
were verified against Python tomllib.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* build(nix): update the pnpm deps hash for the smol-toml devDependency

The lockfile changed, so the fixed-output derivation hash moved; value
taken from the CI mismatch report.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

ZshInstaller tests fail on machines with a real Oh My Zsh install (17/50 failures)

2 participants