Skip to content

docs(claude-ops): state the sink's real route in its header - #3796

Merged
kyle-sexton merged 3 commits into
mainfrom
claude/posttool-hooks-review-ji6rl5
Sep 6, 2026
Merged

kyle-sexton merged 3 commits into
mainfrom
claude/posttool-hooks-review-ji6rl5

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

No related issue: a documentation inaccuracy found by a post-merge verification of this topic's four PRs, small enough to fix directly rather than file.

Summary

#3769 (claude-ops 0.42.11) moved the reference sink's routing to the envelope spine, .session_id // .data.session_id, and left the file header saying the route is "decided by the envelope's data.session_id". That header is the sink's only description of its own routing rule, so a reader of the file got the pre-1.1 answer. docs/conventions/hook-telemetry/README.md states it correctly, which is what makes this an oversight rather than a disagreement about intent.

Fix

The header now names the spine key first and the data.session_id fallback second, matching line 62, and states the spine key as optional: contract 1.1 has a producer carry session_id when its payload held a well-formed one and omit it otherwise, in which case this sink takes the legacy route. Applied to both copies, plugins/claude-ops/hooks/hook-telemetry-sink.sh and the repo-local .claude/hooks/hook-telemetry-sink.sh.

Those two files are not byte-identical and are not meant to be — the repo-local copy reaches hook-utils.sh and session-log-lib.sh by relative path because they are not colocated with it, and says so in its own comment. The stale header had drifted identically in both, so both needed the same correction.

Comment only. No behavior change. claude-ops 0.42.16 to 0.42.17.

Review findings, both addressed

Verification

  • Comment-only proven three ways rather than asserted: comment-and-blank-stripped executable bodies compare byte-identical; shfmt -mn (a real shell parser) gives matching SHA-256 per file pair across base and head; every one of the 8 changed lines begins with #. File modes unchanged.
  • Behavior driven, not inferred: the sink itself run 8 times across {base, head} × {plugin copy, repo-local copy} × {spine-only envelope, data-only envelope}, each into a fresh project dir. Every base/head pair produced a byte-identical JSONL line and the same route.
  • plugins/claude-ops/hooks/hook-telemetry-sink.test.sh: PASS=41 FAIL=0. scripts/check-changelog-parity.sh --check-bump origin/main, --check, --check-preserved origin/main, scripts/validate-plugins.sh, scripts/sync-hook-utils.sh --check, scripts/check-hook-wiring-liveness.sh: all pass. shellcheck clean on both files.
  • scripts/affected-tests.sh --run: only pre-existing failures, cache-content-check.test.sh's two "process budget" cases, which reproduce on unmodified main.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme

0.42.11 moved the reference sink's routing to the envelope spine
(`.session_id // .data.session_id`) and left the header saying the route is
"decided by the envelope's `data.session_id`". That header is the file's only
description of the rule, so a reader of the sink got the pre-1.1 answer while
docs/conventions/hook-telemetry/README.md gave the right one.

Both copies carry it: the plugin's and the repo-local .claude/hooks/ one. They
differ only in how they reach their shared sources, by design, so the header
had drifted identically in both.

Comment only. hook-telemetry-sink.test.sh 41/41, shellcheck clean.

Found by a fresh-context verifier auditing what actually shipped on main after
the four PRs of this topic landed together.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
@kyle-sexton
kyle-sexton marked this pull request as ready for review September 6, 2026 06:52
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 6, 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-06T06:57:43.252907Z a87ea83 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 6, 2026

Copy link
Copy Markdown
Contributor

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


/review:code-review — code-review lane

  • Gather context (PR diff, CLAUDE.md, skill instructions)
  • Run /review:code-review skip gate checks
  • Review changed files
  • Post findings

Skip gate: change is trivial with no meaningful review surface. All four
changed files are comment/metadata only:

  • plugins/claude-ops/hooks/hook-telemetry-sink.sh and .claude/hooks/hook-telemetry-sink.sh: header comment only, no code lines touched.
  • plugins/claude-ops/.claude-plugin/plugin.json: version bump 0.42.150.42.16.
  • plugins/claude-ops/CHANGELOG.md: matching changelog entry.

I checked the corrected header against the actual routing code rather than
taking the PR description's claim at face value: hook-telemetry-sink.sh#L58-L62
does read (.session_id // .data.session_id // ""), so the new header text
("read from the spine ... and falling back to data.session_id") accurately
describes the code, matching docs/conventions/hook-telemetry/README.md:184-193.
The two copies (plugins/claude-ops/hooks/ and .claude/hooks/) carry the
identical header edit while retaining their pre-existing, intentional
source path difference, as the PR body describes.

No correctness, maintainability, or security issues found. No inline comments
posted per the skip gate.

@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

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


Security review

  • Gather context (PR diff, changed files)
  • Run /review:security-review skip-gate check
  • Post result

Skip gate hit: no security-relevant surface.

This PR changes only:

No executable logic, control flow, input handling, or trust boundaries change in either shell script — verified against gh pr diff for this PR. Per this skill's criteria, no security issues to report.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Last security-reviewed head: a87ea83c0dda2e71128f118608a9afdc681370aa. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Claude has reviewed this PR 1 time. 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: a87ea83c0d

ℹ️ 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/hooks/hook-telemetry-sink.sh Outdated
The header this branch just corrected replaced one inaccuracy with a smaller
one: it said `session_id` is "carried by every 1.1 producer". Contract 1.1
makes the four correlation keys OPTIONAL — "copied verbatim from the hook
payload by hook::emit_telemetry when present and well-formed, omitted
otherwise" — so a 1.1 producer whose payload carries no session id, or a
malformed one, puts nothing on the spine and this sink takes the legacy route.

The header now says the producer carries it when its payload held a
well-formed one and omits it otherwise, which is what the emitter does and
what the contract promises. Both copies.

Reported by Codex on this PR.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
…-review-ji6rl5

# Conflicts:
#	plugins/claude-ops/CHANGELOG.md
@kyle-sexton
kyle-sexton merged commit 4929ec4 into main Sep 6, 2026
18 checks passed
@kyle-sexton
kyle-sexton deleted the claude/posttool-hooks-review-ji6rl5 branch September 6, 2026 07:25
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.

2 participants