Feat/eng 2516#577
Conversation
…rite messages Replace the whitelist tuple enumeration in classifyTuple with rule-based stagedDiff/unstagedDiff/untracked helpers that are total over the statusMatrix encoding's value range. Out-of-range columns still throw GitError so a real encoding shift surfaces loudly. Surfaces two silent-drops the previous whitelist missed: - [1,1,3]: now reports both staged + unstaged modifications, matching native git (was reporting only the staged side after the prior fix) - [1,0,2]: now reports both staged-modified + unstaged-deleted (was dropping the staged side entirely; same bug class as [1,1,3]) Extract formatOverwriteMessage into git-error-messages.ts, used by all five overwrite paths (checkout / merge / pull, with and without file lists). Drop the "stash them" trailer because brv has no stash command; trailer now reads "commit or discard". The "would be overwritten" anchor is preserved so vc-handler's UNCOMMITTED_CHANGES mapping still works. Cross-property invariants in the unit suite now run exhaustively over the full (h, w, s) cartesian product (24 tuples x 6 invariants = 144 assertions) so any future algorithmic regression is caught structurally. Drops the brittle source-scan format-lock test in favor of helper unit tests against the extracted formatter.
|
Claude finished @bao-byterover's task in 5m 46s —— View job PR Review: feat/ENG-2516Tasks
SummaryScore: 4 / 5 This is a well-scoped, well-reasoned fix. The root cause ( What works well
IssuesSuggestion — integration test uses old pull filter, not new classifier Nitpick — Nitpick — shared state in fuzz harness summary test Minor — No security issues found. Correctness is solid for all reachable tuples. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
- Switch cross-property invariant test to classifyTuple().dirty so it
tracks pull()'s live filter instead of the removed raw formula
- Add expectedTuple identity assertions to every named-tuple scenario
in the reachability fuzz, locking scenario names to actual matrix rows
- Fix [1,2,1] and [1,2,3] recipes that silently produced [1,1,1] and
[1,2,2] due to stat-cache aliasing on same-size payloads (utimes bump,
same workaround the existing [1,1,3] scenario uses)
- Remove misnamed [1,0,2] scenario; the tuple is unreachable by the
encoding (w=0 forces s in {0,3}, never 2) and the recipe duplicated [1,0,3]
- Add comments noting the (s=2,w=0) and [0,0,0] unreachable edges in
status-row-classifier
|
Claude finished @bao-byterover's task in 2m 12s —— View job PR Review: feat/ENG-2516 (Re-review after synchronize)Tasks
Re-Review SummaryScore: 5 / 5 All four issues from the previous review have been addressed cleanly, plus two bonus fixes surfaced by the work. This is ready to merge. Previous issues — all resolvedSuggestion (was: critical) — integration test used old pull filter Nitpick — Nitpick — shared state dependency in summary test Minor — Bonus fixes in commit 724a54b
No new issues found. Correctness, test coverage, and documentation are all solid. |
2 participants
Summary
brv vcdeadlocked whenstatusreported the working directory clean whilepullerrored "would be overwritten" andpusherrored "Run brv vc pull first." User had no actionable signal and no documented escape.vc add→ editor autosave reverts disk content).[1,1,3]is the canonical isomorphic-git encoding for "WORKDIR matches HEAD, INDEX diverges from both" and was silently dropped by the status whitelist.parseMatrixwith a singleclassifyTuplehelper derived from the isomorphic-git encoding spec, routed through everystatusMatrixconsumer (status, pull dirty-check, staged classifier, unstaged classifier,guardStagedConflicts,resetUnstage).brv vc reset <file>.formatOverwriteMessagehelper with unified "commit or discard" trailer.vc-handlerpattern-matches on it to mapGitError→VcError(UNCOMMITTED_CHANGES).classifyTuplebecause the bug fix itself required deriving classification from the encoding rules instead of an ad-hoc whitelist; flagging here so reviewers know to expect more diff than a one-tuple addition.Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)parseMatrixinsrc/server/infra/git/isomorphic-git-service.tswas a hardcoded whitelist of 11[head, workdir, stage]tuples. Tuple[1,1,3](workdir matches HEAD, stage diverges from both) is reachable viavc addfollowed by a disk-only revert (editor undo, agent revert, two-editor race) and was missing from the list, so status silently treated it as clean.pullused a genericrow[2] !== 1 || row[3] !== 1filter, caught[1,1,3], and refused.[1,1,3]) had no test.Test plan
test/unit/infra/git/status-row-classifier.test.ts(new, 245 lines) - exhaustive per-tuple expectations for the classifier.test/unit/infra/git/git-error-messages.test.ts(new, 58 lines) - format-lock for each operation × file-count combo.test/integration/infra/git/status-tuple-reachability.test.ts(new, 316 lines) - fuzz that constructs every reachable tuple via real isomorphic-git operations and asserts the cross-property invariant.test/integration/infra/git/isomorphic-git-service.test.ts(+101 lines) - adds[1,1,3]repro and pull-error-names-files assertion.test/unit/infra/transport/handlers/vc-handler.test.ts(+36 lines) - confirms anchor string still maps toVcError(UNCOMMITTED_CHANGES).vc addthen disk-only revert, status now reports the file as staged + modified instead of clean.abcdef.md").status.isClean === trueimplies pull never throws "would be overwritten", verified across all reachable tuples.User-visible changes
brv vc statusnow reports[1,1,3]files (added then disk-reverted) as staged + modified instead of silently treating them as clean.brv vc pulloverwrite error now lists the affected files so users know which paths tobrv vc reset. Old format: "Local changes would be overwritten by pull. Commit or discard your changes first." New format when files are known: "Your local changes to the following files would be overwritten by pull:\n\t\nPlease commit or discard your changes before you pull."Evidence
[1,1,3]repro test inisomorphic-git-service.test.tsfails onmain, passes on this branch. Tuple-reachability fuzz exercises every reachable encoding.test/unit/infra/git/git-error-messages.test.ts.Checklist
npm test)npm run lint)npm run typecheck)npm run build)main(merge commit 674d061)Risks and mitigations
status-tuple-reachability.test.tsfuzz constructs every reachable tuple via real git operations and asserts old behavior for the 11 previously-whitelisted tuples is preserved.validateEncodingthrows if isomorphic-git ever emits an out-of-range column, surfacing future encoding drift loudly instead of silently misclassifying.vc-handler.test.tsadds an explicit format-lock case that the anchor still maps toVcError(UNCOMMITTED_CHANGES).