fix(desktop-notification,markdown-format): assert the real C1 slow-sink invariant, not half the sleep

[kyle-sexton]

Closes #448

Summary

• The #751 fix that closed #448 could not have worked: THRESHOLD_MS was derived as
  SINK_SLEEP * 1000 / 2, so widening SINK_SLEEP widened the threshold by the same ratio and left
  the margin unchanged by construction. #448 was reopened after this reproduced on clean main
  (delta=3697ms false-fail with no leak present), and the same construction was found ported
  verbatim into markdown-format's C1 copy (from #1209).
• THRESHOLD_MS in both plugins' C1 fd1-leak detector now asserts the actual invariant directly —
  sink-sleep-minus-a-safety-margin (SAFETY_MARGIN_MS), not half the sleep — and SINK_SLEEP widens
  from 6s to 8s (still under the 10s ceiling documented against EXIT-cleanup file-locking on Windows)
  for more absolute separation between ambient noise and the leak signal.
• Both constants were sized against measurements taken on this machine: the reported 3697ms
  false-fail, and up to ~2150ms of noise generated by 30 concurrent full-suite runs under heavy load
  — the fixed threshold clears both with comfortable margin while staying meaningfully below the
  ~8000ms leak signal.
• Per-plugin version bump + CHANGELOG entry in both desktop-notification and markdown-format.

Test plan

Windows Git Bash (the platform this flake is specific to — Ubuntu CI spawns ~10x cheaper and was
always unaffected).

• 10 consecutive clean sequential runs of desktop-notification.test.sh — all green
  (PASS=54 FAIL=0), C1 deltas -175ms to 644ms against the new <6000ms threshold.
• 40 runs under heavy concurrent load (30x desktop-notification.test.sh + 10x
  markdown-format.test.sh launched simultaneously) — all green, worst observed no-leak delta
  ~1590ms (desktop-notification) / ~1555ms (markdown-format), no FAIL.
• Deliberately reintroduced the fd1 leak (dropped the >/dev/null redirect on the sink spawn in
  hook::emit_telemetry, plugins/desktop-notification/hooks/hook-utils.sh:455) — C1 correctly
  went red: delta 8065ms ≈ sink's 8s sleep. Reverted (git diff against the reverted file is
  empty — confirmed clean).
• shellcheck clean on both modified test files.
• Full suites green after the fix: desktop-notification PASS=54 FAIL=0,
  markdown-format PASS=65 FAIL=0.

Related

#443 (the hook::buffer_stdin migration that thinned the original margin). #751 (the fix that
attempted to close #448 but could not — see Summary). Epic #313 (closed).

🤖 Generated with Claude Code

[claude]

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

---

Security Review

• Gather PR diff and changed files
• Read each changed file in full
• Analyze for security vulnerabilities (injection, secrets, auth gaps, etc.)
• Post findings

---

No security issues found.

The six changed files are: two plugin.json manifests (version bumps only), two CHANGELOG.md entries (documentation only), and two test scripts with the following narrow changes:

Location	Change
desktop-notification.test.sh:250–251 / markdown-format.test.sh:680–681	SINK_SLEEP 6 → 8; add SAFETY_MARGIN_MS=2000 (integer literals)
desktop-notification.test.sh:274 / markdown-format.test.sh:712	THRESHOLD_MS formula: SINK_SLEEP * 1000 / 2 → SINK_SLEEP * 1000 - SAFETY_MARGIN_MS
Both test files	Comment block expansion explaining the invariant

Injection surfaces: make_sink writes caller-supplied strings verbatim into executable temp scripts. The only new caller site that uses $SINK_SLEEP inside the body string is:

SLOW_SINK="$(make_sink "cat >/dev/null; sleep $SINK_SLEEP")"

$SINK_SLEEP is assigned 8 (integer literal, no external input) immediately above and expands before the string reaches make_sink. Body received: cat >/dev/null; sleep 8. No injection surface.

$SAFETY_MARGIN_MS is used only in $((...)) arithmetic expansion — safe for integer variables set to literal values in the same script.

Other vectors checked: No credentials or secrets involved. No network calls or dependency pins changed. No GitHub Actions files in scope. No authorization logic touched. No path construction using the new variables. Production hook code is entirely untouched — this is test-only.

