fix: review hardening — parameter assembly, template quoting, store writes#26
Merged
Conversation
A batch of correctness and robustness fixes found in review. Parameters & delivery: - Repeated options (click multiple / parseArgs multiple) assemble as `--tag a --tag b` instead of the argparse-style `--tag a b`, via a new `repeat` axis on ParamDecl/FormField; the click and JS readers set it. - Declared bool flags with an empty action default to store_true, so the checkbox is no longer a silent no-op; a flagless bool never emits an empty-string argv element. Command templates & exe launch: - Placeholders are quoted for their actual shell context, so a value in a "double-quoted" slot no longer leaves $(...) live; a dangling backslash before a placeholder can no longer re-arm the escape. - A directory (or other non-regular file) given as an exe is refused with a clean 126 instead of crashing subprocess. Store, state & atomicity: - Every argstate read-modify-write holds a per-slug advisory lock, closing a last-writer-wins data-loss window. - Settings and dependency writes are atomic and mode-preserving; the PEP 723 sync no longer re-encodes a non-UTF-8 stored copy into U+FFFD. - Rename uniqueness is re-checked under the registry lock; remove reports when the entry files could not be fully deleted. Analysis & inference: - Sibling local modules are no longer suggested as PyPI dependencies. - A script that shadows `input` is not treated as calling the builtin, for both the analyzer and the injection shim. CI & docs: - Skill markdown edits (skills/**, src/skit/skills/**) still trigger CI. - Correct the "secrets never touch disk" wording in all three READMEs and refresh the i18n catalogs.
Three independent reviews (plus a verification pass on each finding) went over a1c9ec7. Ten defects survived verification; each was reproduced before it was fixed, and the two that did not survive are recorded at the end. Template quoting: - `_posix_quote_state` tracks a FRAME STACK, not one quote character. A command substitution restarts quoting, so a placeholder inside `"$(cmd {v})"` sits in the nested command's own unquoted context — it was being escaped for double quotes, leaving `;`/`|` live and splitting `a b` into two words — while one inside `"$(cmd "{v}")"` was shlex-quoted and arrived wrapped in literal apostrophes. Backticks are frames too. - Double quotes nested in a backtick substitution are refused rather than guessed: that form strips one layer of backslashes before parsing the inner command, so the `\$` guard arrives bare. `$(…)` has no such layer. Analysis: - `input` shadowing is resolved PER SCOPE. The file-wide predicate came from constant folding, where an extra binding merely skips a fold; here it meant a single `def sanitize(input)` anywhere disabled injection for the whole script and turned stored parameters into hard drift errors on the next run. - A sibling directory only counts as a local module when it holds Python. PEP 420 makes a bare directory a namespace *portion*, which never shadows an installed regular package — so a `docs/` or `assets/` folder colliding with an import name was silently dropping a real dependency. Parameter assembly: - `nargs=N>1` is a multi-value field for both argparse and click, and a typed multi-value field validates each piece — `--point 1 2` was rejected as "not a whole number" and then assembled as one quoted token. - click's `multiple=True, nargs=N>1` has no representable shape at all, so the option goes to the passthrough field instead of an invalid `--point 1 --point 2`. - fish's `=+`/`=*` set the `repeat` axis, which had been applied per-reader. - A declared bool flag that is ON by default is refused: its flag can only turn it on again, so store_true made the unticked checkbox deliver nothing. Writes that must not lie: - Script settings saves are byte-lossless. The screen read its copy with errors="replace" and universal newlines and wrote that back as strict UTF-8, so one checkbox tick flattened a CRLF script and burned every non-UTF-8 byte to U+FFFD. `detect_newline`/`restore_newline` move to rewrite.py, which both callers now share. - `skit deps --clear` / `--python -` on a non-UTF-8 copy that carries its own PEP 723 block is refused before meta is written. meta cannot express a clear (an empty value means "defer to the block"), so the edit reported success while uv kept installing the old list. - `atomic_write_text_keep_mode` fchmods the temp file before the rename, making the mode part of the atomic swap instead of a follow-up a crash can skip. - `store.remove`'s partial-deletion error reaches the user: the CLI prints it and exits 1 instead of a traceback, and the TUI shows it in the status line instead of crashing out of the terminal. Not changed, deliberately: - `argstate.forget()` taking no lock — the reported race is not closed by one. - `script_dir` on the copy-mode add path — the local-module filter it feeds is what stops `--no-input` installing an unrelated same-named PyPI package. The real gap is that copy mode stores a single file. Verification: 100% coverage, and targeted mutation testing over all 19 changed functions went from 23 surviving mutants to none.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR synchronizes state writes, preserves file modes and newline styles, expands repeated-option modeling, improves Python analysis and dependency discovery, hardens launch behavior, handles cleanup errors, and refreshes documentation, CI filters, translations, and regression coverage. ChangesSkit behavior updates
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![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 `@tests/test_store_fix.py`:
- Around line 548-569: Hoist the duplicated Path.read_bytes OSError mock used by
test_update_dependencies_copy_sync_swallows_read_oserror and the corresponding
tests into a shared pytest fixture in conftest.py, such as
unreadable_named_path(monkeypatch). Update each test to use the fixture with its
target filename, preserving the existing simulated error behavior and
assertions.
🪄 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
Run ID: 3f6b6398-4181-402d-bdc4-976b43f11cf8
📒 Files selected for processing (53)
.github/workflows/ci.ymlREADME.mdREADME.zh-CN.mdREADME.zh-TW.mdsrc/skit/argstate.pysrc/skit/atomic.pysrc/skit/cli.pysrc/skit/flows.pysrc/skit/langs/fish/cli_reader.pysrc/skit/langs/javascript/cli_reader.pysrc/skit/langs/launch.pysrc/skit/langs/python/analyzer.pysrc/skit/langs/python/argspec.pysrc/skit/langs/python/shim.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/pep723.pysrc/skit/rewrite.pysrc/skit/store.pysrc/skit/tui.pysrc/skit/tui_add.pysrc/skit/tui_settings.pytests/test_add_feedback_contracts.pytests/test_analyzer.pytests/test_argspec.pytests/test_argspec_click_typer.pytests/test_argstate_mut.pytests/test_atomic.pytests/test_cli_mut_part04.pytests/test_fish.pytests/test_flows.pytests/test_js_analyzer.pytests/test_js_cli_reader_mut.pytests/test_launch_mut.pytests/test_launcher.pytests/test_params_edit.pytests/test_params_model.pytests/test_pep723_mut.pytests/test_phase1.pytests/test_py_analyzer_mut.pytests/test_rewrite.pytests/test_settings_and_draft_review_atomicity.pytests/test_shim.pytests/test_show.pytests/test_store_fix.pytests/test_store_mut.pytests/test_template_context_quoting.pytests/test_tui_core_cov.pytests/test_tui_settings_cov.py
Comment on lines
+548
to
+569
| def test_update_dependencies_copy_sync_swallows_read_oserror(tmp_path, monkeypatch): | ||
| """The sync's guard covers OSError too, not just decode failures: if the stored copy can't be | ||
| read back at all, the edit still lands in meta and the sync degrades silently rather than | ||
| crashing the whole update.""" | ||
| from skit import store | ||
|
|
||
| src = tmp_path / "plain.py" | ||
| src.write_text("print(1)\n", encoding="utf-8") | ||
| entry = store.add_python(src) | ||
|
|
||
| real_read_bytes = Path.read_bytes | ||
|
|
||
| def boom(self): | ||
| if self.name == "script.py": | ||
| raise OSError("simulated: unreadable stored copy") | ||
| return real_read_bytes(self) | ||
|
|
||
| monkeypatch.setattr(Path, "read_bytes", boom) | ||
| updated = store.update_dependencies(entry.slug, ["httpx"]) # must not crash | ||
|
|
||
| assert updated.meta.dependencies == ["httpx"] | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Duplicated boom/monkeypatch helper across test files.
The boom OSError-simulation helper (lines 560-566) is byte-identical to the same helper already added in tests/test_atomic.py, tests/test_flows.py, tests/test_store_mut.py, and tests/test_tui_core_cov.py (all at the same line range 560-563). Consider hoisting this into a shared conftest.py fixture (e.g., unreadable_script_path(monkeypatch)) to avoid five copies of the identical mock diverging over time.
♻️ Example shared fixture
# conftest.py
import pytest
from pathlib import Path
`@pytest.fixture`
def unreadable_named_path(monkeypatch):
real_read_bytes = Path.read_bytes
def _apply(target_name: str):
def boom(self):
if self.name == target_name:
raise OSError(f"simulated: unreadable {target_name}")
return real_read_bytes(self)
monkeypatch.setattr(Path, "read_bytes", boom)
return _apply🤖 Prompt for 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.
In `@tests/test_store_fix.py` around lines 548 - 569, Hoist the duplicated
Path.read_bytes OSError mock used by
test_update_dependencies_copy_sync_swallows_read_oserror and the corresponding
tests into a shared pytest fixture in conftest.py, such as
unreadable_named_path(monkeypatch). Update each test to use the fixture with its
target filename, preserving the existing simulated error behavior and
assertions.
Windows CI caught a data-corrupting regression from a1c9ec7 that all three review passes missed — none of them ran on Windows. a1c9ec7 switched `_sync_python_block` from `read_text` (universal newlines) to `read_bytes().decode()` to stop a lossy re-encode. That also stopped folding CRLF, and the block engine's fences match on "\n" only. On a CRLF copy — the default for anything Windows wrote — `parse_block` therefore found nothing, so `skit deps` PREPENDED a second `# /// script` block instead of updating the one already there, and every `[tool.skit]` parameter below it became unreadable. Reproduced end to end: two stacked blocks, `read_params` returning [] where it had returned the managed list. `add_python` had the same hole on its own lane: `has_block` on unfolded CRLF text said False, so a script that already declared its dependencies got a second block injected on top. Both sites now fold for the engine and restore the copy's own line endings on the way out — the discipline `cli._edit_params` already followed, through the `detect_newline`/`restore_newline` pair that moved to rewrite.py earlier in this branch. The injected write also moves from `write_text` to `write_bytes`, since `write_text` re-expands every "\n" to os.linesep and would rewrite an LF script's whole file to CRLF on Windows for the sake of a comment block. Regression tests cover all three directions (CRLF deps edit keeps one block and its params, CRLF add does not double-block, LF add stays LF). Nothing covered any of them before, which is why it shipped. Also: four permission assertions hardcoded 0o755, which Windows cannot represent — it reports 0o666 whatever it is handed. They now capture what chmod actually produced and assert preservation, which is the contract under test.
Owner
Author
Declining — the premise doesn't hold. The five
They also aren't at "the same line range 560-563" in each file. |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
Two commits: a batch of review fixes (
a1c9ec7), then the repairs for the regressions that batch itself introduced (abf7f83).Why the second commit exists
a1c9ec7was reviewed by three independent passes, and every finding was then re-verified against the real code with a reproduction before anything was changed. Ten defects survived that verification — four of them regressions introduced bya1c9ec7, the rest pre-existing. Two reported findings were rejected and are recorded in the commit message with the reasoning.What the second commit fixes
Template quoting. The context-aware quoter added in
a1c9ec7tracked a single quote character, but a command substitution restarts quoting. A value in"$(cmd {v})"was escaped for double quotes and reached the nested command bare (;/|live,a bsplit into two words); a value in"$(cmd "{v}")"was shlex-quoted and arrived wrapped in literal apostrophes. The state is now a frame stack. Double quotes nested in a backtick substitution are refused rather than guessed — that form strips one layer of backslashes before parsing, so the\$guard arrives bare.inputshadowing is resolved per scope.a1c9ec7reused a constant-folding predicate that counts bindings file-wide. For folding, an extra binding just skips a fold; for injection it meant onedef sanitize(input)anywhere disabled the whole script and turned already-stored parameters into hard drift errors on the next run.Parameter assembly.
nargs=N>1is a multi-value field for argparse and click alike, and typed multi-value fields validate per piece (--point 1 2was rejected as "not a whole number", then assembled as one quoted token). click'smultiple=True, nargs=N>1has no representable shape and goes to the passthrough field. fish's=+/=*gained therepeataxis the other readers got. A bool flag that is on by default is refused instead of being stampedstore_true, which made its unticked checkbox deliver nothing.Writes that must not lie. Script settings saved a lossily-read string back as strict UTF-8, so one checkbox tick flattened a CRLF script and burned every non-UTF-8 byte to U+FFFD.
skit deps --clearon a non-UTF-8 copy carrying its own PEP 723 block reported success while uv kept installing the old list — now refused before meta is written.atomic_write_text_keep_modefchmods before the rename so the mode is part of the atomic swap.store.remove's partial-deletion error now reaches the user instead of a traceback (CLI) or a crash out of the terminal (TUI).PEP 420. A bare sibling directory is a namespace portion and never shadows an installed package, so a
docs/orassets/folder colliding with an import name no longer drops a real dependency.Verification
ruff format/ruff check/tyclean, 4969 tests pass, 100% coverage, i18n gate passes with both catalogs at 100%. Targeted mutation testing over all 19 changed functions went from 23 surviving mutants to zero; the one genuine equivalent mutant carries a pragma explaining why.Summary by CodeRabbit
New Features
repeatdetails inshow --jsonform fields).Bug Fixes
input()as managed prompts.Documentation