feat(plugin): schema-driven config with live-reload scopes + reorganized layout; v0.28.0 - #63
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a schema-driven configuration system for the prerender plugin, reorganizing configuration options under namespaces like ingress and origin while maintaining backward compatibility for legacy paths. It also enables background schedulers to dynamically re-arm themselves on configuration changes without requiring a worker restart. The review feedback is highly constructive and focuses on strengthening the configuration validation engine: specifically, ensuring that nonEmpty checks explicitly reject null and undefined values, and marking critical fields like refreshTime, timezone, securityToken.header, and debugHeader.key as nonEmpty to prevent potential runtime crashes or invalid HTTP requests.
| reject(path, node, `must be one of ${node.enum.map((v) => `'${v}'`).join(' | ')}`); | ||
| return; | ||
| } | ||
| if (node.nonEmpty && (value === '' || (Array.isArray(value) && value.length === 0))) { |
There was a problem hiding this comment.
If a nonEmpty option (such as deviceTypes.supported or cacheKey.attributes) is overridden with null or undefined, the current check value === '' || (Array.isArray(value) && value.length === 0) will evaluate to false and fail to reject the invalid value. This can lead to runtime crashes when downstream code attempts to access properties or methods on the null/undefined value.
We should explicitly check for null and undefined in the nonEmpty validation.
| if (node.nonEmpty && (value === '' || (Array.isArray(value) && value.length === 0))) { | |
| if (node.nonEmpty && (value === '' || value === null || value === undefined || (Array.isArray(value) && value.length === 0))) { |
References
- Validate and reject unusable or structural configuration overrides at the entry point (where configuration is parsed or applied) by restoring them to safe defaults, rather than adding defensive guards in downstream utility functions.
There was a problem hiding this comment.
Applied in dc6ab4a, with one caveat noted in a comment: null/undefined can't actually reach this check through applyOptions — mergeInto skips null/undefined overrides and every schema path exists in the defaults — but the validator shouldn't depend on the merge layer's behavior to be safe, so the explicit checks are worth having.
| refreshTime: option('12:00', 'Local time-of-day ("HH:MM") for the daily sitemap refresh.'), | ||
| timezone: option('America/New_York', 'IANA timezone `refreshTime` is interpreted in.'), |
There was a problem hiding this comment.
The refreshTime and timezone options are critical for scheduling the daily sitemap refresh. If either is configured as an empty string, parsing will fail or throw an error at runtime. Marking them as nonEmpty: true ensures that any empty overrides are rejected and reverted to their safe defaults at apply time.
| refreshTime: option('12:00', 'Local time-of-day ("HH:MM") for the daily sitemap refresh.'), | |
| timezone: option('America/New_York', 'IANA timezone `refreshTime` is interpreted in.'), | |
| refreshTime: option('12:00', 'Local time-of-day ("HH:MM") for the daily sitemap refresh.', { nonEmpty: true }), | |
| timezone: option('America/New_York', 'IANA timezone refreshTime is interpreted in.', { nonEmpty: true }), |
References
- Validate and reject unusable or structural configuration overrides at the entry point (where configuration is parsed or applied) by restoring them to safe defaults, rather than adding defensive guards in downstream utility functions.
There was a problem hiding this comment.
Done in dc6ab4a — both marked nonEmpty. (A malformed-but-non-empty value like 25:99 or a bad IANA name still passes; if that ever bites, a pattern field def on the schema is the natural extension.)
| 'bot mitigation). Set the value per deployment — preferably via `valueEnv` so the secret ' + | ||
| 'stays out of config.yaml.', | ||
| { | ||
| header: option('x-harper-renderer-bypass', 'Header name carrying the token.'), |
There was a problem hiding this comment.
The securityToken.header option specifies the header name used to authenticate prerender requests. If configured as an empty string, it will result in an invalid header name when constructing fetch requests, potentially causing runtime errors. Marking it as nonEmpty: true ensures that empty overrides are safely rejected.
| header: option('x-harper-renderer-bypass', 'Header name carrying the token.'), | |
| header: option('x-harper-renderer-bypass', 'Header name carrying the token.', { nonEmpty: true }), |
References
- Validate and reject unusable or structural configuration overrides at the entry point (where configuration is parsed or applied) by restoring them to safe defaults, rather than adding defensive guards in downstream utility functions.
There was a problem hiding this comment.
Done in dc6ab4a — origin.securityToken.header is now nonEmpty.
| }), | ||
|
|
||
| debugHeader: group('Debug response headers, emitted when the request carries this header (any value).', { | ||
| key: option('x-harper-prerender-debug', 'Request header name that turns on debug response headers.'), |
There was a problem hiding this comment.
The debugHeader.key option specifies the request header name that enables debug response headers. If configured as an empty string, it will result in an invalid header name. Marking it as nonEmpty: true ensures that empty overrides are safely rejected.
| key: option('x-harper-prerender-debug', 'Request header name that turns on debug response headers.'), | |
| key: option('x-harper-prerender-debug', 'Request header name that turns on debug response headers.', { nonEmpty: true }), |
References
- Validate and reject unusable or structural configuration overrides at the entry point (where configuration is parsed or applied) by restoring them to safe defaults, rather than adding defensive guards in downstream utility functions.
There was a problem hiding this comment.
Done in dc6ab4a — debugHeader.key is now nonEmpty.
|
Addressed all four review findings in dc6ab4a:
Added a test covering the four new nonEmpty rejections. 359/359 tests pass, lint + format clean. |
…zed layout; v0.25.0 Every option now lives in src/configSchema.js — a single declarative catalog of default, operator-facing description, reload scope (live vs restart), and validation/display hints (enum, min/max, nonEmpty, unit, secret, itemType). defaultConfig(), merge validation, redaction paths, and the management API's machine-readable schema all derive from it, ready for an admin-UI config editor. Live reload now actually reaches the background schedulers: the queue status sync, sitemap refresh scheduler, reconciler, backlog snapshotter, and unrouted reporter subscribe to onConfigApplied and re-arm when their gate or interval changes — so sitemap.node pinning, render.reconcile.enabled, and every timer cadence apply without a restart. The only restart-scoped options left are the reconciler's boot-stagger knobs; changing one live logs a warning and is reported as pendingRestart on GET /prerender_admin/config. Config layout reorganized (21 -> 14 top-level keys), old paths still apply via movedFrom aliases with a deprecation warning: - botPathPrefix, excludePathPatterns -> ingress.* - securityToken, staging, userAgents, ignoredHeaders -> origin.* - url.queryParams -> cacheKey.queryParams (url group dissolved) - sitemapUserAgent -> sitemap.userAgent - debugHeader.value removed (was never read; the header is presence-gated) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eader names + sitemap time/zone nonEmpty Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…o schema, reactive flush timer; v0.28.0 - port crawlStats options into configSchema.js (descriptions + min bounds) - crawlStats lazy flush timer now follows crawlStats.enabled/flushInterval live (disable flushes pending sketches instead of holding them) - re-stamp v0.25.0 -> v0.28.0: main moved to v0.27.0 under this branch Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dc6ab4a to
b3128ca
Compare
|
Rebased onto main (v0.26.0 bot_serve/page_age and v0.27.0 crawl-breadth landed underneath) and re-stamped v0.25.0 → v0.28.0:
377/377 tests, lint + format clean. |
What
Introduces a declarative config schema (
src/configSchema.js) as the single source of truth for every plugin option — default, operator-facing description, reload scope (live vs restart), and validation/display hints — and makes the background schedulers genuinely live-reloadable. Groundwork for managing config from the admin UI.Schema (
src/configSchema.js)Each option/group declares:
description— served by the management API; written for operators, will back the admin-UI config editorscope—'live'(default) or'restart'; groups can set a scope their children inheritsecret— drives redaction (replaces the hardcodedSECRET_PATHSinutil/redact.js)enum,min/max,nonEmpty— enforced at apply time (violation warns + keeps the default; generalizes the old hand-rolled empty-delimiter rejection)unit,itemType— display hints for the future editormovedFrom— legacy path alias (see reorg below)defaultConfig(), merge validation, redaction, anddescribeConfigSchema()(JSON-serializable, served onGET /prerender_admin/configasschema) all derive from the one tree.Live reload reaches the schedulers
Previously the five background schedulers captured their gates/intervals at boot, so options like
sitemap.nodesilently required a restart (bitten by this during the sitemap-daily rollout). They now subscribe toonConfigAppliedand re-arm on change:node/workerIndexpinning,refreshTime,timezone— pin/unpin/re-time without restartenabled(runtime kill-switch),intervalmanagement.enabled,backlogSnapshotInterval(0 = manual-only)enabled,intervalstatusSyncInterval(Re)enabling a per-node sweep re-arms boot-shaped (delay + per-node stagger), since a config change reaches all nodes at the same moment — same herd concern as a rolling restart.
The only restart-scoped options left are
render.reconcile.startDelay/startJitter. Changing one live logs a warning and is reported aspendingRestartonGET /prerender_admin/config(cleared if flapped back to the boot value).Config reorganization (21 → 14 top-level keys)
All moves keep a
movedFromalias — old paths still apply, with a deprecation warning; when both old and new are set, the new path wins:botPathPrefix,excludePathPatternsingress.*securityToken,staging,userAgents,ignoredHeadersorigin.*(everything about fetching from the origin)url.queryParamscacheKey.queryParams(one-fieldurlgroup dissolved)sitemapUserAgentsitemap.userAgentRemoved:
debugHeader.value— dead config; both consumers presence-checkdebugHeader.keyonly.The deployed consumer config sets none of the moved keys except the
origin.*ones, which the aliases cover — verified against the consumer repo'sconfig.yaml.Admin API
GET /prerender_admin/confignow returnsschemaandpendingRestartalongside the redacted config + warnings. The console's config view renders generically, so the new layout displays as-is; the schema-driven editor is deliberately not wired yet (stubbed-panel rule — awaiting design).Testing
test/configReactivity.test.js: scheduler re-arm state transitions under mock timers (arm/disarm/interval swap/pin move), plus a real flush-cadence test for the unrouted reporterconfig.test.js: schema completeness (every option/group described), alias coverage, enum/bounds/nonEmpty enforcement, pending-restart tracking, listener semanticsredact.test.js: redaction list derived from schema🤖 Generated with Claude Code