Prelude: split memory.rs and main.rs so adaptive-context Phases 2 and 3 can run in parallel by macanderson · Pull Request #733 · macanderson/stella

macanderson · 2026-07-26T20:56:20Z

Pure structural move, no behavior change. This is the prelude that lets Phase 2 (#713) and Phase 3 (#714) be built on parallel branches instead of strictly in sequence.

The plan sanctions it directly — docs/design/adaptive-context-plan.md:33-35: "Where a phase must grow a file already at its baseline, splitting that file is an explicit first task, done as a pure move with no behavior change."

Why

Phase 2 and Phase 3 have disjoint deliverables, but three shared files would have collided in ways a textual merge resolves wrongly:

File	Phase 2 wants	Phase 3 wants	Hazard
`stella-cli/src/memory.rs` (929)	the recall half — telemetry, provenance	the learning half — reflection, mining	Both rewrite opposite halves; they meet in `SessionMemory` and merge clean while being wrong
`stella-cli/src/main.rs` (1796)	inspect surface	a `context` command group	Sat at exactly its ratchet ceiling, zero headroom
both migration ladders	store.db slot	store.db + context.db slots	`SCHEMA_VERSION = MIGRATIONS.len()` over an index-ordered array — two branches each appending "the next slot" produce a broken ladder git merges without complaint

What changed

memory.rs 929 → 403, split along the recall/learning seam into memory/recall.rs (315) and memory/learning.rs (262). memory.rs keeps the type, the constructors, and episode/taxonomy recording. Every public path is unchanged.
main.rs 1796 → 1337. Storage, scripts, and graph commands moved into owning modules, following the pattern already used by stats.rs and usage_cmd.rs. It is now under the 1500 limit, so its baseline entry is deleted rather than raised — the grandfathered count drops 28 → 27.
Migration slots reserved by comment at both append points: store.db v15→v16 for Phase 2, v16→v17 for Phase 3; context.db v6 for Phase 3. The comments explain the hazard so they read as self-explanatory later.

Behavior neutrality

Checked two ways, not asserted:

Every removed line reappears somewhere. The only removals without an identical counterpart are visibility widenings to pub(super), path qualifications (GraphOp → contextgraph::GraphOp), regrouped use blocks, and the deleted baseline row.
The binary was built on both trees and the generated clap help diffed for stella, graph, scripts, scripts list/run, storage, storage show/prune, memory, memory forget/edit, and observe — 387 lines, byte-for-byte identical.

Tests were deliberately not moved: memory/tests.rs interleaves recall, learning, skills, and private-state assertions, and a wrongly-moved test is worse than an unmoved one.

Gate

make gate green — exit 0, 61 test suites, check-file-size: OK — 443 Rust files, none over 1500 lines except 27 grandfathered (none grew).

Refs #713, #714, #469

…in parallel Pure structural move, no behavior change. The adaptive-context plan sanctions this explicitly (docs/design/adaptive-context-plan.md:33-35): a phase that must grow a file already at its ratchet baseline splits that file first, as a pure move. Phases 2 (#713) and 3 (#714) are being built on parallel branches. Their deliverables are disjoint, but three shared files would have collided in ways a textual merge resolves *wrongly*: - `stella-cli/src/memory.rs` (929 lines) held both the recall plane (Phase 2's telemetry and provenance work) and the learning loop (Phase 3's observation and mining work). Two branches rewriting opposite halves of one file meet in `SessionMemory` and merge clean while being wrong. Split along that seam into `memory/recall.rs` and `memory/learning.rs`; `memory.rs` keeps the type, the constructors, and the episode/taxonomy recording. - `stella-cli/src/main.rs` sat at exactly its 1796-line ceiling with zero headroom, and both phases add subcommand surface. Moved storage, scripts, and graph commands into owning modules, following the pattern already used by stats.rs and usage_cmd.rs. It now lands at 1337 — under the 1500 limit, so its baseline entry is deleted rather than raised. - Both migration ladders are silent-merge hazards: store.db's is an index-ordered array where SCHEMA_VERSION = MIGRATIONS.len(), and context.db's is an `if version < N` chain. Two branches each appending "the next slot" produce a broken ladder that git merges without complaint. Slots are now reserved by comment at both append points. Behavior neutrality was checked two ways: every removed line reappears (the only exceptions are visibility widenings to `pub(super)`, path qualifications, and the deleted baseline row), and the generated clap help for every moved command is byte-identical across 387 lines. `make gate` green: 61 test suites, file-size OK at 27 grandfathered files. Refs #713, #714, #469

sourcery-ai

Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

vercel · 2026-07-26T20:56:28Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
stella-cli-docs	Ignored		Jul 26, 2026 8:56pm

#735 merged to main while this branch was in flight and took context.db v6 and v7 (a compaction watermark and an ANN index), moving SCHEMA_VERSION 5 -> 7. This branch also defined a v6. Two different migrations, one version number, one database. WHY RENUMBER RATHER THAN RESOLVE THE CONFLICT IN PLACE The collision surfaces as a merge conflict rather than silently, which is the ladder working. But the obvious resolution is wrong: folding both bodies behind one `if version < 6` gate means a database already at user_version 6 — which every #735 user has — skips the second body entirely and is then stamped current while missing its tables. That is unrecoverable without a hand-written repair, on a file holding episodic memory that is not rebuildable. This branch moves because #735 targets main directly while this one is stacked behind the prelude (#733), so this lands last; renumbering the later-landing branch is standard, and it was one migration to move instead of two. VERIFIED SAFE STANDALONE, NOT ASSUMED The question is whether `if version < 8` is correct on a branch that has no v6/v7 blocks. It is, and the reason is structural: `migrate` is a flat sequence of INDEPENDENT `if version < N` gates — not an else-chain, not a match — terminated by a single stamp to SCHEMA_VERSION. Every gate therefore runs exactly when the database has not seen it, and a step appended at the end composes with steps that land before it later. Rather than assert that, `every_reachable_start_version_migrates_to_current_with_the_ledger` walks user_version 0..7 and checks each one ends at SCHEMA_VERSION with the ledger present AND usable — an append is attempted, and episode.lineage_id is checked — because a version stamp without the tables is precisely the failure being avoided, so asserting the stamp alone would miss it. Versions 5, 6 and 7 are the load-bearing rows: 5 is what a pre-#735 binary leaves, 6 and 7 are what a #735 binary leaves. The loop stops one short of SCHEMA_VERSION on purpose. A database at the current version cannot be reached by stamping one — `migrate` early-returns on equality, so such a fixture gets no tables and is not a state any real database can be in. Reaching the current version by actually migrating is covered by the two idempotence tests. THE RESERVED-SLOT COMMENT NOW RECORDS WHAT HAPPENED #733 reserved v6 for Phase 3 in a comment at the append point. #735 branched from main, never saw it, and deleted it. A reservation comment on a branch only binds branches built on that branch — worth writing down where the next person will look, rather than leaving a scheme that has already failed once looking authoritative. Refs #714

…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.

…ete baseline entry (#739) **`main` is red, and in two ways.** The second one is the real problem. ## 1. `main` does not compile `cargo check -p stella-cli` fails with nine `E0308`s: ``` error[E0308]: mismatched types --> stella-cli/src/main.rs:1140:17 = note: `MemoryCmd` and `memory_cmd::MemoryCmd` have similar names, but are actually distinct types ``` **Cause: two PRs that each merged cleanly and are each 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. 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 surfaces as 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` with their doc comments intact, delete the stale duplicate from `main.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-size` runs 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 gate` green: **exit 0, 3670 tests**, `check-file-size: OK — 456 Rust files, none over 1500 lines except 27 grandfathered (none grew).` - `stella memory --help` lists all **nine** subcommands, so #735's `compact` and `index` survive the move — the variants were relocated, not dropped. Unblocks #736 and #738, which cannot get a meaningful CI signal against a `main` that does not build.

… 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.

…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).`

sourcery-ai Bot reviewed Jul 26, 2026

View reviewed changes

macanderson marked this pull request as ready for review July 26, 2026 22:33

macanderson merged commit 84dbd41 into main Jul 26, 2026
9 checks passed

macanderson deleted the worktree-adaptive-context-prelude branch July 26, 2026 22:33

macanderson mentioned this pull request Jul 26, 2026

fix: repair main — duplicate MemoryCmd broke the build, plus an obsolete baseline entry #739

Merged

macanderson mentioned this pull request Jul 26, 2026

Never clobber a hand-edited skill: the no-clobber guard was testing a list, not the filesystem #742

Merged

macanderson mentioned this pull request Jul 26, 2026

fix: repair main — Phases 2 and 3 each shipped their own lifecycle reader #745

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Prelude: split memory.rs and main.rs so adaptive-context Phases 2 and 3 can run in parallel#733

Prelude: split memory.rs and main.rs so adaptive-context Phases 2 and 3 can run in parallel#733
macanderson merged 1 commit into
mainfrom
worktree-adaptive-context-prelude

macanderson commented Jul 26, 2026

Uh oh!

sourcery-ai Bot left a comment

Uh oh!

vercel Bot commented Jul 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

macanderson commented Jul 26, 2026

Why

What changed

Behavior neutrality

Gate

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

vercel Bot commented Jul 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant