Remediate dynamic-regexp custom-linter findings across pkg/agentdrain, stringutil, parser, cli, workflow#50995
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
No ADR enforcement needed: PR does not have the implementation label and has 94 new lines of code in business logic directories (threshold: 100). |
|
|
|
No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Remediates dynamic-regexp linter findings while preserving existing parsing, sanitization, and workflow behavior.
Changes:
- Replaces avoidable dynamic regex compilation with string matching or precompiled patterns.
- Documents and suppresses intentional safe dynamic regex usage.
- Simplifies unreachable sanitization fallback behavior.
Show a summary per file
| File | Description |
|---|---|
pkg/agentdrain/mask.go |
Justifies intentional rule compilation. |
pkg/stringutil/sanitize.go |
Removes dynamic fallback compilation. |
pkg/parser/frontmatter_content.go |
Documents escaped section matching. |
pkg/parser/json_path_locator.go |
Replaces regex key matching with string operations. |
pkg/parser/schema_suggestions.go |
Annotates escaped dynamic patterns. |
pkg/cli/codemod_activation_outputs.go |
Precompiles activation-output patterns. |
pkg/cli/firewall_policy.go |
Documents intentional policy regex compilation. |
pkg/workflow/gh_cli_permissions.go |
Annotates embedded-config regex compilation. |
pkg/workflow/mcp_renderer_guard.go |
Documents trusted sentinel construction. |
pkg/workflow/observability_otlp.go |
Documents constant-derived endpoint pattern. |
pkg/workflow/template_injection_utils.go |
Expands heredoc patterns into literals. |
pkg/workflow/yaml.go |
Documents escaped cached patterns. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Remediate dynamic-regexp linter findings
Overall the approach is sound — genuine rewrites where feasible, well-justified (nolint/redacted) annotations with inline reasoning elsewhere. Two non-blocking concerns worth addressing:
1. Silent skip in codemod_activation_outputs.go (line 38)
The continue on a missing map key drops the transformation silently. The map and the outputs slice are currently in sync, but the invariant is fragile. A log line would surface future drift immediately.
2. Silent behaviour change in sanitize.go (line 171)
The fallback changed from compile a pattern for the actual chars to use a different, stricter pattern. The comment asserts this path is unreachable, but if it ever is reached (e.g., during a future refactor) the sanitization will silently produce wrong results. A log warning on the fallback path would make this visible.
Everything else looks correct:
json_path_locator.gorewrite is semantically equivalent and eliminates three hot-path compiles.template_injection_utils.gounrolling is mechanical and correct.- All
(nolint/redacted)annotations include a specific, accurate justification. gh_cli_permissions.go,mcp_renderer_guard.go,observability_otlp.goannotations are accurate (constants / QuoteMeta-escaped values).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 29.3 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — commenting (no blocking issues, but a few correctness and maintainability points worth addressing).
📋 Key Themes & Highlights
Key Themes
- Silent failure on unknown keys (
codemod_activation_outputs): the new map +continuepattern creates a silent divergence point. Log a warning or iterate the map directly. - Silent fallback in sanitize (
sanitize.go): replacing the dynamic compile with a hardcoded fallback changes behavior without any log output; future callers won't know why their output is unexpectedly stripped. - Maintainability of the unrolled heredoc list (
template_injection_utils.go): correct, but needs a doc comment to prevent a future "simplification" that would reintroduce the lint violation. matchesPathSegmentKeycorrectness (json_path_locator.go): the string-based replacement is semantically equivalent to the original regex, but a regression test would make the invariant explicit and protect against future edits.
Positive Highlights
- ✅ Two-strategy approach (eliminate vs. annotate) is well-reasoned and consistently applied
- ✅ All
(nolint/redacted)annotations include clear justifications — exactly the right practice - ✅ The
json_path_locatorrewrite is cleaner and faster (no regex allocation per call) - ✅ Dead code removal in
sanitize.gois a clean improvement - ✅ PR description is thorough and self-documenting
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 44.2 AIC · ⊞ 7.1K
Comment /matt to run again
There was a problem hiding this comment.
REQUEST_CHANGES — one high-severity behavioral regression in the regex-to-string-matching rewrite, plus two medium-severity silent-degradation traps introduced by the "precompile everything" refactor.
💡 Themes
pkg/parser/json_path_locator.go:matchesPathSegmentKey's whitespace class narrowed from Go regex\s*(space/tab/newline/CR/FF/VT) toTrimLeft(" \t"), silently changing which lines match a key/property lookup for edge-case whitespace.pkg/cli/codemod_activation_outputs.goandpkg/stringutil/sanitize.goboth moved from dynamic-but-correct fallback behavior to precompiled maps with silentcontinue/default-fallback branches on lookup miss — a reasonable pattern, but each removes a correctness guarantee that a future edit (new output name, new preserve-char combo) will be caught rather than silently mishandled.pkg/workflow/template_injection_utils.go's loop-to-literal unrolling is a maintainability tradeoff (7x duplicated pattern pairs) worth flagging but not blocking.- The
nolint:regexpdynamicpatternannotations for patterns built viaregexp.QuoteMetaover trusted/constant inputs are well-justified and not flagged.
The most important fix before merge is the whitespace-matching regression in json_path_locator.go, since it can cause incorrect (silent) location results in the JSON/YAML path locator.
🔎 Code quality review by PR Code Quality Reviewer · auto · 53.5 AIC · ⊞ 7.8K
Comment /review to run again
| if !strings.HasPrefix(trimmedLine, key) { | ||
| return false | ||
| } | ||
| rest := strings.TrimLeft(trimmedLine[len(key):], " \t") |
There was a problem hiding this comment.
Whitespace handling narrowed from \s* to only space/tab, which can silently change key-matching results for lines with other whitespace (CR, form-feed, etc.) before the colon.
💡 Details
The original code used regexp.MustCompile("^" + regexp.QuoteMeta(key) + "(s/redacted)*:"), where Go's \s matches [\t\n\f\r ]. The rewrite only strips literal space/tab:
rest := strings.TrimLeft(trimmedLine[len(key):], " \t")
return strings.HasPrefix(rest, ":")trimmedLine only has leading/trailing whitespace on the whole line stripped via strings.TrimSpace; whitespace between the key and colon is untouched. A line like key\r: matched before but silently fails to match now, changing behavior of matchesPathAtLevel/findFirstAdditionalProperty (used for JSON/YAML path location) without any error or test signal.
Fix: use strings.TrimLeftFunc(rest, unicode.IsSpace) to preserve the original whitespace class, or add an explicit comment + test documenting that only space/tab are supported.
| // This ensures we don't match things like "needs.activation.outputs.text_custom" | ||
| // The pattern matches the old expression followed by a non-word character or end of string | ||
| pattern := regexp.MustCompile(`needs\.activation\.outputs\.` + output + `\b`) | ||
| pattern, ok := activationOutputPatterns[output] |
There was a problem hiding this comment.
The map lookup silently skips rewriting an output if it's missing from activationOutputPatterns, which can silently degrade the codemod with no error or log.
💡 Details
pattern, ok := activationOutputPatterns[output]
if !ok {
continue
}Previously every entry in the outputs slice was compiled and applied unconditionally. Now outputs and the keys of activationOutputPatterns are two separate hardcoded lists that must be kept in sync manually. If someone adds a new activation output to outputs without updating the map (or the reverse), the codemod will silently no-op for that output instead of failing loudly — a subtle regression risk for a migration tool where silent partial application is worse than an explicit panic/error.
Suggest deriving outputs from the map's keys (or asserting the two are consistent, e.g. via an init() check or unit test) so drift is caught immediately rather than silently swallowed.
| pattern, ok := sanitizePatterns[allowedChars] | ||
| if !ok { | ||
| pattern = regexp.MustCompile(`[^` + allowedChars + `]+`) | ||
| pattern = sanitizePatterns["a-z0-9-"] |
There was a problem hiding this comment.
The fallback now silently degrades to the base a-z0-9- pattern instead of honoring the requested preserved characters, which would drop '.'/'_' from output if buildSanitizePreservePattern and sanitizePatterns ever drift.
💡 Details
pattern, ok := sanitizePatterns[allowedChars]
if !ok {
pattern = sanitizePatterns["a-z0-9-"]
}Previously, an unmapped allowedChars combination fell back to regexp.MustCompile("[^" + allowedChars + "]+"), which — while flagged by the linter as dynamic — was at least functionally correct for any future preserve-char combination. Now, if someone adds a new PreserveSpecialChars option (e.g. a third preservable character) without also adding the corresponding entry to sanitizePatterns, this silently strips the newly-requested character instead of preserving it or erroring. That's a correctness landmine: the caller asked for characters to be preserved and got wrong output with no warning, versus the previous behavior which at least behaved as documented.
Consider asserting/panicking on the branch (since it should be unreachable given all current combinations are precompiled), or add a unit test that iterates over the reachable outputs and asserts every one exists in , so any future combination gap fails CI instead of shipping silently.
| pattern, ok := sanitizePatterns[allowedChars] | ||
| if !ok { | ||
| pattern = regexp.MustCompile(`[^` + allowedChars + `]+`) | ||
| pattern = sanitizePatterns["a-z0-9-"] |
There was a problem hiding this comment.
The fallback now silently degrades to the base a-z0-9- pattern instead of honoring the requested preserved characters, which would drop ./_ from output if buildSanitizePreservePattern and sanitizePatterns ever drift.
💡 Details
pattern, ok := sanitizePatterns[allowedChars]
if !ok {
pattern = sanitizePatterns["a-z0-9-"]
}Previously, an unmapped allowedChars combination fell back to regexp.MustCompile("[^" + allowedChars + "]+"), which — while flagged by the linter as dynamic — was at least functionally correct for any future preserve-char combination. Now, if someone adds a new PreserveSpecialChars option without also adding the corresponding entry to sanitizePatterns, this silently strips the newly-requested character instead of preserving it, with no warning.
Consider asserting/panicking on the !ok branch (it should be unreachable given current combinations), or add a unit test iterating over all reachable buildSanitizePreservePattern outputs asserting each exists in sanitizePatterns, so a future gap fails CI instead of shipping silently.
| // Pattern for quoted delimiter ending with suffix: << 'PREFIX_SUFFIX' or << "PREFIX_SUFFIX" | ||
| // \w* matches zero or more word characters (allowing both exact match and prefixes) | ||
| // (?ms) enables multiline and dotall modes, .*? is non-greedy | ||
| // \s*\wSUFFIX\s*$ allows for leading/trailing whitespace on the closing delimiter |
There was a problem hiding this comment.
Unrolling the loop into 7 duplicated literal pattern pairs increases maintenance risk — any future tweak to the shared heredoc regex logic must now be copy-pasted correctly across all 7 entries.
💡 Details
The prior loop-based construction guaranteed all suffixes shared identical regex structure by definition. The new hardcoded slice relies on manual consistency across 14 regex literals (quoted/unquoted × 7 suffixes). This is a reasonable tradeoff to satisfy the linter, but it removes a structural guarantee: a future edit (e.g. adding a suffix, or fixing a bug in the shared pattern) is now an error-prone find-and-replace across many near-identical lines rather than a single-point change.
Consider adding a test that programmatically verifies each pair matches the loop-generated pattern that would have been produced (e.g. comparing .String() against fmt.Sprintf output), to catch drift if these are ever hand-edited individually.
|
Great work on addressing the dynamic-regexp linter findings, One area to strengthen:
Otherwise, this looks solid and ready for review. The inline
|
|
@copilot Please address the open review feedback, refresh the branch if needed, and run the Open review items to address:
Branch refresh was requested. Run context: https://github.com/github/gh-aw/actions/runs/31150518831
|
…ack paths, add matchesPathSegmentKey tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in the latest commit:
|
PR TriageCategory: refactor (remediate custom-linter findings) · Risk: medium · Score: 55/100 (impact 22, urgency 12, quality 21) Recommendation: Suppresses/annotates
|
Branch is confirmed ready for maintainer review. Here's a concise summary of what this PR contains: Scope: Remediates 23 Approach:
Review-feedback items addressed (commit
All four directly-touched packages ( |
|
@copilot Please address the remaining review feedback and run the Open review items to address:
Branch refresh was requested.
|
All review items are addressed on the current head ( What's in the PR:
The |
|
@copilot Please address the remaining review feedback and run the Open review items to address:
Branch refresh was requested.
|
Confirmed — current head All directly-touched packages pass locally: Open items status:
Ready for maintainer merge. |
|
@copilot Please triage this PR, refresh the branch if needed, and run the skill before handing back to maintainers. Branch refresh was requested. No failed checks are currently reported on the latest head SHA. Please confirm reviewer feedback state and hand back a concise maintainer-ready summary. Run context: https://github.com/github/gh-aw/actions/runs/31180522666
|
Branch is confirmed green on |
The daily
regexpdynamicpatterncustom linter flagged 23 call sites whereregexp.Compile/regexp.MustCompilepatterns were not compile-time constants. Dynamic patterns risk runtime panics and ReDoS if ever influenced by untrusted input.Changes
Rewrote to eliminate dynamic compilation where a safer equivalent was straightforward:
pkg/parser/json_path_locator.go: replaced^key\s*:regex matching with plainstrings.HasPrefix/TrimLeftlogic (3 call sites consolidated into one helper).pkg/cli/codemod_activation_outputs.go: replaced per-iterationregexp.MustCompilewith a package-level map of precompiled literal patterns keyed by output name.pkg/workflow/template_injection_utils.go: unrolled thefmt.Sprintf-based loop over heredoc suffixes into explicit literal patterns per suffix.pkg/stringutil/sanitize.go: removed a dead dynamic-compile fallback branch that could never be reached (all callableallowedCharscombinations are already precompiled insanitizePatterns).Annotated as safe with justification where the pattern is already built from
regexp.QuoteMetaover a trusted constant or embedded config (no realistic ReDoS/panic risk, but not a literal string):pkg/agentdrain/mask.go,pkg/cli/firewall_policy.go: patterns come from trusted config (defaults / admin-supplied rules), errors are already handled instead of panicking.pkg/parser/frontmatter_content.go,pkg/parser/schema_suggestions.go,pkg/workflow/yaml.go: patterns areregexp.QuoteMeta-escaped user-supplied identifiers (section/field/key names), so compilation cannot fail or backtrack pathologically.pkg/workflow/gh_cli_permissions.go: patterns built from embedded, build-time JSON config.pkg/workflow/mcp_renderer_guard.go,pkg/workflow/observability_otlp.go: patterns built from package-level string constants.Each of these got a
//nolint:regexpdynamicpatternwith an inline comment explaining why the pattern is safe.Example of the rewrite pattern used in
json_path_locator.go:Run context: https://github.com/github/gh-aw/actions/runs/31180522666> Generated by 👨🍳 PR Sous Chef · gpt54 · 7.9 AIC · ⊞ 8.3K · ◷