Files

T

Tianyi Cui c9dc097749 docs: fix NIH-audit review findings (Codex round 1)

- Drop the AGENTS.md budget bump: trim filler words in the layout map
  and command comments so the new convention line fits the existing
  1680 ceiling (1679/1680; master was 1680/1680)
- timers/promises note: 'Replace both' -> all three sites, and add
  pty-local to the acceptance criteria (EN+ZH)
- execa note: 17 value-taking options plus boolean flags, not 18 (EN+ZH)
- rejected roll-up: lsp-local src is ~1,800 lines, not 2,112 (EN+ZH)
- re-record the three touched i18n pairs

2026-07-26 19:53:42 +08:00

2.6 KiB

Raw Blame History

Agent Note: Use node:timers/promises for hand-rolled cancellable sleeps

Status: proposed

English | 中文

Problem

Three packages hand-roll promise-wrapped timers that the node:timers/promises builtin already provides, while other packages (dsh-llm-mock-server pause(), dsh-lsp-local, dsh-acp-snapshot) already use the builtin — so the hand-rolled copies are also a consistency gap:

packages/llm/llm-retry/src/index.ts cancellableDelay() (~14 lines): new Promise + setTimeout + manual abort-listener add/remove, resolving true on elapse and false on abort, consumed once for the backoff wait.
packages/workflow/workflow-workerthread/src/host.ts sleep() (~7 lines): promise-wrapped unref'd setTimeout used as the dispose-grace bound.
packages/pty/pty-local/src/session.ts delay() (~4 lines): bare promise-wrapped setTimeout used in polling/teardown waits.

Proposal

Replace all three with import { setTimeout } from 'node:timers/promises':

llm-retry: try { await setTimeout(delayMs, undefined, { signal }); /* retry */ } catch { /* abort → fail */ } — with a signal, the promise rejects only with the abort error, and a pre-aborted signal rejects immediately; behavior is identical, including timer clearing on abort. The empty catch names the abort rejection per the repo's empty-catch rule.
workflow-workerthread: setTimeout(ms, undefined, { ref: false }) — exact semantics including not holding the event loop open.
pty-local: import { setTimeout as delay } from 'node:timers/promises' — identical signature, call sites unchanged.

No dedicated tests pin the helpers themselves; the packages' behavior suites keep passing.

Alternatives considered

p-timeout/p-defer style packages. Rejected: the builtin covers both call sites exactly; an external package for a one-line await is negative-net.
Leave them. Rejected only weakly — the cost is small, but the repo already uses the builtin idiom elsewhere, and two hand-rolled variants of a builtin invite a third.

Acceptance criteria

None of the three packages defines a promise-wrapped setTimeout helper; all import from node:timers/promises.
The llm-retry, workflow-workerthread, and pty-local test suites pass unchanged (behavioral parity).

Risks

Essentially none: no model-visible output, no platform concerns, no new dependency. The llm-retry rewrite changes a boolean-returning helper into try/catch control flow — a local readability judgment the implementing PR makes.

2.6 KiB Raw Blame History