Repository navigation
π fix(pushbroker): amend normalised HEAD blobs, not working-tree content - #16157
Conversation
Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
|
Hive status β updated in place; edits do not notify. Full history is in the Activity Monitor. hive/awaiting-ci: previous π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 Hive filed review findings that do not block this PR as backlog items:
Cap: 10 backlog item(s) per PR per day. Review findings from The content-preservation fix is correct, but the stderr guard it depends on does not cover the production runner. Read Finding β
|
There was a problem hiding this comment.
The amend is now content-preserving for the commit, but it destroys the agent's uncommitted edit on disk. Recommend one more pass before this lands.
Mechanism (read at head e566405):
src/crux/pushbroker/pushbroker.go:680now readsgit show HEAD:<rel>β good, the commit gets HEAD's content normalised.src/crux/pushbroker/pushbroker.go:690still doesos.WriteFile(abs, normalized, β¦)β the working-tree file is overwritten with HEAD's normalised blob. Any uncommitted edit to that path is gone, with no backup and no log line.pushbroker.go:701commit --amend --only -- <touched>then commits that working-tree content, as before.
So the failure mode in #16144 ("edit folded into the commit") becomes "edit silently deleted from the working tree". For an agent mid-edit on a file it already pushed once, the old behaviour at least kept its bytes somewhere; the new one loses them. The doc comment added at pushbroker.go:665-668 describes this ("The normalised blob replaces the working-tree copy") as if it were fine β I'd treat it as the remaining defect.
The new test TestBrokerAmendExcludesWorkingTreeEditsOfTouchedFile (pushbroker_test.go:1099) writes leaked() to disk and asserts only git show HEAD:main.go; it never reads main.go back, so it passes while the edit is destroyed. Adding os.ReadFile(filepath.Join(dir, "main.go")) == edited after Run would fail on this head.
Two ways to make it genuinely content-preserving:
- Don't touch the working tree or index at all. Build the amended commit with a temporary index:
GIT_INDEX_FILE=<tmp> git read-tree HEAD,git hash-object -w --stdinper touched file,git update-index --cacheinfo <mode>,<blob>,<path>,git write-tree,git commit-tree <tree> -p HEAD^ β¦(reuse author/message via-Ffromgit log -1 --format=%B),git update-ref HEAD <new>.--onlyis no longer needed because the index is private. More git calls, but the agent's tree and index are untouched. - Minimal: read the working-tree bytes before the
WriteFile, and restore them after the amend succeeds (and on the error paths). Simpler, still a window where the file is wrong, and it leaves the mode/infologic as is.
Either way the changelog line ("uncommitted edits are never folded into the commit") stays true; today it is true only because the edits no longer exist.
Minor, no action needed: looksBinary now runs on HEAD's blob rather than the disk file β same heuristic, correct source.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
stripTrailingBlankLines overwrote the working-tree file with HEAD's normalised blob and left it there, so an agent's uncommitted edit to a touched path was deleted from disk. Snapshot any file that differs from HEAD before overwriting it and put it back once the amend has run, on both the success and failure paths. Files identical to HEAD are left normalised as before so the workspace stays clean. Hive-Fix-Attempt: 1/6 Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Correctness β delta re-review at 26624bd (vs e566405): approve.
Prior blocker fixed: stripTrailingBlankLines now snapshots a working-tree file that differs from HEAD before overwriting it (src/crux/pushbroker/pushbroker.go:703-709) and the deferred restore (pushbroker.go:676-682) writes it back on every exit path, joining any restore error into the named return. Both the success and failed-amend paths are covered (pushbroker_test.go:1097-1125, :1127-1149), and the Go test shards are green on this head.
Advisory (low, not blocking, adjacent to #16144): content is preserved, but staged state is not. git add -- <touched> (pushbroker.go:716) replaces any index entry the agent had staged for that path with the normalised blob, and the restore only rewrites the file on disk β so a previously staged edit comes back as an unstaged modification. Pre-existing behaviour of the old git add path; noting it since the new doc comment says "no uncommitted edit is lost".
β hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
The normaliser now commits the bytes `git show HEAD:<path>` returns, and ExecRunner captures stderr with stdout, so a GIT_TRACE* variable inherited from the agent process would amend a trace line into the pushed blob. Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Read the full function, Broker.git, ExecRunner.Run and PushEnv at head 26624bd. The core change β normalise HEAD:<path> rather than the on-disk file, snapshot/restore uncommitted edits around add + commit --amend --only β does what #16144/#16145 ask, the restore-on-failure defer is sound, and the three new tests exercise the leak, the restore, and the git show failure path. One defect needs fixing before this lands.
git show output is CombinedOutput, so any stderr text becomes committed file content β severity: medium
src/crux/pushbroker/pushbroker.go:692reads the blob withb.git(ctx, "show", "HEAD:"+rel).b.git(pushbroker.go:800-806) returns whateverExecRunner.Runreturns, andExecRunner.Runiscmd.CombinedOutput()(pushbroker.go:241) β stdout and stderr interleaved.- That byte slice is treated as the blob: normalised, written over the working tree (
pushbroker.go:710),git added and amended into HEAD (pushbroker.go:717-722).
Mechanism: anything git writes to stderr on a successful show lands inside the committed file. PushEnv (pushbroker.go:859-874) only strips the credential prefixes at pushbroker.go:51-54, so GIT_TRACE*, GIT_CURL_VERBOSE, or an inherited config that makes git warn (unreadable ~/.gitconfig, ambiguous HEAD refname, etc.) all pass through to the broker's git. Reproduced on this checkout:
$ GIT_TRACE=1 git show HEAD:changelog.d/fixed-16144.md 2>&1 | head -2
06:07:30.606073 git.c:479 trace: built-in: git show HEAD:changelog.d/fixed-16144.md
- Pushbroker newline normalisation now amends ...
With the PR as written, that trace line would be the first line of the amended changelog.d/fixed-16144.md. Before this PR the content came from os.ReadFile, so this exposure is new here. The existing rev-parse/diff --name-only callers share the same runner, but there contamination fails a parse; here it silently rewrites a source file and pushes it.
Suggested fix: read the blob through a stdout-only path β e.g. give CommandRunner (or a sibling helper used just here) an Output()-based call so stderr is only folded into the error message, as b.git already does on failure. A test with a runner that appends a stderr line to the show HEAD: output and asserts the amended blob is unchanged would pin it.
Minor, not blocking: the three test fakes (nthCallFailingRunner, failingRunner) match on "show HEAD:main.go"/"commit --amend" substrings β fine, just noting that a stdout-only runner method would need the same fakes extended.
I have not approved or requested changes as a review event; this is advisory.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
β¦blobs ExecRunner.Run returned CombinedOutput, so any exit-0 stderr from `git show HEAD:<path>` (config warnings, ambiguous-ref warnings) was written over the file and amended into the pushed commit. Return stdout alone on success and append ExitError.Stderr only on failure so error text and isEmptyAmendError keep working. GIT_TRACE* scrubbing stays as defence in depth. Adds an ExecRunner contract test and an end-to-end normaliser test whose `git show` emits stderr noise. Hive-Fix-Attempt: 2/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
β¦malize.go pushbroker.go grew past its crux-file-size-ratchet baseline (919 > 911); move the stripTrailingBlankLines helpers into their own file. No behaviour change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Conclusion: the core change is right β reading HEAD's blob instead of the working-tree file (src/crux/pushbroker/normalize.go:52) does close the fold-in leak, and the snapshot/restore in the defer at normalize.go:36-42 is exercised by both the success and the failed-amend tests. One regression, though, which I reproduced against git.
Regression: a path deleted in HEAD but still on disk now fails the whole push
changedFiles() runs with --no-renames (pushbroker.go:458,464,467), so deletions appear in res.ChangedFiles. stripTrailingBlankLines then does os.Lstat (normalize.go:47) β which passes when the file still exists on disk β and immediately git show HEAD:<rel> (normalize.go:52), returning the error on any failure (normalize.go:53-54).
For a commit that stops tracking a file that stays on disk (the ordinary git rm --cached + .gitignore case for a generated artefact), git exits 128:
$ git rm --cached out.bin && git commit -m "stop tracking"
$ git diff --no-renames --name-only master...HEAD
out.bin
$ git show HEAD:out.bin
fatal: path 'out.bin' exists on disk, but not in 'HEAD'
So Run() fails at pushbroker.go:363-364 with normalising trailing newlines: reading out.bin from HEAD: β¦ for a perfectly valid commit. On v5 this case was a continue (the on-disk file was read and, unless it ended in \n\n, left alone). The doc comment at normalize.go:21-23 still says "a changed file that was deleted has nothing to normalise", but the mechanism now only honours that when the file is also gone from disk.
Suggested fix: treat "not in HEAD" as skip, e.g. probe with git cat-file -e HEAD:<rel> before the show (or match git's exists on disk, but not in message), and add a test: commit a file, git rm --cached it in a second commit, leave it on disk, assert Run() pushes. TestBrokerSurfacesReadFailureDuringNormalisation would still cover the genuine read-failure path.
Docs drift (low)
src/cmd/hive/automaxprocs_banner_test.go:45-46sayspkg/pushbroker'sExecRunnercaptures "stdout AND stderr into one buffer via cmd.CombinedOutput()"; after this PR it is stdout-only on success (pushbroker.go:246).- The new
TestGitEnvDropsInheritedGitTracecomment (pushbroker_test.go) says "the runner captures stderr alongside stdout", which this same PR makes untrue β thegitEnv()comment atpushbroker.go:691-696gets it right ("defence in depth").
Everything else I checked holds: releasesentinel/git_retagger.go:215-219 only uses out for error text, which still carries stderr via exitErr.Stderr; --only -- <paths> keeps staged-but-uncommitted paths out of the amend as before; git show HEAD: returns the unsmudged blob, so an autocrlf/LFS working-tree copy is just snapshotted and restored.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Read the tree at 96b6025 (not just the diff). This looks correct to me.
What I verified
src/crux/pushbroker/normalize.go:76-89β content comes fromgit show HEAD:<rel>, the on-disk file is snapshotted only when it differs from HEAD's blob, and thedeferat:46-52restores every snapshot whether or not the amend succeeds, joining restore errors into the returned error.TestBrokerAmendExcludesWorkingTreeEditsOfTouchedFileandTestBrokerRestoresWorkingTreeEditWhenAmendFailsfail against the old on-disk read, so the leak in #16144 is covered.normalize.go:56-58β gating onheadPaths(ls-tree -r -z) means a path the outgoing commits deleted but the agent re-created uncommitted is skipped instead of being resurrected;TestBrokerNormalisationSkipsPathDeletedInHeadpins it.ExecRunner.RunswitchingCombinedOutputβOutput(pushbroker.go:242-254) is the widest-blast-radius change, so I walked every consumer of a successful result:pushbroker.golines 162, 328, 366, 458β467, 514, 529, 573, 629, 668β677 are all stdout-only plumbing (rev-parse,diff --name-only,ls-tree,rev-list,log,show), and thepushresult at:421is discarded.releasesentinel.GitRetagger(git_retagger.go:69) also defaults toExecRunner; its only stderr-dependent read isleaseLostMarkeratgit_retagger.go:179-180, which is on the error path whereExitError.Stderris still appended, andisAncestor(:225-233) keys off exit code, not text. No caller regresses.- Checked empirically that plain
git show HEAD:<path>does not apply adiff.*.textconvfilter to the blob, so no converted text can be committed via this path.
One note for whoever merges, not a defect: the doc comment at normalize.go:33-36 is accurate that a staged edit to a touched path comes back unstaged after the amend (the git add at :91 replaces the index entry). That's a deliberate trade-off and it's documented; flagging only so it isn't a surprise. #16161 carries the follow-up doc fix for GitRetagger.git.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
CI triage β Job 124964866598 on No code change needed; a re-run of the workflow should go green. π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Read the diff against the v5 tree and the callers. This looks correct to me.
What I verified
src/crux/pushbroker/normalize.go:68-83: the normalised bytes now come fromgit show HEAD:<path>, and an on-disk copy that differs from HEAD is snapshotted before the overwrite and restored by thedeferat:45-51on both the success and failure paths.TestBrokerAmendExcludesWorkingTreeEditsOfTouchedFile/TestBrokerRestoresWorkingTreeEditWhenAmendFailsfail against the oldos.ReadFile-based implementation, so the fix is pinned.headPaths(normalize.go:109-121) is what makes the "deleted by outgoing commits, re-created uncommitted" case safe;scriptedGitinpushbroker_coverage_test.go:63was taught the newls-treecall so the fake-workspace scenarios are unchanged.- The
ExecRunner.Runchange tocmd.Output()(pushbroker.go:243-253) is package-wide, so I checked the other consumer:src/pkg/releasesentinel/git_retagger.go:177-181only inspects push output on the error path (leaseLostMarker), where stderr is still appended viaExitError.Stderr.isAncestorand therev-parse/rev-listreads use stdout. No behaviour change there. go build ./crux/pushbroker/ ./pkg/releasesentinel/passes atb44b28d; CI test shards are green.
One behavioural note, not a defect (documented in the doc comment at normalize.go:28-36): when a touched path has an agent edit that was staged, git add of the normalised blob replaces the index entry, so the edit survives on disk but comes back unstaged. That is a deliberate trade-off and matches the issue's ask (#16144: don't fold uncommitted edits into the amend); flagging only so nobody is surprised by a git status change after a broker push.
Body matches the diff; closes #16144 and #16145 as stated.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
β¦tree sandboxExecRunner, the runner production Brokers actually use, still returned CombinedOutput, so exit-0 git stderr could be amended into the pushed commit via `git show HEAD:<path>`. It now delegates to pushbroker.ExecRunner (stdout only on success). headPaths scopes `git ls-tree` to the changed files instead of walking the whole HEAD tree, and its doc comment no longer implies no `git show` runs. Test scaffolding: fold hookedRunner into failingRunner via an optional hook. Changelog now mentions the staged-edit side effect and the runner change. Hive-Fix-Attempt: 6/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Read the diff against the tree at 0e94fdd (cloned the branch; go build ./crux/pushbroker/ ./crux/agent/ passes). This looks correct to me.
What I checked, beyond the diff:
- Trust boundary of
git show HEAD:<path>.git showon a blob honoursdiff.<driver>.textconv, which would turn the new read into agent-controlled command execution as the broker's UID. That is already closed:rejectHostileWorkspaceConfig(src/crux/pushbroker/pushbroker.go:325) runs beforestripTrailingBlankLines(:363) and rejects any local/worktree key outsideallowedWorkspaceConfigKeys(:128), so adiff.*.textconvkey refuses the push outright.core.hooksPathis pinned to/dev/null(:86) for theadd/commit --amendthat follow. - Stdout-only runner has no other consumer that relied on stderr. Every
b.git(...)whose output is used on success is data (rev-parse,diff --name-only -z,ls-tree,config --list,rev-list,var,show);push/worktree addoutput is only read in the error path, where stderr is still appended (pushbroker.go:683,src/crux/sandbox/worktree.go:63-64).sandbox.WorktreeManageris fed by the samesandboxExecRunner(sandbox_executor.go:168) and usesrev-parsestdout only (worktree.go:66). - Restore path. The deferred restore in
normalize.go:47-53runs on both the success and failure returns, joins rather than replaces the amend error, and only snapshots files whose on-disk bytes differ from HEAD (:82-84) β so an untouched workspace stays clean after the amend, as the doc comment says.headPathsuses the same-zsplitting aschangedFiles, so a deleted-then-recreated path is skipped rather than failinggit show.
Body and Closes #16144 / Closes #16145 match the diff: HEAD blob is what gets normalised, and the "no additional git call" sentence is replaced with the actual calls made.
One non-blocking note: after a successful amend, a path the agent had staged comes back unstaged (documented at normalize.go:33-36). That is the right trade-off for a push broker, but worth a line in the changelog fragment for operators β which the fragment already has.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
The normalisation amend read and rewrote the on-disk file, so uncommitted working-tree edits to a touched file were folded into the amended commit. stripTrailingBlankLines now normalises HEAD's blob (git show HEAD:), making the amend content-preserving. The doc comment that claimed no git calls is corrected to describe the calls made. Test added for the leak; read-failure test adapted to the git show path.
Closes #16144
Closes #16145
β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88
Runner-wide change
pushbroker.ExecRunner.Run(and the sandbox runner that production Brokers use, which now delegates to it) returns stdout only on success, appending stderr only on failure, andgitEnvstrips inheritedGIT_TRACE*. This stops exit-0 git warnings from being committed as file content by the normaliser, and affects everyCommandRunnerconsumer (e.g.releasesentinel.GitRetagger) β only in the safer direction, since no consumer relied on stderr in a successful result. Staged edits to normalised paths are restored to the working tree only, so they come back unstaged after the amend.