chore: update IR docs#72
Open
Agustindeleon wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR updates CRSLang documentation: examples move Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
docs/adr/0004-structured-actions.md (1)
300-300: Inconsistent emphasis markup across the PR.This file changes
*outcome*to_outcome_, but other files in this PR change emphasis in the opposite direction (e.g.,docs/README.mdlines 134, 154, anddocs/adr/0007-phase-inference.mdline 249 change from_text_to*text*).Consider standardizing on a single emphasis style across all documentation files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0004-structured-actions.md` at line 300, The PR uses inconsistent emphasis styles (this file changed `*outcome*` to `_outcome_` while other docs changed `_text_` to `*text*`); pick the project's standard emphasis style and make all files consistent (e.g., change `_outcome_` in docs/adr/0004-structured-actions.md to `*outcome*` or convert the other changed files to `_text_`), and update instances referenced (`docs/README.md` lines ~134/154 and `docs/adr/0007-phase-inference.md` line ~249) and any other docs changed in this PR to match the chosen style.docs/adr/0003-boolean-algebra.md (1)
393-393: Minor: Consider consistent terminology.Line 393 says "Option B: pipeline function" but the previous text no longer discusses options labeled A, B, C, D in this section. The "Alternatives documented" list should clarify what Option A was (the chosen approach of metadata property).
Consider adding: "Option A (chosen): metadata property —
multi_match = truein metadata block."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0003-boolean-algebra.md` at line 393, Update the Alternatives documented list to use consistent option labels and explicitly state the chosen approach: add an entry like "Option A (chosen): metadata property — `multi_match = true` in metadata block" and keep the existing "Option B: pipeline function — `multi_match(lowercase, remove_whitespace)`" text so readers see both the chosen metadata-property solution and the pipeline-function alternative; ensure the label "Option A (chosen)" replaces any ambiguous references and that `multi_match = true` is shown exactly as the metadata syntax.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0001-typed-field-namespace.md`:
- Line 107: The ADR mixes list accessor names (`.names` vs `.keys`); pick one
canonical accessor (e.g., `.keys`) and update all references so they match,
specifically replace `request.headers.names` and `request.args.names` to
`request.headers.keys` and `request.args.keys` (or vice versa if you prefer
`.names`), and ensure the summary at Line 116 and any other occurrences use the
same chosen accessor throughout the document.
In `@docs/adr/0002-pipeline-operator.md`:
- Line 62: Update the fenced code blocks that currently use bare triple
backticks by adding a language identifier `text` or `plaintext` (do not use
`crslang`) for each example shown (e.g., the blocks containing "request.uri |>
matches("^/admin")", "count(tx.crs_setup_version) |> eq(0)", "client.ip |>
ip_in_range("10.0.0.0/8", "172.16.0.0/12")",
"matches(lowercase(url_decode(request.headers[\"User-Agent\"])), \"pattern\")",
"request.headers[\"User-Agent\"].urlDecode().lowercase().matches(\"pattern\")",
and "request.uri | lowercase | matches(\"pattern\")"); modify those same other
occurrences noted (lines referenced in the comment) so all fenced blocks use
`text`/`plaintext` and rerun markdownlint.
In `@docs/adr/0003-boolean-algebra.md`:
- Around line 67-70: The example rule declares the phase twice—once inline in
the rule header `rule 920170 (phase: request_headers)` and again inside the
`metadata { phase: request_headers }` block—so remove the inline phase parameter
and change the header to `rule 920170 {` to rely on the metadata block; update
the `rule 920170` declaration accordingly so only `metadata { phase:
request_headers }` specifies the phase.
In `@docs/adr/0007-phase-inference.md`:
- Around line 86-88: This ADR mixes two phase-declaration syntaxes — metadata
blocks like "metadata { phase = request_headers }" and inline rule declarations
like "rule XXXXX (phase: request_headers)", and another ADR
(docs/adr/0003-boolean-algebra.md) uses a different metadata separator ("phase:
request_headers"), so normalize to one canonical form across the document and
repo; pick the agreed canonical syntax (either metadata { phase =
request_headers } or metadata { phase: request_headers } or only inline "rule
NAME (phase: request_headers)"), then update all occurrences in this file
(replace lines with "metadata { phase = request_headers }" and inline "rule
XXXXX (phase: request_headers)" to the chosen format) and likewise align
docs/adr/0003-boolean-algebra.md to the same metadata block syntax so all ADRs
use a consistent phase declaration.
- Around line 79-81: The docs promise multi-phase inference but the
implementation only stores a single phase string via OnlyPhaseMetadata.Phase
with no merging logic; update the metadata and inference path so
referenced-field phases are merged to the "latest" phase: either change
OnlyPhaseMetadata.Phase from a single string to a phase enum/list (or add an
InferredPhase field) and implement phase-merging logic in the metadata
resolution code that iterates referenced field metadata, computes the maximum
phase according to the phase ordering, and sets the resulting phase on the rule
metadata (or emits a compile error when an explicit phase conflicts). Ensure the
merge is used wherever OnlyPhaseMetadata.Phase is consumed so multi-field rules
get the inferred/latest phase.
In `@docs/adr/0010-multi-target-compilation.md`:
- Line 49: The ADR states a backends metadata field but the SecRuleMetadata
struct in types/metadata.go lacks it; add a Backends []string (or appropriate
type) field to SecRuleMetadata and update the metadata parsing/decoding path to
populate it, then adjust the compilation logic (where rules are
filtered/compiled for a target—e.g., the compiler's rule compilation function)
to skip rules whose Backends is non-empty and does not include the current
target; ensure tests that load metadata and compile for a given target assert
the new behavior.
In `@docs/README.md`:
- Around line 77-86: Add a short explanatory sentence near the Target State rule
block (rule 920170) stating that the metadata field severity = warning now
automatically determines the anomaly score so manual tx.anomaly_score or setvar
adjustments are no longer needed; reference ADR-0011 in that sentence (e.g.,
"The severity = warning metadata now automatically drives scoring—no manual
setvar/tx.anomaly_score needed (see ADR-0011)"). Ensure this sentence appears
directly before or after the shown rule block so readers see why the manual
scoring line was removed.
---
Nitpick comments:
In `@docs/adr/0003-boolean-algebra.md`:
- Line 393: Update the Alternatives documented list to use consistent option
labels and explicitly state the chosen approach: add an entry like "Option A
(chosen): metadata property — `multi_match = true` in metadata block" and keep
the existing "Option B: pipeline function — `multi_match(lowercase,
remove_whitespace)`" text so readers see both the chosen metadata-property
solution and the pipeline-function alternative; ensure the label "Option A
(chosen)" replaces any ambiguous references and that `multi_match = true` is
shown exactly as the metadata syntax.
In `@docs/adr/0004-structured-actions.md`:
- Line 300: The PR uses inconsistent emphasis styles (this file changed
`*outcome*` to `_outcome_` while other docs changed `_text_` to `*text*`); pick
the project's standard emphasis style and make all files consistent (e.g.,
change `_outcome_` in docs/adr/0004-structured-actions.md to `*outcome*` or
convert the other changed files to `_text_`), and update instances referenced
(`docs/README.md` lines ~134/154 and `docs/adr/0007-phase-inference.md` line
~249) and any other docs changed in this PR to match the chosen style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6595db4e-231f-45af-90dd-28484e1c871f
📒 Files selected for processing (7)
docs/README.mddocs/adr/0001-typed-field-namespace.mddocs/adr/0002-pipeline-operator.mddocs/adr/0003-boolean-algebra.mddocs/adr/0004-structured-actions.mddocs/adr/0007-phase-inference.mddocs/adr/0010-multi-target-compilation.md
Member
|
@copilot solve conflicts and address all coderrabit's comments. |
- Fix .names -> .keys accessor in typed-field-namespace ADR - Add text language identifier to all bare fenced blocks in pipeline-operator ADR - Remove redundant inline phase declaration in boolean-algebra ADR - Add multi-phase inference example and clarify text in phase-inference ADR - Standardize phase declaration syntax (metadata blocks) in phase-inference ADR - Clarify backends feature as planned/unimplemented in multi-target-compilation ADR - Add explanation of automatic severity-based scoring in README Agent-Logs-Url: https://github.com/coreruleset/crslang/sessions/84e0ce9b-8b9d-4d6e-8991-9433a1c35582 Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com>
Contributor
Addressed all CodeRabbit comments in commit
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0003-boolean-algebra.md`:
- Around line 71-73: The boolean example has its second predicate inverted:
condition2 uses matches("^0?$") which matches empty or "0" and thus flips
intended logic; update the example so the predicate is negated (either change
the pattern to the negated form or apply a logical negation to condition2 in the
when clause) so that condition2 now represents "Content-Length is not 0/empty"
and keep references to condition1, condition2, the matches call and the when
expression when making the change.
- Around line 376-389: The fenced code block starting with the example showing
"Without multi_match" / "With multiMatch()" is missing a language tag; add the
neutral language identifier `text` to the opening triple-backtick (i.e., change
``` to ```text) so the MD040 linter warning is resolved and the CRSLang-style
example is treated as plain text while leaving the block contents unchanged
(references: the lines containing "When request.args |> lowercase() |>
remove_whitespace() |> detect_sqli()" and the "metadata { multi_match = true }"
example).
In `@docs/README.md`:
- Around line 97-106: The fenced code block containing the CRSLang example for
rule 901001 lacks a language tag (triggers MD040); update the opening fence to
include the plaintext tag by changing the opening ``` to ```text (or
```plaintext) for the block that contains rule 901001 { ... } so the example is
treated as plain text instead of an untagged fence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0729effd-d222-4c76-a545-686b948f93ab
📒 Files selected for processing (6)
docs/README.mddocs/adr/0001-typed-field-namespace.mddocs/adr/0002-pipeline-operator.mddocs/adr/0003-boolean-algebra.mddocs/adr/0007-phase-inference.mddocs/adr/0010-multi-target-compilation.md
✅ Files skipped from review due to trivial changes (3)
- docs/adr/0001-typed-field-namespace.md
- docs/adr/0007-phase-inference.md
- docs/adr/0002-pipeline-operator.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/adr/0010-multi-target-compilation.md
Comment on lines
+71
to
+73
| let condition1 = request.method |> matches("^(?:GET|HEAD)$") | ||
| let condition2 = request.headers["Content-Length"] |> matches("^0?$") | ||
| when condition1 and condition2 |
There was a problem hiding this comment.
Fix semantic inversion in the boolean example.
Line 72 currently matches ^0?$, which flips the intended logic from the earlier negated chain example. To preserve meaning, negate this predicate (or negate condition2 in when).
Suggested fix
- let condition2 = request.headers["Content-Length"] |> matches("^0?$")
- when condition1 and condition2
+ let condition2 = request.headers["Content-Length"] |> not(matches("^0?$"))
+ when condition1 and condition2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/0003-boolean-algebra.md` around lines 71 - 73, The boolean example
has its second predicate inverted: condition2 uses matches("^0?$") which matches
empty or "0" and thus flips intended logic; update the example so the predicate
is negated (either change the pattern to the negated form or apply a logical
negation to condition2 in the when clause) so that condition2 now represents
"Content-Length is not 0/empty" and keep references to condition1, condition2,
the matches call and the when expression when making the change.
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.
Discussion points
requestshould not be a shorthand forrequest_headers, since in HTTP request includes both request_headers and request_body.backendsto restrict which backends can use a given rule. Its main purpose is to make explicit to users which rules are supported by each backend. By default, the compiler will attempt to compile all rules that do not define the backends parameter.I haven’t updated all the examples with the proposed changes yet. Let’s discuss these points first, and then I will update the examples.
Summary by CodeRabbit