feat(debug): unify thread spawn + lazy attach for callScript breakpoints (PR 2 of #691 plan)

[jsavin]

Summary

Implements PR 2 of the two-PR sequence in docs/THREAD_DEBUG_ATTACH_PLAN.md. Closes the menu-handler debugging gap: thread.callScript-spawned threads (e.g. File>Open, Edit>Find — anything routed through the headless menu dispatch) are now debuggable when a protocol debug client is attached. Previously you could set a breakpoint inside commands.open and it would never fire on a menu click; only PTY screen-scraping worked. After this PR, attach a protocol debug client, set the breakpoint, click File>Open from the palette, and the debugger suspends inside commands.open like any other script.

PR 1 (the isolated #706 fix on debug/run) already shipped as #692 on 2026-06-01. This PR builds directly on it.

Two commits

Commit 1 — refactor(thread): extract headless_spawn_script_thread (80b87c95c)

Pure-parity refactor. Extracts a single primitive headless_spawn_script_thread in new file frontier-cli/headless_spawn.{c,h} that absorbs the ~90% common boilerplate between debug/run's spawn (in debug_handler.c) and thread.callScript + thread.evaluate's spawn (in headless_thread_verbs.c). The two old entry functions (debug_thread_entry, thread_entry_point) are replaced by one unified entry. Both callers now pass:

• thread_run_spec — selects langruncode vs langrunscriptcode
• thread_debug_opts (nullable) — carries transport, debugstate slot, start_suspended; NULL = detached + no debug

No behavior change. All existing tests stay green.

Commit 2 — feat(debug): lazy breakpoint-driven attach for callScript spawned threads (bf51d1868)

Adds the lazy attach mechanism:

1. TLS current-script tracking. debug_push_sourcecode / debug_pop_sourcecode now unconditionally update __thread char tls_current_script[256] + a 64-frame TLS stack, even when state == NULL. Gives the breakpoint callback a way to match against spawned threads' current script.

2. Atomic attach-transport pointer. New _Atomic(transport_t *) g_debug_attach_transport in debug_handler.c, set by protocol_main when a protocol session attaches, cleared at session exit. WS-attached debugging is intentionally out of scope for this commit (the per-message WS stack transport would create a UAF; deferred to a follow-up that introduces a per-WS-client heap-allocated transport).

3. Lazy-attach branch in the breakpoint callback. When state == NULL && g_has_breakpoints && g_debug_attach_transport != NULL && breakpoint_matches(tls_current_script, lnum): lazily call debug_register_thread, store into (**hglobals).debugstate, fall through to the existing transport-agnostic suspension-and-wait loop. Lazy unregister fires at thread exit so debug slots don't leak.

The zero-breakpoint menu-click hot path is unchanged in cost: still one relaxed atomic load (g_has_breakpoints) before any other work. The new acquire-load on g_debug_attach_transport is gated behind both the state == NULL branch (only fires for non-debug threads) and the g_has_breakpoints check (only fires when at least one breakpoint exists).

Scope explicitly deferred

• WS-attached lazy attach — would require a per-connection heap-allocated transport_t in ws_server.c to avoid the UAF on the per-message stack transport. Tracked for follow-up. Today's WS debugging continues to work via debug/run, which captures the per-message transport at register-time.
• Bare-linenoise debug transport — issue Interactive REPL debugging: provide a debug transport for bare linenoise sessions #691 itself (the parent issue) tracks the broader question; the audit found it's now likely subsumed by the boxen-based debugger TUI plan (task docs: align headless plan with compiled glue and add CLI hydration test #23 in this session). Interactive REPL debugging: provide a debug transport for bare linenoise sessions #691 stays open.

Test plan

• make build — clean (only pre-existing warnings)
• ./tools/run_headless_tests.sh — 591/591 pass (no regression vs develop baseline)
• ./tests/debug_protocol_test.sh — 73/73 pass (was 65 pre-Commit-2; +2 categories x ~4 assertions each = +8 new assertions for Tests 21 and 21b)
• cd tests && make test-integration — 2186 pass / 21 baseline failures (matches PR test(#715): behavioral coverage for copyctopstring deep-stack defenses #717/fix(#716): item 3 — document headless getenv path; refactor legacy guard #719/fix(#716): item 5 — bound >255-byte hostname in tcp_address_to_name #720 baseline; all 21 are pre-existing html/tcp/startup flakiness, identical names to baseline)

Tests added in tests/debug_protocol_test.sh

Test 21 — lazy attach: breakpoint in thread.callScript spawned thread:

1. script/eval  -> install @system.temp.csBp = "local (x = 1)\rreturn (x + 1)"
2. debug/setBreakpoint -> {"script":"system.temp.csBp", "line":1}
3. script/eval  -> "thread.callScript(@system.temp.csBp, {})"
4. wait_for_notification(op="debug/suspended", reason="breakpoint", timeout=5)
   ASSERT: notification arrives; params.script == "system.temp.csBp";
   params.line == 1; params.threadId is a number
5. debug/continue -> {"threadId": <from step 4>}
6. wait_for_notification(op="debug/completed", success=true)

Confirmed RED-then-GREEN: pre-fix, Step 4 timed out (state == NULL early return at the callback fired for every callScript thread); post-fix, all assertions pass.

Test 21b — zero-breakpoint hot path: with no breakpoints set, thread.callScript of a tight loop completes within 5 seconds with no unexpected suspension. Guards against the lazy path accidentally slowing the menu-click case.

Sentinel observed

This PR touches the GIL hot path + pthread spawning. Per /auto Phase 5 criterion 8 and docs/AUTO_CHAIN_LESSONS_2026-05-08.md, integration verification was run from the main session at the PR's exact HEAD (not just sub-agent reported). Counts and failing-test names match the PR #720 baseline character-for-character; no foundational-area regressions snuck in.

Risks documented during planning (all mitigated or scoped out)

Risk	Mitigation
Stack-allocated WS transport stored in atomic global -> UAF	Protocol-mode-only setter in this PR; WS deferred
tls_current_script empty at first breakpoint of fresh thread	debug_push_sourcecode unconditionally updates TLS even when state == NULL; verified by Test 21 passing
Refactor regresses joinable-vs-detached attr handling	Commit 1 is pure parity; full integration suite gates it
Hot-path overhead on zero-breakpoint menu clicks	Guarded by existing g_has_breakpoints relaxed atomic; Test 21b enforces no measurable cost
debug_register_thread slot exhaustion (MAX_DEBUG_THREADS=16) under burst menu clicks	Lazy path runs only on breakpoint match (rare); lazy-unregister at thread exit prevents leaks; log_warn fallback if registration fails — never crash
Virgin.root touch	None; verified clean by diff inspection

What this unblocks

After this lands, File>Open is a normal debuggable bug: attach a protocol debug client, breakpoint commands.open, trigger File>Open from the menu, step through the file.getFileDialog -> spawned-thread-stdin conflict (separately documented as the actual File>Open defect).

🤖 Generated with Claude Code

[jsavin]

/gate Round 4 — Convergence

Verdict: PASS — all reviewers approve, 0 P0/P1 remaining.

Reviewer	Round 1	Round 2	Round 3	Round 4
bar-raiser	2 P1	1 P1 + APPROVE round 2 P1s	1 P2, APPROVE	APPROVE
security	2 P1	APPROVE round 1 + recommends P2 follow-ups	1 P1 (NULL deref)	APPROVE
concurrency	2 P1	NEEDS WORK (drain timeout + Test 22 happy-path)	APPROVE	APPROVE

P1s addressed across rounds (8 total)

Round 1 fixes (commit 11b3aa7):

1. Transport UAF (security + concurrency) — heap-allocate transport in protocol_main, add g_lazy_attached_count drain counter
2. pthread_join(0) on detached lazy threads (security) — fldetached field; debug_kill_all_threads/debug_join_all_threads skip detached
3. lazy_state declared after goto (bar-raiser) — hoisted to function top
4. Evaluate-path "register after pthread_create" comment (bar-raiser) — sharpened

Round 2 fixes (commit 8f3c9ea):
5. hcode leak in headless_spawn_script_thread failure paths (bar-raiser) — single fail: goto label disposes hcode when !is_callscript
6. Drain has no timeout (concurrency) — 5s deadline + force-kill detached + 500ms grace + bail
7. Stale main-thread globals after drain (security/concurrency bonus) — protocol_main snapshots and restores hthreadglobals/hashtablestack/currenthashtable/langcallbacks around the drain
8. Test 22b added — exercises the actual UAF window (session-close while suspended), confirms drain timeout fires and process exits cleanly within 13s

Round 3 fix (commit 9955824):
9. NULL deref in cleanup ladder (security P1 + bar-raiser P2) — if (rec != NULL) guard around rec->user_thread_id dereferences

Tests at HEAD 9955824

• make build: clean
• ./tools/run_headless_tests.sh: 591/591 pass (no regression vs develop baseline of 589 pre-tcp tests from PR fix(#716): item 5 — bound >255-byte hostname in tcp_address_to_name #720)
• ./tests/debug_protocol_test.sh: 81/81 pass (was 65 pre-PR; +16 across Tests 21/21b/22/22b)
• cd tests && make test-integration: 2186 pass / 21 baseline failures (failure NAMES identical to PR test(#715): behavioral coverage for copyctopstring deep-stack defenses #717/fix(#716): item 3 — document headless getenv path; refactor legacy guard #719/fix(#716): item 5 — bound >255-byte hostname in tcp_address_to_name #720 baseline; no foundational-threading regression)

Deferred P2 items (for follow-up issues)

• TLS overflow counter asymmetry (bar-raiser, round 1)
• start_suspended documented but currently always true (bar-raiser, round 1)
• Pascal-string const-drop refactor (bar-raiser, round 1)
• Test 21b iteration-count too loose to defend the hot-path claim (bar-raiser, round 1)
• Test 21 doesn't assert lazy thread is unregistered post-completion (bar-raiser, round 1)
• TLS update on resolution failure (security, round 1)
• Drop-notification during teardown (bar-raiser, round 2)
• Bail-after-grace UAF on wedged threads (concurrency, round 2; documented residual risk)
• OOM-path test-coverage gap (security, round 3)

🤖 Generated with Claude Code /gate

[jsavin]

/auto Summary

What went well: The TDD discipline held up across 4 fix-loop rounds. The Plan-first design was strong enough that the unified primitive's seam (thread_run_spec + nullable thread_debug_opts) survived 3 commits of incremental fixes without re-shaping. Each round's reviewers found genuinely new P1s — not re-flags of prior findings — and the count strictly decreased: 3 -> 2 -> 1 -> 0. That's the convergence signal the /auto skill is designed for.

What could improve: The lazy-attach lifetime contract turned out to be more intricate than the Plan anticipated. Three of the 8 P1s (transport UAF, pthread_join(0), bail-after-grace) were all variants of "ownership of process-global state across detached threads in a session whose owner is leaving." A larger up-front design pass on the lifecycle diagram — specifically "what holds what, when does it die, who waits for what" — would have caught at least two of the P1s before the first review pass. The sub-agent doing Commit 2 had the right pieces but didn't synthesize the cross-cutting lifetime story.

Recommendations: For future threading PRs in this codebase, add a "lifetime contract sketch" subsection to the Plan agent's prompt template. The sub-agent doing the implementation should write the lifetime story BEFORE writing the C, in the form of "what objects exist; who owns each; what events transfer or release ownership; what's the worst-case 'who's left holding what'." This forces the cross-cutting analysis up front. Also: Test 22b (the actual UAF window — session close while suspended) should be the FIRST test designed when adding a session-scoped resource that other threads can hold pointers to. We added it in round 2; should have been in commit 2.

P2 follow-ups filed as issue #723 (9 polish items in a single tracking bucket).

🤖 Generated with Claude Code /auto