feat(official): credentialed vendoring gets a command — the "owner step" was a tooling gap - #365
Conversation
…ep" was a tooling gap 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q3BNWAJwJJ5QjP1X6JpHUo
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b759e777f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| --measure "${{ inputs.measure }}" \ | ||
| --catalog-id "${{ inputs.catalog_id }}" \ | ||
| --strip-elm-annotations \ | ||
| --complete-terminology \ | ||
| ${{ inputs.ref && format('--ref {0}', inputs.ref) || '' }} |
There was a problem hiding this comment.
Because measure, catalog_id, and especially the unquoted ref are interpolated directly into a run: script, a user allowed to dispatch this workflow can supply shell syntax such as $(...); command substitution still executes inside the double quotes on lines 91–92, and this step exposes WORKWELL_VSAC_API_KEY. Pass the inputs through env and reference the resulting shell variables after validating them, rather than embedding ${{ inputs.* }} in the credentialed script.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fe048da. Right, and the severity point stands even though only write-access users can dispatch — that lowers the odds, not the consequence, and the credential is right there in the same step.
Every ${{ inputs.* }} now reaches the shell through env: only. Added a Validate dispatch inputs step that checks each against the same character classes parseArgs already enforces in the vendor script ([A-Za-z0-9] for measure, [a-z0-9] for catalog id, 40-char lowercase hex for ref) and re-exports them as step outputs, so later steps read a value that has already been checked rather than the raw input. ref — which you correctly singled out as the worst, since it was unquoted inside a format() — is now built with set -- instead of string concatenation.
Pinned by no dispatch input is interpolated into a shell script, which tracks run: block boundaries rather than scanning the whole file (an env: mapping carrying ${{ inputs.* }} is the fix, so a naive scan would forbid the remedy). Mutation-checked: putting ${{ inputs.measure }} back into the credentialed run fails it.
| if ((t.truncated ?? []).length > 0) { | ||
| console.log("::warning::truncated is NOT empty — this artifact is still incomplete and routing will refuse it."); |
There was a problem hiding this comment.
When the secret is nonempty but expired, VSAC is unavailable, or VSAC returns an invalid expansion, completeTerminology catches the error and exits successfully with the capped or absent set unresolved. This step merely emits a warning and the following steps still publish the files; for CMS138 an unresolved absent set is not even represented in truncated, so this report remains warning-free. The workflow therefore returns exactly the unroutable artifact it claims to reject and should verify that no required set remains unresolved before staging.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fe048da, and this was the best finding on the PR — you identified the precise reason the check was worthless rather than just that it was weak.
"for CMS138 an unresolved absent set is not even represented in truncated, so this report remains warning-free" — exactly. The workflow was built for CMS138, and its completeness check was warning-free by construction for the only condition CMS138 has. It would have uploaded exactly the unroutable artifact it claims to reject. That is the third instance of this shape in this run of work (a check reading a field the failure does not appear in), and this one was inside a guard I added to close a blind spot of the same kind.
Your point about completeTerminology exiting 0 is also load-bearing and I had leaned on it the wrong way: it fails closed by design — expired key, VSAC unreachable, short expansion, wrong-OID echo all leave upstream terminology untouched and exit 0 — so the vendor step succeeding carries no information about usability at all.
There is now a Verify the artifact is actually complete step that runs the real runtime predicates over the produced artifact: absentValueSets(artifact, requiredOids(artifact)) plus the manifest's truncated, failing the job if either is non-empty. Deliberately the same functions officialRoutingProblems calls, so "this passed the workflow" and "routing will accept it" cannot drift apart — that required adding pnpm install to the job, which is worth it to avoid a fourth reimplementation of the rule.
Pinned by it REFUSES to upload an artifact whose terminology is still incomplete, which also asserts the step ORDER (verification before staging and upload — a check that reports on an already-published artifact is not a gate). Mutation-checked: narrowing it back to truncated alone, or deleting the step, each fail it.
| 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 <run-id> -n vendored-cms138 -D backend-ts/measures/official/ |
There was a problem hiding this comment.
With -n selecting one artifact, gh run download extracts that artifact directly into the requested destination, as documented by the GitHub CLI manual. Because the uploaded directory is rooted at _vendored/cms138, its archive contains bundle.json and manifest.json at the artifact root, so this command writes them to backend-ts/measures/official/ rather than backend-ts/measures/official/cms138/, where the vendor and runtime expect them. Point -D at the catalog directory or retain that directory in the uploaded archive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, fixed in fe048da. Verified against the CLI manual you linked: with -n selecting one artifact, gh run download extracts that archive's contents directly into -D. The archive root holds bundle.json/manifest.json because path: is the staged _vendored/<catalog_id> directory, so -D backend-ts/measures/official/ would have written them a level too high — where neither loadOfficialArtifact nor the vendor script looks.
DEPLOY.md now points -D at the catalog directory (backend-ts/measures/official/cms138) and says why, so the next person does not re-derive it from a silent failure. Chose that over restructuring the upload: keeping the archive rooted at the two files is what lets vendor-workflow-safety.test.ts assert exactly two explicitly-named cp lines with no wildcard and no recursive copy — the guard that keeps the licensed terminology.json off the artifact.
…ess check read the wrong field 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q3BNWAJwJJ5QjP1X6JpHUo
1 participant
The correction
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_VENDORhas 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. GitHub secrets are write-only —
gh secret listreturns names, never values — so no clone can produce a completed artifact, however willing its owner is. A tooling gap recorded as an owner gap.What this adds
vendor-official-measure.yml— aworkflow_dispatchjob that vendors one measure with--complete-terminology, printstruncated+completioninto the log, and uploads the two committable files as an artifact.It takes
contents: readand 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. It fails rather than running uncredentialed: an artifact that looks vendored and cannot be routed is worse than none.The load-bearing part: the licensed-content boundary
Three files land in the output directory and only two may leave the runner.
bundle.json,manifest.jsonterminology.jsonThe difference between those outcomes is one
cpline, or onepath:that globs the directory. That is exactly the edit made in a hurry to "just grab everything", and it would be invisible in review — an artifact is a zip nobody opens.vendor-workflow-safety.test.tsasserts named files, no wildcard, no recursive copy, dispatch-only triggers,contents: read, and the fail-closed credential check. Mutation-checked againstcp -r …/*, which fails exactly one test. Runs unconditionally — no sidecar, no checkout, no network — so it cannot self-skip.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.ps1checks out CMS130/CMS138/CMS165 as candidates — sparse-checked out but deliberately absent fromOFFICIAL_GATED_MEASURES, since their artifacts are not vendored and adding them to the gate would fail the deck. That is what letspnpm official:terminology-auditread their bundles at the pinned commit, which is how ADR-053's finding was made.Why this lands alone
GitHub requires a
workflow_dispatchfile to exist on the default branch before it can be dispatched. So the workflow ships first; the vendored artifacts, gate wiring and MADiE verdict follow in the next PR.For CMS138 that verdict is the real check: nothing in the vendoring can distinguish a correct expansion from a wrong one of the right size. 47 committed cases with upstream's own expected population vectors can. It is 0/47 today.
Suite 1755 pass / 0 fail / 15 skipped; typecheck clean.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Q3BNWAJwJJ5QjP1X6JpHUo