Scope autoCommit to exactly the files each write touches - #796
Open
jacobjove wants to merge 4 commits into
Open
Conversation
… files written document_update (and create) staged the whole backlog dir and then ran a bare `git commit -m msg` with no pathspec, so it swept in any other dirty state already sitting in the working tree or index — including a concurrent peer's unreviewed plan draft, landing under a misleading "Add document" message. createDecision, updateTasksBulk, archiveTask, and completeTask shared the same unsafe shape (some already staged narrowly but still committed with a bare, unscoped `git commit`). Route them all through addFile/stageFileMove + commitFiles, which stage and commit only the exact paths each write touched. Also hardens commitFiles: without `--no-renames`, its internal staged-paths check collapses a staged delete+add pair (e.g. an archive move) into a single rename entry naming only the destination, silently dropping the source path from the commit — this affected the pre-existing archiveMilestone/renameMilestone callers too. And guards against a requested path with no git history at all (e.g. created and archived in the same operation while autoCommit was off), which isn't a valid pathspec and would otherwise abort the whole commit. Five more call sites (promoteDraftWithUpdates, demoteTaskWithUpdates, archiveDraft, promoteDraft, demoteTask) share the same unsafe pattern and are left for a follow-up to keep this change reviewable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ed files Follow-up to the document/decision/bulk-task/archive auto-commit scoping fix. The five remaining draft/task lifecycle auto-commit sites in Core still used the unsafe stageBacklogDirectory() + bare commitChanges() pattern, which stages the whole backlog directory and commits with no pathspec. That sweeps a peer's untracked file living inside backlog/ and any unrelated pre-staged change into the commit. Each site now commits scoped to exactly the paths it moved, reusing the existing commitWrittenFile() helper (stage the rename-or-add, then commitFiles() with the exact paths): - promoteDraftWithUpdates / demoteTaskWithUpdates: use the original draft/task filePath (already in scope) plus the saved path. - archiveDraft: resolve the source path via getDraftPath() before fs.archiveDraft unlinks it; derive the archive destination from the now-public FileSystem.getArchiveDraftsDir(). - promoteDraft (single-shot): thread the previous and saved paths out of the create-lock closure (savedPath was previously discarded). - demoteTask (single-shot): reimplement inline (mirroring promoteDraft) so the moved paths are in scope, rather than delegating to fs.demoteTask, which returns only a boolean. Adds src/test/draft-lifecycle-autocommit-scope.test.ts: one regression test per site (git fixture + unrelated staged deletion + peer untracked file inside backlog/), asserting the commit excludes both and the unrelated dirty state survives. Verified RED (old pattern swept UNRELATED.txt and peer-plan.md) then GREEN. Full affected suites (147 tests) and bunx tsc --noEmit pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
addAndCommitTaskFile() ran an unscoped `git reset HEAD` before staging its own file, then committed the whole index. Single-user that is invisible, but with a concurrent session in the repo it silently discards work: a peer that has staged a hunk and not yet committed loses its index, and its subsequent `git commit` lands empty or short with nothing announcing the loss. Stage the task file and commit it through commitFiles(), which pathspecs the commit to exactly that file. The pathspec is what enforces the isolation the reset was reaching for, without touching anyone else's staged work. This preserves BACK-163's intent, which introduced the reset: its acceptance criteria were that task edit "should only stage and commit the modified task file" and "should not commit any other staged or unstaged changes in the repository". The reset was the mechanism, not the goal, and a scoped commit reaches the goal without the collateral damage. It also completes the auto-commit scoping pass (MrLesk#795/MrLesk#796), which scoped every add and commit but left this reset. resetIndex() and commitStagedChanges() have no callers left and are removed; the scoped resetPaths() sibling is untouched and remains the convention. Verified: new regression test fails on the old code (the peer's staged path comes back empty) and passes now; full suite 1719 pass / 0 fail; and the rebuilt CLI binary keeps a peer's staged hunk through a `backlog task edit`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author
|
Pushed a third commit, for one more site in this family.
It now stages and commits through The reset came from BACK-163, whose acceptance criteria were that task edit "should only stage and commit the modified task file" and "should not commit any other staged or unstaged changes in the repository". Isolation was the goal and the reset was the mechanism; a scoped commit reaches the same goal without clearing an index it does not own. The new test fails on the old code, where the peer's staged path comes back empty, and passes now. |
commitFiles pathspecs the `git diff --name-only --cached` output to scope the commit. Under git's default core.quotepath, a non-ASCII path (e.g. an em dash in a task filename) comes back octal-escaped and double-quoted, so the quoted pathspec matches no file: the commit aborts with "did not match any file(s)" and the file is left staged. Force core.quotepath=false at the execGit spawn so every git command emits raw UTF-8 paths, keeping the scoped-commit pathspec correct. Adds a regression test that creates a task with an em-dash title under autoCommit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #795.
Every
autoCommitsite that staged the whole backlog directory, or finished with a bare pathspec-lessgit commit, now stages and commits exactly the paths it wrote.First commit —
document_update/create,createDecision,updateTasksBulk,archiveTask,completeTaskrouted throughaddFile/stageFileMove+commitFiles. Also hardenscommitFilesitself:--no-renames, so its staged-paths check stops collapsing a delete+add pair into a rename entry naming only the destination (this silently dropped the source path, and it affected the existingarchiveMilestone/renameMilestonecallers)Second commit — the five remaining draft/task lifecycle sites (
promoteDraftWithUpdates,demoteTaskWithUpdates, and the archive paths) reusecommitWrittenFile()rather thanstageBacklogDirectory()+ barecommitChanges().Third commit —
addAndCommitTaskFile(), behind every task create and edit. Same shape as the sites above (stages narrowly, then commits with no pathspec), plus an unscopedgit reset HEADbefore staging that discards a concurrent session's staged work outright rather than only sweeping it. Now goes throughcommitFiles(); the deadresetIndex()andcommitStagedChanges()are removed. See the comment below for why this does not regress BACK-163, which introduced the reset.Tests cover what the old shape allowed: a peer's dirty or untracked file under
backlog/must not appear in a document/decision/bulk/lifecycle commit, a peer's staged file must survive a task create/edit and stay out of its commit, plus bothcommitFilescases above.bunx tsc --noEmitclean; suite green on currentmain.Split into three commits because each is a follow-up sweep over sites the previous one missed — happy to squash.