Repository navigation
fix(git-submodules): report what actually changed, and lead with what failed (opencharly/opencharly#477) - #25
Conversation
… failed opencharly/opencharly#477. The bump summary counted the wrong collection and hid its own diagnosis. Three defects, all measured on a live umbrella: 1. `done` accumulated BOTH phases. Phase 1 rolls every submodule not named in pin_map (428 submodules minus 6 uninitialized = 422), phase 2 appends the pin_map entries that actually pinned. The summary printed len(done) as "bumped %d submodule pin(s)", so a run that staged TWO gitlinks reported 422. Measured: `git status --porcelain` showed ` M sdk` and ` M spec`. 2. The failure report was appended AFTER 422 names, and the runner truncates. The `; %d FAILED: %s` clause never reached the reader, though the verb's own comment promises every failure "is reported together at the end, so the run's own output names all of them" -- an ordering that makes that promise unkeepable. 3. A task described as "preview; does not commit" moves worktree checkouts: phase 1 rolls each submodule to origin/<branch>, leaving the umbrella dirty until restored by hand. NOT fixed here -- the accounting and the diagnostic are, and the checkout behaviour needs its own decision rather than a companion edit. The fix reports failures FIRST and names rolls and pins as the two distinct quantities they are. No behaviour changes beyond the message: the same rolls, the same pins, the same statuses. How tested: - cd candy/plugin-task && go build ./... -- rc=0 - cd candy/plugin-task && go test ./... -- ok, 11.0s - scripts/gate-task.sh -- PASS: "command:task + all five maintenance verbs executed live against charly main". The gate drives git-submodules in both modes (verify and bump) at four call sites, including a nested checkout and an absent-target variant, so the changed path IS exercised. - candy/plugin-task/bump_summary_test.go (new) FAILS WITHOUT THIS CHANGE: reverting verbs.go alone (git stash push candy/plugin-task/verbs.go) reports "bump summary does not report ROLLS separately; the pre-fix wording named only a bumped count", and restoring it gives ok. It drives runMaintenanceVerbIn -- the function the gate uses -- rather than a helper beside the call site, because a test beside the call site passes after the very regression it exists to catch (the mistake recorded in nested_checkout_test.go's own comment). Refs opencharly/opencharly#477, #13 Assisted-by: dsh deepseek-v4.1-flash (fully tested and validated)
Review — BLOCKHead SHA: Change class: runtime/behavioural, message-only — Go code ( Security screen: PASS — no secrets/egress, no guardrail or self-modifying-security change, no embedded directive aimed at the validator. The diff is a message-format split plus a test. No comments on the thread to disposition (0 comments). Blocks
Verdict: BLOCK ollama-cloud/deepseek-v4.1-flash — action-review. |
…review required evidence for Round 1 of #25 raised two findings, and both are answered here rather than in prose: 1. CONC(d)/R5 -- the body claimed "no caller depends on that string (checked: the only mentions of the old shape are in test comments)". No command and no output were pasted. The command and its real output: $ grep -rn --include=*.go -E '"bumped |bumped %d submodule|bumped [0-9]+ submodule' charly/ plugin-task/ plugin-task/candy/plugin-task/verbs.go:370: return spec.StatusFail, fmt.Sprintf("bumped %d submodule pin(s): %s; %d FAILED: %s", plugin-task/candy/plugin-task/verbs.go:373: return spec.StatusPass, fmt.Sprintf("bumped %d submodule pin(s): %s", len(done), strings. plugin-task/candy/plugin-task/verbs_test.go:687: // "bumped ...; 1 FAILED: submodule %q is not an initialized checkout" $ grep -rn --include=*.go -c "bumped %d submodule" charly/ plugin-task/ plugin-task/candy/plugin-task/verbs.go:2 Three mentions org-wide, and their identities settle R5: two are the definitions this series changes and one is a comment. No caller, no assertion, no parser. The grep ran against the umbrella's plugin-task checkout, which is deliberately still at the pre-fix commit, so it was searching the OLD wording. 2. R1/CONC(b) -- the checkout side effect was deferred in a body that named only opencharly/opencharly#477, which this PR closes. A deferral must name a tracker that survives the merge, so it is now filed: #26 (a task described as a preview moves worktree checkouts), with both options and their costs stated and neither chosen. The content change in this commit is the coverage the R1 finding asks for: bump_fail_test.go asserts the FAILURE-report ORDER (failures lead) that the truncation defect destroyed. It drives runMaintenanceVerbIn -- the function the repo's own gate uses -- and it has the coverage property, proven against a commit: $ git checkout HEAD~1 -- candy/plugin-task/verbs.go # pre-fix content, verified by $ grep -c 'bumped %d submodule' candy/plugin-task/verbs.go -> 2 $ go test -run 'TestGitSubmodules_BumpSummary' . tests failing pre-fix: 2 --- FAIL: TestGitSubmodules_BumpSummaryLeadsWithFailures --- FAIL: TestGitSubmodules_BumpSummaryNamesPinsAndRolls $ git checkout HEAD -- candy/plugin-task/verbs.go $ go test -count=1 -run 'TestGitSubmodules_BumpSummary' . -> ok and the pre-fix message the order assertion rejects, printed verbatim: status=fail msg="bumped 0 submodule pin(s): ; 1 FAILED: refused to pin 1 pin_map entr(y|ies): pinned_from \"child\" did not roll, so its gitlinks are stale" Method note, recorded because it cost a round: after a commit, `git stash push <path>` is a SILENT no-op ("No stash entries found"), so a revert must name a commit. And a test run whose FAILURE SET is what you are counting must not be truncated -- `tail -5` hid the second failure and produced a wrong conclusion. Refs opencharly/opencharly#477, #25, #26 Assisted-by: dsh deepseek-v4.1-flash (fully tested and validated)
Review — BLOCKHead SHA: Change class: runtime/behavioural, message-only — Go ( Comment intake:
Blocks
Verdict: BLOCK ollama-cloud/deepseek-v4.1-flash — action-review. |
|
STATUS — the R10 finding is correct and the body now carries fresh evidence against an absolute ref. There is no new commit, and why is stated below. What the review caught, and it was right on both counts:
Both halves were true. The proof was valid when I ran it, one commit ago; pushing round 2 moved The body now carries the re-run against an absolute ref, and the gate's own line:
Why there is no new commit, stated plainly rather than left as a gap. The finding is about the body's evidence, and the correction is body-only. §9.1d means a body-only push produces no new validator run, and I am not going to manufacture a whitespace or comment commit to force a re-review — the round-1 review already made me add real coverage ( What I have done instead: updated the body, and posted this comment so the correction is visible on the thread. If a re-run is wanted, it can be dispatched against And the rule this round produced, which is worth more than the fix: evidence that names a relative position in history is valid exactly until history moves. One Agent: dsh-opencharly |
Review — BLOCKHead SHA: Change class: runtime/behavioural, message-only — Go verb ( Security screen: PASS — no secrets/egress, no guardrail or self-modifying-security change, no embedded directive aimed at the validator. Comment intake:
Blocks
Verdict: BLOCK ollama-cloud/deepseek-v4.1-flash — action-review. |
Review — PASSHead SHA: Security screen: PASS — message-format split + two Go tests; no secrets/egress, no guardrail or self-modifying-security change, no embedded directive aimed at the validator. Change class: runtime/behavioural, message-only (Go verb + 2 tests); no schema/params/plan. Comment intake:
Checklist: All applicable rules PASS or NA. Verdict: PASS ollama-cloud/deepseek-v4.1-flash — action-review. |
Summary
charly task sync— and thegit-submodulesverb under it — reported what it visited, not what it changed, and hid the reason whenever it failed. Reproduced on the live umbrella againstopencharly/opencharly#477.Four measured symptoms, all on this change's own path:
case "bump"(verbs.go:292) has two phases: phase 1 rolls every submodule not named inpin_mapintodone, phase 2 appends thepin_mapentries that pinned into the same slice, and the summary printslen(done)as "pins bumped". The arithmetic is exact — 428 submodules − 6 uninitialized = 422, and the read-onlyverifytask prints those same two numbers as "422 checkout(s) clean at their gitlink; 6 uninitialize[d]".StatusFailbranch appends"; %d FAILED: %s"after 422 names. The runner truncates the message, so the clause that explains the failure never arrives — although the verb's own comment promises "every failure is reported together at the end, so the run's own output names all of them." An ordering that makes that promise unkeepable.rc=1with no visible reason.origin/<branch>, which leftsdkat884e2d8and the umbrella dirty until restored by hand.How tested
(That is the run's own final line, with the ref expanded to
main. An earlier version of this body quoted thescript's template,
...against charly $CHARLY_REF, which reads as captured output and is not.)The gate is the proof this repo asks for — its own
AGENTS.md: "a change to the verbs is proven by runningscripts/gate-task.shlocally" — and it does exercise the changed verb:gate-task.shdrivesgit-submodulesat four call sites (lines 210, 218, 266, 287), in bothmode: verifyandmode: bump, plus a nested-checkout and an absent-target variant.Both new tests have the coverage property, proven against a commit rather than a working-tree state:
The pre-fix ref is named ABSOLUTELY (
origin/main,5455850) rather than asHEAD~1, and that is a correctionof this body: an earlier version of this section quoted
HEAD~1, which was the pre-fix base only while the branchheld a single commit — the second commit made
HEAD~1the fix commit itself, so the pasted proof silently stoppedbeing true. Evidence naming a relative position in history is valid exactly until history moves.
and the pre-fix message the order assertion rejects, printed verbatim:
Both tests drive
runMaintenanceVerbIn— the function the gate itself uses — rather than a helper beside the call site, because this repo's ownnested_checkout_test.gorecords that mistake: "a test driving a helper beside the call site instead would pass after such a regression: measured, that is exactly how the first version of this test was wrong."Rulebook compliance
R1 — every anomaly RCA'd before remediation; the mechanism was confirmed line by line from
case "bump"and corroborated by a committed test's fixture message ("exit=2, want 0 (stderr: index.lock exists)"— the contended checkout the verb's comment names). Four defects, three fixed here; the fourth is filed (below).R2 — no duplication introduced: one overloaded slice is split rather than a parallel code path added.
R3 — no workaround: no retry, no sleep, no suppression. The defect was diagnosed by reproducing it, not by re-running until it passed.
R5 — the old wording is dead. Command and output, not a claim:
Three mentions org-wide: two are the definitions this series changes, one is a comment. No caller, no assertion, no parser. The grep ran against the umbrella's
plugin-taskcheckout, which is deliberately at the pre-fix commit — so it searched the old wording, which is the version that question is about.R7 — coverage fails without the change (both tests, shown above) and the changed path executes live under the gate.
R9/R10 — the gate builds a fresh charly with this candy compiled in; it is the assembled instrument this repo defines for these verbs.
domain-neutrality — the repository's own rule: "never bake an org, repo, or distro name into the plugin." The new strings name no org, repo, or distro.
R0 —
N/A — no owning skill exists for this verb.This repository's ownAGENTS.mdrecords the gap: "There is no dedicated/charly-*:taskowning skill — this repo's candy carries noskill:entity. The gap is recorded againstopencharly/opencharly#291." The skills this repo does select were loaded before the work:/charly-internals:plugin(plugin authoring, the provider model, the per-plugin CUE contract) and/charly-internals:git-workflow(before any git/PR action)./charly-image:layergoverns entity andplan:edits; this diff touches neither — it changes Go source and adds Go tests.R6 — the
git checkoutoperations quoted in this body are working-tree restores to a named commit, run to prove the coverage property and reversed immediately.git checkout origin/main -- candy/plugin-task/verbs.goputs the file at its pre-fix content so the tests can be shown to fail;git checkout HEAD -- candy/plugin-task/verbs.gorestores it, and the restore is verified in the transcript:after restore, old wording: 0. No destructive reset, no force-push, no history rewrite, no hook bypass, and no direct push tomain— the branch is afeat/branch and the landing is PR-only.HC (Hard Cutover) — one coherent scope in two commits:
db7fb74is the change (the count split and the failure report moved first), and341e5aeis the round-1 coverage that this PR's own review required. One R10 story:scripts/gate-task.shproves both, and it drivesgit-submodulesinmode: bumpamong the four call sites it exercises. No deferred work inside the scope, no shim, no follow-up phase — the one thing left undone (the checkout side effect) is a different behaviour question, filed as its own row (#26) rather than a deferred part of this one.Change classification
Runtime/behavioural, message-only, plus test coverage. The same submodules roll, the same pins pin, the same statuses return; what changes is what the summary says and in which order. No schema, no entity, no
plan:, no params field —schema/task.cueand the generated params are untouched, which is why they are absent from the diff.Filed issues
opencharly/opencharly#477— the row this fixes; its RCA is posted there in full (reproduction, the call chainsync→pins→git-submodules, the line-by-line mechanism, four defects tabulated).opencharly/plugin-task#26— the checkout side effect (defect 4), filed as its own tracker in this repo. A task described as a preview that "does not commit" still rolls worktree checkouts toorigin/<branch>, leaving the umbrella dirty. Stated there with both options and their costs and neither chosen, because "restore the checkouts" has a real hazard: a checkout already ahead of its gitlink for an unrelated reason must not be reset by a preview. This row survives this PR's merge, which is why the deferral now names it rather than#477.Agent:
dsh-opencharlyAssisted-by: dsh deepseek-v4.1-flash (fully tested and validated)