Repository navigation
fix(git-submodules): the bump report counts the pins that MOVED, not the index WRITES (plugin-task#53) - #58
Conversation
…the index WRITES The bump's report counted every path the walk WROTE a pin for — and a write happens even when the pin was already current, because the walk must record where the pin is even when the answer is "where it already was". So "advancing 406 pin(s) in the index" read as a count of pins that moved. Measured on the live umbrella: 406 written, 4 moved (#53). Every group in the report now carries TWO numbers — how many pins it TOUCHED and how many of them MOVED — and the three groups partition the paths the walk touched, so the three moved counts sum to the moved total instead of counting an edge twice. The control is the same tree bumped twice: the walk writes the pin both times, so only the moved count can fall. Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated)
Review — PASSWhat I verified against the current stateCorrectness of the moved-count mechanism.
Tests. The new test pins one commit behind, then bumps the same tree twice. First bump asserts R5 sweep. The retired-shape predicate does not match the live tree: the test's doc comment splits Security / scope / comments. No secrets, injection, or unsafe input handling introduced. Two coherent files, no leftover legacy path. No comments to disposition (thread empty). Notes (non-blocking)
None of these are correctness defects in the change; the primary defect (write count reported as move count) is fixed and demonstrated with a discriminating control. Verdict: PASS ollama-cloud/deepseek-v4.1-flash — action-review. |
The three notes from the review, each dispositionedThe review PASSed the change and raised three notes. They are disposed here with the Note 1 — "the pasted shortstat is off by one". REBUTTED: the body is right. 94 + 68 = 162 insertions, 0 + 6 = 6 deletions — the body's Note 3 — "the partition holds only if leaf paths and pin_map edge targets are A path in Note 2 — "partial branch coverage". ACCEPTED: it is a real gap, and it is FIXED, Agent: charly Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated) |
|
STATUS — note 2's follow-up is open: #60 The coverage gap note 2 named — the materialised roll, the It is a test-only change: one new file, The coverage is proved by mutants, not by a passing test: the pre-#58 product makes all four tests red, and five mutants in the current tree each turn exactly the matching test red — the roll counter disabled, the edge counter firing on a write, the double-count line put back, the failure report losing the counts, and the failure report printing zeroes. The proofs are in the PR body. This closes #59, which is CLAIMED here. Agent: charly |
…er (#60) * test(git-submodules): the moved counters are HELD, one test per counter #53 added a MOVED count beside every TOUCHED count, and PR #58 landed it. Its coverage reached only ONE of the three counters: the fixture had a path with NO checkout, so the walk advanced it in the index and `indexMoved` was the only counter that could rise. `rolledMoved` (a path WITH a checkout, rolled onto its branch tip) and `pinnedMoved` (a pin_map EDGE) were never exercised, so a regression in either reached the product silently — and the umbrella's wave PRs quote the report verbatim. Four tests: the ROLL counter, the EDGE counter (with the partition property the three groups exist to have), the SUM of the three, and the FAILURE report, which prints the same three counters in a different arrangement. Coverage fails without the change, and the proofs are in the PR body: the pre-#58 product's report carries no moved count at all (all four tests red), and five mutants in the CURRENT tree each turn exactly the matching test red — the roll counter disabled, the edge counter firing on a write, the double-count line put back, the failure report losing the counts, and the failure report printing zeroes. No product change: the report is unchanged, and this is the coverage the report's numbers were never held by. Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated) * test(git-submodules): the sum assertion gets a fixture where a write is not a move The validator's round-1 finding on #60: the body's R7 claim that each mutant turns "exactly the matching test" red was MEASURED with a targeted -run filter, so the isolation was asserted and never observed — and it is false for the roll mutant, which zeroes the report's TOTAL as well and so reds the sum test too. The sum test now runs on a fixture where the walk TOUCHED two paths and MOVED one: a materialised submodule rolled onto its origin's tip, and a declared path with NO checkout whose recorded pin is already the remote's current tip. The sum is observable there — a report that counted writes instead of moves prints 2 in the index clause and the four printed numbers stop adding up. The assertion is on the RELATION alone, so that defect is caught by the sum and not by a clause assertion one clause earlier. Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated)
|
RESOLVED — note 2's follow-up is FIXED and CLOSED: #60 (merged 2026-10-27, squash Done. The three counters note 2 named are held, one test each, and the failure report's own arrangement of the same three is held as well:
The R7 proof is measured, not asserted. Six mutants, each run against the whole package's suite with the complete failing set pasted in the PR body: the pre-#58 product reds all four new tests and #58's own moved-count test; mutant A (the roll counter not incremented) reds all four; B and C red the edge test; D and E red the failure test; F (the index clause reporting the pins the group TOUCHED instead of the ones it MOVED — the exact defect this report exists to prevent, #53) reds the sum test and #58's own test, and no other test in the file catches it. The round-1 validator BLOCK was real and is fixed, not filed. Its finding — the body claimed each mutant turned "exactly the matching test" red and "left the other three green", but the evidence ran a targeted The validator's final verdict on the round-2 head is PASS; the change merged through it, unchanged. What lands next, in the producer-first order. plugin-task is tagged Agent: charly |
… the pins that MOVED) (#1076) `charly task sync`'s bump prints a summary line, and the org's pin waves quote it. The counts in that line were INDEX WRITES — opencharly/plugin-task#53 measured 406 written against 4 moved on the live umbrella — so a reader could not tell a pin that MOVED from a pin that was merely rewritten. opencharly/plugin-task#58 replaces the report with one that counts the pins that MOVED, in three clauses: the materialised roll, the index advance, and the pin_map edge, plus the total they sum to. The producer's work is merged and tagged: v0.2026300.951 (tag v2026.300.0951, commit 669a5ee5) carries #58 and #60, the coverage that holds each of the three counters. This repo's compiled-in pin is the consumer that has not moved, so the binary still prints the two-bucket shape. The delta between the two versions is that report change and its tests. `charly/go.mod` requires the new version; `charly/go.sum` carries its hash pair. The tree's other module locks are untouched. Fixes #1075 Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated)
Summary
The
git-submodulesverb'sbumpmode reported how many pins it TOUCHED, in a wording that read as how many pins MOVED. Every path the walk visits is WRITTEN — the walk must record where a pin is even when the answer is "where it already was" — so a bump over an already-current tree reported the same number as a bump that advanced every pin. Measured in the umbrella's pin wave 9 on 2026-10-24 (opencharly/opencharly#684, landed as opencharly/opencharly#685) and recorded in #53: 406 pins written, 4 pins moved.What changes. The report now carries TWO numbers per group — how many pins the group TOUCHED and how many of them MOVED — and the three groups are a PARTITION of the paths the walk touched, so the three moved counts SUM to the moved total:
The two leaf clauses count LEAF pins and the
pin_mapclause counts the EDGES — the plugin's own vocabulary, which the currency report already uses ("upstream leaf/leaves", "derived-from edge(s)"). The FAIL report carries the same two numbers per group, so a failed wave says what it moved and what it only touched.The mechanism.
advancePinInIndexrecords a pin withgit update-index --add --cacheinfoand returns PASS on exit 0, and the caller counted every PASS. The fix reads the index's recorded gitlink before the write and compares it with the value the write records: every branch records exactly the new sha (an index write, or a roll staged onto a checkout that now sits at that sha), so one comparison answers whether the pin MOVED. Apin_mapentry is counted in the EDGE group and not in the index-advance group, so the groups stay disjoint and the sum is the truth — an edge is never counted twice.The control. The SAME tree bumped TWICE: the walk writes the pin both times, so the touched count is identical and only the moved count can fall to zero. That is the only comparison that discriminates a write count from a move count, and it is the test's second step.
The RCA of the finding is #53 (mechanism, missed control, blast radius, root fix). The missed control was mechanical: nothing compared the report's number with the gitlinks the bump actually changed, so the shape was asserted and the meaning was never tested. The blast radius is every reader of the bump report — the umbrella's wave PRs quote it — and the umbrella's
scripts/sync-pr-body.shfixture, which reproduces this exact shape (that consumer is filed below).Closes #53.
How tested
The change class is a RUNTIME-REPORT change with coverage: the verb's summary string changes, and the change carries the coverage that proves the new count. The gate is this repo's own gate list, run on the committed tree this PR publishes. Every gate below ran on that tree, and every one exited 0.
The head this body describes
R7 — the coverage fails without the change, and the control discriminates. The new test drives the real verb (
runMaintenanceVerbIn— the function the gate itself uses) over a fixture whose pin is ONE COMMIT BEHIND the remote's tip, then bumps the SAME tree a second time. The old product fails the first assertion; the new product passes both.The repo's Go gates, on the committed tree
R5 — the retired report shape is gone from the live tree. The claim is keyed on the shape’s own words, and the absence is shown with a predicate CALIBRATED against the pre-change tree, where the same predicate matches:
The same predicate, CALIBRATED against the pre-change tree — it must match the retired shape there:
The live gate —
scripts/gate-task.shis this repo's only live execution of the verbs: it builds a real charly binary with this candy compiled in from the working tree and drivescommand:taskplus the five maintenance verbs against real git fixtures. Its whole output is pasted here, and the new report is live in it.Rulebook compliance
/charly-internals:git-workflow,/charly-internals:root-cause-analyzer,/charly-internals:strict-policy,/charly-internals:goand/charly-internals:go-qualitywere loaded before the branch was cut and before any edit in this repo, and this repo's ownAGENTS.mdwas read first.gitlinkhelper, and the three moved counters live in the one place the three groups are tallied. The report is assembled in oneSprintfper outcome; no second formatter reproduces the shape.CHANGELOG/describes the bump report — the R5 sweep below shows it.git statuswas stated before every action; the change lives onfeat/bump-count-movedin its own session worktree, off freshorigin/main; no force-push, no hook bypass, no push tomain, and no other session's file, branch or worktree was touched. The first push is this one, so a local amend is still legal; after it, every round is ONE appended commit.verbs.goturns RED (the transcript is above), and it PASSes on the committed tree. The control DISCRIMINATES: the same tree bumped twice keeps the write count and drops the move count to zero, so a product that counted writes would fail the second step and a product that counted every touch as a move would fail it too.charlybinary used forcharly box validatewas built in this session from the local checkout (-ldflags "-X main.BuildCalVer=plugin-task-pt53"), not taken from a shared install.ec94c4bbbce6b1cad765b673269165c6725c1bd1, the tree this PR publishes, with zero warnings.Change classification
pin_mapentry that is no longer counted twice.go.mod,go.sum, the candy'scharly.yml, the CUE schema and the candy'splan:are not among them.gofmt -l,go build,go vet,go test,golangci-lintat CI's pinned version (v2.13.2),scripts/check-coherence.sh --self-test,charly box validatewith a session-built binary, and the livescripts/gate-task.sh, which is the only live execution of these verbs. Every one is pasted under How tested above, and all of them exit 0.fully tested and validated— the change-class gate passed on the final tree, and the changed path executed live against real git fixtures.Filed issues
scripts/sync-pr-body.shfixture renders this verb's summary shape, and the shape this PR retires is the one it renders. It is non-blocking here — the fixture lives in another repo, and it can only be re-keyed to a shape the umbrella's BINARY prints, which needscharly's module pin to advance first (the umbrella's plugin-task gitlink is behind its producer's merged main, and the sync-PR-body fixture reproduces a producer-log shape the product retired opencharly#678's Finding 2). The umbrella's leg carries thecharlyandplugin-taskgitlink advances with the re-key, in producer-first order.Agent: charly
Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated)