fix(claude-ops): emit plugin ids from fleet-state.sh instead of a hand-written jq loop (#2578) - #2581
Conversation
|
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 finished @kyle-sexton's task in 3m 45s —— View job Code review complete
Findings1 inline comment posted — sync.md Step 2 ( What I checked and didn't flag
I did not independently execute the shell test suite, |
|
Claude finished @kyle-sexton's task in 1m 45s —— View job Security Review —
|
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 136c680e5b
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 4 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…with each id (#2578) Review findings on #2581. Error objects out of the id stream: emit_marketplace writes a {marketplace:{name,error}} block to stdout and returns nonzero, but the documented consumer is `while read ... done < <(... --ids ...)`, and a process substitution does not propagate its command's exit status. The block was read as an id and handed to `claude plugin update` verbatim. Under --ids that block now goes to stderr; report mode is unchanged. Scope pairing: `--ids current-project` projected only .id, but one plugin can hold BOTH a project- and a local-scope record for the same repo, each currentProject:true. The id appeared twice with nothing to distinguish the lines, so Step 2 could not pick the right `-s` flag and a `sort -u` would drop an update. Records are now tab-separated with the id always first; current-project carries scope as a second field. Step 2 reads it with `IFS=$'\t' read -r id scope`. Also hoists selector validation to parse time, so a bad selector reports as a usage error (exit 2) instead of being masked by an unresolvable marketplace's exit 1, and adds `set -o pipefail` to the test's jq stub so it reports jq's status rather than sed's. New cases: dual-scope pairing, error-block-off-stdout, bad-selector-outranks- unresolvable-marketplace, and --ids on the zero-arg default path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W
|
Claude finished @kyle-sexton's task in 2m 5s —— View job Code review complete
Scope of this passThis is a
I re-reviewed both fixes end-to-end rather than trusting the description:
1 new inline comment posted — a stale leftover comment in What else I checked and didn't flag
I did not independently execute the shell test suite, |
|
Claude finished @kyle-sexton's task in 1m 18s —— View job Security Review —
|
|
Claude finished @kyle-sexton's task in 53s —— View job Code review complete
Scope of this passThis is a What I checked
No issues found — this commit is small, correct, and self-contained. No inline comments to post. |
|
Claude finished @kyle-sexton's task in 34s —— View job Security Review —
|
CI trigger note — not merge-ready until the
|
| Commit | Change |
|---|---|
4e955c90 |
deletes 5 stale comment lines in emit_one (review finding) |
391eade2 |
adds 7 documentation lines to SKILL.md |
59c387d8 |
empty commit, CI retrigger attempt |
Local verification on the current head (59c387d8), Windows / Git Bash with the real native jq:
| Gate | Result |
|---|---|
fleet-state.test.sh |
45 cases, 0 failed |
shellcheck (both changed .sh) |
clean |
markdownlint-cli2 (all 4 changed .md) |
0 errors |
scripts/validate-plugins.sh |
pass |
check-changelog-parity.sh --check / --check-bump / --check-preserved |
pass |
check-shell-portability.sh origin/main |
no unexcused constructs |
check-skill-portability.sh origin/main |
no unexcused tokens |
check-silent-skips.sh / check-discriminating-test-skips.sh |
pass |
Please re-run CI before merging rather than relying on the a32d625e result — I am not treating
this as green.
…d-written jq loop (#2578) `sync` Steps 2-5 told the reader to take an id list out of fleet-state.sh's JSON and loop a `claude plugin` call over it, but never supplied the extraction, so every reader hand-wrote their own `jq -r`. The native Windows jq writes stdout in text mode and `$(...)` strips only the trailing CRLF, so every id but the last reached the CLI as `<name>@<marketplace>\r` and failed with `Plugin "<name>" not found` -- text identical to the bare-name gotcha, which misdirects the diagnosis. Observed live: 64/65 updates failed. Adds `fleet-state.sh --ids <selector>`, emitting one fully-qualified id per line, CR-free by construction via the wrapper the script already has, so the sweep needs no reader-side jq at all. Steps 2-5 now cite it. Also corrects the mechanism gotchas.md described: a single-line capture is clean (not corrupt as claimed), only multi-line output keeps a CR on every line but the last, `mapfile -t` has no last-element reprieve, and jq->jq relays self-clean because jq's stdin is text-mode too. Regression test is host-independent: a PATH stub normalizes jq's output to exactly one trailing CR, so it exercises the same bytes on Linux as on Windows, and goes red both if --ids is removed and if the wrapper is deleted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W
…with each id (#2578) Review findings on #2581. Error objects out of the id stream: emit_marketplace writes a {marketplace:{name,error}} block to stdout and returns nonzero, but the documented consumer is `while read ... done < <(... --ids ...)`, and a process substitution does not propagate its command's exit status. The block was read as an id and handed to `claude plugin update` verbatim. Under --ids that block now goes to stderr; report mode is unchanged. Scope pairing: `--ids current-project` projected only .id, but one plugin can hold BOTH a project- and a local-scope record for the same repo, each currentProject:true. The id appeared twice with nothing to distinguish the lines, so Step 2 could not pick the right `-s` flag and a `sort -u` would drop an update. Records are now tab-separated with the id always first; current-project carries scope as a second field. Step 2 reads it with `IFS=$'\t' read -r id scope`. Also hoists selector validation to parse time, so a bad selector reports as a usage error (exit 2) instead of being masked by an unresolvable marketplace's exit 1, and adds `set -o pipefail` to the test's jq stub so it reports jq's status rather than sed's. New cases: dual-scope pairing, error-block-off-stdout, bad-selector-outranks- unresolvable-marketplace, and --ids on the zero-arg default path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W
The doc snippets used `[ -n "$id" ]`, which shellcheck flags as SC2292 in this repo's own lint. A reader copying the Step 2/3 or gotchas loop into a .sh file would trip the rule the test file was just corrected for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W
The --ids stderr routing added in ae07eef left the previous paragraph in place next to its replacement, so two adjacent comments made opposite claims about where a per-marketplace failure block goes. Keeps only the paragraph that matches the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W
…2578) The state-inspection block showed only the JSON-report form, so a reader who never opens sync.md still has no signal that an id list is available and hand-writes a jq extraction — the exact path this fix exists to close. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W
The pull_request-triggered workflows stopped firing repo-wide after 05:33 UTC; this commit carries no content change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W
59c387d to
006a021
Compare
|
Claude finished @kyle-sexton's task in 2m 3s —— View job Code review complete
Scope of this passThis is a Of those, only two carry content not yet independently reviewed by Claude (the prior passes covered up through
What I checked on this pass
I was not able to execute the shell test suite, No new issues found. No inline comments to post — this pass is a clean confirmation that the two prior review findings were fixed correctly and nothing regressed. |
|
Claude finished @kyle-sexton's task in 1m 9s —— View job Security Review —
|
1 participant
Summary
plugins'syncsteps told the reader to take an id list out offleet-state.sh's JSON and loop aclaude plugincall over it, but never supplied the extraction — so every reader hand-wrote theirown
jq -r. On Windows that hand-writtenjqreintroduces a CR thatfleet-state.shis careful tostrip, and the sweep then fails for every id but the last. Observed live: 64 of 65 user-scope
updates failed, all with
Plugin "<name>" not found— text identical to the documented bare-namegotcha, so it reads as "the marketplace is broken".
This adds
fleet-state.sh --ids <selector>, which emits the id list directly (CR-free byconstruction), points Steps 2-5 at it, and corrects the CRLF mechanism
gotchas.mddescribes.Related
Closes #2578.
The mechanism, corrected
The native Windows
jqopens stdout in text mode, so every\nit writes becomes\r\n.Which capture is actually corrupted depends on how it is read — verified on jq 1.8.2 (winget
jqlang.jq) with MSYS bash 5.3.9:x=$(… ), single-line output$(…)strips the trailing\r\nas a unitx=$(… ), multi-line output\rmapfile -t/readarray -t\r, last includedTwo of those contradict what
gotchas.mdsaid, and the corrections matter:\r. It does not, and that claim predicts the wrongsymptom — all ids failing rather than all but the last. The all-but-last pattern is the single
most useful diagnostic signal here (if only the last item in a batch worked, stop looking for a
logic bug), and the old text talked the reader out of it.
it is jq's line output reaching a non-jq consumer — an external command's argv, a string
comparison, a file write. That is exactly the shape the production loop had.
IFS=$'\n'does not help in any case:\ris not the separator, it rides inside the token.Repo-wide sweep (item 2)
Scripted, paren-aware scan of all 499 tracked
.shfiles and 1120.mdfiles for jq outputreaching a line-wise consumer —
jq | while read,< <(jq),mapfile/readarray,for x in $(jq),and
VAR=$(jq …)later split via<<<"$VAR". 31 shell sites and 4 markdown fenced blocks; 0unguarded. (Rows below group sites that share a file or a synced source; the per-row counts sum to
31 + 4.)
lib/hook-utils.sh:935+ 16 synced plugin copiesread -d '' < <(jq -j)clean="${v//$'\r'/}"after the read, with a comment naming this exact hazard. One source, 17 files.scripts/check-hook-exec-form.sh:282while read < <(jq -r)rel="${rel%$'\r'}"in the loop bodyscripts/check-hook-userconfig-argv.sh:113while read < <(jq -r).claude/hooks/hook-telemetry-sink.sh:42+plugins/claude-ops/hooks/copymapfile < <(jq)| tr -d '\r'in the pipelineplugins/claude-ops/.../fleet-state.sh:591while read <<<"$names"jq()wrapper (line 105)plugins/claude-config/.../check-plugin-drift.sh:270while read <<<"$var"| tr -d '\r'plugins/work-items/.../claim.sh:102,lib/lease.sh:54while read < <(jq -c)rowis only ever re-parsed by jq, and the scalars derived from it are single-line$()capturesplugins/work-items/scripts/backfill-capability-tier-labels.sh:90jq -c | while readbodyreachesgrep -Ewith unanchored patterns, so a trailing CR cannot change the matchplugins/source-control/.../fetch-annotations.sh:128while read <<<"$var"cr_id/cr_nameare single-line capturesplugins/claude-config/.../fix-plugin-drift.sh:125,134(2)while read <<<"$var"jq -nR '[inputs]', which strips the CR on input. Reproduced end-to-end:settings.jsonkeys come out clean and the orphandel()succeedsplugins/claude-config/.../fix-plugin-drift.sh:140,146(2)while read <<<"$var"printfwayfind/tracker-mechanics.md,babysit-loop,attend-queue,work-loop)--jq | while read| tr -d '\r'fix-plugin-drift.shwas the one site that looked genuinely broken — it builds"\(.name)@\(.marketplace)"ids, the exact shape from the incident, and feeds them into aread-modify-write of the user's
settings.json. It was worth reproducing rather than reasoningabout, and the reproduction is what surfaced jq's input-side text-mode translation: the CR is
stripped again by
jq -nR,del()matches, and nothing corrupt is written. Classified benign onevidence, not inspection.
Remediation chosen, and why
Eliminate the reader-side
jqrather than document a guard around it. Every guard style alreadyin this repo (
| tr -d '\r',${v//$'\r'/},${var%$'\r'}, the file-scopedjq()wrapper) onlyhelps code that is in the repo. The failure here was in shell an agent improvised from prose, so
no guard convention could have reached it.
--idsremoves the improvisation surface: the step isnow a
readloop over a script that already routes everyjqcall through its wrapper.Rejected alternatives: adding
| tr -d '\r'to the prose (still hand-written, still forgettable,and only fixes Step 3 of four);
IFS=$'\n'(verified not to help);jq --raw-output0+read -d ''(works, but would introduce a second idiom next to the
tr -d '\r'convention already establishedacross 20+ sites).
Red-first evidence (item 4)
New assertions run against the unmodified
fleet-state.sh(pre-fix script + new tests, in theworktree so
hook-utils.shstill resolves):Every pre-existing case stayed green, so the 8 failures are the new contract and nothing else.
The CR case is host-independent. A PATH stub named
jqnormalizes every emitted line to exactlyone trailing CR (
sed 's/\r*$/\r/'), so it exercises the same bytes on a Linux runner (where realjq emits bare LF and the stub adds the CR) as on Windows (where the stub is a no-op).
command jqresolves through PATH, so
fleet-state.sh's own wrapper calls the stub exactly as it would call anative Windows jq — meaning the case goes red both if
--idsis removed and if that wrapper isever deleted.
Two things learned from #2571's lesson and applied here:
make the CR assertion vacuously pass.
was empty and
*$'\r'*was vacuously false — the assertion passed for the wrong reason until thatbranch was added. That is the same "test encodes the bug as the contract" failure feat(ci): gate exec-form hooks against bare command names (#2569) #2571 fixed, and
it showed up here in the first red run.
Should this be a CI lint? (item 6) — No
Not in #2569/#2571's gate:
check-hook-exec-form.shis exec-form/bare-command-name domain, and a CRclass shares none of its parsing. The only sane home would be
scripts/check-shell-portability.sh(changed-
.shlint, data-driven token list, an existing!namemechanism for classes needing coderather than an ERE, and a per-site
portability-ok:escape). Even so, it should not be added now:gate in this repo reads the repo.
zero violations is pure false-positive risk on unrelated PRs.
often in the loop body, after the redirect that names
jq—check-shell-portability.sh'sline-record model has no loop-scope view, and every existing class is decidable within a record.
If an unguarded site ever does land,
check-shell-portability.shis the home and this section isthe design note. Filing a follow-up issue for a lint I am arguing against would just park the same
false-positive risk in the backlog.
Changes
plugins/claude-ops/skills/plugins/scripts/fleet-state.sh—--ids <selector>(installed-user,current-project,missing-user-install,missing-enabled). One tab-separated record per line,first field always the fully-qualified id;
current-projectcarriesscopeas a second field.Selector validated at parse time; refuses
--all; per-marketplace failure blocks go to stderr inthis mode. Header documents the record shape and the widened exit-2 set.
plugins/claude-ops/skills/plugins/scripts/fleet-state.test.sh— 22 assertions across 10 cases,incl. the host-independent CR regression, the dual-scope pairing case, and the
error-block-off-stdout case.
context/sync.md— Steps 2-5 cite--ids; Step 3 carries the full guarded loop.context/gotchas.md— mechanism corrected (per-read-form table, all-but-last signature, jq→jqself-cleaning,
mapfilecaveat) and pointed at--ids. No second CR section added.CHANGELOG.md/plugin.json— 0.31.15.Verification
fleet-state.test.sh(Windows, real native jq)scripts/validate-plugins.shscripts/check-changelog-parity.sh --check/--check-bump origin/mainshellcheck(both changed.sh)markdownlint-cli2(all changed.md)scripts/check-shell-portability.sh --pathsscripts/check-skill-portability.sh --pathsThe
hook-utils-windowslane coverslib/hook-utils.test.sh; this change is claude-ops-scoped, sothe meaningful Windows evidence is the local run above against the real native jq — the exact binary
whose text-mode stdout causes the defect.