Repository navigation
RE001: read workflows as YAML does (quoted keys, aliases, with: inputs) - #975
Conversation
Codex review of 6f57606: - A declared job that reads secrets by a computed key is reported even when harden-runner is installed: a fixed :secrets list cannot describe it, and declarations are attestations whether or not the job is hardened. The fix text says the entry can simply be removed. - Quoted keys are the same keys. One matcher, key_line?/2, now serves the top-level env: and jobs:, and the job's env:, runs-on:, steps: and if:. A quoted "runs-on" used to drop the whole job from RE001, and a quoted top-level "env" hid inherited secrets from drift checks. The exposure model's uses:/ref:/repository:/run: keys get the same. - The harden-runner policy is read from the step's with: inputs (block or flow mapping), with YAML comments stripped, so `uses: ...@v2 # egress-policy: block` no longer attests block. - YAML aliases are expanded before jobs are read: `env: *common`, `<<: *defaults` and `deploy: *build` bring the anchored lines into the job, at the alias's line number. Expansion is limited to jobs and the top-level env:, within a 5,000-line budget per workflow, so a billion-laughs file costs a bounded amount of work. Also takes CodeRabbit's nit: deed_test.exs uses ExUnit's per-test tmp_dir instead of hand-made directories under System.tmp_dir!(). research_extensions_test + deed_test: 161 tests, 0 failures; the 9 new or extended tests fail against 6f57606. kyaml_workflow_test 10/10. RE001 on real trees is unchanged: hypatia 9, standards 15, rsr-template-repo 0, choreographic-types 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J4QvDithtn5Xk5MbSPm4Px
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 21 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis 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. |
![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: c97ceb90a0
ℹ️ 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".
Codex review of c97ceb9: - A `<<:` merge no longer overrides a key the destination mapping sets itself. YAML gives the explicit key precedence, and the anchored entries used to land first: a job's own unhardened steps: could be read as the anchor's hardened ones, suppressing RE001. - Anchors and aliases are recognised only where a YAML node starts, outside quoted scalars and block-scalar text, and a `,` counts only inside a flow collection. `echo "x *common"`, `echo x *common` and a `run: |` line `ls *common` stay text. - An alias that is a key's whole value is replaced by its anchor's inline content, so `run: *script` reads as `run: |` with the script under it and step_run_lines sees the commands. `run: &script |` was also misread (the anchor hid the block header) and is fixed in step_run_lines and block_scalar?. research_extensions_test + deed_test: 164 tests, 0 failures; the 3 new tests fail against c97ceb9. kyaml_workflow_test 10/10. RE001 on real trees is unchanged: hypatia 9, standards 15, rsr-template-repo 0, choreographic-types 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J4QvDithtn5Xk5MbSPm4Px
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05fc9a4cf8
ℹ️ 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".
| indent = indent_of(code) | ||
| inline_lines = if inline == "", do: [], else: [{indent_spaces(indent + 2) <> inline, n}] | ||
| {copy, budget} = anchored_block(inline, inline_lines ++ block, anchors, budget) | ||
| {renumber(reindent(copy, column(code, at - 1)), n), budget} |
There was a problem hiding this comment.
When a job uses the valid merge-sequence form <<: [*defaults], this branch treats the alias as an ordinary flow alias and reindents the copied mapping at the alias character's column. Inherited fields such as runs-on, env, and steps therefore remain too deeply indented for the job parser; a cloned secret-bearing job can be dropped by direct_job_field?/2 and receive no RE001 finding. Expand aliases under <<: [...] as mapping entries at the merge key's column, including normal merge precedence.
Useful? React with 👍 / 👎.
| defp step_inputs(step) do | ||
| lines = Enum.map(step, fn {line, _} -> strip_yaml_comment(line) end) | ||
|
|
||
| case Enum.find_index(lines, &Regex.match?(@with_key, &1)) do |
There was a problem hiding this comment.
When a harden-runner step has another block-scalar property whose text contains with: and egress-policy: block—for example a multiline name: |—this unrestricted search parses that scalar text as the action's inputs. A hardener that actually uses its default audit policy is then recorded as blocking, so a deed claiming harden-runner-block incorrectly passes without a finding. Limit the search to direct step properties and skip block-scalar bodies.
Useful? React with 👍 / 👎.
| [_ | captures] <- [Regex.run(@block_input, line)], | ||
| into: %{} do | ||
| case captures do | ||
| [name, value] -> {name, scalar_value(value)} |
There was a problem hiding this comment.
When egress-policy is written as a valid YAML block scalar, such as egress-policy: >- followed by block, this path captures only the >- header and records that as the policy. GitHub passes the folded value block to harden-runner, but RE001 reports that a truthful harden-runner-block declaration has drifted. Consume and fold the scalar body before comparing the input value.
Useful? React with 👍 / 👎.
Summary
Follow-up to #964, which merged while these fixes were in progress. It fixes the four findings Codex left on #964's last commit (
6f57606), and the three it left on this PR's first commit (c97ceb9). Each one let RE001 report less than it should, report what isn't there, or let a deed claim more than is true.Changes
Computed key on a hardened, declared job. The computed-key finding was skipped when the job installs harden-runner, and the drift check was skipped too, so the deed's fixed
:secretslist stood unchallenged. A declaration is an attestation whether or not the job is hardened, so the finding now fires (:warnfor a hardened job). Its fix text says the entry can simply be removed.Quoted keys. YAML reads
env:,"env":and'env':as one key. A newkey_line?/2matcher now covers:env:andjobs:;env:,runs-on:,steps:andif:.Before this, a quoted
"runs-on":dropped the whole job from RE001, and a quoted top-level"env":hid inherited secrets from the drift check. The exposure model'suses:,ref:,repository:andrun:keys get the same treatment.harden-runner policy. RE001 now reads
egress-policyfrom the step's actualwith:inputs (block or flow mapping), with YAML comments stripped.uses: …@v2 # egress-policy: blockused to attestblock. It now runs the default, audit.YAML aliases, expanded by node. Aliases are expanded before jobs are read, and every line they bring carries the alias's line number:
env: *common,run: *script,- *step,deploy: *build): replaced by its anchor's inline content (a scalar, a block scalar's|, a flow collection), with the anchor's indented lines under it.<<:merge: puts the anchored entries into the destination mapping, minus any key that mapping sets itself, since YAML gives the explicit key precedence.,counts only inside a flow collection.jobs:and the top-levelenv:are expanded in place, and copying stops after 5,000 lines per workflow, so a billion-laughs file costs bounded work.run: &script |also used to hide its block header fromstep_run_lines, and is fixed.It also takes CodeRabbit's nit from #964:
test/deed_test.exsuses ExUnit's per-testtmp_dir.Testing
research_extensions_test+deed_test: 164 tests, 0 failures. Every new or extended test fails against the commit before its fix: 9 against6f57606, 3 againstc97ceb9.kyaml_workflow_test: 10/10.mix format --check-formattedis clean.research_extensions_wiring_testruns first in CI.Still open (#971, a parsed workflow model): a job written as a flow mapping is not read, and a merge whose anchor is a flow mapping (
&d {runs-on: x}) is not looked into.🤖 Generated with Claude Code
https://claude.ai/code/session_01J4QvDithtn5Xk5MbSPm4Px