Reduce EnforceSafeUpdate parameter count with options struct#52614
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail review: PR #52614 groups 4 related bool params into a PullRequestEventTransition struct to satisfy the 8-parameter lint limit. Single call site, no speculative abstraction, no added dependencies. Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review for #52614; no actionable changed-line issues found, so no review comments or review submission were emitted.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52614 does not have the 'implementation' label and has only 38 new lines of code in business logic directories (threshold is 100).
|
There was a problem hiding this comment.
Pull request overview
Refactors safe-update enforcement to satisfy parameter-count linting without changing behavior.
Changes:
- Adds
PullRequestEventTransitionto group event-state flags. - Updates production and test call sites to use the struct.
- Preserves pull-request-target escalation detection.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_update_enforcement.go |
Introduces the transition struct and updates enforcement. |
pkg/workflow/compiler.go |
Constructs the transition at the production call site. |
pkg/workflow/safe_update_enforcement_test.go |
Updates direct unit-test calls. |
pkg/workflow/compiler_threat_detection_formal_test.go |
Updates formal security-test calls. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Clean refactor. Replacing four positional bool parameters with a named PullRequestEventTransition struct improves readability and eliminates the risk of argument-order bugs. All call sites and tests are updated consistently, and the zero value of the struct correctly maps to false, false, false, false. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.4 AIC · ⌖ 9.86 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — approving. The refactor is clean and well-scoped.
📋 Key Themes & Highlights
Positive Highlights
- ✅
PullRequestEventTransitionis well-named, uses the project's domain language, and is clearly documented with a godoc comment explaining the privilege-escalation intent - ✅ All call sites (production + tests) updated consistently — no drift between the new struct and the old positional booleans
- ✅ Zero behavioural change:
hasPullRequestTargetEscalationlogic unchanged, struct fields map 1:1 - ✅ Formal tests correctly use the zero-value
PullRequestEventTransition{}for non-escalation scenarios, preserving their original specification intent
Minor Observation (non-blocking)
EnforceSafeUpdate is now at 6 parameters. If future work adds more groups, the remaining positional args (secretNames, actionRefs, currentRedirect, currentMemoryValidationScripts) could benefit from a wider options struct — but that is out of scope here.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 19.3 AIC · ⌖ 9.44 AIC · ⊞ 7K
Comment /matt to run again
🧪 Test Quality Sentinel Report✅ Test Quality Score: 93/100 — Excellent
📊 Metrics (13 tests)
NotesAll 13 test changes are pure mechanical call-site refactors — replacing 4 positional boolean parameters ( No new test functions were introduced. The test-to-production line ratio is ≈1:1 (19 added test lines, 19 added production lines) — no inflation. Verdict
|
Triage: refactor / risk: lowScore: 34/100 (impact+urgency+quality) · Priority: low · Action: Reduces EnforceSafeUpdate parameter count via options struct (golint-custom finding). CI green. Automated PR triage — see full report issue for details.
|
PR Triage\n\n- Category: refactor\n- Risk: low\n- Priority: medium\n- Score: 58/100 (impact 25 + urgency 15 + quality 18)\n- Recommended action:
|
PR Triage
Automated triage by PR Triage Agent.
|
PR TriageCategory: Score breakdown
Recommended action:
|
|
@copilot Quick triage for maintainer-ready follow-up: Outstanding review items (newest first): No unresolved review threads were available from the current compact context; please verify the latest reviewer expectations and prepare this PR for maintainer handoff. Failed checks from the compact candidate set: None listed. Branch update was requested automatically for this run when GitHub allows it.
|
Handled in |
PR TriageCategory: refactor | Risk: low | Score: 68/100
Recommended action:
|
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Confirmed on current head |
golint-customflagged two function-signature findings:extractNestedYAMLValueexceeding the 60-line limit, andEnforceSafeUpdateexceeding the 8-parameter limit.extractNestedYAMLValue(pkg/parser/schema_suggestions.go)Already resolved by a prior change —
buildNestedYAMLScalarMatchersandextractNestedYAMLScalarhelpers have already been extracted, leaving the function at 55 lines. No further action needed here.EnforceSafeUpdate(pkg/workflow/safe_update_enforcement.go)Grouped the four pull_request/pull_request_target boolean flags into a new
PullRequestEventTransitionstruct, bringing the function from 9 parameters down to 6:compiler.goto construct the struct.safe_update_enforcement_test.goandcompiler_threat_detection_formal_test.go.hasPullRequestTargetEscalation) is unchanged, just reads from the new struct fields.