fix(pull-base): arg-bearing /clear skips the pull; no-identity refusal blocks even a fast-forward #151

New issue

Closed

opened 2026-07-13 00:06:59 +02:00 by dominik.polakovics · 1 comment

dominik.polakovics commented

2026-07-13 00:06:59 +02:00

Owner

Two narrow gaps in the /pull-base lab command, both found while landing PR #150 (issue #149). Neither is a regression — each degrades to the pre-#149 behaviour — but both defeat the pull in cases the design intended to cover.

1. An arg-bearing clear silently skips the pull

isClearCommand matches the provider's role=clear entry by exact string equality:

// internal/httpapi/chat.go:372
if c.Role == provider.CommandRoleClear && cmd == "/"+c.Name {

But claude-code's clear entry carries an argument hint:

// internal/provider/claudecode/commands.go:42
{Name: "clear", ..., ArgHint: "[name]", Role: provider.CommandRoleClear, ChatSafe: true},

So /clear my-session fails the exact match, falls through to the ordinary reply path, and is forwarded to the provider without a base pull and without a notice. The operator gets a fresh context on a stale base — the precise failure #149 exists to prevent — with no indication it happened.

The bare /clear (the common case) works correctly. This matters more than it first looks because the autocomplete gesture model (#122) makes an arg-hinted command insert rather than send on click, which actively invites the operator to type a name.

Codex is unaffected: its clear-role /new carries no ArgHint.

Fix sketch: match on the command word rather than the whole string — split cmd at the first whitespace and compare the head to "/"+c.Name. The /pull-base interception above it already distinguishes bare from arg-bearing (exact match, then a "/pull-base " prefix → 400), so the two paths should agree on how a command word is recognised. Decide explicitly whether an arg-bearing clear pulls-then-forwards-verbatim (preserving the argument) — that is almost certainly the intent.

2. The no-identity refusal fires even for a fast-forward

// internal/pull/pull.go:182-188
if name == "" || email == "" {
    return Result{}, ErrNoAuthorIdentity
}

The refusal runs before any git does, so a repo with neither git_author_name/git_author_email nor the global settings pair can never /pull-base — even when the pull would trivially fast-forward, which authors no commit and needs no identity at all. The identity is only genuinely required on the diverged path, where a merge commit gets written.

The check was inherited from crmerge, where every merge does author a commit, so the up-front refusal is right there. Here it over-refuses.

Fix sketch: gitx.PullBase already computes ffable before merging. Either defer the identity check to the diverged branch, or keep the early check but let it pass when the pull turns out to be a fast-forward. Note the ordering constraint: the ff-ness is only known after the fetch, so the check has to move below it rather than being deleted.

Acceptance

/clear <arg> pulls the base before forwarding, and the argument survives the forward.
A repo with no configured git author identity can still /pull-base when the result is a fast-forward; the diverged case still refuses with ErrNoAuthorIdentity.
Regression tests for both, alongside the existing clear-hook and no-identity tests.

Two narrow gaps in the `/pull-base` lab command, both found while landing PR #150 (issue #149). Neither is a regression — each degrades to the pre-#149 behaviour — but both defeat the pull in cases the design intended to cover. ## 1. An arg-bearing clear silently skips the pull `isClearCommand` matches the provider's `role=clear` entry by **exact** string equality: ```go // internal/httpapi/chat.go:372 if c.Role == provider.CommandRoleClear && cmd == "/"+c.Name { ``` But claude-code's clear entry carries an argument hint: ```go // internal/provider/claudecode/commands.go:42 {Name: "clear", ..., ArgHint: "[name]", Role: provider.CommandRoleClear, ChatSafe: true}, ``` So `/clear my-session` fails the exact match, falls through to the ordinary reply path, and is forwarded to the provider **without a base pull and without a notice**. The operator gets a fresh context on a stale base — the precise failure #149 exists to prevent — with no indication it happened. The bare `/clear` (the common case) works correctly. This matters more than it first looks because the autocomplete gesture model (#122) makes an arg-hinted command *insert* rather than send on click, which actively invites the operator to type a name. Codex is unaffected: its clear-role `/new` carries no `ArgHint`. **Fix sketch:** match on the command word rather than the whole string — split `cmd` at the first whitespace and compare the head to `"/"+c.Name`. The `/pull-base` interception above it already distinguishes bare from arg-bearing (exact match, then a `"/pull-base "` prefix → 400), so the two paths should agree on how a command word is recognised. Decide explicitly whether an arg-bearing clear pulls-then-forwards-verbatim (preserving the argument) — that is almost certainly the intent. ## 2. The no-identity refusal fires even for a fast-forward ```go // internal/pull/pull.go:182-188 if name == "" || email == "" { return Result{}, ErrNoAuthorIdentity } ``` The refusal runs before any git does, so a repo with neither `git_author_name`/`git_author_email` nor the global settings pair can never `/pull-base` — **even when the pull would trivially fast-forward**, which authors no commit and needs no identity at all. The identity is only genuinely required on the diverged path, where a merge commit gets written. The check was inherited from crmerge, where every merge does author a commit, so the up-front refusal is right there. Here it over-refuses. **Fix sketch:** `gitx.PullBase` already computes `ffable` before merging. Either defer the identity check to the diverged branch, or keep the early check but let it pass when the pull turns out to be a fast-forward. Note the ordering constraint: the ff-ness is only known after the fetch, so the check has to move below it rather than being deleted. ## Acceptance - `/clear <arg>` pulls the base before forwarding, and the argument survives the forward. - A repo with no configured git author identity can still `/pull-base` when the result is a fast-forward; the diverged case still refuses with `ErrNoAuthorIdentity`. - Regression tests for both, alongside the existing clear-hook and no-identity tests.

dominik.polakovics added the

bug

ready-for-agent

labels

2026-07-13 00:06:59 +02:00

dominik.polakovics referenced this issue from a commit

2026-07-13 00:26:39 +02:00

fix(pull-base): word-match the clear hook; defer no-identity refusal to diverged pulls

dominik.polakovics referenced this issue from a pull request that will close it,

2026-07-13 00:26:54 +02:00

fix(pull-base): word-match the clear hook; defer no-identity refusal to diverged pulls #153

dominik.polakovics commented

2026-07-13 12:27:09 +02:00

Author

Owner

This was generated by AI while landing a PR.

Landing audit — PR #153 → PASS

Verification signal relied on: Forgejo Actions CI, ci / native (pull_request) — success (5m20s, run 167). Not re-run locally.

Checked:

Conventions — title fix(pull-base): word-match the clear hook; defer no-identity refusal to diverged pulls is Conventional Commits; head afk/151 carries a working Closes #151. Single commit, no conflict with origin/main (clean merge-tree against the post-#155 tip).
Gap 1 (arg-bearing clear) — commandWord (first token, unicode.IsSpace) now drives both dispatch paths, as the issue sketched. /clear my-session matches the clear hook, pulls, then forwards req.Text verbatim (the original untrimmed text, argument intact) via handleClearWithPull — the "pull-then-forward-preserving-the-argument" intent the issue asked to decide explicitly. /pull-base bare executes; any argument, space- or tab-separated, 400s.
Gap 2 (no-identity vs fast-forward) — the refusal moved below the fetch into gitx.PullBase, gated on !ffable. Verified the predicate is sound: the upToDate short-circuit (isAncestor(base, HEAD)) returns before ffable is computed, so a worktree merely ahead of an unchanged base — also not ff-able, also needing no commit — exits early and is never caught by the new refusal. !ffable at that point genuinely means diverged. The refusal lands before runPullMerge touches the worktree, and internal/pull re-flags gitx.ErrAuthorIdentityRequired as its own ErrNoAuthorIdentity, so the 409 classification is unchanged.
Tests — the two gaps and the prefix-confusion edge are pinned: TestAPI_ClearHook_argBearingClearPulls, TestAPI_ClearHook_prefixIsNotClear (/clearx is not /clear+arg), TestAPI_PullBase_tabArgRejected, plus gitx/pull-service pairs for ff-empty-identity-succeeds / diverged-empty-identity-refused.

Findings: none blocking. Scope matches the issue exactly — no divergence, no scope creep.

> *This was generated by AI while landing a PR.* ## Landing audit — PR #153 → PASS **Verification signal relied on:** Forgejo Actions CI, `ci / native (pull_request)` — **success** (5m20s, run 167). Not re-run locally. **Checked:** - **Conventions** — title `fix(pull-base): word-match the clear hook; defer no-identity refusal to diverged pulls` is Conventional Commits; head `afk/151` carries a working `Closes #151`. Single commit, no conflict with `origin/main` (clean merge-tree against the post-#155 tip). - **Gap 1 (arg-bearing clear)** — `commandWord` (first token, `unicode.IsSpace`) now drives *both* dispatch paths, as the issue sketched. `/clear my-session` matches the clear hook, pulls, then forwards `req.Text` **verbatim** (the original untrimmed text, argument intact) via `handleClearWithPull` — the "pull-then-forward-preserving-the-argument" intent the issue asked to decide explicitly. `/pull-base` bare executes; any argument, space- **or** tab-separated, 400s. - **Gap 2 (no-identity vs fast-forward)** — the refusal moved below the fetch into `gitx.PullBase`, gated on `!ffable`. Verified the predicate is sound: the `upToDate` short-circuit (`isAncestor(base, HEAD)`) returns *before* `ffable` is computed, so a worktree merely **ahead** of an unchanged base — also not ff-able, also needing no commit — exits early and is never caught by the new refusal. `!ffable` at that point genuinely means diverged. The refusal lands before `runPullMerge` touches the worktree, and `internal/pull` re-flags `gitx.ErrAuthorIdentityRequired` as its own `ErrNoAuthorIdentity`, so the 409 classification is unchanged. - **Tests** — the two gaps and the prefix-confusion edge are pinned: `TestAPI_ClearHook_argBearingClearPulls`, `TestAPI_ClearHook_prefixIsNotClear` (`/clearx` is not `/clear`+arg), `TestAPI_PullBase_tabArgRejected`, plus gitx/pull-service pairs for ff-empty-identity-succeeds / diverged-empty-identity-refused. **Findings:** none blocking. Scope matches the issue exactly — no divergence, no scope creep.

dominik.polakovics closed this issue

2026-07-13 15:46:02 +02:00