Skip to content

chore(devex): install a make check pre-push hook via make setup (backend#1606) - #657

Merged
LukasWodka merged 5 commits into
developfrom
chore/1606-pre-push-hook
Aug 11, 2026
Merged

chore(devex): install a make check pre-push hook via make setup (backend#1606)#657
LukasWodka merged 5 commits into
developfrom
chore/1606-pre-push-hook

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

What

Step 4 of #1606, fanned out to client. make setup now installs a git pre-push hook that runs make check; factored into a standalone install-hooks target. Mirrors the merged reference e2e-test-agent#46.

Design (honest by construction)

  • Catches forgetting, not defiance: git push --no-verify always skips it.
  • Won't clobber a foreign pre-push hook — it declines and says so.
  • Worktree/submodule-safe via git rev-parse --git-path hooks.

Test plan

Mirrors #46 (verified end-to-end there). Here: make -n install-hooks and make help parse cleanly.

Parent epic: #1680.


Note

Low Risk
Makefile-only developer workflow change; no runtime, auth, or deployment behavior is affected.

Overview
make setup now installs a git pre-push hook that runs make check, so fast lint/drift/helm checks run before pushes without relying on contributor memory.

A new install-hooks target writes the hook (also invoked from setup). The hook is worktree/submodule-safe via git rev-parse --git-path hooks, refuses to overwrite an existing non-tracebloc pre-push hook, and unsets GIT_* env vars in the hook so nested git (e.g. Go buildvcs) does not fail in linked worktrees. help and header comments document install-hooks and git push --no-verify as the escape hatch.

Reviewed by Cursor Bugbot for commit ce2dbbd. Bugbot is set up for automated code reviews on this repo. Configure here.

…end#1606)
Step 4 of backend#1606, fanned out to client. `make setup` now installs a
git pre-push hook that runs `make check`, factored into a standalone,
independently runnable `install-hooks` target. Mirrors the merged reference
e2e-test-agent#46.
Honest by construction: the hook catches forgetting, not defiance
(`git push --no-verify` always skips it); it declines to clobber a foreign
pre-push hook rather than stomping it; and it resolves the hooks dir via
`git rev-parse --git-path hooks` so it is worktree/submodule-safe.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Aug 10, 2026
LukasWodkaand others added 3 commits August 10, 2026 21:10
…(backend#1606)
The copied block said 'need not rebuild the (minutes-long) venv', which is false for cli (Go) and client (tool-check only) and loose elsewhere. Generalise to 'need not rerun the full make setup' — true in every repo.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…urvives worktrees (backend#1606)
Git exports GIT_DIR/GIT_WORK_TREE/etc into hook processes; a nested git (e.g. Go buildvcs under go test) then fails with 'exit status 128' in a linked worktree. The generated hook now unsets those before exec make check. Uniform with the fix on cli#482 (the Go repo where it actually bites); harmless in normal clones.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eport false success (backend#1606)
Bugbot (Low) on cli#482: the install steps were ';'-chained, so a failed printf or chmod still exited 0 and printed 'installed'. Git skips a non-executable hook, so make setup could look green while no hook runs. Chain with '&&' so any failure fails the recipe. Uniform across the fleet.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@aptraceblocaptracebloc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LukasWodkaApproved. Verified the hook behavior end-to-end on GNU Make 3.81 / macOS (the portability floor).

  • make install-hooks writes an executable .git/hooks/pre-push running make check; make setup wires it in.
  • Clobber-safe: with a foreign pre-push hook present, the target declines and leaves it byte-for-byte untouched (marker-based detection); idempotent over its own hook.
  • Degrades gracefully outside a checkout (exit 0); --no-verify escape hatch documented in the hook, setup, help, and header.
  • Failure propagates: forced an unwritable hooks dir → make Error, no false "installed" (the earlier ;&& Bugbot fix, confirmed working). No unresolved Bugbot findings; root Makefile isn't code-owned.

Two Low nits only (marker substring-match could false-positive a hook literally containing the marker text; hook assumes make on PATH at push time) — neither worth changing.

🤖 Generated with Claude Code

LukasWodka added a commit that referenced this pull request Aug 11, 2026
… PR trigger (backend#1681) (#665)
`Source-of-truth drift` is a REQUIRED status check on develop and on main, but
`drift-checks.yaml` only fires on PRs touching scripts/, client/ or itself. A PR
outside those paths therefore never produces the check, GitHub leaves it at
"Expected - waiting for status to be reported", and the PR is unmergeable with no
failing check to point at.
Measured on 2026-08-11 this was blocking three open PRs at once - client#651
(org-standards sync), #657 (Makefile) and #660 (docs) - none of which touch the
filtered paths. Two of the three had already been approved.
`pull_request` loses its paths filter so the required check always reports. The
job is ~10s (measured across its last three runs), which is nothing next to a
permanently stuck PR. `push` keeps its filter: pushes are not gated by required
checks, so path-scoping there is free and correct.
The repo already documents this hazard in standard-checks.yml's header; this is
the same rule applied to the workflow that broke it.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka
LukasWodka merged commit 41fad41 into developAug 11, 2026
12 checks passed
@LukasWodka
LukasWodka deleted the chore/1606-pre-push-hook branch August 11, 2026 11:07
LukasWodka added a commit that referenced this pull request Aug 14, 2026
… (backend#1729) (#715)
The guard added in #706 parses all four CLIENT_ENV declarations and compares
them. It ran in helm-ci.yaml's `Helm lint` job. `Helm lint` is not a required
status check on this repo -- the required contexts on develop are `Unit tests`,
`Lint`, `quality/*`, `version-bump-gate/version-check` and `Source-of-truth
drift` -- so a PR that made the four declarations disagree was mergeable with
the guard red. It could advise; it could not block.
That is the shape backend#1729 is about, in the epic's own flagship deliverable:
a mechanism that appears to verify something without being connected to the
outcome it claims to control.
`Source-of-truth drift` is the correct home and needed no new configuration:
- it is REQUIRED on develop and on main, so a disagreement blocks the merge;
- it deliberately carries NO `paths:` on pull_request (its header records why
-- a path-filtered required check bricks PRs outside those paths, which is
what happened to #651/#657/#660 on 2026-08-11), so the guard now also runs
on the installer-only PRs helm-ci's filter would have skipped;
- it is already the home of the other duplicated-declaration guard
(check-drift.sh), and the guard needs only bash + python3, ~2s.
helm-ci loses the step and the two `paths:` entries that arrived with it in
#706. `scripts/install-k8s.ps1` genuinely leaves -- helm-ci is the repo's
heaviest workflow (a real k3d cluster plus two 4-platform matrices) and a
PowerShell installer edit has no other reason to start it. The explicit
`scripts/lib/common.sh` entry also goes, but that one was always redundant:
the pre-existing `scripts/lib/**` glob still matches it, so helm-ci's
triggering on a common.sh change is unchanged.
Mutation-proved rather than assumed: with `staging)` renamed in
scripts/lib/common.sh the guard exits 1; restored, it exits 0. The mutation
anchor was asserted to have applied first -- an inert mutation and good
coverage look identical in a log.
Verified green on this branch before arming: all four declarations agree and
all six spellings are exercised (dev 12, development 2, prod 11, production 4,
staging 9, stg 6).
Refs backend#1729
LukasWodka added a commit that referenced this pull request Aug 27, 2026
* ci(2651): split the chart test tier so it can be a required check
The 585-test helm-unittest tier (34 suites, ~4s) lived in helm-ci.yaml and was
required on no branch. All of it could be red and the PR still merged, which
made every assertion in it advisory — including the ones #2606 had just
repaired. backend#1729 rule 2: a guard in a non-required CI job is advice.
A NEW WORKFLOW RATHER THAN A PATHS EDIT, and the reason matters because the
ticket's scope assumed the edit.
`helm-ci.yaml` could not simply lose its `pull_request` paths filter. It is the
repo's heaviest workflow, and two jobs carry NO job-level `if:` — `upgrade-e2e`
(Fleet auto-upgrade E2E, k3d, timeout 30m) and `seal-check-e2e` (Seal-check
egress-enforcement, k3d, timeout 30m). They are gated ONLY by that filter, so
dropping it would have started ~60 minutes of k3d E2E on every PR in the repo,
docs-only ones included. Moving the cheap tier out costs nothing and leaves the
expensive jobs byte-identical: helm-ci.yaml keeps its filter and now holds
lint / template / ingestor-multiarch / the three e2e jobs.
`helm-unit.yaml` therefore has NO `paths:` on `pull_request` — a path-filtered
required check never creates its check run on a PR outside those paths, so
GitHub leaves it at "Expected - waiting for status to be reported" and the PR is
unmergeable forever. drift-checks.yaml's header records that blocking
client#651, #657 and #660 on 2026-08-11. `push` keeps a filter: pushes are not
gated by required checks, so scoping there is free. Same split drift-checks uses.
scripts/tests/helm-unittest-gated.sh keeps it that way, and runs inside the
REQUIRED `Source-of-truth drift` job — so the guard protecting the gate is
itself gated. It refuses a `pull_request` paths filter, pins the job name (the
name IS the status-check context, so a near-miss un-gates the tier exactly as
silently as the paths trap), refuses a second job producing the same context,
and refuses a gate that no longer runs `helm unittest ./client` — a required
check that executes nothing reports success forever.
WHAT THE GUARD DOES NOT DO, said in its header rather than implied: it does not
read the live branch-protection list, so it cannot prove the context is
currently required. That copy lives in GitHub's config, not this repo, and the
drift job has no token for it. The header carries the `gh api` one-liner to
check by hand. An earlier draft of helm-unit.yaml claimed the guard "asserts the
name against the live protection list" — false, and precisely the rule-7 defect
this ticket family is about, so it is corrected in the same change.
ARMED WHILE GREEN (rule 4): `helm unittest ./client` on develop @ 76f4690 is
34 suites / 585 tests / 4.1s, all passing. Note 585, not the 581 the ticket
measured — it has grown, which is the argument for gating it.
Mutations on the guard, each asserting its anchor applied:
G1 THE TRAP: paths filter on pull_request -> FAIL
G2 rename the job to a near-miss -> FAIL
G3 gate stops running the suite -> FAIL
G4 a second workflow produces the context -> FAIL
G5 workflow deleted -> FAIL
G6 workflow unparseable -> FAIL
G7 push loses its paths filter -> FAIL
G8 NEGATIVE CONTROL, reword a comment -> PASS
Makefile: the guard is registered in DRIFT_GUARDS, the repo's only declaration
of that set. Two claims there are now false and fixed in the same change — that
"`Helm unit tests` is required on neither" (it is the whole point of this PR),
and that `helm-unittest` maps to "helm-ci.yaml `unittest`" (it moved).
drift 24/24 green · helm-lint, helm-vocab, helm-template, check-style, shellcheck
all clean.
PROTECTION IS NOT CHANGED BY THIS PR. Adding the context to client/develop and
client/main is an admin action on a shared repo, so it needs an explicit
decision — the command and the measurements are in the PR body.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* ci(2651): the paths guard could not see paths-ignore (backend#2651)
The guard exists to stop a required check falling into the permanent-pending
trap, and it looked for `paths:` only. `paths-ignore:` is its sibling and skips
the workflow the same way -- a PR outside either filter never gets a check run,
so the required context sits at "Expected - waiting for status to be reported"
forever. A later `paths-ignore:` would have reopened exactly the trap this
guard exists to close, while the guard stayed green. Bugbot, #872.
The same blind spot was in the `push` arm with the opposite sign: it warned
"push lost its paths: filter" for a workflow filtered with `paths-ignore:` --
a false positive in the one place this guard is meant to be trusted.
Both keys are now named once, in PATH_FILTER_KEYS, so the check and its own
error message cannot disagree about which keys were examined. Two entries
rather than a `paths*` prefix rule: the set is closed and short, and a prefix
would also catch a future key that does something else.
Mutation-proved against the real workflow, each anchor asserted before the
run:
pull_request + paths-ignore: -> exit 1, message names `paths-ignore:`
pull_request + paths: -> exit 1, message names `paths:` (unchanged)
push paths -> paths-ignore -> exit 0, no false "lost its filter"
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LukasWodka@aptracebloc