Never clobber a hand-edited skill: the no-clobber guard was testing a list, not the filesystem#742
Merged
Merged
Conversation
`main` is red. `scripts/check-file-size.sh` rejects a baseline entry for a file that no longer needs one — "an exemption must not outlive the problem it covered" — and `stella-cli/src/main.rs` is now 1421 lines with a 1816 ceiling still on the books. This is an interaction between two PRs that were in flight at the same time, not a mistake in either. #733 moved the storage/scripts/graph commands out of main.rs, took it to 1337, and *deleted* its baseline entry because the file no longer crossed the limit. #735 landed separately, grew main.rs back to 1421, and carried a baseline entry for it. Both were individually correct; together they produce a file that is under the limit and exempted from it, which the ratchet treats as an error by design. Dropping the entry is the fix the script itself prescribes. main.rs stays covered by the ordinary 1500-line limit with 79 lines of headroom. check-file-size: OK — 456 Rust files, none over 1500 lines except 27 grandfathered (none grew).
…ling `main` does not build. `cargo check -p stella-cli` fails with nine E0308s: "`MemoryCmd` and `memory_cmd::MemoryCmd` have similar names, but are actually distinct types". Cause: two PRs that each merged cleanly and are individually correct. #733 *moved* `MemoryCmd` into `memory_cmd.rs` to get `main.rs` under its size ratchet. #735 was cut from a `main` that predated that move, so it carried its own copy of the enum and added two new variants — `Compact` and `Index` — to that copy. Git merged both without a textual conflict, and the result declares the enum twice: `Command::Memory` holds a `memory_cmd::MemoryCmd`, while the dispatch arms match on the stale `crate::MemoryCmd`. This is the failure mode a textual merge is worst at: neither side touched the same lines, so nothing conflicted, and the incompatibility is a type error two files away from either change. Fix, in the direction #733 intended: - Move #735's `Compact` and `Index` variants onto `memory_cmd::MemoryCmd`, doc comments intact, with their arg types qualified through `crate::`. - Delete the stale duplicate from `main.rs`. - Qualify the dispatch arms. Also drops the obsolete `main.rs` baseline entry, which is what CI reported first: #733 deleted it when the file fell to 1337 lines, #735 re-added it at 1816, and the file is now 1421 — under the limit and exempted from it, which the ratchet rejects by design. That check masked the compile error, because file-size runs before clippy and exits first. Verified beyond the build: `stella memory --help` lists all nine subcommands, so #735's `compact` and `index` survive the move. `make gate` green: exit 0, 3670 tests, check-file-size OK.
Spec §8 (docs/design/adaptive-context.md) lists "never clobbering a
hand-edited file" as a guarantee the adaptive loop preserves. It did not
hold: auto-created skills could silently destroy a user's edits.
The root cause is a list-versus-filesystem confusion. `decide_auto_creation`
asks whether the target path is in `existing_paths`, and `auto_create_skills`
built that list from `self.load_skills()` — the loaded, filtered skill list.
So the guard answered "is this a skill I successfully loaded and kept?", not
"does this file exist?", and `AutoCreateSkip::FileExists` never checked
existence at all. Any file on disk but absent from the loaded list was
overwritten by an unconditional `std::fs::write`.
Three things drop a real file out of that list, all in
`load_workspace_skills_with_authority`: workspace skills excluded by
authority, `filesystem_settings_disabled()`, and — the reachable one, needing
no unusual configuration — `retain_enabled`, where a skill disabled from the
SKILLS tab is dropped from the list while, as the comment there says, "its
file stays on disk". Mined identity is a deterministic `{slug}-{hash8}`, so a
recurring lesson cluster reliably re-targets the very path the user edited:
the property that makes mining idempotent is what aims it at their work.
`stella-core` stays pure. It has no filesystem dependency and is "the engine —
no I/O, fastest to test"; adding `path.exists()` there would trade one design
defect for another. The decision function is unchanged in behavior — the
caller now hands it accurate input. `stella-cli` enumerates the `*.md` files
actually present in the target directory (`skill_paths_on_disk`) and unions
the loaded paths in, so the "known skill" signal is kept and the guard stays
armed for loaded skills even if the directory read fails. The parameter is
renamed `occupied_paths` and documented as a filesystem fact, since the old
name is what invited the confusion.
Authority is also made coherent: auto-creation is skipped entirely when
`include_workspace_skills` is false. Reading and writing that directory are
one authority, and a skill written there under a session forbidden to read it
could never be loaded back — the same flag excludes it. The narrower
alternative, redirecting creation to the user-global directory, is worse: it
would escalate prose mined under an untrusted workspace into the scope that
applies to every other workspace.
Tests are the deliverable, because the loss is silent. Both regression tests
fail on the unfixed code, the first by finding the generated markdown where
the user's text was. Also pinned: the per-session cap, the ordinary
no-clobber of a loaded skill, and tombstone filtering.
Closes #737
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
macanderson
added a commit
that referenced
this pull request
Jul 26, 2026
The rebase onto main brings in #742, which fixed the no-clobber defect this suite had pinned. Three consequences, none of them optional. THE AUTHORITY GATE MOVED TO THE DISPATCHER #742 made auto-creation return early when `include_workspace_skills` is false: reading and writing the workspace skills directory are one authority, and writing into a directory the session was just told it may not read was the defect's other half. The migration turned `auto_create_skills` into a dispatcher over the lexical and typed paths, so the gate now sits on the dispatcher rather than inside either arm. A guard that protected only one path would be a guarantee that depended on a feature flag. THE FIXTURES NEEDED THE AUTHORITY THEY WERE ALWAYS ASSUMING Every guarantee here exercises mining, and mining now legitimately requires workspace-skill authority. The default fixture passed `false` and got away with it only because the pre-#742 loop wrote to that directory regardless — the bug itself. The default is now `true`, the normal configuration. This is a change to a fixture, not to an assertion: no assertion was edited, which is the line this file's module doc draws. THE PIN IS DELETED, NOT INVERTED `finding_mining_clobbers_hand_edited_skills_when_workspace_skills_are_untrusted` asserted the defect still bit. It does not: the guard now tests the filesystem rather than the loaded-skill list. Its own note said the fix would have to delete it on purpose, so that is what this does. The guarantee it was protecting is covered by `memory::tests::a_disabled_hand_edited_skill_is_never_overwritten_ by_auto_creation`, added by #742, and by `a_hand_edited_skill_file_is_never_clobbered` here, which runs on both paths. Worth recording: git merged #742's `occupied_paths` fix straight into the shared `write_candidates` this migration had already extracted, so the fix covers the typed path and the lexical one without anybody arranging it. `make gate` green: exit 0, 3777 tests, check-file-size OK.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #737.
The bug
Auto-created skills could silently overwrite a user's hand-edited skill file. No prompt, no backup, no message beyond the ordinary "new skill auto-created" line — and the file is not versioned by the app, so the loss is unrecoverable.
Spec §8 (
docs/design/adaptive-context.md) lists "never clobbering a hand-edited file" among the five guarantees the adaptive loop must preserve. It did not hold.Root cause: a list is not the filesystem
stella-core/src/skills.rsdecide_auto_creationasks whether the target path is inexisting_paths.stella-clibuilt that list fromself.load_skills()— the loaded, filtered skill list — and then did an unconditionalstd::fs::write.So the guard answered "is this a skill I successfully loaded and kept?", never "does this file exist?".
AutoCreateSkip::FileExistswas misnamed: it never checked existence. Loaded-skill paths and on-disk paths are different sets, and the guarantee is about the latter.Three things drop a real on-disk file out of the loaded list, all in
load_workspace_skills_with_authority:include_workspace_skills == false— the workspace dir is passed asString::new(), so nothing from it loads;filesystem_settings_disabled()— returns an emptyLoadedSkills;retain_enabled(...)— a skill disabled from the SKILLS tab is dropped, and the comment directly above it says "its file stays on disk".(3) is the reachable one and needs no unusual configuration. Because mined identity is a deterministic
{slug}-{hash8}, a recurring lesson cluster reliably re-targets the exact path the user edited: the property that makes mining idempotent is what aims it at their work.The fix
stella-corestays pure. It is "the engine — no I/O, fastest to test" and has essentially no filesystem dependency; puttingpath.exists()indecide_auto_creationwould trade one design defect for another and make the function much harder to test. Its behavior is unchanged here — the caller now hands it accurate input, which is the same shaperules::decide_promotion(approve, file_exists)already uses.stella-cligainsskill_paths_on_disk, which enumerates the*.mdfiles actually present in the target directory, rebuilding path strings the same way the guard builds its target so they compare exactly.existing_paths→occupied_pathsand documented as a filesystem fact. The old name is what invited the confusion, and the rename is line-neutral —stella-core/src/skills.rssits exactly on its 1502-line baseline ceiling, so nomod testsextraction was needed.Structurally this closes all three triggers at once: the guard no longer depends on
load_skills()at all.Authority incoherence
auto_create_skillscomputed its write target fromworkspace_skills_dir()unconditionally, so a session forbidden from reading project prompts still wrote mined prose into that directory — where the same flag guaranteed it would never be loaded back.Auto-creation is now skipped entirely when
include_workspace_skillsis false. Reading and writing that directory are one authority. The narrower alternative — redirect creation to the user-global dir — is worse: it would escalate prose mined under an untrusted workspace into the scope that applies to every other workspace.What the tests prove
The bug's whole character is that it is silent, so the tests are the deliverable. They live in
stella-cli/src/memory/tests.rs, which is the only place this seam is reachable —stella-coreonly ever sees the list this crate hands it.Both regression tests fail on the unfixed code, verified by reverting the fix:
a_disabled_hand_edited_skill_is_never_overwritten_by_auto_creation— trigger (3), end to end: mine a skill, hand-edit it, disable it from the SKILLS tab, re-mine the same cluster. Before the fix this fails by finding the generated markdown where the user's text was. It asserts the preconditions too (file on disk, absent from the loaded list), so it cannot silently stop testing the thing.auto_creation_never_writes_into_a_skills_dir_the_session_may_not_read— the authority fix, with a control proving the same log does create a skill once the workspace scope is allowed.Existing guarantees pinned so this change cannot regress them:
auto_creation_still_caps_each_session_and_spares_loaded_skills— the per-session cap of 2 holds, stays capped within a session, resets next session, and the ordinary no-clobber-of-a-loaded-skill case still holds.a_forgotten_lesson_still_cannot_return_as_an_auto_created_skill— tombstone filtering still gates auto-creation.make gategreen: 3674 tests passed, 0 failed.Notes for reviewers
maindoes not currently compile (a duplicateMemoryCmdfrom Prelude: split memory.rs and main.rs so adaptive-context Phases 2 and 3 can run in parallel #733/The three #729 follow-ups: compaction, an ANN index, and ADR 0006's amendment #735 merging cleanly but incompatibly), so this branch sits onfix/main-ci-obsolete-baseline. The diff will reduce to my five files once fix: repair main — duplicate MemoryCmd broke the build, plus an obsolete baseline entry #739 merges. Nothing here depends on it beyond being able to build.stella-core's contract is enforced by documentation ("occupied_pathsMUST be the paths present on disk") rather than by the type system. Making it unforgeable would mean either I/O in the engine or a port/closure parameter; given the 1502-line ceiling on that file and the "keep it tight" bar for a data-loss fix, the doc plus the CLI-side tests seemed the right trade. Happy to revisit.