* refactor(tools): consolidate duplicated _truncate and get_mcp_manager into src/tool_utils
Move all copies of _truncate(), get_mcp_manager(), and set_mcp_manager()
into a single leaf module (src/tool_utils.py) that imports only from
src.constants. This eliminates the lazy-import hack
('from src import agent_tools' inside function bodies) in tool_execution.py
and tool_implementations.py, and fixes a latent bug: the _truncate copy in
tool_execution.py was missing the isinstance guard and would crash on None.
Also deletes mcp_servers/_common.py — it was dead code with zero callers
anywhere in the codebase, containing its own copy of truncate() and
constants that already exist in src/constants.py.
* fix(tools): route remaining get_mcp_manager imports to src.tool_utils
The maintainer's feedback flagged src/task_scheduler.py:1857 and
routes/task_routes.py:977. A project-wide search found a third call site
in src/agent_loop.py that also imported get_mcp_manager from
src.agent_tools instead of src.tool_utils.
All three are now sourced from the canonical location in src.tool_utils.
---------
Co-authored-by: mcnoliveira <mcnoliveira@gmail.com>
Add focused tests for the z.ai/api/coding path override:
- _match_provider_curated: 5 tests verifying coding vs base key
- _probe_endpoint: 3 tests verifying model preservation, curated
append on partial response, and base-zai exclusion
Rebased onto dev per reviewer request.
Fixes#2230
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Alexandre Teixeira <111787685+alteixeira20@users.noreply.github.com>
Three issues combined to make the per-user 'Allowed models' checklist
unreliable (#3032):
1. admin.js _loadModelsForUser fetched /api/models, which is backed by
cached_models — endpoints that haven't been probed yet (e.g. a
freshly-added DeepSeek API endpoint) simply didn't show up in the
checklist. Switched to /api/model-endpoints, which always reflects
every configured endpoint regardless of cache state.
2. _saveModels sent allowed_models: [] both when the admin clicked
[All] (no restriction) and [None] (block everything) — the backend
had no way to distinguish the two.
3. _enforce_chat_privileges treated an empty allowed_models list as
'no restriction' (falsy -> skip the check), so [None] had no effect.
Added an explicit block_all_models privilege flag (defaulting to False,
and forced to False for admins) that admin.js now sets when zero models
are checked. _enforce_chat_privileges checks it first and 403s
regardless of allowed_models contents.
* fix(agent): stop executing illustrative Markdown fences as tool calls for native function-calling models
_resolve_tool_blocks fell back to the textual parse_tool_blocks() fenced-block
parser whenever a model produced no native tool_calls, regardless of whether
that model has a reliable native function-calling channel. Native models
(GPT/Claude/Grok/Qwen3/DeepSeek-V, etc. - _is_api_model true) commonly write
illustrative ```bash/```python/```json examples in guide-only prose; the
fallback parser matched these and executed them as real commands, sometimes
looping for several rounds as the model tried to clarify with more examples
(#3222).
Restrict the textual fenced-block fallback to non-native models, which rely
on it as their only tool-invocation channel. Native models are trusted to use
their structured tool_calls channel for real invocations; when they don't
emit one, a bare fence in their response is prose, not an action. The native
tool_calls path itself is untouched.
This sits one layer below #3088's guide-only policy enforcement: that PR
blocks tool exposure/execution on explicit no-tools requests, while this fixes
the parser so ordinary illustrative fences are never misread as calls in the
first place, on any turn.
* fix(agent): gate only the fenced-example pattern for native models, preserve DSML/invoke recovery and persistence
_resolve_tool_blocks previously short-circuited the entire textual parser
(tool_blocks = [] if is_api_model else parse_tool_blocks(...)) for native
function-calling models with no native tool_calls. That also dropped Patterns
2-5 (explicit [TOOL_CALL]/<invoke>/<tool_code>/DSML markup leaked into content
as text), which are real calls a model couldn't emit on its structured channel
(e.g. DeepSeek-V falling back to DSML), not illustrative examples.
parse_tool_blocks/strip_tool_blocks now take a skip_fenced flag that gates ONLY
Pattern 1 (the fenced ```bash/```python/```json block matcher). _resolve_tool_blocks
passes skip_fenced=is_api_model so fenced examples stop being executed for
native models while [TOOL_CALL]/<invoke>/<tool_code>/DSML stay fully active and
recoverable. cleaned_round mirrors the same gate when persisting round text, so
an illustrative fence that wasn't executed isn't stripped from saved/reloaded
history either (it was streaming once and then disappearing on reload).
extract_urls() stripped any trailing ')' unconditionally via
`re.sub(r'[.,;:!?\)]+$', '', url)`. That corrupts URLs that legitimately
end in a parenthesis — most commonly Wikipedia disambiguation links like
https://en.wikipedia.org/wiki/Python_(programming_language), which became
...Python_(programming_language and then 404 when fetched by the web/research
tools.
Strip trailing sentence punctuation as before, but only drop a ')' when it is
unbalanced (more ')' than '('), so a prose-glued "(see https://example.com)"
still loses its closing paren while balanced URLs keep theirs.
Added tests/test_extract_urls.py covering balanced, unbalanced, nested, and
trailing-punctuation cases.
* fix(email): close IMAP socket when connect/login fails (#3174)
_imap_connect opened a live socket via _open_imap_connection and then
called conn.login() with no try/finally, and _open_imap_connection called
conn.starttls() unguarded. When auth fails (e.g. an Office 365 app password
on an MFA-enabled tenant, #3174) or STARTTLS is rejected, the already-open
socket was orphaned. Every IMAP caller funnels through _imap_connect,
including the 30-minute _auto_summarize_poller, so a persistently
misconfigured account leaked one descriptor per pass toward FD exhaustion.
The previously merged leak fixes (#1325/#1330/#1423/#1530) only guard the
post-connect body and monkeypatch _imap_connect to succeed, so this
connect-time path was uncovered. Wrap login() and starttls() so a failure
calls conn.shutdown() (low-level close; logout() can't run pre-auth) before
re-raising. Adds two regression tests that fail without the guard.
* fix(email): guard MCP IMAP+SMTP connect-time leaks too (#3174)
Folds in the sibling connect-time leaks vdmkenny flagged on #3363, so the
whole connect-then-step leak class is closed in one place:
- mcp_servers/email_server.py::_imap_connect — guard starttls() and login();
close pre-auth with conn.shutdown() before re-raising.
- mcp_servers/email_server.py::_smtp_connect — guard starttls() and login();
SMTP has no shutdown(), so close with conn.close() (socket close, no QUIT).
Routes SMTP (_send_smtp_message) is already safe via 'with smtplib.SMTP(...)'.
Adds four regression tests (one per guard), verified to fail without the fix.
* fix(presets): scope expand-prompt model resolution to owner
/api/presets/expand resolved its model endpoint with no owner, so in a
multi-user setup it could match another user's endpoint and use its URL
and decrypted api_key. Pass effective_user(request) to _resolve_model so
resolution is owner-scoped. Adds a regression test.
* fix(presets): scope teacher and audit model resolution to owner
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Alex Little <alexwilliamlittle@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Kenny Van de Maele <kenny@kvandemaele.be>
Pin actions to commit SHAs, set persist-credentials: false on every
checkout, and scope token permissions to the jobs that use them. Suppress
the two findings that are safe by design: the description bot's
pull_request_target trigger (no fork code runs) and an intentional
word-split in the docker manifest step.
Clears actionlint and zizmor against dev so the blocking gate from #1314
can pass once both land.
fork_session passed each source message's metadata dict by reference into the
new session. add_message() -> _persist_message() stamps _db_id (and timestamp)
onto that dict in place, so persisting the fork overwrote the SOURCE messages'
_db_id with the forked rows' ids — silently breaking edit/delete-by-id on the
original conversation.
Copy the metadata dict per message so the fork and source no longer alias.
Adds tests/test_fork_session_metadata.py asserting the source session's
message metadata is unchanged after a fork.
GET /api/models swallowed any non-HTTPException raised while checking
whether the caller is authenticated (bare except Exception: pass), so a
broken auth_manager or an exception from get_current_user silently
granted the full model list to an anonymous caller instead of rejecting
the request. Now any unexpected exception logs and returns HTTP 500.
Split out of #2360 per reviewer request to keep the deny-list and the
auth-gate fix as separate, single-purpose PRs.
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(memory): auto-memory extracted nothing — flatten window so the prompt ends on a user turn
extract_and_store appended the recent window as raw alternating role messages
after the system prompt. Since the window is the last N messages, the prompt
usually ENDED on an assistant turn — and a chat model given a prompt ending on
an assistant turn returns an empty completion (nothing to answer). The result
was facts=[] → "Auto memory extraction ran: 0 candidates" on every run, so no
memories were ever stored, while skill extraction (which flattens the transcript
into a single user message) worked fine.
Flatten the window into one user message ending with an explicit instruction,
mirroring the skill extractor, so the model always responds. Also harden parsing
for reasoning models, matching the audit path which already does this:
- raise max_tokens 500 → 4096 (a reasoning model spends the budget on <think>
before emitting JSON; 500 truncated it before any JSON appeared);
- strip <think>/prose preambles via strip_think and slice the embedded JSON
array before json.loads, instead of bombing on char 0.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* chore: tighten memory-extraction-empty-completion — clarify JSON-slice comment re prior strip steps
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs(memory): reframe the comment to the accurate root cause (raw-chat framing)
The earlier comment leaned on "ends on an assistant turn -> empty completion",
which is only one failure mode. The dominant cause, confirmed by a controlled
repro (0/6 old vs 6/6 new on this model), is that passing the window as raw chat
messages makes the model treat it as a conversation to continue rather than a
transcript to analyze, so it returns [] even when durable facts are present.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(memory): cover extraction JSON parsing + slice trailing commentary unconditionally
Factor the strip/fence/slice/json.loads logic out of extract_and_store into
a pure module-level helper _parse_extraction_json(raw) -> list and drop the
'text[0] != "["' guard so the array is sliced whenever both brackets exist
(fixes trailing commentary like '[...] Done!' reaching json.loads).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
PATCH /api/tokens/{id} unconditionally recomputed scopes from
payload.get("scopes"). On a rename — body {"name": "..."} with no "scopes"
key — that is None, so _normalize_scopes(None) returned the default ["chat"]
and the handler overwrote token.scopes, silently dropping every scope the
token had been granted (e.g. email:read, calendar:write).
Only write scopes when the request actually includes them, and return the
token's real stored scopes in the response (matching the GET /tokens display
shape) instead of the recomputed default.
tests/test_api_token_routes.py: add rename-preserves-scopes,
explicit-scopes-applied, and missing-token-404 cases for the PATCH handler.
PresetManager.save() used a plain open("w") + json.dump, which truncates
presets.json before writing the new content. A crash, power loss, or
serialization error mid-write leaves the file truncated/empty and every
saved preset is lost.
Route the write through core.atomic_io.atomic_write_json (tmp file +
os.replace), matching how the rest of the codebase persists JSON state.
The helper is imported lazily so this module stays free of the heavy core
package import graph at module load time.
Adds tests/test_preset_atomic_save.py covering the source contract, a
failed-write leaving the existing file intact, and a round trip.
* fix(caldav): don't prune the whole window when no objects could be parsed
The post-sync prune deletes local origin=="caldav" rows in the window whose UID
the server didn't just return. With an empty seen_uids it falls back to
`uid.isnot(None)` — a match-all delete. That's right when the calendar is
genuinely empty, but when the server returns objects and every one fails to
parse (malformed iCal / an icalendar error), seen_uids is empty only because
nothing could be read, so the match-all branch silently deletes every local
event in the 90-day-back/365-day-forward window.
Track whether any object failed to parse and gate the prune with a small pure
helper `_should_prune_window(seen_uids, parse_failed)`: prune when something was
read, or when the calendar is genuinely empty (no objects, no parse errors), but
never when objects came back unreadable.
Adds tests/test_caldav_prune_parse_failure.py for the three cases.
* fix(caldav): skip the prune on any parse failure, not just total
Review follow-up (#3454): _should_prune_window returned True whenever seen_uids
was non-empty, so a partial parse failure (say 48 of 50 objects parse) still
pruned the 2 unreadable-but-still-upstream events, because their UIDs were absent
from seen_uids. Any parse failure makes seen_uids an incomplete view of the
server, so pruning against it is unsafe whether the failure is total or partial.
Skip the prune on any parse failure (return not parse_failed); only prune on a
clean read (a genuinely empty window is still safe to prune). Tradeoff: one
permanently-unparseable event pauses deletion mirroring until it is fixed, which
is the safe direction (false-keep beats false-delete).
Replace the now-incorrect "partial failure still prunes" assertion with a
partial-failure regression: one object parses, one fails, so the prune is
skipped and the unparsed event's local copy is not deleted.
---------
Co-authored-by: Kenny Van de Maele <kenny@kvandemaele.be>
* fix(memory): only delete memories the model explicitly drops in tidy
The AI memory-tidy path computed deletions as the complement of the model's
`keep` list (`if mid not in keep_ids: continue`). When the model returned a
valid response that simply omitted some existing ids — a common LLM lapse — every
omitted memory was silently deleted, even though it was neither a duplicate nor
listed in `drop`.
Honor the explicit `drop` set instead: delete only ids the model dropped (minus
any it saw only truncated), and preserve everything else, still applying cleaned
text/category from `keep`.
Adds tests/test_consolidate_memory_explicit_drops.py: a memory the model omits
from both keep and drop survives; an explicitly dropped one is removed.
* refactor(memory): remove now-dead keep_ids from tidy
After deletion switched to drop_ids and text/category rewrites to cleaned_by_id,
keep_ids was written but never read. Remove the init, the .add(mid) in the keep
loop, and the truncated .update() (its truncated-protection is already covered by
`drop_ids -= truncated_ids`). Pure deletion, no behavior change; tests stay green.
Addresses review feedback on #3455.
---------
Co-authored-by: Kenny Van de Maele <kenny@kvandemaele.be>
Pressing ArrowUp on an empty #message composer restores the last sent user text, matching common chat-app UX (Slack, Discord, ChatGPT).
- Read from #chat-history .msg-user dataset.raw (same path as resend/regenerate), not session sidebar metadata
- Literal empty check (whitespace-only drafts are preserved); ignore Shift/Alt/Ctrl/Meta and IME composition
- Extract wiring to composerArrowUpRecall.js; rAF + 250ms retry only (no global MutationObserver)
- Add tests/test_composer_arrow_up_recall_js.py
Co-authored-by: Cursor <cursoragent@cursor.com>
* Scope secondary endpoint lookups by owner
* Reject unregistered image endpoint URLs for non-admins
* Adjust owner-scope tests for rebased routes
* Allow non-admins to compare endpoints they own
The compare owner-scope guard called _reject_raw_endpoint_url_for_non_admin
with endpoint_id=None, so it rejected every signed-in non-admin
/api/compare/start request — even for endpoints the caller owns — because
compare resolves endpoints by URL and carries no endpoint_id. That locked
non-admins out of compare entirely.
Resolve the owned ModelEndpoint first and pass its id, so a registered
endpoint the caller owns is allowed while only truly raw, unregistered URLs
are rejected (mirrors the gallery inpaint/harmonize checks in this PR).
Replace the source-only reject test with deterministic reject + allow
regressions that no longer depend on the dev DB contents.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Bind compare sessions to the resolved owner-scoped endpoint
/api/compare/start created the [CMP] helper sessions with the raw
caller-supplied endpoint URL and only used the owner-scoped lookup to
decide whether to copy an API key. That stopped key borrowing but still
let a non-admin inject an arbitrary raw endpoint URL into the compare
session path.
Now, when the supplied URL resolves to a registered endpoint visible to
the caller, the session binds to that row's own normalized base URL
(build_chat_url(normalize_base(ep.base_url))) plus its headers — the same
registered-endpoint shape session_routes uses. The raw URL survives only
when ep is None, which non-admins already hit a 403 on, leaving raw URLs
reachable solely for admins / single-user mode with no borrowed key.
Adds compare-specific behavior tests: another user's private endpoint is
rejected (nothing created), the session binds to the stored URL rather
than the raw input, and an admin raw URL is allowed but carries no
inherited key.
Addresses the review on #1511.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Validate both compare endpoints before creating any session
start_comparison resolved + created each [CMP] session inside one loop,
so a request pairing a valid owned endpoint A with an unregistered raw
endpoint B raised 403 only after A's session was already created — and
its Authorization header copied in. The rejected request left a partial
compare session with that header behind.
Split the flow into two phases: phase 1 resolves and owner-validates
both endpoints (running the raw-URL reject helper) and stashes the
session URL + headers; phase 2 creates the two sessions only once both
passed. A 403 on either endpoint now aborts with nothing created and no
header copied.
Adds a regression test: owned endpoint A + unregistered/raw endpoint B
-> 403 with no sessions created.
Addresses the follow-up review on #1511.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Resolve compare credentials by endpoint id, not URL alone
Two endpoints visible to a caller can share a base_url but hold different
api_keys. _owned_endpoint_by_url returned whichever row sorted first, so
/api/compare/start could copy the wrong key into the [CMP] session.
Add _owned_endpoint_by_id (same owner scoping) and optional endpoint_a_id/
endpoint_b_id form fields. The id pins the exact registered endpoint; URL
resolution remains only for legacy/admin raw-URL callers. An id the caller
can't see 404s instead of falling back to a same-URL row.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Loosen research-routes owner-scope assertion to the stable substring
The rebased _resolve_research_endpoint generalized its owner derivation to
honor an explicit owner arg first (owner = owner or getattr(sess, ...)), so
the exact-line assertion broke CI. Assert the stable session-derivation
substring instead of the full line.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
The Inspect step had `${{ github.ref == ''refs/heads/main'' ... }}` with
doubled single quotes (YAML-scalar escaping) inside a `run: |` block, which
GitHub's expression parser rejects, failing the whole workflow at startup
(no jobs run). Replace with a plain shell conditional on $GITHUB_REF.
* ci: build and publish multi-arch Odysseus image to GHCR
Push to main publishes :latest and :X.Y.Z; push to dev publishes :dev and
an immutable :X.Y.Z-dev.<sha>. Multi-arch (linux/amd64 + linux/arm64) via
per-arch native runners building by digest, merged into one manifest list.
Uses the in-repo GITHUB_TOKEN (packages: write), actions pinned by SHA.
* ci(docker): pin actions to latest major releases
checkout v6.0.3 (matches the PR-checks workflow), setup-buildx v4.1.0,
login v4.2.0, build-push v7.2.0, metadata v6.1.0, upload-artifact v7.0.1,
download-artifact v8.0.1 — all by commit SHA.
Cloners default to the dev branch (CONTRIBUTING: main is the curated
release, dev is where fixes land). The bug template required ticking
'latest code from main', so reporters confirm a stale branch and bugs
already fixed on dev get re-filed. Ask them to reproduce on latest dev.
When in chat mode, the shell access and plan mode buttons should not be
visible. These buttons are only relevant in agent mode where the AI can
use shell commands and planning features.
Changes:
- Modified applyModeToToggles() to hide bash-toggle-btn and plan-toggle-btn
when mode is 'chat'
- Added immediate hiding on page load to prevent flash of buttons
- Buttons are shown again when switching to agent mode
Fixes#3411
Co-authored-by: michaelxer <michaelxer@users.noreply.github.com>
* feat: Add ChatGPT Subscription support and related features
- Introduced a new provider option for ChatGPT Subscription in the endpoint selection UI.
- Implemented OAuth flow for ChatGPT Subscription sign-in, including polling for authorization status.
- Updated admin interface to handle ChatGPT Subscription, including disabling API key input and providing user guidance.
- Enhanced cost tracking logic to differentiate between subscription and non-subscription endpoints.
- Added new slash commands for managing skills, including listing, searching, and invoking skills.
- Implemented caching for skill catalog to optimize performance.
- Updated tests to cover new ChatGPT Subscription functionality and ensure proper endpoint probing.
- Refactored existing code to accommodate new features and improve maintainability.
* refactor: share provider device-flow setup
- reuse one device-flow backend for Copilot and ChatGPT Subscription
- add one frontend device-flow helper for Settings and /setup
- put GitHub Copilot back into Add Models, now as a dropdown option
- make provider selection just select; clicking Add starts sign-in
- stop ChatGPT Subscription setup from opening auth tabs automatically
- make /setup copilot and /setup chatgpt-subscription work from chat
- show ChatGPT Subscription in the /setup suggestions
- show the real error message when setup fails
- add focused tests for the shared flow and setup UI
* feat(chatgpt-subscription): harden credential lifecycle and streamline auth UX
Backend:
- Resolve runtime bearer for provider-auth endpoints at probe time via a
shared _resolve_probe_key() that delegates to resolve_endpoint_runtime,
applied across all probe/refresh call sites.
- Skip live completion probes and health pings for discovery-only providers
(centralized behind _is_discovery_only_provider) — the Codex/Responses API
has no such endpoints, so status is derived from cached models.
- Never persist the short lived ChatGPT bearer to the plaintext sessions
table; proactively clear any stale bearer left by an earlier code path.
- Revoke orphaned ProviderAuthSession credentials when the last endpoint
backing them is deleted (_delete_orphaned_provider_auth), surfaced via
cleared_provider_auth in the delete response.
Frontend (admin.js):
- Auto-start the device-auth flow on provider selection so the authorization
panel (code + Authorize) shows immediately instead of behind a "Sign in" click.
- Remove the redundant top button for device auth providers, move retry
into the panel via an inline "Try again".
- Drop the self-evident hint text and add an execCommand clipboard fallback so
Copy works in non-secure (HTTP/LAN) contexts.
* fix: harden chatgpt subscription provider
* chore: remove PR media from branch
* Fix chatgpt subscription recovery and token handling
---------
Co-authored-by: 5p00kyy <admin@5p00ky.dev>
* refactor(constants): single source of truth for data dir + merge core/src constants
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(contributing): use named src.constants for data paths, drop core/constants references
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous approach polled request.is_disconnected() inside the
async-for body of the chat/agent streaming loops. That happens too
late: by the time the poll runs, __anext__() has already awaited and
consumed the next upstream chunk, so a slow or silent generation could
still run for a full round-trip (or until a read timeout) after the
client disconnected. It was also unconditional, which would have made
ordinary chat navigation/refresh/tab-close stop a run that the
detached-run design intentionally keeps going server-side.
Both problems trace back to the same root cause: chat_stream always
wraps its generator in agent_runs (the detached-run manager), which
decouples the generator's lifetime from the SSE response on purpose so
normal chat/agent streams survive the client going away. Polling
disconnection inside a detached generator can never be "prompt" — the
generator isn't tied to that request anymore — and doing so defeats the
whole point of detaching it.
Compare panes don't need (or want) that: each pane's session exists
only to drive that one generation, there's nothing meaningful to
/resume, and the user expects the pane's Stop button — which aborts the
fetch and closes the SSE — to cancel the upstream call right away. So
route compare-mode requests around the agent_runs wrapper entirely and
stream the generator directly as the SSE body. Starlette already
cancels a streaming response's body iterator (raising
CancelledError/GeneratorExit into it) the instant it notices the client
disconnected — including while the generator is mid-await on the next
upstream chunk — and the existing except (CancelledError, GeneratorExit)
handlers in both the chat-mode and agent-mode loops already save the
partial response exactly once. No polling needed; the redesign just
stops getting in its own way.
Normal (non-Compare) chat and agent streams are untouched and keep
going through agent_runs, preserving detached-run semantics (surviving
tab close / navigation / refresh, reconnect via /api/chat/resume).
Replaces the source-text assertions in
tests/test_compare_stop_disconnect_poll.py with runtime tests that
actually exercise the cancellation contract: a Compare-shaped generator
is cancelled mid-await (not after the next chunk arrives) and saves its
partial exactly once; a normal completion still saves exactly once via
the completion path; agent_runs keeps a detached run alive when its
subscriber disconnects and only stops it on an explicit stop()/cancel
(also saving the partial exactly once); and the cancellation contract
is pinned for both chat-mode- and agent-mode-shaped chunk sequences.
raise_for_status() raises httpx.HTTPStatusError for 4xx/5xx responses,
but the surrounding try/except only caught httpx.RequestError (network
errors) and RateLimitError (429). Any other HTTP error code propagated
uncaught up through chat_processor -> chat_helpers -> chat_routes and
surfaced as a 500 Internal Server Error.
Added an explicit except httpx.HTTPStatusError clause that logs a warning
and returns an empty result, matching the behaviour already in place for
network errors.
Also adds focused regression tests that exercise the real
fetch_webpage_content() path with a mocked _get_public_url:
- 403/404 responses return the standard empty-result shape instead of
raising, proving the new HTTPStatusError handling works end to end.
- 429 responses still take their own dedicated rate-limit branch (the
status_code == 429 check runs before raise_for_status() is reached),
keeping that behaviour distinct from the new generic HTTPStatusError
handling.
Dropped the unrelated builtin_mcp.py change that had been carried over
from a rebase; that fix is tracked separately in #2018 and this branch
should stay scoped to the search content fetch path.
Closes#2148
Three call sites hardcoded Path("/app/data/cookbook_state.json"), which only
exists in Docker; on a native run the real path is <repo>/data, so the state
file looked missing and cookbook serve-state was silently ignored. Two others
used os.environ.get("DATA_DIR", "data") (a relative fallback, since DATA_DIR is
never set as an env var). Route all five through core.constants.DATA_DIR so the
path is consistent and absolute on both Docker and native.
Part of #3331.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* fix(platform): add WSL compatibility functions and path translation
fix(cookbook): enhance model scan script to support additional HuggingFace cache paths
fix(hardware): improve cache key generation for remote SSH context
test(tests): add tests for WSL detection and path translation functionality
* fix(cookbook): prefer prebuilt wheels for llama-cpp-python and normalize package aliases
* fix: enable StrictHostKeyChecking in nvidia probe
refactor: consolidate ssh & powershell command execution to utility functions in core module
refactor: consolidate nvidia path candidates in to single variables in core module
tests: add tests for new utility functions
* fix: correct wrong variable name
* fix(skill-extractor): walk all brace candidates so stray braces in prose do not swallow valid JSON
The extractor sliced from the FIRST brace to the LAST brace to recover
JSON embedded in surrounding commentary. When the model emits stray
braces before the JSON object, the slice produces invalid JSON,
json.loads raises, and the exception is swallowed -- the skill is
silently lost.
Fix: walk each brace candidate left-to-right and attempt json.loads on
each slice. The first candidate that parses successfully wins. If none
parse, json.loads on the original text raises and the existing
JSONDecodeError handler logs and returns None as before.
Tested locally -- 8/8 tests passed:
tests/test_extract_skill_json_nonstring.py (2 passed)
tests/test_skill_extractor_rows.py (1 passed)
tests/test_search_content_extraction_parity.py (2 passed)
tests/test_deep_research_search_error.py (3 passed)
Closes#2199
* test(skill-extractor): add focused repro for stray-brace JSON recovery
* test(skill-extractor): add regression test for leading invalid-brace fragment
Addresses the remaining edge case from review: a response that *starts*
with a brace but the leading fragment isn't valid JSON (e.g.
'{not json}\n{"title": "Valid later", ...}') still needs to recover
the valid skill object that follows.
_extract_json_object (already on dev) handles this correctly — it tries
the whole de-fenced string first, then walks each '{' candidate left-to-
right regardless of whether the response begins with '{', so the leading
invalid fragment no longer short-circuits recovery of the real object.
Updates the comment at the call site to call this out explicitly and adds
a regression test covering exactly the scenario described in review.
* fix: expose supports_tools toggle for local endpoints in UI
Local endpoints (Ollama, vLLM, etc.) default to fenced tool blocks
when supports_tools is not set, which breaks tool calling for models
that support native function calling. The backend already supports
per-endpoint supports_tools overrides via the PATCH API, but there
was no UI to set it.
Add a 'Tools: Auto/On/Off' toggle button for local endpoints that
cycles through the three states:
- Auto (null): use the existing heuristic
- On (true): always use native function calling
- Off (false): always use fenced tool blocks
Fixes#3141
* docs: add screenshot of supports_tools toggle showing Auto/On/Off states
* Add Tools toggle screenshot for PR #3195
* refactor: convert Tools toggle to select dropdown per review feedback
Replace cycle-through button with a <select> dropdown for the
supports_tools tri-state setting. Options: Auto / On / Off with
explicit labels. Uses existing admin select styling. Fires PATCH
on change event. Same API contract (Auto=null, On=true, Off=false).
* Update Tools toggle screenshot (now dropdown select)
* fix: remove orphan screenshot and move Tools dropdown below button row
- Remove docs/screenshots/tools-toggle-three-states.png (unreferenced image causing test_no_orphan_images_in_docs to fail)
- Move Tools dropdown to its own line below Disable/Delete buttons, aligned right
- Keep Disable and Delete buttons grouped together per maintainer feedback
* fix: move Tools select onto same row left of Disable/Delete, use CSS class
Per vdmkenny feedback: move the Tools dropdown select from its own row below
the button group onto the same row, to the left of the Disable and Delete
buttons (which stay adjacent on the right). Replace inline style on the
button row with the existing .admin-ep-actions CSS class, adding
align-items:center for proper vertical alignment.
* chore: remove committed screenshots from tree
Screenshots should be in PR description/comments, not in repo history.
---------
Co-authored-by: michaelxer <michaelxer@users.noreply.github.com>
#3336 reduced the PR-checks workflow to pull-requests:read on the
assumption that PR labels/comments only need issues:write (the REST path
is /issues/{n}/...). They do not: modifying a pull request's labels or
comments requires the pull-requests scope, so issues:write alone returns
403 and crashed the description check on every PR. Restore
pull-requests:write, and fail soft in swapLabel so a label-permission
error can never mask the description verdict.
* ci(pr-checks): add Conventional Commits PR-title check, pin actions by SHA
Add a check-title job that fails the PR when the title is not Conventional
Commits format (type(scope): summary), via an inline github-script regex.
Pin the workflow's actions to their latest release commit SHAs:
actions/checkout v6.0.3 and actions/github-script v9.0.0.
* ci(pr-checks): flag unmergeable PRs in the PR-checks workflow
Add a check-mergeable job to the (renamed) PR checks workflow: on PR events,
poll the PR's mergeable state and, when it conflicts with the base, remove
'ready for review', add a red 'merge conflict' label (auto-created), and
comment; clear the label once mergeable again. Single-PR, no push trigger.
Add ready_for_review to the trigger types.
* ci(pr-checks): drop the comment from check-mergeable, label swap only
* ci(pr-checks): least-privilege workflow permissions
contents:read for base-ref checkout, pull-requests:read for pulls.get
mergeability, issues:write for label + comment management. Drops the
unused pull-requests:write (labels and PR comments go through the issues
API).
services/search/cache.py set CACHE_DIR = services/cache (the source tree) and
mkdir'd it at import, unguarded. In Docker services/ is the read-only image
layer, so the mkdir fails at import (same class as the analytics bug #2366).
Move the cache under DATA_DIR/cache (writable on Docker and native) and wrap
the mkdir so an unwritable path disables disk cache instead of crashing import.
Part of #3331.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* docs(contributing): require constants/helpers over hardcoded paths and URLs
Add a Code conventions section: don't hardcode filesystem paths or loopback
URLs, use DATA_DIR / internal_api_base() from core.constants, guard dir
creation, and add a constant when a repeated literal has none. Codifies the
class of bug behind #2366, #2752, and #3331.
Part of #3331.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs(contributing): add Conventional Commits to code conventions
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
PDF uploads are stored as markdown wrappers with pdf_source or pdf_form_source markers so the editor can preserve extracted text, form fields, and annotations. The library exposed that internal wrapper: auto-created PDF documents used the hashed storage filename as the title, and row/facet language reported markdown instead of pdf.
Derive chat-upload PDF titles from the original upload name, derive document-library display language from the PDF source marker for rows, filters, and facets, and keep markdown wrappers excluded from the markdown facet when they represent PDFs.
The expanded library card already renders PDF-backed documents through /api/document/{id}/render-pdf. Allow only that inline PDF preview endpoint to be framed by same-origin app pages while leaving normal routes on X-Frame-Options: DENY and frame-ancestors none.
Also tighten the existing PDF marker regression assertion so it matches the actual historical corruption signature instead of contradicting the preserved [Page 1 text]: marker.
Fixes#2468
#2753 made the agent loopback base port-configurable but only for
_COOKBOOK_BASE in tool_implementations. Several other in-process loopback
calls still hardcoded http://localhost:7000 and broke off port 7000:
cookbook_serve_lifecycle (model-endpoints x2, shell/exec), builtin_actions
(model/serve), task_routes (calendar x3), and the gallery/email calls in
tool_implementations.
Extract the resolution (ODYSSEUS_INTERNAL_BASE / APP_PORT / 7000 fallback,
127.0.0.1 to avoid IPv6 ambiguity) into core.constants.internal_api_base()
and route every call site through it. Rename the now-misnamed _COOKBOOK_BASE
to _INTERNAL_BASE since it serves gallery/email/calendar/serve too. Adds a
test for the resolver plus a regression guard against reintroducing the
literal.
Part of #2752.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* fix(security): harden untrusted_context_message against delimiter spoofing
Root cause: untrusted_context_message() did not sanitise content before
interpolating it into the <<<UNTRUSTED_SOURCE_DATA>>> / <<<END_UNTRUSTED_SOURCE_DATA>>>
delimited sandbox block. Malicious content embedding the literal delimiter
strings could prematurely close the sandbox and inject instructions that
the LLM treats as trusted.
Fix: add _escape_guard_markers() helper that replaces the guard marker
strings with structurally inert tokens (<<<_UNTRUSTED_DATA>>> and
<<<_END_UNTRUSTED_DATA>>>) before the content is wrapped. The function is
applied in untrusted_context_message() after casting content to str.
The existing ~13 call sites (chat_processor.py, agent_loop.py,
deep_research.py, chat_helpers.py, chat_routes.py) are unaffected because
they pass content through without inspecting the output delimiters.
Regression tests added in tests/test_prompt_security.py covering:
- _escape_guard_markers unit tests (open, close, both, benign passthrough)
- untrusted_context_message integration tests (delimiter spoofing
neutralisation, type coercion, None handling, metadata preservation)
Resolves#3056
* fix(security): sanitize label for newlines and guard markers
Addresses reviewer feedback on PR #3086:
- Normalize label: strip CR/LF to prevent pre-guard line injection
- Escape guard marker literals in label via _escape_guard_markers()
- Add regression tests for label-based newline injection, GUARD_OPEN
and GUARD_CLOSE in label, and exactly-one-structural-guard assertion
* fix(security): move Source label inside GUARD_OPEN block
The reviewer correctly identified that even after sanitizing the label,
any user-derived label text (e.g. `f"web page: {url}"`) still appeared
before GUARD_OPEN in the trusted framing zone, where the LLM treats it
as trusted instructions.
Fix: move the 'Source: {label}' line to inside the guarded block so
only the hardcoded UNTRUSTED_CONTEXT_HEADER sits before GUARD_OPEN.
The raw label is still kept in metadata["source"] for traceability.
_sanitize_label() and _escape_guard_markers() are kept for defence-in-
depth on the label stored inside the block.
Update test_label_newline_injection_is_blocked to assert no label-
derived instruction text appears before GUARD_OPEN (pre-guard zone is
now empty of any user-derived content).
* refactor(tests): replace local function copies in test_endpoint_resolver with real imports
The test file carried 9 verbatim copies of src/endpoint_resolver.py functions
to avoid import-pollution concerns, but these copies are a drift hazard — PR #3343
had to update both in parallel. Replace them with direct imports so future changes
to endpoint_resolver are automatically exercised by the test suite.
Also fixes _ollama_api_root in endpoint_resolver.py: the bare-URL Ollama case
(e.g. http://nas:11434 with empty path) was already handled correctly in the test
copy but was missing from the real function, which would return /chat instead of
/api/chat for native Ollama endpoints without an explicit /api prefix.
Closes#3351
* refactor: import _ollama_api_root from llm_core instead of duplicating it
endpoint_resolver already imports _detect_provider and _host_match from
llm_core. Add _ollama_api_root to that import and remove the local copy,
collapsing two implementations to one source of truth.
llm_core's version is a superset (also strips /api/chat|tags|generate paths),
and since normalize_base already removes those suffixes upstream the result
is identical for every input used here.
The document editor stores the AI-edit diff state (_diffModeActive,
_diffOldContent, _diffNewContent, _diffChunks) as a module-global
singleton bound to whatever document was active when the diff opened,
and every document shares one #doc-editor-textarea. When the active
document is switched while an unapproved diff is open, the stale diff
must be discarded first or a later exitDiffMode (tab switch /
Accept-Reject-All) flushes the old document's content into the new
active document and overwrites it (issue #2467).
Guard both paths that switch the active document for an AI update,
while activeDocId still points at the previously-active doc:
- handleDocUpdate(): a doc_update targeting a different document.
- streamDocOpen(): the AI streaming a NEW document — this runs first on
that path, so a guard only in handleDocUpdate would fire too late and
still overwrite the streamed document.
Both reuse the exact `if (_diffModeActive) exitDiffMode(true);` guard
switchToDoc() and enterDiffMode() already use.
Fixes#2467
Ocean Bennett