github source: rearm comment cites a stderr mirror the plugin logger never opts into, Only-ever-shortens wrong again on the throwing-tick path, one warn-less capturing fixture (#2126 residuals)

Aperta
#2,129 2 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
3/5
Tempo stimato
1-2 giorni
Idoneità per principianti
48/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
javascript

Direzione di ricerca

Read hypaware-core/plugins-workspace/github/src/source.js around lines 138–161 and 203, plus test/plugins/github-source.test.js around lines 80–90. Update the two comments to limit the seenImports claim to ticks that return and remove the unsupported stderr-mirror explanation; preserve the schedule-before-log order. Add ...silentLog to the capturing fixture, then run the focused test and the stated test and typecheck commands.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

neutral:stuck

Deferred from PR #2126, review round 2, head f739509a3bc0132a81b1f7f33e94ccc708476c91. Both review rounds for that PR are spent (cap N=2), so these land here instead of in-PR. Three residuals: one evidenced test defect (N3) and two comment-accuracy defects (N1, N2) in the same two files. All three were measured in round 2 and independently re-verified at triage; none blocks #2126, whose fix for #1789 was reproduced before and after in both rounds.

The common thread worth naming: N1 and N2 are comments that assert something untrue about why the code is shaped as it is, and N2 is the second correction of the same paragraph (round 1's F5 was the first). A comment that misattributes why a guard exists is what leads the next editor to remove the guard, so these are not typo-grade even though they are prose.

N1 - the rearm-before-log comment cites a throw source that does not exist at this call site

hypaware-core/plugins-workspace/github/src/source.js:158-161:

// Re-arm before logging: between the clearTimeout above and this call the
// source has no timer at all, and the stderr mirror behind `log.info` may
// throw (LLP 0329#stderr-mirror), which from a watch callback would leave
// the source permanently dead while status() reports the cancelled timer.

There is no stderr mirror behind this log.info. The chain, verified statically and dynamically at triage:

  • source.js holds the runtime set at github/src/index.js with log: ctx.log.
  • ctx.log comes from createPluginLogger (src/core/runtime/activation.js:1575), which calls getLogger('plugin') with no opts.
  • src/core/observability/logger.js:159: const mirror = opts.mirrorStderr ?? false, so the process.stderr.write mirror branch is dead for every plugin log, and the OTEL emit is wrapped in try/catch routing to reportTelemetryFailure.
  • The mirrorStderr: true opt-in set (grep -rn mirrorStderr src/) is exactly the containment-refusal sites (capture_spool, dep_graph, cache/spool, cache/partition, cache/sweep_guard, config/grep_migration, plugin_doctor/dry_run, registry/commands). No plugin logger is in it.

Measured (stderr replaced with a thrower): getLogger('plugin').info(...) does not throw (plainThrew: null); getLogger('plugin', { mirrorStderr: true }).info(...) throws EPIPE: stderr gone. LLP 0329#stderr-mirror is the decision that refusal reports opt in per call site; it does not support the use this comment makes of it, and because the citation is prose rather than an @ref, /ref-check will not catch it.

The reorder itself is correct and must stay. It is right on the src/core/daemon/control.js:322-331 precedent (a watch-callback body must not be able to throw between disarm and re-arm) and it is free. Only the stated mechanism is wrong. Suggested wording from round 2: "Re-arm before logging: the source has no timer between the clearTimeout above and this call, and a throw from the log (a foreign global LoggerProvider, a fields bag with a throwing getter) must not be able to land in that gap." Optional nit while there: hoist const delayMs = nextDelayMs() above the clearTimeout so the window the comment describes is literally empty.

N2 - the Only ever shortens paragraph is wrong for the second time, now on the throwing-tick path

source.js:138-145 (as reworded by the round 1 triage) now credits the seenImports snapshot, "already consumed by the tick's own end-of-tick read of the same write", with keeping the source off its own output. That holds only when the tick returns:

  • backlogPending = stagedImportPending() || result.pending sits inside tick()'s try (source.js:57); the catch at :77 never calls stagedImportPending(), so a throwing tick refreshes nothing and seenImports stays stale (empty from boot after a restart).
  • runCaptureTick persists cursors before rethrowing (tick.js:187-192), so a throwing tick still writes the sidecar this watcher watches.
  • Round 2 measured the consequence: with a standing authorization and a first throwing tick, one forced late event from the source's own closing write re-armed and latched backlog_pending with no external writer at all (armed 1799996ms -> 900000ms, rearms=1). Under natural inotify delivery it did not occur in 0/15 trials, because the events landed while inFlight was still set; that is a delivery-timing accident, not the invariant the prose claims (round 1 measured 3 of 287 real events arriving after the reschedule).

Smallest honest wording: seenImports stops the self-re-arm on a tick that returns; a tick that throws leaves the snapshot stale, with the behavioral consequence owned by #2127. Doc-only; the code change belongs to #2127, not here.

N3 - one warn-less capturing logger fixture drives a live source (evidenced test defect)

Round 1's F3 converted five warn-less inline fixtures to silentLog but missed the capturing logger at test/plugins/github-source.test.js:80-90, which supplies only info/error and drives a live startGithubSource() at :99. watchSidecar's failure path calls runtime.log.warn('github.cursor_watch_unavailable', ...) (source.js:203). Demonstrated in round 2 by making fs.watch() throw:

not ok 2 - source runs shortly after boot and reports structured completion-relative cadence
  error: 'runtime.log.warn is not a function'

Unreachable with a healthy mkdtemp directory on Linux, so minor, but inotify exhaustion on a shared CI runner turns a real assertion into a TypeError. One-token fix: spread ...silentLog (already imported at :11) before the capturing overrides.

Acceptance

  • The source.js:158-161 comment no longer claims the stderr mirror can throw at this call site, and the reorder (schedule before runtime.log.info) is untouched.
  • The Only ever shortens paragraph scopes the seenImports claim to ticks that return and points the throwing-tick consequence at #2127.
  • The test:80-90 fixture survives a throwing fs.watch into a real warn (an fs.watch-throws run fails tests 10-12 for the legitimate no-watcher reason, never with runtime.log.warn is not a function).
  • test/plugins/github-source.test.js stays green (12/12) and npm test / npm run typecheck stay clean.
Lingua principale
JavaScript
Stelle
11
Fork
0
Merge medio
2h 49m
PR unite (30g)
297

Preparare l'ambiente

Questo progetto non fornisce container di sviluppo, Dockerfile né guida per i contributori, quindi l'ambiente è a tuo carico: parti dal suo README e consulta la nostra guida al primo contributo per i passaggi generali.

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di hyparam/hypaware

Tutte le issue di hyparam/hypaware

Issue simili

Altre issue su JavaScript

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.