Skip to content

fix(hook-utils): floor stdin_read_timeout below 0.00001 s - #2393

Merged
kyle-sexton merged 3 commits into
mainfrom
cursor/hook-utils-read-timeout-floor-63e1
Aug 12, 2026
Merged

fix(hook-utils): floor stdin_read_timeout below 0.00001 s#2393
kyle-sexton merged 3 commits into
mainfrom
cursor/hook-utils-read-timeout-floor-63e1

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #1883.

hook::resolve_read_timeout now refuses positive values below 0.00001 s (same silent-disable class as exact zero). Synced across all 16 carrying plugins; versions bumped.

Verification

  • lib/hook-utils.test.sh: PASS=185 FAIL=0 (includes sub-minimum regression rows)

Related

@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
Contributor

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


Security Review

  • Gather context (PR diff, changed files)
  • Review lib/hook-utils.sh timeout-flooring logic
  • Review synced per-plugin hooks/hook-utils.sh copies
  • Review plugin.json version bumps / manifest changes
  • Review test file changes
  • Post final findings

No security issues found in this PR.

Scope reviewed: lib/hook-utils.sh, its 16 synced plugins/*/hooks/hook-utils.sh copies, lib/hook-utils.test.sh, the 16 plugin.json version bumps, and plugins/guardrails/CHANGELOG.md.

Reasoning:

  • The new floor logic in hook::resolve_read_timeout only tightens an existing fail-closed path (sub-minimum values now degrade to the default 2, same as the pre-existing zero/invalid handling) — it doesn't add a new attacker-reachable code path.
  • whole/frac are extracted from BASH_REMATCH after the value already matched ^[0-9]+(\.[0-9]+)?$, so they're guaranteed pure digit strings before being used in the $(( )) arithmetic expansion — no injection surface, and the 10# base prefix correctly avoids the classic bash octal-parsing pitfall for values with leading zeros (e.g. 010).
  • CLAUDE_PLUGIN_OPTION_STDIN_READ_TIMEOUT is a plugin-config option (trusted local/managed settings), not attacker-supplied network input, so this isn't a trust-boundary crossing even before considering the regex guard.
  • Verified all 16 plugins/*/hooks/hook-utils.sh copies are byte-identical to lib/hook-utils.sh (matching blob SHA 98e7bc7... across every one) — no divergent or backdoored copy was slipped into a single plugin under cover of the bulk sync.
  • The plugin.json diffs are version bumps only (plus incidental JSON re-serialization in unrelated description text); no permissions, hook registrations, or other manifest capabilities changed. plugins/guardrails/CHANGELOG.md is a documentation-only addition.
  • No GitHub Actions workflow files are touched by this PR.

@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: 0f4d5379e1

ℹ️ 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/actionlint/.claude-plugin/plugin.json Outdated
@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 8m 14s —— View job


Code Review

  • Gather context (CLAUDE.md, REVIEW.md, PR diff)
  • Review lib/hook-utils.sh core logic change
  • Review lib/hook-utils.test.sh new tests
  • Review consistency across 16 plugin copies of hook-utils.sh
  • Review plugin.json version bumps
  • Post findings

Summary

The core fix in lib/hook-utils.sh:561-584 is sound. I traced the fractional-to-microsecond conversion by hand against the boundary cases (0.00001 honored, 0.000001/0.0000001 floored) and it's correct: truncating the fractional string to 6 digits before comparing to HOOK_STDIN_READ_TIMEOUT_MIN_MICROS=10 never changes which side of the 10-µs floor a value falls on, since truncation only discards sub-microsecond precision. I also diffed all 16 plugins/*/hooks/hook-utils.sh copies against lib/hook-utils.sh directly — byte-identical, no sync drift — and confirmed exactly the 16 plugins that carry the file (plugins/*/hooks/hook-utils.sh) got a manifest version bump, satisfying scripts/sync-hook-utils.sh --check-bump.

🔴 Important — this PR will fail the required check-changelog-parity.sh --check-bump CI gate (wired at .github/workflows/ci.yml:529). That gate requires every plugin whose manifest version changed to add a matching ## [<version>] entry to its own CHANGELOG.md in the same change set. Of the 17 plugin.json version bumps in this PR, only guardrails (0.28.1 → 0.28.2) added one. The other 16 did not — each CHANGELOG's newest heading is still the previous version:

The gate's baseline (scripts/changelog-parity-baseline.txt) grandfathers missing changelogs for --check but explicitly never relaxes --check-bump, so none of these are exempt. Each needs a ## [<new-version>] entry (guardrails's new entry is a good template to copy across the fleet).

Inline findings posted:

  1. 🔴 plugins/architecture/.claude-plugin/plugin.json — unrelated version bump swept into this PR (architecture doesn't carry hook-utils.sh).
  2. 🟡 lib/hook-utils.sh:577 — unbounded digit count into the microsecond arithmetic; possible overflow on a pathological config value, worth confirming the earlier probe closes it off.
  3. 🟡 plugins/autonomy/.claude-plugin/plugin.json:28 (+3 more spots there, +2 in guardrails) — incidental em-dash→ re-encoding noise, apparently from the version-bump tooling round-tripping the whole JSON file through a serializer, inconsistent with how the other 14 bumped manifests were touched.

Verification note: I independently diffed all 16 hook-utils.sh copies (identical) and hand-traced the floor arithmetic against the stated boundary values. I was not able to execute lib/hook-utils.test.sh myself in this sandbox (the run required an approval that wasn't available), so the PR description's "PASS=185 FAIL=0" is author-claimed and unverified by me.

Security-relevant surfaces (this repo has .github/workflows/claude-security-review.yml) are out of scope for this lane per REVIEW.md.

Branch

Comment thread plugins/architecture/.claude-plugin/plugin.json
Comment thread lib/hook-utils.sh
Comment thread plugins/autonomy/.claude-plugin/plugin.json
@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.

Fixes #1883.

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@cursor
cursor Bot force-pushed the cursor/hook-utils-read-timeout-floor-63e1 branch from 0f4d537 to 02f3060 Compare August 12, 2026 09:10
@cursor
cursor Bot force-pushed the cursor/hook-utils-read-timeout-floor-63e1 branch from 02f3060 to 1bcdefa Compare August 12, 2026 09:10
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Re-run the job to retry the review. A new push re-triggers this lane only if the caller's pull_request triggers include synchronize (the canonical caller omits it).
An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator (auth).

@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging.

Re-run the job to retry the review; a new push also retries it only if the caller's pull_request triggers include synchronize (the canonical security caller keeps it). An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator.

Re-running does NOT help for every class:

  • rate-limit that persists across re-runs, or auth — the credential or usage budget needs an operator; retrying will not clear it.
  • a run that exhausted its turn budget ("subtype":"error_max_turns" above) will exhaust it again. As the PR author, split the change into smaller PRs; raising --max-turns is a change to the caller workflow, not something you can set on this PR.

@kyle-sexton
kyle-sexton merged commit f3f29e2 into main Aug 12, 2026
34 checks passed
@kyle-sexton
kyle-sexton deleted the cursor/hook-utils-read-timeout-floor-63e1 branch August 12, 2026 09: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.

hook-utils: a repo env block can disable every hook by starving the stdin read timeout

2 participants