Repository navigation
test(ci): make the workflow spec read quoted settings and fail on a step it cannot read - #591
Merged
Merged
Conversation
…tep it cannot read
The spec read a key only when it was written bare and a value only when it was
written without quotes. A step written with a quoted key (`"uses": actions/cache@...`)
was not seen, and a quoted value (`cache_key: "..."`) failed the spec although
YAML reads it the same. field() now accepts a quote around the key and strips
one pair of quotes from the value.
Three more checks:
- Every jdx/mise-action step must turn its cache off or save only on a push. A
second step with the default settings would save the tool cache on a pull
request.
- The rock restore key must be luarocks-${{ runner.os }}-${{ hashFiles('mise.toml') }}
as a whole, with an optional version after the prefix, as the mise key is.
- The spec fails when actions/cache or jdx/mise-action is named on a line that is
not the uses: of a step it reads. That covers a flow-style step
(`- { uses: actions/cache@..., with: { ... } }`) and an upper-case spelling.
_Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…rkflow spec checks
The line count cut each line at its first " #" as a trailing comment. A " #" can sit
inside a quoted value of a flow-style step, such as { name: "Cache #2", uses:
actions/cache@... }, so the cut hid the name and the step passed. The count now
keeps every line that is not a comment line as it is. A trailing comment that
names an action on another line now fails the count too.
The spec comments now say what the spec does: it fails when actions/cache or
jdx/mise-action is named on a line that is not the uses: of a block-style step,
and it does not decode YAML escapes.
_Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
_Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: d9ae8f78e1
ℹ️ 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".
uses_action matched any value that began with the action name, so a similarly prefixed repository (jdx/mise-action-fork@...) counted as jdx/mise-action. The line-count guard and the "at least one mise-action step" check then passed with no real jdx/mise-action step left. The value must now continue with @ or /. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 14, 2026
…ey (coder#596) ## Why coder#591 made `field()` in the workflow spec accept a quote around a key. Its pattern `^ *["']?([%w_%-]+)["']?:` also accepts a quote on one side of the key only. YAML reads `cache_save": ...` as the key `cache_save"`, so mise-action ignores it and keeps its default `true`, which saves the tool cache on a pull request. The spec read the same line as `cache_save` and passed. The same gap hides a second `jdx/mise-action` step with `cache': false`, a `cache_key": ...` and a `MISE_HTTP_RETRIES": "6"`. `actionlint` and `zizmor` pass all four. A second gap is older and main has it too. A second `jdx/mise-action` step with `with:` and then `env :` or `"env" :` holding `cache: false` passes the spec. No pattern read the `env` line, so the spec still thought it was inside `with:` and counted that `cache` as an input. YAML puts it under `env`, so the action keeps its defaults and saves its cache on a pull request. `actionlint` and `zizmor` pass it. ## What changed Only `tests/unit/workflow_cache_spec.lua` (+99, -16). The workflows are unchanged. The spec still has no YAML parser. 1. `field()` accepts a key written bare, as `"key":` or as `'key':`. A quote on one side only is not a key. 2. Any non-blank line at the step-key indent now sets the current block, even when no pattern reads its key. A line such as `env :` ends the `with:` block. 3. Four new tests check how the spec reads keys and blocks, so that a full revert of these changes makes `mise run test` fail. A partial change to the quoted-key patterns can still pass (a follow-up PR adds a test for it). `parse_workflow` now takes an iterator over lines, and the tests feed it small workflow texts as inline strings. They assert that an `env :` or `"env" :` line after `with:` ends the block, that a line of spaces does not end it, that `cache': false` and `cache_save":` are not read as keys, and that `"key":`, `'key':` and a bare key are read. 4. The comments say what the spec does. The header says that full-line comments are skipped, so a comment can name an action (the one above the rock cache steps does). The comment of the line-count test says that `@` or `/` must follow the action name in `uses:`. The comment above the block line says that a non-blank line opens a new block and a line of spaces only does not. The reader reads no new YAML form. Aliases, YAML escapes and expression spellings stay under "Not covered" in coder#591, with their reason. ## Evidence Before is the spec of coder#591 (12 tests, merged as `b44dab46`), after is this branch (16 tests). Each mutant is the workflow of coder#591 with one edit that applies exactly once. | Mutant | Edit | Before | After | | --- | --- | --- | --- | | `SQ1` | the setup step has `cache_save": ${{ github.event_name == 'push' }}` | green | red (2 tests) | | `SQ2` | a second `jdx/mise-action` step has `cache': false` | green | red (1 test) | | `SQ3` | the setup step has `cache_key": ...` | green | red (1 test) | | `SQ4` | the setup step has `MISE_HTTP_RETRIES": "6"` | green | red (1 test) | | `SQ7` | `"cache_save: ...` (a quote on the left only) | green | red (2 tests) | | `SQ5`, `SQ6` | controls: `"cache_save":` and `'cache_save':` | green | green | | `K30` | a second `jdx/mise-action` step with `with:`, then `env :` and `cache: false` under it | green | red (1 test) | | `K31` | the same with `"env" :` | green | red (1 test) | | `K32` | the same with `env":` (a quote on one side only) | red | red (1 test) | | `K33` | control: the same step with `"env":` right after `with:` | red | red | The failing tests are the ones that read the key: the `cache_save`, `cache_key` and `MISE_HTTP_RETRIES` tests of the setup step, and the test that every `jdx/mise-action` step keeps its cache off a pull request. Counts over all 112 mutants (the 102 of coder#591, `SQ1` to `SQ7` and `K30` to `K32`). Before, 74 are red and 38 are green. After, 81 are red and 31 are green. 74 stay red, 7 turn red and 31 stay green. `K32` is red with coder#591's spec, which read `env":` as the key `env`. The first commit of this PR made it pass, and `36feb3b7` makes it red again. The 102 mutants of coder#591 give the same result as before, so no test result changed for them. The 31 green mutants are the 29 controls and known gaps of coder#591 and the two controls `SQ5` and `SQ6`. `SQ7` is not valid YAML, and `actionlint` and `zizmor` reject it. They pass `SQ1` to `SQ4`, `K30` and `K31`. actionlint rejects `K32`, and zizmor passes it. I also ran all 169 workflows of the verifier's table (the 112 mutants, the other probes and the base trees) with the spec of `b558d22d` and of `36feb3b7`. 111 stay red, 3 turn red (`K30`, `K31`, `K32`) and 55 stay green. No workflow turns green, and no failing test changed for the red ones. I ran the 169 workflows again with the four new tests. 114 are red and 55 are green, and the result and the failing test names are the same for all 169. The four new tests are checked against changes of the reader itself. Each change is one edit of the spec, and the spec runs on the workflow of main. Before is the spec without the new tests, after is the spec with them. | Change of the reader | Before | After (failing test) | | --- | --- | --- | | `S1`: the block line back to `if name and indent == 8` (the code of coder#591) | green | red (the `env :` test) | | `S2`: the block line without its `%S` check | green | red (the line of spaces test) | | `S3`: the key pattern of coder#591, which takes a quote on one side | green | red (the one-sided quote test) | | `S4`, `S5`: no pattern for `"key":`, or none for `'key':` | green | red (the quoted keys test) | | `S7`: the loop over the key patterns without its `break` | green | red (the quoted keys test) | | `S8`: the bare pattern takes a trailing quote | green | red (the one-sided quote test) | | `S9`: the `"key":` pattern takes the left quote as optional | green | red (the one-sided quote test) | | `S10`: the block of an entry is not compared | green | red (the `env :` test) | | `S6`: no bare pattern | red | red | Each of the 10 changes fails exactly one new test, except `S6`, which fails 15. Without the new tests, only `S6` fails. | Claim | Evidence | | --- | --- | | The spec passes on main with this change | 16 successes, 0 failures | | The workflows are valid | The workflows are identical to main. `actionlint` 1.7.12 on all 5: exit 0. `zizmor` 1.25.2 `--offline` on all 5: no findings (25 suppressed). `mise run format-check`: exit 0 (444 files, 0 changed) | | Gate | `mise run clean && mise run all` exits 0 in 495 s: 1908 successes, 0 failures, 0 errors, 36 pending (seed 1794545933). It ran on the merge of main (`bcf1f084`) into this branch. `luacheck`: 0 warnings, 0 errors in 200 files. The stub `claude` and GNU `timeout` first on `PATH` | --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
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.
Why
#579 added a spec that reads
.github/workflows/test.ymlas text and checks the cache settings. The independent reviews of its heads found gaps in how the spec reads the file. Each gap lets a risky edit pass or fails a harmless one."uses": actions/cache@...) was not seen, so a second step that caches.luarockspassed.jdx/mise-actionstep with the default cache settings would save the tool cache on a pull request. Nothing failed.hashFiles('mise.toml'). Text that changes on each run (-${{ github.sha }}) kept the spec green and the cache would never hit.cache_key, a quoted restore path, a quoteduses:.- { uses: actions/cache@..., with: { path: .luarocks, key: ... } }) was not seen.What changed
Only
tests/unit/workflow_cache_spec.lua(+98, -19). The workflows are unchanged. The spec still has no YAML parser.field()accepts a quote around the key ("uses":or'uses':).jdx/mise-actionstep turns its cache off (cache: falseorcache_save: false) or saves only on a push.luarocks-${{ runner.os }}-${{ hashFiles('mise.toml') }}as a whole. A version after the prefix (luarocks-v2-) is allowed, asv1is in the mise key.field()strips one pair of YAML quotes from a value, and reads''in a single-quoted value as'.actions/cacheorjdx/mise-action(a trailing comment stays in the line), and the steps that have the name as theiruses:. The two counts must be equal. So the spec fails for a flow-style step or list of steps, even with a#inside a quoted value, and for the name on any other line (a stepname:, arun:line, a trailing comment). The line count ignores the letter case and theuses:match does not, so an upper-case spelling such asActions/cache@...fails too. The one exception is a name written with a YAML escape ("actions\x2fcache@..."), which the spec does not decode (see Not covered).@or/after the action name. A similarly prefixed repository such asjdx/mise-action-fork@...no longer counts asjdx/mise-action, so it cannot satisfy the line count or the check that ajdx/mise-actionstep exists.Evidence
Before is the spec of #579 (10 tests, merged as
b450eb91), after is this branch (12 tests). Each mutant is the workflow of #579 with one edit that applies exactly once.m,L,XandYare the mutants of #579,Z1toZ12are the probes of its last review,N1toN20are mine (there is noN6),P1toP26are the probes of the review of this PR (the ones in the tables below), andQ1toQ4come from a Codex finding on the second head.Z9andN5: a secondactions/cache@step for.luarockswith a quoted key"uses":or'uses':Y3: a secondjdx/mise-actionstep with the default cache settings.N10: it setscache_save: true.N11: it setscache_saveto!= 'pull_request'N7(cache: false),N8(cache_save: false),N9(the push-only expression in quotes)Y4:-${{ github.sha }}on the rock key.N14: an extra segment in itN12(the key in quotes),N13(luarocks-v2-)Z1(quotedcache_key),Y5(quoted restore path),Z7(quoteduses:on the restore step),N15(quotedcache_save),N16(quoted saveif:),N17(single-quoted mise key with''),N19(quoted save path),N20(quoted step name)N18(MISE_HTTP_RETRIES: '6')N1: a flow-style second cache step for.luarocks.N2: the same for another path, which fails closed on purpose.N3: a flow-style secondjdx/mise-actionstep.Z6:Actions/cache@P23andP25: a flow-style second cache step for.luarockswith a#inside a quoted value.P24: the same for a secondjdx/mise-actionstep.P26: the same step asP23without the#Q1: the setup step usesjdx/mise-action-fork@....Q2: a stepactions/cache-extra@...for another path.Q3: the same step for.luarocksQ1)Q4: the setup step usesjdx/mise-action/sub@...N4: a flow-stylewith:on the rock restore. It fails the path and key tests, as it did before, and not the new testP3(a trailing comment that namesactions/cacheon arun:line),P4(the name in a stepname:),P5(the name in arun:line),P13(cache: Falseon a secondjdx/mise-actionstep)Counts over all 102 mutants (the 86 of the first head,
P1,P1b,P2,P3,P4,P5,P13,P14,P23toP26, andQ1toQ4). Before, 59 are red and 43 are green. After, 73 are red and 29 are green. 51 stay red, 22 turn red, 8 turn green and 21 stay green. Of the 29 green mutants, 8 are harmless edits that used to fail, 13 are controls (m12,X8,X19,X23,Z2,Z3,N7,N8,N9,N12,N13,N18,Q4), and 8 are gaps (X3,X10,Z10,Z11, and the four YAML escapesP1,P1b,P2,P14, see below). Five of the 51 red mutants (m2,m3,X1,X11,X14) and the tree before #560 now also fail the newjdx/mise-actiontest next to the test that failed before. No other test result changed.b450eb91) with this changeb43d1c23,c7e2eb4e)actionlint1.7.12 on all 5: exit 0.zizmor1.25.2--offlineon all 5: no findings (25 suppressed). On the mutants,actionlintexits non-zero only onL8,L9andX3, andzizmoronly onL10andL11, as for #579.mise run format-check: exit 0 (441 files, 0 changed)mise run clean && mise run allexits 0 in 540 s: 1904 successes, 0 failures, 0 errors, 35 pending (seed 1794532755). It ran on the merge of main (692b460c) into this branch.luacheck: 0 warnings, 0 errors in 198 files. The stubclaudeand GNUtimeoutfirst onPATHNot covered
.luarocks(a glob such as.luaroc*,Z10, or a parent folder,Z11). The save step is not checked for its place in the job, so moving it after the tests keeps the spec green (X10). A secondcache_save: truekey is caught byactionlintand not by the spec (X3).N2), the name of an action in a stepname:(P4), in arun:line (P5) or in a trailing comment (P3), andcache: Falseon a secondjdx/mise-actionstep (P13). The action readsFalseas false and the spec compares the textfalse. A flow-stylewith:on a step the spec reads fails the path and key tests (N4), as it did before."actions\x2fcache@...","actions\u002fcache@...","jdx\x2fmise-action@...", or a path"\x2eluarocks") is not decoded, so the spec does not see it (P1,P1b,P2,P14). The spec guards against accidental edits, and an escape spelling is not an accident, so it has no reader for it. A double-quoteduses:split by an escaped line break ("actions/\and thencache@...") falls in the same class.uses:, as the runner log inDownload action repositorycalled repeatedly for actions that have to be the same actions/runner#3731 shows. SoActions/cache@...is a real cache step, and the line-count test rejects it. The spec does not read it as a rock-cache step.with: *rockson a secondactions/cachestep, where the restore step'swith:has the anchor&rocks) or from an expression such as${{ format(...) }}is not seen. GitHub supports anchors and aliases. An alias is a deliberate structure, like an escape, not an accidental edit, so the spec has no reader for it.field()also accepts a quote on one side of a key (cache_save": ...,cache': false). YAML reads that as another key (cache_save"), so the action ignores it and uses its default (true). The spec reads it ascache_save, so a Set up mise step written this way passes this spec (the spec on main fails it), and so does a secondjdx/mise-actionstep withcache': false. actionlint 1.7.12 exits 0 on both. A follow-up PR requires a matching pair of quotes.git archive, because.gitattributesexport-ignores.github. The failure message says so.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh