Repository navigation
chore: pre-push fmt hook to stop fmt-fail CI recurrence - #503
Conversation
Three fmt-fail CI incidents in the last week motivated a local gate. Catching fmt drift at push time saves a CI round-trip and avoids mid-review doc rewrites blocking on a one-char formatting diff. - .githooks/pre-push runs 'cargo fmt --all --check'; fails fast with a clear instruction to run 'cargo fmt --all'. - scripts/install-hooks.sh one-shot: 'git config core.hooksPath .githooks' + chmod. Contributors opt in explicitly — matches the repo's no-silent-magic convention. - CLAUDE.md updated with the setup command and a fmt check line under Key Commands.
ChatGPT ReviewPrinciple audit. FAIL-CLOSED. Satisfied. This is the only principle the diff really exercises, and it does the right thing: ILLEGAL STATES UNREPRESENTABLE. Satisfied for scope. The PR does not introduce or widen any compiler data model, so there is no new ambiguous FACTS FLOW FORWARD. Satisfied. No parse/lower/infer/emit boundary is touched here. The only new fact is “this repo wants a local fmt gate,” and it is carried consistently through the installer, the hook, and the contributor doc update rather than being computed in one place and dropped in another. chatgpt-review-ed028e0f-8ddc-48… COPROD DISSOLUTION. Satisfied / not engaged. No new Rust enum or substrate coproduct appears in the diff, so the dissolution test does not really come into play. This is exactly the kind of implementation-local change the coproduct rule is not meant to over-police. chatgpt-review-ed028e0f-8ddc-48… SINGLE AUTHORITY. Basically satisfied. chatgpt-review-d0ca1204-da31-40… API-LEVEL ENFORCEMENT. Satisfied for the chosen scope. This does not make formatting impossible to forget globally, because hook installation is still opt-in, but once a clone enables repo hooks the enforcement becomes automatic at push time rather than “please remember to run fmt.” For workflow tooling, that is forward progress, not debt. chatgpt-review-ed028e0f-8ddc-48… Design question. Should repo-local quality gates eventually come from one canonical entrypoint instead of re-embedding What is at stake is the thesis’s cost-of-change test: today this is small and harmless, but if fmt/lint/test/ratchet gates each get mirrored across hooks, docs, and CI by hand, the repo will slowly accumulate parallel representations of the same process rule. I would not block this PR on that; I just think this is the structural direction to watch. chatgpt-review-d0ca1204-da31-40… Verdict. APPROVE. This looks clean. It is a small implementation-local ergonomics change that moves a repeated CI-only failure earlier into the local workflow without introducing a new substrate shape, scaffold, or bridge. LOOP HEALTH: converging — this round turns a recurring fmt-only CI failure into an earlier local gate and does so without shifting debt elsewhere. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · bad97acb
✅ Review (blocking: 0, non-blocking: 1+/0-)
Non-blocking — Strengths
.githooks/pre-pushThe hook stays explicitly opt-in and fails closed with a concrete remediation, which fits the repo’s preference for loud, documented workflow checks rather than letting formatting drift surface only in CI.
ROADMAP — Incomplete
- M5 Meta-process modeling: ROADMAP.md still treats full dev-process modeling as future work, so this lands as a bootstrap shim rather than the final
dag run-centered end state.
✅ I don’t see any blocking issues in the changed lines; the change is small, explicit, and consistent with the project’s current bootstrap stage.
|
✅ Review (blocking: 0, non-blocking: 1+/0-) Non-blocking — Strengths
ROADMAP — Incomplete
✅ I don’t see any blocking issues in the changed lines; the change is small, explicit, and consistent with the project’s current bootstrap stage. |
Changed from 'fail, tell developer to run cargo fmt' to 'run cargo fmt and commit as chore' based on review feedback. Rationale: automation is the point of the hook; failing and asking the developer to do two more steps (fmt + re-push) defeats it. Behavior: - Clean tree required at push time — bails if uncommitted changes exist (don't silently sweep unfinished work into a fmt commit). - If fmt --check passes: exit 0, push continues. - If drift: run cargo fmt --all, stage tracked files, commit as 'chore: apply cargo fmt'. Push continues with the new commit. The chore commit lands cleanly on top of developer intent — original commits unchanged. Audit trail preserved.
Hand-written shell scripts (.githooks/pre-push from PRs #503/#509, scripts/install-hooks.sh, scripts/check-stage0-freshness.sh, scripts/regenerate-stage0.sh) bypass the compiler's dependency- analysis machinery. Per the compiler-as-dependency-analyzer thesis (tonight's framing), they should be .dag programs composing existing service operations from extdeps/ and emitted as standalone shell scripts at build time. Recon done on existing substrate: - dsl/extdeps/git.dag: service git.Core with 7 operations + shell transport + mock_response pattern. 181 lines. - dsl/extdeps/cargo.dag: Build, Test, Clippy, Doc, Run. MISSING Fmt. - dsl/extdeps/shell.dag: POSIX Find, Env. Adequate. - dsl/extdeps/github/: auth, pulls, gists, actions (not on critical path for pre-push hook). Separable prerequisite deferrals named: - cargo.dag Fmt operations (S) — mechanical extension - Shell-emission target (M, needs design) — v3 emits Rust/Go/Python today; shell is used as transport but not as emission target. Needs a DB clarifying what "emit to shell" means structurally (likely E-9-pattern with output as standalone executable text). - "Hook-as-program" pattern (M, needs design) — how a .dag program declares its invocation contract (stdin format, env, exit codes). - Test coverage via DB-15 R2's MockBackedInvariant predicate. Yellow-flag threshold: triggers actively when a *second* hand- written workflow script needs the same modeling. Until then, the hand-written pre-push hook (PRs #503 + #509) is tolerated as the one instance. Added under Active Deferrals §"Cross-cutting — workflow scripts modeled in .dag". When PR #507 (Scheduled Deletions) merges, the hand-written .githooks/pre-push also gets a row there with trigger "emitted pre-push hook replaces it."
… as first use case) (#510) * docs: ROADMAP — track the commit-pipeline-in-dag modeling work Hand-written shell scripts (.githooks/pre-push from PRs #503/#509, scripts/install-hooks.sh, scripts/check-stage0-freshness.sh, scripts/regenerate-stage0.sh) bypass the compiler's dependency- analysis machinery. Per the compiler-as-dependency-analyzer thesis (tonight's framing), they should be .dag programs composing existing service operations from extdeps/ and emitted as standalone shell scripts at build time. Recon done on existing substrate: - dsl/extdeps/git.dag: service git.Core with 7 operations + shell transport + mock_response pattern. 181 lines. - dsl/extdeps/cargo.dag: Build, Test, Clippy, Doc, Run. MISSING Fmt. - dsl/extdeps/shell.dag: POSIX Find, Env. Adequate. - dsl/extdeps/github/: auth, pulls, gists, actions (not on critical path for pre-push hook). Separable prerequisite deferrals named: - cargo.dag Fmt operations (S) — mechanical extension - Shell-emission target (M, needs design) — v3 emits Rust/Go/Python today; shell is used as transport but not as emission target. Needs a DB clarifying what "emit to shell" means structurally (likely E-9-pattern with output as standalone executable text). - "Hook-as-program" pattern (M, needs design) — how a .dag program declares its invocation contract (stdin format, env, exit codes). - Test coverage via DB-15 R2's MockBackedInvariant predicate. Yellow-flag threshold: triggers actively when a *second* hand- written workflow script needs the same modeling. Until then, the hand-written pre-push hook (PRs #503 + #509) is tolerated as the one instance. Added under Active Deferrals §"Cross-cutting — workflow scripts modeled in .dag". When PR #507 (Scheduled Deletions) merges, the hand-written .githooks/pre-push also gets a row there with trigger "emitted pre-push hook replaces it." * docs: commit-pipeline deferral — address codex BLOCKING + chatgpt non-blockings Three reviewer concerns addressed: 1. BLOCKING (codex): yellow-flag threshold understated. Four hand- written scripts already exist, so the deferral is active now — not 'tolerated until a second instance.' Reframed: marked ACTIVE; THESIS.md's meta-process claim is the justifying authority, not an arbitrary 'second script' trigger. 2. NON-BLOCKING (codex): 'First concrete use case' over-described the pre-push hook. At PR #509 HEAD the hook fmt-checks / fmt-fixes / commits; the stdin/delete/HEAD contract is implementation detail that belongs in the .dag design work, not the ROADMAP entry. Trimmed. 3. NON-BLOCKING (chatgpt facts-flow-forward): Track 15 (CLI tool modeling — bare command names are hidden PATH dependencies) wasn't threaded into the prerequisite list. Added as a prerequisite deferral referencing ROADMAP.md:810-826 directly. 4. SINGLE-AUTHORITY pressure (chatgpt): Shape A vs Shape B was an implicit lean toward Shape A ('shell-emission target'). Per Track 16 (ROADMAP.md:920-935), the thesis puts shell scripts as Shape B — .dag programs build them via concat/fold/match, parallel to Track 16's YAML emission. Updated entry to make Shape B explicit, remove the 'shell-emission target' framing, and align with tools/ratchet.dag's grep-command generation as precedent. Prerequisite list rewritten: - cargo.dag Fmt operations (S, mechanical) - Track 15 tool resolution (already-tracked prerequisite) - Shape B emission via existing Track 16 pattern (no new compiler concept) - Hook invocation contract as structural declaration (small type in a .dag program) - Test coverage via DB-15 R2 MockBackedInvariant Scope for the other three scripts (install-hooks.sh, check-stage0-freshness.sh, regenerate-stage0.sh) noted: same Shape B pattern, dissolves individually once pre-push proves it.
1 participant
Summary
Three fmt-fail CI incidents in the last week (#491, #496, earlier). Each costs a CI round-trip and, mid-review, a small doc-or-code rewrite turns into a formatting correction. Fixable with a local pre-push gate.
Not in scope
Test plan
🤖 Generated with Claude Code