Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions docs/adr/29008-compile-time-warning-slash-command-bots-conflict.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# ADR-29008: Compile-Time Warning for slash_command + bots Conflict

**Date**: 2026-04-29
**Status**: Draft
**Deciders**: pelikhan, copilot-swe-agent

---

## Part 1 — Narrative (Human-Friendly)

### Context

Agentic workflow frontmatter supports two independent trigger mechanisms: `slash_command` (explicit manual invocation by a user) and `bots` (automatic invocation when a named bot actor performs a matching event). When both are configured in the same workflow, bot activity — such as `copilot[bot]` opening a pull request — can match the `slash_command` event types (e.g., `pull_request`, `pull_request_comment`) and start the workflow automatically. This causes any subsequent manual slash command invocation to be silently ignored while the bot-triggered run is already in progress. The failure mode is invisible to the workflow author at authoring time and difficult to diagnose at runtime.

### Decision

We will emit a non-fatal compile-time warning in `validateToolConfiguration` whenever a workflow's parsed frontmatter contains both a non-empty `slash_command` (via `workflowData.Command`) and a non-empty `bots` list (via `workflowData.Bots`). The warning message names the conflict, provides a concrete example of how it manifests, and directs the author to remove the `bots:` field if exclusively slash-command-driven execution is intended. Using a warning rather than a hard error preserves backward compatibility for any workflow that intentionally wants both triggers.

### Alternatives Considered

#### Alternative 1: Hard Compiler Error

Reject compilation entirely when both `slash_command` and `bots` are present. This is more forceful and guarantees the conflict never reaches production, but it is a breaking change for any existing workflows that may have intentionally paired the triggers for a dual-invocation use case. A hard error would require all such authors to migrate immediately, with no grace period.

#### Alternative 2: Documentation-Only

Document the conflict in the workflow authoring guide without any code change. This is the least disruptive approach, but it provides no signal at the point of workflow authoring and leaves authors who have not read the documentation entirely exposed to the silent failure mode.

#### Alternative 3: Auto-Remove bots: at Compile Time

Silently strip the `bots:` field from the compiled output when `slash_command` is also present. This eliminates the conflict without requiring author action, but it modifies the workflow against the author's explicit configuration without consent or explanation, which is worse than the original problem.

### Consequences

#### Positive
- Authors receive an actionable, human-readable warning at compile time — before the workflow is deployed — rather than discovering the issue through silent runtime failures.
- The warning message includes both the root cause and the recommended fix, minimizing the debugging burden.

#### Negative
- The warning is non-fatal and can be ignored; a deployed workflow with both triggers will still exhibit the conflict if the author does not act on the warning.
- The check must be updated if the semantics of `slash_command` event routing change (e.g., if `slash_command` is decoupled from `pull_request` event types in a future compiler revision).

#### Neutral
- The warning is emitted to `stderr` using the existing `formatCompilerMessage` infrastructure, consistent with all other compiler diagnostics.
- Warning count is incremented via `IncrementWarningCount()`, so tooling that surfaces the aggregate warning count will reflect the new check without further changes.

---

## Part 2 — Normative Specification (RFC 2119)

> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).

### Conflict Detection

1. The compiler **MUST** emit a warning diagnostic when a workflow's parsed frontmatter contains both a non-empty `slash_command` trigger (i.e., `workflowData.Command` is non-empty) and a non-empty `bots` list (i.e., `workflowData.Bots` has at least one entry).
2. The warning **MUST** be written to `stderr` using the `formatCompilerMessage` helper with severity `"warning"`.
3. The warning **MUST** increment the compiler's warning counter via `IncrementWarningCount()`.
4. Compilation **MUST NOT** fail (i.e., return a non-nil error) solely because this conflict is detected; the check **MUST** be non-fatal.
5. The warning message **MUST** identify the conflict, describe the failure mode (bot activity starting the workflow before a manual slash command), and provide a remediation step (removing the `bots:` field).

### Conflict Scope

1. The check **MUST** fire if and only if both conditions are true simultaneously: `len(workflowData.Command) > 0` AND `len(workflowData.Bots) > 0`.
2. A workflow with `slash_command` and no `bots` entries **MUST NOT** trigger the warning.
3. A workflow with `bots` entries and no `slash_command` **MUST NOT** trigger the warning.
4. The number of entries in `workflowData.Bots` (one vs. multiple) **MUST NOT** affect whether the warning fires; the warning **MUST** fire for any non-zero `bots` list size when `slash_command` is also present.

