Add prestashop-pr-qa guide an AI agent through QA of a pull request

[tblivet]

Questions	Answers
Description?	Adds a new qa domain with one developer skill, prestashop-pr-qa. It QAs a pull request in a real browser against an already-running shop: reads the PR and its linked issue, reproduces the bug on the pre-fix code, verifies the fix on the PR's code, and writes a report stating approved / not approved / not reproducible with video and per-step screenshots as proof. Works for core, modules, themes and libraries, front office and back office. The skill is prose-only (SKILL.md plus two references/ files) it ships no executable code. The developer switches the shop between the two states; the skill prints the commands, waits, then verifies the state actually changed before measuring. Nothing is ever posted to GitHub. Also updates README.md: new domain section, inventory row, directory tree, and a contribution step recommending references/ for long knowledge so SKILL.md stays focused.
Type?	new skill
Domain / Category?	qa / dev
How to test?	Install with npx skills install PrestaShop/skills/qa/dev, then with a PrestaShop shop running, either invoke it directly in Claude Code: /prestashop-pr-qa or just describe the task: QA the PR PrestaShop/hummingbird#1092 against http://localhost:8887 The skill asks for the shop URL, asks you to put the checkout on the PR's merge base, runs the reproduction, asks you to switch to the PR branch, runs the verification, and writes report.md with the verdict plus before/ and after/ videos and screenshots. Validated on PrestaShop/hummingbird#1092 (keyboard accordion toggles): reproduced on the merge base, confirmed fixed on the PR head, verdict approved.
Fixed issue?	--
Related PRs	Run against this PR while testing the skill: PrestaShop/hummingbird#1089
Sponsor company	@PrestaShopCorp

[ps-jarvis]

Hello @tblivet!

This is your first pull request on skills repository of the PrestaShop project.

Thank you, and welcome to this Open Source community!

[mattgoud]

Thanks for this. The two-phase design with the state canary is a real addition to how we QA PRs today, and splitting long knowledge into references/ is the right call for this repo. I reviewed the content, checked the packaging against the skills CLI, and cross-checked the process against our QA onboarding documentation and the labels actually in use across our repositories. Notes below, roughly in priority order. Nothing here is blocking on its own.

Two concrete bugs

1. The run-directory guard never runs. In SKILL.md the loop is for GUARD in "$CHECKOUT" "$SHOP_ROOT", but neither variable is defined anywhere in the skill. [ -d "" ] fails, continue skips both iterations, and the protection that keeps the video and the screenshots out of the checkout and out of the document root is a no-op. Both paths should be asked for in Requirements, or SHOP_ROOT derived from the shop URL. Minor: the mkdir -p happens before the check.

2. The flake re-sample is inert in the scenario template. assert.bug accepts a function so a failing check can be re-evaluated, but the template captures the value first:

const shown = (await page.locator('...').first().innerText()).trim();
await assert.bug('...', async () => shown === '...', `observed "${shown}"`);

The closure returns the same shown on the second call, so the re-sample proves nothing. The template is the part that gets copied for every run, so it should re-read the DOM inside the callback.

Related: when the re-sample flips, assert.bug records passed: false, flaky: true and pushes to harness, while the comment says the assertion is "ignored". Worth making the two agree.

Packaging: run.js should be a file, not a fenced block

references/runner.md ships a ~200 line program with "copy it verbatim". That program is the judge of the whole run: it decides what is a precondition, a bug assertion, a harness error. A single line retyped differently changes a verdict silently, and nothing can lint, test or diff it between versions.

The skill format has a place for this: scripts/ next to SKILL.md, invoked by path. I checked the installer rather than assuming:

