_start_channel built the default seen-event store path inline, and
Paths.base_dir resolves through realpath. Channel startup reaches that
wiring from POST /api/channels/{name}/restart, so every restart ran a
filesystem syscall on the Gateway event loop; the rest of the same
function already keeps its disk IO in a worker thread.
The existing wiring test stubs get_paths with a plain-string base_dir,
so it cannot see the syscall. The new anchor keeps the real Paths object
and isolates it with DEER_FLOW_HOME instead.
* feat(memory): add cognitiveStyle for collaboration preferences
Extend cross-session memory with user.cognitiveStyle and cognitive facts,
backward-compatible normalization, prompt injection as Thinking Style, and
settings UI. Add design notes and unit tests.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(memory): normalize legacy memory imports on frontend
Add normalizeMemoryPayload aligned with backend normalize_memory_data,
use it for Settings import and API reads, and add unit/regression tests.
Document schema-evolution steps and sync FACT_EXTRACTION_PROMPT cognitive category.
* fix(memory): harden legacy memory normalization
* fix(memory): keep import envelope validation strict
* fix(memory): keep unknown fields through frontend import normalization
normalizeMemoryPayload rebuilt the memory document from a section whitelist,
so every key it did not name was dropped. Because readMemoryResponse routes
all memory API responses through it, the loss showed up on read, export, and
import alike: revision vanished, and any future backend field would silently
break lossless export/import.
The Gateway models are extra="allow" and deermem normalizes additively, which
left the frontend as the only lossy hop in the chain. Spread before overriding
at each nesting level so normalization only validates and defaults
contract-owned fields, leaving the envelope check as strict as before.
Covered by a unit test on the strict import path, two backend tests for
extension preservation and caller immutability, and an E2E round-trip that
asserts on the captured import request body.
* docs(memory): state the normalization invariant that actually holds
The docstring cited `display` as a backend-driven field alongside
`revision`, but #4546 was closed unmerged and no such field exists in the
contract. It also promised lossless export/import, which the Gateway's
`extra="ignore"` models preclude for any undeclared field.
Replace both with the invariant the code is really defending: the frontend
must not be narrower than the Gateway contract, using the declared
top-level `revision` as the concrete example.
* fix(memory): preserve legacy migration and reject lossy imports
* fix(memory): align legacy fact normalization defaults
* test(memory): use authenticated app for import regression
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* fix(gateway): keep custom-skill rollback filesystem IO off the event loop
Closes#5747. Three blocking sites remained on the Gateway event loop in
the rollback route and its shared response reader, all offloaded through
asyncio.to_thread following the #5729 pattern:
- storage.validate_skill_markdown_content() creates a temp dir and writes
SKILL.md into it (site 1)
- the post-scan read of the file being replaced (site 2, named by #5729's
test docstring)
- _read_custom_skill_response(): load_skills() walks every skill directory
and read_custom_skill() opens SKILL.md — O(installed skills) loop time
paid per accepted rollback and per GET /skills/custom/{name} (site 3)
A blocking_io regression drives the full accepted-rollback path (past the
security scan) under the strict gate; the two pre-existing cases only
covered the pre-scan branches.
* fix(gateway): keep the skill element type in the response-parts closure
Review nit on #5751: tuple[object, ...] erased the element type for the
_skill_to_response call; Skill is already imported in this module.
* fix(authz): cover create/run/memory/agent routes with permission checks; scope USER.md per user
Route permissions: the AuthorizationProvider model only applied to routes
carrying @require_permission, so POST /api/threads, /api/threads/search,
POST /api/runs/stream and /api/runs/wait (runs:create), every /api/memory
route and the custom-agent routes ran with authentication only — a
provider configured to deny threads:write/runs:create/... could not
enforce those decisions. Add the missing decorators (threads:write,
threads:read, runs:create, memory:read/write, agents:read/write) and
register the new permission names in authz.Permissions. No owner checks
are added: per-user data scoping stays in the repository layer.
User profile: GET/PUT /api/user-profile read and wrote a single global
{base_dir}/USER.md, so in a multi-user deployment any authenticated user
could overwrite the prompt context injected for everyone else (and read
it). Scope the file to the caller's bucket
({base_dir}/users/{user_id}/USER.md) like user-scoped skills; no other
consumer of the old global path exists in the tree (verified by grep and
the updated tests). test_put_user_profile asserted the wrong effective
user under the autouse conftest user fixture; fixed to test-user-autouse.
* fix(authz): bind positional args in require_permission; migrate legacy USER.md
Review follow-up on #4989 (willem-bd):
- require_permission's wrapper only looked for request in keyword
arguments, so direct positional calls like
create_thread(body, request) made the injected keyword stub collide
with the positional request (TypeError: multiple values). The wrapper
now binds the wrapped signature via inspect.signature().bind() and
honors a positionally-passed request (and thread_id) instead of
assuming kwargs; the test-stub injection only fires when request is
absent everywhere. The two positional callers in
tests/test_threads_router.py pass again (8/8 channel tests).
- migrate_user_isolation.py now claims the legacy global USER.md for
--user-id (default 'default') like the other unowned legacy
artifacts; without it, upgrading installs with an existing profile
would read content: null and later strand the old file beside the
new per-user one. Conflict handling mirrors migrate_memory (rename to
USER.legacy.md).
* style(scripts): keep migrate_user_isolation help within the line budget
* test(authz): give internal-request stubs realistic auth fields
The create_thread permission wrapper added in this PR authenticates
the direct calls in the internal-owner tests; their SimpleNamespace
requests had state.user but no auth_source and no cookies, so
get_current_user_from_request fell through to request.cookies.get and
raised AttributeError (verified introduced by this branch: both tests
pass on the base commit).
The stubs now carry cookies={} and
state.auth_source=AUTH_SOURCE_INTERNAL, which is exactly what
AuthMiddleware stamps on real internal requests, so state.user is
honored without the JWT path. All 80 tests in the file pass.
* fix(pat): keep memory/agent permissions out of PAT scopes by design
The route permissions exist (they guard the memory/agent routers) but PATs
govern the thread/run lifecycle only: _PAT_ROUTE_RULES default-denies those
routers for PAT callers regardless of scopes. The alignment invariant
becomes a subset check plus a pinned exclusion, and pat.py documents that
opening these scopes is a product decision requiring three synchronized
changes.
* test/docs: cover migrate_user_profile; drop the unclaimed USER.md injection wording
Five-scenario test class mirroring the sibling migration steps (move,
conflict-rename, noop, both under dry-run). The GET/PUT descriptions no
longer say USER.md is 'injected into agents' — nothing at this head consumes
it for prompts; the routes are storage/retrieval only. The memory AGENTS.md
migration pointer now enumerates skills/ and the global USER.md.
* docs(paths): align the USER.md layout comment with the injection disclaimer
* test(authz): align effective permission contracts
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* fix(logging): collapse space-carrying Redirecting slots (#5225 round 16)
A Redirecting slot was kept verbatim whenever a scheme matched at
position 0, but the generic absolute-URL pass it is handed to stops
both its `host` and `rest` groups at whitespace. A Location header
value carrying an interior space (legal field syntax a misbehaving
server can emit, and the shape round 13 deliberately kept matching)
therefore survived in the clear after the first space:
'Redirecting /private/x?token=Q -> https://cdn.example/other page?sig=LeakedSig'
redacted to '... -> https://cdn.example/<redacted> page?sig=LeakedSig'.
The slot is now kept only when a whitespace-free absolute URL fills it,
so the pass is guaranteed to consume it whole; space-carrying slots
collapse like every other shape neither pass could cover.
* fix(logging): ask the URL pass whether it consumes a Redirecting slot whole (#5225 round 16 review)
The kept-set regex ([a-zA-Z...]*://\S+) was a stand-in for the invariant
"the generic absolute-URL pass consumes the slot whole", and two
whitespace-free absolute shapes satisfied it while the pass actually
stopped early: a quote that reads as a closing mark (https://h/a')b?sig=…)
and an empty host before the first /?# (https:///path?sig=…), which its
host group never matches. Both left the signed tail in the log.
Replace the stand-in with the check itself: run the pass's own pattern
against the slot and keep it only when the match spans it end to end, so
the rule cannot drift from the pattern's stop conditions again.
---------
Co-authored-by: sxh313 <sxh313@users.noreply.github.com>
The Gateway's staged upload commit links the `.part` file to its final name and
then unlinks the staged name. The conversion path duplicates a descriptor on the
staged inode *before* that commit (deliberately: it is what keeps conversion
reading the bytes this request wrote rather than whatever the committed name
points at afterwards), so the staged file still has an open handle when the
commit tries to remove it.
POSIX allows that; Windows does not. Uploading a convertible document
(`uploads.auto_convert_documents: true`, a PDF/DOCX/PPTX/XLSX upload) on a
Windows host therefore failed with a 500:
Failed to upload report.pdf: [WinError 32] The process cannot access the
file because it is being used by another process
Split publishing from removing the staged name: `_commit_upload_temp_no_overwrite`
now takes `unlink_staged`, and the ingestion service keeps ownership of the
staged path whenever it still holds the conversion descriptor, removing it once
that descriptor is released. The same best-effort removal replaces the
`os.unlink` in `_abort_upload_temp`, where an abandoned duplication worker can
hold the inode open and turn a cancellation into a secondary permission error.
Both removals are best-effort: a staged name that cannot be removed yet is
already hidden from every upload listing, and the Gateway sweeps leftovers on
startup.
Verified on Windows (Python 3.12):
- the new regression test reproduces the sharing violation portably (it pins the
staged name while the descriptor lives) and fails on main with the exact 500
above; it passes here
- `pytest tests/test_uploads_router.py tests/test_project_documents_promotion.py
tests/blocking_io/test_project_documents_promotion.py` -> 88 passed
(16 of those failed on main on this host)
- `ruff check` / `ruff format --check` clean
Co-authored-by: Shxiao101 <Shxiao101@users.noreply.github.com>
_receive_single_file dropped an attachment with zero log lines whenever
_download_by_code returned None (or empty bytes) - each None reason
already has its accurate line inside the download function (non-200
exchange, missing downloadUrl, oversize abort, transport failure), but
nothing tied the skip to the file being received, and the empty-bytes
case landed in the same silent branch. The caller now logs a neutral
guard line naming the file - the same shape the WeChat channel's callers
and the manager reader use since #5225.
Both the None and empty-bytes paths are pinned by caplog tests;
reverting to the silent branch turns them red. Full DingTalk suite
135 passed / 1 skipped.
* fix(skills): rollback reads its history off the event loop
rollback_custom_skill constructed the user-scoped storage, probed whether the
skill exists and parsed custom/.history/<name>.jsonl inline, while the sibling
get_custom_skill_history two lines above already offloads exactly those three
steps and documents them as "blocking filesystem IO that must stay off the
event loop". The history file grows one entry per edit carrying full previous
and new content, so a rollback request parsed the whole edit history on the
Gateway loop; the strict Blockbuster gate catches the construction as a blocking
os.getcwd() through SkillsConfig.get_skills_path() -> project_root().
Offload the three steps through the same worker-thread closure and anchor both
pre-scan branches under tests/blocking_io/.
* test(skills): address rollback anchor review feedback
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
backend/app/gateway/AGENTS.md reached 49,155 bytes against the
49,152-byte hard limit, so tests/test_agent_guidance_check.py fails on
main for every change, not just ones that touch the file.
This trims wording I added to the Uploads row in #5547, #5611 and #5673
rather than anyone else's documented invariants: the delete clause keeps
both facts — symlinks 404, a converted .md is kept — without restating
that the 404 matches GET /list.
That clears the overage with 13 bytes to spare, which is not much. The
file is effectively full, so the next addition needs a real slimming
pass or a budget decision.
* fix(channels): fix Telegram inbound file download and sandbox readability
- Shut down the download Bot on the Telegram loop (Bot.shutdown(), which
closes its HTTPX clients) before the loop stops, instead of the
non-existent Bot.session.close().
- Grant group/other read on channel-downloaded uploads so the non-root
AIO/Docker sandbox process can read the root-written 0o600 file.
- Apply the sandbox permission change with os.fchmod on a descriptor
opened with O_NOFOLLOW (validated as a regular file via fstat), bound
to the validated upload inode, so a symlink swapped in after lstat
cannot redirect the chmod to a target outside the uploads directory.
The open also uses O_NONBLOCK so a sandbox-swapped FIFO cannot block the
read-only open before the regular-file check (matching the existing
open_upload_file_no_symlink convention).
Centralized in a shared apply_upload_sandbox_permits helper reused by
the channel inbound path and the HTTP upload readable/writable helpers.
- Surface the download failure cause chain in logs with the Bot API URL
masked: the configured token is redacted and both URL forms are
collapsed, covering the file download URL (/file/bot<token>/...) and
the method URLs (/bot<token>/getMe, /bot<token>/getFile).
- Migrate the existing receive_file tests onto the download Bot and add
coverage for _get_download_bot (loop-bound creation + caching, cleanup
on init failure), download-bot routing over the application bot,
real-Bot shutdown closing both HTTPX clients, receive_file
timeout-containment, masked cause-chain logging (file and method URL
forms), and inbound-file sandbox perms (including the swap-after-lstat
symlink regression).
* fix(uploads): surface sandbox permission failures
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* Initial plan
* docs(gateway): trim AGENTS guidance to stay within hard budget
Co-authored-by: WillemJiang <219644+WillemJiang@users.noreply.github.com>
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: WillemJiang <219644+WillemJiang@users.noreply.github.com>
* fix: rebuild todo reminders after context compaction
* chore: remove implementation plan from PR
* refactor: share todo reminder message name
* docs: keep agent guidance within CI size budgets
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* fix(uploads): stop deleting a converted companion we cannot prove we wrote
Conversion names a document's Markdown companion after the document's
stem and falls back to a _N suffix when that name is already taken, so
the .md beside a document may belong to another document sharing the
stem, or to the user. Delete removed it anyway: uploading a.docx and
a.pdf produces a.md and a_1.md, and deleting a.pdf destroyed a.docx's
companion while orphaning a.pdf's own.
Delete now removes only the file it was asked to remove. The companion
stays listed and can be deleted on its own. Orphans are the cost of not
guessing; issue #5672 covers giving companions a provable owner, which
is what a safe cleanup needs, along with the two related readers that
still guess (the outline injected for a document and the agent's file
listing).
convertible_extensions loses its last use and is dropped from the
signature and both call sites. The gateway router keeps importing
CONVERTIBLE_EXTENSIONS for the ingestion bridge and now declares it in
__all__, where that module documents its re-exports.
* docs(changelog): note that delete keeps the converted markdown (#5673)
A Redirecting slot stayed verbatim unless it started with "/", but the
Location field-value grammar (RFC 3986 relative-part) also admits
slash-less relative references: 'Redirecting /private/x?token=Q ->
download?sign=LeakedSig' rendered the signed query verbatim, and neither
the slot rule nor the generic absolute-URL pass (which needs a scheme)
could see it. A slot is now kept only when a scheme matches at position
0 (left for the generic pass to rewrite); everything else collapses -
slash-less relative paths, query-only and fragment-only forms,
network-path references (collapsing any userinfo they carry), and
non-hierarchical schemes such as data: URIs. The ^Redirecting prefix
anchor is unchanged, so non-URL '-> /path' arrows (sandbox mount
mappings) keep passing through untouched.
Regression table covers all relative-reference forms plus the absolute
regression anchor; mutation-verified (reverting to startswith('/') goes
red). Aligned with the fix sketched in the #5225 round-15 review thread.
* fix(uploads): convert the bytes we wrote, not the name they landed under
Document conversion re-opened the upload by name after it was already
visible in the thread's uploads directory: the Gateway converted the
committed file_path, and DeerFlowClient converted the copy it had just
placed there. That directory is writable from local and AIO sandboxes,
so a process watching it can replace the name with a symlink in the
window between the upload landing and the converter opening it. The
converter then reads whatever host file the link points at and writes
that content back into the thread as the .md companion, which the
sandbox can read. Reproduced end to end on both paths with a real xlsx:
the companion came back holding the host file's rows.
The Gateway now duplicates the descriptor of the staged file before the
link-commit, copies those bytes into a private directory outside the
uploads tree, and converts there. A descriptor cannot be redirected by
replacing a name, so the conversion input is the content this request
wrote. The client converts the caller's own source file instead of the
copy in uploads; the source is the file the caller handed in, which the
sandbox cannot reach.
Both already wrote the companion without following a symlink, so only
the read side changes. The uploads directory still receives exactly the
same files.
* docs(changelog): note upload conversion source fix (#5611)
* fix(uploads): close the conversion descriptor when staging its copy fails
Review follow-up. The private directory for the conversion copy was
created before the try that owns the duplicated descriptor, so a failure
there — a full or unwritable temporary filesystem — propagated without
closing it. The upload's own cleanup only unlinks the committed name and
releases the sandbox lease, so the descriptor stayed open for the life of
the process and kept the unlinked staged bytes allocated with it; repeated
failures accumulated both.
Directory creation now happens inside that try, and the finally removes
the directory only once it exists.
* fix(uploads): keep the conversion descriptor owned across cancellation
Review follow-up. run_file_io cannot interrupt its worker, so cancelling
the await around os.dup only abandoned the result: the duplicate was
created moments later with nothing left to close it, and it pinned the
staged bytes of an upload whose name the cleanup had already unlinked.
Cancellation after the duplication was just as leaky, because the
commit-path handler caught Exception and CancelledError is not one.
The duplication now runs as its own task, shielded from the caller's
cancellation, and closes its own result when the caller is gone by the
time the worker finishes. The commit path catches BaseException, closing
the descriptor it already owns before re-raising.
Both windows are pinned: one test stalls the duplication worker after it
allocates and cancels ingestion, the other stalls the commit so the
cancellation lands while the descriptor is owned.
* fix(uploads): drain the conversion copy so its descriptor always closes
Review follow-up. The copy worker owns the duplicated descriptor and
closes it in its own finally, but a bare await let a cancellation cancel
the executor job while it was still queued: the worker never ran, so that
finally never ran either, and the enclosing scope had already handed
ownership away and saw None. Draining also keeps a late worker from
writing into a private directory this scope has since removed.
The copy now goes through await_drained, the shield-and-drain helper the
Gateway already uses for offloads that must not be abandoned mid-flight.
Pinned by a test that holds the copy job queued, cancels ingestion, then
releases it and requires the descriptor to come back closed.
* feat(mcp): re-scope to MCP task claim lifecycle only
Keep PR #4966 a small, closed MCP lease/cancellation state-machine change and
move RunJournal and Run lifecycle work into dedicated follow-ups. This branch
contains only the MCP task claim lifecycle:
- mcp task release/snapshot fencing by owner + per-claim lease token
- phase-level single-flight poll/cancel/notification owners with retained handoff
- routine cancellation no longer persisted as a task failure diagnostic
- bounded ordinary release ownership retention past the drain deadline
- 0018_mcp_task_lease_tokens migration + migration/bootstrap head assertions
- wait_for_task_until helper (MCP uses it); worker-specific capture helper moved
to the run-finalization follow-up
RunJournal (journal.py + test_run_journal.py) and run lifecycle
(manager/worker/store/run sql + run tests) are preserved on
backup/cancellation-safety-full and will be raised as separate follow-ups.
* fix(mcp): unblock claims after ambiguous handoff resolves
A phase-level single-flight owner only guards an ambiguous claim outcome. Once
the claim resolves, the phase owner is released immediately; the handoff may
continue releasing returned rows as bounded, service-owned background work
(transferred to _compensation_tasks on timeout). Per-claim token fencing rejects
a late release against a newer claim generation, so a stuck release no longer
locks the whole phase until process restart.
- README: drop the stale progress-snapshot sentence from the bounded ordinary
release description.
- service: pop the identity-checked phase owner as soon as the claim outcome is
known, then release returned rows with the bounded path; carry the release in
_compensation_tasks if it exceeds the drain deadline.
- mcp/AGENTS.md: document that only an unresolved claim outcome (not the handoff)
blocks later phase scans, and that returned-row releases may continue in the
background once the owner is released.
- tests: pin that the phase owner is released before a stuck release finishes
while the release stays service strong-owned.
* refactor(mcp): remove unused single-record claim wrappers
_poll_one, _cancel_one, and _notify_one are unreachable in production: the
worker always processes claimed records through _run_claimed_batch, so these
wrappers preserved a second, dead single-record lifecycle (state is None)
whose only observable behavior was a wrapper-specific cancellation release.
Remove the three wrappers and migrate the regressions that guarded their
cancel/release invariants to exercise the production _run_claimed_batch path
(operation=_*_one_claimed, release=_release_*_after_cancellation). The single
wrapper-only "state is None" contract (test_poll_release_hang_without_batch)
is deleted; all 11 remaining invariants (CancelledError preservation, repeated
cancellation, poll-only token-fenced lease release, notification claimed vs
dispatched phase release, hung compensation -> service ownership, and
background compensation exactly-once observation) are now covered through the
real batch lifecycle.
* fix(mcp): fence claim-owned mutations against stale generations
The per-claim token check in the ORM release/apply paths was only in the
SELECT; the final write went out by primary key. On SQLite (where
with_for_update() is a no-op) a mutation from an older claim generation
could therefore clear a claim that a newer generation had reclaimed after lease
expiry — the exact distributed lease-fencing failure the per-claim token was
meant to prevent.
Make every claim-owned mutation a single atomic conditional UPDATE with the
owner and per-claim token in the WHERE clause (rowcount 0 => stale, return
False, no mutation):
- release_claim: atomic fence; record the poll-failure event after the fence
wins (same transaction, holding the write lock).
- apply_snapshot / apply_cancel_snapshot: atomic fence; record the event after.
- finish_notification_run: atomic fence; use a CASE on event_version >>
dispatch_version to keep a newer event pending for redelivery instead of
swallowing it as delivered.
Add one regression per path: a stale generation's release/apply/finish after a
same-worker reclaim is rejected and never clears the newer claim.
* test(mcp): pin the migration chain head to the lease-token revision
0026_mcp_task_lease_tokens becomes the alembic head, so the chain-head pin in the 0025 repair test had to move on. Follow the 0023 precedent there (single head plus expected predecessor) instead of pinning a literal head, and give the new revision its own migration test, which owns the pin and covers the nullable claim-token columns on upgrade and their removal on downgrade.
* refactor(mcp): close cancellation cleanup leftovers
* fix(mcp): retain cancelled release diagnostics
* test(mcp): remove obsolete settled compensation case
* test(mcp): cover interleaved lease reclaim races
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* feat(channels): add WeChat QR login and binding recovery
* fix(channels): enforce single-worker WeChat QR login and preserve bot ID
Reject QR login endpoints when multiple Gateway workers are configured, while keeping manual token setup available.
Preserve the configured bot ID when the provider omits it or returns an empty value. Add regression coverage for worker guards and credential persistence.
* fix(channels): sync WeChat completion state on provider updates
Show the connected step when refreshed provider data confirms the binding, so the dialog no longer waits indefinitely after polling is cancelled.
Add regression tests for provider updates during pending poll and binding requests, including late responses and expiry.
* fix(channels): preserve WeChat pairing codes across waits and redirects
---------
Co-authored-by: YxinMiracle <“939157765@qq.com”>
* fix(uploads): delete the requested upload, not a symlink's target
delete_file_safe resolved the requested path before unlinking it. The
uploads directory is writable from local and AIO sandboxes, so a
symlink planted under an upload name was followed: deleting alias.pdf
removed the victim.pdf it pointed to, and the companion cleanup then
removed victim.md, while the link itself survived and the call reported
"Deleted alias.pdf". A link resolving outside the directory was already
refused by the traversal check, so the damage stayed inside the
thread's uploads.
The function now checks and unlinks the requested entry itself and
treats a symlink as not found, the same way list_files_in_dir already
hides it. unlink() never follows the final component, so a file swapped
for a link between the check and the unlink removes only the link.
Tests cover the helper, the Gateway DELETE route, and
DeerFlowClient.delete_upload.
* docs(changelog): note upload delete symlink fix (#5547)
* fix(events): serialize DB deletion with thread writers
* fix(runs): delete thread history without dropping reservations
* fix(feedback): support owner-scoped thread cleanup
* fix(threads): clean persisted records on deletion
* fix(threads): correct the feedback cleanup rationale
* test(runs): drop the wall-clock probe from the in-flight delete test
* docs: record the thread-delete and event-store fence contracts
* fix(threads): preserve legacy event-store delete compatibility
* feat(gateway): thread checkpoint retention service on the #4189 deletion contract
Implements exactly the two contract-proven deletion shapes (trailing
duration-only leaves, opt-in leaf sibling branches) with head-chain
protection, explicit id protection, a strict pending-writes guard, and
joint writes-row cleanup. Head resolution uses LangGraph's time-ordered
checkpoint ids; storage deletion mirrors the contract's per-backend data
model. Ships without a production trigger by design. Validated against
the contract suite (12 passed) plus 14 service scenarios across memory
and SQLite; Postgres paths are gated on TEST_POSTGRES_URI.
Signed-off-by: zengbohan1 <310902929+zengbohan1@users.noreply.github.com>
* fix(gateway): survivor-reachability blob GC and memory blob stats in retention service
Aligns the deletion service with the review-hardened contract: blob rows
are garbage-collected in a whole-thread pass against surviving
checkpoints' channel_versions (a real duration-only leaf shares its
parent's versions, so per-checkpoint version deletion would corrupt the
surviving state), the memory branch of the stats helper counts
saver.blobs and returns the full normalized shape, and per-node channel
versions are collected during the graph pass that already exists.
Signed-off-by: zengbohan1 <310902929+zengbohan1@users.noreply.github.com>
* fix(gateway): address review findings on checkpoint retention service
Resolves the review at a479cfe (willem-bd):
- Untested savers now fail fast: an explicit isinstance allowlist
(InMemorySaver / AsyncSqliteSaver / AsyncPostgresSaver) raises
NotImplementedError before any row is read or deleted, so a shallow or
third-party saver can never issue partial DELETEs.
- The chain walk ends (break) instead of raising KeyError when the head's
ancestor row is missing, matching the deletable loop's tolerance for
missing parents.
- enforce_thread_retention takes an optional per-thread lock and documents
the concurrency requirement: classification and deletion are two separate
passes, so callers must serialize per-thread mutation (runtime
_checkpoint_thread_lock) or guarantee quiescence.
- Dropped the dead mid-run guard: CheckpointTuple has no `next` field in
langgraph-checkpoint 4.1.1, and pending_writes is populated for committed
writes too (verified on the list path), so neither is a usable mid-run
signal; the caller-held thread lock is the actual protection.
- Removed the write-only _node_step/_Node.step and fixed the head-selection
docstring (newest by checkpoint id, not (step, checkpoint_id)).
- Documented the E1 leaf / history fast-path interaction in the contract doc
and module docstring: the wiring PR must sequence retention away from
history reads or adopt a policy that spares cache-carrying leaves.
- Added regression tests: unsupported saver, missing ancestor row, thread
lock parameter.
Validation: test_checkpoint_retention_service 18 passed / 8 postgres-gated
skipped; contract + lineage suites 18 passed / 6 skipped; ruff check and
format clean.
* fix(retention): count non-empty writes dicts on memory saver
- _checkpoint_ids_with_writes now requires a non-empty writes dict on
InMemorySaver: the empty phantom entry for checkpoints whose task wrote
nothing no longer counts as "owns writes rows", so the default E1 pruning
reaches the memory backend again (it was a silent no-op there).
- test_runtime_duration_leaf_pruned_by_default runs the shipping default
(strict_pending_write_guard=True) and proves E1 is reachable out of the
box on every backend; the stale override and its wrong SQLite premise
are dropped.
- document that _checkpoint_thread_lock is non-reentrant: a caller already
holding it must not pass it in, or retention self-deadlocks.
* test(checkpoint-retention): fix stray duplicated def token in test_duration_link_protected_after_next_run
The previous push left `async def def test_...` at line 244, which made the
module unimportable and failed collection of the whole suite (and ruff
format --check). Local copy was already correct; this commit re-pushes the
clean file. 18 passed / 8 postgres-skipped verified from a head worktree.
* fix(gateway): make retention correct on Postgres and fail closed on a bad cap
* validate max_delete_per_run before any store read: a negative cap used to
widen the batch (Python slicing) instead of being rejected;
* report identical before/after stats for an empty thread instead of returning
before stats_after is collected;
* protect each namespace's resume head and ancestor chain, so a persistent
subgraph's latest checkpoint is no longer treated as a sibling leaf;
* read Postgres columns through a row-factory-agnostic helper (the PG savers
open cursors with dict_row, where positional access raises KeyError: 0);
* classify the duration-only leaf without relying on metadata["writes"], which
the Postgres saver strips via get_serializable_checkpoint_metadata.
Verified locally on memory, SQLite and a real Postgres 16 instance (62 passed,
0 skipped): the E1 shape now fires on Postgres, which no backend test covered
before CI ran the Postgres lig.
Signed-off-by: zeng-bohan <zengbh1@gmail.com>
* test(gateway): pin the Postgres-shape duration classifier; report per-namespace heads
- Deterministic regression for _mark_duration_leaves_without_the_marker:
hand-put the Postgres round-trip shape (writes marker popped, source=
update + accumulated run_durations + channel_versions identical to the
parent) and assert the shipping default prunes it; a control that bumps
one channel version (the client update_state shape) with otherwise
identical metadata stays protected. Both legs run on memory and SQLite,
so the class cannot silently re-widen (a resumable head losing head
protection) or re-narrow (E1 never firing on Postgres) without a
locally-executing test failing.
- RetentionReport.protected_head_id -> protected_head_ids: heads are now
selected per namespace, so the report carries every namespace's head
(root key = what an unsaved aget_tuple resolves) instead of only the
global max - reshape it before the wiring PR starts consuming reports
for audit/aggregation.
---------
Signed-off-by: zengbohan1 <310902929+zengbohan1@users.noreply.github.com>
Signed-off-by: zeng-bohan <zengbh1@gmail.com>
Co-authored-by: zengbohan1 <310902929+zengbohan1@users.noreply.github.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* fix(gateway): preserve owner isolation when thread metadata is missing
Follow-up to the #5448 review P1 (post-merge finding): owner_check=True
also authorizes threads whose meta row is missing (legacy compatibility)
or NULL-owner (shared/pre-auth data). _run_scope_user_id returned None
for every trusted internal caller, which dropped the only remaining
per-user filter on those threads and let an internal caller acting for
owner A read owner B's persisted runs.
_run_scope_user_id now takes the thread_id and consults the thread meta
store: when an existing meta row establishes ownership, the authorized
thread's runs are still read unfiltered (merged #5448 semantics,
including owner-header-less internal callers); when the meta row is
missing or NULL-owner, the filter falls back to the acting owner's raw
stamp (the exact value start_run writes) — or the synthetic "default"
identity without an owner header — so cross-user runs stay hidden.
Isolation coverage uses the real MemoryThreadMetaStore with no metadata
row (and a NULL-owner row) plus another user's persisted run: /runs and
/runs/page must be empty and /runs/{run_id} must 404 for internal
callers, while an established-ownership thread keeps the unfiltered
read.
* fix(gateway): gate run-scoped sub-resource reads for internal callers
Review follow-up on #5484: the P1 owner-isolation class remained
reachable through run-scoped sibling reads that apply no per-user filter
at all — /runs/{run_id}/messages, /events, /join, /stream and
/workspace-changes query by (thread_id, run_id) directly, so on
missing/NULL-owner threads an internal caller acting for owner A could
still read owner B's run content by id (verified 200 at the previous
head).
- Extract _thread_ownership_established (shared meta-row check) and add
_require_run_visible_to_scope: for internal callers on threads without
established ownership, the run's own user_id stamp must match the
acting owner's raw value (or the legacy "default" stamp) or the read
404s. Established-ownership threads and every non-internal caller keep
their existing thread-scoped semantics.
- Wire the gate into join, stream, messages, events and
workspace-changes; reword the now-stale messages comment to track the
new scoping semantics.
Regression tests: sub-resource reads 404 for a mismatched internal
owner while the matching owner reads them normally, and the owner-less
fallback branch (synthetic "default" filter on missing-meta threads) is
pinned. Red confirmed against the pre-gate head.
* fix(gateway): gate cancel and artifact archive for internal callers
Review follow-up on #5484 round 2: POST /cancel resolved runs unscoped
(require_existing=True only closes the missing-meta case — NULL-owner
meta rows still pass), so an internal caller acting for a different
owner could interrupt another owner's active run on a shared thread
while /join and /stream were already gated. The archive manifest and
download pair likewise leaked the other owner's delivered-file count
and a 200-vs-409 delivery oracle on NULL-owner threads (missing-meta
threads were already denied by require_existing=True).
All three routes now call _require_run_visible_to_scope; its docstring
records the extended coverage. NULL-owner-thread regression tests pin:
a mismatched internal owner gets 404 from cancel, manifest and archive
download, while the acting owner reaches the real conflict path (409 on
a terminal run) and reads the manifest (file_count 2).
* fix(gateway): tolerate state-less request stand-ins in the scope helpers
The new owner-isolation gate and _run_scope_user_id read request.state
directly, which crashed the FakeRequest-based unit suites for the run
events, workspace-changes and scope endpoints (backend-unit-tests shards
1/2/4 on #5484). Read the state object defensively first: a request
without state is simply not an internal caller, so those paths keep
their pre-gate semantics.
* fix(gateway): scope the thread token-usage aggregate by owner
Review follow-up on #5484 round 4: GET /{thread_id}/token-usage called
aggregate_tokens_by_thread(thread_id) with no user filter at all, so on
missing/NULL-owner threads an internal caller acting for owner A read
owner B's spend, model names, run count and (with include_active=true)
live activity; the NULL-owner variant reached browser sessions too.
build_context_usage's latest-model lookup was unfiltered as well.
aggregate_tokens_by_thread gains an optional user_id (mirroring
list_by_thread: explicit None = unfiltered, AUTO resolves the contextvar)
in the memory store, the SQL repository and the store base;
build_context_usage/_resolve_thread_model_name thread the scope through
the latest-run lookup; the token-usage endpoint passes
_run_scope_user_id's value. Established-ownership threads aggregate
unfiltered as before; shared/missing-meta threads narrow to the acting
identity. Stale helper-test comment reworded after the #5482 merge
adaptation.
* test(gateway): pin the unfiltered aggregate on established-ownership threads
Review follow-up on #5484 round 5: the established-ownership branch of
the token-usage scoping (store receives user_id=None) was the only
unpinned half of the contract — the round-4 call-assertions never set
app.state.thread_store, so their None came from the user-less stand-in
path. test_token_usage_unfiltered_on_established_ownership_for_
internal_callers seeds an established meta row plus runs stamped by two
different identities and asserts the totals fold (166 = 111 + 55);
together with the isolation tests it now catches both failure modes
(always-stamp narrowing and always-None leak).
* feat(authz): filter per-caller skill visibility on the listing surfaces (#4063 Phase 4)
GET /api/skills, GET /api/skills/custom, and GET /api/skills/{name} now
filter the user-scoped catalog through filter_resources(principal,
"skill", ...) — mirroring list_models. Anonymous callers are unfiltered;
provider errors follow authorization.fail_closed (fail-closed -> empty
listing / 404, fail-open -> full listing). An invisible skill on the
detail surface returns the standard 404 so the endpoint cannot become an
existence oracle the filtered list closed. Management endpoints stay
require_admin_user-gated; runtime activation is #4541's layer.
resolve_skill_authorization joins resolve_model_authorization as a thin
sibling over a shared _resolve_route_scoped_authorization core.
* docs(authz): reflect per-caller skill visibility in OpenAPI metadata and implementation notes (#5489)
Address the two non-blocking review findings on #5489:
- The three user-facing GET routes (/skills, /skills/custom,
/skills/{name}) now say in their /docs-visible descriptions that
authorization filters the response (hidden skills 404 on detail).
- Add the dated Phase 4 decision-log entry to the authorization
implementation notes, per the convention of every prior merged
authz PR: listing-visibility semantics, the 404-vs-403
existence-oracle rationale, anonymous-caller behavior, and the
#4541 rebase reconciliation points (config.example.yaml roles
comment + this file's decision log).
* docs(authz): move route guidance into Gateway module guide
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
The message edit/regenerate prepare chain resolves source runs through
_resolve_run_id_for_message, _require_successful_source_run and
_find_interrupted_target_run_id, which filtered by the authorization
identity (get_current_user) — the same conflation fixed for the runs and
messages read endpoints in #5448, left out of scope there. For trusted
internal callers the authorization identity never matches the raw
owner-stamped run rows, so regenerate/edit-regenerate prepare fails with
409 on threads the caller is authorized on (#5482).
The three helpers now resolve their filter id through _run_scope_user_id
as well; both prepare endpoints keep their owner_check=True
authorization and browser/API sessions keep the per-user filter.
Regression tests extend test_thread_runs_internal_scope.py to the helper
fallback paths: internal callers resolve raw-owner-stamped runs
(including the interrupted-run and status-409 paths), browser sessions
keep the per-user filter and 409 on cross-user runs.
