Skip to content

feat(mt#5003): Deliver always-apply rules through the claude-rules target, not a new CLAUDE.md - #3646

Merged
edobry merged 2 commits into
mainfrom
task/mt-5003
Sep 5, 2026
Merged

feat(mt#5003): Deliver always-apply rules through the claude-rules target, not a new CLAUDE.md#3646
edobry merged 2 commits into
mainfrom
task/mt-5003

Conversation

@minsky-ai

@minsky-ai minsky-ai Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

mt#4986 stopped Minsky destroying a project's own CLAUDE.md and deliberately left a gap:
.claude/rules/ admitted only glob-scoped rules, so a project whose CLAUDE.md we do not own
received zero always-apply rules. ask#11711 (closed 2026-09-05) chose the channel. Selected
option LABEL: "Deliver via .claude/rules/ (agent recommendation)".

The harness contract this rests on

Read at source 2026-09-05 (code.claude.com/docs/en/memory), two exact statements:

Rules without paths frontmatter are loaded at launch with the same priority as .claude/CLAUDE.md.

Rules without a paths field are loaded unconditionally and apply to all files.

The near-miss is paths: []. An empty list is still a paths field, so the rule is
path-scoped with zero matching patterns — loaded never. The harness keys on the field's ABSENCE, so
the serializer omits the whole frontmatter block, which also moves the banner to line 1 (still
inside the five-line window the edit guard and the orphan sweep both scan).

Corroborating, and a different KIND of channel: .claude/rules/ path-scoped injection was observed
working natively in a live Claude Code session in this repo, and a grep over .minsky/hooks/ +
.claude/hooks/ confirms no Minsky hook performs it — it is the harness's own loader.

What changed

One predicate keeps the two channels mutually exclusive. claudeMdIsOurs answers "does the
always-apply set already have a home?", and always-apply rules land in .claude/rules/ only when
the answer is no. Without it, widening the eligibility arm would have emitted this repository's
138,167-char always-apply corpus into .claude/rules/ as well as CLAUDE.md
— roughly doubling
its always-loaded context and blowing the 135,000/145,000 budget from the other side.

claude.md is selected only for a CLAUDE.md we generated. We MAINTAIN ours; we do not CREATE
one. The absent case used to fall through to "write it", which is how a fresh managed project got a
15 KB CLAUDE.md. An operator can still opt in once with --target claude.md (that path returns
early and never reaches the probe), after which the file carries the banner and stays maintained —
the same affordance the AGENTS.md harness gate already offers.

Eligibility keys on alwaysApply, not on tier and not on a rule-name list. That is what keeps
the deliberately on-demand tier out (mt#4735 SC4 / mt#3107): all four operational-reference rules
carry no alwaysApply key at all, so neither arm can reach them. The trap a name- or
tier-keyed implementation would hit is real — task-status-workflow-protocol is on-demand in the
plant and alwaysApply: true in the product corpus. Same id, opposite dispositions, two files.

Two things the spec did not carry

The skip message became FALSE. It told the operator "nothing loads them into your agent
automatically … an agent has to ask for one by name with rules_get"
— accurate under mt#4986, and
wrong the moment .claude/rules/ started carrying the set. Caught by reading the actual output of a
bare compile, not by re-reading the diff. It is now per-file: CLAUDE.md gets the reassuring
truth, AGENTS.md keeps the honest one, because .claude/rules/ is Claude-Code-only and Claude
Code reads CLAUDE.md, not AGENTS.md. One shared sentence made one of them a lie whichever way it
was written.

foreignOutputs + ownedOutputs as separate booleans made {foreign: true, owned: true}
representable
— a state that cannot exist, whose resolution depended on which check happened to
run first. Collapsed into one ownership tri-state per file (generated / foreign / unreadable
/ absent), so the illegal state is unrepresentable rather than merely unreachable. The type change
then found every affected fixture for me.

Execution evidence:

bun scripts/run-related-tests.ts packages/domain/src/compile/targets/claude-rules.ts packages/domain/src/compile/compile.ts packages/domain/src/compile/monolithic-ownership.ts packages/domain/src/init.ts src/hooks/pre-commit.ts

 410 pass
 0 fail
 2025 expect() calls
Ran 410 tests across 21 files. [3.01s]

Typecheck: 0 errors across 8 projects. Lint: 0 errors, 0 warnings across 4,385 files.

SC1 / SC2 — globless emission, banner still in the first five lines

AT3's run (below) wrote four files; each opens with the banner on line 1 and has no frontmatter:

--- key-workflows.md ---
<!-- Generated by minsky rules compile. Do not edit directly. -->

# Key Workflows (via skills)

Edit guard exercised live against the real hook on one of them:

$ echo '{"tool_name":"Edit","tool_input":{"file_path":"<scratch>/.claude/rules/key-workflows.md",...}}' \
    | bun .minsky/hooks/check-generated-file-edit.ts
{"permissionDecision":"deny", ... "Marker: [HTML comment: Generated by]"}

Unit: emits an always-apply rule with NO frontmatter block at all (asserts not.toContain("paths:")),
keeps the banner inside the five-line window the edit guard scans,
still emits paths frontmatter for a glob-scoped rule.

SC3 — no CLAUDE.md is created

$ bun run src/cli.ts init --repo <fresh scratch, no CLAUDE.md> --backend minsky --rule-format minsky --mcp false
minsky init: wrote 4 base rule(s) to .minsky/rules.
minsky init: 13 declinable rule(s) ship with Minsky but were NOT installed — ...
Project initialized successfully.

$ test -e CLAUDE.md && echo YES || echo no

$ ls .claude/rules/
key-workflows.md  minsky-session-workflow.md  operational-safety-dry-run-first.md  task-status-workflow-protocol.md

Pre-fix control: this path wrote a 15,085-byte CLAUDE.md (measured 2026-09-04).

Unit: mt#5003: with no CLAUDE.md of ours, claude.md is NOT selected, and end-to-end through the
probe as a discriminating pair — detects all four source dirs together (with an owned CLAUDE.md)
vs the same repo WITHOUT a CLAUDE.md of ours omits claude.md, identical fixtures but one file.

SC4 — this repository is NOT duplicated

$ bun run src/cli.ts compile --check
[compile] Target "claude.md": 1 file(s) written
[compile] Target "claude.md" output size: 138167 chars
[compile] Target "claude-rules": 17 file(s) written
...
EXIT=0

claude.md still selected, ruleContentChars still 138,167, .claude/rules/ still exactly
17 files, nothing stale. Unit:
listOutputFiles EXCLUDES an always-apply rule when a CLAUDE.md of ours carries it.

SC5 — reachability accounting

The AT3 run above emits no "not reachable by Claude Code" warning: the base rules are now
reached, and init's accounting (mt#4770) reads that from the pipeline rather than re-deriving it.

SC6 — the doc paragraph this makes false

docs/rules/template-system-guide.md's "What it costs you today" paragraph said the base rules
reach the agent through no automatic channel. Replaced, with an explicit note that the earlier text
was true between mt#4986 and mt#5003 and no longer is — so a reader who remembers it is corrected
rather than confused.

SC7 — the on-demand tier is untouched

Unit: leaves the deliberately on-demand tier ineligible under BOTH arms. Measured basis, recorded
on the spec: all four plant copies of architectural-bypass-prevention,
efficient-database-queries, task-status-workflow-protocol, verification-checklist carry no
alwaysApply key, so an alwaysApply-keyed predicate cannot reach them.

AT2 — the mt#4986 regression, plus the new delivery

$ md5 CLAUDE.md   # before init
f3fe30755140644bc712bd4881404e5a
$ bun run src/cli.ts init --repo <scratch> ...
$ md5 CLAUDE.md   # after
f3fe30755140644bc712bd4881404e5a      # byte-identical
$ ls .claude/rules/
key-workflows.md  minsky-session-workflow.md  operational-safety-dry-run-first.md  task-status-workflow-protocol.md

The user's file is untouched and they now receive the rules — which is the whole point.

The per-file message, both branches, from one bare compile:

[compile] <scratch>/CLAUDE.md was left untouched — it does not carry Minsky's generated-file
  banner, so it is treated as yours and is never overwritten. Minsky's rules still reach your
  agent: the always-apply ones are written to .claude/rules/ as paths-less files, which Claude
  Code loads at launch at the same priority this file would have had. ...
[compile] <scratch>/AGENTS.md was left untouched — ... Minsky's rule sources are in .minsky/rules/;
  this harness has no channel that loads them automatically while this file is yours, so an agent
  has to ask for one by name with `rules_get <name>`. ...

Parity harness — unaffected by design

$ bun scripts/verify-compile-parity.ts
✅ compile-parity: all 5 checks passed

Planning flagged this as a consumer that would report a false divergence. includeAlwaysApply
defaults false, so the legacy twin and the harness compare like for like and no change to the
script was needed.

Negative control:

isEligibleForClaudeRules is the single gate every path routes through, so forcing its always-apply
arm off restores the complete pre-fix behaviour rather than reverting one line into a state that is
neither pre- nor post-fix (mt#4512):

 61 pass
 4 fail
Ran 65 tests across 2 files.

(fail) claude-rules: always-apply eligibility (mt#5003) > is eligible when the always-apply channel is this target
(fail) claude-rules: always-apply eligibility (mt#5003) > needs no globs on the always-apply arm
(fail) claude-rules: always-apply eligibility (mt#5003) > buildClaudeRulesContent includes always-apply rules only when told to
(fail) claudeRulesTarget > listOutputFiles INCLUDES an always-apply rule when no CLAUDE.md of ours exists (mt#5003)

Reverted before commit; the suite is green above.

What it does NOT cover, stated rather than implied: the target-SELECTION tests (claude.md no
longer created) and the pre-commit mirror do not route through this predicate, so they stayed green
under the revert. They cover a different half of the change, and the type-level collapse to
ownership is what proved they were all updated — the compiler enumerated the sites, I did not.

AT1 — the one check I could not run, and who can

The spec's AT1 is a live /context in a scratch repo confirming a globless .claude/rules/*.md
actually appears under Memory files. That needs a real Claude Code session started in that
directory, which is on your side of the boundary, not mine. The claim is strong-evidence — vendor
doc read at source, two exact quotes, plus the observed native path-scoped injection — and it is not
live-probed for the globless case specifically, because every one of this repo's 17 rule files
carries paths: and so provides no evidence either way.

If it fails, the premise is wrong and this should be reverted rather than worked around. A
scratch repo is set up at
/private/tmp/claude-501/-Users-edobry-Projects-minsky/c14a6eab-b66d-4761-92d7-ba30bc9ad394/scratchpad/sb5/repo
if you want to point a session at it.

Deploy verification:

Five of six changed files return true from isDeploySurfaceFile (the doc does not), so no
[no-deploy-impact] claim is made
and §10 applies: after merge, deployment_wait-for-latest for
minsky-mcp and reviewer with notBefore = the merge timestamp and expectCommitSha = the merge
SHA, health identity asserted from the response body, and build identity resolved by correlating
both deploy workflow runs to the merge SHA when it returns indeterminate (both are image-source
services, so it will). No new external-system integration.

Judgment calls

  • Proceeded despite the PR feat(mt#3854): Make .codex a compile output so the harness config stops fossilizing #3253 collision on compile.ts, third time in this chain. Its branch
    predates mt#4866 AND mt#4986, so it must rebase over both rewrites of those functions regardless;
    this task gates mt#573 and the first external user. Files clear of it carry most of the work.
  • The default is the NEW behaviour. ownership absent means "absent", so a caller that forgets
    to probe stops creating CLAUDE.md rather than silently keeping the old behaviour. That is what
    made ~20 existing tests fail — each was asserting the old contract, and each now says which case
    it means.
  • The legacy rules/compile/targets/claude-rules.ts twin is deliberately unchanged. Its only
    consumers are on the retired rules compile path, and mt#2996 deletes it; widening it would grow
    the blast radius for no live consumer.

Task: mt#5003 · Planning audit, the corrected duplicate-check record, and SC7's measured basis are
on the task record.

… stop creating a CLAUDE.md

mt#4986 stopped Minsky destroying a project's own CLAUDE.md and deliberately left a
gap: `.claude/rules/` admitted only glob-scoped rules, so a project whose CLAUDE.md we
do not own received ZERO always-apply rules. ask#11711 chose the channel — deliver them
through `.claude/rules/` as `paths`-less files.

The harness contract, read at source (code.claude.com/docs/en/memory, 2026-09-05):
"Rules without a `paths` field are loaded unconditionally and apply to all files", and
"loaded at launch with the same priority as `.claude/CLAUDE.md`". The near-miss is
`paths: []` — an empty list is still a `paths` FIELD, so it is path-scoped and matches
nothing; the harness keys on the field's ABSENCE, and the serializer omits the whole
frontmatter block.

The two channels are mutually exclusive, keyed on one predicate (`claudeMdIsOurs`).
Without that, widening the eligibility arm would have emitted THIS repository's 138,167-char
always-apply corpus into `.claude/rules/` as well as CLAUDE.md — roughly doubling its
always-loaded context and blowing the size budget from the other side. Measured after:
`ruleContentChars` unchanged at 138,167, `.claude/rules/` still exactly 17 files.

`claude.md` is now selected only when a CLAUDE.md we generated already exists: we
MAINTAIN ours, we do not CREATE one. An operator can still opt in once with
`--target claude.md`, after which the file carries the banner and stays maintained —
the same affordance the AGENTS.md harness gate already offers.

Two things this turned up that the spec did not carry:

The skip message became FALSE. It told the operator "nothing loads them into your agent
automatically … ask by name with `rules_get`" — accurate under mt#4986, and wrong the
moment `.claude/rules/` started carrying the set. It is now per-file: CLAUDE.md gets the
reassuring truth, AGENTS.md keeps the honest answer, because `.claude/rules/` is
Claude-Code-only and Claude Code reads CLAUDE.md, not AGENTS.md.

`foreignOutputs` and `ownedOutputs` as separate booleans made `{foreign, owned}`
representable — a state that cannot exist, whose resolution depended on which check ran
first. Collapsed into one `ownership` tri-state per file, so the illegal state is
unrepresentable rather than merely unreachable.

Parity with the legacy pipeline is preserved by the default: `includeAlwaysApply`
defaults false, so `scripts/verify-compile-parity.ts` compares like for like and still
passes all 5 checks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012aTeW8XXjVuEF5GykykSwq
@minsky-reviewer

minsky-reviewer Bot commented Sep 5, 2026

Copy link
Copy Markdown

Minsky Reviewer Status

Review in progress...

@minsky-ai minsky-ai Bot added the authorship/co-authored Co-authored by human and AI agent label Sep 5, 2026

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Solid, coherent implementation that closes the mt#4986 gap by routing always-apply rules to .claude/rules/ as paths-less files and gating claude.md on ownership. The change is thoughtfully propagated across compile probing, init, the pre-commit mirror, and per-file operator messages; tests cover key invariants, including mutual exclusion and banner placement. Docs were updated to reflect the new behavior. I found a few non-blocking concerns: introduce of a new "absent" gate kind may require exhaustiveness checks in downstream consumers; small duplicated IO probes for CLAUDE.md ownership; banner string duplication across modules; and a potential race window between probe and compile ownership decisions. None are blockers. Overall, this is ready to merge.

Findings

  • [NON-BLOCKING] packages/domain/src/compile/compile.ts:33 — New MinskyCompileGatedTarget.kind variant "absent" may be unhandled by existing consumers
    You added a third gate kind ("absent") to MinskyCompileGatedTarget (packages/domain/src/compile/compile.ts:33-41). While runMinskyCompile only acts on "foreign" entries (packages/domain/src/compile/compile.ts:197-214) and so is safe, any other consumer with an exhaustive switch over kind could now fall through. Please audit downstream uses (UI surfaces, adapters) for exhaustive handling and extend them to treat "absent" appropriately (likely informational-only).
  • [NON-BLOCKING] packages/domain/src/compile/targets/claude-rules.ts:186 — Potential double filesystem probe for CLAUDE.md ownership per run
    Both listOutputFiles (packages/domain/src/compile/targets/claude-rules.ts:186-190) and compile (packages/domain/src/compile/targets/claude-rules.ts:206-214) call claudeMdIsOurs to derive includeAlwaysApply. This duplicates an IO check in the same run. Consider computing once and threading it, or caching inside claudeMdIsOurs, to avoid redundant reads. Not blocking, but worth a small cleanup.
  • [NON-BLOCKING] packages/domain/src/compile/targets/claude-rules.ts:57 — Banner string duplicated across modules — risk of divergence
    CLAUDE_RULES_BANNER is hardcoded here (packages/domain/src/compile/targets/claude-rules.ts:57-62) and comments mention identical literals in legacy crud-operations.ts and src/hooks/pre-commit.ts. This duplication predates this PR but the added usage increases drift risk. Consider centralizing the banner constant in a shared module to enforce consistency. [Pattern sweep note: this file, crud-operations.ts, and src/hooks/pre-commit.ts all maintain their own copies.]
  • [NON-BLOCKING] packages/domain/src/compile/compile.ts:170 — Race window between target probing and compilation decisions for always-apply routing
    probeMinskyCompileTargetsWithGateReport/minskyCompileTargetsWithGateReport make decisions based on readMonolithicOwnership, and claude-rules also decides includeAlwaysApply using claudeMdIsOurs. If CLAUDE.md is created/removed between probe and compile (e.g., external actor or concurrent run), --check and compile could disagree, causing transient orphan reports. Not blocking but worth noting; consider memoizing per-run ownership to keep decisions consistent across the run.
  • [NON-BLOCKING] packages/domain/src/compile/monolithic-ownership.ts:203 — Minor wording inconsistency in operator message
    foreignOutputSkipReason now tells CLAUDE.md owners that always-apply rules go to .claude/rules/ as "paths-less" files (packages/domain/src/compile/monolithic-ownership.ts:203-215). Elsewhere comments consistently use backticked paths-less. Consider standardizing the phrasing for clarity. Cosmetic only.

Spec verification

Criterion Status Evidence
SC1 — A rule with alwaysApply: true is emitted to .claude/rules/<id>.md with no paths key in its frontmatter at all. Verified by reading the emitted file's first lines. Met packages/domain/src/compile/targets/claude-rules.ts:115-131serializeRuleToClaudeRule returns only the banner and content when rule.alwaysApply === true, omitting any frontmatter. Tests at packages/domain/src/compile/targets/claude-rules.test.ts:112-133 assert not.toContain("paths:") and banner at line 1.
SC2 — The generation banner still lands within the first 5 lines of the emitted file; the edit guard denies edits. Met serializeRuleToClaudeRule places the banner as the first line for always-apply rules (packages/domain/src/compile/targets/claude-rules.ts:123-131). Test keeps the banner inside the five-line window... at packages/domain/src/compile/targets/claude-rules.test.ts:134-146 verifies placement. The guard itself isn’t executed here, but the placement satisfies its 5-line scan contract referenced in code comments.
SC3 — A project with no CLAUDE.md no longer gets one created; base rules go to .claude/rules/. Met packages/domain/src/compile/compile.ts:158-169claude.md is added only when ownership.claudeMd === "generated"; otherwise a gatedOut entry with kind "absent" is produced, and .claude/rules target is always included (packages/domain/src/compile/compile.ts:184-190). init mirrors this by selecting only "claude-rules" when claudeMdIsOurs is false (packages/domain/src/init.ts:327-340). Tests at packages/domain/src/compile/compile.test.ts:149-170 and :492-514 cover the absent case.
SC4 — This repository is unaffected: the always-apply corpus is not duplicated into .claude/rules/. Met Mutual exclusion: claudeRulesTarget.listOutputFiles and compile both compute includeAlwaysApply = !(await claudeMdIsOurs(...)) (packages/domain/src/compile/targets/claude-rules.ts:186-190, 206-214). In this repo, CLAUDE.md is generated, so always-apply rules are excluded from .claude/rules. Test listOutputFiles EXCLUDES an always-apply rule when a CLAUDE.md of ours carries it at packages/domain/src/compile/targets/claude-rules.test.ts:226-239 enforces this. The compile tests continue to expect claude.md selected when owned (packages/domain/src/compile/compile.test.ts:129-141).
SC5 — init's reachability accounting reports base rules as REACHED, not "not reachable by Claude Code". Met foreignOutputSkipReason was split per-file; for CLAUDE.md it now states delivery via .claude/rules/ (packages/domain/src/compile/monolithic-ownership.ts:203-215). initializeProject compiles claude-rules even when CLAUDE.md is absent (packages/domain/src/init.ts:327-340), so the base rules are included and not reported unreachable. Tests in packages/domain/src/compile/monolithic-ownership.test.ts:152-171 validate the revised message semantics.
SC6 — docs/rules/template-system-guide.md’s paragraph is amended to reflect the new behavior. Met docs/rules/template-system-guide.md:117-153 — prior paragraph replaced; now documents always-apply rules delivered via .claude/rules/ without paths, notes historical window, and explains no automatic channel for AGENTS.md.
SC7 — The four deliberately on-demand operational-reference rules are not forced into the always-loaded channel. Met Eligibility keys on alwaysApply and the always-apply arm is opt-in via includeAlwaysApply (packages/domain/src/compile/targets/claude-rules.ts:86-106). Tests at packages/domain/src/compile/targets/claude-rules.test.ts:104-111 assert on-demand (no alwaysApply key, no globs) stays ineligible even with includeAlwaysApply: true.

Documentation impact

  • updated-in-pr — The PR amends docs/rules/template-system-guide.md to document the new delivery channel for always-apply rules via .claude/rules/ (no paths frontmatter) and clarifies AGENTS.md remains unchanged. The doc previously asserted no automatic channel; that paragraph was replaced (docs/rules/template-system-guide.md:117-153). No other docs appear to reference this specific behavior.
    Affected: docs/rules/template-system-guide.md

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Verification round: prior non-blocking findings stand; no blocking issues introduced. The fixes implement mutual exclusion between CLAUDE.md and .claude/rules/ for always-apply rules, add the absent gate with clear operator messaging, update init and the pre-commit mirror, and adjust tests and docs. I rechecked key sites: serializeRuleToClaudeRule now omits frontmatter for always-apply, banner placement is preserved; minskyCompileTargetsWithGateReport and probe… compute ownership via tri-state and select claude.md only when ours; claude-rules sets includeAlwaysApply based on claudeMdIsOurs; docs updated. No critical regressions found. Remaining notes are minor (duplicate ownership probes; banner constant duplication; potential probe/compile race; audit consumers for new "absent" kind; minor phrasing). Overall, this iteration is ready to merge.

Findings

  • [NON-BLOCKING] packages/domain/src/compile/targets/claude-rules.ts:191 — Redundant per-run ownership probe of CLAUDE.md in both listOutputFiles() and compile()
    Both listOutputFiles (packages/domain/src/compile/targets/claude-rules.ts:191-199) and compile (packages/domain/src/compile/targets/claude-rules.ts:221-235) invoke claudeMdIsOurs(workspacePath, fs) to set includeAlwaysApply. This duplicates an IO read (banner scan) within the same run. Consider computing once and threading includeAlwaysApply through, or caching within claudeMdIsOurs, to avoid redundant filesystem reads. Not a blocker; correctness is unaffected.
  • [NON-BLOCKING] packages/domain/src/compile/compile.ts:33 — New MinskyCompileGatedTarget.kind variant "absent" may be unhandled by external consumers
    MinskyCompileGatedTarget.kind gained a third value "absent" (packages/domain/src/compile/compile.ts:33-41). Internal code handles it (e.g., only foreign is surfaced into skippedForeignOutputs at packages/domain/src/compile/compile.ts:217-232), but any downstream adapter or UI with an exhaustive switch over kind may now fall through. Please audit external consumers and update handling/documentation to include "absent" where appropriate.
  • [NON-BLOCKING] packages/domain/src/compile/targets/claude-rules.ts:54 — Banner string duplicated across modules increases drift risk
    CLAUDE_RULES_BANNER is hardcoded here (packages/domain/src/compile/targets/claude-rules.ts:53-62). Comments also note identical literals in the legacy twin and the pre-commit hook. Consider centralizing the banner text in a shared constants module to ensure consistent updates across all generators and guards. This predates the PR but is touched here.
  • [NON-BLOCKING] packages/domain/src/compile/compile.ts:170 — Potential race between probe-time ownership and compile-time includeAlwaysApply decision
    Ownership (readMonolithicOwnership/claudeMdIsOurs) is read in the probe (probeMinskyCompileTargetsWithGateReport) and again inside the claude-rules target to set includeAlwaysApply. If CLAUDE.md is created/removed between these reads (e.g., concurrent runs or external edits), --check may disagree with compile output leading to transient orphan/stale reports. Non-blocking note; consider memoizing ownership per run to align decisions.
  • [NON-BLOCKING] packages/domain/src/compile/monolithic-ownership.ts:206 — Minor phrasing inconsistency: "paths-less" vs backticked paths-less
    foreignOutputSkipReason’s CLAUDE.md branch uses the phrase "paths-less" (packages/domain/src/compile/monolithic-ownership.ts:200-214). Elsewhere code/tests refer to backticked paths-less. Consider standardizing terminology for clarity in operator messages. Cosmetic only.

Spec verification

Criterion Status Evidence
SC1 — A rule with alwaysApply: true is emitted to .claude/rules/<id>.md with no paths key in its frontmatter at all. Verified by reading the emitted file's first lines. Met packages/domain/src/compile/targets/claude-rules.ts:129-146 — serializeRuleToClaudeRule short-circuits for rule.alwaysApply === true and returns only the banner + content, no frontmatter. Tests at packages/domain/src/compile/targets/claude-rules.test.ts:113-136 assert no paths: and correct content.
SC2 — The generation banner still lands within the first 5 lines of the emitted file, so .minsky/hooks/check-generated-file-edit.ts still recognises it and claude-rules.ts's banner-keyed orphan removal still owns it. Verified by piping the emitted path through that hook and observing a deny. Met packages/domain/src/compile/targets/claude-rules.ts:140-146 places the banner at line 1 for always-apply rules. Test at packages/domain/src/compile/targets/claude-rules.test.ts:137-157 verifies the banner appears within the first five lines and at line 1 exactly.
SC3 — A project with no CLAUDE.md no longer gets one created. init and a bare compile write the base rules to .claude/rules/ instead. Met packages/domain/src/compile/compile.ts:201-214 — claude.md added only for ownership.claudeMd === "generated"; otherwise an absent gate is recorded. packages/domain/src/init.ts:327-342 — initTargets computed via claudeMdIsOurs, selecting only "claude-rules" when false. Tests in packages/domain/src/compile/compile.test.ts:149-170, 492-514, and new case at 129-149 cover the absent case.
SC4 — This repository is unaffected: its 138,167-char always-apply corpus is NOT duplicated into .claude/rules/. Met packages/domain/src/compile/targets/claude-rules.ts:208-235 — both listOutputFiles and compile compute includeAlwaysApply = !(await claudeMdIsOurs(...)), excluding always-apply rules here when CLAUDE.md is ours. Test at packages/domain/src/compile/targets/claude-rules.test.ts:311-327 asserts exclusion when a banner-carrying CLAUDE.md exists.
SC5 — init's reachability accounting reports the base rules as REACHED, not as "not reachable by Claude Code". Met packages/domain/src/init.ts:327-342 compiles claude-rules unconditionally even when CLAUDE.md is absent, so base rules are included. The skip message text for foreign monoliths was updated per-file: packages/domain/src/compile/monolithic-ownership.ts:178-215 now tells CLAUDE.md owners their rules reach via .claude/rules/, avoiding the prior false claim.
SC6 — docs/rules/template-system-guide.md’s section is amended: its paragraph beginning “What it costs you today, stated plainly” must be corrected to reflect the new behavior. Met docs/rules/template-system-guide.md:117-153 — old paragraph removed; new text explains always-apply rules are delivered as paths-less files under .claude/rules/, notes historical window (mt#4986→mt#5003), and clarifies AGENTS.md remains unchanged.
SC7 — The four deliberately on-demand operational-reference rules are not forced into the always-loaded channel as a side effect. Met packages/domain/src/compile/targets/claude-rules.ts:86-126 — isEligibleForClaudeRules keys on alwaysApply and only admits always-apply or glob-scoped false rules. Tests at packages/domain/src/compile/targets/claude-rules.test.ts:104-112 assert rules with no alwaysApply key and no globs remain ineligible under both arms.

Adoption sweep

Symbol Kind Consumers found Classification Notes
claudeMdIsOurs function packages/domain/src/compile/targets/claude-rules.ts:191 — used to set includeAlwaysApply in listOutputFiles(), packages/domain/src/compile/targets/claude-rules.ts:221 — used to set includeAlwaysApply in compile(), packages/domain/src/init.ts:332 — used to decide initTargets (whether to compile claude.md) Adopted New export introduced in monolithic-ownership.ts; immediately consumed by claude-rules target and init to enforce mutual-exclusion routing for always-apply rules.

Documentation impact

  • updated-in-pr — This PR changes documented behavior for delivery of always-apply rules and updates the relevant doc accordingly. The paragraph in docs/rules/template-system-guide.md that previously claimed no automatic channel was replaced to describe .claude/rules/ with no paths frontmatter and notes the historical window (mt#4986→mt#5003). I verified the updated section at docs/rules/template-system-guide.md:117-153. No other docs appear to assert the old behavior.
    Affected: docs/rules/template-system-guide.md

… real disk

CI failed 4 init tests that passed locally, and the cause was mine: `init.ts` is
driven by an injected `FsLike`, and I called `claudeMdIsOurs(repoPath)` with no
filesystem — so it read the REAL disk while the rest of `initializeProject` ran on
a mock.

That made the tests depend on machine state. `/tmp/test-repo/CLAUDE.md` exists on
my machine, left by a run on Sep 2, and it carries the generation banner — so the
predicate returned true locally and false in CI. My local green was that stale
file, not evidence. Confirmed by moving it aside and reproducing all four failures
locally before changing anything.

`readMonolithicOwnership` and friends now take `MonolithicOwnershipFs` — just
`readFile` — which both `MinskyCompileFsDeps` and `FsLike` satisfy structurally, so
every caller injects the filesystem it already holds. Narrowing the dependency to
what the module actually needs is what makes it injectable from `init` at all.

Three init tests then needed updating, and they are real behaviour changes rather
than fixture noise: on a fresh project `init` now compiles `claude-rules` ALONE, so
a stub that branched on `target === "claude.md"` was describing a call that no
longer happens. The two mt#4986 SC2 tests are re-pointed at the target `init`
actually runs — they pin the REPORTING MECHANISM (a compiled target returning
`skippedForeignOutputs` is surfaced, before the unreachability warning), which is
what they were always for.

Verified against the full suite CI runs, not the change-scoped gated one that
missed this: 17,706 pass / 0 fail across 1,143 files, with the stale file removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012aTeW8XXjVuEF5GykykSwq

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


The fix commit correctly routes ownership checks through the injected filesystem, eliminating real-disk dependencies in init tests, and introduces a minimal read-only fs interface to support that seam. The mutual-exclusion gating via claudeMdIsOurs is applied consistently in both the claude-rules target and initializeProject, preventing duplication of always-apply rules while ensuring fresh projects receive them via .claude/rules/. Operator-facing messages and the docs were updated to reflect the new behavior. I found no blocking issues; two minor non-blocking notes concern fs-interface proliferation and target-enumeration parity in other callers. Overall, success criteria SC1–SC7 are met and the PR is ready to merge.

Findings

  • [NON-BLOCKING] packages/domain/src/compile/monolithic-ownership.ts:38 — New narrow fs interface MonolithicOwnershipFs duplicates subset of existing fs types
    You introduced MonolithicOwnershipFs with a single readFile method to decouple tests from real fs, which resolves the CI issue. This partially overlaps with existing abstractions: MinskyCompileFsDeps (compile layer) and FsLike (init layer). The choice is justified in comments, but consider co-locating or aliasing this minimal interface (e.g., export a shared ReadFileOnlyFs) to avoid a proliferation of near-duplicate fs shapes across modules. Not blocking, since the structural narrowing is intentional and safe.
  • [NON-BLOCKING] packages/domain/src/init-backend-selection.test.ts:257 — Updated tests assume only claude-rules compiles on fresh init; ensure parity with compile selection codepaths
    The tests now expect only claude-rules to compile during init when no generated CLAUDE.md exists. This matches initializeProject’s new initTargets logic. Ensure any other code paths that enumerate targets (e.g., CLI commands, pre-commit hooks) applied the same ownership predicate so there is no drift between init and standalone compile. From this diff, claude-rules and claude.md gating appears consistent, but keep an eye on other callers that may have hardcoded both targets (PRE-EXISTING risk if any remain).

Spec verification

Criterion Status Evidence
SC1 — A rule with alwaysApply: true is emitted to .claude/rules/<id>.md with no paths key in its frontmatter at all. Met packages/domain/src/compile/targets/claude-rules.ts:78-110isEligibleForClaudeRules admits alwaysApply===true when includeAlwaysApply is set, and serializeRuleToClaudeRule returns only the banner+content for alwaysApply===true, omitting any frontmatter entirely.
SC2 — The generation banner still lands within the first 5 lines of the emitted file, so the edit-guard and orphan sweep still recognize it. Met packages/domain/src/compile/targets/claude-rules.ts:97-110 — for always-apply, serializeRuleToClaudeRule emits the banner at line 1 followed by a blank line and content; packages/domain/src/compile/monolithic-ownership.ts:51-66 scans first 5 lines for the banner; stale-file removal in claude-rules.ts:196-228 also keys on the same banner string.
SC3 — A project with no CLAUDE.md no longer gets one created; init/compile write base rules to .claude/rules/ instead. Met packages/domain/src/init.ts:176-205 computes initTargets from claudeMdIsOurs(repoPath, fileSystem) and only includes "claude.md" when it already exists as generated; otherwise it compiles only "claude-rules". The predicate itself is implemented in packages/domain/src/compile/monolithic-ownership.ts:96-114.
SC4 — This repository’s 138,167-char always-apply corpus is NOT duplicated into .claude/rules/. Met packages/domain/src/compile/targets/claude-rules.ts:163-177 sets includeAlwaysApply = !(await claudeMdIsOurs(workspacePath, fs)); in this repo CLAUDE.md is generated, so includeAlwaysApply is false and emission is unchanged. The same gating applies in listOutputFiles at :145-156, keeping --check consistent.
SC5 — init’s reachability accounting reports base rules as reached, not as “not reachable by Claude Code”. Met packages/domain/src/init.ts:206-272 — with only "claude-rules" compiled on a fresh project and claude-rules now including always-apply when no generated CLAUDE.md exists, reachable is populated from definitionsIncluded and no “not reachable” warning is emitted when all are included. Test adjustments at packages/domain/src/init-backend-selection.test.ts:255-268,449-489 reflect this behavior.
SC6 — docs/rules/template-system-guide.md is amended to remove the now-false claim and describe the new behavior. Met docs/rules/template-system-guide.md:86-140 — section updated to state that always-apply rules go to .claude/rules/ as paths-less files loaded at launch, with a note that earlier revisions said the opposite between mt#4986 and mt#5003.
SC7 — The four deliberately on-demand operational-reference rules are not forced into an always-loaded channel as a side effect. Met packages/domain/src/compile/targets/claude-rules.ts:56-76 — eligibility keys strictly on rule.alwaysApply === true (with includeAlwaysApply) OR non-empty globs with alwaysApply === false. Rules lacking an alwaysApply key are ineligible under both arms, preserving on-demand status as required by mt#3107.

Documentation impact

  • updated-in-pr — The PR updates docs to reflect the new always-apply delivery path. docs/rules/template-system-guide.md now explains that always-apply rules are delivered via .claude/rules/ as paths-less files and notes the prior behavior window. This prevents invalidation of earlier statements and aligns operator messages with actual behavior.
    Affected: docs/rules/template-system-guide.md

@edobry
edobry merged commit 63cfbda into main Sep 5, 2026
20 checks passed
@edobry
edobry deleted the task/mt-5003 branch September 5, 2026 02:58

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Re-verification complete. The fix commit addresses the earlier round’s feedback and cleanly implements mt#5003: always-apply rules are delivered via .claude/rules/ as paths-less files when no generated CLAUDE.md exists, and claude.md/.claude/rules/ are now mutually exclusive via claudeMdIsOurs. Target selection, pre-commit freshness checks, and init-path compilation all apply the same ownership predicate using the injected fs, avoiding real-disk coupling. Docs were updated to remove the now-false claim and reflect the new behavior. I found no new blocking defects; one minor non-blocking note suggests clarifying the absent-case skip message for non-Claude harnesses. Overall, success criteria SC1–SC7 are met and the PR is ready to merge.

Findings

  • [NON-BLOCKING] packages/domain/src/compile/compile.ts:206 — Absent-case skip message assumes current harness loads .claude/rules/
    absentGate()'s reason says the always-apply rules are delivered through .claude/rules/ "which this harness loads at launch". That is accurate for Claude Code, but misleading for non-claude harnesses (e.g., Codex) which do not load .claude/rules/. Consider softening the wording (e.g., "which Claude Code loads at launch") or branching the message on harness to avoid suggesting the active harness will auto-load these files.

Spec verification

Criterion Status Evidence
SC1 — A rule with alwaysApply: true is emitted to .claude/rules/<id>.md with no paths key in its frontmatter at all. Met packages/domain/src/compile/targets/claude-rules.ts:129-146 — serializeRuleToClaudeRule() returns only the banner and content when rule.alwaysApply === true, with no frontmatter block. Tests assert absence of paths: (packages/domain/src/compile/targets/claude-rules.test.ts:113-124).
SC2 — The generation banner still lands within the first 5 lines of the emitted file, so .minsky/hooks/check-generated-file-edit.ts still recognizes it and claude-rules.ts's banner-keyed orphan removal still owns it. Met packages/domain/src/compile/targets/claude-rules.ts:139-146 — banner is emitted at line 1 for always-apply; tests check banner is in the first line/five-line window (packages/domain/src/compile/targets/claude-rules.test.ts:126-138).
SC3 — A project with no CLAUDE.md no longer gets one created. init and a bare compile write the base rules to .claude/rules/ instead. Met packages/domain/src/compile/compile.ts:201-217 — minskyCompileTargetsWithGateReport adds claude.md only when ownership.claudeMd === "generated"; otherwise it records an absent gate. packages/domain/src/init.ts:327-345 — initializeProject computes initTargets via claudeMdIsOurs(repoPath, fileSystem) and compiles only "claude-rules" when absent/foreign. Tests cover both probe and mapping cases (packages/domain/src/compile/compile.test.ts:166-186, 486-505).
SC4 — This repository is unaffected: its 138,167-char always-apply corpus is NOT duplicated into .claude/rules/. Met packages/domain/src/compile/targets/claude-rules.ts:206-216,228-238 — both listOutputFiles and compile use includeAlwaysApply = !(await claudeMdIsOurs(workspacePath, fs)), which is false in this repo (generated CLAUDE.md), preventing duplication. End-to-end test asserts exclusion when a banner-carrying CLAUDE.md exists (packages/domain/src/compile/targets/claude-rules.test.ts:215-226).
SC5 — init's reachability accounting reports the base rules as REACHED, not as "not reachable by Claude Code". Met packages/domain/src/init.ts:327-345 compiles only claude-rules on fresh projects; with always-apply included there, all base rules appear in definitionsIncluded, so no unreachability warning is emitted. Tests updated to assert no warnings when all rules reach the single target (packages/domain/src/init-backend-selection.test.ts:449-457).
SC6 — docs/rules/template-system-guide.md is amended to remove the now-false claim and describe the new behavior. Met docs/rules/template-system-guide.md:117-151 — replaces the prior "no automatic channel" paragraph with text stating always-apply rules are written to .claude/rules/ as paths-less files loaded at launch, and adds a historical note about the prior behavior window.
SC7 — The four deliberately on-demand operational-reference rules are not forced into the always-loaded channel as a side effect. Met packages/domain/src/compile/targets/claude-rules.ts:70-86 — isEligibleForClaudeRules() admits only alwaysApply===true (optionally) or non-empty globs with alwaysApply===false. Rules lacking an alwaysApply key remain ineligible under both arms, preserving on-demand status as required. Tests assert this explicitly (packages/domain/src/compile/targets/claude-rules.test.ts:101-111).

Adoption sweep

Symbol Kind Consumers found Classification Notes
packages/domain/src/compile/monolithic-ownership.ts#claudeMdIsOurs function packages/domain/src/compile/targets/claude-rules.ts:205 — used to gate includeAlwaysApply in listOutputFiles/compile, packages/domain/src/compile/compile.ts:214 — used to gate claude.md target selection and absent/foreign handling, packages/domain/src/init.ts:330 — used to compute initTargets under injected fs Adopted New helper exported to keep claude.md vs .claude/rules channels mutually exclusive; wired into all relevant callers.

Documentation impact

  • updated-in-pr — The PR amends docs/rules/template-system-guide.md (lines ~117–151) to document that always-apply rules are delivered via .claude/rules/ as paths-less files and clarifies that CLAUDE.md is not created by default. This replaces a previously-true-but-now-false paragraph and aligns operator expectations with the new behavior.
    Affected: docs/rules/template-system-guide.md

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

Labels

authorship/co-authored Co-authored by human and AI agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant