Skip to content

Fix grounded follow-up audit findings#2

Merged
0disoft merged 8 commits into
mainfrom
fix/audit-grounded-followup
Jul 9, 2026
Merged

Fix grounded follow-up audit findings#2
0disoft merged 8 commits into
mainfrom
fix/audit-grounded-followup

Conversation

@0disoft

@0disoft 0disoft commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Summary

This PR applies only the follow-up review findings that were supported by current code, tests, docs, or direct reproduction.

Implemented fixes:

  • Defer runtime.close() when it is called from inside the currently running scoped task callback, so the callback can resolve and the scoped task can become terminal before JSON summary finalization.
  • Timestamp task events at emission time so parent aggregate task events no longer reuse stale parent updatedAt values when child progress changes.
  • Add the public task.skip(message?) API because TaskStatus, summary counts, and rendering already supported skipped, but callers had no handle method to produce it.
  • Add a pull request and main push CI workflow that runs bun run check and bun run pack:check on Node 24.
  • Split release publish and GitHub Release creation permissions so the npm publish job no longer has contents: write.

Tests added:

  • Scoped close inside a task callback emits a final summary with running: 0 and succeeded: 1.
  • Parent aggregate task event timestamps advance after child progress updates.
  • task.skip() emits skipped task status and increments skipped summary count.

Local verification run in the analysis environment:

  • npx tsc -p tsconfig.json --noEmit
  • npx oxlint src test
  • npx oxfmt --check .
  • rm -rf dist-test dist && npx tsc -p tsconfig.test.json && node --test "dist-test/test/**/*.test.js"
  • npx tsc -p tsconfig.build.json
  • node test/fixtures/consumer-esm.mjs
  • npx tsc -p test/fixtures/consumer-ts/tsconfig.json --noEmit
  • npm pack --dry-run --json

Result: 98 Node tests passed locally. The local runtime was Node 22, while this package targets Node 24+, so the new CI workflow should be treated as the final Node 24 gate.

Deliberately not included

  • Shared stream coordinator for concurrent live/plain runtimes: the issue is reproducible, but the correct fix is architectural. A partial change would either break the documented plain fallback contract or silently drop secondary runtime output.
  • Bounded backpressure queues and dropped-frame diagnostics: the risk is real, but it changes buffering and event-loss policy. Existing tests intentionally preserve pending plain/json frames before timeout.
  • Text byte caps for title/message/detail/log: the risk is valid for untrusted child process output, but default caps would be a public API policy change and need explicit option names and docs.
  • Tombstone retention pruning: valid long-running memory concern, but it requires a handle lifecycle redesign rather than a safe localized patch.
  • Pinning third-party GitHub Actions to commit SHAs: the release workflow still uses version tags. I only separated job permissions here because exact SHA pinning needs a repository-wide dependency update policy and should be done consistently for CI and release workflows together.
  • stdout option removal/rename, attachLogStream, scaffold policy docs, and Node/npm fallback scripts: these are product/API/documentation tasks, not directly necessary to fix the reproduced failures in this PR.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new skip API to mark tasks as skipped (e.g., for cache hits) and updates the task event creation timestamp to use Date.now(). It also adds context tracking using AsyncLocalStorage to defer runtime closing when requested inside a scoped task callback. The review feedback highlights two critical issues: a potential memory leak where task handles are not disposed of if a scoped task callback throws an error, and a bug in #shouldDeferScopedClose where checking #handles.has(activeHandle) can prevent deferred closing if the handle was already manually disposed.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/runtime.ts
Comment on lines 133 to 140
} finally {
this.#activeScopedTasks -= 1;
await this.flush();
if (this.#shouldRunDeferredScopedClose()) {
this.#closeRequestedByScopedTask = false;
await this.close();
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

When a scoped task callback throws an error, the catch block handles updating the task status in the store, but the task handle itself is never disposed. This leads to a memory leak because the handle remains in the #handles set indefinitely, and any registered AbortSignal listeners are not cleaned up.\n\nTo fix this, we should explicitly call handle.dispose() in the finally block of runtime.task. Since dispose() is idempotent, this is safe to call even if the task succeeded and was already disposed.

    } finally {\n      handle.dispose();\n      this.#activeScopedTasks -= 1;\n      await this.flush();\n      if (this.#shouldRunDeferredScopedClose()) {\n        this.#closeRequestedByScopedTask = false;\n        await this.close();\n      }\n    }

Comment thread src/runtime.ts
Comment on lines +237 to +246
#shouldDeferScopedClose(): boolean {
const activeHandle = this.#taskCloseContext.getStore();
return (
activeHandle !== undefined &&
this.#handles.has(activeHandle) &&
this.#activeScopedTasks > 0 &&
!this.#closing &&
!this.#closed
);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The check this.#handles.has(activeHandle) in #shouldDeferScopedClose can cause unexpected behavior. If a scoped task manually disposes its handle (e.g., by calling handle.succeed() or handle.fail()) and then calls runtime.close() from within its callback, the handle is no longer in #handles. This causes #shouldDeferScopedClose to return false, meaning the close is not deferred and the runtime is closed immediately, potentially cutting off other active parent or concurrent tasks.\n\nSince activeHandle is retrieved from this.#taskCloseContext (which is an instance-specific AsyncLocalStorage), any returned handle is guaranteed to belong to this runtime. We can safely remove the #handles.has check to ensure that close() is always deferred when called from within any scoped task callback, regardless of whether the handle has been manually disposed.

  #shouldDeferScopedClose(): boolean {\n    return (\n      this.#taskCloseContext.getStore() !== undefined &&\n      this.#activeScopedTasks > 0 &&\n      !this.#closing &&\n      !this.#closed\n    );\n  }

@0disoft 0disoft merged commit 78af399 into main Jul 9, 2026
3 checks passed
@0disoft 0disoft deleted the fix/audit-grounded-followup branch July 9, 2026 13:43
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.

1 participant