Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/e0d1eeba-1e83-4b71-9ad8-7d48cbdb4d2a Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…rect string ops Replace parser.ParseImportDirective (regex + struct allocs) in findIncludesInContent with inline parseIncludePath helper using strings.HasPrefix/Cut/TrimSpace. Benchmark (AMD EPYC 7763): Before: 2109 ns/op 840 B/op 18 allocs/op After: 265 ns/op 112 B/op 3 allocs/op (~8x faster, 6x fewer allocs) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/e0d1eeba-1e83-4b71-9ad8-7d48cbdb4d2a Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves performance of include scanning in findIncludesInContent by replacing regex-based import directive parsing with a zero/low-allocation string parser, addressing a reported regression.
Changes:
- Replaced
parser.ParseImportDirectiveusage infindIncludesInContentwith a newparseIncludePathhelper usingHasPrefix/Cut/TrimSpace. - Added
pull-requests: writeto theactivationjob permissions in several*.lock.ymlworkflows. - Removed now-unused
parserimport frompkg/cli/remove_command.go.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/remove_command.go | Introduces parseIncludePath and switches include scanning to use it instead of the regex-based parser. |
| .github/workflows/unbloat-docs.lock.yml | Adds pull-requests: write permission to the activation job. |
| .github/workflows/tidy.lock.yml | Adds pull-requests: write permission to the activation job. |
| .github/workflows/plan.lock.yml | Adds pull-requests: write permission to the activation job. |
| .github/workflows/pdf-summary.lock.yml | Adds pull-requests: write permission to the activation job. |
| .github/workflows/mergefest.lock.yml | Adds pull-requests: write permission to the activation job. |
| .github/workflows/brave.lock.yml | Adds pull-requests: write permission to the activation job. |
| .github/workflows/ace-editor.lock.yml | Adds pull-requests: write permission to the activation job. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
.github/workflows/tidy.lock.yml:95
- This PR’s title/description focuses on the Go perf regression in FindIncludesInContent, but it also changes workflow token permissions by adding
pull-requests: writehere (and in other lock workflows). If this is intentional, please update the PR description to explain why these permission changes are needed (or consider splitting them into a separate PR).
actions: read
contents: read
issues: write
pull-requests: write
- Files reviewed: 8/8 changed files
- Comments generated: 9
| // Extract path up to closing "}}" | ||
| before, _, ok := strings.Cut(rest, "}}") | ||
| if !ok { |
There was a problem hiding this comment.
For the {{#import}} form, this parses everything up to the first "}}" but does not verify that the closing braces are at the end of the directive (the previous anchored regex required the line to end at "}}" apart from whitespace). As a result, a line like "{{#import shared/x.md}} trailing" would now be treated as a directive and produce a false include. Consider validating that the text after the cut is empty/whitespace (e.g., capture the remainder from strings.Cut and require strings.TrimSpace(remainder)=="") before returning a path.
| // Extract path up to closing "}}" | |
| before, _, ok := strings.Cut(rest, "}}") | |
| if !ok { | |
| // Extract path up to closing "}}" and require only whitespace after it. | |
| before, after, ok := strings.Cut(rest, "}}") | |
| if !ok || strings.TrimSpace(after) != "" { |
| actions: read | ||
| contents: read | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
Adding pull-requests: write increases the GITHUB_TOKEN scope for this job. The activation job’s “Add comment with workflow run link” script posts via the issues comments endpoint (/repos/{owner}/{repo}/issues/{number}/comments in actions/setup/js/add_workflow_run_comment.cjs), which only requires issues: write. Unless another step in this job truly needs PR write access, please drop pull-requests: write to follow least-privilege permissions.
This issue also appears on line 92 of the same file.
| pull-requests: write |
| contents: read | ||
| discussions: write | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
Adding pull-requests: write increases the GITHUB_TOKEN scope for this job. The activation job’s “Add comment with workflow run link” script posts via the issues comments endpoint (/repos/{owner}/{repo}/issues/{number}/comments in actions/setup/js/add_workflow_run_comment.cjs), which only requires issues: write. Unless another step in this job truly needs PR write access, please drop pull-requests: write to follow least-privilege permissions.
| pull-requests: write |
| actions: read | ||
| contents: read | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
Adding pull-requests: write increases the GITHUB_TOKEN scope for this job. The activation job’s “Add comment with workflow run link” script posts via the issues comments endpoint (/repos/{owner}/{repo}/issues/{number}/comments in actions/setup/js/add_workflow_run_comment.cjs), which only requires issues: write. Unless another step in this job truly needs PR write access, please drop pull-requests: write to follow least-privilege permissions.
| pull-requests: write |
| // parseIncludePath extracts the file path from @include/@import/{{#import}} directive lines | ||
| // without allocating a regex submatch slice or a directive struct. | ||
| // Returns an empty string if the line is not a recognised directive. | ||
| // Section references (e.g. file.md#Section) are stripped from the returned path. | ||
| func parseIncludePath(line string) string { |
There was a problem hiding this comment.
This new parsing logic supports @import and {{#import}} (with optional '?' and optional ':'), but the existing findIncludesInContent tests only cover @include. Please add unit tests that exercise @import and the {{#import}} variants (colon/no-colon and optional marker) to ensure behavior stays aligned with the legacy regex-based parser.
| actions: read | ||
| contents: read | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
Adding pull-requests: write increases the GITHUB_TOKEN scope for this job. The activation job’s “Add comment with workflow run link” script posts via the issues comments endpoint (/repos/{owner}/{repo}/issues/{number}/comments in actions/setup/js/add_workflow_run_comment.cjs), which only requires issues: write. Unless another step in this job truly needs PR write access, please drop pull-requests: write to follow least-privilege permissions.
| pull-requests: write |
| actions: read | ||
| contents: read | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
Adding pull-requests: write increases the GITHUB_TOKEN scope for this job. The activation job’s “Add comment with workflow run link” script posts via the issues comments endpoint (/repos/{owner}/{repo}/issues/{number}/comments in actions/setup/js/add_workflow_run_comment.cjs), which only requires issues: write. Unless another step in this job truly needs PR write access, please drop pull-requests: write to follow least-privilege permissions.
| pull-requests: write |
| actions: read | ||
| contents: read | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
Adding pull-requests: write increases the GITHUB_TOKEN scope for this job. The activation job’s “Add comment with workflow run link” script posts via the issues comments endpoint (/repos/{owner}/{repo}/issues/{number}/comments in actions/setup/js/add_workflow_run_comment.cjs), which only requires issues: write. Unless another step in this job truly needs PR write access, please drop pull-requests: write to follow least-privilege permissions.
| pull-requests: write |
| actions: read | ||
| contents: read | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
Adding pull-requests: write increases the GITHUB_TOKEN scope for this job. The activation job’s “Add comment with workflow run link” script posts via the issues comments endpoint (/repos/{owner}/{repo}/issues/{number}/comments in actions/setup/js/add_workflow_run_comment.cjs), which only requires issues: write. Unless another step in this job truly needs PR write access, please drop pull-requests: write to follow least-privilege permissions.
| pull-requests: write |
|
🛠️ Agentic Maintenance updated this pull request branch. |
|
@copilot review all comments |
…t}} trailing content, add tests
- Revert unintended pull-requests:write additions to 7 lock files
- Fix {{#import}} parser to reject directives with trailing content after }}
- Add test cases for @import and {{#import}} directive variants
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/042e8ccf-a206-4fd4-8347-dcf4020b615f
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all comments in commit 482efaa:
|
|
🛠️ Agentic Maintenance updated this pull request branch. |
findIncludesInContentcalledparser.ParseImportDirectiveper line, which runs a complex alternation regex viaFindStringSubmatch— allocating a submatch[]string, backtracking state, and anImportDirectiveMatchstruct on every directive hit. This caused 18 allocs/op and was the source of the reported regression.Changes
pkg/cli/remove_command.go: ReplaceParseImportDirectivewith a newparseIncludePathhelper that usesstrings.HasPrefix+strings.Cut+strings.TrimSpaceinstead of regex. Handles all three directive forms:@include,@import,{{#import}}.{{#import}}colon handling: Explicit single-colon removal to match the original regex's\s*:?\s*semantics.{{#import}}trailing content: Validates that text after}}is empty/whitespace, rejecting lines like{{#import shared/x.md}} trailing(matching the original anchored regex's behavior).@import(basic, optional marker, section reference) and all{{#import}}variants (colon, no-colon, optional marker, section reference, trailing-content rejection).parserimport.Benchmark (AMD EPYC 7763)