Fix GraphQL ID injection in project_command.go (Semgrep #627/#628, CWE-89)

[Copilot]

Semgrep alerts #627/#628 flagged createProject and linkProjectToRepo for concatenating API-returned node IDs directly into gh api graphql -f key=value arguments without sanitization.

Changes

• pkg/workflow/github_cli.go — adds RunGHInputContext: a new helper mirroring RunGHContext but accepting an io.Reader for stdin, enabling safe use of gh api graphql --input - with spinner support.
• pkg/workflow/github_cli_wasm.go — adds matching no-op stub for wasm builds.
• pkg/cli/project_command.go — refactors both mutation call sites to build a typed {"query": ..., "variables": {...}} body via json.Marshal and pipe it through RunGHInputContext(..., "--input", "-"), eliminating raw string concatenation for ownerId, title, projectId, and repositoryId.

Before (vulnerable pattern):

workflow.RunGH("Creating project...", "api", "graphql",
    "-f", "query="+mutation,
    "-f", "ownerId="+ownerId,   // raw concatenation
    "-f", "title="+title,
)

After:

requestJSON, _ := json.Marshal(map[string]any{
    "query":     mutation,
    "variables": map[string]any{"ownerId": ownerId, "title": title},
})
workflow.RunGHInputContext(ctx, "Creating project...", bytes.NewReader(requestJSON), "api", "graphql", "--input", "-")

This matches the approach already used by updateSingleSelectFieldOptions in the same file.

---

Generated by 👨‍🍳 PR Sous Chef · 8.37 AIC · ⌖ 7.92 AIC · ⊞ 3.7K · ◷
_{Comment /souschef to run again}

---

Generated by 👨‍🍳 PR Sous Chef · 8.12 AIC · ⌖ 9.29 AIC · ⊞ 3.7K · ◷
_{Comment /souschef to run again}

[github-actions]

⚠️ PR Code Quality Reviewer failed during code quality review.

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. Test Quality Sentinel skipped.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (56 additions detected).

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[Copilot]

Pull request overview

This PR addresses Semgrep-reported GraphQL argument injection by removing -f key=value concatenation of node IDs/titles and instead sending a JSON {"query": "...", "variables": {...}} request body to gh api graphql via stdin (--input -). To support this safely (and with consistent UX), it adds a new workflow helper that runs gh with a spinner while piping an io.Reader into stdin.

Changes:

• Add workflow.RunGHInputContext to execute gh commands with context + spinner while piping stdin from an io.Reader.
• Refactor createProject and linkProjectToRepo to json.Marshal GraphQL {query, variables} and pass it via --input - (no raw concatenation of IDs/titles).
• Update multiple compiled workflow .lock.yml files (embedded firewall config/model lists).

Show a summary per file

File	Description
pkg/workflow/github_cli.go	Adds RunGHInputContext helper to run gh with spinner while piping stdin.
pkg/workflow/github_cli_wasm.go	Adds wasm stub for RunGHInputContext.
pkg/cli/project_command.go	Builds GraphQL request JSON with variables and pipes it via --input - to avoid CLI arg injection.
.github/workflows/workflow-health-manager.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/schema-consistency-checker.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/safe-output-health.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/pr-triage-agent.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/poem-bot.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/gpclean.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/daily-team-evolution-insights.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/daily-safe-output-optimizer.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/daily-regulatory.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/daily-observability-report.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/daily-max-ai-credits-test.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/daily-credit-limit-test.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/daily-compiler-quality.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/daily-cli-tools-tester.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/daily-cache-strategy-analyzer.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/copilot-centralization-optimizer.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/copilot-centralization-drilldown.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/code-simplifier.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/ci-coach.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/agentic-token-optimizer.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).
.github/workflows/agent-performance-analyzer.lock.yml	Compiled workflow lock updated (embedded awf/firewall config content changed).

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 83/260 changed files
• Comments generated: 1
• Review effort level: Low

[github-actions]

Review Summary 🧠

Applied /diagnosing-bugs, /tdd, and /codebase-design.

Verdict: REQUEST_CHANGES — one remaining injection surface and missing regression tests.

📋 Issues by Priority

🔴 Blocking

1. pkg/cli/project_command.go line 368–373 — linkProjectToRepo still passes repoOwner and repoName as raw -f arguments in the repoIdQuery call. This is the same user-controlled input → CLI argument pattern the PR was fixing. Migrate to RunGHInputContext + --input -.

🟡 Important

2. pkg/cli/project_command.go line 317 / pkg/workflow/github_cli.go line 176 — No regression tests for createProject, linkProjectToRepo, or RunGHInputContext. Without tests, a future refactor could silently reintroduce the vulnerable -f pattern.

🟢 Minor

3. pkg/workflow/github_cli.go line 185 — spinner.Stop() is not deferred; use defer spinner.Stop() for panic-safety.
4. pkg/cli/project_command.go line 404 — updateSingleSelectFieldOptions uses the older ExecGH + .Stdin path; consider migrating to RunGHInputContext for a single canonical stdin-piping pattern.
5. pkg/workflow/github_cli_wasm.go line 47 — Wasm stub lacks a doc comment noting that input is intentionally ignored.

@copilot please address the review comments above.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 112.1 AIC · ⌖ 6.31 AIC · ⊞ 6.6K · ◷
_{Comment /matt to run again}

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on two correctness/security issues.

📋 Key Themes & Highlights

Key Themes

• Incomplete fix: The prerequisite repoIdQuery inside linkProjectToRepo still passes owner and name as raw -f arguments — the same injection surface the PR was intended to close. This is a blocking issue.
• Missing regression tests: Neither createProject/linkProjectToRepo in project_command_test.go nor RunGHInputContext in github_cli_test.go have tests that would catch a regression to the vulnerable pattern.
• Spinner safety: RunGHInputContext calls spinner.Stop() manually rather than via defer, leaving a gap if cmd.Output() panics.
• Pattern fragmentation: updateSingleSelectFieldOptions (pre-existing) still uses the lower-level ExecGH(...).Stdin path; this PR is a good opportunity to migrate it to RunGHInputContext for a single canonical approach.

Positive Highlights

• ✅ Clean fix for the two Semgrep-flagged mutations (createProject, linkProjectToRepo) — json.Marshal + --input - eliminates the raw concatenation correctly.
• ✅ RunGHInputContext follows the established runGHWithSpinnerContext pattern, making it easy to audit.
• ✅ Wasm stub is present and consistent with the rest of the wasm build file.
• ✅ The PR description is clear, includes before/after snippets, and references the existing safe pattern.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 112.1 AIC · ⌖ 6.31 AIC · ⊞ 6.6K
_{Comment /matt to run again}

Comments that could not be inline-anchored

pkg/workflow/github_cli.go:185

[/diagnosing-bugs] Spinner is not stopped if cmd.Output() panics — consider using defer spinner.Stop() to ensure the spinner always clears.

<details>
<summary>💡 Suggested change</summary>

spinner := console.NewSpinner(spinnerMessage)
spinner.Start()
defer spinner.Stop()
output, err := cmd.Output()
return output, enrichGHError(err)

