From 6b759e777f02fccb4de8947c60e4d0cca790826b Mon Sep 17 00:00:00 2001 From: Taleef Date: Fri, 31 Jul 2026 11:36:34 -0400 Subject: [PATCH 1/2] =?UTF-8?q?feat(official):=20credentialed=20vendoring?= =?UTF-8?q?=20gets=20a=20command=20=E2=80=94=20the=20"owner=20step"=20was?= =?UTF-8?q?=20a=20tooling=20gap?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CMS130, CMS165 (capped, ADR-041) and CMS138 (absent, ADR-053) were all recorded as blocked on "an owner step with the VSAC key". That framing was wrong. WORKWELL_VSAC_API_KEY_VENDOR has been a GitHub secret since 2026-07-29 and CI has used it on every push since. What was missing was not the credential but a way to run the vendor command in the one place it already lives — secrets are write-only, so a clone cannot produce a completed artifact however willing its owner is. `vendor-official-measure.yml` is a workflow_dispatch job: vendors one measure with `--complete-terminology`, prints `truncated` + `completion` into the log, and uploads the two committable files as an artifact. `contents: read` only — deliberately not a workflow that can push, because that is a standing capability where this is a one-shot whose output a human reviews. THE LICENSED-CONTENT BOUNDARY IS THE LOAD-BEARING PART. Three files land in the output directory and only two may leave the runner: bundle.json and manifest.json are already committed to this public repo, while terminology.json holds thousands of AMA CPT and SNOMED CT codes under an NLM licence and is gitignored precisely so it is never redistributed (ADR-036). An artifact URL is redistribution. The difference is one `cp` line and it would be invisible in review — an artifact is a zip nobody opens. `vendor-workflow-safety.test.ts` pins it: named files, no wildcard, no recursive copy, dispatch-only triggers, `contents: read`, and the fail-closed credential check (an uncredentialed vendor produces an artifact that looks vendored and cannot be routed, which is worse than none). Mutation-checked against `cp -r …/*` — the exact edit someone makes to "just grab everything" — which fails exactly one test. Its first cut failed on its OWN comment: the workflow header explains why it does not take `contents: write`, and the prohibition matched that sentence. Now scans non-comment lines only. Also: fetch-official-cases.ps1 checks out CMS130/CMS138/CMS165 as CANDIDATES — sparse checked out but deliberately absent from OFFICIAL_GATED_MEASURES, since their artifacts are not vendored and adding them to the gate would fail the deck. That is what lets `pnpm official:terminology-audit` read their bundles at the pinned commit, which is how ADR-053's finding was made. Lands alone because GitHub requires a workflow_dispatch file on the DEFAULT BRANCH before it can be dispatched. The artifacts, gate wiring and MADiE verdict follow. Suite 1755 pass / 0 fail / 15 skipped; typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q3BNWAJwJJ5QjP1X6JpHUo --- .github/workflows/vendor-official-measure.yml | 128 ++++++++++++++++++ backend-ts/scripts/fetch-official-cases.ps1 | 13 +- .../src/wiring/vendor-workflow-safety.test.ts | 105 ++++++++++++++ docs/DEPLOY.md | 31 +++++ docs/JOURNAL.md | 37 +++++ 5 files changed, 313 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/vendor-official-measure.yml create mode 100644 backend-ts/src/wiring/vendor-workflow-safety.test.ts diff --git a/.github/workflows/vendor-official-measure.yml b/.github/workflows/vendor-official-measure.yml new file mode 100644 index 00000000..4dd9ff57 --- /dev/null +++ b/.github/workflows/vendor-official-measure.yml @@ -0,0 +1,128 @@ +# Vendor an official measure artifact WITH the VSAC credential, and hand back the committable files. +# +# ## Why this exists +# +# `WORKWELL_VSAC_API_KEY_VENDOR` is a GitHub secret. Secrets are write-only — `gh secret list` returns +# names, never values — so nobody working from a clone can produce a *completed* artifact, even though +# the credential is configured and CI uses it on every push. +# +# That produced a false blocker. Three priority measures (CMS130, CMS165 capped; CMS138 absent — see +# ADR-041 and ADR-053) were recorded as needing "an owner step", when what they actually needed was a +# way to run the existing vendor command in the one place the credential already lives. This is that +# way: a manual trigger that vendors, prints what moved, and uploads `bundle.json` + `manifest.json` as +# an artifact to download and commit. +# +# ## Why it uploads rather than commits +# +# It needs no `contents: write`. A workflow that can push to the repo is a standing capability; this is +# a one-shot that produces two files a human then reviews and commits. The reproducibility gate in +# `ci.yml` re-derives those exact bytes on the next push, so a bad upload cannot survive review. +# +# ## What is safe to upload, and what is not +# +# `bundle.json` and `manifest.json` are **committed to this public repo already** — the manifest carries +# counts, provenance and the sidecar's SHA-256, no codes. `terminology.json` is NOT uploaded: it holds +# thousands of AMA CPT and SNOMED CT codes under an NLM licence and is gitignored for that reason +# (ADR-036). Uploading it would redistribute licensed content through an artifact URL, so the copy step +# names the two files explicitly rather than globbing the directory. +name: Vendor official measure (credentialed) + +on: + workflow_dispatch: + inputs: + measure: + description: "Upstream measure directory, e.g. CMS138FHIRTobaccoScrnCessation" + required: true + type: string + catalog_id: + description: "WorkWell catalog id, lowercase alphanumeric, e.g. cms138" + required: true + type: string + ref: + description: "Upstream commit sha (blank = the pinned default in the vendor script)" + required: false + type: string + +permissions: + contents: read + +jobs: + vendor: + name: Vendor ${{ inputs.catalog_id }} with the VSAC credential + runs-on: ubuntu-latest + timeout-minutes: 20 + defaults: + run: + working-directory: backend-ts + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: { node-version: 24 } + + # Fails the job rather than vendoring an incomplete artifact that looks fine. Without the + # credential the script warns and leaves capped/absent value sets exactly as upstream shipped + # them — correct behaviour, and precisely the outcome this workflow exists to avoid producing. + - name: Refuse to run without the credential + env: + WORKWELL_VSAC_API_KEY: ${{ secrets.WORKWELL_VSAC_API_KEY_VENDOR }} + run: | + if [ -z "$WORKWELL_VSAC_API_KEY" ]; then + echo "::error::WORKWELL_VSAC_API_KEY_VENDOR is not available here. A fork PR or a repo without the secret cannot produce a completed artifact — that is the whole point of this workflow." + exit 1 + fi + + # Same sparse checkout the eCQM gate uses, so the bundle is read locally at the pinned commit + # instead of pulling ~17 MB from raw.githubusercontent. + - name: Fetch official content (pinned commit) + run: pwsh -NoProfile -File scripts/fetch-official-cases.ps1 + + # The measurement from ADR-053, run BEFORE vendoring: it says whether this measure declares a + # value set the bundle does not ship, which decides whether the run below can succeed at all. + # Never fails the job — it is a measurement, and `official-terminology-audit.mjs` exits 0 by design. + - name: Audit declared-vs-shipped terminology + continue-on-error: true + run: node scripts/official-terminology-audit.mjs "${{ inputs.measure }}" + + - name: Vendor with completion + env: + WORKWELL_VSAC_API_KEY: ${{ secrets.WORKWELL_VSAC_API_KEY_VENDOR }} + run: | + node scripts/vendor-official-measure.mjs \ + --measure "${{ inputs.measure }}" \ + --catalog-id "${{ inputs.catalog_id }}" \ + --strip-elm-annotations \ + --complete-terminology \ + ${{ inputs.ref && format('--ref {0}', inputs.ref) || '' }} + + # The manifest is the evidence. Printed into the log so the outcome is visible without + # downloading anything: `truncated` must be empty, and `completion` must record what was sourced + # and from which VSAC release. + - name: Report what the credential actually changed + run: | + node -e ' + const m = require("./measures/official/${{ inputs.catalog_id }}/manifest.json"); + const t = m.terminology ?? {}; + console.log(`measure ${m.measureName} v${m.version} (${m.cmsId ?? "no cmsId"})`); + console.log(`terminology ${t.valueSets} value sets, ${t.codes} codes`); + console.log(`truncated ${JSON.stringify(t.truncated ?? [])}`); + console.log(`completion ${JSON.stringify(t.completion ?? null, null, 2)}`); + if ((t.truncated ?? []).length > 0) { + console.log("::warning::truncated is NOT empty — this artifact is still incomplete and routing will refuse it."); + } + ' + + # Explicit paths, never a directory glob: `terminology.json` sits beside these two and must not + # leave the runner (see the header). + - name: Stage the committable files only + run: | + mkdir -p ../_vendored/${{ inputs.catalog_id }} + cp measures/official/${{ inputs.catalog_id }}/bundle.json ../_vendored/${{ inputs.catalog_id }}/ + cp measures/official/${{ inputs.catalog_id }}/manifest.json ../_vendored/${{ inputs.catalog_id }}/ + echo "staged:"; ls -la ../_vendored/${{ inputs.catalog_id }}/ + + - uses: actions/upload-artifact@v4 + with: + name: vendored-${{ inputs.catalog_id }} + path: _vendored/${{ inputs.catalog_id }} + if-no-files-found: error + retention-days: 7 diff --git a/backend-ts/scripts/fetch-official-cases.ps1 b/backend-ts/scripts/fetch-official-cases.ps1 index 10bc3af6..07eb315a 100644 --- a/backend-ts/scripts/fetch-official-cases.ps1 +++ b/backend-ts/scripts/fetch-official-cases.ps1 @@ -14,17 +14,28 @@ param( $ErrorActionPreference = "Stop" $repo = "https://github.com/cqframework/dqm-content-qicore-2025.git" +# The five GATED measures, plus three CANDIDATES. A candidate is checked out but is deliberately NOT in +# `OFFICIAL_GATED_MEASURES` — its artifact is not vendored yet, so adding it to the gate would fail the +# deck. Checking them out is what lets `pnpm official:terminology-audit` and the credentialed +# `vendor-official-measure.yml` workflow read their bundles at the pinned commit without a 17 MB pull +# (ADR-053: that audit is how CMS138's absent value set was found). $paths = @( "bundles/measure/CMS122FHIRDiabetesAssessGT9Pct", "bundles/measure/CMS125FHIRBreastCancerScreen", "bundles/measure/CMS2FHIRPCSDepScreenAndFollowUp", "bundles/measure/CMS68FHIRDocumentationCurrentMeds", "bundles/measure/CMS951FHIRKidneyHealthEval", + "bundles/measure/CMS130FHIRColorectalCancerScrn", + "bundles/measure/CMS138FHIRTobaccoScrnCessation", + "bundles/measure/CMS165FHIRControllingHighBP", "input/tests/measure/CMS122FHIRDiabetesAssessGT9Pct", "input/tests/measure/CMS125FHIRBreastCancerScreen", "input/tests/measure/CMS2FHIRPCSDepScreenAndFollowUp", "input/tests/measure/CMS68FHIRDocumentationCurrentMeds", - "input/tests/measure/CMS951FHIRKidneyHealthEval" + "input/tests/measure/CMS951FHIRKidneyHealthEval", + "input/tests/measure/CMS130FHIRColorectalCancerScrn", + "input/tests/measure/CMS138FHIRTobaccoScrnCessation", + "input/tests/measure/CMS165FHIRControllingHighBP" ) $ContentDir = [System.IO.Path]::GetFullPath($ContentDir) diff --git a/backend-ts/src/wiring/vendor-workflow-safety.test.ts b/backend-ts/src/wiring/vendor-workflow-safety.test.ts new file mode 100644 index 00000000..09d6f2aa --- /dev/null +++ b/backend-ts/src/wiring/vendor-workflow-safety.test.ts @@ -0,0 +1,105 @@ +/** + * The credentialed vendor workflow must not leak licensed terminology, and must not gain write access. + * + * ## What this guards + * + * `vendor-official-measure.yml` runs `vendor:official` with `WORKWELL_VSAC_API_KEY_VENDOR` and uploads + * the result as a downloadable artifact. Three files land in the output directory and only two of them + * may leave the runner: + * + * - `bundle.json`, `manifest.json` — already committed to this **public** repo. Counts, provenance, + * and the sidecar's SHA-256. No codes. + * - `terminology.json` — **thousands of AMA CPT and SNOMED CT codes under an NLM licence.** It is + * gitignored precisely so it is never redistributed (ADR-036). An artifact URL is redistribution. + * + * The difference between those outcomes is one `cp` line, or one `path:` that globs the directory + * instead of naming files. That is exactly the kind of edit that gets made in a hurry to "just grab + * everything", and it would be invisible in review — the artifact is a zip nobody opens. + * + * ## Why a text scan rather than a YAML parse + * + * No YAML parser is available (CLAUDE.md forbids new dependencies) and one is not needed: the property + * is "the string `terminology.json` never appears in a step that copies or uploads", which a scan + * answers directly. The same reason `official-flip-config.test.ts` reads its workflows as text. + * + * It runs unconditionally — no sidecar, no checkout, no network — so it cannot self-skip. + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +const WORKFLOW = fileURLToPath( + new URL("../../../.github/workflows/vendor-official-measure.yml", import.meta.url), +); +const yaml = readFileSync(WORKFLOW, "utf8"); + +test("the vendor workflow exists and is dispatch-only", () => { + // `workflow_dispatch` alone: no `push`, no `pull_request`. A credential-consuming job that ran on + // every push would hit NLM on every commit, and would run on any branch a contributor could create. + assert.match(yaml, /^on:\s*$/m); + assert.match(yaml, /^\s{2}workflow_dispatch:/m); + for (const trigger of ["push:", "pull_request:", "pull_request_target:", "schedule:"]) { + assert.ok(!new RegExp(`^\\s{2}${trigger.replace(":", ":")}`, "m").test(yaml), `must not trigger on ${trigger}`); + } +}); + +test("it never copies or uploads the licensed terminology sidecar", () => { + // The load-bearing assertion. `terminology.json` may be MENTIONED in comments — the header explains + // at length why it is excluded — so this checks the lines that move bytes, not the whole file. + const moving = yaml + .split("\n") + .filter((line) => !line.trim().startsWith("#")) + .filter((line) => /\bcp\b|\bmv\b|\bcp -|path:|paths:|tar\b|zip\b/.test(line)); + assert.ok(moving.length > 0, "expected to find the copy/upload lines — if this is empty the scan is vacuous"); + for (const line of moving) { + assert.ok( + !line.includes("terminology.json"), + `a step that moves bytes must never name terminology.json — it carries licensed codes (ADR-036): ${line.trim()}`, + ); + } +}); + +test("the staged directory is populated by NAMED files, never a directory glob", () => { + // A glob (`cp -r …/${id}/ …` or `cp …/*`) would sweep the sidecar in without ever naming it, so the + // assertion above would pass while the artifact carried licensed codes. Both committable files must + // be copied explicitly, and nothing may be copied by wildcard. + const copies = yaml.split("\n").filter((l) => /^\s*cp\s/.test(l)); + assert.equal(copies.length, 2, `expected exactly two explicit cp lines, got ${copies.length}`); + assert.ok(copies.some((l) => l.includes("bundle.json")), "bundle.json must be copied explicitly"); + assert.ok(copies.some((l) => l.includes("manifest.json")), "manifest.json must be copied explicitly"); + for (const line of copies) { + assert.ok(!/[*?]/.test(line), `no wildcard may appear in a copy step: ${line.trim()}`); + assert.ok(!/\s-r\b|\s-a\b|--recursive/.test(line), `no recursive copy may appear: ${line.trim()}`); + } +}); + +test("it asks for read permissions only", () => { + // It uploads an artifact for a human to review and commit. `contents: write` would make it a standing + // ability to push to the repo, which is a much larger capability than the job needs. + assert.match(yaml, /^permissions:\s*\n\s+contents:\s*read\s*$/m); + // Comment lines excluded, because the workflow's own header EXPLAINS why it does not take + // `contents: write` — and the first cut of this test failed on that sentence. A guard that cannot + // tell a prohibition from its own rationale is a guard that gets deleted rather than fixed. + const effective = yaml.split("\n").filter((l) => !l.trim().startsWith("#")); + assert.ok( + !effective.some((l) => /contents:\s*write/.test(l)), + "the vendor workflow must not request write access", + ); +}); + +test("it refuses to run without the credential rather than producing an incomplete artifact", () => { + // Without the key the vendor script WARNS and leaves capped/absent value sets as upstream shipped + // them. That is correct behaviour and exactly the artifact this workflow exists to avoid producing — + // one that looks vendored and cannot be routed. Failing early is the difference between "no artifact" + // and "an artifact someone commits". + assert.match(yaml, /Refuse to run without the credential/); + assert.match(yaml, /if \[ -z "\$WORKWELL_VSAC_API_KEY" \]/); + assert.match(yaml, /exit 1/); +}); + +test("it passes --complete-terminology, which is the entire point of running it credentialed", () => { + assert.match(yaml, /--complete-terminology/); + assert.match(yaml, /--strip-elm-annotations/); + assert.match(yaml, /WORKWELL_VSAC_API_KEY_VENDOR/); +}); diff --git a/docs/DEPLOY.md b/docs/DEPLOY.md index 5d63962f..ee1479b1 100644 --- a/docs/DEPLOY.md +++ b/docs/DEPLOY.md @@ -105,6 +105,37 @@ period against). Only the capped OIDs are re-expanded — today one, two request > exists to keep those two terminology authorities apart; one secret name would invite exactly the > conflation it forbids. +##### Step 1b — vendoring a NEW measure when you cannot read the secret + +`WORKWELL_VSAC_API_KEY_VENDOR` is a GitHub secret. Secrets are write-only — `gh secret list` returns +names, never values — so a completed artifact cannot be produced from a clone even though the credential +is configured and CI uses it on every push. + +That is a tooling gap, not an owner gap, and it had been recorded as the latter. The fix is a manual +trigger that runs the existing command in the one place the credential already lives: + +```bash +gh workflow run vendor-official-measure.yml \ + -f measure=CMS138FHIRTobaccoScrnCessation -f catalog_id=cms138 +gh run watch # the log prints truncated + completion +gh run download -n vendored-cms138 -D backend-ts/measures/official/ +``` + +It uploads **`bundle.json` and `manifest.json` only**. `terminology.json` stays on the runner: it holds +thousands of AMA CPT and SNOMED CT codes under an NLM licence, is gitignored for that reason (ADR-036), +and an artifact URL is redistribution. `vendor-workflow-safety.test.ts` pins that — named files, no +directory glob, no recursive copy, `contents: read` only — and is mutation-checked against the exact +edit that would sweep the sidecar in (`cp -r …/*`). + +The job **fails** rather than running without the credential: an uncredentialed vendor produces an +artifact that looks vendored and cannot be routed, which is worse than none. + +Then, in one PR: commit the two files, add the measure to `OFFICIAL_GATED_MEASURES`, to the deploy +workflows' vendor lists, and to `fetch-official-cases.ps1` if it is not already a candidate there. CI +re-derives the same bytes and runs the measure's MADiE deck — **that deck is the check**, especially for +an ADR-053 absent value set, where nothing in the vendoring can tell a correct expansion from a wrong +one of the right size. + ##### Step 1a (cont.) — value sets upstream ships **no ValueSet resource for at all** (ADR-053) A second, different incompleteness, and the same flag now handles it. A measure's ELM can retrieve a diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index b002b694..8eb77d3c 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -1,5 +1,42 @@ # Journal +## 2026-07-31 (M-A) — the "owner step" was a tooling gap: credentialed vendoring gets a command (branch `feat/vendor-workflow-cms138`) + +Three measures — CMS130, CMS165 (capped, ADR-041) and CMS138 (absent, ADR-053) — were recorded as +blocked on "an owner step with the VSAC key". **That framing was wrong, and the owner was right to push +back on it.** `WORKWELL_VSAC_API_KEY_VENDOR` has been a GitHub secret since 2026-07-29. What was missing +was not the credential but a way to run the vendor command in the one place the credential already +lives: CI has used it on every push for two days. + +`vendor-official-measure.yml` is a `workflow_dispatch` job that vendors one measure with +`--complete-terminology` and uploads the two committable files as an artifact. It takes **`contents: +read`** and uploads nothing else — deliberately not a workflow that can push, because that is a standing +capability where this is a one-shot whose output a human reviews. + +**The licensed-content boundary is the load-bearing part.** Three files land in the output directory and +only two may leave the runner: `bundle.json` and `manifest.json` are already committed to this public +repo, while `terminology.json` holds thousands of AMA CPT and SNOMED CT codes under an NLM licence and +is gitignored precisely so it is never redistributed (ADR-036) — an artifact URL is redistribution. The +difference is one `cp` line, and it would be invisible in review because an artifact is a zip nobody +opens. `vendor-workflow-safety.test.ts` therefore asserts named files, no wildcard, no recursive copy, +dispatch-only triggers, `contents: read`, and the fail-closed credential check. Mutation-checked against +`cp -r …/*` — the exact edit someone makes to "just grab everything" — which fails exactly one test. + +Its first cut failed on its OWN comment: the workflow header explains why it does not take +`contents: write`, and the prohibition matched that sentence. A guard that cannot tell a rule from its +own rationale gets deleted rather than fixed, so it now scans non-comment lines only. + +Also: `fetch-official-cases.ps1` now checks out CMS130/CMS138/CMS165 as **candidates** — sparse-checked +out but deliberately absent from `OFFICIAL_GATED_MEASURES`, since their artifacts are not vendored and +adding them to the gate would fail the deck. That is what lets `pnpm official:terminology-audit` read +their bundles at the pinned commit, which is how ADR-053's finding was made in the first place. + +**GitHub requires a `workflow_dispatch` file to exist on the default branch before it can be +dispatched**, so this lands on its own rather than bundled with the vendored artifacts it produces. The +artifacts, the gate wiring and the MADiE verdict follow in the next PR — and for CMS138 that verdict is +the real check, because nothing in the vendoring can distinguish a correct expansion from a wrong one of +the right size. + ## 2026-07-31 (M-A) — "will not expand" was the wrong sentence: upstream ships CMS138 one value set short (branch `fix/official-terminology-absent-valuesets`) ADR-047's table reads *"CMS138 tobacco screening | **0/47, 47 errors** — one value set (…3.526.3.1278) From fe048da631d0211915bb89f80dde648d94292e67 Mon Sep 17 00:00:00 2001 From: Taleef Date: Fri, 31 Jul 2026 11:49:19 -0400 Subject: [PATCH 2/2] fix(official): dispatch inputs never reach a shell, and the completeness check read the wrong field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from review of #365, both real. P1 — SHELL INJECTION. `${{ inputs.* }}` was interpolated directly into `run:` scripts, including the step that holds WORKWELL_VSAC_API_KEY. Command substitution executes inside double quotes, so `$(...)` in a dispatch input would run with the credential in the environment. Only write-access users can dispatch, which lowers the odds and not the severity. Inputs now pass through `env:`, are validated against the same character classes `parseArgs` enforces, and are re-exported as step outputs so later steps read a value already checked. P2 — THE COMPLETENESS CHECK COULD NOT SEE ITS OWN SUBJECT. The report step warned on a non-empty `manifest.terminology.truncated`. An ABSENT value set never appears there — so for CMS138, the one measure this workflow was built for, the check was warning-free BY CONSTRUCTION and the workflow would have uploaded exactly the unroutable artifact it claims to reject. Worse, `completeTerminology` fails closed and exits 0: an expired key, an unreachable VSAC, a short expansion or a wrong-OID echo all leave terminology as upstream shipped it while the vendor step still succeeds. There is now a verification step that runs the REAL runtime predicates — `absentValueSets(artifact, requiredOids(artifact))` and the manifest's `truncated` — over the produced artifact, and fails the job if either is non-empty. Same functions `officialRoutingProblems` calls, so "passed here" and "routing accepts it" cannot drift. That is the third instance this run of a check reading a field the failure does not appear in. Both new guards are mutation-checked: re-introducing interpolation, checking `truncated` alone, and deleting the verification step each fail exactly one test. Also from review: `gh run download -n` extracts into `-D` directly, so DEPLOY.md's command pointed one level too high — corrected to the catalog directory. Suite 1757 pass / 0 fail / 15 skipped; typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q3BNWAJwJJ5QjP1X6JpHUo --- .github/workflows/vendor-official-measure.yml | 119 ++++++++++++++---- .../src/wiring/vendor-workflow-safety.test.ts | 50 ++++++++ docs/DEPLOY.md | 15 ++- docs/JOURNAL.md | 11 ++ 4 files changed, 169 insertions(+), 26 deletions(-) diff --git a/.github/workflows/vendor-official-measure.yml b/.github/workflows/vendor-official-measure.yml index 4dd9ff57..31af74e9 100644 --- a/.github/workflows/vendor-official-measure.yml +++ b/.github/workflows/vendor-official-measure.yml @@ -9,8 +9,8 @@ # That produced a false blocker. Three priority measures (CMS130, CMS165 capped; CMS138 absent — see # ADR-041 and ADR-053) were recorded as needing "an owner step", when what they actually needed was a # way to run the existing vendor command in the one place the credential already lives. This is that -# way: a manual trigger that vendors, prints what moved, and uploads `bundle.json` + `manifest.json` as -# an artifact to download and commit. +# way: a manual trigger that vendors, VERIFIES, and uploads `bundle.json` + `manifest.json` as an +# artifact to download and commit. # # ## Why it uploads rather than commits # @@ -25,6 +25,14 @@ # thousands of AMA CPT and SNOMED CT codes under an NLM licence and is gitignored for that reason # (ADR-036). Uploading it would redistribute licensed content through an artifact URL, so the copy step # names the two files explicitly rather than globbing the directory. +# +# ## Dispatch inputs never reach a shell (review of #365, P1) +# +# Every `${{ inputs.* }}` is passed through `env:` and validated before use. Interpolating them into a +# `run:` script would splice attacker-controlled text into a step that holds the VSAC credential — +# command substitution executes inside double quotes, so `$(...)` in an input would run with the secret +# in the environment. Only users with write access can dispatch, which lowers the odds and not the +# severity. name: Vendor official measure (credentialed) on: @@ -48,7 +56,7 @@ permissions: jobs: vendor: - name: Vendor ${{ inputs.catalog_id }} with the VSAC credential + name: Vendor with the VSAC credential runs-on: ubuntu-latest timeout-minutes: 20 defaults: @@ -58,6 +66,36 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: { node-version: 24 } + - uses: pnpm/action-setup@v4 + with: { version: 10 } + + # Validated BEFORE anything uses them, and re-exported as step outputs so later steps read a value + # this step has already checked rather than the raw input. The character classes are the same ones + # `parseArgs` enforces in the vendor script, so a value that passes here cannot be rejected there. + - name: Validate dispatch inputs + id: inputs + env: + IN_MEASURE: ${{ inputs.measure }} + IN_CATALOG_ID: ${{ inputs.catalog_id }} + IN_REF: ${{ inputs.ref }} + run: | + case "$IN_MEASURE" in + *[!A-Za-z0-9]*|"") echo "::error::measure must be alphanumeric (got '$IN_MEASURE')"; exit 1 ;; + esac + case "$IN_CATALOG_ID" in + *[!a-z0-9]*|"") echo "::error::catalog_id must be lowercase alphanumeric (got '$IN_CATALOG_ID')"; exit 1 ;; + esac + if [ -n "$IN_REF" ]; then + case "$IN_REF" in + *[!0-9a-f]*) echo "::error::ref must be a 40-char lowercase hex sha"; exit 1 ;; + esac + [ "${#IN_REF}" -eq 40 ] || { echo "::error::ref must be exactly 40 characters"; exit 1; } + fi + { + echo "measure=$IN_MEASURE" + echo "catalog_id=$IN_CATALOG_ID" + echo "ref=$IN_REF" + } >> "$GITHUB_OUTPUT" # Fails the job rather than vendoring an incomplete artifact that looks fine. Without the # credential the script warns and leaves capped/absent value sets exactly as upstream shipped @@ -71,6 +109,9 @@ jobs: exit 1 fi + - name: Install dependencies + run: pnpm install --frozen-lockfile + # Same sparse checkout the eCQM gate uses, so the bundle is read locally at the pinned commit # instead of pulling ~17 MB from raw.githubusercontent. - name: Fetch official content (pinned commit) @@ -81,45 +122,75 @@ jobs: # Never fails the job — it is a measurement, and `official-terminology-audit.mjs` exits 0 by design. - name: Audit declared-vs-shipped terminology continue-on-error: true - run: node scripts/official-terminology-audit.mjs "${{ inputs.measure }}" + env: + MEASURE: ${{ steps.inputs.outputs.measure }} + run: node scripts/official-terminology-audit.mjs "$MEASURE" - name: Vendor with completion env: WORKWELL_VSAC_API_KEY: ${{ secrets.WORKWELL_VSAC_API_KEY_VENDOR }} + MEASURE: ${{ steps.inputs.outputs.measure }} + CATALOG_ID: ${{ steps.inputs.outputs.catalog_id }} + REF: ${{ steps.inputs.outputs.ref }} run: | - node scripts/vendor-official-measure.mjs \ - --measure "${{ inputs.measure }}" \ - --catalog-id "${{ inputs.catalog_id }}" \ - --strip-elm-annotations \ - --complete-terminology \ - ${{ inputs.ref && format('--ref {0}', inputs.ref) || '' }} + set -- --measure "$MEASURE" --catalog-id "$CATALOG_ID" --strip-elm-annotations --complete-terminology + if [ -n "$REF" ]; then set -- "$@" --ref "$REF"; fi + node scripts/vendor-official-measure.mjs "$@" - # The manifest is the evidence. Printed into the log so the outcome is visible without - # downloading anything: `truncated` must be empty, and `completion` must record what was sourced - # and from which VSAC release. - - name: Report what the credential actually changed + # THE GATE, and it is not a formality (review of #365, P2). + # + # `completeTerminology` fails CLOSED: an expired key, an unreachable VSAC, a short expansion or a + # wrong-OID echo all leave the terminology as upstream shipped it and exit 0. So the vendor step + # succeeding says nothing about whether the artifact is usable, and the first cut of this workflow + # checked only `manifest.terminology.truncated` — which an ABSENT value set never appears in. + # For CMS138, the one measure this was built for, that check was warning-free by construction and + # the workflow would have uploaded exactly the unroutable artifact it claims to reject. + # + # So it now runs the REAL runtime predicates over the produced artifact: `absentValueSets` (the + # ELM's declared canonicals minus what the sidecar holds) and the manifest's own `truncated`. Same + # functions `officialRoutingProblems` calls, so "this passed here" and "routing will accept it" + # cannot drift apart. + - name: Verify the artifact is actually complete + env: + CATALOG_ID: ${{ steps.inputs.outputs.catalog_id }} run: | - node -e ' - const m = require("./measures/official/${{ inputs.catalog_id }}/manifest.json"); - const t = m.terminology ?? {}; + pnpm exec node --import tsx -e ' + const id = process.env.CATALOG_ID; + const { loadOfficialArtifact } = await import("./src/wiring/official-artifacts.ts"); + const { absentValueSets } = await import("./src/wiring/official-terminology.ts"); + const { requiredOids } = await import("./src/wiring/official-executor-adapter.ts"); + const artifact = loadOfficialArtifact(id); + if (!artifact) { console.log(`::error::${id}: the vendor step produced no loadable artifact`); process.exit(1); } + const m = artifact.manifest, t = m.terminology ?? {}; console.log(`measure ${m.measureName} v${m.version} (${m.cmsId ?? "no cmsId"})`); console.log(`terminology ${t.valueSets} value sets, ${t.codes} codes`); - console.log(`truncated ${JSON.stringify(t.truncated ?? [])}`); console.log(`completion ${JSON.stringify(t.completion ?? null, null, 2)}`); - if ((t.truncated ?? []).length > 0) { - console.log("::warning::truncated is NOT empty — this artifact is still incomplete and routing will refuse it."); + const truncated = t.truncated ?? []; + const absent = absentValueSets(artifact, requiredOids(artifact)); + console.log(`truncated ${JSON.stringify(truncated)}`); + console.log(`absent ${JSON.stringify(absent)}`); + if (truncated.length > 0 || absent.length > 0) { + console.log(`::error::${id} is INCOMPLETE — ${truncated.length} capped, ${absent.length} absent. Routing would refuse it, so nothing is uploaded. Check the vendor log above for the VSAC warning that explains why.`); + process.exit(1); } + console.log(`${id}: terminology complete — nothing capped, nothing absent.`); ' # Explicit paths, never a directory glob: `terminology.json` sits beside these two and must not # leave the runner (see the header). - name: Stage the committable files only + env: + CATALOG_ID: ${{ steps.inputs.outputs.catalog_id }} run: | - mkdir -p ../_vendored/${{ inputs.catalog_id }} - cp measures/official/${{ inputs.catalog_id }}/bundle.json ../_vendored/${{ inputs.catalog_id }}/ - cp measures/official/${{ inputs.catalog_id }}/manifest.json ../_vendored/${{ inputs.catalog_id }}/ - echo "staged:"; ls -la ../_vendored/${{ inputs.catalog_id }}/ + mkdir -p "../_vendored/$CATALOG_ID" + cp "measures/official/$CATALOG_ID/bundle.json" "../_vendored/$CATALOG_ID/" + cp "measures/official/$CATALOG_ID/manifest.json" "../_vendored/$CATALOG_ID/" + echo "staged:"; ls -la "../_vendored/$CATALOG_ID/" + # `path:` is the STAGED directory, so the archive root holds the two files. `gh run download -n` + # extracts an artifact's contents directly into `-D`, so the documented command points `-D` at the + # catalog directory itself (review of #365) — pointing it at `measures/official/` would drop the + # files a level too high, where neither the vendor script nor the runtime looks for them. - uses: actions/upload-artifact@v4 with: name: vendored-${{ inputs.catalog_id }} diff --git a/backend-ts/src/wiring/vendor-workflow-safety.test.ts b/backend-ts/src/wiring/vendor-workflow-safety.test.ts index 09d6f2aa..b224fa3e 100644 --- a/backend-ts/src/wiring/vendor-workflow-safety.test.ts +++ b/backend-ts/src/wiring/vendor-workflow-safety.test.ts @@ -103,3 +103,53 @@ test("it passes --complete-terminology, which is the entire point of running it assert.match(yaml, /--strip-elm-annotations/); assert.match(yaml, /WORKWELL_VSAC_API_KEY_VENDOR/); }); + +test("no dispatch input is interpolated into a shell script (review, #365)", () => { + // `${{ inputs.* }}` inside a `run:` block splices attacker-controlled text into the shell — and + // command substitution executes inside double quotes, so `$(...)` in an input would run in a step + // that holds the VSAC credential. Only write-access users can dispatch, which lowers the odds and + // not the severity. Inputs go through `env:` and are validated first. + // + // Scanned line-by-line with a `run:`-block tracker rather than over the whole file, because + // `env:` mappings and `with:` blocks legitimately carry `${{ inputs.* }}` — that IS the fix. + const lines = yaml.split("\n"); + let inRun = false; + let runIndent = 0; + const offenders: string[] = []; + for (const line of lines) { + const indent = line.length - line.trimStart().length; + if (/^\s*(- )?run: \|/.test(line) || /^\s*run: \|/.test(line)) { + inRun = true; + runIndent = indent; + continue; + } + // A `run:` block ends at the next key at or above its own indentation. + if (inRun && line.trim() !== "" && indent <= runIndent && /^\s*[-\w]/.test(line)) inRun = false; + if (inRun && /\$\{\{\s*inputs\./.test(line)) offenders.push(line.trim()); + } + assert.deepEqual(offenders, [], "dispatch inputs must reach the shell via env:, never by interpolation"); + // Non-degeneracy: if the tracker never entered a run block, the loop above proves nothing. + assert.ok(yaml.includes("run: |"), "expected multi-line run blocks to scan"); +}); + +test("it REFUSES to upload an artifact whose terminology is still incomplete (review, #365)", () => { + // `completeTerminology` fails closed and exits 0 — an expired key, an unreachable VSAC, a short + // expansion or a wrong-OID echo all leave the terminology as upstream shipped it. So the vendor step + // succeeding says nothing about whether the artifact is usable. + // + // The first cut checked only `manifest.terminology.truncated`, which an ABSENT value set never + // appears in — so for CMS138, the measure this workflow was built for, the check was warning-free by + // construction and the workflow would have uploaded exactly the unroutable artifact it claims to + // reject. It must consult BOTH conditions, using the real runtime predicates. + assert.match(yaml, /Verify the artifact is actually complete/); + assert.match(yaml, /absentValueSets/, "must consult absent value sets, not only `truncated`"); + assert.match(yaml, /requiredOids/, "absentValueSets needs the ELM's declared canonicals"); + assert.match(yaml, /truncated\.length > 0 \|\| absent\.length > 0/, "both conditions must fail the job"); + + // And it must come BEFORE the staging/upload steps, or it reports on an artifact already published. + const verifyAt = yaml.indexOf("Verify the artifact is actually complete"); + const stageAt = yaml.indexOf("Stage the committable files only"); + const uploadAt = yaml.indexOf("upload-artifact"); + assert.ok(verifyAt > 0 && stageAt > verifyAt, "verification must precede staging"); + assert.ok(uploadAt > verifyAt, "verification must precede upload"); +}); diff --git a/docs/DEPLOY.md b/docs/DEPLOY.md index ee1479b1..f57256c5 100644 --- a/docs/DEPLOY.md +++ b/docs/DEPLOY.md @@ -118,9 +118,14 @@ trigger that runs the existing command in the one place the credential already l gh workflow run vendor-official-measure.yml \ -f measure=CMS138FHIRTobaccoScrnCessation -f catalog_id=cms138 gh run watch # the log prints truncated + completion -gh run download -n vendored-cms138 -D backend-ts/measures/official/ +gh run download -n vendored-cms138 -D backend-ts/measures/official/cms138 ``` +`-D` points at the **catalog directory**, not at `measures/official/`. With `-n` selecting a single +artifact, `gh run download` extracts its contents directly into `-D`, and this artifact's root holds +`bundle.json`/`manifest.json` — so the shorter path drops them one level too high, where neither the +vendor script nor the runtime looks (review of #365). + It uploads **`bundle.json` and `manifest.json` only**. `terminology.json` stays on the runner: it holds thousands of AMA CPT and SNOMED CT codes under an NLM licence, is gitignored for that reason (ADR-036), and an artifact URL is redistribution. `vendor-workflow-safety.test.ts` pins that — named files, no @@ -128,7 +133,13 @@ directory glob, no recursive copy, `contents: read` only — and is mutation-che edit that would sweep the sidecar in (`cp -r …/*`). The job **fails** rather than running without the credential: an uncredentialed vendor produces an -artifact that looks vendored and cannot be routed, which is worse than none. +artifact that looks vendored and cannot be routed, which is worse than none. It also **fails rather than +uploading an incomplete artifact** — `completeTerminology` fails closed and exits 0, so an expired key, +an unreachable VSAC, a short expansion or a wrong-OID echo all leave the terminology as upstream shipped +it while the vendor step still succeeds. A verification step runs the real runtime predicates +(`absentValueSets` + the manifest's `truncated`) over the produced artifact and stops the job if either +is non-empty. Checking only `truncated` would have been useless for the very measure this was built +for — an ABSENT value set never appears there. Then, in one PR: commit the two files, add the measure to `OFFICIAL_GATED_MEASURES`, to the deploy workflows' vendor lists, and to `fetch-official-cases.ps1` if it is not already a candidate there. CI diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index 8eb77d3c..755804b1 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -31,6 +31,17 @@ out but deliberately absent from `OFFICIAL_GATED_MEASURES`, since their artifact adding them to the gate would fail the deck. That is what lets `pnpm official:terminology-audit` read their bundles at the pinned commit, which is how ADR-053's finding was made in the first place. +**Review (#365) found two more, both of the same family as the rest of this run.** (1) Dispatch inputs +were interpolated straight into `run:` scripts — including the step holding the VSAC credential, where +`$(...)` in an input would execute. Only write-access users can dispatch, which lowers the odds and not +the severity; inputs now pass through `env:` and are validated first. (2) **The completeness report read +the wrong field.** It warned on non-empty `truncated` — which an ABSENT value set never appears in — so +for CMS138, the one measure this was built for, it was warning-free by construction and the workflow +would have uploaded exactly the unroutable artifact it claims to reject. `completeTerminology` fails +closed and exits 0, so the vendor step succeeding says nothing. There is now a verification step running +the REAL runtime predicates (`absentValueSets` + `truncated`) before staging, and both new guards are +mutation-checked. Third instance this run of "a check that reads a field the failure does not appear in". + **GitHub requires a `workflow_dispatch` file to exist on the default branch before it can be dispatched**, so this lands on its own rather than bundled with the vendored artifacts it produces. The artifacts, the gate wiring and the MADiE verdict follow in the next PR — and for CMS138 that verdict is