• npx skills add 'tblivet/skills_fork/qa/dev#feat/prestashop-pr-qa' installs SKILL.md plus both references/*.md intact, so your references/ split works as intended today.
• src/installer.ts copyDirectory recurses into subdirectories (excluding only .git, __pycache__, __pypackages__ and metadata.json) and applies the source file mode. Verified with a test skill: scripts/run.js lands as -rwxr-xr-x, assets/ is copied too.
• The blob fast path, which writes files from a snapshot instead, is limited to a small owner allowlist we are not part of, so a PrestaShop skill always takes the clone plus recursive copy path.

So a scripts/run.js would be installed, and executable. Keeping scenario.js as a markdown template is right, since it is rewritten for every PR, but the invariant runner does not belong there. If you go that way, reference it as a file sitting next to SKILL.md rather than through ${CLAUDE_SKILL_DIR}, which is Claude Code specific while the CLI installs to many agents.

Content: the QA process is org-wide, its values are not

Writing the skill for a generic project is the right target. It stops one step short of our actual process, and two things need separating.

Shared across every repository, so the skill can rely on it:

• the queue and the label cycle. Waiting for QA, Waiting for author, Waiting for dev, Waiting for PM, Waiting for UX, Waiting for QA by Community exist identically on core, hummingbird, classic-theme, autoupgrade, ps_apiresources, ui-testing-library and the native modules;
• the PR body. Same Questions/Answers table with a How to test? row on core, on themes and on modules;
• the QA rules themselves. Reproduce first, verify second, always leave a comment even when it works, Approve when it works and a plain Comment when it does not (never Request changes), never test your own PR.

Different per repository, so the skill must discover it instead of hardcoding it:

• the validation label. Core uses QA ✔️, hummingbird has QA ✓, QA ✔️ and QA by Dev ✔️, autoupgrade adds waiting for QA by dev, and a few module repositories use a literal QA approved. The skill should list the target repository's labels and pick the matching one;
• the environment. make docker-start on :8001 with /admin-dev/ for core, a clone into modules/ plus composer plus a module reset for a module, a theme in themes/ with an npm build for hummingbird. Nothing transposes.

Concretely, what I would add:

1. Pre-flight before touching anything. Our process requires How to test filled in or a linked issue carrying the steps, no merge conflicts, green CI, no pending changes requested, and a look at the linked UI test run when there is one. When the steps are missing, the process says stop, comment, and set Waiting for author. The skill currently derives steps from the diff and flags them as inferred, which produces a verdict our process would not accept.
2. Close the loop on GitHub. Not posting anything is a good default for an agent, but comments/ should tell the user which review type to use and which labels to swap, read from the repository. That handover is the actual deliverable of a QA pass.
3. Redaction before sharing. Back office screenshots regularly contain API client secrets or personal data, and the comment is meant for a public PR. Add an inspection step, and a solid box rather than a blur (a blur over a short string is partly recoverable).
4. Bug triage. "Failing in both phases is pre-existing" is right. Our process then adds: search the existing issues first, if the bug is known link it and the PR can still be validated, if it is unknown replay on the closest previous version, then either open an issue or send it back to the author as a regression.
5. Module reset. Once a module PR is in place, the module has to be reset in the Module Manager for the change to apply. It is the first cause of "my change is not visible" on module PRs, and it belongs right next to your composer install --no-dev note.
6. Per-project profiles. references/prestashop.md already starts this with its "Where the code sits" table. The theme profile is the one that is really filled in, which is visible in the result: the skill was validated on a hummingbird PR. Core deserves the same depth, in particular the multistore and responsive checks that our process attaches to Migration PRs, the standard docker environment, and rm -rf var/cache/dev rather than bin/console cache:clear, which can OOM and leave the directory root owned, breaking the next web request.

A suggestion upstream of all this: harmonize the QA labels

The heterogeneity above is not this PR's fault, and it is not cosmetic. QA ✓ versus QA ✔️ versus QA by Dev ✔️ versus QA approved across repositories means any tooling has to guess, and it already makes org-wide queue searches unreliable for humans, not just for agents. It may be worth aligning the QA labels across repositories as a separate task, with the skill reading them dynamically in the meantime. Happy to open that discussion on the QA side if there is interest.

[Quetzacoalt91]

Tested on PrestaShop/PrestaShop#42229 (working on a new feature) and PrestaShop/PrestaShop#41414, looks really cool !

[mattgoud]

Re-reviewed at 348dcdc after the rewrite (three probes, scripts/, verdict.json, the HTML report). It is a much stronger skill than the version I read two weeks ago. Since it is already approved, treat what follows as findings to fold in here or in a follow-up, not as a hold.

Summary

#	Finding	Severity	Where	What it takes
1	m.rendered instead of rendered, so ok is undefined and every narrow page is badged "pre-existing" with an empty reason	🔴 Blocking	scripts/run.js:379	Fix the expression, ideally recompute ok in report.js from responds / rendered / overflowPx
2	loggedIn inferred from the current page, so a bo: surface opened with no session records the login form as ok: true	🟠 Major	scripts/run.js:297	Set a flag in loginBO() when the precondition passes, read it here
3a	put() refuses functions but not promises: one missing await records a precondition nobody evaluated	🟡 Nit	scripts/record.js:70	One more clause on the same guard
3b	Response bodies written in full, and only authorization is redacted, not cookie	🟠 Security	scripts/run-http.js:63-81	Mirror the CLI probe's rule, add cookie
3c	Screenshots are never reviewed before being attached to a public PR	🟠 Security	references/reporting.md	An inspection step, and a solid box rather than a blur
4	Pre-flight, triage of what the run finds, multistore	🔵 Enhancement	SKILL.md	Later iteration. Not reading labels is settled
5	Whole-README reflow plus a new contribution rule	⚪ Maintainers' call	README.md	Explicit agreement, or split the reflow out

Only 1 is worth fixing before this ships, since it shows up in every report produced. 2 is next: it fails silently green.

First, what I checked and found genuinely fixed:

• pick-run-dir.sh works. Run from a real checkout it refuses <checkout>/qa-evidence-test naming the offending directory, accepts a path outside it, and writes env/guarded-paths.txt. Deriving the Docker mounts from the port in the URL is a good call.
• The runner moved to scripts/. I reinstalled the branch with npx skills add: all seven scripts and all four references land in .agents/skills/prestashop-pr-qa/, so nothing is retyped per run any more.
• The re-sample trap is closed: rule 4 is explicit, assert.ok and assert.detail refuse a function loudly, and the three templates read inside the callback.

1. The responsive net is broken in every browser report

scripts/run.js:379

ok: status > 0 && status < 400 && !fatal && m.rendered && m.over === 0,

m is what page.evaluate returned, which is { over, worst }. The render flag is the rendered const two lines above, not a field of m. So m.rendered is undefined, ok is undefined, and JSON.stringify drops the key from phase.json entirely.

report.js then reads it through attribute(), whose bad(r) is !r.ok. Both phases are "bad", so every page at every width comes out badged pre-existing, and because why() rebuilds the reason from responds / rendered / overflowPx, which are all correct, the explanation columns are empty.

Rendered from a synthetic run where the narrow page is perfectly healthy (status 200, rendered, overflowPx: 0):

| Viewport and page   | Verdict       | before | after | Boxes sticking out |
| mobile · http://... | pre-existing  |        |       |                    |

So every run ships two to six unexplained yellow warnings in the section a reviewer looks at to decide whether the PR broke the narrow layouts. Fix is && rendered &&.

Worth a second look at the same spot: responds and rendered are stored per row, so ok could just be computed in report.js from those three fields rather than trusted from the phase file. One definition, no chance of the two disagreeing again.

2. A back-office surface can pass green with no session

scripts/run.js:297

const loggedIn = BO ? (await page.locator('input[name="passwd"]').count()) === 0 : false;

That does not test "the scenario logged in", it tests "the page currently loaded is not a login form". After a front-office scenario the current page is a front-office page, so loggedIn is true even though nothing ever authenticated. The bo: surface is then opened without a session, PrestaShop answers the login form with 200, it renders text, it trips neither FATAL nor the token wall, and the row is recorded ok: true.

The report then says back office /route: 200, renders when the only thing observed was the login screen. That is the "passes for the wrong reason" family the skill exists to catch, and it lands on the case surfaces was built for, a legacy to Symfony migration. Setting a flag inside loginBO() when the back office is logged in precondition passed, and reading that flag here, is enough. Detecting input[name="passwd"] on the surface page after navigation and recording it as unreachable: 'redirected to the login form' would be belt and braces.

3. Smaller ones

• put() refuses functions but not promises (record.js:70). assert.ok('x', page.locator(s).count()) without await hands it a Promise, which is always truthy, so a precondition is recorded as satisfied for a condition nobody evaluated. It is the exact slip the error message invites ("Call it yourself and pass the result"). One more clause: reject passed && typeof passed.then === 'function'.
• The HTTP probe writes response bodies in full to transcript.txt, and 4000 characters of them into phase.json (run-http.js:66-81). The CLI probe carries an explicit rule about never running a command that prints a secret; the HTTP probe has no mirror rule for what comes back, and a webservice response is exactly where customer data lives. Only the authorization request header is redacted, not cookie.
• Screenshots are never reviewed before they are attached. The rules cover local paths, credentials and transcripts, but not what the image shows. A real back office puts customer e-mail addresses and API client secrets in success banners, and comments/index.md tells the user to attach those files to a public pull request. One inspection step, and a solid box rather than a blur, since a blur over a short string is partly recoverable.

4. Process, for a later iteration

Not reading or setting labels is a deliberate choice and it is stated in the skill, so that one is settled. Three gaps against how we actually QA remain:

• no pre-flight before an hour is spent on an environment: green CI, no merge conflicts, no pending changes requested;
• no triage of what the run finds: search the existing issues, replay on the closest previous version, then either link a known issue or call it a regression;
• nothing on multistore, which appears only in the theme shop1.json paragraph. It is the dimension most often forgotten on Migration PRs, and surfaces is the mechanism that could cover it.

5. One scope question

The PR reflows the whole README (32 lines rewritten that have nothing to do with the skill) and adds "one paragraph per line, no hard wrapping" to the contribution guide. The rule is reasonable and the reflow follows from it, but it is a repository-wide convention arriving inside a skill PR. Worth the maintainers agreeing to it explicitly rather than inheriting it.

[mattgoud]

Requesting changes for finding 1 of the review above: scripts/run.js:379 reads m.rendered, which does not exist on the object page.evaluate returns, so ok is undefined on every responsive row and the report badges every narrow page "pre-existing" with no reason given. It is a one-word fix, but it is in the section a reviewer reads to decide whether the PR broke the narrow layouts, so it affects every report the skill produces.

Finding 2 (run.js:297, a back-office surface recorded green when only the login form was seen) is the other one I would not ship, since it fails silently in the direction of approving.

Everything else in that comment is a follow-up, and the rest of the rewrite is a clear step up.

[cnavarro-prestashop]

Is not rendered instead ? Before m returns a Page instance and there's not rendered attribute

[cnavarro-prestashop]

Maybe precise to check if sensitive data is not inside screenshot and video before uploading it.

[cnavarro-prestashop]

Number('foo') will return NaN. Use parseInt instead

[mattgoud]

parseInt('foo') returns NaN as well, so the swap does not change the outcome:

Number("foo")    = NaN   | parseInt("foo")    = NaN
Number("120abc") = NaN   | parseInt("120abc") = 120

It would actually loosen the check: parseInt silently accepts 120abc where Number rejects it, and a truncated timeout is worse than a rejected one.

There is a real defect behind it, though. A bad --timeout reaches execFile, which throws before running anything:

RangeError [ERR_OUT_OF_RANGE]: The value of "timeout" is out of range. It must be an unsigned integer. Received NaN

sh() runs inside step(), so that throw is caught and recorded as a harness error: the phase exits 2 with an obscure message on the first command instead of a usage line at startup. --phase already gets that treatment in commonArgs, and --timeout deserves the same:

const TIMEOUT = Number(arg('timeout', 120)) * 1000;
if (!Number.isFinite(TIMEOUT) || TIMEOUT <= 0) { console.error(USAGE); process.exit(2); }

--timeout is also missing from the USAGE string, so nothing tells the caller it exists.

[cnavarro-prestashop]

It's a good idea to write a new scenario to test the PR but what about selecting existing tests campaigns stored in tests/UI depending of what PR changes ? I know it's required to run UI Tests for each PR but it could be another guardrail to be sure that there's no regression.

[tblivet]

Thanks for the reviews. The findings are fixed in the commits above.

Two things I did not change

• The HTTP probe still writes response bodies in full. For an endpoint PR the body is the measurement, so truncating it would destroy the evidence. Instead there is a new inspection step before anything is attached, and a line saying not to point the probe at a shop holding real customer data. Both are in reporting.md.
• @cnavarro-prestashop, parseInt would loosen the check as @mattgoud showed, so Number stays. The defect behind your comment was real and is fixed.

One thing worth flagging

@cnavarro-prestashop, you said screenshot and video, and I had only covered the screenshot. The video is the bigger risk and the only file that cannot be edited. It records the whole pass, including the login form with the e-mail typed into it. A screenshot can be retaken or covered with a solid box, a video cannot. The rule now says to decide before the after pass, since deciding afterwards costs a re-run.

Follow-ups

Pre-flight and multistore in a later iteration. I would leave out triage and the tests/UI campaigns. They answer "is this a known problem" or "does the shop still work" rather than "does this PR do what it claims", and the campaigns should already run in CI on every PrestaShop PR.

The README reflow and the wrapping rule are out, so this PR no longer touches an existing line there.

@mattgoud could you re-review when you have a moment, to clear the changes-requested? 🙏

[mattgoud]

Re-reviewed at ea0f461. All seven findings are addressed, and I checked them in the code rather than from the commit messages.

The blocker is fixed at both ends: run.js reads rendered, and report.js recomputes ok from responds / rendered / overflowPx instead of trusting the phase file. I replayed the case that produced it, a healthy narrow page whose phase.json carries no ok key at all, and the row now renders fine with ok / ok where it used to render pre-existing with an empty reason. An old run directory therefore reports correctly too.

The back-office surface is now gated on a flag set by loginBO(), with the login-form detection as a second line, which is the right shape: the failure it prevents is a page recorded green that nobody ever saw. The header redaction became a pattern rather than a name, the promise guard names which of the two was passed, and the evidence review now covers the video, which is the part I had not thought through: it cannot be edited after the fact, so deciding at the end costs a re-run. Good catch to fold in.

The two things you did not change are argued, and I agree with both. Truncating a response body would destroy the measurement for an endpoint PR, and a mandatory inspection step plus "not a shop with real customer data" is the right trade. Your reading of triage is consistent with the rule you already have, that a check failing in both phases is pre-existing.

Nothing new broken in the delta as far as I can see. Pre-flight and multistore stay on the follow-up list, neither blocks this.

thanks @tblivet 🙏