Repository navigation
feat(plugin): 1.9.0, comprehensive /lua-qa suite (smoke/medium/production-ready tiers, inferred gates, parallel workflows, PDF report) - #19
Conversation
…tes, parallel workflows, PDF report) /lua-qa full: discovery (flow diagram, decision trees), qualifying answers and metrics inferred from the code (--interview to ask), environment gate, then ICP personas, red team, workflow flow tests, direct tool tests and stress run in parallel through the Workflow tool, Opus grading, root-cause clusters with move-logic-out-of-the-prompt fix loci, and a PDF/HTML/JSON report in the CI/CD-guide style. Three tiers: smoke (30-min cap), medium (default) and production-ready (release gate). Safety in code: scrubbed env for helper-spawned lua, a coded argv allowlist, fake data or a user-agreed company email domain, a production consent token behind an ask rule, the guard-qa-helper hook, a sealed plan, and platform memory switched off only with consent. Tool-call evidence is read from `lua logs` (sandbox only). Squashed from feat/lua-qa-suite and feat/lua-qa-suite-logs onto main. 1.8.0 shipped web apps (#17), so this release is 1.9.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ck start) - The cover no longer claims a workflow test when the agent has none. - The flow-test tile reads only "n/a: no workflows" when nothing ran. - cards write fills in a missing schema field per card and plan, and is all or nothing: a schema error writes no file (BUNDLE_INVALID). - init-run says the clock starts at the plan approval; clockStart no longer falls back to the init-run entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
richard-lua
left a comment
There was a problem hiding this comment.
Code review — HIGH risk
This is a very large (≈23k-line, 135-file) feature PR adding the /lua-qa suite. The code that was visible — schema validators, flow-graph path enumeration, report section builders, and the test suites — is careful and well-covered. The security-relevant additions (a new Bash allow glob, a consent ask rule, the scrubbed-spawn design) are clearly thought through, but the pieces they depend on for safety (hooks/guard-qa-helper.mjs, lib/qa/spawn.mjs, the deploy classifier) were not in the review set, and the project's own tests flag an unverified assumption about Claude Code glob semantics. Under strict risk tolerance the open major finding on the new auto-approval rule makes this HIGH risk and requires a human sign-off on the gate pieces before merge.
Major
plugins/lua-agent-builder/lib/permissions-template.json:114— The newBash(node *lua-agent-builder*/lib/qa/cli.mjs *)allow rule auto-approves anycli.mjsunder alua-agent-builderpath. permissions-qa.test.mjs itself notes the cross-/glob behavior was not verified live, and the safety argument rests entirely onhooks/guard-qa-helper.mjsandlib/qa/spawn.mjs, neither of which is in this review. Confirm the guard hook resolves realpath and rejects spoofed/symlinked/..paths (with tests) and verify the live glob via aclaude -pprobe; if it can't be verified, narrow the glob or move it toask.
Minor
plugins/lua-agent-builder/test/lib/qa/fixtures/runtime-helpers.mjs:62— FixturepluginVersion/cache paths hardcode1.8.0while the package bumps to1.9.0; parameterize or read from package.json to avoid drift.plugins/lua-agent-builder/lib/qa/report/sections.mjs:200—table()takes two params but several callers pass a third width-hint array that is silently ignored. Implement the hint or drop the dead argument.plugins/lua-agent-builder/package.json:3— The 1.9.0 jump depends on cross-PR choreography (tagged v1.8.0, open 1.7.1). Re-run lint-release-version and reconcile CHANGELOG ordering at merge time.
PR Risk Reviewer — automated senior review of c817476 · risk: high · confidence: 0.45
- readability --turns-file / --vocabulary-file use isAbsolute, so a D:\ path is not joined onto the cwd. - The pandoc/weasyprint lookup on Windows accepts only PATHEXT names; Windows has no execute bit, so a bare file used to count. - Tests read files through fileURLToPath and compare with resolve. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code review: Needs a human · high risk
What to do next: the reviewer cannot judge this change, so it neither approves it nor asks for changes. Read the reasons below; a person who owns this area decides.
Review for @lua-stefan-kruger, requested through the code-reviewer team.
A person needs to make this call. The reviewer will not approve it:
- Too large to review reliably: 135 files, 23167 changed lines (limits 60 files / 2500 lines). Split the change or have a human review it.
Contracts and consumers
Surfaces touched: event payload, env var, service client.
Consumer search: nothing to search for.
Automated review by lua-code-reviewer[bot]. Every new push is reviewed again automatically. To have this head reviewed again, request the code-reviewer team as a reviewer.
Machine data (for the fleet)
Reviewed head 68601dd5a6fa (round 1) · CI: green · no model review (deterministic checks only) · model: anthropic/claude-opus-5-5
richard-lua
left a comment
There was a problem hiding this comment.
Code review — MEDIUM risk
Previously flagged
- Previously flagged New broad Bash allow glob relies on an unverified guard hook — resolved:
hooks/guard-qa-helper.mjsnow exists and is imported/exercised bypermissions-qa.test.mjs, which asserts it blocks spoofed/tmpand cache paths that match the glob, and the deploy classifier is re-asserted not to flag helper calls. The residual (live cross-/glob behavior unverified) is downgraded to a minor note with a documented safe prompt fallback. - Still open (minor): 1.8.0 fixture drift — fixtures/tests still hardcode 1.8.0 cache paths while package and lockfiles are 1.9.0.
- Still open (minor): table() unused third argument — callers still pass an ignored width-hint array.
- Still open (minor): 1.9.0 version choreography — depends on tag/CHANGELOG ordering vs open #18.
The guard-hook addition materially tightens the one blocking concern from round 1, so this drops from HIGH to MEDIUM. Remaining items are advisory.
Minor
test/lib/qa/permissions-qa.test.mjs:5— The*-across-/glob semantics forBash(node *lua-agent-builder*/lib/qa/cli.mjs *)remain unverified live. Safe (fails closed to a prompt) but confirm via aclaude -pprobe; the guard hook is the enforced boundary regardless.test/lib/qa/fixtures/runtime-helpers.mjs:62— Fixture/cache paths still hardcode1.8.0while the package is1.9.0; parameterize or read from package.json.lib/qa/report/sections.mjs:200—table(headers, rows)ignores the third width-hint array several callers pass; implement it or drop the dead argument.package.json:3— Re-run lint-release-version and reconcile CHANGELOG ordering at merge given the open 1.7.1 PR.
PR Risk Reviewer — automated senior review of 68601dd · risk: medium · confidence: 0.50 · re-review · round 2
Present-tense wording for behaviour that ships for the first time, the tier pass bars in the report section, and the agreed-domain rule in the fake-data bullet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
richard-lua
left a comment
There was a problem hiding this comment.
Previously flagged
- Previously flagged New broad Bash allow glob relies on an unverified guard hook — resolved in round 2: the guard hook exists and is exercised by
permissions-qa.test.mjs, which asserts spoofed/cache paths are blocked; remains closed. - Still open (minor): Cross-
/glob semantics unverified live —permissions-qa.test.mjsstill documents "NOT verified live"; noclaude -pprobe added. Safe: fails closed to a prompt and the guard hook is the enforced boundary. - Still open (minor): 1.8.0 fixture drift — fixtures/tests still hardcode
1.8.0cache paths while the package and lockfiles are now1.9.0. - Still open (minor):
table()unused third width-hint argument —sections.mjsstill ignores the hint array passed by some callers. - Still open (minor): 1.9.0 version choreography — version bumped consistently across package.json and both lockfiles, but ordering vs open #18 and the existing
v1.8.0tag must be reconciled at merge.
The new commits do not introduce new blocking issues; the visible additions (schema validators, flow-model graph walk, report sections) remain well-covered and defensively coded. All remaining items are advisory. The large blast radius and the unverified live glob keep this at MEDIUM under strict tolerance.
Minor
test/lib/qa/permissions-qa.test.mjs:5— Cross-/glob behavior forBash(node *lua-agent-builder*/lib/qa/cli.mjs *)is still unverified live. Confirm via aclaude -pprobe; the guard hook enforces the real boundary regardless.test/lib/qa/fixtures/runtime-helpers.mjs:62— Fixture/cache paths still hardcode1.8.0; read from package.json to avoid drift.lib/qa/report/sections.mjs:200—table(headers, rows)ignores the third width-hint array some callers pass; implement it or drop the dead argument.package.json:3— Re-run lint-release-version and reconcile CHANGELOG ordering at merge given the open 1.7.1 PR and existingv1.8.0tag.
PR Risk Reviewer — automated senior review · re-review · round 3
PR Risk Reviewer — automated senior review of 61aa627 · risk: medium · confidence: 0.50 · re-review · round 3
…ive) - Fixtures and permission tests read the plugin version from package.json (test/lib/qa/fixtures/plugin-version.mjs) instead of hardcoding 1.8.0. - table() derives column widths from content; drop the ignored third width-hint argument from all 19 callers (rendered output unchanged). - permissions-qa.test.mjs: record the docs quote and the live claude -p probe (Claude Code 2.1.293) showing `*` crosses `/` in Bash(node *lua-agent-builder*/lib/qa/cli.mjs *); rule unchanged. - CHANGELOG: spell out the 1.9.0 / #18 version order for both merge orders. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@richard-lua, round 3 is addressed in e92aed6 (SSH-signed):
Gates: |
richard-lua
left a comment
There was a problem hiding this comment.
Code review — MEDIUM risk
Previously flagged
- Previously flagged Cross-
/glob semantics unverified live — resolved:permissions-qa.test.mjsnow documents a live probe (Claude Code 2.1.293, 2026-10-08) confirming*crosses/, with refused controls; the realpath guard hook remains the enforced boundary. - Previously flagged 1.8.0 fixture drift — resolved: new
test/lib/qa/fixtures/plugin-version.mjsreads the version frompackage.json;runtime-helpers,permissions-qaandpermissions-mirrornow usecacheCliPath()and no1.8.0cache path remains in the QA tests. - Previously flagged
table()unused third width-hint argument — resolved: widths are now content-derived and the ignored argument was dropped from all callers; snapshots unchanged. - Still open (minor): 1.9.0 version choreography — version is bumped consistently across package.json and both lockfiles, and both merge orders are documented, but CHANGELOG ordering and
lint-release-versionmust be reconciled at merge vs open #18 and the existingv1.8.0tag.
The new commits (e92aed6) address all three outstanding advisory items and introduce no new blocking issues. Nothing blocking remains open; the remaining item is advisory and a merge-time check. This PR is ready pending that reconciliation.
Minor
package.json:3— Re-runlint-release-versionand confirm CHANGELOG ordering at merge given the open #18 (1.7.1) and the existingv1.8.0tag.
PR Risk Reviewer — automated senior review · re-review · round 4
PR Risk Reviewer — automated senior review of e92aed6 · risk: medium · confidence: 0.50 · re-review · round 4 (converged — no new blocking findings)
There was a problem hiding this comment.
Code review: Needs a human · high risk
What to do next: the reviewer cannot judge this change, so it neither approves it nor asks for changes. Read the reasons below; a person who owns this area decides.
Review for @lua-stefan-kruger, requested through the code-reviewer team.
A person needs to make this call. The reviewer will not approve it:
- Too large to review reliably: 136 files, 23179 changed lines (limits 60 files / 2500 lines). Split the change or have a human review it.
Contracts and consumers
Surfaces touched: event payload, env var, service client.
Consumer search: nothing to search for.
Automated review by lua-code-reviewer[bot]. Every new push is reviewed again automatically. To have this head reviewed again, request the code-reviewer team as a reviewer.
Machine data (for the fleet)
Reviewed head e92aed6e1593 (round 2) · CI: green · no model review (deterministic checks only) · model: anthropic/claude-opus-5-5
release-prod.yml only checks that the tag is new, so an unbumped 1.7.1 would cut an out-of-order v1.7.1 release rather than skip; say so. The probe comment now says which `/` each `*` crossed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Follow-up in the next commit: the CHANGELOG and PR body had said #18's 1.7.1 would get no release. That was wrong. |
richard-lua
left a comment
There was a problem hiding this comment.
Code review — MEDIUM risk
Previously flagged
- Previously flagged New broad Bash allow glob relies on an unverified guard hook — resolved (round 2): guard hook present and exercised by
permissions-qa.test.mjs. - Previously flagged Cross-
/glob semantics unverified live — resolved (round 4): live probe documented inpermissions-qa.test.mjs(Claude Code 2.1.293), realpath guard hook is the enforced boundary. - Previously flagged 1.8.0 fixture drift — resolved (round 4):
fixtures/plugin-version.mjsreads version from package.json; QA tests usecacheCliPath(). - Previously flagged
table()unused third width-hint argument — resolved (round 4): widths are now content-derived insections.mjsand the ignored argument dropped from all callers. - Still open (minor): 1.9.0 version choreography — the new commits correct the earlier CHANGELOG/PR-body claim (an unbumped #18 at 1.7.1 would in fact cut an out-of-order tag, not be skipped) and the permissions-template
_commentnow documents the 1.9.0 rules. The remaining reconciliation vs open #18 and the existingv1.8.0tag is a merge-time human decision, not a code defect.
The new commits are documentation/version-note corrections plus the rebuilt bundle; they introduce no new blocking issues. The schema validators, flow-model graph walk, and report sections remain well-covered and defensively coded (markdown/HTML escaping, path allowlisting in figure(), env scrubbing). This PR has reached the round limit with nothing blocking open.
Minor
package.json:3— Re-runlint-release-versionand confirm CHANGELOG ordering at merge given open #18 and the existingv1.8.0tag; ensure #18 is bumped above 1.8.0.
Decision for a human: no blocking findings remain open. The PR is ready pending the merge-time version reconciliation.
PR Risk Reviewer — automated senior review · re-review · round 5 (converged — no new blocking findings)
PR Risk Reviewer — automated senior review of 555105f · risk: medium · confidence: 0.50 · re-review · round 5 (converged — no new blocking findings)
There was a problem hiding this comment.
Code review: Needs a human · high risk
What to do next: the reviewer cannot judge this change, so it neither approves it nor asks for changes. Read the reasons below; a person who owns this area decides.
Review for @lua-stefan-kruger, requested through the code-reviewer team.
A person needs to make this call. The reviewer will not approve it:
- Too large to review reliably: 136 files, 23180 changed lines (limits 60 files / 2500 lines). Split the change or have a human review it.
Contracts and consumers
Surfaces touched: event payload, env var, service client.
Consumer search: nothing to search for.
Automated review by lua-code-reviewer[bot]. Every new push is reviewed again automatically. To have this head reviewed again, request the code-reviewer team as a reviewer.
Machine data (for the fleet)
Reviewed head 555105fb42a3 (round 3) · CI: green · no model review (deterministic checks only) · model: anthropic/claude-opus-5-5
Version note: this PR ships as 1.9.0, not 1.8.0. While it was being built, #17 (web apps) merged as 1.8.0 and the
v1.8.0tag exists, andrelease-prod.ymlskips the release when the tag already exists. Every placelint-release-versionchecks says 1.9.0, and the committed MCP bundle is rebuilt. #18 (fix/deploy-hook-false-positives) is still open and declares 1.7.1, which is below the released 1.8.0, so #18 must take a version above it before merging. As it stands,release-prod.ymlonly checks that the tag is new, so it would cut an out-of-order v1.7.1. Whichever PR merges second rebases and re-runslint-release-version:The CHANGELOG's 1.9.0 entry says the same.
What it does
/lua-qa fullis a complete QA suite for a Lua agent. Bare/lua-qa(and/lua-qa quick,/lua-qa <name>) is the quick pass from earlier releases, unchanged.lua status,lua version listand the workflows. It draws a flow diagram, a decision tree per skill (persona must-never and escalation rules, per-tool conditions, ask-for-field paths) and a branch tree per workflow. The user confirms the map.file:linefor each.--interviewbrings the questions back.results.jsonin the Lua CI/CD-guide style: a dark cover with a tier badge, a TOC, an orange lede, stat tiles and Pass / Partial / Fail chips. Without pandoc a built-in renderer writes the HTML, and the PDF is skipped (with an install line) when WeasyPrint is missing.The three tiers
Smoke: no blockers found/blockers found, never release-readyProduction ready: YES/NO, <n> blockersThe helpers enforce the tier.
init-runwrites it tostate.json, which the planner never writes.validaterefuses a plan that is over or under its tier, or over its time budget.Safety enforced in code, not only in prompts
luacall goes throughlib/qa/spawn.mjswith an environment allowlist (LUA_API_KEYis always dropped) and a coded argument allowlist, with the deploy classifier as a second check. A sandbox chat therefore cannot upload the shell's secrets.@example.*. A company email domain that a tool insists on can be agreed only at the environment gate. Addresses on it need an obviously fake local part, and public mailbox providers are refused.I consent to running this against production. The stamp command matches anaskpermission rule, so Claude Code itself prompts the user, andstate.jsonkeeps only a hash of the token.guard-qa-helperhook. The helper's allow rule is a glob, so the hook resolves the script's real path and blocks anything that is not this plugin's ownlib/qa/cli.mjs. That makes 13 hooks.lua features list. Switching memory off for the run needs a verbatim consent, the user runs the switch commands themselves (anaskrule),start-runrefuses until memory is verified off, and cleanup lists the restore until it reads back on. Otherwise memory findings carry a caveat, and cross-run recall is filed as a harness artefact, not an agent defect.PLAN_CHANGED).Tool-call evidence from
lua logs(sandbox only)The sandbox history route returns no tool calls.
prechecksreadslua logs --type skillonce per run, groups the rows byexecutionIdinto calls (tool, input, result, warnings, errors) and assigns each call to the turn whose window holds it. Claims are then checked against real calls: a ticket id or action claimed in a turn with no logged call is a confirmed candidate failure.backfill-toolsfills in runs recorded earlier.Live trial: Helix IT Desk (sandbox)
NOT_PLAYEDfor runs that never started (instead of FAIL), and the cross-run memory isolation.n/a: no workflows;cards writefills in a missingschema;cards writewrites nothing when the bundle has a schema error, and says so;init-run.Permission glob: verified live
Bash(node *lua-agent-builder*/lib/qa/cli.mjs *): Claude Code's*crosses/, so the rule stays as written.*in a Bash rule matches any text, including spaces", and "Bash rules match the whole command text, with*standing in for any text."claude -p --setting-sources project --settings <file with only this allow rule> --permission-mode default. It was asked to runnode <tmp>/fake/lua-agent-builder/x/lib/qa/cli.mjs --help, against a dummycli.mjsthat only echoes. The command ran, with no permission denial:PROBE_OK cli.mjs ran with args: --help. The first*crossed every/of the temp path and the second crossed/x/, the same shape as a real cache path (lua-agent-builder/1.9.0/lib).{}settings file, andnode <tmp>/fake/other/x/lib/qa/cli.mjs --helpwith the rule..claude/settings.jsonis ignored until that workspace is trusted, so the rule was passed with--settings.Because the glob is this wide,
hooks/guard-qa-helper.mjs(a realpath check) is the real boundary. That is unchanged. The result is recorded intest/lib/qa/permissions-qa.test.mjs.Known limitations
lua-qa-playerand the other roles cannot be resolved, each role runs asgeneral-purposeand first reads its own agent file. The trial ran this way, so the real agent types (claude --plugin-dir) are untested.Decisions for reviewers
--turns-fileusedstartsWith('/'), and the report-tool lookup counted a bare file without a PATHEXT extension. A fourth tightens the CHANGELOG into one coherent 1.9.0 entry. A fifth addresses review round 3: the fixtures read the version from package.json, the deadtable()width-hint argument is gone, the glob's live verification is recorded, and the version order is spelled out. The rebase conflicted on the CHANGELOG,hooks/hooks.jsonand the permission template (feat(plugin): 1.8.0 — web apps (Lua Apps) in /lua-new, /lua-test, /lua-push, /lua-deploy #17's web-app rules). Both sides are kept.askprompt sits on top of /lua-qa's own consent question, on purpose.Test plan
npm run lint(eslint + 17 scripts; release version 1.9.0 consistent)npm test: 67 suites, 2060 testsnpm run test:coveragepluscheck-coverage.mjsper-file thresholdsnpx eslint --no-ignoreon every changed.mjsnpm ci,npm test(149),npm run lint,npm run build(dist committed),check-bundle-size(0.50 MB)/lua-qa full smokewith the plugin installed (claude --plugin-dir), so the real agent types are exercised live🤖 Generated with Claude Code