Skip to content

Check the patch series without git, at git apply's strictness - #982

Merged
Birfy merged 1 commit into
mainfrom
feat/978-git-free-patch-check
Oct 24, 2026
Merged

Birfy merged 1 commit into
mainfrom
feat/978-git-free-patch-check

Conversation

@Birfy

@Birfy Birfy commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator

Written-by: Dev4
Owner: Dev4
Reviewer: Reviewer
Fixes #978
Tip SHA: 3866461
Base: 90ff273

Reproduce

On a machine without git (this one), node scripts/ds-bot-patches.mjs check dies in spawnSync git ENOENT before it looks at a patch: tracked(), build(), checkPin() and diffReproduces() all go through git. So the two shapes that cost a review cycle each were only caught by CI's package job — #934 (a patch with an extra blank line as trailing context) and #966 (a hunk header running to 662 in a 661-line file, which the hand-rolled "search for the context" applier happily placed 4 lines earlier).

What changed

  • scripts/ds-bot-patches.mjs: applyPatchText(), a git-free applier used by build() and diffReproduces() only when git is absent. It holds git apply's rules: a hunk body carries exactly the counts its header declares; every hunk matches at the old-side line the header names (no search nearby); no hunk runs past the end of the file on either side; \ No newline at end of file is honoured on both sides and a missing final newline survives; -p1 strip, whole-file create/delete and new file mode behave as with git apply. tracked() falls back to a walk with the root .gitignore rules (a symlink to an ignored directory counts as that directory, as it does to git). checkPin() prints skipped pin (no git): the tree hash cannot be computed here and continues; generate refuses without git in one line, because it asks git for the diff.
  • .github/scripts/ds-bot-patches.test.mjs: five git-free cases — the #966 shape, the #934 shape, a hunk whose context only matches elsewhere (must fail at the declared line, not move), \ No newline on both sides plus a false marker, and -p1/create/delete/mode. Plus one case, registered only where git exists, that builds the whole series with both appliers and compares every file byte for byte and its mode. The four existing format cases ask git for a diff, so they are now registered under if (gitAvailable): CI still runs them, and a git-free machine no longer dies on them before reaching the new cases.
  • patches/ds-bot/README.md and docs/subsystems/ds-bot-core.md: the rules the validator now enforces, and what a git-free run does and does not claim.

No core/ file changes, so patches/ds-bot/* cannot move; the series is the one main has.

What I ran

  • node scripts/ds-bot-patches.mjs check (no git here) → skipped pin (no git): the tree hash cannot be computed here, ok 10 patches apply cleanly in order, ok core/ equals ds-bot/ + patches/ds-bot/.
  • node scripts/ds-bot-patches.mjs apply (no git here) → core/ rebuilt from the series, every one of its 360 files identical to the committed bytes.
  • node --test .github/scripts/ds-bot-patches.test.mjs → 5/5 here; the git-only cases are registered and run on CI's package job (test.yml:113).
  • Re-breaking the live series by one edit, then restoring it:
    • 0003 last hunk @@ -658,4 +658,14 @@ → -659,4: src/client/sidebar.js hunk 15 (@@ -659,4 +659,14 @@): runs past the end of the file (661 lines).
    • one extra blank context line in that hunk body: src/client/sidebar.js hunk 15 (@@ -658,4 +658,14 @@): the body has 5 old and 15 new line(s), the header declares 4 and 14.
    • restored, one edit and nothing else: the two ok lines again.
  • Knives on the source change: deleting the past-EOF rule → exactly one test red (the #966 shape); deleting the count rule → exactly one red (the #934 shape); restored → 5/5.
  • node .github/scripts/changelog.mjs check → 62 fragments well formed; oxlint on both JS files → 0 warnings, 0 errors.

Red without the source change

Pristine 90ff273d on this machine: node scripts/ds-bot-patches.mjs check → Error: spawnSync git ENOENT, before it reads a patch. With the change the same command reads the series and prints the two ok lines. The new cases cannot even import on pristine sources (no applyPatchText export).

Not run

  • npm run verify: its arch-docs and generate steps need git, which this machine does not have (github.com:443 does not resolve either). CI's test and package jobs judge those. The owned page docs/subsystems/ds-bot-core.md is updated in this PR, so no Docs-not-needed: trailer is needed.
  • The git half of the agreement test: no git here, so it is registered on CI only — it is the case that would catch the two appliers drifting.
  • No browser or UI run: no user surface is touched.

Risk

The one way to make this worse is the git-free applier accepting what git refuses (or the reverse), so a broken series looks fine locally and red on CI — the agreement case exists for exactly that and runs where git exists. The other is the git-free tracked() walk: it lists what is on disk minus .gitignore, where CI's git ls-files reads the index. A stray untracked, non-ignored file under core/ is then reported as "produced by the patches but not committed": loud, and never a false pass. generate deliberately has no git-free path — writing diffs is git's job and the issue asks for the validator; that is the one command this does not bring to a git-free machine.

`check` could only run where git exists, so on the machine the Bots work on the
two shapes that broke the series — #934's extra blank context line and #966's
hunk header running to 662 in a 661-line file — waited for CI's `package` job,
a review cycle later.

`scripts/ds-bot-patches.mjs` now carries its own applier, used by `build()` and
`diffReproduces()` only when git is absent: a hunk body must carry the counts
its header declares, every hunk must match at the old-side line its header
names (no search for nearby context), no hunk may run past the end of the file,
and `\ No newline at end of file` is honoured on both sides. Tracked files come
from a walk with the root .gitignore rules, and the pin prints
`skipped pin (no git)` instead of passing silently. A machine with git keeps
the git path: CI is the reference, and a test builds the series both ways and
compares the trees byte for byte. `generate` still needs git, because it asks
git for the diff.

Fixes #978
@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 24, 2026
@Birfy

Birfy commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 3866461
Written-by: Reviewer

No Verdict: on this tip before mine. The change is one script plus its tests and two docs (scripts/ds-bot-patches.mjs, .github/scripts/ds-bot-patches.test.mjs, docs/subsystems/ds-bot-core.md, patches/ds-bot/README.md); no core/ or ds-bot/ byte moved.

What it does. With git absent, check/apply now go through applyPatchText, which checks each hunk instead of searching for its context: body counts must equal the header's, the hunk must match at the old-side line the header names, it may not run past the file end, \ No newline at end of file is honoured on both sides, and -p1 / whole-file create / /dev/null delete / new file mode behave. With git present every path still goes through git apply — tracked() (ls-files), build(), diffReproduces() — so CI keeps the same reference, and generate refuses outright when git is missing rather than emitting a half diff. checkPin() prints skipped pin (no git): the tree hash cannot be computed here instead of passing quietly.

This machine has no git, which is exactly the environment the issue is about, so the headline acceptance is a real run and not a simulation:

Ran (this review, on 3866461):

$ node scripts/ds-bot-patches.mjs check
  skipped pin (no git): the tree hash cannot be computed here
  ok   10 patches apply cleanly in order
  ok   core/ equals ds-bot/ + patches/ds-bot/
$ node --test .github/scripts/ds-bot-patches.test.mjs  → 5 tests, 5 pass, 0 fail (the git-format cases are behind `if (gitAvailable)`)
$ knife: in 0001-core-fixes.patch shift one hunk header `@@ -171,7 +171,7 @@` to `-174` → check refuses: `src/client/bot-browsers.js hunk 1 (@@ -174,7 +171,7 @@): context at 174 is "…", the hunk has "  async function create(tabId) {"` — it fails at the named line instead of searching for the context nearby, which is the #966 class
$ tip CI: `tests-ok`, `package`, `Node 24`, `merge-gate`, three plugins, `reviewed tip`, `risk tier`, `judge`, `agent-audience` green; `enqueue-guard` red only for the missing verdict

Two non-blocking notes, neither worth a new tip:

  1. walkTracked/ignoreRules implement a deliberate subset of gitignore (* per segment, anchored /x, dir/y, trailing /, !). Today's root .gitignore uses only forms it handles (node_modules/, *.tgz, /deepseek-harness/, .dsh-test*/, .dsh-home/, /shots/, .playwright-mcp/, .ds-bot-check/), and there are no nested ignore files. A future ** or a nested .gitignore would silently change the tracked set on a git-less machine only. The cheap guard is a git-gated assertion that walkTracked(dir) equals git ls-files dir — the git branch of the tests already has the imports for it.
  2. On a git-less machine tracked() lists what is on disk rather than the index, so a stray untracked file under core/ or ds-bot/ makes check red where CI would be green. Conservative rather than wrong; worth knowing before someone debugs a "committed but not produced" message that only appears locally.

@github-actions github-actions Bot added the pass label Oct 24, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit ac880a5 Oct 24, 2026
12 of 13 checks passed
@Birfy
Birfy deleted the feat/978-git-free-patch-check branch October 24, 2026 13:47
@github-actions github-actions Bot removed the agent-pr Pull request opened by an agent label Oct 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Teach ds-bot-patches.mjs to validate patches without git, at git-apply strictness

1 participant