refactor: EngineRegistry.Register returns error instead of panicking on invalid port

[Copilot]

EngineRegistry.Register panicked on dedicatedLLMGatewayPort < 0, making misconfiguration an unrecoverable crash with no testable path.

Changes

• Register signature: Register(engine CodingAgentEngine) → Register(engine CodingAgentEngine) error; replaces panic with an error return
• NewEngineRegistry: loops over built-in engines and handles registration errors via panic(fmt.Sprintf(...)), preserving fail-fast semantics with stack traces and deferred cleanup intact (avoids os.Exit in library code)
• Call sites: updated engine_config_render_test.go and engine_definition_test.go to handle the error return from Register using require.NoError
• Tests: updated existing Register call sites to assert NoError; added new test case confirming an engine with port -1 returns an error rather than panicking, and that the engine is not registered on failure

// Before
func (r *EngineRegistry) Register(engine CodingAgentEngine) {
    if p, ok := engine.(portProvider); ok && p.getDedicatedLLMGatewayPort() < 0 {
        panic(fmt.Sprintf("engine '%s': dedicatedLLMGatewayPort must be >= 0, got %d", ...))
    }
}

// After
func (r *EngineRegistry) Register(engine CodingAgentEngine) error {
    if p, ok := engine.(portProvider); ok && p.getDedicatedLLMGatewayPort() < 0 {
        return fmt.Errorf("engine '%s': dedicatedLLMGatewayPort must be >= 0, got %d", ...)
    }
    // ...
    return nil
}

[Copilot]

Pull request overview

Refactors engine registration to return a recoverable error instead of panicking when an engine has an invalid dedicatedLLMGatewayPort, enabling testable validation paths while keeping built-in engine registration fail-fast.

Changes:

• Changed EngineRegistry.Register to return error and replaced the negative-port panic with a validation error.
• Updated NewEngineRegistry to register built-in engines in a loop and terminate on registration failure.
• Updated/extended unit tests to assert Register succeeds for valid engines and returns an error for a negative port engine.

Show a summary per file

File	Description
pkg/workflow/agentic_engine.go	Updates Register to return error; updates built-in engine registration to handle errors (currently via stderr + os.Exit(1)).
pkg/workflow/agentic_engine_test.go	Updates Register call sites to assert NoError; adds a new negative-port regression test helper type and test case.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 2/2 changed files
• Comments generated: 3

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 100/100

✅ Excellent test quality

Metric	Value
New/modified tests analyzed	3
✅ Design tests (behavioral contracts)	3 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	3 (100%)
Duplicate test clusters	0
Test inflation detected	No (ratio: 1.2×)
🚨 Coding-guideline violations	None

---

Test Classification Details

View all test details

Test	File	Classification	Issues Detected
TestEngineRegistry_Register / "registers a custom engine" (modified)	pkg/workflow/agentic_engine_test.go:88	✅ Design	None — now captures and asserts on error return
TestEngineRegistry_Register / "makes engine valid after registration" (modified)	pkg/workflow/agentic_engine_test.go:101	✅ Design	None — now captures and asserts on error return
TestEngineRegistry_Register / "registering an engine with negative dedicatedLLMGatewayPort returns error" (new)	pkg/workflow/agentic_engine_test.go:112	✅ Design	None

---

Analysis Notes

All three changes are high-quality:

1. Modified happy-path subtests: Both existing subtests now capture the error return from Register() and assert require.NoError(...) with a descriptive message — converting implicit success into an explicit behavioral contract.

2. New error-path subtest: "registering an engine with negative dedicatedLLMGatewayPort returns error" directly exercises the new validation logic. It uses a real helper struct (negativePortEngine) rather than a mock, consistent with the project's no-mock policy. Both require.Error and assert.Contains on the error message are present with descriptive messages.

Minor cosmetic note: There is a typo in one assertion message — "error message should describe t he constraint" (stray space in t he). This does not affect test behavior but is worth fixing for clarity.

Build tag: ✅ //go:build !integration on line 1
Mock libraries: ✅ None used
Assertion messages: ✅ All assertions include descriptive messages

---

Language Support

Tests analyzed:

• 🐹 Go (*_test.go): 3 tests — unit (//go:build !integration)
• 🟨 JavaScript (*.test.cjs, *.test.js): 0 tests

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). Tests enforce clear behavioral contracts and cover both the happy path and the new error path introduced by this refactor.

---

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · ● 465.2K · ◷

[github-actions]

✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%).

[pelikhan]

@copilot review all comments

[Copilot]

@copilot review all comments

Addressed all three reviewer comments in f44a14c:

1. os.Exit(1) in library code → replaced with panic(fmt.Sprintf(...)), preserving fail-fast semantics while keeping stack traces and deferred cleanup intact.
2. Unchecked Register returns → both engine_config_render_test.go:142 and engine_definition_test.go:117 now use require.NoError(t, registry.Register(...)).
3. Missing side-effect assertion → the negative-port test now also asserts assert.False(t, registry.IsValidEngine("claude")) to confirm the engine was not registered on error.