Repository navigation
Deduplicate AW import normalization in source policy lint rules - #71887
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The focused refactor preserves existing behavior and diagnostics, with regression coverage and no blocking issues identified.
No open issues 🎉
What changed in this PR
This PR centralizes AW import normalization in the source-policy lint rules, addressing #71499 while preserving caller-specific behavior and diagnostics.
Changes:
- Extracts
authoredImportEntriesto normalize direct and mapping-form imports. - Updates both consumers while preserving absent-versus-null handling.
- Adds regression coverage for import shapes and all five downstream deferral rules.
| File | Description |
|---|---|
pkg/workflow/lint/source_policy.go |
Shares import normalization between policy deferral and conditional-import checks. |
pkg/workflow/lint/source_policy_test.go |
Covers supported shapes, null handling, and downstream rule behavior. |
Review effort: Max
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
![github-actions[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: COMMENT
No actionable blocking issues found in the changed import normalization and deferral logic. The reviewed behavior preserves the existing caller-specific handling for top-level and imports.aw null values, and the added tests cover the supported shapes and downstream deferral rules.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~2 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
🏗️ Design Decision Gate — ADR RequiredThis PR adds more than 100 new lines under 📄 Draft ADR committed: Decision captured: AW import-shape handling (
📋 What to do next
Once the PR body links an ADR, this gate re-runs and checks that the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. 📋 Michael Nygard ADR Format Reference
ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design (pr-triage was skipped in favor of direct classification: this is a clean refactor/dedup change) — one non-blocking suggestion, otherwise solid.
📋 Key Themes & Highlights
Key Themes
- Shared helper, preserved semantics:
authoredImportEntriescleanly unifies theimports/imports.awresolution and[]any/[]stringnormalization previously duplicated acrossauthoredImportsPendingandcheckConditionalImports, while preserving each caller's distinct error messages. - Minor fragility: the
path == "imports"string comparison (source_policy.go:66) used to detect "top-level null" vs "imports.aw null" is an implicit coupling to the helper's path-formatting convention rather than an explicit signal — flagged inline with a concrete alternative.
Positive Highlights
- ✅ Excellent regression coverage: new table-driven
TestAuthoredImportEntriescovers absent/null/empty/malformed/conditional-entry cases, andTestAuthoredImportDeferralRulesexercises the shared logic across all five downstream rules (ToolScopeRule,PushTargetRule,CheckoutModeRule,PullRequestTargetCheckoutRule,NeedsConsistencyRule). - ✅ Error messages don't leak user-controlled values (
PRIVATE_VALUEassertions confirm this). - ✅
[]stringnormalization avoids unnecessary allocation when the list is nil/empty.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~31 AIC · ⌖ ~15 AIC · ⊞ 10.2K
Comment /matt to run again
| entries, path, _, err := authoredImportEntries(input.Document.Frontmatter) | ||
| if errors.Is(err, errNullImportList) { | ||
| // Top-level null has historically meant no inherited configuration. | ||
| if path == "imports" { |
There was a problem hiding this comment.
[/codebase-design] The path == "imports" string comparison is a fragile way to distinguish top-level-null from imports.aw-null. It silently breaks if authoredImportEntries ever changes its path format (e.g., adds a third level), and the coupling between the two functions isn't obvious from the signature.
💡 Suggested fix
Have authoredImportEntries return a distinguishable sentinel (e.g., a small importLocation enum/bool like isNestedAw bool) alongside path, so callers branch on an explicit value rather than re-parsing the string the function itself produced. This keeps the 'top-level null is historically OK' special case robust to future path changes.
entries, path, nested, present, err := authoredImportEntries(frontmatter)
...
if nested { // imports.aw
return false, errors.New(...)
}
return false, nil // top-level imports: null@copilot please address this.
There was a problem hiding this comment.
Added explicit nested-import metadata to authoredImportEntries, removing the path-string check while preserving top-level imports: null and nested imports.aw: null behavior. Fixed in de945ee.
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: bf2d489
|
…e-aw-import-normalization Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Authored-policy deferral and conditional-import linting duplicated import-shape handling, risking inconsistent validation as supported forms evolve.
authoredImportEntriesto resolveimportsorimports.aw, normalize[]anyand[]string, and return entries, effective path, presence, and shape errors.awremains absent;aw: nullremains invalid. Top-levelimports: nullretains its intentional caller-specific treatment.