Skip to content

compile: stop adding ineffective merge=ours to lock.yml .gitattributes entry - #50639

Merged
pelikhan merged 3 commits into
mainfrom
copilot/re-add-merge-ours-to-gitattributes
Aug 5, 2026
Merged

compile: stop adding ineffective merge=ours to lock.yml .gitattributes entry#50639
pelikhan merged 3 commits into
mainfrom
copilot/re-add-merge-ours-to-gitattributes

Conversation

Copilot AI commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

gh aw compile appended merge=ours to the lock-file .gitattributes entry. merge=ours is not a built-in git merge driver — without merge.ours.driver configured it is inert, falsely implying generated-file conflicts are auto-resolved. The old code also rewrote any pre-existing line starting with the lock-yml glob, clobbering repository-owned policy on every compile.

Changes

  • pkg/constants/constants.go: WorkflowsLockYmlGitAttributesEntry drops merge=ours.github/workflows/*.lock.yml linguist-generated=true. Added WorkflowsLockYmlGitAttributesEntryLegacy for the old value, used only to detect entries gh-aw previously wrote.
  • pkg/cli/git.go (ensureGitAttributes): replaced the strings.HasPrefix(glob) match with an exact match against the legacy entry. Only gh-aw's own prior entry is migrated; other repo-owned lock-yml lines are left untouched.
  • Tests / docs: updated expectations, added cases for legacy-entry migration and repo-owned-policy preservation; updated create.md and pkg/constants/README.md.

Behavior

Existing line Result
absent adds ... linguist-generated=true
... linguist-generated=true merge=ours (legacy gh-aw) migrated, merge=ours removed
... linguist-generated=true merge=union (repo-owned) untouched

The repository's own legacy entry will be migrated on the next compile run.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix ineffective merge=ours in .gitattributes handling compile: stop adding ineffective merge=ours to lock.yml .gitattributes entry Aug 5, 2026
@pelikhan
pelikhan marked this pull request as ready for review August 5, 2026 17:38
Copilot AI balanced review requested due to automatic review settings August 5, 2026 17:38
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR does not have the implementation label and has ≤100 new lines of code in business logic directories (38 additions detected).

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Test Quality Sentinel completed test quality analysis.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

⚠️ PR Code Quality Reviewer failed during code quality review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes ineffective merge=ours handling while preserving repository-owned .gitattributes policies.

Changes:

  • Adds exact legacy-entry migration.
  • Updates tests and documentation.
  • Updates the repository’s own lock-file attribute.
Show a summary per file
File Description
.gitattributes Removes merge=ours.
create.md Updates authoring guidance.
pkg/cli/add_gitattributes_test.go Updates add-command expectations.
pkg/cli/git.go Restricts migration to the exact legacy entry.
pkg/cli/gitattributes_test.go Tests migration and policy preservation.
pkg/cli/init_command_test.go Updates initialization expectations.
pkg/cli/init_integration_test.go Updates integration expectations.
pkg/cli/init_test.go Updates initialization tests.
pkg/constants/README.md Updates the generated constants appendix, but incompletely.
pkg/constants/constants.go Defines current and legacy entries.

Review details

Tip

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

  • Files reviewed: 10/10 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread pkg/constants/README.md Outdated
| `constants.go` | `const` | `WorkflowsDir` | `const WorkflowsDir = ".github/workflows"` | WorkflowsDir is the GitHub Actions workflow directory path (without trailing slash). |
| `constants.go` | `const` | `WorkflowsDirSlash` | `const WorkflowsDirSlash = WorkflowsDir + "/"` | WorkflowsDirSlash is WorkflowsDir with a trailing slash. |
| `constants.go` | `const` | `WorkflowsLockYmlGitAttributesEntry` | `const WorkflowsLockYmlGitAttributesEntry = WorkflowsLockYmlGlob + " linguist-generated=true merge=ours"` | WorkflowsLockYmlGitAttributesEntry is the . |
| `constants.go` | `const` | `WorkflowsLockYmlGitAttributesEntry` | `const WorkflowsLockYmlGitAttributesEntry = WorkflowsLockYmlGlob + " linguist-generated=true"` | WorkflowsLockYmlGitAttributesEntry is the . |
Copilot AI requested a review from pelikhan August 5, 2026 17:43

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is correct and well-implemented.

  • merge=ours without a configured merge.ours.driver is a no-op, so removing it fixes a misleading gitattributes entry.
  • The migration uses an exact-match against the legacy constant rather than HasPrefix, which correctly prevents clobbering repository-owned policy (e.g. merge=union).
  • The duplicate-entry case for repo-owned lines is intentional and tested: git unions attributes from both matching lines.
  • Tests cover all four scenarios (absent, legacy, current, repo-owned), and docs are updated consistently.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16 AIC · ⌖ 10.2 AIC · ⊞ 5.4K

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /diagnosing-bugs and /codebase-design — approving with one minor documentation note.

📋 Key Themes & Highlights

Key Themes

  • Root cause properly addressed: the merge=ours attribute was silently inert without a configured merge driver; removing it eliminates the false assurance.
  • Surgical migration logic: switching from strings.HasPrefix to exact match on the legacy constant ensures only gh-aw's own prior entry is touched — repository-owned policy lines are preserved.
  • Good test coverage: new cases for legacy-entry migration and repo-owned-policy preservation directly capture the previously missing scenarios.

Minor Finding

  • WorkflowsLockYmlGitAttributesEntryLegacy is not yet reflected in pkg/constants/README.md — see inline comment.

Positive Highlights

  • ✅ Clear naming: legacy constant name makes the intent unmistakable.
  • ✅ Behaviour table in the PR description precisely documents all three cases.
  • ✅ All existing tests updated consistently.
> 🧠 *Reviewed using Matt Pocock's skills by [Matt Pocock Skills Reviewer](https://github.com/github/gh-aw/actions/runs/31030978513)* · sonnet46 · 19.2 AIC · ⌖ 9.78 AIC · ⊞ 7.1K > Comment /matt to run again

Comment thread pkg/constants/README.md Outdated
| `constants.go` | `const` | `WorkflowsDir` | `const WorkflowsDir = ".github/workflows"` | WorkflowsDir is the GitHub Actions workflow directory path (without trailing slash). |
| `constants.go` | `const` | `WorkflowsDirSlash` | `const WorkflowsDirSlash = WorkflowsDir + "/"` | WorkflowsDirSlash is WorkflowsDir with a trailing slash. |
| `constants.go` | `const` | `WorkflowsLockYmlGitAttributesEntry` | `const WorkflowsLockYmlGitAttributesEntry = WorkflowsLockYmlGlob + " linguist-generated=true merge=ours"` | WorkflowsLockYmlGitAttributesEntry is the . |
| `constants.go` | `const` | `WorkflowsLockYmlGitAttributesEntry` | `const WorkflowsLockYmlGitAttributesEntry = WorkflowsLockYmlGlob + " linguist-generated=true"` | WorkflowsLockYmlGitAttributesEntry is the . |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] The new WorkflowsLockYmlGitAttributesEntryLegacy constant is missing from the README appendix — only WorkflowsLockYmlGitAttributesEntry appears on this line.

If this README is auto-generated (e.g. via make), re-run the generator to include the new constant. If it is manually maintained, add the missing row so the constant is discoverable.

@copilot please address this.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: N/A — No New Tests

Analyzed 0 new or modified test functions. The 5 changed test files contain only string-constant updates (expected .gitattributes value changes) — no new behavioral test functions were added or modified.

📊 Metrics
Metric Value
Changed test files 5
New func Test* added 0
Modified test functions 0
🚨 Violations 0

Changed test files:

  • pkg/cli/add_gitattributes_test.go (+2/-2)
  • pkg/cli/gitattributes_test.go (+17/-11)
  • pkg/cli/init_command_test.go (+3/-3)
  • pkg/cli/init_integration_test.go (+1/-1)
  • pkg/cli/init_test.go (+2/-2)

All changes are string-literal updates reflecting the new .gitattributes constant (removing merge=ours).

Verdict

passed. No new behavioral tests to evaluate; existing tests updated to match changed constant.

🧪 Test quality analysis by Test Quality Sentinel · sonnet46 · 28.6 AIC · ⌖ 8.98 AIC · ⊞ 7.7K ·
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: N/A — No New Tests. 0 violations. Existing tests updated to reflect constant changes only.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

PR Triage: #50639

Category: refactor | Risk: low | Priority Score: 50/100 (impact 20, urgency 15, quality 15)
Recommended action: batch_review
Grouped with compile/trial cluster (50640, 50638).

Generated by 🔧 PR Triage Agent · auto · 45.2 AIC · ⌖ 2.9 AIC · ⊞ 8K ·

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please follow up on the latest review feedback on this PR:

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 10.8 AIC · ⌖ 5.72 AIC · ⊞ 8.3K ·
Comment /souschef to run again

…ME appendix, update counts

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot AI commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please follow up on the latest review feedback on this PR:

Fixed in the latest commit: added the missing WorkflowsLockYmlGitAttributesEntryLegacy row to the appendix table, corrected the Constants count (305→306), and updated the Additional symbols count (99→100). Also fixed the truncated description for WorkflowsLockYmlGitAttributesEntry while I was there.

Copilot AI requested a review from gh-aw-bot August 5, 2026 19:33
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please follow up on the latest review feedback on this PR:

Generated by 👨🍳 PR Sous Chef · run: https://github.com/github/gh-aw/actions/runs/31043697225

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 12.2 AIC · ⌖ 6.03 AIC · ⊞ 8.3K ·
Comment /souschef to run again

@pelikhan
pelikhan merged commit 86aa220 into main Aug 5, 2026
26 of 28 checks passed
@pelikhan
pelikhan deleted the copilot/re-add-merge-ours-to-gitattributes branch August 5, 2026 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compile: re-adds ineffective merge=ours to .gitattributes, overwriting repo-owned policy

4 participants