Skip to content

hook: the compiled-file skip in both pre-commit hooks is dead code — tidy is invoked without --source #320

Description

@stackbilt-admin

Found during the review of #319 (fixing #296). Pre-existing, not introduced by that PR.

The compiled-file skip in both hooks has never done anything

.githooks/pre-commit:14-19 and the generated equivalent in packages/cli/src/commands/hook.ts:53-59 both filter compiled vendor files out of the staged list:

if [ -f "$vf" ] && grep -q 'GENERATED by charter adf compile' "$vf"; then
  continue
fi
STAGED_VENDORS="$STAGED_VENDORS $vf"

Two lines later, the tidy invocation passes no --source:

TIDY_OUTPUT=$(node packages/cli/dist/bin.js adf tidy --dry-run --format json 2>/dev/null || echo '{}')

And adfTidyCommand (packages/cli/src/commands/adf-tidy.ts:106-108) falls back to scanning everything when no source is given:

const targets = sourceFile
  ? [sourceFile]
  : VENDOR_FILES.filter(f => fs.existsSync(path.resolve(f)));

So STAGED_VENDORS never reaches tidy. It is used only for the echo on the preceding line and for re-staging afterwards. Every vendor file on disk is scanned regardless of what the filter excluded.

Why it matters

This is why #296 escaped the hook. The grep reads as protection — it was cited as evidence that skipping compiled files was already proven in the field — but it only ever suppressed the reporting path. tidy still ingested compiled output on every commit, which is exactly the corruption #296 reported.

The real fix landed in #319 at adf-tidy.ts:263, inside the command, where it applies to every caller rather than to a list the command never receives.

Options

  1. Delete both greps. They are now genuinely redundant — adf tidy skips compiled files itself as of fix(adf): stop tidy ingesting compiled vendor output #319. Smallest change, removes a comment that actively misleads the next reader.
  2. Pass --source per staged file. Makes the hook do what it looks like it does, and narrows tidy to staged files rather than the whole tree — arguably the better behavior, since a pre-commit hook rewriting unstaged files is surprising. Bigger change; needs a loop and care about the re-staging step.

Option 1 unless there is appetite for the scoping change in option 2. Either way the two copies (.githooks/pre-commit and the generator in hook.ts) must move together, or a charter hook install --pre-commit will silently reintroduce the old text.

Note

packages/cli/src/__tests__/integration/precommit-hook.test.ts:117 pins the totalExtracted grep string, so changes here should keep that passing or update it deliberately.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions