refactor(rules): introduce block DSL, phase-based execution#203
Merged
refactor(rules): introduce block DSL, phase-based execution#203
Conversation
… validation - add block syntax parser/scanner with nested @blocks and elif/else support - restructure rule execution into explicit pre/post phases with phase flags - classify commands by phase and termination behavior - enforce flow semantics (default rule handling, dead-rule detection) - expand HTTP flow coverage with block + YAML parity tests and benches - refresh rules README/spec and update playground/docs integration
Split error handling into isUnexpectedError predicate and logFlushError function. Use rm.AppendError() to collect unexpected errors during rule execution, then log after FlushRelease completes rather than immediately. Also updates goutils dependency for AppendError method availability.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Owner
Author
|
@coderabbitai review this pr |
|
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the routing rules engine to a phase-aware model (PhasePre/PhasePost), adds a block-rule DSL with nested if/elif/else parsing, replaces CommandHandler with function-based Handler/HandlerFunc using httputils.ResponseModifier, expands env-var/template handling with phase tracking, updates many tests, and removes ParsedRule.isResponseRule from docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RulesEngine as "Rules"
participant Parser as "Parser/Tokenizer"
participant PrePhase as "PreHandlers"
participant Upstream
participant PostPhase as "PostHandlers"
Client->>RulesEngine: BuildHandler(config)
RulesEngine->>Parser: Parse(config) (block DSL or YAML)
Parser-->>RulesEngine: Rules + Phase metadata
RulesEngine->>RulesEngine: Validate()
Client->>RulesEngine: ServeHTTP(req)
RulesEngine->>PrePhase: Execute pre-phase handlers (use ResponseModifier)
PrePhase-->>RulesEngine: Continue or Terminate (errTerminateRule)
RulesEngine->>Upstream: Call upstream if not terminated
Upstream-->>RulesEngine: Response
RulesEngine->>PostPhase: Execute post-phase handlers (response checks)
PostPhase-->>RulesEngine: Finalize response
RulesEngine-->>Client: Return response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/route/rules/README.md (1)
72-112:⚠️ Potential issue | 🟡 MinorMermaid class diagram is out of date with the new struct definitions.
The class diagram at lines 72–112 still shows the old field names and interfaces:
RuleOn.isResponseChecker bool→ should bephase PhaseFlag(as documented at lines 39–42).Command.exec CommandHandler/isResponseHandler bool→ should bepre Commands/post Commands(lines 44–48).- The
CheckerandCommandHandlerinterfaces may also need updating to reflect theResponseModifier-based signatures.This directly contradicts the struct definitions shown just above, which will confuse anyone reading the spec.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/README.md` around lines 72 - 112, The Mermaid class diagram is stale: update the RuleOn node to replace "isResponseChecker bool" with "phase PhaseFlag", update the Command node to replace "exec CommandHandler" and "isResponseHandler bool" with "pre Commands" and "post Commands", and adjust the Checker and CommandHandler interface method signatures to match the current code (use ResponseModifier-based signatures and response-aware method names) so the diagram matches the structs Rule, RuleOn, Command, PhaseFlag, Commands, Checker, CommandHandler and ResponseModifier.internal/api/v1/route/playground.go (1)
296-302:⚠️ Potential issue | 🟡 MinorRemove the dead
IsResponseRulefield fromParsedRulestruct.The
IsResponseRulefield (line 67) is never populated becauseRuleOn.phaseis private with no public accessor method, making the commented-out assignment impossible to implement. Since the field always defaults tofalseand is never updated, it misleads playground API consumers. Remove it:Diff
type ParsedRule struct { Name string `json:"name"` On string `json:"on"` Do string `json:"do"` ValidationError error `json:"validationError,omitempty"` - IsResponseRule bool `json:"isResponseRule"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/v1/route/playground.go` around lines 296 - 302, Remove the dead IsResponseRule field from the ParsedRule struct and any related commented-out assignment; specifically delete the IsResponseRule declaration in ParsedRule and remove the commented line in the slice append that attempts to set IsResponseRule (the commented reference to rule.Requirement()/RuleOn.phase is impossible because RuleOn.phase is private), and scan for and remove any other usages or JSON/schema references to ParsedRule.IsResponseRule so the playground API no longer exposes a misleading always-false field.internal/route/rules/var_bench_test.go (1)
19-26:⚠️ Potential issue | 🟡 Minor
Header.Setonly stores one value, so$header(X-Custom, 1)always resolves to"".Line 19 uses
Header.Set("X-Custom", "value1,value2"), which stores a single value"value1,value2". The benchmark then expands$header(X-Custom, 1), which requests the second value at index 1 — but there's only one entry, so it silently returns"". If the intent is to benchmark multi-value header lookup, useHeader.Add:- testRequest.Header.Set("X-Custom", "value1,value2") + testRequest.Header.Set("X-Custom", "value1") + testRequest.Header.Add("X-Custom", "value2")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/var_bench_test.go` around lines 19 - 26, The benchmark incorrectly uses testRequest.Header.Set("X-Custom", "value1,value2") which stores a single header value so $header(X-Custom, 1) will always be empty; change the setup to create two header values (e.g., call Header.Add("X-Custom", "value1") and Header.Add("X-Custom", "value2") or assign the slice directly) so the ExpandVars call (the invocation of ExpandVars(...) that expands $header(X-Custom, 0) and $header(X-Custom, 1)) actually exercises multi-value header lookup.
♻️ Duplicate comments (1)
internal/route/rules/http_flow_block_test.go (1)
816-845: SonarCloud credential warnings are false positives for test-only code.The
bcrypt.GenerateFromPasswordandSetBasicAuthcalls here use test-only credentials that are not real secrets. These were already flagged in previous reviews.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/http_flow_block_test.go` around lines 816 - 845, SonarCloud flags on bcrypt.GenerateFromPassword and SetBasicAuth are false positives for test-only credentials; suppress them by adding a short inline suppression comment immediately above the test lines using the project's linter/SAST suppression convention (e.g. // nolint or // `#nosec` or the specific Sonar rule suppression used in tests) and an explanatory note like "test-only credentials, not a secret" so the suppression targets the bcrypt.GenerateFromPassword and req.SetBasicAuth calls in this test.
🧹 Nitpick comments (24)
internal/route/rules/matcher.go (2)
17-17: Unbounded cache — acceptable for config-driven usage, but worth a note.
matcherCachegrows without bound. This is fine when expressions come from static configuration, but if the playground or other user-facing paths can generate arbitrary expressions, it becomes a slow memory leak. Consider documenting this assumption or adding a size cap if user-driven input is expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/matcher.go` at line 17, matcherCache (xsync.NewMap[string, Matcher]) is unbounded and can leak if expressions are user-driven; either document that expressions are static-only or add a bounded cache/eviction around matcher creation (e.g., wrap accesses to matcherCache in a size-capped LRU or TTL eviction) and update usage in functions that call matcherCache/Matcher to use the capped cache API; ensure tests or comments explain the assumption and chosen eviction policy.
64-103: Consider deduplicating the cache-store pattern across matcher types.Lines 83–87, 89–93, and 95–99 repeat the identical "store on success" pattern. A small refactor keeps the cache logic in one place:
♻️ Proposed refactor
switch t { case MatcherTypeString: - m, err := StringMatcher(expr, negate) - if err == nil { - matcherCache.Store(origExpr, m) - } - return m, err + m, err = StringMatcher(expr, negate) case MatcherTypeGlob: - m, err := GlobMatcher(expr, negate) - if err == nil { - matcherCache.Store(origExpr, m) - } - return m, err + m, err = GlobMatcher(expr, negate) case MatcherTypeRegex: - m, err := RegexMatcher(expr, negate) - if err == nil { - matcherCache.Store(origExpr, m) - } - return m, err + m, err = RegexMatcher(expr, negate) + default: + return nil, ErrInvalidArguments.Withf("invalid matcher type: %s", t) } - // won't reach here - return nil, ErrInvalidArguments.Withf("invalid matcher type: %s", t) + if err == nil { + matcherCache.Store(origExpr, m) + } + return m, err(You'd need to declare
var m Matcher; var err gperr.Errorbefore the switch.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/matcher.go` around lines 64 - 103, ParseMatcher repeats the same "store on success" cache logic for each matcher type; refactor by declaring a single Matcher variable (e.g., m) and gperr.Error variable (err) before the switch, assign m, err inside each case using StringMatcher/GlobMatcher/RegexMatcher, then after the switch perform one conditional matcherCache.Store(origExpr, m) when err == nil; keep existing negate handling, ExtractExpr call and the final ErrInvalidArguments return unchanged.internal/route/rules/parser_test.go (1)
95-98: Error expectation short-circuit is clean, though no outer test cases exercisewantErryet.The
wantErrfield withexpect.ErrorIsearly-return pattern is well-structured. Currently, none of the outer test cases setwantErr, so this path is untested. If this was added in preparation for future negative cases, consider adding at least one to exercise the branch (e.g., an invalid or malformed input that triggers a parse error).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/parser_test.go` around lines 95 - 98, The test includes an early-return branch that checks tt.wantErr with expect.ErrorIs(t, tt.wantErr, err) but no outer test case currently sets wantErr, so add at least one negative test case to exercise this path: create a table entry in parser_test.go where you set tt.wantErr to the expected parse error and provide an invalid or malformed input (e.g., malformed rule string) that triggers the parser error; ensure the test uses the same tt structure so expect.ErrorIs(t, tt.wantErr, err) is hit and validates the error.internal/route/rules/scanner.go (3)
27-85:atLineStartis never set totruewhen a newline is consumed as whitespace.When
\nis encountered in the whitespace branch (Line 32),atLineStartis set tofalse(Line 34). Since\nis a space character, this meansatLineStartis never correctly re-established after a blank line. Currently, this is benign because the#comment check also acceptsprevIsSpace(which is correctlytrueafter whitespace), and//comments don't gate onatLineStartat all. However, it makesatLineStartsemantically incorrect, which could lead to subtle issues if future logic relies on it.Proposed fix
if unicode.IsSpace(rune(c)) { + if c == '\n' { + atLineStart = true + } else { + atLineStart = false + } pos++ - atLineStart = false prevIsSpace = true continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/scanner.go` around lines 27 - 85, The atLineStart flag isn't restored when a newline is consumed in the whitespace branch of Tokenizer.skipComments; change the unicode.IsSpace branch to detect '\n' and set atLineStart = true and prevIsSpace = true for newline characters, otherwise keep atLineStart = false and prevIsSpace = true for other whitespace, so atLineStart is semantically correct after blank lines.
61-78: Unterminated block comment insidefindMatchingBracesilently continues without a specific error.In the
findMatchingBracemethod, if a/* ... */block comment inside braces is never closed, the parser continues scanning and eventually reports "unmatched '{'" instead of "unterminated block comment". By contrast,skipComments(Line 73-74) reports the specific error. Consider adding a check after the brace-matching loop to catch this case for better diagnostics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/scanner.go` around lines 61 - 78, In findMatchingBrace, the inline block-comment handling currently sets closed=false and will let the outer brace scan continue, causing a later "unmatched '{'" instead of a specific unterminated comment error; change the block that scans for "/* ... */" inside findMatchingBrace (the loop using pos, closed, and t.src checks) to check closed immediately after the inner loop and return ErrInvalidBlockSyntax.Withf("unterminated block comment") when not closed (same behavior as skipComments), ensuring atLineStart/prevIsSpace handling remains consistent before any continue/return.
117-249:atLineStart/prevIsSpacestate leaks from inside quoted strings into post-quote context.Inside the quote-handling block (Lines 152-164),
atLineStartandprevIsSpaceare updated based on characters within the quoted string. When the quote closes, these flags retain whatever the last character inside the quote set them to. For a multi-line quoted string ending with a newline before the closing quote (e.g.,"...\n"),atLineStartwould betrueafter the quote closes, potentially causing a#character immediately following to be misinterpreted as a line comment.Consider resetting
atLineStart = falseandprevIsSpace = falsewhen the closing quote is encountered (Line 153-154) to ensure the flags reflect the position of the quote character itself, not its contents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/scanner.go` around lines 117 - 249, In Tokenizer.findMatchingBrace the atLineStart and prevIsSpace flags are being set based on characters inside a quote and thus leak into the post-quote context; when clearing quote (inside the quote-handling block where quote is set to 0), reset atLineStart = false and prevIsSpace = false so the state reflects the quote boundary rather than the last char inside the quoted string; update the quote-handling branch around where quote is cleared to explicitly set those flags before continuing.internal/route/rules/scanner_test.go (1)
1-39: Good foundational tests; consider expanding coverage for nested constructs.The existing tests cover core tokenizer behavior well. Consider adding tests for:
- Nested braces:
{ { inner } }— verifies correct depth tracking.${...}env vars inside braces:{ ${VAR} }— verifies env vars are skipped correctly.- Comments inside braces:
{ // comment\n code }— verifies comment handling during brace matching.scanToBracewith unmatched}: e.g.,cond }— verifies the specific error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/scanner_test.go` around lines 1 - 39, Add unit tests that cover nested braces, env var substitutions, comments inside braces, and unmatched closing braces to strengthen tokenizer coverage: create new tests using newTokenizer and call scanToBrace, findMatchingBrace, and skipComments as appropriate—e.g., a test asserting correct depth and matching position for "{ { inner } }" using findMatchingBrace; a test with "{ ${VAR} }" ensuring env var sequences are skipped and the outer brace matches; a test with "{ // comment\n code }" verifying skipComments is invoked and findMatchingBrace returns the correct closing position; and a test for scanToBrace where input like "cond }" returns the specific unmatched-brace error—use require.Error/require.NoError and require.Equal with strings.Index/LastIndex to assert positions.internal/route/rules/vars_dynamic.go (1)
67-96:VarFormandVarPostFormshare identical form-parsing boilerplate.Both getters duplicate the
req.Form == nil→req.ParseForm()pattern. Consider extracting a small helper to reduce duplication and make future changes easier.Possible helper extraction
func ensureFormParsed(req *http.Request) error { if req.Form == nil { return req.ParseForm() } return nil }Then each getter simplifies to:
VarForm: { phase: PhaseNone, get: func(args []string, w *httputils.ResponseModifier, req *http.Request) (string, error) { key, index, err := getKeyAndIndex(args) if err != nil { return "", err } - if req.Form == nil { - if err := req.ParseForm(); err != nil { - return "", err - } - } + if err := ensureFormParsed(req); err != nil { + return "", err + } return getValueByKeyAtIndex(req.Form, key, index) }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/vars_dynamic.go` around lines 67 - 96, Extract a small helper (e.g., ensureFormParsed(req *http.Request) error) and call it from both VarForm.get and VarPostForm.get to remove the duplicated req.Form == nil / req.ParseForm() boilerplate; keep the existing behavior (return any parse error) and then proceed to call getValueByKeyAtIndex(req.Form, ...) or getValueByKeyAtIndex(req.PostForm, ...) after calling ensureFormParsed, leaving getKeyAndIndex and getValueByKeyAtIndex unchanged.internal/route/rules/errors.go (1)
35-35: Track the TODO for structuringErrInvalidBlockSyntax.The TODO indicates this error should be turned into a structured error type. This would improve error messages by carrying context (e.g., line number, block name).
Would you like me to open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/errors.go` at line 35, ErrInvalidBlockSyntax is currently a plain gperr.New error with a TODO to convert it to a structured error; replace this sentinel error with a concrete error type (e.g., type InvalidBlockSyntaxError struct { Line int; BlockName string; Err error } implementing Error() and Unwrap()) and update places that return or compare ErrInvalidBlockSyntax to construct and return the structured error (and where tests/assertions compare the sentinel, adapt to check error type or use errors.As). Ensure the new type is defined near ErrInvalidBlockSyntax and export fields needed for consumers (Line, BlockName, underlying Err) so callers can log/contextualize the error.internal/route/rules/parser.go (1)
64-93: Consider merging the two argument-extraction passes into one.Lines 64–76 count args, lines 80–93 extract them. For a fast-path function, the pre-allocation is sensible, but the near-identical loop logic is duplicated. A single pass with
append(relying on Go's doubling growth) would be simpler and nearly as fast given the typical small arg count. That said, this is a minor nit in a performance-sensitive path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/parser.go` around lines 64 - 93, The current parser has two nearly identical passes to count and then extract arguments (variables argCount, args, v, i, start) which duplicates logic; simplify by replacing both passes with a single loop that scans v from i, skips spaces, captures start/end substrings and appends them directly to args (letting Go grow the slice) and remove the separate argCount pre-scan and the separate allocation; keep the early-return when no args found by checking if args is empty after the single pass and return subject, nil, nil, true as before.internal/route/rules/block_parser_test.go (2)
39-41: Redundant assertion on line 41.Line 40 already asserts
rules[0].Do.raw == "upstream", and sinceCommandUpstreamis"upstream"(per the relevant snippet), line 41 is equivalent. Consider removing one of them.Proposed fix
assert.Equal(t, OnDefault, rules[0].On.raw) - assert.Equal(t, "upstream", rules[0].Do.raw) - assert.True(t, rules[0].Do.raw == CommandUpstream) + assert.Equal(t, CommandUpstream, rules[0].Do.raw)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/block_parser_test.go` around lines 39 - 41, The test contains a redundant assertion: after assert.Equal(t, "upstream", rules[0].Do.raw) you also assert the same value with assert.True(t, rules[0].Do.raw == CommandUpstream); remove the redundant assert.True line so the test only checks rules[0].Do.raw once (keep the existing assert.Equal or, if you prefer, replace the equal check to compare against CommandUpstream consistently).
324-326: Remove commented-out assertion.Line 325 has a stale commented-out assertion. Since the implementation reports "unexpected token after else block" for multiple-else cases (line 326 confirms this), the old expectation is dead code.
Proposed fix
require.Error(t, err) - // assert.Contains(t, err.Error(), "multiple 'else' branches") assert.Contains(t, err.Error(), "unexpected token after else block")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/block_parser_test.go` around lines 324 - 326, Remove the stale commented-out assertion line "// assert.Contains(t, err.Error(), "multiple 'else' branches")" in the test that checks error messages after parsing (the block parser test that calls require.Error(t, err) and assert.Contains(t, err.Error(), "unexpected token after else block")); simply delete that commented line so the test only asserts the current expected message "unexpected token after else block".internal/route/rules/vars_test.go (1)
630-650: Verify:remote_hostreturns empty string whenRemoteAddrhas no port.Lines 641 and 648 expect empty strings for both
$remote_hostand$remote_portwhenRemoteAddr = "192.168.1.1"(no colon-separated port). This is becausenet.SplitHostPortfails on addresses without a port. The behavior is internally consistent, but it means$remote_hostis also empty (not"192.168.1.1"), which could surprise users. If this is intentional, consider adding a brief comment in the test explaining whyremote_hostis also empty in this scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/vars_test.go` around lines 630 - 650, Update the TestExpandVars_NoRemotePort test to document why both $remote_host and $remote_port are expected to be empty when testRequest.RemoteAddr is set to "192.168.1.1" (no port): add a brief comment in TestExpandVars_NoRemotePort explaining that ExpandVars uses net.SplitHostPort (so without a colon-separated port both remote_host and remote_port will be empty), referencing the testRequest.RemoteAddr setup and the assertions for $remote_host and $remote_port to make this intentional behavior clear.internal/route/rules/rules.go (2)
297-304: Pre-rule slice is rebuilt on every request.
preRules,executedPre, andterminatedInPreare allocated per-request inside the handler closure. Since the rule set is fixed afterBuildHandler, consider pre-building the combinedpreRulesslice once in the closure's setup (before the returnedfunc) and only allocating the boolean tracking slices per-request:Proposed approach
+ preRules := make(Rules, 0, len(nonDefaultRules)+1) + if defaultRule != nil { + preRules = append(preRules, *defaultRule) + } + preRules = append(preRules, nonDefaultRules...) + preRuleCount := len(preRules) + return func(w http.ResponseWriter, r *http.Request) { rm := httputils.NewResponseModifier(w) defer func() { ... }() - preRules := make(Rules, 0, len(nonDefaultRules)+1) - if defaultRule != nil { - preRules = append(preRules, *defaultRule) - } - preRules = append(preRules, nonDefaultRules...) - - executedPre := make([]bool, len(preRules)) - terminatedInPre := make([]bool, len(preRules)) + executedPre := make([]bool, preRuleCount) + terminatedInPre := make([]bool, preRuleCount)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/rules.go` around lines 297 - 304, The code currently rebuilds the combined preRules slice and its tracking slices on every request inside the returned handler closure; move the one-time construction of the combined preRules slice (append of *defaultRule and nonDefaultRules) into the BuildHandler closure's setup (before returning the func) so preRules is computed once, and keep only the per-request allocations for executedPre and terminatedInPre inside the returned handler; update references to preRules, defaultRule, and nonDefaultRules accordingly so the handler uses the prebuilt preRules while still allocating executedPre and terminatedInPre per request.
159-189: Redundant re-parse when input has a top-level brace.When
hasTopLevelLBraceis true, block parsing is attempted at line 167. If it fails, YAML is tried (line 177). If YAML also fails, block parsing is retried at line 183 with the exact same input, producing the same error. Consider returning the original block-parse error directly in this case:Proposed fix
func (rules *Rules) Parse(config string) error { config = strings.TrimSpace(config) if config == "" { return nil } + var blockErr gperr.Error // Prefer block syntax if it looks like block syntax. if hasTopLevelLBrace(config) { - blockRules, err := parseBlockRules(config) - if err == nil { + blockRules, blockErr := parseBlockRules(config) + if blockErr == nil { *rules = blockRules return nil } // Fall through to YAML (backward compatibility). } // YAML fallback var anySlice []any yamlErr := yaml.Unmarshal([]byte(config), &anySlice) if yamlErr == nil { return serialization.ConvertSlice(reflect.ValueOf(anySlice), reflect.ValueOf(rules), false) } - // If YAML fails and we didn't try block syntax yet, try it now. - blockRules, err := parseBlockRules(config) - if err == nil { - *rules = blockRules - return nil + if blockErr == nil { + // Didn't try block syntax yet; try it now. + blockRules, blockErr := parseBlockRules(config) + if blockErr == nil { + *rules = blockRules + return nil + } } - return err + return blockErr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/rules.go` around lines 159 - 189, The Parse method redundantly re-runs parseBlockRules when hasTopLevelLBrace(config) is true: call parseBlockRules once and if it returns an error, return that error immediately instead of falling through to YAML parsing and re-invoking parseBlockRules. Modify Rules.Parse so that inside the hasTopLevelLBrace(config) branch you capture the result of parseBlockRules(config) and on err != nil return that err; otherwise assign *rules = blockRules and return nil; keep the existing YAML fallback only for cases where hasTopLevelLBrace(config) is false.internal/route/rules/on.go (1)
508-660:splitAnd/splitPipeandforEachAndPart/forEachPipePartare parallel implementations.Both pairs do the same splitting logic — one returns a slice, the other invokes a callback. The callback variants were added for the new
Parsemethod. Consider consolidating: either makesplitAnd/splitPipecall theforEachvariants internally, or vice versa, to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/on.go` around lines 508 - 660, The splitAnd/splitPipe functions duplicate logic already implemented by forEachAndPart/forEachPipePart; refactor by re-implementing splitAnd to build and return a []string by calling forEachAndPart (collect parts into a slice inside the callback) and similarly implement splitPipe to call forEachPipePart, removing the duplicated parsing code in splitAnd/splitPipe; keep existing behaviors (trim spaces, skip empty parts, respect escapes/quotes/brackets) and preserve function signatures for indexAnd/countAnd usage elsewhere so only the internals change.internal/route/rules/block_parser.go (2)
386-406:scanToBraceerror is silently swallowed.When
scanToBracereturns an error (line 400),parseRuleHeadersets*pos = lengthand returns the rest of the input as the header string. The error itself is discarded. While the caller (parseBlockRules) will subsequently fail because it won't find{, the original error context (e.g. "unterminated quote") is lost, making debugging harder.Consider propagating the error or at least returning an empty string so the caller produces a more informative diagnostic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/block_parser.go` around lines 386 - 406, The function parseRuleHeader currently swallows errors from t.scanToBrace; change parseRuleHeader to return (string, error) instead of just string, preserve *pos when scanToBrace fails, and return an empty header with the wrapped error (from scanToBrace) so callers like parseBlockRules can surface the original diagnostic (e.g., "unterminated quote"); update all call sites (parseBlockRules and any others) to handle the returned error and propagate or log it accordingly.
311-333: Escape sequences processed inside backtick-quoted strings.When
quoteis a backtick (`), the backslash escape handling at line 313 still applies. In most languages/DSLs, backtick-quoted strings are raw literals where\has no special meaning. If this DSL follows that convention, a condition likequote != ''` should gate the escape logic.If backticks intentionally support escapes in this DSL, a brief comment would prevent future confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/block_parser.go` around lines 311 - 333, The backslash-escape handling currently runs regardless of the active quote type; update the logic so escapes are only processed when the active quote (variable quote) is not a backtick (`'`'`), e.g. add a guard around the backslash branch that checks quote != '`' (or the equivalent rune check) before treating '\' as an escape; reference the variables and branches in this block (quote, c, src, out, atLineStart, prevIsSpace) and either add that guard or, if backticks intentionally support escapes, add a clarifying comment above the escape handling explaining that backticks are not raw in this DSL.internal/route/rules/http_flow_yaml_test.go (4)
514-517: Unused test file:tempDir/index.htmlis created but never requested.Only
/files/index.html(mapped totempDir/files/index.html, created on line 537) is used in the test. This initial file write is dead setup code.Proposed cleanup
- // Create test files directly in the temp directory - testFile := filepath.Join(tempDir, "index.html") - err = os.WriteFile(testFile, []byte("<h1>Test Page</h1>"), 0o644) - require.NoError(t, err) - var rules Rules🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/http_flow_yaml_test.go` around lines 514 - 517, Remove the dead test setup that writes tempDir/index.html: the variable testFile and the os.WriteFile call that creates "<h1>Test Page</h1>" are unused in this test, so either delete those two lines (the testFile assignment and the os.WriteFile(err) block) or change them to create the file under tempDir/files/index.html to match the actual asserted route; update any variable names accordingly (testFile, tempDir) so the created file matches the path used by the test (/files/index.html).
866-888: Misleading header value "should-not-run" in a command that does run.Line 871 sets header
X-Lateto"should-not-run", but line 886 asserts this header is present—meaning theset resp_headercommand did execute (in the post phase). The literal value"should-not-run"contradicts the actual test expectation and will confuse future readers.Consider renaming to something like
"post-phase-ran"to clarify that this command is expected to execute in the post phase despite pre-termination.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/http_flow_yaml_test.go` around lines 866 - 888, Rename the misleading header value used in the test to reflect that the second `set resp_header X-Late` runs in the post phase; update the YAML in the `parseRules` call so the `do: set resp_header X-Late` value is something like "post-phase-ran" (or similar) instead of "should-not-run", and adjust the corresponding assertion on `w.Header().Get("X-Late")` in the same test (`http_flow_yaml_test.go`, the test that builds `rules` and calls `rules.BuildHandler(upstream)`) to expect the new value so the test clearly documents that the post-phase header was applied.
247-259: Block DSL syntax test mixed into YAML test suite.This test uses the block DSL syntax (
status 4xx { ... }) rather than the YAML list format used by every other test in this file. The YAML suffix in the test name suggests this should be testing the YAML variant. If the block DSL is intentionally also valid YAML input (parsed viaparseRules), this is fine—but consider adding a brief comment explaining why this test deviates from the YAML list-of-rules pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/http_flow_yaml_test.go` around lines 247 - 259, The test in http_flow_yaml_test.go constructs rules using the block DSL string passed to parseRules (see parseRules, Rules, TestRandomFileName) but lives in the YAML test suite; either convert the input to the YAML list-of-rules format to match the rest of the file or add a one-line comment above the test explaining that the block DSL is intentionally accepted by parseRules and therefore included here; update the test name or comment to clarify intent so future readers know why block syntax appears in a YAML-focused test.
586-605: Notify test only verifies the HTTP response, not the notification itself.The test confirms the request completes successfully (status 200, body "ok") but doesn't assert that any notification was dispatched. Consider injecting a mock notification provider or verifying side effects to ensure the
notifycommand actually fires.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/http_flow_yaml_test.go` around lines 586 - 605, The test TestHTTPFlow_NotifyCommandYAML only asserts the HTTP response but not that the notify action ran; modify the test to inject a mock notification provider and assert it received the expected call (e.g., capture title/body/level) when the handler built by Rules.BuildHandler handles the request. Create a mock provider (or spy) and register it with the same provider registry / Rules instance used by the notify command before calling handler.ServeHTTP, then assert the mock recorded a single notification with content matching "title $req_method" and "body $req_url $status_code". Ensure references to Rules, BuildHandler and the notify command are used so the test verifies the side effect rather than only the HTTP response.internal/route/rules/do.go (2)
341-368: Per-request transport allocation in the no-host proxy path should be considered for optimization.When
target.Hostis empty (line 346), a newreverseproxy.NewReverseProxyandgphttp.NewTransport()are created on every request (line 354) instead of being reused. This prevents HTTP connection pooling and can impact performance under load. For scenarios with bounded dynamic hosts, consider implementing a transport cache keyed by host to maintain connection reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/do.go` around lines 341 - 368, The current build closure creates a new reverseproxy and gphttp.NewTransport on every request when target.Host == "" (see build, target, reverseproxy.NewReverseProxy, gphttp.NewTransport, TryGetUpstreamHostPort), preventing connection reuse; change this by introducing a host-keyed cache (map or sync.Map with mutex) that stores a reusable transport (and optionally a reverseproxy) per upstream host, lookup the upstream host via routes.TryGetUpstreamHostPort(r), create and insert the transport/proxy only when absent, and then reuse the cached transport/proxy inside the returned handler so rp.ServeHTTP uses pooled connections and avoids per-request allocations (preserve existing behavior of setting r.URL.Path/RawPath and returning errTerminateRule).
297-324: Help text forrequire_basic_authis misleading about credential verification.The command unconditionally responds with
401 UnauthorizedandWWW-Authenticateheader without checking credentials. This behavior is intentional (confirmed by test: "require_basic_auth should terminate before calling upstream"), but the help text "Require HTTP basic authentication for incoming requests" obscures this design. The actual credential validation uses thebasic_authcondition inonclauses. Update the help description to clarify that this command challenges for basic auth rather than verifies credentials, and add a brief comment distinguishing it fromrequire_auth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/do.go` around lines 297 - 324, The help text for CommandRequireBasicAuth is misleading because the handler returned by build (require_basic_auth) only challenges clients with a 401/WWW-Authenticate and does not verify credentials; update the Help.description for CommandRequireBasicAuth to clearly state it "challenges for HTTP Basic auth (sends WWW-Authenticate and 401) but does not verify credentials — use the basic_auth condition in on clauses or require_auth to perform verification", and add a short inline comment near CommandRequireBasicAuth or above the build/terminate block clarifying the difference between this challenge-only command and the credential-checking basic_auth/require_auth logic so reviewers can find it (referencing CommandRequireBasicAuth, require_basic_auth, basic_auth, and require_auth).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/route/rules/block_parser.go`:
- Around line 20-86: expandEnvVarsRaw currently returns ErrUnterminatedEnvVar
but drops the partially-read variable from the output; when inEnvVar is true at
EOF you must write the raw "${" plus the contents of envVar back into buf so the
returned string preserves the original input. Locate expandEnvVarsRaw and after
the final loop (before setting err = ErrUnterminatedEnvVar) append the literal
open sequence and the current envVar contents into buf (i.e. write "$" + "{" +
envVar.String() or equivalent using buf.WriteRune/WriteString and
envVar.String()), then set the error as before; keep the existing handling for
expectingBrace and missingEnvVars unchanged.
In `@internal/route/rules/do_blocks.go`:
- Around line 65-85: The ServeHTTP loop in IfElseBlockCommand currently skips
branches with a matching checker when br.Do == nil, allowing later Else to run;
update the logic so that when br.On.checker is non-nil and
br.On.checker.Check(w,r) returns true, the branch always consumes the match: if
br.Do is nil then return nil (matched-but-no-op), otherwise call and return
Commands(br.Do).ServeHTTP(...). Also mirror the unconditional branch handling
(when br.On.checker == nil) so it likewise returns nil for an empty Do instead
of continuing.
In `@internal/route/rules/do.go`:
- Around line 104-111: The inline handler returned by build calls authHandler
without checking for nil, which can panic; update the closure in build (the
HandlerFunc) to first check if authHandler == nil and if so return
errTerminateRule (or an appropriate error), otherwise call authHandler(w, r) and
proceed with the existing logic; reference authHandler, build, HandlerFunc,
errTerminateRule and httputils.ResponseModifier when making the change.
In `@internal/route/rules/http_flow_block_test.go`:
- Around line 449-456: The comment "Public IP => public" is incorrect because
10.0.0.1 is a private RFC1918 address; update the test to either use a truly
public TEST-NET address (e.g., change req2.RemoteAddr to "203.0.113.1:12345") or
change the comment to "Non-matching IP => public" so it accurately reflects the
intent; locate the test block that creates req2 (req2 :=
httptest.NewRequest(...), req2.RemoteAddr = "10.0.0.1:12345",
handler.ServeHTTP(w2, req2)) and apply the comment or IP change there.
In `@internal/route/rules/http_flow_yaml_test.go`:
- Line 383: The assert call uses assert.Equal with the actual value
w2.Body.String() in the expected position and the literal "Unauthorized\n" in
the actual position; swap the arguments so assert.Equal receives the expected
value first and the actual value second (i.e., pass the string literal as
expected and w2.Body.String() as actual) for the assert.Equal call in this test.
In `@internal/route/rules/on.go`:
- Around line 55-72: The OnDefault entry currently provides a builder that
always returns false, which makes YAML-specified "on: default" never match;
update the OnDefault.builder to return a CheckFunc that returns true so default
rules always match (modify the anonymous function in the OnDefault.builder to
return true), or alternatively adjust RuleOn.Parse to special-case the "default"
token and leave the checker as nil to mirror block_parser behavior—pick one
approach and update the corresponding symbol (OnDefault.builder or RuleOn.Parse)
so YAML-parsed default rules behave like block-parsed defaults.
---
Outside diff comments:
In `@internal/api/v1/route/playground.go`:
- Around line 296-302: Remove the dead IsResponseRule field from the ParsedRule
struct and any related commented-out assignment; specifically delete the
IsResponseRule declaration in ParsedRule and remove the commented line in the
slice append that attempts to set IsResponseRule (the commented reference to
rule.Requirement()/RuleOn.phase is impossible because RuleOn.phase is private),
and scan for and remove any other usages or JSON/schema references to
ParsedRule.IsResponseRule so the playground API no longer exposes a misleading
always-false field.
In `@internal/route/rules/README.md`:
- Around line 72-112: The Mermaid class diagram is stale: update the RuleOn node
to replace "isResponseChecker bool" with "phase PhaseFlag", update the Command
node to replace "exec CommandHandler" and "isResponseHandler bool" with "pre
Commands" and "post Commands", and adjust the Checker and CommandHandler
interface method signatures to match the current code (use
ResponseModifier-based signatures and response-aware method names) so the
diagram matches the structs Rule, RuleOn, Command, PhaseFlag, Commands, Checker,
CommandHandler and ResponseModifier.
In `@internal/route/rules/var_bench_test.go`:
- Around line 19-26: The benchmark incorrectly uses
testRequest.Header.Set("X-Custom", "value1,value2") which stores a single header
value so $header(X-Custom, 1) will always be empty; change the setup to create
two header values (e.g., call Header.Add("X-Custom", "value1") and
Header.Add("X-Custom", "value2") or assign the slice directly) so the ExpandVars
call (the invocation of ExpandVars(...) that expands $header(X-Custom, 0) and
$header(X-Custom, 1)) actually exercises multi-value header lookup.
---
Duplicate comments:
In `@internal/route/rules/http_flow_block_test.go`:
- Around line 816-845: SonarCloud flags on bcrypt.GenerateFromPassword and
SetBasicAuth are false positives for test-only credentials; suppress them by
adding a short inline suppression comment immediately above the test lines using
the project's linter/SAST suppression convention (e.g. // nolint or // `#nosec` or
the specific Sonar rule suppression used in tests) and an explanatory note like
"test-only credentials, not a secret" so the suppression targets the
bcrypt.GenerateFromPassword and req.SetBasicAuth calls in this test.
---
Nitpick comments:
In `@internal/route/rules/block_parser_test.go`:
- Around line 39-41: The test contains a redundant assertion: after
assert.Equal(t, "upstream", rules[0].Do.raw) you also assert the same value with
assert.True(t, rules[0].Do.raw == CommandUpstream); remove the redundant
assert.True line so the test only checks rules[0].Do.raw once (keep the existing
assert.Equal or, if you prefer, replace the equal check to compare against
CommandUpstream consistently).
- Around line 324-326: Remove the stale commented-out assertion line "//
assert.Contains(t, err.Error(), "multiple 'else' branches")" in the test that
checks error messages after parsing (the block parser test that calls
require.Error(t, err) and assert.Contains(t, err.Error(), "unexpected token
after else block")); simply delete that commented line so the test only asserts
the current expected message "unexpected token after else block".
In `@internal/route/rules/block_parser.go`:
- Around line 386-406: The function parseRuleHeader currently swallows errors
from t.scanToBrace; change parseRuleHeader to return (string, error) instead of
just string, preserve *pos when scanToBrace fails, and return an empty header
with the wrapped error (from scanToBrace) so callers like parseBlockRules can
surface the original diagnostic (e.g., "unterminated quote"); update all call
sites (parseBlockRules and any others) to handle the returned error and
propagate or log it accordingly.
- Around line 311-333: The backslash-escape handling currently runs regardless
of the active quote type; update the logic so escapes are only processed when
the active quote (variable quote) is not a backtick (`'`'`), e.g. add a guard
around the backslash branch that checks quote != '`' (or the equivalent rune
check) before treating '\' as an escape; reference the variables and branches in
this block (quote, c, src, out, atLineStart, prevIsSpace) and either add that
guard or, if backticks intentionally support escapes, add a clarifying comment
above the escape handling explaining that backticks are not raw in this DSL.
In `@internal/route/rules/do.go`:
- Around line 341-368: The current build closure creates a new reverseproxy and
gphttp.NewTransport on every request when target.Host == "" (see build, target,
reverseproxy.NewReverseProxy, gphttp.NewTransport, TryGetUpstreamHostPort),
preventing connection reuse; change this by introducing a host-keyed cache (map
or sync.Map with mutex) that stores a reusable transport (and optionally a
reverseproxy) per upstream host, lookup the upstream host via
routes.TryGetUpstreamHostPort(r), create and insert the transport/proxy only
when absent, and then reuse the cached transport/proxy inside the returned
handler so rp.ServeHTTP uses pooled connections and avoids per-request
allocations (preserve existing behavior of setting r.URL.Path/RawPath and
returning errTerminateRule).
- Around line 297-324: The help text for CommandRequireBasicAuth is misleading
because the handler returned by build (require_basic_auth) only challenges
clients with a 401/WWW-Authenticate and does not verify credentials; update the
Help.description for CommandRequireBasicAuth to clearly state it "challenges for
HTTP Basic auth (sends WWW-Authenticate and 401) but does not verify credentials
— use the basic_auth condition in on clauses or require_auth to perform
verification", and add a short inline comment near CommandRequireBasicAuth or
above the build/terminate block clarifying the difference between this
challenge-only command and the credential-checking basic_auth/require_auth logic
so reviewers can find it (referencing CommandRequireBasicAuth,
require_basic_auth, basic_auth, and require_auth).
In `@internal/route/rules/errors.go`:
- Line 35: ErrInvalidBlockSyntax is currently a plain gperr.New error with a
TODO to convert it to a structured error; replace this sentinel error with a
concrete error type (e.g., type InvalidBlockSyntaxError struct { Line int;
BlockName string; Err error } implementing Error() and Unwrap()) and update
places that return or compare ErrInvalidBlockSyntax to construct and return the
structured error (and where tests/assertions compare the sentinel, adapt to
check error type or use errors.As). Ensure the new type is defined near
ErrInvalidBlockSyntax and export fields needed for consumers (Line, BlockName,
underlying Err) so callers can log/contextualize the error.
In `@internal/route/rules/http_flow_yaml_test.go`:
- Around line 514-517: Remove the dead test setup that writes
tempDir/index.html: the variable testFile and the os.WriteFile call that creates
"<h1>Test Page</h1>" are unused in this test, so either delete those two lines
(the testFile assignment and the os.WriteFile(err) block) or change them to
create the file under tempDir/files/index.html to match the actual asserted
route; update any variable names accordingly (testFile, tempDir) so the created
file matches the path used by the test (/files/index.html).
- Around line 866-888: Rename the misleading header value used in the test to
reflect that the second `set resp_header X-Late` runs in the post phase; update
the YAML in the `parseRules` call so the `do: set resp_header X-Late` value is
something like "post-phase-ran" (or similar) instead of "should-not-run", and
adjust the corresponding assertion on `w.Header().Get("X-Late")` in the same
test (`http_flow_yaml_test.go`, the test that builds `rules` and calls
`rules.BuildHandler(upstream)`) to expect the new value so the test clearly
documents that the post-phase header was applied.
- Around line 247-259: The test in http_flow_yaml_test.go constructs rules using
the block DSL string passed to parseRules (see parseRules, Rules,
TestRandomFileName) but lives in the YAML test suite; either convert the input
to the YAML list-of-rules format to match the rest of the file or add a one-line
comment above the test explaining that the block DSL is intentionally accepted
by parseRules and therefore included here; update the test name or comment to
clarify intent so future readers know why block syntax appears in a YAML-focused
test.
- Around line 586-605: The test TestHTTPFlow_NotifyCommandYAML only asserts the
HTTP response but not that the notify action ran; modify the test to inject a
mock notification provider and assert it received the expected call (e.g.,
capture title/body/level) when the handler built by Rules.BuildHandler handles
the request. Create a mock provider (or spy) and register it with the same
provider registry / Rules instance used by the notify command before calling
handler.ServeHTTP, then assert the mock recorded a single notification with
content matching "title $req_method" and "body $req_url $status_code". Ensure
references to Rules, BuildHandler and the notify command are used so the test
verifies the side effect rather than only the HTTP response.
In `@internal/route/rules/matcher.go`:
- Line 17: matcherCache (xsync.NewMap[string, Matcher]) is unbounded and can
leak if expressions are user-driven; either document that expressions are
static-only or add a bounded cache/eviction around matcher creation (e.g., wrap
accesses to matcherCache in a size-capped LRU or TTL eviction) and update usage
in functions that call matcherCache/Matcher to use the capped cache API; ensure
tests or comments explain the assumption and chosen eviction policy.
- Around line 64-103: ParseMatcher repeats the same "store on success" cache
logic for each matcher type; refactor by declaring a single Matcher variable
(e.g., m) and gperr.Error variable (err) before the switch, assign m, err inside
each case using StringMatcher/GlobMatcher/RegexMatcher, then after the switch
perform one conditional matcherCache.Store(origExpr, m) when err == nil; keep
existing negate handling, ExtractExpr call and the final ErrInvalidArguments
return unchanged.
In `@internal/route/rules/on.go`:
- Around line 508-660: The splitAnd/splitPipe functions duplicate logic already
implemented by forEachAndPart/forEachPipePart; refactor by re-implementing
splitAnd to build and return a []string by calling forEachAndPart (collect parts
into a slice inside the callback) and similarly implement splitPipe to call
forEachPipePart, removing the duplicated parsing code in splitAnd/splitPipe;
keep existing behaviors (trim spaces, skip empty parts, respect
escapes/quotes/brackets) and preserve function signatures for indexAnd/countAnd
usage elsewhere so only the internals change.
In `@internal/route/rules/parser_test.go`:
- Around line 95-98: The test includes an early-return branch that checks
tt.wantErr with expect.ErrorIs(t, tt.wantErr, err) but no outer test case
currently sets wantErr, so add at least one negative test case to exercise this
path: create a table entry in parser_test.go where you set tt.wantErr to the
expected parse error and provide an invalid or malformed input (e.g., malformed
rule string) that triggers the parser error; ensure the test uses the same tt
structure so expect.ErrorIs(t, tt.wantErr, err) is hit and validates the error.
In `@internal/route/rules/parser.go`:
- Around line 64-93: The current parser has two nearly identical passes to count
and then extract arguments (variables argCount, args, v, i, start) which
duplicates logic; simplify by replacing both passes with a single loop that
scans v from i, skips spaces, captures start/end substrings and appends them
directly to args (letting Go grow the slice) and remove the separate argCount
pre-scan and the separate allocation; keep the early-return when no args found
by checking if args is empty after the single pass and return subject, nil, nil,
true as before.
In `@internal/route/rules/rules.go`:
- Around line 297-304: The code currently rebuilds the combined preRules slice
and its tracking slices on every request inside the returned handler closure;
move the one-time construction of the combined preRules slice (append of
*defaultRule and nonDefaultRules) into the BuildHandler closure's setup (before
returning the func) so preRules is computed once, and keep only the per-request
allocations for executedPre and terminatedInPre inside the returned handler;
update references to preRules, defaultRule, and nonDefaultRules accordingly so
the handler uses the prebuilt preRules while still allocating executedPre and
terminatedInPre per request.
- Around line 159-189: The Parse method redundantly re-runs parseBlockRules when
hasTopLevelLBrace(config) is true: call parseBlockRules once and if it returns
an error, return that error immediately instead of falling through to YAML
parsing and re-invoking parseBlockRules. Modify Rules.Parse so that inside the
hasTopLevelLBrace(config) branch you capture the result of
parseBlockRules(config) and on err != nil return that err; otherwise assign
*rules = blockRules and return nil; keep the existing YAML fallback only for
cases where hasTopLevelLBrace(config) is false.
In `@internal/route/rules/scanner_test.go`:
- Around line 1-39: Add unit tests that cover nested braces, env var
substitutions, comments inside braces, and unmatched closing braces to
strengthen tokenizer coverage: create new tests using newTokenizer and call
scanToBrace, findMatchingBrace, and skipComments as appropriate—e.g., a test
asserting correct depth and matching position for "{ { inner } }" using
findMatchingBrace; a test with "{ ${VAR} }" ensuring env var sequences are
skipped and the outer brace matches; a test with "{ // comment\n code }"
verifying skipComments is invoked and findMatchingBrace returns the correct
closing position; and a test for scanToBrace where input like "cond }" returns
the specific unmatched-brace error—use require.Error/require.NoError and
require.Equal with strings.Index/LastIndex to assert positions.
In `@internal/route/rules/scanner.go`:
- Around line 27-85: The atLineStart flag isn't restored when a newline is
consumed in the whitespace branch of Tokenizer.skipComments; change the
unicode.IsSpace branch to detect '\n' and set atLineStart = true and prevIsSpace
= true for newline characters, otherwise keep atLineStart = false and
prevIsSpace = true for other whitespace, so atLineStart is semantically correct
after blank lines.
- Around line 61-78: In findMatchingBrace, the inline block-comment handling
currently sets closed=false and will let the outer brace scan continue, causing
a later "unmatched '{'" instead of a specific unterminated comment error; change
the block that scans for "/* ... */" inside findMatchingBrace (the loop using
pos, closed, and t.src checks) to check closed immediately after the inner loop
and return ErrInvalidBlockSyntax.Withf("unterminated block comment") when not
closed (same behavior as skipComments), ensuring atLineStart/prevIsSpace
handling remains consistent before any continue/return.
- Around line 117-249: In Tokenizer.findMatchingBrace the atLineStart and
prevIsSpace flags are being set based on characters inside a quote and thus leak
into the post-quote context; when clearing quote (inside the quote-handling
block where quote is set to 0), reset atLineStart = false and prevIsSpace =
false so the state reflects the quote boundary rather than the last char inside
the quoted string; update the quote-handling branch around where quote is
cleared to explicitly set those flags before continuing.
In `@internal/route/rules/vars_dynamic.go`:
- Around line 67-96: Extract a small helper (e.g., ensureFormParsed(req
*http.Request) error) and call it from both VarForm.get and VarPostForm.get to
remove the duplicated req.Form == nil / req.ParseForm() boilerplate; keep the
existing behavior (return any parse error) and then proceed to call
getValueByKeyAtIndex(req.Form, ...) or getValueByKeyAtIndex(req.PostForm, ...)
after calling ensureFormParsed, leaving getKeyAndIndex and getValueByKeyAtIndex
unchanged.
In `@internal/route/rules/vars_test.go`:
- Around line 630-650: Update the TestExpandVars_NoRemotePort test to document
why both $remote_host and $remote_port are expected to be empty when
testRequest.RemoteAddr is set to "192.168.1.1" (no port): add a brief comment in
TestExpandVars_NoRemotePort explaining that ExpandVars uses net.SplitHostPort
(so without a colon-separated port both remote_host and remote_port will be
empty), referencing the testRequest.RemoteAddr setup and the assertions for
$remote_host and $remote_port to make this intentional behavior clear.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
goutilsinternal/api/v1/route/playground.gointernal/net/gphttp/README.mdinternal/route/rules/README.mdinternal/route/rules/block_parser.gointernal/route/rules/block_parser_bench_test.gointernal/route/rules/block_parser_test.gointernal/route/rules/check_on.gointernal/route/rules/command.gointernal/route/rules/do.gointernal/route/rules/do_blocks.gointernal/route/rules/do_blocks_test.gointernal/route/rules/do_log_test.gointernal/route/rules/do_set.gointernal/route/rules/do_set_test.gointernal/route/rules/errors.gointernal/route/rules/help.gointernal/route/rules/http_flow_block_test.gointernal/route/rules/http_flow_yaml_test.gointernal/route/rules/io.gointernal/route/rules/matcher.gointernal/route/rules/on.gointernal/route/rules/on_test.gointernal/route/rules/parser.gointernal/route/rules/parser_test.gointernal/route/rules/phase.gointernal/route/rules/rules.gointernal/route/rules/rules_test.gointernal/route/rules/scanner.gointernal/route/rules/scanner_test.gointernal/route/rules/template.gointernal/route/rules/validate.gointernal/route/rules/var_bench_test.gointernal/route/rules/vars.gointernal/route/rules/vars_dynamic.gointernal/route/rules/vars_test.go
💤 Files with no reviewable changes (1)
- internal/net/gphttp/README.md
The default rule should runs only when no non-default pre rule matches, instead of running first as a baseline. This follows the old behavior as before the pr is established.: - Default rules act as fallback handlers that execute only when no matching non-default rule exists in the pre phase - IfElseBlockCommand now returns early when a condition matches with a nil Do block, instead of falling through to else blocks - Add nil check for auth handler to allow requests when no auth is configured - Fix unterminated environment variable parsing to preserve input Updates tests to verify the new fallback behavior where special rules suppress default rule execution.
There was a problem hiding this comment.
Actionable comments posted: 5
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)internal/route/rules/do_log_test.go (1)
73-109:⚠️ Potential issue | 🟠 MajorData race:
bytes.Bufferis not goroutine-safe with async logging.
bytes.Bufferprovides no synchronization.require.Eventuallyruns its condition function in a separate goroutine that readsstdoutBuf.String()/stderrBuf.String(), while the (buffered) logger writes to those same buffers from the logging goroutine. The commit note "Buffered log output before writing to stdout/stderr" confirms the write is asynchronous, making this a real data race detectable by-race.Use a goroutine-safe writer in the test (e.g. wrap with
sync.Mutex) instead of a barebytes.Buffer.🛡️ Proposed fix
+type syncBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *syncBuffer) Write(p []byte) (n int, err error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *syncBuffer) Close() error { return nil } + +func (b *syncBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +}Then replace the test scaffolding:
-var stdoutBuf bytes.Buffer -var stderrBuf bytes.Buffer -stdout = noopWriteCloser{&stdoutBuf} -stderr = noopWriteCloser{&stderrBuf} +stdoutBuf := &syncBuffer{} +stderrBuf := &syncBuffer{} +stdout = stdoutBuf +stderr = stderrBufand update the
require.Eventually/assertreads to callstdoutBuf.String()/stderrBuf.String()on the new type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/do_log_test.go` around lines 73 - 109, TestLogCommand_StdoutAndStderr uses plain bytes.Buffer (stdoutBuf, stderrBuf) and races with the async logger; replace those buffers with a goroutine-safe writer type (e.g., a small wrapper around bytes.Buffer that embeds a sync.Mutex and exposes Write and String methods) and assign noopWriteCloser{&safeBuf} to stdout and stderr instead of noopWriteCloser{&stdoutBuf}/noopWriteCloser{&stderrBuf}; update all reads in require.Eventually and subsequent assert/Count calls to use the safe writer's String() method (keeping noopWriteCloser and handler usage the same) so concurrent writes/reads are synchronized and the -race is eliminated.internal/route/rules/README.md (1)
84-112:⚠️ Potential issue | 🟡 MinorMermaid class diagram still reflects the old pre-refactor API.
The diagram at lines 84–112 uses the removed fields and interfaces:
Diagram (old) Current code RuleOn.isResponseChecker boolRuleOn.phase PhaseFlagCommand.exec CommandHandlerCommand.pre Commands/Command.post CommandsCommand.isResponseHandler boolremoved Checker.Check(r *http.Request) boolCheck(w *ResponseModifier, r *http.Request) boolCommandHandler.Execute(w, r, rm)CommandHandler.ServeHTTP(w *ResponseModifier, r *http.Request, upstream http.HandlerFunc) errorThis diagram will mislead anyone consulting the architecture docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/README.md` around lines 84 - 112, The Mermaid class diagram is outdated and must be updated to match the refactor: replace RuleOn's removed isResponseChecker with phase PhaseFlag; replace Command's single exec/CommandHandler with pre Commands and post Commands (remove isResponseHandler); update Checker to expose Check(w *ResponseModifier, r *http.Request) bool instead of Check(r *http.Request) bool; update CommandHandler to use ServeHTTP(w *ResponseModifier, r *http.Request, upstream http.HandlerFunc) error instead of Execute(...); adjust the class names and relationships so RuleOn --> Checker, Command --> Commands (pre/post), and Command --> CommandHandler reflect the current API surface (use PhaseFlag, ResponseModifier, Commands, ServeHTTP, upstream handler).
♻️ Duplicate comments (1)
internal/route/rules/on.go (1)
14-25:checker == nil → truecorrectly fixes the default-rule matching issue from the prior review.The prior review's concern about
OnDefaultnever matching for YAML-parsed rules is now resolved both here (nil guard at line 21 returnstrue) and in theOnDefaultbuilder (line 70 returnstrue).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/on.go` around lines 14 - 25, The RuleOn.Check method must treat a nil Checker as a matching default; ensure the nil-guard in RuleOn.Check returns true (as implemented) and that the OnDefault builder function also returns true for default YAML-parsed rules; verify the logic in RuleOn.Check, the Checker interface usage, and the OnDefault builder to keep consistent behavior where checker == nil implies a match.
🧹 Nitpick comments (3)
internal/route/rules/do_blocks.go (1)
276-278:forloop is effectively anif— the update expression is unreachable.The body always
returns on the first iteration, soq2 = skipSameLineSpace(src, q2)is never evaluated. This reads like a loop but behaves as a guard.♻️ Proposed simplification
-for q2 := skipSameLineSpace(src, p); q2 < length && src[q2] != '\n'; q2 = skipSameLineSpace(src, q2) { - return nil, 0, ErrInvalidBlockSyntax.Withf("unexpected token after else block") -} +if q2 := skipSameLineSpace(src, p); q2 < length && src[q2] != '\n' { + return nil, 0, ErrInvalidBlockSyntax.Withf("unexpected token after else block") +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/do_blocks.go` around lines 276 - 278, The loop in do_blocks.go behaves like an if because its body always returns, making the update expression unreachable; replace the for loop with a single conditional check: call skipSameLineSpace once into q2, then if q2 < length && src[q2] != '\n' return the ErrInvalidBlockSyntax.Withf(...) error. Update the code that uses skipSameLineSpace and ErrInvalidBlockSyntax to reflect the new if-style guard (remove the loop and its unreachable update).internal/route/rules/on.go (2)
662-687: Two-pass counting inParseandparseOnis unnecessary.Both
Parse(here) andparseOn(lines 697–724) walk the input twice — first to count items for slice pre-allocation, then to process them. The pre-count is only a capacity hint;appendhandles growth automatically. The same double-pass pattern repeats inparseOnforforEachPipePart.♻️ Single-pass Parse
func (on *RuleOn) Parse(v string) error { on.raw = v - - ruleCount := 0 - forEachAndPart(v, func(_ string) { - ruleCount++ - }) - checkAnd := make(CheckMatchAll, 0, ruleCount) + checkAnd := make(CheckMatchAll, 0, 4) errs := gperr.NewBuilder("rule.on syntax errors") - i := 0 + lineNum := 0 forEachAndPart(v, func(rule string) { - i++ + lineNum++ parsed, phase, err := parseOn(rule) if err != nil { - errs.AddSubjectf(err, "line %d", i) + errs.AddSubjectf(err, "line %d", lineNum) return } on.phase |= phase checkAnd = append(checkAnd, parsed) }) on.checker = checkAnd return errs.Error() }♻️ Single-pass parseOn (OR branch)
func parseOn(line string) (Checker, PhaseFlag, error) { - orCount := 0 - forEachPipePart(line, func(_ string) { - orCount++ - }) - if orCount > 1 { + var checkOr CheckMatchSingle + forEachPipePart(line, func(_ string) { checkOr = append(checkOr, nil) }) + if len(checkOr) > 1 { var phase PhaseFlag errs := gperr.NewBuilder("rule.on syntax errors") - checkOr := make(CheckMatchSingle, orCount) - i := 0 + checkOr = checkOr[:0] + idx := 0 forEachPipePart(line, func(or string) { - i++ + idx++ checkFunc, req, err := parseOnAtom(or) if err != nil { - errs.AddSubjectf(err, "or[%d]", i) + errs.AddSubjectf(err, "or[%d]", idx) return } - checkOr[i-1] = checkFunc + checkOr = append(checkOr, checkFunc) phase |= req })Alternatively, just drop the pre-count and use
make(CheckMatchSingle, 0, 4)directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/on.go` around lines 662 - 687, The Parse method and parseOn function both do an unnecessary two-pass walk (using forEachAndPart and forEachPipePart) just to pre-count for slice capacity; remove the initial counting loops and pre-allocation based on counts, and instead build slices in a single pass by initializing with a sensible default capacity (e.g. make(CheckMatchAll, 0, 4) or zero length) and appending as you parse inside the existing forEach... callbacks; update Parse (the Parse method on RuleOn), parseOn, and any similar two-pass logic to only iterate once and rely on append growth, keeping existing uses of on.raw, on.phase, on.checker, CheckMatchAll, CheckMatchSingle, forEachAndPart and forEachPipePart unchanged otherwise.
527-611:splitPipeduplicates the escape/quote/bracket state machine fromforEachPipePart, butsplitAndandsplitPipeare not dead code.Both functions are actively used in
matcherSignature()inrules.goand are tested inon_internal_test.go. However,splitPipeandforEachPipePartmaintain identical escape/quote/bracket tracking logic, which violates DRY. BecauseforEachPipePartuses a callback pattern whilesplitPipereturns a slice, extracting the shared state machine into a common helper function would better serve both use cases than simple delegation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/on.go` around lines 527 - 611, splitPipe duplicates the escape/quote/bracket state machine already implemented in forEachPipePart; extract that shared parsing state machine into a single helper (e.g., iteratePipeParts or parsePipeParts) that accepts the input string and a callback to be invoked for each part, then refactor forEachPipePart to call the helper directly and reimplement splitPipe by calling the helper and collecting parts into a slice; update usages in matcherSignature (rules.go) and ensure tests in on_internal_test.go still pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/route/rules/http_flow_yaml_test.go`:
- Around line 893-923: The test
TestHTTPFlow_PreTermination_SkipsLaterPreCommands_ButRunsPostOnlyAndPostMatchersYAML
uses the confusing constant "should-not-run" as the header value and then
asserts the header exists, which obscures intent; update the YAML rule value for
the X-Late header (in the test's parseRules string where it says `set
resp_header X-Late should-not-run`) to something explicit like "post-ran" and
change the corresponding assertion (the assert.Equal that compares
w.Header().Get("X-Late")) to expect "post-ran" so the test clearly shows the
post-phase command ran.
In `@internal/route/rules/on.go`:
- Around line 657-659: forEachPipePart can invoke fn("") for trailing
whitespace-only segments; update forEachPipePart to trim the slice (using
strings.TrimSpace) and skip calling fn when the trimmed part is empty (mirror
the behavior in forEachAndPart) so empty segments aren't propagated to
parseOnAtom; locate forEachPipePart and add a guard like the one used in
forEachAndPart around the fn(strings.TrimSpace(...)) call.
In `@internal/route/rules/rules.go`:
- Around line 105-116: doesTerminateInPre currently only checks commands that
assert to Handler and misses block commands (IfBlockCommand, IfElseBlockCommand)
that may contain terminating Handlers; update doesTerminateInPre to type-switch
on cmd and, for IfBlockCommand and IfElseBlockCommand, inspect their nested
command lists (e.g., then/else bodies) and recursively check contained Handlers'
Terminates() to detect termination inside blocks so Validate() can correctly
flag dead rules. Ensure you reference and handle both IfBlockCommand and
IfElseBlockCommand and preserve existing behavior for plain Handler commands.
- Around line 159-189: In Parse(), avoid retrying block parsing if it was
already attempted and ensure the returned error is the more informative one:
introduce a flag (e.g., blockTried) and a variable to capture the initial
parseBlockRules error when hasTopLevelLBrace(config) is true; set blockTried =
true and store blockErr when the first parseBlockRules call fails, then attempt
YAML; if YAML succeeds use serialization.ConvertSlice, if YAML fails and
blockTried is false try parseBlockRules once and return its result, but if
blockTried is true return the yaml error (yamlErr) instead of re-running
parseBlockRules or returning the earlier block error. This references Parse,
hasTopLevelLBrace, parseBlockRules, yaml.Unmarshal, and
serialization.ConvertSlice.
- Around line 421-425: Remove the fragile //go:linkname usages for
errStreamClosed and errClientDisconnected and instead detect HTTP/2 errors via
errors.As checks against the exported types: use errors.As to check for
*http2.StreamError and then compare ErrCode to http2.ErrCodeStreamClosed for
stream-level cases, and use errors.As to check for http2.ConnectionError (or
*http2.ConnectionError if that’s the concrete type) for connection-level cases;
update any branches that reference errStreamClosed/errClientDisconnected to use
these exported type checks (e.g., replace references to errStreamClosed with a
StreamError match + ErrCode == http2.ErrCodeStreamClosed, and replace
errClientDisconnected with a ConnectionError match).
---
Outside diff comments:
In `@internal/route/rules/do_log_test.go`:
- Around line 73-109: TestLogCommand_StdoutAndStderr uses plain bytes.Buffer
(stdoutBuf, stderrBuf) and races with the async logger; replace those buffers
with a goroutine-safe writer type (e.g., a small wrapper around bytes.Buffer
that embeds a sync.Mutex and exposes Write and String methods) and assign
noopWriteCloser{&safeBuf} to stdout and stderr instead of
noopWriteCloser{&stdoutBuf}/noopWriteCloser{&stderrBuf}; update all reads in
require.Eventually and subsequent assert/Count calls to use the safe writer's
String() method (keeping noopWriteCloser and handler usage the same) so
concurrent writes/reads are synchronized and the -race is eliminated.
In `@internal/route/rules/README.md`:
- Around line 84-112: The Mermaid class diagram is outdated and must be updated
to match the refactor: replace RuleOn's removed isResponseChecker with phase
PhaseFlag; replace Command's single exec/CommandHandler with pre Commands and
post Commands (remove isResponseHandler); update Checker to expose Check(w
*ResponseModifier, r *http.Request) bool instead of Check(r *http.Request) bool;
update CommandHandler to use ServeHTTP(w *ResponseModifier, r *http.Request,
upstream http.HandlerFunc) error instead of Execute(...); adjust the class names
and relationships so RuleOn --> Checker, Command --> Commands (pre/post), and
Command --> CommandHandler reflect the current API surface (use PhaseFlag,
ResponseModifier, Commands, ServeHTTP, upstream handler).
---
Duplicate comments:
In `@internal/route/rules/on.go`:
- Around line 14-25: The RuleOn.Check method must treat a nil Checker as a
matching default; ensure the nil-guard in RuleOn.Check returns true (as
implemented) and that the OnDefault builder function also returns true for
default YAML-parsed rules; verify the logic in RuleOn.Check, the Checker
interface usage, and the OnDefault builder to keep consistent behavior where
checker == nil implies a match.
---
Nitpick comments:
In `@internal/route/rules/do_blocks.go`:
- Around line 276-278: The loop in do_blocks.go behaves like an if because its
body always returns, making the update expression unreachable; replace the for
loop with a single conditional check: call skipSameLineSpace once into q2, then
if q2 < length && src[q2] != '\n' return the ErrInvalidBlockSyntax.Withf(...)
error. Update the code that uses skipSameLineSpace and ErrInvalidBlockSyntax to
reflect the new if-style guard (remove the loop and its unreachable update).
In `@internal/route/rules/on.go`:
- Around line 662-687: The Parse method and parseOn function both do an
unnecessary two-pass walk (using forEachAndPart and forEachPipePart) just to
pre-count for slice capacity; remove the initial counting loops and
pre-allocation based on counts, and instead build slices in a single pass by
initializing with a sensible default capacity (e.g. make(CheckMatchAll, 0, 4) or
zero length) and appending as you parse inside the existing forEach...
callbacks; update Parse (the Parse method on RuleOn), parseOn, and any similar
two-pass logic to only iterate once and rely on append growth, keeping existing
uses of on.raw, on.phase, on.checker, CheckMatchAll, CheckMatchSingle,
forEachAndPart and forEachPipePart unchanged otherwise.
- Around line 527-611: splitPipe duplicates the escape/quote/bracket state
machine already implemented in forEachPipePart; extract that shared parsing
state machine into a single helper (e.g., iteratePipeParts or parsePipeParts)
that accepts the input string and a callback to be invoked for each part, then
refactor forEachPipePart to call the helper directly and reimplement splitPipe
by calling the helper and collecting parts into a slice; update usages in
matcherSignature (rules.go) and ensure tests in on_internal_test.go still pass.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/route/rules/README.mdinternal/route/rules/block_parser.gointernal/route/rules/do.gointernal/route/rules/do_blocks.gointernal/route/rules/do_blocks_test.gointernal/route/rules/do_log_test.gointernal/route/rules/http_flow_block_test.gointernal/route/rules/http_flow_yaml_test.gointernal/route/rules/on.gointernal/route/rules/rules.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/route/rules/http_flow_block_test.go
Comment on lines
+893
to
+923
| func TestHTTPFlow_PreTermination_SkipsLaterPreCommands_ButRunsPostOnlyAndPostMatchersYAML(t *testing.T) { | ||
| upstreamCalled := false | ||
| upstream := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| upstreamCalled = true | ||
| w.WriteHeader(http.StatusOK) | ||
| w.Write([]byte("upstream")) | ||
| }) | ||
|
|
||
| var rules Rules | ||
| err := parseRules(` | ||
| - on: path / | ||
| do: error 403 blocked | ||
| - on: path / | ||
| do: set resp_header X-Late should-not-run | ||
| - on: status 4xx | ||
| do: set resp_header X-Post true | ||
| `, &rules) | ||
| require.NoError(t, err) | ||
|
|
||
| handler := rules.BuildHandler(upstream) | ||
|
|
||
| req := httptest.NewRequest(http.MethodGet, "/", nil) | ||
| w := httptest.NewRecorder() | ||
| handler.ServeHTTP(w, req) | ||
|
|
||
| assert.False(t, upstreamCalled) | ||
| assert.Equal(t, 403, w.Code) | ||
| assert.Equal(t, "blocked\n", w.Body.String()) | ||
| assert.Equal(t, "should-not-run", w.Header().Get("X-Late")) | ||
| assert.Equal(t, "true", w.Header().Get("X-Post")) | ||
| } |
There was a problem hiding this comment.
Misleading assertion value obscures test intent.
Line 921 asserts assert.Equal(t, "should-not-run", w.Header().Get("X-Late")) — this is asserting the post-phase command did run. The constant "should-not-run" as the expected value implies the header shouldn't be set, which is the opposite of what the test verifies. The rule set resp_header X-Late should-not-run has no pre-commands, so its post-command runs even after pre-phase termination. A value like "post-ran" would make the intent clear.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/route/rules/http_flow_yaml_test.go` around lines 893 - 923, The test
TestHTTPFlow_PreTermination_SkipsLaterPreCommands_ButRunsPostOnlyAndPostMatchersYAML
uses the confusing constant "should-not-run" as the header value and then
asserts the header exists, which obscures intent; update the YAML rule value for
the X-Late header (in the test's parseRules string where it says `set
resp_header X-Late should-not-run`) to something explicit like "post-ran" and
change the corresponding assertion (the assert.Equal that compares
w.Header().Get("X-Late")) to expect "post-ran" so the test clearly shows the
post-phase command ran.
Comment on lines
421
to
+425
| //go:linkname errStreamClosed golang.org/x/net/http2.errStreamClosed | ||
| var errStreamClosed error | ||
|
|
||
| func logError(err error, r *http.Request) { | ||
| if errors.Is(err, errStreamClosed) { | ||
| return | ||
| //go:linkname errClientDisconnected golang.org/x/net/http2.errClientDisconnected | ||
| var errClientDisconnected error |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full context of the file and how these symbols are used
cd internal/route/rules
wc -l rules.goRepository: yusing/godoxy
Length of output: 69
🏁 Script executed:
# Search for usages of errStreamClosed and errClientDisconnected
rg -n "errStreamClosed|errClientDisconnected" --type goRepository: yusing/godoxy
Length of output: 513
🏁 Script executed:
# Check the broader context around lines 421-425 and after
head -n 450 internal/route/rules/rules.go | tail -n 50Repository: yusing/godoxy
Length of output: 1310
🌐 Web query:
golang.org/x/net/http2 error handling StreamError public API
💡 Result:
In golang.org/x/net/http2, StreamError is a public (exported) error type intended for callers to inspect when an error affects only a single HTTP/2 stream (not the whole connection). It has these exported fields: StreamID uint32, Code ErrCode, and optional Cause error. [1]
How to handle it (client side)
When a request fails (e.g., from (*http2.Transport).RoundTrip, (*http2.ClientConn).RoundTrip, etc.), unwrap and type-check:
var se http2.StreamError
if errors.As(err, &se) {
// Per-stream failure (often corresponds to a received/sent RST_STREAM)
// se.Code is the HTTP/2 error code; se.StreamID identifies the stream.
}se.Code is an http2.ErrCode (the HTTP/2 spec error code enum). [1]
Distinguish from connection-level errors
If the entire HTTP/2 connection is being torn down, you’ll typically see http2.ConnectionError instead (it’s documented as “termination of the entire connection”). Handle similarly with errors.As. [2]
If you’re using net/http instead of x/net/http2
The stdlib’s HTTP/2 support is mostly internal; you generally won’t reliably get x/net/http2.StreamError as part of a stable public API surface unless you’re using the x/net/http2 types directly (its docs explicitly describe it as “low-level”). [1]
Sources:
[1] golang.org/x/net/http2 package docs (type StreamError, ErrCode, overview) https://pkg.go.dev/golang.org/x/net/http2
[2] golang.org/x/net/http2 package docs (type ConnectionError) https://pkg.go.dev/golang.org/x/net/http2
🏁 Script executed:
# Check if there's any documentation or comments about why these symbols were used
rg -n "errStreamClosed|errClientDisconnected|stream.*closed|client.*disconnect" --type go -C 2 -iRepository: yusing/godoxy
Length of output: 1605
🏁 Script executed:
# Look for http2 imports and how they're used elsewhere in the codebase
rg -n "http2\." --type go | head -30Repository: yusing/godoxy
Length of output: 597
🏁 Script executed:
# Check go.mod for x/net version constraints
rg -n "golang.org/x/net" --type go-modRepository: yusing/godoxy
Length of output: 87
🏁 Script executed:
# Look for any recent commits or comments explaining the linkname usage
git log --oneline --all -- internal/route/rules/rules.go | head -10Repository: yusing/godoxy
Length of output: 750
🏁 Script executed:
# Check if there are any tests that verify these error conditions
fd -e "test.go" -e "_test.go" | xargs rg -l "isUnexpectedError|errStreamClosed|errClientDisconnected" 2>/dev/nullRepository: yusing/godoxy
Length of output: 86
🏁 Script executed:
# Look at the specific commit that added these linknames
git show 0f78158:internal/route/rules/rules.go | grep -A 10 -B 5 "errStreamClosed\|errClientDisconnected" || echo "Not in that commit"Repository: yusing/godoxy
Length of output: 528
🏁 Script executed:
# Check all occurrences to understand the exact purpose
rg -n "errStreamClosed|errClientDisconnected" --type go -B 5 -A 10Repository: yusing/godoxy
Length of output: 1471
🏁 Script executed:
# Check if go.mod exists and its location
find . -name "go.mod" -type fRepository: yusing/godoxy
Length of output: 205
🏁 Script executed:
# Look at that file
cat go.mod | grep -A 2 "golang.org/x/net"Repository: yusing/godoxy
Length of output: 243
🏁 Script executed:
# Look more closely at what ConnectionError is and could it replace errClientDisconnected
rg -n "ConnectionError" --type goRepository: yusing/godoxy
Length of output: 133
🌐 Web query:
golang.org/x/net/http2 ConnectionError public error type
💡 Result:
Yes. In golang.org/x/net/http2, ConnectionError is an exported (public) error type:
type ConnectionError ErrCodeIt implements error via:
func (e ConnectionError) Error() stringand is used for errors that terminate the entire HTTP/2 connection (e.g., it may be returned by (*Framer).ReadFrame). (pkg.go.dev)
To detect it:
var ce http2.ConnectionError
if errors.As(err, &ce) {
// ce is the HTTP/2 ErrCode (castable) that caused connection termination
}
``` ([pkg.go.dev](https://pkg.go.dev/golang.org/x/net/http2?utm_source=openai))
Citations:
- 1: https://pkg.go.dev/golang.org/x/net/http2?utm_source=openai
- 2: https://pkg.go.dev/golang.org/x/net/http2?utm_source=openai
---
</details>
**Remove fragile `//go:linkname` directives and use only public HTTP/2 error types.**
The `//go:linkname` references to `errStreamClosed` and `errClientDisconnected` are brittle and unnecessary. The same file already uses public equivalents:
- `http2.StreamError` with `http2.ErrCodeStreamClosed` for stream-level errors
- `http2.ConnectionError` is available for connection-level errors from the public API
Refactor lines 421-425 to remove these linknames and rely exclusively on `errors.As()` checks against the exported error types. This eliminates the dependency on unexported symbols that can change silently across `x/net` updates.
<details>
<summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @internal/route/rules/rules.go around lines 421 - 425, Remove the fragile
//go:linkname usages for errStreamClosed and errClientDisconnected and instead
detect HTTP/2 errors via errors.As checks against the exported types: use
errors.As to check for *http2.StreamError and then compare ErrCode to
http2.ErrCodeStreamClosed for stream-level cases, and use errors.As to check for
http2.ConnectionError (or *http2.ConnectionError if that’s the concrete type)
for connection-level cases; update any branches that reference
errStreamClosed/errClientDisconnected to use these exported type checks (e.g.,
replace references to errStreamClosed with a StreamError match + ErrCode ==
http2.ErrCodeStreamClosed, and replace errClientDisconnected with a
ConnectionError match).
</details>
<!-- fingerprinting:phantom:poseidon:churro -->
<!-- This is an auto-generated comment by CodeRabbit -->
Refactors the termination detection in the rules DSL to properly handle if-block and if-else-block commands. Adds new functions `commandsTerminateInPre`, `commandTerminatesInPre`, and `ifElseBlockTerminatesInPre` to recursively check if command sequences terminate in the pre-phase. Also improves the Parse function to try block syntax first with proper error handling and fallback to YAML. Includes test cases for dead code detection with terminating handlers in conditional blocks.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/route/rules/rules.go (2)
465-489:⚠️ Potential issue | 🟠 MajorRemove //go:linkname usage for HTTP/2 errors.
Linking to unexportedx/net/http2symbols is brittle across version bumps and breaks the public‑API boundary. You can detect stream/connection closure with exportedhttp2.StreamError/http2.ConnectionErrorinstead.🔧 Suggested fix
-//go:linkname errStreamClosed golang.org/x/net/http2.errStreamClosed -var errStreamClosed error - -//go:linkname errClientDisconnected golang.org/x/net/http2.errClientDisconnected -var errClientDisconnected error - func isUnexpectedError(err error) bool { - if errors.Is(err, errStreamClosed) || errors.Is(err, errClientDisconnected) { - return false - } - if h2Err, ok := errors.AsType[http2.StreamError](err); ok { - // ignore these errors - if h2Err.Code == http2.ErrCodeStreamClosed { - return false - } - } + var se http2.StreamError + if errors.As(err, &se) && se.Code == http2.ErrCodeStreamClosed { + return false + } + var ce http2.ConnectionError + if errors.As(err, &ce) { + return false + } if h3Err, ok := errors.AsType[*http3.Error](err); ok { // ignore these errors switch h3Err.ErrorCode { case http3.ErrCodeNoError, http3.ErrCodeRequestCanceled: return false } } return true }golang.org/x/net/http2 StreamError ConnectionError ErrCodeStreamClosed exported types🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/rules.go` around lines 465 - 489, Remove the //go:linkname hacks and the errStreamClosed/errClientDisconnected vars; in isUnexpectedError use errors.As to test for http2.StreamError and http2.ConnectionError (check their Code fields against http2.ErrCodeStreamClosed / connection-close codes as appropriate) instead of errors.Is against linked symbols; keep the existing http3 checks intact. This replaces the unexported-link checks with exported types: isUnexpectedError, http2.StreamError, http2.ConnectionError, and http2.ErrCodeStreamClosed.
193-233: Parse returns block errors even for YAML‑looking inputs.
WhenhasTopLevelLBraceis false and YAML unmarshal fails, the function still returnsblockErr, which can hide the YAML error message. Consider returningyamlErrin that path for clearer diagnostics.🔧 Suggested adjustment
// If YAML fails and we haven't tried block syntax yet, try it now. if !blockTried { blockRules, err := parseBlockRules(config) if err == nil { *rules = blockRules return nil } - blockErr = err + return yamlErr } return blockErr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/rules.go` around lines 193 - 233, The Parse function can return blockErr even when YAML parsing actually failed first; update the final error-return logic in Rules.Parse to prefer the YAML error when YAML unmarshal failed and block syntax wasn't already tried. Concretely: in Parse (referencing hasTopLevelLBrace, blockTried, blockErr, yamlErr and parseBlockRules), after the YAML unmarshal fails and you try parseBlockRules only because blockTried was false, if parseBlockRules also fails return yamlErr (the original YAML error) instead of blockErr so callers see the YAML diagnostic; keep returning blockErr in the cases where block parsing was attempted earlier and YAML wasn't the primary failure.
🧹 Nitpick comments (2)
internal/route/rules/http_flow_yaml_test.go (1)
252-259: This YAML‑named test is still using block DSL.
TestHTTPFlow_ResponseRuleWithStatusConditionYAMLfeeds block‑syntax braces, so it doesn’t exercise the YAML parser. Consider expressing the rules as YAML list items to keep coverage aligned with the test name.🔧 Example YAML conversion
- err := parseRules(fmt.Sprintf(` - status 4xx { - log error %s "$req_url returned $status_code" - } - status 200 { - log info %s "$req_url returned $status_code" - } -`, errorLog, infoLog), &rules) + err := parseRules(fmt.Sprintf(` +- name: log-4xx + on: status 4xx + do: log error %s "$req_url returned $status_code" +- name: log-200 + on: status 200 + do: log info %s "$req_url returned $status_code" +`, errorLog, infoLog), &rules)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/http_flow_yaml_test.go` around lines 252 - 259, The test TestHTTPFlow_ResponseRuleWithStatusConditionYAML is still passing block-DSL text to parseRules, so update the test to provide actual YAML content instead of the brace-based DSL: replace the formatted string passed to parseRules with a YAML document (e.g., a list of rules) that represents the same status 4xx and status 200 rules and their log entries, ensuring the YAML is valid and still uses the same errorLog and infoLog variables; keep the call to parseRules(...) and the rules variable intact so the test exercises the YAML parser.internal/route/rules/http_flow_block_test.go (1)
1276-1308: Rename X‑Late value to reflect that the post‑only rule runs.
The test asserts the header is present, so"should-not-run"reads backwards and can confuse intent.🔧 Suggested tweak
path / { error 403 blocked } path / { - set resp_header X-Late should-not-run + set resp_header X-Late should-run } status 4xx { set resp_header X-Post true } @@ - assert.Equal(t, "should-not-run", w.Header().Get("X-Late")) + assert.Equal(t, "should-run", w.Header().Get("X-Late"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/http_flow_block_test.go` around lines 1276 - 1308, The test TestHTTPFlow_PreTermination_SkipsLaterPreCommands_ButRunsPostOnlyAndPostMatchers currently sets the response header X-Late to "should-not-run" which contradicts the assertion that the header should be present; update the rule value for the header in the parsed rules (the "set resp_header X-Late ..." line) to a name that reflects it ran (e.g., "late" or "post-only-ran") and update the corresponding assertion assert.Equal(t, "<new-value>", w.Header().Get("X-Late")) so the test intent and header value align.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/route/rules/on.go`:
- Around line 605-608: splitPipe currently appends the final trimmed segment
even when it's empty (e.g., "a | "), causing inconsistency with forEachPipePart
and making matcherSignature comparisons fail; change the final append logic in
splitPipe (the block that trims s[start:] into result) to trim the segment into
a temp string and only append it to result if that trimmed string is non-empty
so empty pipe segments are skipped just like forEachPipePart.
In `@internal/route/rules/rules.go`:
- Around line 155-163: matcherSignature currently returns ("", false) for empty
raw, causing Validate() to skip shadowing checks for unconditional terminating
rules; change matcherSignature so that when raw is empty it returns a canonical
non-empty signature (the same canonical form used/expected for ruleOnAlwaysTrue)
and true instead of ("", false) so unconditional rules are represented and
participate in validation/shadowing detection; update any test expectations or
callers of matcherSignature to accept the canonical signature for
empty/raw-always-true inputs.
---
Duplicate comments:
In `@internal/route/rules/rules.go`:
- Around line 465-489: Remove the //go:linkname hacks and the
errStreamClosed/errClientDisconnected vars; in isUnexpectedError use errors.As
to test for http2.StreamError and http2.ConnectionError (check their Code fields
against http2.ErrCodeStreamClosed / connection-close codes as appropriate)
instead of errors.Is against linked symbols; keep the existing http3 checks
intact. This replaces the unexported-link checks with exported types:
isUnexpectedError, http2.StreamError, http2.ConnectionError, and
http2.ErrCodeStreamClosed.
- Around line 193-233: The Parse function can return blockErr even when YAML
parsing actually failed first; update the final error-return logic in
Rules.Parse to prefer the YAML error when YAML unmarshal failed and block syntax
wasn't already tried. Concretely: in Parse (referencing hasTopLevelLBrace,
blockTried, blockErr, yamlErr and parseBlockRules), after the YAML unmarshal
fails and you try parseBlockRules only because blockTried was false, if
parseBlockRules also fails return yamlErr (the original YAML error) instead of
blockErr so callers see the YAML diagnostic; keep returning blockErr in the
cases where block parsing was attempted earlier and YAML wasn't the primary
failure.
---
Nitpick comments:
In `@internal/route/rules/http_flow_block_test.go`:
- Around line 1276-1308: The test
TestHTTPFlow_PreTermination_SkipsLaterPreCommands_ButRunsPostOnlyAndPostMatchers
currently sets the response header X-Late to "should-not-run" which contradicts
the assertion that the header should be present; update the rule value for the
header in the parsed rules (the "set resp_header X-Late ..." line) to a name
that reflects it ran (e.g., "late" or "post-only-ran") and update the
corresponding assertion assert.Equal(t, "<new-value>", w.Header().Get("X-Late"))
so the test intent and header value align.
In `@internal/route/rules/http_flow_yaml_test.go`:
- Around line 252-259: The test TestHTTPFlow_ResponseRuleWithStatusConditionYAML
is still passing block-DSL text to parseRules, so update the test to provide
actual YAML content instead of the brace-based DSL: replace the formatted string
passed to parseRules with a YAML document (e.g., a list of rules) that
represents the same status 4xx and status 200 rules and their log entries,
ensuring the YAML is valid and still uses the same errorLog and infoLog
variables; keep the call to parseRules(...) and the rules variable intact so the
test exercises the YAML parser.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/api/v1/docs/swagger.jsoninternal/api/v1/docs/swagger.yamlinternal/api/v1/route/playground.gointernal/route/rules/http_flow_block_test.gointernal/route/rules/http_flow_yaml_test.gointernal/route/rules/on.gointernal/route/rules/rules.gointernal/route/rules/rules_test.go
💤 Files with no reviewable changes (2)
- internal/api/v1/docs/swagger.json
- internal/api/v1/route/playground.go
… requirement
Changes the nested block syntax detection from requiring `@`
as the first non-space character on a line to using a line-ending brace heuristic.
The parser now recognizes nested blocks when a line ends with an unquoted `{`,
simplifying the syntax and removing the mandatory `@` prefix while maintaining the same functionality.
This is a test in yaml_test which meant to be testing old YAML syntax instead of new DSL
The matcherSignature function now treats empty strings as unconditional rules that match any request, returning "(any)" as the signature instead of rejecting them. This enables proper dead code detection when an unconditional terminating rule shadows later rules. Adds test coverage for detecting dead rules caused by unconditional terminating rules.
…and syntax Fixes previous commit: 2a51c2e
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/route/rules/README.md (1)
84-112:⚠️ Potential issue | 🟡 MinorStale Mermaid class diagram contradicts the new data model.
The diagram still references the old types removed in this PR:
RuleOn.isResponseChecker bool→ should bephase PhaseFlagCommand.exec CommandHandler/isResponseHandler bool→ should bepre Commands/post CommandsCheckerinterface shows oldCheck(r)/CheckResponse(w, r)signatures → should beCheck(w *ResponseModifier, r *http.Request) boolCommandHandlerinterface is no longer the same abstractionThe "Exported Types" section at lines 38–49 is correct; this diagram should be updated to match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/README.md` around lines 84 - 112, The Mermaid class diagram is out of date: update RuleOn to replace the old isResponseChecker bool with phase PhaseFlag, change Command to expose pre Commands and post Commands instead of exec/isResponseHandler, replace the Checker interface methods with the new signature Check(w *ResponseModifier, r *http.Request) bool, and remove/replace the CommandHandler abstraction to reflect the new Commands type used in the exported types; ensure the diagram matches the "Exported Types" section (types RuleOn, PhaseFlag, Command with pre/post, Checker with Check(w *ResponseModifier, r *http.Request)) and adjust relationships accordingly.
♻️ Duplicate comments (1)
internal/route/rules/rules.go (1)
465-491://go:linknamedirectives for unexportedhttp2symbols are still present.These link to internal
golang.org/x/net/http2symbols (errStreamClosed,errClientDisconnected) that can silently break acrossx/netupdates. The publichttp2.StreamErrorcheck is already in place (line 475); a similar approach usingerrors.Asagainst exported types could replace the linkname directives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/rules.go` around lines 465 - 491, Remove the //go:linkname declarations for errStreamClosed and errClientDisconnected and stop relying on those unexported symbols in isUnexpectedError; instead, detect equivalent cases using exported/standard error types: use errors.As to check for http2.StreamError (in isUnexpectedError already but move it before other checks) and for *http3.Error, and use errors.Is against standard errors like context.Canceled, io.EOF or net.ErrClosed (as appropriate for your codebase) to cover client-disconnect/stream-closed cases; update the checks in isUnexpectedError to only use these exported types and standard sentinel errors and remove any references to errStreamClosed/errClientDisconnected.
🧹 Nitpick comments (5)
internal/route/rules/http_flow_block_test.go (1)
21-46: First test uses YAML syntax in the block test file.
TestHTTPFlow_BasicPreRulesuses YAML rule syntax (- name: ...), while the rest of the file uses block syntax. Consider using block syntax for consistency with the file's intent, or moving this test to the YAML test file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/http_flow_block_test.go` around lines 21 - 46, The test TestHTTPFlow_BasicPreRules currently passes a YAML list (starting with "- name: ...") to parseRules, which is inconsistent with this file's block-syntax tests; update the rules string passed to parseRules to use the same block syntax as other tests (e.g., remove the leading "-" and dedent so it is "name: add-header\non: path /\ndo: set header X-Custom-Header test-value") so parseRules, Rules, and rules.BuildHandler are exercised in the same style as the rest of the file.internal/route/rules/rules.go (1)
300-451:BuildHandlerphase execution is well-structured but has a subtle semantic concern with post-only default rules.The default rule is only evaluated in the pre-phase path (line 377). If a default rule is configured with a response-phase matcher (e.g.,
on: status 200), it would be skipped in the pre-phase (due toIsPostRule()) and also missed by the post-matcher loop at line 426 (which only iteratesnonDefaultRules). This would silently drop the rule.This is extremely unlikely in practice (a default rule with a response matcher is semantically odd), but it might be worth a defensive comment or a validation warning in
Validate().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/rules.go` around lines 300 - 451, BuildHandler can silently skip a default rule that is post-only (defaultRule with On.phase.IsPostRule()), so add a defensive validation in Validate(): detect rules where isDefaultRule(rule) is true and rule.On.phase.IsPostRule() (or equivalent phase check on the rule's On), and return an explicit validation error or warning (via Validate) explaining "default rules cannot be post-only" so such misconfigurations are caught early instead of being dropped at runtime; reference BuildHandler, isDefaultRule, defaultRule and the On.phase.IsPostRule() check when implementing the validation.internal/route/rules/on.go (2)
629-664: Mid-stream empty pipe parts can still reachfn("")inforEachPipePart.The trailing-part guard at line 660 was added (per prior review), but the mid-stream split at line 654 does not guard against empty segments. Input like
"a||b"would callfn(""). In practice,parseOnAtomwould return a parse error, but the inconsistency with the trailing guard is a latent footgun.🔧 Optional guard
case '|': if quote == 0 && brackets == 0 { - fn(strings.TrimSpace(s[start:i])) + if part := strings.TrimSpace(s[start:i]); part != "" { + fn(part) + } start = i + 1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/on.go` around lines 629 - 664, forEachPipePart currently calls fn(strings.TrimSpace(s[start:i])) for mid-stream '|' splits without checking for empty segments, allowing calls like fn("") for inputs such as "a||b"; update the mid-stream branch (the case '|' handling inside forEachPipePart) to trim the candidate part (e.g. strings.TrimSpace(s[start:i])) and only call fn(part) and advance start when part != "" (matching the trailing-part guard behavior), so empty segments are skipped before invoking fn.
269-316:OnProto"h1" check excludes HTTPS/1.1 — verify this is intended.
h1matchesr.TLS == nil && r.ProtoMajor == 1, meaning it only matches plaintext HTTP/1.x. Users expecting "h1" to mean "any HTTP/1.x regardless of TLS" would be surprised. The help text says(http, https, h1, h2, h2c, h3)but doesn't clarify thath1implies no TLS. Consider documenting this explicitly in the help description or the README.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/on.go` around lines 269 - 316, OnProto's builder currently treats the "h1" case as r.TLS == nil && r.ProtoMajor == 1 which excludes HTTPS/1.1; decide intended semantics and implement accordingly: if "h1" should mean any HTTP/1.x regardless of TLS, change the builder's "h1" branch in OnProto to only check r.ProtoMajor == 1 (remove the r.TLS == nil condition); if the current behavior is intended, update the OnProto help text (Help.args["proto"] or surrounding documentation) to explicitly state that "h1" means plaintext HTTP/1.x (not HTTPS/1.1) so callers are not surprised.internal/route/rules/do_blocks.go (1)
340-439:parseDoWithBlockshas a redundantlineEndrecalculation.Lines 403–406 compute
lineEndfor the brace heuristic check, but if the heuristic doesn't match, lines 420–423 recomputelineEndfrompos(instead of reusing the first one fromlinePos). Both reach the same\n, so the result is identical, but the second scan is redundant work.♻️ Minor optimization
if linePos < length && lineEndsWithUnquotedOpenBrace(src, linePos, lineEnd) { h, next, err := parseAtBlockChain(src, linePos) if err != nil { return nil, err } handlers = append(handlers, h) pos = next lineStart = false continue } // Not a nested block; parse the rest of this line as a command. - lineEnd = pos - for lineEnd < length && src[lineEnd] != '\n' { - lineEnd++ - } if lerr := appendLineCommand(src[pos:lineEnd]); lerr != nil { return nil, lerr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/do_blocks.go` around lines 340 - 439, The code in parseDoWithBlocks redundantly re-scans to compute lineEnd: you already compute lineEnd after finding linePos for the brace heuristic (variable names linePos and lineEnd); if lineEndsWithUnquotedOpenBrace(...) returns false, reuse that previously computed lineEnd for parsing the command instead of recomputing it from pos. Change the branch after the brace check to use the existing lineEnd when calling appendLineCommand(src[pos:lineEnd]) (and remove the second loop that recomputes lineEnd), keeping appendLineCommand, parseAtBlockChain and lineEndsWithUnquotedOpenBrace references intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/route/rules/http_flow_block_test.go`:
- Around line 1276-1308: Update the HTTP test so the late-response header value
matches the YAML variant: in
TestHTTPFlow_PreTermination_SkipsLaterPreCommands_ButRunsPostOnlyAndPostMatchers
change the rule `set resp_header X-Late should-not-run` to `set resp_header
X-Late should-run` and update the corresponding assertion that checks
w.Header().Get("X-Late") to expect "should-run" (references: test function
TestHTTPFlow_PreTermination_SkipsLaterPreCommands_ButRunsPostOnlyAndPostMatchers
and the rule line containing `set resp_header X-Late`).
---
Outside diff comments:
In `@internal/route/rules/README.md`:
- Around line 84-112: The Mermaid class diagram is out of date: update RuleOn to
replace the old isResponseChecker bool with phase PhaseFlag, change Command to
expose pre Commands and post Commands instead of exec/isResponseHandler, replace
the Checker interface methods with the new signature Check(w *ResponseModifier,
r *http.Request) bool, and remove/replace the CommandHandler abstraction to
reflect the new Commands type used in the exported types; ensure the diagram
matches the "Exported Types" section (types RuleOn, PhaseFlag, Command with
pre/post, Checker with Check(w *ResponseModifier, r *http.Request)) and adjust
relationships accordingly.
---
Duplicate comments:
In `@internal/route/rules/rules.go`:
- Around line 465-491: Remove the //go:linkname declarations for errStreamClosed
and errClientDisconnected and stop relying on those unexported symbols in
isUnexpectedError; instead, detect equivalent cases using exported/standard
error types: use errors.As to check for http2.StreamError (in isUnexpectedError
already but move it before other checks) and for *http3.Error, and use errors.Is
against standard errors like context.Canceled, io.EOF or net.ErrClosed (as
appropriate for your codebase) to cover client-disconnect/stream-closed cases;
update the checks in isUnexpectedError to only use these exported types and
standard sentinel errors and remove any references to
errStreamClosed/errClientDisconnected.
---
Nitpick comments:
In `@internal/route/rules/do_blocks.go`:
- Around line 340-439: The code in parseDoWithBlocks redundantly re-scans to
compute lineEnd: you already compute lineEnd after finding linePos for the brace
heuristic (variable names linePos and lineEnd); if
lineEndsWithUnquotedOpenBrace(...) returns false, reuse that previously computed
lineEnd for parsing the command instead of recomputing it from pos. Change the
branch after the brace check to use the existing lineEnd when calling
appendLineCommand(src[pos:lineEnd]) (and remove the second loop that recomputes
lineEnd), keeping appendLineCommand, parseAtBlockChain and
lineEndsWithUnquotedOpenBrace references intact.
In `@internal/route/rules/http_flow_block_test.go`:
- Around line 21-46: The test TestHTTPFlow_BasicPreRules currently passes a YAML
list (starting with "- name: ...") to parseRules, which is inconsistent with
this file's block-syntax tests; update the rules string passed to parseRules to
use the same block syntax as other tests (e.g., remove the leading "-" and
dedent so it is "name: add-header\non: path /\ndo: set header X-Custom-Header
test-value") so parseRules, Rules, and rules.BuildHandler are exercised in the
same style as the rest of the file.
In `@internal/route/rules/on.go`:
- Around line 629-664: forEachPipePart currently calls
fn(strings.TrimSpace(s[start:i])) for mid-stream '|' splits without checking for
empty segments, allowing calls like fn("") for inputs such as "a||b"; update the
mid-stream branch (the case '|' handling inside forEachPipePart) to trim the
candidate part (e.g. strings.TrimSpace(s[start:i])) and only call fn(part) and
advance start when part != "" (matching the trailing-part guard behavior), so
empty segments are skipped before invoking fn.
- Around line 269-316: OnProto's builder currently treats the "h1" case as r.TLS
== nil && r.ProtoMajor == 1 which excludes HTTPS/1.1; decide intended semantics
and implement accordingly: if "h1" should mean any HTTP/1.x regardless of TLS,
change the builder's "h1" branch in OnProto to only check r.ProtoMajor == 1
(remove the r.TLS == nil condition); if the current behavior is intended, update
the OnProto help text (Help.args["proto"] or surrounding documentation) to
explicitly state that "h1" means plaintext HTTP/1.x (not HTTPS/1.1) so callers
are not surprised.
In `@internal/route/rules/rules.go`:
- Around line 300-451: BuildHandler can silently skip a default rule that is
post-only (defaultRule with On.phase.IsPostRule()), so add a defensive
validation in Validate(): detect rules where isDefaultRule(rule) is true and
rule.On.phase.IsPostRule() (or equivalent phase check on the rule's On), and
return an explicit validation error or warning (via Validate) explaining
"default rules cannot be post-only" so such misconfigurations are caught early
instead of being dropped at runtime; reference BuildHandler, isDefaultRule,
defaultRule and the On.phase.IsPostRule() check when implementing the
validation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/route/rules/README.mdinternal/route/rules/block_parser_bench_test.gointernal/route/rules/block_parser_test.gointernal/route/rules/do_blocks.gointernal/route/rules/http_flow_block_test.gointernal/route/rules/http_flow_yaml_test.gointernal/route/rules/on.gointernal/route/rules/rules.gointernal/route/rules/rules_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/route/rules/rules_test.go
…and remove redundant calculation in parseDoWithBlocks function
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
internal/route/rules/http_flow_block_test.go (3)
844-873: Hard-coded test passwords — duplicate of GitHub Advanced Security findings at these lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/http_flow_block_test.go` around lines 844 - 873, The test currently embeds hard-coded plaintext passwords ("adminpass"/"guestpass") passed to bcrypt.GenerateFromPassword and later used with req.SetBasicAuth; replace those literals with generated ephemeral test passwords so no secrets are committed. Create a small helper (e.g., generateTestPassword or use an existing testutil) that returns a secure random string, assign adminPass/guestPass from it, pass those to bcrypt.GenerateFromPassword and to req.SetBasicAuth, and keep the rest of the flow (parseRules, Rules.BuildHandler, handler.ServeHTTP, assertions) unchanged.
323-323: Hard-coded test credentials — already flagged by GitHub Advanced Security.
SetBasicAuth("user", "pass")here (and the corresponding bcrypt-hashed passwords at lines 844–873) are test-only values, but the security scanner has flagged them. Consider using constants defined in a shared test-helpers file or moving to table-driven credential setup to make the test-only nature explicit and reduce scanner noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/http_flow_block_test.go` at line 323, The test embeds hard-coded credentials via req3.SetBasicAuth("user", "pass") (and matching bcrypt-hashed passwords elsewhere), which triggers security scanners; refactor to use shared test constants or a test-helpers utility (e.g., TestUser, TestPassword) or a table-driven credential setup so the values are clearly test-only. Update the test to call req3.SetBasicAuth(TestUser, TestPassword) (or pull creds from a helper function that returns generated creds) and replace the inline bcrypt hashes with values derived from a single source in the helper (or generate hashes at test setup) so the test no longer contains hard-coded secrets and scanners will recognize them as test fixtures.
1275-1307: Past finding resolved —should-runvalue and assertion are now consistent.Line 1288 uses
should-runand the assertion at line 1305 correctly expects"should-run", aligning with the test name_ButRunsPostOnlyAndPostMatchers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/http_flow_block_test.go` around lines 1275 - 1307, No code change needed in TestHTTPFlow_PreTermination_SkipsLaterPreCommands_ButRunsPostOnlyAndPostMatchers — the header value "should-run" and the assertion expecting "should-run" are consistent; mark the review comment as resolved and remove the duplicate_comment note referencing this test and the "should-run" assertion to avoid redundant feedback.internal/route/rules/do_blocks.go (1)
65-85: Past fix correctly applied —IfElseBlockCommand.ServeHTTPnow properly consumes matched branches with empty bodies.The unconditional (
br.On.checker == nil) and conditional (br.On.checker.Check) paths both returnnilwhenbr.Do == nil, preventing theElsebranch from firing for a matched-but-empty if/elif.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/do_blocks.go` around lines 65 - 85, The ServeHTTP implementation in IfElseBlockCommand duplicates logic for unconditional (br.On.checker == nil) and conditional checks and must preserve the behavior that a matched branch with br.Do == nil returns nil (preventing Else from executing); refactor ServeHTTP (function IfElseBlockCommand.ServeHTTP) to compute a single "matched" condition (using br.On.checker == nil || br.On.checker.Check(w,r)) and then, when matched, return nil if br.Do == nil or call Commands(br.Do).ServeHTTP(w,r,upstream), keeping the final Else handling (Commands(c.Else).ServeHTTP) unchanged.
🧹 Nitpick comments (4)
internal/route/rules/on.go (1)
566-666:splitPipeandforEachPipePartduplicate the same quote/bracket-aware split logic.Both implement ~25 lines of identical
'|'-splitting with quote and bracket tracking. Consider implementingsplitPipein terms offorEachPipePartto keep the parsing logic in one place.♻️ Proposed simplification of
splitPipe-// splitPipe splits a string by "|" but respects quotes, brackets, and escaped characters. -// It's similar to the parser.go logic but specifically for pipe splitting. func splitPipe(s string) []string { if s == "" { return []string{} } - - result := make([]string, 0, 2) - quote := byte(0) - brackets := 0 - start := 0 - - for i := 0; i < len(s); i++ { - switch s[i] { - case '\\': - // Skip escaped character. - if i+1 < len(s) { - i++ - } - case '"', '\'', '`': - if quote == 0 && brackets == 0 { - quote = s[i] - } else if s[i] == quote { - quote = 0 - } - case '(': - brackets++ - case ')': - if brackets > 0 { - brackets-- - } - case '|': - if quote == 0 && brackets == 0 { - result = append(result, strings.TrimSpace(s[start:i])) - start = i + 1 - } - } - } - - // drop trailing empty part. - if start < len(s) { - if part := strings.TrimSpace(s[start:]); part != "" { - result = append(result, part) - } - } - - return result + result := make([]string, 0, 2) + forEachPipePart(s, func(part string) { + result = append(result, part) + }) + return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/on.go` around lines 566 - 666, splitPipe duplicates the quote/bracket-aware splitting logic found in forEachPipePart; replace splitPipe's loop with a simple collector that calls forEachPipePart to append each part to a result slice (preserving current behavior of returning [] when input is "" and dropping empty/trailing parts), i.e., create result := []string{} and call forEachPipePart(s, func(part string){ result = append(result, part) }) then return result; ensure function signatures (splitPipe and forEachPipePart) are used exactly as named so parsing logic remains centralized.internal/route/rules/do_blocks.go (3)
174-186:elifkeyword detection lacks a word-boundary check — produces misleading errors for tokens that merely start with "elif".
strings.HasPrefix(src[q:], "elif")matches any token beginning with those four letters. A token likeelifhandlerfalls into this branch and emits"expected whitespace after 'elif'"instead of the generic "unexpected token" at line 285. An identical issue exists forelseat line 232.♻️ Suggested fix — enforce word boundary
if strings.HasPrefix(src[q:], "elif") { next := q + len("elif") + // Ensure "elif" is a whole word, not a prefix of another identifier. + if next < length && !unicode.IsSpace(rune(src[next])) && src[next] != '\n' { + return nil, 0, ErrInvalidBlockSyntax.Withf("unexpected token after nested block; expected 'elif'/'else' or newline") + } if next >= length {Apply the same guard immediately after
next := q + len("else")at line 236.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/do_blocks.go` around lines 174 - 186, The keyword detection branch that uses strings.HasPrefix(src[q:], "elif") erroneously matches tokens like "elifhandler"; after computing next := q + len("elif") check for a word-boundary by treating any letter, digit, or underscore at src[next] as part of an identifier and skip the 'elif' branch in that case. Apply the same guard to the "else" branch (the analogous check after next := q + len("else")) so only standalone keywords enter the existing whitespace/brace/error checks; reference the strings.HasPrefix(...) branches using q, next and length in do_blocks.go.
279-281:forwhose body alwaysreturns — the update clause is dead code; useifinstead.
q2 = skipSameLineSpace(src, q2)is never reached because the loop body always returns.♻️ Replace with `if`
- for q2 := skipSameLineSpace(src, p); q2 < length && src[q2] != '\n'; q2 = skipSameLineSpace(src, q2) { - return nil, 0, ErrInvalidBlockSyntax.Withf("unexpected token after else block") - } + if q2 := skipSameLineSpace(src, p); q2 < length && src[q2] != '\n' { + return nil, 0, ErrInvalidBlockSyntax.Withf("unexpected token after else block") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/do_blocks.go` around lines 279 - 281, The for loop starting with "for q2 := skipSameLineSpace(src, p); q2 < length && src[q2] != '\n'; q2 = skipSameLineSpace(src, q2)" should be replaced with an if because the body always returns and the update clause is dead; change it to "if q2 := skipSameLineSpace(src, p); q2 < length && src[q2] != '\n' { return nil, 0, ErrInvalidBlockSyntax.Withf(\"unexpected token after else block\") }" so you still call skipSameLineSpace once and preserve the ErrInvalidBlockSyntax check (references: skipSameLineSpace, ErrInvalidBlockSyntax, q2, src, p, length).
205-215:bracePosshadowed inside theelifbranch — rename to avoid linter warnings.The outer
bracePos(line 139) is still in scope here; using:=creates a shadowing variable thatgo vet -shadowwill flag.♻️ Rename inner variable
- elifOnExpr, bracePos, herr := parseHeaderToBrace(src, p2) + elifOnExpr, elifBracePos, herr := parseHeaderToBrace(src, p2) if herr != nil { return nil, 0, herr } if elifOnExpr == "" { return nil, 0, ErrInvalidBlockSyntax.Withf("expected on-expr after 'elif'") } - if bracePos >= length || src[bracePos] != '{' { + if elifBracePos >= length || src[elifBracePos] != '{' { return nil, 0, ErrInvalidBlockSyntax.Withf("expected '{' after elif condition") } - p2 = bracePos + p2 = elifBracePos🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/rules/do_blocks.go` around lines 205 - 215, The inner use of "bracePos" is shadowing the outer variable when calling parseHeaderToBrace in the elif branch; change the declaration to reuse the existing variable (use = instead of :=) or rename the inner variable (e.g., innerBracePos) so you don't create a new shadowed variable; update the code around parseHeaderToBrace, elifOnExpr and the subsequent checks to reference the reused or renamed variable accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/route/rules/http_flow_block_test.go`:
- Around line 285-310: The token random_uuid in the rule set resp_header
X-Correlation-Id random_uuid is being treated as a literal which is confusing;
either (A) if the header should be a literal, change the token to an unambiguous
literal (e.g., set resp_header X-Correlation-Id corr-id-test) and update the
assertion that checks w1.Header().Get("X-Correlation-Id") to expect
"corr-id-test", or (B) if a UUID should be generated, change the rule to use the
DSL function (e.g., set resp_header X-Correlation-Id $random_uuid), implement
support for $random_uuid in parseRules/DSL evaluation (the code path exercised
by parseRules and rules.BuildHandler), and change the test assertion to validate
the header value against a UUID regex instead of expecting the literal
"random_uuid".
---
Duplicate comments:
In `@internal/route/rules/do_blocks.go`:
- Around line 65-85: The ServeHTTP implementation in IfElseBlockCommand
duplicates logic for unconditional (br.On.checker == nil) and conditional checks
and must preserve the behavior that a matched branch with br.Do == nil returns
nil (preventing Else from executing); refactor ServeHTTP (function
IfElseBlockCommand.ServeHTTP) to compute a single "matched" condition (using
br.On.checker == nil || br.On.checker.Check(w,r)) and then, when matched, return
nil if br.Do == nil or call Commands(br.Do).ServeHTTP(w,r,upstream), keeping the
final Else handling (Commands(c.Else).ServeHTTP) unchanged.
In `@internal/route/rules/http_flow_block_test.go`:
- Around line 844-873: The test currently embeds hard-coded plaintext passwords
("adminpass"/"guestpass") passed to bcrypt.GenerateFromPassword and later used
with req.SetBasicAuth; replace those literals with generated ephemeral test
passwords so no secrets are committed. Create a small helper (e.g.,
generateTestPassword or use an existing testutil) that returns a secure random
string, assign adminPass/guestPass from it, pass those to
bcrypt.GenerateFromPassword and to req.SetBasicAuth, and keep the rest of the
flow (parseRules, Rules.BuildHandler, handler.ServeHTTP, assertions) unchanged.
- Line 323: The test embeds hard-coded credentials via req3.SetBasicAuth("user",
"pass") (and matching bcrypt-hashed passwords elsewhere), which triggers
security scanners; refactor to use shared test constants or a test-helpers
utility (e.g., TestUser, TestPassword) or a table-driven credential setup so the
values are clearly test-only. Update the test to call
req3.SetBasicAuth(TestUser, TestPassword) (or pull creds from a helper function
that returns generated creds) and replace the inline bcrypt hashes with values
derived from a single source in the helper (or generate hashes at test setup) so
the test no longer contains hard-coded secrets and scanners will recognize them
as test fixtures.
- Around line 1275-1307: No code change needed in
TestHTTPFlow_PreTermination_SkipsLaterPreCommands_ButRunsPostOnlyAndPostMatchers
— the header value "should-run" and the assertion expecting "should-run" are
consistent; mark the review comment as resolved and remove the duplicate_comment
note referencing this test and the "should-run" assertion to avoid redundant
feedback.
---
Nitpick comments:
In `@internal/route/rules/do_blocks.go`:
- Around line 174-186: The keyword detection branch that uses
strings.HasPrefix(src[q:], "elif") erroneously matches tokens like
"elifhandler"; after computing next := q + len("elif") check for a word-boundary
by treating any letter, digit, or underscore at src[next] as part of an
identifier and skip the 'elif' branch in that case. Apply the same guard to the
"else" branch (the analogous check after next := q + len("else")) so only
standalone keywords enter the existing whitespace/brace/error checks; reference
the strings.HasPrefix(...) branches using q, next and length in do_blocks.go.
- Around line 279-281: The for loop starting with "for q2 :=
skipSameLineSpace(src, p); q2 < length && src[q2] != '\n'; q2 =
skipSameLineSpace(src, q2)" should be replaced with an if because the body
always returns and the update clause is dead; change it to "if q2 :=
skipSameLineSpace(src, p); q2 < length && src[q2] != '\n' { return nil, 0,
ErrInvalidBlockSyntax.Withf(\"unexpected token after else block\") }" so you
still call skipSameLineSpace once and preserve the ErrInvalidBlockSyntax check
(references: skipSameLineSpace, ErrInvalidBlockSyntax, q2, src, p, length).
- Around line 205-215: The inner use of "bracePos" is shadowing the outer
variable when calling parseHeaderToBrace in the elif branch; change the
declaration to reuse the existing variable (use = instead of :=) or rename the
inner variable (e.g., innerBracePos) so you don't create a new shadowed
variable; update the code around parseHeaderToBrace, elifOnExpr and the
subsequent checks to reference the reused or renamed variable accordingly.
In `@internal/route/rules/on.go`:
- Around line 566-666: splitPipe duplicates the quote/bracket-aware splitting
logic found in forEachPipePart; replace splitPipe's loop with a simple collector
that calls forEachPipePart to append each part to a result slice (preserving
current behavior of returning [] when input is "" and dropping empty/trailing
parts), i.e., create result := []string{} and call forEachPipePart(s, func(part
string){ result = append(result, part) }) then return result; ensure function
signatures (splitPipe and forEachPipePart) are used exactly as named so parsing
logic remains centralized.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/route/rules/do_blocks.gointernal/route/rules/http_flow_block_test.gointernal/route/rules/on.go
Comment on lines
+285
to
+310
| err := parseRules(fmt.Sprintf(` | ||
| { | ||
| set resp_header X-Correlation-Id random_uuid | ||
| } | ||
| path /protected { | ||
| require_basic_auth "Protected Area" | ||
| } | ||
| { | ||
| log info %q "$req_method $req_url -> $status_code" | ||
| } | ||
| status 4xx { | ||
| log error %q "ERROR: $req_method $req_url $status_code" | ||
| } | ||
| `, logFile, errorLogFile), &rules) | ||
| require.NoError(t, err) | ||
|
|
||
| handler := rules.BuildHandler(upstream) | ||
|
|
||
| // Test successful request | ||
| req1 := httptest.NewRequest(http.MethodGet, "/public", nil) | ||
| w1 := httptest.NewRecorder() | ||
| handler.ServeHTTP(w1, req1) | ||
|
|
||
| assert.Equal(t, http.StatusOK, w1.Code) | ||
| assert.Equal(t, "success", w1.Body.String()) | ||
| assert.Equal(t, "random_uuid", w1.Header().Get("X-Correlation-Id")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=go -C3 '\brandom_uuid\b'Repository: yusing/godoxy
Length of output: 2314
🏁 Script executed:
rg -n --type=go -A5 'resp_header|set.*header' internal/route/rules/ | head -100Repository: yusing/godoxy
Length of output: 5839
🏁 Script executed:
cat -n internal/route/rules/do_set.goRepository: yusing/godoxy
Length of output: 11555
🏁 Script executed:
rg -n --type=go 'validatePostResponseKVTemplate|ExpandVarsToString' internal/route/rules/ | head -20Repository: yusing/godoxy
Length of output: 1090
🏁 Script executed:
sed -n '311,350p' internal/route/rules/validate.goRepository: yusing/godoxy
Length of output: 605
🏁 Script executed:
rg -n --type=go -B5 -A20 'func validateTemplate' internal/route/rules/validate.goRepository: yusing/godoxy
Length of output: 865
🏁 Script executed:
rg -n --type=go 'func NeedExpandVars|func ValidateVars' internal/route/rules/Repository: yusing/godoxy
Length of output: 216
🏁 Script executed:
sed -n '36,100p' internal/route/rules/vars.goRepository: yusing/godoxy
Length of output: 1990
🏁 Script executed:
sed -n '1,50p' internal/route/rules/vars.goRepository: yusing/godoxy
Length of output: 943
🏁 Script executed:
sed -n '88,200p' internal/route/rules/vars.goRepository: yusing/godoxy
Length of output: 2935
🏁 Script executed:
rg -n --type=go 'random_uuid|staticRespVarSubsMap|dynamicVarSubsMap' internal/route/rules/Repository: yusing/godoxy
Length of output: 864
🏁 Script executed:
cat -n internal/route/rules/vars_dynamic.goRepository: yusing/godoxy
Length of output: 4150
🏁 Script executed:
cat -n internal/route/rules/vars_static.goRepository: yusing/godoxy
Length of output: 4184
The random_uuid token is treated as a literal string; clarify intent and naming.
The rule set resp_header X-Correlation-Id random_uuid (without $ prefix) is correctly asserted to produce the literal header value "random_uuid". However, the naming strongly implies UUID generation functionality. If the intent is truly to set a literal value, rename the token to something unambiguous like "corr-id-test" or "placeholder" to avoid confusion. If UUID generation is intended, the rule should use a proper DSL function (e.g., $random_uuid) with corresponding implementation and validation of the generated UUID format in the assertion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/route/rules/http_flow_block_test.go` around lines 285 - 310, The
token random_uuid in the rule set resp_header X-Correlation-Id random_uuid is
being treated as a literal which is confusing; either (A) if the header should
be a literal, change the token to an unambiguous literal (e.g., set resp_header
X-Correlation-Id corr-id-test) and update the assertion that checks
w1.Header().Get("X-Correlation-Id") to expect "corr-id-test", or (B) if a UUID
should be generated, change the rule to use the DSL function (e.g., set
resp_header X-Correlation-Id $random_uuid), implement support for $random_uuid
in parseRules/DSL evaluation (the code path exercised by parseRules and
rules.BuildHandler), and change the test assertion to validate the header value
against a UUID regex instead of expecting the literal "random_uuid".
Refactored splitPipe function to use forEachPipePart helper, which filters out empty segments instead of including them in the result. Updated test expectation to match the new behavior where empty parts between pipes are no longer included.
|
yusing
added a commit
that referenced
this pull request
Feb 25, 2026
* chore(deps): update submodule goutils * docs(http): remove default client from README.md * refactor(rules): introduce block DSL, phase-based execution, and flow validation - add block syntax parser/scanner with nested @blocks and elif/else support - restructure rule execution into explicit pre/post phases with phase flags - classify commands by phase and termination behavior - enforce flow semantics (default rule handling, dead-rule detection) - expand HTTP flow coverage with block + YAML parity tests and benches - refresh rules README/spec and update playground/docs integration - Default rules act as fallback handlers that execute only when no matching non-default rule exists in the pre phase - IfElseBlockCommand now returns early when a condition matches with a nil Do block, instead of falling through to else blocks - Add nil check for auth handler to allow requests when no auth is configured * fix(rules): buffer log output before writing to stdout/stderr * refactor(api/rules): remove IsResponseRule field from ParsedRule and related logic * docs(rules): update examples to use block syntax
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
1 participant
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.
@blocksand elif/else supportjoinCookieValues helper.
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation