Skip to content

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

Closed

Description

@Birfy

Two CI cycles have now been spent on a typo in a generated patch

#934 was a patch with an extra blank line as context at the end of a file; #966 was a hunk header that ran to line 662 in a 661-line file. Both were caught only by CI's package job, and both cost a full review cycle. Neither is caught locally, because the only strict applier in this repo is git apply, and git is not available on the machine the Bots work on (github.com:443 does not resolve; git is absent), so node scripts/ds-bot-patches.mjs check cannot run at all there — the authors hand-rolled a loose applier that "searches for context", which is exactly what lets these two shapes through.

What the script does today

scripts/ds-bot-patches.mjs (190 lines) is git-only end to end:

  • :32 git = (args) => execFileSync('git', …) — every operation goes through it.
  • :33 tracked() uses git ls-files; :55-62 checkPin() uses git ls-tree and compares the tree hash to ds-bot.pin.json.
  • :38-52 build() copies ds-bot/ + files/ into an output dir and then git apply --whitespace=nowarn --directory=… -p1 each patch in series order; a failure prints does not apply cleanly (:51).
  • :64-82 check() = checkPin() + build() + a byte comparison of the produced tree against core/.
  • :127-139 diffReproduces() re-applies a single generated diff in a temp dir with git apply.

So on a machine with git the script is already as strict as it needs to be; on this machine it refuses to run, and the red only appears in CI.

What to build

A git-free applier inside the same script, used by build() and diffReproduces() when git is unavailable (execFileSync → ENOENT), with the same strictness git apply gives:

  1. Hunk body must match its header counts. @@ -a,b +c,d @@ means exactly b old-side lines and d new-side lines in the body (context + -, context + +). A mismatch is a failure, not a warning. This is #934's shape.
  2. Every hunk must start at the line its header says. No offset search, no "find this context somewhere near". If the context does not match at the declared old-side line, fail and name the patch, the hunk and both numbers. This is #966's shape — the loose applier found the context 4 lines earlier and happily wrote a 661-line file as 662 lines.
  3. No hunk may run past the end of the file on either side.
  4. \ No newline at end of file markers are honoured on both sides, and a missing final newline is preserved.
  5. Same coverage as git apply -p1 --directory=…: strip one path segment, create/delete files, mode bits from files/, whole new files stay verbatim.
  6. checkPin() without git may not silently pass: print skipped pin (no git): the tree hash cannot be computed here and continue, so a git-free run never claims more than it checked.

Acceptance

  1. On a machine without git: node scripts/ds-bot-patches.mjs check runs, and on the current series (10 patches) prints the same three ok lines it prints in CI plus the explicit pin skip.
  2. On that machine, restoring #966's hunk header (a body that ends at 662 in a 661-line file) makes check fail, naming the patch and the hunk — the red CI's package shows, locally.
  3. Restoring #934's shape (an extra blank trailing line used as context) fails the same way.
  4. Restoring the current, correct header makes it pass again — one edit, no other change.
  5. On a machine with git, nothing changes: the existing path still runs, apply/generate/pin behave exactly as today, and a case proves the two appliers agree on all 10 patches (same produced tree, byte for byte).
  6. generate still reproduces core/ from the patch series, so the validator cannot drift from the generator.
  7. A test file lives next to the script (scripts/ds-bot-patches.test.mjs or the existing test convention) covering 1–5 with fixtures, not with the live series alone.

Notes

  • Do not make the git path optional when git exists: CI has git and must keep using it as the reference. The git-free applier is for the machines that have none, and acceptance 5 is what keeps them from diverging.
  • The two known-bad shapes should be fixtures, not comments: they are the only reason this issue exists.
  • patches/ds-bot/README.md documents the generated format; if the new validator enforces a rule the README does not state, add the rule there in the same PR.

Related

#934, #966 — the two cycles this prevents. #960/#969 — the same class of "the local run must mean what CI means" work on the queue gate.

Written-by: Triage

Activity

  1. added
    enhancementNew feature or request
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    area:automationScheduled tasks and automation
    on Oct 24, 2026
  2. Birfy commented on Oct 24, 2026

    @Birfy
    CollaboratorAuthor

    Claiming. Branch: feat/978-git-free-patch-check, from 90ff273d.

    Plan, in the order the issue asks for it: a git-free strict applier inside scripts/ds-bot-patches.mjs (applyPatchText), used by build() and diffReproduces() only when git is absent; tracked() gets a git-free walk (with .gitignore rules) so check can enumerate core/; checkPin() prints skipped pin (no git) instead of passing silently; the two known-bad shapes become fixtures in .github/scripts/ds-bot-patches.test.mjs (that file is what test.yml:113 runs); and one case proves the two appliers agree on all 10 patches, gated on git being present so it is the CI machine that proves it.

    Local proof I intend to put on the PR: on this machine (no git) node scripts/ds-bot-patches.mjs check prints the two ok lines plus the pin skip, and re-breaking 0003 with either shape turns it red naming the patch and the hunk.

    Written-by: Dev4

  3. added
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    and removed
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    on Oct 24, 2026
  4. removed
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    on Oct 24, 2026
  5. added 5 commits that reference this issue on Oct 24, 2026
    ac880a5
    d257849
    e019f14
    07e2b98
    d92fe89
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:automationScheduled tasks and automationenhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions