svc_update / forge merge auto-commits dirty repos and pushes via Claude — must not #181

New issue

Closed

opened 2026-04-30 12:27:29 +00:00 by mahmoud · 2 comments

mahmoud commented

2026-04-30 12:27:29 +00:00

Owner

Problem

forge_merge_path (in tools/modules/forge.nu:783-839) silently rewrites local git history as a side effect of service_* install --update. On any dirty repo it:

Auto-commits everything — git add -A followed by a Claude-generated commit message via the a command (forge.nu:807-818). As root it falls back to a plain chore: auto-commit local changes before pull commit.
AI-resolves merge conflicts — a 1 -i ... spawns Claude to read conflicted files and write git add / git commit --no-edit itself (forge.nu:826-832).
Force-pushes to origin — git push -u origin <branch> always fires after the pull (forge.nu:834-836).

The behaviour is documented in services/lib.nu:161-174 as intentional ("Thin wrapper around forge merge — which fetches, commits any local changes, pulls, resolves conflicts (via AI if needed) and pushes when ahead"). It is the wrong design for a tool people run during active development.

Trigger chain

Fires whenever --update reaches svc_update:

service_<x> start --update         (or service_<x> install --update, or service_complete --update)
  -> svc_install                   tools/modules/services/lib.nu:343
  -> if $update { svc_update }     line 353
  -> forge merge                   services/lib.nu:168-174
  -> forge_merge_path              forge.nu:783-839

--update is the only service_* flag that hits this. Without --update the install path goes directly to svc_cargo_install and never touches git state. forge merge standalone, forge merge_all, and skills.nu (which calls forge merge hero_skills) hit the same code.

Real damage

