feat: compile-time warning when slash_command and bots are configured together#29008
feat: compile-time warning when slash_command and bots are configured together#29008
Conversation
…sue) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/063087cd-de65-4d57-ba91-37d72c50058f Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a compile-time warning to help users avoid an unintended trigger interaction when on.slash_command and on.bots are configured together.
Changes:
- Emit a compiler
warningwhen bothworkflowData.Command(slash_command) andworkflowData.Botsare non-empty. - Add an integration test suite covering warning/no-warning scenarios for slash_command and bots combinations.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_validators.go | Adds a new validation warning for conflicting slash_command + bots configuration. |
| pkg/workflow/slash_command_bots_warning_test.go | Adds integration tests asserting the warning behavior across configuration permutations. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/workflow/slash_command_bots_warning_test.go:127
- The read end of the pipe (
r) isn't closed andio.Copy's error/byte count are ignored. This can leak file descriptors across the integration test suite. Closerafter reading (preferdefer r.Close()right after creation) and check theio.Copyerror to ensure stderr capture succeeded.
w.Close()
os.Stderr = oldStderr
var buf bytes.Buffer
io.Copy(&buf, r)
stderrOutput := buf.String()
- Files reviewed: 2/2 changed files
- Comments generated: 1
| r, w, _ := os.Pipe() | ||
| os.Stderr = w | ||
|
|
||
| compiler := NewCompiler() | ||
| compiler.SetStrictMode(false) | ||
| err := compiler.CompileWorkflow(testFile) | ||
|
|
||
| w.Close() | ||
| os.Stderr = oldStderr | ||
| var buf bytes.Buffer | ||
| io.Copy(&buf, r) |
There was a problem hiding this comment.
os.Pipe() errors are being ignored and os.Stderr is reassigned without a defer/t.Cleanup restore. If CompileWorkflow (or any earlier call) fails the test via panic/fatal, this can leak the modified os.Stderr into other tests. Capture and handle the os.Pipe error, and restore os.Stderr via defer/t.Cleanup immediately after reassignment.
This issue also appears on line 123 of the same file.
| r, w, _ := os.Pipe() | |
| os.Stderr = w | |
| compiler := NewCompiler() | |
| compiler.SetStrictMode(false) | |
| err := compiler.CompileWorkflow(testFile) | |
| w.Close() | |
| os.Stderr = oldStderr | |
| var buf bytes.Buffer | |
| io.Copy(&buf, r) | |
| r, w, err := os.Pipe() | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| os.Stderr = w | |
| t.Cleanup(func() { | |
| os.Stderr = oldStderr | |
| _ = w.Close() | |
| _ = r.Close() | |
| }) | |
| compiler := NewCompiler() | |
| compiler.SetStrictMode(false) | |
| err = compiler.CompileWorkflow(testFile) | |
| if err := w.Close(); err != nil { | |
| t.Fatal(err) | |
| } | |
| os.Stderr = oldStderr | |
| var buf bytes.Buffer | |
| if _, err := io.Copy(&buf, r); err != nil { | |
| t.Fatal(err) | |
| } |
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification Details
Flagged Tests — Requires Review
|
![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: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All four scenarios verify observable compiler behavior with proper positive and negative cases. No coding-guideline violations.
…and + bots conflict Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (164 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §25085998674
|
|
@copilot review all comments |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/918651d3-e28b-434c-8cc1-51c68f067ba1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot analyze how slash_commands and bots (copilot code review) may clash |
Here's how the clash happens: The trigger sequence
Why the slash command invocation then gets blocked
The practical impact with copilot code review
What the warning targets The compile-time warning fires precisely when |
|
@copilot the comment posted by the bot should also comply with the /slash-command string matching requirement |
…rement Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7489c959-1896-4849-82b3-e2a2b047a754 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Correct — the Updated the warning text in 6e7ba1b to accurately reflect this: the risk is only when a bot in New text:
|
Combining
slash_commandwithbots:creates a potential concurrency clash. When a bot listed inbots:posts a comment that starts with the slash command text (e.g.,/command-name), thecheck_command_positioncheck passes and the bot triggers the workflow — occupying the concurrency slot and potentially blocking a simultaneous manual invocation (sincecancel-in-progressis disabled for command-trigger workflows).Changes
pkg/workflow/compiler_validators.go— Emits awarninginvalidateToolConfigurationwhenworkflowData.Command(slash_command) andworkflowData.Botsare both non-empty, pointing users to removebots:if they want exclusively user-driven slash-command execution.pkg/workflow/slash_command_bots_warning_test.go— Integration tests covering: both configured → warning, slash_command only → no warning, bots only → no warning, multiple bots → warning.Example
A frontmatter like this now produces a compile-time warning: