fix: full-codebase design audit — write-back corruption, secret heuristic, contract repairs

[t41372]

What this is

A full-codebase design audit: one reviewer read the entire source (~28k lines minus locales/tests) in a single pass, hunting cross-file incoherence — modules that fight each other, code that violates AGENTS.md's own rules, CLI/TUI divergence, UX dead ends — on top of conventional correctness/security/perf review. Findings were verified against source line-by-line before fixing; the review then looped over the fixes themselves until it came back clean (4 rounds total, including a post-#33-merge round).

Fixes, worst first

Round 1 (fix: design-audit round 1) — 12 verified findings:

• Stored-copy corruption on the default add path (HIGH). The TUI add-review panel wrote parameter blocks back with read_text/write_text — universal-newline fold plus os.linesep re-expansion rewrote every line ending of the just-stored copy (CRLF-ifying it on Windows; first run then fails with $'\r': command not found), while its four sibling write-back sites each hand-rolled the correct bytes discipline. All six sites now share one pair: rewrite.read_for_block_edit / write_block_edit (surrogateescape, LF fold, newline restore, atomic + mode-preserving). The python onboarding lane's non-UTF-8 traceback and --normalize's non-atomic write go away with it.
• --max-tokens became a permanent password field (HIGH). is_secret_name matched substrings (TOKEN ⊂ MAX_TOKENS), and reader-reflected fields have no override surface — masked, never prefilled, never remembered, forever. Now whole-word matching (snake/kebab/camelCase/sentence-aware).
• skit remove prompted inside pipes/CI, violating the absolute non-interactive contract its own sibling runner remove implements (and SKILL.md claimed they behaved alike). It now takes --no-input and refuses with exit 2; preset delete (unrecoverable user data, previously deleted with no ask) gets the same confirmation ceremony. SKILL.md and the docs now tell the same story as the code.
• Cursor-move subprocess freeze. The Library detail pane rebuilt the form plan on every RowHighlighted — a synchronous pwsh spawn (30 s timeout) per cursor move over a PowerShell entry. Plans are now mtime-cached; the add-review panel memoizes its reader probe.
• One remembered extra-args tail, two expansion regimes. CLI replayed it literally, the TUI token/glob-expanded it — same state, different argv, and each side's comment claimed it matched the other. The tail's provenance is now recorded (extra_args_raw) and every replay follows it in both faces.
• Ctrl+O meant three things on three sibling screens while README documents one. Choose-variables → Ctrl+L (both screens), Preferences' squatters → Ctrl+G/Ctrl+Y, and AGENTS.md's key grammar now lists Ctrl+O/Ctrl+L plus the unclaimed-chord rule.
• Six LOWs: README ×3 stop over-claiming uv consent (non-tty downloads without asking, by design); params --manage dead ends now name the --add escape; non-UTF-8 python onboarding no longer tracebacks; ArgSpec moves to the neutral analysis module the four non-python readers were told not to reach around; params --json rows carry an additive binding key so kind no longer puns across sibling commands; preset-delete ordering (unknown-name feedback before the ask).

Round 2 (fix: design-audit round 2) — two regressions the re-review caught in round 1's own fixes:

• Word-boundary matching dropped the jammed spellings the old rule caught — APIKEY/AUTHTOKEN stopped matching, a C3 regression in the leak direction. Suffix rule + credential-qualifier KEY-compounds restore them (never MONKEY/TURKEY/HOTKEY).
• The new plan cache keyed only on the script's mtime, blind to meta.toml edits from a concurrent agent. Now keyed on both mtimes.

Tests (test: cover the design-audit fixes) — 2 new files (57 behavior tests: byte-exact CRLF/CR/non-UTF-8 round-trips including the TUI accept path, the 40-case secret matrix, exit-2 refusals, the provenance replay matrix across both faces, cache invalidation, key-remap pilots plus old-chord negatives) + 17 existing files adapted. No pragmas added; four provably-equivalent mutants documented instead of suppressed.

Round 4 (fix: design-audit round 4) — after rebasing onto merged #33: the review read all of #33 with perf-specific lenses (fast/slow path parity, cache freshness proofs, laziness vs contracts, locking) and came back clean at medium+; two lows fixed: extra_args_raw read with is True per the house rule for hand-editable bools, and AGENTS.md records #33's sanctioned read-path registry self-heal next to the contract it bends.

Round 13 (bae1a3a…f3af36f) — an external line-by-line review of this PR produced 8 findings; 6 were verified real and fixed, 2 were declined with evidence (auto-download under --no-input is the recorded A9 policy this PR did not change — it fixed the pre-existing infinite input() block; the process-global interaction latch is a documented trade-off, kept as a follow-up). A second review pass added 3 more, all fixed:

