Validate every uses: value, and type the callsite (#59, #57) - #60
Merged
Conversation
`ActionPinFile.parse` dropped every line its regex did not match, so a typo'd key yielded the jar-baked pin instead: a pin a consumer had deliberately held back reverted on the next generate, with no diagnostic anywhere. #57's half was that `ActionPins` fields were `String`, so a pin could not reach `ActionRef`'s check and a consumer had to handle a failure that could not happen. Auditing them turned up a case neither issue named: `checkout: actions/checkout` passes the old regex, and `Planner` built `Step(uses = Some(config.actions.checkout))` by direct case-class construction, which bypasses `ActionRef` entirely. So an unpinned action rendered into `ci.yml`, and `annotateUses` stamped the version comment beside it, describing a pin that was not there. Both halves are closed at the tier that owns them: - `Step.uses`, `Job.uses`, `WorkflowCall.uses` and all seven `ActionPins` fields are `ActionRef`, making an unvalidated ref unrepresentable rather than merely rejected. zio-blocks derives a neotype as its underlying primitive, so not one rendered byte moves. - `parse` returns `Either`, refusing four ways: a line that is not `key: ref # version`, a key that is not an `ActionPins.Field`, a ref `ActionRef` rejects, and a ref that is valid but names a *different* action than its key does. Only `Field.prefix` can catch that last one. - `ZipxPlugin.orFail` reports it, so a present-but-unreadable file is a build error naming the line. An absent file still falls back to the jar defaults. Writing the properties found two more: a pin line with no `# vX.Y.Z` inherited the base label, letting `annotateUses` stamp `# v7.0.1` onto a SHA that was not v7.0.1; and the field-matching predicate used a bare `startsWith`, filing `actions/cache/restore@v4` under the `actions/cache` pin. Grepping the tests for the shape that hid the first turned up a third: `String.contains` and `indexOf` widen their argument to `Any` through `StringOps`, so three assertions comparing an `ActionRef` against rendered YAML compiled and passed vacuously. `ConsumerStepsSpec` is the proof for #57: its `throw AssertionError` on an unreachable branch is gone, replaced by `Step.usesRef(pins.cache)`. Verified: 543 tests across core/workflow/central/docs; `zipxWorkflowGenerate` and `zipxActionsPull` both leave `.github/` byte-identical; `plugin/scripted zipx/generate-check` passes; the three pathological pin lines each fail the real plugin naming the line; and `grep -rn "throw \|makeOrThrow\|orThrow" modules/*/src/main` still returns nothing. Closes #59 Closes #57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #59. Closes #57. Both are half of one gap, and auditing them turned up a case neither names.
What was wrong
#59:
ActionPinFile.parsedropped every line its regex did not match, then folded overField.valuesfromActionPins.Bootstrap. A typo'd key (setup-jav:) or a stray indent silently yielded the jar-baked pin, andloadOptionreturnedSomebecause the file existed and was readable, so there was no diagnostic anywhere. A pin a consumer deliberately held back reverted on the nextzipxWorkflowGenerate.#57:
ActionPinsfields wereString, so a pin could not reachStep.uses'sinlinecheck and a consumer had to handle a failure that could not happen.The case neither issue names.
checkout: actions/checkout(no@sha) passes the old regex, andPlannerbuilt the step by direct case-class construction:That bypasses
ActionRefentirely, andRender.checkedonly runs the uses-vs-run shape check. So an unpinned action rendered intoci.yml, andannotateUsesstamped# v7.0.1beside it, describing a pin that was not there. Confirmed onmainbefore writing any of this, so the fix is against observed behaviour.A probe over the real regex found the failures split into three classes, not the one #59 describes.
ActionRef.makealone closes only part of it: an unknown key (setupJava2:, or the wrong-caseCheckout:) still vanishes, andcheckout: evil/malware@<sha>is a structurally valid ref pointing at the wrong action.What changed
Types, so the bad value is unrepresentable.
Step.uses,Job.uses,WorkflowCall.usesand all sevenActionPinsfields areActionRef. The literal pins keep their compile-time check for free. zio-blocks 0.0.51 derives a neotype as its underlying primitive, soOption[ActionRef]encodes exactly asOption[String]did and no rendered byte moves.StepBuilder.usesRefis now the in-zipx path;usesMakestays for genuinely raw strings.parsereturnsEither, refusing four ways, one per probed class:setup-jav: actions/setup-java@abc123key: ref # versionat allsetupJava2: actions/x@abc123ActionPins.Field(message lists the legal keys)checkout: actions/checkoutActionRefrejects an unpinned refcheckout: evil/malware@abc123The last is the one a shape check cannot see; only the key's own
Field.prefixcan.ZipxPlugin.orFailreports it, so a present-but-unreadable file is a build error naming the line. An absent file still falls back to the jar defaults, which is the documented behaviour.Three more defects, found by the tests rather than by reading.
# vX.Y.Zinherited the base version label, soannotateUsescould stamp# v7.0.1onto a SHA that was not v7.0.1. Same false-assurance shape as the unpinned ref. Found by the render/parse round-trip property.startsWith, which filedactions/cache/restore@v4under theactions/cachepin and failed to see that an unpinneduses: actions/checkoutwas the checkout pin at all. Now one sharednamesAction(== prefixorprefix + "@") used by bothparseandpullFromWorkflow.String.containsandindexOfwiden their argument toAnythroughStringOps, so an assertion comparing anActionRefagainst rendered YAML compiles and never matches. One real test failure led to a sweep:Packs.scalaandDependencyUpdates.scalaeach had assertions passing vacuously. Main sources were clean. Recorded inScalaStewardWorkflowSpec's scaladoc so the next.unwrapis not read as decoration.Tests
ActionPinFileSpecis rewritten: 10 negative cases grouped by defect class, each assertingisLeftand that the message names the line; 7 acceptances that must not regress (no space after the colon, tab separator, CRLF, no trailing newline, no version label, the committed pin file itself); and a line-number test that puts the bad line on line 6 so it cannot pass on an off-by-default.Five properties, all sourced from
ActionPins.Field.valuesso a new pin field is covered automatically:parse(render(pins)) == Right(pins)over generated pins (this is what found the version-label bug)Left(ActionPinFile.parse silently ignores unrecognized lines, so a malformed pin file yields bootstrap pins #59's claim, generalized over which field breaks)atob.prefix + "@" + shaisLeft, with the same-field case still parsing (all 49 pairs, not one example)Field.valuesis rejected as a keyActionRefaccepts anyowner/repo@40-hex, and an@refalone never suffices (inNamesSpec)ConsumerStepsSpecis the proof for #57: itsthrow AssertionErroron an unreachable branch is deleted, replaced byStep.usesRef(pins.cache). That was test scope, so it never appeared in thethrowgrep; its removal is what the issue was filed for.Verification
testFull, nottest.)zipxWorkflowGenerateandzipxActionsPullboth leave.github/byte-identical (git diff --exit-code). This is the real check thatOption[ActionRef]renders asOption[String]did.plugin/scripted zipx/generate-checkpasses, independent of the unit tests since it asserts literal gate strings.setup-jav:→.github/zipx/action-pins.yml:6: not a pin, a # comment, or a blank line; expected 'key: owner/action@ref # vX.Y.Z'checkout: actions/checkout→:5: invalid uses: value 'actions/checkout': add an @ref (a commit SHA pin); GitHub requires one and an unpinned action is a supply-chain riskcheckout: evil/malware@abc123→:5: pin 'checkout' must name actions/checkout, but this ref is 'evil/malware@abc123'grep -rn "throw \|makeOrThrow\|orThrow" modules/*/src/mainstill returns nothing.Breaking
zipxActions := ActionPins.Defaults.copy(setupSbt = "sbt/setup-sbt@<sha>")becomessetupSbt = ActionRef("sbt/setup-sbt@<sha>"). That is the point: the ref is now checked whilebuild.sbtcompiles, andActionRef.makereturns anEitherfor a ref a build computes. Mechanical, the compiler names every site, pre-1.0. Documented on the Action pins page, which also gains a section on what a malformed pin file reports.