Add new hummingbird QA skill

[tblivet]

Questions	Answers
Description?	Adds hummingbird-theme-qa to the qa domain. It reads the theme's checklist (docs/qa/testing-checklist.md), drives the shop in a browser, and writes a report of what was checked, what was found, and what was not covered. The checklist is the only source of what gets tested: the skill reads the one shipped with the theme under test, from the release tag when there is one, and carries no copy.
Type?	new skill
Domain / Category?	qa / dev
How to test?	--
Fixed issue?	--
Related PRs	Depends on PrestaShop/hummingbird#1104, which adds the checklist. Until it is merged no tag carries the file, so the skill reads the branch and says so in the report.
Sponsor company	@PrestaShopCorp

[mattgoud]

Impressive design, and the parts I could exercise hold up. I couldn't run the PR's own code (unreviewed external code), so I re-implemented parse()'s classification rules myself and ran my version against the real checklist from PrestaShop/hummingbird#1104. It behaves exactly as documented:

• 219 points across 32 sections
• all 17 tables classified correctly. The six | Module | tables and the four | Setting | tables and the | BO tab | one come out as TESTS; Field, Profile, Product type, Breakpoint, Kind and Source come out as reference. TEST_TABLE_HEADERS is exactly right for this checklist
• the (config) inference finds 3.7 through its prose line and 4 through its explicit declaration, and propagates 4 down to 4.1-4.5. The section.id !== '0' guard correctly stops the preamble's "Items marked (config)" sentence from flagging everything
• #### Option A/B/Both options have no leading number, so they correctly stay inside 1.2 rather than opening sections

Matching points on textSha256 rather than position is the right call, and the decision to carry no copy of the checklist means the fix I asked for on #1104 flows through here with no change on this side. Worth saying explicitly.

One blocker below, then a few real ones.

---

The headline guarantee is not implemented

SKILL.md states it twice, and the README repeats it:

Every green points at a file. report.js refuses to build a report that claims a check whose evidence is not on disk.

will not build a report whose evidence is missing

Nothing enforces that. All three gates check the assertion and never the evidence:

• report.js verify(): if (o.outcome === 'pass' && !o.assertion)
• observe.js:50: same condition
• record.js:79: same condition

An observation with evidence: [] is accepted everywhere. The evidence loop only checks that files named in o.evidence exist, so an empty array has nothing to fail on, and proofRows() then renders that row as settled, holds with an em-dash in the evidence column.

It isn't theoretical, and there are two ways in:

1. The documented path. SKILL.md:164's own canonical observe.js example passes no --evidence. Run it as written and you get an unbacked green. references/suites.md doesn't mention evidence for a pass either, it names it once as something snap() happens to produce.
2. The silent one, which is worse. snap() (run-suite.js:408-413) and capture() (:81-86) both catch { return null; }. Every built-in check does evidence: [shot], and record.js:92 does evidence.filter(Boolean), so a null shot becomes []. If the screenshot path breaks — a headless quirk, a read-only out, a page that never settles — every check in the campaign still passes, with no evidence and no complaint. The report says settled, holds throughout and nothing anywhere says the proof went missing.

The whole argument for this skill is that the tooling enforces the guarantee rather than good intentions. Right now the assertion half is enforced and the evidence half is not.

Fix is small: add if (o.outcome === 'pass' && !(o.evidence || []).length) to the same three places, make snap()/capture() returning null a fault() rather than silence, and add --evidence to the SKILL.md:164 example so the documented path is the correct one.

---

Everything else is inline. The two pick-run-dir.sh points are both about the rule 4 promise ("Evidence never lands in the theme folder") being weaker than it reads.

Happy to re-review quickly once the evidence gate is in.

[tblivet]

Thanks @mattgoud useful review!

The five inline points are fixed: URL placeholder check, theme folder as a served-dir, ambiguous table prefix, QA_SQL, and refs/tags.

On the blocker, I agree the guarantee wasn't implemented but I didn't take the fix as written. Only accessibility attaches evidence on a pass; open, see, count, moduleRenders, noPageErrors and loginBO don't. Gating all three places would outlaw them and force a screenshot for "the page answered 200".

Instead: a browser pass leans on the screenshot of the step it ran in, and a pass from a command or a person has to name its own file. A step that threw lends its picture to nothing. Your snap()/capture() point is in as written, a failed screenshot is a fault now.

Two others while I was in there: record.js wiped the folder observe.js writes into, so browser runs were deleting answers recorded by command; and the settings baseline was written to a relative path before the campaign folder existed.

