[code-scanning-fix] Fix workflow-graphql-static-concat: extract GraphQL query to named constant#41357
Conversation
Extract the hardcoded GraphQL query string in checkRepositoryHasDiscussionsUncached into a package-level named constant checkRepositoryHasDiscussionsQuery. This addresses code scanning alert #626 (workflow-graphql-static-concat / CWE-89): static analysis flagged the string concatenation pattern 'query='+query even though the query variable held a hardcoded literal. Using a named constant makes it unambiguous to static analysis tools that the value is not user-controlled input. No runtime behavior changes — the same query string is passed to gh api graphql. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. Only changed file: pkg/workflow/repository_features_validation.go (production code refactoring to extract GraphQL query to named constant). |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41357 does not have the 'implementation' label and has only 12 new lines (≤100) in business logic directories across 1 file. |
There was a problem hiding this comment.
Pull request overview
This PR refactors checkRepositoryHasDiscussionsUncached in gh-aw’s workflow validation code to reduce a code-scanning false positive by making the GraphQL query string clearly immutable.
Changes:
- Added a package-level constant
checkRepositoryHasDiscussionsQueryholding the GraphQL query. - Removed the local
queryvariable incheckRepositoryHasDiscussionsUncachedand referenced the constant instead. - Updated nearby comments to clarify the intent (CWE-89 /
workflow-graphql-static-concat).
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/repository_features_validation.go |
Extracts the discussions-check GraphQL query to a named constant and updates the gh.Exec call to use it. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
| stdOut, _, err := gh.Exec("api", "graphql", "-f", "query="+checkRepositoryHasDiscussionsQuery, | ||
| "-f", "owner="+owner, "-f", "name="+name) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /improve-codebase-architecture — approving with two minor non-blocking observations.
📋 Key Themes & Highlights
Key Themes
- Correct and minimal fix: Promoting a function-scoped
varto a package-levelconstis exactly the right move to satisfy theworkflow-graphql-static-concat/ CWE-89 scanner rule. No runtime behavior changes. - Redundant call-site comment: The inline comment added at the
gh.Execcall site repeats reasoning already captured in theconstdoc comment — a minor cleanup opportunity. - Similar patterns elsewhere:
pkg/cli/project_command.gohas 7 similar"query="+<var>call sites, some usingfmt.Sprintfwith user input. Worth a follow-up to ensure full CWE-89 coverage.
Positive Highlights
- ✅ Laser-focused change — exactly one file, minimum necessary diff
- ✅ Package-level
constnaming (checkRepositoryHasDiscussionsQuery) is clear and self-documenting - ✅ Comprehensive
constdoc comment explains the security rationale for future readers
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 50.9 AIC · ⌖ 7.6 AIC · ⊞ 6.5K
|
|
||
| // Execute GraphQL query using gh CLI | ||
| // Execute GraphQL query using gh CLI. | ||
| // checkRepositoryHasDiscussionsQuery is a package-level constant — not user-controlled. |
There was a problem hiding this comment.
[/improve-codebase-architecture] This call-site comment repeats what the package-level const doc comment (lines 57–59) already explains — consider removing it to keep the call site clean.
💡 Detail
The const declaration already documents:
Declared as a named constant to make clear it is not user-controlled input (CWE-89 / workflow-graphql-static-concat).
The name checkRepositoryHasDiscussionsQuery itself carries enough signal at the call site. Removing the inline comment reduces redundancy without losing any safety intent.
| } | ||
|
|
||
| stdOut, _, err := gh.Exec("api", "graphql", "-f", "query="+query, | ||
| stdOut, _, err := gh.Exec("api", "graphql", "-f", "query="+checkRepositoryHasDiscussionsQuery, |
There was a problem hiding this comment.
[/zoom-out] pkg/cli/project_command.go has 7 similar "query="+<var> call sites — some of which interpolate user-supplied input directly into the query string via fmt.Sprintf. These may trigger the same scanner rule (or a stricter variant) and are worth a follow-up sweep.
💡 Affected lines in project_command.go
| Line | Pattern |
|---|---|
| 235–240 | fmt.Sprintf(query { organization(login: "%s") ... }, escapeGraphQLString(owner)) then "query="+query |
| 237–240 | same for user login |
| 275 | "query="+query after dynamic Sprintf build |
| 306, 361, 385 | mutation strings with "query="+mutation |
| 664, 671 | project info/fields queries |
Even with escapeGraphQLString, the approach differs from this PR's pattern (a pure static const). Tracking these as potential follow-up items would help close out the broader CWE-89 coverage.
There was a problem hiding this comment.
No blocking issues found. The refactoring is correct Go practice for suppressing workflow-graphql-static-concat findings: moving the query to a package-level const makes its compile-time immutability explicit to static analysis tools. Behavioral change: none.
The residual concern about the + operator at the call site (line 282) is already covered by the existing inline review comment — no new issues to add.
🔎 Code quality review by PR Code Quality Reviewer · 49.5 AIC · ⌖ 7.31 AIC · ⊞ 5.2K
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
test