refactor(tweak-system): fix snapshot loss on failed rollback, remove v1 profile system, add CI - #31
Merged
Merged
Conversation
This is a Windows-only application that is never built or run on another
platform, so the working tree should be CRLF unconditionally.
Previously .gitattributes was "* text=auto", which normalizes blobs to LF and
leaves the working-tree ending up to each developer's core.autocrlf setting.
That made CRLF a property of one machine rather than of the repository, and
eight tracked files had already drifted to LF in the working tree, including
build.rs, hosts_service.rs, firewall_service.rs and Cargo.toml.
- .gitattributes: "* text=auto eol=crlf", plus explicit binary declarations
for fonts and images so they are never converted
- .prettierrc: endOfLine auto -> crlf
- rustfmt.toml: newline_style Auto -> Windows ("Auto" preserves whatever a
file already has, which is how LF crept in)
- .editorconfig: end_of_line = crlf
Blobs remain LF-normalized in the object database, which keeps diffs, merges
and blame readable.
Planning artifacts for the tweak system remediation work. - docs/TWEAK_SYSTEM_REVIEW.md: audit of the Rust backend, build-time validator and YAML corpus against the docs-as-spec. 47 findings (41 confirmed by an independent adversarial pass, 5 plausible, 1 refuted), each with a failing scenario, evidence and a fix sketch. Refuted findings are retained and labelled rather than dropped. - docs/TWEAK_SYSTEM_PLAN.md: phased remediation plan. Records the execution order and, importantly, the three places where two independent research passes contradicted each other and how each was resolved -- notably the rejection of the windows-service crate, whose change_config() omits SERVICE_NO_CHANGE and would wipe service account passwords. - docs/adr/0001-0004: the design decisions taken deliberately, with their rejected alternatives, so they are not re-litigated later. - CONTEXT.md: project glossary. Separates "Stock Default" (what Windows ships) from "System Default" (what this machine was on before we touched it) -- conflating those two is the root of several doc contradictions.
apply_tweak discarded the rollback result and then deleted the snapshot
unconditionally:
let _ = backup_service::restore_from_snapshot(&snapshot);
backup_service::delete_snapshot(&tweak_id)?;
restore_from_snapshot can fail in two distinct ways. It returns
Ok(RestoreResult { success: false, .. }) when service, scheduler, hosts or
firewall restores fail and are collected, and a hard Err when the registry
phase aborts -- in which case the remaining phases never run at all. Both were
thrown away.
So an apply that failed after mutating services, and whose rollback then could
not restore them, left the machine half-changed, deleted the only record of the
user's original state, and reported nothing beyond the apply error. revert_tweak
afterwards answers "No snapshot found for this tweak". This also violated the
project's own rule in .github/copilot-instructions.md: do not silently ignore
privileged operation failures.
The snapshot is now released only when the rollback is verifiably complete.
Otherwise it is kept and every unrestored resource is reported, mirroring the
partial-failure shape revert_tweak already uses so the UI can surface detail
rather than a bare error string.
Secondary fix: `delete_snapshot(..)?` replaced the real apply error with a
file-deletion error. It is now logged instead of propagated.
The classification is extracted into `classify_rollback` so the decision that
was wrong -- when is a rollback good enough to release a snapshot -- is pure and
testable without Windows state. Six tests cover it; all four negative cases were
confirmed to fail against the reintroduced bug before the fix landed.
Implements docs/adr/0001 and docs/adr/0002.
build.rs collected tweaks into a HashMap and serialized it straight to tweaks.json. Rust's HashMap uses a randomly-seeded hasher, so the keys came out in a different order on every build: three build directories on this machine held three different 436,243-byte files, all from identical YAML input. That defeats build caching, makes the artifact useless as a diffable record, and makes it impossible to verify that a change to the YAML or to the parser left the output untouched -- which is exactly the check needed before swapping the YAML parser. Switching to BTreeMap makes the key order the tweak IDs' sort order. The runtime still deserializes into a HashMap; JSON object order is irrelevant there, so nothing downstream changes. Verified: two consecutive forced rebuilds now produce byte-identical output, and a deep comparison against the pre-change artifact confirms all 189 tweaks are semantically unchanged -- only the key order moved.
serde_yml is affected by RUSTSEC-2025-0068: unsound, every version affected, no patched release, and the upstream repository is archived. Not exploitable here -- the parser runs at build time over nine trusted in-repo files -- but there is no reason to carry a known-unsound unmaintained dependency. serde_yaml_bw over the other serde_yaml forks because it is the only one still alive. Last releases: serde_yaml_bw 2.5.6 (2026-05-02, six releases since Nov 2025); serde_yaml_ng 0.10.0 (2024-05-26); serde_norway 0.9.42 (2024-12-21); serde_yaml 0.9.34 (deprecated upstream 2024-03-25). serde_yml's own 0.0.13 release is its deprecation notice, pointing at noyalib -- a new crate by the same maintainer whose previous crate was archived for unsoundness, so not followed. Verified before adopting, made possible by the preceding determinism fix: - tweaks.json is byte-identical across all 189 tweaks, so no authored value parses differently under the new parser. - The deny_unknown_fields diagnostic still enumerates every valid field, so the build-time typo message an author sees is unchanged. - "value: null" still deserializes to None, producing the same "'set' action requires value" error. ADR-0004 is therefore unaffected, now confirmed against the parser actually in use rather than inferred from serde_yml's source.
The plan was largely executed (08fa2e6, 522d9e1, 5131569, 2621dda) but every checkbox still reads "- [ ]", so a finished plan looks pending. The boxes are deliberately left unticked rather than marked done: ticking them without re-verifying each task individually would replace honest ambiguity with a false record. Instead the header states what was carried forward and what no longer applies. Carried forward: Task 1 is genuinely incomplete. Its acceptance criterion required REG_BINARY values to detect consistently, but the unification reached detection.rs and stopped before inspection.rs, which still compares with raw JSON equality. A value authored in the supported "00,A0,FF" hex form matches in one and mismatches in the other. No longer applicable: Tasks 2, 3 and 6 concern the profile system, which is being deleted and rebuilt.
The v1 profile system is about to be deleted and rebuilt. The .mgx archive is self-describing -- a Deflate ZIP with a format_version field and plain JSON payloads -- with exactly one exception: the option content hash, which is embedded in stored profiles to resolve options that moved index, and which cannot be reconstructed from an archive alone. Records the three details that would silently break a reimplementation: the b"profile-option-v2" domain separator is hashed before the payload, the payload is compact JSON while the archive checksums use pretty-printed JSON, and the digest is truncated to the first 32 hex characters. Also records the legacy field-order variant that import still accepted. Also notes for the rebuild: system_state.json was exported and never read, per-tweak progress events were modelled and never emitted, and the hash's sensitivity to TweakOption's serde representation argues for a structural hash over meaningful fields rather than over serde output.
The profile system was incomplete -- system_state was exported and never read,
per-tweak progress events were modelled and never emitted -- and it is going to
be rebuilt rather than repaired. Carrying a half-built subsystem through the
remaining refactor stages would mean keeping a second apply path working for
code that is going to be thrown away.
Backend deleted (2,169 lines): models/profile.rs, commands/profile.rs, and
services/profile/{mod,archive,export,import,migration,validation}.rs, along with
their 7 Tauri commands, their module wiring, and 6 tests.
Dependencies dropped: zip, sha2, hex and hostname. All four were used only by
profile code -- notably hex, which looked like it was used by registry_value.rs
but is not: the apparent matches there are a local variable named `hex`, error
string prose, and test names.
Also removes RuntimeContext::windows_build, which the deletion orphaned.
Frontend kept, deliberately disabled rather than removed, so the feature reads
as "coming back" rather than "gone":
- The 6 invoke() bodies in src/lib/api/profile.ts are neutralized at that single
choke point; the ~195 lines of type definitions have no backend dependency and
still compile. Parameters are underscore-prefixed to preserve the signatures
for the rebuild.
- ProfileManager's onMount no longer calls loadSavedProfiles(); leaving it would
have produced an error toast every time the tab is opened.
- A banner explains the state, and the Export/Import entry points in
SnapshotsView and SettingsModal are disabled.
The archive format and the option content hash are recorded in
docs/spec/profile-v1.md, committed beforehand, since the hash cannot be
reconstructed from a .mgx file alone.
… drift Removes code with no remaining callers, verified by grep across the Rust source and the TypeScript frontend rather than by compiler warnings -- cargo check does not flag most of it, because a pub item in an rlib is API surface, not dead code. Only unregistering the Tauri commands made the compiler agree. Deleted: - models/backup.rs in full (RegistryKeyId, make_key_id, one test); nothing referenced it. - The second, unused YAML pipeline in models/tweak.rs: TweakDefinitionRaw, from_raw, TweakFile, validate, all_registry_keys, all_service_names, get_registry_changes_for_version, applicable_versions, and the four tests that existed only to exercise them. from_raw held a duplicate of the ti > system > admin inference rule; only the build.rs copy that actually runs remains, so the two can no longer disagree. - The `aliases` field. Its sole reader was the profile system's validation, so it became write-only when that was deleted. Removed from both models/tweak.rs and the build.rs mirror -- removing only one side would emit a field that TweakDefinition's deny_unknown_fields rejects, panicking on first tweak lookup. - Eight registered-but-never-invoked Tauri commands and their bodies: get_available_tweaks_for_version, get_tweaks_by_category, get_tweak, get_backup_system_status, cleanup_old_backups, apply_registry_as_system, delete_registry_as_system, get_debug_mode. None appears in any frontend invoke() call. cleanup_old_backups migrated away from a legacy exe_dir/backups layout and had never been wired to anything. - Error variants never constructed: UnsupportedWindowsVersion, StateLock, Timeout, and ProfileError, plus their code() arms. - The generated TWEAK_COUNT constant, which nothing read. CATEGORY_COUNT is kept; tweak_loader uses it. Added, in tweak_loader.rs: a test that forces the embedded tweak data to deserialize into the runtime types. build.rs parses YAML with a hand-maintained copy of these types and the runtime deserializes the result, so drift between them surfaces as a panic inside a LazyLock on a user's machine. This turns it into a test failure instead, and it earned its place immediately: it caught the aliases mismatch above when a careless `git checkout` reverted one side of it.
The project had no CI: .github/ contained only copilot-instructions.md and
hooks, so the gate documented there ("ALWAYS run bun run validate before
committing") was enforced by discipline alone.
Windows-only by design -- the app is never built or run elsewhere and the Rust
calls Win32 directly.
Four jobs:
- validate: runs the project's own `bun run validate` (prettier, tsc,
svelte-check, cargo fmt, cargo clippy -D warnings, eslint) plus
`cargo test --all-targets`.
- line-endings: asserts a fresh checkout really is CRLF. .gitattributes pins
this, but a violation otherwise surfaces as a confusing `cargo fmt --check`
failure against newline_style = "Windows" rather than as the line-ending
problem it is. This job caught two files on its first local run
(docs/spec/profile-v1.md and Cargo.lock), both fixed here.
- reproducible-build: builds twice from identical YAML and compares the sha256
of the generated tweaks.json. Guards the property established when build.rs
moved from HashMap to BTreeMap; without it the regression is invisible until
someone tries to diff the artifact.
- audit: rustsec/audit-check, which would have flagged RUSTSEC-2025-0068 in
serde_yml on its own.
RUSTFLAGS: -D warnings is deliberately not set globally -- it applies to
dependencies too, so an upstream warning would fail our build. The clippy step
inside `bun run validate` already scopes -D warnings to this crate.
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.
Remediation of a full audit of the tweak system (Rust backend, build-time
validator, YAML corpus) against the docs-as-spec. Planning artifacts are in
docs/TWEAK_SYSTEM_REVIEW.md,docs/TWEAK_SYSTEM_PLAN.mdanddocs/adr/.Rust: -2,397 net lines. Dependencies: 45 → 42. Tests: 61 → 57 (11 deleted with
their dead code, 7 added). No CI → CI.
Critical fix
apply_tweakdiscarded the rollback result and deleted the snapshot regardless: