Skip to content

fix(spawn): harden abort/reset race handling, bump Pi to 0.84.1, clear security allowlist - #23

Open
ofriw wants to merge 11 commits into
mainfrom
fix/ci-7-8-26
Open

fix(spawn): harden abort/reset race handling, bump Pi to 0.84.1, clear security allowlist#23
ofriw wants to merge 11 commits into
mainfrom
fix/ci-7-8-26

Conversation

@ofriw

@ofriw ofriw commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Note: This PR was generated by an AI agent. If you'd like to talk with other humans, drop by our Discord!


What Changed

Spawn lifecycle hardening — Race conditions between signal aborts, reset invalidation, and prompt rejections are now handled deterministically via a shared abort-promise pattern (abortChildSession in state.ts). Previously, concurrent abort + reset could double-fire session.abort() or swallow real prompt errors; now the shared WeakMap<AgentSession, Promise<void>> ensures one abort per session, and non-AbortError prompt failures propagate even during abort races.

CI / compat testing overhaul

  • Removed the hardcoded Pi 0.82.0 floor-compat lane. Replaced with a latest-Pi-lane that dynamically resolves the current Pi release + its TypeBox peer from the npm registry.
  • Added runNpmWithRetry() with exponential backoff for transient registry failures.
  • New compat-fixture.mjs extracts shared compat utilities (createCompatCopy, prepareCompatCopy, resolveLatestPi, diagnostics).
  • CI now uploads diagnostic artifacts on failure for both the packed-host and current-Pi lanes, plus a workflow_dispatch trigger.

Security audit cleanup — Emptied the audit-ci allowlist entirely. All previously suppressed advisories for brace-expansion and undici (pinned by Pi 0.82.0) are resolved by bumping to Pi 0.84.1 / TypeBox 1.3.7.

Renovate config added.github/renovate.json pins Pi packages and TypeBox to exact versions.

Breaking Changes

None user-facing. The test-compat-floor.mjs script and test:compat:floor npm script are removed.

Why

  • The spawn race fix closes edge cases where aborted children could leak or real errors could be silently swallowed during /reset or signal cancellation — important for correctness of child session lifecycle management.
  • The compat test modernization ensures the extension is validated against the current Pi release rather than a stale 0.82.0 floor, catching upstream regressions earlier and more often.
  • The empty allowlist enforces a clean security posture — no more suppressing known advisories.

Testing

  • New abort-child-session.test.ts + 4 new race-condition tests in spawn.test.ts cover abort dedup, sync/async abort throws, and signal-vs-reset priority.
  • runNpmWithRetry tested with fixture npm CLI simulating transient failures.
  • Config invariant tests now assert empty allowlist + pinned Pi 0.84.1 / TypeBox 1.3.7.

Attached is an agent optimized description of the changes in this PR - AGENT_REVIEW.md

@grzegorznowak
grzegorznowak self-requested a review August 13, 2026 03:39

@grzegorznowak grzegorznowak left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. A rendered child can be aborted twice or remain registered after failure. spawn/renderer.ts, the module that renders child-agent activity, calls session.abort() directly instead of using the new shared abort helper in state.ts, the module that tracks active child sessions.

    Why it matters: A reset and a screen cleanup can cancel the same child twice. If cancellation throws immediately, the stale child remains in the live-session registry.

    Concrete example: The review traces this sequence: a signal starts the first abort, then renderer disposal starts a second abort. It also shows that a synchronous abort error prevents registry deletion.

    Action: Route renderer disposal through abortChildSession() and delete the registry entry in a finally block.

  2. Reset errors change depending on race timing. spawn/index.ts, the module that creates and runs child agents, reports an abort failure if reset happens before the child is registered. If reset happens later, it replaces that failure with “Spawn invalidated by reset.”

    Why it matters: The same failed cleanup can produce different operator-visible results. In one path, the cleanup failure can disappear completely.

    Concrete example: A reset during child creation can return “abort failed.” A reset during an active prompt can return only “Spawn invalidated by reset,” with no cleanup notification.

    Action: Define one reset error policy and preserve abort failures consistently across every timing path.

  3. Cleanup reporting can cause an unhandled promise rejection. A promise rejection is an asynchronous failure. spawn/index.ts, the child execution module, starts a detached error handler that calls the user-interface notification function. It does not handle a failure from that notification call.

    Why it matters: If notification itself throws, the extension can produce a process-level unhandled rejection. The extension runs inside the same process as the terminal interface, so this can destabilize the interface.

    Concrete example: The review traces: abort rejects, the error handler runs, ctx.ui.notify() throws, and the resulting promise has no observer.

    Action: Catch notification failures in the abort-reporting path and preserve them deterministically, as the existing dispose path already does.

  4. One Windows compatibility failure hides the next Windows result. .github/workflows/test.yml, the GitHub Actions workflow that runs automated checks, runs the current-Pi Windows check after the packed-host check. The later step uses the default “run only after success” behavior.

    Why it matters: When packaging fails, maintainers lose the independent Windows runtime result they need to determine whether the problem affects packaging, the Pi API, or both.

    Concrete example: If “Packed latest Pi host contract” fails, its diagnostics upload runs, but “Synchronized current Pi compatibility on Windows” is skipped.

    Action: Give the current-Pi step an explicit non-cancelled condition or move the two checks into separate jobs.

  5. The package claims wider compatibility than CI verifies. A peer dependency is a library that the host application must provide. package.json, the package manifest, accepts every Pi and TypeBox version through *, but the old minimum-version test was removed.

    Why it matters: Developers can install this extension with older versions without an npm warning, even though CI now tests only the pinned development versions and the latest available host.

    Concrete example: An older Pi host satisfies the wildcard peer declarations, but no remaining compatibility lane tests that combination.

    Action: Set peer ranges to the versions the project supports, or restore a test for the oldest supported host.

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.

2 participants