### Conformance

An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. In particular, converting the warning to a hard error or suppressing the `IncrementWarningCount()` call both constitute non-conformance.

---

*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/25085998674) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*
14 changes: 14 additions & 0 deletions pkg/workflow/compiler_validators.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,20 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow
c.IncrementWarningCount()
}

// Warn when slash_command and bots are both configured: if a bot listed in bots: posts
// a comment that starts with the slash command text (e.g. /command-name), the
// check_command_position check will pass and the bot will trigger the workflow —
// occupying the concurrency slot and potentially blocking a simultaneous manual invocation.
if len(workflowData.Command) > 0 && len(workflowData.Bots) > 0 {
fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning",
"Both slash_command and bots triggers are configured. If a bot listed in bots: "+
"posts a comment that starts with the slash command text (e.g., /command-name), "+
"it will trigger the workflow and occupy the concurrency slot, potentially "+
"blocking simultaneous manual invocations. To ensure the workflow only runs on "+
"explicit user commands, remove the 'bots:' field."))
c.IncrementWarningCount()
}

// Inform users when this workflow is a redirect stub for updates.
if workflowData.Redirect != "" {
fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "info",
Expand Down
162 changes: 162 additions & 0 deletions pkg/workflow/slash_command_bots_warning_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
//go:build integration

package workflow

import (
"bytes"
"io"
"os"
"path/filepath"
"strings"
"testing"

"github.com/github/gh-aw/pkg/testutil"
)

// TestSlashCommandBotsWarning tests that combining slash_command and bots triggers
// emits a compile-time warning about the potential conflict.
func TestSlashCommandBotsWarning(t *testing.T) {
tests := []struct {
name string
content string
expectWarning bool
}{
{
name: "slash_command with bots produces conflict warning",
content: `---
on:
slash_command:
name: rust-review
events: [pull_request, pull_request_comment]
bots:
- "copilot[bot]"
engine: copilot
permissions:
contents: read
pull-requests: read
issues: read
---

# Rust Review Workflow
Review Rust code on demand.
`,
expectWarning: true,
},
{
name: "slash_command without bots does not produce warning",
content: `---
on:
slash_command:
name: rust-review
events: [pull_request, pull_request_comment]
engine: copilot
permissions:
contents: read
pull-requests: read
issues: read
---

# Rust Review Workflow
Review Rust code on demand.
`,
expectWarning: false,
},
{
name: "bots without slash_command does not produce warning",
content: `---
on:
pull_request:
types: [opened]
bots:
- "dependabot[bot]"
engine: copilot
permissions:
contents: read
pull-requests: read
---

# Dependabot Workflow
Handle dependabot PRs.
`,
expectWarning: false,
},
{
name: "slash_command with multiple bots produces conflict warning",
content: `---
on:
slash_command:
name: review
bots:
- "copilot[bot]"
- "renovate[bot]"
engine: copilot
permissions:
contents: read
pull-requests: read
issues: read
---

# Review Workflow
Review code on demand.
`,
expectWarning: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
tmpDir := testutil.TempDir(t, "slash-command-bots-warning-test")

testFile := filepath.Join(tmpDir, "test-workflow.md")
if err := os.WriteFile(testFile, []byte(tt.content), 0644); err != nil {
t.Fatal(err)
}

oldStderr := os.Stderr
r, w, pipeErr := os.Pipe()
if pipeErr != nil {
t.Fatal(pipeErr)
}
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)
}
stderrOutput := buf.String()

if err != nil {
t.Errorf("expected compilation to succeed but it failed: %v", err)
return
}

expectedPhrase := "Both slash_command and bots triggers are configured"
if tt.expectWarning {
if !strings.Contains(stderrOutput, expectedPhrase) {
t.Errorf("expected warning containing %q, got stderr:\n%s", expectedPhrase, stderrOutput)
}
if compiler.GetWarningCount() == 0 {
t.Error("expected warning count > 0 but got 0")
}
return
}

if strings.Contains(stderrOutput, expectedPhrase) {
t.Errorf("did not expect warning %q, but got stderr:\n%s", expectedPhrase, stderrOutput)
}
})
}
}