From 0eb2036059e413046b2c7fa01c64ce97827228a6 Mon Sep 17 00:00:00 2001 From: Rand Lee Date: Sun, 20 Sep 2026 15:40:26 -0700 Subject: [PATCH] fix: render QA task as strict XML --- .../codex-orchestration/qa-template.xml.j2 | 72 ++++++----- .../tests/test_orchestration_contracts.py | 117 +++++++++++++++++- 2 files changed, 150 insertions(+), 39 deletions(-) diff --git a/.claude/skills/codex-orchestration/qa-template.xml.j2 b/.claude/skills/codex-orchestration/qa-template.xml.j2 index aad9384f..2426d808 100644 --- a/.claude/skills/codex-orchestration/qa-template.xml.j2 +++ b/.claude/skills/codex-orchestration/qa-template.xml.j2 @@ -1,6 +1,6 @@ --- name: qa-task -version: 1.1.0 +version: 1.2.0 description: quality-mgr QA assignment template for codex-orchestration sprint and phase reviews. format: xml required_variables: @@ -37,49 +37,47 @@ metadata: stage: qa transition: start --- -{% autoescape false -%} - {{ description }} + - {{ review_mode }} - {{ sprint_doc }} - {{ pr_number }} - {{ branch }} - {{ worktree_path }} - {{ commits }} - `{{ branch }}` is a frozen layer of the phase stack at the commits above. Review exactly that head (`git show :`), never a moving checkout. Report every finding, at every severity, in this one verdict; none is deferred. {{ lead }} records each one through `/triaging-findings` the same way and every finding is fixed on a new layer above this one, which the next QA round reviews. + + + + + + + :`), never a moving checkout. Report every finding, at every severity, in this one verdict; none is deferred. {{ lead | string | cdata_escape }} records each one through `/triaging-findings` the same way and every finding is fixed on a new layer above this one, which the next QA round reviews.]]> - -{{ review_targets }} - + - -{{ changed_files }} - + - -{{ triage_records }} - + - -{{ references }} - + - Start the task immediately with `atm task start {{ task_id }} ""`, then claim the matching QA bead with `bd update {{ task_id }} --claim` and begin the QA review in the same work session. - Your queue runs in parallel: QA tasks never wait for each other. Right after the task start run `atm task list --json` and treat every open task assigned to quality-mgr as live now — its assignment body is the `description` field — and run each one concurrently with its own background reviewers. Close each task with its own `atm task close completed …` the moment that verdict is ready, in any order; a queued position is ordering, not a wait, and a task can be closed without ever being started. - Reject the task immediately if the assignment is not an XML payload rendered from this template. Read `{{ sprint_doc }}` first and treat it as the authoritative source for QA scope. Render structured JSON assignments for `req-qa` and `arch-qa` using the templates in `.claude/skills/codex-orchestration/`. Do not send free-form reviewer prompts. - Extract every deliverable, acceptance criterion, deletion requirement, required validation item, and expected artifact directly from `{{ sprint_doc }}`. - Treat the sprint doc and planning artifacts as explicit acceptance-gate items: verify `{{ sprint_doc }}` exists, the sprint doc frontmatter sets `status: complete`, `branch:`, and `worktree:`, and `docs/project-plan.md` includes the sprint entry before QA can PASS. - Read `.claude/assets/sc-rust/quality-mgr/quality-mgr.rust.md` and render any required Rust reviewer assignments from `.claude/assets/sc-rust/quality-mgr/templates/`. For `phase_end`, the rendered `rust-qa-agent` assignment must retain its required `artifact_commands: "just lint && just test"` execution proof; collect its `executed_checks.artifacts` result before reporting PASS. `rust-best-practices-agent`, `ruthless-boundary-qa`, and `rust-service-hardening-agent` are QA-1 (the sprint's first QA pass) only — omit all three unconditionally from QA-2 and later fix-verification rounds on the same sprint branch; they reliably surface findings on any diff regardless of size, and re-running them on a fix round turns it into unbounded churn instead of verification. QA-2+ rounds launch `req-qa` + `arch-qa` (scoped to the dispatched finding ids) plus `rust-qa-agent` (its objective execution-fact gates — fmt, clippy, tests, lint, RULE-003, pytests — are not a subjective findings pass and stay in every round). The verdict is each dispatched finding's fixed/regressed/open status plus `rust-qa-agent`'s gate results, nothing else — anything req-qa/arch-qa notices outside the dispatched scope goes in a debt-notes section of the report and does not affect the verdict. `ruthless-boundary-qa` and `rust-best-practices-agent` remain part of docs-only plan review and phase-ending review regardless of sprint round. - If triage records are provided, read them before spawning reviewers. Treat them as the canonical prior scope for follow-up QA, not as a substitute for verification. - Spawn every selected reviewer in background mode. Do not run cargo, clippy, or broad QA analysis yourself in the foreground. - Collect reviewer outputs and summarize PASS or FAIL, including blocking finding ids and file:line references where available. Re-verify every cited file:line against the current branch and worktree before reporting it; stale or missing evidence is a finding, not a guess. Ruthless-boundary-qa must run a workspace-wide duplicate sweep for every constant, function, or type touched by the sprint diff and report divergent duplicates as boundary findings. For every finding that is a repeatable pattern (missing gate, wrong value, forbidden construct, naming violation), search the entire workspace for other occurrences of the same pattern and include all affected locations in the finding — not just the first one found. A finding is not complete until the full scope is known. For deletion sprints, req-qa must run whole-workspace pattern searches for each removed legacy construct family named in the sprint doc. For observability sprints, req-qa must also verify that every sprint-diff code path that can reach a user-visible failure has an emit call site. - When triage records are present, explicitly state whether each rechecked finding is still open, fixed on the reviewed branch, regressed, or requires a new triage pass because the scope changed. - If a PR number is present, check stack and CI state with `gh stack view --json` from a worktree checked out on `{{ branch }}` (if it reports the branch is not part of a stack, run `gh stack checkout ` there first) and `gh pr checks {{ pr_number }} --json name,state,bucket`. Never use `gh pr checks --watch` or parse its text output. Report a PR with no CI run as conflicting or needs-rebase, with the `gh stack view --json` output. - Install the daemon-readable templates with `mkdir -p ~/.atm/templates/quality-management-gh && cp .claude/skills/quality-management-gh/*.j2 ~/.atm/templates/quality-management-gh/`. Append the completed QA report to the PR whenever a PR exists: render `FAIL`/`IN-FLIGHT` with `atm compose --template ~/.atm/templates/quality-management-gh/findings-report.md.j2 --vars /qa--vars.json | gh pr comment --body-file -`, or `PASS` with the same command using `~/.atm/templates/quality-management-gh/quality-report.md.j2`. Build vars for this QA run from the selected template's `required_variables` frontmatter; never reuse a previous or sample report's vars. Write the vars file outside the repository working tree (in the session scratchpad or a temp directory); never commit or stage it, and delete it or let it expire after the send. The PR comment remains required. - Deliver the final QA verdict by closing this task with the rendered report as the body: `atm task close {{ task_id }} completed --template ~/.atm/templates/quality-management-gh/findings-report.md.j2 --vars /qa--vars.json` for `FAIL`/`IN-FLIGHT`, or `atm task close {{ task_id }} completed --template ~/.atm/templates/quality-management-gh/quality-report.md.j2 --vars /qa--vars.json` for `PASS`, then run `bd close {{ task_id }}`. The close delivers the report to the assigner (the sender of this message) and frees your queue; a plain `atm send {{ lead }}` leaves `{{ task_id }}` open and the next QA task never starts. A `FAIL` verdict still closes the task as `completed` — the QA round is the task, the code is not; use `refused` only for an assignment that cannot be reviewed, leave its bead open, and add `bd update {{ task_id }} --notes ""`. Use the same outside-repository scratchpad or temp vars file, never commit or stage it, and delete it or let it expire after the send. Include deliverable completion as `X/Y (Z%)`. PASS requires 100% deliverable completion, a PASS verdict from every required reviewer, an active PR for the branch, and no unresolved blocking findings. {% if cc and cc != lead %}Copy a one-line summary to `{{ cc }}` with a plain `atm send {{ cc }} --stdin`.{% endif %} -{% if triage_records and triage_records|trim %} For every triage record in `{{ triage_records }}` (or any other `.triage/*/findings/*.ttl` record in scope for this round) that this round's reviewers confirm is fixed on the reviewed branch, update that record's `triage:status` to `"fixed"` and set `triage:closedAt`/`triage:closedBy "quality-mgr"` before sending the final verdict — per the triaging-findings skill's closure rule that quality-mgr, not {{ lead }}, owns finding closure once follow-up QA confirms the fix. Do not close a record that is still open, regressed, or unverified this round. List every record you closed (finding id + file) in the final verdict message so {{ lead }} can reconcile it. + "`, then claim the matching QA bead with `bd update {{ task_id | string | cdata_escape }} --claim` and begin the QA review in the same work session.]]> + completed …` the moment that verdict is ready, in any order; a queued position is ordering, not a wait, and a task can be closed without ever being started.]]> + + + + + + + + + ` there first) and `gh pr checks {{ pr_number | string | cdata_escape }} --json name,state,bucket`. Never use `gh pr checks --watch` or parse its text output. Report a PR with no CI run as conflicting or needs-rebase, with the `gh stack view --json` output.]]> + /qa--vars.json | gh pr comment --body-file -`, or `PASS` with the same command using `~/.atm/templates/quality-management-gh/quality-report.md.j2`. Build vars for this QA run from the selected template's `required_variables` frontmatter; never reuse a previous or sample report's vars. Write the vars file outside the repository working tree (in the session scratchpad or a temp directory); never commit or stage it, and delete it or let it expire after the send. The PR comment remains required.]]> + /qa--vars.json` for `FAIL`/`IN-FLIGHT`, or `atm task close {{ task_id | string | cdata_escape }} completed --template ~/.atm/templates/quality-management-gh/quality-report.md.j2 --vars /qa--vars.json` for `PASS`, then run `bd close {{ task_id | string | cdata_escape }}`. The close delivers the report to the assigner (the sender of this message) and frees your queue; a plain `atm send {{ lead | string | cdata_escape }}` leaves `{{ task_id | string | cdata_escape }}` open and the next QA task never starts. A `FAIL` verdict still closes the task as `completed` — the QA round is the task, the code is not; use `refused` only for an assignment that cannot be reviewed, leave its bead open, and add `bd update {{ task_id | string | cdata_escape }} --notes ""`. Use the same outside-repository scratchpad or temp vars file, never commit or stage it, and delete it or let it expire after the send. Include deliverable completion as `X/Y (Z%)`. PASS requires 100% deliverable completion, a PASS verdict from every required reviewer, an active PR for the branch, and no unresolved blocking findings.{% if cc and cc != lead %} Copy a one-line summary to `{{ cc | string | cdata_escape }}` with a plain `atm send {{ cc | string | cdata_escape }} --stdin`.{% endif %}]]> +{% if triage_records and triage_records|trim %} {% endif %} -{%- endautoescape %} diff --git a/bindings/sc-lint-py/python/sc_lint/tests/test_orchestration_contracts.py b/bindings/sc-lint-py/python/sc_lint/tests/test_orchestration_contracts.py index 9ef93da7..7ad6a6b9 100644 --- a/bindings/sc-lint-py/python/sc_lint/tests/test_orchestration_contracts.py +++ b/bindings/sc-lint-py/python/sc_lint/tests/test_orchestration_contracts.py @@ -16,6 +16,7 @@ import tempfile import unittest from unittest import mock +from xml.etree import ElementTree from sc_lint.lint_common import discover_repo_root @@ -191,10 +192,21 @@ def compose_with_jinja(self, template: Path, sample: Path) -> str: defaults, body = self.jinja_parts(template) variables = {**defaults, **json.loads(sample.read_text(encoding="utf-8"))} + xml_template = template.name.endswith(".xml.j2") renderer = Environment( - autoescape=False, + autoescape=xml_template, undefined=StrictUndefined, - finalize=lambda value: value if isinstance(value, Markup) else json.dumps(value), + finalize=lambda value: ( + value + if isinstance(value, Markup) or (xml_template and isinstance(value, str)) + else json.dumps(value) + ), + ) + renderer.filters["cdata_escape"] = lambda value: Markup( + str(value).replace("]]>", "]]]]>") + ) + renderer.filters["string"] = lambda value: ( + value if isinstance(value, str) else json.dumps(value) ) return renderer.from_string(body).render(**variables) @@ -332,3 +344,104 @@ def test_quality_report_machine_json_round_trips_strings(self) -> None: self.assertEqual(payload[field], adversarial) self.assertEqual(payload["pr"], 123) self.assertEqual(payload["findings"], {"blocking": 1, "important": 2, "minor": 3}) + + def test_qa_template_renders_strict_xml_with_typed_values(self) -> None: + adversarial = 'quoted "value" \\ path\nline & snowman ☃ ]]> twice ]]>' + attribute_adversarial = 'quoted "value" \\ path & snowman ☃ ]]> twice ]]>' + variables = { + "task_id": attribute_adversarial, + "sprint": attribute_adversarial, + "sprint_doc": adversarial, + "review_mode": adversarial, + "description": adversarial, + "pr_number": 187, + "branch": adversarial, + "worktree_path": adversarial, + "commits": [adversarial, "def456"], + "review_targets": [adversarial], + "references": [adversarial], + "lead": adversarial, + "cc": adversarial, + "changed_files": [adversarial], + "triage_records": [adversarial], + } + template = REPO / ".claude/skills/codex-orchestration/qa-template.xml.j2" + source = template.read_text(encoding="utf-8") + self.assertNotIn("{% autoescape false", source) + for entity in ("<", ">", "&", """): + self.assertNotIn(entity, source) + + with tempfile.TemporaryDirectory() as directory: + vars_path = Path(directory) / "vars.json" + vars_path.write_text(json.dumps(variables), encoding="utf-8") + if shutil.which("sc-compose"): + result = subprocess.run( + [ + "sc-compose", + "render", + "--strict", + "--check-render", + "--json", + "--file", + str(template), + "--var-file", + str(vars_path), + ], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stderr) + envelope = json.loads(result.stdout) + self.assertFalse( + [ + diagnostic + for diagnostic in envelope["diagnostics"] + if diagnostic["severity"] == "error" + ] + ) + rendered = envelope["payload"]["body"] + else: + rendered = self.compose(template, vars_path) + root = ElementTree.fromstring(rendered) + + self.assertEqual(root.attrib["id"], attribute_adversarial) + self.assertEqual(root.attrib["sprint"], attribute_adversarial) + self.assertEqual(root.findtext("description"), adversarial) + self.assertEqual(root.findtext("branch"), adversarial) + self.assertEqual(root.findtext("worktree"), adversarial) + self.assertEqual(json.loads(root.findtext("pr-number")), variables["pr_number"]) + self.assertEqual(json.loads(root.findtext("commits")), variables["commits"]) + self.assertEqual( + json.loads(root.findtext("review-targets")), variables["review_targets"] + ) + self.assertEqual( + json.loads(root.findtext("changed-files")), variables["changed_files"] + ) + self.assertEqual( + json.loads(root.findtext("triage-records")), variables["triage_records"] + ) + self.assertEqual(json.loads(root.findtext("references")), variables["references"]) + + legacy_strings = { + **variables, + "pr_number": "", + "commits": "HEAD", + "review_targets": "- src/", + "changed_files": "", + "triage_records": "", + "references": "- plan", + } + with tempfile.TemporaryDirectory() as directory: + vars_path = Path(directory) / "legacy-vars.json" + vars_path.write_text(json.dumps(legacy_strings), encoding="utf-8") + legacy_root = ElementTree.fromstring(self.compose(template, vars_path)) + for field, expected in ( + ("pr-number", ""), + ("commits", "HEAD"), + ("review-targets", "\n- src/\n "), + ("changed-files", "\n\n "), + ("triage-records", "\n\n "), + ("references", "\n- plan\n "), + ): + self.assertEqual(legacy_root.findtext(field), expected)