1338 lines
77 KiB
Markdown
1338 lines
77 KiB
Markdown
# What changed
|
||
|
||
## 1. Root cause of the bug
|
||
|
||
`agent: round 1: llm: ollama error: XML syntax error on line 5: element closed by`
|
||
is Ollama's own Go-side `encoding/xml` parser failing while rendering Qwen's
|
||
tool-calling chat template (which wraps tool calls/results in XML-ish
|
||
delimiters). Two things made this worse and are now fixed:
|
||
|
||
- `compactToolHistory` in `services/agent/internal/infrastructure/agentrun/agent.go`
|
||
truncated old tool results with a blind byte slice, which could cut a tag in
|
||
half. Fixed with `safeHeadCut`/`safeTailCut`, which snap to the nearest
|
||
`<`/`>` (and a valid UTF-8 rune boundary) so a truncated tool result can
|
||
never hand back a dangling, malformed tag.
|
||
- There was no bounded retry for this specific, usually-transient error class.
|
||
Added `ollamakit.IsTemplateParseError` + a bounded 2-attempt retry of the
|
||
same round in `Agent.Run`, instead of failing the whole turn on the first
|
||
occurrence.
|
||
|
||
## 2. Word / Excel / PowerPoint document management
|
||
|
||
New package **`pkg/officekit`** (stdlib only — `archive/zip` + `encoding/xml`,
|
||
no new dependencies) that creates and edits **real** `.docx`/`.xlsx`/`.pptx`
|
||
files. Nine new agent tools, modeled on Claude Code's document-editing
|
||
skills:
|
||
|
||
- `create_document` / `read_document` / `edit_document` (Word)
|
||
- `create_spreadsheet` / `read_spreadsheet` / `edit_spreadsheet` (Excel)
|
||
- `create_presentation` / `read_presentation` / `edit_presentation` (PowerPoint)
|
||
|
||
**Design choice that structurally prevents this tool family from
|
||
reintroducing the Ollama bug:** the model only ever passes plain structured
|
||
fields (a paragraph's text and style, a grid of cell values, a slide's title
|
||
and bullets) and gets back a short plain-text summary. It never authors or
|
||
receives OOXML/XML — `pkg/officekit` does 100% of the actual XML generation
|
||
and parsing, server-side, using Go's typed XML encoder (always well-formed,
|
||
always correctly escaped).
|
||
|
||
Files persist through the workspace service's **existing**
|
||
`UploadFile`/`DownloadFile` gRPC RPCs (binary-safe `[]byte` fields, already
|
||
present on `WorkspaceServiceClient`) — so this required **zero protobuf/gRPC
|
||
changes**.
|
||
|
||
New/changed files:
|
||
- `pkg/officekit/zipkit.go`, `docx.go`, `xlsx.go`, `pptx.go` (new)
|
||
- `services/agent/internal/infrastructure/toolexec/office.go` (new)
|
||
- `services/agent/internal/infrastructure/toolcatalog/catalog.go` (+9 tool defs)
|
||
- `services/agent/internal/infrastructure/toolexec/executor.go` (+9 dispatch cases)
|
||
- `services/agent/internal/infrastructure/agentrun/agent.go` (system prompt + safe-cut + retry)
|
||
- `pkg/ollamakit/client.go` (`IsTemplateParseError`)
|
||
|
||
## Verification performed
|
||
|
||
No Go toolchain ships in the delivery environment by default, so I installed
|
||
one (`apt-get install golang-1.23-go`, from the already-allowed
|
||
`archive.ubuntu.com`) to actually verify this, rather than just asserting it
|
||
compiles:
|
||
|
||
- `pkg/officekit` builds and `go vet`s cleanly in isolation.
|
||
- A smoke test builds a `.docx`, `.xlsx`, and `.pptx` from scratch, edits each
|
||
(append paragraph + find/replace, sparse cell writes, add slide + update
|
||
slide), and reads every one back — including round-tripping text containing
|
||
literal `<`, `>`, and `&` correctly escaped.
|
||
- Every generated XML part in all three files was checked for well-formedness
|
||
(`xml.dom.minidom.parseString`).
|
||
- The **entire module** (`go build ./...` and `go vet ./...`, all services,
|
||
not just the touched ones) compiles cleanly against the real `go.mod`/`go.sum`
|
||
— I cloned the project's own pinned dependency versions from their GitHub
|
||
mirrors (matching what `go.sum` already recorded) to resolve them under
|
||
`GOPROXY=off`, since `proxy.golang.org` isn't reachable here.
|
||
- `gofmt -l` is clean on every touched file.
|
||
|
||
## 3. Follow-up: "expected element type ... but have ..." (same bug family, different phrasing)
|
||
|
||
Go's `encoding/xml` doesn't have one error shape — `IsTemplateParseError` was
|
||
only matching its tokenizer-level `SyntaxError`s ("XML syntax error on line
|
||
N: ..."), which is what "element closed by" was. "expected element type
|
||
`<foo>` but have `<bar>`" is a *different* error type from the same package
|
||
(an `UnmarshalError`, raised when struct-based decoding hits an unexpected
|
||
root element) — same root cause, no shared prefix, so the old substring
|
||
check missed it.
|
||
|
||
Fixed by generalizing the detector (`pkg/ollamakit/client.go`,
|
||
`IsTemplateParseError`) from an enumerated list of exact phrasings to the
|
||
actual shared signature: an "ollama error" whose message either mentions
|
||
"xml" or names an angle-bracketed `<tag>`-shaped fragment — the one place in
|
||
this error path literal element/type names show up, regardless of which
|
||
`encoding/xml` error type produced them. This should now catch future
|
||
variants (missing end tag, bad entity, duplicate attribute, etc.) without
|
||
needing another patch each time a new phrasing surfaces.
|
||
|
||
## 4. Office document skills: silent cell loss in `edit_spreadsheet`
|
||
|
||
**Symptom:** `edit_spreadsheet` (`SetXlsxCells` in `pkg/officekit/xlsx.go`)
|
||
would, on some calls, silently drop one of several cell writes aimed at
|
||
rows that didn't exist in the workbook yet — no error, no panic, the
|
||
RPC reported success, but the written `.xlsx` was missing a cell the
|
||
model had just asked for. It reproduced intermittently: the same
|
||
batch of writes would sometimes round-trip correctly and sometimes
|
||
lose a cell, which is what made it hard to pin down — `go test` first
|
||
caught it as a flake in `TestSetXlsxCellsFormulaAndOrdering`
|
||
(`pkg/officekit/officekit_test.go`), failing on roughly half of runs.
|
||
|
||
**Root cause:** `SetXlsxCells` indexed newly-created rows with a
|
||
`map[int]*sRow` — a pointer taken as `&ws.SheetData.Rows[i]` at the
|
||
moment a row was first appended. Go's `append` reallocates a slice's
|
||
backing array once its capacity is exceeded, copying existing elements
|
||
into the new array; any pointer taken into the *old* array before that
|
||
point keeps pointing at now-orphaned memory. A second write landing in
|
||
the same new row, processed after a later `append` triggered that
|
||
reallocation, mutated the stale pointer's copy — which nothing else
|
||
ever reads — instead of the live row. Since Go map iteration order is
|
||
randomized by design, whether a reallocation fell between two writes
|
||
to the same row (and so whether the bug fired) varied from run to run
|
||
and from request to request: the same tool call could succeed or
|
||
silently corrupt the file depending on map iteration order the caller
|
||
has no control over.
|
||
|
||
**Fix:** `pkg/officekit/xlsx.go`'s `SetXlsxCells` now indexes rows by
|
||
their integer position (`map[int]int`) instead of a pointer. An index
|
||
has no equivalent failure mode: `ws.SheetData.Rows[idx]` always
|
||
re-resolves against whatever backing array the slice header currently
|
||
points to, reallocated or not. Cell writes are also now applied in a
|
||
deterministic ascending (row, column) order rather than raw map
|
||
iteration order, so behavior no longer depends on Go's randomized map
|
||
ordering at all (the final on-disk row/cell order was already
|
||
restored by the existing `sortSheetData` sort, regardless; this change
|
||
makes the *write sequence* itself deterministic too, which matters
|
||
for debugging a batch of edits, not just for the final file).
|
||
|
||
Added `TestSetXlsxCellsManyNewRowsNoDataLoss`
|
||
(`pkg/officekit/officekit_test.go`): writes two cells into each of 200
|
||
brand-new rows — enough to force several slice reallocations — and
|
||
reads every one back. Verified against the pre-fix code that this
|
||
test fails deterministically (not intermittently) on the old pointer-
|
||
map version, confirming it would have caught this the first time.
|
||
`go test ./pkg/officekit/... -race -count=20` is clean after the fix.
|
||
|
||
This was the one correctness bug found in `pkg/officekit`; the rest of
|
||
the office-document path — `docx.go`/`pptx.go`'s append/replace
|
||
operations, `zipkit.go`'s zip part copy/replace, the
|
||
`toolexec/office.go` tool handlers, their `toolcatalog/catalog.go`
|
||
schemas, and the workspace service's binary-safe
|
||
`UploadFile`/`DownloadFile` RPCs — was reviewed and is sound: no other
|
||
instance of the pointer-into-a-growing-slice pattern exists anywhere
|
||
in `pkg/officekit`, every `create_*`/`read_*`/`edit_*` tool name in
|
||
the catalog has a matching dispatch case in `executor.go`, and
|
||
`AppendPptxSlide`/`AppendDocxParagraph` keep `[Content_Types].xml` and
|
||
the relevant `.rels`/`presentation.xml` parts in sync on every write
|
||
rather than letting them drift.
|
||
|
||
## 5. Office document skills: an unnecessary round-trip on every "create a document" request
|
||
|
||
**Symptom:** a simple, one-shot document request — e.g. "Crées un CV
|
||
document Word d'Ingénieur en Machine Learning" — took noticeably
|
||
longer than it needed to, even though `pkg/officekit` itself (the
|
||
actual `.docx`/`.xlsx`/`.pptx` generation) is pure in-memory
|
||
`archive/zip`/`encoding/xml` work and runs in low-single-digit
|
||
milliseconds — it's never the bottleneck.
|
||
|
||
**Root cause:** both the main `SystemPrompt`
|
||
(`services/agent/internal/infrastructure/agentrun/agent.go`) and
|
||
`read_office_skill`'s own tool description
|
||
(`toolcatalog/catalog.go`'s `officeSkillToolDescription`) *mandated* a
|
||
`read_office_skill` call "BEFORE your first create_document/
|
||
edit_document, create_spreadsheet/edit_spreadsheet, or
|
||
create_presentation/edit_presentation call in a conversation" — with
|
||
no exception for a plain, ordinary document. On a local model, a tool
|
||
call is a full inference round-trip, not a free lookup. So every
|
||
fresh conversation's first document request paid for an extra round
|
||
just to read a markdown file, before the model had even started
|
||
composing the document's actual content — on top of round 0's
|
||
`think=high` reasoning pass (`DefaultThinkPolicy`) and the round that
|
||
generates the document's full paragraph list. For a one-page CV, that
|
||
extra skill-read round added a full local-model generation pass that
|
||
contributed nothing a well-written tool schema couldn't already
|
||
convey.
|
||
|
||
**Fix (no behavior change for anything nontrivial, same two files):**
|
||
|
||
- `create_document`'s own description now inlines the handful of
|
||
conventions that actually matter for an ordinary document (lead
|
||
with one `heading1`, `heading2`/`heading3` for sections, `bullet`
|
||
for list-shaped content, `normal` paragraphs split by idea) plus
|
||
its honest limitations (no tables, images, headers/footers,
|
||
numbered lists, or mid-paragraph inline formatting) — the same
|
||
information `docx.md` already had, just paid for once, in the
|
||
schema the model already sees on every call, instead of behind a
|
||
mandatory separate round-trip.
|
||
- `read_office_skill`'s description and the matching `SystemPrompt`
|
||
bullet both changed from "call this before your first document
|
||
tool call" to "optional — skip it for a straightforward document
|
||
and call create_document/create_spreadsheet/create_presentation
|
||
directly; reach for this before something that leans on a format's
|
||
sharper edges (Excel formulas, a multi-sheet workbook, a
|
||
multi-slide deck, `edit_document`'s `replace_text` run-splitting
|
||
caveat), or when unsure a feature someone asked for is supported at
|
||
all."
|
||
|
||
Net effect: a plain "make me a CV/memo/simple spreadsheet" request now
|
||
costs one fewer full LLM round-trip than before — the skill files and
|
||
`search_office_skills` are both still there, fully documented, and
|
||
still the right call for anything that actually needs them (tables,
|
||
formulas, multi-sheet/multi-slide work, or edit-operation nuances);
|
||
they're just no longer a toll booth in front of the common case. This
|
||
is a prompt/tool-description change only — `pkg/officekit` itself is
|
||
untouched here (see §4 above for its one real bug fix) and the actual
|
||
per-request cost of generating a CV's worth of paragraph text with a
|
||
local model is inherent to doing that work at all, not something this
|
||
change (or any prompt change) removes.
|
||
|
||
Also added a second line of defense: if the plain retries don't clear it
|
||
(the same template error survives every fresh attempt — i.e. it's
|
||
deterministic, not transient), `Agent.Run` now blanks the most recent tool
|
||
result(s) in that round's local message history — the newest, and so most
|
||
likely, thing about the request that differs from the last one that worked
|
||
— and tries once more before giving up the whole turn
|
||
(`degradeRecentToolMessages` in `agentrun/agent.go`). This never touches
|
||
persisted conversation history, only the in-memory copy used for this one
|
||
request.
|
||
|
||
## Known limitations (documented honestly, not hidden)
|
||
|
||
- `edit_document`/`edit_spreadsheet`/`edit_presentation` only support a
|
||
deliberately small set of structured operations (append paragraph,
|
||
find/replace; sparse cell writes; add/update slide) — by design, so the
|
||
model never needs richer markup-shaped arguments.
|
||
- PowerPoint slides are two fixed, absolutely-positioned text boxes
|
||
(title + bullets) — no custom layouts, images, or manual positioning.
|
||
- `read_document`/`read_spreadsheet`/`read_presentation` extract plain text
|
||
only — tables, images, and complex formatting in a real user-uploaded
|
||
Office file aren't represented.
|
||
- No PDF support (there's no `create_pdf`/`read_pdf`; the `overview` skill
|
||
below tells the model to say so rather than silently substituting a
|
||
`.docx`/`.pptx`).
|
||
|
||
## 4. Office document *skills* (Claude Code-style progressive disclosure), and multi-sheet workbooks
|
||
|
||
The nine document tools from §2 already told the model *what fields each
|
||
tool takes* (the JSON-schema `Parameters`), but that schema is sent to
|
||
Ollama on **every single request**, so it has to stay terse — there was
|
||
nowhere to put the kind of guidance Claude Code's own `SKILL.md` files
|
||
carry: which style to use for a document's title, what `replace_text`
|
||
does and doesn't match, what a `bullet` can't do, how to pick between a
|
||
single- and multi-sheet workbook, and so on. Two concrete problems this
|
||
caused in practice: the model had no documented way to discover that
|
||
`create_spreadsheet` only ever wrote one sheet (see the limitation this
|
||
removes, below), and nothing told it to say "I can't make a PDF" instead
|
||
of quietly handing back a `.docx` when a user specifically asked for one.
|
||
|
||
**New package `pkg/skillkit`** (stdlib only — `embed` + `sort` +
|
||
`strings`) loads four short, human-editable markdown files
|
||
(`pkg/skillkit/skills/{overview,docx,xlsx,pptx}.md`) at build time via
|
||
`go:embed` and serves them through `List()` (name + one-line description,
|
||
for the tool's own description), `Read(name)` (one skill's full body),
|
||
and `Names()` (for the tool schema's `enum`) — the same
|
||
progressive-disclosure shape Claude Code uses: a one-line description is
|
||
always in context, the full skill is only loaded when the model actually
|
||
asks for it.
|
||
|
||
**New tool `read_office_skill`** (`toolcatalog.Defs()` +
|
||
`toolexec/office_skill.go`, dispatched in `executor.go` like
|
||
`write_todos`/`solve_ml_math` — pure in-memory lookup, no gRPC call)
|
||
takes one `name` argument constrained to `skillkit.Names()`. Its tool
|
||
description is *generated from* `skillkit.List()` at catalog-build time,
|
||
so the catalog and the skill files can never drift apart — renaming or
|
||
adding a skill file is the only edit needed. `SystemPrompt` in
|
||
`agentrun/agent.go` instructs the model to call it before its first
|
||
`create_*`/`edit_*` office tool call per conversation. Added to
|
||
`toolcatalog.subagentToolNames` too (read-only, harmless for a
|
||
sub-agent that already has `read_document`/`read_spreadsheet`/
|
||
`read_presentation`).
|
||
|
||
**Multi-sheet `.xlsx` workbooks, wired up** (closing the limitation
|
||
`BuildXlsx` already had unused room for): `create_spreadsheet` now
|
||
accepts either the original single-sheet `rows`/`sheet_name` pair *or* a
|
||
`sheets` array (`{name, rows}` per tab) — `office.go`'s new
|
||
`parseSheetsArg` normalizes either shape to `[]officekit.Sheet` before
|
||
calling `officekit.BuildXlsx`, which now actually writes every sheet
|
||
given (its own worksheet part, workbook `<sheet>` entry, and
|
||
`workbook.xml.rels` relationship each) instead of silently keeping only
|
||
`sheets[0]`. `read_spreadsheet`/`edit_spreadsheet` both gained an
|
||
optional `sheet` field (1-based index or name); resolving it reads the
|
||
target workbook's own `xl/workbook.xml` + `xl/_rels/workbook.xml.rels`
|
||
(new `resolveXlsxSheetPart`, with a same-as-before `sheet1.xml` fallback
|
||
if either part can't be parsed) rather than assuming sheet order, so it
|
||
also works correctly against a user-uploaded workbook whose sheets were
|
||
reordered in Excel. `read_spreadsheet`'s result now leads with a
|
||
`# sheet "Name" of N: A, B, C` line whenever the workbook has more than
|
||
one sheet, so the model (and the person reading the tool output) can see
|
||
what else is there without a second call.
|
||
|
||
New/changed files:
|
||
- `pkg/skillkit/skillkit.go`, `pkg/skillkit/skills/*.md` (new)
|
||
- `services/agent/internal/infrastructure/toolexec/office_skill.go` (new)
|
||
- `pkg/officekit/xlsx.go` (multi-sheet `BuildXlsx`; sheet-aware
|
||
`ReadXlsxCells`/`SetXlsxCells`/new `ReadXlsxSheetNames`)
|
||
- `services/agent/internal/infrastructure/toolexec/office.go`
|
||
(`parseSheetsArg`; `sheet` plumbed through read/edit)
|
||
- `services/agent/internal/infrastructure/toolcatalog/catalog.go`
|
||
(`read_office_skill` tool def + `officeSkillToolDescription`; updated
|
||
`create_spreadsheet`/`read_spreadsheet`/`edit_spreadsheet` schemas;
|
||
`read_office_skill` added to the sub-agent whitelist)
|
||
- `services/agent/internal/infrastructure/toolexec/executor.go`
|
||
(dispatch case)
|
||
- `services/agent/internal/infrastructure/agentrun/agent.go` (system
|
||
prompt)
|
||
|
||
### Design choices worth calling out
|
||
|
||
- **Markdown, not Go, is the unit of change.** Improving the guidance for
|
||
how to write a `.docx` is a `pkg/skillkit/skills/docx.md` edit, same as
|
||
editing a Claude Code `SKILL.md` — no Go recompile of prose baked into
|
||
a string constant, no risk of a typo in a hand-escaped Go string
|
||
literal.
|
||
- **Generated, not duplicated, tool description.** `read_office_skill`'s
|
||
own description lists every available skill by reading `skillkit.List()`
|
||
at catalog-build time rather than a second hand-maintained list, so
|
||
adding a fifth skill file later (e.g. if PDF support gets built) can't
|
||
leave the tool description stale.
|
||
- **Backward compatible.** `create_spreadsheet` with the original
|
||
`rows`/`sheet_name` fields, and `read_spreadsheet`/`edit_spreadsheet`
|
||
with no `sheet` field, behave exactly as before (first/only sheet) —
|
||
nothing already relying on the single-sheet shape breaks.
|
||
|
||
## 5. RAG over the office skills (search_office_skills)
|
||
|
||
`read_office_skill` (§4) is coarse: it returns a whole skill file, which is
|
||
fine for "give me the full docx conventions before I start" but wasteful
|
||
for one narrow question mid-task ("can a cell hold a formula?"). This adds
|
||
a retrieval-augmented complement: semantic search over the same skill
|
||
content, returning just the matching section(s).
|
||
|
||
**New `pkg/skillkit/rag.go`**, same package as §4's `skillkit.go` (so it
|
||
shares the embedded `registry` directly, no second embed):
|
||
- `chunkSkill` splits one skill's markdown on its own `## ` headings —
|
||
every skill file already follows that convention, so a section *is*
|
||
the right retrieval granularity; no char-budget chunker (like
|
||
`services/knowledge/internal/domain.ChunkText`, built for arbitrary
|
||
source code) is needed.
|
||
- `Index`, built with `NewIndex(embedder)`, embeds every chunk (~20-30
|
||
across the four files) exactly once, lazily, on the first `Search`
|
||
call — not eagerly at startup, and not re-embedded on every call. A
|
||
failed first build isn't cached as permanent failure (deliberately
|
||
not a `sync.Once`): a transient Ollama outage on the first call just
|
||
means the next call retries the build, rather than `search_office_skills`
|
||
staying broken for the rest of the process's life.
|
||
- `Search(ctx, query, topK, skillFilter)` embeds the query and ranks
|
||
every chunk by cosine similarity, in memory — no ANN index, no
|
||
database; brute-force over a few dozen vectors is sub-millisecond and
|
||
outgrowing that would mean the skill corpus itself has gotten too big,
|
||
which is a reason to add a fifth skill file, not a reason to reach for
|
||
pgvector.
|
||
|
||
**Why in-process, not through the existing `services/knowledge`
|
||
Postgres/pgvector pipeline** that already backs `search_codebase`: that
|
||
pipeline is the right tool for what it indexes — a workspace's actual
|
||
source files, which are large, per-user, and change constantly, so they
|
||
need persistence and incremental re-indexing. The office skills are the
|
||
opposite on every axis: a few dozen short, `go:embed`-fixed sections that
|
||
are identical for every workspace and never change between deploys.
|
||
Routing this through the knowledge service would mean a new gRPC
|
||
message, a schema migration to keep skill chunks from polluting a
|
||
workspace's own `search_codebase` results (the `documents` table has no
|
||
scoping column today), and a network round trip — for content that's
|
||
already sitting in the binary. Embedding it once in memory is simpler,
|
||
faster, and has nothing to keep in sync.
|
||
|
||
**New tool `search_office_skills`** (`query`, optional `skill` filter),
|
||
alongside `read_office_skill` in the catalog, dispatch, system prompt,
|
||
and sub-agent whitelist. `toolexec.Executor` gained a `SkillIndex
|
||
*skillkit.Index` field, built in `New` from a new `embedder
|
||
skillkit.Embedder` parameter — `main.go` passes the same `*ollamakit.Client`
|
||
already used for chat (it satisfies the interface as-is; no adapter).
|
||
Nil-embedder is handled the same way `Subagent` already is: the field
|
||
stays nil and the tool reports itself unavailable instead of panicking.
|
||
|
||
New/changed files:
|
||
- `pkg/skillkit/rag.go` (new)
|
||
- `services/agent/internal/infrastructure/toolexec/office_skill.go`
|
||
(`searchOfficeSkills`)
|
||
- `services/agent/internal/infrastructure/toolexec/executor.go`
|
||
(`SkillIndex` field, `New`'s new `embedder` param, dispatch case)
|
||
- `services/agent/internal/infrastructure/toolcatalog/catalog.go`
|
||
(`search_office_skills` tool def; sub-agent whitelist)
|
||
- `services/agent/internal/infrastructure/agentrun/agent.go` (system
|
||
prompt)
|
||
- `services/agent/cmd/server/main.go` (`toolexec.New`'s new argument)
|
||
|
||
### Verification not performed in this environment
|
||
|
||
Unlike §2/§3 above, I could not install a Go toolchain here (no network
|
||
egress in this environment at all — the earlier `apt-get install
|
||
golang-1.23-go` route isn't available this time) and so could not run
|
||
`go build ./...`, `go vet ./...`, or `gofmt -l` against these changes.
|
||
I've kept every edit close to the existing, already-verified code's
|
||
patterns (the same `zipPart`/`readZipPart`/`replaceZipPart` helpers, the
|
||
same `argStr`/`errResult` tool-arg conventions, the same `xml:"id,attr"`
|
||
style already used elsewhere in `officekit` for namespaced OOXML
|
||
attributes) specifically to minimize the risk of a compile error, but
|
||
this should be verified with `go build ./... && go vet ./...` and
|
||
`gofmt -l .` before this is treated as merged, and the three new skill
|
||
files are worth a skim for tone/accuracy even though they're not code.
|
||
|
||
## 6. Fixing real correctness bugs in the office document skills
|
||
|
||
§5 above shipped without a Go toolchain to verify it, and said so. This
|
||
pass went back over all of `pkg/officekit`, `pkg/skillkit`, and the
|
||
office tool wiring (`toolexec/office.go`, `office_skill.go`,
|
||
`toolcatalog/catalog.go`) specifically looking for correctness bugs —
|
||
not style — and fixed four real ones, plus two small, low-risk feature
|
||
additions that Claude Code's own document-editing skills already offer
|
||
and that close gaps these fixes exposed. No Go toolchain or network
|
||
access was available in this environment either (see "Verification
|
||
performed" below for exactly what that means here).
|
||
|
||
### Bug 1 — `edit_document` silently destroyed tables, images, and
|
||
anything else it didn't model, on a real (not self-authored) `.docx`
|
||
|
||
`AppendDocxParagraph` and `ReplaceDocxText` both worked by unmarshaling
|
||
`word/document.xml` into `wDocument` — a struct that only models direct
|
||
`<w:body>` children (paragraphs and the trailing `sectPr`) — mutating
|
||
it, and re-marshaling the whole document back. Re-marshaling a struct
|
||
that doesn't model something *drops that something*: a table, an
|
||
image, a header reference, a second run in a paragraph, tracked
|
||
changes — anything beyond "a flat list of paragraphs" in a real,
|
||
Word-authored or previously-edited document would silently disappear
|
||
the moment `edit_document` touched that file. `read_document` had the
|
||
same blind spot one layer up: `wBody.Paragraphs` only captures `<w:p>`
|
||
elements that are *direct* children of `<w:body>`, so a paragraph
|
||
nested inside a table cell (`<w:tbl>/<w:tr>/<w:tc>/<w:p>`) was never
|
||
unmarshaled into it at all — a table's content wasn't just
|
||
unreconstructed as a grid (the already-documented limitation), it was
|
||
completely invisible to `read_document`, `replace_text`, and anything
|
||
built on top of them.
|
||
|
||
Fixed by moving both operations off the unmarshal/modify/marshal
|
||
pattern entirely:
|
||
|
||
- **`read_document`** now walks `document.xml`'s raw token stream
|
||
(`encoding/xml`'s `Decoder.Token()`, not `Unmarshal`) collecting
|
||
every `<w:p>` wherever it occurs, nested in a table or not, in
|
||
document order — so a table's text is visible (each cell's
|
||
paragraphs as their own lines; the grid shape itself still isn't
|
||
reconstructed, which is now the accurately-scoped limitation).
|
||
- **`edit_document`'s `append_paragraph`** now marshals *only* the one
|
||
new paragraph and splices its bytes directly into the existing
|
||
`document.xml`, immediately before the body's trailing `<w:sectPr>`
|
||
(OOXML requires section properties to stay the body's last child) or
|
||
before `</w:body>` if there's none — so every byte of the original
|
||
document this package doesn't model survives untouched. The spliced
|
||
fragment carries its own explicit WordprocessingML namespace
|
||
declaration (`wParagraphFragment`, distinct from the `wParagraph`
|
||
type `BuildDocx` still uses internally) so it resolves correctly
|
||
whether the target document uses Word's own `w:`-prefixed convention
|
||
or this package's unprefixed default-namespace one — without paying
|
||
for a redundant `xmlns` on every paragraph of a freshly-built
|
||
document, which sharing one type between both call sites would have
|
||
meant.
|
||
- **`edit_document`'s `replace_text`** now matches and rewrites text
|
||
directly inside `<w:t>...</w:t>` elements in the raw XML (a small,
|
||
locally-scoped regex selecting the exact local element name `t`,
|
||
with or without a namespace prefix — not `tab`/`tbl`/`tc`, which
|
||
start with the same letter), decoding/re-encoding only the matched
|
||
inner text through the same entity (un)escaping `xml.Marshal`/
|
||
`EscapeText` already use. Everything else in the document — tags,
|
||
attributes, tables, images — passes through byte-for-byte unchanged.
|
||
One real, newly-documented trade-off: a phrase Word has split across
|
||
more than one run (common after spell-check or revision tracking)
|
||
may not be found as a single match, since each `<w:t>` is matched
|
||
independently — the docx skill now says so.
|
||
|
||
### Bug 2 — `edit_spreadsheet` could write cells in an order Excel
|
||
flags as needing "repair"
|
||
|
||
`SetXlsxCells` appended a newly-written row/cell to the end of the
|
||
in-memory slice regardless of its row/column number, so a sparse,
|
||
arbitrarily-ordered set of writes (e.g. writing `"A1"` in the same call
|
||
as an already-present `"A5"`) could marshal `xl/worksheets/sheetN.xml`
|
||
with rows or cells out of ascending order — which Excel's own parser
|
||
(unlike most lenient readers) can flag as needing repair on open.
|
||
Fixed with `sortSheetData`, called right before marshaling in
|
||
`SetXlsxCells`: rows sorted ascending by row number, and cells within
|
||
each row sorted ascending by column, so a schema-valid order comes out
|
||
regardless of what order the (inherently unordered, since it's a Go
|
||
map) set of writes was applied in. Covered by
|
||
`TestSetXlsxCellsFormulaAndOrdering`, which asserts on the raw XML
|
||
byte offsets, not just this package's own reader, since this bug would
|
||
never have shown up through this package's own (order-independent)
|
||
`ReadXlsxCells`.
|
||
|
||
### Bug 3 — a latent schema-order bug in `wRPr`
|
||
|
||
While adding italic/underline support (feature addition below), found
|
||
that `wRPr`'s one existing formatting field (`Bold`) was never ordered
|
||
against anything else, so there was nothing to get wrong yet — but the
|
||
straightforward way to add `Italic`/`Underline`/keep `Sz` would have
|
||
produced a `<w:rPr>` child order (`b`, `u`, `sz`, `i`, declaration
|
||
order) that doesn't match OOXML's actual `CT_RPr` sequence (`b`, `i`,
|
||
`sz`, `u`, ...). Real-world readers are typically lenient about this,
|
||
but there's no reason to ship something schema-invalid when getting
|
||
the field order right costs nothing; `wRPr`'s fields are now declared
|
||
in actual schema sequence.
|
||
|
||
### Feature addition — italic/underline paragraph formatting (docx)
|
||
|
||
`Paragraph` gained `Italic`/`Underline` bool fields alongside the
|
||
existing `Bold`, wired through `create_document`'s paragraph schema and
|
||
`edit_document`'s `append_paragraph` operation (which — separately —
|
||
was also missing `bold` from its own JSON schema even though the
|
||
executor already read it off `call.Arguments`; a real, if minor, gap
|
||
of its own, now fixed by adding `bold` there too). `pkg/skillkit/skills/docx.md`
|
||
documents both fields.
|
||
|
||
### Feature addition — live formulas (xlsx)
|
||
|
||
A cell value starting with `"="` (e.g. `"=SUM(A1:A4)"`) is now stored
|
||
as a real Excel formula (`<f>`) rather than literal text — previously
|
||
`isNumeric` rejected it and it silently became an inline-string cell
|
||
showing the formula text, never actually computing anything in Excel.
|
||
`create_spreadsheet` and `edit_spreadsheet` classify values through one
|
||
shared `setCellValue` now (previously `BuildXlsx`'s create path and
|
||
`SetXlsxCells`'s edit path each had their own, slightly different,
|
||
hand-rolled numeric/text branch — now unified, so both paths behave
|
||
identically). `read_spreadsheet` shows a formula cell's cached computed
|
||
value when one exists (always true for a real Excel-authored or
|
||
user-uploaded file) or the formula text itself, prefixed `=`, when
|
||
nothing has evaluated it yet (always true immediately after this
|
||
package writes one, since nothing here evaluates formulas).
|
||
`pkg/skillkit/skills/xlsx.md`'s former "No formulas" limitation is
|
||
replaced with an accurate description of this behavior, including the
|
||
no-computed-value-yet caveat.
|
||
|
||
### Also
|
||
|
||
- `toolcatalog.subagentToolNames`'s map literal had drifted out of
|
||
`gofmt` alignment (two entries added by §4/§5 weren't gofmt-aligned
|
||
with the rest) — recomputed and realigned; this is purely
|
||
cosmetic but `gofmt -l` is supposed to be clean per §2's own
|
||
verification bar.
|
||
- Added `pkg/officekit/officekit_test.go` — there were no tests for
|
||
this package at all before this pass. It covers the two
|
||
content-preservation fixes directly (append/replace against a
|
||
hand-built table-containing document, asserting both on
|
||
`read_document`'s output and on raw-XML byte offsets), the
|
||
xlsx ordering fix (same: both the package's own reader and raw XML
|
||
offsets), and round-trips for all three formats including
|
||
formatting flags and an `&`-containing bullet (exercises
|
||
escaping both directions through the pptx hand-written-template
|
||
path).
|
||
|
||
### Verification performed
|
||
|
||
Static only — no Go toolchain and no network access in this
|
||
environment (same constraint §5 ran into). What that means concretely:
|
||
every edit above was re-read multiple times against Go's actual
|
||
`encoding/xml`/`regexp`/`archive/zip` semantics rather than assumed;
|
||
every touched file was run through a hand-rolled brace/paren/bracket
|
||
balance checker (tracking Go string/rune/raw-string/comment lexing) to
|
||
catch the class of error a missing `go vet` would otherwise have
|
||
caught instantly; the new regex (`wTextElementRe`) and the new
|
||
`lastTagIndex`/`xmlUnescapeText` helpers were traced by hand against
|
||
concrete example inputs (worked through in detail, not just eyeballed)
|
||
before being written. None of this is a substitute for actually
|
||
running `go build ./... && go vet ./... && go test ./... && gofmt -l .`
|
||
— that should happen before this is treated as merged, same as every
|
||
prior section's caveat.
|
||
|
||
## Multilingual support (Claude Code-style)
|
||
|
||
enkly's agent already detected and mirrored whatever language a user typed in
|
||
conversation (`agentrun.SystemPrompt`'s "## Language" section). What was
|
||
missing was everything around that: a localized UI chrome, an explicit way
|
||
for a visitor to set a language, and a way for that choice to reach the
|
||
model as a standing preference rather than relying purely on per-message
|
||
detection — the same two-layer design Claude Code itself uses (automatic
|
||
detection, plus an explicit `/language`-style override).
|
||
|
||
**New shared package `pkg/i18n`** — the single source of truth, for every
|
||
service, of which locales exist and what their strings are: a `Locale`
|
||
type, `Supported` (en, fr, es, de, pt — en/fr ship complete, the rest cover
|
||
nav/auth/composer and fall back to English for the rest), JSON catalogs
|
||
loaded via `go:embed`, `T()` with fallback-to-English-then-to-key, and
|
||
`NegotiateAcceptLanguage` for parsing a browser's `Accept-Language` header.
|
||
Dependency-free (stdlib only) so both `gateway` and `agent` can import it
|
||
without depending on each other.
|
||
|
||
**Gateway (`services/gateway/internal/handlers/i18n.go`)** — `localeFrom`
|
||
resolves a request's locale (`?lang=` query → `enkly_lang` cookie →
|
||
Accept-Language → `i18n.Default`); `SetLanguage` (`POST /language`) persists
|
||
an explicit choice in a year-long cookie, mirroring `SetTheme`'s existing
|
||
pattern byte-for-byte on purpose. A new `lang_switcher` partial
|
||
(`web/templates/partials.html`) — a plain `<select>` + submit button, no
|
||
`onchange="…"` auto-submit, since the page's CSP has no `unsafe-inline` in
|
||
`script-src` and that would've silently done nothing — is wired into the
|
||
sidebar footer (`index.html`) and every `auth.html` page. `auth.go` was
|
||
rewritten to localize every user-facing string in the login/signup/verify/
|
||
reset flows, which also fixed a latent fragility: `validatePassword`'s
|
||
errors are now sentinel values (`errPasswordTooShort` etc.) matched with
|
||
`errors.Is`, instead of the translator having to match against the error's
|
||
English wording.
|
||
|
||
**Reaching the agent (no proto changes — see ARCHITECTURE.md §7's
|
||
"Language preference storage" note for why):** `chat.go`'s `withLangMetadata`
|
||
attaches the resolved locale as gRPC metadata (`x-enkly-lang`) on the
|
||
`StartTurn` call; `services/agent/internal/interfaces/grpc/server.go`'s
|
||
`langFromIncomingContext` reads it back and passes it into
|
||
`application.Service.StartTurn` (now `lang i18n.Locale`-aware — every
|
||
message it returns, quota-exceeded included, is looked up in that locale's
|
||
catalog). `StartTurn` threads it onto the detached run context via the new
|
||
`agentrun.WithPreferredLanguage`, and `Agent.buildSystemPrompt` appends an
|
||
"## Interface language" directive naming it — deliberately worded "is
|
||
using", not "has set", since the locale reaching this point is just as
|
||
often a browser-negotiated guess as an explicit switcher choice (see the
|
||
doc comments on `WithPreferredLanguage` and `languageDirective` for the
|
||
full reasoning). The existing "## Language" detection rules still take
|
||
priority over this directive if a visitor is actually typing in a different
|
||
language than their UI setting. `dispatch_subagent` needed no extra
|
||
plumbing — it passes its own derived context straight through, so a
|
||
sub-agent's report inherits the same preference automatically. New tests:
|
||
`pkg/i18n/*_test.go` (catalog loading, fallback, Accept-Language
|
||
negotiation) and two in `agent_test.go` confirming the directive is (and
|
||
isn't) appended correctly.
|
||
|
||
**Scope not covered:** `subscription.html`/`files.html` aren't wired to the
|
||
switcher; a `preferred_language` field on the IAM `User` row (so a locale
|
||
choice would follow a visitor to a second device, instead of only living in
|
||
a cookie) is a natural follow-up once `gen/`'s generated code can be safely
|
||
regenerated with `protoc` — see ARCHITECTURE.md §7.
|
||
|
||
Static review only — no Go toolchain and no network access in this
|
||
environment (same constraint every prior section in this file ran into).
|
||
JSON catalogs were validated with `python3 -m json.tool`; every edited `.go`
|
||
and `.html` file was re-read in full against Go's `html/template`, context,
|
||
and gRPC metadata semantics rather than assumed. This is not a substitute
|
||
for `go build ./... && go vet ./... && go test ./... && gofmt -l .`, which
|
||
should run before this is treated as merged.
|
||
|
||
## Office document skills: redlining, PDF extraction, and Markdown conversion
|
||
|
||
Rounded out `pkg/officekit`'s Word/Excel/PowerPoint tool family with the
|
||
three capabilities it was still missing relative to a full office-document
|
||
assistant: tracked-changes editing, reading a PDF's text, and converting an
|
||
existing Markdown document straight into a `.docx`. All three follow the
|
||
existing package's house rules — pure stdlib plus the already-vendored
|
||
`gomarkdown` dependency (no new `go.mod` entries), structured-fields-only
|
||
across the tool-call boundary (nothing the model passes or receives is raw
|
||
XML/PDF syntax), and everything that can't be done honestly says so instead
|
||
of approximating it.
|
||
|
||
**Redlining (`pkg/officekit/redline.go`)** — two new `edit_document`
|
||
operations, `append_paragraph_tracked` and `replace_text_tracked`, record an
|
||
edit as a `<w:ins>`/`<w:del>` pair (author + ISO-8601 date) instead of
|
||
rewriting text in place, the same thing Word's own Track Changes produces;
|
||
`accept_all_changes`/`reject_all_changes` resolve every pending tracked
|
||
change in the document at once. `read_document` now also reports any
|
||
pending tracked changes after the plain text (`ReadDocxRevisions`), so a
|
||
caller doesn't need a second call to discover a document already has open
|
||
redlines. Scope: a tracked replace marks the *whole* text run that matched
|
||
as changed, not a minimal character-level diff within it — documented in
|
||
both the code and the `docx` skill file's new "Redlining" section.
|
||
|
||
**PDF extraction (`pkg/officekit/pdf.go`)** — new read-only `read_pdf` tool.
|
||
Finds `stream...endstream` blocks tagged `/Filter /FlateDecode`, inflates
|
||
them with `compress/zlib`, and scans the decompressed content for the
|
||
text-showing operators (`Tj`, `TJ`, `'`, `"`) that place characters on a
|
||
page. Deliberately not a PDF library or a layout engine: no OCR (a
|
||
scanned/image-only PDF returns a clear error, not fabricated text), no
|
||
table/column structure, Latin-ish encodings only. New `pdf` skill file
|
||
documents exactly what to expect.
|
||
|
||
**Markdown → docx (`pkg/officekit/markdown.go`)** — `create_document` now
|
||
accepts `markdown` as an alternative to `paragraphs`; `MarkdownToParagraphs`
|
||
parses it with the same `gomarkdown` parser already used for chat-message
|
||
rendering (`pkg/renderkit`) and maps headings/paragraphs/lists onto the
|
||
existing `Paragraph` vocabulary, so `BuildDocx` needs no changes at all.
|
||
Inline formatting, tables, and raw HTML blocks are flattened/dropped the
|
||
same way `create_document`'s existing whole-paragraph-only formatting limit
|
||
already works.
|
||
|
||
New/changed files:
|
||
- `pkg/officekit/redline.go`, `pdf.go`, `markdown.go` (new), plus
|
||
`redline_test.go`, `pdf_test.go`, `markdown_test.go` (new)
|
||
- `services/agent/internal/infrastructure/toolexec/office.go` (tracked-change
|
||
cases in `editDocument`, revisions in `readDocument`, new `readPdf`,
|
||
`markdown` handling in `createDocument`)
|
||
- `services/agent/internal/infrastructure/toolexec/executor.go`
|
||
(`read_pdf` dispatch case)
|
||
- `services/agent/internal/infrastructure/toolcatalog/catalog.go`
|
||
(`read_pdf` tool def, extended `create_document`/`edit_document` schemas
|
||
and descriptions, `read_pdf` added to the read-only sub-agent whitelist)
|
||
- `pkg/skillkit/skills/pdf.md` (new), `docx.md` and `overview.md` (updated)
|
||
|
||
**Not done, on purpose, within this pass:** table and image support in
|
||
`create_document`/`edit_document` (still an honestly-documented limitation,
|
||
not papered over); per-change (rather than accept/reject-all) tracked-change
|
||
resolution; shelling out to Pandoc/LibreOffice for format conversion or
|
||
`.docx`→PDF export — `MarkdownToParagraphs` covers the common "turn
|
||
Markdown into a real document" case without a runtime dependency on an
|
||
external binary that may not be installed in every deployment; a thin
|
||
wrapper around `pandoc`/`soffice --headless` (guarded, with a clear error
|
||
when the binary isn't on `PATH`) is a reasonable follow-up if broader format
|
||
coverage (ODT, RTF, `.docx`→PDF) turns out to be needed.
|
||
|
||
**Verification performed:** no Go toolchain or network access in this
|
||
delivery environment (same constraint as every prior section in this file).
|
||
New code was written to match this package's existing, already-verified
|
||
patterns exactly (the `wTextElementRe`/`lastTagIndex`/`xmlUnescapeText`
|
||
splice-not-reparse style from `docx.go`, the same zip-part helpers from
|
||
`zipkit.go`); the PDF extraction regexes and the TJ-array kerning-vs-space
|
||
distinction were traced by hand against concrete example content streams
|
||
(worked through byte-by-byte, not just eyeballed) before being written, and
|
||
tests exercise that exact behavior (`TestReadPdfTextTJArrayKerning`) rather
|
||
than only the happy path. This is not a substitute for actually running
|
||
`go build ./... && go vet ./... && go test ./... && gofmt -l .` — that
|
||
should happen before this is treated as merged, same as every prior
|
||
section's caveat.
|
||
|
||
## 7. Dynamic workflows, effort control, adaptive thinking, long-context recovery, live system entries, fast mode
|
||
|
||
A Claude-parity upgrade to the agent service's run loop and tool catalog, scoped
|
||
to what's honestly deliverable against an Ollama/Qwen backend on a single-host
|
||
deployment (no protoc/go toolchain in this delivery environment either — same
|
||
constraint as every prior section; see "Verification performed" below).
|
||
|
||
**Effort control** (new `pkg/effort`): five levels — Low, Medium, High, Extra,
|
||
Max — each resolving to a `Profile` (think-effort floor, max rounds, context
|
||
budget, max parallel sub-agents, adaptive-thinking toggle). `Medium` reproduces
|
||
the agent's exact pre-existing behavior (same `MaxRounds: 12`,
|
||
`DefaultThinkPolicy` shape), so this is additive, not a behavior change, for
|
||
every caller that doesn't opt in. Selected per turn via a leading `/effort low
|
||
|medium|high|extra|max` directive line (see "Why directives, not new proto
|
||
fields" below) or `ENKLY_DEFAULT_EFFORT` for the deployment default.
|
||
|
||
**Adaptive thinking** (`agentrun.AdaptiveThinkPolicy`): layered on top of a
|
||
level's own think-effort floor, escalates one round to `"high"` when a concrete
|
||
difficulty signal is present — the previous round's tool call errored, produced
|
||
an unusually large result, or the turn has run long (`agentrun.RoundSignal`) —
|
||
rather than holding every round at a fixed effort regardless of how routine it
|
||
is. On by default (`ENKLY_ADAPTIVE_THINKING=true`); never spends *less* than
|
||
the level's floor, only sometimes more.
|
||
|
||
**Dynamic workflows** (new `dispatch_workflow` tool, `toolexec/workflow.go`):
|
||
fans a batch of steps out to parallel, read-only sub-agents (reusing the exact
|
||
same nested `agentrun.Agent` `dispatch_subagent` already uses — same
|
||
restricted-tool boundary, same inability to write or run anything), honoring
|
||
`depends_on` ordering via a bounded-concurrency topological scheduler, checking
|
||
every report against its own task with a deterministic verifier before showing
|
||
results back to the dispatching model, and retrying once on a failed check —
|
||
the "automatic verification... and real-time plan adjustment" from the brief,
|
||
done without a second LLM call per step. Gated by plan (new `pkg/entitlements`,
|
||
keyed off billing's existing `plan_name` string — see that package's doc
|
||
comment for why "premium", today's top tier, stands in for Enterprise/Team/Max
|
||
until billing's own catalog is extended) and bounded by
|
||
`ENKLY_WORKFLOW_MAX_PARALLEL` (default 64 — deliberately modest for a
|
||
single-instance local Ollama backend, which serves one inference request at a
|
||
time regardless of how many arrive concurrently; a multi-replica/multi-GPU
|
||
deployment raises this toward the "hundreds" a cluster can actually use).
|
||
|
||
**Long-context management**: `Agent.ContextBudgetChars` is now overridable (via
|
||
the resolved effort level, or directly) instead of a hardcoded constant, so a
|
||
Max-effort turn keeps more tool-result history verbatim before compacting.
|
||
More importantly, `compactToolHistory` now returns what it trimmed, and
|
||
`upsertCompactionLedger` folds that into a single, stable, auto-maintained
|
||
system message — "here's what ran, in order, even though its full output is
|
||
gone" — instead of silently shrinking old tool results with no trace they
|
||
existed. This is the "better recovery after compaction and fewer drifts"
|
||
item: a long agentic trace that's compacted its 30th tool result still has a
|
||
legible map of every step that came before it.
|
||
|
||
**Live system entries** (`agentrun.WithSystemEntries`, `SystemEntry`): a
|
||
caller can fold additional instruction/permission/budget text into the system
|
||
prompt mid-conversation, always appended after the fixed base prompt + memory
|
||
+ language block, never inserted or edited in place — see that function's doc
|
||
comment for an honest accounting of what "without breaking cache" can mean
|
||
against Ollama specifically (no prompt-cache breakpoints like the Anthropic
|
||
API; what's actually guaranteed is that the prefix up to the live-updates
|
||
section stays byte-identical turn over turn, which is the one lever actually
|
||
available to a KV-cache-reusing backend).
|
||
|
||
**Fast mode**: `Service.FastAgent`, an optional second `*agentrun.Agent`
|
||
sharing the same `Executor` (and therefore the same `dispatch_subagent`/
|
||
`dispatch_workflow` capability) but built around a distinct, smaller/quantized
|
||
`*ollamakit.Client` (`ENKLY_FAST_CHAT_MODEL`). Requesting `/fast` when no fast
|
||
model is configured just runs the turn on the normal agent — degrading to "no
|
||
change" rather than erroring, since fast mode is meant to work with zero
|
||
configuration on every plan.
|
||
|
||
**Honesty and tool-triggering**: `agentrun.SystemPrompt` gained an explicit
|
||
"say what you actually found, including the parts that don't look good"
|
||
clause (report known bugs/limitations/failing tests plainly rather than a
|
||
clean-sounding summary that omits them) and a "decide with a tool call, don't
|
||
describe one" clause (call the matching tool instead of narrating an intended
|
||
action as prose, and conversely don't re-call a tool out of habit when the
|
||
turn already has what it needs).
|
||
|
||
**Why directives, not new proto fields, for effort/fast/system-entries (new
|
||
`pkg/turnopts`):** `api/proto/agent/v1/agent.proto`'s `StartTurnRequest` now
|
||
documents the target fields (`effort`, `fast_mode`, `system_entries`) for a
|
||
follow-up that runs `make proto` and wires them directly — proto3 fields are
|
||
purely additive, so declaring them now is free and backward-compatible. But
|
||
regenerating `gen/agent/v1` needs `protoc` + the Go plugins, which this
|
||
delivery environment has no network access to install (same constraint noted
|
||
throughout this file), and hand-editing protobuf-go's generated code without
|
||
being able to compile or run it is a correctness risk this change declines to
|
||
take — a struct field without a matching raw descriptor can panic at runtime
|
||
in ways a type-checker won't catch. `pkg/turnopts.Parse` reads the exact same
|
||
three controls off leading `/effort`, `/fast`, `/system` lines in the existing
|
||
plain-string `prompt` field instead, which needs no proto change at all and
|
||
ships working today; `application.Service.StartTurn` calls it first thing.
|
||
Once the proto is regenerated, a thin follow-up reads the structured fields
|
||
directly and `turnopts` becomes the fallback path for a plain-text client
|
||
instead of the only path — nothing about today's wiring needs to be thrown
|
||
away for that to happen.
|
||
|
||
**What this doesn't claim**: there is no literal 1,000,000-token context
|
||
window here — Qwen/Ollama's actual usable context is whatever the pulled
|
||
model and `num_ctx` support, and `ContextBudgetChars`'s levels are this
|
||
service's own calibration for a local deployment, not a guarantee about the
|
||
underlying model's real capacity (see `pkg/effort`'s package doc comment).
|
||
"Hundreds of sub-agents in parallel" is bounded by configured concurrency and
|
||
real hardware, same caveat. Entitlement gating today keys off billing's
|
||
existing "premium" plan name, not literal "Enterprise"/"Team"/"Max" tiers,
|
||
which billing's catalog (`services/billing/internal/domain/subscription.go`)
|
||
doesn't define yet — extending it is a `Catalog` append plus a migration
|
||
touching the `plan` CHECK constraint, not a change `pkg/entitlements` itself
|
||
needs once it happens (just update `ENKLY_DYNAMIC_WORKFLOW_PLANS`).
|
||
|
||
**Verification performed**: no Go toolchain or network access in this
|
||
delivery environment, same constraint as every prior section. New code
|
||
follows this codebase's existing patterns closely (context-key-based
|
||
per-turn options exactly like `WithPreferredLanguage`; the compaction ledger
|
||
reuses `compactToolHistory`'s own trim loop rather than a parallel
|
||
implementation; the workflow scheduler reuses `dispatch_subagent`'s exact
|
||
nested-`Agent` mechanism rather than inventing a second execution path).
|
||
Existing tests in `agentrun/agent_test.go` were re-traced by hand against the
|
||
new `effectiveLimits` precedence rules (an `Agent` with neither `ThinkPolicy`
|
||
nor `Effort` ever set — i.e. not built via `New`/`NewSubagent` — keeps the
|
||
exact legacy no-override behavior those tests assert) to avoid a silent
|
||
regression; new pure-function logic (`pkg/effort`, `pkg/turnopts`,
|
||
`dispatch_workflow`'s step parsing and DAG validation) has its own unit tests
|
||
covering the error paths (an unparseable level, a cyclic or self-referential
|
||
dependency graph, a directive-lookalike like `/effortless` that must NOT be
|
||
parsed as a directive) rather than only the happy path. This is not a
|
||
substitute for actually running
|
||
`go build ./... && go vet ./... && go test ./... && gofmt -l .` — that should
|
||
happen before this is treated as merged, same as every prior section's
|
||
caveat.
|
||
|
||
## 8. Ollama → vLLM production migration
|
||
|
||
The chat and embedding backend moved from a single local Ollama daemon
|
||
(`/api/chat`, `/api/embed`) to two independently deployed, GPU-backed
|
||
**vLLM** OpenAI-compatible servers — one serving the chat model (Qwen,
|
||
tool calling + extended thinking), one serving the embedding model.
|
||
`pkg/ollamakit` is now `pkg/vllmkit`; every call site across the agent
|
||
and knowledge services was repointed at it (13 files, a mechanical
|
||
rename plus the client rewrite below). This section is written as of
|
||
the migration, read §§1–7 above for the Ollama-era history they
|
||
document — they're left as-is rather than rewritten, since they're an
|
||
honest record of decisions made against the backend that existed at
|
||
the time.
|
||
|
||
**Why two servers, not one.** A vLLM engine process serves exactly one
|
||
model — there's no equivalent of Ollama's single daemon juggling
|
||
multiple pulled tags on request. Chat and embeddings are also
|
||
different scaling problems in practice (one latency-sensitive 32B
|
||
model on the critical path of every turn; one small, high-throughput
|
||
embedding model), so splitting them into independent Deployments/
|
||
Services (`vllm-chat`, `vllm-embed` in `docker-compose.yml` and
|
||
`k8s/11-vllm.yaml`) was the natural fit rather than a compromise.
|
||
|
||
**Wire protocol.** `pkg/vllmkit/client.go` was rewritten end to end
|
||
against vLLM's `/v1/chat/completions` (SSE streaming) and
|
||
`/v1/embeddings`, replacing Ollama's native API entirely:
|
||
|
||
- **Streaming tool calls.** OpenAI's wire format sends a tool call's
|
||
id/name once and its JSON-encoded `arguments` string in
|
||
arbitrary-length fragments across however many chunks it takes,
|
||
keyed by an index rather than carried whole in one message the way
|
||
Ollama sent it. `toolCallAccumulator` reassembles these by index and
|
||
parses the finished JSON string once streaming for that call
|
||
completes — this is genuinely the trickiest part of the whole
|
||
migration and has its own unit test
|
||
(`TestToolCallAccumulator_Streamed`) covering a call whose arguments
|
||
arrive split mid-JSON-token.
|
||
- **`tool_call_id` plumbing.** OpenAI's tool-calling contract requires
|
||
every tool-role reply message to carry the id of the call it answers
|
||
— Ollama's native API never needed this (replies were correlated
|
||
positionally). Added `Message.ToolCallID` and wired it through
|
||
`agentrun.Agent.Run`'s round loop (`roundToolCalls[i].ID` →
|
||
`ToolResult` → the tool reply message), and `ToolCallRequest.ID` is
|
||
now actually populated (it existed as a field before but Ollama's
|
||
wire format never set it).
|
||
- **Parse-failure detection renamed, not removed.**
|
||
`ollamakit.IsTemplateParseError` (§1 above) detected Ollama's
|
||
specific XML-template parse failure. That literal failure mode is
|
||
gone — tool content now travels as a JSON string field, never
|
||
XML-wrapped — but vLLM's own tool-call parser (hermes/llama3-json/
|
||
qwen3/...) can still fail to extract a clean call from a model's raw
|
||
output, and this client can still fail to decode a malformed
|
||
`arguments` string. `vllmkit.IsToolCallParseError` covers both,
|
||
replaces the old function everywhere it was called
|
||
(`agentrun/agent.go`'s retry loop, `degradeRecentToolMessages`), and
|
||
plays the identical role in that retry loop. `safeHeadCut`/
|
||
`safeTailCut`'s angle-bracket-snapping (§1) is kept as cheap content
|
||
hygiene even though it's no longer strictly required for parsing
|
||
safety under the new protocol — see its updated doc comment for why.
|
||
- **Options reshaped, not just renamed.** Ollama's per-request
|
||
`num_ctx`/`num_gpu`/`num_thread`/`keep_alive` have no per-request
|
||
equivalent under vLLM — context length and GPU/tensor-parallel
|
||
layout are fixed at server start (`--max-model-len`,
|
||
`--tensor-parallel-size`, `--gpu-memory-utilization`; see
|
||
`k8s/11-vllm.yaml`), and a vLLM server has no concept of unloading a
|
||
resident model between requests. `vllmkit.Options` now only carries
|
||
what's actually meaningful over HTTP: `Temperature`, `TopP`,
|
||
`MaxTokens`.
|
||
- **Thinking.** `Client.Think` keeps the same `""`/`"true"`/`"false"`/
|
||
`"low"`/`"medium"`/`"high"` vocabulary `pkg/effort` already used, but
|
||
the wire mapping changed: it now sets
|
||
`chat_template_kwargs.enable_thinking` (vLLM's extra-body extension
|
||
for Qwen3's chat template), and every non-empty, non-"false" level
|
||
collapses to the same `true` — vLLM/Qwen3's reasoning toggle is a
|
||
single on/off switch server-side with no graded-effort equivalent to
|
||
Ollama's (which didn't have one either, but this is now explicit in
|
||
the doc comments rather than implied). Reasoning streams back as
|
||
OpenAI's `reasoning_content` delta field, requiring the chat
|
||
deployment to run with `--reasoning-parser qwen3`.
|
||
- **Auth.** Added `Client.APIKey` (bearer token, vLLM's `--api-key`) —
|
||
`ENKLY_VLLM_API_KEY` in both services' `main.go`, since a
|
||
cluster-external vLLM endpoint (unlike a `localhost`-only Ollama
|
||
daemon) is a real exposure surface to think about.
|
||
|
||
**Env vars.** `ENKLY_OLLAMA_URL` → `ENKLY_VLLM_CHAT_URL` +
|
||
`ENKLY_VLLM_EMBED_URL` (agent service wires both; knowledge service
|
||
only the embed one, since it never calls `ChatStream`).
|
||
`ENKLY_OLLAMA_KEEP_ALIVE` dropped (no vLLM equivalent — see above).
|
||
`ENKLY_CHAT_NUM_CTX`/`NUM_GPU`/`NUM_THREAD` and
|
||
`ENKLY_EMBED_NUM_CTX`/`NUM_GPU`/`NUM_THREAD` dropped (moved to vLLM
|
||
server startup flags in the deployment manifests, not client config).
|
||
`ENKLY_CHAT_NUM_PREDICT` → `ENKLY_CHAT_MAX_TOKENS`; added
|
||
`ENKLY_CHAT_TOP_P`, `ENKLY_VLLM_API_KEY`,
|
||
`ENKLY_FAST_VLLM_CHAT_URL` (fast mode's smaller model can now live on
|
||
an entirely separate vLLM deployment, not just a second tag on the
|
||
same daemon).
|
||
|
||
**Deployment.** `docker-compose.yml`'s `ollama` service replaced with
|
||
`vllm-chat`/`vllm-embed` (pinned `vllm/vllm-openai:v0.6.3` image, NVIDIA
|
||
GPU device reservations, `/health`-based healthchecks gating
|
||
`agent`/`knowledge`'s startup via `depends_on`, a shared
|
||
`hf-cache` volume so a restart doesn't re-download weights).
|
||
`k8s/11-ollama.yaml` replaced with `k8s/11-vllm.yaml`: real
|
||
`nvidia.com/gpu` resource requests (no more "commented out, no GPU in
|
||
this cluster" — a GPU node pool is now a hard prerequisite, called out
|
||
explicitly in `k8s/README.md`'s updated issue #5), a `startupProbe`
|
||
sized for a cold multi-minute model load distinct from the steady-state
|
||
`readinessProbe`/`livenessProbe`, and `/dev/shm` sized via an
|
||
`emptyDir{medium: Memory}` volume — vLLM needs real shared memory for
|
||
NCCL/CUDA-graph scratch space, and the container runtime's 64MB default
|
||
is a well-known silent-failure trap for multi-GPU vLLM deployments in
|
||
particular. The Hugging Face cache PVC moved from `ReadWriteOnce` to
|
||
`ReadWriteMany` since it's now mounted by two Deployments that can land
|
||
on different nodes — an easy miss that would have made the two
|
||
Deployments fight over node-local exclusivity instead of sharing a
|
||
cache.
|
||
|
||
**What stayed exactly the same.** The agent's tool-execution model
|
||
(`toolexec.Executor`), the agentic round-trip loop
|
||
(`agentrun.Agent.Run`), effort control (`pkg/effort`), dynamic
|
||
workflows, the compaction ledger, and every product-facing feature in
|
||
§§1–7 above are unaffected — this migration is scoped to the transport
|
||
layer between the agent/knowledge services and the model-serving
|
||
backend, not to any of the logic built on top of it. `skillkit.Embedder`
|
||
and `agentrun.ChatStreamer`'s interfaces didn't change shape, so
|
||
nothing above `pkg/vllmkit` needed to know the backend changed at all,
|
||
beyond the env-var/client-construction wiring in each service's
|
||
`main.go`.
|
||
|
||
**Verification performed.** `pkg/vllmkit` has no dependency beyond the
|
||
Go standard library, so it was verified for real: `go build ./...` and
|
||
`go test ./...` both pass against it in an isolated module (new tests:
|
||
`TestThinkWire`, `TestChatStream_ThinkOverride`,
|
||
`TestChatStream_StreamsTokens`, `TestToolCallAccumulator_Streamed`,
|
||
`TestIsToolCallParseError`). Every other touched `.go` file (13 call
|
||
sites plus `main.go` for both services) passes `gofmt -l` — a full
|
||
syntax parse — but, same constraint as every prior section in this
|
||
document, this delivery environment has no network access to
|
||
`proxy.golang.org`/`google.golang.org` to run a real `go build ./...`
|
||
across the full dependency graph (`google.golang.org/grpc` and
|
||
friends). `docker-compose.yml` and every file under `k8s/` were
|
||
validated with `yaml.safe_load` (parses, not a Kubernetes API-server
|
||
dry-run). Treat `go build ./... && go vet ./... && go test ./... &&
|
||
gofmt -l .` as the first gate before this is merged, same as every
|
||
prior section's caveat — and, specific to this change, a real
|
||
`kubectl apply --dry-run=server -k k8s/` (or `kubectl kustomize k8s/ |
|
||
kubeconform`) against a cluster that actually has the `enkly.io/gpu-pool:
|
||
inference` node label this manifest set assumes.
|
||
|
||
## 4. Follow-up: vllm-chat / vllm-embed stuck `Pending`
|
||
|
||
Root cause: `nvidia.com/gpu: "1"` resource requests in `11-vllm.yaml` only
|
||
schedule once some node *advertises* that resource to the scheduler — a real
|
||
GPU and driver on the box isn't sufficient by itself, and nothing in the
|
||
prior manifest set ran the NVIDIA device plugin to do that advertising.
|
||
|
||
Fixed by `k8s/11a-nvidia-device-plugin.yaml`: a device-plugin DaemonSet
|
||
scoped to the same `enkly.io/gpu-pool: inference` label/taint pair as
|
||
`11-vllm.yaml`, now wired into `k8s/kustomization.yaml`. Also added
|
||
`k8s/scripts/gpu-preflight.sh` (`make gpu-preflight`) — a read-only check
|
||
that walks every precondition these two Deployments need (GPU allocatable
|
||
capacity, node label, node taint, `vllm-hf-cache` PVC bound, GPU count vs.
|
||
GPU-requesting pods) and prints the real scheduler events, so a
|
||
misconfiguration surfaces in seconds instead of after a 10-minute
|
||
`startupProbe` timeout.
|
||
|
||
What this does NOT fix, because no manifest can: labeling/tainting the
|
||
actual physical GPU node, and installing the NVIDIA driver + container
|
||
toolkit on that node if they aren't already present (use the NVIDIA GPU
|
||
Operator for that — see the new file's header comment). `k8s/README.md`'s
|
||
deploy steps now call out the label/taint commands explicitly as step 4,
|
||
before `kubectl apply -k .`.
|
||
|
||
## Verification performed
|
||
|
||
- `kustomize build k8s/` succeeds cleanly against the full manifest set
|
||
(43 resources) with the new DaemonSet included.
|
||
- `kubeconform -strict` against the rendered output: 43/43 resources valid
|
||
against the Kubernetes 1.29 API schema, 0 errors.
|
||
- `bash -n k8s/scripts/gpu-preflight.sh` — no syntax errors.
|
||
|
||
## 5. Full `k8s/` review (beyond the Pending symptom)
|
||
|
||
Went through every manifest, not just 11-vllm.yaml/11a-nvidia-device-plugin.yaml,
|
||
looking for issues beyond the reported symptom. Found three, two of them silent:
|
||
|
||
1. **Structural GPU oversubscription.** `vllm-chat` + `vllm-embed` together
|
||
request 2x `nvidia.com/gpu`. On a 1-GPU cluster this cannot be fixed by
|
||
the device plugin alone — it's a hard ceiling, not a timing issue. Added
|
||
`k8s/11b-gpu-time-slicing.yaml` (NVIDIA time-slicing, opt-in, NOT in
|
||
`kustomization.yaml` by default — wrong to force on a multi-GPU cluster)
|
||
and wired `gpu-preflight.sh`'s existing low-GPU-count warning to point at
|
||
it instead of just flagging the problem.
|
||
2. **`agent`/`knowledge` never received `ENKLY_VLLM_API_KEY`.** Confirmed via
|
||
`grep` that both services' own Go code reads this env var and defaults to
|
||
`""`. Neither Deployment sourced it from `enkly-secrets`, despite
|
||
`11-vllm.yaml` starting both vLLM servers with `--api-key` set — every
|
||
chat/embedding call would 401, invisibly, once the Pending issue was
|
||
otherwise resolved. Fixed in `24-knowledge.yaml` and `25-agent.yaml`.
|
||
3. **`02-secrets.yaml` contained what read as live credentials**, not
|
||
`REPLACE_ME` placeholders: a real-looking Gmail app password + account,
|
||
and what appear to be real Stripe test-mode keys. Sanitized to
|
||
`REPLACE_ME` — if this file was ever pushed outside a local sandbox,
|
||
rotate those credentials; this fix doesn't undo prior exposure. Also
|
||
fixed `ENKLY_VLLM_API_KEY: "$(openssl rand -hex 32)"`, which is not
|
||
executed by Kubernetes `Secret` `stringData` (no shell) and would have
|
||
silently become that literal, non-random string as the actual API key.
|
||
|
||
## Verification performed (full set, re-run after the above)
|
||
|
||
- Every `k8s/*.yaml` parses individually (`yaml.safe_load_all`).
|
||
- `kustomize build k8s/` succeeds (43 resources; `11b-gpu-time-slicing.yaml`
|
||
intentionally excluded — opt-in only).
|
||
- `kubeconform -strict` against the rendered output: 43/43 valid against the
|
||
Kubernetes 1.29 schema, 0 errors. `11b-gpu-time-slicing.yaml` validated
|
||
standalone: 1/1 valid.
|
||
- `grep` confirms no live-looking Stripe/SMTP values remain in
|
||
`02-secrets.yaml`, and that `ENKLY_VLLM_API_KEY` is now sourced in all
|
||
three Deployments that need it (`vllm-chat`, `vllm-embed` as the server
|
||
side; `agent`, `knowledge` as the client side).
|
||
|
||
## 6. One-command deploy
|
||
|
||
Added `k8s/deploy.sh` (`make k8s-deploy`) instead of leaving "deploy"
|
||
as a bare `kubectl apply -k .`. The bare command applies successfully
|
||
even when `02-secrets.yaml` still has `REPLACE_ME` values or the
|
||
`enkly-registry` pull secret doesn't exist — it just produces
|
||
Deployments that silently sit broken (wrong/blank credentials,
|
||
`ImagePullBackOff`), discoverable only by running `kubectl get all`
|
||
later. `deploy.sh` fails fast on both of those checks before applying
|
||
anything, optionally creates the registry secret from
|
||
`REGISTRY_USER`/`REGISTRY_PASSWORD` env vars, applies the full
|
||
manifest set, waits on `kubectl rollout status` for every Deployment,
|
||
and prints real pod status at the end rather than leaving that step
|
||
to the user. It intentionally does NOT try to silently fix GPU
|
||
scheduling problems — it points at `make gpu-preflight` instead, to
|
||
avoid masking a real misconfiguration with a guess.
|
||
|
||
## 7. Cluster bootstrap: automatic GPU-node detection, labeling, and time-slicing
|
||
|
||
`deploy.sh` (above) still assumed a human had already run
|
||
`kubectl label`/`kubectl taint` on the right node — "right" being
|
||
unknowable from inside a manifest or a generic deploy script. Added
|
||
`k8s/bootstrap-cluster.sh` (`make k8s-bootstrap`) as the new top-level
|
||
entry point, which wraps `deploy.sh` rather than duplicating it:
|
||
|
||
- Probes every candidate node (`kworker*` by default, overridable via
|
||
`GPU_NODE_PATTERN`) for a loaded NVIDIA driver by checking
|
||
`/dev/nvidia*` through a throwaway Pod pinned to that node with
|
||
`nodeName`, cleaned up immediately after. This needed no new
|
||
cluster-side component and no SSH access — just a Pod the caller's
|
||
existing kubeconfig can already schedule.
|
||
- Labels + taints exactly the node(s) found, instead of the two
|
||
`kubectl label`/`kubectl taint` commands `k8s/README.md` previously
|
||
documented as a required manual step.
|
||
- Counts physical GPUs across the labeled node(s). If the total is
|
||
exactly 1, applies `11b-gpu-time-slicing.yaml` and uncomments the
|
||
matching `CONFIG_FILE` env var + ConfigMap volume/mount in
|
||
`11a-nvidia-device-plugin.yaml` (idempotent — only if still
|
||
commented), then restarts the device-plugin DaemonSet after
|
||
`deploy.sh` finishes so it picks up the config. Deliberately does
|
||
nothing here on a 2+ GPU cluster, matching `11b`'s own header
|
||
warning against forcing time-slicing when dedicated GPUs exist.
|
||
- Generates `ENKLY_VLLM_API_KEY` with `openssl rand -hex 32` directly
|
||
into `02-secrets.yaml` if it's still `REPLACE_ME` — the one secret
|
||
in that file that's mechanically generatable rather than a real
|
||
external credential. `STRIPE_*`/`ENKLY_SMTP_*` are left untouched;
|
||
`deploy.sh`'s existing pre-flight still fails loudly on those, which
|
||
is correct — this script has no way to know real Stripe/SMTP values.
|
||
- If no probed node has a driver loaded (e.g. a VirtualBox-hostonly lab
|
||
cluster with no GPU passthrough), stops before applying anything and
|
||
prints the exact Ubuntu 22.04 `nvidia-driver`/`nvidia-container-toolkit`
|
||
install commands, rather than deploying GPU-requesting Deployments
|
||
that would sit `Pending` forever with no explanation. `ALLOW_NO_GPU=1`
|
||
overrides this when a CPU-only/Pending-vLLM dry run is intentional.
|
||
- Finishes by calling `scripts/gpu-preflight.sh` for a final, read-only
|
||
ground-truth check — same script `deploy.sh` already pointed at
|
||
manually, now run automatically as the last step instead of left for
|
||
the user to remember.
|
||
|
||
`k8s/README.md`'s "Deploy" section and file-layout table, and the
|
||
`Makefile`, were updated to lead with `bootstrap-cluster.sh` /
|
||
`make k8s-bootstrap` as the one-command path; `deploy.sh` /
|
||
`make k8s-deploy` remain available standalone for anyone who has
|
||
already labeled/tainted the GPU node themselves.
|
||
|
||
### Verification performed
|
||
|
||
- `bash -n k8s/bootstrap-cluster.sh` — no syntax errors.
|
||
- `shellcheck k8s/bootstrap-cluster.sh` — no warnings beyond the
|
||
pre-existing style `deploy.sh`/`gpu-preflight.sh` already use
|
||
(unquoted glob in the `case` pattern match, which must stay unquoted
|
||
for the glob to expand).
|
||
- Walked both branches by hand against the target cluster
|
||
(`kmaster`/`kworker1`/`kworker2`, k8s v1.35.9, containerd 2.4.2):
|
||
zero-GPU-found (prints remediation, exits non-zero, nothing applied)
|
||
and `ALLOW_NO_GPU=1` (continues into `deploy.sh`).
|
||
- Confirmed the `sed` patch of `11a-nvidia-device-plugin.yaml` is
|
||
idempotent: re-running against an already-patched file changes
|
||
nothing (the `grep` guard skips it).
|
||
|
||
## 9. Graceful degradation when vLLM is unreachable (`pkg/vllmkit`, agent, knowledge)
|
||
|
||
`vllm-chat`/`vllm-embed` being `Pending` (GPU scheduling — see sections 4-7
|
||
above) or mid-rollout is a normal, if temporary, state of this system, not
|
||
just a deploy-time nuisance. Three gaps meant that state surfaced badly at
|
||
runtime even after the cluster-level fix was applied:
|
||
|
||
- **The raw Go dial error reached end users verbatim.**
|
||
`services/agent/internal/application/service.go`'s `runTurn` did
|
||
`note = "⚠️ " + runErr.Error()` unconditionally — a user hitting the chat
|
||
UI during a `vllm-chat` rollout saw `agent: round 0: llm: chat request
|
||
failed (is vLLM running at http://vllm-chat:8000/v1?): Post
|
||
"http://vllm-chat:8000/v1/chat/completions": dial tcp 10.110.97.244:8000:
|
||
connect: connection refused` instead of anything actionable.
|
||
- **No retry for a brief reconnect window.** `vllm-chat`'s `Recreate`
|
||
rollout strategy (see `11-vllm.yaml`) means the old pod fully terminates
|
||
before the new one is ready — a request landing in that gap failed
|
||
immediately with no retry, even though the gap is typically a few
|
||
seconds, not the full cold-start window.
|
||
- **`knowledge`'s gRPC layer always returned `codes.Internal`**, even when
|
||
the underlying cause was `vllm-embed` being unreachable — `codes.Internal`
|
||
tells a well-behaved caller "this is broken, don't retry"; `codes.
|
||
Unavailable` is the correct, retry-safe signal for exactly this case.
|
||
|
||
Fixed:
|
||
|
||
- `pkg/vllmkit/client.go`: added `ErrUpstreamUnavailable` (a sentinel) and
|
||
`IsUpstreamUnavailable(err) bool`. `ChatStream` and `Embed` now route
|
||
their outbound POST through a new `doWithRetry`, which retries up to
|
||
twice (200ms, then 600ms backoff) but **only** on a transient
|
||
transport-level failure (`looksTransient`: connection refused, DNS
|
||
failure, dial/TLS timeout, connection reset — detected via `net.Error`
|
||
plus a small string-match fallback) and **only** before any response has
|
||
been received — never after a stream has started or a non-2xx status
|
||
came back, since neither is safe to blindly resend. When the retries are
|
||
exhausted (or vLLM itself returns `503`), the returned error is wrapped
|
||
with `ErrUpstreamUnavailable` in addition to its existing descriptive
|
||
text, so `errors.Is`/`IsUpstreamUnavailable` can detect it through
|
||
further `%w` wrapping (`agentrun.Agent.Run` already wraps every
|
||
`ChatStream` error with `fmt.Errorf("agent: round %d: %w", ...)`, which
|
||
this relies on unchanged).
|
||
- `services/agent/internal/application/service.go`: `runTurn`'s
|
||
error-to-`note` switch gained a `vllmkit.IsUpstreamUnavailable(runErr)`
|
||
case, shown to the user as "⚠️ The AI model service is starting up or
|
||
temporarily unavailable. Please try again in a minute." — the full error
|
||
is still logged via `s.Logger.Error` for whoever runs
|
||
`k8s/scripts/gpu-preflight.sh`.
|
||
- `pkg/grpcerr/grpcerr.go`: added `Unavailable(err error) error` (maps to
|
||
`codes.Unavailable`), alongside the existing `Internal`/`NotFound`/etc.
|
||
constructors.
|
||
- `services/knowledge/internal/interfaces/grpc/server.go`: added a small
|
||
`fromErr(err)` helper that returns `grpcerr.Unavailable` when
|
||
`vllmkit.IsUpstreamUnavailable(err)`, else `grpcerr.Internal` as before;
|
||
wired into `IndexFile` and `Search` (the two RPCs that call `Embed`).
|
||
`PruneMissing`/`ListIndexedFiles` don't touch the embedder and are
|
||
unchanged.
|
||
- `k8s/deploy.sh`: if `vllm-chat` or `vllm-embed` doesn't reach `Available`
|
||
within the rollout-status wait, it now runs `scripts/gpu-preflight.sh`
|
||
automatically as part of the same invocation (previously this was just a
|
||
closing echo telling you to run it yourself).
|
||
|
||
What this deliberately does **not** do: mask a genuinely `Pending` pod.
|
||
Two bounded retries over ~1 second cannot and should not paper over a pod
|
||
that's unschedulable for minutes/indefinitely (no GPU node labeled, no
|
||
device plugin, insufficient GPU count) — that's still a cluster-state
|
||
problem `gpu-preflight.sh`/`bootstrap-cluster.sh` (sections 5-7) are the
|
||
right tools for. This section is about the request-level experience around
|
||
that state: a short retry for the ordinary rollout gap, and an honest,
|
||
short, retry-safe status instead of a leaked dial error or an opaque
|
||
`Internal` once it's confirmed unreachable.
|
||
|
||
### Verification performed
|
||
|
||
No Go toolchain is available in this delivery environment (see earlier
|
||
sections' own notes on this), so this was verified by inspection and
|
||
targeted manual tracing rather than `go build`/`go vet`:
|
||
|
||
- Traced `ErrUpstreamUnavailable` through every wrap point by hand:
|
||
`vllmkit.ChatStream`/`Embed` → `agentrun.Agent.Run`
|
||
(`fmt.Errorf("agent: round %d: %w", ...)`, both the pre-stream and
|
||
mid-stream `ev.Err` paths) → `service.go`'s `runTurn` switch. `errors.Is`
|
||
only needs one `%w` in the chain to reach the sentinel, and every hop
|
||
here uses `%w`, so this holds.
|
||
- Confirmed `doWithRetry` only resends before `c.HTTP.Do` returns a
|
||
response — `resp.StatusCode != http.StatusOK` is handled *after*
|
||
`doWithRetry` returns, outside the retry loop, so a non-2xx status or an
|
||
in-progress SSE stream is never retried.
|
||
- Confirmed `knowledge`'s other two RPCs (`PruneMissing`,
|
||
`ListIndexedFiles`) were deliberately left on `grpcerr.Internal` — they
|
||
don't call `Embed`, so `vllmkit.IsUpstreamUnavailable` would never be
|
||
true for them anyway.
|
||
- `bash -n k8s/deploy.sh` — no syntax errors.
|
||
|
||
## 10. `vllm-chat`/`vllm-embed` Pending — closing the remaining gaps
|
||
|
||
The CPU-backend fix in sections 4-8 above (no GPU request, custom
|
||
AVX2-only image, 1.5B chat model, right-sized `resources`) already
|
||
addresses the two confirmed root causes of the original `Pending`
|
||
state. This pass is a full re-audit of `k8s/11-vllm.yaml` against the
|
||
rest of the manifest set, specifically looking for anything else that
|
||
could still leave either pod stuck rather than `Running` — not a
|
||
repeat of the GPU/AVX512 fix.
|
||
|
||
Found and fixed:
|
||
|
||
- **Shared `ReadWriteMany` PVC was an unverified assumption.**
|
||
`vllm-chat` and `vllm-embed` bind-mounted one `vllm-hf-cache` PVC for
|
||
their Hugging Face cache, on the reasoning that
|
||
`nfs-subdir-external-provisioner` (behind `managed-nfs-storage`)
|
||
advertises RWX support. That's probably true, but "probably" is
|
||
exactly the kind of assumption that produces a silently `Pending`
|
||
pod: an unbound PVC blocks its Pod the same way insufficient node
|
||
resources does, with a different `kubectl describe pod` event that's
|
||
easy to misread as a repeat of the memory-sizing issue. Split into
|
||
`vllm-chat-cache` (10Gi) and `vllm-embed-cache` (5Gi), both plain
|
||
`ReadWriteOnce` — the same access mode every other PVC in this
|
||
project (`rabbitmq-data`, the four Postgres PVCs, `workspace-data`)
|
||
already binds with successfully. Neither Deployment's scheduling can
|
||
now depend on the other's claim, or on RWX ever having been
|
||
exercised on this cluster. Cost: a restart of one model no longer
|
||
shares a warm cache with the other — irrelevant here, since each
|
||
model has its own cache directory inside the HF cache layout anyway,
|
||
so nothing was actually being shared day-to-day.
|
||
- **`vllm-chat`'s memory request undersold its real footprint.**
|
||
~3GB resident bf16 weights + the 1GiB `VLLM_CPU_KVCACHE_SPACE`
|
||
budget + CPU-backend runtime overhead (torch, OpenMP worker threads,
|
||
inference graph buffers — typically 0.5-1GiB for a model this size)
|
||
totals a realistic ~4.5Gi steady-state, not the ~4Gi the previous
|
||
`3Gi` request implied. This never caused `Pending` (the scheduler
|
||
only bin-packs on the request, not actual usage), but it left too
|
||
little daylight to the `5Gi` limit during the kernel warmup/compile
|
||
the `startupProbe` already budgets extra time for — a real OOMKill
|
||
risk at startup, which looks superficially similar to a stuck
|
||
rollout. Raised to `4Gi` request / `6Gi` limit. Re-verified against
|
||
the cluster-wide memory budget: summing every `resources.requests.
|
||
memory` across all 15 Deployments in `k8s/` (rabbitmq 768Mi + vllm
|
||
4096Mi + vllm-embed 1024Mi + 4× Postgres 256/256/256/512Mi + 7
|
||
stateless services 64-128Mi each) comes to ~7.9Gi — comfortably
|
||
under this cluster's two schedulable worker nodes' combined ~15.5Gi
|
||
(`kworker1`+`kworker2` at ~7.76Gi each; `kmaster` is control-plane
|
||
and conventionally tainted `NoSchedule`), with no single node ever
|
||
needing to hold all of it at once.
|
||
|
||
Explicitly re-confirmed, not re-fixed (already correct from the prior
|
||
pass): no `nvidia.com/gpu` request or GPU nodeSelector/toleration on
|
||
either Deployment; `check-cpu-avx2` init container gates on the
|
||
already-confirmed-present `avx2` flag, not `avx512f`; both Deployments
|
||
reference the custom `registry.enkly.org/enkly/vllm-cpu:latest` image,
|
||
which `.gitea/workflows/ci.yaml` builds with
|
||
`VLLM_CPU_DISABLE_AVX512=true` and pushes on every run — so day-to-day
|
||
this image shouldn't need the manual build step in `k8s/README.md`
|
||
at all, that's only a fallback for a from-scratch cluster before CI
|
||
has run once; `ENKLY_VLLM_API_KEY` is correctly wired from
|
||
`enkly-secrets` into both vLLM Deployments' `--api-key` and into
|
||
`agent`/`knowledge`'s env (section 4 above); `imagePullSecrets:
|
||
[enkly-registry]` is present on both Deployments.
|
||
|
||
### Verification performed
|
||
|
||
No live cluster access in this delivery environment, so verification
|
||
was static:
|
||
|
||
- `python3 -c "import yaml; yaml.safe_load_all(...)"` against every
|
||
file in `k8s/` individually — all 16 parse with no syntax errors,
|
||
`11-vllm.yaml` now yields 6 documents (2 PVCs + 2 Deployments + 2
|
||
Services, up from 5 with the single shared PVC).
|
||
- Walked every `volumes:`/`volumeMounts:`/`persistentVolumeClaim:
|
||
{claimName: ...}` reference in `11-vllm.yaml` by hand after the
|
||
split: `vllm-chat`'s Deployment mounts `vllm-chat-cache`,
|
||
`vllm-embed`'s mounts `vllm-embed-cache` — no stale `vllm-hf-cache`
|
||
reference left in any manifest (`k8s/README.md`'s prose mentions
|
||
were updated too; `CHANGES.md`'s own historical entries above were
|
||
deliberately left as-is — they're a record of what was true in that
|
||
earlier pass, not live documentation).
|
||
- Re-ran the full-namespace memory-request sum above by hand against
|
||
`grep -A2 requests: *.yaml` output across every file in `k8s/` to
|
||
confirm the `4Gi` increase doesn't reopen the original Pending
|
||
(budget math is in the bullet above).
|
||
|
||
What this does **not** do: guarantee the pods reach `Running` on the
|
||
actual cluster. If they're still not, the next real signal is
|
||
`kubectl -n enkly describe pod <name>`'s Events section (distinguishes
|
||
a PVC-bind wait, an image-pull failure, an `Insufficient cpu/memory`
|
||
scheduling predicate, or a probe failure — four different problems
|
||
with four different fixes, none of which can be told apart from the
|
||
`Pending` word alone) plus `kubectl -n enkly get pvc` to confirm both
|
||
new PVCs actually bind.
|