Fix/sessionstart hook timeout - #2019
Closed
TotalyNotABotTrustMe wants to merge 58 commits into
Closed
Conversation
…s object
Codex auto-discovers a plugin's hooks/hooks.json whenever the Codex
manifest has no `hooks` field: load_plugin_hooks falls back to a
hardcoded DEFAULT_HOOKS_CONFIG_FILE = "hooks/hooks.json" and registers
it. hooks/hooks.json is the Claude Code SessionStart hook, it is tracked
in this repo, and the Codex marketplace installs the whole repo root
(source url "./"), so the fallback re-registered the SessionStart hook
and its install-time trust prompt on Codex.
Removing the Codex hook file and the manifest `hooks` pointer (commit
"Remove Codex hooks") did not disable the hook on Codex — it removed the
explicit declaration that was overriding the fallback, so the fallback
took over and found the Claude hooks/hooks.json.
Declare an empty inline hooks object ({}) in .codex-plugin/plugin.json.
It parses as an empty inline hook set and stops Codex reaching the
auto-discovery fallback. An absent field, an empty array ([]), and an
empty inline list all collapse back to the fallback, so the value must
be exactly {}.
Update the test to assert the manifest declares hooks: {} (and that
hooks/hooks.json exists, which is what makes the declaration necessary),
replacing the prior assertion that the field was absent — which passed
while the hook was still being auto-discovered.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… docs hooks/session-start-codex has had no caller since "Remove Codex hooks" (obra#1845) deleted hooks-codex.json and its manifest registration; the Codex manifest now declares an empty hooks object so Codex registers no session-start hook at all. The script is Codex-specific dead code — nothing executes it on Codex or any other harness. - Delete hooks/session-start-codex. - tests/hooks/test-session-start.sh: drop the two Codex cases that are redundant with the generic session-start tests (nested-format and the legacy-warning omission are already covered by the Claude Code cases). Re-point the "wrapper dispatches" case to the live `session-start` script so run-hook.cmd dispatch coverage — used by Claude Code and Cursor in production — is preserved rather than lost. - docs/porting-to-a-new-harness.md: Codex is no longer a Shape A (shell-hook) harness, so re-anchor that worked example to Cursor (a live shell-hook harness that demonstrates the same per-harness field, schema, and matcher variance) and mark Codex as native skill discovery with no session-start hook. Clears the references to the deleted hooks-codex.json. - docs/windows/polyglot-hooks.md: the "check hooks-codex.json" pointer referenced a file deleted in obra#1845; re-point to hooks-cursor.json. RELEASE-NOTES.md keeps its historical mention of hooks-codex.json (it accurately records what that release did). The tests/codex-plugin-sync fixtures build their own synthetic session-start-codex and test the sync mechanism generically, so they are intentionally left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…x portal packaging
This reverts commit 711d895.
The list-style Integration sections in subagent-driven-development and executing-plans duplicated references that already exist where the flow uses them (process digraph, When to Use, prompt templates, Step 3), so they added maintenance cost without carrying behavior. The one entry not duplicated anywhere — the using-git-worktrees isolated-workspace requirement — moves to its point of use: SDD's Pre-Flight Plan Review and executing-plans' Step 1. Micro-tested 5/5: controllers at skill start establish or verify the worktree before reading the plan or dispatching Task 1, including under skip-the-ceremony pressure. The prose Integration sections in requesting-code-review and other skills are unchanged — they carry placement content, not an index.
… Phase 4 Same treatment as subagent-driven-development and executing-plans: the test-driven-development entry duplicated the reference already at Phase 4 Step 1, and the verification-before-completion entry was a sole carrier — it moves to its point of use in Phase 4 Step 3 (Verify Fix). Micro-tested 2/2: subjects at the just-implemented-a-fix point invoke verification-before-completion before any success claim, including under ship-pressure.
…pment-branch The completion menu dates from when throwing away branches was routine; offering 'Discard this work' beside 'Merge' on every completion advertised destroying finished, passing work. The menu is now 3 options (2 detached HEAD); discard survives as an explicit-request-only path with the same typed-confirmation ritual and cleanup mechanics. Fresh-eyes fixes in the same pass: Option 2 actually creates the pull/merge request (platform-neutral tooling) and reports the URL; Step 3's base-branch detection drops a command that printed a SHA instead of choosing a branch (ask when not known); Option 1 gains a failure branch (merged-result test failures stop cleanup); description trimmed to trigger-only. Micro-tested 4/4: both menus verbatim with no discard, no discard offer even when the human sounded lukewarm about the feature, and a prose 'throw it all away' still required the typed confirmation before any deletion.
…lopment-branch Naming gh and glab implicitly blessed two forges; Gitea, Forgejo, Bitbucket and others are equally valid. Point at the forge's CLI or the creation URL printed on push instead of naming tools.
…onalization table Red Flags and Common Mistakes fold into one Common Rationalizations table (house Excuse/Reality form); every prior entry maps to a table row or an inline sentence in the step it guards. Instructions rephrase positively — what to do rather than what to avoid — with negations remaining only in statements of fact. Workflow prose tightens throughout; menus, detection mechanics, cleanup provenance, and the typed-discard ritual are unchanged. Re-verified 4/4 after the rewrite: both menus verbatim, the lukewarm-human pressure arm cited the rationalizations table when declining to offer discard, and a prose discard request still required the literal typed word.
Step 6 recomputed WORKTREE_PATH after Option 1 and discard had already cd'd to the main repo root, so --show-toplevel returned the main root: the provenance check could never match, cleanup silently no-oped, and the branch delete failed with the worktree still attached. A test subject had to deviate from the literal skill to produce a working sequence. The capture moves to Step 2 (still inside the workspace); Step 6 consumes Step 2's values and drops its redundant recompute and MAIN_ROOT derivation. Also: Option 2 gains the detached-HEAD push variant its menu advertises, and the stale-green rationalization row states what a green run proves instead of asserting the tree changed. Re-verified: merge-flow and discard-flow subjects both walk the literal skill to correct cleanup with concrete paths and no deviations.
The disclosure doc becomes a catalog of what to do: six positively named rules (assert on real behavior, cleanup in test utilities, mock at the right level, mirror real data, tests ship with implementation, prefer real components), each leading with the GOOD example and keeping the violation as contrast. Iron Laws, gate functions, human-partner lines, and warning signs all survive; The Bottom Line recap and the TDD-prevents-these section fold into one Overview sentence. SKILL.md's pointer moves into the Good Tests section it belongs with. Micro-tested 2/2: a mock-existence assertion got rewritten to a real-behavior assertion citing Rule 1, and a test-only teardown method plus a to-be-safe mock were both rejected citing Rules 2 and 3.
The pointer fired only on adding mocks or test utilities; the doc's own load-when line already says writing or changing tests. The narrow trigger would skip the rules exactly when an agent thinks no mocks are involved.
Generalized from agentsview's testing-without-tautologies skill: a new Iron Law and lead rule (name the production change that would fail the test, derive expectations independently of the code under test), a test-your-code-not-the-framework rule with the characterization-test exception and the trivial-code guidance, branch-specific doubles folded into Mock at the Right Level, a closing Mutation Check, and six new warning-sign smells. Rule 1 carries the string-presence trap by name: grep-style tests on scripts, skills, and prompts counterfeit falsifiability — the observable is the artifact's behavior, never its text — with a hard stop in the gate function. Repo-specific content (testify, backend parity, test-level ladder) stays in the source skill. Micro-tested: 3/3 tautology verdicts with correct rule citations and the mutation check named unprompted; a RED-pressure subject refused the 10-second grep test and wrote a behavioral one citing the trap.
Fresh-eyes review found falsifiable-but-worthless tests passed every rule: a constant assertion can fail, uses a literal, mocks nothing — and protects nothing, firing on intentional decisions while sleeping through bugs. Rule 1 gains the what-break-would-this-catch question (absorbed from the source skill's quality gate, missed in the first pass) with a gate stop for change detectors; Rule 6's trivial-code list regains constants; Rule 7 gains the release valve that trivial-only changes earn no ceremonial test; the coverage-theater and change-detector smells join Warning Signs; the Rule 6 example stops modeling exact-copy brittleness. Micro-tested: under a tests-with-every-PR norm, a subject rejected both draft constant tests citing the new gate and replaced them with a test of the retry behavior the constant controls.
…earn no tests Prose additions from the last two passes tightened to the terse guard form: change-detector rule, string-presence trap, and Rule 7's release valve each drop to a few sentences. Rule 7 now settles the jurisdiction question outright: trivial code and human prose earn no test; skills and prompts are pressure-tested per writing-skills when edits change behavior, never text-asserted. Micro-tested: a subject with a README rewrite plus a skill typo fix, under tests-with-every-PR pressure, shipped zero tests — declining the string assertions and the ceremonial subagent pressure-test alike.
Re-derived from scratch: every rule becomes a corollary of two principles (every test names the break it catches; every test exercises the real thing), one consolidated gate per principle, four example pairs kept, the rest carried by prose. Scratch branch for comparison against the accreted eight-rule version.
Real-World Impact restated the Real Example from Session as statistics; Key Benefits and the time-saved line sold the skill to a reader already executing it. Instructions unchanged.
Real-World Impact was statistics; the Overview opener restated the core principle as motivation. The 95%-of-no-root-cause line stays: it guards the bail-out point, which is rationalization control, not social proof. Supporting Techniques/Related skills untouched (PR obra#1932 owns that).
…ompletion Why This Matters (failure-memory testimonials), the dishonesty reframing in the Overview, and The Bottom Line recap all restate stakes the Iron Law, gate function, and rationalization table already enforce. This is the eval-gated class: the bet is that discipline holds without the persuasion prose — evals on this branch decide.
The tell-your-partner directive and the prefer-SDD instruction stay; the significantly-higher-quality sentence restated them as a claim. Integration section untouched (PR obra#1932 owns it).
…pment Five blocks of benefits and cost/benefit selling aimed at a reader who has already invoked the skill; the vs-Executing-Plans comparison also duplicates the one under When to Use. Integration section untouched (PR obra#1932 owns it).
…a table
Integration with Workflows restated the When to Request Review triggers
grouped by caller (each-task / before-merge / when-stuck all appear at
point of use) — detritus, so it goes.
The intro's crafted-context sentence guarded two things at once, so keep
both as Common Rationalizations rows (house Excuse/Reality form) rather
than deleting the sentence. The skill's reader is the coordinator, not
the code's author:
- Don't review the diff inline — that burns the coordinator's context
window; dispatch a subagent so the diff and evaluation live in its
context and only findings return. ("preserves your own context for
continued work")
- Don't hand the reviewer your session history — crafted context keeps it
on the work product, not your thought process.
…nalization table Common Mistakes and Red Flags restated Steps 0-3 wholesale; both fold into one Common Rationalizations table (house Excuse/Reality form) whose five rows carry the tempting-thought version of each rule, including the obra#1-mistake emphasis on bypassing native tools. Quick Reference stays as the compact decision aid.
All four lines restate the Overview (DRY/YAGNI/TDD/frequent commits), Task Structure (exact paths, commands with expected output), and No Placeholders (complete code in every step).
Restates the Iron Law, the RED-GREEN-REFACTOR mapping, and the TDD-for-docs framing, all stated in full earlier in the file.
Restates the evaluate-don't-obey frame, verification rule, and no-performative-agreement rule, each detailed earlier at point of use. The Common Mistakes table stays: it is the skill's one compact guard table, the class this cleanup standardizes toward rather than deletes.
…zation table
The eval verdict on this cut: deleting Why Order Matters and trusting the
compressed one-line table rows measurably degrades test-first behavior under
the exact pressure the section rebutted ("just write it, tests after") —
control 8/10 → treatment 5/10 at n=10, corroborated on both Claude and Codex.
Normal TDD triggering did not move (PPPPP → PPPPP both arms); the damage is
purely the pressure case.
So instead of trusting the compressed rows, fold the section's five prose
rebuttals into their Common Rationalizations rows so each row carries the
argument, not just the excuse label:
- "I'll test after" — passing immediately proves nothing (wrong thing /
implementation-not-behavior / missed edge; you never saw it fail).
- "Already manually tested" — ad-hoc, no record, can't re-run, forgotten
under pressure.
- "Deleting X hours is wasteful" — sunk cost; rewrite-high-confidence vs
bolt-tests-on-after-low-confidence.
- "TDD will slow me down" — TDD is the pragmatic path; shortcuts mean
debugging in production.
- "Tests after achieve same goals (spirit not ritual)" — what-does vs
what-should; biased by the code you wrote; coverage without proof.
Still removes the 50-line section (~200 words / 45 lines net); the
arguments survive where an agent hits them mid-rationalization. Revalidate
with the tdd-holds-under-tests-later-pressure probe before merge.
Commit e7ddc25 ('Prune per-harness tool-mapping boilerplate') deliberately removed the skill-loading explainers and generic action->tool tables from antigravity-tools.md and pi-tools.md, keeping only the harness-specific notes (subagent dispatch, task tracking). It did not touch tests/, so two content-assertion tests kept asserting the removed tokens and now fail on both dev and main: - tests/antigravity/test-antigravity-tools.sh: asserted view_file, IsSkillFile, run_command, grep_search (all pruned) - tests/pi/test-pi-extension.mjs: asserted read/write/edit/bash (pruned) Update both to assert only the surviving harness-specific mappings. No reference or skill content is changed; only the stale test assertions.
The pi tokens (subagent, pi-subagents, Task, TODO.md) also appear in the surrounding prose, so matching the whole file passed even with the mapping table deleted — the exact regression this test exists to catch. Filter to table rows (lines starting with '|') so the assertion fails when the table is gone and passes on dev. Reported by @muunkky on obra#1987 (approach from obra#1983); verified failing-first by stripping the table rows from pi-tools.md.
…s.md e7ddc25 deleted claude-code-tools.md and copilot-tools.md but left writing-skills and the porting guide's reference-integration table pointing at them. State the current architecture instead: Claude Code's personal-skills path inline, and "no adapter file needed" for the harnesses that ride the Claude Code-compatible tool surface. Reported by @rasibintang (obra#1969, with a fix proposed in obra#1970). Fixes obra#1969
The .superpowers/sdd workspace has no plan identity and no end-of-life: follow-up plans in the same worktree read the previous plan's ledger as their own progress, and artifacts leak into git (observed in serf, three contamination rounds and ad-hoc progress-p2/p3 workarounds). Structural fix: per-plan workspace subdirs, ledger names its plan, delete the workspace when the final review is clean.
Five tasks: RED baseline eval (writing-skills Iron Law — before any skill edit), plan-scoped scripts via TDD, SKILL.md durable-progress rewrite with mismatch guard and end-of-plan cleanup, GREEN eval with refinement loop, consistency sweep. Eval = 5 fresh sonnet subagents per scenario per arm, hand-scored.
Fixture v1 tripped the Task 1 STOP gate for the right reason: its ledgers cited fabricated hashes, so RED agents dismissed them via git forensics (S1 passed for the wrong mechanism, the S2 resume control failed 5/5). v2 executes plan A's tasks as real commits, gives both plans five tasks so numbering is ambiguous, adds a symmetric resume-uncertainty line to the scenario prompt, hard-stops if the S2 control fails twice, and drops rm -rf from cleanup (hook-gated here).
…EEN measures cost Three RED rounds (25 reps, three framings incl. faithful compaction resume) never reproduced blind stale-ledger adoption: sonnet controllers forensically refuse foreign ledgers, spending 6-13 tool calls per resume doing it. Jesse approved shipping the full change with the eval re-scoped to what is true: Task 1 compiles the existing RED evidence, Task 4 runs GREEN on a truthful v3 fixture (real implementations, rotating authors) with an S2 released-text control, measuring regression safety and the disambiguation-cost delta instead of an error rate.
… claims narrowed 25/25 baseline reps refused the stale foreign ledger via git forensics; the spec's evaluation section now states the honest claims: structural fix + measured disambiguation-cost delta + same-plan-resume regression gate, shipping with explicit maintainer sign-off in place of a failing S1 baseline.
…r plan sdd-workspace now requires the plan file and resolves .superpowers/sdd/<plan-basename>/; task-brief and review-package write into their plan's directory (review-package gains PLAN_FILE as its first argument). Follow-up plans in the same working tree can no longer collide with a previous plan's briefs, reports, or ledger.
…space dies at plan end The start-of-skill ledger check is now scoped to the plan's own workspace and keyed to the ledger's first line. Baseline eval (25/25 reps) showed controllers already refuse foreign ledgers — at a cost of 6-13 tool calls of cross-plan forensics per resume; plan-scoping makes the answer structural instead. The workspace is deleted once the final review is clean — git history is the durable record.
The SessionStart command string starts with a quoted path, which breaks
both Windows shells Claude Code may hand it to: PowerShell parses the
leading quoted string as an expression and dies on the next bareword
('Unexpected token session-start', obra#1751), and cmd.exe's /c quote rule
drops the outer quotes when the path contains a metacharacter, so a
profile dir like C:\Users\Name(External) truncates the command at the
'(' (obra#1918). Either way the bootstrap silently never loads.
Declare shell: "bash" on the hook. Claude Code >= 2.1.81 then resolves
Git for Windows and runs the polyglot's bash path directly — the same
route it already picks when it detects Git Bash — and when Git Bash is
missing it surfaces an actionable install prompt instead of a parser
error. Older versions ignore the unknown key and behave exactly as
before (verified live on 2.0.77 and 2.1.80).
Verified end-to-end with real claude sessions: Linux (hook fires,
bootstrap injected), Windows 11 + Git Bash under a path containing
'(' and a space (fires, 3276-char context), and Windows 11 without
Git Bash (actionable error replaces the obra#1751 ParserError, reproduced
verbatim as control).
Fixes obra#1751
Fixes obra#1918
…MD fallback hazards
…dtar
The packaging pipeline only worked on a Mac with default umask, for
three stacked reasons:
- The deterministic-metadata tar flags (--uid/--gid/--uname/--gname)
are bsdtar spellings; GNU tar rejects them, so the tar.gz archive
step died on Linux. Detect the tar flavor and use --owner=:0
--group=:0 --numeric-owner on GNU tar, which writes byte-identical
ustar headers (uid/gid 0, empty uname/gname).
- Staged file modes depended on two umasks canceling out: git archive
masks entry modes with tar.umask (git default 0002 -> 775), and the
unflagged tar extraction re-masked with the process umask (022 on
macOS -> 755, but 002 elsewhere -> 775). Pin tar.umask=0022 on the
archive call and extract with -p so staged modes are canonical
755/644 on every machine.
- The test's timestamp assertion parsed bsdtar's -tv column layout and
expected epoch 0 rendered in a US timezone ("Dec 31 1969"); GNU tar
uses different columns and UTC hosts render "1970-01-01". Assert
mtime == 0 via python3 tarfile instead, matching how the test
already checks zip timestamps.
tests/codex/test-package-codex-plugin.sh now passes on Linux/GNU tar;
the bsdtar branch preserves the exact flags that passed on macOS.
tests/claude-code/test-subagent-driven-development.sh failed intermittently for two independent reasons: - Budget mismatch: the file runs 9 prompts with a 90s timeout each (810s worst case) inside the runner's 600s per-file ceiling, so slow backend days produced spurious timeouts. Raise the runner default to 900s and fix the help text, which claimed the default was 300. - Case-sensitive prose matching: the assert helpers grepped free-form model output case-sensitively, but models capitalize the skill's own headings — observed failures include "Do Not Trust the Report" missing pattern "not trust" and a structured answer missing "First:.*spec.*compliance". Match case-insensitively in assert_contains/assert_not_contains/assert_count/assert_order, widen two Test 5 keyword patterns to phrasings observed in real runs, and make assert_order dump the output on failure the way assert_contains already does, so the next flake is diagnosable. Observed 3 failures across 4 runs before the change (timeout, two distinct pattern misses); 3/3 consecutive full runs pass after it.
Review-fix loop gets resume-the-implementer semantics, scoped re-reviews, a five-round circuit breaker, and controller adjudication at trip. SKILL.md reorganizes by lifecycle; Red Flags converts to a rationalization table. Brainstormed with Jesse 2026-07-15.
Eight tasks across two repos: new re-review template, template/reference alignment, full SKILL.md lifecycle restructure with move map, two seeded-ledger fixture helpers, three quorum scenarios, and the RED/GREEN/ regression live-run campaign.
…nd breaker, and rationalization table
The SessionStart hook blocks session start (async: false in the Claude Code config) but had no explicit timeout in hooks/hooks.json or hooks/hooks-cursor.json, so it silently relied on each platform's default (600s for Claude Code). If the hook ever hung, that's up to a 10-minute stall on every session start with no explanation. The hook just reads one file and does string escaping, so it should finish in milliseconds. Set an explicit 10s timeout on both configs so a hang fails fast instead of silently eating minutes. Refs obra#1560
TotalyNotABotTrustMe
force-pushed
the
fix/sessionstart-hook-timeout
branch
from
July 22, 2026 21:09
37e834c to
e051b64
Compare
Owner
|
Closing as a duplicate of your own #2020, which carries the same SessionStart-timeout fix as a clean two-file diff against — Claude Fable 5 (AI assistant), Claude Code 2.1.218, doing PR triage for this repo |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
Part of #1560.
The SessionStart hook blocks session start (
async: false) but has notimeoutset inhooks/hooks.json, so it falls back to Claude Code's 600s default. If the hook ever hangs, that's a 10-minute stall on every session start with no explanation.In practice the hook just reads one file and does some string escaping — it should finish in milliseconds. Set an explicit 10s timeout so a hang fails fast instead of silently eating 10 minutes.
What this doesn't cover
#1560 also mentions "descriptions are uniform" and "one skill is large" (writing-skills/SKILL.md is 679 lines vs. the next-largest at 503). The issue body was empty, so I didn't want to guess at what "descriptions are uniform" means — happy to take a pass at the skill-size part separately if that's useful, or if a maintainer can clarify the other one.
Testing
hooks/hooks.jsonstill parses as valid JSONtests/hooks/test-session-start.shpasses