From 0bd6f25710cc220350896fc1c4645e8523b897b5 Mon Sep 17 00:00:00 2001 From: Brad Edwards Date: Sat, 23 May 2026 05:37:45 +0200 Subject: [PATCH 1/5] Audit runtime comments for stale future-work language Updated comment passages in src/main.ts, src/runtime/{audio,presenter, prompter,scene-loader,timeline}.ts, and src/system/presenter/prompter- window.ts that described already-landed work as future or placeholder: the GSAP composition timeline (PUL-F022), the cue gate (PUL-F017), the presenter command controller (PUL-F020 / PUL-F021), and the L2 createChromePrompterRenderer. Optional-API doc strings reworded from 'workbench bootstrap that has not yet wired' to factual 'callers that don't supply X omit the field'. Source-policy comments, biome-ignore rationales, runtime behavior, exported types, error strings, DOM attributes, validation, and tests are unchanged. The architecture-preflight binding note lives at docs/design/issue-100-runtime-comment-audit-preflight.md and is registered in docs/design/README.md. Closes #100 --- changelog.d/100.changed.md | 11 ++ docs/design/README.md | 1 + ...sue-100-runtime-comment-audit-preflight.md | 135 ++++++++++++++++++ src/main.ts | 47 +++--- src/runtime/audio.ts | 4 +- src/runtime/presenter.ts | 4 +- src/runtime/prompter.ts | 30 ++-- src/runtime/scene-loader.ts | 23 ++- src/runtime/timeline.ts | 14 +- src/system/presenter/prompter-window.ts | 17 ++- 10 files changed, 213 insertions(+), 73 deletions(-) create mode 100644 changelog.d/100.changed.md create mode 100644 docs/design/issue-100-runtime-comment-audit-preflight.md diff --git a/changelog.d/100.changed.md b/changelog.d/100.changed.md new file mode 100644 index 0000000..5f10071 --- /dev/null +++ b/changelog.d/100.changed.md @@ -0,0 +1,11 @@ +Audited runtime source comments and rewrote stale future-work +language across `src/main.ts`, `src/runtime/{prompter,scene-loader, +timeline,presenter,audio}.ts`, and `src/system/presenter/prompter- +window.ts`. Outdated phrases naming a placeholder timeline runner, an +unwired cue gate, an uninterpreted presenter command controller, or a +captions/script UI surface "until the renderer lands" now describe +the wired adapters (PUL-F017 / PUL-F020 / PUL-F021 / PUL-F022 / +PUL-F024 / `createChromePrompterRenderer`). Source-policy comments +(`// PUL-*-allow:`, `// biome-ignore`), runtime behavior, exported +types, error strings, DOM attributes, validation, and tests are +unchanged. diff --git a/docs/design/README.md b/docs/design/README.md index 4453758..92fc3f9 100644 --- a/docs/design/README.md +++ b/docs/design/README.md @@ -11,6 +11,7 @@ Design context for Pulsar. Source material for the ADRs in `../adrs/`. | [issue-097-browser-runtime-smoke-preflight.md](issue-097-browser-runtime-smoke-preflight.md) | Guardrails for expanding browser runtime smoke coverage through the existing Playwright workbench gate. | | [issue-098-vertical-slice-demo-preflight.md](issue-098-vertical-slice-demo-preflight.md) | Guardrails for adding authored demo scenes that exercise composition, timeline, captions, assets, and workbench routing through existing runtime seams. | | [issue-099-repeated-scene-activation-context-preflight.md](issue-099-repeated-scene-activation-context-preflight.md) | Guardrails for supporting repeated scene entries through per-occurrence activation ownership without changing scene, composition, URL, registry, timeline, or audio boundaries. | +| [issue-100-runtime-comment-audit-preflight.md](issue-100-runtime-comment-audit-preflight.md) | Guardrails for auditing stale runtime comments without changing behavior, duplicating ADR rationale, or weakening source-policy comments. | | [positioning-and-landscape.md](positioning-and-landscape.md) | Category, adjacent OSS projects, differentiation, strategic risks. | | [pul-p002-validation-ci-preflight.md](pul-p002-validation-ci-preflight.md) | Guardrails for gating pull-request CI on the canonical runtime validation pass. | | [pul-p003-adr-format-preflight.md](pul-p003-adr-format-preflight.md) | Guardrails for keeping ADR markdown and Ground Control ADR records aligned without duplicate schemas or workflow logic. | diff --git a/docs/design/issue-100-runtime-comment-audit-preflight.md b/docs/design/issue-100-runtime-comment-audit-preflight.md new file mode 100644 index 0000000..448e44f --- /dev/null +++ b/docs/design/issue-100-runtime-comment-audit-preflight.md @@ -0,0 +1,135 @@ +# Issue 100 Runtime Comment Audit Preflight + +Date: 2026-05-23 + +Issue 100 is source-comment hygiene. The change should make runtime +comments trustworthy without changing runtime behavior or moving broad +architecture rationale back into code. ADRs and design docs remain the +durable home for system-level decisions; source comments should explain +local invariants, boundary contracts, machine-read policy exceptions, +and non-obvious constraints. + +No new ADR is needed. Existing ADRs already decide the runtime seams: +scene and composition contracts (ADR-002 / ADR-008), timeline and audio +adapters (ADR-003 / ADR-004 / ADR-025), workbench URL dispatch +(ADR-007 / ADR-013 / ADR-014), validation (ADR-008 / PUL-F028), error +surfaces (ADR-028 plus PUL-Q006 / PUL-Q009), and chrome/workbench +ownership (ADR-031). + +## Boundary + +- Scope the audit to runtime-authored source under `src/main.ts`, + `src/runtime/**`, `src/system/**`, `src/scenes/**`, and + `src/compositions/**`, including TypeScript and runtime CSS comments. + Tests and docs may be consulted to verify status, but they are not + the primary target. +- Review comments containing `future`, `placeholder`, `not yet`, + `follow-up`, `OMITTED`, `TODO`, `FIXME`, ADR references, and PUL + references. A match is not automatically stale; decide from the + current code and the relevant ADR/design doc. +- Keep comments that state local invariants, security boundaries, + policy-exemption rationale, ownership boundaries, or failure + envelopes. Trim broad history and duplicated ADR prose when the code + nearby is self-evident. +- Do not change exported types, runtime strings, DOM attributes, + validation logic, policies, source scanners, tests, build config, or + behavior unless a comment exposes a real defect. If that happens, + keep the behavioral fix focused and test it as a defect, not as a + comment-audit side effect. +- This requirement-free issue does not create or transition Ground + Control requirements and does not create IMPLEMENTS / TESTS + traceability links. + +## Required Reuse + +- Durable rationale: `docs/adrs/**`, `docs/design/**`, + `docs/design/README.md`, and `docs/requirements/conventions.md`. +- Workflow policy: `.ground-control.yaml`, `.gc/plan-rules.md`, + `AGENTS.md`, and `changelog.d/README.md`. +- Validation/schema incumbents: `assertSceneModule()`, + `assertCompositionManifest()`, `createIdRegistry()`, + `validateRuntime()`, `resolveAssetUrl()`, `PRESENTER_COMMAND_KINDS`, + `isPresenterCommand()`, `formatSceneContext()`, + `describeError()`, and `describeErrorDetailed()`. +- Policy-source incumbents: `tests/runtime/source-policy.ts`, + `tests/runtime/screenshot-determinism-source.test.ts`, + `tests/runtime/policy-*.test.ts`, and Biome `biome-ignore` + comments with non-empty rationales. +- Verification commands stay the repo defaults from `package.json` and + `.ground-control.yaml`: `pnpm lint`, `pnpm typecheck`, `pnpm test`, + and the combined completion command. + +## Cross-Cutting Layers + +| Layer | Guardrail | +|-------|-----------| +| Source-policy comments | `// PUL-*-allow: ` comments are executable policy inputs. Preserve the exact allow tag, same-line placement, and non-empty reason unless replacing it with an equivalent valid exemption. | +| Lint suppressions | `// biome-ignore ...: ` comments are lint policy, not prose. Do not remove or generalize them while trimming architectural comments. | +| Scene/composition schemas | Comment edits must not introduce parallel DTO language such as `SceneDTO`, `CaptionSchema`, duplicate manifest shapes, or new validation tables. Refer to the canonical runtime types and validators. | +| URL and mode dispatch | Do not imply scenes own query parsing, mode flags, or history state. URL grammar remains `parseNavigationSearch()` and mode resolution remains `effectiveMode()` at the runtime/workbench boundary. | +| Timeline boundary | `src/runtime/timeline.ts` is the GSAP adapter and beat-label home. Comments should not describe a placeholder runner where `createGsapCompositionTimeline()` now owns the path. | +| Audio boundary | Audio remains `ctx.audio`, `createAudioService()`, and the Howler engine wrapper. Comments should distinguish Howler's engine behavior from the explicit present-mode unlock adapter in `audio-unlock-dom.ts`. | +| Validation boundary | Validation remains an orchestrator over existing runtime contracts. Do not document validation as a second registry, schema system, linter, or lifecycle runner. | +| Error envelope | Public diagnostics stay bounded through `describeErrorDetailed()` and `formatSceneContext()`. Do not change error messages or add comments that encourage parsing `Error.message` for structured data. | +| Security policy | The audit adds no auth surface, secret handling, cookies, local/session storage, `import.meta.env`, `process.env`, `process.argv`, remote dynamic import, `eval`, or generated code path. Existing source-policy gates remain authoritative. | +| OS/process exposure | No command should pass source snippets, tokens, headers, credentials, or env values through process argv. The audit is static source review plus repo-local tests. | +| Changelog policy | Docs-only preflight changes need no fragment. Source-comment-only changes are documentation/process-only unless they alter user-visible behavior or public surfaces; do not hand-edit `CHANGELOG.md`. | + +## Extensibility + +The only useful seam for future repeat audits is the search scope and +term set. Keep it parameterized as roots plus terms, not as a runtime +concept or comment taxonomy. If stale-comment enforcement becomes +recurring, the canonical place is a focused source-policy test that +reuses `tests/runtime/source-policy.ts`; do not add a separate scanner, +package script, config schema, or CI workflow for this issue. + +## Gotchas + +- `placeholder` is often an intentional scene id, tag, DOM marker, or + no-op adapter. Do not rename identifiers or remove local comments + just because the word appears. +- `future` can mean a deliberate extension seam, not stale work. + Examples include optional future URL parameters, future command + variants, and future renderer surfaces. Keep these when they explain + why the current boundary is shaped for extension. +- `not yet run` in fixtures can describe observed runtime state rather + than missing implementation. +- Some comments are known high-risk candidates because adjacent code + has moved: bootstrap text that still says a placeholder timeline + runner is waiting for ADR-003, prompter comments that predate the L2 + renderer wiring, and audio comments that say unlock is entirely + auto-handled by Howler. Verify against code before editing. +- ADR references inside source can drift when later ADRs supersede a + section. Prefer a short local invariant plus a single current ADR + reference over a historical chain. +- Broad rationale belongs in ADRs/design docs. Do not expand source + comments to compensate for deleting stale text. +- Removing a source-policy exemption rationale can break tests even + though runtime behavior is unchanged. + +## Anti-Patterns + +- Implementing a comment-audit runtime helper, enum, config file, lint + plugin, or custom script. +- Rewriting comments by requirement status alone without checking the + current source and accepted ADRs. +- Updating runtime behavior to make an old comment true. +- Changing public error strings, data attributes, URL grammar, command + kinds, validation findings, or policy allow tags as part of prose + cleanup. +- Duplicating scene, composition, caption, presenter, audio, asset, or + error schemas in comments. +- Adding changelog fragments for docs-only or source-comment-only + changes when no user-visible behavior changed. + +## Non-Goals + +Issue 100 does not add new architecture, requirements, ADR decisions, +workflow automation, source-policy enforcement, validation behavior, +runtime features, browser UI, telemetry, security policy, persistence, +or release tooling. + +It also does not require exhaustive prose normalization. The goal is to +remove or correct stale future-work language while preserving comments +that carry real local maintenance value. diff --git a/src/main.ts b/src/main.ts index c058b2b..6344af9 100644 --- a/src/main.ts +++ b/src/main.ts @@ -10,8 +10,9 @@ // scene/composition modules. Both registries are immutable after // construction (ADR-008 #2 "manifests over flow control"). // 3. Build a `SceneLoader` (PUL-F008) wired to those registries plus -// the lifecycle adapters (PUL-F005 asset preloader; placeholder -// timeline runner until ADR-003's GSAP runner lands). +// the lifecycle adapters: PUL-F005 asset preloader, the ADR-003 / +// PUL-F022 GSAP-backed composition timeline, the PUL-F024 / ADR-004 +// audio service, and the PUL-F030 / ADR-029 audio-unlock adapter. // 4. Subscribe to the parsed-target events PUL-F007's // `bootstrapNavigation` dispatches: `pulsar:navigate` carries a // parsed `NavigationTarget`, `pulsar:navigate-error` carries a @@ -144,16 +145,15 @@ const createPreloader = (signal: AbortSignal): ReturnType` parented to // `document.body` (above the chrome surface). The L2 transitions // library (cut / dissolve / hard-slam / hold-on-black / push) tweens @@ -229,20 +229,15 @@ const buildCtx = ( return { ...base, chrome: chromeSlots as unknown as Readonly> }; }; -// Prompter renderer placeholder (PUL-F019 / ADR-022). Under -// `mode=prompter` the loader bypasses the resolver lifecycle -// structurally — no preload, no `create`, no `timeline`, no -// `cleanup` — and hands a `PrompterScript` (captions aggregated -// from the addressed scene or composition slice) to this adapter. -// Until the captions/script UI surface lands, the placeholder -// produces no visible output; the structural visual-rendering -// suppression is delivered by the loader's lifecycle bypass, not by -// this adapter. -// -// Pulsar L2 renderer: paints the full prompter script (composition -// id + per-scene captions) into the workbench. Falls back to -// `document.body` if the chrome slot resolution returns null. The -// returned dispose callback removes the panel on next navigation. +// Prompter renderer (PUL-F019 / ADR-022). Under `mode=prompter` the +// loader bypasses the resolver lifecycle structurally — no preload, +// no `create`, no `timeline`, no `cleanup` — and hands a +// `PrompterScript` (captions aggregated from the addressed scene or +// composition slice) to this adapter. The L2 +// `createChromePrompterRenderer` paints the full script (composition +// id + per-scene captions) into the chrome lower-third slot, falling +// back to `document.body` when the slot is unavailable. The returned +// dispose callback removes the panel on the next navigation. const renderPrompter: PrompterRenderer = createChromePrompterRenderer( () => chromeSlots?.lowerThird ?? document.body, ); diff --git a/src/runtime/audio.ts b/src/runtime/audio.ts index 9d6de09..fcd4c46 100644 --- a/src/runtime/audio.ts +++ b/src/runtime/audio.ts @@ -441,8 +441,8 @@ export const AUDIO_OUTPUT_POLICIES = Object.freeze(['audible', 'silent', 'log-cu * {@link AudioServiceOptions.onCue} sink. The PUL-F026 / ADR-004 * rehearsal-mode contract: "audio is silenced OR logged as cues * without altering timeline state." With no sink wired the policy - * is effectively silent (the workbench has not yet attached a cue - * UI / log surface). + * is effectively silent (the workbench attaches no cue UI / log + * surface today). * * Future variations (silent rehearsal as a distinct mode, export * silence, ducking, bus volume, an audio-status UI) extend this same diff --git a/src/runtime/presenter.ts b/src/runtime/presenter.ts index de26242..028bc9b 100644 --- a/src/runtime/presenter.ts +++ b/src/runtime/presenter.ts @@ -309,8 +309,8 @@ export function createPresenterController( // Lazy source attachment: register the central wrapper on the // source the first time a subscriber attaches. A controller with - // zero subscribers (e.g., the placeholder timeline runner that - // ignores `input.presenter`) does not pay a source registration. + // zero subscribers (e.g., a timeline runner that ignores + // `input.presenter`) does not pay a source registration. // Subscribe-time failures from the workbench-supplied source are // routed through `onError` here rather than escaping the loader's // `buildLoad` (codex review, post-PUL-F025: a throwing source diff --git a/src/runtime/prompter.ts b/src/runtime/prompter.ts index 5533f6b..87f688b 100644 --- a/src/runtime/prompter.ts +++ b/src/runtime/prompter.ts @@ -3,8 +3,9 @@ // Pure function over a {@link SceneNavigationTarget} (already validated // by `resolveSceneNavigation` per PUL-F008 / ADR-014). Produces a // {@link PrompterScript} carrying the captions metadata of the -// addressed scene or composition slice. The future captions/script UI -// surface consumes this shape; the scene loader hands it to a +// addressed scene or composition slice. The captions/script UI surface +// (the L2 `createChromePrompterRenderer` the workbench bootstrap +// wires) consumes this shape; the scene loader hands it to a // {@link PrompterRenderer} adapter without interpreting it. // // Why prompter is structurally different from `mode=loop` / @@ -113,20 +114,19 @@ export type PrompterDispose = () => void | Promise; * obtain the dispose callback (if any), then proceeds. * * A renderer can ALSO use the older parking-until-abort pattern - * (return a `Promise` that resolves on `signal.aborted`, - * with cleanup in the abort listener), which the placeholder under - * `mode=present` uses for the timeline runner. Both patterns - * satisfy the contract; the dispose-return pattern is preferred - * for renderers that mount DOM because the loader OWNS the abort - * sequencing — there is no documentation-only "you must keep your - * promise pending" convention for the renderer to forget. + * (return a `Promise` that resolves on `signal.aborted`, with + * cleanup in the abort listener). Both patterns satisfy the contract; + * the dispose-return pattern is preferred for renderers that mount + * DOM because the loader OWNS the abort sequencing — there is no + * documentation-only "you must keep your promise pending" convention + * for the renderer to forget. * - * Optional on {@link import('./scene-loader').SceneLoaderOptions}: a - * workbench bootstrap that has not yet wired a captions UI omits the - * field and the loader dispatches `mode=prompter` without invoking any - * renderer (visual rendering is still structurally suppressed because - * the resolver lifecycle is bypassed). Production bootstrap supplies a - * concrete renderer when the UI surface lands. + * Optional on {@link import('./scene-loader').SceneLoaderOptions}: + * callers that don't render a captions view (test harnesses, + * embedders) omit the field and the loader dispatches `mode=prompter` + * without invoking any renderer (visual rendering is still + * structurally suppressed because the resolver lifecycle is bypassed). + * The workbench supplies the L2 `createChromePrompterRenderer`. */ // `void` in this union is intentional: the "no cleanup obligation" // half of the contract must accept implicit-return arrow functions diff --git a/src/runtime/scene-loader.ts b/src/runtime/scene-loader.ts index 2f96312..2f278a9 100644 --- a/src/runtime/scene-loader.ts +++ b/src/runtime/scene-loader.ts @@ -335,14 +335,13 @@ export interface SceneLoaderOptions { * cleanup. A trivial / no-DOM renderer (e.g. a test stub) is free * to return synchronously because there is nothing to tear down. * - * Optional: a workbench bootstrap that has not yet wired a captions - * UI omits the field. Under `mode=prompter` the loader still - * suppresses the resolver lifecycle (no preload, no `create`, no - * `timeline`, no `cleanup`) because that suppression is the - * structural defense PUL-F019 / ADR-022 record; the captions data - * path simply has no consumer until the UI lands. Production - * bootstrap supplies a concrete renderer when the captions/script - * UI surface lands. + * Optional: callers that don't render a captions view (test + * harnesses, embedders) omit the field. Under `mode=prompter` the + * loader still suppresses the resolver lifecycle (no preload, no + * `create`, no `timeline`, no `cleanup`) because that suppression + * is the structural defense PUL-F019 / ADR-022 record; the captions + * data path simply has no consumer in that configuration. The + * workbench supplies the L2 `createChromePrompterRenderer`. */ readonly renderPrompter?: PrompterRenderer; /** @@ -360,11 +359,11 @@ export interface SceneLoaderOptions { * subscribes via `input.presenter.subscribe(...)` and forgets to * unsubscribe cannot leak across navigations. * - * Optional: a workbench bootstrap that has not yet wired a - * presenter UI omits the field. Under that configuration the - * loader does not build a controller, runners see + * Optional: callers that don't supply presenter input (test + * harnesses, embedders, non-presenter contexts) omit the field. The + * loader then does not build a controller, runners see * `input.presenter === undefined`, and the seam is structurally - * inert until the workbench wires a real source. + * inert. */ readonly presenterCommands?: PresenterCommandSource; /** diff --git a/src/runtime/timeline.ts b/src/runtime/timeline.ts index 993dc6d..2d97f58 100644 --- a/src/runtime/timeline.ts +++ b/src/runtime/timeline.ts @@ -40,8 +40,8 @@ // resolves on the master's natural completion (so the resolver tears // every scene down) or on navigation abort. // -// Scenes that have not yet authored a timeline return `null` (the -// placeholder scene does this); the adapter composes such a scene as a +// Scenes that don't author a timeline return `null` (the placeholder +// scene does this); the adapter composes such a scene as a // zero-duration segment rather than rejecting it. // // References: @@ -114,8 +114,8 @@ const isGsapTimeline = (value: unknown): value is GsapTimeline => /** * Validate the value a scene's `timeline(ctx)` returned, including its - * beats. `null` / `undefined` are accepted as "no timeline authored - * yet" (the placeholder scene returns `null`); any other non-timeline + * beats. `null` / `undefined` are accepted as "no timeline authored" + * (the placeholder scene returns `null`); any other non-timeline * value is a scene-contract violation and throws * {@link SceneTimelineTypeError} with the scene id in the message. * @@ -1024,9 +1024,9 @@ function runMasterUntilDone( } /** - * Build the GSAP-backed {@link CompositionTimelineAdapter} the workbench - * wires onto the composition resolver (replacing the placeholder - * runner). Each `run(segments, opts)` call: composes `segments` (the + * Build the GSAP-backed {@link CompositionTimelineAdapter} the + * workbench wires onto the composition resolver. Each + * `run(segments, opts)` call: composes `segments` (the * active composition slice's scene timeline values, in manifest order) * into one master GSAP timeline; applies the head hints; reports the * live master to `onMaster`; then plays the master, resolving on its diff --git a/src/system/presenter/prompter-window.ts b/src/system/presenter/prompter-window.ts index c5b07c8..4016a7d 100644 --- a/src/system/presenter/prompter-window.ts +++ b/src/system/presenter/prompter-window.ts @@ -1,12 +1,12 @@ // Pulsar L2 — prompter window helper. // // `openPrompterWindow(url)` spawns a separate browser window booted -// in `mode=prompter`. The runtime's existing `renderPrompter` adapter -// in the spawned window receives the prompter script and renders it -// (the L2 system layer ships a default renderer below that upgrades -// the placeholder in `src/main.ts`). +// in `mode=prompter`. The runtime's `renderPrompter` adapter in the +// spawned window receives the prompter script and renders it (the L2 +// system layer ships `createChromePrompterRenderer` below, which the +// workbench bootstrap wires in `src/main.ts`). // -// Because pulsar already has end-to-end prompter wiring (loader-side +// Because pulsar has end-to-end prompter wiring (loader-side // lifecycle bypass + buildPrompterScript caption aggregation), the // prompter window just needs to open the same URL with mode=prompter. @@ -33,10 +33,9 @@ export const openPrompterWindow = ( }; /** - * A non-trivial `PrompterRenderer` that renders the full prompter - * script into the workbench chrome's lower-third slot (or a fallback - * `