• resolve() refuses an ambiguous name. The round-9 sweep answered a name two hand-edited metas carried by returning whichever slug sorted first — executing an entry the user never picked. New AmbiguousNameError (usage exit) lists every claimant; a lone sweep hit is re-verified against its meta like the fast path.
• State writes join the exit taxonomy. argstate writers (forget included) fail as StateWriteError — an OSError subclass mirroring ConfigWriteError, so the run lane's persistence wrapper needed no change — mapped to the operational exit at the root boundary; the TUI notifies instead of dying (error-and-stay where the work is undone, warn-and-continue where it already happened). store.remove reports state-cleanup failure as honest partial success, and the CLI's three runner-pick memories are best-effort: a read-only state dir warns instead of vetoing the add/run the pick rode on.
• Secret transitions are fail-closed. All four param-edit lanes purge stale plaintext BEFORE committing the secret flag: no interruption can leave "schema says secret, plaintext still on disk". A prompt's managed list and declared rows commit in ONE meta write.
• Raw runs honor the C3 scrub. record_run(values=None) now strips the preserved snapshot; the raw lane feeds it the stored schema's own secrecy (grammar-free reads, per the A2 rule), re-read FRESH at stamp time and unioned with the launch-time set — a mid-run flip can only widen the scrub — and an entry removed mid-run gets no posthumous state.
• The Windows CI blocker was a test fixture (write_text newline translation), fixed as such; byte-preservation in production is untouched.
• Meta-test infrastructure: four source-reading meta-tests added on this branch (blind-spot ratchet, exit-route whitelist, and friends) read the REAL tree via conftest.real_repo_root — inside mutmut's rewritten mutants/ copy they measured the trampoline machinery and would have killed the nightly's stats phase on merge. Targeted mutation testing over every changed module followed, with each reported survivor re-verified against the live mutant; real gaps got killing tests, and the false-survivor mechanism found on the way is written up in mutmut reports false survivors: the cached test-selection map is partial and never repaired #38.

