Advertise typed workflow safe-output tools - #54817
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has 79 new lines of code in business logic directories (threshold: 100).
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Ponytail over-engineering review found nothing to cut: the typed/generic if-else branches for DispatchWorkflow, DispatchRepository, and CallWorkflow are minimal and non-redundant (each operates on a distinct config type), test additions are real regression coverage, and the schema.json change is a no-op key reorder.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
Non-blocking change overall: the prompt now advertises typed safe-output tool identifiers and the regression tests cover the drift that caused the bug.
Highlights
buildSafeOutputsSectionsnow lists configured dispatch/call tool identifiers instead of steering agents toward generic names.- The fallback to generic
dispatch_workflow,dispatch_repository, andcall_workflowwhen no typed targets are configured preserves backward compatibility. - The added tests cover jobs, scripts, actions, dispatch-workflow targets, dispatch-repository targets, and reusable workflow targets, which is the right regression surface for this fix.
I did not find a changed-line correctness or performance issue worth blocking this PR on.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 9.7 AIC · ⌖ 6.77 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Updates agent prompts to advertise typed safe-output tools instead of generic workflow dispatch names.
Changes:
- Lists normalized per-target workflow and repository tools.
- Adds regression coverage for typed tool manifests.
- Refreshes schema ordering and generated command routing.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/unified_prompt_step.go |
Builds typed safe-output prompt entries. |
pkg/workflow/safe_outputs_prompt_tools_test.go |
Tests typed tool advertisement. |
pkg/workflow/schemas/github-workflow.json |
Reorders the drives permission schema. |
.github/workflows/agentic_commands.yml |
Adds generated smoke-drive routing. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
pkg/workflow/unified_prompt_step.go:619
CallWorkflow.Maxis enforced globally across all reusable-workflow targets by one handler counter (actions/setup/js/call_workflow.cjs:28,36,49). Annotating each typed target with the same(max:N)incorrectly presents that limit as per-target, so an agent can emit calls that the handler rejects once the shared budget is exhausted. Please represent the limit as shared across this tool family.
for _, workflowName := range safeOutputs.CallWorkflow.Workflows {
tools = append(tools, toolWithMaxBudget(stringutil.NormalizeSafeOutputIdentifier(workflowName), safeOutputs.CallWorkflow.Max))
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| for _, workflowName := range safeOutputs.DispatchWorkflow.Workflows { | ||
| tools = append(tools, toolWithMaxBudget(stringutil.NormalizeSafeOutputIdentifier(workflowName), safeOutputs.DispatchWorkflow.Max)) |
There was a problem hiding this comment.
The logic for advertising typed workflow safe-output tool names is clean and correct:
- When
DispatchWorkflow.Workflows,CallWorkflow.Workflows, orDispatchRepository.Toolsare non-empty, specific normalized tool names replace the genericdispatch_workflow/call_workflow/dispatch_repositoryin the prompt — exactly the right behaviour. sliceutil.SortedKeysis correctly used for the map-keyedDispatchRepository.Toolsto ensure deterministic output; slice-basedWorkflowspreserve their natural declaration order, which is intentional.- Tests cover both the substitution behaviour and the consistency invariant (generic name must not appear when typed names are present).
- The
drivespermission reordering in the JSON schema is a harmless alphabetical fix.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 32.6 AIC · ⌖ 9 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on two actionable issues.
📋 Key Themes & Highlights
Key Themes
- Missing fallback tests: The backward-compat empty-
Workflows/empty-Toolsbranches (which preserve the generic tool names) have no test coverage — a future regression would be silent. - Ordering inconsistency:
DispatchRepositorytools usesliceutil.SortedKeys(deterministic), butDispatchWorkflowandCallWorkflowiterate the slice in declaration order. Consider documenting or standardising. - Schema reorder side effect: The
drivespermission entry has been moved out of alphabetical position as a side effect of the schema fix.
Positive Highlights
- ✅ Core fix is clean and well-scoped: typed tool names are only substituted when typed targets are present, preserving backward compat.
- ✅ Consistency assertions in the test clearly specify the absence contract (
assert.NotContains), which is exactly the right guard. - ✅ Using
sliceutil.SortedKeysfor the map case is a good defensive choice.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.7 AIC · ⌖ 11.2 AIC · ⊞ 7.6K
Comment /matt to run again
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { |
There was a problem hiding this comment.
[/tdd] The empty-Workflows backward-compat fallback is not tested — if DispatchWorkflow is configured without any Workflows entries, the code falls back to the generic dispatch_workflow tool, but no test case exercises this path.
💡 Suggested test cases
{
name: "empty Workflows falls back to generic dispatch_workflow",
safeOutputs: &SafeOutputsConfig{
DispatchWorkflow: &DispatchWorkflowConfig{Workflows: []string{}},
NoOp: &NoOpConfig{},
},
expectedTools: []string{"dispatch_workflow", "noop"},
},
{
name: "empty Tools falls back to generic dispatch_repository",
safeOutputs: &SafeOutputsConfig{
DispatchRepository: &DispatchRepositoryConfig{Tools: map[string]*DispatchRepositoryToolConfig{}},
NoOp: &NoOpConfig{},
},
expectedTools: []string{"dispatch_repository", "noop"},
},
{
name: "empty Workflows falls back to generic call_workflow",
safeOutputs: &SafeOutputsConfig{
CallWorkflow: &CallWorkflowConfig{Workflows: []string{}},
NoOp: &NoOpConfig{},
},
expectedTools: []string{"call_workflow", "noop"},
},Without these, a future refactor could accidentally drop the fallback branches with no test failure.
@copilot please address this.
| if len(safeOutputs.DispatchWorkflow.Workflows) == 0 { | ||
| tools = append(tools, toolWithMaxBudget("dispatch_workflow", safeOutputs.DispatchWorkflow.Max)) | ||
| } else { | ||
| for _, workflowName := range safeOutputs.DispatchWorkflow.Workflows { |
There was a problem hiding this comment.
[/codebase-design] The DispatchWorkflow and CallWorkflow branches both iterate safeOutputs.X.Workflows with identical structure, but the DispatchRepository branch uses sliceutil.SortedKeys (deterministic) while the workflow loops use range order (non-deterministic map-equivalent for slices, but still not sorted). This inconsistency means the prompt tool list order for dispatch/call workflows can vary between Go versions or map iterations — and tools are order-sensitive in some downstream parsers.
Consider wrapping the slice loops in a sort or documenting why order is intentionally preserved (e.g., user-defined priority).
@copilot please address this.
| "type": "string", | ||
| "enum": ["write", "none"] | ||
| }, | ||
| "drives": { |
There was a problem hiding this comment.
[/codebase-design] The drives permission entry has been moved out of alphabetical order — it now sits between vulnerability-alerts and watches instead of between discussions and id-token. This breaks the consistent alphabetical ordering of the rest of the permissions definitions and will make future diffs harder to read.
💡 Suggestion
If this move is intentional (e.g., drives is a non-standard permission that should be grouped at the end), add a brief comment explaining why. If unintentional, move drives back between discussions and id-token to restore alphabetical order.
@copilot please address this.
|
@copilot run pr-finisher skill |
|
@pelikhan Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed the PR-finisher pass in |
|
@copilot this PR still needs forward progress.
Run: https://github.com/github/gh-aw/actions/runs/32605456597
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in Validation: targeted |
|
🎉 This pull request is included in a new release. Release: |
dispatch-workflow,dispatch-repository, andcall-workflowregistered typed per-target MCP tools but advertised generic tool names in the agent prompt. This could steer agents to an untypeddispatch_workflowcall and let an invalid output displace a corrected typed call.Typed prompt manifest
replaces:
Regression coverage