feat(run): bucket dirty paths; recommend gitignore vs commit per kind#111
Merged
Conversation
Today's dirty-tree refusal recommends `git add` + `git commit` for
*every* dirty path. That's wrong for the common case where the dirty
path is an output of an earlier roar run (e.g. `model.pkl`) — the
right fix is `.gitignore`, not commit.
Splits the dirty paths into three buckets and tailors the recommendation
per bucket:
(a) code — tracked-modified files: `git add` + `git commit` (today).
(b) prior-roar outputs — untracked paths that match an artifact
in the local roar DB by `first_seen_path`: recommend
`.gitignore` (with a `*.<ext>` pattern when ≥3 files share an
extension), fall back option to commit them.
(d) unknown untracked — no DB match: offer both `.gitignore` and
commit. User decides.
Mixed cases get segmented blocks (Code / Roar outputs / Other
untracked), each with its own fix line, then a single retry footer.
Deliberately *not* hashing untracked files to look for content matches
in the DB. The fix recommendation is the same for path-match vs.
hash-match (both → gitignore), so the hash cost isn't worth it.
End-of-run prevention. After a successful run, emits a one-line
`warning:` if any untracked paths now sit in the tree (which would
block the next `roar run`):
warning: 3 outputs make this repo dirty and will block the next
`roar run`. Add to .gitignore or commit.
The warning is one self-contained line so it stands on its own when
`hints.enabled = false`. Follow-up `hint:` lines suggest the actual
`echo … >> .gitignore` commands (with the same `*.<ext>` pattern
collapsing) and are silenced by the usual gate (`hints.enabled`,
quiet verbosity).
gitignore_lines() — shared helper that builds the suggestion block:
groups by extension, emits a `*.<ext>` pattern with `(covers N of M)`
when ≥3 paths share an extension, and caps stragglers at 8 with
`# and N more` so output stays bounded for large produce-sets.
What changed where
* `dirty_tree_classify.py` (new) — `classify_dirty_paths(...)` plus
the porcelain parser (moved from dirty_tree_error.py).
* `gitignore_suggest.py` (new) — `gitignore_lines(paths)` helper.
* `dirty_tree_error.py` — dispatcher + per-bucket message variants.
The `.roar/`-only and `$HOME` special cases preserved as-is.
* `execution.py` — `validate_git_clean(...)` gains optional
`roar_dir`; opens a read-only DB context for the classifier.
`execute_and_report` calls `emit_dirty_outputs_warning` after
`next_steps_hint`.
* `output_followup.py` (new) — `emit_dirty_outputs_warning(...)`.
* `service.py` — passes `request.roar_dir` to `validate_git_clean`.
Tests (1204 unit pass, 1 pre-existing skipped):
* `test_dirty_tree_error.py` rewritten for bucketed message shape
(code-only / roar_outputs-only / unknown-only / mixed) plus
classifier + parser tests.
* `test_gitignore_suggest.py` — threshold logic, cap, dotfile no-op,
multiple-pattern groups.
* `test_output_followup.py` — silent paths (clean tree, quiet,
hints-off, no git), warning shape (single / multi / pattern),
tracked-modified ignored.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The first cut wrapped the dirty-tree-refusal `raise ValueError(...)`
inside a `@contextmanager` whose try/except was too broad:
@contextmanager
def _maybe_artifact_lookup(roar_dir):
...
try:
with create_query_database_context(...) as db_ctx:
yield db_ctx.artifacts # <-- ValueError thrown back here
except Exception:
yield None # <-- second yield → RuntimeError
When the caller raised ValueError after the yield, Python threw it back
into the generator, the except caught it, and the generator tried to
yield a second time. That's illegal for a contextmanager, so the user
saw:
RuntimeError: generator didn't stop after throw()
…instead of the intended bucketed dirty-tree message. Reported via a
plain `touch test.txt && roar run echo` in a clean repo.
Fix: replace the `@contextmanager` with an `ExitStack` and a small
`_open_artifact_lookup` helper. The DB context is registered with the
stack for cleanup; the message is built inside the `with` block; the
`raise ValueError(message)` happens outside any context manager, so
nothing can intercept it.
Regression test added in `tests/unit/test_dirty_tree_error.py` —
stubs git's `rev-parse` + `status` to fake a dirty tree, sets
`roar_dir` so the DB-context branch fires, asserts a plain
`ValueError` (not `RuntimeError`) surfaces. Catches this exact bug.
1205 unit + execution + application tests passing.
`git status --porcelain` emits a two-column status code where the X
column (index status) is a literal space for worktree-only
modifications: ` M train.py`. validate_git_clean was calling .strip()
on the whole multi-line output, which ate the leading space and
shifted the parser by one character. Result:
$ git status
Modified: train.py
$ roar run echo
...
To proceed:
git add rain.py ← :(
...
Drop the `.strip()` on the captured output; keep the truthiness check
by `.strip()`-ing only inside the `if`. The parser already handles the
trailing newline via `splitlines()`.
Regression test stubs subprocess to return the exact porcelain shape
(` M train.py\n`) and asserts the action line says `git add train.py`,
not `git add rain.py`. Catches this exact bug.
Bug was latent in the original code too — the path-truncation always
existed for worktree-only modifications. Less visible there because
that flow only ever showed the bad path in the action line; users
probably blamed git for "remembering" wrong filenames.
1206 unit + execution + application tests passing.
The end-of-run `warning:` line was using `warn_amber` (ANSI-256 #172) — the same color as the `hint:` lines below it. A reader can't tell the "this is the situation" line from the "these are suggested fixes" lines at a glance, which dilutes the warning. Adds a `warn_yellow` semantic token (ANSI-256 #226, pure yellow) for warnings, distinct from amber (advisory hints) and red (errors). The end-of-run warning line gets `warn_yellow`; hint lines stay amber. Aligns with the existing `Warning:` bright-yellow convention in `presenters/console.py:48`. Test asserts the two streams carry different ANSI codes when color is on, so a future regression that collapses them back to one color fails fast. 1207 unit + execution + application tests passing.
| # generator with a too-broad `try/except`; the re-thrown | ||
| # ValueError was caught and the generator tried to yield a | ||
| # second time, surfacing as `generator didn't stop after throw()`. | ||
| with ExitStack() as stack: |
Member
There was a problem hiding this comment.
This is neat; I didn't know about ExitStack
TrevorBasinger
approved these changes
May 19, 2026
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
3 participants
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.
Summary
Today's dirty-tree refusal recommends `git add` + `git commit` for every dirty path. That's wrong for the modal case where the dirty file is an output of an earlier `roar run` — the right fix is `.gitignore`. This PR teaches the refusal to discriminate.
Buckets
Mixed dirty trees render segmented blocks (Code changes / Roar outputs / Other untracked), each with its own fix line, then a single retry footer at the bottom.
The `.roar/`-only and `$HOME` special cases are preserved.
No hashing
Deliberately not hashing untracked files to look for content matches in the DB — the fix recommendation is identical whether the match is by-path or by-hash, so the cost isn't justified.
End-of-run prevention
After a successful run, a one-line warning fires if any untracked paths now sit in the tree (which would block the next `roar run`):
```
warning: 3 outputs make this repo dirty and will block the next `roar run`. Add to .gitignore or commit.
hint: echo '*.pkl' >> .gitignore (covers all 3)
hint: git add .gitignore && git commit -m 'ignore roar outputs'
```
One self-contained `warning:` line so users with `hints.enabled = false` still see what's wrong and what to do. The follow-up `hint:` block is gated the usual way.
Pattern suggestions
`gitignore_lines()` groups paths by extension; ≥3 same-extension paths collapse to a single `echo '.' >> .gitignore (covers N of M)` line. Stragglers get individual literal lines, capped at 8 with `# and N more` past that. Extensionless and dotfile paths only get literal suggestions (no `.env` over-reach).
Test plan
Independence
Not stacked on the open hints PRs (#109, #110). The end-of-run warning uses the same hint gating those PRs were polishing, so it inherits whatever stream/TTY semantics land first.
🤖 Generated with Claude Code