The existing runGHWithSpinnerContext calls spinner.Stop() manually without defer, so neither is currently panic-safe. Since `RunGHInputCon…

pkg/cli/project_command.go:371

[/diagnosing-bugs] The repoIdQuery call in linkProjectToRepo still passes owner and name as raw -f arguments — the same injection surface Semgrep flagged. The PR fixed the mutation that follows, but not this prerequisite query.

<details>
<summary>💡 Suggested change</summary>

Replace the raw -f calls with a json.Marshal + --input - pattern consistent with the rest of this function:

repoIdQueryBody, err := json.Marshal(map[string]any{
    "query": repoIdQuery,
    "v…

</details>

<details><summary>pkg/cli/project_command.go:317</summary>

**[/tdd]** No regression test covers `createProject` using the new `--input -` path. If the `json.Marshal` + `RunGHInputContext` wiring regresses, the bug would be silent (no Semgrep alert, no test failure).

&lt;details&gt;
&lt;summary&gt;💡 What to test&lt;/summary&gt;

A table-driven test in `project_command_test.go` could mock `RunGHInputContext` (or use an `httptest` server via `GH_HOST`) and assert:
1. The request body sent to stdin is valid JSON with `query` and `variables` keys.
2. `ownerId` and `title` …

</details>

<details><summary>pkg/workflow/github_cli.go:176</summary>

**[/tdd]** `RunGHInputContext` is not covered in `github_cli_test.go` — there are tests for `RunGH` / `RunGHCombined` consistency but none for the new stdin-piping function.

&lt;details&gt;
&lt;summary&gt;💡 Suggested tests&lt;/summary&gt;

Add a test that:
1. Calls `RunGHInputContext` with a known `bytes.NewReader` payload and verifies stdin was passed through (e.g. via a helper command that echoes stdin).
2. Verifies the function returns the correct error when `cmd.Output()` fails and stderr is populated (mir…

</details>

<details><summary>pkg/cli/project_command.go:404</summary>

**[/codebase-design]** `updateSingleSelectFieldOptions` uses `workflow.ExecGH(...).Stdin = ...` directly (bypassing the spinner), while the two new call sites use `RunGHInputContext`. The same functional pattern is now expressed three ways in the same file — consider migrating `updateSingleSelectFieldOptions` to `RunGHInputContext` to consolidate the pattern.

&lt;details&gt;
&lt;summary&gt;Context&lt;/summary&gt;

- `createProject` → `RunGHInputContext` (new, with spinner + context)
- `linkProjectToRepo` → `Run…

</details>

<details><summary>pkg/workflow/github_cli_wasm.go:47</summary>

**[/codebase-design]** The wasm stub silently returns `nil, errors.New(&quot;gh CLI not available in Wasm&quot;)` without using the `input io.Reader` parameter at all. Go will not warn about the unused parameter, but it is worth noting in the function comment that the `input` argument is intentionally ignored in this build target.

&lt;details&gt;
&lt;summary&gt;💡 Suggested doc comment addition&lt;/summary&gt;

