Skip to content

fix(catchup-after-startup): detect submodule/worktree checkouts + de-flake commits section - #1076

Merged
sonichi merged 3 commits into
mainfrom
fix/catchup-git-submodule-detection
May 23, 2026
Merged

sonichi merged 3 commits into
mainfrom
fix/catchup-git-submodule-detection

Conversation

@qingyun-wu

Copy link
Copy Markdown
Collaborator

Problem

In a submodule or worktree checkout, .git is a file (a gitdir: pointer) rather than a directory. catchup-after-startup.sh tested [ -d "$REPO/.git" ] in two places, so on such a checkout:

  • The repo auto-detect probe rejected an otherwise-valid checkout (even with CLAUDE.md + skills/ present and git fully functional).
  • The "Recent commits" section short-circuited to (no .git at $REPO) — even though git -C "$REPO" log works perfectly.

Once that gate is opened, a second, flaky bug surfaces in the same section: git log … | head -25 under set -o pipefail intermittently prints (git failed). When the window holds >25 commits, head closes the pipe after 25 lines, git receives SIGPIPE (exit 141), and pipefail propagates it as a pipeline failure.

Fix

  1. -d → -e at both the auto-detect probe and the commits-section gate. -e accepts a .git that is a file or a directory, so normal, submodule, and worktree checkouts all pass.
  2. Decouple head from git's pipe. Capture git log output inside the if (so git's real exit status is the only failure signal), then trim with head on a plain string where a SIGPIPE can't affect git.

Verification

On a submodule checkout (public sutando pinned inside a private super-repo, .git = gitdir: ../.git/modules/sutando):

  • Before: ## Recent commits → (no .git at $REPO).
  • After the -e fix: section entered git path but flaked — (git failed) on ~1 in 3 runs.
  • After both fixes: 8/8 runs populate the commits list reliably.
  • Auto-detect also now finds the repo when SUTANDO_REPO_DIR is unset and cwd is the submodule.

bash -n clean. No set -e in the script, so the decoupled printf … | head assignment is safe. Scope is limited to the two .git checks and the one pipeline; no behavior change for plain-directory checkouts.

Follow-up to #1056.

…flake commits section

The "Recent commits" section never populated in a submodule or worktree
checkout, where `.git` is a file (gitdir pointer) rather than a directory:

- The repo auto-detect probe and the commits-section gate both tested
  `-d "$REPO/.git"`, rejecting such checkouts even when git works fine.
  Use `-e` so a `.git` file or dir both pass.
- Once past the gate, `git log ... | head -25` under `set -o pipefail`
  intermittently reported "(git failed)": head closes the pipe after 25
  lines, SIGPIPEs git (exit 141), and pipefail surfaces it whenever the
  window holds >25 commits. Capture git's output first (its real exit
  governs the failure branch), then trim with head on a plain string.

Verified on a submodule checkout (sutando pinned inside a private
super-repo): the commits section now populates reliably (8/8 runs) and
auto-detect finds the repo when SUTANDO_REPO_DIR is unset.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Q: Does -d → -e have side effects? Does it break the normal (non-submodule) case?

No regression for normal checkouts — -e is a strict superset of -d here. -d path = "exists and is a directory"; -e path = "exists (any type)". Everything -d accepted, -e still accepts; it only adds the file-form .git.

Scenario .git is -d (old) -e (new) Change?
Normal checkout directory ✅ pass ✅ pass none
Submodule / worktree file (gitdir pointer) ❌ fail (the bug) ✅ pass the fix
No git repo absent ❌ fail ❌ fail none

In a normal checkout .git is a directory, so -d and -e return identical results — byte-for-byte same behavior. The change only accepts one new shape (the file-form .git), which is exactly the submodule/worktree case being fixed.

