Skip to content

refactor(logs): nest hook output by branch/source/hook-type/name#2041

Merged
max-sixty merged 6 commits intomainfrom
logs
Apr 9, 2026
Merged

refactor(logs): nest hook output by branch/source/hook-type/name#2041
max-sixty merged 6 commits intomainfrom
logs

Conversation

@max-sixty
Copy link
Copy Markdown
Owner

Flattens categorization of .git/wt/logs/ by making the filesystem encode what the old scheme crammed into filenames.

Layout: top-level files are shared logs (commands.jsonl*, verbose.log, diagnostic.md); top-level directories are per-branch log trees — {branch}/{source}/{hook-type}/{name}.log for hook output, {branch}/internal/remove.log for background removal, wt/internal/trash-sweep.log for the trash sweeper. Categorization becomes a trivial file-vs-directory check, eliminating the exclusion rule ends_with(".log") && !is_diagnostic_file(name).

Wins: per-branch listing/clearing is now O(that branch) instead of O(all logs); orphan cleanup for a removed branch is a single remove_dir_all; filenames drop the joined-tuple collision hashes they only needed to disambiguate flat keys.

Transition: clear_logs keeps a self-healing sweep of legacy top-level .log files so users transition without an explicit migration. A pinning test (test_state_clear_logs_sweeps_legacy_flat_files) guards that behavior.

Observable change: logs get --format=json now puts relative paths in the file field (e.g. main/user/post-start/server.log). Log locations are listed as "flexible" in CLAUDE.md, so this is in scope.

Reviewer orientation:

  • src/commands/process.rsHookLog::path() rewritten; suffix() / filename() deleted.
  • src/commands/config/state.rs — new walk_hook_output_files / walk_branch_dir / HookOutputEntry; clear_logs handles legacy sweep; partition_log_files_json + render_* split along the top-level vs hook-output seam; module docstring pins the invariant.
  • src/testing/mod.rswait_for_file_count walks recursively.
  • Test fixtures in tests/integration_tests/config_state.rs use new hook_log_rel_path / internal_log_rel_path / write_log_at helpers.

🤖 Generated with Claude Code

max-sixty and others added 2 commits April 9, 2026 15:33
Flatten categorization of `.git/wt/logs/` by making the filesystem encode
what the old scheme crammed into filenames. Top-level files are shared logs
(commands.jsonl, verbose.log, diagnostic.md); top-level directories are
per-branch log trees (`{branch}/{source}/{hook-type}/{name}.log` for hooks,
`{branch}/internal/remove.log` for background removal). Categorization
becomes a trivial file-vs-directory check, eliminating the exclusion rule
`ends_with(".log") && !is_diagnostic_file(name)`.

Per-branch listing and clearing are now O(that branch) instead of O(all
logs), orphan cleanup for a removed branch is a `remove_dir_all`, and
filenames drop the joined-tuple collision hashes they only needed to
disambiguate flat keys.

`clear_logs` keeps a self-healing sweep of legacy top-level `.log` files so
users transition without explicit migration. The JSON output of
`logs get --format=json` now contains relative paths (e.g.
`main/user/post-start/server.log`) in the `file` field — log locations are
listed as "flexible" in CLAUDE.md, so this observable change is in scope.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
# Conflicts:
#	docs/content/remove.md
#	skills/worktrunk/reference/remove.md
#	src/cli/mod.rs
#	src/commands/process.rs
#	tests/snapshots/integration__integration_tests__help__help_remove_long.snap
Copy link
Copy Markdown
Collaborator

@worktrunk-bot worktrunk-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two issues found:

Garbled doc comment on log_path_for_debug — The new function was inserted between the tail of create_detach_log's doc comment and create_detach_log itself, so log_path_for_debug absorbs text that belongs to create_detach_log ("Create the log directory and file...", "Returns (log_path, log_file). Shared by..."). create_detach_log ends up with no doc comment. Fix: move log_path_for_debug and its doc block to after create_detach_log.

Trash-sweep log path wrong in docsdocs/content/remove.md, src/cli/mod.rs, and skills/worktrunk/reference/remove.md all say .git/wt/logs/wt/internal/trash-sweep.log, but sanitize_for_filename("wt") produces wt-boj (confirmed by the snapshot in test_hook_log_path), so the actual path is .git/wt/logs/wt-boj/internal/trash-sweep.log. See inline suggestion on docs/content/remove.md.

worktrunk-bot and others added 3 commits April 9, 2026 23:17
`sanitize_for_filename("wt")` produces `wt-boj` (confirmed by snapshot
test), so the sweep log is at `.git/wt/logs/wt-boj/internal/trash-sweep.log`,
not `wt/internal/trash-sweep.log`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two issues flagged by worktrunk-bot on #2041:

1. Garbled doc comment: `log_path_for_debug` was inserted between
   `spawn_detached`'s doc block and `create_detach_log`'s doc block, so
   all three doc comments coalesced onto the helper and both original
   functions ended up undocumented. Delete `log_path_for_debug`
   (which only added context to a `log::debug!` call, and was uncovered
   anyway — coverage gaps in log-macro args are not worth chasing per
   `tests/CLAUDE.md`), revert the debug lines to match main, and move
   `spawn_detached`'s doc comment to its proper position right above the
   function. `create_detach_log` also gets its doc block back, fixing a
   pre-existing latent bug where the two doc blocks had no blank line
   between them and both attached to `create_detach_log`.

2. Wrong trash-sweep path in docs: `sanitize_for_filename("wt")` returns
   `wt-boj`, so the sweep log lives at
   `.git/wt/logs/wt-boj/internal/trash-sweep.log`, not `.../wt/...`.
   Fixed in `src/cli/mod.rs`, `docs/content/remove.md`, and
   `skills/worktrunk/reference/remove.md`.

Also bump two test fixtures to lift coverage on the nested-layout paths:
`test_state_get_logs_with_files` now writes a file ≥ 1024 bytes to
exercise the `{}K` size-formatting branch, and `test_logs_get_json_with_files`
includes a diagnostic file so the JSON partition covers the diagnostic
push.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolved the rebase conflict on the trash-sweep-path fix by merging the
bot's wording ("`sanitize_for_filename`") with mine ("pseudo-branch") —
which required regenerating the `wt remove --help` snapshot since it
pins the `src/cli/mod.rs` text verbatim.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
`log_path.display()` emits backslashes on Windows, so the test assertion
`stdout.contains("branch/user/hook-type/name.log")` always fails there.
Switch to `to_slash_lossy()` (already imported) so the absolute path
uses forward slashes on all platforms, matching the forward-slash relative
paths produced by `logs list`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@worktrunk-bot
Copy link
Copy Markdown
Collaborator

The previous CI run (d6456bc) had two failures I investigated:

  1. test_help::case_11 (snapshot help_remove_long) — already resolved by your commit f96b126 (sync after wording merge).

  2. Four Windows test failures in test_state_logs_get_hook_* — these are still present. The root cause is println\!("{}", log_path.display()) in handle_logs_get (state.rs:483): on Windows display() emits backslashes, so stdout.contains("branch/user/hook-type/name.log") always fails. path_slash::PathExt was already imported; I switched to to_slash_lossy() in c30d804.

@max-sixty max-sixty merged commit f8f291b into main Apr 9, 2026
25 checks passed
@max-sixty max-sixty deleted the logs branch April 9, 2026 23:47
max-sixty pushed a commit to max-sixty/tend that referenced this pull request Apr 10, 2026
## Summary

- Add a **"Non-issues: do not flag these"** section to
`review-reviewers/SKILL.md` so the hourly judgment doesn't keep
re-discovering the same non-problem.
- Document in `review/SKILL.md` that commit and PR authorship don't
affect review behavior — reviewing/re-approving after a bot-pushed fix
is expected.

## Evidence — two prior rejections of the same fix

`review-reviewers` has now made two attempts to guard `tend-review` from
re-approving on bot-pushed fix commits. Both were closed by the
maintainer as solving a non-problem.

| PR | Title | Closed | Maintainer feedback |
|---|---|---|---|
| [#154](#154) | `fix(review):
skip re-review when bot pushes to already-approved PR` | 2026-04-07 |
"but presumably this does require a new review, no? why is this case
special?" |
| [#212](#212) | `fix(review):
skip APPROVE when incremental commits are bot-authored` | 2026-04-10
01:07Z | "this is totally fine — we should *not* treat a bot PR
differently from any other PR. We should make that clear in our docs" |

The second rejection explicitly asked for the principle to be
documented. This PR does that.

## Root cause

Run
[24219300860](https://github.com/max-sixty/tend/actions/runs/24219300860)
(prior `review-reviewers` run at 23:59Z) observed a `tend-review` run on
max-sixty/worktrunk#2041 submitting a fourth APPROVED review on a
bot-authored commit pushed by `tend-notifications`, classified it as
"Structural — recurs whenever the bot pushes to a PR it already
reviewed," and filed #212. The judgment was wrong: commit authorship is
not a review-logic problem. Run
[24220122739](https://github.com/max-sixty/tend/actions/runs/24220122739)
(`tend-review` on #212) then correctly executed the new logic the PR
added, but the PR's premise was rejected by the maintainer minutes later
in run
[24221123712](https://github.com/max-sixty/tend/actions/runs/24221123712).

The underlying 5-stacked-approvals incident that originally motivated
#154 ([PRQL/prql#5774](PRQL/prql#5774)) was a
*concurrency* issue — cancelled runs racing to POST approvals before
SIGTERM arrived — not a review-logic issue. This is called out in the
new "Non-issues" entry so it isn't confused for the same pattern in
future.

## Gate assessment

| Gate | Result |
|---|---|
| **Evidence level** | Critical — two closed PRs with explicit
maintainer rejection, plus an explicit "make that clear in our docs"
request |
| **Occurrences** | 3 independent instances (PR #154 authoring run, PR
#212 authoring run 24219300860, and each closure) |
| **Classification** | **Structural** — the skill has no guidance
distinguishing expected re-approval from a loop; it will make the same
judgment again without a fix |
| **Change type** | **Targeted fix** — 21 lines total across two files,
no restructuring |
| **Passes Gate 1 (confidence)** | Yes — Critical bar is 1 occurrence;
this has 2 |
| **Passes Gate 2 (magnitude)** | Yes — targeted fix at normal threshold
|

## Test plan

- [ ] Next time a `tend-review` run re-approves after
`tend-notifications` or `tend-ci-fix` pushes a fix, the subsequent
`review-reviewers` run should not flag it as a behavioral problem.
- [ ] The `review` skill continues reviewing bot-pushed fixes normally
(no behavior change — only documentation added).

Co-authored-by: continuous-bot <269947486+continuous-bot@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants