Skip to content

docs(attachment): state the real termination argument for the defuse loop - #4372

Merged
aheritier merged 1 commit into
docker:mainfrom
PerryLink:fix/4054-defuse-termination-comment
Oct 5, 2026
Merged

aheritier merged 1 commit into
docker:mainfrom
PerryLink:fix/4054-defuse-termination-comment

Conversation

@PerryLink

Copy link
Copy Markdown
Contributor

Fixes #4054.

maxDefusePasses justified its bound with an argument that is measurably false, and the issue is right about that. While correcting it I found the argument wrong in a third way the issue does not mention, so this records the real invariant instead of a patched-up version of the old one.

What was wrong

  1. "Each pass strictly shortens the body" — false. The placeholder is 42 bytes and a short delimiter like </document-x> is 13, so defusing grows the body. Measured: 13 -> 42, and 26 -> 43.
  2. "One pass can leave a delimiter-shaped residue behind" — false. envelopeTagRe runs to the first >, so </TAG</TAG>> is a single match and any nested delimiter is consumed with it. One pass is always sufficient.
  3. The issue's own third data point doesn't reproduce. It reports 117 -> 50 (shrank) for many delimiters. I measured </document-x> × 9 (117 bytes) as 117 -> 378 (grew). Shrinking happens when a match span is longer than 42 bytes, not when there are many delimiters.

What actually holds

Termination rests on one property of the replacement text: delimiterPlaceholder contains no <. A replacement can therefore never introduce a delimiter start, while every match consumes at least one <. Each changing pass strictly reduces the body's < count, which is a non-negative integer — so the loop cannot run forever.

I verified "one pass is always enough" rather than asserting it: brute-forced 846,383 bodies (all {<,>} strings up to length 14, plus token-alphabet enumeration to depth 5). With maxPasses=1, zero delimiters survived and the < count never increased.

The placeholder's character set is worth recording as load-bearing rather than cosmetic: the loop returns whatever it has once the bound is exhausted, silently and with no error, so a placeholder that gained an angle bracket (say <removed>) could let a live delimiter through inside the envelope.

Also

envelope_test.go:95's comment repeated the same false premise, so it is corrected too. The assertion itself is correct and unchanged — a nested delimiter must genuinely leave no residue, whether it is neutralised in the matching pass or a later one.

Scope

Comment-only, 2 files, +23/−9. Verified no code line changes: every added/removed line is a // comment.

go test -count=1 ./pkg/attachment/   ok
gofmt -l pkg/attachment/             clean
go vet ./pkg/attachment/             exit 0
go run ./lint ./pkg/attachment       no offenses

task test (go test ./...) has unrelated failures in pkg/rag/treesitter (needs CGO_ENABLED=1) and pkg/workspacemedia (Windows symlink privilege); both reproduce with my changes stashed.

@PerryLink
PerryLink requested a review from a team as a code owner September 21, 2026 14:01
@aheritier aheritier added the status/needs-signed-commits Some commits in the PR are signed with a valid SSH/GPG key label Sep 21, 2026
@aheritier

Copy link
Copy Markdown
Collaborator

👋 Some commits in this PR are not signed and verified by GitHub. Please sign your commits with a GPG or SSH key registered in your GitHub account, then force-push.

Commits that are not verified: 386a0b5

See GitHub's guide on signing commits for setup instructions. I've added status/needs-signed-commits; it will be removed automatically once every commit in this PR carries a valid GitHub-verified signature.

…loop

maxDefusePasses justified its bound with an argument that is measurably false: replacing a delimiter with the placeholder grows the body, because the placeholder is 42 bytes and </document-x> is 13. The claim that one pass can leave a residue is wrong too, since envelopeTagRe stops at the first '>' and so consumes a nested delimiter in the same pass.

Termination actually rests on the replacement text containing no '<': a replacement can never introduce a delimiter start, while every match consumes at least one '<', so each changing pass strictly reduces the body's '<' count. Record that, since the loop returns silently when the bound is exhausted and the placeholder's character set is therefore load-bearing.

Comment-only change; no behaviour change.

Signed-off-by: PerryLink <255665900+PerryLink@users.noreply.github.com>
@aheritier aheritier added area/core Core agent runtime, session management kind/docs Documentation-only changes labels Sep 21, 2026
@PerryLink
PerryLink force-pushed the fix/4054-defuse-termination-comment branch from 386a0b5 to 50dda3c Compare September 21, 2026 15:08
@aheritier aheritier removed the status/needs-signed-commits Some commits in the PR are signed with a valid SSH/GPG key label Sep 21, 2026
@PerryLink

Copy link
Copy Markdown
Contributor Author

Confirming this is resolved: the branch now carries a signed commit — GitHub reports the head commit as verified: true (reason: valid), and the status/needs-signed-commits label has been removed automatically, as your message said it would be.

Sorry for the silence on it. Nothing further needed from this end unless you would like the branch rebased onto the current base.

@aheritier
aheritier enabled auto-merge October 5, 2026 11:57
@aheritier
aheritier added this pull request to the merge queue Oct 5, 2026
Merged via the queue into docker:main with commit 54cea98 Oct 5, 2026
15 checks passed
PerryLink added a commit to PerryLink/perrylink that referenced this pull request Oct 5, 2026
A merge landed after the last round was written: docker/docker-agent#4372, merged
2026-10-05T12:14:03Z by maintainer aheritier. That is the whole of the movement -
the contributor set goes 43 -> 44 repositories and 317 -> 318 merges, the merged
total 350 -> 351 across 45 -> 46 repositories, open proposals 94 -> 102 across
56 -> 64, and the star table 20 -> 21 rows with Docker joining as the seventh
company or jointly-governed organization.

laya#943 also opened, so the laya line reads 46 merged of 50 opened, 1 open and
3 closed unmerged rather than 0 open. All twenty star counts re-measured.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core Core agent runtime, session management kind/docs Documentation-only changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

maxDefusePasses documents a termination argument that is false

2 participants