Preserve JSONL rows during repo-memory merge conflicts - #60663
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ 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
- "registry.npmjs.org"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. 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
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
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.
|
|
No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation satisfies the stated conflict policy and includes representative regression coverage.
Pull request overview
Adds checkout-local Git attributes to preserve concurrent JSONL appends during repo-memory merge conflicts.
Changes:
- Applies Git’s union merge driver to
*.jsonl. - Retains local-wins behavior for other files.
- Adds regression coverage and a patch changeset.
File summaries
| File | Description |
|---|---|
actions/setup/js/push_repo_memory.cjs |
Configures the local JSONL merge policy before retry pulls. |
actions/setup/js/push_repo_memory.test.cjs |
Tests preservation of concurrent JSONL rows. |
.changeset/patch-repo-memory-jsonl-merge.md |
Documents the patch-level behavior change. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — no blocking issues, two suggestions to tighten test coverage and error-handling clarity around the new merge-policy helper.
📋 Key Themes & Highlights
Key Themes
- Test/wiring gap: the new regression test validates
configureRepoMemoryMergePolicy+git mergemanually, but doesn't verify it's actually called before the retrygit pullinmain(). - Error-handling ambiguity:
configureRepoMemoryMergePolicy(workspaceDir)and thegit pullshare onetry/catch, so a genuine failure to write the merge-union attribute is indistinguishable from a benign "branch doesn't exist yet" pull failure in the logs.
Positive Highlights
- ✅ Verified locally:
git merge -X ourswithoutmerge=uniondrops the remote-only JSONL row, while the new attribute correctly preserves rows from both sides — the core fix works as intended. - ✅ Scoping the attribute to checkout-local
info/attributes(not committed) is the right call — avoids polluting the memory branch with repo-specific merge policy. - ✅ Idempotent write (checks
attributeLines.includes(...)before appending) avoids duplicate entries across retries. - ✅ Changeset included.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 78.5 AIC · ⌖ 15 AIC · ⊞ 10.4K
Comment /matt to run again
| // Note: this may produce a merge commit; if so, pushSignedCommits | ||
| // will fall back to git push for this retry attempt. | ||
| try { | ||
| configureRepoMemoryMergePolicy(workspaceDir); |
There was a problem hiding this comment.
[/tdd] The new test covers configureRepoMemoryMergePolicy in isolation plus a manual git merge, but nothing asserts it's actually invoked before the retry pull in main(). If this call were accidentally removed or moved after the pull, no test would fail.
💡 Suggested coverage
Add a test that spies on configureRepoMemoryMergePolicy (or asserts call ordering) during the retry-push path to confirm it always runs before git pull -X ours. This closes the gap between "the helper works standalone" and "the helper is correctly wired into the flow it's meant to protect."
@copilot please address this.
There was a problem hiding this comment.
Added a source-level ordering assertion in push_repo_memory.test.cjs that fails if configureRepoMemoryMergePolicy(workspaceDir) is removed or moved after the retry git pull -X ours (f16bd25).
| // Note: this may produce a merge commit; if so, pushSignedCommits | ||
| // will fall back to git push for this retry attempt. | ||
| try { | ||
| configureRepoMemoryMergePolicy(workspaceDir); |
There was a problem hiding this comment.
[/diagnosing-bugs] configureRepoMemoryMergePolicy is called inside the same try block as the pull, so if it throws (e.g. git rev-parse --git-path or fs.appendFileSync fails), the error is swallowed by the generic catch (pullError) and logged as "Pull on retry failed (may be expected for new branches)" — even though the pull never ran and the merge policy was never set up.
💡 Suggested fix
This conflates two distinct failure modes:
- Expected "pull failed on new branch" (benign, safe to ignore)
- Unexpected failure to configure merge attributes (silently causes the union-merge feature to be disabled without any dedicated warning, then falls through to a plain conflict-prone pull attempt)
Consider calling configureRepoMemoryMergePolicy(workspaceDir) in its own try/catch (or outside the pull's try) with a distinct core.warning on failure, so a broken merge-policy setup doesn't masquerade as a normal missing-branch pull failure and is visible to operators debugging why JSONL rows were still lost.
@copilot please address this.
There was a problem hiding this comment.
configureRepoMemoryMergePolicy now runs in its own try/catch and emits a distinct core.warning about JSONL rows possibly being lost, so it can no longer be reported as a benign pull failure (f16bd25).
There was a problem hiding this comment.
Review scope
This PR only changes non-UI code (Git merge-driver configuration, retry-loop wiring, tests, changeset), so no Impeccable UI mode applies. Performed a direct correctness/security review instead.
Summary
The approach is sound: setting *.jsonl merge=union in the checkout-local info/attributes file before the retry git pull -X ours correctly preserves rows from both sides for JSONL conflicts while leaving -X ours in effect for everything else. The included regression test exercises the actual git merge behavior end-to-end and passes.
One non-blocking suggestion left inline: the shared try/catch around configureRepoMemoryMergePolicy + git pull could mask a genuine merge-policy configuration failure behind the generic "Pull on retry failed (may be expected for new branches)" log message.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 90.5 AIC · ⌖ 13.6 AIC · ⊞ 8.4K
| // Note: this may produce a merge commit; if so, pushSignedCommits | ||
| // will fall back to git push for this retry attempt. | ||
| try { | ||
| configureRepoMemoryMergePolicy(workspaceDir); |
There was a problem hiding this comment.
The try/catch here wraps both configureRepoMemoryMergePolicy(workspaceDir) and the git pull. If configureRepoMemoryMergePolicy throws (e.g. .git/info is not writable, or rev-parse --git-path fails), it will be swallowed by the same generic message: Pull on retry failed (may be expected for new branches): .... That message is misleading for a merge-policy configuration failure and could hide a real, actionable problem (union-merge policy silently not applied, falling back to plain -X ours local-wins behavior that this PR is meant to fix) behind a log line that reads as an expected/benign case.
Consider either:
- Calling
configureRepoMemoryMergePolicy(workspaceDir)in its owntry/catchwith a distinct warning message, or - Including the specific failing step (e.g. "merge policy setup" vs "git pull") in the logged message.
@copilot please address this.
There was a problem hiding this comment.
Split the merge-policy setup out of the pull's try/catch; a failure there now logs a dedicated warning instead of the "Pull on retry failed (may be expected for new branches)" message (f16bd25).
|
@copilot this PR still needs forward progress. Unresolved review feedback to address first:
Failed checks from the current head: Please refresh the branch as needed, address the unresolved review feedback, and then run the 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
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Concurrent repo-memory pushes previously resolved every conflict with the local version, potentially discarding remotely added
.jsonlrows.Changes
JSONL conflict policy
*.jsonl.Scope
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.laiyagushi.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.