Weekly QA report 2026-07-13 + full fix-up: all findings to Met#265
Draft
lupinesse wants to merge 26 commits into
Draft
Weekly QA report 2026-07-13 + full fix-up: all findings to Met#265lupinesse wants to merge 26 commits into
lupinesse wants to merge 26 commits into
Conversation
Scoreboard: 74 Met / 8 Partial / 1 Not met / 4 N/A (87 total). Top finding: #251's @commitlint/config-conventional bump (^21.0.2 -> ^21.2.0) pulls in ESM-only conventional-changelog-conventionalcommits ^10.0.0, which crashes the commit-msg hook on every invocation (ERR_PACKAGE_PATH_NOT_EXPORTED) -- reproduced standalone, unrelated to this commit's own message. This commit uses --no-verify solely to work around that crash; the report documents it as priority 1 (revert or fix the pin) and downgrades "Committing standards followed" to Not met. Also flags newly-measurable Prettier format:check drift on 5 files (not wired into CI or npm run lint), two documentation items upgraded now that npm ci/docs/build/lint/tests all run locally in this environment, and corrects a bookkeeping error in the 2026-07-06 report's own scoreboard (its Full Results table sums to 73/10/4, not the printed 77/6/4). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ApCMeEybbs7c3ZFU9Rznp
github-actions
Bot
added
the
chatgpt-review
#251 bumped this to ^21.2.0, which resolves conventional-changelog- conventionalcommits ^10.0.0 (ESM-only, no CJS `exports` condition). Every commit crashed the Husky commit-msg hook with ERR_PACKAGE_PATH_NOT_EXPORTED, regardless of message content - the 2026-07-13 QA report had to be committed with --no-verify to work around it. Pinned to the last known-good exact version (21.0.2, which resolves conventional-changelog-conventionalcommits ^9.2.0) rather than a caret range, since a caret range would let the next `npm install` silently re-resolve to 21.2.0 and reintroduce the break.
npm run format:check was failing on 5 files (unwrapped long lines Prettier would reflow) with no CI job checking it, so the drift was invisible to reviewers - only npm run lint (ESLint + Stylelint) runs in CI today, and Prettier isn't part of that. Reformatted the 5 files and added a Format check step to the lint job so future drift fails CI instead of accumulating silently.
Impact AnalysisChanged modules
Downstream dependants
Test coverage for changed modules
Automated analysis · commit |
JSDoc CoverageSummary
🟡 Partial — incomplete JSDoc[src/js/pure-fns-export.js line 102] [src/js/pure-fns-export.js line 303] [src/js/pure-fns-tasks.js line 93] [src/js/pure-fns-tasks.js line 167] Verdict: WARN Automated check · commit |
lupinesse
changed the title
Accessibility Audit (WCAG 2.1 AA)
|
Renames the accumulated Unreleased section to [1.9.1] - 2026-07-13 and adds an empty Unreleased placeholder at the top. Bumps package.json version 1.9.0 -> 1.9.1. Refreshes QA.md's test-count delta table (was one release stale at 466/73; now 474/74 unit, 171/32 CI-scripts, verified locally this session) and records the v1.9.1 tag reference. No later git tag existed since v1.9.0 (2026-06-12) despite 5+ merged PRs since - this closes that gap. The actual `git tag v1.9.1` is applied after this lands on main, matching how v1.9.0 was tagged.
"Users encouraged to participate in peer review" was Partial in the QA audit because CONTRIBUTING.md documented the AI-pair review model but never actually invited outside review. The repo is public and MIT-licensed already; this makes the existing openness explicit and discoverable (PRs, issues, and comments on open work all count), rather than leaving it implicit.
Part of the QA-audit sweep to eliminate ambiguous single-letter arrow- function parameters. Pure renames only, no logic changes; each rename checked against its enclosing scope to avoid shadowing (documented per file in the PR). One index-counter exception left intentionally in 14-jira.js's other call sites is itself renamed here for full consistency (i -> index). Files: 11-timeflow.js, 12-misc.js, 14-jira.js, 15-notion.js, pure-fns-export.js. Remaining files are still being swept in parallel and will land in follow-up commits.
Continuation of the lambda-naming sweep. Pure rename only, checked against enclosing scope for shadowing; no logic changed.
Continuation of the lambda-naming sweep. Pure renames only, checked against enclosing scope for shadowing; no logic changed. Files: 05a-export.js, 08a-pomo-dashboard.js, 09-clock-weather.js, 10c-tasks-board.js, 12a-changelog.js, 13-calendar.js, 16-rapid.js, 18-dailylog.js, 23-sprints.js.
Continuation of the lambda-naming sweep. Pure renames only, checked against enclosing scope for shadowing; no logic changed. Files: 01-state.js, 05b-filesystem.js, 08-pomodoro.js, 10a-tasks-row.js, 11-timeblock.js, 25-rollingsummary.js.
Continuation of the lambda-naming sweep. Pure renames only, checked against enclosing scope for shadowing; no logic changed. Files: 01b-migrate.js, 03-timer.js, 06-focus.js, 06a-hero.js, 21-reflection.js, pure-fns-tasks.js.
Continuation of the lambda-naming sweep. Pure renames only, checked against enclosing scope for shadowing; no logic changed. Files: 10a-tasks-render.js, 10b-signifiers.js, 19-monthlylog.js, 20-migration.js, pure-fns-validate.js.
Completes the lambda-naming sweep across src/js/ - the QA audit's "Names are explicit rather than implicit" finding (280 occurrences) is now fully resolved: 0 single-letter arrow-function parameters remain anywhere in src/js/. Pure renames only, checked against enclosing scope for shadowing; no logic changed. Files: 07-lifecycle.js, 22-trackers.js.
04-render.js was 646 non-blank lines mixing three distinct concerns: the entry-timeline render+event-binding orchestrator, two independent secondary panel renderers (quick-pick, chart), and four small formatting/DOM helpers used only by the inline time editor. Split into: - 04-render.js (425 lines) - core render() orchestrator, unchanged behavior - 04a-render-widgets.js (187 lines) - renderQuickPick(), renderChart() - 04b-render-time-helpers.js (44 lines) - closeAllEditors(), toTimeInput(), applyTime(), durLabel() No top-level eager execution in the original file (confirmed before splitting), so alphabetical concatenation order needed no changes. Pure move, no logic changed - verified via build/lint/format/both test suites, all passing. ARCHITECTURE.md's module map updated for the new files; the source-file-count line at the top will be refreshed once the remaining large-module splits land.
13-calendar.js was 494 non-blank lines mixing the Outlook meetings fetch/render orchestrator with an unrelated self-contained feature (the post-meeting "build a bridge to your next task" banner), already visually separated in the source with its own comment divider. Split into: - 13-calendar.js (351 lines) - calAccountLabel(), renderCalStrip(), fetchAndRenderCalendar(), plus the bootstrap tail and window.__wl test harness (left in place per 12c-startup.js's documented load-order constraint) - 13a-calendar-bridge.js (151 lines) - getSeenEnded/setSeenEnded/ getMeetingKey, showBridgeBanner(), buildBridge(), fetchBridge() No top-level eager execution moved (bridge functions are only called from inside 13-calendar.js's own setInterval), so alphabetical concatenation order needed no changes. Pure move, no logic changed - verified via build/lint/format/both test suites, all passing.
09-clock-weather.js was 565 non-blank lines mixing three unrelated concerns: the live clock tick + midnight rollover, Open-Meteo weather fetch/render, and a Finnish nameday/flag-day/moon-phase almanac. Split into: - 09-clock-weather.js (82 lines) - tickClock()/getISOWeek()/ totalISOWeeks(), plus the jiraTicketHtml() orphan helper (flagged for a future move, not touched here) - 09a-weather.js (116 lines) - weatherEmoji(), fetchWeather(), its config-fetch bootstrap and refresh interval - 09b-almanac.js (370 lines) - getMoonData/renderMoon, flag-day calculation + fallback list, fetchNameday()/fetchCalendarEvents(), plus the original file's bootstrap-tail calls (kept together and in their original relative order, since they run after this file's own eager calls in both the old and new layout) Alphabetical concatenation (09 < 09a < 09b) preserves the exact original top-level execution order: tickClock() eager call, then the weather config-fetch kickoff + interval registration, then the almanac fetch kickoffs and bootstrap tail - verified by reading the original file's eager-execution sequence before splitting. Pure move, no logic changed - verified via build/lint/format/both test suites, all passing.
06a-hero.js was 512 non-blank lines mixing the Hero Card state machine (render + panel fills + clock tick) with its button action handlers and a distinct category quick-switch picker widget. Split into: - 06a-hero.js (274 lines) - heroGetState()/renderHeroCard()/panel fills/heroUpdateClock()/heroEnterStopped(), plus the _heroLastNoteText()/_heroSessionCount() formatting helpers - 06b-hero-events.js (143 lines) - _heroHandleStart/_heroStartFromChip/ _heroHandleUndo/_heroHandleDone action handlers and initHero() - 06c-hero-category.js (113 lines) - _heroSetCategory()/ _heroBindCatPicker()/_heroCatSelect(), the category dropdown widget No top-level eager execution in the original file (confirmed before splitting), so alphabetical concatenation order (06a < 06b < 06c) needed no changes. Pure move, no logic changed - verified via build/lint/format/both test suites, all passing.
11-timeflow.js was 536 non-blank lines mixing the tab-switching orchestrator with the section-header chrome (day strip, gap reminder, totals) and the entire Flow tab's own render + note editor. Split into: - 11-timeflow.js (142 lines) - view-state orchestrator: renderTodayFlow(), getFlowView()/setFlowView(), focusTabAt(), initTodayFlow() - 11c-timeflow-header.js (196 lines) - renderFlowHeader(), renderDayStrip(), renderGapReminder(), findLargestGap(), activeTimerDurationMs(), stripPct(), tsToMins(), fmtHm() - 11d-timeflow-flowview.js (217 lines) - renderFlowView(), partitionSessionNotes(), buildSessionNotesHtml(), buildFlowTaskNoteHtml(), bindFlowNoteEvents() 11a/11b were already taken by the unrelated 11-timeblock.js module, so the new siblings use 11c/11d. No top-level eager execution in the original file, so alphabetical concatenation order needed no changes. test/unit.mjs's loadTimeflowSandbox() read only 11-timeflow.js's source into its VM sandbox to test findLargestGap/activeTimerDurationMs/ stripPct/partitionSessionNotes/buildSessionNotesHtml in isolation - moving those functions out broke 35 unit tests until the sandbox source was updated to concatenate all three sibling files (same pattern already used for the pure-fns-*.js sub-modules). Caught by running the full test suite before committing, not just build/lint. Pure move otherwise, no logic changed - verified via build/lint/format/ both test suites, all passing (474/74 unit, 171/32 integration).
58 files now (was 49) after splitting 04-render.js, 09-clock-weather.js, 11-timeflow.js, 06a-hero.js, and 13-calendar.js. The design certificate (version/reviewed-by/status) is intentionally left untouched here - that requires the human author's own re-review given the scope of these changes, not something to claim on their behalf.
An adversarial review of the accumulated diff (280-lambda rename sweep
+ 5 module splits) found the 5 module splits mechanically sound
(function-for-function verified, no duplication/truncation/dangling
references) but flagged:
- QA.md claimed "v1.9.1 tags this delta" and an unchanged, still-valid
ARCHITECTURE.md certificate - both premature. No v1.9.1 git tag
exists yet (tagging happens post-merge), and the 5 module splits ARE
architecturally significant, so the certificate genuinely needs the
human author's re-review, not a claim that nothing changed. Reworded
both rows to reflect reality instead of anticipated state.
- 02-utils.js's lambda-rename sweep renamed most `categories.find/filter`
callbacks to `category` but left two as `cat` - harmless (no shadowing,
separate scopes) but inconsistent with the sweep's own stated goal of
descriptive, unambiguous names, and `cat` is also used elsewhere in
this file for getCat()'s normalized return value. Renamed both to
`category` for consistency.
- 06b-hero-events.js's `_heroStartFromChip` renamed a `.find()` callback
to `entry`, the same identifier as the function's own later
`const entry = {...}` a few lines down. Safe at runtime (callback
param is out of scope before the const initializes) but exactly the
kind of same-function name reuse the sweep was meant to avoid -
contrast with `_heroHandleUndo` a few lines above, which correctly
used `logEntry` to sidestep this. Renamed to `existingEntry`, matching
the variable it's assigned to.
Verified via build/lint/format/both test suites, all passing.
lupinesse
changed the title
CodeQL flagged js/xss-through-dom (high) on renderTagRow(): the raw <input type="color"> value flowed into cat.color and back into innerHTML/style without an explicit sanitizer at the source or sink. getCat() already ran safeCssColor() internally, but CodeQL's taint tracking didn't credit that as a barrier across the object spread. Apply safeCssColor() at the DOM read (quickColorPick.value) and again at the innerHTML sink, closing the flagged path with no behaviour change.
4 tasks
04a/04b-render, 06a-hero, 09a/09b-clock-weather, and 13a-calendar split several pure/localStorage-backed functions into new sibling files with no dedicated unit tests of their own (weatherEmoji, getMoonData, the flag-day date helpers, the inline time-editor helpers, and the transition-bridge seen-meeting tracking). Add VM-sandbox tests for each, following the existing extract-just-the-function pattern used for updateHeaderTracking, so these don't regress silently in future splits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The CodeQL js/xss-through-dom fix on renderTagRow() (commit 7514698) shipped without a dedicated regression test, which CLAUDE.md requires for every bug fix. Add four VM-sandbox tests covering the input and change handlers, the innerHTML render sink, and valid-hex passthrough. Two of them fail if the safeCssColor barrier is removed, so the fix is now pinned by tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@commitlint/config-conventional is exact-pinned to 21.0.2 to avoid the ESM-only transitive dep that crashes the commit-msg hook, but the cli was left on ^21.0.2 and could float independently (e.g. dependabot #264 to 21.2.1) into an untested combination. Pin the cli to the same exact version so the pair upgrades together. Lockfile regenerated; the commit-msg hook was verified on good and bad sample messages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… pin Add a Security entry under v1.9.1 for the renderTagRow safeCssColor sanitisation, and extend the existing commit-msg hook entry to note the @commitlint/cli exact pin. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three js/xss-through-dom alerts have been open on main since 2026-05-28
(02-utils.js, 12-misc.js, 20-migration.js). All three are the same false
positive: getCat() returns { ...cat, color: safeCssColor(cat.color) } and
that colour is interpolated into innerHTML.
safeCssColor is a strict allowlist — it returns its input only when it
matches an anchored hex/hsl regex and otherwise returns the constant
'#888780', so its output cannot contain HTML meta-characters. CodeQL
cannot infer this: the build concatenates src/js/*.js into one classic
script, so callers reference safeCssColor as a free global rather than
importing it, and the sanitiser sits behind an object spread the taint
tracker does not follow. Adding safeCssColor calls at the source and sink
(7514698) did not convince it either.
Add a repo-local model pack declaring safeCssColor's return value as a
barrierModel for html-injection, resolving all three at the root instead
of dismissing them one by one. security-extended moves from the workflow
queries input into the new config file, which also loads the pack.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This reverts commit 34b509d.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Started as the weekly automated QA audit report. At the user's request ("fix issues", then "the goal is to have all findings as Met, no partials left"), it grew into a full remediation pass addressing every finding in the report.
Before → after: 74 ✅ / 8⚠️ / 1 ❌ / 4 ➖ → all 83 scoreable items now ✅ Met or ➖ N/A, with one exception noted below (tagging + certificate re-review are pending human/post-merge steps, not code).
What changed, in order
QA audit report (
docs/qa-reports/qa-review-2026-07-13.md) — audit only, no code changes.Fixed the broken commit-msg hook —
#251bumped@commitlint/config-conventionalto^21.2.0, which resolved an ESM-onlyconventional-changelog-conventionalcommits@^10.0.0with no CJSexportscondition. Every commit crashed withERR_PACKAGE_PATH_NOT_EXPORTED. Pinned back to the last known-good exact version (21.0.2).Fixed Prettier formatting drift on 5 files and wired
npm run format:checkintoci.ymlso this can't silently recur.Consolidated the release:
CHANGELOG.md'sUnreleased→[1.9.1],package.jsonversion bump, refreshedQA.md's stale test-count table.Made community review an explicit invitation in
CONTRIBUTING.md(was implicit; now genuinely documented, not just tolerated).Renamed all 280 single-letter lambda-function parameters to descriptive names across 34 files — pure renames, each checked against its enclosing scope for variable shadowing. Verified 0 remaining via
grep -roP '\([a-z]\) =>' src/js/.Split the 5 oversized modules flagged by the audit into smaller, single-purpose siblings, following the project's existing numbered-suffix convention:
04-render.js(646→425 lines) →04a-render-widgets.js,04b-render-time-helpers.js13-calendar.js(494→351 lines) →13a-calendar-bridge.js09-clock-weather.js(565→82 lines) →09a-weather.js,09b-almanac.js06a-hero.js(512→274 lines) →06b-hero-events.js,06c-hero-category.js11-timeflow.js(536→142 lines) →11c-timeflow-header.js,11d-timeflow-flowview.jsEach split was checked before touching code: no top-level eager-executing statements that would create load-order dependencies (build.js concatenates
src/js/*.jsalphabetically into one shared-scopescript.js), and every moved function's cross-file callers were grepped first. One real bug was caught this way:test/unit.mjs'sloadTimeflowSandbox()read only11-timeflow.js's source into an isolated VM sandbox — splitting broke 35 unit tests until the sandbox source list was updated to concatenate all three sibling files (same pattern already used for thepure-fns-*.jssub-modules).ARCHITECTURE.mdupdated throughout: module map entries for all 9 new files, line counts, and the overview file count (49→58). The design certificate'sApprovedstatus andReviewed byfield were deliberately left untouched — 5 architecturally significant splits genuinely warrant the human author's own re-review, not a claim made on their behalf.Adversarial
/pr-reviewon the full accumulated diff (57 files, ~4850 lines) found the module splits mechanically sound (function-for-function verified — zero duplication, truncation, or dangling references) but caught:QA.mdclaiming av1.9.1tag existed when it didn't yet, and two minor naming inconsistencies left by the lambda sweep (cat/categorymixed in one file; a harmless-but-confusing same-name reuse in06b-hero-events.js). All three fixed and re-verified.What's still pending (not code — process steps)
git tag v1.9.1: needs to happen onmainafter this merges, matching howv1.9.0was tagged.QA.mdnow says so explicitly instead of claiming it's already done.ARCHITECTURE.mdcertificate re-review: the module splits are architecturally significant; the human author should re-review and bumpDocument version/Last reviewed/Reviewed bythemselves.Pre-submit checklist
npm run buildpassesnpm run lint— 0 errors (1 pre-existing, unrelated warning about the gitignored rootscript.jsmatching the lint glob)npm run format:check— cleannpm test— 474/74 unit + 171/32 integration tests pass throughout every commit on this branchnpm run build's output is byte-identical in structure and all automated tests passCHANGELOG.mdupdated (the v1.9.1 consolidation commit)Notes for reviewers
This PR is large for a single review pass (57 files). The safest way to review it is commit-by-commit rather than as one flat diff — each commit is a single discrete change (hook fix, format fix, release consolidation, docs, one lambda-rename batch per commit, one module split per commit, review-fixups) and was verified independently with the full build/lint/test suite before landing.
git log --oneline origin/main..HEADgives the ordered list.