test: make the memory stall timing assertion load-tolerant - #516
Merged
Conversation
resolveSpecPath now strips the synthetic builtin:// marker and resolves by name through the library chain (project shadows global shadows builtin), so the path list advertises round-trips as a spec_path input. Unknown builtin names fail as a library lookup instead of a cwd-joined extension error.
Workflow blocks compile coding/prototype to build and plan blocks to plan, but the generated worker_type catalog filtered every primary agent out, so the model could never name the defaults explicitly. The workflow catalog now lists native primaries (build/plan); user-defined primary modes and hidden agents stay excluded and the task catalog is unchanged.
Four guide/schema mismatches from the 2026-09-02 verification: replan and extend auto-resume a paused workflow (manual control(resume) after a successful replan dies on InvalidTransitionError), the exhaustive node field table missed review, the tool reference missed draft/guide/validate, and the name rule missed the leading-dot and control-character exclusions plus the builtin:// marker round-trip.
The SpecGit Acceptance verdict requires required-check runs that started at or after the draft-to-ready transition. Both CI gates only listened to the default pull_request types, so a ready transition never produced fresh runs and the verdict timed out waiting for them.
spec_path now mentions the builtin:// marker round-trip; the review field states the input_mapping wiring validateReviewLifecycle enforces for diff reviews; timeout_ms documents that the budget runs from admission (queue wait counts, an expired queued node fails without spawning).
validateDiffReview enforces seven wiring checks beyond the two node ids (transitive dependency chain, three input_mapping bindings, PASS-gated condition, verdict+fingerprint output_schema), and validateReviewLifecycle only turns them into authoring errors in deep mode — standard mode warns. The previous description understated both.
Regenerate the parameters snapshot after the description changes (the CI unit-test matrix never exercised parameters.test.ts, so the stale snapshot only failed locally). Align blocks.ts with validation.ts (expired-queued-node clause), add the capped-escalation semantics of the deadline watcher and the deep-only final-gate requirement to the timeout/review descriptions, and sync the worker_config guide row.
…fresh The replay list still named specgit@^0.5.0 and omitted three specializations the refreshed harness actually carries (45/40-minute timeout split, node 22, hand-parsed policy.yaml), so the next re-init would replay the wrong version and drop them.
fix: accept builtin:// spec_path returned by list
…ldren cannot hang triggers A timed-out command hook only SIGTERM'd the shell wrapper; grandchildren keeping the stdio pipes open meant the close event never fired and the hook trigger hung forever. Exit and stream-drain are now awaited separately with a bounded grace, then the whole group (detached, negative pid on POSIX; taskkill /T /F on Windows) is SIGKILL'd and reaped.
watch() discarded every Unsubscribe, so each instance dispose/remount cycle leaked five permanent EventV2 listeners holding the instance context. The finalizer now unsubscribes before closing the scope.
Three event.on handlers discarded their unsubscribe functions and accumulated in the app-level SDK handler set on every route transition, retaining the opentui editor and renderer trees.
…nt shutdown stop() on POSIX returned right after SIGTERM, so servers ignoring it stayed alive as orphans while instance finalizers reported success. It now waits a bounded grace, escalates to SIGKILL, and awaits exit; the SDK copy adapts the same escalation synchronously with an unref'd timer. MCP client shutdown reaps the whole process tree through a shared shutdownClient used by the state finalizer, closeClient, and the create rollback path.
Every memory commit copied the full topic set into a new generations directory that was never removed; keep the latest 3 plus orphan-staging sweep, best-effort so a cleanup failure never fails a commit. Heap snapshots (hundreds of MB each) now rotate to the latest 2 in the log directory.
Redundant toString on stream text, .resolves on a synchronous prune helper, and one intentional wire-shape JSON assertion (disabled inline with reason, matching the schema-contract test precedent).
heap-<pid>-<ts> sorted lexicographically orders snapshots by pid across runs (digit-count changes, wraparound), so pruning could delete the newest snapshot and keep stale ones. Sort by the timestamp segment instead, surface non-ENOENT readdir failures as warnings, fix the always-true missing-dir assertion (unawaited async), and add a cross-pid regression test.
The finally-block process.exit() cut off pending async cleanup (instance finalizers, MCP/process stop escalation) — the #503 clause the first pass skipped. An unref'd 5s force-exit timer (matching the stop escalation budget) keeps the hanging-subprocess protection while a clean drain exits immediately with all cleanup completed.
…in window" This reverts commit f40bf17.
The escalates-to-SIGKILL child wrote "ready" before registering its trap, so a parent preempted between the two statements delivered SIGTERM to the default handler and the child exited SIGTERM instead of escalating — observed as a load-dependent failure on the linux runner (262ms, signalCode SIGTERM). Ordering the trap first makes the ready signal prove the handler is installed.
fix: hook command grandchildren hold stdio pipes and hang triggers forever
docs: add v1.0.38 release notes
This was referenced Sep 2, 2026
The 200ms bound around a 40ms connectTimeout flaked at 288ms on a loaded linux runner (raw setTimeout, no TestClock possible). 2000ms still separates fail-fast from hang — a hang trips the test timeout instead — and the Stalled error-class assertion remains the semantic check.
Closes the dependabot high advisory (alert 110): mysql2 < 3.22.0 leaks plaintext credentials on an auth-plugin downgrade to mysql_clear_password.
The RouteProbe seam test mirrors the production subscription shape, so removing an onCleanup wrapper from the real Session/Prompt routes left it green. Mount the actual route components inside the production provider stack and assert the SDK event bus returns to its baseline listener count after unmount; verified red when either wrapper is removed. The two bridge lint suppressions carry reasons (restoration point; overloaded on widened through bind).
win32 taskkill /T /F vs negative-pid kill existed as diverging copies in util/process.stop and the hook execShell killGroup. Extract killGroupPid (POSIX group signal; win32 awaits the taskkill exit code and throws, leaving fallback and logging to callers) and reuse it in both. The SDK sync copy stays as documented in #503. No behavior change: same signals, ordering, and grace constants; the #500 process-group regression suite passes.
LeXwDeX
marked this pull request as ready for review
September 2, 2026 12:58
…ening # Conflicts: # .specgit.yaml
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
1 participant
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.
Closes #512
Closes #513
Closes #514
Closes #515
Why
Close the four actionable findings left over from the v1.0.38 train review: a load-dependent flake in the memory stall test, the open high-severity mysql2 credential-leak advisory (dependabot 110), a seam-only TUI cleanup test that did not protect the #502 production change, and a diverging duplicate of the platform group-kill primitive flagged by the #500-504 review.
What changed
test(memory): stall wall-clock bound 200ms -> 2000ms with a load-tolerance comment; the Stalled error-class assertion stays the semantic check (test: make the memory stall timing assertion load-tolerant #512).fix(console): mysql2 3.14.4 -> 3.22.0 + lockfile, closing the auth-plugin downgrade credential leak (fix: bump mysql2 to 3.22.0 closing the credential-leak advisory #513).test(tui): mount the REAL Session/Prompt route components inside the production provider stack and assert the SDK event bus returns to its baseline listener count after unmount; RouteProbe seam tests kept as the context-level contract (test: pin TUI route event cleanup against the real components #514).refactor(process): extractkillGroupPid(POSIX negative-pid signal; win32 taskkill /T /F awaiting the exit code, throwing to callers) and reuse it instop's win32 branch and the hookexecShellkillGroup; SDK copy untouched per fix: unify child-process stop semantics with await-exit and SIGKILL escalation #503 (refactor: converge the process-group kill primitive into util/process #515).Evidence
bun testopencode memory+process+hook: 215 pass / 0 fail (incl. the fix: hook command grandchildren hold stdio pipes and hang triggers forever #500 process-group regression with real grandchildren); tuitest/cli/tui/: 48 pass / 0 fail.onCleanupwrapper turns the new test red (Expected 0, Received 1), restoring turns it green.bun turbo typecheck29/29;bun run lint4839 warnings / 0 errors (below the 4850 ratchet, no new warnings; the two suppressions in the new test carry reasons).Checklist
specgit finishexits 0.