Repository navigation
docs(workflow_yaml) + test(workflow_hardening): document the reader; clear reused temp repos - #1027
Conversation
Hypatia.WorkflowYaml became a public module in #1005, but most of its functions still carried their documentation as `#` comments from their time as private RE001 helpers, and two were `@doc false`. CodeRabbit's docstring-coverage check on #1005 flagged it. Every public function now has an @doc: the existing comment, moved verbatim where there was one (step_key_column, job_steps, key_line?, lines_in_steps, step_indent, strip_yaml_comment, workflow_job_lines, read_workflow, flow_to_block), and a short new one where there was none (expand_aliases, nesting_at, direct_job_field?, job_field_indent, indent_of). No code changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM
|
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 29 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. |
WorkflowHardeningTest names its temporary repositories `wh_test_<unique_integer>` and `wh_empty_<unique_integer>`. System.unique_integer/1 restarts with every VM, so a later run reuses the names an earlier run used. A run that crashed before its cleanup (for example, one whose modules failed to compile) leaves its workflows behind, and the next run scans them alongside its own. A stale pull_request_target workflow then trips WH001 in unrelated tests. Clear the directory before writing, as ResearchExtensionsTest's helper already does. With 2,000 stale directories planted under TMPDIR, the suite fails 24 of 104 on main and 0 of 104 with this change; it also passes on a clean TMPDIR. CI is unaffected (fresh runners); this guards local re-runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM
hyperpolymath
changed the title
Codex Review Summary: ❌ FailedThis 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. |
|
This security review couldn’t start because the paying account has exhausted its Codex allowance and has insufficient available credits. The payer can check their balance and add credits in the Codex usage dashboard, then try again. If you do not manage the paying account, contact this repository's admins. |
![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: ae522d3bca
ℹ️ 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".
…esting docs; keep concurrent VMs apart Codex review on #1027 raised three points: - expand_aliases/1: the new @doc said every alias is replaced in place. The code expands only within the top-level jobs: and env: sections, so the doc now says so, and says an alias elsewhere is returned unchanged. - nesting_at/2: it counts every bracket character, including those in quoted scalars and comments. The doc now says callers pass text with those already blanked. - wh_test_* and wh_empty_* fixtures: unique_integer/1 is unique only within one VM, so two concurrent `mix test` runs on one TMPDIR could pick the same name, and the new rm_rf! could delete the other run's live fixture. The OS pid is now part of the name. The rm_rf! still clears what a crashed run with a reused pid left behind. RE + WH suites: 222/222, twice. workflow_yaml.ex compiles with --warnings-as-errors. mix format is clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM
|
This security review couldn’t start because the paying account has exhausted its Codex allowance and has insufficient available credits. The payer can check their balance and add credits in the Codex usage dashboard, then try again. If you do not manage the paying account, contact this repository's admins. |
…otes (#1039) ## Summary This is the follow-up named in #1027. `Hypatia.WorkflowYaml.nesting_at/2` counts every bracket it is given. Callers must therefore pass text with comments and quoted scalars blanked, as its doc now says. RE001's trigger reader passed raw text in three places: | Where | Input | Effect on main | |---|---|---| | Continuation line of a multi-line `on: [...]` | `workflow_dispatch] # was pull_request_target` | A phantom `pull_request_target` trigger becomes an untrusted-input **exposure**. | | Same | `workflow_dispatch] # see [ops notes` | The bracket stays open and the reader swallows the rest of the file. A `run: ./issue_comment.sh` line then becomes an `issue_comment` trigger. | | Inline `on: {...}` map | `{push: {tags: ["v1.0]"]}, issue_comment: …}` | The depth is shifted, so the real top-level `issue_comment` is **dropped**. | | A step's flow `with: {...}` | a quoted `]` or `[` | Same miscount when deciding whether the mapping continues. | Only whole-line comments are dropped before a rule reads a workflow (`active_lines/1`). Trailing comments on continuation lines therefore reached both the bracket count and the event-word scan. ## Fix - Each continuation line is passed through `strip_yaml_comment/1` before it is appended. The `on:` line uses the same function instead of a `\s+#` regex. - A new `open_flow?/1` reads the depth after blanking quoted scalars with `strip_yaml_comment(_, true)`. Byte offsets are preserved. Both continuation readers use it: the `on:` reader and the `with:` reader. - `inline_events/1` takes key names from the raw map but the depth from its blanked copy. ## Verification - Four new RE001 tests: - a positive control: an `issue_comment` on a continuation line with a balanced `[bot]` comment is still an exposure; - three decoys from the table above. On `main` the three decoys **fail** and the control passes. On this branch all four pass. - `research_extensions_test`: **122/122**. Compiled standalone with `elixirc --warnings-as-errors` because hex.pm is unreachable here. `mix format --check-formatted` is clean. - RE001 findings over five real trees, before and after, are **identical**: 18 findings across hypatia, standards, echidna, gitbot-fleet and mathesis. ## Aspect notes - **Type/proof route:** the cleaner fix is a type distinction between raw line text and "code" text. A caller could then not hand raw text to `nesting_at/2` at all. In Elixir that is a tagged tuple or a struct at the `WorkflowYaml` boundary. It touches every caller of the shared reader (WH and RE rules), so it is better as its own change than folded into this fix. --- --- _Generated by [Claude Code](https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
|
This security review couldn’t start because the paying account has exhausted its Codex allowance and has insufficient available credits. The payer can check their balance and add credits in the Codex usage dashboard, then try again. If you do not manage the paying account, contact this repository's admins. |
|
There was a problem hiding this comment.
💡 Codex Review
hypatia/lib/hypatia/workflow_yaml.ex
Lines 31 to 33 in a63c7d1
Scope the alias guarantee in active_lines/1
When a workflow contains an alias under on:, permissions:, or another section outside top-level jobs:/env:, this function returns that alias unchanged because expand_aliases/1 only expands those two sections. The follow-up scoped the separate expand_aliases/1 documentation, but this public wrapper still promises aliases are expanded without that qualification, so callers can rely on behavior it does not provide; give this contract the same section restriction.
ℹ️ 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".
| as GitHub does, within what jobs read: the top-level `jobs:` and `env:` | ||
| sections. There each `*name` is replaced in place by the node anchored | ||
| `&name` (the nearest one above, wherever it is defined), a `<<:` merge adds | ||
| the anchored entries the mapping does not set itself, and every line an |
There was a problem hiding this comment.
When an alias appears inside a flow collection, the implementation takes the value == nil branch and returns the original alias-containing line followed by copied anchor lines rather than replacing *name in place. The new public contract therefore misdescribes the returned representation for inputs such as env: [*common]; either document this append-only flow behavior, as the existing internal comment does, or actually replace the alias token.
Useful? React with 👍 / 👎.
| `&name` (the nearest one above, wherever it is defined), a `<<:` merge adds | ||
| the anchored entries the mapping does not set itself, and every line an |
There was a problem hiding this comment.
When callers pass <<: *defaults, this reader expands the merge even though GitHub Actions rejects YAML merge keys: an Actions runner collaborator explicitly confirmed they are unsupported, while GitHub's documentation only describes whole-node aliases (runner issue, GitHub documentation). Describing the merge behavior as what GitHub does can cause consumers to treat an invalid workflow as executable; document it as a reader extension or stop expanding merge keys.
Useful? React with 👍 / 👎.
| @doc """ | ||
| A line without its YAML comment: a `#` at the start or after whitespace, | ||
| outside a quoted scalar. A quote opens a scalar only where one can start | ||
| (after whitespace or a flow indicator), so the apostrophe in `it's` does not. |
There was a problem hiding this comment.
For compact JSON-style YAML such as {"k":"x # y"}, the value's opening quote follows :, but the implementation only recognizes quotes after whitespace, {, [, or ,. It consequently treats the # inside the quoted value as a comment and returns the truncated string {"k":"x, contrary to this newly public contract; recognize : as a valid quoted-scalar start or document that compact flow mappings are unsupported.
Useful? React with 👍 / 👎.
| Read a workflow for the line- and indent-based rules. A KYAML workflow | ||
| (YAML-POLICY Y-3: the estate's base form, `yq -o kyaml`) is rewritten to | ||
| the equivalent block layout one line for one line, so physical line | ||
| numbers in findings still point at the source. Block YAML is returned | ||
| byte-identical. |
There was a problem hiding this comment.
When KYAML contains a string such as FLAG: "true",, unquote_scalar/1 emits FLAG: true, changing the value from a string to a boolean; similarly, SCRIPT: "echo # literal", becomes SCRIPT: echo # literal, exposing part of the value as a YAML comment. The result is therefore not an equivalent block layout as the public contract promises, and downstream line-based rules can inspect different content from GitHub; retain quotes unless removing them is YAML-semantics-preserving.
Useful? React with 👍 / 👎.
| Anchors and aliases count only where a YAML node starts, never inside | ||
| quoted scalars or block-scalar text. Expansion stops | ||
| after #{@alias_budget} copied lines, so hostile input costs bounded work. |
There was a problem hiding this comment.
When the first alias references an anchor containing more than 5,000 block lines, anchored_block/3 subtracts the entire block length before invoking the private expander, but that expander still returns every line when its budget is non-positive. Thus a single alias can copy an arbitrarily large block despite this contract promising expansion stops after 5,000 copied lines; cap the copied block itself rather than only suppressing further nested aliases.
Useful? React with 👍 / 👎.
Summary
Two small, independent commits that touch the same module family, plus a follow-up for Codex's review. I meant the first two as two stacked PRs, but they landed on one branch. Each commit stands alone and can be reviewed on its own.
1.
docs(workflow_yaml): document the shared reader's public API (17731bb)Adds an
@docto every public function ofHypatia.WorkflowYaml. The module became public in #1005, but most of its functions still carried their docs as#comments from their time as private RE001 helpers, andread_workflow/1andflow_to_block/1were@doc false. CodeRabbit's docstring-coverage pre-merge check on #1005 flagged this.@docforstep_key_column,job_steps,key_line?,lines_in_steps,step_indent,strip_yaml_comment,workflow_job_lines,read_workflowandflow_to_block.expand_aliases,nesting_at,direct_job_field?,job_field_indentandindent_of.No code changes.
2.
test(workflow_hardening): clear reused temp repos before writing (ae522d3)WorkflowHardeningTestreuses temporary repository paths across runs without clearing them.wh_test_<unique_integer>, and the empty-repo test useswh_empty_<unique_integer>.System.unique_integer/1restarts in every VM, so a later run reuses the same names.pull_request_targetworkflow then trips WH001 in unrelated tests. I hit this as a single WH001 failure that wouldn't reproduce.The fix clears the directory before writing, as
ResearchExtensionsTest's helper already does.3. Codex review follow-up (
fbbb573)expand_aliases/1doc: it now gives the real scope: only the top-leveljobs:andenv:sections. An alias elsewhere is returned unchanged.nesting_at/2doc: it now says every bracket counts, so callers pass text with quoted scalars and comments already blanked.wh_test_*andwh_empty_*now carrySystem.pid(), so two concurrentmix testruns on oneTMPDIRnever share a path that the newrm_rf!would clear.Type of change
How has this been verified?
mainworkflow_hardening_testwith 2,000 stalewh_test_*+wh_empty_*dirs planted underTMPDIRTMPDIRresearch_extensions_test+workflow_hardening_testfbbb573: twice more)Code.fetch_docs(Hypatia.WorkflowYaml)shows a doc for every public function.strip_yaml_comment/1shares the/2entry, as Elixir does for default arguments.workflow_yaml.excompiles withelixirc --warnings-as-errors, standalone Elixir 1.14 (hex.pm is unreachable here).mix format --check-formattedis clean.Follow-up (not in this PR)
ResearchExtensionspasses rawon:flow text tonesting_at/2when it reads anon:collection that spans several lines. That is a pre-existing caller of the contract now documented above.Checklist
https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM