Split oversized engine_validation.go (521 lines) into domain-focused files - #48568
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Excellent refactoring! 🎯 This PR cleanly decomposes What looks great:
This aligns perfectly with the project's refactoring thresholds and code quality standards. Ready for review! 🚀
|
There was a problem hiding this comment.
Pull request overview
Splits engine validation into focused modules while preserving validation behavior.
Changes:
- Separates driver and inline-definition validation.
- Adds direct script-filename tests.
- Updates validation architecture documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/engine_validation.go |
Retains top-level engine validation. |
pkg/workflow/engine_driver_validation.go |
Houses driver and harness validation. |
pkg/workflow/engine_inline_definition_validation.go |
Houses inline definition and auth validation. |
pkg/workflow/engine_driver_validation_test.go |
Adds script-filename tests. |
scratchpad/validation-architecture.md |
Documents the split architecture. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Medium
| // TestValidateEngineScriptFilename tests the validateEngineScriptFilename helper directly. | ||
| // The function validates that a script name is a safe Node.js basename (no paths, safe chars, | ||
| // correct extension). It is also exercised indirectly by TestValidateEngineHarnessScript. | ||
| func TestValidateEngineScriptFilename_ValidInputs(t *testing.T) { |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Split oversized engine_validation.go
Clean structural refactoring with no behavioral changes. Domain boundaries are clear and well-documented.
Two existing review comments already cover the actionable issues:
- Stale line counts in scratchpad/validation-architecture.md (~155 documented vs 171 actual)
- Test coverage gap — validateEngineHarnessScript and validateEngineDriver retain only indirect coverage; direct tests should be added to engine_driver_validation_test.go
No additional blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.8 AIC · ⌖ 4.54 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting on test coverage gaps and a minor logging ordering issue. The structural refactor itself is clean and well-motivated.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap:
validateEngineDriver(the most security-relevant function — it enforces path-traversal and metacharacter rejection) has no direct unit tests in the newengine_driver_validation_test.go. The added tests cover onlyvalidateEngineScriptFilename. The extension-dispatch branching for copilot vs. other engines is untested directly. - Inline-driver functions also untested directly:
validateInlineEngineDriver,validateEngineHarnessScriptmoved here but their tests remain inengine_validation_test.go, splitting related tests across files. - Minor log ordering bug in
validateEngineInlineDefinition: the log fires before the empty-string guard.
Positive Highlights
- ✅ Excellent file-level doc comments with clear "When to Add Validation Here" guidance — this is exactly the kind of deep-module documentation that makes a codebase navigable.
- ✅
engineInlineDefinitionLoguses a domain-specific prefix (workflow:engine_inline_definition_validation) — consistent with the existing logger pattern. - ✅ No behavioral changes; the refactor is purely mechanical — verified by identical function bodies in the diff.
- ✅
scratchpad/validation-architecture.mdupdated to reflect the new three-file structure.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.4 AIC · ⌖ 4.91 AIC · ⊞ 6.7K
Comment /matt to run again
| // TestValidateEngineScriptFilename tests the validateEngineScriptFilename helper directly. | ||
| // The function validates that a script name is a safe Node.js basename (no paths, safe chars, | ||
| // correct extension). It is also exercised indirectly by TestValidateEngineHarnessScript. | ||
| func TestValidateEngineScriptFilename_ValidInputs(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] validateEngineDriver — the most complex function in this new file — has no direct unit tests covering its copilot-vs-other-engine extension branching (.py, .ts, .mts, .rb pass for copilot, fail for others) or segment-safety checks.
💡 Suggested test skeleton
func TestValidateEngineDriver_ExtensionsByEngine(t *testing.T) {
tests := []struct{
name string
engineID string
driver string
wantErr bool
}{
{"py allowed for copilot", "copilot", "driver.py", false},
{"py rejected for pi", "pi", "driver.py", true},
{"ts allowed for copilot", "copilot", "driver.ts", false},
{"bare name ok", "pi", "mydriver", false},
{"traversal rejected", "copilot", "../bad.js", true},
{"empty segment rejected", "copilot", "a//b.js", true},
}
// build a minimal *Compiler and call c.validateEngineDriver for each case
}@copilot please address this.
| // TestValidateEngineScriptFilename tests the validateEngineScriptFilename helper directly. | ||
| // The function validates that a script name is a safe Node.js basename (no paths, safe chars, | ||
| // correct extension). It is also exercised indirectly by TestValidateEngineHarnessScript. | ||
| func TestValidateEngineScriptFilename_ValidInputs(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] validateInlineEngineDriver and validateEngineHarnessScript also moved to this file but remain untested directly — only the harness script test was previously in engine_validation_test.go indirectly. The split is a good opportunity to add direct tests for inline-driver edge cases (empty source, unsupported runtime, wrong engine ID).
@copilot please address this.
| func (c *Compiler) validateEngineInlineDefinition(config *EngineConfig) error { | ||
| if !config.IsInlineDefinition { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
[/codebase-design] validateEngineInlineDefinition logs runtimeID=%s using config.ID, but config.ID is checked for emptiness two lines later — the log line will print an empty string if config.ID == "". Swap the guard and the log, or defer the log until after the empty check.
@copilot please address this.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (463 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
APPROVE — clean, verified refactor
Build passes (go build ./...), the extraction is behavior-preserving (function bodies moved verbatim, imports correctly split, no leftover/duplicate declarations), and no new blocking issues were found beyond what existing bot review comments already flag.
💡 Review notes
- Verified
validateEngineScriptFilename,validateEngineHarnessScript,validateEngineDriver,validateInlineEngineDrivermoved byte-for-byte intoengine_driver_validation.gowith correct import adjustments (addedpath/filepath,regexp; removed fromengine_validation.go). - Verified
validateEngineInlineDefinition,registerInlineEngineDefinition,validateEngineAuthDefinitionmoved intoengine_inline_definition_validation.go; the newengineInlineDefinitionLoglogger (workflow:engine_inline_definition_validation) replacesengineValidationLogconsistently in all six call sites, and no test asserts the old prefix string, so this rename is safe. go build ./...succeeds with no compile errors from the split (no duplicate declarations, no import cycles).- Two pre-existing bot review comments already cover the two real (non-blocking) issues found independently by my own pass and by a grumpy sub-agent review: (1) stale
~155 linesdoc count forengine_driver_validation.go(actual: 171 lines) inscratchpad/validation-architecture.md, and (2)TestValidateEngineHarnessScript/TestValidateEngineCopilotSDKDriver_Copilotstill live inengine_validation_test.goinstead of being colocated with the newengine_driver_validation_test.go. Both are minor maintainability nits, not correctness bugs — no new comments added to avoid duplicating existing feedback.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 67 AIC · ⌖ 4.35 AIC · ⊞ 7.4K
Comment /review to run again
|
@copilot run pr-finisher skill |
…n_test.go; fix doc line count Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 2a556ff. Both review threads addressed:
|
engine_validation.goat 521 lines exceeded the 300-line refactor threshold and mixed three unrelated validation domains in a single file, making it difficult to maintain and test independently.Changes
engine_driver_validation.go(171 lines) — extractedvalidateEngineScriptFilename,validateEngineHarnessScript,validateEngineDriver,validateInlineEngineDriver, and the associatedsafeHarnessScriptPattern/safeSDKDriverSegmentPatternregexesengine_inline_definition_validation.go(177 lines) — extractedvalidateEngineInlineDefinition,registerInlineEngineDefinition,validateEngineAuthDefinition; uses a newengineInlineDefinitionLoglogger with a domain-specific prefixengine_validation.go(255 lines, trimmed from 521) — retains engine version warning, MCP timeout validation, multi-file engine specification consistency, andEngineHasValidateSecretStepengine_driver_validation_test.go— adds direct unit tests forvalidateEngineScriptFilename(valid basenames, whitespace, path traversal, metacharacters, unsupported extensions); previously only had indirect coverage through harness script testsscratchpad/validation-architecture.md— updated to document the three-file structure with per-file function inventories and guidance on where to add new validationNo behavioral changes — pure structural refactoring with all function signatures preserved.
pkg/workfloworpkg/cli#48156