fix(doctor): the commit path becomes a checked precondition - #905
Draft
wenzowski wants to merge 4 commits into
Draft
fix(doctor): the commit path becomes a checked precondition#905wenzowski wants to merge 4 commits into
wenzowski wants to merge 4 commits into
Conversation
`batten doctor` reported six checks and none of them asked whether a commit in this clone runs the gate. In the container this repository provisions for itself, those came apart: `git commit` ran neither `pre-commit` nor `commit-msg`, so every commit bypassed the gate while `batten startup` reported every row green. The session-start advisory's "every declared repair has already run this session; what is listed is what it did not fix" was TRUE and useless — the hooks were never in the declared set at all. `mise-tasks/doctor.sh` did see it and emitted two `::error::` lines, which is the CLOUD-1454 shape one layer up: a reporter is not a gate. Worse, its remedy named `.claude/hooks/session-start.sh`, a program 7d18858 deleted, so the refusal was right and its instruction could not be followed — an agent reading it top to bottom gets `No such file or directory` and has to re-read the sentence to find the half that works. Both remedies now name `mise run session:git-hooks`, and a case asserts the named task and hook body resolve in the tree, because prose cannot hold that and a case over the tracked file can. `diagnose_commit_gate` is the predicate, and it is one predicate with two callers: the row the bare report pushes, and the `doctor commit-gate` sub-verb. A second implementation of it would be the defect this change repairs, one layer along — `doctor.sh` and the committed authority disagreeing about what an installed gate is. THE SUB-VERB IS NOT A DUPLICATE OF THE ROW, and `the_bare_diagnosis_is_unchanged_by_the_sub_verb` is where that has to be argued. The axis that case defends is `Mediator`'s: bare `doctor` answers a property of the COMMIT, a sub-verb answers a property of the WORLD. `doctor mediator` is excluded because install recency is a container fact that would make a commit gate answer on it. Whether THIS clone's commit path runs the gate is neither — it is a property of the checkout, byte-stable across machines, the same class as `git-repo`. The sub-verb exists for an unrelated reason: a `[[startup]]` row decides on an exit status and cannot select one line out of a report, so it needs a command answering this question alone. THE COMMON DIR, NEVER THE PER-WORKTREE ONE. `git::git_dir` is per-worktree and is right for receipts and HEAD; hooks are not per-worktree, and git resolves `hooks/<name>` against the common dir — so a linked worktree checked the other way would report the gate missing while every commit in it runs the gate correctly. `core.hooksPath` outranks both, resolved across every scope exactly as git resolves it: a repository that redirects its hooks has hooks, and a probe ignoring the key would send its owner to install a second copy somewhere git never reads. STATS, NEVER EXECUTES, and follows the symlink deliberately. Running the hook to see whether it works is what `doctor.sh` does behind a probe variable; reaching user-supplied code from a `read` verb on the derived allowlist is CLOUD-170's actual invariant. Following the link is required rather than incidental — the installer makes these symlinks into the tree precisely so the checked-in body stays the one authority, and a check refusing to follow one would fail the shape it certifies. The executable bit is asked because it is what git itself asks: a present, non-executable hook is one git skips silently, which a file-existence probe reports as healthy. Pointer-only. The subjects are the hook NAMES — git's own vocabulary — and never the directory they were looked for in: that path is absolute and per-machine, which would defeat byte-stability and put the layout of somebody's disk in a diagnostic that promises not to carry one. Could-not-look passes, this module's posture: a directory whose hooks path cannot be resolved is one `git-repo` has already failed on, and double-counting it would redden a checkout for a read that failed elsewhere. Refs: CLOUD-1398
|
Warning Review limit reachedNext included review available in 6 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The engine-side check landed in the previous commit and nothing asked it. This is the half that makes it a gate rather than a reporter — `batten startup` now carries a sixth row, and `--repair` installs the hooks a fresh clone is missing. Measured on this container before the row existed: `batten startup` reported five rows green while `git commit` ran neither `pre-commit` nor `commit-msg`. The session-start advisory's "every declared repair has already run this session; what is listed is what it did not fix" was true and useless, because the hooks were never in the declared set at all. NOT `hk install`, and that is the row's §8 answered by measurement rather than preference. On this container `hk` resolves only through the pin — `mise exec -- hk --version` answers 1.56.1 while doctor's bare-PATH probe reports `program-not-on-path hk` and the pin record is absent. `hk install` generates a hook whose body calls `hk` BARE, so the hook it installs makes every `git commit` fail with `hk: not found`: a repair that reads as installed and breaks the thing it installed. `session:git-hooks` is the symlink-based form that works and is already the session-start step, so this row adds an ASSERTION rather than a second installer. THE SUB-VERB RATHER THAN BARE `doctor`, which is `host-dependencies-present`'s trap approached from the other side. A row decides on an exit status and cannot select one line out of a report, so `check = ["batten", "doctor"]` would fail here whenever any unrelated declared program was unreachable — the state this very container is in, on the `hk` reading above — and would then fire a git-hook repair that cannot fix that, reporting `repair-failed` forever over a gate that is installed. The repair writes under `$GIT_DIR/hooks`, outside the worktree and so outside `protected`; the `repair` key in the committed authority is the authorisation to run it. Verified on this clone with the new binary installed: `batten doctor commit-gate` reports `commit-gate ok` and `batten startup` reports six rows, none failed. On a clone with no hooks the sub-verb reports `commit-gate failed commit-hook-missing commit-msg pre-commit` at exit 1 and the row reports `failed not-provisioned`. Refs: CLOUD-1398 Admits: 520e162f8cc4f9bc5202f66af1d7b60ebdbcd5ded6055a13c8d59ed8c7732584 Admits-rule: protected-mutation Admits-verdict: path write refused Admits-subject: batten.toml Admits-anchor: call:12a12ca9677cc60befac3409f65d4b6ddb44e448 Admits-epoch: 34a3ca72f9f6da3b52df73c607444383dade972800f4d78cdf198bbede317b2e Admits-author: alec@wenzowski.com Admits-prev: 05ef30b5f6748b6aaf3d352dbe1c58268a1577c5b58e6abf898c802635b84d6f Admits-answer-lost: CLOUD-1398 cannot be implemented at all. Its Ready block's §1 names the `[[startup]]` table in batten.toml as the authority for the declared precondition, and the engine-side `doctor commit-gate` check without a row asking it is precisely the reporter-that-is-not-a-gate defect the row was filed to close (the CLOUD-1454 class). The concrete cost is that `git commit` in a fresh clone keeps bypassing pre-commit and commit-msg while `batten startup` reports every row green, which is the measured state of this container. Admits-answer-precondition: The class names `mise run config-lint` and `batten config` as the surface, and neither can ADD a row: batten has no verb that writes its own committed authority, deliberately, so a `[[startup]]` row can only arrive as a direct edit to batten.toml. The write is one a reviewer sees in the diff it lands in — it is on branch claude/cloud-1398-doctor-commit-gate, off origin/main, and lands through a draft PR that `land` readies only after `verify` (which runs config-lint) is green. Admits-answer-rejected-route: Both. `config read first` is not a route to this outcome: I did read the config first — the five existing rows, the `protected` list and the `[[redirect]]` table are what this change is written against — but reading cannot add a row, so it is a precondition I satisfied rather than an alternative I could take instead. `patch run first` does not apply either: it addresses changing an EXISTING declaration, and this is an addition of a sixth row that no patch anchor exists for. Neither route weakens anything: this edit only ADDS a gate, it removes and loosens nothing, which is the opposite direction from the maximal-weakening case the protected list exists for.
Three corrections to the previous two commits, each made by a gate rather than by argument. The engine-side check and its `[[startup]]` row stand; where the check is ASKED changed, the verb was renamed, and the shell task is left alone. THE ROW CAME OUT OF THE BARE REPORT, and the suite is what said so. `container-health` renders `diagnose` at session start, so a `commit-gate` row there made every checkout with no git hooks announce itself as unhealthy — measured, it reddened `contract_drift::a_session_seeded_at_session_start_is_silent_and_stays_silent` over a fixture that has no hooks and wants none. The argument for putting it there was that a clone's commit path is a property of the CHECKOUT rather than of the world, byte-stable across machines, the same class as `git-repo`. That is right about the predicate and wrong about the report: batten requires git hooks of nobody, so WHETHER a commit path should run a gate is the consumer's judgement, and minting it in `crates/batten` is non-negotiable rule 1's violation. The predicate stays in the engine where a caller asks for it; the judgement lives in this repository's own `[[startup]]` row. `doctor gate` RATHER THAN `doctor commit-gate`, on a constraint measured rather than reasoned. A man page is committed as the hyphen-joined command path, and `surface.rs`'s suite maps that filename back by replacing EVERY hyphen — so `batten-doctor-commit-gate.1` reads back as the command `doctor commit gate`, renders nothing, and takes three cases down at once. No verb on this surface has ever carried an internal hyphen; the reason is now written at the declaration and beside the row, so the next author does not rediscover it. THE SHELL TASK IS LEFT ALONE, and CLOUD-1398's own body is wrong about why it could be edited. It claims the remedy-string fix "is exactly the class `only_drops_a_retired_reference` and `drops_a_retired_name` already admit". Measured, that arm requires every removed line to name a path THIS SAME DELTA deleted, and this delta deletes nothing — `.claude/hooks/session-start.sh` went in 7d18858. So `shell-rule-retired` refuses the edit and the two landable shapes are retire it whole or leave it alone. Both governed files are reverted, and the case that asserted the remedy is re-aimed at the half this change owns: the `[[startup]]` row's own `repair` argv, which must name a task the manifest declares and a hook body present in the tree. The stale `::error::` string survives; it needs a retirement, which is not this row's shape. Three obligations a new verb owes here, each found by its own gate: the derived read-only allowlist and the emitted row set (both sorted, both committed), a declared pointer-only disposition, and the generated man page and completions. AND ONE DEFECT IN THE NEW CASE ITSELF. Its fixture row spawned bare `batten`, so `startup` resolved it on PATH and the case graded the container's INSTALL currency rather than this tree — it passed while the installed copy happened to carry the verb and went red the moment the verb was renamed here. Pinned to `CARGO_BIN_EXE_batten`. That is CLOUD-1650's subject arriving inside this suite, which is worth recording rather than quietly fixing. Refs: CLOUD-1398 Admits: 075493a65c80d64b2e6be85464946f2de0323a507426be70bf3f45427fd885ef Admits-rule: protected-mutation Admits-verdict: path write refused Admits-subject: batten.toml Admits-anchor: call:30f7730dec1c3a06f1caf172617dcdba08c3a24c Admits-epoch: 4326c29555225ffac55fc1d3db17442d15d8697c4d6659204ee9c5553a08d068 Admits-author: alec@wenzowski.com Admits-prev: 520e162f8cc4f9bc5202f66af1d7b60ebdbcd5ded6055a13c8d59ed8c7732584 Admits-answer-lost: CLOUD-1398 cannot be implemented at all. Its Ready block's §1 names the `[[startup]]` table in batten.toml as the authority for the declared precondition, and the engine-side `doctor commit-gate` check without a row asking it is precisely the reporter-that-is-not-a-gate defect the row was filed to close (the CLOUD-1454 class). The concrete cost is that `git commit` in a fresh clone keeps bypassing pre-commit and commit-msg while `batten startup` reports every row green, which is the measured state of this container. Admits-answer-precondition: The class names `mise run config-lint` and `batten config` as the surface, and neither can ADD a row: batten has no verb that writes its own committed authority, deliberately, so a `[[startup]]` row can only arrive as a direct edit to batten.toml. The write is one a reviewer sees in the diff it lands in — it is on branch claude/cloud-1398-doctor-commit-gate, off origin/main, and lands through a draft PR that `land` readies only after `verify` (which runs config-lint) is green. Admits-answer-rejected-route: Both. `config read first` is not a route to this outcome: I did read the config first — the five existing rows, the `protected` list and the `[[redirect]]` table are what this change is written against — but reading cannot add a row, so it is a precondition I satisfied rather than an alternative I could take instead. `patch run first` does not apply either: it addresses changing an EXISTING declaration, and this is an addition of a sixth row that no patch anchor exists for. Neither route weakens anything: this edit only ADDS a gate, it removes and loosens nothing, which is the opposite direction from the maximal-weakening case the protected list exists for.
The previous commit claimed to revert `mise-tasks/doctor.sh` and did not. `git checkout -- <path>` restores from HEAD, and HEAD already carried the edit, so the revert restored the edited bytes and the PR kept a change `shell-retirement` refuses. Reverted against `origin/main` this time, which is the comparison that was actually meant. The finding channel did not catch it either: `shell-rule-retired` read 0 while the file still differed from the base. What surfaced it was a review bot listing the file among the PR diff, which is worth recording — the store lagged the tree, and the tree wins. So the stale `::error::` remedy naming `.claude/hooks/session-start.sh` survives on main, and `tests/doctor.bats` still asserts it, which keeps the two consistent. Fixing it needs a retirement rather than an edit; the reason is on CLOUD-1398. Refs: CLOUD-1398
|
❌ The last analysis has failed. |
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 CLOUD-1398.
What was wrong
batten doctorreported six checks and none of them asked whether a commit inthis clone runs the gate. In the container this repository provisions for itself
those came apart:
git commitran neitherpre-commitnorcommit-msg, soevery commit bypassed the gate while
batten startupreported every row green.The session-start advisory's "every declared repair has already run this
session; what is listed is what it did not fix" was true and useless — the
hooks were never in the declared set at all.
mise-tasks/doctor.shdid see it and emitted two::error::lines, which is theCLOUD-1454 shape one layer up: a reporter is not a gate. Worse, its remedy named
.claude/hooks/session-start.sh, a program7d188580deleted — the refusal wasright and its instruction could not be followed.
What this changes
doctor.rs—diagnose_commit_gate, one predicate with two callers: thecommit-gaterow in the bare report, and the newdoctor commit-gatesub-verb. Resolves the hooks directory through the common dir (hooks are
not per-worktree) and honours
core.hooksPathacross every scope, as git does.Stats and follows symlinks; never executes.
cli.rs/surface.rs/lib.rs— the sub-verb,read, oneCommandDecl.batten.toml— the sixth[[startup]]row,commit-gate-installed, withrepair = ["mise", "run", "session:git-hooks"].mise-tasks/doctor.sh— both remedy strings and one stale commentrepointed onto
mise run session:git-hooks. A retired-reference drop, which isthe one edit shape
shell-retirementadmits by name.tests/it/startup.rs— four cases, including the fail →ok repaired→okarm and an assertion that the remedy names a task and a hook body thatresolve in the tree.
Two decisions worth review
The sub-verb is not a duplicate of the row.
doctor mediatoranddoctor egresssit outside the bare report because they answer properties of theworld; whether this clone's commit path runs the gate is a property of the
checkout — byte-stable across machines, the same class as
git-repo— so it isreported where a reader already looks. It exists as a verb as well because a
[[startup]]row decides on an exit status and cannot select one line out of areport:
check = ["batten", "doctor"]would fail whenever any unrelated declaredprogram was unreachable (the state this container is in, per §8 below) and then
fire a git-hook repair that cannot fix that —
repair-failed, forever, over agate that is installed.
Not
hk install. §8 of the row blocked on whetherhkresolves here.Measured:
hkis installed at 1.56.1 and reachable only through the mise pin— not on bare
PATH, with the pin record under$GIT_DIRabsent. So neithertoolchain-is-provisionednordoctor.shis lying; the missing object is therecord, not the tool.
hk installgenerates a hook whose body callshkbare,so the hook it installs makes every
git commitfail withhk: not found— arepair that reads as installed and breaks the thing it installed. Recorded as a
comment on CLOUD-1398 and on CLOUD-476, whose
hk installhalf depends on thesame answer.
Verification
Measured on this clone with the new binary installed, hooks removed and restored:
batten doctor commit-gate, hooks removedcommit-gate failed commit-hook-missing commit-msg pre-commit, exit 1batten startup, hooks removedcommit-gate-installed failed not-provisionedbatten startup --repaircommit-gate-installed ok repairedbatten startupagaincommit-gate-installed ok— no repair.claude/hooks/git-hook.shcommit-gate ok;startup: 6 row(s), 0 failedThe third arm is the one that matters:
okwithoutrepairedis whatseparates a repair that worked from one that exits zero having fixed nothing.
mise run verifygreen and rebased onorigin/mainbefore ready;batten mutatereddens
hooks-check-reports-without-deciding.Note for review
The
batten.tomlcommit carriesAdmits-*trailers.batten.tomlis aprotectedpath, so the write went through the repository's declared overrideroute — preconditions answered on the record, admission anchored to the parent
commit — and
batten commit checkrefused the commit until those trailers wereattached. The audit record is in the history deliberately.