feat: add production-side compile-time interface assertions for engine and condition types#45773
Conversation
…e and condition types Add `var _ Interface = (*Concrete)(nil)` guards to all 7 CodingAgentEngine implementors and all 10 ConditionNode implementors, so contract violations fail at the declaration site rather than a far-away call site. Engine assertions added to: - pkg/workflow/copilot_engine.go - pkg/workflow/claude_engine.go - pkg/workflow/codex_engine.go - pkg/workflow/gemini_engine.go - pkg/workflow/antigravity_engine.go - pkg/workflow/pi_engine.go - pkg/workflow/behavior_defined_engine.go ConditionNode assertions added to: - pkg/workflow/expression_nodes.go (10 types: ExpressionNode, AndNode, OrNode, NotNode, DisjunctionNode, FunctionCallNode, PropertyAccessNode, StringLiteralNode, BooleanLiteralNode, ComparisonNode) Closes #45762 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ 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 (28 additions detected). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
This PR adds production-side compile-time interface assertions to ensure all CodingAgentEngine implementations and all ConditionNode expression node types continue to satisfy their respective interfaces, causing contract breaks to fail at the type declaration site instead of at downstream call sites.
Changes:
- Added
var _ CodingAgentEngine = (*<Engine>)(nil)assertions for each of the 7 engine structs. - Added a grouped
var (...)block asserting all 10ConditionNodeimplementation types satisfyConditionNode.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/pi_engine.go | Adds compile-time assertion that *PiEngine implements CodingAgentEngine. |
| pkg/workflow/gemini_engine.go | Adds compile-time assertion that *GeminiEngine implements CodingAgentEngine. |
| pkg/workflow/expression_nodes.go | Adds grouped compile-time assertions that all ConditionNode node types implement ConditionNode. |
| pkg/workflow/copilot_engine.go | Adds compile-time assertion that *CopilotEngine implements CodingAgentEngine. |
| pkg/workflow/codex_engine.go | Adds compile-time assertion that *CodexEngine implements CodingAgentEngine. |
| pkg/workflow/claude_engine.go | Adds compile-time assertion that *ClaudeEngine implements CodingAgentEngine. |
| pkg/workflow/behavior_defined_engine.go | Adds compile-time assertion that *BehaviorDefinedEngine implements CodingAgentEngine. |
| pkg/workflow/antigravity_engine.go | Adds compile-time assertion that *AntigravityEngine implements CodingAgentEngine. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Low
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
LGTM. Straightforward compile-time interface assertions placed correctly at declaration sites. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 11.4 AIC · ⌖ 6.68 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — changes are solid.
📋 Key Themes & Highlights
Key Themes
- Correct idiom —
var _ Interface = (*Concrete)(nil)is the canonical Go compile-time assertion pattern; all 17 guards are correctly placed immediately after struct definitions. - Zero runtime cost — no behavioural change, purely a compile-time safety net.
- Grouped vs scattered —
ConditionNodeassertions are consolidated in a singlevar (\u2026)block inexpression_nodes.go;CodingAgentEngineassertions are one-per-file. This is a minor inconsistency (see inline comment).
Positive Highlights
- ✅ Placement immediately after each struct definition makes the contract visible at the declaration site.
- ✅ Grouped block for
ConditionNodetypes is easy to scan at a glance. - ✅ PR description is clear and self-contained.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 18.1 AIC · ⌖ 4.41 AIC · ⊞ 6.7K
Comment /matt to run again
| BaseEngine | ||
| } | ||
|
|
||
| var _ CodingAgentEngine = (*CopilotEngine)(nil) |
There was a problem hiding this comment.
[/codebase-design] Minor: the CodingAgentEngine assertions are scattered one-per-file while the ConditionNode assertions are consolidated in a single var (...) block. Consider grouping all engine assertions in a dedicated file (e.g. engine_assertions.go) or at minimum alongside the CodingAgentEngine interface declaration, to make the full set of implementors visible at a glance — mirroring the style already used in expression_nodes.go.
@copilot please address this.
There was a problem hiding this comment.
Compile-time interface guards are correctly placed and complete.
Coverage check
CodingAgentEngine (7 types)
All 6 builtins registered in NewEngineRegistry plus BehaviorDefinedEngine (registered dynamically) are guarded. UniversalLLMConsumerEngine is a mixin only, never directly registered — no guard needed there.
ConditionNode (10 types)
Grouped block in expression_nodes.go covers all concrete node types visible in the package.
Placement
Guards are placed immediately after each struct definition, which is the idiomatic Go convention for these assertions. Zero runtime overhead, correct semantics.
🔎 Code quality review by PR Code Quality Reviewer · 50.7 AIC · ⌖ 4.32 AIC · ⊞ 5.6K
Comment /review to run again
|
🎉 This pull request is included in a new release. Release: |
The repo had zero production-side
var _ Interface = (*Concrete)(nil)guards — all 7CodingAgentEngineimplementors and all 10ConditionNodenode types were entirely unguarded, meaning a broken contract surfaces at a distant call site rather than at the declaration.Changes
7
CodingAgentEngineassertions — one guard added directly after each struct definition:CopilotEngine,ClaudeEngine,CodexEngine,GeminiEngine,AntigravityEngine,PiEngine,BehaviorDefinedEngine10
ConditionNodeassertions — grouped block added inexpression_nodes.goimmediately after the interface declaration:ExpressionNode,AndNode,OrNode,NotNode,DisjunctionNode,FunctionCallNode,PropertyAccessNode,StringLiteralNode,BooleanLiteralNode,ComparisonNodeZero runtime overhead. Engine refactors now fail at the declaration site.