Skip to content

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

Open
tblivet wants to merge 10 commits into
PrestaShop:mainfrom
tblivet:feat/prestashop-pr-qa
Open

Add prestashop-pr-qa guide an AI agent through QA of a pull request#4
tblivet wants to merge 10 commits into
PrestaShop:mainfrom
tblivet:feat/prestashop-pr-qa

Conversation

@tblivet

@tblivet tblivet commented Aug 20, 2026

Copy link
Copy Markdown
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

@tblivet
tblivet force-pushed the feat/prestashop-pr-qa branch from bf3a1ba to f4c0c44 Compare August 20, 2026 14:11
@tblivet
tblivet marked this pull request as ready for review August 20, 2026 14:28
@ps-jarvis

Copy link
Copy Markdown

Hello @tblivet!

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

Thank you, and welcome to this Open Source community!

@github-project-automation github-project-automation Bot moved this to Ready for review in PR Dashboard Aug 20, 2026
@tblivet
tblivet force-pushed the feat/prestashop-pr-qa branch from f4c0c44 to a547b5a Compare August 20, 2026 14:32
@tblivet
tblivet force-pushed the feat/prestashop-pr-qa branch from a547b5a to 0e4681a Compare August 20, 2026 14:45
@mattgoud

Copy link
Copy Markdown

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.

Comment thread qa/dev/prestashop-pr-qa/SKILL.md Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready for review

Development

Successfully merging this pull request may close these issues.

4 participants