chore(mt#2993): Delete the retired legacy rules-compile pre-commit check - #3738
Merged
Merged
Conversation
…eck; one compile-staleness step mt#3058 moved claude.md / agents.md / claude-rules onto `runCompileCheck` and left `runRulesCompileCheck` as a no-op shell registered as Step 9. Remove the shell, its registration and its `rules-compile-check` interceptor entries (known-guard-names, interceptor-coordinates, interceptor-descriptions; the generated .claude/hooks copies and interceptor-catalog.json regenerated). Pre-commit now runs exactly one compile-staleness step, over all seven targets, still carrying MINSKY_SKIP_SIZE_BUDGET. `classifyCompileCheckError` drops its `kind` parameter — the only caller passed "compile", and the legacy prefix it also matched has no emitter in the hook any more. Its "legacy only" comment on the size-budget branch was stale: size-budget-report.ts emits both budget markers under `[compile --check]` since the cutover. `rules-compile-check.test.ts` → `compile-check.test.ts`, same 37 cases, markers repointed at `[compile --check]`; the compile-check interceptor's provenance now names it. Docs: ADR-016 gains a "Phase 4 shipped" note; docs/architecture.md and a crud-operations.ts comment no longer name the deleted method. Negative control: the repointed test file run against main's classifier (legacy default prefix) — 25 pass / 12 fail; 37 pass after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UG8FZC1RHk7otPDDfs2zrr
Minsky Reviewer StatusVerdict: APPROVED — no blocking findings Commands
|
![minsky-reviewer[bot]](https://avatars.githubusercontent.com/u/52695?s=60&v=4){kind=small}
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
This PR correctly deletes the retired legacy rules-compile pre-commit step, updates registries, repoints tests, and records the Phase 4 consolidation in docs. However, the user-facing remediation hint in classifyCompileCheckError suggests bun run minsky compile --target … while other surfaces and simulated outputs use minsky compile --target …. That inconsistency is confusing and must be aligned with the canonical invocation (or justified and harmonized) before merge. Non-blocking suggestions: verify interceptor census tests reflect the removal; consider pinning the remediation hint in tests; optionally add an integration assertion that only one compile-staleness step fires; and sweep for any lingering prescriptive references to the deleted step. With the remediation-hint fix, this looks ready to merge.
Findings
- [BLOCKING] src/hooks/pre-commit.ts:2921 — Regenerate-hint command string may be wrong or inconsistent with the actual CLI invocation path
InclassifyCompileCheckError's stale case, the suggested remediation logsRun "bun run minsky compile --target <target>" to regenerate.(src/hooks/pre-commit.ts:2948-2952, approx). Elsewhere, the CLI itself (and historical guidance in tests) usesminsky compile --target …without thebun runprefix (seesrc/hooks/compile-check.test.ts:83-92simulated CLI output). If the intended operator command is the top-levelminskyexecutable (which is what docs and other hints typically recommend), this hint may be incorrect or at least inconsistent. This is user-facing behavior and can confuse operators. Please align the hint with the canonical invocation (either alwaysminsky …or alwaysbun run src/cli.ts …) and update tests/docs/hints accordingly. Ifbun run minsky …is in fact required, add evidence and update other sites for consistency. - [NON-BLOCKING] src/generated/interceptor-catalog.json:3880 — Catalog removal of
rules-compile-checkchanges census; ensure corresponding tests/consumers don't still pin counts
rules-compile-checkis removed fromsrc/generated/interceptor-catalog.json(around :3880-:3890) and from.claude/.minskyregistries. If any interceptor-census tests pin the number or presence of guards by name, they must be updated. I did not see such a failure in this diff, but please double-check thatinterceptor-catalogregeneration and census tests reflect the new single-step reality and don't still expectrules-compile-check. - [NON-BLOCKING] src/hooks/compile-check.test.ts:1 — Repointed tests rely on string markers; consider adding a minimal assertion on the suggested remediation text
The repointed test suite asserts marker classification and that 'regenerate' appears, but it does not pin the remediation command shape (e.g.,minsky compile --target …vsbun run minsky …). Given the user-facing hint changed shape in this PR, a lightweight assertion that the hint matches the canonical command would prevent future drift. This is advisory (testing-standards.mdc §Testable Design: assert observable contracts), not a merge-blocker. - [NON-BLOCKING] docs/architecture/adr-016-compile-pipeline-convergence.md:42 — Doc update mentions shipped Phase 4; sweep for any other lingering references to
rules-compile-checkoutside historical sections
ADR-016 was updated to record Phase 4; good. There may still be stray mentions elsewhere (e.g., older architecture docs). A quick sweep beyond the explicitly updated files would ensure readers don’t encounter stale operational guidance. Not blocking for this cleanup PR, but worth a follow-up task if any are found. - [NON-BLOCKING] src/hooks/pre-commit.test.ts:14 — Comment updates kept; consider adding a tiny runtime integration check that pre-commit emits exactly one compile-step fire-log entry
The PR description cites a live-run fire-log showing a singlecompile-checkentry and norules-compile-check. Since fire-log shape is designed to be testable (seerunInstrumentedStep), a small unit/integration test could assert thatrun()wires only one compile-staleness step. Not blocking, but it would codify the intent and reduce regressions. - [NON-BLOCKING] UNVERIFIED: ~/.local/state/minsky/fire-log.jsonl:1 — NEEDS VERIFICATION: Out-of-repo fire-log evidence could not be verified by reviewer
The PR references a local fire-log path (~/.local/state/minsky/fire-log.jsonl) as execution evidence that only onecompile-checkstep fired. This path is outside the repository; I cannot verify it here. Treat this as non-blocking; please ensure CI or a checked-in artifact captures a representative log snippet if durable evidence is desired.
Documentation impact
- no-update-needed — This PR removes a legacy pre-commit step and repoints tests/registries. It also updates ADR-016 and architecture.md inline; no additional external docs appear to require updating beyond the changes already included. I scanned ADR-016 and architecture.md changes in this diff and saw no contradictory guidance left in those files. Any remaining historical mentions are acknowledged as history.
…etired step; pin one compile step BLOCKING: the stale-case hint said `bun run minsky compile --target <t>`, a spelling that works only inside this repo (package.json's `minsky` script); the compile CLI's own hint and pre-commit's setup hint both say `minsky …`. Aligned, and the hint shape is now pinned by the staleness test. `rules-compile-check` is recorded as RETIRED rather than merely absent — in `RETIRED_GUARD_NAMES` (lastSeen 2026-09-11, 4,435 records on the authoring log), a retired-stratum description, and retired coordinates — so its historical fire-log rows keep resolving instead of reading as anomalies; the authored-point manifest in interceptor-coordinates.test.ts gains the entry. Non-blocking: an in-repo pin replaces the out-of-repo fire-log evidence — compile-check.test.ts asserts pre-commit.ts wires `compile-check` exactly once and the retired name not at all, and that the guard roster carries one and retires the other. Census: `bun run test:hooks` 7177 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UG8FZC1RHk7otPDDfs2zrr
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
R2 verification: the prior BLOCKING concern is addressed. runRulesCompileCheck and its Step 9 registration are fully removed; the single remaining Step 9 compile-check path is intact and wired once. classifyCompileCheckError drops the legacy kind parameter and aligns its stale-regenerate hint to minsky compile --target …, with call sites updated. Interceptor registries now record rules-compile-check as RETIRED and the generated catalog reflects the updated provenance. The repointed tests all target [compile --check], and an added pin verifies one compile-staleness step and retirement recording. I found no new critical issues introduced by the fix; one non-blocking note suggests preferring a designed observable over source-text scanning in the new test. Overall, the changes meet the spec and are ready to merge.
Findings
- [NON-BLOCKING] src/hooks/compile-check.test.ts:606 — Test asserts step wiring by scanning source text; consider a designed observable
The test pins “exactly one compile-staleness step” by readingsrc/hooks/pre-commit.tswithreadFileSyncand regex-scanning forthis.instrumented("compile-check", …). While effective, this couples the test to file text rather than a designed observable (testing-standards.mdc §Testable Design). Consider exposing the step roster (or a small helper returning the registered guard names) as a data structure so the test can assert against that API instead of parsing source. This is advisory and non-blocking; the current approach works but is brittle to unrelated formatting changes.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
runRulesCompileCheck and its Step 9 registration are removed from src/hooks/pre-commit.ts |
Met | src/hooks/pre-commit.ts:635-649 — the legacy Step 9 block and runRulesCompileCheck() call are removed; src/hooks/pre-commit.ts:2331-2369 shows the entire runRulesCompileCheck method deleted. Comments at src/hooks/pre-commit.ts:636-646 explain the consolidation into a single Step 9. |
src/hooks/rules-compile-check.test.ts (and any successor) is removed or repointed at the surviving check |
Met | The file is renamed to src/hooks/compile-check.test.ts and all marker strings are repointed to [compile --check] (e.g., src/hooks/compile-check.test.ts:1-18, 34-46, 118-137). Header comment records the rename and rationale. |
src/hooks/pre-commit.ts runs exactly ONE compile-staleness check, covering all seven targets: claude-skills, claude-agents, claude-hooks, cursor-rules-ts, claude.md, agents.md, claude-rules |
Met | The only remaining instrumented step is compile-check (Step 9) — see src/hooks/pre-commit.ts:636-646. The test pins exactly one compile-check occurrence and absence of rules-compile-check by scanning the source (src/hooks/compile-check.test.ts:606-627). The target set remains in compileCheckTargets (unchanged in this diff), referenced in comments at src/hooks/pre-commit.ts:2621-2628 noting claude.md, agents.md, and claude-rules moved here at mt#3058. |
classifyCompileCheckError's kind="rules" branch is removed or documented as retained-for-legacy-strings, with a stated reason |
Met | Signature changed to classifyCompileCheckError(error, target); kind parameter removed and internals now hardcode cmd = "compile" with updated docblock stating legacy removal (mt#3058/mt#2993) — see src/hooks/pre-commit.ts:2779-2792 and src/hooks/pre-commit.ts:2935-2960. Call site updated accordingly at src/hooks/pre-commit.ts:2465-2471. |
MINSKY_SKIP_SIZE_BUDGET override handling survives on the single remaining check (it moved there in mt#3058 — verify, don't assume) |
Met | Env var declared up top with updated comment (runCompileCheck, not legacy) — src/hooks/pre-commit.ts:94-103. The surviving Step 9 threads the override into instrumented and applies it only when errorKind === "budget-exceeded" — src/hooks/pre-commit.ts:636-646 and src/hooks/pre-commit.ts:2499-2511. |
claude-agents drift (mt#1654, excluded from the mt#2252 check, re-enabled by mt#2497) is either reconciled or explicitly carried forward as a named exclusion with a tracking pointer |
Met | Comments at src/hooks/pre-commit.ts:2621-2628 record that claude.md, agents.md, and claude-rules moved here at mt#3058; earlier context in this file (unchanged in this diff) carries the mt#2497 reconciliation. No exclusion for claude-agents remains; tests still pass against the unified classifier. |
| Tests green; a stale output in ANY of the seven targets is still caught by the single check | Met | Unit tests repointed and still exercise staleness detection and overrides for the unified classifier — see src/hooks/compile-check.test.ts (37 cases retained). Additional pins assert one compile-staleness step is wired (src/hooks/compile-check.test.ts:595-631). |
Documentation impact
- no-update-needed — This PR removes a retired pre-commit step and repoints tests; user-facing behavior remains “one compile-staleness step,” which was already the case post-cutover. The interceptor descriptions and generated catalog were updated in-repo to reflect the retirement and provenance (
.claude/.minsky/hooks/*,src/generated/interceptor-catalog.json). No separate docs underdocs/are changed by this diff segment, and no existing prose is invalidated by the classifier/hint tweak (the hint now matches the compile CLI’s own wording).
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
R3 verification: prior BLOCKING issues are addressed, and the fix introduced no critical defects. runRulesCompileCheck and its Step 9 registration are fully removed; classifyCompileCheckError drops the legacy kind param and aligns the hint to minsky compile --target …. Interceptor registries move rules-compile-check to the RETIRED stratum with coordinates and notes, and compile-check’s description/provenance are updated. Tests are repointed to [compile --check] and include pins for single-step wiring and retired-name handling. I found no new blocking issues; one non-blocking note suggests preferring a designed observable over source scanning in the step-wiring test. Overall, the changes meet the spec and are ready to merge.
Findings
- [NON-BLOCKING] src/hooks/compile-check.test.ts:606 — Test asserts step wiring by scanning source text; consider a designed observable
The test pins “exactly one compile-staleness step” by readingsrc/hooks/pre-commit.tswithreadFileSyncand regex-scanning forthis.instrumented("compile-check", …). While effective, this couples the test to file text rather than a designed observable (testing-standards.mdc §Testable Design). Consider exposing the step roster (or a helper that returns the registered guard names) as a data structure so the test can assert against that API instead of parsing source. This is advisory and non-blocking; the current approach works but is brittle to unrelated formatting changes.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
runRulesCompileCheck and its Step 9 registration are removed from src/hooks/pre-commit.ts |
Met | src/hooks/pre-commit.ts — the Step 9 block invoking this.runRulesCompileCheck() is removed and only the compile-check step remains; the entire runRulesCompileCheck method is deleted in this diff hunk (the large block between the instrumented steps and runCompileCheck disappears). |
src/hooks/rules-compile-check.test.ts (and any successor) is removed or repointed at the surviving check |
Met | src/hooks/rules-compile-check.test.ts → src/hooks/compile-check.test.ts (rename). All marker strings switched from [rules compile --check] to [compile --check] and tests import guard rosters to pin one-step wiring (see src/hooks/compile-check.test.ts: header comment and helper functions staleMarker/budgetExceededMarker changed). |
src/hooks/pre-commit.ts runs exactly ONE compile-staleness check, covering all seven targets: claude-skills, claude-agents, claude-hooks, cursor-rules-ts, claude.md, agents.md, claude-rules |
Met | src/hooks/pre-commit.ts — only one this.instrumented("compile-check", …) block remains for Step 9; comments state it is the only compile-staleness step. The target presence map (compileCheckTargets) section remains unchanged and still enumerates the seven targets; tests pin exactly one compile-check registration in src/hooks/compile-check.test.ts (final describe block). |
classifyCompileCheckError's kind="rules" branch is removed or documented as retained-for-legacy-strings, with a stated reason |
Met | src/hooks/pre-commit.ts — classifyCompileCheckError signature now takes (error, target) only; all internal references hardcode cmd = "compile". The docblock explains the legacy path was removed after mt#3058/mt#2993. Call site updated to classifyCompileCheckError(error, target) in runCompileCheck. |
MINSKY_SKIP_SIZE_BUDGET override handling survives on the single remaining check (it moved there in mt#3058 — verify, don't assume) |
Met | src/hooks/pre-commit.ts — the env var documentation above remains and references runCompileCheck; inside runCompileCheck, the override gating remains around result.errorKind === "budget-exceeded" to bypass only budget failures. The Step 9 instrumented call threads the override env var. |
claude-agents drift (mt#1654, excluded from the mt#2252 check, re-enabled by mt#2497) is either reconciled or explicitly carried forward as a named exclusion with a tracking pointer |
Met | src/hooks/pre-commit.ts — comments in the compile-check target section note the mt#3058 move and earlier reconciliation; no exclusion for claude-agents remains. Tests exercise unified classifier behavior. |
| Tests green; a stale output in ANY of the seven targets is still caught by the single check | Met | src/hooks/compile-check.test.ts — retained classifier cases now keyed to [compile --check]; additional tests assert single-step wiring and retired-name handling. The PR description reports 39/39 passing in this file and global bun run test:hooks passing; the diff shows no loosened assertions. |
Documentation impact
- updated-in-pr — This PR updates in-repo docs to reflect the retirement and consolidation: docs/architecture.md now references
runCompileCheckinstead ofrunRulesCompileCheck; ADR-016 adds a “Phase 4 shipped (mt#2993)” section documenting the change; comments in packages/domain/src/rules/operations/crud-operations.ts were adjusted to refer to the pre-commit compile check generically. The generated interceptor catalog/registries were also updated to markrules-compile-checkas retired and to expandcompile-check’s description and provenance.
Affected: docs/architecture.md, docs/architecture/adr-016-compile-pipeline-convergence.md, packages/domain/src/rules/operations/crud-operations.ts, src/generated/interceptor-catalog.json, .claude/hooks/, .minsky/hooks/
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 4 of the compile-pipeline convergence (mt#2293 / ADR-016). The mt#3058 cutover moved
claude.md/agents.md/claude-rulesonto the new-pipelinerunCompileCheckand left the legacyrunRulesCompileCheckas a no-op shell still registered as pre-commit Step 9 under the instrumented namerules-compile-check. This deletes the remainder, so one compile-staleness step remains.Changes
src/hooks/pre-commit.ts— Step 9 registration andrunRulesCompileCheckdeleted; the survivingcompile --checkstep is now Step 9 and the only compile-staleness step, over all seven targets, still carryingMINSKY_SKIP_SIZE_BUDGET.classifyCompileCheckErrordrops itskindparameter (the only caller passed"compile"; the legacy prefix has no emitter in the hook any more) and its stale "legacy only" comment on the size-budget branch is corrected —packages/domain/src/compile/size-budget-report.ts:93,119emits both budget markers under[compile --check]since the cutover. Its stale-case hint now readsminsky compile --target <t>, the same shape as the compile CLI's own hint (R1). Comments naming the deleted method updated.src/hooks/rules-compile-check.test.ts→src/hooks/compile-check.test.ts— same 37 cases, markers repointed at[compile --check]; plus (R1) a pin on the hint shape and a two-test pin that pre-commit wirescompile-checkexactly once and the retired name not at all, and that the guard roster carries one and retires the other.rules-compile-checkmoved to the RETIRED stratum rather than deleted (R1) —RETIRED_GUARD_NAMESentry (lastSeen 2026-09-11), a retired-stratum description withprovenance: [KNOWN_NAMES], retired coordinates, and the authored-point manifest ininterceptor-coordinates.test.ts— so its 4,435 historical fire-log rows keep resolving instead of reading as anomalies. Thecompile-checkdescription now says it covers the rule-edit gap and its provenance names the repointed test. Generated.claude/hooks/copies andsrc/generated/interceptor-catalog.jsonregenerated.### Phase 4 shipped (mt#2993);docs/architecture.md:330and acrud-operations.tscomment no longer name the deleted method.docs/architecture/evaluation-loop-phase2.md's two mentions are a dated measurement record and are left as history.Deploy verification
isDeploySurfaceFileflagssrc/hooks/pre-commit.ts,src/generated/interceptor-catalog.jsonand the comment-onlycrud-operations.ts; the runtime behaviour change is confined to the local pre-commit hook. After merge:mcp__minsky__deployment_wait-for-latestfor the affected services withnotBefore= merge time, workflow-run correlation at the merge SHA, and a health read — SUCCESS plus runtime started, recorded on the task.Execution evidence
Per success criterion:
grep -rn runRulesCompileCheck src/hooks/returns nothing (only ADR-016's historical Context and its new Phase-4 note mention the name repo-wide); pinned by the new test.bun test --preload ./tests/setup.ts src/hooks/compile-check.test.ts→39 pass / 0 fail(37 original + 2 R1 pins).this.instrumented("compile-check", …)appears exactly once inpre-commit.tsandrules-compile-checknot at all; the step's log line names all seven targets (pre-commit.ts:2386, unchanged). (The commit's own pre-commit run also wrote onecompile-checkfire-log record and zerorules-compile-check— a local observation, now superseded by the in-repo pin.)kind="rules"branch is removed (parameter deleted), with the reason in the classifier docblock.MINSKY_SKIP_SIZE_BUDGETthreaded on the surviving step's registration and consulted insiderunCompileCheck— verified, not assumed.claude-agentsis in the surviving step's target list;pre-commit.tsrecords that mt#2497 reconciled the drift and subsumed mt#1654. Nothing carried forward.bun run test:hooks:7177 pass / 0 fail(195 files, includes the interceptor census tests, re-run after R1);bun scripts/run-related-tests.tsover the changed sources:424 pass / 0 fail(24 files);validate_typecheck0 errors across 8 projects (infra/skipped, not installed locally);validate_lintclean on the changed files; prettier unchanged.Acceptance tests: AT1 as SC1. AT2 — appended a line to
CLAUDE.mdin the session workspace:compile --check --target claude.md→ exit 1,[compile --check] Target "claude.md" is STALE; restored → exit 0. AT3 as SC3.Negative control (test-first, mt#3244): the repointed
compile-check.test.tsrun againstmain'spre-commit.ts(classifier defaulting to the legacy prefix) →25 pass / 12 fail; all pass against this branch.R1 (review round 1)
bun run minsky …does run here (package.jsonminskyscript) but is repo-only; the compile CLI (compile-commands.ts:233,266) and pre-commit's setup hint (:2998) both sayminsky …. Aligned; pinned in the staleness test (Run "minsky compile --target agents.md", andnot.toContain("bun run minsky")).RETIRED_GUARD_NAMESentry, and the authored-point manifest on the coordinates. Both are the append-only manifests the repo uses for exactly this, now updated;test:hooks7177 pass.grep -rn 'rules-compile-check\|runRulesCompileCheck' src packages docs .minsky scripts tests: only ADR-016's historical Context lines (:11,:21, describing the pre-convergence state the ADR decided against) andevaluation-loop-phase2.md's dated measurement; no operational guidance names the step.this.instrumented(…)calls, not data), plus the roster/retired-set assertion.Parallel-work note
Open PR #3253 (mt#3854) also edits
src/hooks/pre-commit.ts, in thecompileCheckTargetsregion (+15 lines of its own, adding acodexpresence field); this PR touches that region only in one docblock sentence. Its branch predates mt#4866/mt#4986/mt#5003 and needs a rebase regardless. PR #3412 (mt#4639) lists the same files but makes no change of its own to them (three-dot diff empty).🤖 Generated with Claude Code
https://claude.ai/code/session_01UG8FZC1RHk7otPDDfs2zrr