feat: git-native commit guard — check mode, pre-commit hook, install walkthrough#1
feat: git-native commit guard — check mode, pre-commit hook, install walkthrough#1flo-kn wants to merge 4 commits into
Conversation
The convention rests on data folders being unignorable-proof: after
setup-data.sh links a folder it is a symlink, and a ".gitignore" pattern
with a trailing slash ("name/") matches directories only — it silently
stops ignoring the symlink, so the link into PII-bearing cloud storage
can get committed. install.sh already writes the no-slash form; nothing
tested it.
Add test-setup-data.sh covering:
- no-slash pattern keeps the data symlink ignored;
- trailing-slash pattern leaks it (negative control);
- setup-data.sh refuses a folder that isn't gitignored;
- the happy-path link lands in the sync root and stays ignored.
Vendor the test via install.sh so every consumer inherits the guard, and
add a CI workflow (shellcheck + test). Raised by the first external user.
|
Holding this open pending #2. Design issue surfaced after opening: two parts of this PR couple the protection story to a CI platform, which violates the platform-independence principle now written down in ARCHITECTURE.md (#3) — the tool may assume git, nothing else.
Depending on how #2 resolves, the vendoring part may be walked back in favor of extending install-hooks.sh with a pre-commit guard. |
…walkthrough Resolves the enforcement design question (#2): the guard against accidentally committing data folders must not depend on a CI platform. - setup-data.sh gains a `check` mode: exits non-zero if any .datadirs folder is unignored, tracked, or staged. This is the single canonical check — every enforcement option just runs it. Unlike link mode it does not skip when CI is set. - install-hooks.sh now installs a pre-commit hook running the check (blocks the leak before anything leaves the machine) alongside the existing post-checkout repair hook, and refuses to overwrite hooks it didn't write (husky/lefthook etc. — prints the line to add instead). - install.sh walks the user through three guard options: pre-commit hook (recommended default, also the non-interactive fallback), a CI check (GitHub Actions or GitLab CI recipe, written only on explicit opt-in), or none. Flags: --enforce=hooks|ci-github|ci-gitlab|none. The consumer test vendoring is dropped — the test suite is a development artifact of this repo, not a consumer artifact. - README gains "Guarding against accidental commits": why the symlink/ trailing-slash leak exists, the recommended default, and when a CI check is genuinely preferred (teams, branch protection, fresh clones). - test-setup-data.sh grows from 7 to 15 assertions: check-mode pass/ fail/recover, hook installation, hook blocking a leaking commit, letting a clean one through, and leaving foreign hooks alone.
|
Reworked per the resolution of #2 — enforcement is now git-native by default (pre-commit hook running the new |
Review feedback from a live install walkthrough. - install.sh: the guard prompt asked "GitHub Actions check" and "GitLab CI check" as sibling top-level options, mixing two decision levels. Now: 1) hook 2) CI check 3) skip, with a platform sub-prompt (GitHub/ GitLab) only after choosing CI. The --enforce=ci-github|ci-gitlab flags are unchanged for non-interactive use. - test-e2e-docker.sh + test/: end-to-end install test on a clean machine. test-setup-data.sh checks the pieces; this checks the experience. The container holds what a real user has — git, a sync client, and a project whose private docs sit in a folder gitignored the old directory-only way — and the test asserts the whole outcome: documents migrate into the sync folder, the folder returns as an ignored symlink, the app still reads through it, the gitignore gains the no-slash entry, and the pre-commit hook blocks a leak. A second scenario drives the interactive walkthrough through a pty, covering the new sub-prompt. Skips cleanly where Docker is unavailable. - README: explain why scripts/ is committed (fresh clones and the hooks depend on it; it pins the reviewed version), and note --no-verify as the hook's by-design gap that the CI option covers. - CI: shellcheck test/*.sh too; new e2e job. The e2e caught a real bug on its first run: without PROJECT_DATA_SRC the installer vendored setup-data.sh from main, so the test exercised published code instead of the working tree.
|
Updated from a live install walkthrough:
The e2e earned its keep immediately — first run caught that without |
Review of this PR found the guard could fail open — silently. install-hooks.sh reconstructed the hooks directory instead of asking git. Two real setups broke: - A *relative* core.hooksPath (exactly how husky v8+ and lefthook configure things — the tools this script claims to coexist with) is resolved by git against the repo root, but the script resolved it against the current directory. Running it from a subdirectory wrote the hook to a path git never reads, printed "pre-commit hook installed", and left the repo unguarded. Reproduced: a data symlink was then committed with no block. - In a git worktree .git is a file, so "$repo_root/.git/hooks" is not a directory and mkdir -p aborts. `git rev-parse --git-path hooks` answers both correctly (verified: it returns the shared hooks dir as an absolute path from inside a worktree), so ask git instead of guessing. Also from the review: - setup-data.sh: pass "--" before pathnames in check-ignore/ls-files, so a folder named like an option (-data) is treated as a path. It failed closed before (permanently un-passable check, plus raw git usage text leaking to the terminal), but it was still wrong. - install.sh: match .gitignore entries literally (grep -Fxq) instead of building an ERE from the folder name, where a name containing "." or "*" could match an unrelated line and skip adding the real entry. Tests: 15 -> 21 assertions, covering the gap that let this through — no test installed hooks from a non-root cwd or with core.hooksPath set. The new cases assert against git's own answer (rev-parse --git-path), not a guessed path, and one asserts the hook actually *blocks* rather than merely exists. Verified they fail against the old logic (5 red) and pass against the fix.
Review found a critical fail-open — fixedA review of this PR turned up a bug where the guard silently protected nothing while reporting success. Reproduced end-to-end before fixing.
Also fixed from the same review:
The test gap that let it through: no assertion installed hooks from a non-root cwd or with |
Closes #2.
Why
The convention rests on data folders never becoming committable. The subtle failure: after linking, a data folder is a symlink, and a
.gitignorepattern with a trailing slash (data/) matches directories only — it silently stops ignoring the symlink, letting a path into private cloud storage get committed (caught by our first external user in review).The first iteration of this PR guarded that with a test wired into consumer CI. Per the platform-independence principle (ARCHITECTURE.md, #3) and the resolution of #2, enforcement is now git-native by default, with CI as an optional, explicitly-opted-in extra.
What
setup-data.sh check— the single canonical guard: exits non-zero if any.datadirsfolder is unignored, tracked, or staged. Runs everywhere including CI (unlike link mode). Every enforcement option below just runs this one command.install-hooks.sh— now installs a pre-commit hook running the check (blocks the leak before anything leaves the machine) plus the existing post-checkout repair hook. Hooks it didn't write (husky/lefthook…) are left alone; the line to add manually is printed.install.shwalkthrough — three options, interactive when a terminal is available,--enforce=flag otherwise:hooks— pre-commit hook (recommended; also the non-interactive default)ci-github/ci-gitlab— writes a CI recipe only on explicit opt-in; the recipe just runs the checknone— DIY; the canonical check is printedThe consumer test vendoring from the first iteration is dropped — the test suite is a development artifact of this repo.
--no-verify-skippable; CI behind branch protection is the only guard an owner can require — plus fresh clones/ephemeral environments). Includes both CI snippets for manual copy.test-setup-data.sh— 7 → 15 assertions: check pass/fail/recover (incl. the trailing-slash regression), hook install, hook blocking a leaking commit end-to-end, letting a clean commit through, leaving foreign hooks alone..github/workflows/ci.yaml— shellcheck + the test suite for this repo (exempt per ARCHITECTURE.md: it tests the tool's development).Verification
shellcheck ./*.shclean;./test-setup-data.sh→ 15 passed, 0 failed.--enforcemodes: correct files/hooks written in each, no prompt hang without a tty (defaults to hooks).