Repository navigation
Fix repo-memory pushes on shared branches - #66682
Conversation
Preserve sibling workflow files on shared memory branches and scope formatting and custom validation to eligible files. Include normalized existing files in the managed commit and pass Git authentication through ancestry and retry operations. Related to githubnext/gh-aw-rai#214. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed for PR #66682: no 'implementation' label (has_implementation_label=false) and 0 new lines in default business logic directories (threshold 100, requires_adr_by_default_volume=false). Evidence: /tmp/gh-aw/agent/adr-prefetch-summary.json; no custom .design-gate.yml present (defaults used).
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
There was a problem hiding this comment.
🟡 Changes recommended
The validation copy can retain empty sibling directories outside the eligible scope.
1 open finding
What changed in this PR
Preserves sibling workflow files on shared repo-memory branches while keeping normalization, validation, staging, and retry authentication correctly scoped.
Changes:
- Applies file eligibility consistently during normalization and validation.
- Stages normalized existing files without deleting sibling files.
- Propagates authentication and ambient environment through Git retry operations.
| File | Description |
|---|---|
docs/src/content/docs/reference/repo-memory.md |
Documents shared-branch eligibility behavior. |
actions/setup/js/push_signed_commits.cjs |
Authenticates partial-clone ancestry probes. |
actions/setup/js/push_signed_commits.test.cjs |
Tests ancestry-probe environment propagation. |
actions/setup/js/push_repo_memory.cjs |
Preserves siblings and scopes formatting, validation, and commits. |
actions/setup/js/push_repo_memory.test.cjs |
Adds shared-branch concurrency regression coverage. |
actions/setup/js/memory_custom_validation.cjs |
Filters disposable validation copies. |
actions/setup/js/memory_custom_validation.test.cjs |
Tests filtered validation without source mutation. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs (fallback heuristic used — triage agent not invoked given the diff's small, well-scoped surface area already fully analyzed). No blocking issues found.
📋 Key Themes & Highlights
Root cause addressed, not just symptoms
The fix threads an isEligibleFile predicate through formatJSONFiles, runCustomMemoryValidation's disposable copy filter, and the commit's managedPaths/literalPathspecs set, rather than deleting sibling files up front (the old filterIneligibleMemoryFiles call). This directly matches the root cause in the linked issue: destructive deletions of sibling-workflow files on a shared branch, not just the symptomatic rebase failure.
Strong regression coverage
The new it.each([false, true])("preserves sibling files through validation and concurrent-head retries ...") test in push_repo_memory.test.cjs is a genuine end-to-end regression test using a disposable bare remote and two writers — it reproduces the original unstaged-changes rebase failure, verifies sibling preservation, validation scope, normalized-file staging, and a successful retry onto a concurrent head. This is a textbook /diagnosing-bugs regression test: it would have failed before the fix and passes after.
Consistent env propagation fix
gitEnv = { ...process.env, ...(gitAuthEnv || {}) } in reconcileRepoMemoryRetry and the analogous fix in push_signed_commits.cjs's ancestry probe correctly address the could not read Username / lost PATH/HOME failure mode — previously execGitSync/getExecOutput received only gitAuthEnv (dropping ambient env), now both are merged, consistent with the existing pattern already used elsewhere in push_signed_commits.cjs (e.g. lsRemoteHeadOid, pushBranchAndResolveHead).
Positive highlights
- ✅
isEligibleFiledefaults to() => trueinformatJSONFiles, preserving backward compatibility for the two other call sites (safe_outputs_handlers.cjs,validate_memory_step.cjs) that don't pass a predicate. - ✅ New
literalPathspecs.length === 0early-return inpush_repo_memory.cjscleanly handles the case where normalization touches no eligible files, avoiding an unnecessarygit status/addon an empty pathspec set. - ✅ Docs updated (
repo-memory.md) to describe the new shared-branch preservation behavior and clarify the validator copy now reflects "eligible" files only. - ✅ No unrelated changes; the diff stays tightly scoped to the three implicated files plus tests and docs.
I did not find actionable issues worth blocking on. The PR's own "Remaining validation" section is transparent about the unverified npm ci/tsc gate (pinned vite unavailable) — this is an environment constraint, not a code defect, and the described verification (Go build, lint-cjs, fmt-cjs, 11 passing regression/unit cases via a dependency-free harness) is reasonable compensating evidence.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 98 AIC · ⌖ 13.5 AIC · ⊞ 10.1K
Comment /matt to run again
There was a problem hiding this comment.
Reviewed with the impeccable skill's harden and audit modes (bug-fix PR touching edge-case eligibility filtering, credential propagation, and retry logic).
Scope reviewed: memory_custom_validation.cjs, push_repo_memory.cjs, push_signed_commits.cjs, their test files, and docs/repo-memory.md.
Findings: No blocking issues.
- The
isEligibleFilepredicate is threaded consistently throughformatJSONFiles,runCustomMemoryValidation's disposable copy, andscanDirectory's eligibility check, so sibling files from other workflows sharing a branch are no longer normalized, validated, or deleted — matching the stated root cause. gitAuthEnvis now correctly merged intoprocess.env(not replacing it) forfetch/rebase/rebase --abortinreconcileRepoMemoryRetry, and for themerge-base --is-ancestorancestry probe inpush_signed_commits.cjs— fixing the "could not read Username" and "unstaged changes" failures described in the linked issue.managedPathscorrectly includesledgerActivity.normalizedso normalized-but-unchanged-via-copy files are still staged/committed, and the new early return onliteralPathspecs.length === 0avoids an unnecessarygit status/add/commit cycle while still writing the ledger summary.- New regression test (
it.each([false, true])(...)) is thorough: verifies sibling preservation, scoped validation, normalization, clean worktree, retry onto a concurrent head, and credential/env propagation through mockedexecGitSync. - Docs updated to describe the new shared-branch scoping behavior and the validator's "eligible files" copy.
- Ran the affected vitest files directly; confirmed failures in
push_repo_memory.test.cjs(17 on base vs 14 on this PR's head) are pre-existing, environment-dependent (network/cert, token-redaction formatting) and unrelated to this change — not introduced by it.
No actionable inline comments to add.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 135.4 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
|
@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: 922bfee
|
…push-failure 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>
|
@copilot run pr-finisher skill |
|
🎉 This pull request is included in a new release. Release: |
Summary
Preserve files managed by sibling workflows on a shared repo-memory branch instead of deleting them when they do not match the current workflow's filters. Apply the same eligibility predicate to JSON normalization and the validator's disposable copy, and include normalized existing files in the scoped commit. Propagate Git authentication and the ambient environment through the ancestry probe and retry fetch/rebase operations.
Root cause
Addresses githubnext/gh-aw-rai#214.
Observed in run 37678745693, workflow revision
44d482df4239e62407b3b2d99040de0298430a91, using setup runtime v0.91.1:memory/rai-dsb-validationwith sibling validation workflows, but itsfile-globselected onlyjailbreak-upia-*.jsonl.push_repo_memorydeleted 30 sibling files from the checkout. Those deletions were not included in the artifact-scoped commit.cannot rebase: You have unstaged changes.could not read Username.The fix does not stage the sibling deletions, discard them, or stash around the failure: it prevents those destructive deletions in the first place.
Verification
make fmt-cjs,make lint-cjs, the Go build, module syntax checks, schema freshness, andgit diff --checkpassed. Impacted Go validation found no changed Go files.Remaining validation
The standard pinned JavaScript suite and type-check remain unverified:
npm cifailed because pinnedvite@8.3.2was unavailable from both the configured registry and the approved 1ES feed. Dependencies were not changed. The finalmake agent-report-progressgate was run with the installed modern Bash and stopped at missingtscafter the other gates passed.No workflow was dispatched and no hosted fix is claimed. The affected repository must consume a release of the corrected runtime before this changes its future runs.