Skip to content

fix(auto-commit): skip detached HEAD commits - #1754

Merged
akramcodez merged 2 commits into
Nano-Collective:mainfrom
ducanhnguyen223:codex/issue-1747-detached-auto-commit
Oct 20, 2026
Merged

akramcodez merged 2 commits into
Nano-Collective:mainfrom
ducanhnguyen223:codex/issue-1747-detached-auto-commit

Conversation

@ducanhnguyen223

@ducanhnguyen223 ducanhnguyen223 commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Description

Closes #1747.

With nanocoder.autoCommit on, a commit made while HEAD is detached belongs to no branch and is easy to lose at the next checkout, and the user was not told. Auto-commits also carried no marker, so they could not be found later for squashing or review.

  • Skip auto-commit when HEAD is detached or cannot be verified (git symbolic-ref --quiet HEAD), before the index is touched, and show one warning per repository.
  • Add a Nanocoder-Auto-Commit: true trailer to agent-generated commits, so git log --grep Nanocoder-Auto-Commit finds them.
  • Add regressions for: detached HEAD preserving staged and untracked changes, the one-time warning, the trailer format, and finding the marker with git log --grep.

Description reformatted to the PR template by a maintainer; the original verification notes are kept below.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

All 17 hosted checks pass on the current head. pnpm test:all is not ticked because the contributor's local run (macOS) stopped in AVA on failures unrelated to this change (see below); CI is the source of truth for the full suite.

Contributor's verification at head d09e729

  • Focused auto-commit tests: 24 passed.
  • TypeScript type checks: passed.
  • Changeset status against main: patch bump detected for @nanocollective/nanocoder.
  • git diff --check: passed.
  • pnpm run test:all: format, lint and TypeScript stages passed (Biome printed a schema-version notice and an internal UTF-8 I/O diagnostic). AVA ended with 51 failed and 15 skipped tests, including CLI integration failures because the test harness mock for ink has no Text export, and macOS /var versus /private/var path assertions. The script stopped at AVA, so Knip and the security stages did not run.

Maintainer re-test (this branch merged into current main, Linux)

  • auto-commit.spec.ts: 24 passed; tsc --noEmit and Biome clean.
  • The two new/changed tests fail against unfixed main and pass with this change.
  • Real-git scenarios, main vs this PR: on a detached HEAD, main makes the commit (HEAD moves, no branch contains it) and this PR skips it with one warning. On a branch, the commit carries the trailer and git log --grep / git interpret-trailers --parse find it. A multi-paragraph model message keeps the trailer as its own final paragraph. Re-attaching to a branch resumes committing. A new repo with no commits is unaffected. A linked worktree on a branch commits; one created with --detach is skipped. A commit-msg hook that checks the subject line still passes.

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Not applicable: the change only touches the git steps of auto-commit and does not depend on the provider or MCP.

Checklist

  • If this was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md)

Open item: docs/configuration/index.md ("Auto-commit") still lists the skipped cases without detached HEAD and does not mention the new trailer. The warning reuses the existing logWarning message-queue path, the same one as the "merge/rebase in progress" warning.

@github-actions

Copy link
Copy Markdown
Contributor

nc-review: comments — 1 important, 2 nits

@ducanhnguyen223 — a few things worth a look, none blocking.

The PR resolves both halves of #1747: it skips auto-commit when HEAD is detached (via git symbolic-ref --quiet HEAD) before the git add, and it appends a Nanocoder-Auto-Commit: true trailer to every auto-commit message. Both new behaviors are exercised by focused regression tests that simulate detached HEAD with staged and untracked changes. The change is correct, narrowly scoped, and small. It is missing the changeset the project's release flow expects for a user-facing patch.

🟠 important · changeset · source/services/auto-commit.ts

The PR is a user-facing bug fix (a new warning, a skipped-commit path, and a mandatory commit-trailer) but no .changeset/*.md file was added. The repo's release flow relies on changesets to roll patches into the changelog; the existing auto-commit entries (auto-commit-edits.md, auto-commit-literal-pathspecs.md, auto-commit-failure-rollback.md) show the convention. A maintainer will have to write one themselves; the contributor should add a patch-bump changeset under the heading it implies (warning about detached HEAD + new Nanocoder-Auto-Commit trailer, closing #1747).

⚪ nit · tests · source/services/auto-commit.spec.ts:113

The test covers detached HEAD and the one-time warning, but does not cover the diff branch added to commitFile for the trailer itself. The trailer assertion added to 'commits the edited file with the generated message' only checks %B equals 'feat: add a constant\n\nNanocoder-Auto-Commit: true', which still passes if the trailer were accidentally attached to the first commit only or if a stray newline were missing. A small extra check (git -C dir log --grep 'Nanocoder-Auto-Commit' --format=%H finding the new commit) would prove git log --grep works, which is the discovery path the issue calls out. Minor — not load-bearing.

⚪ nit · design · source/services/auto-commit.ts:135

warnedDetachedHeadRoots is module-scope mutable state keyed by root. Once a repo has been seen detached in this process, the warning will not reappear even if the user attaches and detaches again. That is probably acceptable (the user has already been warned) but the test only validates the in-process single-detached case, and a contributor reusing this Set as a "I have warned" ledger elsewhere would get an unintended suppression. Worth a one-line comment note that the Set is intentionally not cleared on checkout; it has the comment but only as a one-liner above the Set itself, not at the call site.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:comments nc-review left non-blocking findings label Oct 13, 2026

@akramcodez akramcodez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ducanhnguyen223, I reviewed and ran it against real git: detached HEAD is now skipped with one warning, and commits on a branch carry a Nanocoder-Auto-Commit trailer that git log --grep finds. I also tidied the PR body to the template. One follow-up: docs/configuration/index.md should mention the new skip case and trailer. LGTM!

@akramcodez
akramcodez merged commit 2e15c91 into Nano-Collective:main Oct 20, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:comments nc-review left non-blocking findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Auto-commit on a detached HEAD gives no warning, and agent commits carry no marker

2 participants