Skip to content

fix(git-read): disambiguate revision and path arguments - #564

Open
ooiuuii wants to merge 1 commit into
openpi-dev:mainfrom
ooiuuii:fix/git-read-revision-boundary
Open

ooiuuii wants to merge 1 commit into
openpi-dev:mainfrom
ooiuuii:fix/git-read-revision-boundary

Conversation

@ooiuuii

@ooiuuii ooiuuii commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Problem

Fixes #563.

The structured git_show, git_log, and git_diff tools distinguish revisions from optional paths, but their argv builders omitted Git's -- separator when no path was supplied. A branch sharing a name with a worktree file therefore failed as ambiguous; a file-only name supplied as a revision could instead silently become a path filter.

Value

Agents can inspect a named revision regardless of matching worktree filenames, and receive a proper failure for a nonexistent revision instead of apparently successful but incorrect evidence. This also applies to child agents using these existing read-only tools.

Approach

  • Always place -- after the existing options/revisions in all three builders, then append the optional path.
  • Correct the nearby comments to distinguish option validation from revision/path disambiguation.
  • Add six real-Git regression cases: each tool with a same-named branch/file, and each tool with a file-only name in its revision field.

One git-read argument-boundary fix, across three existing files (+46/-8). No new tool, preflight subprocess, configuration, or shared-runtime refactor. Existing #368's process/output seam refactor is separate.

Validation

Environment: Windows, Node 22.22.3, Bun 1.3.14, Git 2.54.0.windows.1; base f6b49ae59605b1276b8267f2886d22c03f01533c.

  • RED, before production change: node --experimental-strip-types --test tests/extensions/git-read/index.test.ts — 10 existing cases pass, all 6 new regressions fail.
  • GREEN: node --experimental-strip-types --test tests/extensions/git-read/args.test.ts tests/extensions/git-read/index.test.ts19/19 pass. These execute the production builders and runGit against a disposable two-commit repository; output and error semantics are asserted, not only argv shapes.
  • Existing explicit-path, staged/worktree, revision-range, timeout, external-diff and textconv tests remain passing.
  • bun run checkpass, including build, format, lint, and typecheck (existing Vite chunk-size advisory).
  • git diff --check — pass.
  • One scoped structured AutoReview (--mode local --engine codex --model gpt-6-astra --thinking xhigh --no-web-search) — clean, no actionable findings; no scope expansion or review-driven code changes.

Local full-suite limitations: the default bun run test attempt reached 1,556 passing / 1 failing / 10 skipped tests in its parallel Node group. The failure is the unchanged git-info/process.test.ts case captures output and tolerates command failures (1-second subprocess bound, expected 7 but got -1). A clean checkout of the exact base reproduces the same failure (plus an unrelated narrator VM timeout), while the original git-info test file passes 3/3 in isolation. That attempt stopped before the later Windows/Vitest groups.

A diagnostic rerun used the same repository test discovery and Windows group separation, with Node file concurrency bounded to 2: 1,557 pass / 0 fail / 10 skipped in the Node parallel group, then 220/220 Vitest tests pass across 11 files. The unchanged Windows background-terminal group had 7 process-lifecycle failures and hung; its own test process tree was terminated and cleaned up so validation could continue. That group was not completed, and the local full suite is not claimed green. Its termination problem is not diagnosed by this PR; no unrelated timeout/lifecycle changes are included.

Remote CI, exact head 6471ee2: Node 22.19.0 / 24 / 26 and Web E2E pass. The Windows job has one different failure in the unchanged pending completion retains its spill until delivery releases it test (oldestDone.stdout.spillPath absent); its later full-suite step was not run. This standalone suite does not import git-read. A maintainer rerun was requested, since this account lacks permission to rerun that workflow. No claim of all-green CI or merge readiness.

Impact

  • User/model-visible behavior: matching filenames no longer break valid revision requests; file-only revision inputs now fail rather than being reinterpreted as path filters.
  • Tools/compatibility: schemas and validators unchanged; use the existing path/file fields for path filtering. Worktree/staged modes and from...to comparison semantics are unchanged.
  • Runtime/lifecycle: same subprocess runner, timeouts, cancellation, and output limits.
  • Persisted config/data: none.
  • Security: existing read-only command set and --no-ext-diff/--no-textconv protections preserved. No broader path or revision syntax accepted.

@ooiuuii

ooiuuii commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

CI follow-up for 6471ee28ed4752716b20c9c902be6bf366d24842:

  • Node 22.19.0 / 24 / 26 and Web E2E passed.
  • The Windows job failed in the existing pending completion retains its spill until delivery releases it test: assert.ok(oldestDone.stdout.spillPath) at tests/extensions/background-terminals/manager.test.ts:1134 (33 pass / 1 fail / 4 skipped). It did not reach its later full-suite step.
  • This PR does not change the background-terminal implementation, its tests, or the workflow, and that standalone failing suite does not import git-read. The same Windows job passed on base f6b49ae. This looks like a separate intermittent lifecycle/spill test failure; I am not claiming its cause is established or making an unrelated change here.

Could a maintainer rerun the failed Windows job? I attempted gh run rerun --failed, but GitHub requires repository administrator rights for this account. The six new Git subprocess regressions, all 19 focused tests, and bun run check pass. Keeping this PR scoped to revision/path disambiguation.

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.

bug(git-read): revision fields are interpreted as paths without a separator

1 participant