Skip to content

[codex] Add pre-commit quality gate#35

Merged
moose-lab merged 2 commits into
mainfrom
codex/pre-commit-quality-gate
May 30, 2026
Merged

[codex] Add pre-commit quality gate#35
moose-lab merged 2 commits into
mainfrom
codex/pre-commit-quality-gate

Conversation

@moose-lab

Copy link
Copy Markdown
Owner

Summary

  • Add Husky-powered pre-commit hook for DevLog.
  • Centralize quality gates in package scripts: staged whitespace check, TypeScript typecheck, tests, and CI build gate.
  • Reuse the same CI quality script from GitHub Actions and add a regression test so hook/script drift is caught.

Validation

  • .husky/_/pre-commit passed on the PR branch: staged whitespace check, typecheck, and 166 tests.
  • bun run quality:ci passed on the PR branch: typecheck, 166 tests, Next.js production build, and CLI build.
  • git diff --cached --check passed before commit.

Notes

  • This PR targets main; no direct push to main was made after the flow was corrected.
  • The original working tree still has unrelated docs/plans changes and they were intentionally excluded.

@claude

claude Bot commented May 30, 2026

Copy link
Copy Markdown

Claude finished @moose-lab's task in 2m 39s —— View job


PR Review: Add pre-commit quality gate

  • Gather context
  • Read changed files
  • Analyze code
  • Post review findings

Findings

  • [nit] quality-gates.test.ts:18–26: Script values are checked with assert.equal (exact string), while the hook check at line 34 uses assert.match (regex). The exact-match approach for scripts is intentional and catches drift well, but it means any improvement to those script strings (e.g. moving TZ=Asia/Shanghai into a wrapper) requires a dual update. A one-line comment above the equality assertions explaining why they're exact would save the next person from loosening them unnecessarily.

  • [nit] package.json (quality:precommit / quality:ci): The TZ=Asia/Shanghai pin is embedded silently in both script strings. The TODO explaining the coupling exists in ci.yml but not here. A developer who only reads package.json won't know why the TZ is hardcoded. Consider a short comment in README or the script name itself, since JSON can't hold comments.

  • [nit] quality-gates.test.ts lives under src/core/__tests__/ but tests infrastructure config (package.json + a dotfile), not core logic. Not a correctness issue — just mildly surprising if someone scans that directory expecting domain tests.

No correctness bugs, no cross-platform regressions (CI is Linux-only; TZ=x bun run ... syntax is fine there), no security concerns, and no type-safety issues — the test file is pure Node built-ins with strict assertions.

The prepare: husky addition is safe for CI because GitHub Actions sets CI=true and Husky 9 skips hook installation in that environment.

Verdict: APPROVE

@claude

claude Bot commented May 30, 2026

Copy link
Copy Markdown

Claude finished @moose-lab's task in 2m 24s —— View job


PR Review: Add pre-commit quality gate

  • Gather context
  • Read changed files
  • Analyze code
  • Post review findings

Context note: This sync was a merge from main — the quality gate code in commit b3a87d2 is unchanged. Review covers the full PR diff.


Findings

  • [nit] quality-gates.test.ts:36: assert.equal(mode & 0o111, 0o111) requires all three execute bits (owner + group + other). A restrictive umask (e.g. 077) would cause this assertion to fail even on a correctly executable file. assert.ok(mode & 0o100, "pre-commit hook is not executable") checks owner-execute only, which is what actually matters and is umask-independent.

  • [nit] package.json:24,26: TZ=Asia/Shanghai is embedded silently in quality:precommit and quality:ci. The TODO explaining why lives in ci.yml but JSON can't hold comments. A developer who only reads package.json or runs bun run quality:ci locally won't know why the timezone is pinned. The new README paragraph added in this PR (README.md:138-145) is a natural place for a one-liner callout about the TZ coupling.

  • [nit] quality-gates.test.ts:15–27: The exact-match assertions on script strings mean any improvement to those scripts (e.g. adding a --reporter flag) requires a dual update here. This is intentional drift protection, but a short comment explaining why these are exact matches (rather than assert.match) would prevent a future reader from loosening them unnecessarily.

  • [nit] src/core/__tests__/quality-gates.test.ts tests infrastructure config (package.json + a dotfile), not core domain logic. Not a correctness issue — just mildly surprising for someone scanning that directory expecting domain tests. src/__tests__/ or a top-level tests/ would be a more natural home.

No correctness bugs, no security concerns, no type-safety issues. build:web (next build) already existed in mainquality:build calling it is fine. Husky 9 correctly skips prepare when CI=true, so the prepare: "husky" addition is safe for GitHub Actions. The TZ=x bun run ... prefix syntax works on Linux/macOS (CI is Linux-only; local Windows dev is the only gap, which is pre-existing).

Verdict: APPROVE

@moose-lab
moose-lab marked this pull request as ready for review May 30, 2026 15:34
@moose-lab
moose-lab merged commit 6743c94 into main May 30, 2026
4 checks passed
@moose-lab
moose-lab deleted the codex/pre-commit-quality-gate branch May 30, 2026 15:37
Sign up for free to 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.

1 participant