Repository navigation
kimicli hardening, safe tar extraction, classmethod fixtures - #19
Merged
Merged
Conversation
… safe tar extraction; classmethod fixtures
Reviewer's GuideThis PR hardens installation archive handling, improves kimicli’s session-aware staging and recovery guidance, and updates class-scoped browser fixtures to classmethods, with targeted regression coverage. Sequence diagram for session-aware kimicli stagingsequenceDiagram
participant CLI as kimicli
participant Session
participant Applier
participant User
CLI->>Session: stage_turn
CLI->>Applier: validate current turn
Applier-->>CLI: plans and problems
alt turn has file problems and no parse errors
CLI->>Applier: session_applies_cleanly
Applier-->>CLI: merged_ok
alt earlier turns make the session apply cleanly
CLI-->>User: offer --apply-fixes session
else session still fails
CLI->>CLI: free_skip_command
alt some files apply cleanly
CLI-->>User: offer --skip route
else no safe partial apply
CLI-->>User: offer paid repair
end
end
else parse errors or clean turn
CLI-->>User: show staging result
end
Flow diagram for safe uv archive extractionflowchart TD
A[Open uv archive] --> B{Python supports data_filter}
B -->|Yes| C[extractall with data filter]
B -->|No| D[Resolve each archive target]
D --> E{Path stays under destination}
E -->|No| F[Raise InstallError]
E -->|Yes| G{Entry is link or device}
G -->|Yes| F
G -->|No| H[extractall after manual checks]
C --> I[Locate uv binary]
H --> I
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="kimicli.py" line_range="3269-3273" />
<code_context>
+ with --skip and fix the failing files by hand. None when skipping cannot
+ help - a parse error (a cut-off reply, a malformed block) or no file that
+ applies."""
+ pr = s.parse
+ if any(p.severity == "error" for p in pr.problems + s.group_problems):
+ return None
+ failing = [pl for pl in s.plans if pl.problems]
+ fine = [pl for pl in s.plans if not pl.problems and pl.action != "identical"]
+ if not failing or not fine:
+ return None
</code_context>
<issue_to_address>
**Session command omits earlier proposals**
When an earlier turn has a selected failing proposal or needs creation permissions that are absent from the current turn's plans, `free_skip_command` derives skips and creation flags from the current turn, but `--apply-fixes` loads proposals from every turn; an earlier failing proposal remains selected or its required creation permission is omitted, so the suggested dry run rejects the batch.
Build the skip list and creation flags from the full set of proposals that `--apply-fixes` will load.
Also at `kimicli.py:3276-3279`, `kimicli.py:3362`.
</issue_to_address>
### Comment 2
<location path="kimicli.py" line_range="3362" />
<code_context>
+ print("\n These blocks fail on their own but the session applies cleanly as a whole:")
+ print(" they rely on an earlier turn of this session, and --apply-fixes <session>")
+ print(" applies every turn together (latest emission per file). Next:")
+ print(f"\n python kimicli.py --apply-fixes {session.id} --dry-run --allow-new-files")
+ elif s.repair_path:
print("\n NOT APPLICABLE AS-IS. Nothing will be written until this is fixed.")
</code_context>
<issue_to_address>
**Merged-session command rejects new directories**
When a merged proposal creates a file beneath a directory that does not yet exist, `session_applies_cleanly` validates with `allow_new_dirs=True`, but the suggested merged-session command omits `--allow-new-dirs`, so `run_apply` rejects the session instead of confirming it applies cleanly.
Include `--allow-new-dirs` in the suggested merged-session command.
Also at `kimicli.py:3276-3278`.
</issue_to_address>
### Comment 3
<location path="kimicli.py" line_range="3236-3240" />
<code_context>
errors = [p for p in all_parse if p.severity == "error"]
- if errors or any(pl.problems for pl in plans):
+ if any(pl.problems for pl in plans) and not errors:
+ summary.merged_ok = session_applies_cleanly(session, turn)
+ if errors or (any(pl.problems for pl in plans) and not summary.merged_ok):
body = build_repair_prompt([pl for pl in plans if pl.problems], errors, turn)
</code_context>
<issue_to_address>
**Concurrent turns hide failed proposals**
When two processes stage different turns of the same session concurrently, `session_applies_cleanly` validates a live session snapshot, while concurrent `stage_turn` calls can replace each other’s `proposals.json` updates; it can therefore report a clean merge and suppress the repair even though the completed session contains a failing proposal.
Serialize staging and proposal-index updates per session, and validate a consistent snapshot before suppressing the repair.
</issue_to_address>
### Comment 4
<location path="kimicli.py" line_range="3273" />
<code_context>
+ if any(p.severity == "error" for p in pr.problems + s.group_problems):
+ return None
+ failing = [pl for pl in s.plans if pl.problems]
+ fine = [pl for pl in s.plans if not pl.problems and pl.action != "identical"]
+ if not failing or not fine:
+ return None
</code_context>
<issue_to_address>
**Skipping one failure invalidates another plan**
When a failing Python module edit has a syntax error and a clean plan imports a name that edit adds, `free_skip_command` assumes every non-failing plan remains applicable after the failing plans are skipped. When a failing Python edit has a syntax error but introduces a name imported by a clean plan, validation can accept that import while considering the staged module; applying the suggested skip removes that module from the batch, and revalidation rejects the remaining plan.
Validate the proposal set after excluding the failing files before offering the skip command, and include any newly failing dependent plans in the skips.
</issue_to_address>
### Comment 5
<location path="kimicli.py" line_range="5196-5197" />
<code_context>
+ check("when only some files fail, the free --skip route is offered before a paid repair; "
+ "never when the reply itself is broken",
+ sh.repair_path is not None and free is not None and "--skip notes.md" in free
+ and "pkg/lib.py" not in free.split("--dry-run", 1)[1]
+ and free_skip_command(broken, "x") is None, str(free))
finally:
g.update(saved_paths)
</code_context>
<issue_to_address>
**Invalid skip command passes**
When `free_skip_command` returns a dry-run string containing `--skip notes.md` but omitting or misidentifying the session, `selftest_h` only checks for `--skip notes.md` and `--dry-run`; a command that omits `--apply-fixes selftest_h` still passes, even though it cannot apply this session’s other files.
Assert the returned command includes `--apply-fixes selftest_h` as well as the expected skip and dry-run arguments.
</issue_to_address>Sourcery assessment
Approval pending. 5 findings to address first.
Blocking findings: kimicli.py:3273, kimicli.py:3362, kimicli.py:3240, kimicli.py:3273, kimicli.py:5197
Comment on lines
+3269
to
+3273
| pr = s.parse | ||
| if any(p.severity == "error" for p in pr.problems + s.group_problems): | ||
| return None | ||
| failing = [pl for pl in s.plans if pl.problems] | ||
| fine = [pl for pl in s.plans if not pl.problems and pl.action != "identical"] |
There was a problem hiding this comment.
🟡 Medium · Session command omits earlier proposals
When an earlier turn has a selected failing proposal or needs creation permissions that are absent from the current turn's plans, free_skip_command derives skips and creation flags from the current turn, but --apply-fixes loads proposals from every turn; an earlier failing proposal remains selected or its required creation permission is omitted, so the suggested dry run rejects the batch.
Build the skip list and creation flags from the full set of proposals that --apply-fixes will load.
Also at kimicli.py:3276-3279, kimicli.py:3362.
Prompt for AI agents
In `kimicli.py` at lines 3269-3273:
**Session command omits earlier proposals**
When an earlier turn has a selected failing proposal or needs creation permissions that are absent from the current turn's plans, `free_skip_command` derives skips and creation flags from the current turn, but `--apply-fixes` loads proposals from every turn; an earlier failing proposal remains selected or its required creation permission is omitted, so the suggested dry run rejects the batch.
Build the skip list and creation flags from the full set of proposals that `--apply-fixes` will load.
Also at `kimicli.py:3276-3279`, `kimicli.py:3362`.| print("\n These blocks fail on their own but the session applies cleanly as a whole:") | ||
| print(" they rely on an earlier turn of this session, and --apply-fixes <session>") | ||
| print(" applies every turn together (latest emission per file). Next:") | ||
| print(f"\n python kimicli.py --apply-fixes {session.id} --dry-run --allow-new-files") |
There was a problem hiding this comment.
🟡 Medium · Merged-session command rejects new directories
When a merged proposal creates a file beneath a directory that does not yet exist, session_applies_cleanly validates with allow_new_dirs=True, but the suggested merged-session command omits --allow-new-dirs, so run_apply rejects the session instead of confirming it applies cleanly.
Include --allow-new-dirs in the suggested merged-session command.
Also at kimicli.py:3276-3278.
Prompt for AI agents
In `kimicli.py` at line 3362:
**Merged-session command rejects new directories**
When a merged proposal creates a file beneath a directory that does not yet exist, `session_applies_cleanly` validates with `allow_new_dirs=True`, but the suggested merged-session command omits `--allow-new-dirs`, so `run_apply` rejects the session instead of confirming it applies cleanly.
Include `--allow-new-dirs` in the suggested merged-session command.
Also at `kimicli.py:3276-3278`.
Comment on lines
+3236
to
3240
| summary.merged_ok = session_applies_cleanly(session, turn) | ||
| if errors or (any(pl.problems for pl in plans) and not summary.merged_ok): | ||
| body = build_repair_prompt([pl for pl in plans if pl.problems], errors, turn) | ||
| if body: | ||
| summary.repair_path = session.dir / f"repair_turn{turn}.md" |
There was a problem hiding this comment.
🟡 Medium · Concurrent turns hide failed proposals
When two processes stage different turns of the same session concurrently, session_applies_cleanly validates a live session snapshot, while concurrent stage_turn calls can replace each other’s proposals.json updates; it can therefore report a clean merge and suppress the repair even though the completed session contains a failing proposal.
Serialize staging and proposal-index updates per session, and validate a consistent snapshot before suppressing the repair.
Prompt for AI agents
In `kimicli.py` at lines 3236-3258:
**Concurrent turns hide failed proposals**
When two processes stage different turns of the same session concurrently, `session_applies_cleanly` validates a live session snapshot, while concurrent `stage_turn` calls can replace each other’s `proposals.json` updates; it can therefore report a clean merge and suppress the repair even though the completed session contains a failing proposal.
Serialize staging and proposal-index updates per session, and validate a consistent snapshot before suppressing the repair.| if any(p.severity == "error" for p in pr.problems + s.group_problems): | ||
| return None | ||
| failing = [pl for pl in s.plans if pl.problems] | ||
| fine = [pl for pl in s.plans if not pl.problems and pl.action != "identical"] |
There was a problem hiding this comment.
🟡 Medium · Skipping one failure invalidates another plan
When a failing Python module edit has a syntax error and a clean plan imports a name that edit adds, free_skip_command assumes every non-failing plan remains applicable after the failing plans are skipped. When a failing Python edit has a syntax error but introduces a name imported by a clean plan, validation can accept that import while considering the staged module; applying the suggested skip removes that module from the batch, and revalidation rejects the remaining plan.
Validate the proposal set after excluding the failing files before offering the skip command, and include any newly failing dependent plans in the skips.
Prompt for AI agents
In `kimicli.py` at line 3273:
**Skipping one failure invalidates another plan**
When a failing Python module edit has a syntax error and a clean plan imports a name that edit adds, `free_skip_command` assumes every non-failing plan remains applicable after the failing plans are skipped. When a failing Python edit has a syntax error but introduces a name imported by a clean plan, validation can accept that import while considering the staged module; applying the suggested skip removes that module from the batch, and revalidation rejects the remaining plan.
Validate the proposal set after excluding the failing files before offering the skip command, and include any newly failing dependent plans in the skips.
Comment on lines
+5196
to
+5197
| and "pkg/lib.py" not in free.split("--dry-run", 1)[1] | ||
| and free_skip_command(broken, "x") is None, str(free)) |
There was a problem hiding this comment.
🟡 Medium · Invalid skip command passes
When free_skip_command returns a dry-run string containing --skip notes.md but omitting or misidentifying the session, selftest_h only checks for --skip notes.md and --dry-run; a command that omits --apply-fixes selftest_h still passes, even though it cannot apply this session’s other files.
Assert the returned command includes --apply-fixes selftest_h as well as the expected skip and dry-run arguments.
Prompt for AI agents
In `kimicli.py` at lines 5196-5197:
**Invalid skip command passes**
When `free_skip_command` returns a dry-run string containing `--skip notes.md` but omitting or misidentifying the session, `selftest_h` only checks for `--skip notes.md` and `--dry-run`; a command that omits `--apply-fixes selftest_h` still passes, even though it cannot apply this session’s other files.
Assert the returned command includes `--apply-fixes selftest_h` as well as the expected skip and dry-run arguments.
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
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.
--skiproute before a paid repair.Summary by Sourcery
Harden kimicli turn staging and installer archive extraction while modernizing browser fixtures.
New Features:
Bug Fixes:
Enhancements:
Tests: