Repository navigation
fix(research_extensions): read on: brackets outside comments and quotes - #1039
Conversation
…otes `Hypatia.WorkflowYaml.nesting_at/2` counts every bracket it is given, so callers must pass text with comments and quoted scalars blanked (its doc says so since #1027). RE001's trigger reader passed raw text in three places. Each place had a failure mode: - Continuation lines of a multi-line `on: [...]`: only whole-line comments are dropped before the rule reads a workflow, so a trailing comment reached both the bracket count and the event scan. - `issues] # was pull_request_target` named a trigger that is not there. - `] # see [ops notes` left a bracket open, so the reader swallowed the rest of the file and took words from `run:` lines as events. - Inline `on: {...}` maps: a bracket inside a quoted scalar (`tags: ["v1.0]"]`) shifted the depth, and a real top-level event key was dropped. - Flow `with:` mappings on a step: they had the same quoted-bracket miscount when deciding whether the mapping continues. The fix strips each continuation line's comment, and reads the depth through `open_flow?/1`, which blanks quoted scalars first (`strip_yaml_comment/2`, offsets preserved). Tests: - 4 new RE001 tests: a positive control plus 3 decoys. The 3 decoys fail on main; the control passes on both. - research_extensions_test: 122/122. It compiles with --warnings-as-errors, and `mix format --check-formatted` is clean. - RE001 findings on hypatia, standards, echidna, gitbot-fleet and mathesis are identical before and after (18 findings). 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 20 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. |
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. |
![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: be443dc386
ℹ️ 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 found two cases the line-by-line comment stripping missed:
- A quoted scalar spanning lines lost its quote state at the line break,
so `foo #hi"}}}, issue_comment: {}}` was cut at `#`. The closing quote,
the braces and the issue_comment event went with it.
- An apostrophe inside a plain scalar (`description: Say 'hi`) opened a
quote, blanked the structure after it, and hid a later top-level event.
flow_text/2 replaces strip_yaml_comment/2 for flow values. It scans the
whole value, carrying quote state across newlines. A quote opens a scalar
only where a flow node starts, at the start or after `{`, `[`, `,` or `:`.
A ` #` outside quotes starts a comment that ends at its own line. Byte
offsets are preserved, so the depth and key positions line up as before.
Two new tests reproduce the findings and fail on be443dc; 124/124 pass.
`mix format` is clean and the code compiles with --warnings-as-errors.
RE001 findings on hypatia, standards, echidna, gitbot-fleet and mathesis
are unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72d632a64b
ℹ️ 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".
| :plain when c in [?', ?"] and sig in [nil, ?{, ?[, ?,, ?:] -> | ||
| flow_scan(rest, b, {:quote, c}, c, c, [c | acc]) |
There was a problem hiding this comment.
When an inline on: map anchors a quoted node, such as description: &text "[draft", sig contains the final anchor-name byte, so this condition does not recognize the opening quote. The bracket intended as scalar text is then counted as flow syntax, causing a later top-level key such as issue_comment to be evaluated at the wrong depth and omitted from workflow_events; RE001 consequently misses the untrusted-trigger exposure for credential-bearing jobs. Account for anchor and tag properties when determining whether a quoted scalar starts here.
Useful? React with 👍 / 👎.
|
Add Carrot credits or activate Agent usage billing to use Autofix |
|
Summary
This is the follow-up named in #1027.
Hypatia.WorkflowYaml.nesting_at/2counts 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:on: [...]workflow_dispatch] # was pull_request_targetpull_request_targettrigger becomes an untrusted-input exposure.workflow_dispatch] # see [ops notesrun: ./issue_comment.shline then becomes anissue_commenttrigger.on: {...}map{push: {tags: ["v1.0]"]}, issue_comment: …}issue_commentis dropped.with: {...}]or[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
strip_yaml_comment/1before it is appended. Theon:line uses the same function instead of a\s+#regex.open_flow?/1reads the depth after blanking quoted scalars withstrip_yaml_comment(_, true). Byte offsets are preserved. Both continuation readers use it: theon:reader and thewith:reader.inline_events/1takes key names from the raw map but the depth from its blanked copy.Verification
Four new RE001 tests:
issue_commenton a continuation line with a balanced[bot]comment is still an exposure;On
mainthe three decoys fail and the control passes. On this branch all four pass.research_extensions_test: 122/122. Compiled standalone withelixirc --warnings-as-errorsbecause hex.pm is unreachable here.mix format --check-formattedis clean.RE001 findings over five real trees, before and after, are identical: 18 findings across hypatia, standards, echidna, gitbot-fleet and mathesis.
Aspect notes
nesting_at/2at all. In Elixir that is a tagged tuple or a struct at theWorkflowYamlboundary. 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