P1 subset: close the store-open races and stop the docs lying about the shell#764
Draft
macanderson wants to merge 2 commits into
Draft
P1 subset: close the store-open races and stop the docs lying about the shell#764macanderson wants to merge 2 commits into
macanderson wants to merge 2 commits into
Conversation
…he shell Lands the rows across the five P1 umbrella issues that need no maintainer ruling. The other ~79 rows are decision-gated by construction and are left alone; see the PR body for the audit. **#617 item 8 + 10 — concurrent first open of a workspace.** The version read that decides which migration to run sat outside the transaction that applies it, so two processes opening one fresh workspace could both act on the same observation. Closing that surfaced three further real races on the same path, each found by the test the issue asked for (four threads, one fresh path): - `apply_migration` re-applied a step another process had already stamped. Now IMMEDIATE and re-reads `user_version` under the write lock, so losing the race is the no-op it should always have been — which is also what makes a future non-replay-safe step safe to append. - `enterprise_export_migration`'s state row was inserted after an unguarded existence check, so the loser failed `Store::open` outright on `UNIQUE constraint failed`. Now `ON CONFLICT DO NOTHING`, matching what its sibling branch already did. - `PRAGMA journal_mode=WAL` ran before `busy_timeout` was set, and SQLite skips the busy handler on a WAL upgrade, so the one statement needing an exclusive lock had no retry budget. Now behind a bounded retry, mirroring `stella-media`'s `initialize_journal_database`, which already documented this exact hazard. - The generated `.stella/.gitignore` was written with check-then-rename, so concurrent openers all renamed over the target and a racer validating the winner's file saw `nlink() == 0` and failed closed. Now an `O_EXCL | O_NOFOLLOW` exclusive create; losers read the settled file. Verified end-to-end, not just in tests: five concurrent `stella init` runs on one fresh workspace report "database is locked" three times on `origin/main` and zero times here. Each fix was negative-controlled by reverting it and watching the specific failure return. The same version-read-outside-the-transaction shape was an unfixed audit note in `stella-context` and `stella-fleet`; both now use IMMEDIATE too, and the notes are replaced with what the code does. **#615 — the shipped docs inverted a security claim.** #710 moved `bash` and the key-free web family to registered-by-default and never swept the prose, so every release since told operators the opposite of the surface they had: `README.md`, `AGENTS.md`, `stella-tools/README.md`, `docs/why-stella.md`, `docs/design/threat-model.md` and the semantic-resolution ADR all still said the shell was opt-in. The model was told too — `tool_steering!` carried "There is no shell unless the workspace enables it", and the prompt's own test pinned that false sentence. `web.rs` additionally justified having no SSRF guard (localhost, cloud metadata) on the grounds that the family was opt-in, a compensating control that no longer exists; that is now recorded as an open ruling rather than a retired guarantee presented as current. `stella-tools/tests/doc_truth.rs` gates all of it so the claim cannot invert again silently. Item 1 (the `run_tests` shell injection) was already fixed in #624 — verified, including its negative control. The `command.started` fence gap on `screenshot` / `ci_status` / `start_work_on_issue` is recorded at `command_line_for` rather than fixed: closing it makes those tools newly deniable by an existing policy, which is an operator-visible change. **#618 item 5, 17 · #616 item 8 — three small ones.** - Journal failures were reported as `MediaError::Artifact`, sending operators to `.stella/artifacts/` for a fault in the journal sidecar. New `Journal` variant. - The speculation `select!` had an `unreachable!` on a provider-driven path; it now reports a typed terminal error (invariant 5 outranks asserting a structural claim). - `upsert` handed the embedder every missing body in one request, so a full-workspace first index put the whole corpus in a single call. Batched at `warm::BATCH`, the width the warm indexer has always used. The file-size baseline moves three lines: the growth in `driver.rs`, `stella-store/src/lib.rs` and `registry.rs` is in-place code plus its rationale and cannot be extracted. The 50-line doc-truth test was moved to its own file rather than raising a baseline for it.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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
Both rows were left out of the previous commit because both destroy data. The owner ruled: fix the root cause before sweeping, and constrain new fleet ledgers only. **Item 12 — the orphan session-sidecar sweep.** The blocker was that `SessionRegistry::list` cannot tell "no record" from "unreadable record": it drops anything that fails to parse (`.ok()?`), and `upsert`'s own comment records that a power cut leaves a zero-length `<id>.json`. A sweep keyed on what `list` accounts for therefore deletes the sidecar of a session whose record was merely truncated — and `history.json` in there is the only continuable copy of the conversation. So the conflation is fixed first. `scan` reports three states instead of one: healthy records, `damaged` records (present but unusable, sidecar intact), and `orphan_sidecars` (no `<id>.json` beside them at any readability). The orphan test is answered from the directory entry, never from a parse. `list` now delegates to `scan().healthy`, so there is one implementation. `prune_orphan_sidecars` then deletes only genuine orphans, re-checking for a record under the same name immediately before removing — a session that started since the scan has written one. Damaged records are reported rather than repaired, and the type says why: `id`, `pid` and `started_at_ms` are recoverable from the filename (ids are self-minted `ses-<ms>-<pid>`), but `workspace` is held nowhere else on disk — not in `journal.jsonl`, not in `history.json` — and resuming needs it. Silently hiding the session (today) and silently deleting it (as filed) are both wrong; surfacing it is what is actually available. The power-cut test asserts the whole chain: `list` cannot see the record, `scan` calls it damaged with its sidecar noted, the sweep is a no-op, and `history.json` survives. Negative-controlled against the filed definition (orphan = "not in `list`"), which classifies that session as an orphan and would delete it. **Item 5 — the fleet run references.** Retrofitting `REFERENCES runs (id)` onto a deployed `fleet.db` cannot succeed while an orphan row exists: the migration runner aborts on `pragma_foreign_key_check`, so the file stops opening, and the only way through is deleting fleet history — which tasks ran, what was attempted, what it cost. The issue filed this as a routine schema tidy-up and did not say so. Fresh files now get the constraints via a new `FRESH_SCHEMA` (the current post-v2 shape plus the four `run_id` references), so a ledger created by this build cannot acquire an orphan. Existing files take the unchanged ladder and are not touched. The tradeoff is stated at `FRESH_SCHEMA`: `SCHEMA_VERSION` no longer determines shape alone, so a future step rebuilding one of these tables must reproduce the right variant or read the DDL from `sqlite_master`. `Ledger::orphan_rows` reports what an unconstrained file holds, and `enforces_run_references` answers which variant a file is — read from `sqlite_master`, because both are v2. That report is surfaced by a second `stella doctor` check rather than landing as uncalled API. It passes either way: inert rows on an old ledger are not a fault, and failing the command over them would make `doctor` unusable on exactly the workspaces with history worth keeping. `--repair` deliberately does not act on it, asserted by a test that checks the rows are still there afterwards. Verified against the real binary: a legacy ledger holding 4 orphan rows across 3 tables reports each class with its count and an explanation, exits 0, and `stella doctor --repair` leaves every row and the $4.20 of recorded spend intact. `doctor`'s module doc said "today there is exactly one check"; it now describes both, and the count assertion in its tests checks names rather than a number.
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 this is
The ask was "fix all P1 issues in one PR." That is not possible as stated, and the reason is structural rather than a matter of effort — so this PR lands the part that is landable and puts the rest in front of you as decisions.
I classified all 105 open checkbox rows across the six P1 issues against the code on
main:79 of 105 rows cannot be closed by anyone but you, and the issues say so themselves — each opens with "What decision this asks for." #618's two answers (take one coordinated protocol break vs. declare the types frozen) lead to opposite diffs and it explicitly notes freezing is "equally valid and much cheaper." #616's rows are blocked on numbers nobody has written down. #617 needs a ruling on two destructive operations. Guessing any of these wrong is expensive, so I didn't.
This PR is the 17 unblocked rows minus the cloud/serve surface you asked me to skip — and it grew, because closing one of them surfaced three further real bugs on the same path.
#617 items 8 + 10 — four races on the workspace-open path
Item 8 was "the version read is outside the transaction." Fixing it and adding the test the issue asked for by name (four threads, one fresh path) turned up three more, all real:
apply_migrationre-applied a step another process had already stamped. NowIMMEDIATE, re-readinguser_versionunder the write lock. Losing the race is now the no-op it should always have been — which is also what makes a future non-replay-safe step (ALTER TABLE … ADD COLUMN) safe to append.Store::openfailed outright withUNIQUE constraint failed: enterprise_export_migration.singleton— an existence check followed by an unguarded INSERT. NowON CONFLICT DO NOTHING, which is what its own sibling branch already did.PRAGMA journal_mode=WALran beforebusy_timeoutwas set, and SQLite skips the busy handler on a WAL upgrade — so the one statement needing an exclusive lock was the one statement with no retry budget. Now behind a bounded retry that mirrorsstella-media'sinitialize_journal_database, whose doc comment already described this exact hazard. The main store never got the same treatment..stella/.gitignoreused check-then-rename, so concurrent openers all renamed over the target; a racer validating the winner's file sawnlink() == 0and failed closed with "must be an owner-controlled single-link regular file" — for a file nothing had attacked. NowO_EXCL | O_NOFOLLOWexclusive create; losers read the settled file. Two new tests pin that a planted symlink is still rejected and that the file stays committable-mode (0644), not 0600.Verified end-to-end, not just in tests. Five concurrent
stella initruns against one fresh workspace:database is lockedorigin/mainEvery fix was negative-controlled by reverting it and confirming the specific failure came back. The four-thread test went 40/40 and the store suite 6/6 under full parallel load.
The same version-read-outside-the-transaction shape was sitting as an unfixed audit note in
stella-contextandstella-fleet— the fleet one explicitly warning that a future non-replay-safe step would be applied twice. Both now useIMMEDIATE, and the notes describe what the code does.#615 — the shipped docs inverted a security claim
#710 moved
bashand the key-free web family to registered-by-default and never swept the prose. Every release since has told operators the opposite of the surface they actually had:README.md("Bash is opt-in. The default tool surface has no free-form shell tool")AGENTS.md,stella-tools/README.md,docs/why-stella.md,docs/design/semantic-resolution-bridge.mddocs/design/threat-model.md— "bashis opt-in… the default surface has no shell at all"tool_steering!carried "There is no shell unless the workspace enables it", andprompt.rs's own test pinned that false sentence in placeA false claim in a threat model is worse than no claim, and a false one in the system prompt is read every turn.
web.rsis the sharpest case: it justified having no SSRF guard (localhost, cloud metadata endpoints reachable) on the grounds that the family was opt-in — "the gate is the settings opt-in, not a network allowlist." That gate no longer exists. R4 in the threat model says the same. Both now record it as an open ruling rather than presenting a retired guarantee as current. I did not add an SSRF guard: that's strict-vs-permissive and would break "fetch my internal dev server."New
stella-tools/tests/doc_truth.rsgates all of it, so the claim cannot invert silently again. It caught one instance I'd missed while I was writing it.Item 1 (the
run_tests→bash -cinjection) was already fixed in #624. I verified it properly rather than taking the checkbox's word: per-tokenshell_quote, and the test runs a realbash -cwith a negative control. Tick it and don't reopen.Not fixed, recorded instead:
screenshot,ci_statusandstart_work_on_issuecomposebash -clines that never reachcommand_line_for, so a policy denying command execution cannot see three real shell invocations — contradicting the MUST stated right there in the doc comment. None is injectable (all shell-quote). I left it as a note at that function because closing it makes those tools newly deniable by an existing operator policy, which is an operator-visible change that should land deliberately.Three small ones
MediaError::Artifact, pointing operators at.stella/artifacts/for a fault in the journal sidecar. NewJournalvariant. (Safe:MediaErrorisn'tSerializeand has no exhaustive match anywhere.)select!hadunreachable!on a provider-driven path. Now a typed terminal error; invariant 5 outranks asserting a structural claim.ProviderError::Terminalalready existed, so the "fresh error-semantics decision" this row was deferred on was already moot.upserthanded the embedder every missing body in one request, so a full-workspace first index put the whole corpus in a single call. Batched atwarm::BATCH, the width the warm indexer has always used. Witness test asserts no request exceeds it; negative-controlled at 133 texts.Things worth your attention
remove_dir_allsession dirs containinghistory.json— the LLM conversation, irrecoverable, and the only thing making a session continuable. The false-positive path is concrete:list()swallows unparsable records with.ok()?, andupsert's own comment documents that a power cut can leave a zero-length<id>.json. The exact failure the fsync work was added to survive is the one that makes a live session look orphaned and get deleted. It needs an explicit go/no-go.fleet.dbcan't succeed while orphan rows exist —apply_migrationhard-fails onpragma_foreign_key_check— so the only way through is deleting fleet history.publish = falseis set workspace-wide, so "public API break" always means an in-repo compile-and-fix, never an external consumer — the same premise that already retired three of that issue's four checked rows.merge_grouptrigger inci.yml. Not something I should flip unilaterally, and the issue itself says to verify the auto-merge interaction against GitHub's current UX rather than assume.Gate
make gateexits 0 (checked directly — piping it totailmasks the exit code). Rebased ontoeb663f0d;origin/mainhad moved 0.5.52 → 0.5.56 under me and #760 restructuredAGENTS.md, so the doc-truth test and baseline were re-verified after the rebase.The file-size baseline moves three lines.
driver.rs(+11),stella-store/src/lib.rs(+13) andregistry.rs(+5) are in-place code plus the rationale the surrounding style expects, and can't be extracted. The 50-line doc-truth test was extracted to its own file rather than raising a baseline for it.Update — #617 items 12 and 5, per the owner's ruling
Both destructive rows are now handled, neither by deleting anything.
Item 12: fix the conflation, then sweep
The sweep was unsafe because
SessionRegistry::listcannot tell "no record" from "unreadable record" — it drops whatever fails to parse, andupsert's own comment records that a power cut leaves a zero-length<id>.json. Anything keyed on whatlistaccounts for therefore deletes the sidecar of a session whose record was merely truncated.So the conflation is fixed first.
scan()now reports three states wherelistreported one:healthydamaged<id>.jsonpresent but unusable (zero-length / truncated) — sidecar intactorphan_sidecars<id>.jsonbeside it at any readabilityThe orphan test is answered from the directory entry, never from a parse.
list()delegates toscan().healthy, so there is one implementation rather than two definitions that can drift.prune_orphan_sidecars()re-checks for a record under the same name immediately before removing, so a session that started since the scan is safe.Damaged records are reported, not repaired, and the type documents why:
id,pidandstarted_at_msare recoverable from the filename (ids are self-mintedses-<ms>-<pid>), butworkspaceis held nowhere else on disk — not injournal.jsonl, not inhistory.json— and resume needs it. Silently hiding the session (what happens today) and silently deleting it (as filed) are both wrong; surfacing it is what's actually available.The witness test asserts the full chain —
listcan't see the record,scancalls it damaged with its sidecar noted, the sweep is a no-op,history.jsonsurvives — and is negative-controlled against the filed definition (orphan = "not inlist"), which classifies that session as an orphan and deletes it.Item 5: constrain new ledgers, report on old ones
Fresh
fleet.dbfiles get the fourREFERENCES runs (id)constraints via a newFRESH_SCHEMA, so a ledger created by this build cannot acquire an orphan. Existing files take the unchanged ladder and are never rebuilt — no migration can brick afleet.db, and no history is deleted.The tradeoff is stated at
FRESH_SCHEMArather than left implicit:SCHEMA_VERSIONno longer determines shape by itself, since both variants are v2. A future step that rebuilds one of these four tables must reproduce the right variant or read the DDL fromsqlite_master. That's a real wart; the alternatives were deleting history or leaving new databases unconstrained forever.Ledger::orphan_rows()reports what an unconstrained file holds andenforces_run_references()says which variant a file is. Rather than land as uncalled API, it's surfaced as a secondstella doctorcheck. It passes either way — inert rows on an old ledger aren't a fault, and failing the command over them would makedoctorunusable on exactly the workspaces with history worth keeping.--repairdeliberately does not act on it, asserted by a test that checks the rows are still present afterwards.Driven against the real binary on a legacy ledger holding 4 orphans across 3 tables:
After
stella doctor --repair: all 4 rows and the $4.20 of recorded spend still there.doctor's module doc claimed "today there is exactly one check" — corrected, and its test now asserts check names rather than a count.make gateexits 0. Two rustdoc failures caught on the way (a public doc linking a privateFRESH_SCHEMA, and aFleetLedgertype that is actuallyLedger) are fixed.