fix: repair main — duplicate MemoryCmd broke the build, plus an obsolete baseline entry#739
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).
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
🧙 Sourcery has finished reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
…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.
macanderson
changed the title
macanderson
added a commit
that referenced
this pull request
Jul 26, 2026
… list, not the filesystem (#742) 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.rs` `decide_auto_creation` asks whether the target path is in `existing_paths`. `stella-cli` built that list from `self.load_skills()` — the **loaded, filtered** skill list — and then did an unconditional `std::fs::write`. So the guard answered *"is this a skill I successfully loaded and kept?"*, never *"does this file exist?"*. `AutoCreateSkip::FileExists` was 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`: 1. `include_workspace_skills == false` — the workspace dir is passed as `String::new()`, so nothing from it loads; 2. `filesystem_settings_disabled()` — returns an empty `LoadedSkills`; 3. `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-core` stays pure.** It is "the engine — no I/O, fastest to test" and has essentially no filesystem dependency; putting `path.exists()` in `decide_auto_creation` would 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 shape `rules::decide_promotion(approve, file_exists)` already uses. - `stella-cli` gains `skill_paths_on_disk`, which enumerates the `*.md` files **actually present** in the target directory, rebuilding path strings the same way the guard builds its target so they compare exactly. - The loaded paths are still unioned in. They cost nothing, they carry the "this is a known skill" signal, and they keep the guard armed for already-loaded skills if the directory read ever fails. - The parameter is renamed `existing_paths` → `occupied_paths` and documented as a filesystem fact. The old name is what invited the confusion, and the rename is line-neutral — `stella-core/src/skills.rs` sits exactly on its 1502-line baseline ceiling, so no `mod tests` extraction was needed. Structurally this closes all three triggers at once: the guard no longer depends on `load_skills()` at all. ## Authority incoherence `auto_create_skills` computed its write target from `workspace_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_skills` is 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-core` only 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 gate` green: **3674 tests passed, 0 failed**. ## Notes for reviewers - **Stacked on #739.** `main` does not currently compile (a duplicate `MemoryCmd` from #733/#735 merging cleanly but incompatibly), so this branch sits on `fix/main-ci-obsolete-baseline`. The diff will reduce to my five files once #739 merges. Nothing here depends on it beyond being able to build. - **#736 contains a test that deliberately pins the current buggy behavior**, written to prove a migration was behavior-compatible. This fix invalidates that pin. Expected and already flagged — whoever rebases #736 should update it to assert the file survives rather than that it is overwritten. - `stella-core`'s contract is enforced by documentation ("`occupied_paths` MUST 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.
macanderson
added a commit
that referenced
this pull request
Jul 26, 2026
…ader (#745) **`main` does not compile.** ``` error[E0432]: unresolved import `tuning::session_lifecycle_enabled` --> stella-cli/src/memory.rs:78:9 ``` ## Cause Phase 2 (#738) and Phase 3 (#736) were built in parallel, and each independently needed to read `context.lifecycle.enabled`. Neither could see the other, so both wrote the reader — with a **byte-identical body** and the same fail-closed reasoning, differing only in name and visibility: | | | |---|---| | Phase 3 | `pub(super) fn lifecycle_enabled(..)` | | Phase 2 | `pub fn session_lifecycle_enabled(..)` | Phase 3 merged first. Phase 2 merged second **from a rebase that predated it**, so `tuning.rs` kept Phase 3's function while `memory.rs` kept Phase 2's re-export of a name that no longer exists. Neither PR was wrong, and neither conflicted textually with the `main` that existed when it merged. ## Fix The resolution both branches were converging on anyway: one reader, `pub` because Phase 2's engine-config builder calls it from outside the module, documented with both phases' reasoning, and one call site updated. ## The pattern, since this is the third repair this week - #739 — #733 moved `MemoryCmd`; #735 was cut from an older main and re-added it. Zero textual conflict, nine type errors. - This one — same shape, one level down. **Parallel branches that merge cleanly are not thereby compatible.** A branch that lands second is verified against the main it was *rebased onto*, not the main it *lands on*, and nothing in the tooling notices the difference. Re-verifying the second branch against the actual post-merge tree is the step that catches this, and it is far cheaper than a red main. ## Gate `make gate` green: **exit 0, 3801 tests**, `check-file-size: OK — 475 Rust files, none over 1500 lines except 25 grandfathered (none grew).`
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.
mainis red, and in two ways. The second one is the real problem.1.
maindoes not compilecargo check -p stella-clifails with nineE0308s:Cause: two PRs that each merged cleanly and are each individually correct.
MemoryCmdintomemory_cmd.rs, to getmain.rsunder its size ratchet.mainthat predated that move, so it carried its own copy of the enum — and added two new variants,CompactandIndex, to that copy.Git merged both without a textual conflict. The result declares the enum twice:
Command::Memoryholds amemory_cmd::MemoryCmd, while the dispatch arms match on the stalecrate::MemoryCmd.This is the failure mode a textual merge is worst at — neither side touched the same lines, so nothing conflicted, and the incompatibility surfaces as a type error two files away from either change.
Fix, in the direction #733 intended: move #735's
CompactandIndexvariants ontomemory_cmd::MemoryCmdwith their doc comments intact, delete the stale duplicate frommain.rs, and qualify the dispatch arms.2. An obsolete baseline entry
This is what CI reported first, and it masked the compile error —
check-file-sizeruns before clippy and exits first.#733 deleted
main.rs's baseline entry when the file fell to 1337 lines; #735 re-added it at 1816. The file is now 1421 — simultaneously under the limit and exempted from it, which the ratchet rejects by design, since an exemption must not outlive the problem it covered.Verification
make gategreen: exit 0, 3670 tests,check-file-size: OK — 456 Rust files, none over 1500 lines except 27 grandfathered (none grew).stella memory --helplists all nine subcommands, so The three #729 follow-ups: compaction, an ANN index, and ADR 0006's amendment #735'scompactandindexsurvive the move — the variants were relocated, not dropped.Unblocks #736 and #738, which cannot get a meaningful CI signal against a
mainthat does not build.