feat(pi): add bare mode support - #49137
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds Pi bare-mode support as an accepted no-op, preventing spurious warnings.
Changes:
- Enables Pi’s bare-mode capability.
- Updates validation messaging.
- Adds capability test coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/pi_engine.go |
Enables Pi bare mode. |
pkg/workflow/engine_config_test.go |
Tests Pi capability. |
pkg/workflow/agent_validation.go |
Includes Pi in supported-engine warning text. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Medium
| MaxContinuations: false, | ||
| WebSearch: false, | ||
| NativeAgentFile: false, | ||
| BareMode: true, // Pi is bare by default; bare mode is a no-op |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49137 does not have the 'implementation' label and has only 7 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The changes look correct and well-scoped:
BareMode: trueis properly added toPiEngine.capabilities- Warning message updated to include
pi - Test case added to
TestSupportsBareMode
The existing review comment about stale docs in engines.md/glossary.md should be addressed but is non-blocking.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.1 AIC · ⊞ 5.3K
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Nice work! 🚀 This PR looks solid and ready for review: ✅ Focused change — adds bare mode support for the pi engine in a clean, minimal diff (8 lines) The changes are straightforward:
This looks ready for review! 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.
|
There was a problem hiding this comment.
Test Quality Sentinel: APPROVED
✅ Score: 85/100 — Excellent
Summary: Single test case added via table-driven pattern. 100% design-driven tests, 0% implementation tests (threshold: 30%). No violations.
Test Details:
- Test:
TestSupportsBareModerow "pi supports bare mode" added atengine_config_test.go:1287–1290 - Classification:
design_test,high_value,behavioral_contract - Coverage: Verifies Pi engine correctly declares bare mode support (
BareMode: true) in capabilities
Quality Signals:
- Proper build tag (
//go:build !integration) - Integrated into existing table-driven test pattern
- Meaningful assertions with descriptive context message
- Supports design invariant enforced by compiler validation logic
Verdict: ✅ Passed. Test properly validates user-visible behavior change.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /codebase-design — one maintainability observation, otherwise LGTM.
📋 Key Themes & Highlights
Key Themes
- Hardcoded engine list in warning message: The list of bare-mode-capable engine names in
agent_validation.gomust be kept in sync manually; this PR already required touching it, and the next bare-capable engine will too.
Positive Highlights
- ✅ Minimal, surgical change — exactly the right scope for a bug fix
- ✅ Test case added in the same PR, covering the new capability flag
- ✅ Clear inline comment explaining the no-op semantics (
Pi is bare by default) - ✅ Good consistency: matches the pattern already used by
copilot_engine.goandclaude_engine.go
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 28.6 AIC · ⊞ 7K
Comment /matt to run again
| if !engine.GetCapabilities().BareMode { | ||
| agentValidationLog.Printf("Engine %s does not support bare mode, emitting warning", engine.GetID()) | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Engine '%s' does not support bare mode (engine.bare: true). Bare mode is only supported for the 'copilot' and 'claude' engines. The setting will be ignored.", engine.GetID()))) | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Engine '%s' does not support bare mode (engine.bare: true). Bare mode is only supported for the 'copilot', 'claude', and 'pi' engines. The setting will be ignored.", engine.GetID()))) |
There was a problem hiding this comment.
[/codebase-design] The engine name list in this warning is hardcoded and will drift each time a new engine gains BareMode: true — it already required a manual update in this PR.
💡 Suggested fix: derive the list dynamically
Consider building the supported-engines string from the registered engines at runtime, or centralising it as a constant so there is a single place to update:
supported := []string{}
for _, e := range allEngines {
if e.GetCapabilities().BareMode {
supported = append(supported, "'"+e.GetID()+"'")
}
}
msg := fmt.Sprintf("... Bare mode is only supported for the %s engines.", strings.Join(supported, ", "))This keeps the warning accurate automatically as new engines are added.
@copilot please address this.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: no blocking issues, one stale-docs gap
The code change is a correct, minimal capability flip: PiEngine.capabilities.BareMode = true is a genuine no-op for Pi (the codebase never reads EngineConfig.Bare inside the Pi engine -- bare-by-default behavior needs no plumbing), and the warning-message + test updates are consistent with claude/copilot precedent. TestSupportsBareMode passes locally.
Details
- Verified via grep that no
pi_engine*.gofile references.Bare, confirming the no-op claim in the added comment is accurate rather than aspirational. validateBareModeSupportwarning string update is consistent with existing message formatting; no test asserts the literal old string, so nothing breaks.- One gap:
docs/src/content/docs/reference/engines.md(feature-comparison table, line 43) anddocs/src/content/docs/reference/glossary.md(Bare Mode glossary entry, ~line 512) still describe bare mode as unsupported by Pi / list only Copilot, Claude, Codex, and Gemini. This was already flagged in a prior review comment on this PR and remains unresolved -- left a targeted comment on the table row.
🔎 Code quality review by PR Code Quality Reviewer · auto · 79.4 AIC · ⊞ 7.8K
Comment /review to run again
|
🎉 This pull request is included in a new release. Release: |
engine.bare: trueemitted a spurious warning for thepiengine even though pi is bare by default (the flag is a no-op).Changes
pi_engine.go: SetBareMode: trueinPiEngine.capabilities— bare mode is a supported no-op for piagent_validation.go: Include'pi'in the warning message listing engines that support bare modeengine_config_test.go: Addpi supports bare modecase toTestSupportsBareMode