Remediate targeted custom-linter findings - #51942
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage: Remediate targeted custom-linter findings
Notes: Draft PR, no CI run yet, no reviews. Small and low-risk but not ready — needs to be marked ready for review and CI to run before it can be fast-tracked or auto-merged.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. Changes are production-only (pkg/parser/inline_section_helpers.go, pkg/workflow/mcp_setup_gateway.go).
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #51942 does not have the 'implementation' label and has only 15 new lines of code in business logic directories (threshold: 100).
|
There was a problem hiding this comment.
Pull request overview
Resolves the two targeted custom-linter findings while preserving existing behavior.
Changes:
- Extracts inline-section end-marker matching into a focused helper.
- Replaces the hard-coded GitHub CLI path with a named constant.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/inline_section_helpers.go |
Extracts end-marker matching logic. |
pkg/workflow/mcp_setup_gateway.go |
Defines and uses the GitHub CLI path constant. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- 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.
Two clean linter remediations with no behavior change: matchInlineSectionEndMarker extracts an inline loop with identical semantics, and mcpGatewayGitHubCLIPath constant replaces the magic string /usr/bin/gh. No security, correctness, or reliability concerns.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.8 AIC · ⌖ 6.2 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — requesting one change.
📋 Key Themes & Highlights
Key Themes
- Duplicated path literal:
mcpGatewayGitHubCLIPath = "/usr/bin/gh"is a new file-local constant that re-declares the same path already embedded inconstants.DefaultGhBinaryMount. Both now need updating if theghbinary path ever changes.
Positive Highlights
- ✅
matchInlineSectionEndMarkeris a clean, well-scoped extraction — good single-responsibility refactor. - ✅ The
usedEndmutation side-effect is preserved correctly across the refactor. - ✅ Naming is clear and consistent with the existing
inlineSectionEndMarkertype.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.1 AIC · ⌖ 7 AIC · ⊞ 7.1K
Comment /matt to run again
| const mcpGatewayCustomEnvNamesVar = "GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES" | ||
| const mcpGatewayCustomEnvTransportPrefix = "GH_AW_MCP_GATEWAY_ENV_" | ||
| const mcpGatewayCustomEnvMarker = "__GH_AW_MCP_GATEWAY_CUSTOM_ENV__" | ||
| const mcpGatewayGitHubCLIPath = "/usr/bin/gh" |
There was a problem hiding this comment.
[/codebase-design] The new constant mcpGatewayGitHubCLIPath = "/usr/bin/gh" duplicates a path literal that already lives in pkg/constants as part of DefaultGhBinaryMount = "/usr/bin/gh:/usr/bin/gh:ro". A future path change must now be updated in two places.
💡 Suggested fix
Add a GhCLIPath constant in pkg/constants and derive DefaultGhBinaryMount from it:
const GhCLIPath = "/usr/bin/gh"
const DefaultGhBinaryMount = GhCLIPath + ":" + GhCLIPath + ":ro"Then use constants.GhCLIPath here and drop the local constant.
@copilot please address this.
There was a problem hiding this comment.
Centralized the path as constants.GhCLIPath, derive DefaultGhBinaryMount from it, and use it in the gateway allowlist (369d04e).
🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — No Tests Modified
📊 Analysis Summary
Files changed:
Verdict
|
PR Triage
|
|
@copilot Please refresh this branch, verify checks, and use the pr-finisher skill before handing back. No failed checks were reported in the compact candidate set for this run.
|
…remediate-findings Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Two targeted custom-linter findings flagged an oversized inline-section parser helper and a hard-coded GitHub CLI path.
Parser
extractInlineSectionsinto a focused helper.MCP gateway
"/usr/bin/gh"literal with the namedmcpGatewayGitHubCLIPathconstant.