Conversation
|
@copilot review what generated SENT_DIST_URL |
…in safe-outputs.steps (#29085) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/1035e50f-37cf-4419-9661-839ea3584a2e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
…hen handling chart URLs Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7482b2f0-e47d-453a-9615-094212c7d72f Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The Fixed in d1fea34:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. |
🧪 Test Quality Sentinel ReportTest Quality Score: 97/100✅ Excellent test quality
Test Classification DetailsView All Test Classifications (20 tests)
Flagged Tests — Requires ReviewNo tests flagged. One minor observation (non-blocking): i️
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 97/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 20 test scenarios cover behavioral contracts with full error-path coverage, correct build tag, no mock libraries, and no test inflation.
There was a problem hiding this comment.
Pull request overview
Adds compile-time validation to detect shell expansion patterns in safe-outputs.steps[].run that are blocked by the safe-outputs runtime harness, and updates the Copilot PR NLP Analysis workflow prompt to avoid triggering those runtime blocks when handling uploaded chart URLs.
Changes:
- Introduces
validateSafeOutputsStepsShellExpansion/validateRunScriptForShellExpansionto scansafe-outputs.steps[].runfor blocked bash constructs and emit compiler errors with actionable guidance. - Registers the new validator in the compiler validation pipeline.
- Updates
.github/workflows/copilot-pr-nlp-analysis.mdto record uploaded chart URLs to files and later build the discussion body from those values.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_steps_shell_expansion_validation.go |
New compile-time validator and error messaging for blocked shell expansion patterns in safe-outputs.steps[].run. |
pkg/workflow/safe_outputs_steps_shell_expansion_validation_test.go |
Unit tests covering allowed vs blocked patterns and error message contents. |
pkg/workflow/compiler_validators.go |
Wires the new validator into the compiler validation sequence. |
.github/workflows/copilot-pr-nlp-analysis.md |
Prompt changes to avoid generating blocked shell expansion patterns when embedding upload URLs. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 4
| // - "commandSubst" matches starting with "$({" or "$({{" that form "${{" are ignored | ||
| // because "${{" is not a valid shell construct (GitHub Actions uses "${{ }}"). |
There was a problem hiding this comment.
The inline comment claims that command substitution matches starting with "$({" or "$({{" should be ignored because of GitHub Actions "${{ }}" expressions, but the implementation only skips "$((" arithmetic expansion and does not (and probably should not) exempt "$({". Please update the comment (or implement the described exemption if it's truly required) so it matches the actual behavior and GitHub Actions expression syntax ("${{" not "$({{").
| // - "commandSubst" matches starting with "$({" or "$({{" that form "${{" are ignored | |
| // because "${{" is not a valid shell construct (GitHub Actions uses "${{ }}"). | |
| // - GitHub Actions expressions use "${{ ... }}", not "$(...)", so they do not | |
| // match the "commandSubst" pattern. |
| "Avoid shell variable expansion in safe-outputs run scripts. "+ | ||
| "Write URL values and other dynamic content to files in /tmp/gh-aw/agent/ "+ | ||
| "during the agent turn, then read the file contents in the safe-outputs step "+ | ||
| "without using shell expansion (e.g. with 'cat' or a script argument)", |
There was a problem hiding this comment.
The remediation text says "Avoid shell variable expansion in safe-outputs run scripts", but this validator explicitly allows simple "$VAR" and "${VAR}" expansions (and tests cover those as allowed). This wording is misleading—please narrow it to the specific blocked constructs (command substitution/backticks/indirect expansion/parameter transformation) or describe the safe alternative without implying all variable expansion is forbidden.
| "Avoid shell variable expansion in safe-outputs run scripts. "+ | |
| "Write URL values and other dynamic content to files in /tmp/gh-aw/agent/ "+ | |
| "during the agent turn, then read the file contents in the safe-outputs step "+ | |
| "without using shell expansion (e.g. with 'cat' or a script argument)", | |
| "Avoid command substitution, backticks, indirect expansion, and parameter "+ | |
| "transformation in safe-outputs run scripts. Write URL values and other "+ | |
| "dynamic content to files in /tmp/gh-aw/agent/ during the agent turn, then "+ | |
| "read the file contents in the safe-outputs step (e.g. with 'cat' or by "+ | |
| "passing a script argument)", |
| func TestValidateRunScriptForShellExpansion(t *testing.T) { | ||
| t.Run("empty script passes", func(t *testing.T) { | ||
| err := validateRunScriptForShellExpansion(0, "") | ||
| assert.NoError(t, err, "empty script should pass") | ||
| }) | ||
|
|
||
| t.Run("error includes step index", func(t *testing.T) { | ||
| err := validateRunScriptForShellExpansion(3, "$(echo bad)") | ||
| require.Error(t, err, "command substitution should be rejected") | ||
| assert.Contains(t, err.Error(), "safe-outputs.steps[3]", | ||
| "error should include the step index") | ||
| }) | ||
|
|
||
| t.Run("error includes offending snippet", func(t *testing.T) { | ||
| err := validateRunScriptForShellExpansion(0, `URL=$(cat /tmp/url.txt)`) | ||
| require.Error(t, err, "should reject command substitution") | ||
| // The snippet includes at least the $( opener | ||
| assert.Contains(t, err.Error(), "$(", "error should include the offending snippet") | ||
| }) | ||
|
|
||
| t.Run("error includes remediation guidance", func(t *testing.T) { | ||
| err := validateRunScriptForShellExpansion(0, "$(echo hi)") | ||
| require.Error(t, err, "should reject command substitution") | ||
| assert.Contains(t, err.Error(), "/tmp/gh-aw/agent/", | ||
| "error should include remediation guidance about writing to a file") | ||
| }) | ||
|
|
||
| t.Run("long snippet is truncated", func(t *testing.T) { | ||
| longScript := "$(echo " + strings.Repeat("a", 100) + ")" | ||
| err := validateRunScriptForShellExpansion(0, longScript) | ||
| require.Error(t, err, "should reject command substitution") | ||
| // Snippet in error should not exceed 60+3="..." = 63 characters | ||
| assert.NotContains(t, err.Error(), strings.Repeat("a", 61), | ||
| "snippet in error message should be truncated") | ||
| }) | ||
| } |
There was a problem hiding this comment.
The implementation intentionally skips arithmetic expansion "$((" (see validateRunScriptForShellExpansion), and the PR description mentions tests for this edge case, but there isn't currently a test asserting that "$((1+1))" (or similar) is allowed. Adding an explicit passing test would prevent regressions and keep the test suite aligned with the intended behavior.
| 2. **Upload each chart** using the `upload asset` MCP tool (call it directly — do NOT wrap in a shell command or use `$()` to capture the URL) | ||
|
|
||
| 3. **Record the returned URL** from each upload by writing it to a plain text file in `/tmp/gh-aw/agent/` immediately after the MCP tool returns: | ||
| - `sentiment_distribution.png` → write URL to `/tmp/gh-aw/agent/url-sentiment-distribution.txt` | ||
| - `sentiment_timeline.png` → write URL to `/tmp/gh-aw/agent/url-sentiment-timeline.txt` | ||
| - `topic_frequencies.png` → write URL to `/tmp/gh-aw/agent/url-topic-frequencies.txt` | ||
| - `topics_wordcloud.png` → write URL to `/tmp/gh-aw/agent/url-topics-wordcloud.txt` | ||
| - `keyword_trends.png` → write URL to `/tmp/gh-aw/agent/url-keyword-trends.txt` | ||
|
|
||
| For example, after the `upload asset` tool returns `https://github.com/.../chart.png`, write it with: | ||
| ```bash | ||
| echo -n "https://github.com/.../chart.png" > /tmp/gh-aw/agent/url-sentiment-distribution.txt | ||
| ``` | ||
|
|
||
| **Do NOT** store URLs in shell variables or use command substitution (`$(...)`) — this triggers the security harness. | ||
|
|
||
| ### Phase 6: Create Analysis Discussion | ||
|
|
||
| Build the discussion body by reading the URL files saved in Phase 5, then post a comprehensive discussion. | ||
|
|
||
| **Before constructing the body**, read the uploaded chart URLs: | ||
| ```bash | ||
| SENTIMENT_DIST_URL=$(cat /tmp/gh-aw/agent/url-sentiment-distribution.txt 2>/dev/null || echo "") | ||
| SENTIMENT_TIME_URL=$(cat /tmp/gh-aw/agent/url-sentiment-timeline.txt 2>/dev/null || echo "") | ||
| TOPIC_FREQ_URL=$(cat /tmp/gh-aw/agent/url-topic-frequencies.txt 2>/dev/null || echo "") | ||
| TOPICS_CLOUD_URL=$(cat /tmp/gh-aw/agent/url-topics-wordcloud.txt 2>/dev/null || echo "") | ||
| KEYWORD_TRENDS_URL=$(cat /tmp/gh-aw/agent/url-keyword-trends.txt 2>/dev/null || echo "") | ||
| ``` |
There was a problem hiding this comment.
Phase 5 explicitly says not to store URLs in shell variables or use command substitution ($(...)), but Phase 6 immediately uses SENTIMENT_DIST_URL=$(cat ...) (both a shell variable and command substitution). If the intent is "don’t do this inside safe-outputs.steps run scripts" but it’s OK in the agent bash turn, please clarify the wording in Phase 5 (or adjust Phase 6 to avoid $()/shell vars by reading the files directly in Python) to remove the internal contradiction.
See below for a potential fix:
**Before constructing the body**, use Python to read the uploaded chart URLs directly from the files saved in `/tmp/gh-aw/agent/`. Do not use shell variables or command substitution here either; have Python read each file and treat missing files as empty strings.
Use a Python script to read:
- `/tmp/gh-aw/agent/url-sentiment-distribution.txt`
- `/tmp/gh-aw/agent/url-sentiment-timeline.txt`
- `/tmp/gh-aw/agent/url-topic-frequencies.txt`
- `/tmp/gh-aw/agent/url-topics-wordcloud.txt`
- `/tmp/gh-aw/agent/url-keyword-trends.txt`
Then write the fully-substituted discussion body to `/tmp/gh-aw/agent/discussion_body.md`, inserting the literal URL strings directly into the markdown body. After that, pass the body to the `create_discussion` safe-output tool.
Post a comprehensive discussion with the following structure:
**Title**: `Copilot PR Conversation NLP Analysis - [DATE]`
**Content Template** (substitute `[SENTIMENT_DIST_URL]`, `[SENTIMENT_TIME_URL]`, `[TOPIC_FREQ_URL]`, `[TOPICS_CLOUD_URL]`, and `[KEYWORD_TRENDS_URL]` with the literal URL strings read by Python from the files above):
…tputs shell expansion Generated by the Design Decision Gate workflow. The PR author should review, complete, and finalize before changing status from Draft to Accepted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
|
@/tmp/gh-aw/agent/comment-body.md
|
…trict mode Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4416e886-fc60-4fe4-be34-8583c9b1ca0d Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit
Recompiled all 205 workflows with the updated binary — all compile cleanly with zero new shell expansion warnings. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot review all comments |
…flow prompt Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a049b317-5c77-409c-8266-695748d751b9 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all 4 review comments in commit
|
The safe-outputs security harness blocks shell expansion patterns at runtime (
${var@op},${!var},$(...), backticks), but previously gave no signal at compile time — leading to confusing runtime failures. Additionally, the Copilot PR NLP Analysis workflow prompt was generating$(...)command substitution when handling chart upload URLs, directly triggering the runtime block.Changes
Compiler validation (new)
New validator
validateSafeOutputsStepsShellExpansionscans everyrun:script insafe-outputs.steps[]for the four blocked patterns:${var@operator}— parameter transformation${!var}— indirect expansion$(...)— command substitution (arithmetic$((excluded)`...`— backtick command substitutionSimple
$VAR,${VAR}, and${{ }}GitHub Actions expressions are allowed.Registered in
compiler_validators.goaftervalidateSafeOutputsMax.Severity: warning in non-strict mode (compilation continues) and hard error in strict mode.
18 tests covering all pattern categories, safe pass-through cases, and edge cases (arithmetic expansion
$((allowed, GitHub Actions expressions, non-map steps).Error message accurately describes only the blocked constructs ("Avoid command substitution, backticks, indirect expansion, and parameter transformation") rather than implying all variable expansion is forbidden.
Root cause fix —
copilot-pr-nlp-analysis.mdThe
SENT_DIST_URLvariable was generated by the agent because:SENT_DIST_URL="$(upload_asset ...)"command substitution to capture each URL.URL_FROM_UPLOAD_ASSET_*placeholders that the agent replaced by expanding those shell variables in a bash command passed to the safe-output step.Fix: Phase 5 now explicitly instructs the agent to write each URL to a plain file (
/tmp/gh-aw/agent/url-<name>.txt) immediately after the MCP tool returns, and explicitly warns against$(...)command substitution. Phase 6 now uses a Python script to read the URL files directly viaopen()— eliminating the previous contradiction where Phase 5 forbade$(...)but Phase 6 used$(cat ...)shell command substitution. Python builds the fully-substituted discussion body and writes it to a file before passing it tocreate_discussion.