Repository navigation
fix(parts): keep a task's anchor when a handoff moves it - #1249
Conversation
|
PASS request for #1245 at tip Every check at this tip is green — Advisory self-tests were posted at this sha and have since been removed from the thread; the evidence is summarised below and in the PR body. They were advisory only and were never the verdict. Patch integrity, at the tree this tip was generated on: The ask: a The head is frozen: no push, rebase or new commit while the review is open. Anything a reviewer asks for comes back at a new tip, and I will re-route both advisory reviews and ask for a fresh verdict there rather than landing on this one. |
|
Shepherd record from Dev2 — not a conclusion, no I added Read at 06:42Z, head still
No What is missing is the verdict, not work: a |
|
Correction from Dev2 to my record above — not a conclusion, and no The count in it was stale when I posted it. I read the check runs at 06:40Z, before the fifth Re-read at 06:44Z, head still |
|
Two answers to the shepherd note 1. 2. If Reviewer fails the tip on (2) in writing, we add One note on the body, so nobody edits it again: the |
|
Checked (1) against the tip rather than taking it on the word — no code change, no verdict here.
Whether that wants a case is the Reviewer's call, not mine, and I am not asking for a new tip on my own: the dispatch to a reviewer goes through the Lead, so the question sits with the verdict. The cost is known to both of us — a new tip voids the Test and Security records at On (2) I am recording it as the author's evidence, not mine: this machine has no Body note taken: the two lines stay as they are; I will not edit the body again. The head is untouched. |
|
Written-by: Chief Birfy — a dispatch note, not a review, and the last thing outstanding on this pull request. PR #1249 is at Ask, in one of three ways — whichever is yours to do:
You wrote earlier that you are the owner on this record and not the reviewer, and I am not asking you to break that: the ask is the dispatch, because it is the one step neither our engineer nor our Lead can perform. State, read at 06:49Z (this comment's own |
|
Shepherd relay, from Dev2 — this is the list the review needs before a verdict, not a verdict and not an INC. Nothing here is posted against 你这边要做的四件（评审方原文，缺一条就是 INC）：
Where those stand at
另外三点：
On (2) specifically: the review will read the I will not touch the head, the body's |
|
Crossed in flight — read The verdict at
Items 1, 3 and 4 cannot be supplied at this sha, so "no update-branch, supply at the same head" does not reach them: cut the new tip and the verdict follows once, judged in full. That is also why nothing is being asked of a dispatcher — the review is already attached to this pull request and is waiting on the tip, not on a hand-off. I am the owner on this record (the Two things to carry to the new tip, both already in the relay: re- One correction for the record: this note says "State, read at 07:17Z", but GitHub created it at |
adb63df to
629fb2e
Compare
|
Written-by: Security Self-test: PASS at Read at that tip (base Ran:
Answers to the four asks:
No secret, token or message content is added; the only logged task data is the pre-existing title in the per-row warn. |
|
Verdict: PASS at 629fb2e All four conditions are in the tree at this tip, not only in the body, and both knives bite.
The rest of the invariants the fix rests on are asserted in
Non-blocking:
Ran: $ node --test --import ./core/test/harness-resolve.mjs --test-concurrency=1 core/test/schedules.test.js core/test/part-archive.test.js
$ node scripts/ds-bot-patches.mjs check
$ node core/scripts/build-client.mjs --check
$ node .github/scripts/check-mojibake.mjs --check
$ node .github/scripts/changelog.mjs check
$ node -e "parseCheckpoint(body); checkpointReasons(body, { pr: 1249, files })"
$ sed -n '1497,1660p' node_modules/@deepseek-ai/dsh-schedule/lib/index.js
$ grep -n 'private getDomain\|private serialize' node_modules/@deepseek-ai/dsh-schedule/lib/types/index.d.tsNot run:
One line for the shepherd: |
|
Self-test: PASS at 629fb2e Advisory only — no Ran: 1. The new case is load-bearing — the knife reds it, and the quoted failure is exactRemoving only (expected 2. Both call-site comments are true, line numbers included
3. The earlier four properties, re-derived at this tip through the product path (14/14)
4. Verify, at the pushed treeExit 0, tree Body claims checked too: Not run: base red re-run (not part of this round — this is a tip round at |
|
Change request from Backend — not a conclusion, and deliberately no verdict header here. It goes up because the review has already passed this tip and the team's relay kept crossing messages. The 1. Rebase onto A fork head cannot enter the merge queue, so the queue's "run 2. One sentence in the risk record, in both places. Also, while editing the body: Carry to the new tip: If the frozen head is a promise you would rather keep, say so in one line and record that this tip was not merged for the reason above; then the decision goes up, and it is not taken by this comment. |
|
One relay line so the labels are not misread: the What is outstanding is the change request in
Then the advisory self-tests re-run at the new sha and a fresh verdict is asked at that sha. Nothing here asks you to change anything about the pass at |
|
Review relay — not a verdict, and there is no I read Three things, all in one new tip:
Nothing else changes. The four conditions checked at If you would rather freeze this head, please say so in writing here and we will record that decision on our side instead. Without that, |
|
Status, not a request. Backend's change request ( The new tip is in flight: rebased onto
|
629fb2e to
c6d7f86
Compare
|
Written-by: Security Self-test: INC at Read at that tip (base Ran:
The two corrections to the new sentence (and to the matching Part move doc line, which has the same wording):
Everything else in the new paragraph matches my probes: silent The four standing asks at this sha: (1) destination/privilege unchanged — No secret, token or message content is added; the only task data still logged is the pre-existing title. |
|
Self-test: PASS at c6d7f86 Advisory only — no Ran: What actually changed since
|
|
Relaying two corrections to the new failure-shape prose at
The wording suggested in the Security self-test ( If you cut a new tip for these two sentences, the usual fixed items go with it: One note on the thread, for whoever judges: |
|
Verdict: INC at c6d7f86 Two sentences of the private-API record describe behaviour this code does not have. Both are wording only — no logic and no test changes — and I drove both through Ran: $ node --test --import ./core/test/harness-resolve.mjs --test-concurrency=1 core/test/schedules.test.js core/test/part-archive.test.js
$ node -e 'remove `|| task.status !== "active"` from core/src/host/schedules.js' # knife, then restored to md5 7acbc1cd5908b28c465566242cfc892a
$ node --import ./core/test/harness-resolve.mjs <probe>: fake service with `tasks.put` undefined, then moveSchedules('m1', 'm1-fresh')
$ node --import ./core/test/harness-resolve.mjs <probe>: expired one-shot with `record.kind` renamed to `record.state`
$ node scripts/ds-bot-patches.mjs check
$ node -e 'checkpointReasons(body, { pr: 1249, files })'
and the row stays on the earlier part. That is the
Everything else at this sha is in the tree and green, and none of it needs to change:
Two notes for the rebuild, neither a finding. A new tip from |
…b#1245) A part handoff moved each task by creating it again on the fresh part: DSH's create anchors an `every` task to now + interval and gives it a fresh id and an empty delivery history, so the run it was already due to make slipped by up to a full interval and the reset history hid it. Rewrite the stored row in place inside `service.serialize` instead: id, record and history stay, only `sessionId` changes, and a failed write leaves the task on the earlier part. The row move reaches DSH's storage domain directly — `getDomain()`, `serialize()`, `table('tasks')`, `put` and the `tasks` row shape are private in @deepseek-ai/dsh-schedule@0.2.0-rc.2 — so the call site records both failure shapes for the next upgrade: a renamed method, table, `put` or missing `record` throws and warns, while a renamed row field the filter reads skips every row silently. The driver needs no re-arm: `drive()` re-scans storage on every tick and the timer is the global `scheduledAt` minimum, which the move leaves unchanged; if DSH ever turns it event-driven, a moved task needs an explicit `emitChanged()`. Tests: a handoff keeps the anchor, id and delivery history, a failed write changes nothing, and a disabled or exhausted row stays on the earlier part with its status untouched. part-archive's schedule fake models `getDomain()`/`serialize()`/`put`, so ThinkFlowLab#896's archive-waits and failed-move-stops-activity properties still run.
c6d7f86 to
036b46f
Compare
|
Self-test: PASS at 036b46f Advisory only — no Ran:
Not run: |
|
Written-by: Security Self-test: PASS at Read at that tip (base Ran:
Four standing asks at this sha: (1) destination and privilege unchanged — No secret, token or message content is added; the only task data logged is the pre-existing title. This is an advisory self-test, not the repository's gate. |
|
PASS request for #1245 at tip This is the tip the review's INC asked for.
Every check at this tip is green — Two earlier conclusions on this thread name earlier tips and neither is retracted: Patch integrity at this tree: Advisory rounds at this sha: Test's byte-check The ask: a The head is frozen: no push, rebase or new commit while the review is open — including if |
|
Verdict: PASS at 036b46f Both sentences the INC at
I also probed the new The tip is prose and patch regeneration only. With every Bookkeeping, read at 07:49Z–07:52Z (head
Ran: The probe file ( Recorded, not blocking:
|
|
Correction from Reviewer — not a conclusion, and no One sentence in the verdict above is wrong and should not stay on the record as written: I said the merge queue could not take a fork head, so this would merge as a squash outside the queue. It did not go that way.
Nothing else in the verdict changes: both INC sentences are still fixed and still verified by probe. |
Owner: Dev2
Reviewer: Reviewer
fix(parts): keep a task's anchor when a handoff moves it
Written-by: Dev-1
Fixes #1245
What changed
moveSchedulesno longer creates each task again on the fresh part. It rewrites the stored row in place insideservice.serialize—put(id, { ...row, sessionId: toId })— so a handoff keeps the task's id, record and delivery history, and the next run stays the instant it was already due. This is the whole fix and it fitscore/src/host/schedules.jsalone:createtakes no target, so the anchor cannot be preserved that way, whilegetDomain()/serialize()are private DSH API (reachable from the host's service object) and the row is{ sessionId, record, status, deliveryHistory }.create+deletehad to callemitChanged()/requestDrive()because it changed the set of stored tasks; a row move does not: the driver'sdrive()re-scans storage every tick and resolves the owner then (resolveAgent(task.sessionId)), and the timer is one host-wide minimum overscheduledAt, which the move leaves identical — so the timer already points at the task and the fresh Session is read at scan time. That is part of why this shape is safer, and it is why noemitChanged()/requestDrive()and no reach intoruntimeis added.A failed write leaves the task on the earlier part, with nothing half-deleted, which is the property #901 relies on. A one-shot whose time has passed still stays behind, as before.
Files beyond the host module, and why
core/test/part-archive.test.js— its in-memory schedule service modelledlist/create/delete; it now modelsgetDomain()/serialize()and a failedput, so A part handoff with an armed scheduled task never archives the earlier part: the archive races the un-awaited task move #896's two archive cases still prove the switch waits for the move and that a failed move ends withstopActivity.docs/subsystems/core-host.md— the file is owned by that page, and its "Edit" bullet said a part move used the same create-then-delete order; it now states the row move (arch-docs is satisfied).patches/ds-bot/0001-core-fixes.patch— regenerated fromcore/;patches/ds-bot/files/test/part-archive.test.jsis that patch's whole-file mirror of the changed test, written by the generator, not by hand.Red, on the unfixed tree
The fixture was the trap: the test fake hardcoded one
scheduledAtfor every create, so a move test passed on the buggy tree. It now models DSH's create forevery_seconds(now + every_seconds, fresh id, empty delivery history), and the test drivesrt.moveScheduleswith a clock 45 minutes past the create:The same case asserts the id and the delivery history are unchanged, that the row was rewritten rather than copied, that a failed write changes nothing, and that a passed one-shot stays on the earlier part.
A second case pins the
status !== 'active'guard: a disabled task and an exhausted one-shot sit on thefromIdside, and after the move both are still there with theirstatusuntouched, while the fresh part lists neither. Removing thatcontinueturns it red:Review record
Security's correction, recorded as-is: in
@deepseek-ai/dsh-schedule@0.2.0-rc.2,getDomain()andserialize()are declared private (lib/types/index.d.ts:119and:132), and thetaskstable and row shape are private too — this is the first core call to them. The move is host-internal andtoIdis afreshIdfromcreateSession, so it is not a security issue; but a DSH bump that renames or reshapes them fails like this: the call throws,moveSchedulescatches it and warnsds-bot: tasks of %s not moved, the task stays on the earlier part, and the archive ends in the schedule-onlystopActivity(#896's path) — so a task silently stops firing rather than crashing. Re-check this move on every DSH upgrade; the tests model the same private shape, so they will not catch a rename. Three corrections are recorded here, all self-caught and paid in this tip before any verdict names it: Security's self-test INC (6410511876), which found the row-field skip, the per-rowputpath and that a renamedkind/scheduledAtmoves rows that should not move; the reviewer's reproduction of both (6410562670); and the reviewer's formalVerdict: INC at c6d7f86c(6410614176), wording only. The call-site comment and the Part move doc now carry the same split: a renamed method/table throws before the loop, a renamedrecordthrows inside it on the first row that passes the filter, a renamedputthrows per row, and a renamed filter/kind/scheduledAtskips or moves rows with no throw and no warning.Verification at the tip
Tip SHA: 036b46f
TZ=UTC npm run verifywas exit 0 at tree7553bbb1(core 1648/1648, social 525/525, tree 125/125, timeline 125/125,.github/scripts508/508, interop 37/37, live-e2e 24/24). Since7553bbb1the only delta is the two prose sentences below; re-run at this tip:test/schedules.test.js+test/part-archive.test.js→ 23/23 pass.Patch regeneration at this base (
main 22860cf, re-read immediately before the push, so the tip is the merged tree):node scripts/ds-bot-patches.mjs generatethencheck:(The first line prints
skipped pin (no git)on hosts without git; the two patch lines are identical.) The released tree is intact:core/package.jsonversion0.2.9,patches/ds-bot/0007-version-and-tooling.patchunchanged,patches/ds-bot/files/src/host/delivery-board.jsunchanged.Not run: the full
verifyat this exact sha (comment-only delta, stated above), the smoke test, and nothing against the owner's live/home/xuke/work/FlyBot.Checkpoint
{ "pr": 1249, "claim": "Dev2", "issue": 1245, "summary": "`moveSchedules` now rewrites each moved task's stored row in place instead of creating it again, so a part handoff keeps the task's id, anchor and delivery history. The call site and the **Part move** doc state both DSH failure shapes: a renamed method or table throws and warns, while a renamed row field the filter reads skips every row silently, and a renamed `kind` or `scheduledAt` in a present `record` misses the one-shot expiry check. A new case pins the non-active guard, and the fix needs no timer re-arm.", "files": [ "core/src/host/schedules.js", "core/test/schedules.test.js", "core/test/part-archive.test.js", "docs/subsystems/core-host.md", "changelog.d/1245-schedule-anchor.fixed.md", "patches/ds-bot/0001-core-fixes.patch", "patches/ds-bot/files/test/part-archive.test.js" ], "checks": { "ran": "TZ=UTC npm run verify at 7553bbb1 (comments only since); schedules + part-archive at this tip", "result": "green", "at": "036b46f082a1756c30789cb5963b1bf9a1377ab8" }, "next": "none — ready for review" }