Theoretical false positive? If .git existed but were garbage (e.g. an empty file that isn't a valid gitdir pointer), -e would pass the gate where -d wouldn't — but both call sites degrade gracefully:

  • Commits section: a subsequent git -C "$REPO" log failure falls to (git failed), no crash.
  • Auto-detect probe (line 25): the .git test is ANDed with -f CLAUDE.md and -d skills, so a stray .git file alone can't trigger a false repo detection.

-e also matches git's own notion of "is this a repo" better than -d — git treats both the directory and the gitdir-pointer file as valid repos.

Verified across all three cases:

=== Case 1: normal checkout — .git is a DIRECTORY ===
  -d : PASS
  -e : PASS
  => identical (both PASS). No behavior change for normal repos.

=== Case 2: submodule/worktree — .git is a FILE (gitdir pointer) ===
  -d : FAIL  <- the bug (valid repo rejected)
  -e : PASS  <- the fix

=== Case 3: no .git at all ===
  -d : (correctly) FAIL
  -e : (correctly) FAIL
  => both reject. No false positive.

Net: safe relaxation, zero behavior change for standard non-submodule checkouts.

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM — two correct + well-traced fixes.

Submodule/worktree .git detection: -d → -e is the right invariant. Per git help repository-layout: in a submodule or git worktree add-style secondary checkout, the .git entry is a file containing a gitdir: <path> line, not a directory. Your guard now matches both shapes — and rejects nothing legitimate, since -e still requires the entry to exist.

De-flake commits section: classic git log | head -25 SIGPIPE-via-pipefail trap. head closes the read end after 25 lines, kernel sends SIGPIPE to git log, git exits 141, set -o pipefail propagates that as the pipeline status, and the if branch falsely reports "(git failed)" even when git did exactly the right thing. The capture-then-trim split is the cleanest fix — keeps real git failures distinguishable from "I asked for the first 25 of many" — and matches how scripts elsewhere in the repo handle the same pattern.

Tests + CLA green; per the no-bot-merge-authority rule leaving the merge press to Chi.

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM — both fixes are correct and well-scoped.

  1. -e not -d for .git — exactly right for submodule/worktree checkouts where .git is a gitdir-pointer file. Applied in both the REPO probe (line ~24) and the section-8 commits check (line ~258). Symmetric, no regression for normal checkouts.

  2. SIGPIPE-pipefail de-flake — capture-then-head pattern correctly isolates git's real exit status from the head-closes-pipe race. The inline comment explains the failure mode clearly. Bonus that the empty-window vs git-failed distinction now lives in the explicit if block rather than the __FAIL__ sentinel hack.

Nothing in my recent PRs (#1072 /startup, #1074 /task-orphan-check, #1075 revert) touches this file, so no merge conflict risk. Ship.

@sonichi
sonichi merged commit a83c4db into main May 23, 2026
2 checks passed
@sonichi
sonichi deleted the fix/catchup-git-submodule-detection branch May 23, 2026 22:05
bassilkhilo-ag2 added a commit to bassilkhilo-ag2/sutando that referenced this pull request May 27, 2026
…pient_label (sonichi#1078) and submodule/worktree detection (sonichi#1076)

sonichi#1078: recipient_label= in outbox_log.append, '#<channel>' format,
'<user> DM' format, getattr safe access, 'DM' fallback, None fallback
for missing channel name, 3 delivery paths all covered.

sonichi#1076: [ -e .git ] not [ -d .git ] in repo discovery AND commits
section, no bare -d .git checks remaining, submodule/worktree comment,
CATCHUP_HOURS + positional arg, git log capture-then-head SIGPIPE safety.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
…flake commits section (sonichi#1076)

The "Recent commits" section never populated in a submodule or worktree
checkout, where `.git` is a file (gitdir pointer) rather than a directory:

- The repo auto-detect probe and the commits-section gate both tested
  `-d "$REPO/.git"`, rejecting such checkouts even when git works fine.
  Use `-e` so a `.git` file or dir both pass.
- Once past the gate, `git log ... | head -25` under `set -o pipefail`
  intermittently reported "(git failed)": head closes the pipe after 25
  lines, SIGPIPEs git (exit 141), and pipefail surfaces it whenever the
  window holds >25 commits. Capture git's output first (its real exit
  governs the failure branch), then trim with head on a plain string.

Verified on a submodule checkout (sutando pinned inside a private
super-repo): the commits section now populates reliably (8/8 runs) and
auto-detect finds the repo when SUTANDO_REPO_DIR is unset.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bassilkhilo-ag2 added a commit to bassilkhilo-ag2/sutando that referenced this pull request Jun 8, 2026
…ask-metadata, and outbox labels

Covers:
- 22 tests for delivery-idempotency sentinels + send_allowlist (sonichi#1048/sonichi#1029)
- 19 tests for task-file channel/guild metadata + inline-tools workspace scan (sonichi#1077/sonichi#1081)
- 15 tests for outbox_log recipient_label (sonichi#1078) and submodule/worktree detection (sonichi#1076)

All 56 tests pass against current main.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bassilkhilo-ag2 added a commit to bassilkhilo-ag2/sutando that referenced this pull request Jun 8, 2026
…pient_label (sonichi#1078) and submodule/worktree detection (sonichi#1076)

sonichi#1078: recipient_label= in outbox_log.append, '#<channel>' format,
'<user> DM' format, getattr safe access, 'DM' fallback, None fallback
for missing channel name, 3 delivery paths all covered.

sonichi#1076: [ -e .git ] not [ -d .git ] in repo discovery AND commits
section, no bare -d .git checks remaining, submodule/worktree comment,
CATCHUP_HOURS + positional arg, git log capture-then-head SIGPIPE safety.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bassilkhilo-ag2 added a commit to bassilkhilo-ag2/sutando that referenced this pull request Jun 8, 2026
…ask-metadata, and outbox labels

Covers:
- 22 tests for delivery-idempotency sentinels + send_allowlist (sonichi#1048/sonichi#1029)
- 19 tests for task-file channel/guild metadata + inline-tools workspace scan (sonichi#1077/sonichi#1081)
- 15 tests for outbox_log recipient_label (sonichi#1078) and submodule/worktree detection (sonichi#1076)

All 56 tests pass against current main.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bassilkhilo-ag2 added a commit to bassilkhilo-ag2/sutando that referenced this pull request Jun 8, 2026
…pient_label (sonichi#1078) and submodule/worktree detection (sonichi#1076)

sonichi#1078: recipient_label= in outbox_log.append, '#<channel>' format,
'<user> DM' format, getattr safe access, 'DM' fallback, None fallback
for missing channel name, 3 delivery paths all covered.

sonichi#1076: [ -e .git ] not [ -d .git ] in repo discovery AND commits
section, no bare -d .git checks remaining, submodule/worktree comment,
CATCHUP_HOURS + positional arg, git log capture-then-head SIGPIPE safety.

Co-Authored-By: Claude Sonnet 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