Skip to content

fix(server): report the webjs.headers rule dropped for having no directives - #1362

Merged
vivek7405 merged 4 commits into
mainfrom
fix/headers-silent-rule-drop
Aug 9, 2026
Merged

fix(server): report the webjs.headers rule dropped for having no directives#1362
vivek7405 merged 4 commits into
mainfrom
fix/headers-silent-rule-drop

Conversation

@vivek7405

@vivek7405 vivek7405 commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #1300, fixing two defects a review found in the webjs.headers warning pass that landed in #1355 (merged as b4488f4).

Both are in packages/server/src/headers.js. Neither was catchable by the tests that shipped with them, which is how they got past a green suite and ten green CI checks.

The rule that loses every directive is dropped in silence

if (directives.length) rules.push(...) discarded a whole rule with no warning. Two ways in, and both are the failure the warning pass exists to end.

{ "source": "/x", "headers": [] } is schema-valid: webjs-config.schema.json puts no minItems on headers, and the boot config check added in the same PR does not descend into headers. So the rule was dropped and nothing anywhere said so, four lines below a docblock sentence claiming every malformed rule is dropped with a warning. Measured before the fix: compileHeaderRules({ webjs: { headers: [{ source: '/empty', headers: [] }] } }) returned [] and emitted zero warnings.

The other way in is a rule whose every directive was individually dropped. Those directives each warned, but nothing said the rule went with them, so an author reading the log saw three directive complaints and no indication that the path now has no rule at all.

compileHeaderRules had lost its docblock

warnDrop's JSDoc was inserted between that function's JSDoc and its declaration. Two consecutive JSDoc blocks means the second binds to what follows, so warnDrop was documented twice and compileHeaderRules was left with none: its @param, its @returns, the "Shape consumed" example, and the value: null removal contract were all orphaned. dev.js types readHeaderRules off ReturnType<typeof compileHeaderRules>, which was inferring rather than reading a declared return.

redirects.js never had this problem because its warnDrop sits after the function. This now matches it, with a comment recording why the order is load-bearing rather than cosmetic.

Also fixed here, from the review

The same silence existed one level UP, at the config key itself, in both readers. "headers": {} or "redirects": "nope" discarded the entire config and returned [] with nothing said: the schema types both keys array, but the boot check inspects only boolean / integer / enum leaves, so it never looked. That is the identical failure this PR fixes at the rule level, and the warnDrop docblock claimed every drop branch already went through it, which was untrue.

Both now warn, and an ABSENT key deliberately does not, since that is the default rather than a mistake. Asserted both ways, because a warning on every app that simply has no webjs.headers would be worse than the silence it replaces.

Verified against the real configs: examples/blog and website produce 0 warnings between them across both readers, and the scaffold emits neither key.

Behaviour change worth naming: an explicit null now warns

{"headers": null} and {"redirects": null} previously produced no warning and now produce one. That is deliberate rather than incidental: the author wrote the key, so it is present and wrong-typed, not absent. It is the one shape where this PR's presence rule differs from "did the old code warn", so it is called out rather than buried, and both tests assert it explicitly.

Everything else moves the other way, toward silence. Eight of the sixteen no-key shapes in the new sweep warned before this PR ('a-string', 42, 0, false, '', and a falsy or non-object config block), and the truthy-non-object-pkg class is unbounded, since any such value short-circuited the old chain to the boolean false.

Test plan

  • packages/server/test/headers/headers.test.js: a new case for the empty-array form (asserts the warning names /empty AND that the valid sibling still compiles), plus the existing multi-drop case extended to assert the rule-level warning for /bad-dir, whose count moves 7 to 8.
  • Counterfactual: revert the !directives.length guard to if (directives.length) rules.push(...) and 2 tests red; restore and 19 pass.
  • Docblock binding verified structurally rather than by eye, re-derived from the committed file at each push: compileHeaderRules is at line 72 with its docblock opening at 51, warnDrop at 178 with its own at 160, so no two JSDoc blocks are adjacent.
  • Browser, e2e: N/A. Config parsing with no browser-facing surface.
  • Bun: this is a console.warn in a config parser, on none of the runtime-sensitive surfaces AGENTS.md lists. The full matrix is run below regardless, since headers.js is adjacent to the response path.

Docs

No surface changed. The docblock sentence this restores was already accurate about the intent; it just was not attached to the function any more, and the code now does what it says.

…ctives

Two defects from the warning pass that landed in #1355, both in headers.js.

`{ source: "/x", headers: [] }` is schema-valid (no minItems) and the boot
config check does not descend into headers, so the rule was dropped with
nothing anywhere saying so. That is the silent-config failure the warnings
were added to end, four lines under a docblock claiming it could not happen.
A rule whose every directive was dropped hit the same branch: the
per-directive warnings named the directives and never said the rule went
with them.

The warnDrop helper also sat between compileHeaderRules' JSDoc and its
declaration, so the JSDoc bound to the helper and the reader lost its
@param, its @returns, the shape example, and the value:null contract.
redirects.js puts its helper after the function, which is why it was fine;
this now matches, with a comment recording why the order matters.
@vivek7405 vivek7405 self-assigned this Aug 9, 2026
Both readers discarded the ENTIRE config in silence when the key was present
but not an array. The schema types both keys `array` and the boot config
check inspects only boolean / integer / enum leaves, so a `"headers": {}`
was reported by nothing anywhere, which is the same silent-config failure
the rule-level and directive-level warnings just fixed one level down.

