feat: implement scoped prefixes/suffixes - #282
Conversation
There was a problem hiding this comment.
Pull request overview
Implements block-scoped handling of ##!^ (prefix) and ##!$ (suffix) directives so prefixes/suffixes are applied by each Assemble processor instance rather than globally, simplifying regex assembly behavior.
Changes:
- Added prefix/suffix directive handling to the
Assembleprocessor and applied them duringComplete(). - Updated the parser to pass prefix/suffix directives through as raw lines (instead of collecting them on the parser).
- Removed operator-level prefix/suffix application and updated/added tests for the new scoping behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| regex/processors/assemble.go | Adds block-scoped prefix/suffix collection and applies them on completion of an assemble block. |
| regex/parser/parser.go | Changes prefix/suffix parsing to pass directives through verbatim rather than populating Parser.Prefixes/Suffixes. |
| regex/parser/include_test.go | Updates include parsing expectations to reflect pass-through directive behavior. |
| regex/operators/assembler.go | Removes global prefix/suffix application and leaves a note about new behavior. |
| regex/operators/assembler_test.go | Adds tests validating block-scoped prefix/suffix behavior across top-level and multiple blocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
f0a39e2 to
afb8fd0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughParser now emits prefix/suffix directive lines inline. Assemble applies them per block, including to stored expressions and included content. Operator-level wrapping is removed. Tests cover scoping, nesting, inclusion, and stashing. ChangesBlock-scoped prefix/suffix directive handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The scoped prefix and suffix changes are merge-ready after normal checks and review; no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 1
🤖 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/parser/parser.go`:
- Around line 297-301: The code currently returns out verbatim which leaks
included file prefix/suffix directives (##!^ / ##!$) into the caller's top-level
assemble scope; change mergePrefixesSuffixes (and the include handling path that
currently returns out) to detect if the included content has Parser.Prefixes or
Parser.Suffixes and, when present, inject an explicit local assemble boundary
(or equivalent scope delimiter) before and after the included content so those
directives apply only to the included block; update the return path so it
returns the wrapped/merged content (not raw out) and keep existing
Prefixes/Suffixes collection for compatibility, ensuring assemble.go's
store()/Complete() behavior no longer extends the caller scope.
🪄 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: 6f17f55f-9ba7-4827-a08d-bbffb1897063
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
regex/operators/assembler.goregex/operators/assembler_test.goregex/parser/include_test.goregex/parser/parser.goregex/processors/assemble.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
regex/parser/parser.go (1)
279-285:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-scope included prefix/suffix directives before returning the parsed buffer.
Line 285 still returns the included output verbatim, and
validateIncludedFileDirectivesonly rejects flags. That means##!^/##!$lines from the child file stay in the caller's current assemble block;regex/processors/assemble.gokeeps them active untilstore()orComplete(), so caller lines after the include get wrapped too. Please restore a local assemble boundary here, or an equivalent scoping wrapper, whenever the included output contains prefix/suffix directives.Also applies to: 288-295
🤖 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/parser/parser.go` around lines 279 - 285, The included buffer is returned verbatim so any child-file prefix/suffix assemble directives (e.g., ##!^ / ##!$) leak into the caller; modify the return path in the Parse/validate block to detect those directives on newP's output and wrap the parsed buffer in a local assemble-scoping boundary before returning. Concretely: after calling newP.Parse(false) and validateIncludedFileDirectives(newP), check newP (or out.String()) for prefix/suffix directive markers and, if present, create a short-lived assemble scope (e.g., call a helper like reScopeIncludedAssemble(newP) or newP.WrapWithLocalAssembleBoundary()) that inserts the equivalent begin/end assemble tokens around out so the caller's assembler (assemble.go store()/Complete()) does not inherit the child's active prefix/suffix directives; then return that wrapped buffer and newP.variables.
🤖 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.
Duplicate comments:
In `@regex/parser/parser.go`:
- Around line 279-285: The included buffer is returned verbatim so any
child-file prefix/suffix assemble directives (e.g., ##!^ / ##!$) leak into the
caller; modify the return path in the Parse/validate block to detect those
directives on newP's output and wrap the parsed buffer in a local
assemble-scoping boundary before returning. Concretely: after calling
newP.Parse(false) and validateIncludedFileDirectives(newP), check newP (or
out.String()) for prefix/suffix directive markers and, if present, create a
short-lived assemble scope (e.g., call a helper like
reScopeIncludedAssemble(newP) or newP.WrapWithLocalAssembleBoundary()) that
inserts the equivalent begin/end assemble tokens around out so the caller's
assembler (assemble.go store()/Complete()) does not inherit the child's active
prefix/suffix directives; then return that wrapped buffer and newP.variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e8d9ac11-6d71-469c-b83e-86678124dc1c
📒 Files selected for processing (1)
regex/parser/parser.go
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
…er handling Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com>
Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com>
Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com>
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
Address review feedback from theseion: - Remove redundant else-if branch in Complete() since the outer guard already ensures at least one of prefixes/suffixes is non-empty - Add test for multiple nesting levels to verify block-scoped prefix/suffix isolation across nested assemble blocks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
9a7d8e9 to
eb3944a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new prefix/suffix handling in Assemble can emit incorrect output for directive-only/stash-only blocks and currently clears directive state on stash, which can silently break later content/stashes in the same block.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
regex/processors/assemble.go:140
store()clearsa.prefixes/a.suffixesafter stashing. That mutates the directive scope for the rest of the assemble block, so any subsequent##!=<stores (or remaining emitted output in the same block) will silently lose the active prefix/suffix. Keeping the directives in-scope avoids surprising behavior; theComplete()guard can prevent stash-only blocks from emitting stray prefix/suffix.
// Clear prefixes/suffixes after applying to stored expression
// so they won't be applied again in Complete()
a.prefixes = nil
a.suffixes = nil
}
regex/processors/assemble.go:95
Complete()applies prefixes/suffixes even whenresultis empty (e.g., an assemble block that only contains directives, or a block that only stashes content). That can incorrectly emitprefix+suffixas output. Guard prefix/suffix application onresult != ""so directive-only/stash-only blocks stay silent.
// Apply block-scoped prefixes and suffixes
if len(a.prefixes) > 0 || len(a.suffixes) > 0 {
result = strings.Join(a.prefixes, "") + result + strings.Join(a.suffixes, "")
logger.Trace().Msgf("Applied block-scoped prefixes/suffixes: %s", result)
}
regex/operators/assembler.go:135
- This comment claims
Parser.Prefixes/Parser.Suffixesare still populated, but the parser now passes prefix/suffix directives through as raw lines and no longer appends to those fields. Updating this avoids leaving misleading documentation in the operator path.
// Note: Prefix/suffix application is now handled by individual Assemble processors (block-scoped).
// While Parser.Prefixes and Parser.Suffixes are still populated by the parser for include file
// merging compatibility, they are not used in the operator path as prefix/suffix directives are
// passed through as raw lines and processed by each Assemble instance.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Passing `##!^` and `##!$` through as raw lines left them active for the rest
of the including file. The Assemble processor holds a directive until store()
or Complete(), so an include carrying a prefix wrapped the caller's own
content as well, regardless of where the include appeared:
include/inc.ra: ##!^ PRE_
alpha
932100.ra: ##!> include inc
beta
before: PRE_(?:alph|bet)a
after: (?:PRE_alph|bet)a
Wrap included content in its own assemble block when the included file
contains prefix or suffix directives, which bounds them to that content. As
before, the block is omitted when there are no such directives, because
wrapping changes the semantics of content processed by the cmdline processor
in the including file.
This restores the local scope that mergePrefixesSuffixes used to provide.
Parser.Prefixes and Parser.Suffixes are populated again, now only so
parseFile can tell whether an included file needs the block, and the
assembler comment describing them is corrected.
The three include tests asserted the leaking output, so they could not catch
this. Restore their expectations and add end-to-end tests covering an include
before, after, and on both sides of the caller's own content.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed cb88d62, which fixes the include scoping problem @coderabbitai raised in this comment. The finding was correct and had not been addressed — the 2026-06-07 commits renamed the helper but left the behaviour unchanged. The regressionPrefix and suffix directives from an included file were passed through as raw lines. Built both branches and compared against
The leak was position-independent: the include's prefix applied to the entire caller block no matter where the include sat. That is the opposite of what #41 asks for, since it makes include-supplied prefixes maximally global. Root cause
The fix
The block is still omitted when there are no such directives, preserving the reason for the original
Why CI stayed green
Their expectations are restored, and
@theseion your three review comments look addressed by the earlier commits (field rename, the redundant branch, and the multi-level nesting test). This should be ready for another look when you have time. |
|
Tip For best results, initiate chat on the files or code changes.
The conditional The restored parser expectations and the new end-to-end include-scoping tests cover the failure mode. ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
what
why
refs
Summary by CodeRabbit
Bug Fixes
Tests