[claude]

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

---

PR Review

• Gather context
• Read changed files
• Review threshold math correctness
• Review test logic
• Review CHANGELOG / version bumps
• Post findings

---

Summary

This fix is correct, well-reasoned, and adequately tested. The root cause analysis is sound, the arithmetic checks out, and the empirical verification data is solid. Two minor observations below, neither a blocker.

---

Threshold math: correct

The core fix — THRESHOLD_MS=$((SINK_SLEEP * 1000 - SAFETY_MARGIN_MS)) — resolves the structural defect cleanly:

	SINK_SLEEP	THRESHOLD_MS	Leak signal	Clearance above noise (~2150ms)	Clearance below leak (~8065ms)
Old (broken)	6s	3000ms	~6000ms	850ms	3000ms
#751 (if SINK_SLEEP had been bumped)	8s	4000ms	~8000ms	1850ms	4000ms
This PR	8s	6000ms	~8065ms	3850ms	2065ms

The new formula decouples the threshold from the sleep ratio: widening SINK_SLEEP in the future won't silently erode the threshold margin. The 3850ms clearance above the observed noise ceiling (with 30 concurrent runs) is comfortable.

The identical change is correctly applied to the markdown-format copy at markdown-format.test.sh:710.

---

Minor observations (not blocking)

1. SAFETY_MARGIN_MS names the leak-side clearance, not the noise-side clearance

desktop-notification.test.sh, line ~251 — The constant measures the gap between the threshold and the leak signal (how far below the leak signal the threshold sits), not the gap above the noise floor. The actual no-leak safety headroom is THRESHOLD_MS - noise_max ≈ 3850ms, not 2000ms. The name isn't wrong, but a future reader sizing this value might assume it governs the noise headroom. A comment like # gap between threshold and leak signal; see noise measurements above would disambiguate. Not a correctness issue.

2. SAFETY_MARGIN_MS absent from the diagnostic echo

desktop-notification.test.sh, line ~273 — The diagnostic line emits threshold <${THRESHOLD_MS}ms, sink sleeps ${SINK_SLEEP}s but not SAFETY_MARGIN_MS. If the threshold is ever wrong in a future run, the emitted value (6000ms) tells you the threshold but not how it was derived. Including margin=${SAFETY_MARGIN_MS}ms would let you cross-check the formula from the log line alone. Very minor quality-of-life nit.

---

Version bumps and CHANGELOGs

Both correct:

• desktop-notification: 0.5.1 → 0.5.2 — patch bump, appropriate for a test-logic fix
• markdown-format: 0.6.2 → 0.6.3 — patch bump; the CHANGELOG correctly notes "No behavior change for this plugin — the hook is untouched; test-only"

CHANGELOG entries for both are accurate, cite the relevant issue numbers and empirical measurements, and follow the Keep a Changelog format already in use.

---

Verdict: LGTM. The fix is correct and the test verification is thorough. The two observations above are cosmetic; merge at your discretion.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d5c9730366

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 2m 33s —— View job

---

PR Review

• Gather PR context and changed files
• Read changed test scripts
• Analyze threshold math and invariants
• Evaluate Codex false-negative concern
• Post findings

---

Summary

The core fix is correct. The structural bug — THRESHOLD_MS derived as SINK_SLEEP * 1000 / 2, coupling the threshold to the sleep and leaving the margin unchanged when the sleep widened — is properly resolved in both plugins. Commit d811b6f then correctly addressed the Codex concern about asymmetric margins before the PR was last synchronized. One stale value in the PR body test plan and one lingering diagnostic echo nit are noted below; neither is a blocker.

---

Threshold math: correct (and improved since prior review)

The chain of changes:

State	SINK_SLEEP	THRESHOLD_MS	Noise-side margin (vs ~2150ms)	Leak-side margin (vs ~8065ms)
Old broken	6 s	3000 ms	850 ms	5065 ms
Initial PR (SAFETY_MARGIN_MS=2000)	8 s	6000 ms	3850 ms	2065 ms
Final (SAFETY_MARGIN_MS=3000)	8 s	5000 ms	2850 ms	3000 ms

