Skip to content

fix(memory): forward-compatible migration stubs for v55-v58 - #8068

Closed
DaBlitzStein wants to merge 3 commits into
librefang:mainfrom
DaBlitzStein:fix/migration-forward-compat
Closed

DaBlitzStein wants to merge 3 commits into
librefang:mainfrom
DaBlitzStein:fix/migration-forward-compat

Conversation

@DaBlitzStein

Copy link
Copy Markdown
Contributor

Summary

  • Bump SCHEMA_VERSION from 54 to 58
  • Add idempotent migration stubs for v55-v58 so rollback from a v58 DB to a v54 binary does not fail on schema-version mismatch

Migrations added

Version DDL Guard
v55 template_versions + manifest_versions tables IF NOT EXISTS
v56 task_queue.timeout_secs + sessions.parent_session_id + index try_column_exists
v57 agents.parent_id + agents.parent_recorded + index try_column_exists (idempotent over v54)
v58 template_versions table IF NOT EXISTS (idempotent over v55)

Verification

  • cargo check -p librefang-memory passes clean

Closes #8066

Idempotent stubs so rolling back from a v58 DB to a v54 binary does
not fail on schema-version mismatch.

Closes librefang#8066
@github-actions github-actions Bot added the size/M 50-249 lines changed label Aug 31, 2026

@houko houko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot achieve what #8066 asks for, and merging it would make the four PRs whose schemas it guesses unshippable. Please close it and take the rollback problem head-on instead.

Blocking

1. The stated goal is unreachable from this side of the rollback.

#8066's scenario is: a newer binary applied v55-v58, the operator rolls back to a v54 binary, and that binary refuses to boot. Adding stubs to main changes what main's binary does; it cannot change what the already-shipped v54 binary does. That binary still has SCHEMA_VERSION = 54, still reads user_version = 58, and still hits the guard at migration.rs:16-28 — which is deliberate:

// Refuse to run if the DB was created by a newer binary. Silently
// downgrading `user_version` would corrupt v(N+1)+ columns/indexes.

If that policy should change, the change is to the guard (accept user_version > SCHEMA_VERSION when every unknown version is additive, or add a documented --allow-newer-schema escape hatch), not four speculative migrations. Either way it only helps binaries built after the change.

2. Pre-claiming 55-58 makes the real migrations unreachable.

run_step! is if current_version < $version (migration.rs:95-104). Once a DB has run these stubs it sits at user_version = 58, so when #8047 (template/manifest version history), #7991 (session parent lineage) and #7974 (per-task claim deadline) land with their own migrate_v55 / migrate_v56, none of them will ever run on that DB. The feature code then queries a schema that came from a guess in this PR. A silently-diverged schema with no error at any point is the one failure a migration ladder must never allow.

The guesses are already visible: v56 adds task_queue.timeout_secs, while #7974's subject is "per-task priority and claim deadline". If that PR names the column claim_deadline_secs — or stores an absolute timestamp instead of a duration — this leaves a permanent dead column and a missing real one.

The convention this repo already uses is the right one, and the migrate_v54 comment records it (migration.rs:276): each schema change ships in the PR that consumes it and takes the next free number at merge time. The 51-53 contention noted there resolved exactly that way.

3. v57 and v58 are copies of v54 and v55.

migrate_v57 is migrate_v54 line for line — same two ALTER TABLE agents guarded by try_column_exists, same idx_agents_parent_id. migrate_v58 is the first half of the migrate_v55 added in this same diff. Two version numbers are burned to re-run DDL that has already run in the same run_migrations call. Even granting the premise, these two do nothing.

4. manifest_versions breaks the agent-delete cascade — CI is red on it.

assertion `left == right` failed: cascade missed target rows in agent-keyed table 'manifest_versions' (col=agent_id)
— add a `DELETE FROM manifest_versions WHERE agent_id = ?1` line in execute_structured_agent_deletes

(structured::tests::agent_cascade_purges_every_agent_keyed_table, structured.rs:1690)

Deleting an agent would leave its entire manifest history behind — and a manifest carries workspace paths, the env var names holding provider credentials, command allowlists and system-prompt text. That guard exists to stop precisely this, and it is worth noting #7986 ("purge every trace of an agent") is in flight against the same invariant. Any PR creating manifest_versions owns the cascade line, which is a further argument for the table arriving with #8047 rather than here.

5. v56 assumes tables exist that a ladder step must not assume.

conn.execute("CREATE INDEX IF NOT EXISTS idx_sessions_parent ON sessions(parent_session_id) WHERE parent_session_id IS NOT NULL", [])?;

IF NOT EXISTS guards the index, not the table: this hard-errors when sessions is absent, and try_column_exists(conn, "task_queue", …) has the same exposure. That is the second CI failure — v54_marks_pre_existing_agent_rows_as_lineage_unknown (migration.rs:4157) builds a pre-v54 fixture holding only agents, memories, group_roster and migrations, and run_migrations(&conn).unwrap() now panics inside v56. migrate_v54 guards every ALTER for exactly this reason; a step that reaches a table it did not create has to check the table too.

If the rollback problem is real

