Skip to content

perf(context-guard): skip state writes that would not change the marker - #3736

Merged
kyle-sexton merged 8 commits into
mainfrom
claude/prompt-hooks-review-cf00b1
Sep 5, 2026
Merged

kyle-sexton merged 8 commits into
mainfrom
claude/prompt-hooks-review-cf00b1

Conversation

@kyle-sexton

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

Copy link
Copy Markdown
Contributor

No related issue: the defect was found by an in-session fleet audit that had no tracking issue; the audit's findings on other lanes are tracked in #3685, #3721 and #3727.

Summary

zone-crossing-inject.sh fires once per UserPromptSubmit and once per PostToolBatch, so a three-batch turn that stays in one zone fired four times and rewrote both state markers four times to the values they already held. The armed-rank gate already suppressed the extra notices, so this was wasted I/O rather than duplicate injection. Alongside it, hooks.json lacked its documented top-level description.

Fix

  • Each marker is written only when its raw on-disk bytes differ from the new value. The comparand is the read before normalization, so a legacy-format marker is still rewritten in the current format and an absent marker still latches on the first fire. Write ordering, the fail-open posture and the .zone rollback on a failed .armed write are unchanged; the rollback now fires only when this call was the one that moved .zone.
  • A new contract-test case (12c) pins the skip. Every existing assertion reads stdout, which is identical whether the markers were rewritten or skipped, so a revert to unconditional writes would have passed all 58. The new case backdates both markers to 2000-01-01 and a reference file to 2000-01-02, fires on an unchanged zone, and asserts neither marker is newer than the reference, with a positive control that a mismatched marker is still rewritten. bash -nt keeps it free of any stat(1) dialect, and mtimes are not subject to root's permission bypass, which is why it is not a chmod test.
  • hooks.json gains a top-level description.
  • context-guard bumps to 0.7.42 with a CHANGELOG entry. Main shipped 0.7.39 in feat(session-flow): shape-2 handoff save-points with the resume prompt on screen and in the file #3718, 0.7.40 in fix(hooks): PreToolUse guard remediation — scratch-root default, kill-switch hoist, MCP coverage, two conventions #3727 and 0.7.41 in perf(hook-utils): drop buffer_stdin startup subshells, repo_root tr, and emit_tel jq #3732 while this branch was open; each merge placed this entry above the newer one under its own version, so consumers on any of them still receive this change.

Withdrawn in review. The draft also dropped the four hooks.json timeouts from 60 to 10, reasoning from the hooks reference's 30-second UserPromptSubmit default and a 30 ms quiet-host measurement. The Codex review on 5acee106 pointed at the 0.4.8 CHANGELOG entry and the README paragraph that own that value: on Windows 11 / Git Bash with Defender real-time protection, zone-crossing-inject.sh measured 22.0 s, and a 10-second cap was the exact defect 0.4.8 fixed, cancelling the advisory on essentially every fire while still charging the full wall time. Nothing on this branch re-measured that profile, so 3db2c457 restores all four rows to 60 and the CHANGELOG entry says why they stay.

Five files: zone-crossing-inject.sh, zone-crossing-inject.test.sh, hooks.json, plugin.json, CHANGELOG.md, all under plugins/context-guard/. The interview Brief that scoped this lived in docs/topics/prompt-hooks-review/ on the branch and is pruned before merge per the topic-docs convention; its substance is this body, and the audit's measured baseline is routed to #3685.

Verification

  • shellcheck clean on the changed script and the test, plain and with the repo .shellcheckrc; check-shell-portability.sh origin/main clean on both.
  • plugins/context-guard/hooks/zone-crossing-inject.test.sh: 58 pass, 0 fail at every head through 3db2c457; 63 pass, 0 fail on 860d66d6 with case 12c. Against origin/main's hook the suite reads 61 pass, 2 fail, the two failures being exactly the skip assertions. The first attempt at the fix failed one case (0.7.1-format marker not normalized): comparing against the normalized read skipped the rewrite of a legacy marker. Fixed by comparing against the raw read.
  • scripts/affected-tests.sh --run: all selected suites pass at every head (26 before the first merge, 27 on f7cb502f, 2 on the pruned head 511b549f once the Brief stopped driving reference-based selection, 2 on fcc3f8cd, all selected on 5acee106, 2 on 3db2c457, 3 on 860d66d6), zero FAIL lines. A mid-merge run had reported three failures in guardrails, session-flow and source-control; on a clean origin/main worktree the guardrails suite passes 213/0 and the session-flow failure reproduces identically, and this branch touches none of those plugins.
  • On fcc3f8cd, after merging the fix(hooks): PreToolUse guard remediation — scratch-root default, kill-switch hoist, MCP coverage, two conventions #3727 squash: the three context-guard suites pass 18/58/25, and main's new scripts/check-killswitch-hoist.sh reports 15 of 15 guards clean.
  • On 5acee106, after merging the perf(hook-utils): drop buffer_stdin startup subshells, repo_root tr, and emit_tel jq #3732 squash (resolved by the babysit lane, see its comment on this PR): the three context-guard suites pass 18/58/25 again, scripts/sync-hook-utils.sh --check reports all 17 copies matching, and the diff against main is still exactly the files above.
  • On 3db2c457: the three context-guard suites pass 18/58/25, markdownlint clean on the CHANGELOG, hooks.json parses and every timeout reads 60 again.
  • scripts/check-changelog-parity.sh --check-bump origin/main, --check-order, --check-preserved origin/main: all exit 0 on every head.
  • scripts/check-contract-slice-prune.sh --check-diff origin/main: passes on 511b549f. This was the one failing sub-gate of 56 on the lint lane at f7cb502f (contract-slice-diff); every other hygiene sub-gate was already green there.
  • Review lanes on 5acee106: security-review reported no findings; the Claude review lane found no blocking issue and raised the missing skip test, addressed in 860d66d6; the Codex review raised the timeout finding above, addressed in 3db2c457.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01ECoW7mi7Zod61utjdyWtRE

Persist the /planning:interview contract for the prompt-hooks review at
docs/topics/prompt-hooks-review/PLAN.md, the contract tier per
docs/conventions/topic-docs (committed on the task branch only, pruned
before merge).

Seven register rows, all answered. Scope locks to the two files this lane
owns: plugins/context-guard/hooks/hooks.json and zone-crossing-inject.sh.
Everything larger the fleet audit found is recorded as out-of-scope with
its receiving lane named.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ECoW7mi7Zod61utjdyWtRE
zone-crossing-inject.sh fires once per UserPromptSubmit and once per
PostToolBatch, so a three-batch turn in one zone rewrote both state
markers four times to the values they already held. Each marker is now
written only when its raw on-disk bytes differ from the new value. The
raw read, before normalization, is the comparand, so a legacy-format
marker is still rewritten in the current format and an absent marker
still latches on the first fire. Write ordering, the fail-open posture
and the .zone rollback on a failed .armed write are unchanged; the
rollback now fires only when this call was the one that moved .zone.

hooks.json: all four rows drop timeout from 60 to 10 (the hooks
reference lowers the UserPromptSubmit command default to 30; the
scripts measure at roughly 30 ms), and the file gains its documented
top-level description.

The Brief gains a measured-baseline table from the fleet audit, holding
only the timings peer review left standing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ECoW7mi7Zod61utjdyWtRE
…eview-cf00b1

# Conflicts:
#	plugins/context-guard/CHANGELOG.md
docs/topics/<slug>/ is contract tier per the topic-docs convention:
committed on the task branch, pruned before merge, with durable outcomes
graduated first. The Brief's substance is the PR body; the measured
baseline table's numbers are routed to #3685 as evidence. The
contract-slice-diff hygiene gate was the one failing sub-gate of 56 on
the lint lane.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ECoW7mi7Zod61utjdyWtRE
…eview-cf00b1

# Conflicts:
#	plugins/context-guard/CHANGELOG.md

Copy link
Copy Markdown
Contributor Author

Babysit lane claim pinned to head fcc3f8cd2609813de1b00e586037ffa4ce06baa9.

Observed at claim time: draft, mergeable: false, mergeable_state: dirty (real merge conflict against main), last branch activity 2026-09-05T02:36:56Z. No competing lane claim present.

Scope of this claim is conflict resolution only. Draft promotion is a separate decision and is not part of this intervention. Claim expires at the timestamp above.


Generated by Claude Code

…eview-cf00b1

Resolves the context-guard CHANGELOG conflict. Both sides independently
claimed 0.7.41: main's #3732 shipped the vendored hook-utils.sh
buffer_stdin/repo_root change under that version, and this branch's
027d11a claimed it for the marker-write skip. Main's 0.7.41 is already
released, so it keeps the version it shipped under and this branch's entry
moves up to 0.7.42, with plugin.json bumped to match. Both entries survive,
per the branch's own stated convention that each merge places this entry
above the newer one under its own version.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VPLatLkg4329L8eyfxhuMa

Copy link
Copy Markdown
Contributor Author

Merge conflict resolved — still draft, promotion deferred

Lane babysit-loop, instance ccr-session-babysit-loop-20260905.

What was conflicting

One file: plugins/context-guard/CHANGELOG.md. Both sides independently claimed 0.7.41.