Round 14 (bf6e1ab) — the third external review pass raised one P1 against round 13's own boundary: the raw lane got re-resolve + secret-set union, but the normal lane every accepted run takes still trusted a launch-time slug and a launch-time secret set across a run that can last hours — and _unique_slug legally reissues a removed entry's slug to a later add, so post-run persistence could land on a stranger (added_at can't tell the owners apart; now_iso drops sub-second precision). Fixed as one universal mechanism, not a second local patch:

• Entries now have an identity. ScriptMeta.id (uuid4 hex), minted at the store's one meta-write door (_write_meta): adds reach disk stamped, every edit and rename preserves it, a meta from before ids existed parses as the wildcard and heals on its next write (reads stay reads), and a hand-mistyped id is meta corruption like a mistyped runner. Never surfaced in --json or registry rows; an unstamped id is omitted on serialization so legacy metas round-trip byte-identically.
• One guard, four doors. flows.persistence_target re-resolves the held entry and compares identities — gone, unreadable, or a different incarnation means write NOTHING. Every post-acceptance state write now passes through it: save_after_run (takes the Entry, no longer a slug), save_after_raw_run (refactored onto the shared guard), new save_preset_for (run --save-preset and the form's Ctrl+S — refusal is reported with the entry's name, never silent, because a preset is an explicit request), and new clear_remembered_tail (--forget-args — a vanished entry is vacuously forgotten; writing the clear would resurrect the state file remove() deleted). The strip set everywhere is the union of launch-time secrecy (the plan's, heuristics included) and both stored readings — a race can only widen the scrub.
• The census. A source test pins every remaining direct argstate writer call site in src/skit with its justification (immediate resolve→write command lanes hold no handle long enough to go stale); a new write path must use a door or amend the census in the open. The one surviving window of the same class — settings screens holding an Entry across user-paced time, writing through the store's re-resolving APIs — is recorded as TUI settings screens hold a stale Entry: saves can land on a reincarnated slug #39 with an expected_id design sketch.

Round 15 (a7ac6ac) — the identity-guard review: both P1s verified real against round 14's own code, and the P2's deferral overruled:

• Exact-match write authorization replaces the wildcard. The legacy-id wildcard ("" matches anything) let an upgrade user's long run land state on a reincarnated slug — unknown identity may serve reads, but it cannot authorize a write. persistence_target now requires exact id equality, and every lane that holds an Entry across user-paced time claims it at hold-start via store.ensure_identity (CLI run, the TUI's run/rerun/settings lanes and the post-edit reconcile picker): a pre-id meta is stamped before the hold begins, a stamped meta is never rewritten (its mtime keys the plan cache and row stamps), and a read-only library degrades honestly — nothing else can stamp there either, so "" == "" holds exactly where the ids cannot diverge.
• The doors are transactions now, not checks. persistence_target was check-then-act: verify, release everything, then write. Every flows door now holds the ENTRY LOCK — the same lock every meta mutator and remove() (through its state forget) already hold — across verify + strip-set read + all writes, so a remove, a reincarnation, or a secret flip can no longer slip between the identity check and the last write. The two secret-transition commits became locked store transactions with the C3 scrub inside: write_parameters (declared lane) and the new write_source_params (spec lane), collapsing all four CLI/TUI transition lanes onto them — purge-then-commit is enforced by construction, not call-site ordering. Lock-held probes pin the contract for every door, and the lock file's path is pinned as the cross-version contract it is. Declined as designed: merging the door's three per-slug state writes into one RMW — under the entry lock there is no interleave window left, each write is individually atomic, and every crash intermediate is C3-consistent; a merged mega-writer would couple argstate's per-writer API to the door's shape for no observable-state gain.
• TUI settings screens hold a stale Entry: saves can land on a reincarnated slug #39 fixed, not deferred. Store mutators take expected_id, checked under the lock (StaleEntryError, with the reopen remedy); the settings screen authorizes every axis of its save against the identity it was OPENED on — pinned by a per-axis stale matrix (rename, template, interpolate, declared rows, source block, deps, npm clear, needs, launch policy, runner pin) — its preset cleanup goes through the guarded flows.delete_presets_for door, and its write pass gained the one catch the old bare calls never had (a mid-save refusal keeps the screen instead of crashing the app). The census tightened: tui_settings no longer touches argstate at all.

Round 16 (eb351c3) — the doors-outside-the-doors review: round 15 built the locked, identity-authorized transactions and then mis-filed several CLI writers as "immediate" that genuinely wait on a human or derive their write from an earlier read. All four findings accepted (one sub-point declined with a proof):

• preset save/preset delete go through the doors. preset save waited on an interactive intake and then wrote argstate bare — inside a secret transition's own window, it could re-persist plaintext the scrub had just removed, un-serialized with the very transaction round 15 built. It now claims identity at hold-start (after its static refusals) and saves through flows.save_preset_for — entry lock, exact-id re-proof, launch∪current strip — refusing an entry that vanished or a slug reissued mid-intake (exit 127). preset delete claims before its confirmation ask and deletes through flows.delete_presets_for: an answer given about the old entry can never delete the new owner's same-named preset. The argstate-writer census now allows NO direct writer outside flows' doors and store's two locked scrubs.
• Every params/deps edit lane authorizes its write. The rule the review stated is now the code's rule: a write that depends on an earlier read carries the identity claimed before that read — "usually fast" is not an authorization. The params edit lanes (spec, declared, launch policy, runner pin, interpolate) and both deps axes claim via ensure_identity (the read views stay pure reads — no claim, no stamp) and pass expected_id to every store call; a reissued slug gets an honest stale refusal (exit 125), never the dead handle's analysis. --normalize — the worst case, a semantic rewrite computed from a pre-consent read — moved into store.rewrite_source: read, transform, and byte-disciplined write all under the entry lock with the identity check first, re-derived from the fresh text.
• run stamps nothing on a refused invocation. The hold-start handshake moved below every static refusal, so run x --raw --set a=1 (exit 2) on a legacy entry no longer heals its meta — "a refused invocation leaves no fingerprints" now includes the id stamp.
• "" is an expectation, not an off-switch. The id or None idiom silently disabled the guard for exactly the handle that most needed it; expected_id now passes verbatim, and an unstamped handle meeting a stamped entry refuses (the asymmetry proves the disk changed owners). ensure_identity re-reads after a failed stamp, so a half-landed id (meta written, registry row failed) can never strand a handle behind its own entry. Declined with a proof: skipping post-run persistence outright for unstamped handles — a reincarnated slug always meets the guard stamped (_add_entry cannot write a meta without an id), so "" == "" proves no reincarnation happened and the write is sound; refusing it would only strip read-only-library users (writable state dir, frozen data dir — a legitimate deployment) of run history for no safety gain.

Round 17 (eec0533) — the claim-by-address review: all four findings verified real and closed:

• Compare-and-claim replaces claim-by-address. Round 16's ensure_identity(slug) re-resolved an address and adopted whoever owned it — a remove + same-name re-add between a lane's resolve and its claim was silently blessed, with every later guard protecting the stranger. store.claim_identity(entry) verifies UNDER THE ENTRY LOCK that the disk still holds the entry the caller resolved (exact id; unstamped accepts only unstamped), stamps a pre-id meta there, and refuses a changed owner. Every lane passes its held Entry; TUI ghost rows stop their lane and refresh the list with the reason. The races are tested REAL — reincarnations injected between resolve and claim, the claim itself unpatched.
• skit remove is identity-authorized on both faces. store.remove takes expected_id, checked under the lock: the deletion is authorized against the entry the confirmation ask NAMED, so a slug reissued while the user answered refuses (with the stale message) instead of deleting the new owner's registry row, stored copy, meta and state.
• Editor sessions are staged. The longest user-paced hold skit has no longer touches the stored path: copy-mode edits work on a unique draft in skit's drafts dir and land through store.commit_copy_edit's identity-checked, mode-preserving transaction. A stale landing keeps the draft and names it in the refusal ("Your edit was kept at: …"); an invalid prompt edit now never reaches the stored copy at all — strictly better than the old in-place refusal. Reference mode stays direct: it edits the user's own file, which no reincarnation moves.
• The "" == "" proof is withdrawn. The reviewer's cross-version counterexample stands: an OLDER skit's adds write no id, so a symmetric blank can be a reincarnation this version never saw. Unknown identity persists nothing — post-run persistence requires a STAMPED exact match; a library this process cannot stamp (not provably unwritable by others) runs fine and simply keeps no state.

Gates

ruff format --check · ruff check · ty check (strictest) · pytest --cov — 5983 passed, 31 skipped, 100.00 % coverage · i18n gate 100 % both locales (the stale-refusal msgid reworded face-neutral in round 16; 13 new msgids translated across the audit + review rounds; pybabel fuzzy mismatches corrected) · targeted mutmut on the changed modules with every reported survivor re-verified against the live mutant: the round-14 doors ran 191/191 killed; round 15 re-ran the full identity/door/settings surface with a FRESH stats map (the #38 false-survivor trap) — every mutant on changed lines killed, three documented selector equivalents pragma'd, and the only remaining targeted survivors are pre-existing mutants on untouched validation-pass lines (the #38-recorded baseline, out of this PR's scope). Rounds 16–17's targeted sets ran 1000+ mutants each across the reworked lanes — every mutant on changed lines killed (round 17's three reported survivors were proven killed against the live mutants, the #38 stale-selection artifact; one true equivalence was refactored away rather than pragma'd). Full remote matrix green through b6d73db, Windows included (round 17 needed one test-side portability follow-up: platform-neutral mode/newline assertions).

Deliberately deferred (on the books)

Reader-lane secret override: exact-word false positives (sort_key, foreign_key) on reader-reflected forms still have no off switch; the audit ruled the fix shippable without it, but the [[parameters]]-rider override should follow as its own design round — it also doubles as the recovery path for any future heuristic miss.

Summary by CodeRabbit

• New Features
  • Added --no-input support to removal commands, with safe refusal when confirmation is required.
  • Improved replay of remembered trailing arguments, preserving quoting and expansion behavior.
  • Added parameter binding details to JSON output.
  • Updated TUI shortcuts for variable selection, agent management, and skill installation.
• Bug Fixes
  • Preserved original bytes, line endings, and file permissions during script edits.
  • Improved secret-name detection to reduce false matches.
• Documentation
  • Clarified non-interactive installation, deletion, command behavior, and argument replay.
  • Refreshed localized interface translations.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR centralizes ArgSpec, preserves bytes during script block edits, tracks extra-argument provenance, strengthens non-interactive deletion behavior, updates TUI caches and keyboard chords, improves secret detection, expands tests, and synchronizes documentation and localization catalogs.

Changes

Core behavior

Layer / File(s)	Summary
Argument provenance and analysis contracts src/skit/analysis.py, src/skit/argstate.py, src/skit/flows.py, src/skit/cli.py, src/skit/tui.py, src/skit/langs/*	Adds shared ArgSpec ownership and persists extra_args_raw so CLI and TUI replay can distinguish raw tails from shell-processed arguments.
Byte-preserving block edits src/skit/rewrite.py, src/skit/cli.py, src/skit/store.py, src/skit/tui_add.py, src/skit/tui_settings.py, tests/test_design_audit_fixes.py, tests/test_design_audit_tui.py	Routes block edits through newline-aware, surrogateescape-preserving atomic writes that retain file modes and original bytes.
CLI safety and data contracts src/skit/cli.py, src/skit/params.py, docs/content/docs/cli.mdx, skills/skit/SKILL.md, tests/test_design_audit_fixes.py	Adds non-interactive refusal handling for destructive commands, pre-confirmation unknown-preset checks, additive JSON binding output, whole-word secret detection, and clearer parameter-management guidance.
TUI cache and keyboard behavior src/skit/tui.py, src/skit/tui_add.py, src/skit/tui_prefs.py, src/skit/tui_settings.py, tests/test_design_audit_tui.py, tests/test_prompt_tui.py	Replaces drift caching with mtime-keyed plan caching, memoizes reader probing, and assigns Ctrl+L, Ctrl+G, and Ctrl+Y to their updated actions.
Documentation, tests, and catalogs AGENTS.md, README*, docs/content/docs/*, tests/*, src/skit/locales/*	Documents keyboard, bootstrap, replay, and confirmation contracts while updating regression coverage and gettext catalogs.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant CLI_or_TUI
  participant flows
  participant argstate
  participant Launcher
  User->>CLI_or_TUI: run with remembered or new extra arguments
  CLI_or_TUI->>argstate: load extra_args_raw
  CLI_or_TUI->>flows: assemble with expand_extra
  flows->>Launcher: launch entry with resolved arguments
  CLI_or_TUI->>flows: save_after_run with extra_raw
  flows->>argstate: persist extra_args and provenance

Loading

Possibly related PRs

• t41372/skit#26: Also modifies argstate.py persistence and related write-back paths.

Poem

I’m a rabbit with bytes in my burrow,
Keeping each newline neat, never blurry.
Chords hop to their proper new keys,
Raw tails replay with the greatest of ease.
Safe prompts refuse when consent isn’t shown—
And catalogs bloom in languages known.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 56.15% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the audit-driven fixes for write-back corruption, secret detection, and contract repairs.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codspeed-hq]

Merging this PR will not alter performance

✅ 18 untouched benchmarks
⏩ 2 skipped benchmarks¹

---

_{Comparing fix/design-audit (b6d73db) with main (2a35f19)²}

Footnotes

1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

2. No successful run was found on main (206f9ef) during the generation of this report, so 2a35f19 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/skit/cli.py`:
- Around line 4526-4536: The error message currently concatenates two
independently translated strings, making the combined sentence difficult to
localize. Update the message construction in the managed-parameters failure path
around entry.meta.name and entry_spec.params_io to use a single
gettext-translated msgid containing the optional --add hint, while preserving
both existing message fragments and their conditional behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bada59f9-e67c-4fe4-9afe-20f21954b12e

📥 Commits

Reviewing files that changed from the base of the PR and between 2a35f19 and 9ed2152.

📒 Files selected for processing (49)

• AGENTS.md
• README.md
• README.zh-CN.md
• README.zh-TW.md
• docs/content/docs/cli.mdx
• docs/content/docs/parameters.mdx
• skills/skit/SKILL.md
• src/skit/analysis.py
• src/skit/argstate.py
• src/skit/cli.py
• src/skit/flows.py
• src/skit/langs/base.py
• src/skit/langs/fish/cli_reader.py
• src/skit/langs/javascript/cli_reader.py
• src/skit/langs/powershell/cli_reader.py
• src/skit/langs/python/argspec.py
• src/skit/langs/shell/cli_reader.py
• src/skit/locales/skit.pot
• src/skit/locales/zh_CN/LC_MESSAGES/skit.mo
• src/skit/locales/zh_CN/LC_MESSAGES/skit.po
• src/skit/locales/zh_TW/LC_MESSAGES/skit.mo
• src/skit/locales/zh_TW/LC_MESSAGES/skit.po
• src/skit/params.py
• src/skit/rewrite.py
• src/skit/skills/skit/SKILL.md
• src/skit/store.py
• src/skit/tui.py
• src/skit/tui_add.py
• src/skit/tui_prefs.py
• src/skit/tui_settings.py
• tests/test_argstate_mut.py
• tests/test_cli.py
• tests/test_cli_design_cov.py
• tests/test_cli_gaps_cov.py
• tests/test_cli_mut_part03.py
• tests/test_default_semantics_review_fixes.py
• tests/test_design_audit_fixes.py
• tests/test_design_audit_tui.py
• tests/test_flows.py
• tests/test_prompt_tui.py
• tests/test_source_default_semantics.py
• tests/test_store_mut.py
• tests/test_tui_edit.py
• tests/test_tui_mut.py
• tests/test_tui_mut_part01.py
• tests/test_tui_mut_part05.py
• tests/test_tui_mut_part09.py
• tests/test_tui_prefs_agents_cov.py
• tests/test_tui_prefs_mut.py

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!