Skip to content

Address review feedback: dedupe normalized Claude Bash tools and tighten release bash permissions#31615

Merged
pelikhan merged 1 commit into
mainfrom
copilot/review-all-comments-pr-31599
May 12, 2026
Merged

Address review feedback: dedupe normalized Claude Bash tools and tighten release bash permissions#31615
pelikhan merged 1 commit into
mainfrom
copilot/review-all-comments-pr-31599

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 12, 2026

Bug Fix

This PR addresses all review comments from PR review 4268553507. It resolves formatting drift in new tests, removes duplicate normalized Claude Bash(...) allowlist entries, and narrows overly broad release workflow shell permissions.

What was the bug?

  • Duplicate Bash(...) entries could be emitted after wildcard normalization (for example both jq * and jq normalizing to jq).
  • release.md allowed plain bash, which enabled arbitrary bash -c ... execution instead of command-scoped access.
  • Newly added normalization test block in engine_helpers_test.go was not gofmt-formatted.

How did you fix it?

  • pkg/workflow/claude_tools.go — de-duplicate canonicalized allowlist entries

    • Added a final uniqueness pass over allowedTools before sorting/joining, preventing repeated Bash(cmd) tokens in generated output.
  • pkg/workflow/claude_engine_tools_test.go — lock in dedup behavior

    • Added a regression test for ["jq *", "jq"] to verify output contains a single Bash(jq).
  • pkg/workflow/engine_helpers_test.go — enforce style consistency

    • Applied gofmt to the new TestNormalizeBashCommand block.
  • .github/workflows/release.md (+ regenerated .github/workflows/release.lock.yml) — tighten shell scope

    • Removed bash from the workflow bash tool list, preserving explicit command allowlisting only.
// Deduplicate tools before sort/join to avoid duplicate Bash(...) entries
seen := make(map[string]struct{}, len(allowedTools))
deduped := make([]string, 0, len(allowedTools))
for _, tool := range allowedTools {
	if _, ok := seen[tool]; ok {
		continue
	}
	seen[tool] = struct{}{}
	deduped = append(deduped, tool)
}
allowedTools = deduped

Testing

  • Added targeted regression coverage for normalized-bash deduplication in Claude tool generation.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title Address review feedback from PR #31599 Address review feedback: dedupe normalized Claude Bash tools and tighten release bash permissions May 12, 2026
Copilot AI requested a review from pelikhan May 12, 2026 02:04
@pelikhan pelikhan marked this pull request as ready for review May 12, 2026 02:33
Copilot AI review requested due to automatic review settings May 12, 2026 02:33
@pelikhan pelikhan merged commit 0a9aebd into main May 12, 2026
80 of 81 checks passed
@pelikhan pelikhan deleted the copilot/review-all-comments-pr-31599 branch May 12, 2026 02:35
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses prior review feedback by ensuring normalized Claude bash tool permissions don’t produce duplicate Bash(...) entries and by tightening release workflow shell permissions while keeping tests and formatting consistent.

Changes:

  • De-duplicates the final computed Claude --allowed-tools list to prevent repeated Bash(cmd) tokens after wildcard normalization.
  • Adds a regression test to lock in dedup behavior for ["jq *", "jq"] → single Bash(jq).
  • Removes broad bash permission from the release workflow tool allowlist and regenerates the lockfile output accordingly.
Show a summary per file
File Description
pkg/workflow/engine_helpers_test.go gofmt formatting cleanup for TestNormalizeBashCommand.
pkg/workflow/claude_tools.go Adds a dedupe pass over allowedTools before sorting/joining.
pkg/workflow/claude_engine_tools_test.go Adds regression coverage to ensure normalized bash entries are deduplicated.
.github/workflows/release.md Removes bash from the bash tool allowlist to avoid unrestricted bash -c ....
.github/workflows/release.lock.yml Regenerated lock output reflecting the removed shell(bash) permission.

Copilot's findings

Tip

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

  • Files reviewed: 4/5 changed files
  • Comments generated: 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants