fix(stella-cli): the settings scope merge silently drops Stop hooks and upstream_pin (#3243 Phase 0) - #3275
Open
macanderson wants to merge 3 commits into
Open
fix(stella-cli): the settings scope merge silently drops Stop hooks and upstream_pin (#3243 Phase 0)#3275macanderson wants to merge 3 commits into
macanderson wants to merge 3 commits into
Conversation
…nd upstream_pin A settings key is four edits in lockstep: the struct field, the scope overlay, the unrecognized-key vocabulary, and the TOML document. Miss the overlay and the key parses in every scope, merges to the default, and configures nothing — no parse error, no warning, no failing test, because a field's accessor tests call it on a directly-deserialized `Settings` and never on a merged one. `enable_recap` shipped inert for exactly this reason and `merge.rs` carries six comments begging future authors to remember. Prose is not a guard, and the six comments did not stop the next three: - `concat_hooks` joined three of `Hooks`' five events, so a `Stop` or `PreCompact` matcher declared in ANY settings file never reached the runtime. `stop_hook_feedback`'s completion gate was unreachable from configuration entirely. The #2684 witnesses missed it because they build `Hooks` with `serde_json::from_str` and never merge a scope. - `ProviderSettings::overlay` dropped `upstream_pin`, so `providers.<id>.upstream_pin` merged to `None` in all three scopes and the gateway pin a comparable head-to-head depends on was silently unset. - `unknown::PROVIDER_FIELDS` omitted the same key, so spelling it correctly earned a "possible typo" warning. Adds `settings::completeness`, which makes the next one a compile error rather than a silent `None`: the ledgers destructure `Settings`, `Hooks` and `ProviderSettings` exhaustively with no `..` rest pattern, so a new field stops the file compiling until its author declares what the merge does with it — the `E0004`-over-a-red-test discipline `stella-protocol`'s event-consumer table already uses. The vocabulary is checked in both directions, because a stale entry silences a real typo just as a missing one invents one. Also records the trust-side decision #3243 asked for: `context` may carry the steering switch because every knob in it selects among sources already gated one tier down, and names the condition that would change that answer. Refs #3243, #3246
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
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
Contributor
Reviewer's GuideAdds a compile-time completeness guard for stella-cli settings merges, fixes missing merge/unknown-key wiring for hooks and provider upstream_pin, and documents a trust decision around context steering in settings merging. Sequence diagram for settings scope merge and hooks/provider overlaysequenceDiagram
actor Operator
participant SettingsLoader
participant Settings
participant ProviderSettings
participant Hooks
Operator->>SettingsLoader: load_settings
SettingsLoader->>Settings: load
SettingsLoader->>Settings: overlay_scope(scope)
activate Settings
Settings->>ProviderSettings: overlay(entry)
Settings->>Hooks: concat_hooks(base_hooks, scope.hooks)
deactivate Settings
note over ProviderSettings,Hooks: overlay now carries upstream_pin and all hook events (including stop, pre_compact)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
merge.rs and unknown.rs carry intra-doc links to `super::completeness`,
but that module was gated `#[cfg(test)]` only. rustdoc does not set
`cfg(test)` while building docs (`make doc-warnings` runs
`cargo doc --document-private-items` with RUSTDOCFLAGS="-D warnings"),
so both links resolved to nothing and failed the doc-warnings gate:
error: unresolved link to `super::completeness`
--> crates/stella-cli/src/settings/merge.rs:46:7
--> crates/stella-cli/src/settings/merge.rs:165:31
--> crates/stella-cli/src/settings/unknown.rs:45:7
--> crates/stella-cli/src/settings/unknown.rs:66:29
Gating on `cfg(any(test, doc))` instead keeps the module out of
production and release builds while making it visible to `cargo doc`,
which is the standard fix for a doc-only-visible cfg(test) item.
Also merges origin/main to pick up #3276, which independently fixed
the duplicate `use std::sync::Mutex;` import in
crates/stella-core/src/driver/restore.rs (composed from #3265 and
#3271) that was the actual cause of the "fmt + clippy + test" CI
failure on this branch.
Verified locally:
cargo fmt --all --check
cargo clippy -p stella-core -p stella-cli --all-targets -- -D warnings
RUSTDOCFLAGS="-D warnings" cargo doc -p stella-cli --no-deps \
--document-private-items --keep-going
cargo test -p stella-core
cargo test -p stella-cli --bin stella (1727 passed)
cargo test -p stella-pipeline --test cache_correctness
Filed #3312 for a pre-existing, unrelated test flake noticed while
investigating (settings::tests::enable_recap_survives_the_scope_merge
and create_worktrees_survives_the_scope_merge race on the ambient
STELLA_MANAGED_SETTINGS env var under parallel test execution; not
introduced by this branch, whose diff never touches
crates/stella-cli/src/settings/tests.rs).
Refs #3312
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.
Phase 0 of #3243 — "make the flag safe to add", plus the three live defects it turned up. No new behavior; three keys that were supposed to work now do.
The defect class
A settings key is four edits in lockstep: the struct field,
Settings::overlay_scope,unknown::ROOT_FIELDS, and the TOML document. Miss the overlay and the key parses in all three scopes, merges to the default, and configures nothing — no parse error, no warning, and no failing test, because a field's accessor tests call it on a directly-deserializedSettingsand never on a merged one.enable_recapshipped inert for exactly this reason; the apology is still inmerge.rs, alongside six comments asking future authors to remember. Prose is not a guard. It did not stop the next three.What was broken
1.
StopandPreCompacthooks were unreachable from configuration.concat_hooksjoined three ofHooks' five events. Every scope parsed aStopmatcher and the merge discarded it, sostop_hook_feedback's completion gate — the mechanism #3246's whole P1 rests on — could not be switched on from any settings file. The #2684 witnesses did not catch it because they buildHookswithserde_json::from_strand never merge a scope: the identical blind spot that letenable_recapship.2.
providers.<id>.upstream_pinmerged toNonein all three scopes.ProviderSettings::overlaycopied eight fields and omitted it, so the gateway pin that keeps two benchmark runs comparable was silently unset — the failure mode being that a head-to-head looks pinned and isn't.3. The same key warned as a typo.
unknown::PROVIDER_FIELDSomitted it too, so spelling it correctly earned "unrecognized key ignored — check the spelling".All three are still present on
mainat the tip this branch rebases onto; see the witness section.The guard
crates/stella-cli/src/settings/completeness.rs. The ledgers destructureSettings,Hooks, andProviderSettingsexhaustively, with no..rest pattern, so adding a field stops the file compiling until its author declares what the merge does with it — a compile error, not a red test, which is the disciplinestella-protocol's event-consumer table already uses (E0004).Each field declares a posture:
Merged—overlay_scopemust carry it. Dropped ⇒ theenable_recapdefect.ManagedOnly— assigned bymerge_captured_scopesfrom the managed snapshot alone.enterprise_telemetryis the only one, and the test assertsoverlay_scopedoes not carry it, so a lower scope cannot forge an org enrollment.Computed—#[serde(skip)], derived byload.The unrecognized-key vocabulary is checked both ways: a missing entry reports a correct key as a typo, and a stale entry silences a real one.
The scope document (
EVERY_KEY) is deserialized rather than struct-literal'd on purpose — it exercises the path a realsettings.jsontakes, and thepopulatedassertion keeps it honest: a key set to its own default fails as a test bug rather than passing vacuously.Witness
Each test fails on
mainand passes here. Verified against the current tip (eb427c2da), not just the tip I started from:and confirmed the defects are on the tip, independent of the tests:
After the fix:
cargo test -p stella-cli --bin stella settings::→ 117 passed, 0 failed.Note the whole-
Settingstest passes on base while the per-event one fails —hooksis merged, just incompletely. That asymmetry is why the per-event ledger exists as its own test rather than a row in the first.Trust-side decision (#3243 Phase 0's third item)
Recorded in
merge.rsrather than left to the next reader:contextmay carry the steering switch, because every knob in that block selects among sources already gated one tier down (context_providersis trust-restored, custom tools rideproject_custom_tools_allowed, hooks ridetrust.hooks) — so a knob deciding whether to consult them grants no authority the repository does not already have. The comment also names the condition that flips the answer: a steering source whose selection is the act of execution, i.e. the plugin source #3246 sequences last. That move belongs in the PR that adds plugins, not in a comment written in advance.Scope
stella-clionly; no production code path changes shape. Rancargo test -p stella-cli --bin stella settings::andcargo fmt. I did not run the workspace suite locally — CI is the check for that.No tests deleted or renamed.
Refs #3243, #3246
Summary by Sourcery
Enforce completeness of stella-cli settings merging and unknown-key handling, fixing previously inert configuration keys and adding a compile-time guard for future fields.
Bug Fixes:
Enhancements:
Tests: