Skip to content

refactor(hooks): share one telemetry-sink test helper across the 14 inline suites - #5373

Merged
kyle-sexton merged 9 commits into
mainfrom
refactor/3412-shared-sink-test-helper
Sep 29, 2026
Merged

kyle-sexton merged 9 commits into
mainfrom
refactor/3412-shared-sink-test-helper

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Closes #3412

Summary

Fourteen hook test suites in 12 plugins each defined their own make_sink/wait_for_sink pair. They now source one helper, canonical at lib/hook-test-sink.sh and carried byte-identical at plugins/<p>/hooks/hook-test-sink.sh (plugins do not import siblings). A contract suite, lib/hook-test-sink.test.sh, pins the helper's behavior.

Fix

  • Add lib/hook-test-sink.sh and lib/hook-test-sink.test.sh; wire the suite in the hook-utils CI job.
  • Vendor the helper into the 12 carrying plugins and register the cluster in scripts/cross-plugin-source-registry.txt, gated by check-cross-plugin-source-drift.sh.
  • Switch the 14 suites to the helper; no assertion added or removed.
  • Amend docs/conventions/shell-test-helpers/README.md to the decided rule.
  • Patch-bump the 12 plugins with changelog entries (test-only, no behavior change).

Verification

  • All 14 suites and lib/hook-test-sink.test.sh pass with FAIL=0 and unchanged assertion counts, after merging current main.
  • check-cross-plugin-source-drift.sh --check, check-changelog-parity.sh --check and --check-bump origin/main pass.
  • No ok/bad/fail line is deleted in the suite diffs.
  • Earlier tasks: shellcheck and shfmt clean, lane coverage clean, mutation of the helper body to a no-op fails the positive-telemetry assertions.
  • Not run: test-windows.yml (Git Bash env -i cannot be verified here).

Related

#3410, #3408, #3411, #5313

🤖 Generated with Claude Code

kyle-sexton and others added 9 commits September 29, 2026 13:35
…ct suite

Refs #3412

lib/hook-test-sink.sh holds the make_sink and wait_for_sink pair that 14
hook suites each define inline, with the real HOOK_TELEMETRY_SINK contract
in its header: one executable path, never a command with arguments. make_sink
returns 2 with a message when $WORK is not a directory. wait_for_sink polls
in 20ms steps and returns 1 when the tries bound runs out.

lib/hook-test-sink.test.sh covers the contract: a single-path stub that
captures stdin, a sink that never fires failing the wait within the bound,
a command-with-args sink delivering nothing through hook::emit_telemetry
while the stub delivers, and the tries bound. The suite runs in the
hook-utils lane beside lib/hook-utils.test.sh.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ng plugins

Refs #3412

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each suite drops its own make_sink/wait_for_sink pair (and the autonomy
wait_sink alias) and sources the sibling hook-test-sink.sh. File-capturing
call sites now pass an explicit body (`cat >"$FILE"`, append form in the
autonomy suite); lib/hook-utils.test.sh passes its 750-poll bound at every
wait_for_sink call and now owns a WORK directory it removes on exit, so its
sink stubs no longer accumulate in TMPDIR.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ll-test-helpers

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Test-only change: the suites use the shared telemetry-sink test helper
vendored from lib/hook-test-sink.sh. No behavior change.

Refs #3412

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review September 29, 2026 18:41
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-29T18:45:51.102166Z 491fd54 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review

  • Gather context (PR diff, changed files)
  • Check for security-relevant surface / skip gate
  • Review shell helper scripts and CI wiring for injection/logic issues
  • Check instruction-surface deletions (docs/rules changes)
  • Post findings

No security issues found.

This PR is a pure test-infrastructure refactor: it extracts the make_sink/wait_for_sink telemetry-sink stub duplicated across 14 hook test suites into one canonical helper (lib/hook-test-sink.sh), vendored byte-identically into each carrying plugin and gated by check-cross-plugin-source-drift.sh. Reviewed:

  • lib/hook-test-sink.sh — the sink body passed to make_sink is always a static string literal supplied by trusted test code (never derived from untrusted/external input), so there's no injection path through the generated stub script. make_sink refuses to write outside $WORK when $WORK is unset, not a directory, or a plain file — correctly scoped to the suite's own temp dir.
  • Verified the vendored copies (plugins/actionlint, plugins/autonomy, plugins/markdown-format, plugins/typos-format, plugins/context-guard) are byte-identical to the canonical lib/hook-test-sink.sh.
  • Diffed each converted suite (e.g. plugins/actionlint/hooks/actionlint-check.test.sh, lib/hook-utils.test.sh) — inline make_sink/wait_for_sink definitions are replaced with a source, no assertions were added or removed, consistent with the PR's stated verification.
  • .github/workflows/ci.yml change just adds bash lib/hook-test-sink.test.sh to an existing test step; no permission, trigger, or secret exposure change.
  • docs/conventions/shell-test-helpers/README.md restructuring documents the new vendoring shape; it doesn't remove or weaken a guardrail, block-list, or standing instruction — checked against the instruction-surface-deletion lens and it doesn't apply (this is a convention doc describing test-helper duplication policy, not an enforcement rule).
  • .claude-plugin/plugin.json / CHANGELOG.md changes across the 12 plugins are routine patch-version bumps with test-only changelog entries.

No injection, authorization, credential-handling, or trust-boundary concerns identified in this diff.

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 25s —— View job


Code review complete

  • Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
  • Understand the request (CI code-review lane via /review:code-review)
  • Review lib/hook-test-sink.sh and lib/hook-test-sink.test.sh
  • Review vendored copies across the 12 plugins for drift
  • Review the 14 updated test suites for assertion parity
  • Review CI wiring, registry, and docs changes
  • Post findings (inline + summary)

No blocking findings. This is a clean, well-executed mechanical refactor.

Verified independently (not just author-claimed):

  • All 12 vendored plugins/*/hooks/hook-test-sink.sh copies are byte-identical to lib/hook-test-sink.sh (checked via md5sum).
  • In every converted suite, $WORK/$HOOK_DIR (or $SCRIPT_DIR) is established before hook-test-sink.sh is sourced and before make_sink is first called, so the new helper's $WORK-must-be-a-directory guard never trips a suite that used to work.
  • Where an inline wait_for_sink had a non-default tries bound (lib/hook-utils.test.sh at 750), the conversion threads it explicitly via SINK_POLLS at every call site rather than silently reverting to the shared default of 150 — no behavior change there.
  • Every other converted suite's inline default was already 150, matching the shared helper's default, so no timing regression.
  • scripts/cross-plugin-source-registry.txt's two new lines follow the exact pattern already used for hooks/hook-utils.sh / lib/hook-utils.sh -> plugins/*/hooks/hook-utils.sh (plain path for the drift script, canonical -> glob cluster line for the duplication audit).
  • scripts/sync-hook-utils.sh is untouched and stays scoped to hook-utils.sh only, consistent with the PR's claim that hook-test-sink.sh drift is covered by the generic check-cross-plugin-source-drift.sh + registry instead.
  • docs/conventions/shell-test-helpers/README.md accurately reflects the new state (retires the "sanctioned inline copies" section, adds "The telemetry-sink helper" section, keeps guardrails/claude-ops correctly described as still-separate plugin-local helpers with a different contract).
  • powershell-format's and context-guard's conversions preserve the exact stub bodies the old inline make_sink produced (verified by diffing the generated script text), so behavior is unchanged even though the call sites now pass the body as an argument to the shared function instead of hardcoding it.

Two things noted but not worth blocking on:

  • plugins/context-guard/hooks/{post-compact-mark,zone-crossing-inject}.test.sh previously built the sink body with printf 'cat >%q\n' "$1" (shell-safe quoting of the capture-file path); the converted call sites now interpolate the path directly ("cat >\"$TEL\""). Harmless in practice since these paths are always $WORK-derived mktemp paths with no special characters, but it's a slightly less defensive pattern than before.
  • The CI step bash lib/hook-utils.test.sh && bash lib/hook-test-sink.test.sh (via GitHub Actions' default set -e bash shell) means a failure in the first suite skips the second entirely rather than running both and reporting jointly. Matches how the rest of this job's steps already behave; not new to this PR.

Not independently verified here (sandbox denied running bash/shellcheck directly): the PR's suite-pass counts, shellcheck/shfmt cleanliness, and mutation-testing claims in the Verification section — these are author-claimed only.

Branch: refactor/3412-shared-sink-test-helper

🤖 Generated with Claude Code

@kyle-sexton
kyle-sexton merged commit 116549e into main Sep 29, 2026
29 of 32 checks passed
@kyle-sexton
kyle-sexton deleted the refactor/3412-shared-sink-test-helper branch September 29, 2026 19:34
kyle-sexton added a commit that referenced this pull request Sep 30, 2026
Closes #3410

## Summary

`envelope.schema.json` had no executing consumer, and eleven hook test
suites each carried a hand-transcribed copy of its required-field list
(12 loops). This makes the schema the single source of the field list
and replaces every copy with one shared check.

## Fix

- `lib/hook-test-sink.sh` (canonical, vendored byte-identical to
`plugins/*/hooks/hook-test-sink.sh`) gains `check_envelope`, which reads
the required fields and types from
`docs/conventions/hook-telemetry/envelope.schema.json`.
- `lib/hook-test-sink.test.sh` asserts the check is non-vacuous:
deliberately malformed envelopes fail it.
- All 12 transcribed `for field in schema_version ...` loops
(`lib/hook-utils.test.sh` x2 and ten plugin suites) are replaced by one
`check_envelope` assertion each; the per-suite stdout-parity assertions
are untouched.
- Docs (`hook-telemetry`, `shell-test-helpers` READMEs) state that the
schema is executed by `check_envelope`.
- Patch bump and CHANGELOG entry for each of the 12 touched plugins
(test-only, no behavior change).

## Verification

- `lib/hook-test-sink.test.sh`: PASS=57 FAIL=0
- `lib/hook-utils.test.sh`: PASS=576 FAIL=0
- Plugin suites (actionlint, bash-format, biome-format,
desktop-notification, eol-normalizer, go-format, markdown-format,
powershell-format, ruff-format, typos-format): all FAIL=0. ruff-format
failed 1 assertion on one run and passed 61/0 on rerun; not reproduced.
- `scripts/check-changelog-parity.sh --check --check-order`: pass
- `scripts/validate-plugins.sh`: all manifests and catalog validated
- `grep -rn "for field in schema_version"` over `*.sh`: no matches

## Related

- Acceptance criteria met: schema executed by the shared check;
transcribed loops replaced; malformed envelope fails the check.
- Home for shared test code was settled by #3412 (merged in #5373), per
the owner decision on #3410.
- Failure mode motivating the change: #3394.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <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

1 participant