Also added gaps.json and --require-complete, so a campaign can be run to the end without someone reading the report between sections.

[mattgoud]

You were right to refuse the fix as I wrote it, and the one you built is better. Mine would have forced a screenshot out of open, see, count, noPageErrors and loginBO just to satisfy a rule, which is proof by paperwork. Tying the green to the step it ran in is the real relationship, and "a step that threw lends its picture to nothing" is the part I would not have thought of: the screenshot is of the wreckage, so it says nothing about the checks that ran before the throw.

I traced the whole chain on the new head rather than taking the description at its word:

• _proof falls back to the step shot only when !st.threw, and verify() refuses a pass whose proof resolves to nothing. A pass recorded outside any step gets step: null, Number(null) is 0, no step is numbered 0, so it lands on _proof = [] and is refused. The Number() on both sides is also what bridges steps[].n being the padded string "01" while observations[].step is the number 1 — that had to be deliberate
• observe.js now requires --evidence on a pass, and SKILL.md:196's example carries one, so the documented path no longer produces the thing the skill says is impossible
• capture() lets the throw out and record.js turns it into a fault, snap() faults, and if (!shot) fault(...) catches the quiet null as well. All three holes closed
• the checklistSha256 chain is consistent end to end: run-suite.js:102 → record.js:255 → report.js:189, and both SKILL.md call sites pass --checklist-sha. Refusing a run that never declared one, rather than only comparing when present, is the right way round

On the two you found while in there: startSuite wiping the folder observe.js writes into was a genuine bug and a nasty one, since the report would then have read those points as never looked at. And the baseline going to a relative baseline/settings.json before $RUN existed is exactly the kind of thing that only shows up on someone else's machine. Reordering so the folder is picked before anything is written, and the || exit 1 note so a refusal cannot leave $RUN empty and write into /suites, are both good.

Two of my own points came back better than I asked for them. The prefix one I got wrong: if (exact.length > 1) die(...) as I wrote it would have broken ordinary single-shop installs that happen to carry something like ps_smartblog_configuration. Probing for a matching shop_url is the right discriminator. And on the URL check you also fixed the IPv6 port parsing that PORT=${HOSTPORT##*:} got wrong, which I had not spotted.

Approving. Three things left, none of them blocking, but the first is a real bug in the new carry-over code and worth fixing before this goes in.

[mattgoud]

Correcting my own review: I approved and in the same breath wrote that the carry-over bug was worth fixing before merge. Those two do not go together, since the approval is what removes the gate that would ensure it. Switching to changes requested so the state matches what I actually meant.

Nothing new to look at, and nothing has changed in my assessment of the branch. Everything I said in the previous review stands: the evidence guarantee is properly implemented now, your design for it is better than the one I proposed, and the two bugs you found yourself were real ones.

The one thing blocking is the spare set in record.js, which holds evidence paths as recorded while readdirSync yields top-level entries, so evidence named with a subpath gets swept away by the block that exists to stop answers being deleted. Details and the repro are in the inline comment on that line. One line to fix.

The other two inline comments are nits and I do not want them read as conditions: drop them, argue them, or take them, whatever you prefer. Re-approving as soon as the sweep is sorted.

[mattgoud]

All three are in, and the one that was blocking is right. 41d4539 touches only record.js and fingerprint.js, so nothing else I had already read has moved.

I replayed my repro against the fixed sweep rather than assuming the one-liner did it:

spare   = [ 'run.json', 'shots', 'flat.txt' ]
readdir = [ '01-home.png', 'flat.txt', 'run.json', 'shots' ]
  DELETE 01-home.png     <- stale shot from the previous run, still swept
  KEEP   flat.txt
  KEEP   shots
shots/lint.txt survives? true

Subpath evidence survives, flat evidence survives, and the sweep still does its job on what the replaced run left behind, which was the point of the block in the first place.

You took the two nits as well, which I had explicitly said were not conditions. The carry-over filter is now just recordedBy === 'observe.js', and I checked the other half of the pair: observe.js:116 sets that marker on every answer it writes, so the two agree and there is no path left that carries a stale non-pass into a re-run. The --prefix guard turns a typo into one of this file's own refusals instead of a raw client error, which is the bit that mattered.

One leftover, not worth a round on its own: --evidence=./lint.txt yields '.' as the spared segment, which readdirSync never returns, so that file would still be swept. path.normalize on the way into spare would close it. Worth doing only if you are in that function again for another reason.

Approving. Thanks for the three rounds, and in particular for pushing back on my first fix, which was the wrong shape.