feat(chat): let Project Assistant read Template README - #339
Conversation
319a14a to
b1b9b30
Compare
zjy365
changed the title
|
@cursoragent review this |
|
Reviewed PR #339 and posted the review plus two inline comments. Isolation and scoping look correct; I am not requesting changes. The tool is Project-only, schema-strict, and allowlisted from this Project’s template tasks and adopted instances. Foreign names never hit the provider, YAML/tokens never reach the model, and workspace chats do not get the tool. Two behaviors are still worth fixing or explicitly accepting: cancel/timeout is stored as “README could not be loaded,” and the 2 MiB cap applies to the whole Walkthrough: PR #339 Template README 审阅   |
ReviewComment, not request-changes. Isolation, scoping, and “README is documentation, not live state” are in good shape. I would not block merge on security. Two behaviors are still worth fixing or explicitly accepting before this ships. What holds
CI Should fix, or accept in writing1. Cancel / timeout is stored as “README could not be loaded” Rethrow abort ( 2. The 2 MiB cap is the whole Return an explicit “provider payload too large” tool error, or spell out in ADR 0077 that 2 MiB is the entire JSON, not README bytes. Do not fetch Suggested, not blockingREADME is raw markdown in the tool result. The description and On an allowlist miss, return the same TestsThe PGlite SQL test is the one that actually proves tenant isolation (kind, status, cross-project, cross-namespace). Worth adding before this bit-rots:
Residual (already in the PR body / ADR)Provider must enable README retrieval or the tool always looks empty. Returned docs are the current catalog version, not the deployed revision. GitHub-source tasks correctly never appear. Historical version matching is out of scope. No migration, env var, public route, or approval-policy change. Direct execution on main is untouched. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // The endpoint also returns template YAML. Bound the entire response before parsing it. | ||
| const reader = response.body.getReader(); | ||
| const decoder = new TextDecoder(); | ||
| let text = ""; | ||
| let bytes = 0; | ||
| try { | ||
| while (true) { | ||
| const chunk = await reader.read(); | ||
| if (chunk.done) { | ||
| break; | ||
| } | ||
| bytes += chunk.value.byteLength; | ||
| if (bytes > 2 * 1024 * 1024) { | ||
| throw new Error("Template README response is too large."); |
There was a problem hiding this comment.
The 2 MiB bound is the whole getTemplateSource JSON (YAML + source + README), not README text.
A small README next to a large appYaml throws "Template README response is too large." before the 32k slice, and tool.ts then collapses that into the generic load error. ADR 0077 is easy to read as a README-size cap.
Either return an explicit “provider payload too large” tool error, or state in the ADR that 2 MiB is the envelope. Do not fetch spec.readme URLs from Brain to work around this.
There was a problem hiding this comment.
Addressed in a1a2f63c. Oversize is now TemplateReadmePayloadTooLargeError (“including YAML and README”), the fixture is a short README plus a 2 MiB appYaml, and ADR 0077 states the cap is the whole provider JSON. The tool returns that message instead of the generic load failure.
| } catch { | ||
| return { | ||
| ok: false as const, | ||
| error: | ||
| "Template README could not be loaded. Continue with other tools.", | ||
| }; | ||
| } |
There was a problem hiding this comment.
Cancel/timeout is stored as a successful “could not be loaded” result.
execution.abortSignal is forwarded, then every rejection — including AbortError from user stop and the 15s AbortSignal.timeout — is mapped to { ok: false, error: "Template README could not be loaded…" }. That persists as a normal tool result, so a later turn treats documentation as missing instead of interrupted.
Rethrow abort (error.name === "AbortError" or execution.abortSignal?.aborted). Keep this catch only for provider/parse failures. The same catch-all exists on catalog/OpenAPI tools, but this tool wired the abort signal on purpose.
There was a problem hiding this comment.
Addressed in a1a2f63c. User cancellation now rethrows (abortSignal.aborted or AbortError). The 15s budget is a retryable timeout result, not “could not be loaded.”
Nit if you touch this again: getTemplateReadme remaps any error to timeout.reason once timeout.aborted is true, so a TooLarge that races the budget would look like a timeout. Not blocking.
|
@cursoragent review this |
|
Re-reviewed PR #339 at User stop now rethrows instead of storing “could not be loaded.” The 15s budget is a retryable timeout result. Crossing 2 MiB on the whole provider JSON (short README + large YAML) returns an explicit oversize error, and ADR 0077 names the envelope. Isolation and Project-only scoping are unchanged. One leftover nit: if the 15s signal is already aborted, Walkthrough: PR #339 Template README 审阅 |
Re-review (
|
|
@cursoragent review this |
|
Re-reviewed PR #339 at The new work after The final prompt rewrite ( Walkthrough: PR #339 Template README 审阅 |
Re-review (
|
| chatTurnId, | ||
| userId: owner.userUid, | ||
| // Group telemetry by the verified workspace, matching Devbox scope. | ||
| userId: owner.namespace, |
There was a problem hiding this comment.
Langfuse userId is now the workspace namespace. Conversation ownership still uses userUid, but ADR-0056 says traces remain associated with the owning user. Metadata has no UID, so every member of ns-… collapses into one Langfuse user.
Keep userId: owner.userUid and put namespace in metadata/tags, or keep this grouping and add metadata.userUid plus an ADR-0056/0059 amendment. There is no route test for this value.
| "Prefer product tools for AP/DB work: read current state, draft the requested change, then apply it. Public addresses and domains belong to AP network settings. Use sandbox commands when product tools are insufficient.", | ||
| "Resolve Project targets from context or Project tools. Delete a Project only with `previewProjectDeletion` then `deleteProject`, copying preview values exactly; never delete a Project or namespace through shell commands. After deletion, refresh frontend caches and navigate away if that Project was active.", | ||
| "For named application deployments, call `searchDeployCatalog` first. Prefer a matching `template`; otherwise use `github` for a supplied repository, or `prompt` for a description. Use `docker` only for an explicitly supplied image. Ask when several Templates match, copy templateName exactly, and collect missing required args in `source.args`. Never invent image names or secrets.", | ||
| "If GitHub authentication is required, help the user connect or sign in; do not switch the repository to another source type.", |
There was a problem hiding this comment.
810f0d1a still named readProductResource → draftProductResourceChange → writeProductResource and “search catalog first, even if you recognize it.” This rewrite keeps the order but drops the tool names and the recognize clause.
The model binds to tool names. Catalog and product-tool descriptions still carry the how; the system prompt is what stops bash/kubectl becoming the default write path. Restore those three 810f0d1a bindings inside this capability framing.
2 participants
Summary
Let the Project Assistant read the current Project's Template README when answering application usage and configuration questions. Add one
readTemplateReadmetool that returns documentation through the existing Chat tool-result flow.getTemplateSource?includeReadme=trueendpoint and its README retrieval. Return only README text, with a 15-second request budget, a 2 MiB response cap, and explicit truncation at 32,000 characters.This PR adds no database migration, environment variable, public route, dependency, or approval requirement. It does not add a general Project Context Index or Runtime Contract, and does not change deployment routing or the direct-execution behavior on main.
Refs labring/sealos-private#154. The broader index proposed in labring/sealos-private#153 is not delivered by this PR.
ns-...) inuserId; conversation ownership continues to use the global user UID.Validation
Rebased onto main at
ffce30ee(includes feat(chat): add guarded Pi-style Devbox tools #320 and fix(chat): execute tools directly by default and unblock pending approvals #350).24 focused tests passed after review fixes (including cancellation, body-read timeout, and short README with oversized YAML): README selection, SQL Project/namespace isolation and adopted Templates, tool-result conversion to model context, provider failures/cancellation/limits, and existing provider behavior.
69 Chat route tests passed in a separate process.
bun typecheck,bun check,bun lint, andgit diff --checkpassed.Live check through the new provider reader returned the Memos README: 1,263 characters, not truncated.
Final product-capability prompt revision: 8 context/quota tests and 69 Chat route tests passed;
bun typecheckandbun checkpassed. Live session confirmed README retrieval before this revision; the revised tool-selection wording still needs a model conversation replay.Limits
The Template Provider must support and enable README retrieval. Projects without a recorded Template source cannot resolve a README. Returned documentation is the provider's current version; historical deployment-version matching is not added. The live check covered the provider reader, not a complete browser conversation with a model.