Repository navigation
feat(memory): session parent lineage for sub-agent runs - #7991
Conversation
houko
left a comment
There was a problem hiding this comment.
parent_session_id is write-only as this stands, so none of the three things the changelog fragment promises actually work yet.
The column is never read back.
Every construction site on the load path hard-codes parent_session_id: None — SessionStore::load (session.rs:360), the second loader at session.rs:426, and session.rs:1251.
ephemeral_spawn.rs:383 is the one place that sets Some(parent.session_id), and the value it writes can never be observed again through Session.
Grepping the diff for the field outside : None returns exactly one assignment and zero reads.
"the parent can enumerate what it delegated" — there is no query for that.
idx_sessions_parent is created and nothing selects against it, so the index only costs writes.
"deleting the parent cascades to its children" — nothing cascades.
ALTER TABLE ... ADD COLUMN parent_session_id TEXT DEFAULT NULL adds a plain column with no foreign key, and SQLite cannot attach one via ALTER TABLE anyway; the delete paths are untouched in this diff.
Deleting a parent today leaves its children pointing at a row that is gone.
No round-trip test.
The change is 27 files of mechanical parent_session_id: None and no test saves a session with a parent and loads it back — which is precisely the assertion that would have failed and surfaced all of the above.
Suggested shape: add parent_session_id to the SELECT lists and parse it into the struct, add the WHERE parent_session_id = ? lookup the "enumerate" claim needs, make the delete path remove or re-parent children explicitly, and add one test that saves with a parent, reloads, and asserts the value survives.
If the intent was to land the schema first and the behaviour in a follow-up, the changelog fragment should say what actually ships in this PR — as written it describes capabilities that are not in the tree.
|
Migration version note: This PR claims |
f1b35e9 to
7fc8e82
Compare
… integration/deploy Session parent lineage: parent_session_id on sessions table, wired in ephemeral spawn and agent loop.
houko
left a comment
There was a problem hiding this comment.
The column is written but never read back, and neither capability the changelog promises exists yet. As it stands this adds a write-only column, an index nothing queries, and 85 parent_session_id: None lines across the test suite.
Blocking
1. The read path hardcodes None, so parentage can never be loaded.
The insert carries it:
"INSERT INTO sessions (…, peer_id, parent_session_id, created_at, updated_at) VALUES (…, ?9, ?10, ?10)",
…
session.parent_session_id.as_ref().map(|p| p.0.to_string()),but both hydration sites construct the struct with a literal:
Ok(Some(Session { id: session_id, agent_id, parent_session_id: None, … })) // session.rs:360
Session { id: session_id, agent_id, parent_session_id: None, … } // session.rs:426Neither SELECT lists the column. So ephemeral_spawn.rs:386 writes Some(parent.session_id), the row holds it, and every subsequent load returns None. This is the both-directions failure the repo has drift guards for elsewhere: a field registered on the write path only compiles fine and disables the feature.
Add the column to both SELECTs, map it back, and add a round-trip test — save a session with a parent, load it, assert the parent survives. That single test is what makes the rest of the diff worth its size.
2. Nothing implements the cascade the changelog and the doc comment promise.
"deleting the parent cascades to its children" —
changelog.d/added/7752-session-parent-lineage.md
"deleting the parent takes its children with it, so a disposable run can leave an audit trail without leaving an orphan" —session.rs:116-121
migrate_v55 adds a nullable column plus a partial index; there is no REFERENCES … ON DELETE CASCADE, and SQLite would need PRAGMA foreign_keys=ON even if there were. There is also no DELETE FROM sessions WHERE parent_session_id = ?1 anywhere in the diff. Either implement it (application-side, next to the other cascade statements) or reword both the fragment and the doc comment — right now they describe behaviour a reader will rely on and that does not happen.
3. Nothing can enumerate children either.
"the parent can enumerate what it delegated"
No children_of / list_children query exists, and idx_sessions_parent has no reader. Combined with #1 and #2, the feature has no observable behaviour. Consider whether this should land at all before the consumer does, or ship with the reader query and one integration test that spawns an ephemeral run and lists the parent's children.
4. SCHEMA_VERSION = 55 — a three-way collision.
#8047 (template_versions) and #8041 (manifest_versions) both also bump 54 → 55 and define a different migrate_v55. Because run_step! is if current_version < $version, the second and third to merge will silently no-op on any database that already ran the first, and their tables will simply not exist at runtime. Please settle an order across the three and have the later ones take 56 and 57. (#8068 additionally proposes stub migrations for 55-58 that would pre-empt all three; that PR should not land.)
Non-blocking
- The changelog fragment ends
(@DaBlitzStein)with no(#NNNN).scripts/check-changelog-attribution.pyonly enforces the(@user)half, so it passes — but perCLAUDE.mdthe generated- <PR title> (#N) (@author)line is suppressed only when the PR number appears in a curated bullet's trailing(#N)group. Without it this PR gets both the curated sentence and an auto-generated duplicate in the release body. - The
// parent_session_id is written on insert but deliberately NOT in the DO UPDATE clause: a session's parentage is decided when it is created and never changescomment is the right call, and worth keeping once #1 is fixed — it is the kind of decision the next reader would otherwise undo. - 85
parent_session_id: Nonelines in test fixtures is a lot of churn for one field. ASession::default()-plus-..Default::default()fixture helper (or#[derive(Default)]on the struct if the other fields allow) would keep the next field from costing another 85 lines.
|
Broadcast to the four PRs holding All four bump
The rulePer Suggested order, by how close each is to mergeable
Nothing about this order is binding; it is a default so the four of you are not each waiting on the other three. If a different one becomes ready first, it should take 55 and the rest shift. Related#8068 proposes forward-compatible stub migrations for v55-v58 and would pre-empt all four of these — a DB that ran those stubs sits at |
Four open PRs each claimed SCHEMA_VERSION = 55 with different bodies, so on any database that had already run one of them the other three steps never executed and the feature code hit a missing column. The ladder order settled for the four PRs is 55 = librefang#7991 (sessions.parent_session_id), 56 = librefang#8041 (agents.manifest_versions), 57 = this PR (task_queue.timeout_secs), 58 = librefang#8047 (template_versions). The two ladder guards that require every version 1..=user_version to have an audit row fail while 55/56 are held by the pending PRs; they go green once those land and this branch merges main again. Also documents in the task_post validation that a registered but stopped assignee is accepted on purpose, and adds the changelog fragment for the breaking change.
|
Status check while I worked through the review queue — this one is still waiting on you, and I would rather say so plainly than leave it looking like my turn. No commits of your own since my review on 2026-09-01T13:51:32Z; the only thing that moved the branch tip is a merge from Still open from that review:
Full reasoning and the file/line evidence for each is in that review. I re-read it before writing this and have not changed my mind on any of them. Separately, and this part is on me: the branch now conflicts with Not asking you to prioritise this over anything else. |
…romised cascade The column was write-only: all three load paths hard-coded parent_session_id: None, so a saved parentage could never be observed through Session. get_session, get_session_with_created_at, and find_session_by_label now list the column in their SELECTs and parse it back through parse_parent_session_id, which errors on a malformed stored value instead of silently re-hydrating it as unparented. children_of adds the reader idx_sessions_parent was created for, so the parent can enumerate what it delegated, and delete_session now cascades to children and grandchildren application-side — sessions rows and their FTS rows together, so deleting a parent leaves no orphan and nothing searchable behind. The cascade is application-side because SQLite cannot attach a foreign key to a column added via ALTER TABLE, and the recursion uses UNION rather than UNION ALL so a parentage cycle written by a foreign tool terminates. Adds three tests: a three-loader round-trip, the cascade (including FTS cleanup and an unrelated-session survivor), and children_of enumeration. The changelog fragment gains the (librefang#7991) trailing group the review flagged, so the release body does not double the curated bullet with an auto-generated duplicate. Refs: librefang#7752
|
Both blocking points addressed, plus the reader, on a fresh base (head
Two scoped-out notes for the record:
Verification on the merged state: |
After merging origin/main the branch ladder was 54 -> 55 -> 57 with a hole at 56, and the ladder guards require every version 1..=SCHEMA_VERSION to have a migrations audit row, so the hole fails them. Renumbered all four sites in one commit: run_step!, fn migrate_v57 -> migrate_v56, SCHEMA_VERSION, and the migrations audit row (the easy miss — migrate_v50 recorded itself as 49, librefang#7924/librefang#7925). Landing-order note: librefang#8041 (agents.manifest_versions) and librefang#7991 (sessions.parent_session_id) also claim v56 in their branches; whichever of the three lands second renumbers on its next rebase. Do not fill the hole with a stub migration — librefang#8068 proposed exactly that and should not land. Also updates the stuck-task sweeper doc comment in accessors.rs that referenced the old v57 numbering.
|
The four red checks were one root cause, now fixed: this branch adds Branch refreshed onto current Verification on the merged tip:
On point 4 (the three-way collision): the settled order is this PR at 56, with #8041 renumbering to 57 on its next push and #7974 taking the next free number after that. The changelog fragment now ends |
|
Heads-up on the migration number, with evidence rather than a preference: this branch needs to move from v56 to v57. The constraint is contiguity with
That is not theory. Running #8041's branch renumbered to 57 gives: Back at 56 (contiguous), both pass and the migration suite is 52/52. I checked both orderings rather than assuming, and #8041 is the one that has to sit at 56: it is already contiguous there, and its own guard tests are green. So the ladder is #8041 = 56 · this PR = 57 · #7974 = 58 · the #6504 recovery PR = 59. Being pushed first does not reserve the number — the tests do not care about push order, only about the gap. When renumbering, all four sites have to move together — Happy to do the renumber here if that is easier than picking it up on your side. |
houko
left a comment
There was a problem hiding this comment.
Re-reviewed at 46ff3d2e. All four blocking items are addressed, and two of them are implemented better than what I asked for. The write-only column is now a feature.
1 — the read path is real. Both hydration SELECTs list parent_session_id, parse_parent_session_id maps it back, and test_parent_session_id_round_trip pins it. That single test is what I said would make the rest of the diff worth its size, and it is there.
2 — the cascade exists, and it is a subtree rather than one level. I suggested a flat DELETE FROM sessions WHERE parent_session_id = ?1. You wrote a recursive CTE instead, which is the correct shape: a sub-agent that itself spawned a sub-agent would have been orphaned by my version.
Three things in it that I want to record, because each is a decision a later reader could undo:
- The FTS delete runs first, while the subtree is still intact. The comment states why: the recursive seed is
SELECT id FROM sessions WHERE id = ?1, so re-running the CTE after the sessions delete would seed nothing and leave every child's FTS row behind — #3548's privacy regression, in child form. Reordering these two statements looks harmless and is not. UNION, notUNION ALL, so a parentage cycle written by a foreign tool terminates instead of recursing forever. Defending against a cycle in a column this code is the only writer of is more care than the situation strictly demands, and it is the right call for aWITH RECURSIVEover user-visible data.- Application-side rather than a foreign key, with the reason given: v56 adds the column via
ALTER TABLE, which SQLite cannot back with an FK. That closes the question before someone "fixes" it withREFERENCES … ON DELETE CASCADE.
Both statements are in one transaction, and the error mappers moved with their statements when the order swapped, so an FTS failure still reports as an FTS failure and still rolls the cascade back.
3 — children_of exists with test_children_of_lists_delegated_children covering the populated case, a leaf, and an unknown id. idx_sessions_parent now has a reader.
So the changelog fragment's two promises — enumerate children, cascade on delete — are both true as written. I went looking for a fragment reword and did not need one, because the code came up to meet it rather than the other way round. The (#7991) is there now too, so the release body will carry the curated sentence without a generated duplicate.
4 — renumbered to 56. Which is the one thing still open, and it is not something this PR can fix alone.
The 55 collision became a 56 collision
Three PRs renumbered off 55 and all three landed on 56:
#8047 kept 55 and merged, so main is at 55 and 56 was legitimately the next free number for each of you independently. The collision reproduced one number up rather than being resolved.
It needs an order, not another renumber. Agree which merges first, then have the second and third move to 57 and 58 before they hit the conflict. Doing it during conflict resolution is the failure path: two branches adding the identical run_step!(56, migrate_v56); line conflict textually, the reflex resolution for a duplicate-looking line is to take one side, and that silently drops the other migration — after which run_step!'s current_version < $version guard means the column is never created on any database that already ran the other 56. It surfaces as a runtime "no such column" on upgraded installations only.
For this PR that failure would be particularly quiet: the column would be missing, parse_parent_session_id would never run, and the feature would look exactly like the write-only version I originally reviewed.
Approving. The remaining item is the ordering decision, which is the same on all three.
One non-blocking thing left from last time
The 85 parent_session_id: None fixture lines are still there. Not worth churn now that they are written, but the next field on Session will cost another 85 — a ..Default::default() fixture helper would be worth it at that point rather than this one.
|
@houko sobre la colisión de migraciones que levantaste el 14/09: confirmada, y he medido algo que cambia cuál es el arreglo. La colisión es real. Las dos ramas definen Cada una es consistente por separado, por eso las dos están verdes: Lo que no funciona es renumerar por adelantado. Lo intenté aquí: Con Así que esto se resuelve en el orden de merge, no en las ramas. La 62 solo es válida una vez que la 61 está en Mi propuesta, y me adapto a lo que prefieras:
Revertido mi cambio local — la rama sigue intacta en Un dato que puede interesarte: la rama de integración de mi fork ya lleva las dos a la vez, resueltas justamente así — |
|
Agreed — and settled the other way round from the one your 09-10 comment implied, so say if you would rather have it the other way. #7974 keeps 61. This one moves to 62, with The reason it went that way is the integration branch. Building it means merging both of these, and there the two Your 09-10 rule pointed the other way because it was about 56/57/58 and the tiebreak was which branch owed a rebase. Both of these owe one now, so that tiebreak no longer separates them, and I would rather match the thing that already exists in one place than have the PRs and the integration disagree. What changes here:
And the failure this avoids, which I had not appreciated before your sweep: Will push once refreshed onto current |
|
Correcting the agreement I posted an hour ago: the order flips. #7991 keeps 61; #7974 takes 62. I had it the other way, and an independent review of my own change found why that is worse rather than equivalent. Assigning 62 to a branch whose base stops at So this PR needs no renumber at all. It is at Two things the review caught in my attempt at the other order, worth recordingThe renumber I first pushed here was half done. I renamed And the merge of That is the same shape you warned about on #8230 — an overlapping hunk resolved in favour of the older side, silently, with the test that would have caught it also being the thing that changed. Worth knowing that it reproduces here without any conflict to notice. |
Two PRs in this integration each claimed `run_step!(61)` with a different body, so the merge had to become two steps and the assignment it picked was the inverse of the one settled in the PRs: 61 was `task_queue.timeout_secs` and 62 was `sessions.parent_session_id`. The agreement reached on librefang#7974 and librefang#7991 is the other way round — librefang#7991 keeps 61, which is contiguous against main's 60 today, and librefang#7974 takes 62 with the rebase it owes once librefang#7991 lands. Deploying the inverse would have written migration rows whose descriptions disagree with what main will produce for the same versions. The ladder stays 60, 61, 62 with no gap, and both steps are idempotent. It also makes `test_migrate_v61_is_a_noop_when_the_column_already_exists` coherent with its own subject: it asserts on `sessions.parent_session_id` and calls `migrate_v61` directly, which until now exercised the task_queue step while passing only because `run_migrations` had already added the column.
A sub-agent run now records which session spawned it. The parent can enumerate what it delegated, and deleting the parent cascades to its children — so a disposable run can leave an audit trail without leaving an orphan. Migration v55 adds the nullable column plus a partial index on non-NULL rows. The INSERT writes parent_session_id once; the ON CONFLICT DO UPDATE clause deliberately excludes it so a later save cannot silently reparent a run. Refs: librefang#7752
…test fixture The PR added migrate_v55 and run_step!(55) but left SCHEMA_VERSION at 54, causing four test failures: the downgrade guard rejected v55 as "newer than this binary supports", the phantom-audit test collided with a real v55 row, and the pre-v54 lineage fixture lacked the sessions table that v55 ALTERs.
…romised cascade The column was write-only: all three load paths hard-coded parent_session_id: None, so a saved parentage could never be observed through Session. get_session, get_session_with_created_at, and find_session_by_label now list the column in their SELECTs and parse it back through parse_parent_session_id, which errors on a malformed stored value instead of silently re-hydrating it as unparented. children_of adds the reader idx_sessions_parent was created for, so the parent can enumerate what it delegated, and delete_session now cascades to children and grandchildren application-side — sessions rows and their FTS rows together, so deleting a parent leaves no orphan and nothing searchable behind. The cascade is application-side because SQLite cannot attach a foreign key to a column added via ALTER TABLE, and the recursion uses UNION rather than UNION ALL so a parentage cycle written by a foreign tool terminates. Adds three tests: a three-loader round-trip, the cascade (including FTS cleanup and an unrelated-session survivor), and children_of enumeration. The changelog fragment gains the (librefang#7991) trailing group the review flagged, so the release body does not double the curated bullet with an auto-generated duplicate. Refs: librefang#7752
CI Gate reports failure because one job was cancelled rather than run, and it does not re-evaluate on its own. Re-running requires admin rights this account does not have, so an empty commit is the only lever.
…ssion_id The ephemeral spawn path was the only production writer of sessions.parent_session_id, and it wrote it onto a session that incognito=true guarantees is never persisted -- the value could never be observed outside a test that calls save_session by hand. The session construction is now a small testable helper that leaves the field None, with the reasoning pinned in a unit test at the construction site (no database round-trip can distinguish the two, since the write was always inert). The changelog entry is corrected to describe what actually shipped: the schema, cascade-delete and children_of query, with no persisted writer yet.
Four independent bugs in the librefang#7752 lineage cascade, none of which needed a production writer to reach: - delete_session seeded its recursive doomed-set from the sessions row the caller asked for, so a session whose row was already gone (a known, reconciled state) skipped its FTS cleanup and stayed searchable -- the seed is now the requested id unconditionally. - delete_session returns every id it actually removed instead of just the requested one, so a caller with per-session state to reclaim can act on the whole cascaded set. - delete_session_only is a new non-cascading primitive (the pre-librefang#7752 single-row delete) for callers that must not take a session's children down with it. - cleanup_expired_sessions and cleanup_excess_sessions delete parents by age or per-agent rank without expanding the subtree, which used to leave children pointing at a parent_session_id that no longer exists; both now reconcile dangling references to None in the same transaction, mirroring reconcile_fts_index's shape for exactly this class of drift. - parse_parent_session_id degrades a malformed stored value to None with a warn! instead of returning Err, matching model_override / peer_id -- every kernel caller of get_session collapsed Err into "session missing" and proceeded to overwrite the real message history with an empty one.
…at do
reset_session / reboot_session reused the cascading delete_session to
wipe the target row, so resetting a chat's own history silently deleted
every sub-agent session it had ever delegated to -- a "reset this one
chat" call taking an unrelated audit trail down with it. They now call
the new delete_session_only primitive, matching what the function's own
doc comment already promised: sibling sessions untouched.
DELETE /api/sessions/{id} keeps the cascade -- that is what the dashboard
delete affordance is for -- and its kernel-level counterpart reclaims
file_read_tracker for every id the cascade actually removed rather than
just the one requested, fixing a per-cascade leak. The route reports the
outcome as 200 {deleted_count, deleted_session_ids, ...} instead of a
bare 204, so a caller who asked to delete one id can tell a subtree came
down with it.
…e tests
Both new reconcile tests built the child session via create_session
followed by mutating parent_session_id and calling save_session a second
time -- which hits the INSERT's ON CONFLICT DO UPDATE branch, and that
clause deliberately excludes parent_session_id from the SET list (by
design: parentage is decided at creation and never changes). The second
save was therefore a no-op on that column, so the child's
parent_session_id was NULL from the very first insert rather than
reconciled to NULL by the fix under test -- both tests passed whether or
not reconcile_dangling_parent_ids was ever called.
Construct the child as a single Session literal saved once instead,
matching the pattern the cascade and round-trip tests next to it already
use. Verified against a manual revert of both reconcile call sites: the
corrected tests fail with the real message ("must be reconciled to None,
not left dangling"); the previous version of these two tests stayed
green through the same revert.
The branch was cut when main's ladder topped out at v54, so it claimed v56 for `sessions.parent_session_id` and set `SCHEMA_VERSION = 56`. Main has since landed v55 (`template_versions`), v56 (`workflow_runs.total_steps`) and v57 (the `memories_fts_au` WHEN guard), which makes that number wrong in two separate ways. The compile error is the obvious half: main already defines `migrate_v56` as the `workflow_runs.total_steps` ALTER, so merging this branch as it stood produced a duplicate function definition. The half that would have reached a deployment is `SCHEMA_VERSION`. Lowering it from 57 to 56 makes every database already at 57 refuse to boot with "Database schema version 57 is newer than this binary supports (56). Downgrade is not supported." We took that outage on 2026-09-04 from hand-numbered migrations in an integration branch; this is the same shape. v58 is the next free number above main's 57. Nothing else in this tree defines `migrate_v58`. The number stays contiguous rather than skipping ahead to reserve room for the other open schema PRs, and that is deliberate. `run_step!` gates on `current_version < N` with `current_version` read once at boot, so a database that reaches N through a binary with a gap below it will never run the skipped migrations — and the audit backfill at the end of `run_migrations` writes their rows anyway, so the skew is silent and permanent. Three other open PRs also want 58; whichever merges first keeps it and the rest renumber to 59, 60, 61 on rebase. Also here, because the renumber is what surfaced it: a no-op test for the re-run case. SQLite has no `ADD COLUMN IF NOT EXISTS`, so the existing `try_column_exists` guard is the only thing between a re-run and "duplicate column name: parent_session_id" on boot. Removing the guard makes the new test fail with exactly that message. A migration that will be renumbered before it merges is one that runs against databases which may already carry its DDL, so the guard needs a test rather than a comment.
…t 60 This branch takes the ladder to 61 and left three assertions reading `get_schema_version(&conn).unwrap(), 60`, so `cargo test -p librefang-memory` is red on the branch itself: - a_database_a_pre_release_build_stamped_at_59_still_opens - the_forward_compat_steps_are_no_ops_when_their_changes_are_already_present - a_database_stamped_past_58_without_manifest_versions_gets_it What each asserts is "and is carried the rest of the way", not "to exactly 60", so the constant is the honest right-hand side — and it is already the spelling the same file uses two hundred lines earlier. Written this way the next migration cannot make them stale again, here or in whatever branch lands alongside. That matters more than usual here: the comment above `run_step!(61, ..)` says outright that other open PRs also want 61 and the losers renumber. Also drops a stray second blank line after that `run_step!`, which was already on the branch and already failing `cargo fmt --check`. `cargo nextest run -p librefang-memory`: 477 passed, 0 failed. `rustfmt --edition 2021 --check`: clean.
… by the schema-61 sync Merging main (schema v61, librefang#7752's sessions.parent_session_id column and the matching Session::parent_session_id field) into this branch left two struct literals uninitialized, both added or left alone before that field existed: - crates/librefang-runtime/src/agent_loop/tests/integration.rs:2490, in test_max_tokens_pure_markup_overflow_replaced_with_honest_reply. - crates/librefang-kernel/src/kernel/tests.rs:14928, seeding the pinned session in hand_runtime_override_survives_restart_via_start_background_agents. Every other Session literal in both files, and in every other crate, already carries the field; these two were the only holdouts, confirmed by `cargo check --all-targets` on librefang-runtime, librefang-kernel, librefang-api, librefang-memory and xtask reporting zero remaining E0063 errors after this patch. This one E0063 was the sole failure behind all five red PR checks: Test / Unit (lib+bin), Quality, Test / Ubuntu (shard 1/4), Build / Linux aarch64 (all four hit the runtime literal) and Kernel Ignored / Restart Override (which hit the kernel literal) — confirmed by extracting each job's raw log via the Actions API and diffing the distinct rustc error lines across all five, which matched exactly these two locations and no others.
`9611b4433` pinned three post-migration assertions to `SCHEMA_VERSION` instead of the literal `60`, so that bumping the ladder could not leave them asserting yesterday's number. The merge of `main` resolved one of the three overlapping hunks in favour of the older side without producing a conflict marker — the shape the maintainer warned about for this family of branches — and `v60_reconciles_a_table_a_pre_release_build_created_with_fewer_columns` went back to asserting `60`. Restored. The other two occurrences still had `SCHEMA_VERSION`, and everything else that differs from `9611b4433` is new content main added since, all of it additions.
8a29c31 to
2bdb754
Compare
`OpenAPI Drift` fails on this branch and the whole diff is the recorded hash: `openapi.json` and the SDKs regenerate byte-identical, and only `xtask/baselines/openapi.sha256` disagrees with what the branch's routes now produce. The baseline was regenerated on the tree with `main` merged, which is the tree the job runs against, rather than resolved by side. A hash resolved by choosing "ours" or "theirs" describes a file that no longer exists after the merge, and the job then fails again for a reason that reads as unrelated (librefang#3300). The three commands from the job's own instructions were run — `cargo xtask codegen --openapi`, `python3 scripts/codegen-sdks.py`, `cargo xtask schema-check gen` — and re-running the last of them leaves the file unchanged. Refs librefang#7991
houko
left a comment
There was a problem hiding this comment.
Re-review at 4b0ea22.
Previous findings (my inline comments at 13fdac4, plus the migration-number thread):
- Resolved — ephemeral writer was inert.
new_ephemeral_session(crates/librefang-kernel/src/kernel/ephemeral_spawn.rs:95) now setsparent_session_id: None, and the changelog fragmentchangelog.d/added/7752-session-parent-lineage.mdsays outright that no production path writes the column yet. Choosing to drop the false claim rather than liftincognitois the right call for the #7744 reasons you gave. - Resolved — FTS delete seeded from
sessions. TheDOOMED_CTEseed isSELECT ?1 AS id(crates/librefang-memory/src/session.rs:764), pinned bytest_delete_session_removes_fts_row_when_sessions_row_already_gone. - Resolved — cascade invisible to the kernel.
LibreFangKernel::delete_session(session_ops.rs:248) loopsforget_sessionover the returned set, and the route reports it (delete_session_cascades_to_children_and_reports_them). Your pushback on JSONL /SessionEndfor the hard-delete path is correct:kernel_api.rsdocumentsdelete_sessionas the plain hard delete that never did either for the single session, so that is not a regression of this PR. - Resolved — reset cascading the subtree.
reset_one_sessionnow callsdelete_session_only(session_ops.rs:529), covered byresetting_a_session_does_not_cascade_delete_its_children. Your pushback onimport_sessionis also correct:SessionExport(session.rs:230) has no parent field and import mints a freshSessionId, so there is nothing to preserve. - Not resolved — reset de-parents a child session. The second half of that comment is still live; see the inline comment on
session_ops.rs:544. - Resolved — bulk cleanups leaving dangling parents.
reconcile_dangling_parent_idsruns inside both transactions, and NULL-and-keep is the better policy for retention cleanup than cascading a recently active child. - Resolved — malformed value costing the history.
parse_parent_session_iddegrades toNonewith awarn!, andtest_get_session_degrades_malformed_parent_session_id_to_noneasserts the two messages survive. - Migration number: this PR is correct, the collision is still live on #7974's side. Here
SCHEMA_VERSION = 61withrun_step!(61, migrate_v61)directly onorigin/main's 60, audit rowVALUES (61, …), contiguous. But #7974's head (8f01f15bc) also still ends atrun_step!(61, migrate_v61)withSCHEMA_VERSION = 61, so the agreed order (#7991 keeps 61, #7974 takes 62) has not been applied there yet. Whichever merges second will now fail #8355's contiguity guard rather than ship silently; the renumber belongs on #7974. #8041 and #8231 still stop at 60 on stale bases and will need the next free number after these two.
New findings in the changes since 13fdac4 are inline: one low on the reported delete set, one low on stale doc comments.
CI is green and the branch is CLEAN against main.
| let mut new_session = librefang_memory::session::Session { | ||
| id: sid, | ||
| agent_id, | ||
| parent_session_id: None, |
There was a problem hiding this comment.
medium — still open from last round. Switching to delete_session_only fixed the cascade half of this comment, but resetting a child session still permanently drops its lineage.
reset_one_session deletes the row at line 529 and then re-inserts at the same sid with parent_session_id: None here. Because save_session's ON CONFLICT DO UPDATE deliberately omits the column, no later save can restore it, so after one /new on a delegated session children_of(parent) no longer lists it and deleting the parent no longer cascades to it — the exact orphan the cascade exists to prevent.
It is latent today only because nothing writes the column yet; it becomes a real bug the day a writer lands, and nothing in the tests would catch it. old_session is already read under the lock at line ~470, so the fix is one line: parent_session_id: old_session.as_ref().and_then(|s| s.parent_session_id), plus a test that resets a child and asserts children_of(parent) still returns it.
| // Resolve the full doomed set BEFORE deleting anything, while the | ||
| // subtree is still there to walk, so it can be returned to the | ||
| // caller once the deletes below have committed. | ||
| let removed_ids: Vec<SessionId> = { |
There was a problem hiding this comment.
low — the reported set is not "every id actually removed". Because the seed is SELECT ?1 AS id unconditionally (correct for the FTS sweep), removed_ids always contains the requested id even when no sessions row existed. So DELETE /api/sessions/{random-uuid} — or a second delete of the same id — returns 200 {"deleted_count": 1, "deleted_session_ids": [<that id>]} for a no-op, which contradicts this function's doc comment, the SessionStore trait doc and the route doc added in this PR.
Keep ?1 in the seed for the two DELETEs, but resolve the reported set from rows that exist: {DOOMED_CTE} SELECT id FROM doomed WHERE id IN (SELECT id FROM sessions). While there, rows.flatten() at line 781 silently drops a row-read error, which would make the kernel skip forget_session for that id; collect::<Result<Vec<_>, _>>() with map_err(LibreFangError::memory) matches how the rest of this function reports errors. The route test would gain one assertion: deleting an unknown id reports deleted_count: 0.
|
|
||
| /// Parse the stored `parent_session_id` back into a `SessionId`. | ||
| /// | ||
| /// `None` for every ordinary session and for rows written before the column existed (v56 adds it NULL). |
There was a problem hiding this comment.
low — stale version number after the renumber. This says "v56 adds it NULL", and the delete_session doc at line 737 repeats "v56 adds the column via ALTER TABLE"; the migration is migrate_v61. The renumber sweep caught the code and tests but not these two comments, which is how a bisect of the ladder gets sent to the wrong step.
The field doc at line 119 has the same drift in a different form: "A sub-agent run hangs off the session that asked for it … the parent can enumerate what it delegated" describes a writer that, per the corrected changelog fragment, does not exist yet. Worth saying there that no production path sets it today, so the next reader does not assume children_of returns live data.
…m deadline (#7974) * feat(tasks): validate the assignee and enforce per-task priority and claim deadline - `task_post` now rejects an unknown `assigned_to` with `AgentNotFound` instead of silently storing a row nothing can claim. - `TaskPostOptions { priority, timeout_secs }` struct lets the API surface expose claim-queue ordering and a per-task TTL override. - `priority` orders the claim queue (higher first, ties by age). - `timeout_secs` overrides the global `claim_ttl_secs` for a single row: the stuck-task sweeper now reads `COALESCE(timeout_secs, ?)`. - Schema migration v55 adds the `timeout_secs` column (NULL = global). - Integration tests: unknown assignee → 400, priority round-trip, timeout_secs stored and retrievable, valid assignee succeeds. * chore: regenerate openapi.json, SDK codegen and schema baselines * fix(memory): guard v55 migration against missing task_queue table * fix(memory): give the v54 fixture test a task_queue table for v55 to touch `v54_marks_pre_existing_agent_rows_as_lineage_unknown` hand-rolls a pre-v54 database with only the tables v51/v52 touch. Migration v55 (#task-validation) now ALTERs `task_queue` unconditionally, and that table is normally created by `migrate_v1` — absent here because the fixture starts post-v1 with a partial schema. Add it, matching what a real database at user_version 50 actually has, and cover v55 itself with its own idempotency + populated-board tests. * refactor(memory): drop the table_exists guard from migrate_v55 task_queue is created unconditionally by migrate_v1, so every migration from v2 onward assumes it exists — none of the other run_step! functions check for their base table before ALTERing it. The guard papered over the actual gap, which was a hand-rolled test fixture missing the table (fixed separately); the production code path never needed it. * fix(memory): renumber the task-queue TTL migration to v57 Four open PRs each claimed SCHEMA_VERSION = 55 with different bodies, so on any database that had already run one of them the other three steps never executed and the feature code hit a missing column. The ladder order settled for the four PRs is 55 = #7991 (sessions.parent_session_id), 56 = #8041 (agents.manifest_versions), 57 = this PR (task_queue.timeout_secs), 58 = #8047 (template_versions). The two ladder guards that require every version 1..=user_version to have an audit row fail while 55/56 are held by the pending PRs; they go green once those land and this branch merges main again. Also documents in the task_post validation that a registered but stopped assignee is accepted on purpose, and adds the changelog fragment for the breaking change. * fix(memory): renumber the task-queue TTL migration to v56 After merging origin/main the branch ladder was 54 -> 55 -> 57 with a hole at 56, and the ladder guards require every version 1..=SCHEMA_VERSION to have a migrations audit row, so the hole fails them. Renumbered all four sites in one commit: run_step!, fn migrate_v57 -> migrate_v56, SCHEMA_VERSION, and the migrations audit row (the easy miss — migrate_v50 recorded itself as 49, #7924/#7925). Landing-order note: #8041 (agents.manifest_versions) and #7991 (sessions.parent_session_id) also claim v56 in their branches; whichever of the three lands second renumbers on its next rebase. Do not fill the hole with a stub migration — #8068 proposed exactly that and should not land. Also updates the stuck-task sweeper doc comment in accessors.rs that referenced the old v57 numbering. * fix(runtime): match the TaskQueue::task_post signature in the vision gate test double The merge with origin/main brought the trait's new sixth TaskPostOptions parameter, and the VisionKernel double still declared the five-parameter shape, so cargo clippy --workspace --all-targets failed with E0050 in the runtime's test build. Adds the unused-options parameter to the double; the body already returned a not-used error. * fix(tasks): the claim-ttl docs must tell the truth and the assignee picker must agree with the kernel Two follow-ups from the review, both on files this PR does not itself touch. The documented meaning of `claim_ttl_secs = 0` still read "disables the sweeper entirely" — the opposite of the semantics this PR implements. An operator running a human-in-the-loop board could set 0 on that promise, then post a task with `timeout_secs = 300` for an unrelated reason and watch the sweeper reclaim it from under the human. The struct doc, the field doc, the config reference table, the dashboard help strings in five locales, and the schema golden fixture now all say the same thing as accessors.rs: 0 disables the *global* clock, a task that declared its own timeout still expires. The new-task assignee picker was built from the `assigned_to` values of existing tasks, so it offered deleted agents (selecting one is a 400 under this PR's validation) and fell through to free text on an empty board, where every typo became a 400. It is now sourced from `GET /api/agents`, so the dropdown agrees with the registry the kernel checks. The filter dropdown still derives from the tasks themselves: filtering by a historical assignee is a read over rows that still exist, not a write the kernel can reject. * fix(api): return 400, not 500, for an unknown comms/task assignee POST /api/comms/task shared the task queue with POST /api/tasks but not its validation: it caught every kernel error with a generic 500 scrub and had no arm for AgentNotFound, so posting a task to an assignee that does not exist crashed the request instead of naming the offending field, the way the /api/tasks sibling route already does. * fix(memory): stop flooring the stuck-task sweep's claim age to a whole second task_reset_stuck compared claim age with strftime('%s', claimed_at), which truncates the fractional second on both sides of the comparison before doing the epoch arithmetic. A claim whose fractional second happened to land just past a whole-second boundary read as a full second older than it really was, which could reclaim an in_progress task from a worker that had barely started. Switched the comparison to julianday(), which keeps the fractional part. Also corrected the sweep-loop comment above the call site, which claimed the whole query was "a single indexed query" that "costs nothing" — only the status = 'in_progress' filter is covered by idx_task_status_claimed_at; the per-row deadline comparison runs on claimed_at directly and cannot use that index, which is fine only because the in_progress set it runs against stays small. * test(api): de-flake the per-task claim-TTL override test task_timeout_secs_overrides_the_global_claim_ttl raced a real 1s deadline: it asserted "not yet stuck" immediately after the claim with no margin, then slept 2.1s and asserted the reclaim. Any stall between the claim and that first assert — a loaded CI runner, SQLite lock contention — could tip real elapsed time past the 1s budget and fail the test on an assertion that has nothing to do with the behavior under test. Replaced both timing points with a direct claimed_at back-date (the same pattern already used elsewhere in this file), so the test no longer depends on the runner's real-time speed at all. * docs(memory): fix the migration range cited in the v54 test fixture comment The comment said "Steps 51-55 all fire from 50" and named migrate_v55 as the one that alters task_queue. v55 adds the template_versions table and never touches task_queue; it is v56 (per-task claim TTL) that adds task_queue's column, and it also fires from a user_version-50 fixture. Range and migration number both corrected to 51-56 / 56. * feat(dashboard): expose task priority and claim timeout in the New Task dialog tool_runner/task.rs already documented these as "operator controls... from the dashboard", and POST /api/tasks already accepted both, but CreateTaskPayload only carried title/description/assigned_to/created_by, so the dashboard itself had no way to set either. Added both fields to the New Task form and to the task card (shown only when set, to avoid noise on the common default board). * docs(changelog): add the fragment for the claim_ttl_secs=0 documentation fix The struct doc, field doc, config reference table, dashboard help strings and schema golden fixture were all corrected in 8f498f3 (this branch) to stop saying claim_ttl_secs = 0 disables the sweeper entirely, but no changelog fragment was written for a fix that changes what operators believe a production knob does. * fix(memory): the stuck-task sweep's regression test passed on the pre-fix query too test_task_reset_stuck_is_subsecond_precise_not_floored_to_a_second used a claimed_at fractional second of .999999999 to reproduce the strftime floor bug fixed in eda0965. SQLite does not truncate that fraction — it rounds to the nearest millisecond, half up, before truncating to a whole second (computeJD: p->iJD += ... + (s*1000 + 0.5)) — so .999999999 rounds up to the next second and happens to cancel the bug out. Verified against both the literal pre-fix and post-fix queries: the fixture reads "not stuck" on both, so reverting the fix would leave this test green. The fixture was also flaky by construction: it derived claimed_at from chrono::Utc::now().timestamp(), which discards the fractional second of the test's own clock read, leaving up to ~1s of uncontrolled slack against the sweep's own full-precision Utc::now() call. Replaced it with a fully deterministic test that checks the two literal boolean expressions (old floor-based comparison vs. the julianday one shipped today) against a fixed claimed_at/now pair, sidestepping the wall clock entirely. .999 sits under SQLite's .9995 rounding threshold and is verified to discriminate: the old expression flags the fixture as stuck, the new one does not. Also corrected the production comment above the query, which claimed the julianday arithmetic is "exact" (it is precise to the millisecond, not exact) and that strftime "truncates" the fraction (true only below .9995). * fix(api): document comms/task's real 201/400 responses in the OpenAPI contract The #[utoipa::path] block for comms_task declared only a 200, when the handler returns 201 on success (preexisting) and, since eda0965's sibling fix, 400 for an unresolvable assignee. Neither reached the generated spec, so a client generated from it would not know to expect either. Brought it in line with task_queue_post_root's equivalent block and regenerated openapi.json. * fix(dashboard): a zero-second timeout badge read as the opposite of what it means TaskCard showed the timeout badge whenever timeout_secs != null, which lets 0 through — and 0 means "never reclaim" per the sweep's own guard (COALESCE(timeout_secs, ?1) > 0) and the New Task modal's own placeholder text. A task posted with timeout_secs: 0 rendered "TTL 0s", which reads as "reclaimed immediately", the opposite of the guarantee it actually carries. Gave the zero case its own label instead of hiding it: an explicit "never reclaim" override is more worth surfacing to an operator than the absence of any override, not less. Also added the test coverage the priority/timeout dashboard wiring shipped without: the conditional payload spread (priority.trim() / timeout_secs.trim()) had nothing pinning it, so a change like `priority.trim() ?` -> `priority ?` would have started dropping the field from every submission silently, and the two new badges had no fixture exercising them at all. * docs(changelog): put the fragment attribution on the last sentence's line The (#7974) (@DaBlitzStein) tag sat on its own trailing line, which the repo convention rejects: attribution must close the bullet's last non-empty line, not sit as a separate one. check-changelog-attribution.py does not enforce this particular shape, so it merged without complaint. * fix(dashboard): the New Task assignee field could silently post a value the operator no longer saw The assignee field swapped between a free-text <input> and a <select> depending on whether the agent registry had loaded, gated on `agents.length > 0`. A value typed while the registry was still empty stayed in state after that swap: the <select> that replaced the input a moment later had no matching <option> for it, so it rendered as unselected while the component still held the typed value, and a submit right after sent it anyway. Replaced both widgets with a single free-text <input> backed by a <datalist> of registry suggestions, so there is no mode to swap and nothing to desync — typing a name outside the suggestion list still works exactly as it did before the registry loaded. That single input also fixes the two narrower gaps in what the suggestion list itself offered: `useAgents()` now passes `includeHands: true`, since GET /api/agents excludes hand agents by default and the kernel accepts them as assignees; and a non-admin caller is still scoped to their own agents by GET /api/agents (deliberately, per #6753's authorization boundary, and not something to relax here), so the same free-text path is what makes an agent authored by someone else still reachable by typing its name. Also named the endpoint's `limit: "500"` as `AGENT_LIST_LIMIT` rather than an inline literal repeated at every call site, documenting that it is a silent ceiling, not a page size — pagination itself is a separate change this does not make. * docs: the task_claim TTL docs still promised a single global number Two more places stated `[task_board] claim_ttl_secs` as *the* TTL for a claimed task, unqualified — true before this branch, false since a task can now carry its own `timeout_secs` override that wins over the global value. agent/tools/page.mdx (and its zh mirror) described task_claim's deadline this way; an agent reading it would not know that a task it claimed through the API or dashboard, rather than through task_post, might already carry a tighter deadline than the documented default. zh/configuration/core/page.mdx's claim_ttl_secs row was the Chinese mirror of the exact English row 8f498f3's docs pass corrected — the English side now says "0 disables the global clock only", the Chinese side still said "0 completely disables the sweep", left behind when the five dashboard locales and the English config page were updated but this mirror was not. * fix(api): POST /api/comms/task discarded the caller's priority and timeout_secs The route hardcoded TaskPostOptions::default(), so a client sending either field got a 201 for a task queued at priority 0 with no per-task deadline, and no way to tell it had been ignored. CommsTaskRequest now carries the same two controls as POST /api/tasks, typed rather than absent so a wrong-typed field is rejected by the deserializer instead of dropped. The stuck-task sweep logged its global TTL under the name ttl_secs while task_reset_stuck resolves the deadline per row via COALESCE(timeout_secs, ?1). With claim_ttl_secs = 0 and a task carrying timeout_secs = 300 that printed 0 next to a reclaim at 300s, pointing the operator at the one knob their config says is off. Renamed to global_ttl_secs and the message says which applied. * fix(memory): renumber the task-queue TTL migration to v58 The branch was cut when main's ladder topped out at v54 and has been renumbered twice since (55 to 57 to 56), each time against a main that moved again before it merged. Main now holds v55 (`template_versions`), v56 (`workflow_runs.total_steps`) and v57 (the `memories_fts_au` WHEN guard), so the current 56 is wrong in two separate ways. The compile error is the obvious half: main already defines `migrate_v56` as the `workflow_runs.total_steps` ALTER, so merging this branch as it stood produced a duplicate function definition. The half that would have reached a deployment is `SCHEMA_VERSION`. Lowering it from 57 to 56 makes every database already at 57 refuse to boot with "Database schema version 57 is newer than this binary supports (56). Downgrade is not supported." We took that outage on 2026-09-04 from hand-numbered migrations in an integration branch; this is the same shape. v58 is the next free number above main's 57. Nothing else in this tree defines `migrate_v58`. The number stays contiguous rather than skipping ahead to reserve room for the other open schema PRs, and that is deliberate. `run_step!` gates on `current_version < N` with `current_version` read once at boot, so a database that reaches N through a binary with a gap below it will never run the skipped migrations — and the audit backfill at the end of `run_migrations` writes their rows anyway, so the skew is silent and permanent. Three other open PRs also want 58; whichever merges first keeps it and the rest renumber to 59, 60, 61 on rebase. Also here, all of it surfaced by the rebase: - Dropped the stale ladder-order comment that claimed 55 = #7991, 56 = #8041, 57 = this PR, 58 = #8047. Main has since taken 55, 56 and 57 for unrelated migrations, so following it would reintroduce the collision. - `task_status_counts_agrees_with_listing_every_row`, a test main added under #8219, called `task_post` with the pre-PR four-argument form. This branch widens that signature with `priority` and `timeout_secs`, so the rebase left the crate not compiling. Passing `0, None` matches every other call site in the file and keeps the test's meaning — it counts statuses and cares about neither field. - Renamed the two existing migration tests to match the new number, and confirmed `migrate_v58_is_idempotent` still discriminates: removing the `try_column_exists` guard makes it fail with "duplicate column name: timeout_secs", which is what a boot would print. - Fixed the `pre-v56` comment in `kernel/accessors.rs` left behind by the previous renumber. * fix(dashboard): adopt the registry-backed assignee select on the task board Both this PR and `feat/dashboard-extensions` fix the same bug in the new-task assignee field, in incompatible ways, and integrating the two left seven failing tests in `TasksPage.test.tsx`. This adopts the other PR's shape so the two stop contending. The bug in main is `agents.length > 0 ? <select> : <input>`, a two-widget swap over a list derived from the `assigned_to` of tasks that already exist. A value typed while `agents` was still `[]` ended up under a dropdown that had since re-rendered empty; a brand-new agent was unreachable until someone had already assigned it something; a deleted one lingered forever. This PR fixed it with a single free-text input plus a `<datalist>` — one widget, so no mode to swap. That is a real fix for the swap, but it does not fix the stale list, and a name-valued control cannot survive a rename. The registry-backed `<select>`, valued by agent id, fixes all three, so it is the one to keep. The datalist is gone. What this PR still owns is untouched: `priority` and `timeout_secs` on the create payload and their two form fields, the badges on the task card, and the `task_queue.timeout_secs` migration at v58. One line deliberately differs from `feat/dashboard-extensions`: `useAgents({ includeHands: true })` rather than `useAgents()`. The kernel accepts hand agents as assignees, and now that the field is a select rather than a suggestion list, an agent the list omits is not merely unsuggested — it is unreachable. The other PR should take this too. Tests keep their intent rather than being deleted: - "keeps a typed assignee once the agent registry loads instead of losing it to a widget swap" becomes "keeps the selected agent id when the registry changes under it, including across a rename". The widget swap it guarded against cannot happen now — there is no second control — so the surviving guarantee is asserted instead: a chosen assignee is not lost when the registry changes beneath it. The rename half is new, and is the case the datalist could never have passed. Verified it discriminates: valuing the options by name instead of id turns it red. - "includes priority and timeout_secs in the payload when set" is unchanged and still discriminates — dropping the two spread entries from the payload builder turns it red. - The other five failures were resolved by the merge itself and needed no edits. Locale keys follow the widget: `assignee_none`, `no_agents_hint` and `field_description_hint` added to all five locales from the sibling branch, and `field_assignee_placeholder` removed from all five — it was the datalist input's placeholder and now has no referent. * fix(memory): renumber the task-queue TTL migration to v61 The v2026.9.14 release moved main's ladder from 57 to 60: v58 creates `manifest_versions`, v59 re-adds `workflow_runs.total_steps` and v60 re-creates `manifest_versions` for databases a pre-release build stamped past 58 without it. This branch's `task_queue.timeout_secs` step was sitting on 58, which main now occupies with different DDL, so it moves to 61 — the next free number — and `SCHEMA_VERSION` follows. The number stays contiguous rather than skipping ahead to reserve room for the other open schema PR. `run_step!` gates on `current_version < N` with `current_version` read once at boot, so a database that reaches N through a binary with a gap below it never runs the skipped migration, and the audit backfill at the end of `run_migrations` writes its row anyway — the skew is silent and self-concealing. `test_every_migration_records_audit_row` pins this: it asserts a fresh database needs zero backfill placeholders, so a ladder that jumped to 62 while leaving 61 empty would fail it. `try_column_exists` still guards the `ALTER TABLE`, which is what makes the renumber safe against instances that already carry the column from an integration build. Earlier commit messages on this branch still say "v58" where they describe this step; they record what was true when they were written. * test(memory): pin the post-migration assertions to SCHEMA_VERSION, not 60 This branch takes the ladder to 61 and left three assertions reading `get_schema_version(&conn).unwrap(), 60`, so `cargo test -p librefang-memory` is red on the branch itself: - a_database_a_pre_release_build_stamped_at_59_still_opens - the_forward_compat_steps_are_no_ops_when_their_changes_are_already_present - a_database_stamped_past_58_without_manifest_versions_gets_it What each asserts is "and is carried the rest of the way", not "to exactly 60", so the constant is the honest right-hand side — and it is already the spelling the same file uses two hundred lines earlier. Written this way the next migration cannot make them stale again. That matters here specifically: #7991 also claims 61, so whichever of the two lands second renumbers, and a literal would go stale a second time on the way in. `cargo nextest run -p librefang-memory`: 472 passed, 0 failed. * test(tasks): register the assignee labels the backpressure tests post to #7974 rejects an `assigned_to` that names no registered agent, which broke three tests that #8373 had already landed on main: `a_zero_cap_is_unlimited`, `per_agent_depth_cap_is_scoped_to_the_assignee` and `assignee_filter_narrows_both_the_page_and_the_total` all address their tasks to `alice` and `bob`, and the 400 discarded every one of those tasks before the queue saw them. Those names are buckets the assertions compare *across*, not agents anything in the file asserts about: the per-agent cap has to see one assignee saturated and another untouched, and the filter has to see two populations to narrow between. Registering them in the harness leaves every assertion and every test body untouched, which is the point — these are backpressure tests, and assignee resolution is not what they measure. `assignee_wake` is off on those two manifests because a default manifest can claim its own tasks: leaving it on would start a turn for every task posted here — 25 of them in the zero-cap test alone — against a provider CI does not have, and against a real one on a workstation running Ollama. * chore(memory): allow the argument count task_post reaches once main lands `task_post` takes one parameter per queue column, which puts it at clippy's seven-argument ceiling; the `caps` argument main already carries makes it eight, and the workspace denies the lint, so the method fails the gate on the merged tree even though this branch compiles clean on its own. A parameter struct would touch every caller in the API, kernel and runtime for a lint that four sibling methods in this file already silence, so this takes the same allowance they do. Verified with clippy-driver on both pinned toolchains: 7 parameters warn nothing, 8 warn "8/7", and the allowance silences the 8. * chore(codegen): refresh the openapi baseline hash after rebasing onto main The rebase conflict on xtask/baselines/openapi.sha256 was resolved with the incoming branch's stale hash, because at that point in the chain the tree does not yet compile and cargo xtask codegen cannot run. Regenerating from the rebased source at the tip produces a different hash, so the committed baseline now matches what the source actually generates. * fix(dashboard): drop the duplicated tail the merge left in the task locales Five keys at the end of the `tasks` object were duplicated (status_failed, assignee_none, no_agents_hint, field_description_hint) and the object then ended without a comma before the closing brace, so all five locale files were invalid JSON — tsc reports "',' expected" and the dashboard cannot build. The duplicated keys already exist earlier in the same object; removing the copies leaves the object ending exactly as main has it, with the seven keys this PR adds as pure additions. * style(memory): wrap the task_post call sites rustfmt wants multiline Resolving the rebase conflict left five task_post test call sites on one line; with seven arguments they exceed rustfmt's fn_call_width, so the pre-commit hook would reject any later commit touching this file. Formatting only: same arguments, same order. * fix(memory): pass priority and timeout_secs at the depth-cap test call sites The task queue's task_post grew from five arguments to seven in this PR, and the depth-cap tests that landed on main afterwards call it with five. Neither side conflicts with the other -- one adds calls, the other changes the signature -- so the merge left seventeen call sites that do not compile, with no conflict marker and no entry anywhere to show it. Inserts the same neutral 0, None the rest of the file already passes. Fix anchored on the compiler's own E0061 spans, so exactly the broken calls changed. * fix(kernel): pass opts at the depth-cap test call sites Same class as the memory ones: main's #8373 added these kernel-level task_post calls against the trait's old four-argument shape, and this PR makes opts the fifth. Nothing conflicts, so the merge left five calls that do not compile. Anchored on the compiler's five E0061 spans. * fix(memory): land the v61 tests on v61, and the last assertion on the schema it asserts `Test / Unit (lib+bin)` fails on this branch with one test: migration::tests::v60_reconciles_a_table_a_pre_release_build_created_with_fewer_columns assertion `left == right` failed left: 61 right: 60 Three of that fixture's four `get_schema_version` assertions were moved onto `SCHEMA_VERSION` when this branch raised it to 61; the fourth kept the literal, so the test asserts the version the branch left behind. Two more of the same shape turned up while looking at that one: - `migrate_v58_is_idempotent` called `migrate_v58` — the agent-manifest version-history migration — while its comment described replaying the step that re-adds `task_queue.timeout_secs`. It passed, and would have kept passing with that guard deleted, because it never reached the migration it named. It is `migrate_v61_is_idempotent` now and calls `migrate_v61`; with the `try_column_exists` guard removed it fails with `duplicate column name: timeout_secs`, which is the failure the guard exists to prevent. - The two new tests and their section comment said `v58`, which is a different and older migration — `migrate_v58` still numbers the manifest history table — so the name collided with a real step. They say `v61` now, which is the step they exercise. Refs #7974 * fix(dashboard): restrict task priority and timeout inputs to whole numbers The New Task dialog accepted fractional values in both fields. The server stores an integer (priority i64, timeout_secs u32) and answers 400, so the fraction could only ever come back as a form error the browser was already able to catch. step={1} states the contract on the input; min={0} on the timeout already matched the server's as_u64() rejection of negatives. * docs(api): document the 400 vs 422 contract on the task routes POST /api/comms/task deserializes a typed CommsTaskRequest, so a body that is valid JSON but the wrong type — a fractional priority or timeout_secs — is refused by the extractor as 422, while semantic mistakes (empty title, unknown assignee) stay the handler's 400. Sibling POST /api/tasks reads an untyped body and answers 400 for the same inputs. The utoipa block and the handler doc now state which side each route is on, and integration tests pin both ends: a decimal is a 400 there and a 422 here. * fix(memory): reconcile the real v61 on a database the pre-renumber build stamped at 61 A build that ran the timeout_secs step while it was numbered 61 left the database stamped at 61 with the column present — and therefore with sessions.parent_session_id absent, because run_step! gates on current_version < 61 and skips main's v61 for that stamp. The try_column_exists guard tolerated the rerun but not the missing column. migrate_v62 now applies migrate_v61 (idempotent) on that footprint and moves the audit row, so version 61 describes the step that now owns the number and the timeout history is re-recorded under 62. Covered by migrate_v62_reconciles_a_database_stamped_at_61_by_the_pre_renumber_build, checked red with the repair removed. * docs(kernel): make the sweep comment migration-number-agnostic "what every pre-v58 row means" survived the timeout_secs migration's renumber to v62; naming the column instead of a version keeps the next renumber from leaving the number behind again. * test(api): cover both edges of the per-task claim-ttl override Extend task_timeout_secs_overrides_the_global_claim_ttl: with claim_ttl_secs = 0 a row that carries its own timeout is still swept and a row that inherits the global clock is not; an explicit per-task timeout_secs = 0 is never reclaimed past a non-zero global TTL. Rewrite both #7974 changelog fragments to match the shipped behaviour: claim_ttl_secs = 0 is now global-only, and the task_post tool fails with a tool error (not an HTTP 400) while /api/comms/task gains the same AgentNotFound -> 400 arm as /api/tasks. --------- Co-authored-by: Evan <suzukaze.haduki@gmail.com>
Summary
parent_session_id: Option<SessionId>to theSessionstruct so a sub-agent run records which session spawned it.sessions.parent_session_idcolumn plus a partial index (WHERE parent_session_id IS NOT NULL).INSERTwritesparent_session_idonce on creation; theON CONFLICT DO UPDATEclause deliberately excludes it so a later save cannot silently reparent a run.ephemeral_spawn.rssetsparent_session_idto the spawning agent's current session, so the worker's session hangs off the conversation that asked for it.Complements
ephemeral_runs(#7904) — that table records the run metadata; this field lets the session itself know its parent for lineage queries and cascading deletes.Verification
cargo check --workspace --lib— cleancargo clippy -p librefang-memory -p librefang-types --all-targets -- -D warnings— zero warningscargo clippy -p librefang-kernel -p librefang-runtime -p librefang-api -p librefang-memory --all-targets -- -D warnings— zero warnings--all-targetscompile clean across kernel, runtime, api, memory cratesTest plan
CREATE INDEX IF NOT EXISTS)Refs: #7752