Say what actually happened — which build produced a v55+ DB, and what the operator had to do — and fix it where it lives:

  • Relax the guard at migration.rs:16-28 to tolerate a newer user_version when the binary can prove the extra versions are additive, and cover it with a test that opens a v58 DB with SCHEMA_VERSION = 54 and boots.
  • Or document the supported recovery (restore from backup) and close #8066, since the message at migration.rs:24-26 already tells the operator to do that.

Both are one focused change and neither reserves numbers that belong to other PRs.

@DaBlitzStein

Copy link
Copy Markdown
Contributor Author

Closing per review. This approach cannot achieve #8066: a rollback to an already-shipped v54 binary cannot be fixed by changing main's binary, which still has SCHEMA_VERSION=54 and still hits the guard at migration.rs:16-28. Pre-claiming v55–v58 also makes the real migrations in #8047/#7991/#7974 unreachable (run_step! is version-gated), v57/v58 are line-for-line copies of v54/v55, and manifest_versions breaks the agent-delete cascade. The rollback problem should be fixed at the guard itself or documented as restore-from-backup, not via speculative stubs.

DaBlitzStein added a commit to DaBlitzStein/librefang that referenced this pull request Sep 2, 2026
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.
houko added a commit that referenced this pull request Sep 10, 2026
…8148)

* docs(operations): document the sanctioned binary-downgrade recovery

Issue #8066 asked for forward-compatible migration stubs so an older binary could boot a database a newer binary had already migrated.
The stub approach was closed (#8068): a stub claims a ladder version, run_step! skips the real migration behind that number, and the schema diverges silently.
The fail-loud guard that fires instead is the fix that already landed upstream (#3962, closing #3656, which documented the pre-guard silent corruption).
What was missing was the operator-facing half: what the refusal means and what the supported way back is.
Adds docs/operations/downgrade-recovery.md: the guard's verbatim error, why it refuses by design, what the backup feature covers (BACKUP_LAYOUT components including the SQLite databases under data/, minus the -shm index sidecar), the rollback procedure (restore the pre-upgrade archive, then restart on the old binary), the manual unzip fallback for a down daemon, and the honest no-backup options.
Carries a changelog fragment under changelog.d/documentation/.

* docs(changelog): reference PR 8148 in the downgrade-recovery fragment

* docs: make the downgrade rollback an offline restore

Review of #8148 flagged an unflagged hazard window between steps 3 and 4 of the sanctioned procedure: `POST /api/restore` ran inside a live daemon holding an open pool on `data/librefang.db`, and the archive carries that database's `-wal` (`is_sqlite_shared_memory_index` excludes the `-shm` and nothing else), so the restore replaced both files under connections still mapped to them and the daemon could checkpoint its stale WAL over the restored database.

Promote what was the "if the daemon is down" fallback to the recommended path — stop every daemon process, unzip the archive over the home directory, delete the leftover `-shm`, start the older binary — so no live daemon exists at any point.
The endpoint keeps a section of its own explaining why it is the wrong tool here, and what to do when it is the only way in.

The old fallback claimed unzipping was "the same write the endpoint performs, minus the `-shm` filtering", which was wrong: `restore_root` also re-roots the `agents/` prefix onto the agent workspaces directory, and extracting it to `<home_dir>/agents/` strands the archived workspaces in the legacy layout.
Both corrections are now spelled out, since the manual path is the sanctioned one.

Also add the missing trailing newline to the document and its changelog fragment.

* docs(operations): fix four wrong assumptions in the downgrade procedure

Review of #8148 found the page asserts things that are only true on a
default layout, and quotes an error line the daemon never prints.

- The whole procedure assumed the SQLite databases sit under
  `<home_dir>/data/`. Both `data_dir` and `[memory] sqlite_path` move
  them, while `backup_source` hard-codes `home_dir.join("data")`, so on
  such a deployment the archive holds no database and the rollback is a
  no-op that still ends in the version refusal. Added an explicit
  precondition section and referenced it from step 3.
- The quoted guard message was the format string from `migration.rs`,
  not the journal line: `LibreFangError::memory` and `BootFailed` prefix
  it with `Memory init failed: Memory error: `. An ops page is used by
  grepping, so the quote now carries both prefixes and names the
  substring to search for.
- The `-shm` bullet said nothing about a stale `-wal`, which is the file
  that can silently undo the rollback: `unzip -o` cannot overwrite an
  entry the archive does not carry, and the newer binary's WAL frames
  replay page 1 (and `user_version`) back over the restored database.
- "Configuration, agent workspaces, skills and workflows survive" was
  false for `cron_jobs.json`, `hand_state.json` and `custom_models.json`,
  which `BACKUP_LAYOUT` names separately but which live inside the `data/`
  tree the "start fresh" option moves aside. They are plain JSON, so the
  page now says to copy them back out.

The changelog fragment advertised "the backup/restore feature the daemon
already ships", which reads as `POST /api/restore` — the one path the
page argues against. Reworded to name the offline restore.

---------

Co-authored-by: Evan <suzukaze.haduki@gmail.com>
DaBlitzStein added a commit to DaBlitzStein/librefang that referenced this pull request Sep 13, 2026
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.
DaBlitzStein added a commit to DaBlitzStein/librefang that referenced this pull request Sep 13, 2026
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.
houko added a commit that referenced this pull request Oct 8, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-changes Changes requested by reviewer size/M 50-249 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(memory): forward-compatible migration stubs for v55-v58

2 participants