feat(assistant): back the Assistant's agent chat with Astrolabe - #266
Conversation
The action promised a document summary and delivered a page. Measured against a 216k-character, 289-chunk PDF, the MCP server's chunk-context endpoint returned about 4k characters at its widest window and reported has_more_after=false — "more" means more of this page, not of the document — and it rejects offsets that don't match an indexed chunk, so the document cannot be walked a window at a time either. The retrieval primitive is page-shaped, so the honest fix is to say so: the button is now "Summarize section", and the result states the scope it covers as well as the tier that produced it. The previous comment claiming this asked for the widest window the server would serve implied roughly 20k characters of document and was misleading about what is actually read. Whole-document summarization is not a bigger window here. It needs the model to fetch the file itself through MCP tools, with this app orchestrating the request rather than moving document bytes — which makes it a use of the agent rather than a separate retrieval path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Groundwork for the agent, and for whole-document summaries by way of it: the model reaches a user's Nextcloud through the MCP tool catalogue rather than this app fetching and processing files. The client is per-user by construction. The server scopes every result by the token's `sub` and filters tools/list by its scopes, so a shared credential would leak one user's data to another — which is exactly why this cannot be delegated to Context Agent, whose MCP config holds one credential per instance. Two things the transport must not conflate: `mcp_server_url` is the address this server can reach, while the token is minted against `mcp_server_public_url` because that is what the MCP server validates `aud` against. Getting them backwards yields either a valid but unusable token or a URL that resolves nowhere. The SDK is loaded through a narrow autoloader rather than vendor/autoload.php. Requiring the whole tree let a dev copy of doctrine/dbal shadow the server's and QueryBuilder::select() began throwing instance-wide, so Nextcloud returned 500s on endpoints having nothing to do with this app. Only the SDK's own namespaces are mapped; anything the server provides is left to the server. astrolabe:mcp-probe exists because the catalogue is filtered server-side per token: what a user can actually do is not answerable by reading this code, only by asking. Also sets OIDC_ISSUER in the e2e stack. The oidc app builds `iss` from scheme and host only, never the port, so on any non-standard port the issuer loses it and /mcp rejects the token — while the management API keeps working, because it deliberately skips the issuer check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The MCP client hung on connect against nextcloud-mcp-server with "Initialization failed: Request timed out", despite the same PSR-18 client, URL and token succeeding when the protocol was driven by hand. The cause is SSE framing, not transport. sse-starlette — which the Python MCP SDK builds on — terminates events with CRLF, while HttpTransport scans only for "\n\n". No event boundary is ever found, the response sits unparsed in the buffer, and the fiber waiting on the initialize reply times out. The SSE specification permits CRLF, LF or CR, so the parser is non-compliant rather than merely unlucky; it went unnoticed upstream because the client is only exercised against the SDK's own PHP server, which emits LF. The patch recognises all three terminators and flushes a trailing event when a stream ends without a blank line. Applied via composer-patches so it survives a clean install and can be dropped when a release carries the fix. Worth recording that my first hypothesis — that our buffered PSR-18 response was starving an incremental reader — was wrong. A buffered response works fine; the bytes were simply never recognised as an event. The escaped newlines in my own diagnostic output hid the carriage returns that would have shown it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Registers an ISynchronousProvider for the core task type core:contextagent:interaction. Assistant decides it has an agent purely by asking whether that task type has a provider, so this is enough to take over "Chat with AI" — no AppAPI, no ExApp, no context_chat. Verified end to end: the model chose nc_semantic_search itself and answered from the user's own note in about twelve seconds. Read-only by construction. The MCP token carries read scopes, and the server filters tools/list by them, so the model is never offered a tool that writes. That is what lets calls execute immediately with no confirmation round-trip — there is nothing to confirm — and it is why the scope setting is not a mere preference: adding a write scope without building that round-trip would let the model change user data unasked. Registration is gated on an admin opt-in, because a task type counts as available the moment any provider registers and silently rewiring every chat on install would be rude. Iteration and wall-clock caps are configurable; the loop reports which one it hit, since one is the user's cue to narrow the question and the other the admin's cue to raise a setting. Two things the wire format demanded, neither obvious from the task-type docs. Tool calls arrive with an `args` key rather than `arguments`, and reading the wrong one silently invokes every tool with no arguments — which presents as the model asking useless questions. And tool results cannot go through the `tool_message` slot at all: integration_openai appends it with role=tool straight after the user message without ever emitting the assistant turn that carried the tool_calls, and places history before the user message so the missing turn cannot be injected. Mistral rejects that outright with "Unexpected role 'tool' after role 'user'". Results are therefore restated as user text, which works against any provider at the cost of the formal call/result linkage. Conversation history is stored per user, keyed by an opaque token, and expired by a timed job — the Assistant never signals that a chat has ended, so nothing else would ever delete the transcripts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Astrolabe is a context and tool provider here; the Assistant owns the conversation, its persona and its user-facing instructions. The system prompt had drifted into telling the model how to behave in a chat, and it showed: asked "is that deployed yet?" about a service mesh, the model answered "no, I am a prototype and not yet deployed" — inventing a persona that competes with the app the user is actually talking to. The prompt now covers only what is true of these tools and could not be known otherwise: that they read the user's own content, which to prefer, how to fill their parameters, and that they cannot write. Cutting it back further was worse, not better — with no guidance on parameter types the model began calling tools with malformed arguments, and those errors surfaced to the user. Tool usage is ours to document because we supply the tools; conversation is not. Also declares the `memories` optional input. ChatService only sends the Assistant's recollection of earlier chats when a provider declares that key, so this is the sanctioned way to receive that context rather than reconstructing it ourselves. It is appended to the prompt as context about the user, not as instructions about the tools. Conversation history itself was never broken: the token round-trips through Assistant correctly and turns accumulate in one row. The follow-up failures came from a prompt that told the model to "answer from" tool output, which reliably produced a restatement of the previous answer instead of a reply to what was asked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Weighted towards the two places bugs actually appeared. Tool calls arrive with an `args` key rather than `arguments`, and reading the wrong one silently invokes every tool with no arguments — which presents as the model asking useless questions rather than as a parsing bug, so it is worth pinning across all the shapes providers emit. The budget paths matter for the same reason: the loop previously reported "ran out of time" after six seconds because it conflated the iteration cap with the clock, and those two need different advice. Also covers the folding of tool output into the prompt, which exists only because the task type's tool_message slot is unusable with this provider, and which a future reader would otherwise be tempted to "simplify" back into the slot. Two harness notes worth keeping in mind when extending this: a PHPUnit stub keeps its first configuration, so per-test settings must be bound before the mock is configured rather than re-stubbed; and captured tasks are returned via an accessor because returning the array yields a copy taken before any turn ran. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @cbcoutinho's task in 2m 14s —— View job Reviewing PR #266
|
The sources popover listed tool names, and the "Source: note #78" line in the answer was model prose — text that reads like a citation whether the document was consulted or invented. Neither told a reader what was actually used. Citations are now built from the structured rows a tool returned: the popover names documents, and the answer carries markdown deep links into the chunk viewer, positioned at the passage where the tool reported offsets. A citation therefore exists only if a document really came back. The links go in the message body because that is the only place Assistant renders markdown — its sources popover interpolates plain text, where a link would print as its own label. The collector is built per turn rather than injected. It is stateful and a TaskProcessing worker is a long-running process serving many tasks, so a container-shared instance would carry one user's citations into another user's answer. Tools that omit doc_type are not cited, and deliberately not worked around. Inferring the type from the tool name was implemented and then removed: it copies the server's tool taxonomy into this app, where it rots as tools are added or renamed, and a wrong guess links confidently into the wrong app. Today that means nc_notes_search_notes results are uncitable, so the collector records which tools returned uncitable rows and the loop logs them — the gap is observable rather than silent. Tracked against nextcloud-mcp-server as the doc_type card. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e agent Round-3 review findings. **History was the one unbounded budget.** Tool output, iterations and wall-clock are all capped, but every turn was appended to the stored history and the whole thing resent on the next turn, so an active conversation grew its own prompt until it hit the provider's context window — paying for the excess on every call before it got there. The 30-day expiry bounds how long a stale conversation lives, not how large an active one gets. Both what is sent and what is stored are now trimmed to the most recent turns, which is where a follow-up's referents live. **The admin opt-in had no UI.** The safety story rests on the agent being off until an admin turns it on, which is thin if the only way to find the switch is occ. Adds a settings form for the toggle, the requested scopes, and the two budgets, with the scope field explaining that it is the real limit on what the assistant can do rather than a preference. That form shares the connection form's external storage and listener rather than using Nextcloud's internal storage, which would otherwise be the obvious choice: DeclarativeManager::getStorageType() scans an app's schemas in order and returns the first schema's storage type once a field is not found in it, so a second form declaring a different type has that type ignored and then fails for want of a handler. Verified live — the form renders with correct types and values, a write round-trips to app config, and the toggle gates provider registration. Also gives astrolabe:mcp-probe a clean error for malformed --args instead of an uncaught JsonException. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round-3 findings addressed in
One thing worth recording, because it cost me a debugging cycle and will bite the next person: the new form uses the connection form's external storage and listener rather than Verified live rather than just in tests: the form renders with correct types and current values, Left as-is: psalm clean, 313 unit tests pass (up from 309), PHP CS Fixer and ESLint clean. |
Round-4 review findings. **The read-then-write on conversation history was a real race, not a theoretical one.** A turn takes as long as the model does, so two turns on the same token overlap easily; both read the same history and the later write replaced one it never saw. Verified against the live stack: with four TaskProcessing workers a single follow-up was picked up twice and answered twice, and before this change one of those turns would have vanished. The column now carries a revision, the update is guarded on the revision it read, and a write that loses re-reads and appends on top of the winner so both turns survive. `updated_at` could not serve as the guard — it is second-granular, and overlapping turns routinely finish inside the same second. The migration adds the column to an existing table rather than returning early, so an instance that ran an earlier build of it does not keep a table the guarded update cannot write to. Appending moves the storage bound with it: the loop now returns just its own two turns and the row owns the trim, since a history computed before someone else's write is exactly what must not be written back. **Split the audience on connection failures.** The diagnosis names system config keys — precisely what the admin who broke the connection needs, and precisely what an ordinary user, who is the one actually sitting in the chat when it fires, can do nothing with. The detail goes to the log; the chat gets a sentence saying who can fix it. Also drops chat-turn3.md, a Playwright accessibility-tree dump committed by accident during the manual multi-turn verification. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round-4 findings addressed in
Left as-is: the timeout being checked only between iterations. psalm clean, 317 unit tests pass (up from 313), PHP CS Fixer and ESLint green. |
…e retry Round-5 review findings. **The audience split was applied in one place and not three.** The previous round moved the MCP connection diagnosis to the log on the grounds that the person reading the chat is whoever asked a question, not the admin who configured anything — but the model-unreachable, model-failed and tools-unlistable paths still concatenated raw provider text into the message. A test was even asserting that "provider quota exceeded" reached the user. All three now log the diagnosis and raise something the reader can act on, and the test asserts the opposite of what it did: the provider's words stay out of the message and stay in the log. **Covers the concurrency fix that round 4 called its headline change.** The retry policy — detecting a lost race, re-applying onto the winner rather than over it, appending this turn once rather than once per attempt, and giving up instead of looping — had no test, which is exactly where an off-by-one would hide. The two statements it drives are protected seams so the policy can be exercised without a database; the SQL itself still has no coverage, because there is no DB-backed tier here, and the PR description now says so rather than implying otherwise. ContextAgentProvider gains a test too: whose conversation is resumed, that an unknown token starts a fresh one instead of erroring, that a history which cannot be stored still returns its answer, and that Assistant's memories survive every shape it might send them in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round-5 findings addressed in
Left as-is: free-text scopes with no validation. Worth doing, but the useful version validates against what the MCP server actually advertises for that instance rather than a hardcoded list — the server gates some scopes on its own config ( psalm clean, 335 unit tests pass (up from 317), PHP CS Fixer and ESLint green. |
Round-6 review findings. **Tool output is arbitrary content out of the user's own documents**, folded back into the prompt as plain text, and Assistant's `memories` were concatenated straight onto the system prompt. A note containing a sentence shaped like an instruction was therefore indistinguishable from something the user actually asked. Both are now labelled as quoted material, and the PR description states the residual risk rather than implying it is solved: framing is mitigation, not a boundary. What bounds the damage is the scope design — the token is read-only and minted per user, so the model can only reach documents that user could already open, and can change nothing. **A bare catch made two very different failures look identical.** Falling back to "agent off" when the config table is not yet readable is right during installation; doing it silently means a broken container disables the agent permanently with nothing in the log to say so. Logged at debug, best-effort, since during installation the logger may be no more available than the config. **The model task claimed a made-up app id.** `astrolabe:agent` is not an app; appId is what Nextcloud attributes a task to in admin task lists and accounting. The kind of task belongs in customId, which is what SummaryService already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round-6 findings addressed in
psalm clean, 336 unit tests pass, PHP CS Fixer and ESLint green. |
It still described the design from before the storage type changed, so it told the reader INTERNAL storage was in use two lines above the schema setting EXTERNAL and the comment explaining why INTERNAL cannot work here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round-7: fixed the stale docblock in Thanks for tracing the retry loop by hand against the doubles — that's the part I most wanted a second pair of eyes on, since the live repro is not something the test suite can reproduce on its own. No other changes; psalm clean, 336 unit tests pass, cs:check and ESLint green. |
|
1 participant
What
Astrolabe now backs the Assistant's "Chat with AI", replacing Nextcloud's Context Agent, and drives
nextcloud-mcp-server's tool catalogue with per-user OIDC tokens.Verified end to end through the real Assistant UI, three turns:
Pronouns resolve across turns, and the third answer surfaced a fact absent from earlier ones, so it genuinely re-read rather than recycling context.
Why this can't be the Context Agent app
Context Agent's
MCP Configcarries one static credential per instance, and the Nextcloud docs state plainly that per-user tokens are unsupported.nextcloud-mcp-serverscopes every result by the token'ssuband filterstools/listby its scopes, so a shared credential would leak one user's data to another. Minting per-user is the whole reason this lives in Astrolabe.core:contextagent:interactionis a core task type, and Assistant decides it has an agent purely by asking whether that task type has a provider (ChatService::isContextAgentAvailable()). Registering is therefore sufficient — no AppAPI, no ExApp, nocontext_chat.Read-only by construction
The token carries read scopes only, and the server filters the catalogue by them, so the model is never offered a tool that writes. That is what lets calls execute immediately with no confirmation round-trip — there is nothing to confirm — and it is why the scope setting is a boundary rather than a preference. Adding a write scope without building that round-trip would let the model change user data unasked; the code says so where it matters.
Registration is gated on an admin opt-in (default off): a task type counts as available the moment any provider registers, so registering unconditionally would silently rewire every chat on the instance.
Things the task-type contract does not tell you
Both found by testing against a live server, both now covered by tests.
argskey, notarguments. Reading the wrong one silently invokes every tool with no arguments, which presents as the model asking useless questions rather than as a parsing bug.tool_messageslot is unusable withintegration_openai. It appends the slot withrole=tooldirectly after the user message without ever emitting the assistant turn that carried thetool_calls, and it placeshistorybefore the user message, so the missing turn cannot be injected either. Mistral rejects the result outright:Unexpected role 'tool' after role 'user'. Results are therefore restated as user text, which works against any provider at the cost of the formal call/result linkage. Arguably an upstream bug inintegration_openai.Prompt ownership
Astrolabe is a context and tool provider; the Assistant owns the conversation. An earlier iteration of the system prompt drifted into conversational instruction and the model answered "is that deployed yet?" with "no, I am a prototype and not yet deployed" — inventing a persona competing with the app the user is talking to.
The prompt now covers only what is true of these tools: what they read, which to prefer, how to fill their parameters, and that they cannot write. Cutting further was worse — with no guidance on parameter types the model began malforming arguments and those errors surfaced to users. Tool usage is ours to document because we supply the tools; conversation is not.
Also declares the
memoriesoptional input:ChatServiceonly sends the Assistant's recollection of earlier chats when a provider declares that key, so this is the sanctioned channel rather than reconstructing that context ourselves.Also in this branch
mcp/sdk) wired to Nextcloud's own PSR-18 client, so MCP traffic keeps its proxy/CA/SSRF handling. Loaded through a narrow autoloader: requiringvendor/autoload.phpwholesale let a dev copy ofdoctrine/dbalshadow the server's andQueryBuilder::select()began throwing instance-wide, so Nextcloud returned 500s on unrelated endpoints.patches/, viacomposer-patches):HttpTransportonly recognised"\n\n"as an SSE event boundary, butsse_starlette— and therefore every MCP Python server — frames with CRLF, soconnect()always timed out. Submitted upstream as modelcontextprotocol/php-sdk#401.OIDC_ISSUERin the e2e stack. Theoidcapp buildsissfrom scheme and host only, never the port, so on any non-standard port the issuer loses it and/mcprejects the token — while the management API keeps working, because it deliberately skips the issuer check. Half the integration looks healthy.chunk-contextreturned ~4k characters at its widest window and reportedhas_more_after=false— "more" means more of this page — and it rejects offsets that don't match an indexed chunk. It was never a document summary and no longer claims to be.astrolabe:mcp-probe, because the catalogue is filtered server-side per token: what a user can actually do is not answerable by reading this code, only by asking.Test coverage
Per CLAUDE.md's gate, stating the position rather than leaving it silent:
AgentLoopTestis weighted towards the wire format and the budget, because that is where both bugs above lived: every tool-call shape providers emit, malformed input, the iteration vs wall-clock distinction, tool-failure recovery, truncation, history threading, and disconnect-on-failure.AgentConversationMapperTestpins the optimistic-concurrency policy — how a lost race is detected, that the retry re-applies onto the winner rather than replacing it, and when it gives up — andContextAgentProviderTestcovers whose conversation gets resumed and what comes back when it cannot be.tests/unitis mocked-only, e2e is Playwright), so the policy is unit-tested and the statements themselves are covered only by manual verification — two entities that both read before either wrote, both turns surviving, revision landing at 2. Worth a real integration tier eventually; noting it rather than implying the SQL is tested./mcpinteractions are SSE-framed, which Pact models poorly, and provider verification of our own API is still blocked on the missing Pact provider-state endpoint (Deck board 11). Card #895 asks for thedoc_typeguarantee to be covered contract-side on the server, which is where that assertion belongs.integration_openaisupplies all three task types, so the blocker I recorded on the phase-1 PR is gone — but the specs themselves are still to write. Tracked on card #885.Citations are built, not narrated
Asking a model to name its sources produces text that reads identically whether the document was consulted or invented. So citations come from the structured rows a tool actually returned: the sources popover names documents, and the answer body carries markdown deep links into the chunk viewer, positioned at the passage wherever the tool reported chunk offsets.
The links live in the message body because that is the only place Assistant renders markdown — its sources popover interpolates plain text (
{{ source }}), where a link would print as its own label.The collector is constructed per turn, not injected. It is stateful and a TaskProcessing worker is a long-running process serving many tasks, so a container-shared instance would have carried one user's citations into another user's answer.
Tools that omit
doc_typeare not cited, and deliberately not worked around. Inferring the type from the tool name prefix was implemented and then removed: it copies the server's tool taxonomy into this app, where it rots as tools are added or renamed, and a wrong guess links confidently into the wrong app. Measured today,nc_semantic_searchandnc_semantic_search_answerare citable andnc_notes_search_notesis not. The collector records which tools returned uncitable rows and the loop logs them, so the gap is observable rather than silent — tracked as Deck card #895 againstnextcloud-mcp-server.Note the model still writes its own inline "Source: …" line above Astrolabe's verified block, so an answer can carry both. Accepted for now; only the appended block is traceable to a tool result.
Known gaps, deliberately not fixed here
memories. Both are now labelled in the prompt as quoted material rather than concatenated bare, which removes the easiest version — where planted text is indistinguishable from what the user asked — but framing is mitigation, not a boundary, and a determined injection can still steer the turn. What bounds the damage is the scope design rather than the prompt: the token is read-only, so nothing can be changed, and it is minted per user, so the model can only ever reach documents that user could already open. The realistic worst case is a steered answer that surfaces the user's own calendar or contact detail they didn't ask for, into a chat someone else may be looking at. Stated as accepted rather than solved.agent_scopestoday, but with no UI.tool_calls, so the turn ended on the narration. Two immediately following runs were clean. If it proves common, the loop could treat "no tool calls and no substantive answer" as a reason to prod once more.Verification
composer run psalmclean,composer run test:unit336 passing,cs:checkandnpm run lintclean,npm run buildsucceeds.Verified live against the e2e stack after each change, including a three-turn conversation through the real Assistant UI and citation links resolving to the cited passage.
This PR was generated with the help of AI, and reviewed by a Human