```go
// RunGHInputContext is a no-op stub for Wasm builds. The input reader is not used.
func RunGHInputContex…

</details>

[github-actions]

Security Fix Review — GraphQL ID Injection (CWE-89, Semgrep #627/#628)

Overall assessment: Correct and well-targeted fix. The two primary injection sites (createProject, linkProjectToRepo mutation) are properly remediated using the RunGHInputContext + --input - pattern with json.Marshal. The approach matches the existing updateSingleSelectFieldOptions pattern in the same file.

What's good

• createProject and linkProjectToRepo mutations now use a typed JSON body via json.Marshal + bytes.NewReader, eliminating raw string concatenation of API-returned node IDs (ownerId, title, projectId, repositoryId).
• RunGHInputContext is a clean, minimal abstraction that mirrors RunGHContext and correctly handles stdin piping and spinner lifecycle.
• The wasm stub correctly returns errors.New("gh CLI not available in Wasm"), consistent with other stubs.
• Mutation queries use typed variables ($ownerId: ID!, $title: String!, etc.) which are correctly parameterized.

Minor observation (non-blocking)

One inline comment on the repoIdQuery call site (line 371): it still uses -f owner= / -f name= with user-provided values. The query uses typed GraphQL variables so there is no injection risk — but for consistency with the new pattern and to avoid a future Semgrep flag, it could be migrated too.

Out-of-scope remaining -f usages (all safe)

• validateOwner / getOwnerNodeId (lines 240, 275): use escapeGraphQLString for owner literals inline in query strings — not changed by this PR.
• createView REST call (line 514): uses -f name= / -f layout= for a REST POST body (not GraphQL) — gh API handles form encoding correctly.
• getProjectFields (lines 680, 687): inline query strings with escaped owner login — not changed by this PR.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 130.9 AIC · ⌖ 6.17 AIC · ⊞ 4.9K

Comments that could not be inline-anchored

pkg/cli/project_command.go:371

The repoIdQuery call passes user-provided repoOwner and repoName values via -f owner= / -f name= string concatenation. This is technically safe because the query uses typed GraphQL variables ($owner: String!, $name: String!) and gh CLI properly binds them, but it is inconsistent with the new RunGHInputContext + --input - pattern adopted for the mutations below, and will likely trigger a future Semgrep flag on this call site.

For full consistency, convert to the JSON-body ap…

[github-actions]

The security fix is correct: both mutations now properly use typed JSON variables via json.Marshal + --input - instead of raw string concatenation, fully resolving Semgrep #627/#628. The 257 lock-file changes are automated recompile noise (adding "lyria" model alias) and are fine.

Issues found

Non-blocking issues (2 medium)

1. Missing defer spinner.Stop() (pkg/workflow/github_cli.go:185) — the new RunGHInputContext calls spinner.Stop() directly after cmd.Output(). A panic inside Output() will leave the terminal with an orphaned spinner. Fixing with defer is trivial.

2. Spinner logic duplicated (pkg/workflow/github_cli.go:176) — RunGHInputContext is a fourth copy of the same TTY/spinner block. This should instead extend runGHWithSpinnerContext with an optional io.Reader stdin parameter so future spinner-behavior changes propagate everywhere automatically.

Neither issue is a correctness or security bug — the core injection fix is sound. Both are straightforward to address before or in a follow-up.

🔎 Code quality review by PR Code Quality Reviewer · 208.9 AIC · ⌖ 5.18 AIC · ⊞ 5.4K
_{Comment /review to run again}

[github-actions]

spinner.Stop() is not deferred and will be skipped if cmd.Output() panics, leaving the terminal with an orphaned spinner.

💡 Suggested fix

Replace the explicit spinner.Stop() call with a defer:

spinner := console.NewSpinner(spinnerMessage)
spinner.Start()
defer spinner.Stop()
output, err := cmd.Output()
return output, enrichGHError(err)

Using defer costs nothing here and protects against terminal corruption if cmd.Output() panics (e.g., OS out of file descriptors). This pattern gap already exists in runGHWithSpinnerContext, RunGHWithHost, and RunGHContextWithHost throughout this file — this new function perpetuates rather than fixes the pattern.

[Copilot]

Fixed in f9e0f79 — runGHWithSpinnerContext (the shared helper) now uses defer spinner.Stop(), so RunGHInputContext inherits the safe pattern by delegation. Also updated RunGHWithHost and RunGHContextWithHost to use defer in 1984fb3.

[github-actions]

RunGHInputContext duplicates the full spinner-management pattern from runGHWithSpinnerContext rather than extending that private helper to accept an optional io.Reader stdin.

💡 Why this matters

The codebase already has four copies of the same spinner management block (runGHWithSpinnerContext, RunGHWithHost, RunGHContextWithHost, and now RunGHInputContext). Every time the spinner behavior needs to change (e.g., always-defer above, context-cancel propagation, non-TTY fallback), all four copies must be updated in sync.

A more maintainable approach would be to extend runGHWithSpinnerContext with an io.Reader parameter (or a functional-options pattern) so that all callers benefit from a single implementation:

func runGHWithSpinnerContext(ctx context.Context, msg string, combined bool, stdin io.Reader, args ...string) ([]byte, error) {
    cmd := ExecGHContext(ctx, args...)
    if stdin != nil {
        cmd.Stdin = stdin
    }
    // single spinner block
}

This is a maintainability concern, not a correctness bug — but the duplication will compound with every future helper added.

[Copilot]

Addressed in f9e0f79 — runGHWithSpinnerContext now accepts an optional io.Reader stdin parameter. RunGHInputContext delegates to it (one line), and RunGHContext/RunGHCombinedContext pass nil. All four spinner-using functions now share a single implementation path.

done

[pelikhan]

@copilot invoke pr-finisher skill

[gh-aw-bot]

@copilot please run the pr-finisher skill and address the unresolved review feedback, including the remaining linkProjectToRepo input-handling and missing regression-test follow-ups, then rerun checks if you make changes.

Generated by 👨‍🍳 PR Sous Chef · 12.3 AIC · ⌖ 5.1 AIC · ⊞ 3.5K · ◷
_{Comment /souschef to run again}

[github-actions]

🤖 PR Triage — Run #7

Field	Value
Category	bug (security)
Risk	🔴 high
Score	80 / 100
Action	⚡ fast_track
Batch	pr-batch:security-fixes

Score breakdown: impact 40 + urgency 28 + quality 12

Fixes GraphQL ID injection (Semgrep #627/#628, CWE-89) in project_command.go. Moves both mutation call sites to json.Marshal + RunGHInputContext piping via --input -, eliminating raw string concatenation of API-returned node IDs. 3 files, +69/-26. Security fix — expedite human review alongside #43468.

Generated by 🔧 PR Triage Agent · 56.3 AIC · ⌖ 8.84 AIC · ⊞ 5.5K · ◷

[gh-aw-bot]

@copilot please run the pr-finisher skill, review the remaining feedback, ensure the branch is current, and rerun checks.

Generated by 👨‍🍳 PR Sous Chef · 16.1 AIC · ⌖ 6.23 AIC · ⊞ 4.6K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot please run the pr-finisher skill, review the remaining feedback, ensure the branch is current, and rerun checks.

...

Done. The only open item was the CI failure in TestPRSousChefWorkflowAddCommentTargetContract (line 30) — the test was checking for "Process at most **5 PRs** per run." but commit 19d59cd (merged from main) intentionally changed the workflow to process all eligible PRs. Fixed in the latest commit by updating the assertion to match the current contract.

Local validation: make fmt, make lint, make test-unit all green. CI needs a human re-trigger after the agent push.

[gh-aw-bot]

@copilot please run the pr-finisher skill, address any unresolved review feedback, ensure the branch is up to date with the base branch, and rerun checks.

Generated by 👨‍🍳 PR Sous Chef · 8.37 AIC · ⌖ 7.92 AIC · ⊞ 3.7K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot please run the pr-finisher skill, address any unresolved review feedback, ensure the branch is up to date with the base branch, and rerun checks.

...

Done. Current state:

• ✅ Reviews — The unresolved reviewer thread about .lock.yml changes is already addressed: the current diff contains no lock files (only the 6 expected files: project_command.go, project_command_test.go, github_cli.go, github_cli_test.go, github_cli_wasm.go, pr_sous_chef_workflow_contract_test.go).
• ✅ Checks (local) — make fmt, make lint, make test-unit, and make agent-report-progress all pass.
• ❓ Checks (CI) — Stale after agent push; needs human re-trigger. Smoke CI at HEAD (bbb5eb82) is ✅. Prior CGO failure at dc73f9ca was the pr-sous-chef contract test, fixed in 4446af3.
• ✅ Mergeable — Branch is up to date with main (last commit is a merge from main).

Hand-off: CI must be re-triggered by a maintainer (close/reopen PR or workflow_dispatch) before merge.

[github-actions]

🤖 PR Triage — Run §28741077056

Field	Value
Category	bug
Risk	🔴 High
Priority Score	83/100 — impact:43 urgency:25 quality:15
Action	🚀 fast_track
Batch	pr-batch:security-fixes

Rationale: CWE-89 GraphQL injection fix in critical-path CLI file. Carry-over from prior run — needs human review ASAP.

Generated by 🔧 PR Triage Agent · 91.2 AIC · ⌖ 11.9 AIC · ⊞ 5.5K · ◷

[gh-aw-bot]

@copilot please run the pr-finisher skill, address unresolved review comments, and rerun checks once the branch is up to date.

Generated by 👨‍🍳 PR Sous Chef · 8.12 AIC · ⌖ 9.29 AIC · ⊞ 3.7K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot please run the pr-finisher skill, address unresolved review comments, and rerun checks once the branch is up to date.

...

Done. Merged main into the branch (resolved a trivial conflict in pr_sous_chef_workflow_contract_test.go — both sides asserted the same string, just with slightly different message text). Local validation passes (make fmt, make agent-report-progress, targeted unit tests). PR diff is now 5 files only (no lock files). CI needs a human re-trigger to confirm on the updated HEAD.