plugin.json did not conflict, because both sides wrote the identical string "0.7.41" — so a naive resolution would have left the manifest silently claiming a version that main already shipped with different contents.

How it was resolved

main's 0.7.41 is already released, so it keeps the version it shipped under. This branch's entry moves up to 0.7.42, and plugin.json is bumped to match. Both entries survive, which is the convention this PR's own body states ("each merge placed this entry above the newer one under its own version, so consumers on either still receive this change").

Merge commit 5acee106, first parent fcc3f8cd (the pinned PR head), second parent 3ea592bb (main). Fast-forward push, no rebase, no force-push, no rewriting of main's history.

Verification on the resolved head

  • scripts/check-changelog-parity.sh --check-order — 89 changelogs newest-first, no duplicate versions.
  • --check-bump origin/main and --check-preserved origin/main — both exit 0; the preserved check confirms main's 0.7.41 heading survived (65 headings compared).
  • scripts/affected-tests.sh --run — all selected suites pass.
  • plugins/context-guard/hooks/zone-crossing-inject.test.sh — 58 pass, 0 fail.
  • GitHub checks on 5acee106: all green (ci-status, lint, test-linux, hook-utils, changes, managed-files-guard, pr-issue-linkage, pr-title, do-not-merge, GitGuardian; test-windows skipped). mergeable_state moved dirtyblocked, i.e. the draft gate rather than a conflict.

Why this is still a draft

Conflict resolution and draft promotion are separate decisions, and two things block a confident promotion:

  1. The PR body is now stale on the one fact this resolution changed. It says "context-guard bumps to 0.7.41 ... Main shipped 0.7.39 in feat(session-flow): shape-2 handoff save-points with the resume prompt on screen and in the file #3718 and 0.7.40 in fix(hooks): PreToolUse guard remediation — scratch-root default, kill-switch hoist, MCP coverage, two conventions #3727 while this branch was open". It now ships 0.7.42, and the entry it sits above is main's 0.7.41 from perf(hook-utils): drop buffer_stdin startup subshells, repo_root tr, and emit_tel jq #3732, which the body does not mention. That paragraph needs correcting before this goes to review.
  2. Readiness is not gate-proven. This repo's AI review lanes skip while a PR is draft, so claude-review and claude-security-review have never run here — the flip to ready is what asks for them. Separately, the babysit merge gate could not be run from this session at all (see the telemetry issue), so no gate verdict exists to quote, and live check state is not a substitute for one.

Nothing in the diff itself looks incomplete — no WIP markers, and the verification section is thorough. The hold is the stale body plus the absent review evidence, not the code.

Lane claim released.


Generated by Claude Code

@kyle-sexton
kyle-sexton marked this pull request as ready for review September 5, 2026 08:20
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 5, 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-05T08:23:36.445222Z 5acee10 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 5, 2026

Copy link
Copy Markdown
Contributor

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


Claude finished reviewing this PR

  • Gather context (diff, changed files, repo conventions)
  • Invoke /review:code-review skill and follow its instructions
  • Post inline findings (if any)
  • Post final summary

Review summary

Scope: plugins/context-guard/{hooks/zone-crossing-inject.sh,hooks/hooks.json,.claude-plugin/plugin.json,CHANGELOG.md} against origin/main.

I traced the write-skip logic in zone-crossing-inject.sh (raw-vs-normalized comparands, the wrote_zone rollback guard, the legacy two-field .armed marker case) line by line and didn't find a correctness bug:

  • zone_on_disk/armed_on_disk are captured from the raw pre-normalization read, so a legacy-format or corrupted marker always differs from the freshly computed value and gets rewritten — confirmed against the existing "0.7.1-format marker" test case (zone-crossing-inject.test.sh:306-329), which predates this PR and already exercises exactly this path.
  • The emit-gate ranks (new_rank/armed_rank) are computed before the write-skip block and are untouched by it, so the skip is purely an I/O optimization with no effect on when a notice fires.
  • The wrote_zone guard correctly narrows the .zone rollback-on-.armed-failure to only the calls that actually moved .zone this fire; test 12b (zone-crossing-inject.test.sh:491-532) exercises this exact interaction (skip-eligible call where .armed's write fails) and passes.
  • hooks.json: all four rows now carry timeout: 10, in line with several other plugins in this marketplace (rate-limit-guard, instruction-placement both use 10), and neither zone-gate.sh nor post-compact-mark.sh do anything (network calls, sleeps) that would need a longer budget. The added top-level description matches every other plugin's hooks.json in the repo.
  • CHANGELOG.md/plugin.json: version ordering is correct (0.7.42 sits above 0.7.41, no duplicate versions), consistent with the babysit-lane conflict resolution described in the PR comments.

One non-blocking observation, not tied to a changed line so it's here rather than inline: the write-skip is the entire point of this PR, but no test asserts the write is actually skipped (e.g., by making the state dir read-only after seeding a marker with the current value and confirming the hook still exits silently with no error — which would fail under the pre-PR code that unconditionally attempts the write). The existing 58 tests all assert externally-observable behavior (silence/injection), which passes identically whether or not the skip fires, so a future regression that quietly reverted to always-writing would go undetected. See the skip-write block itself:

persist_failed=""
wrote_zone=""
if [[ "$zone" != "$zone_on_disk" ]]; then
if printf '%s\n' "$zone" >"$STATE_FILE" 2>/dev/null; then
wrote_zone=1
else
persist_failed="zone"
fi
fi
if [[ -z "$persist_failed" && "$next_armed" != "$armed_on_disk" ]] &&
! printf '%s\n' "$next_armed" >"$ARMED_FILE" 2>/dev/null; then
persist_failed="armed"
# Roll the label back to what it said before, so the message the next
# successful call emits names the zone the session was really in rather than
# the one it is in now. Best-effort: if the rollback itself fails the label is
# stale, which mislabels one message — never a lost or repeated notice,
# because the gate did not move either way.
if [[ -n "$wrote_zone" ]]; then
if [[ -n "$last" ]]; then
printf '%s\n' "$last" >"$STATE_FILE" 2>/dev/null || :
else
rm -f "$STATE_FILE" 2>/dev/null || :
fi
fi
fi

Nothing here rises to a blocking finding. No inline comments posted.

@kyle-sexton
kyle-sexton enabled auto-merge (squash) September 5, 2026 08:20
@claude

claude Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

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


Security review complete ✅

  • Gather context (PR body, diff, skip-gate check)
  • Review changed files for security-relevant surface
  • Report findings (or clean bill)

No security issues found.

This PR is a pure performance/metadata change scoped to plugins/context-guard/:

  • zone-crossing-inject.sh: adds a byte-comparison before writing .zone/.armed state markers to skip redundant writes. The compared values (zone, next_armed, zone_on_disk, armed_on_disk) are all internally computed or read from the hook's own pre-existing state files — no new external/untrusted input is introduced, no new file paths, no change to STATE_FILE/ARMED_FILE construction. The fail-open posture, write ordering (.zone before .armed), and rollback-on-failure logic are preserved; the only rollback change is gating it on whether this call moved .zone, which is a correctness fix, not a security-relevant one.
  • hooks.json: lowers timeout from 60→10 on all four hook entries and adds a description field. Tightening a timeout reduces (not increases) the window an advisory hook can be stalled/held.
  • plugin.json / CHANGELOG.md: version bump and changelog entry only.

No injection surfaces, no authorization/access-control changes, no secrets/token handling, no GitHub Actions workflow changes, and no permission widening in this diff. Per the skip gate in /review:security-review, this lane's scope (logic/trust-boundary/Actions security) has no applicable surface here.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

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

@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: 5acee10636

ℹ️ 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/context-guard/hooks/hooks.json Outdated
The draft dropped every hooks.json timeout from 60 to 10 on the
strength of the hooks reference's 30-second UserPromptSubmit default
and a 30 ms quiet-host measurement. That reintroduced the defect the
0.4.8 entry fixed: on Windows 11 / Git Bash with Defender real-time
protection, zone-crossing-inject.sh measured 22.0 s, so a 10-second cap
cancels the advisory on essentially every fire while still charging the
full wall time. The README sizes the cap from that measurement and
holds it at 60 until the profile is re-measured. Nothing in this branch
re-measured it. Flagged by the Codex review on 5acee10.

The state-write skip and the top-level description are unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ECoW7mi7Zod61utjdyWtRE
@github-actions

github-actions Bot commented Sep 5, 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.

The review lane observed that every assertion in the suite reads
stdout, which is identical whether the state markers were rewritten or
skipped, so a revert to unconditional writes would pass all 58. Case
12c backdates both markers to 2000-01-01 and a reference file to
2000-01-02, fires the hook on an unchanged zone, and asserts neither
marker is newer than the reference; a positive control confirms a
mismatched marker is still rewritten. bash -nt keeps it free of any
stat(1) dialect, and mtimes are not subject to root's permission
bypass, which is why it is not a chmod test. Against the previous hook
exactly the two skip assertions fail.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ECoW7mi7Zod61utjdyWtRE
@kyle-sexton
kyle-sexton merged commit c1c4f32 into main Sep 5, 2026
20 checks passed
@kyle-sexton
kyle-sexton deleted the claude/prompt-hooks-review-cf00b1 branch September 5, 2026 08:34
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