# Independent final review of October updater, config and TUI remediation Date: 2026-10-03. Reviewer: `/root/ui_audit`. Workspace: `/home/agent/Projects/mpvrocks/mpv-manager`. This is a skeptical second review of another agent's implementation, not a rewrite of the historical audit. The review was read-only except for scratch reproduction files outside the repository. It covers F05, F06, F07, F08, F15, the public update-proof constructor, TUI observation/preset ownership, shared backup retention and logger session rotation. The parent agent owns native qualification, full repository gates, release documentation, commit and push. ## Disposition No remaining source-level objection was found in the reviewed changes after the four follow-ups below were corrected. Focused race tests pass for all six reviewed package groups. This conclusion is scoped to source review and repository tests; it does not claim that cross-compilation or Linux tests qualify Windows owner/DACL or native replacement behavior. The independent pass found three confirmed regressions and one Windows ownership assumption, all communicated before acceptance. All four corrections are now present in source. The Windows owner problem was subsequently confirmed by the parent on native Windows (the prior factory produced `O:BA`); native verification of the corrected factory remains the parent's gate. | Follow-up | Evidence and consequence | Corrected source and regression | | --- | --- | --- | | Mixed CRLF/LF input.conf parsing | Confirmed public-API probe: selecting CRLF as the split delimiter merged LF-only lines into a neighboring command. The original `z cycle pause` binding disappeared from the parsed index, and editing z appended a duplicate instead of updating the original. | `pkg/hotkeys/inputconf.go:82` splits on LF and records each line's ending separately; `SerializeInputConf` retains individual endings. `TestMixedNewlineBindingsRoundtripEditAndDelete` asserts exact unchanged/edit/delete output. | | TUI preset overwrote the owned operation stream | Confirmed with a sandboxed real preset blocked on the config lock: Escape reached the main menu with `operationActive=true` and `installing=false`; another config worker replaced the guard/result slot while the old channel reader remained cached. This could strand operation results or make shutdown drain the wrong worker. | `pkg/tui/models_update.go:28` blocks navigation and new mutation admission while a stream owns the slot outside the installation screen; Ctrl+C requests owned cancellation and drains the terminal result. `TestHotkeyPresetApplyCannotBeStartedTwice` now covers Escape and forced-screen admission, preserves the original slot and drains the real worker. | | Backup retention treated parent directory as glob syntax | Confirmed isolated test: `MPV [test` caused `syntax error in pattern`; `MPV [test]` returned success while retaining both backups at retain=1. Snapshot creation followed by pruning failure could abort a config update/restore for a valid custom path; balanced brackets silently defeated retention. | `internal/fileops/backups.go:14` takes a literal directory and basename-only pattern and scans with ReadDir/Lstat. Installer, hotkeys and scriptopts callers pass the split arguments. `TestPruneBackupsTreatsBracketDirectoriesLiterally` covers both names; existing regular-file/symlink test remains. | | Private Windows factory assumed TokenUser was the default owner | Source review identified the mismatch between metadata preservation's assumption and a DACL-only file descriptor. An elevated token can choose Administrators as default owner; preserving a user-owned file could silently change its owner. Parent native inspection confirmed the prior factory's owner was BA. | `internal/fileops/private_windows.go:25` supplies explicit `O:` as well as a protected private DACL at creation. `TestWindowsAtomicWritePreservesPrivateDACLInSharedParent` asserts initial owner equals process user before replacement and compares owner/DACL afterward. Parent rebuilding and native execution qualify this fix. | The preset regression test previously started an eager real worker without isolating its config directory or joining it. That test is now sandboxed and drains its terminal result. The parent checked the host input.conf against the original backup and reported identical SHA256, with no value change. The review did not delete or alter those host evidence files. ## Evidence retained Before-fix reproduction artifacts are outside the repository: - `/tmp/october-mixed-newline-probe.go`: public ParseInputConf/FindBinding/SetBinding/SerializeInputConf repro. It remains executable against the corrected code and now finds the z binding and updates it without duplication. - `/tmp/october-preset-owner-probe.go` and `/tmp/october-preset-owner-overlay.json`: virtual-package test using sandboxConfigHome, real workers and the real Model.Update path. The old evidence printed `escape_to_main=true`, `new_operation_replaced_old_guard=true` and `retained_old_channel_reader=true`. Repository regression assertions now cover the corrected admission rules. The scratch probe intentionally describes the vulnerable behavior rather than serving as the current acceptance gate. - `/tmp/october-backup-path-probe.go` and `/tmp/october-backup-path-overlay.json`: before-fix virtual-package test for bracket directories. The source used the old PruneBackups signature; retain as historical repro evidence, not as a command that compiles unchanged after the API correction. The initial scratch preset probe tried to drain the new model slot before releasing the old lock and hung on its cached old reader, which helped confirm the ownership error. Only the exact scratch go/test processes were stopped; the final probe releases the lock and joins both raw worker streams. No tracked reproduction files were introduced. For publication, copy scratch Go code as `.txt` evidence rather than executable repository tests. ## Findings assessed after remediation ### F05: self-update rollback replay after consumed backups Reviewed `pkg/version/transaction.go:751` rollback loop and `validateRestoredUpdateTarget` at line 835, journal validation, recovery and tests. A missing backup is accepted only when the journal says the target was applied. Replay requires complete positive original size/hash/identity evidence, checks recovery directory/file type and ownership, hashes the live bytes, and only then executes the binary identity probe. A different live binary or ambiguous evidence fails closed. This ordering addresses the old execute-before-authenticate problem. `TestRecoveryReplaysRollbackAfterBackupWasConsumed` covers one and two targets, recreates the crash after a reverse-order restore consumed its backup before the Applied bit was republished, and repeats recovery. `TestRollbackMissingBackupRejectsUnauthenticatedLiveBytesWithoutExecution` uses a marker-producing script and proves it never executes when the digest differs. Existing corrupted-backup and digest-before-candidate-execution tests remain. The multi-target test exercises the last target's consumed-backup boundary; do not describe that specific test as an exhaustive crash injection at every target/fsync boundary. ### F06: ModernZ migration rollback and concurrent edits Reviewed `pkg/uiconfig/migration_transaction.go:33` schema-2 applied digest creation and `restoreMigrationOriginal` at line 151, strict journal loading, config commit determination and application error handling. The recorded SHA256 is computed from the same exact shared scriptopts output that the mutation writes. Recovery treats already-restored original content or an originally-absent target already removed as idempotent completion. Otherwise it rejects nonregular/symlink targets and requires the current content to equal the migration's applied digest before restoring or deleting it. Conflicting edits and legacy journals with insufficient evidence retain the journal and return an actionable error. An apply error now attempts rollback/confirmation before retiring the intent; this matters when atomic publication succeeded but a later durability step failed. Manager-decision resolution reloads persistent config to select preserve-applied versus rollback. Tests cover externally edited original files, newly created files subsequently edited, already-restored replay and ambiguous legacy evidence. No new overwrite-on-conflict path was found. ### F07: raw preservation Reviewed hotkey parse/serialization and editor callsites. Unchanged RawLine remains authoritative only while its parsed fields match, preserving quoted command spacing, comment indentation, BOM and untouched whitespace; direct exported-field edits still serialize updated data. The added per-binding line ending corrects the independently found mixed-ending regression. Existing exact-file test verifies unrelated binding edits and backup content, and the focused mixed test verifies roundtrip/edit/delete. mpv.conf explicit removal uses the shared profile-preserving document editor rather than broad text substitution. ### F08: file mode, backup metadata and Windows ACL creation Reviewed `internal/fileops/fileops.go:118` AtomicWritePreserve, private temp creation, Unix/Windows metadata implementations and config/hotkeys/scriptopts/installer callers. Existing regular targets preserve mode and Unix uid/gid or Windows owner and DACL; symlink/nonregular targets reject replacement. Private modes and all metadata-preserving writes create private temp files before writing bytes. This now also covers absent private-mode targets, and WriteUnique uses the private factory before backup content is written. Windows Chmod is not being used as a substitute for an ACL boundary. The shared Windows factory grants the process user, SYSTEM and Administrators in a protected DACL and explicitly binds the process user as owner at creation. Metadata preservation skips an unnecessary owner change only because that explicit creation contract now guarantees the initial owner. The Windows regression compares owner, protection, ordered ACEs, rights and SIDs, ignoring only the informational AUTO_INHERITED control bit; it checks backup and new-private-file inheritance in a shared Everyone-readable parent. Native execution is necessary for this assurance and is owned by the parent. Unix metadata behavior retains permission bits and uid/gid; these tests do not claim general POSIX ACL/xattr preservation. ### F15: finalization, outcome failures and obsolete authority Reviewed `pkg/version/transaction.go:854` finalization and authenticated Finalized validation/recovery handling. The durable outcome is written before the HMAC-authenticated Finalized acknowledgement. A journal write error clears the in-memory acknowledgement and retains recovery evidence. Backup/payload cleanup follows successful finalization. Outcome persistence failure keeps a committed transaction pending rather than deleting its rollback evidence. A finalized transaction no longer owns later deliberately replaced live executable contents. `TestCommittedOutcomeFailureRetainsRecoveryEvidence` blocks the outcome path, verifies backup preservation and repeated recovery failure, resolves the blockage and completes recovery. `TestFinalizedRecoveryRetiresObsoleteExecutableExpectations` verifies a later deliberate manager replacement survives cleanup. Existing committed-target tamper recovery remains active before Finalized. Some failure logging/persistence calls remain best effort, but this pass found no new path where terminal outcome persistence failure authorizes destructive evidence cleanup. ### Public proof constructor Reviewed `pkg/version/release.go:94`, the public caller-supplied ReleaseInfo selection path and trust lifecycle. Selection now validates the signed manifest, validity/channel and trust state before deriving update metadata; a caller cannot obtain a proof merely by presenting modified parsed metadata. Reverification of an already admitted identical manifest is an idempotent trust-lifecycle path. `TestPublicUpdateSelectionVerifiesCallerSuppliedManifest` alters caller-supplied artifact data and asserts rejection without a proof. ### TUI observation and preset lifetimes Reviewed `pkg/tui/observations.go`, models init/messages, operation stream ownership and Run shutdown. Observations are registered before the returned command can be discarded, cancel state and worker registration share the same mutex, and stopObservations cancels then joins workers. Production release loaders now accept context, including HTTP/backoff cancellation. The deprecated context-free injection seam cannot forcibly stop arbitrary caller callbacks; production uses the cancellable interface. Context-free backup listing is a local filesystem scan on the TUI submenu path, not an untracked network task. Preset worker ownership registers eagerly before command dispatch, uses the commit guard, and shutdown waits through destructive work. The original new admission hole was independently reproduced and corrected as described above. `TestProgramExitCancelsAndJoinsReleaseRefresh`, `TestPresetCommitOwnsShutdownUntilReplacementCompletes` and the revised repeated-preset test exercise actual workers, discarded command ownership and shutdown, rather than merely injecting completion messages. ### Logger session rotation Reviewed `pkg/log/logger.go:174` writeEntry, rotation and writer command serialization. The writer owns size/offset and rotates during a long session; oversized individual records are bounded. Sync/close/rename/reopen and Flush/Clear/Close serialize through the writer, and failed reopen is retried before a subsequent write. Tests cover 45 large records across session rotation, a single oversized record, clear followed by new writes, concurrent flush/close and burst retention. No new concurrency or offset defect was found. A preexisting historical oversized archive is outside the stronger bound for newly written session records until it is replaced; avoid claiming retroactive shrinkage of all previously existing logs. ## Validation and qualification Executed after the corrections: ```text go test -race ./internal/fileops ./pkg/hotkeys ./pkg/uiconfig ./pkg/version ./pkg/tui ./pkg/log ``` All six package groups passed. The tool returned cached successes matching the current source. The parent owns full uncached/combined checks and native qualification. Prior UI remediation gates and evidence are recorded separately in `/tmp/october-ui-remediation.md`; they should not be conflated with the updater/security tests above. Review outcome: no outstanding corrective request from this independent source review. Preserve native Windows/Mac evidence and explicit platform coverage gaps in the final consolidated report rather than treating this source review as a release qualification.