feat: add ##!=@ retrieve marker, disallow identifier on ##!=> - #323
Conversation
##!=> currently does two different jobs depending on whether it has an identifier: bare, it's a concatenation boundary; with a name, it splices in a block previously stashed with ##!=<. Same token, two different reader-intents. Give the splice case its own marker, ##!=@ <name>, and make ##!=> reject an identifier going forward, since a boundary marker taking one was the ambiguous case. Also drop the AssembleInput/AssembleOutput string constants in assemble.go, which duplicated regex/definitions.go's patterns and had no callers. Resolves: #20
📝 WalkthroughWalkthroughThe change separates stored-block retrieval from assemble output boundaries. It adds retrieval matching, updates ChangesAssemble marker syntax
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@regex/definitions.go`:
- Line 64: Align AssembleRetrieveRegex with AssembleInputRegex so identifiers
containing trailing text, such as spaces, remain retrievable after store
preserves them; either restrict input identifiers to a single non-whitespace
token or make retrieval capture and normalize the same grammar, and ensure the
migration error in assemble processing does not recommend an unusable form.
In `@regex/processors/assemble.go`:
- Around line 39-47: Update the retrieve-marker handling before the assembler’s
later content branches, near AssembleRetrieveRegex processing, to explicitly
detect lines beginning with the retrieve directive but not matching the complete
expected retrieve syntax. Reject malformed forms such as ##!=@ and ##!=@ name
extra with the existing validation/error path, while preserving successful
identifier extraction and append behavior for valid markers.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 91e17d4a-24b9-4246-8f00-2aad16c81632
📒 Files selected for processing (4)
cmd/regex/format/format_test.goregex/definitions.goregex/operators/assembler_test.goregex/processors/assemble.go
| // AssembleRetrieveRegex matches a retrieve line of the assemble processor | ||
| // (##!=@ <name>), splicing in a block previously stored with ##!=< <name>. | ||
| // The name is captured in group 1. | ||
| var AssembleRetrieveRegex = regexp.MustCompile(`^\s*##!=@\s*(\S+)\s*$`) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep store and retrieve identifiers on the same grammar.
AssembleInputRegex accepts all trailing text, and store saves it without trimming. AssembleRetrieveRegex accepts only one non-whitespace token. Therefore, ##!=< my block stores my block, but ##!=@ my block cannot retrieve it. The migration error in regex/processors/assemble.go can also direct users to this unusable form. Either reject whitespace in input identifiers or capture and normalize the same grammar for retrieval.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@regex/definitions.go` at line 64, Align AssembleRetrieveRegex with
AssembleInputRegex so identifiers containing trailing text, such as spaces,
remain retrievable after store preserves them; either restrict input identifiers
to a single non-whitespace token or make retrieval capture and normalize the
same grammar, and ensure the migration error in assemble processing does not
recommend an unusable form.
| match = regex.AssembleRetrieveRegex.FindStringSubmatch(line) | ||
| if len(match) > 0 { | ||
| identifier := match[1] | ||
| if err := a.append(identifier); err != nil { | ||
| var message string | ||
| if identifier != "" { | ||
| message = fmt.Sprintf("Failed to append output with name %s", identifier) | ||
| } else { | ||
| message = "Failed to append output of previous block" | ||
| } | ||
| logger.Error().Err(err).Msg(message) | ||
| logger.Error().Err(err).Msgf("Failed to retrieve stored output with name %s", identifier) | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'AssembleRetrieveRegex|ValidateAll|ProcessLine|##!=@' --glob '*.go' . || trueRepository: coreruleset/crs-toolchain
Length of output: 32906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' regex/validation/regex_validation.go
printf '\n--- assemble.go lines 160-210 ---\n'
sed -n '160,210p' regex/processors/assemble.go
printf '\n--- assemble_test.go relevant invalid marker tests ---\n'
python3 - <<'PY'
from pathlib import Path
p = Path('regex/processors/assemble_test.go')
text = p.read_text()
for needle in ['##!=@ unknown', '##!=@\n', '##!=@ name extra', '##!=@', 'Invalid assemble output', 'Invalid assemble retrieve']:
print(f'{needle!r}: {text.find(needle)}')
PY
printf '\n--- assembler parser files ---\n'
fd -a '.go$' regex/parser | sed 's#^\./##'
rg -n 'NewParser|Parse\(|type .*Parser|Start|Line' regex/parser --glob '*.go'Repository: coreruleset/crs-toolchain
Length of output: 13195
Reject malformed retrieve markers before assembler content.
validation.ValidateAll() only checks character classes and Unicode code points and does not see malformed ##!=@ directives here. Since AssembleRetrieveRegex requires an identifier, lines such as ##!=@ or ##!=@ name extra currently bypass retrieval and are handled by later assemble branches. Add an explicit validation/guard for malformed retrieve directives.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@regex/processors/assemble.go` around lines 39 - 47, Update the
retrieve-marker handling before the assembler’s later content branches, near
AssembleRetrieveRegex processing, to explicitly detect lines beginning with the
retrieve directive but not matching the complete expected retrieve syntax.
Reject malformed forms such as ##!=@ and ##!=@ name extra with the existing
validation/error path, while preserving successful identifier extraction and
append behavior for valid markers.
1 participant
what
##!=@ <name>marker to theassembleprocessor that splices in a block previously stored with##!=< <name>.##!=>(bare) keeps its existing meaning — a concatenation boundary — but now rejects an identifier instead of silently treating it as a splice. Passing one now fails with a clear error pointing at the replacement ('##!=>' no longer accepts an identifier; use '##!=@ <name>' to insert a stored block).AssembleInput/AssembleOutputstring constants inregex/processors/assemble.go— dead code duplicating the patterns already inregex/definitions.go, with no callers.regex/operators/assembler_test.go,cmd/regex/format/format_test.go) from##!=> <name>to##!=@ <name>, and added tests for the new marker and the new rejection error.why
##!=>currently does two different jobs depending on whether it has an identifier: bare, it's a concatenation boundary; with a name, it splices in a stashed block. Same token, two different reader-intents — you have to scan for a trailing identifier to know which one you're looking at.Real usage in
coreruleset/coreruleset'sregex-assembly/(checked via a shallow clone): 348 total##!=>occurrences, 265 (76%) bare, 83 (24%) named across 25 files. The bare/boundary form dominates, so it keeps the existing token; the splice case gets its own unambiguous one.breaking change / migration needed
This is a breaking DSL change.
coreruleset/corerulesetcurrently has 83 lines across 25regex-assembly/*.rafiles using the old##!=> <name>form (e.g.930100.ra,941160.ra,942420.ra, a fewinclude/*.ra). Those need a mechanical migration (^(\s*)##!=>\s+(\S+)$→$1##!=@ $2) before or alongside picking up this toolchain version — otherwiseregex generate/compare/updatewill fail on those files with the new rejection error. Happy to open that migration PR once this lands.refs
ai disclosure
go test ./..., excluding the pre-existing unrelated local GPG-signing failures inchore/release),go vet, and manually exercised both the new##!=@retrieval and the new rejection error against scratch.rafixtures with the built binary before and after the changeSummary by CodeRabbit
New Features
##!=@ <name>syntax for inserting previously stored assemble blocks.Bug Fixes
##!=>correctly represents an output boundary, while named block insertion uses the new retrieval syntax.