While working on hero_codescalers issue (lhumina_code/hero_codescalers#19), a single service_codescalers start --root --update did the following without consent:

Picked up an unstaged in-progress edit to crates/hero_codescalers_server/src/jobs.rs (~67 lines).
Ran git add -A, wrote commit bb38fe0 titled feat(jobs): add stats tracking and fix list pagination with a Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> trailer.
Pushed it to origin/development.

Ctrl+C on the running command did not help — the commit was already written before the pull began.

Required behaviour

On --update for each service repo:

Fetch origin.
If repo is dirty (any uncommitted change or untracked non-gitignored file): print a warning, skip the pull, continue building with current source. Tell the user to stash or commit and re-run.
If repo is clean and only behind origin/<current-branch>: fast-forward pull, no merge commit.
If repo is clean and only ahead: print info, do not push, continue building.
If repo is clean and diverged (ahead and behind): print warning, skip the pull, continue building, tell the user to rebase or merge manually.
One repo's warning never aborts the whole --update. End-of-run summary: synced N, skipped (dirty) M, skipped (diverged) K.
Never run: git add, git commit, git push, git rebase, claude / a 0 / a 1, or any other command that mutates history or commits on the user's behalf.

Proposed contract is detailed in the next comment for review.

## Problem `forge_merge_path` (in `tools/modules/forge.nu:783-839`) silently rewrites local git history as a side effect of `service_* install --update`. On any dirty repo it: 1. **Auto-commits everything** — `git add -A` followed by a Claude-generated commit message via the `a` command (`forge.nu:807-818`). As root it falls back to a plain `chore: auto-commit local changes before pull` commit. 2. **AI-resolves merge conflicts** — `a 1 -i ...` spawns Claude to read conflicted files and write `git add` / `git commit --no-edit` itself (`forge.nu:826-832`). 3. **Force-pushes to origin** — `git push -u origin <branch>` always fires after the pull (`forge.nu:834-836`). The behaviour is documented in `services/lib.nu:161-174` as intentional ("Thin wrapper around `forge merge` — which fetches, commits any local changes, pulls, resolves conflicts (via AI if needed) and pushes when ahead"). It is the wrong design for a tool people run during active development. ## Trigger chain Fires whenever `--update` reaches `svc_update`: ``` service_<x> start --update (or service_<x> install --update, or service_complete --update) -> svc_install tools/modules/services/lib.nu:343 -> if $update { svc_update } line 353 -> forge merge services/lib.nu:168-174 -> forge_merge_path forge.nu:783-839 ``` `--update` is the only `service_*` flag that hits this. Without `--update` the install path goes directly to `svc_cargo_install` and never touches git state. `forge merge` standalone, `forge merge_all`, and `skills.nu` (which calls `forge merge hero_skills`) hit the same code. ## Real damage While working on `hero_codescalers` issue (https://forge.ourworld.tf/lhumina_code/hero_codescalers/issues/19), a single `service_codescalers start --root --update` did the following without consent: - Picked up an unstaged in-progress edit to `crates/hero_codescalers_server/src/jobs.rs` (~67 lines). - Ran `git add -A`, wrote commit `bb38fe0` titled `feat(jobs): add stats tracking and fix list pagination` with a `Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>` trailer. - Pushed it to `origin/development`. Ctrl+C on the running command did not help — the commit was already written before the pull began. ## Required behaviour On `--update` for each service repo: - Fetch `origin`. - If repo is dirty (any uncommitted change or untracked non-gitignored file): print a warning, skip the pull, continue building with current source. Tell the user to stash or commit and re-run. - If repo is clean and only behind `origin/<current-branch>`: fast-forward pull, no merge commit. - If repo is clean and only ahead: print info, do not push, continue building. - If repo is clean and diverged (ahead and behind): print warning, skip the pull, continue building, tell the user to rebase or merge manually. - One repo's warning never aborts the whole `--update`. End-of-run summary: `synced N, skipped (dirty) M, skipped (diverged) K`. - **Never** run: `git add`, `git commit`, `git push`, `git rebase`, `claude` / `a 0` / `a 1`, or any other command that mutates history or commits on the user's behalf. Proposed contract is detailed in the next comment for review.

mahmoud commented

2026-04-30 12:28:02 +00:00

Author

Owner

Proposed approach

Replace forge_merge_path (tools/modules/forge.nu:783-839) with a read-only sync function. No git add, no git commit, no git push, no claude invocation, no git rebase. The function only ever runs git fetch and (when safe) git merge --ff-only.

Contract

For each service repo on --update:

1. fetch origin --prune
2. read git status --porcelain (excludes gitignored files by default)
3. read ahead/behind counts vs origin/<current-branch>
4. classify and act:
   dirty (any uncommitted change OR untracked non-gitignored file):
     -> print: "⚠ <repo> on <branch> is dirty — skipping pull.
               Stash or commit and re-run --update to refresh.
               Continuing build with current source."
     -> continue to build

   clean AND behind only:
     -> git merge --ff-only origin/<branch>
     -> continue to build

   clean AND ahead only:
     -> print: "<repo> ahead by N — nothing to pull. Push manually if desired."
     -> continue to build

   clean AND diverged (ahead AND behind):
     -> print: "⚠ <repo> diverged on <branch> — skipping pull.
               Rebase or merge manually and re-run --update."
     -> continue to build

   any --ff-only failure:
     -> print warning with git error, continue to build
5. NEVER: git commit, git add, git push, git rebase, claude / a 0 / a 1, or anything that mutates history

A failure on one repo never aborts the whole --update. End-of-run summary line:

=== service_complete --update: synced N, skipped (dirty) M, skipped (diverged) K, skipped (other) J ===

Design decisions confirmed

Untracked non-gitignored files count as dirty. They could be the user's WIP. The accidental apikeys.db / request_logs.db situation that produced commit 103cc44 is exactly the failure mode this prevents — runtime data ends up in working tree, gets auto-staged, gets committed. The right escape hatch is .gitignore, not "silently ignore untracked".
Branch-agnostic policy. Same rules apply on whatever branch is checked out — development, a feature branch, a release branch. No branch is special-cased; no branch switching happens.
No new flag. Without --update, the build path already skips svc_update entirely (services/lib.nu:343 if $update { svc_update }), so users who don't want any git activity already have an opt-out: just don't pass --update.

Scope of the patch

Single file: tools/modules/forge.nu.

Replace the body of forge_merge_path (lines 783-839) with the read-only sync logic above.
Remove the a 0 -i and a 1 -i calls. Remove the git push line.
The exported forge merge command (forge.nu:941) keeps the same name; only its semantics change. forge merge_all benefits transparently because it loops over forge_merge_path.
svc_update (services/lib.nu:168-174) stays as is — its doc comment needs updating to remove the "commits any local changes / resolves conflicts via AI / pushes" wording.

Approx diff size: -50 / +35 lines, no new files, no new dependencies.

Out of scope for this issue

The standalone forge merge command's behaviour as called from skills.nu and elsewhere is changed by this same patch (intentionally — same fix everywhere).
A separate explicit-opt-in command for users who actually want "commit local changes and push" can be added later if needed; do not name it forge merge.

## Proposed approach Replace `forge_merge_path` (`tools/modules/forge.nu:783-839`) with a read-only sync function. No `git add`, no `git commit`, no `git push`, no `claude` invocation, no `git rebase`. The function only ever runs `git fetch` and (when safe) `git merge --ff-only`. ### Contract For each service repo on `--update`: ``` 1. fetch origin --prune 2. read git status --porcelain (excludes gitignored files by default) 3. read ahead/behind counts vs origin/<current-branch> 4. classify and act: dirty (any uncommitted change OR untracked non-gitignored file): -> print: "⚠ <repo> on <branch> is dirty — skipping pull. Stash or commit and re-run --update to refresh. Continuing build with current source." -> continue to build clean AND behind only: -> git merge --ff-only origin/<branch> -> continue to build clean AND ahead only: -> print: "<repo> ahead by N — nothing to pull. Push manually if desired." -> continue to build clean AND diverged (ahead AND behind): -> print: "⚠ <repo> diverged on <branch> — skipping pull. Rebase or merge manually and re-run --update." -> continue to build any --ff-only failure: -> print warning with git error, continue to build 5. NEVER: git commit, git add, git push, git rebase, claude / a 0 / a 1, or anything that mutates history ``` A failure on one repo never aborts the whole `--update`. End-of-run summary line: ``` === service_complete --update: synced N, skipped (dirty) M, skipped (diverged) K, skipped (other) J === ``` ### Design decisions confirmed 1. **Untracked non-gitignored files count as dirty.** They could be the user's WIP. The accidental `apikeys.db` / `request_logs.db` situation that produced commit `103cc44` is exactly the failure mode this prevents — runtime data ends up in working tree, gets auto-staged, gets committed. The right escape hatch is `.gitignore`, not "silently ignore untracked". 2. **Branch-agnostic policy.** Same rules apply on whatever branch is checked out — `development`, a feature branch, a release branch. No branch is special-cased; no branch switching happens. 3. **No new flag.** Without `--update`, the build path already skips `svc_update` entirely (`services/lib.nu:343` `if $update { svc_update }`), so users who don't want any git activity already have an opt-out: just don't pass `--update`. ### Scope of the patch Single file: `tools/modules/forge.nu`. - Replace the body of `forge_merge_path` (lines 783-839) with the read-only sync logic above. - Remove the `a 0 -i` and `a 1 -i` calls. Remove the `git push` line. - The exported `forge merge` command (`forge.nu:941`) keeps the same name; only its semantics change. `forge merge_all` benefits transparently because it loops over `forge_merge_path`. - `svc_update` (`services/lib.nu:168-174`) stays as is — its doc comment needs updating to remove the "commits any local changes / resolves conflicts via AI / pushes" wording. Approx diff size: -50 / +35 lines, no new files, no new dependencies. ### Out of scope for this issue - The standalone `forge merge` command's behaviour as called from `skills.nu` and elsewhere is changed by this same patch (intentionally — same fix everywhere). - A separate explicit-opt-in command for users who actually want "commit local changes and push" can be added later if needed; do not name it `forge merge`.