Commit d811b6f identifies the problem precisely: the 2000 ms leak-side margin was below the 2150 ms worst-case noise ceiling, meaning a load shift that inflated every baseline sample and then subsided before the slow run could absorb enough of the 8065 ms leak signal to push the delta under 6000 ms. Setting SAFETY_MARGIN_MS=3000 puts the threshold at 5000 ms, roughly midway, with both margins above the measured noise ceiling.

The identical change is correctly applied to the markdown-format copy.

---

Codex concern: acknowledged and directly addressed

The Codex review flagged that min-of-N baselines protect against one slow sample but not a load shift affecting all three. The code comment now explicitly documents this:

The leak side is the one a too-HIGH threshold breaks, and it is not covered by the min-of-N baseline below: that protects against ONE unlucky baseline sample, not against a load shift that inflates ALL of them and then subsides before the slow run.

The 3000 ms leak-side clearance is the mitigation — a baseline inflation of >3000 ms would be required to mask a real leak, and the 40-run load test peaked at ~1590 ms. The tradeoff is accepted with eyes open, which is the right call for a test-only fix.

---

Minor findings (not blocking)

1. PR body test plan references the stale 6000 ms threshold

The test plan bullet — "C1 deltas -175ms to 644ms against the new <6000ms threshold" — was written against the initial commit (SAFETY_MARGIN_MS=2000) and wasn't updated after d811b6f bumped it to 3000. The actual code now produces a 5000 ms threshold. The measured deltas (max 644 ms) clear both values, so the pass/fail outcome is unaffected, but the stated threshold in the PR description is wrong. Cosmetic only.

2. Diagnostic echo still omits SAFETY_MARGIN_MS (line 290)

echo "  (C1 fd1-leak: base=…ms slow=…ms delta=…ms, threshold <5000ms, sink sleeps 8s)"

The log shows the result of the formula but not its inputs, so verifying the threshold value from a log line requires knowing SAFETY_MARGIN_MS from the source. Adding margin=${SAFETY_MARGIN_MS}ms would let you cross-check without opening the file. Same nit flagged in the prior review; still very minor.

---

Version bumps and CHANGELOGs

Both correct:

• desktop-notification: 0.5.1 → 0.5.2 — patch, test-only
• markdown-format: 0.6.2 → 0.6.3 — patch, test-only; CHANGELOG correctly notes no production behavior change

---

Verdict: LGTM. The fix is structurally sound, the commit history shows the margins were consciously sized against real measurements, and the code comment now fully documents the inherent tradeoff. The two nits above are cosmetic.

[claude]

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

---

Security Review

• Get PR diff and identify changed files
• Read changed files in full (both commits: d5c9730, d811b6f)
• Analyze for security vulnerabilities
• Post findings

---

No security issues found.

The six changed files across this PR's two substantive commits are: two plugin.json manifests (version bumps only), two CHANGELOG.md entries (documentation only), and two test scripts. The complete set of code changes:

Location	Change
desktop-notification.test.sh / markdown-format.test.sh	SINK_SLEEP 6 → 8; add SAFETY_MARGIN_MS (3000 in the final commit after d811b6f corrected the initial 2000)
Both test files	THRESHOLD_MS formula: SINK_SLEEP * 1000 / 2 → SINK_SLEEP * 1000 - SAFETY_MARGIN_MS
Both test files	Comment block expansion explaining the two-sided margin rationale

Injection surfaces:

make_sink writes its first argument verbatim into an executable temp script, so every new call site was examined:

SLOW_SINK="$(make_sink "cat >/dev/null; sleep $SINK_SLEEP")"

$SINK_SLEEP is assigned the integer literal 8 two lines above and expands before the string reaches make_sink. The body received is cat >/dev/null; sleep 8 — no external input in the expansion path.

THRESHOLD_MS=$((SINK_SLEEP * 1000 - SAFETY_MARGIN_MS))

$SAFETY_MARGIN_MS is 3000 (integer literal, same script). Both variables are used only in $((...)) arithmetic — safe for variables whose values are proven-integer and sourced entirely within the script.

Other vectors checked: No credentials or secrets. No network calls. No dependency pins. No GitHub Actions files. No authorization logic. No path construction using the new variables. Production hook code is entirely untouched — this is test-only.