Refactor engine secret validation wrappers - #51216
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Centralizes engine secret-validation setup while preserving engine-specific authentication and skip behavior.
Changes:
- Added a shared validation configuration and builder.
- Migrated seven engine validation paths.
- Added focused helper tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/engine_helpers.go |
Adds shared validation configuration and builder. |
pkg/workflow/claude_engine.go |
Migrates Claude validation while preserving WIF handling. |
pkg/workflow/codex_engine.go |
Migrates Codex validation. |
pkg/workflow/copilot_engine_installation.go |
Preserves permission and BYOK skip policies. |
pkg/workflow/gemini_engine.go |
Preserves Vertex WIF handling. |
pkg/workflow/pi_engine.go |
Migrates backend-profile validation. |
pkg/workflow/behavior_defined_engine.go |
Migrates behavior-defined validation. |
pkg/workflow/universal_llm_consumer_engine.go |
Migrates universal backend validation. |
pkg/workflow/secret_validation_test.go |
Tests skip, empty-secret, and rendering behavior. |
pkg/workflow/awf_feature_flags_test.go |
Normalizes import formatting. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 10/10 changed files
- Comments generated: 0
- Review effort level: Balanced
|
|
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 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
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Refactor engine secret validation wrappers
Clean refactoring that consolidates the duplicated guard+delegate pattern across six engine implementations into a single BuildEngineSecretValidationStep function. All skip logic and empty-secret guards are now centrally handled in engine_helpers.go.
The Skip func(*WorkflowData) bool field provides a clean, testable extension point for per-engine skip policies. Tests cover all three paths (skip policy, empty secrets, rendering). No correctness or security issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.9 AIC · ⊞ 5.5K
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (112 new lines in 📄 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
|
Test Quality Sentinel Report 🧪Overall Assessment: ✅ 82/100 ExcellentPR: Refactor engine secret validation wrappers (#51216) Test SummaryNew Test:
|
| Subtest | Type | Value | Edge Case? |
|---|---|---|---|
| "applies skip policy before rendering" | behavioral_contract | high_value | ✅ skip=true |
| "skips empty secret list" | behavioral_contract | high_value | ✅ empty secrets |
| "renders configured validation step" | behavioral_contract | high_value | ✅ happy path |
Assertions: 5 total
require.Empty(t, step, "...")× 2 — guard clausesrequire.NotEmpty(t, step, "...")× 1 — positive caseassert.Contains(...)× 2 — rendered output validation
All assertions include descriptive failure messages ✅
Quality Metrics
| Metric | Result | Status |
|---|---|---|
| Design Tests | 3/3 (100%) | ✅ Excellent |
| Edge Case Coverage | 3/3 subtests | ✅ Skip policy, empty secrets, happy path |
| Assertions | 5 with messages | ✅ All descriptive |
| Mocking | None (real objects) | ✅ Clean test design |
| Test:Prod Ratio | 37:67 ≈ 0.55:1 | ✅ Well under 2:1 threshold |
| Duplicate Patterns | 0 clusters | ✅ No inflation |
| Build Tags | Present on both files | ✅ (go/redacted):build !integration |
Scoring Breakdown
Design tests (3/3): 40 points ✅
Edge cases (3/3): 30 points ✅
No duplicates: 20 points ✅
Test inflation (0.55:1): 10 points ✅
────────────────────────────────────
Total: 82/100
Threshold: >= 80 = Excellent ✅
Key Strengths
- Focused refactoring validation — Test precisely targets the new wrapper's ordering guarantee (skip policies applied before rendering)
- Clean architecture — No mocking; tests real
EngineSecretValidationConfigandWorkflowDataobjects - Guard clauses tested — Both skip policy and empty-secrets edge cases are explicitly validated
- Healthy test:prod ratio — 0.55:1 shows appropriate test focus without over-testing boilerplate
- 100% design tests — All assertions verify user-visible behavior, not implementation details
Minor Notes
- The test delegates multiple-secret scenarios to existing
TestGenerateMultiSecretValidationSteptests — appropriate separation of concerns - Import reordering in
awf_feature_flags_test.go(2 lines) has no test impact; aligns with Go conventions
Verdict
✅ APPROVE — No violations. Implementation tests represent 0% of total (well below 30% threshold). Test design is clean, focused, and validates the refactoring's behavioral contracts.
🧪 Test quality analysis by Test Quality Sentinel · haiku45 · 27.5 AIC · ⊞ 7.7K · ◷
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review
Applied /codebase-design — approving. The refactor cleanly centralizes the skip-then-build pattern into BuildEngineSecretValidationStep with a pluggable Skip callback. Deep-module improvement: simple interface, engine-specific policy preserved. Tests cover the three key branches (skip predicate, empty secrets, rendered step). Net -38 LOC with no behaviour change.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 31.2 AIC · ⊞ 7.1K
Comment /matt to run again
There was a problem hiding this comment.
✅ Test Quality Sentinel: 82/100 Excellent
- Design tests: 3/3 (100%)
- Edge cases: 3/3 (skip policy, empty secrets, happy path)
- Test:prod ratio: 0.55:1 (well under 2:1 threshold)
- Implementation tests: 0% (well below 30% threshold)
- No guideline violations
The new TestBuildEngineSecretValidationStep test precisely validates the refactoring's behavioral contracts with clean, real-object testing and proper assertion messages.
|
@copilot Quick triage nudge for this PR. Please refresh the branch if needed, address any remaining review feedback, update the PR body with the ADR link required by the design-decision gate, and run the Run: https://github.com/github/gh-aw/actions/runs/31225438989
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Quick triage nudge for this PR. Please refresh the branch if needed, update the PR body with the ADR link required by the design-decision gate, run the Outstanding item:
Run: https://github.com/github/gh-aw/actions/runs/31227649983
|
TriageCategory: refactor | Risk: medium | Score: 65/100
Recommended action: CI passing with 3 automated approvals, includes an ADR doc and centralizes engine secret validation across 12 files. Solid quality signal — ready for expedited human review.
|
|
🎉 This pull request is included in a new release. Release: |
Engine-specific secret validation methods repeated the same skip-then-build pattern across multiple workflow engines. This centralizes that flow while keeping engine-specific auth and skip policies intact.
Shared validation helper
EngineSecretValidationConfig.BuildEngineSecretValidationStepto apply skip policy, handle empty secret sets, and delegate rendering.Engine migrations
copilot-requests: write, custom command, environment, and provider-specific behavior.Focused coverage