-
Notifications
You must be signed in to change notification settings - Fork 37
better handling of mid agent session commits #193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Entire-Checkpoint: 02d49af5bfe0
c1f3267 to
f07c305
Compare
Note on TTY behaviorThe
The main mid-session commit fix landed in PR #184 (path normalization, BaseCommit staleness, PendingCheckpointID lifecycle). This PR's fast path is a safety net for headless edge cases, not the primary detection mechanism. The test infrastructure improvements ( |
Correction: TTY behavior is correctDisregard my previous comment — I verified empirically and the assumption is correct. Claude Code spawns Bash subprocesses with pipes (to capture stdout/stderr), not a PTY. This means child processes don't have a controlling terminal: So the actual flow is: The fast path correctly triggers for agent-invoked commits and is skipped for human commits at the terminal. LGTM. |
Claude Code's git commit -m during a session produced commits without Entire-Checkpoint trailers, so session metadata was never linked to commits.
Two problems in the prepare-commit-msg hook's -m flag path:
Tests missed this because GitCommitWithShadowHooks didn't pass source="message", always taking the editor path which unconditionally adds trailers.
Fix: Use TTY availability to distinguish agent from human. Agents don't have a controlling terminal — hasTTY() checks /dev/tty. When !hasTTY() and a session is ACTIVE, a fast path adds the trailer directly, bypassing content detection and TTY prompts. The human flow is unchanged.
Tests now pass source="message" and use ENTIRE_TEST_TTY=1 (human) or ENTIRE_TEST_TTY=0 (agent) to control the TTY check deterministically.
Note
Medium Risk
Changes git hook behavior for a specific non-interactive execution path; risk is mainly misclassifying environments as no-TTY and adding trailers more broadly than intended, affecting commit metadata linkage.
Overview
Fixes missing
Entire-Checkpointtrailers for mid-session agentgit commit -mby detecting lack of a controlling TTY and taking a fast path.manual_commit’sprepare-commit-msghook now, when!hasTTY()and any session is ACTIVE, bypasses transcript/content detection and interactive confirmation and directly writes a generated (or pending) checkpoint trailer viaaddTrailerForAgentCommit.Integration tests were updated to better mirror real
git commit -m(source="message"), add deterministic TTY simulation viaENTIRE_TEST_TTY, and include a new test asserting agent commits always receive the trailer.Written by Cursor Bugbot for commit 2645133. This will update automatically on new commits. Configure here.