chat-ui

1f07fa5c - Port the ML Intern doctrine into the ML Assistant preset (#2526)

Port the ML Intern doctrine into the ML Assistant preset (#2526) * Port the ML Intern doctrine into the ML Assistant preset The preset shipped with a six-bullet prompt. This replaces it with the doctrine ported from ml-intern's system prompt — re-costed against this harness rather than copied, because the differences change what the rules can say. Three properties of chat-ui reshaped the port: - Ten tool rounds per turn, not three hundred. A literature-first crawl does not fit, so the doctrine grounds the claims it is about to act on rather than everything it says. The budget is raised to 100 for the preset alone; an ordinary conversation still stops at ten, and exhausting the budget still ends the turn with an apology. - A human is present, so the autonomous-mode block is the branch that does not apply, and "ask instead of assuming" routes through ask_user_question rather than hoping. - No sandbox and no shell. Rules keyed to bash, read-before-edit or /app paths are dead text here and are dropped; develop-small-launch-big survives as a short job before a long one. A test asserts the prompt names no tool this harness lacks. In the mode, buildToolPreprompt SWAPS its doctrine paragraphs rather than being joined by a second prompt: the generic restraint rule tells the model not to call a tool unless it lacks a capability, and names writing code as a case to answer directly, which is the inverse of the preset's doctrine. Both in one system message is a contradiction no amount of added doctrine fixes — and it is why #2524's GitHub tools needed `exemptFromToolRestraint` at all. In the mode there is no blanket restraint left for a tool to be exempt from. It is a swap inside the one builder, not a second builder, deliberately. Everything else that function sends — the tool list, the clock, the per-builtin guidance from #2514 and #2524, the image rules — has to reach the model either way, and a parallel copy silently stops carrying whatever is added to the other one next. A test pins that builtin guidance survives in the mode. The session context line is stamped in resolvePreprompt, not onto the tool preprompt, so it still reaches the model on the plain generation path. It lands last, after the artifacts prompt, and says User=unknown explicitly when there is no username: the namespace rule keys off that exact value to decide whether to call hf_whoami or ask. push_to_hub is stated twice for the reason ml-intern stated it three times, and a test pins the repetition. The composed system message is ~3,800 tokens and re-sent every round of every turn. A test holds it under an 18k-character ceiling so the next paragraph is a decision rather than a drift. Peter's calls, for the record: the artifact/payload split for scripts is in the prompt; MCP tool descriptions stay HF's, with doctrine in the preprompt only; the research sub-agent is deferred. * Put the job submission contract beside the job tool The densest and most expensive-to-violate text in ml-intern's set is the hf_jobs pre-submission contract, and it does not belong in the preset prompt: it is sent only when the tool it governs is on offer, so a run without hf_jobs never reads a contract for something it cannot do. Mechanism note, because the plan assumed otherwise: #2514's builtin-tool seam carries per-builtin guidance, but hf_jobs is an MCP tool served by hf.co/mcp. So the doctrine is keyed on the tool name in the resolved tool list instead — which means this does not depend on #2514, and extends to any MCP tool worth a contract. hf_fs_write gets one too. The rules are grounded against the current docs rather than recalled, which is what the prompt tells the model to do: - Pushing to the Hub from inside a job needs HF_TOKEN passed explicitly as a secret. Leave it out and the run trains for an hour and fails at the push. This is a harness-specific failure ml-intern never had, and the most expensive one available here. - The default flavor is cpu-basic. A training job that names no GPU flavor does not fail, it crawls. - Hardware guidance is a ladder, not a price table: t4-small for smoke tests through a100-large for real training, with a pointer to hf://docs/hub/jobs-pricing.md for rates. A table in a prompt goes stale silently; a pointer does not. Tests pin the pointer. hf_fs_write recovers the read-before-edit rule in a form this harness can actually back: read the file, then pass the parent commit SHA, so a concurrent change fails loudly instead of being silently clobbered. Both surfaces used to tell the model to print a pre-flight checklist, which would produce two. The prompt owns the instruction to print one; the contract adds its lines to it. The system message ceiling moves 18k -> 19k. Measured at ~17.4k with every preset tool on offer, and the raise is the decision the ceiling exists to force. * Point the preset at the HF MCP endpoint that can authenticate The preset hard-coded https://hf.co/mcp. `isStrictHfMcpLogin` matches on the exact URL including `?login`, and it gates two things: whether the signed-in user's HF token is forwarded to the server, and whether the server card offers the login control at all. Without the parameter the mode's Hub tools run anonymously — no whoami, no jobs, no writes — and there is nothing in the UI to log into, which is how this was found. Worse than getting itself wrong: the preset wins the name collision in withMlAssistantServers, so it would have replaced the `?login` entry that chart/env/prod.yaml and dev.yaml already configure. Turning the mode on in production would have silently downgraded a logged-in HF MCP server to an anonymous one. A test now asserts the preset's HF URL satisfies the same predicate the forwarding checks, rather than asserting the string. * Stop persisting live-only tool progress updates A conversation is one MongoDB document and Mongo caps a document at 16MB. Past that ceiling every write fails — not slowly, permanently: the conversation can never be added to again. That ceiling is reachable, and one ML Assistant run got to 94.7% of it. Polling a training job produced a single assistant message carrying 73,994 updates and 15.1MB, of which 59,487 were MCP progress notifications — 80% of the volume, and 66.6% of the whole document. Progress is live-only UI. Once the result is stored, a progress tick tells a reader nothing the result does not, yet it was persisted forever and replayed to the browser on every load. Dropping it takes that conversation from 15,881,222 bytes to 5,297,794, and its update count from 75,957 to 16,163. Only what is persisted changes; everything still streams to the connected client exactly as before. Also adds a 5,000-update cap as a backstop for whatever the next unbounded emitter turns out to be. Over the cap, stream length-markers go first — they carry no text, that lives on the message, and only record where a tool card sat relative to it. Tool calls and their results are the transcript, so they go last, and the most recent survive. compressUpdatesForStorage moves out of the route into $lib/server/generation so it can be tested directly. replayRoundTrip still passes, which is the contract that matters: it drives the real persistence path end to end and would catch a field that no longer survives the round trip. This treats the symptom. The cause is that the agent has no way to wait for a job, so "check back later" becomes 1,096 log calls at one every two seconds. That fix belongs in the loop, not here. * Add the parked-call record and a wait tool, still dark Groundwork for server-woken background tasks. The agent currently has no way to wait: it decides to check back later, the only action available is to call the tool again, and "wait" becomes 1,096 log calls at one every two seconds. What this adds: - `ParkedCall`: a tool call that ended its turn and expects to be resumed, with `resumeAt` and a `kind` discriminator. Its own record rather than a flag on the elicitation row, because MCP tasks are the expected second kind and calling one a timer and the other an elicitation would read as wrong the moment both exist. Indexed on `{status, resumeAt}` — that pair is the sweep — with a 7-day TTL. - The builtin seam carries conversation and owner identity. A resume runs with no request to read an identity from, so a tool that parks has to record whose turn it was, while there still is one; the resume then acts as the user who parked rather than as whatever swept it. - A `wait` tool that creates the record and parks, with the caps that stop a wait-loop replacing the poll-loop: 15s floor (below it, just call the tool again), 30-minute ceiling per hop, and 40 hops per conversation. Park, resume, park again costs a turn each time and never returns to the user, which is the same failure the repetition guard exists for. The tool is deliberately NOT registered in getEnabledBuiltinTools. It parks a turn, and nothing wakes one yet — enabling it before the sweeper lands would strand runs rather than fix them. Still to come: the sweeper, and the extraction it depends on. A resumed turn has to produce a persisted message the same way an HTTP one does, and today that logic lives in a closure inside the route's POST — it merges pre-tool streamed content, sets interrupted, persists titles. That is worth extracting rather than duplicating into a second implementation that will drift, and it is a refactor of the hottest path in the app, so it wants its own pass with the round-trip tests green. * Extract the update accumulator out of the conversation route Groundwork for waking a parked turn. A resumed turn has to produce a persisted message the same way an HTTP turn does, and that logic lived in a closure inside the route's POST — so the sweeper's only options were to reuse it or to grow a second copy that drifts. Drift here means messages that persist subtly wrong. `applyUpdateToMessage` now owns the folding: stream tokens into content, reasoning, the title strip, the final-answer merge, files, router metadata, and the updates array. It does no I/O. The title write, the generation writer, metrics, the wire padding and the SSE enqueue stay with their callers, because those are exactly what differs between a turn driven by a request and one driven by a sweep. Two details preserved deliberately, both easy to lose in a rewrite: - An empty stream token is dropped whole. It reaches neither the updates array nor the log nor the client, so the extracted function reports `skipped` rather than merely leaving content untouched. - The final answer replaces only what THIS turn produced. `initialContent` is the boundary, which is what lets a resumed turn continue a message that already says something instead of overwriting it — the case the wait tool depends on. The pre-tool merge (A/B/C) was previously only covered through the end-to-end round trip. It now has direct tests, including the case a resumed turn hits. replayRoundTrip stays green, which is the check that matters: it drives the real persistence path and would catch a fold that stopped surviving the trip. * Wake parked turns with a server-side sweeper Turns the wait tool on. The agent can now stop and be resumed instead of polling: it says how long and why, its turn ends, and a sweeper wakes it. How a wake works. `claimDueCall` moves a due row out of `waiting` with the status in the filter, so two pods racing on one row produce a winner and a miss rather than two resumed turns. The claim then rebuilds the identity the turn ran as — user, session token, settings — because there is no request to read one from, which also means a resume can only ever act as the user who parked it. It injects the tool result the parked call has been missing, paired to the original call by uuid so replay puts it in the model's history, and runs the turn through the same `applyUpdateToMessage` an HTTP turn uses. Notes on the parts that were decisions rather than mechanics: - An expired session does not drop the turn. The token can easily expire during a long wait, and silently failing every Hub call afterwards would be worse than saying so — the model is told in the tool result, so it can report it instead of retrying into 401s. - A row that keeps failing is abandoned after three attempts rather than retried forever. A parked turn that cannot resume is a turn nobody is waiting on. - The sweep interval is the floor on how late a wake can be, so it stays well under the shortest wait the tool will accept. Tested: the claim is exercised by two concurrent sweeps over three due rows, asserting one claim each; plus the not-yet-due, conversation-gone and gave-up paths, and the tool's caps and record shape. Not yet covered by an automated test: a full resumed turn end to end, which needs the model and the tool server scripted the way replayRoundTrip does it. That is the next test to write, and until it exists this wants exercising by hand before it goes anywhere near prod. * Teach the preset to cost a run and to read what a paper builds on Four gaps the first real run exposed, all prompt-only. **Hardware was chosen once and never reasoned about.** Every job went to t4-small — because the ladder said "smallest" and never said "how long". The doctrine now reasons in cost to FINISH rather than cost per hour: a GPU at twice the rate that trains three times as fast is both cheaper and sooner, so cheapest-per-hour is rarely the right default for real training. The smoke test already measures steps per second, so the real run's wall-clock is arithmetic — it goes on the pre-flight list with what it will cost, every time. Above about half an hour, or where a faster flavor would materially change the estimate, the choice goes to the user through ask_user_question with the numbers in the options. It read the pricing table in that run and still never told the user a rate. **It followed no citations.** Every research read in 1,166 calls was the same paper. The whole latent-space-versus-image-space failure was a misreading of the flow-matching target — standard prior work, one hop away, ~40 minutes and several dead jobs to rediscover. A bounded hop is now doctrine: read the one or two papers a method builds on before implementing it. Cut originally because it did not fit ten rounds; at a hundred, with `wait`, it does. ml-intern's attribution rule comes back with it. **It searched for a paper with hub_repo_search.** A repo search, twice, concluding nothing was there. hf_fs now carries where papers live and what hub_repo_search is actually for. **Web search would have arrived unguided.** The mode replaces the generic tool preprompt, SEARCH paragraph included, so configuring Exa would have handed the model search tools with nothing said about them. It gets its own block, keyed on the tool, so it appears only where the tool does. The size ceiling fired at 19,774 against 19,000, which is what it is for. Raised to 22k deliberately, with the reasons recorded, and the measurement now composes every tool at once so the ceiling actually binds. * Stop the preset probing permissions it does not have Four fixes, all from watching a real run thrash between calls 31 and 49 trying to find somewhere to put its artifacts. It even searched GitHub for `huggingface/mcp-server` — going after the MCP server's own source to work out how the tools behave, which is the model saying the doctrine does not tell it enough. **Create before writing.** `put` does not create a repo, and writing where nothing exists fails with "Repository not found" — which reads like a permissions problem and is not. Identified after the previous run and not fixed then; it caused one of these two failures outright. **Work in repos you created.** The other failure was an authorization error on a bucket a different app had made. The token carries `contribute-repos`, which is documented as "create repositories and access those created by this app" — so a pre-existing repo or bucket is exactly what it excludes. prod.yaml picks that scope deliberately over `write-repos`, so this is a product boundary, not a misconfiguration. It was simply invisible to the model. **Permission errors are not retries**, as a named failure mode. Told 403, it tried the same call, then a variant, then another tool needing the same permission. A permission you do not have does not appear on the second attempt. **Sandboxes as an optimisation, never a dependency.** A sandbox gives feedback in seconds where a job queues and pulls an image before mentioning a typo, so it is the right place for smoke tests — but availability depends on the account and the deployment, and `hf_sandbox` 403s here even though `jobs` is granted. So: use it if it works, fall back to a small `hf_jobs` run the moment it does not, and never report the task as blocked because of it. Keyed on the tool, so if the sandbox turns out to need a scope we are not requesting, granting it is all that has to change. Whether that scope exists is still open. `jobs` IS granted (confirmed via hf_whoami) and the sandbox still 403s; the public scope list has no sandbox scope but is provably incomplete, since this token holds `read-mcp`, which is not in it. HF does not advertise `scopes_supported`, so it cannot be settled from outside. The ceiling moves 22k -> 24k for headroom, not content: at 21,889 it fired on every edit. ~5,500 tokens re-sent on every round of a hundred is the number to watch. * Don't let a made-up timezone take the turn down From Codex review on #2526. `timezone` reaches the prompt from the request body validated only as `z.optional(z.string())`, and `Intl.DateTimeFormat` throws `RangeError` on a zone it does not know. Confirmed all three legs: the schema, that `resolvePreprompt` runs at index.ts:66 with the try only starting at line 80, and the throw itself. So a client posting `{"timezone":"garbage"}` to an ML Assistant conversation failed the whole generation, because the session-context stamp is built before the generation's own error handling. Both paths fall back to the server's zone now, and the stamp stops claiming a zone it did not honour. The generic tool preprompt had the same exposure and is fixed with it — there the throw is caught upstream, so instead of failing it quietly answered without tools, which is the same bug wearing a better disguise and harder to notice. * Drop comments that narrate the code they sit on Comment hygiene pass over the branch. Almost everything survives: the comments here carry reasons — why the preset swaps a paragraph rather than appending, why progress is not persisted, why a claim puts the status in the filter — and those are the ones that stop a future edit undoing a decision. The exceptions were all in `applyUpdate.ts`, and all inherited: they came across verbatim when the accumulator was extracted from the route, where they had labelled the branches of a long closure. In a function whose shape is now visible at a glance, "Set the title" above the title branch and "Add file" above the file branch say nothing the code does not. One is kept as a reason rather than a description: a title is stripped of think markers because a reasoning model will put them there, which is the part a reader cannot see from the replace() itself. * Fix five review findings, one of them a regression I introduced From Copilot review on #2526. All five were real; the first is the one that matters. **The extraction dropped `writer.push` and the wire padding.** Cutting the accumulation out of the route's update closure took two caller-only steps with it. Every turn since has written nothing to the generation log — so the reattach endpoint has no events to replay and `materializedSeq` never leaves zero — and stream tokens went to the client without the fixed-length padding that mitigates the packet-length side channel. The evidence was in front of me and I read past it: every generation on this branch since the extraction has `seq=0` and zero events, while runs from a main-based worktree against the same database log normally. I had noticed a `seq=0` during the sweeper smoke test and called it "odd but consistent". It wasn't. The log is now covered by a test in replayRoundTrip, which drives the real route. Verified it fails without the fix ("expected 0 to be greater than 0") rather than assuming it would: nothing else in that suite reads the log, which is why the regression passed 996 tests. **A claim was a one-way door.** `claimDueCall` filtered on `waiting` only, so a resume that died before its own error handling left the row in `resuming` until the TTL, and the attempt ceiling could never apply. A claim is now a five-minute lease, and an expired one is fair game. **A parked turn was recorded as an error.** Timer parking emits no elicitation request, so `showedPrompt` stayed false, and with no text produced the no-output guard called a correctly-parked turn "No output was generated". The guard now also asks whether the turn made a tool call. In the sweeper, a resumed turn that parks again no longer gets a Finished status stamped over it. **A resumed run was unreachable.** The browser finds a running turn through the last assistant message's `generationId`, and the sweeper left the parked turn's id in place — so the resumed work only appeared on a manual refresh. This is the mechanism behind the missing stop button. The resumed run now claims the message before it starts producing. **Each .env comment documented the other variable's setting**, having been inserted between an existing comment and the line it described.