fix: full-codebase design audit — write-back corruption, secret heuristic, contract repairs - #34
Open
t41372 wants to merge 25 commits into
Open
fix: full-codebase design audit — write-back corruption, secret heuristic, contract repairs#34t41372 wants to merge 25 commits into
t41372 wants to merge 25 commits into
Conversation
… contract repairs Findings from a full-codebase design audit, worst first: - The TUI add-review panel wrote parameter blocks back with read_text/write_text, silently rewriting every line ending of the just-stored copy (CRLF-ifying it on Windows) 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 + keep-mode). The python onboarding lane's strict-decode traceback on non-UTF-8 input and the --normalize lane's non-atomic write go away with it. - is_secret_name matched substrings, so --max-tokens (TOKEN) became a permanent password field on reader-reflected forms with no override anywhere. Whole-word matching (snake/kebab/camel/sentence-aware) fixes the false positives at the source for every lane. - skit remove prompted inside pipes/CI, violating the non-interactive contract its own sibling runner remove implements; it now takes --no-input and refuses with exit 2, and 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. - The remembered extra-args tail replayed under two different expansion regimes (CLI literal, TUI token/glob-expanded). The tail's provenance is now recorded (argstate extra_args_raw) and every replay follows it in both faces. - The Library detail pane rebuilt the form plan on every cursor move — a synchronous pwsh spawn per RowHighlighted for PowerShell entries; plans are now mtime-cached (the old drift cache generalized), and the add-review panel memoizes its reader-modeled probe. - Ctrl+O meant three things on three sibling screens while README documents one: Choose variables moves to Ctrl+L (both screens), Preferences' squatters move to Ctrl+G/Ctrl+Y, and AGENTS.md's key grammar now lists Ctrl+O and Ctrl+L. - READMEs stop claiming uv is always consent-gated (non-tty downloads without asking, by design); params --manage dead-ends now name the --add escape; params --json rows carry an additive "binding" key so "kind" no longer puns across sibling commands; ArgSpec moves to the neutral analysis module the four non-python readers were told not to reach around. Tests/coverage follow in the next commit.
…cache Two regressions the round-2 review caught in round 1's own fixes: - Word-boundary secret matching dropped the jammed spellings the old substring rule caught — APIKEY/apikey/AUTHTOKEN stopped matching, a C3 regression in the dangerous direction (an unmarked literal is published into current_defaults, --json output, and plaintext state). Words ending in the long secret words (TOKEN/SECRET/PASSWORD/PASSWD) now match, and KEY-compounds match behind a credential-qualifier prefix list (APIKEY, SSHKEY — never MONKEY/TURKEY/HOTKEY). - The new display-plan cache keyed only on the script's mtime, but a plan is a function of meta.toml too (declared [[parameters]] rows, a prompt's managed list / interpolate switch): an agent running skit params --add beside an open TUI left the detail pane stale forever. The cache now keys on both mtimes.
Repairs the suite for the two source commits and adds the behavior coverage that keeps each verified bug dead. Contract repairs — the API changes the fixes made: - flows.save_after_run's now-required extra_raw keyword threaded through its seven test call sites, each carrying the value its scenario really simulates (a TUI-form save is True, a CLI tail False). - tui.PendingRun's new positional extra_raw field at its five construction sites. - MenuApp._drift_cache -> _plan_cache: the seeded/inspected sentinels are now ((script mtime, meta.toml mtime), FormPlan) and still prove what they meant to — the cache hit, and the pop on edit / settings close. - test_store_mut's atomic-write spy repointed at rewrite.atomic_write_bytes_keep_mode, the seam _sync_python_block reaches through write_block_edit now; the two cli_design_cov write-back spies likewise, which also lets them pin the landing as atomic (neither Path.write_bytes nor write_text may touch a stored copy). - Ctrl+O -> Ctrl+L (prompt review, Script settings) and Ctrl+O/Ctrl+K -> Ctrl+G/Ctrl+Y (Preferences) in every pilot test that pressed them. - remove / preset delete need -y or a real terminal now, so their existing tests say so. New coverage (tests/test_design_audit_fixes.py, tests/test_design_audit_tui.py): - rewrite.read_for_block_edit / write_block_edit: CRLF, lone-CR and LF copies each round-trip with only the block changed and every other byte identical; non-UTF-8 bytes survive via surrogateescape; the executable bit survives the atomic write; cli._onboard_python degrades on a non-UTF-8 python file instead of raising. - The round-1 HIGH pinned at the surface it shipped from: an AddReviewScreen accept on a CRLF shell script leaves the stored copy CRLF and byte-exact outside the block. - is_secret_name's matrix in both directions, including one jammed spelling for every suffix and every KEY-prefix the rule recognizes (--max-tokens stays public). - The non-interactive contract for remove and preset delete (worded exit-2 refusal naming --yes, nothing removed), their -y and confirm paths, the unknown-preset error landing before any ask, and the same error when a preset vanishes mid-flight. - Extra-args provenance end to end: the argstate marker is written, cleared, and defaulted for legacy docs; save_after_run threads it; the CLI expands a replayed raw tail and replays an unmarked one literally; a fresh `-- args` clears the marker; --forget-args clears both; the TUI's `r` follows the record while the form's own tail saves marked; and the marker rides PendingRun into the deferred exit-mode save. - The display-plan cache: one build per (script mtime, meta.toml mtime), a rebuild when either moves, no caching at all when the script can't be stat'ed, _has_drift served off the same entry and short-circuiting before any build, and both pop sites. - AddReviewScreen._reader_modeled probes once per text and recomputes after an edit (a single-option getopts pins "modeled" at one field, not more than one). - Positive pilot tests for Ctrl+L / Ctrl+G / Ctrl+Y with a negative for each vacated chord, and Ctrl+O still restoring a run-form default. - params --manage on an exe names the --add door it does have — and the two degraded- spec refusals now assert they do NOT carry that hint; params --json rows carry "binding" beside the frozen "kind" without dropping a key an existing consumer reads.
The round-4 review of merged PR #33 came back clean at medium-and-above and verified all four interaction seams with the audit branch. Two low notes fixed: - argstate reads the extra_args_raw marker with `is True` instead of bool(), matching the house rule for hand-editable bools (models.interpolate, config.enabled): a hand-edited scalar must degrade to the safe literal-replay default, never coerce truthy toward re-expansion. Pinned by a test. - AGENTS.md records #33's one sanctioned bend of the read-command contract (a listing may self-heal skit's own registry index) next to the rules it bends, the way the A5 exception is recorded.
|
Important Review skippedToo many files! This PR contains 109 files, which is 9 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (109)
You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR centralizes ChangesCore behavior
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
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (49)
AGENTS.mdREADME.mdREADME.zh-CN.mdREADME.zh-TW.mddocs/content/docs/cli.mdxdocs/content/docs/parameters.mdxskills/skit/SKILL.mdsrc/skit/analysis.pysrc/skit/argstate.pysrc/skit/cli.pysrc/skit/flows.pysrc/skit/langs/base.pysrc/skit/langs/fish/cli_reader.pysrc/skit/langs/javascript/cli_reader.pysrc/skit/langs/powershell/cli_reader.pysrc/skit/langs/python/argspec.pysrc/skit/langs/shell/cli_reader.pysrc/skit/locales/skit.potsrc/skit/locales/zh_CN/LC_MESSAGES/skit.mosrc/skit/locales/zh_CN/LC_MESSAGES/skit.posrc/skit/locales/zh_TW/LC_MESSAGES/skit.mosrc/skit/locales/zh_TW/LC_MESSAGES/skit.posrc/skit/params.pysrc/skit/rewrite.pysrc/skit/skills/skit/SKILL.mdsrc/skit/store.pysrc/skit/tui.pysrc/skit/tui_add.pysrc/skit/tui_prefs.pysrc/skit/tui_settings.pytests/test_argstate_mut.pytests/test_cli.pytests/test_cli_design_cov.pytests/test_cli_gaps_cov.pytests/test_cli_mut_part03.pytests/test_default_semantics_review_fixes.pytests/test_design_audit_fixes.pytests/test_design_audit_tui.pytests/test_flows.pytests/test_prompt_tui.pytests/test_source_default_semantics.pytests/test_store_mut.pytests/test_tui_edit.pytests/test_tui_mut.pytests/test_tui_mut_part01.pytests/test_tui_mut_part05.pytests/test_tui_mut_part09.pytests/test_tui_prefs_agents_cov.pytests/test_tui_prefs_mut.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…tself The workflow review (max effort) confirmed 15 findings against PR #34's own fixes; the seven correctness ones, worst first: - is_secret_name regressed plurals (API_KEYS, SECRETS, GITHUB_TOKENS) and acronym+lowercase jams (APIkey, SSHkey) to non-secret — false negatives that publish live literals into --json and state files. The heuristic is now segment-based (no camel regex to mangle nonstandard casing; one trailing S folds away) with an explicit TOKEN count-context rule: count qualifiers (max_tokens, token_limit, nTokens) suppress, credential qualifiers (github_tokens, session_token) mask, bare plural 'tokens' reads as a count. - The launch menu was a third face of the provenance rule: a CLI-captured literal tail prefilled into the form and submitted untouched was re-expanded and its stored marker flipped to raw for every later replay. An untouched tail now keeps its recorded provenance; only text the user actually edited is re-captured as form text. - Replaying a marker-less tail that carries token/glob syntax now says so on stderr (legacy pre-provenance state replays literally by design — silently was the bug), and the docs explain the one-time re-capture. - _cached_plan re-resolves the entry from the store on a cache miss: the Library's in-memory row can predate the very meta.toml change that invalidated the key, and building from it pinned a stale plan under the fresh key past every reload. The key also tightens to (mtime_ns, size) per file, narrowing the coarse-mtime blind spot to same-tick same-size writes. - The add panel's reader-modeled memo keys on the reader's runtime fingerprint (new CliReader field, wired for PowerShell) so installing pwsh mid-session is seen instead of serving the tool-less verdict forever. - Preferences action docstrings caught up with the chord move (Ctrl+G/Ctrl+Y). Cleanups: the strict block-edit read lanes (--normalize, deps sync) now go through read_for_block_edit(errors="strict") — the fold/detect discipline lives in exactly one place; the destructive-trio confirmation guard is one helper (_require_yes); the --add hint is one msgid instead of two sentences spliced with a hard-coded ASCII space; _SECRET_WORDS/_SECRET_SUFFIXES no longer shadow each other; analysis.py's header lists its third resident (ArgSpec). i18n at 100% for the new msgids. Tests follow in the next commit.
Repairs the suite for 67d7ee1 and adds the behavior coverage that keeps each of its verified bugs dead. is_secret_name (segment rule): - The matrix grows both directions the round repaired — plurals (API_KEYS, SECRETS, GITHUB_TOKENS, DB_PASSWORDS), acronym+lowercase jams (APIkey, SSHkey, GPGkey, AWSkey), and the qualified plurals that stay credentials (github_tokens, "access tokens", session_token). - Three cases are RE-RULED with the reason in place: photokens now reads secret (anything ending in TOKEN without a count qualifier errs toward masking), while MAX_TOKEN / token_limit / token_count now read public (the count context suppresses the singular too). - One case per member of _SECRET_SUFFIXES, _KEY_PREFIXES and _COUNT_WORDS, each with a completeness assertion, so no member can be dropped, renamed or added without a red test; plus the fused qualifier (maxTokens, nTokens), the plural qualifier (token_limits) and the bare-plural count rule. Launch-menu provenance (tui._submitted, the rule's third face): - A prefilled literal tail submitted untouched is delivered verbatim, assembles with expand_extra=False, and does NOT flip its stored marker; a prefilled raw tail submitted untouched still expands; an edited tail is re-captured as form text (marked raw) — the documented one-time repair for a legacy tail. The new `expansions` fixture records the expand_extra decision itself. CLI as-is note: - A marker-less replay carrying {, *, ? or [ prints the note on stderr (one case per character) and still delivers the tail literally; a plain-word tail and a raw-marked tail both stay quiet. Plan cache: - The key's shape moves to conftest.plan_cache_key ((mtime_ns, size) per file) and the three tests that spelled it out inline now share it. - New: a meta.toml change made through the store API is reflected in the rebuilt plan even though the Library's row is stale (asserted on plan fields), and an entry that stops resolving mid-render serves the snapshot and caches nothing. Reader memo fingerprint: - A tool-gated reader (shaped like PowerShell's) re-probes when its fingerprint appears and vanishes, memoizes while it holds, and the purely-static python/js readers keep memoizing on text alone; powershell.runtime_fingerprint tracks _find_powershell's answer, and the registry wiring is asserted on the builder itself (capabilities are built once per process and cached). read_for_block_edit(errors=): - errors="strict" raises on the bytes the default carries, with the same fold + detect either way; the deps sync now also pins a CRLF copy round-tripping through one block, and the two lanes' docstrings name the shared read. Review findings in the suite itself: - The two write-back spy tests share a copy_io_spy fixture (their assertions are unchanged); _without_block and the block constants move to conftest and are imported by both design-audit files. Mutation: killed the round's new survivors in _powershell_caps and the _edit_params refusal msgid, plus the flip note's wording and its >0 threshold.
…fixes left seams The re-review confirmed 15 findings, most of them narrow gaps at the edges of round 5's own fixes. This round replaces the point patches with structure: - is_secret_name matches TWO word sources per segment — the jammed segment (APIkey) AND its camelCase sub-words (awsSecretKey → AWS/SECRET/KEY). Rounds 2 and 5 each kept only one source and regressed the other's cases. Count suppression is scoped by shape: names suppress on a count word anywhere (maxOutputTokens), sentence prompts only on 'count word, then token word' — 'Enter your API token (max 64 chars):' is a credential ask and stays masked. - Freshness has ONE owner: MenuApp._fresh() re-resolves the record for the detail render AND both launch paths, so the pane and the run it advertises describe the same generation (the pane was made fresher than the launch it fronted; description/deps also no longer mix generations with the plan). _cached_plan no longer resolves internally — it caches whatever generation its caller renders, validates the key with a second stat so a racing write can never pin stale content under a fresh key, caches snapshot fallbacks (a corrupt meta stopped causing a subprocess per cursor move), and keys on the reader-tool fingerprint so installing pwsh mid-session reaches the pane. - Extra-tail provenance is judged by the FORM, not by diffing values against re-read state: RunFormScreen tracks a real dirt bit on the extra row (armed after mount, set by Input.Changed) against its own compose-time snapshot, and returns the verdict in FormResult. A concurrent CLI write no longer fakes an edit; a cleared-and-retyped identical tail now honestly counts as typing into the launch menu. One state read per interaction (was three). - The as-is note prints on BOTH faces (r-rerun and the exit-after-run path, same msgid as the CLI), its predicate lives in flows.tail_looks_expandable and covers leading ~ (tokens.expand expands it), and the docs state honestly that deliberately-quoted CLI tails get the note too. - The --add hint shell-quotes the entry name it tells the user to paste. Tests follow in the next commit.
…oof, platform quoting
Six confirmed findings from the third re-review, the two severe ones both
credential-leak direction:
- is_secret_name judges per SEGMENT now (_judge_segment): a segment's own count
words (fused nTokens, camel maxOutputTokens) veto its token hit, but ONLY a
segment that is itself count-shaped is count context for its neighbors —
camel fragments never leak out, so N8N's stray digit-boundary N no longer
vetoes the TOKEN next door (digits also stay inside segments; _forms strips
them per word so api_key2 still finds its KEY, and a pure number counts as a
count: '60 tokens'). The sentence rule flips from suppress-if-ANY-mention-
count-preceded to secret-if-ANY-mention-is-NOT — 'Paste your GitHub token
(rate limit 60 tokens/min):' names a rate AND asks for a credential, and the
credential wins.
- _cached_plan's caching now needs BOTH freshness proofs: the second stat
(write after first stat) AND a meta re-read equality check (_meta_unchanged)
for the other half of the window — a meta write between the caller's resolve
and the first stat pinned a plan built from the old meta under the new key,
which stat equality alone cannot see.
- The --add hint quotes with argv_text.join (shlex on POSIX, list2cmdline on
Windows) — shlex.quote's single quotes are word-splitting noise to cmd.exe,
on the platform the product explicitly targets.
- flows.tail_looks_expandable delegates token grammar to tokens.has_tokens
(THE authority on what expand() changes) — the hand-rolled brace check
missed }} halving and over-fired on bare { — and the as-is note msgid has
ONE home, flows.as_is_note(), called by both faces.
Tests follow in the next commit.
…ds in front
The round-7 digit handling made any numeric segment count context for the whole
name, so GITHUB_TOKEN_2 / API_TOKEN_1 / slack-token-3 stopped being masked —
digit-INDEXED credentials are common and this was the publishing direction (the
test-repair pass caught it and stopped rather than pin the leak).
A count WORD (max, limit, n) still qualifies a name from anywhere; a bare NUMBER
now qualifies only the segment that FOLLOWS it: '60 tokens' and 2_tokens are
counts by construction, GITHUB_TOKEN_2 is a second GitHub token. In the name
branch, numbers suppress only when EVERY token mention is immediately preceded
by count context; the sentence branch was already positional and gains the
numeric predecessor ('rate limit 60 tokens/min' still suppresses that mention
while the credential ask beside it still wins).
Two documented cases re-ruled with the refactor, both deliberate: a bare-plural
'tokens' knob now reads as a count in ANY casing (toKens/TokenS — no compound
can hide inside six letters, unlike APIkey), and the synthetic jam
EnterYourAPIToken(max64chars) flips to masked (safe direction; every real count
spelling — max_64_tokens, max64Tokens, gpt4_max_tokens — still reads count).
Repairs the suite for 01fb12a + 2d2fc77 and adds the behavior coverage that keeps each of their verified bugs dead. is_secret_name (per-segment verdicts): - The matrix grows the round's own families: the digit-boundary credentials that round 7 repaired (N8N_TOKEN, n8nToken, gpt4_token, api_key2, base64Key), the indexed credentials 7b repaired (GITHUB_TOKEN_2, API_TOKEN_1, slack-token-3), the sentence that names a rate AND asks for a credential (the GitHub rate-limit ask), and the counts that must stay public (max_2_tokens, 2_tokens, "60 tokens", MAX2TOKENS, max64Tokens). - Two documented cases are RE-RULED with the reason in place: the bare-plural knob now reads as a count in ANY casing (toKens/TokenS — six letters cannot hide a compound the way APIkey does), and the synthetic jam EnterYourAPIToken(max64chars) flips to masked (the safe direction; every real count spelling still reads as a count). - New structural tests for the new pieces: _forms' plural + digit folds, the single-qualifier slice in _token_form, a segment's four verdict bits, the jam-only county that stops N8N's camel fragment from vetoing the TOKEN next door, the forward-only numeric context, the name branch's ALL-mentions quantifier, and the sentence rule's exists-unsuppressed shape with a killing case for each of its three pieces. Display-plan cache: - The other half of the cache window: a snapshot older than the meta on disk is built from, and NOT cached under, the current key — the next render with the fresh record sees the declared parameter. Plus _meta_unchanged at its three answers (meta half, dir half, unresolvable-is-unchanged), which keeps round 6's corrupt-meta single-build guarantee pinned beside it. The as-is note and its predicate: - flows.as_is_note() is the one home: tui._as_is_note is gone (asserted), and both faces are pinned against the exact sentence, not a prefix. - The J matrix follows tail_looks_expandable's delegation to tokens.has_tokens — `}}` and `{{` now note, a bare `{x}` does not (it never expands) — with an oracle test that asks the real expander whether each piece would change, and a `}}` case on the CLI replay and the exit-after-run face. --add hint quoting: - The hint is asserted through argv_text.join, and a Windows-convention run (monkeypatched platform) pins the double-quoted spelling cmd.exe can paste; argv_text's own suite gains the join half of its platform pair. Mutation: killed the round's new survivors, including the camel split's separator (it cuts on the space its own sub inserts) and the two-framework flip note, whose ", " separator no single-framework test could reach.
…ak slug Nine distinct defects from the fourth re-review (the round-8 workflow's first run died to a session limit and reported a hollow CLEAN — rerun properly, it found five more leak-direction holes in the round-7 secrecy rework): - is_secret_name v6: THREE word sources per segment (jam + camelCase sub-words + digit-split parts) feed secret/token MATCHING — base64key/sha256key/s3key mask again, N8NToken/N8NTOKEN mask via the un-shattered jam. COUNT CONTEXT got the opposite treatment, because every leak so far was a shard posing as a qualifier: county is the unstripped jam only (N26 no longer strips to a count N), the camel regex drops its digit→Upper boundary (no more stray N fragments), internal count words veto only in NAME shape (a prompt spelling perUserToken is still an ask for that value), and a bare NUMBER counts only a PLURAL mention right after it — '2 tokens' is a count, STEP_2_TOKEN and 'Enter step 2 token:' are indexed credentials and stay masked. - Paste-able command hints interpolate the SLUG, not the display name, at all seven sites (the --add hint, drift resync ×2, placeholder drift, flood cap, runner pin, normalize): no quoting convention survives every shell (shlex's single quotes are noise to cmd.exe, list2cmdline leaves & | ^ bare), but a slug's charset needs none anywhere and resolve() accepts it everywhere. drift_lines gains a target param so prose keeps the display name. - tail_looks_expandable's glob half now reads _GLOB_CHARS, the same authority assemble's glob pass consults; _meta_unchanged delegates its degrade policy to _fresh — one owner for 'what counts as the current generation'. Tests follow in the next commit.
The tests for 84f90a9, plus the one source line mutation testing proved was unpinnable as written. - is_secret_name v6, at each seam the round-8 rework introduced: the six-bit segment verdict (token_plural and internal_count split apart from county, which is what let a prompt spelling perUserToken stop reading as a count); county judged on the unstripped jam (N26 no longer strips to a count N); the camel regex's missing digit boundary (N8NToken); _digit_split as a MATCHING-only third source (base64key/sha256key/s3key/md5key/x509key/gpt4key); a bare number counting a plural and indexing a singular (2_tokens vs STEP_2_TOKEN); and the NAME/SENTENCE asymmetry of the internal veto. - Every paste-able hint, one test per site, all registered under a name that needs quoting in every shell ("a & b"): the --add dead end, drift_lines' new target param (and its name fallback), the launch-menu drift banner, the prompt-body drift line, the flood cap, the no-runner refusal, the normalize hint, and the mid-run resync line. One of them lifts the printed command out of the output, resolves it, and runs it — a hint is only worth printing if it works. - tail_looks_expandable's glob half parametrized over _GLOB_CHARS itself and cross-checked against glob_feedback, so the predicate and the pass that does the globbing cannot drift apart silently. - _meta_unchanged proved to DELEGATE (patch _fresh, the answer follows; the store is not consulted behind its back) rather than merely to agree. One source change, and it is a spelling: internal_count's shard guard read `len(v) >= 2`, whose distinguishing input does not exist — N is the only single-letter count word and there is no two-letter one, so `>= 2`, `> 2` and `>= 3` are the same function and four mutants survived as equivalents. The rule it is proxying is "a one-letter remnant of stripping never counts", so it now says `!= 1`: same behaviour on the whole cumulative matrix, and both mutation directions are killable (maxsTokens and tokenN8 are the names that separate them). Gate: 5624 passed, 100.00% coverage, ruff/ty/i18n clean. Targeted mutmut on the round-8 surface (params.py whole module, analysis.drift_lines, flows.tail_looks_expandable/_placeholder_body_plan, tui._meta_unchanged): zero survivors.
…that vanished
Six defects from the ninth pass, all in territory the last four rounds
under-read (store/doctor/argstate, not params.py secrecy). Every one verified
against an isolated library before it was touched.
- A lost or corrupt registry.toml emptied the whole library in silence, and
`skit doctor` — the command whose job is checking the library is intact —
printed `✓ 0 entries registered` and exited 0 while both entries sat untouched
on disk. `doctor --rebuild` recovers them instantly and was named nowhere.
_fs_truth ALREADY cross-checks the index against disk so a lost registry can't
let `add` overwrite a stored script; store.unindexed_slugs is the read side of
that same check, and it now feeds healthcheck.collect — so doctor, the TUI
Health screen, `doctor --json` (additive `unindexed`), and BOTH blank-library
surfaces report it. Neither face asserts "no entries yet" without asking disk
first; `list --json` keeps stdout to one array and carries the line on stderr.
A promise the code makes to itself is not a promise to the user.
- `skit params X --secret NAME` on a template placeholder with no declared row
was skipped with a warning, a green "Updated" line and exit 0 — and the value
it was meant to protect then landed in the state file in plaintext (C3). The
heuristic is RIGHT to miss `cookie`; the explicit override exists for exactly
that case, and it was dropped by the codebase whose contract is refuse, never
drop. `--add` already knew how to materialize the row (and reaches the
plaintext scrub), so the rule now lives in edit_declared where both doors
share one constructor: a placeholder the entry asks for IS an editable
parameter. Fixing only --secret would have left the other seven flags silent.
- store.resolve trusted the index row's `name` with no freshness check while
_summary_from_row deliberately verifies the same stamp — so a hand-edited meta
name made `skit list` show an entry `skit run`/`show` called not-found, until
some unrelated listing happened to heal the index. list_summaries' own
docstring gives the reason ("the CLI would list entries the TUI, doctor and
`run` all refuse"); `run` reaches the store through resolve, the one door that
never checked. A NAME match is now verified against the meta resolve already
reads a line later, and only the MISS path pays for the sweep.
- record_run(values=None) replaced the whole last_run table, so `skit run --raw`
deleted the value snapshot its own call site promises to preserve — leaving
exactly the shape `preset save --from-last` calls legacy state, which then
refused with "no remembered values yet — run it once first" about an entry
whose values were in the same file and which had just run twice. It now
follows the convention save_last states one screen up.
- doctor prints the config and state roots two docs pages have always claimed it
prints and nothing in skit exposed at all (`config_dir`/`state_dir` in --json,
the same three lines on the Health screen): "what do I back up?" had no answer
from the tool that exists to answer it.
- LangSpec.takes_argv is gone. No code read it, while three comments and
docs/design/prompt.md credited it with the rule placeholder_params enforces —
a trait no code consults is a story, not a contract. The design docs carry a
dated correction rather than a quiet rewrite.
SKILL.md (+ packaged copy) teaches agents the `unindexed` key and the three
roots; cli.mdx/environment.mdx document them; troubleshooting.mdx gains the
recovery recipe. i18n back to 100% (zh_TW/zh_CN), docs site builds with the
link checker green.
Gate: 5662 passed, 100.00% coverage, ruff/ty clean.
…key that was never bound
Eight defects from the tenth pass. One of them was mine, shipped in round 9.
- `--no-input` was a cli.py LOCAL. It threaded through cli's own gates and stopped
there, so the one interactive gate below it — uvman's uv-download consent —
re-derived interactivity from sys.stdin.isatty(), an oracle the flag cannot
reach, and `skit run x --no-input` on a machine without uv printed a question
and blocked on input() forever. That is exactly what the bundled Agent Skill
promises an agent cannot happen. Threading a quiet= keyword down the call chain
would have fixed today's one gate and left the next one to repeat it, so the
verdict now lives in `interaction`: set once at the front door, readable at any
depth, with no parameter to forget. Under a refusal the gate takes the path a
pipe already takes (A9), because --no-input is an assertion of exactly that.
- The panic pane told a user whose index had just vanished to "open Health (h)".
There is no h binding — it is D — and round 9 wrote that line, on the one screen
where the reader has a single instruction and no patience. The glyph now has ONE
spelling (tui.HEALTH_KEY) behind the binding, the chip, the help overlay and the
line, and the line is a CHIP rather than prose: correct by construction, and
clickable, which prose naming a key never was.
- …and the status line was the THIRD blank-library surface. #detail is display:none
at -h-short/-h-tiny while the status line is documented as the channel that
survives every tier, so on a small terminal it was the only blank-state copy on
screen — still asserting a first run over an intact library. One question
(_lost_index_count), asked by all three.
- Entry settings forked on `kind == "prompt"` where AGENTS.md says to key off
placeholder_params — a level worse than the `family` spelling the rule forbids.
A command entry opened a section headed "Parameters (the run form's fields)"
showing none of them: to type {width} you had to retype its name from memory,
with no list in front of you.
- One store.NotFoundError had two exit codes: 127 from `run`, 1 from nine other
commands, against a docs table publishing 127 CLI-wide and a SKILL.md telling
agents to trust exit codes over output text. 1 is inside the band reserved for
the launched script, so the two cases were indistinguishable by any means the
agent was allowed to use. One helper now answers for all ten.
- --forget-args cleared the remembered tail ABOVE four gates that can still refuse,
so `run x --forget-args --set typo=1` destroyed the tail and then exited 2 — with
the invariant written in the comment beside it. Deferred into _on_accepted (the
home --save-preset already used), and the reuse is suppressed too: "forget it"
that replays the tail one last time and writes it straight back forgets nothing.
- The mirror radios rendered on/off/custom untranslated in the one section whose
audience is Chinese-speaking users — and the i18n gate reported "every scanned UI
sink routes through gettext", because the literals sat one hop away in a module
constant and again behind a loop variable. The labels are now a vocabulary of
their own (the stored token stays English, as the CLI requires), and the gate
resolves both hops and stops exempting lowercase identifiers inside LABEL sinks,
where no CSS class or slug can appear. Verified by reverting the fix: the gate
fails. Its remaining limit (a loop name reused by several rows names one of them)
is documented rather than claimed away.
- Unticking a preset destroyed unrecoverable user data with no ask — the fifth
destructive door and the only one without one, on the surface where an untick
plus an unrelated Ctrl+S is easiest to trip.
Round 9's mutation survivors are closed with it: resolve's index-row fast path is
now pinned by a test that proves the sweep is NOT taken on a hit (an optimization
no behaviour can observe is otherwise unkillable), and both corrupt-meta refusals
are pinned to name what the user typed. store.unindexed_slugs keeps one known
survivor — "META.TOML" is equivalent on case-insensitive macOS; Linux CI kills it.
Gate: 5701 passed, 100.00% coverage, ruff/ty/i18n clean, docs site + link checker
green.
…d that killed the app
Six defects from the eleventh pass. Two of them re-opened work from the two
rounds before it, which is where this loop keeps earning its keep.
- `skit edit` was the ONE editor door with no interactivity gate — and the door
the bundled Agent Skill teaches. Two of the four lanes refused on their own
("an editor session IS interaction"), two did not. In a pipe it spawned $EDITOR
against a stdin nobody was typing into: `vi` hung forever, `cat` dumped the
file into the caller's stdout, and skit then printed "Saved" about an edit that
could not have happened. This is round 10's defect one layer up, so it gets
round 10's answer: the gate lives in editor.open_in_editor, where all four
lanes pass, and reads `interaction` rather than a local isatty pair.
- Ctrl+L on Entry settings read #st-interpolate — composed only for prompts —
BEFORE its pure-Python guard, so on every python/shell/js/exe/command entry it
raised NoMatches out of the action handler and took the whole workbench down,
losing every unsaved edit on the screen. Ctrl+L is the terminal's universal
clear-screen reflex; it gets pressed by people who meant nothing by it. Now one
predicate (_can_choose_candidates) drives the chip AND check_action, so the
chord is disabled where it cannot work instead of merely inert — the rule the
Ctrl+R chip beside it already stated in prose — and both prompt-only actions
stay total for callers that reach them directly.
- The preset-delete confirmation round 10 added was a BOOLEAN, set on confirm and
never cleared. Confirm one deletion, abort the save on an unrelated validation
error, untick a second preset, save again: the latch was still standing and the
second preset was deleted with no question ever naming it. It now tracks the
NAMES already agreed to, which is correct under retick/untick churn where a
reset-on-abort boolean still is not.
- Two interactivity oracles disagreed about the same terminal in both directions,
and the module introduced last round to end exactly this created the second one
by not being adopted where the prompts live. `skit run x > out`: cli declined to
prompt while uvman blocked on one. `skit run x 2> log`: cli opened a form while
uvman silently downloaded and executed a network binary with no consent at all.
interaction.allowed() now takes the stream it is answering about (stdout for
cli.py's Prompt.ask, stderr for uvman's consent) and cli._is_interactive
delegates to it — so a refusal can never apply to half of skit.
- `skit remove` repeated "your original file will not be deleted" for a copy-mode
entry whose original the user had already deleted — trusting that very promise.
The TUI modal already withheld the line. launcher.original_survives is now the
one predicate behind both faces, and the third case says what is actually true:
skit holds the only copy.
- Two honesty fixes: the --plain form now names the spelling it cannot offer for a
cleared delivers-empty field (`--set NAME=`; no '-' sentinel, because on a
free-text or path field '-' is very often a real value), and a kind whose
language parser failed to import (the A2 degradation) says so instead of telling
a shell user that "programs have no managed parameters".
Also: interaction.reset()'s docstring named a caller that does not exist. It is a
test seam and now says so — the same story-not-contract line LangSpec.takes_argv
was deleted for two rounds ago.
Round 10's mutation sweep came back clean apart from one equivalent mutant in
reset() (`= None` and `= False` are both falsy at the single read), documented in
place rather than worked around.
Gate: 5718 passed, 100.00% coverage, ruff/ty/i18n clean, docs site + link checker
green.
Seven defects from the twelfth pass. This round's fixes were PLANNED and the plan
REVIEWED before any code was written — the review changed four of the seven, and
one of its catches would have been a fresh instance of the exact defect class this
loop keeps producing (adding a check_action to a screen whose Enter is not a
binding: a branch that cannot fire).
- `skit params` absorbed every invalid edit. A rejected `--type` printed a stderr
line, wrote everything else, exited 0, and then reported the state it had NOT
written through `--json` — both of the channels SKILL.md tells agents to trust
said success. This is round 9's `--secret` finding at the level round 9 stated
it: that round fixed the one path where the drop leaked a credential and left
eight sharing the shape. `params` now refuses ATOMICALLY (exit 2, nothing
written), the answer `--set`/`--dep`/`--python`/`--preset`/`config` already give
and the validate-then-write rule the TUI's own save has always applied.
Three things had to move before that was even expressible. `not-declared` and
`not-managed` each covered an idempotent op AND a refused one under one string
(`--rm GHOST` vs `--type GHOST=int`), so they are split at emission and render
the same sentence. `_apply_env_sources` was the third warning producer and the
only one returning finished prose, so nothing could classify it — which left
`--env-source` warning-and-continuing on the analyzer lane while the same flag
refused on an exe entry. And the prompt managed-list write sat ABOVE the
decision point, so a refused invocation would unmanage a name and only then
refuse: compute → decide → write, now.
- Round 11 unified the removal PREDICATE and left the ANSWER forked: "skit holds
the only copy" existed on the CLI, which makes you type a name, and not in the
Library, where Delete acts on whatever row the cursor is on. launcher.
removal_stake returns the VERDICT and each face writes its own whole sentences
(composing `question + " " + note` would have broken the one-msgid rule cli.py
states 4000 lines up).
- `skit edit` was outside the CLI contract: exit 1 for not-found where the other
ten entry-name commands exit 127, and no `--no-input`, so under a pty with
nobody typing round 11's editor gate was a no-op and it hung. It never raises
NotFoundError — it offers to create instead — which is why round 10's sweep
could not see it. Now: 127 for a missing name or a gone target, 2 for a kind
with no source and for the interactivity refusal, which moved to the front door
(down in editor.py it shares an exception class with "could not launch", so the
two could only ever get one code) and names the file to edit directly.
- …and that gate made `_reconcile_prompt_after_edit`'s non-interactive branch
unreachable — it had been so since round 11, covered only by tests that patched
interactivity AFTER the gate had passed. Deleted: coverage cannot tell an
unreachable branch from a covered one.
- The TUI collapsed three edit refusals into one sentence that denied the source
existed and misclassified the kind — a reference-mode Python entry whose file
had moved was told "programs and command templates run as-is". One shared plan
(launcher.plan_edit) with a reason id per case, and the Library's `e` chip is
now conditional on the same predicate, because a dead chip is what sent the user
there.
- Stored enum tokens shipped raw English inside translated output: `(copy 模式)`,
`工作目錄:store`, and `(Python · copy)` translating the kind but not the mode
inside one parenthesis. No static gate can catch it — the literal is in the
user's meta.toml, not the source. kindnames now owns value labels for all three
axes; the stored token stays English, and a user-typed absolute workdir passes
through unrelabelled.
- Declining a destructive confirm died as click's untranslated red `Aborted.` at
exit 1 — the correct, deliberate answer reading as an error, in the band the
docs reserve for the launched script. Now a translated line at 130, like the
add lanes, and it catches Ctrl+D too (click raises Abort on EOF regardless of
abort=True, so handling only the typed "n" would have been half a fix).
- Round 11's chip↔check_action rule was applied to one action on one screen. The
Health screen's FIRST chip was dead whenever the library was healthy; resync's
condition existed in two spellings; and the add review had two different
predicates for one rule, so Ctrl+L opened a picker no chip advertised.
Deferred, recorded rather than rationalized: kept drafts are listable, resumable
and deletable only from the TUI. That is a principle-4 gap ("every TUI capability
is also a CLI command"), not a non-defect.
Gate: 5753 passed, 100.00% coverage, ruff/ty/i18n clean, docs site + link checker
green.
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.
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:read_text/write_text— universal-newline fold plusos.linesepre-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-tokensbecame a permanent password field (HIGH).is_secret_namematched 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 removeprompted inside pipes/CI, violating the absolute non-interactive contract its own siblingrunner removeimplements (and SKILL.md claimed they behaved alike). It now takes--no-inputand 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.RowHighlighted— a synchronouspwshspawn (30 s timeout) per cursor move over a PowerShell entry. Plans are now mtime-cached; the add-review panel memoizes its reader probe.extra_args_raw) and every replay follows it in both faces.params --managedead ends now name the--addescape; non-UTF-8 python onboarding no longer tracebacks;ArgSpecmoves to the neutralanalysismodule the four non-python readers were told not to reach around;params --jsonrows carry an additivebindingkey sokindno 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:APIKEY/AUTHTOKENstopped matching, a C3 regression in the leak direction. Suffix rule + credential-qualifier KEY-compounds restore them (neverMONKEY/TURKEY/HOTKEY).meta.tomledits 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_rawread withis Trueper 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.Gates
ruff format --check·ruff check·ty check(strictest) ·pytest --cov— 5414 passed, 100.00 % coverage · i18n gate 100 % both locales (4 new msgids translated; two pybabel fuzzy mismatches corrected) · targeted mutmut on the changed modules: survivors killed or documented-equivalent (oneMETA.TOMLcase-folding mutant is unkillable on case-insensitive macOS; Linux CI kills it).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
--no-inputsupport to removal commands, with safe refusal when confirmation is required.