Skip to content

fix(claude-ops): spell both sides of the $HOME exclusion the same way - #1983

Merged
kyle-sexton merged 2 commits into
mainfrom
fix/1972-fleet-state-home-exclusion
Aug 8, 2026
Merged

fix(claude-ops): spell both sides of the $HOME exclusion the same way#1983
kyle-sexton merged 2 commits into
mainfrom
fix/1972-fleet-state-home-exclusion

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Summary

fleet-state.sh excludes $HOME from project context, because that directory is
USER scope and reading $HOME/.claude/settings.json as the project map
duplicates the user map. The exclusion did not hold.

It compared the native path pwd -W reports against $HOME exactly as the
environment carried it — one directory, two spellings. An MSYS mount alias has no
drive letter for the normalizer to reconcile, so $HOME=/tmp/x never equalled the
C:/Users/…/Temp/x reported for the same place. PROJECT_ROOT was then set to
$HOME and the duplication happened silently.

Both sides now go through pwd -W before normalization. Normalizing harder could
not have fixed it: the two inputs disagreed before the normalizer saw them.

Changes

  • fleet-state.sh: derive the $HOME side of the comparison through pwd -W
    (falling back to pwd, then to $HOME), with the reasoning recorded at the site.
  • fleet-state.test.sh: record that the existing case is load-bearing only on
    Git Bash — on a POSIX runner both spellings already agree, so it passes there
    whether or not the exclusion works.
  • claude-ops 0.27.1 → 0.27.2 with a matching CHANGELOG entry.

Closes #1982

Why CI never caught it

plugin-gate runs on ubuntu-24.04, where $HOME and pwd agree, so the case
passes regardless. It was found by a local full-suite sweep and split out of
#1972, which flagged it as a different root cause from that ticket's two ruff
failures.

Related

Test plan

  • Standalone reproduction of the test's exact fixture: currentProject was
    false, is now null
  • fleet-state.test.sh — 34 cases, 0 failed (was 34 / 1 failed)
  • shellcheck -x on the changed script — clean
  • scripts/check-shell-portability.sh — no unexcused GNU-only constructs
  • scripts/check-changelog-parity.sh --check-bump — PASS
  • scripts/validate-plugins.sh — PASS
  • markdownlint-cli2 on the CHANGELOG — 0 issues
  • CI green on the pushed head

The exclusion keeping $HOME out of project context compared `pwd -W`'s native
path against $HOME exactly as the environment carried it. Those are one
directory in two spellings, and an MSYS mount alias has no drive letter for the
normalizer to reconcile, so `/tmp/x` never matched the `C:/...` reported for it.
The exclusion silently failed, $HOME became PROJECT_ROOT, and
$HOME/.claude/settings.json was read as the project map — duplicating the user
map, which is the one outcome the block exists to prevent.

Both sides now go through `pwd -W` before normalization. Normalizing harder was
not an option: the two inputs disagreed before the normalizer saw them.

The existing test case already covered this and was already red; it is green
now. Its comment records that the case is load-bearing only on Git Bash — on a
POSIX runner both spellings agree, so it passes there whether or not the
exclusion works, which is why CI never caught this.

Closes #1982

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@cursor

cursor Bot commented Aug 8, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 40s —— View job


Claude finished @kyle-sexton's task

Todo list

  • Gather context (diff, CLAUDE.md, changed files)
  • Understand the request (code review of this PR)
  • Review fleet-state.sh change for correctness
  • Review fleet-state.test.sh change for coverage
  • Review CHANGELOG/plugin.json version bump
  • Post final review

Review

The fix is correct and the reasoning in the code/CHANGELOG holds up. Traced through hook::physical_path/hook::normalize_path in hook-utils.sh: realpath doesn't translate an MSYS mount alias (/tmp/...) to the native drive-letter form, so pre-fix, $HOME stayed POSIX-spelled while cwd_native (from pwd -W) was native-spelled — same directory, two strings, exclusion silently failed. Routing $HOME through the identical cd "$HOME" && pwd -W (falling back to pwd, then $HOME) at fleet-state.sh#L231-L238 closes that gap and is symmetric with the existing cwd_native derivation just above it. The added -d "$HOME" guard is a sensible defensive addition over the prior -n check.

Test coverage gap (worth flagging, not blocking): confirmed via .github/workflows/ci.yml that every job runs on ubuntu-24.04 — there's no Windows/Git-Bash runner in this repo's CI. That means the $HOME exclusion test case in fleet-state.test.sh — even with the new comment correctly documenting that it's load-bearing only on Git Bash — can never actually fail in CI, with or without the fix. The PR is transparent about this in "Why CI never caught it," which I appreciate, but it means a future regression on this exact path has no automated safety net. Worth a follow-up (out of scope for this PR): a POSIX-runnable case that shadows pwd/cd via a function or PATH shim to synthesize divergent spellings, so the regression is catchable on ubuntu-24.04 too.

Minor nit: plugin.json — beyond the version bump, all 4 pre-existing em dashes in the file (lines 5, 40, 52, 106) changed spelling from literal to escaped , apparently as a side effect of whatever tool rewrote the file for the version bump. Semantically identical, but it's unrelated diff noise in an otherwise tightly-scoped fix — future diffs against this file will show the same file re-serialized again if a different tool touches it next.

Everything else checks out: CHANGELOG entry follows the existing (dateless) ## [x.y.z] / ### Fixed convention used by prior entries, version bump (0.27.10.27.2) is a correct patch bump for a bug fix, and the test-file comment accurately describes why the case only exercises the bug on Git Bash.

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 44s —— View job


Security review complete

  • Gather context (PR diff, changed files)
  • Review fleet-state.sh changes
  • Review fleet-state.test.sh changes
  • Review plugin.json / CHANGELOG.md changes
  • Post findings

Scope: the 4 files changed in this PR — plugins/claude-ops/skills/plugins/scripts/fleet-state.sh, fleet-state.test.sh, .claude-plugin/plugin.json, CHANGELOG.md.

No security issues found.

Notes from the review (informational, not findings):

  • The new $HOME resolution at fleet-state.sh#L232-236 runs cd "$HOME" 2>/dev/null && { pwd -W 2>/dev/null || pwd; } inside a $(...) subshell, so it doesn't perturb the script's own working directory even on failure — no TOCTOU or state-leak concern there.
  • $HOME is read from the process environment the script's own invoker controls, not from any untrusted external input (no PR/network data flows into this comparison), so there's no injection surface introduced by this change. All expansions ("$HOME", "$home_native", "$cwd_native") stay double-quoted throughout.
  • The fallback chain (pwd -Wpwd → literal "$HOME") degrades safely — an empty/failed cd just leaves home_norm="", which can only make the exclusion more conservative (any non-empty cwd_native differs from ""), never less.
  • plugin.json / CHANGELOG.md changes are prose/version-bump only (em dash → escaping, version 0.27.1 → 0.27.2); no schema or permission changes.
  • fleet-state.test.sh changes are comment-only.

No pull_request_target/workflow_run usage, no workflow permission changes, and no unpinned/loosened dependency pins in this diff — nothing in scope for the zizmor advisory lane either, since no .github/workflows files are touched.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a1414ac415

ℹ️ 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".

Comment thread plugins/claude-ops/skills/plugins/scripts/fleet-state.sh Outdated
Review found the inverse failure in the first fix: an exported `cd` function
that returns success WITHOUT changing directory makes `$HOME` resolve to the
cwd, so every corroborated non-git project compares equal to $HOME and loses
both its project settings and its currentProject marker. Reproduced.

`builtin cd` / `builtin pwd` on both sides, which is the discipline this script
already applies to its own directory resolution and to `source` — the header's
environment-trust boundary note covers what that does and does not defend.
`cwd_native` gets the same treatment for the same reason.

Adds the regression test, which fails without the builtin prefix. Unlike the
$HOME-exclusion case it sits beside, this one is load-bearing on every platform:
the failure is the shadow being honoured, not a path-spelling mismatch. Its
fixture derives projectPath through `pwd -W` like the non-git-project case
above it, since a record built from the MSYS path could never match what the
script resolves and the case would report `false` either way.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 24s —— View job


Security review complete

Todo list

  • Gather context (PR diff since previous review, changed files)
  • Review new commit (77b90a8: builtin cd/builtin pwd fix for shell-function shadowing)
  • Review remaining changed files for security issues
  • Post findings

Scope: all 4 files changed in this PR — fleet-state.sh, fleet-state.test.sh, plugin.json, CHANGELOG.md. This PR was already reviewed at a1414ac (no issues found); this pass focuses on what changed since, in commit 77b90a8.

No security issues found.

77b90a8 fixes a real trust-boundary gap, and fixes it completely. The prior commit's $HOME-vs-cwd_native comparison at fleet-state.sh#L232-238 used bare cd/pwd, correctly flagged by @chatgpt-codex-connector: an environment that exports a cd shell function (BASH_FUNC_cd%%) — e.g. inherited from a parent shell, a dev-container profile, or a compromised dotfile — could intercept that call. If the shadow returned success without changing directory, home_native would silently resolve to the current directory instead of $HOME, making every non-Git project compare equal to $HOME and lose its currentProject marker and project-scope settings. That's a real integrity failure of the state this read-only script exists to report accurately.

The fix now routes every cd/pwd/source call in the file through builtin — confirmed by grep, no unqualified call sites remain (SCRIPT_DIR/PLUGIN_ROOT_DEFAULT, HOOK_UTILS source, cwd_native, and now home_native at L237 are all builtin-prefixed). This extends the same shadow discipline the script's header already documents at L61-66 rather than introducing a new defensive pattern, and the header is explicit that this covers a single shadowed builtin/function, not a shadowed builtin keyword itself or BASH_ENV — an honestly-scoped boundary, not a silent gap.

The new regression test at fleet-state.test.sh#L448-486 exercises exactly this attack: it exports BASH_FUNC_cd%%=() { return 0; } into the subprocess running the script and asserts a real project's currentProject flag stays true rather than collapsing to the $HOME case. That's a correct, minimal reproduction of the vulnerability the fix closes, and — unlike the adjacent $HOME-spelling case — it's load-bearing on every CI platform (POSIX runners included), so it will actually catch a regression.

Other notes (informational, not findings):

  • The home_native=$(builtin cd "$HOME" ... ) resolution runs inside a $(...) subshell, so a failed/successful cd never perturbs the script's own working directory.
  • plugin.json/CHANGELOG.md changes in this diff slice are version bump (0.27.1 → 0.27.2) and changelog prose only — no schema, permission, or config-surface changes.
  • No .github/workflows files are touched, so nothing here is in the zizmor advisory lane's scope either.

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 50s —— View job


Code review (post-77b90a8)

Todo list

  • Gather context (diff, CLAUDE.md, changed files)
  • Review fleet-state.sh change for correctness
  • Review fleet-state.test.sh change for coverage
  • Review CHANGELOG/plugin.json version bump
  • Post final review

This lane covers correctness/architecture/tests/maintainability; security aspects of the builtin cd/pwd fix in 77b90a8 are covered by the separate security lane.

Correctness

Traced both fixes end-to-end against the diff at fleet-state.sh#L215-243:

  • $HOME native-spelling fixhome_native is derived the same way as the existing cwd_native two lines above it (pwd -Wpwd → literal fallback), then both go through the identical hook::normalize_path(hook::physical_path(...)) pipeline before comparison. Symmetric, and the $(...) subshell means the cd never perturbs the script's own $PWD even on failure.
  • builtin cd/pwd fix — grepped the whole file; every cd/pwd call site (SCRIPT_DIR, PLUGIN_ROOT_DEFAULT, cwd_native, home_native) is now builtin-prefixed, no stragglers. hook-utils.sh's hook::physical_path/hook::normalize_path (which this resolution calls into) use realpath/readlink, not cd/pwd, so they aren't a second instance of the same shadow gap.

Test coverage

Manually traced the new case at fleet-state.test.sh#L448-486 against both pre- and post-fix code paths:

  • Pre-fix (unqualified cd): the BASH_FUNC_cd%% shadow returns 0 without moving, so home_native's subshell inherits the invoking cwd (proj_dir, since the outer cd "$proj_dir" && env ... already put the test-runner process there) — home_native collapses to proj_dir, matching cwd_native, and currentProject goes to null. Confirmed this reproduces the exact failure the PR describes.
  • Post-fix (builtin cd): the shadow is bypassed, builtin cd "$HOME" actually moves into fake_home, home_native resolves to fake_home, comparison correctly diverges from proj_dir, currentProject stays true.

This case is POSIX-runnable and load-bearing on every platform (unlike the adjacent $HOME-spelling case, which the PR's own comment at L444-447 correctly documents as Git-Bash-only) — good regression coverage for the actual defect class, not just documentation of the gap.

Maintainability / CHANGELOG

  • Version bump (0.27.10.27.2) and dateless ## [x.y.z] / ### Fixed format both match the file's existing convention.
  • plugin.json's em-dash re-serialization () noise, already flagged in an earlier pass on this PR, remains present and still unrelated to the fix — cosmetic, not blocking.
  • Both fixes' reasoning is recorded at the call site (comments) and in the CHANGELOG, consistent with this script's existing documentation density.

Caveat

Sandboxed Bash in this session required interactive approval for script execution (bash fleet-state.test.sh, shellcheck, git fetch) that wasn't obtainable headlessly, so this review is a static trace rather than a live run — the PR's own test-plan checkboxes cover the live run. If a future pass here should execute the suite directly, --allowedTools would need Bash(bash plugins/claude-ops/skills/plugins/scripts/fleet-state.test.sh) and similar added.

No correctness issues found. The fix is symmetric, the shadow-hardening is complete across the file, and the new regression test is a real (not just documentary) safety net.

@kyle-sexton
kyle-sexton merged commit 45e8acf into main Aug 8, 2026
32 checks passed
@kyle-sexton
kyle-sexton deleted the fix/1972-fleet-state-home-exclusion branch August 8, 2026 04:17
kyle-sexton added a commit that referenced this pull request Aug 8, 2026
…-reduction

Resolves the version-line collisions this branch's 16-plugin lib bump has
with three PRs that landed on main first (#1974 guardrails, #1981
session-flow, #1983 claude-ops).

- guardrails: main released 0.19.1 (#1974's PowerShell sink fixes), so the
  shared-lib entry re-heads as 0.19.2.
- claude-ops: main released 0.27.2 (#1983's $HOME spelling fix, which also
  escaped the manifest's em-dashes), so the shared-lib entry re-heads as
  0.27.3 and the manifest keeps main's escaped description.
- Both changelogs keep BOTH entries, each under its own version.

Gates: sync-hook-utils.sh --check (all 16 copies match) and --check-bump
origin/main (lib changed and every carrying plugin bumped) both pass.

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.

fleet-state reads $HOME as project context when the shell and the OS spell it differently

1 participant