[WIP] Fix goose agentic engine configuration#50186
Conversation
…ines Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Thanks for kicking off work on the goose agentic engine configuration fix! 🛠️ This PR is currently a draft with no code changes yet (0 files changed) — just the initial placeholder commit — so there isn't anything to evaluate against CONTRIBUTING.md quite yet. A couple of notes for when the implementation lands:
Note: per CONTRIBUTING.md, this repository expects most external contributions to start as an agentic plan in an issue, with a core team member creating the implementing PR. Since this PR appears to be an automated Copilot coding agent PR responding to a task, that's likely already aligned with the project's process — just flagging for completeness. If useful, here's a prompt to hand to your coding agent to continue the work: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
Triage: bug / low risk
|
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (only 12 additions detected). |
There was a problem hiding this comment.
Pull request overview
Adds runtime setup for behavior-defined engines using Node.js harness scripts.
Changes:
- Adds Node.js setup when no engine installation block exists.
- Attempts to install AWF when firewall configuration enables it.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/behavior_defined_engine.go |
Adds harness-specific installation steps. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
pkg/workflow/behavior_defined_engine.go:162
- Harness execution treats AWF as enabled even when
isFirewallEnabled(workflowData)is false:behaviorDefinedFirewallEnabledforces it on for every harness unlesssandbox.agentdisables it. This helper checks onlyisFirewallEnabled, so a harness workflow with no network/firewall configuration gets a Node setup step but still no AWF installation, while its execution step invokesawf --configand fails with the sameawf: command not founderror. Pass the behavior-defined effective firewall decision into installation generation (or otherwise install AWF whenever harness execution will use it).
return BuildNpmEngineInstallStepsWithAWF([]GitHubActionStep{GenerateNodeJsSetupStep()}, workflowData)
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| if behavior.Installation == nil { | ||
| if behavior.HarnessScript == "" { | ||
| return nil | ||
| } | ||
| return BuildNpmEngineInstallStepsWithAWF([]GitHubActionStep{GenerateNodeJsSetupStep()}, workflowData) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — requesting changes on missing test coverage.
📋 Key Themes & Highlights
Key Themes
- Missing regression test: The new harness-script branch in
GetInstallationStepsis the exact code path that was broken before this fix. Without a test, the same failure mode can silently resurface after any future refactor.
Positive Highlights
- ✅ Root cause correctly identified:
GetInstallationStepswas returningnilfor harness-script engines that declare noinstallation:block. - ✅ The fix is surgical — one small guard added, existing npm-based installation path unchanged.
- ✅ Good explanatory comment documenting why Node.js must always be present for harness-script engines.
- ✅ The early-exit for
EngineConfig.Command != ""is correctly preserved before the new branch.
One Actionable Request
Add two tests in behavior_defined_engine_harness_test.go (see inline comment on line 162):
- Harness engine with no
installation:block →GetInstallationStepsreturns non-empty steps. - Non-harness engine with no
installation:block →GetInstallationStepsreturnsnil.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 25.3 AIC · ⌖ 8.37 AIC · ⊞ 7.1K
Comment /matt to run again
| if behavior.HarnessScript == "" { | ||
| return nil | ||
| } | ||
| return BuildNpmEngineInstallStepsWithAWF([]GitHubActionStep{GenerateNodeJsSetupStep()}, workflowData) |
There was a problem hiding this comment.
[/tdd] The new harness-script installation path lacks a regression test — without one, a future refactor could silently break Goose (and similar harness-only engines) again.
💡 Suggested test skeleton
Add two tests in behavior_defined_engine_harness_test.go:
- Harness engine, no
installation:block →GetInstallationStepsmust return non-empty steps (Node.js setup). - Non-harness engine, no
installation:block →GetInstallationStepsmust returnnil.
The /diagnosing-bugs skill stresses that every root-cause fix needs a regression test to prevent the same symptom recurring silently.
@copilot please address this.
There was a problem hiding this comment.
The fix looks correct and minimal. For harness-script engines (like Goose) with no installation: block, GetInstallationSteps now returns the Node.js setup step (plus AWF when the firewall is enabled), matching the intent described in the PR. The logic is cleanly structured and consistent with how other engines use BuildNpmEngineInstallStepsWithAWF. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.4 AIC · ⌖ 10.1 AIC · ⊞ 5.4K
|
@copilot run pr-finisher skills |
There was a problem hiding this comment.
Verdict: Request Changes
Correct fix in principle, but unverified: missing test coverage for the new branch and the smoke-goose.lock.yml artifact wasn't recompiled/committed as the PR's own checklist admits.
Themes
- The logic change in
GetInstallationSteps(installing Node.js + AWF for harness-script engines lacking aninstallation:block) is a sound fix for the described bug. - However, this is marked WIP and two checklist items are still unchecked: recompiling workflows and running tests. Since compiled
.lock.ymlfiles are the actual deployed artifact, the fix isn't effectively live untilsmoke-goose.lock.ymlis regenerated and committed. - No unit test directly targets the new code path (
Installation == nil && HarnessScript != ""). Existing harness tests coverGetExecutionSteps, notGetInstallationSteps.
🔎 Code quality review by PR Code Quality Reviewer · auto · 53.6 AIC · ⌖ 5.47 AIC · ⊞ 7.9K
Comment /review to run again
| // harness through Node.js, so Node.js (and, when the firewall is enabled, the AWF | ||
| // binary) must always be installed even when no package-manager based installation | ||
| // is declared for the engine's CLI itself. | ||
| if behavior.Installation == nil { |
There was a problem hiding this comment.
Missing test coverage for the new Installation == nil && HarnessScript != "" branch, and the generated smoke-goose.lock.yml wasn't recompiled/committed in this PR (per the PR's own checklist) — the fix is unverified end-to-end.
💡 Why this matters
This branch changes behavior for every harness-script engine without an explicit installation: block (Goose today, potentially others later). Without a unit test asserting GetInstallationSteps returns Node.js (+ AWF when firewall enabled) for such a config, a future refactor could silently regress this exact bug. Also, since make recompile wasn't run, the actual generated .lock.yml for smoke-goose still lacks the fix — so the described root cause is not actually resolved in this PR's committed artifacts, only in source.
Suggested fix: add a test in behavior_defined_engine_harness_test.go similar to newHarnessEngineDefinition() but with Installation: nil, asserting GetInstallationSteps returns Node.js setup (+AWF when firewall enabled), and run/commit make recompile output for smoke-goose.lock.yml.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
/smoke-goose |
|
|
|
@copilot run pr-finisher skill |
|
@copilot fix the configuration on where the mcp config file should go See failure in agent log https://github.com/github/gh-aw/actions/runs/30908234685/job/91988815439#step:26:1 |
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. @copilot This PR needs a quick maintainer-ready refresh.
Run: https://github.com/github/gh-aw/actions/runs/30909334614
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection results could not be parsed. Review the workflow run logs for details. Triage update: bug / medium risk
|
awf: command not found)harness-scriptexecution (runs via Node.js + AWF firewall) butBehaviorDefinedEngine.GetInstallationStepsreturned nil since Goose has noinstallation:block, so Node.js and the AWF binary were never installedGetInstallationStepsto install Node.js (and AWF binary when firewall enabled) for harness-script engines lacking an explicit installation blockCaution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/30909334614