Repository navigation
Link only a closing keyword, and read every comment page, on the delivery board (#1189) - #1190
Merged
Merged
Conversation
…nt page (#1189) The board counted any `#12` in a pull request body as a link, so "also see #12" attached a pull request that closes nothing; and it read one page of comments, so a verdict past the first hundred read as "no verdict at this tip". Only GitHub's closing keywords (close/fix/resolve + #n, case-insensitive) link now, and comments are read page by page through the existing pages() helper, capped at MAX_PAGES. Read-only, no new endpoint, no change to the snapshot shape.
Collaborator
Author
|
Verdict: PASS at 36ba643 Both #1189 points are fixed in the two places the issue named, and nothing else moved: the diff against
Ran: $ TZ=UTC node --import ./test/harness-resolve.mjs --test --test-concurrency=1 test/delivery-board.test.js # from core/: 14/14 pass
$ # knife: CLOSING_KEYWORD back to /#(\d+)/g -> 13/14, only "only a closing keyword links a pull request to an issue" reds
$ # knife: commentsOf back to page=1 only -> 13/14, only "a verdict on a later page of comments still decides the badge" reds
$ grep -rlI $'\xef\xbf\xbd' . --exclude-dir=node_modules # no file listed
$ node scripts/ds-bot-patches.mjs check # ok 10 patches apply cleanly in order; ok core/ equals ds-bot/ + patches/ds-bot/ (pin skipped: no git here)
$ diff -rq <fc19e6d2 tree> <36ba6432 tree> # the 6 files above, nothing elseCI at this tip: every check completed and green except One non-blocking note, no new tip: a keyword list such as |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 25, 2026
HugoXK
pushed a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 25, 2026
…it (ThinkFlowLab#1192) (ThinkFlowLab#1196) Owner: Dev2 Reviewer: Reviewer2 Tip SHA: b9f57ae Base: 6d75f64 Fixes ThinkFlowLab#1012 ## Reproduce Same work as ThinkFlowLab#1192, filed 20 minutes earlier and closed as a duplicate of this reopened issue: the branch, the tip and every number below are unchanged. The first merge_group entry of ThinkFlowLab#1190 (`66a6d9e5`) failed `core/test/follow-through.test.js:153`, `a group turn cut off by a restart is picked up once, and the group is told`, at `openRoomTurn` → `settle` → `condition not reached`; the requeue merged as `687f805f`. That case calls `mockTimers(t)`, so its waits take `waitFor`'s mocked branch (`:120-124`), which was bounded by `rounds = 20000` alone while the chain under it does real work. Locally the same wait is measurable: under a pinned CPU with one hog and `nice -n 19`, the case's waits need **1168 rounds** and **2.7 s** — one round costs ~2.3 ms there, because the round is `tick(1)` + `setImmediate` on a starved core. On an unloaded box the same waits use 8 rounds (p99) out of 20000. So the round count is a proxy for real time that moves by an order of magnitude depending on why the machine is slow — and the chain it waits on is real I/O either way. ## Measure before the change (main `687f805f`, `waitFor` instrumented to print rounds + ms per wait) | what | result | |---|---| | `follow-through.test.js` × 100, plain | **100/100 green**, 0 failures | | mocked-branch waits over those runs (2800 waits) | rounds p50 **2**, p99 **8**, max **9** of 20000 | | real-branch waits over those runs (900 waits) | ms p99 **304**, max **305** | | the failing case only, 10 runs pinned to one CPU with a hog, `nice -n 19` | 0 failures; mocked waits max **1168 rounds** / **2686 ms** | Reading: the budget never ran out locally — a CPU-starved loop spends its rounds slowly, which is the opposite direction. The merge_group failure is the other direction: ThinkFlowLab#1014 measured 20000 rounds at ~70 ms of real time, so a machine slow in **I/O** (the `inflight.json`/`state.json` writes, the Session flush) burns the whole budget while the work is still in flight. Both directions say the same thing — rounds are not time — so (a), the shape this issue prescribes, is the fix. ## What changed `core/test/follow-through.test.js` +23/−1, and the generated `patches/ds-bot/0001-core-fixes.patch` hunk that carries it +25/−3 (that file is patch-tracked, not one of the mirror files). - A mocked wait still calls `tick(1)` every round — only a tick moves the mocked clock, and a `setTimeout` in front of it would decide a race the test controls (ThinkFlowLab#821) — and now keeps going past `rounds` until `Date.now() + REAL_WAIT_MS` (the deadline the real branch already used). The round count is a floor under the budget, never a deadline; `Date.now()` is not frozen by `mock.timers.enable({ apis: ['setTimeout'] })`. - New case `a mocked-clock wait keeps a real-clock floor under its round budget (ThinkFlowLab#1192)`: mocks the clock, starts a 100 ms **real** timer (captured at import, before any case mocks the clock) and calls `waitFor(() => landed, 1)`. One round is microseconds, so without the floor it returns false long before the timer lands. Added wall time ~100 ms. ## What I ran At the tip (`b9f57ae1`, in a tree rebuilt from this branch's tarball, not my working copy): - `node --test test/follow-through.test.js` — **19/19**, 0 fail, 1.28 s - `node scripts/ds-bot-patches.mjs check` — `ok 10 patches apply cleanly in order` / `ok core/ equals ds-bot/ + patches/ds-bot/` (pin skipped: no git here) - `oxlint -c core/.oxlintrc.json core/test/follow-through.test.js` — 0 warnings, 0 errors - `node .github/scripts/testing-layers.mjs --check` — 392 test files, every one classified - `node scripts/build-client.mjs --check` (in `core/`) — exit 0 - loaded run 1 exit=0 secs=77 # pass 19 # fail 0 ; loaded run 2 exit=0 secs=79 # pass 19 # fail 0 ; loaded run 3 exit=0 secs=83 # pass 19 # fail 0 ## Red without the source change Knife: in a copy of the tip, put the mocked loop back to `for (let i = 0; i < rounds && !check(); i++)` (the floor removed, `tick(1)` kept) → **18/19, only** `a mocked-clock wait keeps a real-clock floor under its round budget (ThinkFlowLab#1192)` red. Restore the floor → 19/19. No other case moves, so the other 18 prove the floor did not change what they decide (ThinkFlowLab#821's 3 s mocked delay still resolves by ticking). ## Not run - The `merge_group` entry itself — that is the queue's run, after this merges. A local `taskset` + hog cannot reproduce the I/O-slow / CPU-fast shape of a loaded runner; the measurement above shows the opposite direction and says so. - No changelog fragment (test-only) and no subsystem doc (no owned path). ## Risk - A genuinely lost call now fails after **5 s** on the mocked branch instead of ~70 ms, so a broken case is slower to fail, not quieter. The wait still ends on its own; there is no unbounded loop. - The floor only *adds* time to a wait that is already failing; every passing wait in the 19 leaves the loop through `check()`. - The patch hunk was regenerated by hand (this box has no `git`, so `generate` refuses); `check` reproduces `core/` byte-for-byte from `ds-bot/` + patches, which is the same proof it gives after a `generate`. Docs-not-needed: only `core/test/follow-through.test.js` and its generated patch hunk change; no owned path, no behavior or contract.
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: Dev2
Reviewer: Reviewer
Tip SHA: 36ba643
Base: fc19e6d (current main)
Fixes #1189
Reproduce
On
fc19e6d2,core/src/host/delivery-board.js:242builds the issue → pull request link withtext.matchAll(/#(\d+)/g)over the pull request's title and body, so a body that saysalso see #12makes issue #12 show a pull request that closes nothing.:128-130readscomments?per_page=100&page=1once, andverdictOf(:41) scans only those, so on a pull request with more than 100 comments — ordinary here (#1170, #1183) — the newestVerdict:is never read and the badge falls back tono verdict at this tip.Both are visible through the existing fake-fetch tests: give a pull request the body
also see #12and the card for #12 claims it; put the newest verdict on page two and the card saysno-verdict.What changed
core/src/host/delivery-board.js—closingLinks(): only GitHub's own closing keywords (close[sd]?,fix(?:es|ed)?,resolve[sd]?, case-insensitive, word boundary, optional colon) followed by#nlink a pull request to an issue (Delivery board:linkedTotreats any#12in a PR body as a link, andverdictOfonly reads the first 100 comments #1189).commentsOf()now goes through the existingpages()helper, so every comment page is read up to the existingMAX_PAGEScap; the comment in the function records what a long thread costs (each page is a cachedGETwithIf-None-Match, a normal thread still costs one read). Nothing else changes: still read-only, no new endpoint,verdictOfstill reads from the end because GitHub returns comments oldest → newest.core/test/delivery-board.test.js— two cases: a pull request whose body only mentions#10/#12leaves#10unlinked and#12on the pull request that closes it, whileCloses: #13links; and a 120-comment thread whose newestVerdict: INC at <tip>sits at index 101 decides the badge (inc) with page 2 actually requested.docs/subsystems/core-host.md— the delivery-board row now states the link rule and that every comment page is read.patches/ds-bot/files/src/host/delivery-board.js,patches/ds-bot/files/test/delivery-board.test.js— the mirrors, byte-identical.changelog.d/1189-delivery-board-links.fixed.md— user-visible (the board is in the UI).What I ran
cd core && node --import ./test/harness-resolve.mjs --test --test-concurrency=1 test/delivery-board.test.js test/delivery-view.test.js test/delivery-write-failure.test.js test/kanban.test.js test/bot-tasks.test.js→ 30/30, 0 failed.delivery-boardalone is 14/14 (base 12 + 2).-c core/.oxlintrc.jsonon the two changed files) → 0 warnings, 0 errors.node scripts/ds-bot-patches.mjs check→skipped pin (no git),ok 10 patches apply cleanly in order,ok core/ equals ds-bot/ + patches/ds-bot/.node .github/scripts/changelog.mjs check→57 fragment(s) … well formed.node core/scripts/build-client.mjs --check→ exit 0 (no client change).Red without the source change
closingLinksback to/#(\d+)/g→ 13/14, red only on "only a closing keyword links a pull request to an issue (Delivery board:linkedTotreats any#12in a PR body as a link, andverdictOfonly reads the first 100 comments #1189)".commentsOfback to?per_page=100&page=1→ 13/14, red only on "a verdict on a later page of comments still decides the badge (Delivery board:linkedTotreats any#12in a PR body as a link, andverdictOfonly reads the first 100 comments #1189)".Both restored by
cmpto the tip file, suite back to 14/14.Not run
npm run verifyend to end: itsarch-docsstep needsgitandorigin/main, and this box has nogit; the whole core suite is not runnable here either (a pristinefc19e6d2reports the same eight failures on this box). CI'stests-okis the terminal answer.fetch; nothing here touched the real API.Risk
MAX_PAGESreads of one URL instead of one — bounded, cached per URL with the ETag, and only open pull requests and issues reachcommentsOf.