Repository navigation
refactor: expose awards validation core for mutation attribution - #1757
Conversation
Keep the awards CLI as the input/reporting adapter and exercise its extracted rules directly in the existing mutation scope. Preserve CLI diagnostics and filesystem behavior with real subprocess contracts; no runner hooks, workflow paths or thresholds change. Signed-off-by: mrbobbytables <mrbobbytables@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Deploy Preview for endusers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Architecture review (hold-gated, non-merging) The extraction is clean and faithful: On the one red check: the CodeQL "Incomplete regular expression for hostnames" alert on Not pushing/merging — fork PR, hold-gated review only. 🐝 Hive Agent: — hive: agent=architect backend=copilot model=claude-opus-4-6 copilot=1.0.88 |
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verified against the tree at f47cef0: the extraction is faithful and the tests are equivalent-or-stronger. One CI-blocking item: the new CodeQL alert on a test fixture.
What I checked:
- Extraction fidelity — the block removed from
scripts/validate-awards.mjsand the body ofscripts/lib/validate-awards.mjs::validateAwardsare token-identical; the only changes are invocation-localerrors, parameterizedstaticRoot/awardsLogoRoot, and the comment's relative path tovalidate-utils.mjs. CLI adapter behavior (input location,reportAndExit, success output,data.awards.lengthafter exit-on-error) is unchanged. - Mutation target claim —
stryker.config.mjs:62mutatesscripts/lib/**/*.mjs, so the new core is inside the existing PR-lane target as the body states; thebreak: 83threshold is untouched, and the "Mutation testing" check is green. - Tests — the 30 ported cases assert the same or stricter contracts (several moved from
assert.matchon stderr to exactdeepEqualon structured findings), and four subprocess tests retain the real CLI contracts (success output, diagnostics/exit 1, ENOENT, SyntaxError). The..-escape and non-string-logo edge cases survive the port (tests/validate-awards.test.mjs:316-343).
The blocking item — the CodeQL check fails with one new high-severity alert: "This string, which is used as a regular expression, has an unescaped '.' before 'cncf.io'", anchored at tests/validate-awards.test.mjs:246 (the deliberate https://www.cncf.io.evil.example/… lookalike fixture). The sink is the dynamically built regex in the allow-list test (new RegExp(${field} must be on ) at ~line 252): CodeQL's taint tracking connects the fixture string to a RegExp construction and applies js/incomplete-hostname-regexp. No production code builds a regex from these values — checkUrl in scripts/lib/url-safety.mjs:99-170 uses new URL plus exact/endsWith('.' + domain) host matching — so the alert is a false positive in effect, but it keeps the PR UNSTABLE and the automerge/queue tooling will not pass it.
Two clean resolutions, either of which the author can do without touching production code:
- Replace the dynamic regex with a plain string check, e.g.
errors.some((e) => e.severity === 'error' && e.message.startsWith(${field} must be on))— removing the RegExp sink kills the alert outright; or - A maintainer dismisses the alert as a test-fixture false positive.
Option 1 is preferable since it needs no maintainer action. Everything else looks correct to me; the Refs #1752 non-closing citation is justified in the body (bounded one-validator pilot, rollout decision deliberately left open). This is a fork branch, so the change has to come from the author.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
|
Quality lane — red The required EvidenceCheck run 119421593381 reports "1 new alert including 1 high severity security vulnerability". Its single annotation:
Line 246 is the hostile negative-test fixture Why it is PR-localOn for (const [field, url] of [
['announcementUrl', 'https://www.cncf.io.evil.example/announcements/x'],
...
]) {
assertError(..., new RegExp(`${field} must be on `));
}
Suggested replacementCarry the expected pattern as a literal test('rejects a link field whose host is outside the allow-list', () => {
for (const [field, url, message] of [
[
'announcementUrl',
'https://www.cncf.io.evil.example/announcements/x',
/announcementUrl must be on /,
],
[
'caseStudyUrl',
'https://evil.example/case-studies/acme',
/caseStudyUrl must be on /,
],
['talkUrl', 'https://vimeo.example/9', /talkUrl must be on /],
]) {
assertError(
validateFixture(awardsFixture([{ ...validEntry, [field]: url }])),
message,
);
}
});Verification of the replacementApplied to this PR's head (
That is the same 101/0/100% this PR reports, so the change costs the pilot nothing — it only removes the dynamic-regex construction CodeQL objects to. I have not verified this against CodeQL itself; only a fresh run on the updated head can confirm the alert clears. Why no push from meThis PR's head is |
Preserve the host-rejection assertions without dynamic RegExp construction that confuses CodeQL tuple-value inference. Keep the hostile hostname fixtures and production validation unchanged. Signed-off-by: mrbobbytables <mrbobbytables@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Fixed the failed CodeQL check in 3a3ca3d. The host-rejection test now carries literal expected regex patterns in its fixture tuples instead of constructing RegExp from a tuple-derived string. Hostile hostname inputs and assertion meaning are unchanged; production code is untouched. Fresh CodeQL check 119475296321 is SUCCESS on the repaired commit, with no suppression or alert dismissal. All 34 focused tests pass and the normal scoped mutation run remains 101 killed / 0 survived / 0 NoCoverage / 0 timeouts / 0 errors (100%, threshold83 unchanged). Remaining CI continues normally; no merge requested. |
There was a problem hiding this comment.
Looks correct to me. I verified this by reading the tree at the PR head, not just the diff.
What I checked:
- Faithful extraction. The validation body moved from
scripts/validate-awards.mjsintoscripts/lib/validate-awards.mjs:30-121is statement-for-statement identical to the removed code (samelastYear = Math.max(entry.year, 2015)ordering guard, same resolve-then-prefix logo containment check atscripts/lib/validate-awards.mjs:97-105, same host allow-lists). The only changes are invocation-localerrors/awardsLogoRootand the explicitstaticRootparameter. - Adapter unchanged.
scripts/validate-awards.mjskeeps JSON loading,reportAndExit, and the success line;package.json:52(validate:awards) still points at it, and running it against the checked-indata/awards.jsonat this head printsValidated 15 award entrieswith exit 0. - Tests.
node --test tests/validate-awards.test.mjspasses 34/34 locally at head3a3ca3d. The converted cases now assert exact finding objects (e.g. the non-array-awards case attests/validate-awards.test.mjs:43-50pins a single error with no cascading per-entry noise), and four real subprocess contracts remain (tests/validate-awards.test.mjs:369-400) covering CLI success, diagnostics, missing input and malformed JSON. - Scope matches the body. 3 files changed, exactly as described; no workflow, Stryker config, or threshold edits. The new core falls under the existing
scripts/lib/**mutation target, and the Mutation testing check is green on this head.
The PR body says Related to #1752, deliberately not an automatic closing reference and "Leave unmerged for review" — consistent with the bounded one-validator pilot it describes, so the non-closing reference looks intentional, not an oversight. No findings from me; merge timing is the maintainer's call as the author requested.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
Summary
Implement the maintainer-approved, one-validator callable-core pilot for
#1752.
scripts/lib/validate-awards.mjs::validateAwards(data, staticRoot), returningstructured findings with invocation-local state and an explicit assets root.
scripts/validate-awards.mjsas the input/reporting adapter. JSON loading,input locations, exit behavior, diagnostic formatting and success output remain
unchanged.
files for asset checks. Keep four actual subprocess contracts for CLI success,
validation diagnostics, missing input and malformed JSON.
imprecise assertions, including avoiding cascading entry errors for a
non-array awards collection.
The extracted core is covered by the existing
scripts/lib/**/*.mjsmutationtarget. No Stryker internals, child-output bridge, generic fixture-helper change,
other validator conversion, workflow edit, mutation-glob change or threshold
change is included.
Measured mutation evidence
Using the installed Stryker/TAP runner 10.0.0 on local Node v26.11.1:
Final result: 101 killed, 0 survived, 0 NoCoverage, 0 timeouts, 0 errors;
100% mutation score, with the existing break threshold of 83 unchanged.
Known inclusive-year-boundary and same-year-ordering mutants are killed.
The first direct-core run was 95 killed / 6 survived / 0 NoCoverage. Those six
changed metadata diagnostics or added misleading per-entry errors for an invalid
collection; strengthening the existing diagnostic-contract assertions killed
them without changing validation logic.
This is a scoped core result, not a full-tree baseline or a claim about the
remaining CLI validators. Its 101 mutants are not the same target set as the
historical 104-mutant top-level-script pilot.
Verification
node --test tests/validate-awards.test.mjs: 34 passed.npm run test:unit:coverage:check: 3,026 passed; source 100% lines/regions,harness 100% lines / 99.77% regions; all enforced per-file floors retained.
npm run build:productionpassed.match the original script. The checked-in awards file still validates through
the real CLI.
Based on main
342b7f8; implementation commitf47cef0a650bba3c4d7a0ceb24831b340d4db7ac.Related to #1752, deliberately not an automatic closing reference.
The maintainer approved this bounded pilot; broader rollout and final issue
closure remain separate decisions. Leave unmerged for review.