Repository navigation
Tell engines without MCP that the safeoutputs CLI is the only transport - #63493
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Per-compiler behavior-defined engines such as Aider and DeepSeek Harness remain incorrectly classified as MCP-capable.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds engine-aware safe-output guidance so MCP-disabled engines use the safeoutputs CLI exclusively.
Changes:
- Adds capability-based transport wording.
- Wires prompt placeholders into compilation.
- Adds tests and recompiles generated workflows.
| File | Description |
|---|---|
pkg/workflow/safe_outputs_transport_prompt.go |
Resolves transport guidance from engine capabilities. |
pkg/workflow/unified_prompt_step.go |
Injects safe-output transport wording. |
pkg/workflow/mcp_cli_mount.go |
Injects CLI-specific transport wording. |
pkg/workflow/prompt_validation_test.go |
Tests composed prompts for MCP and non-MCP engines. |
pkg/workflow/pi_safe_outputs_transport_integration_test.go |
Tests compiled Pi and Copilot workflows. |
actions/setup/md/safe_outputs_prompt.md |
Adds transport placeholder. |
actions/setup/md/mcp_cli_tools_with_safeoutputs_prompt.md |
Adds CLI transport placeholder. |
.github/workflows/*.lock.yml (298 files) |
Carries regenerated prompt environment substitutions. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| engine, err := GetGlobalEngineRegistry().GetEngine(strings.ToLower(data.EngineConfig.ID)) | ||
| if err != nil || engine == nil { | ||
| return true |
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Cannot run ponytail review: /tmp/gh-aw/agent/pr-diff.patch is empty, so there are no changed lines to review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch PR file list
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
🏗️ ADR required — draft added for PR #63493I enforced the ADR gate for this PR because the prefetch summary shows 210 additions in default business-logic directories, which is above the 100-line threshold. Evidence used
ResultI added a draft ADR at:
Inferred decision
Next actionPlease review and refine the draft ADR in this branch, especially the decision scope and trade-offs, before merging.
|
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.
Request changes
The fix is still incomplete because the new capability lookup uses the global engine registry. Imported behavior-defined engines with mcp false are registered on the compiler instance, so they still get the old optional transport wording instead of the required CLI-only wording.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 51.2 AIC · ⌖ 9.97 AIC · ⊞ 20.3K
Comment /review to run again
| func TestComposedPromptForNonMCPEngineStatesCLIIsOnlyTransport(t *testing.T) { | ||
| compiler := &Compiler{} | ||
| data := &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ID: "pi"}, |
There was a problem hiding this comment.
This test only covers built-in pi, so imported behavior-defined engines with mcp false can still ship the wrong optional transport prompt; please add a regression for a compiler-registered custom non-MCP engine as well.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design on the new transport-wording module (safe_outputs_transport_prompt.go). The mechanism (registry-driven capability lookup, compile-time placeholder substitution) is sound and well-tested for the direct compilation path, but there's a real gap for engines registered on a per-compiler basis.
📋 Key Themes & Highlights
Key Themes
- Registry scope mismatch:
engineSupportsMCPToolCallsresolves engines viaGetGlobalEngineRegistry(), but behavior-defined engines imported into a specific compiler instance (e.g. shared workflows declaringmcp: false) are registered onc.engineRegistry, not the global singleton. For those engines the lookup misses, falls back totrue, and the exact "optional CLI" wording bug this PR fixes reappears for imported non-MCP engines. This was also flagged by a prior automated reviewer comment on the same line.
Positive Highlights
- ✅ Clean separation: wording constants, capability lookup, and per-file rendering are each isolated and easy to follow.
- ✅ Solid test coverage for the built-in
piengine path — both composed-prompt unit tests and compiled-lock-file integration tests assert the exact wording transitions in both directions (MCP-capable vs. not). - ✅ Good doc comment explaining why the distinction matters (agents hunting for a nonexistent named command).
Minor note
- No
.changeset/*.mdentry found for this user-facing prompt-wording change; the project's other prompt/behavior fixes in.changeset/suggest one may be expected here for the release notes.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 83.6 AIC · ⌖ 14.6 AIC · ⊞ 10.1K
Comment /matt to run again
| if data == nil || data.EngineConfig == nil || data.EngineConfig.ID == "" { | ||
| return true | ||
| } | ||
| engine, err := GetGlobalEngineRegistry().GetEngine(strings.ToLower(data.EngineConfig.ID)) |
There was a problem hiding this comment.
[/codebase-design] engineSupportsMCPToolCalls looks up GetGlobalEngineRegistry() instead of the compiler-instance c.engineRegistry. Behavior-defined engines imported/registered per-compiler (e.g. shared workflows with mcp: false) are absent from the global singleton, so this falls back to true (MCP-capable) and silently re-introduces the exact bug this PR fixes for those engines.
💡 Suggested fix
Thread the compiler's registry through instead of relying on the global singleton, matching how other call sites (e.g. compiler_activation_job.go, compiler_jobs.go) use c.engineRegistry.GetEngine(...). This likely means passing the registry (or the resolved engine) into safeOutputsTransportText/safeOutputsCLITransportText rather than resolving it fresh from the global registry inside a WorkflowData-only helper.
func engineSupportsMCPToolCalls(registry *EngineRegistry, data *WorkflowData) bool {
if data == nil || data.EngineConfig == nil || data.EngineConfig.ID == "" || registry == nil {
return true
}
engine, err := registry.GetEngine(strings.ToLower(data.EngineConfig.ID))
if err != nil || engine == nil {
return true
}
return engine.GetCapabilities().MCP
}@copilot please address this.
|
@copilot run pr-finisher skill |
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. The compiler now resolves safe-output MCP capability through its active engine catalog, including runtime aliases, with global fallback for unresolved engines. Added regression coverage and regenerated the affected Aider and DeepSeek Harness lock files. |
|
@copilot minimize overhead in compiled code |
|
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in 5668619. Safe-output transport wording now uses static prompt fragments, removing the repeated transport environment variables and their substitution entries from compiled workflows while preserving CLI-only selection for MCP-disabled engines. |
The overhead reduction is in 5668619: static prompt fragments replace per-workflow transport substitutions. Refreshed local checks ( |
|
🎉 This pull request is included in a new release. Release: |
The safe-outputs prompt fragments unconditionally describe the
safeoutputsCLI as an optional equivalent to calling safe-output tools directly. That framing is wrong for engines withEngineCapabilities.MCP: false(today,engine: pi), which have no tool-call interface at all — agents there look for a command literally named after the tool (type create_pull_request,compgen -c | grep …), find nothing, conclude the tool is unavailable, and finish without emitting any safe output.Changes
safe_outputs_prompt.mdandmcp_cli_tools_with_safeoutputs_prompt.mdreplace their hard-coded transport sentences with__GH_AW_SAFE_OUTPUTS_TRANSPORT__/__GH_AW_SAFE_OUTPUTS_CLI_TRANSPORT__, resolved by the existing compile-time placeholder-substitution step.pkg/workflow/safe_outputs_transport_prompt.go(new) —engineSupportsMCPToolCalls()resolves the engine from the registry and readsGetCapabilities().MCP(unknown/unset engines stay MCP-capable), plus the two wording variants per fragment. The MCP-false wording states the CLI is the only transport and pre-empts the failed discovery path.collectPromptSections(safe-outputs section) andbuildMCPCLIPromptSection(safeoutputs variant only) attach the resolved text as sectionEnvVars.pivs. an MCP engine, and compiled-lock-file integration assertions that apiworkflow contains no "optional equivalent transport" framing and does carry the "ONLY transport" statement.engine: piworkflows pick up the new wording, all others are unchanged in substance.Rendered for
engine: pi:Not covered here
pi_engine.gohands the prompt to pi — it is piped to stdin as the first user turn today — and is left for follow-up.contextWindowforaw-gatewaymodels) is tracked separately in pi_models_json.cjs registers aw-gateway models with no contextWindow, so pi falls back to a 128k default well below the real model's capacity #63240.