Skip to content

perf(distribution): batch Git index reads in sync-manifest.sh - #361

Merged
kyle-sexton merged 2 commits into
mainfrom
cursor/sync-manifest-index-batch-f19e
Aug 12, 2026
Merged

perf(distribution): batch Git index reads in sync-manifest.sh#361
kyle-sexton merged 2 commits into
mainfrom
cursor/sync-manifest-index-batch-f19e

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

Closes #316 (Part 1). Part 2 (grep → Bash [[:cntrl:]] control-char check) already landed in #358.

Batches git ls-files --stage and git hash-object so production validate uses one of each instead of per-path spawns. Exact recorded-path attribution is preserved.

Test plan

  • bash distribution/sync-manifest.test.sh
  • Nested-prefix fixture + spawn-counting shim
  • CI green on merge

Related

Closes #316

Open in Web Open in Cursor 

@cursor

cursor Bot commented Aug 12, 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 12, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 6m 36s —— View job


Claude's review

  • Gather context (CLAUDE.md, REVIEW.md, diff)
  • Review distribution/sync-manifest.sh changes
  • Review distribution/sync-manifest.test.sh changes
  • Post findings

Scope note: no .github/workflows/claude-security-review.yml exists in this repo yet, so per REVIEW.md this lane also covers security-scoped checks; I didn't find any (no external input reaches a git/shell invocation unsanitized — paths are validated via is_safe_repo_path before use, unchanged by this diff).

Verification note: the Bash tool in this environment required approval for every command I tried (including read-only ones like command -v shellcheck and running the test suite), and none were approved during this review, so I could not independently run shellcheck or bash distribution/sync-manifest.test.sh. Everything below is from static reading of the diff — the PR description's own test plan is author-claimed and unverified by me.

Summary

The refactor batches git ls-files --stage and git hash-object calls behind a root-keyed cache (INDEX_ENTRY_CACHE/WORKTREE_HASH_CACHE), while preserving the original exact-recorded-path attribution semantics (filtering ls-files results, which can include directory-descendant matches, down to only the exact requested paths via the want map). I traced the call sites in validate_manifest and apply_target and the prefetch/preflight path pairing looks correct — apply_target's new collect_destination_index_paths walks the exact same parent-prefix chain that preflight_destination queries later, so the latter should hit a warm cache with no extra spawns.

One inline finding posted (🟡 Nit, on prefetch_worktree_hashes).

Additional finding (no single changed line to anchor)

🟡 Suggestion — test coverage asymmetry. The new spawn-counting test (distribution/sync-manifest.test.sh, the spawn_count_dir block added ~line 431) verifies validate issues exactly one ls-files and one hash-object call via a git shim. This PR adds the equivalent batching to apply_target's destination-index prefetch too (sync-manifest.sh#L912-L956), but no analogous spawn-count assertion exists for apply. The existing "multi-file component applies in one operation" test exercises mixed root/nested destinations functionally, so a regression wouldn't silently corrupt output — but a future change that reintroduces a per-destination ls-files spawn inside apply (defeating half this PR's stated purpose) wouldn't be caught by the suite.

Nothing else stood out

  • Manifest's own worktree hash is added to hash_paths unconditionally (validate_manifest, sync-manifest.sh#L629) while sources are pre-filtered by -f && ! -L first (#L631). I traced through this and it still fails safely — tracked_regular_mode re-checks the manifest's symlink/regular-file status right after — so I'm not flagging it as a real bug, just noting the asymmetry in case it's not intentional.
  • Bash-4.0 empty-array compatibility ("${arr[@]}" under set -u) is guarded correctly at every new call site I checked.
  • Error-handling/die propagation through the new prefetch_* helpers matches the existing exact_index_entries idiom (temp file cleaned up on both success and failure paths).

Branch

Replace per-path mktemp + git ls-files --stage + git hash-object cycles
with one batched ls-files call and one batched hash-object call per Git
root during validate, and one batched ls-files call per apply preflight.

Exact recorded-path attribution is preserved when directory-shaped pathspecs
would also select descendants. prefetch_index_entries populates a per-root
cache consumed by read_exact_index_entries, tracked_regular_mode, and
preflight_destination.

Add nested-prefix fixture coverage and a spawn-counting shim assertion that
production validate issues exactly one ls-files and one hash-object call.

Closes #316 (Part 1; Part 2 landed separately).

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@cursor
cursor Bot force-pushed the cursor/sync-manifest-index-batch-f19e branch from f21a71e to 1ef1a93 Compare August 12, 2026 00:41

@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: f21a71ec1a

ℹ️ 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 distribution/sync-manifest.sh
Comment thread distribution/sync-manifest.sh
@github-actions

Copy link
Copy Markdown

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

…fetch

- Reject FIFO/symlink manifest worktree paths before the first yq read
- Extract assert_tracked_regular_shape for shared index/worktree checks
- Mirror prefetch_index_entries dedup in prefetch_worktree_hashes via claimed
- Add manifest symlink and FIFO rejection tests

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@kyle-sexton
kyle-sexton merged commit d8bdce9 into main Aug 12, 2026
42 checks passed
@kyle-sexton
kyle-sexton deleted the cursor/sync-manifest-index-batch-f19e branch August 12, 2026 01:10
kyle-sexton added a commit that referenced this pull request Sep 7, 2026
<!-- CURSOR_AGENT_PR_BODY_BEGIN -->
No related issue: remaining Bash spawn N+1 after #316/#361, ranked by
spawn census rather than file size.

## Summary

Measurement-first Bash performance pass, following the fleet
`performance@melodic-software` plugin (spawn count over wall clock,
stable PATH shim, interleaved repeats) and the patterns already landed
in #320/#358/#361.

`distribution/sync-manifest.sh` is already a Node wrapper; this change
does not touch it.

Highest-impact remaining subjects on this repo, in order:

1. `check-exec-bit.sh`: **133 → 4** process spawns (**git 131 → 2**)
against 65 shebang-looking blobs.
2. `scan-comment-hygiene.sh`: **81 → 3** (**awk 77 → 0**) — one awk per
git-grep hit.
3. `check-machine-specific-paths.sh`: **10 → 1** (**git 5 → 1**, **head
5 → 0**).
4. `scan-workflow-files.sh`: **yq 11 → 1** on the success path (CI glob
is 11 workflow files).
5. `check-plugin-baseline.sh --compare`: **jq 5 → 3**.
6. cloud-bootstrap warm path: **5 Claude list CLIs → 2**.
7. `setup.sh` plugin stage (fleet+repo, warm): **4 Claude list CLIs + N
grep → 2** (same list-once shape as cloud-bootstrap).
8. `check-claude-settings-targets.sh --file`: **jq 3 → 2** on the
success path.
9. `check-heading-cites.sh`: drop the identity `cat` when `EXCLUDE` is
empty.

Host qualification (`bash -c true`, n=20): spread 1.66× (measurable).
Headline metric is still the drift-immune spawn counter.

## Fix

- **exec-bit**: one `git grep --cached -z -nIE '^#!'` (line 1 ≡ byte-0
shebang) plus one `git ls-files --stage -z` with exact-recorded-path
mode lookup. POSIX awk join (bash 3.2: no `declare -A`). Combined `-z
-n` emits `path NUL lineno NUL text LF` (git 2.43 byte dump; git-grep(1)
only mentions pathname delimiting). The 100644 shebang case fails
closed; a contract test pins that record via `tr '\0' '|'`.
- **comment-hygiene**: `chp::scan_text` matches the comment-start
pattern in-process for newline-free content (the local-lane git-grep hit
shape). Multi-line callers still use one awk.
- **machine-specific-paths**: one combined `git grep -E` over the five
OS/repo bodies; classify hits in-process so per-OS labels and the
20-line cap stay without a second grep or `head`.
- **heading-cites**: split the corpus pipeline so an empty `EXCLUDE`
does not spawn `cat`.
- **check-plugin-baseline `compare_settings`**: one tagged jq pass,
matching `compare_catalog` and the #320 yq consolidation.
- **cloud-bootstrap**: list marketplaces and plugins once; record
installs locally; re-list marketplaces only when this session registered
a new one. Materialized `.claude/cloud-bootstrap.sh` stays
byte-identical to the component.
- **setup.sh plugin stage**: share one `marketplace list` and one
`plugin list` across fleet+repo; in-process membership instead of `grep
-qxF` per entry; record new installs so the second source does not
re-query. Toolchain tracks (apt/dotnet/nvm) are unchanged.
- **settings-targets**: one jq policy pass on the success path; `jq
empty` on the candidate runs only when that pass fails, so parse errors
stay exit 2 and policy misses stay exit 1.
- **pin-comment**: one `yq eval --no-doc` over every existing workflow
argument on the success path (`filename` operator; without `--no-doc`,
`---` document separators are misread as records). Mike Farah yq 4.53.3
multi-file `eval` still stops at the first parse error and has no
`try`/`catch`, so a failed batch falls back to per-file `pcc::scan_text`
and still reports every file.

Left unchanged after census (already at floor, tiny corpus, or
I/O-dominated): `sync-manifest` Node engine, setup.sh apt/dotnet/nvm,
private-repo-inventory (tiny TOML), `repin::ahead_of` (release-time,
small N).

## Verification

- `bash harness/shell/run-tests.sh` — previously 19 passed, 0 failed, 16
skipped on the earlier commits; this revision adds spawn-census coverage
in `pin-comment-convention.test.sh`.
- `comment-hygiene.test.sh`: 8 single-line scans awk=0; multi-line
fixture awk=1
- `local-lane-guards.test.sh`: exec-bit 8-file git shim (grep=1,
ls-files=1, cat-file=0); 100644 shebang fails closed; `git grep -z -n`
layout `bad.sh|1|#!/usr/bin/env bash`; machine-paths git=1 head=0;
comment-hygiene driver awk=0; start-of-line Linux home still labeled
- `check-plugin-baseline.test.sh`: `--compare` jq=3
- `cloud-bootstrap.test.sh`: warm path marketplace-list=1,
plugin-list=1; copies identical
- `setup.test.sh`: fleet+repo warm path marketplace-list=1,
plugin-list=1, marketplace-add=0, plugin-install=2 (beta+gamma); `bash
-n` and stamp-after-wait contracts unchanged
- `check-claude-settings-targets.test.sh`: `--file` success-path jq=2;
malformed candidate still exits 2
- `pin-comment-convention.test.sh`: 8 conforming files yq=1; parse-error
fallback still reports a later missing-comment (batch + per-file yq=4)

| subject | before | after |
| --- | --- | --- |
| `check-exec-bit.sh` | 133 (`git=131`) | **4** (`git=2`) |
| `scan-comment-hygiene.sh` | 81 (`awk=77`) | **3** (`awk=0`) |
| `check-machine-specific-paths.sh` | 10 (`git=5` `head=5`) | **1**
(`git=1`) |
| `scan-workflow-files.sh` (CI glob, success) | 11 yq | **1** yq |
| `--compare` identical fleet list | 10 (`jq=5`) | **6** (`jq=3`) |
| `check-heading-cites.sh` | 8 (incl. `cat=1`) | **7** (`cat=0`) |
| `setup.sh` plugin stage (fleet+repo, warm) | 4 Claude list CLIs + N
grep | **2** (`grep=0`) |
| `check-claude-settings-targets.sh --file` | 3 jq | **2** jq |

Grounding: GNU/Git `cat-file --batch` / `ls-files --stage` / `grep`
(batch vs per-object); Google Shell Style Guide “builtin vs external”
and no UUOC; this repo’s #320/#361 spawn-census method; performance
plugin harness-integrity (stable PATH, fail closed, commit before
verify); [yq file
operators](https://mikefarah.gitbook.io/yq/operators/file-operators).

## Related

- Follows #320, #358, #361 and closed #316 (sync-manifest remaining
spawns; engine now Node).
- Sources: [Git cat-file batch](https://git-scm.com/docs/git-cat-file),
[Git ls-files](https://git-scm.com/docs/git-ls-files), [Git
grep](https://git-scm.com/docs/git-grep), [Google Shell Style
Guide](https://google.github.io/styleguide/shellguide.html), [BashFAQ
001](https://mywiki.wooledge.org/BashFAQ/001), [performance
plugin](https://github.com/melodic-software/claude-code-plugins/tree/main/plugins/performance),
[yq file
operators](https://mikefarah.gitbook.io/yq/operators/file-operators).
<!-- CURSOR_AGENT_PR_BODY_END -->

<div><a
href="https://cursor.com/agents/bc-c3a723a2-54d9-4964-854f-cda78e9fcba6?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-web-light.png"><img
alt="Open in Web" width="114" height="28"
src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a>&nbsp;<a
href="https://cursor.com/background-agent?bcId=bc-c3a723a2-54d9-4964-854f-cda78e9fcba6&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img
alt="Open in Cursor" width="131" height="28"
src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a>&nbsp;</div>

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.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.

perf(distribution): sync-manifest.sh spends 276 process spawns on Git index and control-character checks

2 participants