Repository navigation
Wait for the rollback, not 20 ms, in the failed part-move test - #1168
Merged
Merged
Conversation
The test fails the handoff flush on the fresh Session and then asserts the old part is still current after a fixed settle(). Under load the flush is still in flight at that point, the pointer already sits on the fresh part, and the assertion reads a state the move has not failed out of yet — the red on CI run 40149951373 (#1059). Wait for the rollback instead: the failure path archives the fresh part it never handed over, so the test watches that call and only then asserts. No clock or ordering assumption; a move that stopped rolling back now fails with "the condition did not hold within 2 s" instead of hanging. Fixes #1059
This was referenced Oct 25, 2026
core/ is generated from ds-bot/ plus patches, so the new wait has to live in the patch that adds this test; `ds-bot-patches check` now reports both lines ok again. Fixes #1059
Collaborator
Author
|
Reviewer: test-only,
Enqueuing it. |
This was referenced Oct 25, 2026
HugoXK
pushed a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 25, 2026
…ThinkFlowLab#398) (ThinkFlowLab#1171) Owner: Dev2 Reviewer: Reviewer Tip SHA: a16a38a Base: 7fe7149 (current main; rebuilt on it after ThinkFlowLab#1168 merged) Fixes ThinkFlowLab#398 ## Reproduce On `7fe7149a`, `node dev/live-e2e/run.mjs .dsh-test-live-e2e 3196` runs a bench whose home is the checkout's own `.dsh-test-live-e2e`: the Bot under test can rewrite `dev/live-e2e/fixtures/orders.csv`, the expected totals in `scenarios.mjs` or the report next to it, and a run that was tampered with still reports `pass`. Nothing in `dev/live-e2e` hashes anything, and `guardRun` only looks at the key, the endpoint and `~/.dsh`. The INC on ThinkFlowLab#1171 was one more reproduction of the same hole: `guardRun({ home: '.dsh-test-live-e2e', repoRoot: '/home/me/FlyBot' })` returned `problems: []` because the comparison was lexical, and `bench.mjs` put that relative home inside the checkout by construction. ## What changed - **`dev/live-e2e/integrity.mjs` (new)**: `treeManifest(root, dirs)` (sorted path → sha256), `changedPaths(before, after)`, `canonicalPath` (resolve + realpath of the deepest existing ancestor), `isInside(child, parent, cwd)` on canonical paths, and `benchHome({ name, root, userHome, env })`. - **`bench.mjs`**: the home is `$LIVE_E2E_HOME`, else `~/.cache/flybot-live-e2e/<name>` — **outside the checkout** — and a home that lands inside the checkout (typically `LIVE_E2E_HOME` pointing at one) is refused before anything starts. `status` prints the directory used. - **`run.mjs`**: the positional home and `--evidence` are resolved to canonical absolute paths before any guard runs, and the guard is called with them. - **`report.mjs`**: `OUTCOMES.tampered = 5`; `guardRun` refuses a bench home inside the checkout, a home inside `~/.dsh` (now compared canonically, so a relative value or a symlink cannot pass), and an `--evidence` directory under `dev/live-e2e/` (that run would hash its own report writes as tampering); `integrityOutcome({ outcome, findings, changed })` is the run's last word. - **`.github/workflows/live-e2e.yml`**: the tap, the runner, the stop step and the log tail all use `$LIVE_E2E_HOME` (`$HOME/.cache/flybot-live-e2e/$BENCH`), published through `$GITHUB_ENV`, instead of `.dsh-test-$BENCH` inside the checkout. Without this the nightly would be refused by the new guard. - **Docs**: `dev/live-e2e/README.md` carries the path → read/write table and the three commands with `LIVE_E2E_HOME`; `docs/subsystems/live-e2e.md` and `docs/interop/live-e2e-adapter.md` carry the invariant and the outcome. **The read half is not enforced here, and this PR says so instead of simulating it.** DSH's confining modes restrict file *effects* (`read-only` permits only required sinks such as `/dev/null`; `workspace-write` adds the workspace and a temp area — `@deepseek-ai/dsh-sandbox/lib/types/index.d.ts`), and neither denies a read. So the Bot can still read `dev/live-e2e/**`; moving the grader out of reach needs a deployment-level decision (separate user, container or machine), which is the DSH-side half of ThinkFlowLab#398. What is enforced here is the write half — the workspace itself is outside the checkout and tampering is fatal — plus tamper detection that makes the read-then-write path visible. ## What I ran - `node --test dev/live-e2e/test/*.test.mjs` (keyless): **24/24, 0 failed** (base 16 + 8 here). - oxlint (`-c core/.oxlintrc.json dev/live-e2e scripts/lint-live-e2e.mjs`): **0 warnings, 0 errors**. - `node --test .github/scripts/test-workflow.test.mjs` after the workflow edit: **20/20**. - `node --check dev/live-e2e/{run,bench,report,integrity}.mjs`: clean. ## Red without the source change - `integrityOutcome` returns the checks' outcome without looking at `changed` → **23/24**, red on "the tamper rule has the last word … (ThinkFlowLab#398)". - the checkout refusal in `guardRun` is removed → **21/24**, red on all three cases that assert it (absolute home, relative home, symlinked home) — one guard, one assertion per case. - `canonicalPath` loses `realpathSync` (keeps `resolve`) → **23/24**, red only on "a symlinked path cannot read as outside the checkout (ThinkFlowLab#398)". - `canonicalPath` returns the value unchanged (the shape the INC reported) → **21/24**, red on the relative-home case, the `LIVE_E2E_HOME`-inside-the-checkout case and the symlink case; each keeps its own assertion, and the absolute cases stay green, which is exactly why the guard looked fine before. - `benchHome` defaults back to `<repo>/.dsh-test-<name>` → **23/24**, red only on "the bench home defaults outside the checkout …". - the evidence refusal is removed → **23/24**, red only on "an evidence directory inside the grader is refused …". Every knife reds its own cases and nothing else; each restore is back to 24/24. ## Not run - The live run itself (real key, browser, DSH checkout): no key here, and the live command is never part of `verify`. - `.github/workflows/live-e2e.yml` on a runner: I could not execute it here (no key, no Actions). The edit only replaces the directory string; `test-workflow.test.mjs` still passes and the workflow parses in that suite's reader. - The wiring in `run.mjs` (resolve home/evidence, pass them to the guard, call `integrityOutcome` at the end) is read, not tested: `run.mjs` executes at import and needs a bench, so the rules are tested through the pure functions. - `npm run verify` end to end: no git here (`arch-docs` needs `git log`) and no jsdom/fixtures for the core suite; CI is the reference. ## Risk - Low. A correct setup never hits the new refusals: the README and the CI job now both keep the home outside the checkout, and the old documented shape (`<repo>/.dsh-test-*`) was itself the hazard. A live run walks `dev/live-e2e` twice (12 files) and nothing in a run writes there. A false `tampered` costs one nightly run, never a merge.
HugoXK
pushed a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 25, 2026
…hinkFlowLab#1186) Owner: Dev4 Reviewer: Reviewer2 Base: bb96cc8 Tip SHA: b7a0e29 Written-by: Dev4 Fixes ThinkFlowLab#1169 **Reproduce.** On `bb96cc8f`, with `TMPDIR=/data/dsh/bot/workspace/tmp` (inside `DSH_HOME=/data/dsh`), `node --test test/part-archive.test.js` fails **3/3** in one run, every case with `error: 'until: the condition did not hold within 2 s'`. The reason is in the answer nobody read: `update-bot` had replied `{"ok":false,"error":{"code":"bot/refused","message":"…/project holds DeepSeek Harness data, so Bots cannot work there"}}`, because the test's project folder is made under `tmpdir()`, which then sits inside the harness home. With `TMPDIR=/tmp` the same tree is 3/3 green — the guard is right, the test just hid it (ThinkFlowLab#1059 was opened on this reading). **What changed** (test only, two files, no product behaviour): - The three `ThinkFlowLab#896` cases now take the answer first: `const answer = await h.call('update-bot', …)`, `assert.equal(answer.ok, true, answer.error?.message)`, and only then `until(…)` on the session id. - One new deterministic case, `ThinkFlowLab#1169: a folder under the harness home refuses the move and names itself`: `workDir: h.home` (exposed by `start()`) asserts `ok === false`, `error.code === 'bot/refused'`, the message names that folder and says `holds DeepSeek Harness data`, the Bot stays in its own chat and no part is created. It does not depend on where `TMPDIR` is, so its knife reds in CI too. - The `start()` header records that `TMPDIR` must stay outside `DSH_HOME` for these cases, and points at the refusal by name. - `patches/ds-bot/files/test/part-archive.test.js` is the same file byte for byte. No changelog fragment: T1 test-only, as `ThinkFlowLab#1168`. **What I ran** at `b7a0e2933db57fe138d4107cc6a71ee59b6b1bc3` (Node 22.23.3, `TMPDIR=/tmp DSH_HARNESS=/opt/flybot TZ=UTC`): - `node --test test/part-archive.test.js` → **4/4**, no skips. - the same file with `TMPDIR=/data/dsh/bot/workspace/tmp` → **1/4**: the three cases now fail with `…/project holds DeepSeek Harness data, so Bots cannot work there` (the refusal names itself, exactly what the issue asks for) and the new case still passes, because it names its own folder. - `node scripts/ds-bot-patches.mjs check` → `ok 10 patches apply cleanly in order`, `ok core/ equals ds-bot/ + patches/ds-bot/` (the pin line skips here: no `git` on this box, same as on main). - before the change, same `TMPDIR` inside `DSH_HOME`: **0/3**, all three on the 2 s timeout above. **Red without the source change.** - New case's `workDir: h.home` → `h.project` → **3/4**, red only `ThinkFlowLab#1169: a folder under the harness home refuses the move and names itself`. Restore → 4/4. - The three assertions are the diagnostic, not a new assertion of product behaviour: with `TMPDIR` inside `DSH_HOME` they turn the 2 s timeout into the guard's own sentence (the run above); there is no second code path for a knife to red. **Not run.** Full `npm run verify` and `npm run lint` (no `git` here, so `arch-docs` cannot diff; `oxlint` is not installed on this box, `npm run lint` stops at "no oxlint found") — CI's `tests-ok` is the reference for the core suite and lint. No smoke test: nothing under `core/index.js` or `core/src/host/` changed. No docs change needed: `core/test/part-archive.test.js` is in no subsystem's `owned_paths`, and the `ThinkFlowLab#896` behaviour it covers is already written down in `docs/subsystems/core-host.md`. **Risk.** Low and test-only. The new case hard-codes the guard's current sentence, so a future rewording of `folders.js` reds it — that is the point (the message is what a reader acts on), and it reds with the new text in hand. --- Routing: `Reviewer:` moved to Reviewer2 by Chief on 2026-10-25 so the queue runs on two gates — Reviewer holds ThinkFlowLab#1183 (the contested Delivery board), Reviewer2 takes this one. Code and tip untouched.
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: 24a4020
Tip SHA: f2249da
Written-by: Dev4
Fixes #1059
Reproduce.
core/test/work-dir.test.js:222fails the handoff flush on the fresh Session, then waits a fixedsettle()(20 ms) and asserts the old part is still current. Under load the flush is still in flight when the pause expires, so the pointer already sits on the fresh part andthe old part stays currentreds — the shape CI showed on run 40149951373. Reproduced deterministically by delaying that flush 250 ms: the old test fails with exactly that assertion, the new wait passes.What changed. The test waits for the rollback's own
archiveSessionon the fresh part (the failure path archives the part it never handed over) instead of sleeping. No clock or ordering assumption: passing depends on the event, and a move that stopped rolling back fails withuntil: the condition did not hold within 2 sinstead of hanging.settle()is still used by the other cases in this file. The change lives inpatches/ds-bot/0001-core-fixes.patchas well ascore/, becausecore/is generated from the patches.The three
#896part-archivecases in the same issue are not flaky and not a regression: they red only whenTMPDIRsits insideDSH_HOME(this box's usual/data/dsh/bot/workspace/tmp), wherecleanWorkDir's guard refuses a folder that holds DSH data andupdate-botanswersbot/refused; the test'suntil()turns that into a timeout. Counts are on the issue; no part-move code is touched here. Their opaque timeout is filed as #1169.What I ran at
f2249dadac01a610df3e90f5d5879ea27edc6bc4:cd core && TMPDIR=/tmp DSH_HARNESS=/opt/flybot TZ=UTC node --import ./test/harness-resolve.mjs --test test/work-dir.test.js→ 13/13; the#1059case alone 20 consecutive runs → 20/20.test/part-archive.test.js→ 3/3; its896cases five times on pristine24a40203withTMPDIR=/tmp→ 3/3 each time.node scripts/ds-bot-patches.mjs check→ok 10 patches apply cleanly in order,ok core/ equals ds-bot/ + patches/ds-bot/(the pin line skips here for lack ofgit, same on main).diff -rqagainst a clean24a40203tarball: only the two files in this PR.Red without the source change. With the flush delayed 250 ms, the pre-change test reds on
the old part stays current; the changed test passes the same delay. That is the knife: revert this patch and the CI race is back.Not run. Full
npm run verify(nonpm cihere) and the rest of thecoresuite; this diff touches one test and its patch. No changelog fragment: no user-visible change.Risk. None in product code: the change is the test's wait condition only.