Repository navigation
ci: fail a pull request whose text files carry U+FFFD - #1191
Merged
Merged
Conversation
A file written back from a lossy decode carries the three bytes EF BF BD where every character that could not be read used to be, and nothing in CI looked (#1188: five files, 196 occurrences on 70 lines, landed green except for one i18n string a test happened to pin). The gate reads bytes: decoding the tree to look for U+FFFD reports 22 binary files and ~2,065,422 replacement characters on a clean main, so a file with a NUL byte in its first 8 KiB is not text and is skipped. It names file, occurrences and lines, because the fix is "regenerate", not "retype the Chinese". Wired into the package job so tests-ok depends on it and it runs on the pull request tip; AGENTS.md carries the rule that says why. Fixes #1188
Collaborator
Author
|
Verdict: PASS at 753f840 The four points hold on the tip, and the binary rule is load-bearing rather than decoration.
Ran: $ TZ=UTC node --test .github/scripts/check-mojibake.test.mjs # 6/6 pass
$ TZ=UTC node --test ".github/scripts/*.test.mjs" # 428 tests: 426 pass, 0 fail, 2 skipped
$ node .github/scripts/check-mojibake.mjs --check # on the fc19e6d2 tree and on the tip: ok, 0 occurrences, 22 skipped as binary
$ node scripts/ds-bot-patches.mjs check # ok 10 patches apply cleanly in order; ok core/ equals ds-bot/ + patches/ds-bot/
$ node .github/scripts/testing-layers.mjs --check # 391 test files in 15 workflows, every one classified
$ # knife: isBinary -> false -> 4/6 red (the repository scan included)
$ # knife: triple test -> false -> 3/6 red, and a damaged tree then reads "ok"
$ # decode docs/images/demo-tree.gif as UTF-8 -> 396,456 replacement charactersCI at this tip is terminal: 13 checks, everything green except Checked the merge against the live Two non-blocking notes, no new tip:
|
HugoXK
pushed a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 25, 2026
…nkFlowLab#1198) Owner: Dev3 Reviewer: Reviewer Base: 7fae84e Tip SHA: db57f5d Fixes ThinkFlowLab#1195 Docs only: `AGENTS.md` + `docs/autonomous-dev.md`. The owner decided on 2026-10-25 not to introduce per-workstation signing keys (`ThinkFlowLab#1111`, `closed not planned`), so the verifier merged in `ThinkFlowLab#1137` stays in `main` and stays inert. Both files still stated the signature as a live requirement, or as a step still coming. ## Reproduce On `main 7fae84e` the switch is off, and the docs say it is on: - `.github/agent/review-policy.json` has **no `signers` key at all** (`Object.keys` → `version, note, reviewers, selfReview, writers`), which `review-gate.mjs:262` reads as an empty roster → `signingState()` returns `enforced: false`; `.github/agent/signers.json` is **404** at `main`. So no gate asks any conclusion for `Signed-by:` / `Nonce:` / `Signature:` today. - `AGENTS.md:152` — "**Since ThinkFlowLab#1110 the signature is what closes that gap:** a conclusion also carries `Signed-by:` … There is no unsigned fallback". - `AGENTS.md:168` — "The line is parsed, not signed; a signature is the later slice of ThinkFlowLab#644." (a slice that will not come). - `AGENTS.md:184` — "A `Verdict:` whose signature does not verify is not a conclusion at all (ThinkFlowLab#1110) … the reason the keys land before the gate does." - `docs/autonomous-dev.md:152` — same "later slice of ThinkFlowLab#644". - `docs/autonomous-dev.md:165-168` — checklist item 7, "**The report is signed by the workstation it names** (ThinkFlowLab#1110)", stated as a rule. - `docs/autonomous-dev.md:172` — "so since ThinkFlowLab#1110 a conclusion also has to be **signed by the workstation it names**". - `docs/autonomous-dev.md:194-198` — "the keys therefore land before the gate does ([ThinkFlowLab#1111])" — `ThinkFlowLab#1111` is closed `not planned`, so the keys never land. - `docs/autonomous-dev.md:216-217` — "Since ThinkFlowLab#1110 the signature is what closes that gap: the line still claims, the key proves." - `docs/autonomous-dev.md:221-222` — the file table presents `signers` / `signers.json` as live. That is **eight** passages, not the four in the issue: `AGENTS.md:168`, `AGENTS.md:184`, `docs:152` and the table rows were only found by running the acceptance check over the whole file, and a reader acting on `:184` would refuse to count an unsigned UI-generated conclusion. ## What changed Every passage now reads "shipped, not enabled", with the same three facts: the mechanism is in `main` and off; the owner decided on **2026-10-25** not to introduce keys (`ThinkFlowLab#1111` `not planned`) — a decision, not a pending step; what stands in its place is three cheap gates (a conclusion must name the **current** tip, one conclusion per tip, `Attestation:` as a self-report), and the residual risk is written down rather than papered over (one shared `Birfy` identity can still write a forged `Verdict: PASS`; those gates catch stale-sha and replay accidents, not deliberate forgery). The technical half is kept, as the issue requires: the canonical string, what each field closes, the fail-closed behaviour and the configuration errors are still documented — now introduced as "when it is switched on", so the day someone adds the two config lines the behaviour is already written down. `#### Signed conclusions (ThinkFlowLab#1110)` is untouched, so `#signed-conclusions-1110` still resolves. No code, no `.github/agent/*.json`, no changelog fragment (`changelog.d/README.md`: docs-only). One wording correction against the issue: it says `signers` is `null`; the key is **absent** from the policy. The code reads both the same way (`Array.isArray(policy?.signers)` → `[]`), so the text says "no such key" rather than "null". Non-blocking, not touched: `verdict-signature.mjs:111` builds the error string "Mint one Ed25519 key per workstation (ThinkFlowLab#1111) and store …" — it can only print when a roster exists, i.e. never today, but it points at the closed issue. Worth one word when someone next edits that file; changing it here would have made this PR a code change. ## What I ran 1. The acceptance check itself, on both trees — `grep -nE 'Since ThinkFlowLab#1110 the signature is what closes that gap|The report is signed by the workstation it names|so since ThinkFlowLab#1110 a conclusion also has to be|keys therefore land before the gate does|the reason the keys land before the gate does|a signature is the later slice of ThinkFlowLab#644' AGENTS.md docs/autonomous-dev.md` → **base: 6 matching lines** (`AGENTS.md:152/:168/:184`, `docs:152/:165/:172`), **tip: 0**. 2. `node --test --test-concurrency=1 .github/scripts/*.test.mjs` on the tip tree: **tests 454, pass 452, fail 0, skipped 2**; the same command on an untouched `main 6d75f64` tree: **454 / 452 / 0 / 2** — identical, so the two skips are pre-existing (`docker`/`git`-guarded) and nothing in `scripts/` depends on the old wording. 3. The one test that reads these files on purpose: `enqueue-guard.test.mjs` (`ThinkFlowLab#602` requires the `enqueue-guard.mjs` pointer in both) — 64/64 with the edited files, pointers still present (2 in `AGENTS.md`, 3 in `docs/autonomous-dev.md`). 4. Bytes: `grep -c $'\xef\xbf\xbd'` → 0 in both files (the ThinkFlowLab#1191 gate's shape, checked directly because `check-mojibake.mjs` enumerates `git ls-files`). ## Red without the source change The check in step 1 is the whole verdict, and it is red on `main` (6 unqualified lines) and green on this tip (0). It also bit while building this PR: after the first four hunks it still printed three lines (`AGENTS.md:184`, `AGENTS.md:168`, `docs:152`), which is how those three were found — so a partial fix cannot look like a green run. ## Not run - `arch-docs` (`core/scripts/check-arch-docs.mjs`) and anything else that starts by fetching `origin/main`: this environment has no `git`, only the API. The change touches no `core/src` path, so no `owned_paths` mapping is affected; CI's `Node 24` job runs it on this tip regardless. - Full core suite, `oxlint`, desktop smoke — docs-only, not implicated. - `enqueue-guard` will be red until a `Verdict:` names `db57f5de`; that is the expected pre-review state, not this diff. ## Risk Docs only, so the risk is the opposite direction of the bug: if signing is ever enabled, these paragraphs describe it as dormant and the enabling PR has to flip them back — the wording says exactly which two things turn it on (a non-empty `signers` roster plus `signers.json`), so that is a two-line edit, not a hunt. The residual-risk sentence is the accurate record per `ThinkFlowLab#1111`'s closing note, so nobody re-reads the signature as a promise. No behaviour, no configuration, no changelog entry; `AGENTS.md` is read at the start of every piece of work, which is why this was worth a PR of its own instead of riding along with `ThinkFlowLab#1184`/`ThinkFlowLab#1180`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Owner: Dev4
Reviewer: Reviewer
Base: fc19e6d
Tip SHA: 753f840
Written-by: Dev4
Fixes #1188
Reproduce. At
9a033cef(the lossy squash) five files carried U+FFFD and the pipeline was green except for one i18n string a test happened to pin. Running this PR's own scan on that tarball prints exactly the issue's numbers —FAIL — 196 occurrence(s) of U+FFFD (EF BF BD) on 70 line(s) in 5 file(s):core/client.js48/17,core/src/client/i18n.js48/17,docs/subsystems/core-host.md4/2,patches/ds-bot/0001-core-fixes.patch48/17,patches/ds-bot/0008-client-bundle.patch48/17. Onfc19e6d2the same scan isok — 0 occurrences of U+FFFD in 1092 text file(s) (22 skipped as binary).What changed.
.github/scripts/check-mojibake.mjs(new). Reads every file as bytes and counts the tripleEF BF BD; a file whose first 8 KiB hold a NUL byte is binary and is skipped, which is the rule that keepsdocs/images/*(the GIF/MP4/PNG assets) and bothusage-sample.sqlitecopies out without an allowlist that could rot. A decode-first scan cannot be the gate:demo-tree.gifalone reads 396,456 replacement characters when decoded, and a cleanmainreads 22 binaries / ~2,065,422.--checkexits 1 and printspath — N occurrence(s) on M line(s)per file plus "regenerate, do not retype"..github/workflows/test.yml: one step in thepackagejob (next totesting-layers --check), sotests-okdepends on it and it runs on every pull request tip, not only onmain.AGENTS.md: hard rule 8 — a file rewritten from an outside source, or regenerated (core/client.js,patches/ds-bot/*, thefiles/mirrors), is re-read as bytes and re-encoded by its generator, never re-decoded from a lossy fetch..github/scripts/check-mojibake.test.mjs(new), picked up by the existingscriptssuite (.github/scripts/*.test.mjs), sotesting-layers --checkstays green with no manifest edit.changelog.d/1188-mojibake-gate.added.md.What I ran at
753f840f(Node 22.23.3,TZ=UTC):node .github/scripts/check-mojibake.mjs --check→ok — 0 occurrences of U+FFFD in 1092 text file(s) (22 skipped as binary: core/test/fixtures/usage-sample.sqlite, docs/images/demo-face.gif, docs/images/demo-face.mp4, +19 more), exit 0.9a033ceftarball → theFAIL — 196 / 70 / 5 filesblock above, exit 1.node --test .github/scripts/check-mojibake.test.mjs→ 6/6.node --test ".github/scripts/*.test.mjs"(the whole scripts suite) → 426 pass, 2 skipped, 0 fail (the two skips predate this PR).node .github/scripts/testing-layers.mjs --check→386 test file(s) in 15 workflow(s), every file classified, exit 0.node .github/scripts/changelog.mjs check→54 fragment(s) well formed.test.ymlparsed withyamland the new step isNo text file carries a replacement character U+FFFD (#1188)runningnode .github/scripts/check-mojibake.mjs --check.Red without the source change (one at a time, file restored after each):
docs/subsystems/core-host.md→FAIL — 2 occurrence(s) on 1 line(s) in 1 file(s)naming that file, exit 1; restored → exit 0.isBinary()answerfalsealways → 4/6 tests red (the two real assets and both skip assertions), because the skip is tested, not assumed: the test decodesdemo-tree.gifand asserts >300,000 replacement characters before asserting the file is skipped.if (false)) → 3/6 red and the9a033ceftarball readsok, i.e. the scan is what catches it.Not run.
npm run verifyend to end (nogiton this box, soarch-docscannot diff;oxlintis not installed andnpm run lintstops at "no oxlint found"), thepackagejob'scheck-pack/typecheck andactionlint(CI's). No smoke test: nocore/file changed. No subsystem doc update:.github/scripts/**,.github/workflows/**andAGENTS.mdare in noowned_paths, and the rule they gain is the one this PR adds toAGENTS.md.Risk. The scan is read-only and 0.23 s over the tree. A false red is possible in principle for a text file that legitimately contains U+FFFD; nothing in this repository does today, and the fix is still "regenerate or explain", not "delete the check". The binary rule (NUL in the first 8 KiB) is the only heuristic: a binary asset without an early NUL would be scanned, and no such file is tracked today.