diff --git a/CHANGELOG.md b/CHANGELOG.md index 4af7791..02f7324 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ All notable changes to TangleClaw are documented in this file. ### Fixed +- **CLAUDE.md regeneration no longer silently clobbers PR-driven rule edits (closes #240)** — `data/global-rules.md` (renamed from `data/default-global-rules.md` and populated with the full 191-line ruleset) becomes the single canonical source for global rules. `store.globalRules.load/save` read/write the tracked file directly; the legacy `~/.tangleclaw/global-rules.md` is ignored with a one-time warn + auto-backup to `*.pre-240-backup`. New `engines.writeEngineConfig` helper wraps every engine-config write with drift detection — warns loudly when the on-disk file differs from regenerated content, surfacing the silent-clobber failure mode at all four write sites (`createProject`, `syncAllProjects`, `updateProject` engine-switch, `launchSession`). Comparator normalizes CRLF→LF + trims so Windows editors and auto-formatters don't false-positive. Helper returns `{skipped: true, ...}` for engines without config files (openclaw, genesis) so callers don't surface spurious errors. **Going forward:** PR-driven rule changes edit `data/global-rules.md` and commit; UI/API edits land in the same file. The competing-sources-of-truth class of bug is structurally impossible. Reset button in the Global Rules editor is now a no-op pending UI cleanup in #243. Full suite 2463/2464 pass. + - **Recover global rules from #234 + #236 silently clobbered by CLAUDE.md regeneration (#240)** — `lib/engines.js#_generateClaudeMd` regenerates `CLAUDE.md` from the DB-stored global rules on every session launch / engine PATCH / startup sync, overwriting the on-disk file in place. PR-driven raw-file edits to `CLAUDE.md` (e.g. via `git`) are silently discarded the next time TC restarts — the content lives on in `git log` but is functionally absent from the live ruleset. Two PRs this session were affected: **#234** (the versioning bump-level decision table) and **#236** (the stale-plan archive rule + `gh issue view --json state` issue-state check). Both rule blocks were clobbered after the first TC restart following each merge; the celebrated "structural lessons codified in CLAUDE.md" claim in those PR descriptions was technically true in git history and functionally false in the running ruleset. **Recovery procedure run this session:** read the live DB content via `store.globalRules.load()`, splice the two missing rule blocks back into their respective sections (`Build Plans & Chunks` for #236, `Releases & Versioning` for #234) preserving sibling content, write back via `store.globalRules.save(merged)` (which normalizes and persists to `_globalRulesPath()`). Verified end-to-end: the regenerated `CLAUDE.md` is now byte-for-byte identical to HEAD (diff is empty), so the next TC restart produces no surprise drift. **Defensive comment added** to `lib/engines.js#_generateClaudeMd` documenting the regeneration contract and pointing future readers (or automated agents tempted to commit directly to `CLAUDE.md`) at the durable edit paths: landing-page editor, `PUT /api/rules/global` (10 KB body cap), or `store.globalRules.save()` from a node script. The same regeneration pattern applies to Codex / Aider / Gemini config files — same trap, less commonly hit because TC-v3 itself uses Claude. Public diagnostic + recovery procedure for any other cloner who is affected is documented in #240. Structural fix (preventing the trap from ever firing again — likely either a tracked-file canonical source with startup sync, or an append-only marker pattern that regeneration preserves) is tracked in #240 as separate work. ### Changed diff --git a/data/default-global-rules.md b/data/default-global-rules.md deleted file mode 100644 index 1381eef..0000000 --- a/data/default-global-rules.md +++ /dev/null @@ -1,11 +0,0 @@ -# Global Rules - -These rules apply to all projects managed by TangleClaw, across all engines. -Edit them from the TangleClaw landing page or via the API. - -## General - -- Follow the project's existing code style and conventions -- Prefer small, focused commits over large monolithic ones -- Keep functions short and single-purpose -- Write clear commit messages that explain why, not just what diff --git a/data/global-rules.md b/data/global-rules.md new file mode 100644 index 0000000..389c04f --- /dev/null +++ b/data/global-rules.md @@ -0,0 +1,190 @@ +# Global Rules + +These rules apply to all projects managed by TangleClaw, across all engines. +Edit them from the TangleClaw landing page or via the API. + +## General + +- Follow the project's existing code style and conventions +- Prefer small, focused commits over large monolithic ones +- Keep functions short and single-purpose +- Write clear commit messages that explain why, not just what + +## Build Plans & Chunks + +Claude Code stores plan files globally at `~/.claude/plans/`. This causes problems _ plans get lost across sessions and can collide between projects. + +**Rule: Keep plans local to the project.** +- After creating or updating a plan in Claude Code's plan mode, copy the plan file into the project directory at `/.claude/plans/.md`. +- All memory entries and session handoffs must reference the **project-local copy** using its absolute path (e.g., +`/Users/jasonvaughan/Documents/Projects/MyProject/.claude/plans/my-plan.md`). +- Never reference plans with ambiguous relative paths like `.claude/plans/...` _ always use absolute paths. +- Each project's plans live inside that project. Do not rely on `~/.claude/plans/` as the source of truth across sessions. + +**Rule: Archive plans whose chunk has shipped.** + +A plan file outlives its purpose the moment the corresponding chunk's PR merges. Leaving shipped plans next to active ones traps future sessions into treating closed work as ready-to-execute — exactly the failure mode that bit the 2026-05-23 session (recommended #137 as next chunk because `build-plan.md` was present at the repo root; the issue had been closed 18 days earlier). + +- When a plan's referenced issue closes (or its PR merges), **move the plan to `/.claude/plans/archive/`** rather than deleting. Archive preserves the design rationale for historical greppability without polluting the active plan listing. +- Sessions starting work on a "next chunk" must verify the referenced issue is still **OPEN** via `gh issue view --json state -q .state` before treating any plan as canonical, even if the file is in the active (non-archive) directory. This is the structural guard — archiving is the convention, the issue-state check is the contract. +- The `.claude/plans/` directory and `build-plan*.md` at the repo root are gitignored on TC; the archive is local-only. A fresh clone has no archive — only this clone benefits from the historical record. That's fine; the issue-state check is what protects across clones. + +## Memory Hygiene + +Memory entries that exist only to bridge a specific gap (e.g., "decisions ratified in chat that haven't yet landed in the canonical plan", "context for an in-progress +migration", "notes for an open incident") should not be allowed to outlive their purpose. They turn into stale canon and quietly mislead future sessions. + +**Rule: Bridge memos must self-delete.** + +When you create a bridging memory entry: +- Frontmatter `description` must explicitly state the file is **self-deleting** and reference the cleanup criterion. +- File body must include a prominent "**__ Auto-cleanup check**" section near the top with: + 1. A short, mechanical procedure to verify whether the bridging condition is still relevant (e.g., "read this file path, check whether these specific items appear in +it"). + 2. The action to take if the condition is resolved: `rm` the memory file AND prune its line from `MEMORY.md`. + 3. The action to take if the condition is still pending: leave the file alone, treat its contents as canonical until ratified. +- `MEMORY.md` index line should signal the self-deleting nature (e.g., "**Self-deleting**: read its top 'Auto-cleanup check' section every session_"). + +When you read a bridging memory entry at session start: +- Run the cleanup check before treating the file as canon. +- If the check says delete, delete the file and prune `MEMORY.md` before continuing. + +This prevents memory pollution and ensures bridging notes resolve cleanly into the permanent canonical docs (build plans, specs, ADRs) rather than accumulating as stale +background. + +## Priming Prompts + +When a project develops a recurring "session role" (e.g., advisor session for a parallel build, on-call review session, debugging session), the priming prompt for that role should be saved as a durable artifact, not kept in chat memory. + +**Rule: Save priming prompts as git-tracked files.** +- Path convention: `/.claude/priming/.md`. +- The file should contain the verbatim block to paste, plus a short "How to use" section. +- Update history at the bottom (date + what changed) so divergence over time is visible. + +## Cross-Session Write Boundaries + +When two Claude Code sessions are working on related but distinct repos (e.g., an advisor session and a builder session, or a coordinator and an executor), the sessions +should respect repo ownership. + +**Rule: Don't write to the other session's repo from yours.** +- The advisor / coordinator session should not commit to the builder / executor's repo, even when "it would be faster." +- Suggestions go via memo-back: produce a paste-able block the other session can apply, or write to a shared bridge memo (subject to the Memory Hygiene rule above). +- Both sessions can edit shared infrastructure (TangleClaw config, ports, etc.) since neither owns it. +- This prevents merge conflicts, surprises in the other session's git log, and ambiguity about who's responsible for which commit. + +## Issues & Feature Requests + +GitHub Issues are the canonical place to surface work that doesn't fit the current scope. They're searchable, citeable, assignable, and contribute to the project's public activity stats. + +**Rule: File issues for bugs, deferred features, and open questions — don't bury them in chat or memory files.** + +When the agent surfaces a bug, scope creep, deferred feature, or unresolved question that exceeds the current work item, suggest filing a GitHub issue before continuing. + +- Title format: `[type] subject` where type is `bug`, `feature`, `chore`, `docs`, `question`. +- Body covers: what / why / how-to-reproduce (for bugs) or expected behavior (for features). +- Use templates from `.github/ISSUE_TEMPLATE/` if the repo has them. +- Link issues from PRs and commits with `Fixes #N` / `Closes #N` syntax — auto-closes the issue on merge. +- For solo projects: even when you'll fix it yourself, file the issue first, close it via the PR. The history is the value. + +## Branches & Pull Requests + +Substantive work goes through a feature branch and a pull request, even for solo projects. The PR is documentation of why a change happened — it's not just process overhead. + +**Rule: Branch + PR for substantive work; direct main commits only for trivial doc edits or incident hot-fixes.** + +- Branch naming: `feat/`, `fix/`, `chore/`, `docs/`, `refactor/`. +- PR titles in active voice ("Add X", "Fix Y when Z", "Update W to vN.N"). +- PR body has "What", "Why", and "Test plan" sections; link to issues with `Fixes #N`. +- Delete branches after merge. + +**Rule: Pair `gh pr create` with `gh pr merge --auto --squash --delete-branch` for routine PRs.** + +Routine PRs are docs, chore, version bumps, dependency updates, and test-only changes. Once branch protection gates clear, GitHub merges the PR server-side — no waiting on the session. + +When opening a PR for routine work, immediately follow with: + +``` +gh pr merge --auto --squash --delete-branch +``` + +GitHub holds the PR and merges the instant required checks pass. + +**When to use `--auto`:** +- Documentation-only changes (README, CHANGELOG, MEMORY, plan files, comments, JSDoc). +- Chore PRs (dependency bumps, version updates, gitignore tweaks) where the change is mechanical and doesn't shift active methodology. +- Test-only PRs where the test code is the entire change. + +**When NOT to use `--auto`:** +- Feature PRs (anything user-facing or behavior-changing). +- Refactors with non-trivial code movement. +- Anything that triggered an Independent Critic review — wait until findings are addressed and the user has signed off. +- PRs touching CI workflows, deploy config, secrets, or branch protection rules. +- Any PR the user has explicitly said they want to review before merging. + +**Why `--squash`:** keeps `main` history linear and CHANGELOG-friendly. Matches the one-squash-commit-per-PR pattern most projects already use. + +**Safety note:** branch protection rules still gate. If `main` requires a review, `--auto` waits for that review before merging. This rule never *overrides* protection — it only *removes the wait* once gates clear. Safe to apply broadly. + +**If auto-merge isn't enabled on the repo:** `gh pr merge --auto` will error. Either enable it in repo Settings → General → Pull Requests, or fall back to `gh pr checks --watch` followed by a manual merge. + +## Releases & Versioning + +Substantive milestones become tagged releases on GitHub. Releases create permanent, citeable URLs and contribute to the project's public activity. + +**Rule: Tag releases with semver and create GitHub Releases with CHANGELOG-driven notes.** + +- Use semver: MAJOR for breaking changes, MINOR for new features, PATCH for bug fixes. Pre-1.0 projects can use 0.x freely. +- After a substantive merge, suggest tagging: `git tag -a vX.Y.Z -m "..." && git push --tags`. +- Create the GitHub release: `gh release create vX.Y.Z --notes-from-tag` (or `-F ` for hand-curated notes from CHANGELOG). +- Maintain a `CHANGELOG.md` in Keep a Changelog format with `[Unreleased]` section + dated release entries. Each merged PR adds to `[Unreleased]`; releases promote those entries to a dated section. + +**Rule: TC's `version-bump` wrap step picks the bump level from `[Unreleased]` content. Author CHANGELOG entries under the subsection that produces the intended bump.** + +| `[Unreleased]` content | Bump | +|---|---| +| `BREAKING:` or `BREAKING(` marker anywhere in body | **major** | +| Any `### Added`, `### Changed`, `### Removed`, or `### Deprecated` | **minor** | +| Only `### Fixed`, `### Security`, or `### Internal` | **patch** | + +Rows are evaluated **top-down with first-match-wins** — a body that contains both `### Added` AND `### Internal` matches the minor row (user-visible subsection wins; `### Internal` does not veto a real feature). The patch row only fires when no minor- or major-triggering content is present. + +`### Internal` is a non-Keep-a-Changelog subsection (introduced in #231) for refactors, test-only changes, dev tooling, CI tweaks, and doc-only edits that don't change user-facing behavior. Entries logged here still appear in `CHANGELOG.md` for full auditable history, but the wrap step treats them as patch-tier so a release made up entirely of internal churn doesn't inflate the minor counter. + +Pick the subsection by **user-visible impact**, not file footprint: a one-line behavior change for the user is `### Added` / `### Changed`; a 500-line refactor with zero user-visible effect is `### Internal`. When in doubt between `### Changed` and `### Internal`, ask "would an operator notice a difference next session?" — yes → `### Changed`, no → `### Internal`. + +## Repository Standards + +Every repo should look professional and ready for visitors — future-self, contributors, and hiring evaluators reviewing the user's portfolio. + +**Rule: Maintain a baseline of repository hygiene files; the agent suggests creating missing ones when the project's stage warrants.** + +Baseline: +- `README.md` — what / why / install / use / status +- `LICENSE` — even private repos benefit from explicit licensing for future open-sourcing +- `CHANGELOG.md` — Keep a Changelog format +- `.gitignore` — language defaults + project-specific (env files, build outputs, OS noise) +- `.github/ISSUE_TEMPLATE/bug.md` and `feature.md` — for projects expecting contributions +- `.github/PULL_REQUEST_TEMPLATE.md` — checklist + What/Why/Test-plan sections +- Branch protection rule on `main` — require status checks; require review when contributors arrive + +The agent suggests adding any of these only when it would add real value at the project's current stage (not boilerplate spam). A pre-public solo project doesn't need ISSUE_TEMPLATEs yet; a project being shown to a potential client does. + +## Contributor Readiness + +Once a project is shown to potential contributors, clients, or hiring evaluators, the agent should help the user flip from "solo project hygiene" to "open project hygiene." + +**Rule: Anticipate contributor readiness; flag gaps proactively when the project signals it's going public.** + +Triggers for the agent to do a readiness sweep: user says "this is going public" / "I'm bringing in a contributor" / "this is my dev-for-hire showcase" / "I want this on my portfolio" / project is about to be unprivated on GitHub. + +Readiness checklist the agent runs through: +- README explains setup clearly enough that a stranger can get the project running with no prior context. +- LICENSE is present and intentional (MIT, Apache 2.0, BSL, AGPL — match the project's commercial stance). +- CONTRIBUTING.md exists with: dev setup, branch/PR conventions, test requirements, code-style expectations, where to file issues. +- CODE_OF_CONDUCT.md if the project is open to public participation (Contributor Covenant is a sensible default). +- CI is green and visible (status badges in README link to the CI workflow). +- Recent activity is visible — no months-stale main branch. +- Issue + PR templates are present. +- Discoverable from the user's GitHub profile (pinned repo if it's a showcase; mentioned in the user's profile README if relevant). + +The agent should treat the user's GitHub profile as the user's public portfolio. Public commit activity, public repos, GitHub Releases, stars on others' projects, and contributions to others' repos all feed the user's profile stats and contribute to dev-for-hire credibility. Visibility is a feature, not a side-effect — when work crosses a milestone worth showing, suggest the visibility action. diff --git a/lib/engines.js b/lib/engines.js index 9161705..67a7ad4 100644 --- a/lib/engines.js +++ b/lib/engines.js @@ -1080,6 +1080,99 @@ function _mergeHookObjects(a, b) { * @param {string} projectPath - Absolute path to the project directory * @param {object|null} methodologyTemplate - Methodology template (null clears methodology hooks but keeps baseline) */ +/** + * Generate engine config AND write it to disk at the conventional path, + * with drift detection (#240). + * + * Before overwriting, compares the existing on-disk file against the + * would-be-generated content. When they differ in non-whitespace ways, + * logs a warning naming the file and byte deltas. This surfaces the + * silent-clobber bug class where someone (a contributor PR, a manual + * `vim CLAUDE.md`, an autoformatter) edits the file directly and their + * edit is about to be overwritten by regeneration. The warning lets the + * operator notice before the loss; without it, the overwrite is silent. + * + * The four pre-existing write sites (`launchSession`, `createProject`, + * `updateProject` engine-switch + methodology-switch) all funnel through + * this helper so the warning fires uniformly regardless of which code + * path triggers the regeneration. + * + * **Whitespace tolerance.** `.trim()` on both sides ignores trailing- + * newline differences (auto-formatters, editors). Drift is reported + * only when there's a real semantic change. + * + * **Permissive on read failure.** If the existing file is unreadable + * (permissions, transient FS error), the helper falls through and + * overwrites without warning. The write is the operation that matters; + * the warning is best-effort. + * + * @param {string} engineId - Engine identifier (claude / codex / aider / etc.) + * @param {string} projectPath - Absolute path to the project directory + * @param {object} projectConfig - Per-project config + * @param {object} engineProfile - Engine profile (needed for `configFormat.filename`) + * @param {object} [methodologyTemplate] - Methodology template (optional, passed through to generateConfig) + * **Return shape.** Distinguishes three outcomes the caller may need to + * react to differently: + * - `{written: true, ...}` — wrote successfully (with `drifted: true|false` + * indicating whether the on-disk version differed first). + * - `{written: false, skipped: true, skipReason: '', error: null}` — + * deliberate no-op (engine has no config file by design, e.g. `openclaw` + * or `genesis`; or `generateConfig` returned empty). Callers should + * NOT treat this as an error. Pre-refactor this was a silent + * `if (configContent && engineProfile.configFormat)` guard; the + * explicit field makes the contract visible. + * - `{written: false, skipped: false, error: ''}` — real + * write failure (permissions, ENOSPC, etc.). Callers should surface. + * + * @returns {{written: boolean, skipped: boolean, skipReason: string|null, drifted: boolean, configFilePath: string|null, error: string|null}} + */ +function writeEngineConfig(engineId, projectPath, projectConfig, engineProfile, methodologyTemplate) { + // Deliberate no-op: engine has no config file (openclaw, genesis). + // Per #240 PR Critic — silently skip so callers don't surface a + // spurious "failed to write engine config" error/warning every time + // a non-Claude/Codex/Aider/Gemini project is created or launched. + if (!engineProfile || !engineProfile.configFormat || !engineProfile.configFormat.filename) { + return { written: false, skipped: true, skipReason: 'engine has no config file (configFormat.filename is null)', drifted: false, configFilePath: null, error: null }; + } + const content = generateConfig(engineId, projectConfig, methodologyTemplate); + if (!content) { + return { written: false, skipped: true, skipReason: 'generateConfig returned empty (engine does not support config files for this project shape)', drifted: false, configFilePath: null, error: null }; + } + const configFilePath = path.join(projectPath, engineProfile.configFormat.filename); + let drifted = false; + if (fs.existsSync(configFilePath)) { + try { + const existing = fs.readFileSync(configFilePath, 'utf8'); + // Normalize line endings before comparing — Windows editors save + // CRLF, the regenerator emits LF. Without this normalization a + // Windows operator who never touched the file would see drift + // warnings on every session launch. Per #240 PR Critic. + const normalizedExisting = existing.replace(/\r\n/g, '\n').trim(); + const normalizedContent = content.replace(/\r\n/g, '\n').trim(); + if (normalizedExisting !== normalizedContent) { + drifted = true; + log.warn( + 'engine config drift detected — overwriting on-disk hand-edits (#240)', + { + configFilePath, + engineId, + existingBytes: existing.length, + regeneratedBytes: content.length, + howToInvestigate: 'diff the file against `git show HEAD:' + path.basename(configFilePath) + '`; if the on-disk content has rule additions you want to keep, save them via the landing-page Global Rules editor or edit data/global-rules.md and commit before next regeneration' + } + ); + } + } catch { /* unreadable existing file — fall through to overwrite */ } + } + try { + fs.mkdirSync(path.dirname(configFilePath), { recursive: true }); + fs.writeFileSync(configFilePath, content); + return { written: true, skipped: false, skipReason: null, drifted, configFilePath, error: null }; + } catch (err) { + return { written: false, skipped: false, skipReason: null, drifted, configFilePath, error: err.message }; + } +} + function syncEngineHooks(projectPath, methodologyTemplate) { const projConfig = store.projectConfig.load(projectPath); const settingsDir = path.join(projectPath, '.claude'); @@ -1158,6 +1251,7 @@ module.exports = { getWithAvailability, validateProfile, generateConfig, + writeEngineConfig, validateParity, validateStatusParity, _getRulesContent, diff --git a/lib/projects.js b/lib/projects.js index 001d5dc..3bd6bad 100644 --- a/lib/projects.js +++ b/lib/projects.js @@ -280,16 +280,19 @@ function createProject(data) { errors.push(`Failed to create session memory directory: ${err.message}`); } - // Generate engine config — methodologyTemplate is always non-null since #151 - const configContent = engines.generateConfig(engineId, projectConfig, methodologyTemplate); - if (configContent && engineProfile.configFormat) { - try { - const configFilePath = path.join(projectPath, engineProfile.configFormat.filename); - fs.mkdirSync(path.dirname(configFilePath), { recursive: true }); - fs.writeFileSync(configFilePath, configContent); - } catch (err) { - errors.push(`Failed to write engine config: ${err.message}`); - } + // Generate + write engine config via the #240 drift-aware helper. + // The helper logs a warning if the existing on-disk file differs + // from what we're about to write (surfaces silent-clobber bugs). + // For createProject the project directory is fresh, so drift should + // never fire here — but we route through the helper for uniformity + // with the other three write sites and so a future caller that + // writes into an existing directory automatically benefits. + // + // `skipped: true` is a deliberate no-op (engine has no config file — + // openclaw, genesis). Only surface real `error` strings. + const writeResult = engines.writeEngineConfig(engineId, projectPath, projectConfig, engineProfile, methodologyTemplate); + if (writeResult.error && !writeResult.written && !writeResult.skipped) { + errors.push(`Failed to write engine config: ${writeResult.error}`); } // Sync engine hooks to match methodology @@ -760,18 +763,16 @@ function syncAllProjects() { fs.writeFileSync(memoryFile, '# Session Memory\n\nThis file persists context across AI sessions. Update it with key decisions, progress, and open questions.\n'); } - // Regenerate engine config + // Regenerate engine config via the #240 drift-aware helper. This is + // the startup-sync path that, per the original #240 bug, silently + // clobbers PR-driven CLAUDE.md edits — the helper's warn fires here + // first, giving operators visibility BEFORE the overwrite lands. const projConfig = store.projectConfig.load(project.path); const engineId = projConfig.engine || project.engine || 'claude'; const engineProfile = store.engines.get(engineId); if (engineProfile && engineProfile.configFormat) { const methodologyTemplate = store.templates.get(projConfig.methodology || project.methodology); - const configContent = engines.generateConfig(engineId, projConfig, methodologyTemplate); - if (configContent) { - const configFilePath = path.join(project.path, engineProfile.configFormat.filename); - fs.mkdirSync(path.dirname(configFilePath), { recursive: true }); - fs.writeFileSync(configFilePath, configContent); - } + engines.writeEngineConfig(engineId, project.path, projConfig, engineProfile, methodologyTemplate); } synced++; @@ -1341,15 +1342,14 @@ function updateProject(name, updates) { store.projectConfig.save(project.path, projConfig); const methodologyTemplate = store.templates.get(project.methodology); - const configContent = engines.generateConfig(updates.engine, projConfig, methodologyTemplate); - if (configContent && engineProfile.configFormat) { - try { - const configFilePath = path.join(project.path, engineProfile.configFormat.filename); - fs.mkdirSync(path.dirname(configFilePath), { recursive: true }); - fs.writeFileSync(configFilePath, configContent); - } catch (err) { - errors.push(`Failed to write engine config: ${err.message}`); - } + // #240 drift-aware write — surfaces a warning when the on-disk + // engine config differs from what we're about to write. Catches + // operator hand-edits being lost during engine switches. `skipped` + // (deliberate no-op for engines without config files) is NOT an + // error and must not be pushed. + const writeResult = engines.writeEngineConfig(updates.engine, project.path, projConfig, engineProfile, methodologyTemplate); + if (writeResult.error && !writeResult.written && !writeResult.skipped) { + errors.push(`Failed to write engine config: ${writeResult.error}`); } // Re-sync hooks so an engine flip away from claude clears any orphan diff --git a/lib/sessions.js b/lib/sessions.js index a542883..e5e3999 100644 --- a/lib/sessions.js +++ b/lib/sessions.js @@ -186,17 +186,16 @@ function launchSession(projectName, options = {}) { log.warn('Failed to sync shared docs from group directories', { error: err.message }); } - // Regenerate engine config BEFORE launching (ensures engine reads current methodology) + // Regenerate engine config BEFORE launching (ensures engine reads + // current methodology). #240 drift-aware write — the helper warns + // when the on-disk file differs from regenerated content, surfacing + // silent-clobber bugs. The helper itself handles "engine has no + // config file" (openclaw, genesis) via `skipped: true`, so no + // outer guard is needed; we only surface real `error` strings. const methodologyTemplate = store.templates.get(project.methodology); - const configContent = engines.generateConfig(engineId, projConfig, methodologyTemplate); - if (configContent && engineProfile.configFormat) { - try { - const configFilePath = require('node:path').join(project.path, engineProfile.configFormat.filename); - require('node:fs').mkdirSync(require('node:path').dirname(configFilePath), { recursive: true }); - require('node:fs').writeFileSync(configFilePath, configContent); - } catch (err) { - log.warn('Failed to write engine config', { error: err.message }); - } + const writeResult = engines.writeEngineConfig(engineId, project.path, projConfig, engineProfile, methodologyTemplate); + if (writeResult.error && !writeResult.written && !writeResult.skipped) { + log.warn('Failed to write engine config', { error: writeResult.error }); } // Sync engine hooks to match methodology (before launch so hooks are current) diff --git a/lib/store.js b/lib/store.js index abccde9..a404f9e 100644 --- a/lib/store.js +++ b/lib/store.js @@ -17,7 +17,13 @@ const ENGINES_DIR = path.join(TANGLECLAW_DIR, 'engines'); const TEMPLATES_DIR = path.join(TANGLECLAW_DIR, 'templates'); const BUNDLED_ENGINES_DIR = path.join(__dirname, '..', 'data', 'engines'); const BUNDLED_TEMPLATES_DIR = path.join(__dirname, '..', 'data', 'templates'); -const BUNDLED_GLOBAL_RULES = path.join(__dirname, '..', 'data', 'default-global-rules.md'); +// #240 — global rules are now tracked in git at data/global-rules.md +// (renamed from data/default-global-rules.md in the same PR). This is the +// canonical source for both UI/API edits AND PR-driven edits — there is no +// per-install copy at ~/.tangleclaw/global-rules.md anymore. Migration +// from old installs is handled by `globalRulesApi._maybeWarnLegacyFile` +// at first load. Tests use `_setBundledGlobalRulesPath` to redirect. +let BUNDLED_GLOBAL_RULES = path.join(__dirname, '..', 'data', 'global-rules.md'); /** * Custom error class for store operations. @@ -2533,13 +2539,76 @@ const portLeasesApi = { // ── Global Rules ── /** - * Get the path to the user's global rules file. + * Legacy per-install global-rules path (#240). Prior to the canonical- + * source migration, this was the live file. Now it's only checked once + * per process at startup to warn operators upgrading from a pre-#240 + * install that their per-install customizations are no longer read. + * Recovery instructions are in the warning message + #240's body. * @returns {string} */ -function _globalRulesPath() { +function _legacyGlobalRulesPath() { return path.join(_basePath || TANGLECLAW_DIR, 'global-rules.md'); } +let _legacyGlobalRulesWarned = false; +function _maybeWarnLegacyGlobalRulesFile() { + if (_legacyGlobalRulesWarned) return; + const legacyPath = _legacyGlobalRulesPath(); + if (!fs.existsSync(legacyPath)) { + // No legacy file → mark as warned-or-skipped so subsequent loads + // don't re-stat. The flag means "we've considered this," not + // "we've emitted." Critic-noted: previously the flag was set + // before the existence check; now it's set after the work is + // genuinely done so a future retry-on-load refactor is correct. + _legacyGlobalRulesWarned = true; + return; + } + try { + const legacy = fs.readFileSync(legacyPath, 'utf8'); + const canonical = fs.existsSync(BUNDLED_GLOBAL_RULES) + ? fs.readFileSync(BUNDLED_GLOBAL_RULES, 'utf8') + : ''; + if (legacy.trim() === canonical.trim()) { + // Legacy matches canonical → operator is safe; nothing to warn. + _legacyGlobalRulesWarned = true; + return; + } + // Auto-backup the legacy file ONCE to make recovery trivial even + // if the operator misses the log warning. Critic-noted MEDIUM: + // long-running TC server emits the warn only at init, before the + // UI is connected — the backup is the durable recovery surface. + // Path is sibling to the legacy file with a versioned suffix so + // we never overwrite a prior backup. + const backupPath = legacyPath + '.pre-240-backup'; + let backupCreated = false; + if (!fs.existsSync(backupPath)) { + try { + fs.writeFileSync(backupPath, legacy, 'utf8'); + backupCreated = true; + } catch (err) { + log.warn('failed to write legacy global-rules backup', { backupPath, error: err.message }); + } + } + log.warn( + 'legacy global-rules file detected and IGNORED (#240); content differs from the tracked canonical source', + { + legacyPath, + canonicalPath: BUNDLED_GLOBAL_RULES, + backupPath: backupCreated ? backupPath : (fs.existsSync(backupPath) ? backupPath : null), + legacyBytes: legacy.length, + canonicalBytes: canonical.length, + howToRecover: backupCreated || fs.existsSync(backupPath) + ? `legacy content preserved at ${backupPath}; diff against ${BUNDLED_GLOBAL_RULES} and merge wanted sections via the landing-page Global Rules editor or by editing data/global-rules.md directly and committing` + : 'review the diff and re-apply intended changes via the landing-page Global Rules editor or by editing data/global-rules.md directly and committing' + } + ); + _legacyGlobalRulesWarned = true; + } catch (err) { + log.warn('failed to compare legacy global-rules file', { legacyPath, error: err.message }); + _legacyGlobalRulesWarned = true; // don't retry on every load if the file is broken + } +} + /** * Normalize global-rules markdown so cosmetic whitespace doesn't propagate * into every regenerated CLAUDE.md (#100). Normalizes CRLF→LF, strips trailing @@ -2626,64 +2695,97 @@ const globalRulesApi = { _normalize: _normalizeRulesContent, /** - * Load the user's global rules content. - * Creates from defaults if missing. Auto-heals legacy polluted files - * by rewriting them in-place with normalized content (#100). + * Load global rules from the tracked canonical source (#240). + * + * The file is `data/global-rules.md`, git-tracked in the TC repo. + * UI/API edits AND PR-driven edits both land here — there is no + * per-install copy anymore. Auto-heals normalization-only diffs by + * rewriting in-place (#100 behavior preserved). + * + * On first call per process, warns if the legacy per-install file + * still exists with different content (operators upgrading from + * pre-#240 installs). + * * @returns {string} */ load() { - const userFile = _globalRulesPath(); + _maybeWarnLegacyGlobalRulesFile(); try { - if (fs.existsSync(userFile)) { - const raw = fs.readFileSync(userFile, 'utf8'); - const normalized = _normalizeRulesContent(raw); - if (normalized !== raw) { - try { - fs.writeFileSync(userFile, normalized, 'utf8'); - } catch (err) { - log.warn('Failed to auto-heal global rules', { path: userFile, error: err.message }); - } + if (!fs.existsSync(BUNDLED_GLOBAL_RULES)) { + log.warn('canonical global-rules file missing', { path: BUNDLED_GLOBAL_RULES }); + return ''; + } + const raw = fs.readFileSync(BUNDLED_GLOBAL_RULES, 'utf8'); + const normalized = _normalizeRulesContent(raw); + if (normalized !== raw) { + try { + fs.writeFileSync(BUNDLED_GLOBAL_RULES, normalized, 'utf8'); + } catch (err) { + log.warn('Failed to auto-heal global rules', { path: BUNDLED_GLOBAL_RULES, error: err.message }); } - return normalized; } + return normalized; } catch (err) { - log.warn('Failed to read global rules', { path: userFile, error: err.message }); - } - // Create from bundled defaults - try { - const defaults = _normalizeRulesContent(fs.readFileSync(BUNDLED_GLOBAL_RULES, 'utf8')); - fs.mkdirSync(path.dirname(userFile), { recursive: true }); - fs.writeFileSync(userFile, defaults, 'utf8'); - return defaults; - } catch (err) { - log.warn('Failed to create global rules from defaults', { error: err.message }); + log.warn('Failed to read global rules', { path: BUNDLED_GLOBAL_RULES, error: err.message }); return ''; } }, /** - * Save updated global rules content. Normalized before persisting (#100). + * Save updated global rules content to the tracked canonical source (#240). + * Normalized before persisting (#100). UI/API saves and PR-driven + * file edits write to the same place — no divergence possible. + * * @param {string} content - New global rules markdown */ save(content) { const normalized = _normalizeRulesContent(content); - const userFile = _globalRulesPath(); - fs.mkdirSync(path.dirname(userFile), { recursive: true }); - fs.writeFileSync(userFile, normalized, 'utf8'); - activityApi.log({ eventType: 'rules.global_updated', detail: { length: normalized.length } }); + fs.mkdirSync(path.dirname(BUNDLED_GLOBAL_RULES), { recursive: true }); + fs.writeFileSync(BUNDLED_GLOBAL_RULES, normalized, 'utf8'); + activityApi.log({ eventType: 'rules.global_updated', detail: { length: normalized.length, path: BUNDLED_GLOBAL_RULES } }); }, /** - * Reset global rules to bundled defaults. - * @returns {string} - The default content + * Reset is a no-op under the #240 canonical-source model. + * + * Pre-#240 this restored the per-install file from bundled defaults. + * Under the new model there's no separate "default" — the tracked + * file at `data/global-rules.md` IS the canonical version. To revert + * unwanted edits, use `git checkout data/global-rules.md` (or your + * git workflow's equivalent). Returns the current loaded content + * unchanged so existing callers don't break. + * + * @returns {string} - The current (unchanged) content */ reset() { - const defaults = _normalizeRulesContent(fs.readFileSync(BUNDLED_GLOBAL_RULES, 'utf8')); - const userFile = _globalRulesPath(); - fs.mkdirSync(path.dirname(userFile), { recursive: true }); - fs.writeFileSync(userFile, defaults, 'utf8'); - activityApi.log({ eventType: 'rules.global_reset' }); - return defaults; + log.warn( + 'store.globalRules.reset() is a no-op under the #240 canonical-source model; use `git checkout data/global-rules.md` to revert', + { canonicalPath: BUNDLED_GLOBAL_RULES } + ); + return this.load(); + }, + + /** + * Test-only: redirect the canonical global-rules file path. + * + * Production code reads/writes `data/global-rules.md` in TC's repo, + * which would cause tests to clobber the live file. Tests call this + * to redirect to a tmp file in their before()/beforeEach() and restore + * via `_resetBundledGlobalRulesPath` in their after()/afterEach(). + * + * @param {string} newPath - Absolute path to a writable file + */ + _setBundledGlobalRulesPath(newPath) { + BUNDLED_GLOBAL_RULES = newPath; + _legacyGlobalRulesWarned = false; // reset warn state for tests + }, + + /** + * Test-only: restore BUNDLED_GLOBAL_RULES to the canonical repo path. + */ + _resetBundledGlobalRulesPath() { + BUNDLED_GLOBAL_RULES = path.join(__dirname, '..', 'data', 'global-rules.md'); + _legacyGlobalRulesWarned = false; } }; diff --git a/test/api-globalrules.test.js b/test/api-globalrules.test.js index 5409d38..1b6e627 100644 --- a/test/api-globalrules.test.js +++ b/test/api-globalrules.test.js @@ -57,12 +57,19 @@ function request(server, method, urlPath, body) { describe('API /api/rules/global', () => { let tmpDir; + let tempRulesPath; let server; before(async () => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'tc-api-globalrules-')); store._setBasePath(tmpDir); store.init(); + // #240 — redirect canonical global-rules to tmp and seed with a + // realistic baseline so GET returns something with "Global Rules" + // in it (matching the public API contract). + tempRulesPath = path.join(tmpDir, 'global-rules.md'); + fs.writeFileSync(tempRulesPath, '# Global Rules\n\n- Seed baseline for API test\n'); + store.globalRules._setBundledGlobalRulesPath(tempRulesPath); server = createServer(); await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); @@ -70,6 +77,7 @@ describe('API /api/rules/global', () => { after(async () => { await new Promise((resolve) => server.close(resolve)); + store.globalRules._resetBundledGlobalRulesPath(); store.close(); fs.rmSync(tmpDir, { recursive: true, force: true }); }); @@ -107,18 +115,25 @@ describe('API /api/rules/global', () => { }); }); - describe('POST /api/rules/global/reset', () => { - it('should reset to bundled defaults', async () => { - // Save custom first - await request(server, 'PUT', '/api/rules/global', { content: '# Custom' }); + describe('POST /api/rules/global/reset (#240 no-op contract)', () => { + it('returns current content unchanged — reset is a no-op under the canonical-source model', async () => { + // Pre-#240 reset restored the per-install file from bundled + // defaults. Under the canonical-source model the tracked file + // IS canonical; reset returns current content + the route stays + // for back-compat with the UI button (which should be removed + // in a follow-up; today it just becomes a no-op refresh). + const custom = '# Custom Pre-Reset\n\n- this should survive reset\n'; + await request(server, 'PUT', '/api/rules/global', { content: custom }); const { status, data } = await request(server, 'POST', '/api/rules/global/reset'); assert.equal(status, 200); - assert.ok(data.content.includes('Global Rules'), 'Should return default content'); + assert.equal(data.content, custom, + 'reset returns current content unchanged (no-op)'); - // Verify persisted + // Verify the on-disk file was not modified const { data: loaded } = await request(server, 'GET', '/api/rules/global'); - assert.equal(loaded.content, data.content); + assert.equal(loaded.content, custom, + 'reset did not modify the canonical file'); }); }); diff --git a/test/engines.test.js b/test/engines.test.js index 52c3a1f..03650cc 100644 --- a/test/engines.test.js +++ b/test/engines.test.js @@ -12,14 +12,21 @@ const porthub = require('../lib/porthub'); describe('engines', () => { let tempDir; + let tempRulesPath; before(() => { tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'tangleclaw-engines-test-')); store._setBasePath(tempDir); store.init(); + // #240 — redirect canonical global-rules to tmp and seed so the + // engine config generators have realistic rules content to inject. + tempRulesPath = path.join(tempDir, 'global-rules.md'); + fs.writeFileSync(tempRulesPath, '# Global Rules\n\nThese rules apply to all projects managed by TangleClaw.\n\n- Test seed for engine config generation\n'); + store.globalRules._setBundledGlobalRulesPath(tempRulesPath); }); after(() => { + store.globalRules._resetBundledGlobalRulesPath(); store.close(); fs.rmSync(tempDir, { recursive: true, force: true }); }); @@ -747,6 +754,139 @@ describe('engines', () => { }); }); + describe('writeEngineConfig (#240 drift detection)', () => { + // Captures log.warn calls via the logger module's internal sink so + // we can assert drift warnings fire without sprinkling spies. + let writeDir; + let claudeProfile; + let prawduct; + const minimalProjConfig = { + rules: { core: { changelogPerChange: true, jsdocAllFunctions: true, unitTestRequirements: true, sessionWrapProtocol: true, porthubRegistration: true }, extensions: {} }, + methodologyArchives: [] + }; + + before(() => { + writeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'tc-write-engine-config-')); + claudeProfile = store.engines.get('claude'); + prawduct = store.templates.get('prawduct'); + assert.ok(claudeProfile && claudeProfile.configFormat, 'claude profile must have configFormat for these tests'); + }); + + after(() => { + fs.rmSync(writeDir, { recursive: true, force: true }); + }); + + it('writes the file when it does not exist (no drift)', () => { + const projectPath = fs.mkdtempSync(path.join(writeDir, 'fresh-')); + const result = engines.writeEngineConfig('claude', projectPath, minimalProjConfig, claudeProfile, prawduct); + assert.equal(result.written, true); + assert.equal(result.drifted, false, 'no drift when target file did not exist'); + assert.equal(result.error, null); + assert.ok(fs.existsSync(result.configFilePath), 'file written at the helper-reported path'); + assert.ok(fs.readFileSync(result.configFilePath, 'utf8').includes('Generated by TangleClaw')); + }); + + it('writes the file when it exists and matches (no drift, idempotent)', () => { + const projectPath = fs.mkdtempSync(path.join(writeDir, 'match-')); + const first = engines.writeEngineConfig('claude', projectPath, minimalProjConfig, claudeProfile, prawduct); + assert.equal(first.drifted, false); + const second = engines.writeEngineConfig('claude', projectPath, minimalProjConfig, claudeProfile, prawduct); + assert.equal(second.written, true); + assert.equal(second.drifted, false, 'unchanged content must not register as drift'); + }); + + it('detects drift when the existing on-disk file differs (the #240 surface)', () => { + const projectPath = fs.mkdtempSync(path.join(writeDir, 'drift-')); + // Seed the target file with content that would never match the + // regenerated output — a hand-edit a contributor might have committed. + const configFilePath = path.join(projectPath, claudeProfile.configFormat.filename); + fs.writeFileSync(configFilePath, '# CLAUDE.md\n\n## Manually Added Rule\n\n- This was hand-edited\n'); + const result = engines.writeEngineConfig('claude', projectPath, minimalProjConfig, claudeProfile, prawduct); + assert.equal(result.written, true, 'still writes the regenerated content (warn is informational, not blocking)'); + assert.equal(result.drifted, true, 'drift must be reported'); + assert.equal(result.error, null); + // Overwrite happened — the hand-edited content is gone (this IS + // the silent-clobber failure mode; the warning is what's new). + const after = fs.readFileSync(configFilePath, 'utf8'); + assert.ok(!after.includes('Manually Added Rule'), 'hand-edit was overwritten as expected'); + assert.ok(after.includes('Generated by TangleClaw'), 'replacement content is the regenerated CLAUDE.md'); + }); + + it('treats trailing-whitespace-only differences as non-drift (tolerant comparator)', () => { + const projectPath = fs.mkdtempSync(path.join(writeDir, 'whitespace-')); + const first = engines.writeEngineConfig('claude', projectPath, minimalProjConfig, claudeProfile, prawduct); + // Append/prepend extra newlines to the existing file — semantically + // identical, should not trigger a drift warning on next write. + const configFilePath = first.configFilePath; + const existing = fs.readFileSync(configFilePath, 'utf8'); + fs.writeFileSync(configFilePath, '\n\n' + existing + '\n\n\n'); + const result = engines.writeEngineConfig('claude', projectPath, minimalProjConfig, claudeProfile, prawduct); + assert.equal(result.drifted, false, 'pure whitespace differences must not register as drift'); + }); + + it('returns skipped (not error) when engineProfile has no configFormat — openclaw / genesis path', () => { + // Pre-Critic this returned an error string for what is intentional + // behavior (engines without config files: openclaw, genesis). The + // 4 call sites would surface that as "Failed to write engine + // config" on every createProject / launchSession for such engines. + // The helper now returns `{skipped: true, skipReason, error: null}` + // and callers gate on `!skipped` before pushing errors. + const projectPath = fs.mkdtempSync(path.join(writeDir, 'noformat-')); + const fakeProfile = { id: 'phantom' }; + const result = engines.writeEngineConfig('claude', projectPath, minimalProjConfig, fakeProfile, prawduct); + assert.equal(result.written, false); + assert.equal(result.skipped, true); + assert.equal(result.error, null, 'skipped must NOT surface as an error'); + assert.match(result.skipReason, /configFormat/i); + }); + + it("returns skipped when configFormat exists but filename is null (real openclaw / genesis shape)", () => { + // Pin: openclaw's actual shape is `configFormat: {filename: null, ...}` + // — truthy as an object, but no usable filename. Earlier guard + // `if (engineProfile.configFormat)` would pass and the helper + // would emit an error. Fixed by checking `configFormat.filename` + // directly. + const projectPath = fs.mkdtempSync(path.join(writeDir, 'nullfilename-')); + const openclawShape = { id: 'fake-openclaw', configFormat: { filename: null, syntax: null, generator: null } }; + const result = engines.writeEngineConfig('fake-openclaw', projectPath, minimalProjConfig, openclawShape, prawduct); + assert.equal(result.written, false); + assert.equal(result.skipped, true); + assert.equal(result.error, null); + }); + + it('returns skipped when generateConfig produces empty content (no error)', () => { + // For engines with `supportsConfigFile: false` the generator + // returns null/empty even though configFormat may exist. Helper + // must treat this as a deliberate skip, not an error. + const projectPath = fs.mkdtempSync(path.join(writeDir, 'emptygen-')); + // Synthesize an engineId that has no registered generator — + // generateConfig returns null. Profile-shape is borrowed from + // claude so the configFormat.filename check passes first. + const fakeProfile = { id: 'no-such-engine', configFormat: claudeProfile.configFormat }; + const result = engines.writeEngineConfig('no-such-engine', projectPath, minimalProjConfig, fakeProfile, prawduct); + assert.equal(result.written, false); + assert.equal(result.skipped, true); + assert.equal(result.error, null); + assert.match(result.skipReason, /generateConfig|empty/i); + }); + + it('CRLF line endings in the on-disk file do NOT register as drift (#240 Critic n1)', () => { + // Windows editors save with CRLF; the regenerator emits LF. + // Without normalization, every session launch on a Windows-saved + // file would emit a drift warning that doesn't represent a real + // semantic change. + const projectPath = fs.mkdtempSync(path.join(writeDir, 'crlf-')); + const first = engines.writeEngineConfig('claude', projectPath, minimalProjConfig, claudeProfile, prawduct); + const configFilePath = first.configFilePath; + // Convert the file's line endings to CRLF in-place, semantically + // identical content. + const lf = fs.readFileSync(configFilePath, 'utf8'); + fs.writeFileSync(configFilePath, lf.replace(/\n/g, '\r\n')); + const second = engines.writeEngineConfig('claude', projectPath, minimalProjConfig, claudeProfile, prawduct); + assert.equal(second.drifted, false, 'CRLF-vs-LF must not register as drift'); + }); + }); + describe('validateParity', () => { it('should return valid when all engines pass parity checks', () => { const result = engines.validateParity(); diff --git a/test/store-globalrules.test.js b/test/store-globalrules.test.js index d6a77b1..d181ea6 100644 --- a/test/store-globalrules.test.js +++ b/test/store-globalrules.test.js @@ -7,44 +7,62 @@ const path = require('node:path'); const os = require('node:os'); const store = require('../lib/store'); -describe('store.globalRules', () => { +describe('store.globalRules (#240 canonical-source model)', () => { let tempDir; + let tempRulesPath; before(() => { tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'tangleclaw-globalrules-test-')); store._setBasePath(tempDir); store.init(); + // Redirect BUNDLED_GLOBAL_RULES to a writable tmp file so tests + // don't clobber the repo's data/global-rules.md. + tempRulesPath = path.join(tempDir, 'data', 'global-rules.md'); + fs.mkdirSync(path.dirname(tempRulesPath), { recursive: true }); + // Seed with a minimal canonical file so load() has something to return. + fs.writeFileSync(tempRulesPath, '# Global Rules\n\n- Seed rule from test\n'); + store.globalRules._setBundledGlobalRulesPath(tempRulesPath); }); after(() => { + store.globalRules._resetBundledGlobalRulesPath(); store.close(); fs.rmSync(tempDir, { recursive: true, force: true }); }); - it('should load defaults when no user file exists', () => { + it('should load content from the canonical tracked file', () => { const content = store.globalRules.load(); - assert.ok(content.includes('Global Rules'), 'Should contain default header'); + assert.ok(content.includes('Global Rules'), 'Should contain the canonical header'); assert.ok(content.length > 0); }); - it('should create user file from defaults on first load', () => { - const userFile = path.join(tempDir, 'global-rules.md'); - assert.ok(fs.existsSync(userFile), 'User file should be created'); - }); - - it('should return saved content on subsequent loads', () => { + it('should return saved content on subsequent loads (round-trip)', () => { const custom = '# My Custom Rules\n\n- Rule one\n- Rule two\n'; store.globalRules.save(custom); const loaded = store.globalRules.load(); assert.equal(loaded, custom); }); - it('should reset to bundled defaults', () => { - store.globalRules.save('# Totally custom stuff'); - const defaults = store.globalRules.reset(); - assert.ok(defaults.includes('Global Rules'), 'Reset should restore default header'); - const loaded = store.globalRules.load(); - assert.equal(loaded, defaults); + it('save() writes to the canonical path (not a per-install copy)', () => { + const custom = '# Canonical Save Test\n\n- Verify the write target\n'; + store.globalRules.save(custom); + const onDisk = fs.readFileSync(tempRulesPath, 'utf8'); + assert.equal(onDisk, custom, + 'save() must write to the BUNDLED_GLOBAL_RULES path (tracked canonical), not a separate per-install file'); + }); + + it('reset() is a no-op that returns current content unchanged (#240)', () => { + // Pre-#240 reset() restored the per-install file from bundled + // defaults. Under the canonical-source model there is no separate + // "default" \u2014 the tracked file IS the canonical version. reset() + // returns current content + logs a warning; callers (e.g. the UI + // "Reset" button) should be updated to use `git checkout` instead. + const customBefore = '# Custom content before reset\n\n- I should survive reset\n'; + store.globalRules.save(customBefore); + const returned = store.globalRules.reset(); + assert.equal(returned, customBefore, 'reset() returns current content unchanged'); + const afterReset = store.globalRules.load(); + assert.equal(afterReset, customBefore, 'content on disk is unchanged after reset()'); }); it('should save empty content', () => { @@ -62,13 +80,21 @@ describe('store.globalRules', () => { describe('store.globalRules normalization (#100)', () => { let tmpDir; + let tempRulesPath; beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'tc-store-rules-norm-')); store._setBasePath(tmpDir); + // #240 — redirect the canonical global-rules path to a tmp file so + // tests don't clobber the real data/global-rules.md. tmpDir doubles + // as both the basePath (for session DB etc.) and the parent of the + // tracked-file equivalent under test. + tempRulesPath = path.join(tmpDir, 'global-rules.md'); + store.globalRules._setBundledGlobalRulesPath(tempRulesPath); }); afterEach(() => { + store.globalRules._resetBundledGlobalRulesPath(); store.close(); fs.rmSync(tmpDir, { recursive: true, force: true }); }); @@ -184,13 +210,13 @@ describe('store.globalRules normalization (#100)', () => { describe('save() applies normalization', () => { it('strips trailing whitespace before persisting', () => { store.globalRules.save('# Rules \n\n- item \n'); - const onDisk = fs.readFileSync(path.join(tmpDir, 'global-rules.md'), 'utf8'); + const onDisk = fs.readFileSync(tempRulesPath, 'utf8'); assert.equal(onDisk, '# Rules\n\n- item\n'); }); it('strips uniform body indent before persisting', () => { store.globalRules.save('# Rules\n\n - item one\n - item two\n'); - const onDisk = fs.readFileSync(path.join(tmpDir, 'global-rules.md'), 'utf8'); + const onDisk = fs.readFileSync(tempRulesPath, 'utf8'); assert.equal(onDisk, '# Rules\n\n- item one\n- item two\n'); }); @@ -205,7 +231,7 @@ describe('store.globalRules normalization (#100)', () => { describe('load() auto-heals legacy polluted files', () => { it('rewrites a polluted file in place on first read', () => { - const userFile = path.join(tmpDir, 'global-rules.md'); + const userFile = tempRulesPath; const dirty = '# Rules \n\n - item \n - other\n'; fs.mkdirSync(tmpDir, { recursive: true }); fs.writeFileSync(userFile, dirty, 'utf8'); @@ -217,7 +243,7 @@ describe('store.globalRules normalization (#100)', () => { }); it('does not rewrite a clean file', () => { - const userFile = path.join(tmpDir, 'global-rules.md'); + const userFile = tempRulesPath; const clean = '# Rules\n\n- one\n'; fs.mkdirSync(tmpDir, { recursive: true }); fs.writeFileSync(userFile, clean, 'utf8'); @@ -229,12 +255,20 @@ describe('store.globalRules normalization (#100)', () => { }); }); - describe('reset() applies normalization to bundled defaults', () => { - it('written defaults have no trailing whitespace', () => { - const out = store.globalRules.reset(); - assert.equal(/[ \t]+\n/.test(out), false); - const onDisk = fs.readFileSync(path.join(tmpDir, 'global-rules.md'), 'utf8'); - assert.equal(onDisk, out); + describe('reset() under the #240 canonical-source model', () => { + it('is a no-op — does not write to disk, returns current content unchanged', () => { + // Pre-#240 reset() restored the per-install file from bundled + // defaults. Under the canonical-source model, there is no + // separate "default" — the tracked file at `data/global-rules.md` + // IS canonical. To revert, use `git checkout`. reset() now + // returns current loaded content + logs a warning. + const seeded = '# Canonical Content\n\n- existing rule\n'; + fs.writeFileSync(tempRulesPath, seeded); + const before = store.globalRules.load(); + const returned = store.globalRules.reset(); + assert.equal(returned, before, 'reset() returns current content unchanged'); + const onDisk = fs.readFileSync(tempRulesPath, 'utf8'); + assert.equal(onDisk, before, 'reset() does not modify the on-disk file'); }); }); });