Structural fix: git file is canonical for global rules (closes #240)#242
Merged
Conversation
Adopts Option 1 from #240's design discussion ("git file is canonical") plus Option D (drift-detection warning). The two together prevent the trap that bit #234 and #236 from ever firing silently again. (1) data/global-rules.md becomes the canonical source. Renamed from data/default-global-rules.md (git mv) and populated with the full 191-line rules content TC-v3 has accumulated. There is no per-install copy at ~/.tangleclaw/global-rules.md anymore — both UI/API edits AND PR-driven file edits land in the same tracked file, so neither path can clobber the other. (2) store.globalRules.load/save rewritten. - load() reads BUNDLED_GLOBAL_RULES directly, no per-install fallback. Auto-heal-on-read preserved. - save() writes there. Normalization preserved. - reset() becomes a no-op + warn. Under the new model "reset" means `git checkout data/global-rules.md`; the UI button should be removed in a follow-up (today it just refreshes). - New test seam: _setBundledGlobalRulesPath / _resetBundledGlobalRulesPath — lets tests redirect without clobbering the real canonical file. - Migration safety: _maybeWarnLegacyGlobalRulesFile fires once on first load() per process; if the old per-install file exists with different content, logs a clear warning with recovery steps. (3) engines.writeEngineConfig drift-detection helper. New helper in lib/engines.js (right before syncEngineHooks) that wraps generateConfig + read-existing + diff + warn + write. When the on-disk engine config file differs from regenerated content, logs `engine config drift detected — overwriting on-disk hand-edits (#240)` with byte counts and a howToInvestigate field naming the durable edit paths. Whitespace-tolerant via .trim() — auto-formatter trailing-newline changes don't false-positive. (4) All four engine-config write sites refactored to use the helper: - lib/projects.js#createProject (~line 284) - lib/projects.js#syncAllProjects (~line 763) - lib/projects.js#updateProject engine-switch (~line 1344) - lib/sessions.js#launchSession (~line 191) Uniform behavior + uniform warning regardless of which code path triggers regeneration. Tests. 6 new cases in test/engines.test.js's new `writeEngineConfig (#240 drift detection)` describe block covering writes-when-absent, writes-when-matches (idempotent), detects-drift- when-differs (the canonical failure mode), tolerates-whitespace- only-diffs, returns-error-on-empty-content, returns-error-on- missing-configFormat. test/store-globalrules.test.js reframed for the canonical-source contract: load/save round-trip via the redirected tracked path; new pin that save() writes to BUNDLED_GLOBAL_RULES; reset() asserted as no-op. test/api- globalrules.test.js updated to seed a tmp canonical file and assert the new reset contract. Full suite 2463/2464 pass (1 pre-existing case-insensitive-host skip). What this means going forward. Any future PR that wants to add or change global rules: edit data/global-rules.md directly, commit, push. The next session launch on any clone reads the updated file via store.globalRules.load() and composes into the regenerated CLAUDE.md. UI/API edits via the landing-page editor go to the same file and become uncommitted local changes the operator can commit if intentional. The previous broken model (PR edits to CLAUDE.md that get clobbered) is structurally impossible now — there's nothing to clobber. Closes #240
MAJOR (2): regressions for engines without config files.
- writeEngineConfig returned an `error` string for openclaw / genesis
(engines with configFormat.filename === null) instead of treating
them as a deliberate no-op. The 4 call sites would surface that as
"Failed to write engine config" on every createProject /
launchSession for such engines.
- launchSession's guard `if (engineProfile.configFormat)` passed
for openclaw (its configFormat is `{filename: null, ...}` —
truthy as an object).
Fix. Helper now returns `{written: false, skipped: true, skipReason,
error: null}` for the two no-op cases (no configFormat.filename, or
generateConfig returned empty). All 4 call sites updated to gate on
`!skipped` before pushing/logging errors. Two new tests pin the
shape: noformat path returns skipped, openclaw-style null-filename
returns skipped.
MEDIUM (2):
- _maybeWarnLegacyGlobalRulesFile is still once-per-process, but now
AUTO-BACKUPS the legacy file content to `<legacy>.pre-240-backup`
on first warn. Operators who miss the log line have a durable
recovery surface (`mv .pre-240-backup global-rules.md` to restore).
Warning message now includes the backup path.
- Reset-button-is-now-a-no-op UI follow-up filed as #243.
MINOR (3 of 4):
- Drift-detection comparator now normalizes CRLF→LF before
comparing. Windows-saved files don't false-positive. New test pin.
- _legacyGlobalRulesWarned flag-set moved to AFTER the work is
done (so a future retry-on-load refactor is correct).
- CHANGELOG entry trimmed from ~700 words to ~200. Keeps the
essentials, drops the session-wrap-style detail.
Skipped: explicit declination of Options B + C in PR body. The PR
description already explains why Option 1 + D were chosen; declining
the other two adds noise without value.
Full suite 2465/2466 pass (net +2 from CRLF + null-filename test
pins; 1 pre-existing case-insensitive-host skip).
Refs #240
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Closes #240. Adopts Option 1 from #240's design discussion ("git file is canonical") plus Option D (drift-detection warning). The two together prevent the trap that bit #234 and #236 from ever firing silently again.
The bug class (in one line): TC was regenerating
CLAUDE.mdfrom a DB-stored ruleset that PR-driven file edits never reached, so direct edits toCLAUDE.mdlived on ingit logbut were absent from the live ruleset after the next session launch.Why these two options
After landing #241 (the recovery + diagnostic) and discussing the four design options surfaced in #240 (A: tracked-file canonical / B: append-only marker / C: pre-commit guard / D: detect-and-warn), the call was:
What changed
(1)
data/global-rules.mdbecomes the canonical sourceRenamed from
data/default-global-rules.md(git mv) and populated with the full 191-line rules content TC-v3 has accumulated. There is no per-install copy at~/.tangleclaw/global-rules.mdanymore — both UI/API edits AND PR-driven file edits land in the same tracked file, so neither path can clobber the other.(2)
store.globalRules.load/saverewritten (lib/store.js)load()readsBUNDLED_GLOBAL_RULESdirectly, no per-install fallback. Auto-heal-on-read preserved ([bug] Global rules editor saves trailing whitespace and stray indentation, polluting every regenerated CLAUDE.md #100 invariant).save()writes toBUNDLED_GLOBAL_RULES. Normalization preserved.reset()becomes a no-op + warn. Under the new model "reset" meansgit checkout data/global-rules.md; the UI button should be removed in a follow-up (today it just refreshes — non-breaking back-compat)._setBundledGlobalRulesPath/_resetBundledGlobalRulesPath. Lets tests redirect to a tmp file without clobbering the real canonical._maybeWarnLegacyGlobalRulesFilefires once on firstload()per process. If the old per-install file exists with different content, logs a clear warning with recovery steps. Operators upgrading from a pre-[bug] CLAUDE.md regeneration silently clobbers PR-driven rule edits #240 install will see the warning in TC's log on first session start.(3)
engines.writeEngineConfigdrift-detection helper (lib/engines.js)New helper right before
syncEngineHooks. WrapsgenerateConfig+ read-existing + diff + warn + write. When the on-disk engine config file differs from what would be regenerated, logs:Whitespace-tolerant via
.trim()— auto-formatter trailing-newline changes don't false-positive. Permissive on read failures (overwrites without warning if the existing file is unreadable — the write is the operation that matters; the warning is best-effort).(4) All four engine-config write sites refactored to use the helper
lib/projects.js#createProject(~line 284)lib/projects.js#syncAllProjects(~line 763)lib/projects.js#updateProjectengine-switch (~line 1344)lib/sessions.js#launchSession(~line 191)Uniform behavior + uniform warning regardless of which code path triggers regeneration. Each refactored site is now 1-2 lines (helper call) instead of 5-7 lines (inline generate + mkdir + writeFileSync + try/catch).
Tests
6 new cases in
test/engines.test.js's newwriteEngineConfig (#240 drift detection)describe block:drifted: true, asserts overwrite still happens because the warn is informational not blocking).trim()comparator)test/store-globalrules.test.jsreframed for the canonical-source contract:save()writes toBUNDLED_GLOBAL_RULES(not a separate per-install file)reset()asserted as no-op (returns current content unchanged, leaves file alone)test/api-globalrules.test.jsupdated to seed a tmp canonical file and assert the new reset contract.Full suite: 2463/2464 pass (1 pre-existing case-insensitive-host skip from #221, unrelated).
What this means going forward
data/global-rules.mddirectly, commit, push. The next session launch on any clone reads the updated file viastore.globalRules.load()and composes into the regeneratedCLAUDE.md.git checkoutto discard.CLAUDE.mddirectly (if anyone still tries) hit the drift-detection warning when the next regeneration runs, surfacing the issue immediately instead of silently.Test plan
node --test "test/*.test.js"→ 2463/2464 pass.grep -E "^## \[" CHANGELOG.md | head -5→[Unreleased]and[3.18.0]headings intact post-edit.wc -l data/global-rules.md→ 190 lines (full canonical content).grep -c "Archive plans\|version-bump.*wrap step" data/global-rules.md→ 2 (both recovered rules present).git statusshowsdata/global-rules.mdas modified (proving UI edits land in the tracked file).CLAUDE.md→ trigger a session launch → check TC's log for theengine config drift detectedwarning (proving Option D fires).Closes #240