feat(claude-ops): project --ids from a saved report, journal sync runs, split install/enable steps into a spoke - #3748
Merged
Conversation
kyle-sexton
marked this pull request as ready for review
September 5, 2026 09:48
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
|
Claude finished @kyle-sexton's task in 1m 53s —— View job Security review complete
Scope: this PR's diff only — No security issues found. What I checked specifically, since the diff's main new surface is a shell script reading a caller-supplied file path (
This lane doesn't re-report supply-chain/permissions/zizmor-covered findings, and none of those apply here anyway (no workflow or dependency-pin changes in this diff). |
Contributor
|
Last security-reviewed head: |
Contributor
|
Claude finished @kyle-sexton's task in 7m 2s —— View job Code review complete
Scoped to files changed in this PR ( Finding 1 ( Finding 3 (spoke split): Finding 2 (run journal) — two inline comments posted, one real gap:
Followed literally, Step 2/3's A smaller nit: sync.md:421 (Step 6) refers to the snapshots as bare Everything else — |
![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: b3162a6ae5
ℹ️ 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}
Contributor
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
kyle-sexton
force-pushed
the
feat/3728-plugin-sync-cost-resilience
branch
from
September 5, 2026 10:42
b3162a6 to
681ea5c
Compare
The --ids selector rebuilt state the caller was already holding. `sync` re-reads the full JSON report before each mutating step, and every selector (update-candidates-user, missing-user-install, missing-enabled, current-project, installed-user, user-scope-orphans) is derivable from that report, so the separate live --ids process paid a second process creation to re-parse installed_plugins.json, re-walk the catalog manifests, and re-run realpath in order to recompute a block already in hand. `--ids <selector> --from <report.json>` projects from a saved single-marketplace report instead. The projection is lifted into one PROJECTION_PROGRAM that both pass 3 and --from run, so the CR-free, TAB-separated output contract cannot drift between the two modes, which is the whole reason the selector exists rather than a hand-written jq at each call site. A --from run reads no Claude Code state file at all. Every rejection is exit 2 with stdout left EMPTY, because the documented consumer is a process substitution that cannot see the exit status: --from with --all, --from without --ids, a missing or malformed file, an --all envelope (valid JSON every selector projects to nothing, refused by name rather than silently returning an empty list), and a --marketplace that disagrees with the report's own marketplace.name. Under --from that flag is an optional consistency check, never a second read. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gWgHogJFQeCne5U7vHKAE
…3728) Steps 4 and 5 are roughly a hundred of sync.md's lines: the install_new policy branches, the --setting-sources caveat, the reinstall-recurrence caveat, the normalize-enabled-plugins.sh contract, defaultEnabled precedence, and the project-scope enable-gap suppression ordering. They loaded on every run and are unreachable when missing_from_user_install and missing_from_enabled are both empty, which is the common case on a current fleet. The gating signal is already in the Step 1 report. The text moves verbatim into context/sync-install-enable.md with a header stating its read condition, which carries both gates: either array non-empty, or a marketplace whose Step 1 refresh failed and whose report has to name what these two steps deferred. Same progressive-disclosure pattern the hub already uses for converge.md and scope-semantics.md. References that named "sync.md Step 4" or "Step 5" in converge.md, gotchas.md, and scope-semantics.md now point at the new spoke, and the step-internal "see Step 3" references become explicit cross-file links. sync.md's pointer paragraph and SKILL.md's spoke-table row land in the next commit, which is where the rest of those two files' changes live. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gWgHogJFQeCne5U7vHKAE
A sweep of several dozen mutations was one context compaction away from being
unable to emit its own report. Version capture requires an <old> value that
exists nowhere on the machine once the sweep has run, and a <new> value only
the CLI's own output carries, and the skill's mitigation was to hold both in
context through Step 6.
Every run now creates ${CLAUDE_PLUGIN_DATA}/plugins-sync/runs/<UTC stamp>/,
saves each fleet-state.sh re-read there, and appends each mutating CLI call
and its output to journal.log. Step 6 reads the pairs and the three
divergences[] snapshots out of those files rather than out of conversation,
which also gives converge and a later audit a real before-state.
fleet-state.sh does not write it. This honors the reasoning of the deferred
--run-log finding rather than reversing it: the journal is agent-executed
shell around calls the algorithm already makes, and the script stays the
read-only inspector its header advertises. The saved reports are the re-reads
the concurrency rule already requires, so the journal costs a redirect. audit
mode writes no journal, keeping the action table's "Mutates: No" true.
Steps 2 and 3 now project their id lists with --from against the report each
step just read, replacing the second fleet-state.sh process per step, never
the re-read itself. The mandate to take ids from the script and never from a
hand-written jq is unchanged. SKILL.md carries the substituted journal_root
because ${CLAUDE_PLUGIN_DATA} resolves in skill content and not in a
context/*.md spoke, which is read raw. Also carries the Steps 4 and 5 pointer
paragraph and spoke-table row for the preceding commit's move.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019gWgHogJFQeCne5U7vHKAE
Version bump and CHANGELOG entry for the --from projection, the sync run journal, and the sync-install-enable spoke. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gWgHogJFQeCne5U7vHKAE
…s journal (#3728) Addresses PR review findings on the sync cost/resilience branch. fleet-state.sh: `--from` now validates the fields the CHOSEN selector consumes, additively on top of the baseline `.marketplace.name` + `.installed` shape check. A syntactically valid but incomplete report such as `{"marketplace":{"name":"m"},"installed":[]}` used to evaluate the absent array with `[]?`, emit nothing, and exit 0 -- a silently-empty id list read as "nothing to do". It is now exit 2 with empty stdout and an error naming the file and the field. A field present but empty still exits 0 with empty output, so the exit status discriminates. sync.md: the run directory is created with `mktemp -d` so two sessions starting in the same UTC second cannot share it; every tee-journaled mutating call captures `rc=${PIPESTATUS[0]}` so a failed CLI call is not read as success through tee's status; Steps 2-5 show the redirect that creates their saved report and check the projection's exit status before looping; Steps 4 and 5 gate on a fresh pre-Step-4 re-read rather than Step 1's older report; Step 6 uses the marketplace-suffixed snapshot names. `audit` now runs the same algorithm against a throwaway `mktemp -d` scratch directory it deletes, instead of being forbidden to save the reports its own `--from` projections require. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gWgHogJFQeCne5U7vHKAE
#3728) The Step 2, Step 3, and canonical projection snippets assigned `rc` and then looped unconditionally, so the prose telling the reader to check it sat next to code that did not. That is the same defect as journaling a mutating call through `tee` without capturing `PIPESTATUS[0]`: the status is available and discarded. Each snippet now branches, reporting the failure under "Action needed" instead of falling through to the loop. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gWgHogJFQeCne5U7vHKAE
…te.sh (#3728) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gWgHogJFQeCne5U7vHKAE
kyle-sexton
force-pushed
the
feat/3728-plugin-sync-cost-resilience
branch
from
September 5, 2026 11:56
2eb50a4 to
f4bd1bb
Compare
kyle-sexton
pushed a commit
that referenced
this pull request
Sep 5, 2026
Main took claude-ops 0.42.5 for the plugins skill (#3748) while this PR was open, so this branch's release becomes 0.42.6: the manifest keeps this branch's description at the new version, the changelog carries main's 0.42.5 entry under this branch's entry re-labeled 0.42.6, and the retirement record and the hook-telemetry convention note follow. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
kyle-sexton
added a commit
that referenced
this pull request
Sep 5, 2026
No related issue: the logging pipeline was designed and allocated in the hook-logging-pipeline topic (operator brief 2026-09-04/05, relayed through the prompt-hooks session) and tracked in the topic's contract slice rather than an issue; #930 tracks the envelope follow-up it leaves. ## Summary Claude Code's hooks ran with no per-session record of what fired, what was blocked, or what each hook cost, and the only telemetry store was one shared `hook-events.jsonl` under `.claude/observability/` that no session could be joined to. This PR is the third and last from the hook-logging-pipeline topic (after #3747, the verifier-lane fix, and #3749, the PostToolUse kill-switch hoist). It adds an opt-in, default-off per-session hook event log to claude-ops, moves the reference sink and the observability skill to one hook log root with per-session files, adds `SessionEnd` retention that fits the 1.5 s budget, gives `setup` an `apply` for the root's guard plus a retirement record for the old path, and generates the event list from the hooks reference rather than hardcoding it. ## Fix 1. **`hooks/session-log-lib.sh`** (sourced, no `hook-utils.sh`): root resolution from `session_event_log_dir` (default `.observability/claude`) with containment checked lexically and physically (the nearest existing ancestor is resolved with `cd -P`, a builtin, and must sit below the physical project; a symlinked component out of the project, or back to the project root, is refused), the self-ignoring `.gitignore` guard (healed on first write; an operator-edited guard refuses the write; an empty file is healed, which closed a race the 33-parallel-fires case caught), id validation, process-free timestamps, the category table. 2. **`hooks/session-event-log.sh`**: kill switch first, bounded 4 KB slice read to a 64 KB cap that stops early only when the buffer ends in `}`, carries the event name and has balanced braces (the brace characters come from variables, because bash ends a `${...}` expansion at a literal `}` inside a bracket class; the Win32 late-EOF stall costs one idle slice; a writer that pauses after a nested `}` is read to the bound), bash-regex field extraction, one line `{ts, session_id, hook_event_name, category, status, source: "event-log", duration_ms, prompt_id?, tool_use_id?, agent_id?, tool_name?, file_path? (repo-relative, or the last segment after either separator), reason?, traceparent?}` to `<root>/sessions/<session_id>.jsonl`. 3. **`hooks/session-retention.sh`** (`SessionEnd`, no `timeout`, no stdin): keep newest `session_log_keep_sessions` OR younger than `session_log_keep_days`, four spawns; with `session_log_pre_prune_command`, doomed files move to `prune-pending/<epoch>-<pid>/` and the command runs detached (`nohup`, stdin closed) with that directory; sets older than 24 h are deleted on the next run. 4. **`scripts/gen-hook-event-registry.sh`**: `--fetch | --from <file> | --check`; writes `hooks/hook-events.registry.json` (33 events, 30 observable; `WorktreeCreate`, `MessageDisplay`, `FileChanged` excluded because each replaces or holds native behavior when registered; unknown names excluded with a warning; under 25 rows refuses) with upstream-drift stamps, and regenerates the producer rows and the retention row in `hooks.json` preserving the nine audit handlers. `--check` is offline. 5. **Reference sink** (and the repo-local copy): an envelope carrying a well-formed `data.session_id` routes to `sessions/<id>.jsonl` in the spine shape (`source: "envelope"`, `changed` when sent); any other envelope to `<root>/hook-events.jsonl` in the legacy shape under its lock. The nine audit hooks send `data.session_id`; seven data schemas gain the optional key. 6. **`setup`**: `check | apply`. Probe 5 reports the root, containment, root-equivalence refusal, and the guard; probe 6 is the fixed retired-conventions line. `apply` writes exactly `<root>/.gitignore` and reads back the tracked-versus-ignored pair. `retirements.yaml` gains `claude-ops-r001` (`.claude/observability/hook-events.jsonl`, `migrate`); the helper copy is enrolled in `scripts/sync-check-retirements.sh`; one eval per record plus the guard-only and root-refusal evals. 7. **`observability`**: every whole-root query reads `sessions/*.jsonl` plus the shared file through one `HOOK_NORM` prelude; `session` (newest by mtime) and `session:<id>` render a per-session report (hooks fired, blocked, rewrote, per-hook duration, event timeline); every report ends with the six lines of `probe-observability-state.sh --pipeline` (root, guard, sessions, shared, prune-pending, toggles). The probe gains `--root` and `--pipeline` (the rendered options arrive as flags because a skill subprocess inherits no `CLAUDE_PLUGIN_OPTION_*`); `clean.sh` gains `--hook-root`, prunes the root's shared file, removes session files untouched for the window, and sweeps stale `prune-pending/` sets whether or not the switch is on. 8. claude-ops to 0.42.6 with the CHANGELOG entry (main took 0.42.4 for #3749 and 0.42.5 for #3748 while this PR was open; both are merged in); six new `userConfig` keys and the regenerated README options table; `.gitignore` gains `.observability/`; the hook-observability convention gains the `# silent-skip-ok:` paragraph and the hook-telemetry convention the sink-routing note with the #930 pointer; `docs/CATALOG.md` and `docs/SKILL-CHEAT-SHEET.md` regenerated. ## Verification Measured on the Linux CI host, N = 15 (raw captures in the topic's memory slice; the distilled rows lived in the branch's FINDINGS.md until the prune commit): | Row | Result | | --- | --- | | `session-event-log` disabled (the default) | 2.42 ms against a 2.08 ms bare spawn floor (1.16 S; acceptance bound 1.5 S) | | enabled, 2 KB payload | 4.5 to 5.75 ms | | enabled, 512 KB `tool_response` | 35.7 ms | | sink, envelope with `session_id` | 26.7 ms, off the critical path | | retention, 40 files nothing doomed / 100 files 70 pruned | 4.3 ms / 24 ms | | held-open stdin (late-EOF shape) | producer returns in 262 ms (asserted under 700; a broken early stop measured 1262); retention under 500 ms | | toggle cycle (on 10, off 10, on 10, then `SessionEnd`) | root holds only `.gitignore` and `sessions/cycle-1.jsonl` with 20 parsing lines; `git status` clean | Suites (all beside their scripts): `session-event-log.test.sh` 53, `session-retention.test.sh` 20, `hook-telemetry-sink.test.sh` 38 plus the repo-local drift check, `audit-session-id.test.sh` 27, `gen-hook-event-registry.test.sh` 25, `probe-observability-state.test.sh` 48, `claude-observability.test.sh` 57 (was 33), the guardrails `skill-reference-verify` suite 139 after the merge from main. Four review findings were verified by reproduction and fixed with repro-first cases (each fails on the previous script): the early stop firing on a nested `}` during a mid-message pause (a fresh-context review), a Windows path outside the project not reduced to its last segment (same review), the brace-count class that bash could not parse as written (the Claude review lane), and a configured root escaping through a symlinked component (the Codex lane). The empty-guard race case likewise fails before and passes after. `scripts/affected-tests.sh --run`: 160 shell suites pass; the one failure is `session-flow`'s `save_point.test.sh` (`test_new_origin_falls_back_to_directory_name`, a directory-name assertion in a suite this branch does not touch, failing identically on `origin/main` in this container). `scripts/check-changelog-parity.sh --check-bump origin/main`, `scripts/sync-check-retirements.sh --check`, `scripts/gen-hook-event-registry.sh --check`, `scripts/check-silent-skips.sh`, `scripts/sync-plugin-options-docs.py --check`, `scripts/check-changed-skills.sh origin/main`, and `scripts/validate-plugins.sh` all exit 0; shellcheck at info severity is clean on every changed script. CI is green on the head, the Windows test lane included. Windows Git Bash is the binding host for the hook-budget parallel-wall figure and unmeasured here; the README says so, the switch stays off by default until it is taken, and the Codex thread asking for it is left open for the operator, who has the host. The topic's contract slice (`docs/topics/hook-logging-pipeline/`) rode this branch for review and is pruned on this head, per the topic-docs convention; the Brief and the phases this PR ships are summarized below. <details> <summary>Brief (TLDR, Goal, locked decisions) and Phases 3 to 8 of the plan</summary> ### TLDR Design the logging and telemetry pipeline for the marketplace's hooks, and settle the upstream decisions that determine what it instruments. Evidence base is FINDINGS.md, a measured read-only audit of the 26 wired `PostToolUse` rows plus a doc-alignment pass. Interview complete: five rounds, 21 questions, 19 answered and 2 deferred with named arbiters. ### Goal Observability across every hook event, defaulting to off, costing effectively nothing when off and as close to nothing as measurable when on, with no surface left as a black box and every toggle reachable by Claude on the operator's behalf rather than by hand. ### Decisions locked (the ones this PR implements) Logging hooks stay in `claude-ops` (no plugin per hook); every documented event, plugin default-OFF, from a generated registry with upstream-drift stamps; storage at `.observability/claude/`, configurable, one file per session; three toggle levels (sink unconfigured, per-producer switch, category filter); five correlation keys as a hierarchy (`session_id`, `prompt_id`, `tool_use_id`, `agent_id`, `TRACEPARENT`); a fixed spine with payload only where a decision was made; the producer sources no library; retention keeps the newer of 30 sessions or 14 days at `SessionEnd`; the pre-prune command runs detached; the guard heals on first write inside the plugin-owned root and the project root is refused; only `hook-events.jsonl` migrates, the skill-usage and OTEL stores stay. ### Phases - **3, integration slice:** the library, the producer, the sink routes, `data.session_id` on the nine audit hooks, the six options. - **4, registry:** the generator, the fixture, `--check` in the suite, the regenerated `hooks.json`. - **5, retention:** the `SessionEnd` hook, the prune-pending move-aside, `clean`'s sweep. - **6, setup:** `apply` for the guard, the retirement record and its helper copy, the evals. - **7, reader:** the root-wide queries, the per-session report, the pipeline probe, the path migration across the skill's docs, README and manifest. - **8, docs and PR:** the CHANGELOG entry and version, the two convention notes, the toggle cycle, the Windows recheck table, the affected suites, the code review, this PR, and the prune commit before the ready flip. ### Follow-ups this PR leaves - Windows Git Bash recheck: the hook-budget parallel-wall figure for the always-on kill-switch read; same-second `>>` appends to one session file at 4 KB lines; `ls -t` tie order on NTFS for retention's "newest N" and the reader's `session` scope; the late-EOF slice cost against the 262 ms Linux figure; the peer-reported 11 s cold `cli-flag-verify` run. - #930: `data.session_id` on every producer and the envelope 1.1 spine promotion; until then per-session hook duration covers the nine claude-ops audit hooks. - `changed` is a defined per-session key no formatter emits yet; the "rewrote" block of the per-session report stays empty until one does. </details> ## Related - #930, the envelope follow-up: `data.session_id` on every producer and the `schema_version` 1.1 spine promotion - #3747, #3749 and #3748, the three PRs main took while this branch was open, all merged back in - `docs/conventions/hook-observability/README.md` and `docs/conventions/hook-telemetry/README.md`, both amended here - `docs/conventions/retired-conventions/README.md`, whose two fixed setup lines and eval-per-record rule the setup change follows - `lib/hook-utils.sh` and its 17 vendored copies are untouched 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3728
Summary
Three cost and resilience findings from a
/claude-ops:plugins syncrun against an already-current fleet. None were correctness bugs. The--idsselector rebuilt state the caller already held, the skill kept no durable record of a sweep it later has to report on, and about a hundred lines of install and enable policy loaded on every run while being unreachable in the common case.Fix
--ids <selector> --from <report.json>.fleet-state.shprojects a selector from a report it already emitted instead of recomputing the fleet. The projection is now one jq program shared by the live path and--from, so the CR-free output contract (the reason the selector exists rather than hand-rolled jq at each call site) cannot drift between them.--fromrejects combination with--all/--marketplaces, disagrees loudly if--marketplacenames a different marketplace than the report, and fails exit 2 with empty stdout on a missing or malformed file.sync.mdSteps 2 and 3 now project from the report they already read; Steps 4 and 5 keep their own live re-read (the concurrency rule requires it) and project from the saved copy.syncwrites${CLAUDE_PLUGIN_DATA}/plugins-sync/runs/<UTC timestamp>/with eachfleet-state.shreport (pre, mid, post), and appends each mutating CLI command and its output tojournal.log. Step 6 reads old/new pairs and the threedivergences[]snapshots from the journal instead of conversation memory.auditwrites no journal, so its "Mutates: No" row stays true. Thejournal_rootassignment lives inSKILL.mdbecause${CLAUDE_PLUGIN_DATA}substitutes in skill content and not in acontext/*.mdspoke.context/sync-install-enable.md. Steps 4 and 5 move into their own spoke, read only whenmissing_from_user_installormissing_from_enabledis non-empty or the Step 1 refresh failed.sync.mdkeeps a pointer paragraph with the read condition; the reference index inSKILL.mdgains the row; cross-references inconverge.md,gotchas.md, andscope-semantics.mdare repointed. The Report template andinstall_newrender stay inSKILL.md. Frontmatter is untouched.0.42.4->0.42.5with CHANGELOG entry (main took 0.42.4 mid-flight).Verification
fleet-state.test.sh: 86 cases, 0 failed, post-rebase on main. New cases: incomplete report missing a selector-required field exits 2 naming the field;--fromprojection equals live projection per selector;--fromon a missing or malformed file exits 2 and names the file;--fromcombined with--allis rejected;--marketplacedisagreement with the report is rejected.shellcheck -S warningclean on both scripts.markdownlint-cli2overplugins/claude-ops/**/*.md: 0 issues in 54 files.node scripts/validate-plugin-contracts.mjs: validated.scripts/affected-tests.sh --runnot run locally (it hangs oncheck-changed-skills.test.shon this machine, see fix(claude-ops): resolve default marketplace for out-of-cache plugin roots #3744); CI is the statement of record for that suite.Related
fleet-state.shthis branch was rebased over)🤖 Generated with Claude Code
https://claude.ai/code/session_019gWgHogJFQeCne5U7vHKAE