Repository navigation
distill: mine "A fails here, B works" repair pitfalls - #64
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b8b4bd7da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 0b8b4bd: 4 finding(s), 4 blocking.
The change mines repair claims from failed shell commands followed by successful alternatives. It is not safe to merge yet: it can expose credentials and record repairs unsupported or contradicted by the session.
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 894aef6: 4 finding(s), 4 blocking.
The change mines repair claims from failed commands followed by successful alternatives. It is not safe to merge yet: credentials can leak into shared claims, and invocation matching and shell-chain handling can produce false repair advice.
1 finding(s) dropped by the check
kypp/repairs.py:190Exclude bare probe runs from the unchanged-success guard: For the reported command-not-found failure, a successful bare pytest --version proves pytest is now on PATH, so _works_unchanged_later correctly suppresses the claim that a wrapper is required.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 894aef62a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…line code, no @ in values
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83cad14c43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 83cad14: 3 finding(s), 3 blocking.
The change mines environment repair pitfalls from failed shell commands followed by successful alternatives. Three confirmed cases produce unsupported claims, so these defects should be fixed before merging.
…p fixes that set variables
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of d184772: 4 finding(s), 4 blocking.
The change mines repair advice from failed commands followed by successful alternatives. Four confirmed gaps allow unrelated successes or unproven executions to become repair advice; these should be fixed before merging.
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 2ae4fa8: 3 finding(s), 3 blocking.
The change mines command and environment repairs from session traces and adds them to distillation. Three confirmed defects miss valid Codex repairs or record unproven repairs, so it should be fixed before merging.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e658633e54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of e658633: 3 finding(s), 3 blocking.
The change mines command repairs from session traces and adds them to distilled pitfalls. Three confirmed defects cause missed Codex repairs or unsupported repair claims, so it should be fixed before merging.
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of f0a7964: 4 finding(s), 4 blocking.
The change mines command repairs from session traces and adds them to distilled memory. Four confirmed defects can produce unsupported repair claims or suppress valid ones, so it should be fixed before merging.
…pare place before suppressing
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 2f7db34: 3 finding(s), 3 blocking.
The change adds deterministic mining of command repairs into memory claims. Three introduced defects can suppress valid repairs or report incomplete fixes; these should be fixed before merging.
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 8e01e90: 2 finding(s), 2 blocking.
The change adds deterministic mining of environment repair claims from shell traces. Two confirmed defects can create false repair claims or suppress valid ones, so they should be fixed before merging.
| return [] | ||
| out, cur, before = [], [], "" | ||
| for t in toks + [""]: | ||
| if t in _OPS or t == "": |
There was a problem hiding this comment.
P2 Preserve quoting when identifying shell operators
With a failed pytest followed by successful echo ';' uv run pytest, the miner emits a repair claiming uv run pytest works, although only echo ran. POSIX shlex removes the quotes, and this loop treats the quoted semicolon as a shell separator. Preserve whether operator tokens were quoted or escaped, and add a regression test so printed command arguments cannot become proven executions.
There was a problem hiding this comment.
Fixed in 084ba9e. Before lexing, quoted or escaped ;, & and | are masked, so they stay arguments and never act as operators. Regression cases for echo ';' …, echo "a|b" … and echo \; … are added.
Generated by Claude Code
| for r in runs: | ||
| if r.i > a.i and r.ok and (not same_place or r.where == a.where): | ||
| for env, toks in map(_split_env, r.proven): | ||
| if toks[:1] == [head] and toks[1:1 + len(args)] == args and (var is None or var not in env): |
There was a problem hiding this comment.
P2 Exclude probes from unchanged-run recovery checks
If python app.py fails importing a dependency, uv run python app.py succeeds, and python app.py --help later succeeds through an early help path, this predicate suppresses the valid repair. The help run does not prove the original invocation works. Apply the probe exclusions used for working forms here too, including for missing-environment-variable repairs, and test this sequence.
There was a problem hiding this comment.
Fixed in 084ba9e. A probe no longer counts as the unchanged run working, for both the module and the env repairs. The shared _probes check is now used in all three places, and both sequences are covered by tests.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86c3a5cffb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for t in toks + [""]: | ||
| if t in _OPS or t == "": |
There was a problem hiding this comment.
When a quoted argument is exactly a shell operator, POSIX shlex removes its quotes before this check, so an ordinary argument becomes a command separator. For example, a failed pytest followed by successful echo ";" uv run pytest produces a pitfall claiming that uv run pytest works, although it was never executed. Bash's help echo describes its behavior as “Write arguments to the standard output,” and executing this example confirms it only prints the arguments. Preserve quote information when recognizing operators so printed command text cannot become repair evidence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed in 084ba9e: quoted and escaped operators are masked before lexing, and regression tests cover them.
Generated by Claude Code
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 86c3a5c: 4 finding(s), 4 blocking.
The change mines repair pitfalls from failed shell commands followed by successful alternatives. It is not safe to merge yet: parsing and environment checks can produce unsupported claims or miss valid repairs.
| return [] | ||
| out, cur, before = [], [], "" | ||
| for t in toks + [""]: | ||
| if t in _OPS or t == "": |
There was a problem hiding this comment.
P2 Preserve quoting when identifying shell operators
After a failed pytest, a successful echo ';' uv run pytest produces a claim that uv run pytest works, although the shell only ran echo. POSIX shlex removes quotes, so the quoted semicolon is mistaken for an operator. Preserve whether punctuation was quoted before splitting commands, and add a regression test.
There was a problem hiding this comment.
Already fixed in 084ba9e: quoted and escaped operators are masked before lexing, and regression tests cover them.
Generated by Claude Code
| if not (missing_cmd or missing_mod or missing_env): | ||
| continue | ||
| seg = _target(a.segments, missing_cmd) | ||
| env, toks = _split_env(seg or []) |
There was a problem hiding this comment.
P2 Account for the failing command's environment
A failure of PATH=/nonexistent pytest followed by successful uv run pytest emits the unconditional subject 'pytest is not on PATH', discarding the temporary PATH override that caused the failure. The parsed failure environment is currently ignored for command and module repairs. Reject environment-confounded failures or require equivalent environments and retain the relevant condition.
There was a problem hiding this comment.
Fixed in 6c0b90e. A failure whose run changes env in any way (PATH=/x pytest, export PATH=/x; pytest) is no longer mined. Tests cover both cases.
Generated by Claude Code
|
|
||
| def _env_fix(name: str, head: str, args: list[str], later: _Run) -> str | None: | ||
| """The later run that sets `name` and runs the same thing, as the fact shows it.""" | ||
| for env, toks in map(_split_env, later.proven): |
There was a problem hiding this comment.
P2 Reject additional environment setup in env repairs
For a missing API_BASE error, export PYTHONPATH=src && API_BASE=x python app.py is accepted as proof that setting API_BASE alone works. _env_fix checks only the target segment and omits the additional environment change, unlike _working_form's run-wide guard. Reject other setup or assignments in the successful run so the recorded repair includes every change needed for success.
There was a problem hiding this comment.
Fixed in 6c0b90e. An env fix is now rejected if any other segment of the working run sets env or does setup. The run-wide guard is shared with _working_form as _changes_env, and the test covers it.
Generated by Claude Code
| for i, a in enumerate(trace.actions): | ||
| cmd = _command_of(a) | ||
| if cmd and "<<" not in cmd: # a heredoc body isn't commands | ||
| flat = cmd.replace("\n", "; ") |
There was a problem hiding this comment.
P2 Preserve comment boundaries in multiline commands
A successful repair written as # Run tests\nuv run pytest is silently ignored: replacing the newline with a semicolon makes shlex treat the entire command as a comment. Inline comments likewise swallow subsequent lines. Parse newlines as shell separators while preserving comment boundaries instead of flattening the source first.
There was a problem hiding this comment.
Fixed in 6c0b90e. The masking pass now drops # comments up to the end of the line. An unquoted newline ends a command unless its line ends on an operator, and \-newline joins lines. The source is no longer flattened, and tests cover a leading comment, an inline comment and a continuation.
Generated by Claude Code
…e an unchanged run works
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of b5adba2: 1 finding(s), 1 blocking.
The change adds deterministic mining of successful command repairs from session traces. It is not safe to merge as-is because a script overwrite can be mistaken for an environment repair and produce false shared-memory claims.
| return True | ||
| runs_head = re.compile(rf"(?<![\w./-]){re.escape(head)}\b") | ||
| # The invocation's own `> log` is output, not an edit; any other segment's write may be one. | ||
| return any(_IN_PLACE.search(seg) if runs_head.search(seg) else _SHELL_EDIT.search(seg) |
There was a problem hiding this comment.
P2 Identify the invoked command before exempting output redirections
After python app.py fails importing turso, echo 'print("python")' > app.py && uv run python app.py incorrectly produces a repair and marks the failure explained. runs_head.search(seg) matches python inside the echoed text, so the script overwrite is exempted from _SHELL_EDIT. The successful run proves only that the rewritten script works. Determine which command the segment invokes from its parsed tokens before exempting its output redirection, and add a regression test for this case.
There was a problem hiding this comment.
Fixed in 62a3d66. A segment counts as the invocation only when its parsed words run the command first or behind an allowlisted wrapper, so echo 'print("python")' > app.py and echo python > app.py are both checked as writes. Both cases are added to the test.
Generated by Claude Code
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 62a3d66: 1 finding(s), 1 blocking.
The change mines shell failures followed by successful alternative invocations into repair claims. It can incorrectly attribute success to a wrapper after an intervening directory change, so this finding should be fixed before merging.
| for seg in re.split(r"[;&|]", _mask(b.command))) | ||
|
|
||
|
|
||
| def _same_place(a: _Run, b: _Run) -> bool: |
There was a problem hiding this comment.
P2 Track directory changes between shell actions
In Claude Bash sessions, the working directory persists across calls. For python app.py failing to import project, followed by a separate cd other call and then successful uv run python app.py, both Python runs receive the same where tuple because neither invocation contains cd. The miner consequently claims the wrapper fixes the import even though it ran a different script. Track directory changes across actions, or invalidate place comparisons after an intervening directory change, and add a regression test for this sequence.
There was a problem hiding this comment.
Fixed in 0ad24a8. where now carries a count of the standalone cd/pushd/popd calls made before the run, so a separate cd other between the two runs changes the place. cd x && cmd is still read as self-contained, since its target is already in where. Test: test_a_cd_in_between_changes_the_place.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62a3d66135
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # a relative path (`.venv/bin/pytest`) only works from where it ran | ||
| if form and "/" in form[0] and not _same_place(a, b): | ||
| form = None |
There was a problem hiding this comment.
When a relative executable path appears behind a wrapper, this guard checks only the wrapper name. For example, failed pytest followed by successful cd backend && uv run .venv/bin/pytest produces a claim that uv run .venv/bin/pytest works and marks the failure explained, even though that path is relative to backend, not the original directory. (uv run --help describes its argument as a “command or script.”) Check the matched executable token for a relative path and require matching directories, rather than checking only form[0].
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 106f929. The relative-path check now looks at the executable token instead of the wrapper. cd backend && uv run .venv/bin/pytest was added to the test.
Generated by Claude Code
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 0ad24a8: 2 finding(s), 2 blocking.
The change adds deterministic mining of command repairs from session traces. Two confirmed defects allow misleading repairs after intervening environment changes and host-specific absolute paths in shared claims, so it should be fixed before merging.
| continue | ||
| # Each failure needs its own repair to count as explained; the first one found writes the claim. | ||
| # A script or module is only the same one run from the same place. | ||
| for b in (r for r in runs if r.i > a.i and r.ok and (missing_cmd or _same_place(a, r))): |
There was a problem hiding this comment.
P2 Reject repairs after intervening environment changes
A trace containing failed python app.py with No module named 'project', then export PYTHONPATH=src, then successful uv run python app.py produces a claim that uv run python … works. In a persistent shell, the export can be the actual fix, so the recorded repair omits required setup. Check intervening runs for persistent environment changes, as well as installs and edits, and suppress repairs whose cause is ambiguous.
There was a problem hiding this comment.
Fixed in 2d94ec7: an export, source/., set/unset or bare assignment in any run between the failure and the working run now suppresses the repair, the same way an install does. Test: test_an_export_between_the_runs_may_be_the_fix.
Generated by Claude Code
| break | ||
| form = toks[:j + 1] | ||
| # a host path, URL or `user@` may carry a home dir or a token; none belongs in shared memory | ||
| return None if _ABS.search(" " + " ".join(form)) or any("://" in t or "@" in t for t in form) else form |
There was a problem hiding this comment.
P2 Reject all absolute host paths in working forms
After pytest fails with command-not-found, successful /opt/company/venv/bin/pytest is emitted verbatim as a repair. _ABS only recognizes selected directory prefixes, so absolute paths under /opt, /workspace, /mnt, and other roots bypass the promised host-path guard and enter shared memory. Reject absolute command and wrapper paths structurally rather than using a directory allowlist of exclusions, and add coverage for these roots.
There was a problem hiding this comment.
Fixed in 2d94ec7: _ABS is now structural. Any token, or env value part after :/=, that starts with / or ~ is rejected, so /opt, /workspace, /mnt and ~/.local are all covered. The tests cover those roots and an absolute wrapper path.
Generated by Claude Code
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 106f929: 2 finding(s), 2 blocking.
The change mines repair pitfalls from failed commands followed by successful alternatives. Two confirmed state-tracking defects can produce misleading claims or suppress valid ones, so it should be fixed before merging.
| where = None if any(moves[first_cmd:]) else ( | ||
| moved, str(a.input.get("workdir") or a.input.get("cwd") or ""), | ||
| *(" ".join(t[1:]) for t in segs[:first_cmd])) | ||
| moved += all(moves) |
There was a problem hiding this comment.
P2 Track directory changes in compound commands across runs
In a persistent Bash session, python app.py failing, followed by cd other && ls, then a successful uv run python app.py, produces a repair even though the two invocations run different scripts. moved only changes when every segment is a directory command, so the compound command leaves the subsequent run with the original location identity. Invalidate or update the persistent directory state whenever a run can change it, including compound commands.
There was a problem hiding this comment.
Fixed in 1e32b12: any run containing pushd/popd or a relative cd, compound or not, now moves the shell for later runs. Only an all-absolute cd /x is treated as landing in the same place again. Test: test_a_cd_inside_a_compound_command_moves_the_shell.
Generated by Claude Code
| continue | ||
| # Each failure needs its own repair to count as explained; the first one found writes the claim. | ||
| # A script or module is only the same one run from the same place. | ||
| for b in (r for r in runs if r.i > a.i and r.ok and (missing_cmd or _same_place(a, r))): |
There was a problem hiding this comment.
P2 Account for environment changes between paired runs
Environment checks inspect only the failing and working actions. Thus python app.py failing to import project, then export PYTHONPATH=src, then successful uv run python app.py incorrectly attributes the repair to uv run. The reverse also loses valid repairs: after an explicit env fix, a standalone export followed by a bare success is treated as unchanged and suppresses the claim. Track persistent environment changes across actions, or conservatively stop comparisons across exports, unsets, and sourced setup.
There was a problem hiding this comment.
The first case was already fixed in 2d94ec7 (env changes between the runs suppress the repair). I'm declining the reverse case: it only drops a claim, and dropping one is the safe direction for a heuristic floor.
Generated by Claude Code
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 2d94ec7: 1 finding(s), 1 blocking.
The change mines repair pitfalls from failed shell commands followed by successful alternatives. It should not merge yet: compound directory changes can cause it to record false repairs across different working directories.
| where = None if any(moves[first_cmd:]) else ( | ||
| moved, str(a.input.get("workdir") or a.input.get("cwd") or ""), | ||
| *(" ".join(t[1:]) for t in segs[:first_cmd])) | ||
| moved += all(moves) |
There was a problem hiding this comment.
P2 Track persistent directory changes in compound shell calls
In Claude's persistent Bash environment, python app.py failing, followed by cd other && true, then uv run python app.py succeeding produces a repair even though the two invocations run different scripts. moved advances only when every segment is a directory command, so the compound call leaves subsequent runs with the original directory identity. Track directory changes in compound calls too, or invalidate subsequent directory comparisons when their resulting directory is uncertain; add a regression covering this sequence and relative executable paths.
There was a problem hiding this comment.
Same as the finding above, and already fixed in 1e32b12. A compound cd other && … now moves the shell, so the later run doesn't pair with the earlier one. A relative-path executable needs the same place, so it's covered too.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d94ec7fb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif t and set(t) <= set("<>&") and set(t) & set("<>"): | ||
| skip = True |
There was a problem hiding this comment.
When a script chooses its behavior from stdin, removing input redirections makes different executions appear identical. For example, python app.py < deploy.txt can fail importing a dependency while .venv/bin/python app.py < status.txt succeeds simply because the second input skips that import. I reproduced this using a symlink to the same Python interpreter: the miner nevertheless emits a working-interpreter repair and marks the failure explained. Preserve input redirections in invocation matching, or conservatively reject pairs whose input equivalence cannot be established.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 01146e6: a run with an unquoted < input is now never used as either the failure or the repair. Test: test_a_run_reading_stdin_may_take_another_path.
Generated by Claude Code
| for b in (r for r in runs if r.i > a.i and r.ok and (missing_cmd or _same_place(a, r))): | ||
| if _installed_between(runs, a, b) or (not missing_cmd and _edited_between(trace, a, b, head)): |
There was a problem hiding this comment.
When a missing-command failure has no matching repair, every later successful run triggers _installed_between, which scans the intervening runs and tokenizes their commands again before checking whether the candidate even runs the failed command. A trace with one failed pytest followed by 100, 200, and 400 successful unrelated commands took approximately 0.15, 0.66, and 2.59 seconds respectively in this environment; multiple unresolved failures compound that cost. This makes the newly unconditional repair pass expensive for long transcripts even when it emits nothing. Cache per-run install/environment metadata and check candidate compatibility before performing interval checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 01146e6: install and env flags are now computed once per run, and the between-runs scans run only after a candidate fix is found. Test: test_unrelated_successes_stay_cheap (400 runs).
Generated by Claude Code
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 1e32b12: 1 finding(s), 1 blocking.
The change mines repair claims from failed commands followed by successful alternatives. It is not safe to merge yet: an intervening absolute directory change can produce a false repair claim and incorrectly mark the failure as explained.
| where = None if any(moves[first_cmd:]) else ( | ||
| moved, str(a.input.get("workdir") or a.input.get("cwd") or ""), | ||
| *(" ".join(t[1:]) for t in segs[:first_cmd])) | ||
| moved += any(t[0] != "cd" or not t[1:] or not all(x.startswith("/") for x in t[1:]) for t in segs if t[0] in ("cd", "pushd", "popd")) |
There was a problem hiding this comment.
P2 Track absolute directory changes between runs
An absolute cd never updates moved, so a persistent Bash session containing failing python app.py, successful cd /other, then successful uv run python app.py produces a repair claim even though the two invocations run different scripts. This also incorrectly marks the original failure as explained. Track the current absolute directory, or invalidate directory identity after any directory change unless both invocations explicitly establish the same location; add a regression test for this sequence.
There was a problem hiding this comment.
Fixed in 883c3b4: the shell's directory is now followed through every cd. An absolute cd resets it, a relative one appends to it, and cd -/pushd/popd make it unmatchable. A call with an explicit workdir starts there. Two runs pair only when they start in the same place. Tests are in test_a_cd_inside_a_compound_command_moves_the_shell.
Generated by Claude Code
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 01146e6: 2 finding(s), 2 blocking.
The change adds deterministic mining of command repairs from session traces. Two confirmed directory-tracking defects can generate false repairs or miss valid Codex repairs, so it should be fixed before merging.
| where = None if any(moves[first_cmd:]) else ( | ||
| moved, str(a.input.get("workdir") or a.input.get("cwd") or ""), | ||
| *(" ".join(t[1:]) for t in segs[:first_cmd])) | ||
| moved += any(t[0] != "cd" or not t[1:] or not all(x.startswith("/") for x in t[1:]) for t in segs if t[0] in ("cd", "pushd", "popd")) |
There was a problem hiding this comment.
P2 Track absolute directory changes across persistent shell calls
A failed python app.py, followed by cd /other, then successful uv run python app.py produces a repair even though the commands run different scripts. Absolute cd commands never increment moved, and their target is forgotten by subsequent calls. Track the persistent absolute directory, or invalidate directory equality after such a change; add a regression test for this sequence.
There was a problem hiding this comment.
Already fixed in 883c3b4, where the shell's directory is followed through an absolute cd. The regression for cd /other is in test_a_cd_inside_a_compound_command_moves_the_shell.
Generated by Claude Code
| # A shell that persists (Claude's Bash) keeps every `cd`, so count those before this run; only a | ||
| # `cd /abs` lands in the same place again, wherever the shell was. | ||
| where = None if any(moves[first_cmd:]) else ( | ||
| moved, str(a.input.get("workdir") or a.input.get("cwd") or ""), |
There was a problem hiding this comment.
P2 Avoid carrying relative directory changes across Codex calls
Two Codex exec_command calls using cd backend && python app.py and cd backend && uv run python app.py run from the same directory, but no repair is emitted because the first call increments the persistent moved counter. Codex starts a fresh shell for each call, so the second relative cd does not nest the directory. Make directory tracking harness-aware and test this missing-module repair with two independent Codex calls.
There was a problem hiding this comment.
Fixed in c298b45: only Claude's Bash (with no named workdir) carries its directory across calls. Codex and other shells start fresh each call. Test: test_each_codex_call_starts_a_fresh_shell.
Generated by Claude Code
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 883c3b4: 2 finding(s), 2 blocking.
The change adds deterministic mining of successful repairs from shell-command failures. Two introduced defects suppress valid repairs involving persistent environment exports or independent Codex shell calls; these should be fixed before merging.
| def _works_unchanged_later(runs: list[_Run], a: _Run, head: str, args: list[str], same_place: bool) -> bool: | ||
| """The same thing ran bare later, without the fix, and worked: installed or fixed mid-session.""" | ||
| for r in runs: | ||
| if r.i > a.i and r.ok and (not same_place or _same_place(a, r)) and not _changes_env(r.segments): |
There was a problem hiding this comment.
P2 Track persistent environment changes when checking unchanged success
For node build.js failing with API_BASE is not set, followed by API_BASE=x node build.js, export API_BASE=x, and a successful node build.js, the miner returns no repair. The final invocation still uses the fix through the persistent export, but this check examines only assignments in that invocation. Track intervening persistent environment changes so a run that inherits the fix does not count as unchanged success.
There was a problem hiding this comment.
Declining, same as the reverse case above: this only drops a claim, which is the safe direction for the heuristic floor. The Codex fresh-shell finding in this review is fixed in c298b45.
Generated by Claude Code
|
|
||
|
|
||
| def _split_env(toks: list[str]) -> tuple[dict[str, str], list[str]]: | ||
| env, k = {}, 0 |
There was a problem hiding this comment.
P2 Apply persistent directory tracking only to persistent shells
Two Codex shell calls running cd server && python app.py and then cd server && uv run python app.py execute from the same directory, but the miner treats the second as running in server/server and discards the repair. Codex shell calls launch separate processes, so their cd commands do not persist. Select directory tracking by harness and reset the base directory for independent shell calls.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c298b45d35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if any(moves) and persists: | ||
| shell = _walk(base, [t for t, m in zip(segs, moves) if m], unknown) |
There was a problem hiding this comment.
For Claude Bash actions, this updates the persistent directory from every parsed cd, including pipeline stages that execute in a subshell and do not change the parent shell's directory. A fresh reproducer is successful cd /other | cat, followed by python app.py failing to import project, then successful cd /other && uv run python app.py: mine emits a repair and marks the failure explained, although the two scripts ran in different directories. Track whether each directory change actually affects the persistent shell, or mark its location unknown when execution context cannot be established.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6ca0629: a run that has a cd along with a | or & now leaves the shell's place unknown, so no later run can pair with it. Test: test_a_piped_cd_leaves_the_place_unknown.
Generated by Claude Code
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 6ca0629: 1 finding(s), 1 blocking.
The change adds deterministic mining of command and environment repairs from session traces. A confirmed defect suppresses valid environment repairs after a separate persistent export, so this should be fixed before merging.
| def _works_unchanged_later(runs: list[_Run], a: _Run, head: str, args: list[str], same_place: bool) -> bool: | ||
| """The same thing ran bare later, without the fix, and worked: installed or fixed mid-session.""" | ||
| for r in runs: | ||
| if r.i > a.i and r.ok and (not same_place or _same_place(a, r)) and not _changes_env(r.segments): |
There was a problem hiding this comment.
P2 Account for inherited environment changes before suppressing repairs
A trace containing failing node build.js, successful API_BASE=x node build.js, a separate export API_BASE=x, and successful bare node build.js produces no repair. _works_unchanged_later checks only the bare run's own segments, so it mistakes a run inheriting the exported fix for evidence that the variable is unnecessary. Track intervening persistent environment changes, or exclude such later runs from unchanged-success evidence; add a test with the export in a separate action.
There was a problem hiding this comment.
Fixed in 080c429, since this came up three times and the fix is small. A persistent env change (export/source/unset, or a standalone assignment) between the failure and a bare later run now keeps that run from counting as unchanged success. Inline X=1 cmd is not treated as persistent. Test: test_a_bare_run_after_an_export_inherits_the_fix.
Generated by Claude Code
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 080c429: 2 finding(s), 2 blocking.
The change mines shell failures followed by successful alternatives into repair claims. Two confirmed false-positive paths produce unsupported claims, so these should be fixed before merging.
| base = shell or ("", "") if persists else ("", wd) | ||
| where = None if any(moves[first_cmd:]) else _walk(base, segs[:first_cmd], unknown) | ||
| if any(moves) and persists: | ||
| shell = _walk(base, [t for t, m in zip(segs, moves) if m], unknown) |
There was a problem hiding this comment.
P2 Track only directory changes that actually executed
After false && cd /other, _runs records /other as the persistent directory even though cd never executed. A subsequent failing python app.py and successful cd /other && uv run python app.py then generate a repair despite running different scripts. Update directory state only for proven directory changes, or mark it unknown when execution is uncertain; add a regression test for skipped and failed cd commands.
There was a problem hiding this comment.
Fixed in 2f03dd5: the cds are followed only when the run succeeded and is chained by && alone. Anything else (a failed run, ||, ;, |, &) leaves the place unknown. Test: test_a_cd_that_may_not_have_run_leaves_the_place_unknown.
Generated by Claude Code
| def _env_fix(name: str, head: str, args: list[str], later: _Run) -> str | None: | ||
| """The later run that sets `name` and runs the same thing, as the fact shows it.""" | ||
| for seg in later.proven: | ||
| env, toks = _split_env(seg) |
There was a problem hiding this comment.
P2 Exclude export statements from executable env fixes
A failing python app reporting API_BASE is not set, followed by successful export API_BASE=x python app, produces a claim that setting the variable fixes the command. However, export only exports the identifiers python and app; it never runs Python. Reject export segments as repair invocations rather than treating them like env or leading assignments, and test this case.
There was a problem hiding this comment.
Fixed in 2f03dd5: an export segment now runs no command, so it can't be a repair invocation. Test: test_an_export_runs_nothing.
Generated by Claude Code
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 2f03dd5: 2 finding(s), 2 blocking.
The change mines environment repair claims from failed commands followed by successful alternatives. Two confirmed cases produce false claims, so these should be fixed before merging.
|
|
||
|
|
||
| def _works_unchanged_later(runs: list[_Run], a: _Run, head: str, args: list[str], same_place: bool) -> bool: | ||
| """The same thing ran bare later, without the fix, and worked: installed or fixed mid-session.""" |
There was a problem hiding this comment.
P2 Restrict inherited-environment checks to persistent shells
In a Codex trace, pytest failing, uv run pytest succeeding, an unrelated export FOO=bar, and then bare pytest succeeding still produces a “pytest is not on PATH” claim. _works_unchanged_later treats every intervening export as inherited, although Codex calls start fresh shells. Track whether environment changes persist into the later run so a confirmed bare success suppresses the repair.
There was a problem hiding this comment.
Fixed in 9bd3a06: an env change now counts as inherited only in a persistent shell (Claude's Bash with no named workdir). Test: test_a_codex_export_does_not_reach_the_next_call.
Generated by Claude Code
| return True | ||
| return False | ||
| # The invocation's own `> log` is output, not an edit; any other segment's write may be one. | ||
| return any(_IN_PLACE.search(seg) if runs_head(seg) else _SHELL_EDIT.search(seg) |
There was a problem hiding this comment.
P2 Detect output redirections that overwrite the invoked script
After python app.py fails with a missing import, uv run python app.py > app.py is accepted as a repair. The shell truncates app.py before Python opens it, so success comes from running an empty script. _edited_between exempts all output redirects on the invocation itself. Check whether their destinations overwrite the script or other relevant source files before accepting the repair.
There was a problem hiding this comment.
Declining: an agent running uv run python app.py > app.py to fix an import is contrived. Claims are also candidates until a second session confirms the same subject, so a one-off like this doesn't become accepted memory.
Generated by Claude Code
vu1n
left a comment
vu1n
left a comment
There was a problem hiding this comment.
VPS review of 9bd3a06: 2 finding(s), 2 blocking.
The change adds deterministic mining of command and environment repairs from session traces. Two confirmed defects can record a misleading repair or suppress a valid one, so it should be fixed before merging.
| else: | ||
| form = _working_form(head, args, b) | ||
| # a relative path (`.venv/bin/pytest`, `uv run .venv/bin/pytest`) only works from where it ran | ||
| if form and "/" in form[-1] and not _same_place(a, b): |
There was a problem hiding this comment.
P2 Check relative paths in the wrapper as well as the target
After pytest fails in one directory, cd other && .venv/bin/python -m pytest is accepted as its repair. The directory guard checks only form[-1] (pytest), overlooking the relative interpreter path. This records a working form that resolves to a different environment from the failing location. Require matching directories whenever any executable path in the working form is relative.
| """The same thing ran bare later, without the fix, and worked: installed or fixed mid-session.""" | ||
| for r in runs: | ||
| # a run after an `export` may only work because it inherited the fix | ||
| if r.i > a.i and r.ok and (not same_place or _same_place(a, r)) and not any(x.sets_env for x in runs if a.i < x.i <= r.i): |
There was a problem hiding this comment.
P2 Exclude same-run environment changes from unchanged successes
For Codex calls, a failing node build.js, a successful API_BASE=x node build.js, and then export API_BASE=x && node build.js produce no repair. Codex runs have sets_env=False because their shells do not persist, so the last run incorrectly counts as an unchanged success despite setting the required variable itself. Check environment changes within the candidate run independently of whether they persist across calls.
Requested by Vu · project thread
Adds
kypp/repairs.py, a deterministic miner borrowed from TokenEconomizer's environment facts. It finds a shell command that fails with command-not-found, a missing module, or a missing env var, followed later in the same session by a form that works. It writes the pair as one pitfall, for example "pytest -q …fails here (command not found);uv run pytest …works."`pytest` is not on PATH,`python` can't import `turso`,`node` needs `API_BASE` set). A second session hits the same subject, and the arbiter's (subject, scope, project) corroboration accepts it. A one-session quirk stays a candidate. This is how it answers the LLM prompt's "skip transient environment artifacts" rule without changing that prompt.Bashrepeatedly failed" pitfall for the failures it explains. Unexplained failures keep the generic one.WithRepairsadds the repairs alongside the LLM's drafts and skips any subject the LLM already produced.which,pip install,--version), or when the working form is a host path. A secret-looking env var's value is never written. Codex runs (no status) are read by exit code, and a piped run (| tail) by its error text.Verification:
python -m unittest discover -s testspasses (255 tests, 16 new intests/test_repairs.py).featuremap.py checkreports 0 broken pointers. Thepython -m kypp.distillself-test passes. Write-up:/mnt/project-files/research/tokeneconomizer-review.md.Generated by Claude Code