An absent key stays silent, since that is the default rather than a mistake,
and it is asserted so an app without either key never warns on boot.

The warnDrop docblock claimed every drop branch went through it while this
one did not. It now names all three levels and the absent-key exception.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Read the whole diff against the schema, the boot validator, the dev.js call site, and the doc surfaces. The two fixes hold and the new tests are discriminating, but the change stopped one level too early and the PR body overstated its own verification.

The warnDrop docblock says every drop branch goes through it. It does not: the early if (!Array.isArray(raw)) return [] throws away the ENTIRE config in silence, and nothing else catches it either, since the schema types the key array while the boot validator only inspects boolean, integer, and enum leaves. So "headers": {} is the exact silent-config failure this PR exists to end, one level above the one it fixed, in the same function, under a comment claiming it was covered. redirects.js had the identical hole.

Both fixed in 4df5257b, with an absent key kept deliberately silent and asserted that way, since warning on every app that simply has no webjs.headers would be worse than the silence it replaces. Measured on the real configs: blog and website emit 0 warnings between them across both readers.

The line numbers in the body were also stale, from a run against a pre-commit revision of the file. Corrected and re-derived from the committed file, which is the whole point of checking it structurally rather than by eye.

Comment thread packages/server/src/headers.js
Comment thread packages/server/src/headers.js
The presence test read `raw` off the `&&` chain that walks `pkg.webjs.<key>`.
A chain short-circuits to whatever link FAILED, so for a non-object `pkg`
`raw` was the boolean `false` from `typeof pkg === 'object'`, and for a
falsy config block it was that block. Six shapes therefore warned about a
`headers` or `redirects` key nobody wrote, and printed a value not in the
config, in a change whose whole point is diagnostic accuracy.

It also contradicted `webjs-config-validate.js`, which returns `[]` for a
non-object config block on the stated grounds that the readers treat it as
unconfigured.

The block is now read explicitly and presence tested with `in`, so an
explicit null still warns (the author wrote the key) while every absent
shape stays silent. The old tests covered only null, the one value the
chain passes through unchanged, which is why this sat under a green suite.

Also renames the config-level message from "entry" to "config", since an
entry is an array element, and documents the behaviour on the three doc
surfaces that describe these keys.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This round found a real bug in my own guard, not a wording problem, so it was worth the round.

The presence test read raw off the && chain walking pkg.webjs.<key>. A chain short-circuits to whatever link FAILED, so for a non-object pkg the value was the boolean false produced by typeof pkg === 'object', and for a falsy config block it was that block. Six shapes therefore warned about a headers or redirects key nobody wrote, and printed false as the offending value, in a change whose entire point is diagnostic accuracy. It also contradicted webjs-config-validate.js, which returns [] for a non-object config block precisely because the readers treat it as unconfigured.

My tests covered only null, which is the one non-object value the chain passes through unchanged, so the bug sat under a green suite. Both readers now read the block explicitly and test presence with in, and the tests sweep every absent shape: a string, a number, 0, false, '', [], a falsy block, a non-object block. An explicit "headers": null still warns, since the author did write the key. Counterfactual: restore the chain and 4 tests red.

Also fixed: the config-level message called itself an "entry" when an entry is an array element, and the docs said nothing about a new user-visible boot warning. Three surfaces updated.

Comment thread packages/server/src/headers.js
Comment thread packages/server/test/headers/headers.test.js
Two doc claims overstated the symmetry between the readers. A redirect
entry carries no directives, so redirects has TWO drop levels (the key and
the entry) where headers has three (the key, the rule, the directive). The
built-ins reference said "all three levels" of both, and the server module
table said headers mirrors redirects exactly when only the presence test
does.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Final read over the whole change. No code defects. Both readers were executed over 27 input shapes and agree on every one, 'headers' in block cannot throw because hasBlock already proved the block a non-null object, the !Array.isArray(block) clause is the exact complement of validateAppWebjsConfig's guard so the two cannot disagree, the renamed warning string has no other match anywhere in the repo, and the new assertions do fail on revert.

Three documentation inaccuracies, all mine, all now fixed.

The drop-level counts overstated the symmetry: a redirect entry carries no directives, so redirects has TWO levels (key, entry) where headers has three (key, rule, directive). The built-ins reference claimed three of both and the module table said headers mirrors redirects exactly, when only the presence test does. Fixed in 8521a3d3.

The third is worth more than a correction. {"headers": null} produced NO warning before this PR and produces one now, so this is a behaviour change I described as unchanged. It is the right behaviour, since the author wrote the key and a present wrong-typed value is exactly what the warning is for, and the tests and docs already say so. But it is the one shape where the presence rule diverges from the old code, so it is now named in the PR body rather than left to be discovered. Everything else moves toward silence: eight of the sixteen no-key shapes warned before, and the truthy-non-object-pkg class is unbounded.

Stopping the cycle here. Nothing must-fix is open.

Comment thread .agents/skills/webjs/references/built-ins.md
Comment thread packages/server/AGENTS.md
@vivek7405
vivek7405 marked this pull request as ready for review August 9, 2026 09:35
@vivek7405
vivek7405 merged commit c23c4cd into main Aug 9, 2026
10 checks passed
@vivek7405
vivek7405 deleted the fix/headers-silent-rule-drop branch August 9, 2026 09:44
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