Repository navigation
feat: Frame templates - #84
Conversation
… from it Publishing with template_id now asserts create intent and refuses the publish when a required slot is empty, merging those violations with any schema errors into one response. An unusable template_id (unknown or another org's) is reported as an invalid publish request, not a missing template, so existence does not leak. PublishDoc and PublishDocFrom keep their existing signatures as thin wrappers over the new PublishDocRequest/PublishRequest entry point, so no existing caller changes.
…not InvalidArgument Marshal here serializes a document the server already built and validated, so a failure is a server-side invariant break, not bad caller input. Matches ConvertFrame's handling of the same call elsewhere in this file.
Once a template is picked, fetch it (getFrameTemplate), parse its prefill through the same parseFrameContent/docToForm steps the edit path already uses, and reset the form with the result. Sections the template marks required or recommended are pre-added rather than hidden behind "+ Add section", unioned with whatever the prefill itself populated so prefilled content is never collapsed. A required section's "Remove section" control is withheld, and a rule's note replaces the generic per-slot hint. Also widens frameDocSchema.name to default to "" like its sibling metadata fields: a template's prefill is a strict subset of the Frame schema that never sets identity fields, so the shared parser must decode a prefill missing name the same way it already tolerates a missing description or version.
…n the section The submit handler now sends templateId via publishTemplateID(), so the backend can check the chosen template's required slots on create. Doing so required switching the publish mutation from the callback-style mutate() to mutateAsync() so the request is a single object rather than a request plus an onSuccess/onError pair; publishFromMarkdown and convertFrame are untouched. No change was needed to render a required-slot violation inside its section: TerminologyEditor, ListEditor, and MarkdownField already each render a FieldError for their own slot path, and mapPublishError already keys fieldErrors by that same path, so the message already lands where the section is. Updated FrameAuthoringPage.create.test.tsx's mutation mock to match (mutateAsync instead of mutate, and a rejected promise instead of an onError callback) since the assertions it makes did not need to change.
publishFromMarkdown published without templateId, so an author who chose a template and then switched to the Markdown editor before publishing had the template's required-slot check silently skipped, unlike publishing from the document editor. Both paths now compute the id the same way, so the check an author opted into no longer depends on which editor tab they used.
Adds an admin page for creating, editing, and deleting an org's own Frame templates, plus the per-slot FieldRuleEditor it uses to set each content slot's requirement level and author note. Built-in templates get no edit or delete controls since the backend refuses both for them. Editing replaces the stored template wholesale, so the form always sends every field, and the prefill blob is restricted to slots and extends so no identity field can leak into it.
Adds a test that exercises the actual edit flow: fetch a template row with an existing extends suggestion, change only the title, and assert the update request both carries the new title and still carries the original extends after the round trip through serializeFramePrefill. Writing that test surfaced a real bug: the edit-seeding effect had no guard against re-running once the form was populated, so a query result with a new-but-equivalent object identity (which a background refetch can produce) would silently re-reset the form and drop whatever the admin had typed. Now guarded on a ready flag so the seed runs at most once per dialog open. Also passes the real canManage flag to the built-in template group instead of a hardcoded false; the per-row gate already excludes built-ins on its own, so this only removes a confusing, redundant flag.
…ions, lint e2e, gate authoring template query, clarify docs - Export the slot schema from frame-yaml.ts instead of keeping a byte-identical copy in AdminTemplatesPage.tsx, so the ten slot keys have one source of truth. - Assert the exact slot and level a field rule maps to in the admin template and FieldRuleEditor tests, instead of only checking that some rule exists. - Have `make lint` also run golangci-lint against the build-tagged e2e package, which the default invocation can't see. - Gate the authoring page's template-list query on the picker actually being shown, matching how its sibling query is already gated. - Note in code that the built-ins-first template order is the guarantee the CLI relies on, not something the web picker (which groups by origin) follows. - Correct the ADR's RBAC claim: templates have no per-item grants, but manage operations require an admin caller, not just org membership. - Reword PublishDocRequest's doc comment so it's unambiguous that it loses the author's original bytes, and name the path that preserves them instead.
|
📄 Docs preview for |
…nt works Keycloak's declarative user profile is enabled by default and marks firstName and lastName required. A user created without them is left incomplete, and the direct grant refuses with "Account is not fully set up" even though the account exists, is enabled, and the password was accepted with temporary=false. requiredActions is cleared explicitly for the same reason: a realm default such as UPDATE_PASSWORD blocks the grant identically, and the realm's defaults are not ours to assume. The failure path now dumps what Keycloak thinks is outstanding. The error text alone names no unmet condition, and the account disappears with the cluster, so without this a repeat costs another full sandbox run to diagnose.
Every RPC in the first real sandbox run failed with "certificate signed by unknown authority", while the curl-based provisioning steps in the same job authenticated against the same gateway using the same file. curl and Go do not agree on what may serve as a trust anchor: Go requires the CA basic constraint that curl will do without. The harness now loads FRAMES_E2E_CA_CERT into the root pool itself, alongside the system roots. Verification stays on; a suite that skips certificate checks is not exercising the path it claims to. A file that holds no certificate now fails by name against its path rather than surfacing later as an unexplained verification error. The step also prints the anchor's subject, issuer and basic constraints, and the chain the gateway presents, before running the suite. The error text names no cause and the cluster is gone by the time anyone reads the log, so without this a repeat costs another full sandbox run to explain.
The sandbox's advertised CA file is a self-signed leaf: subject equals issuer, CA:FALSE marked critical, and the gateway presents a chain of one. It cannot be a trust anchor. Go enforces basic constraints and refuses it, which is why the same file verifies under curl, where OpenSSL will anchor on a self-signed certificate by exact match, and fails under Go with "certificate signed by unknown authority". Adding it to a root pool does not help, because the pool is still asked to build a chain. The harness now branches on what the file actually holds. A real CA is added to the root pool and verified normally. A self-signed leaf is pinned instead: chain building is turned off and replaced with a byte-identical comparison against the certificate on disk, which is a stricter test than chain verification rather than a weaker one. Any other certificate fails, whoever signed it. The alternative was to skip verification, which would have made the suite prove nothing about the transport it claims to exercise.
The probe after the CoreDNS patch fired a single nslookup and failed the job on one negative result. Across four sandbox runs it failed, passed, passed, failed, so roughly half of them died before reaching anything under test. It is a race, not breakage. `rollout status` reports the new CoreDNS pods Ready before the new Corefile is necessarily live in them: a ConfigMap mounted as a volume propagates on kubelet's sync interval, and CoreDNS reloads on a poll of its own. Sampling at the earliest possible instant loses that race often, and a lost race looks exactly like the misconfiguration the probe exists to catch. Now retries to a two-minute deadline, so slow propagation reads as slow and only genuine breakage reads as broken, and dumps the Corefile the ConfigMap actually holds when it does give up. Unrelated to the templates work in this branch; it is fixed here because the flake blocked this branch from demonstrating its own e2e suite. Stands alone and can be cherry-picked.
…he action reports Pinning against the sandbox action's ca-cert-path failed with a presented-versus-pinned mismatch. That file is a self-signed leaf, not an authority: CA:FALSE, key usage Digital Signature and Key Encipherment, no Certificate Sign. It is also not the certificate this gateway presents. curl never noticed, because OpenSSL will anchor on a self-signed certificate by exact match and the provisioning calls talk to a different host. The serving certificate now comes from the nebari-gateway-tls secret over the authenticated kubeconfig, so the expected identity is learned from a trusted channel rather than from the connection under test, which is what pinning whatever the server happens to offer would amount to. Also prints the fingerprints of the reported path, the secret, and what the gateway actually serves. Three runs were spent characterising a mismatch that one line of output would have explained.
The e2e-sandbox job pinned envoy-gateway-system/nebari-gateway-tls, which is the catch-all HTTPS listener's certificate. The nebari-operator gives each NebariApp its own Gateway listener with its own certificate whose only SAN is that app's hostname, and Envoy serves that one for the app's SNI - so the suite pinned a certificate the server never presents and failed with a presented-versus-pinned mismatch. Verified against a live cluster: nebari-gateway-tls fingerprints 28:24:56:... (SANs apex, keycloak, argocd) while frames.<domain> presents E0:41:9B:..., matching the per-app secret the operator created. The job now resolves the certificate from the listener whose hostname is under test, which also survives a rename of the operator's "<app>-<namespace>-tls" convention. With that fixed the whole suite passes against a real deployment with real auth: 6/6, through Envoy, on an operator-provisioned OIDC client. Also corrects a wrong explanation that was in both harness.go and the job. Go does not refuse a self-signed CA:FALSE leaf as a trust anchor: x509 short-circuits on opts.Roots.contains(c) and returns a one-element chain without consulting basic constraints. Pinning is still the right choice, but because it states the exact-match requirement in our code rather than inheriting it from an x509 internal - not because chaining would fail. That mattered practically, since it made "unknown authority" look like a constraints problem when it was really the wrong certificate. docs/qa/e2e-token-acquisition.md gains the cluster-specific half of this: the audience is the operator's SPA client read from its secret, Keycloak's declarative user profile requires firstName/lastName, and the trust bundle mounted into the pod is a separate concern that must carry public roots too.
jbouder
left a comment
jbouder
left a comment
There was a problem hiding this comment.
One blocker inline: saving a template then reopening Edit seeds the dialog from the stale cached row, so a second Save reverts the first edit. Four should-fix items follow, all on the same theme of --template forcing create intent and the web surfacing (or not surfacing) the resulting errors. The rest of what I found is minor and can follow up.
| // form - discarding whatever the admin had already typed - every time | ||
| // that reference changes. Once ready, this effect is a no-op for the rest | ||
| // of the dialog's lifetime. | ||
| if (target.mode !== "edit" || ready) return; |
There was a problem hiding this comment.
Blocker: editing a template twice reverts the first edit.
onSaved (line 405) only invalidates the listFrameTemplates key (lines 351-354). The getFrameTemplate row this dialog seeds from is left in the cache. On reopen, React Query hands back that cached row synchronously and refetches in the background. This effect resets the form from the old row and sets ready, and the guard here then ignores the fresh data when it arrives.
Sequence: edit template T, save. Click Edit on T again. The form shows the pre-edit content. Save writes the old values back over the edit you just made, with no warning.
The page tests don't catch it because each query is mocked with fixed data, so nothing is ever stale.
Fix: also invalidate the getFrameTemplate key for the saved id in onSaved. Alternatively give rowQ staleTime: 0 + refetchOnMount: "always" and gate this seed on !rowQ.isFetching so it waits for the fresh row.
There was a problem hiding this comment.
Fixed, though not the way the first fix suggests: invalidating the getFrameTemplate key isn't enough on its own. React Query keeps invalidated data in the cache and only refetches active queries, so the reopened dialog is still handed the pre-edit row synchronously while the refetch runs behind it — and the ready latch then ignores the fresh row when it lands. I took the alternative you offered: refetchOnMount: "always" plus gating the seed on isFetchedAfterMount.
That guard alone was still wrong. A failed fetch also satisfies isFetchedAfterMount (query-core counts errorUpdateCount alongside dataUpdateCount), and its error reducer keeps the cached data rather than clearing it — so a forced refetch that failed over an already-cached row seeded from that row and then latched, hiding the error. Both effects now refuse to seed while error is set. The rule is one function, freshData() in web/src/lib/query-freshness.ts, with the library reasoning in one place.
The authoring page seeded the same way from the same RPC, so your bug was reachable across pages too (admin edits a template, author creates from it in the same session) — same fix, and both surfaces have a stale-seed regression test that I checked fails without the guard.
| // once at creation, so applying one to a Frame that already exists is a | ||
| // mistake rather than an update, and the existing create-intent branch | ||
| // below reports it as AlreadyExists with a message that says so. | ||
| intent = PublishCreate |
There was a problem hiding this comment.
Name collision on create now lands under the version field in the web app.
Every web create publish carries a template id (even "Blank" is builtin:blank), so every web create is now PublishCreate and a name that already exists returns AlreadyExists. web/src/lib/publish-errors.ts:14 unconditionally maps AlreadyExists onto the version field, and FrameAuthoringPage.tsx:332 keeps the publish dialog open whenever version has an error. The author sees "a frame named X already exists; update it instead" under the version input, and changing the version retries into the same error.
This is better than the pre-PR behaviour (a silent upsert onto someone else's Frame), it just stops one step short. Fix on either side: attach a FieldViolation for name here, or have mapPublishError route a frame-name AlreadyExists to name.
There was a problem hiding this comment.
Fixed server-side. nameTakenErr now attaches a FieldViolations detail naming name, and mapPublishError honours a named field on AlreadyExists, keeping version as the fallback for a bare republish. Since name lives in the form and version in the publish dialog, the dialog closes and the message lands on the input that can actually fix it.
I also recorded it in the contract: the FieldViolations proto comment said the detail was a PublishFrame-schema-validation thing, and PublishDoc's error list bound field violations to InvalidArgument. A client author reading either would route every AlreadyExists to the version input — the same bug, one layer up. Both amended, gen/ regenerated.
| fmt.Fprintf(&b, "# %s\n", t.Description) | ||
| } | ||
| b.WriteString("#\n# Fill in name, description, and version, then publish with:\n") | ||
| b.WriteString("# frames publish --dir . --template " + t.Id + "\n") |
There was a problem hiding this comment.
This header documents a command that fails on the second publish.
--template now forces create intent (service.go:125), so frames publish --dir . --template <id> works for 1.0.0 and returns AlreadyExists ("update it instead") for every version after. publish.go:30-35 only special-cases InvalidArgument, so the user sees a bare already_exists: error with no hint to drop the flag. And because the scaffold comment persists into stored content by design, the wrong instruction stays in the file forever.
Fix: word this as "publish the first version with" (and say later versions omit --template), and add an AlreadyExists branch in publish.go that suggests dropping the flag.
There was a problem hiding this comment.
Fixed both halves. The header now names the first-version command and the bare one for every version after, in two lines rather than six, since these comments persist into stored content. frames publish explains an AlreadyExists when --template was passed, and says nothing about the flag when it wasn't — no pointing at a command the author never ran.
One thing beyond what you flagged: the flag's own help text never said it asserts a new Frame, which is the first place someone looks. It does now, and docs/site/.../frames_publish.md is regenerated.
| ); | ||
| // Fetched only once a template is chosen. Disabled otherwise so the picker | ||
| // screen does not issue a pointless request. | ||
| const chosen = useQuery( |
There was a problem hiding this comment.
Template query failures are swallowed in the create flow.
Neither chosen.error nor templates.error is read anywhere in this component.
- A failed
listFrameTemplatesrendersTemplatePickerwith[](line 395), which draws the heading and intro text and no cards. I hit exactly this today against a dev DB missing theframe_templatestable: a blank picker with no explanation. - A failed
getFrameTemplate(deleted org template, foreign org id, typo in?template=) renders a blank form, butpublishTemplateIDstill sends the bad id, so every publish fails with "unknown template_id" and the only way out is hand-editing the URL.
Fix: render templates.error inside the picker, and on chosen.error show an alert with a "choose a different template" action that clears the template search param.
There was a problem hiding this comment.
Fixed. TemplatePicker takes failed/onRetry and renders the failure instead of drawing a heading over an empty catalog. A failed getFrameTemplate is now a dead end with a retry and a "choose a different template" that clears the id from the URL, rather than a blank form whose every publish is refused. I also narrowed the fetch to publishTemplateID(...) !== "", so the fetch and the publish can't disagree about whether the template matters.
One deliberate difference from your description: the picker states a fixed sentence rather than the server's text, matching the eight other read failures in the app. Your case is why — no such table: frame_templates is storage detail an author can't act on, and the alert plus retry answers "why is this empty?" without it. Push back if you'd rather see the raw message on these screens; it's a one-line change and I'd rather the app be consistent either way.
| const templateFormSchema = z.object({ | ||
| title: z.string().trim().min(1, "must not be empty"), | ||
| description: z.string().trim().min(1, "must not be empty"), | ||
| slots: slotsSchema, |
There was a problem hiding this comment.
Blank list rows persist into org template prefill and break the CLI scaffold.
slotsSchema allows empty strings in rules / skills / prompts, and compactSlots (frame-yaml.ts:62) keeps any non-empty array, blanks included. On the server, ParsePrefill (templates.go:207) only calls Parse, never Validate, so rules: [""] is stored as-is.
frames template init then splices the prefill verbatim into frame.yaml, and frames publish fails with slots.rules[0]: must not be empty for a row the author never wrote.
Fix: drop blank list entries in compactSlots, and run the list-item checks from Validate inside ParsePrefill so a bad blob is refused at save time rather than at the author's first publish.
There was a problem hiding this comment.
Fixed, but not inside ParsePrefill. That decoder is also the read path — rowToTemplate runs every stored row through it — so a content rule there would make a template already saved with a blank row unreadable (GetFrameTemplate and the post-save read both fail) instead of merely unpublishable. Strictly worse than the bug.
Instead the checks are shared out of Validate as contentErrors and run at the write boundary, in validateTemplateInput, covering create and update (an update replaces the row wholesale, so a create-only rule is one an update walks past). TestParsePrefill now pins the decoder's leniency on purpose so nobody "fixes" it later. The refusal goes through violationErr like every other ValidationError crossing that boundary, so the field paths reach clients as a detail.
Client side, compactSlots drops blank list rows and fully-empty terminology rows (a half-filled row survives — that's a real error the author meant to write). The template dialog was also validating with frame-yaml's lenient decode schema rather than the authoring form's rules, so an incomplete row still reached the server; both forms now resolve against contentSlotsSchema.
…ot be read Saving a template and reopening Edit seeded the dialog from the row cached before the save, so a second Save wrote the pre-edit content back over the first edit with no warning. Invalidating the row key would not have fixed it: React Query keeps invalidated data in the cache and refetches only active queries, so the reopened dialog is still handed the stale row synchronously while the refetch runs behind it, and the `ready` latch then ignores the fresh row when it lands. The seed instead waits for a fetch this mount performed (`isFetchedAfterMount`, which exists for exactly this), with `refetchOnMount: "always"` so there is always such a fetch to wait for. The authoring page seeded the same way from the same RPC, so an admin's edit was invisible to a create started from that template in the same session. It gets the same guard. Waiting on a fetch makes a failed one matter, and neither page reported those: - A failed listFrameTemplates drew the picker's heading and no cards, which reads as "your org has no templates" - never true, since the built-ins are compiled in. TemplatePicker now takes the error and a retry. - A failed getFrameTemplate rendered a blank form that could never publish: the id stays in the URL and every publish carries it, so the server refuses each one. It now offers retry and "choose a different template", which clears the id. - In the edit dialog, both a failed fetch and a prefill that will not decode left the loading skeleton up forever - the decode failure set `formError`, which only renders inside the form it never reached. The template fetch is also narrowed to the flow that sends its id, so publishTemplateID decides both and the fetch cannot disagree with the publish about whether the template matters.
Every web create publish carries a template id (Blank is builtin:blank), so every web create now asserts create intent, and a name that already exists comes back as AlreadyExists. The web app mapped that code unconditionally onto `version`, so the author read "a frame named X already exists; update it instead" under the version input - and changing the version retried straight into the same error. A republished version uses the same code, and the two are fixed in different inputs, so the code alone cannot route them. The server now attaches a FieldViolations detail naming `name` (connect-go allows details on any error), and the client honors a named field when there is one, keeping `version` as the default for a bare republish. Since `name` lives in the form and `version` in the publish dialog, the dialog now closes and the message appears where the fix is.
The scaffold's header told an author to publish with `--template <id>`. That flag asserts a new Frame, so it works for 1.0.0 and returns AlreadyExists for every version after - and since the header persists into the published content by design, the wrong instruction stayed in the file for good. The header now names the first-version command and the bare command for every later one. `frames publish` also explains the refusal when the flag is what caused it, rather than printing a bare already_exists; it stays quiet about the flag when the author never passed one, so nothing points at a command they did not run.
A list editor leaves an empty row behind the moment an author adds one and thinks better of it, and nothing stopped one from being stored in a template's prefill: the web serializer kept any non-empty array, blanks included, and the server never checked a prefill's content. `frames template init` then spliced it verbatim into frame.yaml and `frames publish` failed with "slots.rules[0]: must not be empty" - naming a row some other author never wrote. The serializer drops blank list rows, for frames and prefills alike, and the content checks are split out of Validate so a prefill is held to the same rules a published Frame is. Those checks run at the write boundary rather than inside ParsePrefill. ParsePrefill is the read path too - rowToTemplate decodes every stored row through it - so refusing a blank row there would make a template already saved with one unreadable, failing GetFrameTemplate and the post-save read instead of merely failing a publish. TestParsePrefill now pins that leniency deliberately, and the write test covers create and update, since an update replaces the stored row wholesale.
Two defects the fixes introduced, one they left half done, and the contract they changed without saying so. - A later query failure replaced a form the author was already typing into. Both error screens gated on error-truthiness alone, so a failed refetch - reconnect, a token refresh, a delete in another session - evicted unsaved work to report a template that had demonstrably just loaded. They now render only while there is nothing to show. - Blank terminology rows still reached the server. `compactSlots` dropped abandoned rows for rules/skills/prompts but passed terminology through whole, and the template dialog validates with the loose slot schema rather than the authoring form's, so "+ Add term" then thinking better of it hit exactly the publish failure this branch exists to prevent. A row with one side filled still survives: that is a real error the author meant to write. - `FieldViolations` is no longer only an InvalidArgument detail, so the proto comment and `PublishDoc`'s error list now say where else it is attached and what its absence means. A client author reading the old enumeration would route every AlreadyExists to the version input, which is the bug the previous commit fixed in the web app. - The prefill check now goes through `violationErr` like every other ValidationError crossing that boundary, so its field paths reach clients as a detail rather than only inside a flattened message. Also: the authoring page's seed guard had no test - deleting it broke nothing - and was not paired with `refetchOnMount: "always"`, so it passed only because no global staleTime is set. Its latch is now per template rather than per mount, so returning to the picker cannot leave the first template's content in a form publishing under the second one's id. Read failures state a fixed sentence, matching every other read failure in the app rather than putting storage detail on screen. `loadBuiltins` runs the content check too, so the rule guards both doors starters arrive through. And `--template`'s own help says it asserts a new Frame.
…lot forms to one content schema A failed fetch satisfies `isFetchedAfterMount` - query-core counts data updates and error updates alike - and its error reducer keeps whatever was already cached, flagging it invalidated rather than clearing it. So a forced refetch that failed over an already-cached row still looked, to both seed effects, exactly like fresh data arriving: they seeded from the stale row, latched, and suppressed the error for the rest of the session. That is the bug this mechanism exists to refuse, reached through the error path instead of the cache path. Both effects now refuse to seed while `error` is set. The dialog's "returned no content" branch was reached by that same path, so it also overrode the accurate message on every ordinary failure - a deleted template reported as empty content. It now speaks only about a fetch that actually succeeded, and its test asserts which sentence appears rather than only that the old one does not. The template dialog also validated slot content with the decode schema rather than the authoring rules, so an admin could save an incomplete terminology row. Serialization drops rows that are entirely empty, so a violation the server reported could name an index that no longer matched the form. The strict rules move to `contentSlotsSchema`, both forms resolve against it, and the decode schema is unexported with the split written down: one shape parses what is stored, the other governs what a human may submit - the same division `Parse` and `contentErrors` make on the server. AGENTS.md records the three invariants this round had to re-derive: the four options that make a latching seed correct, the two-schema split, and read versus write error text.
…e once An array-level rule reaches react-hook-form nested under `<path>.root` whenever that path has registered children - @hookform/resolvers does that in toNestErrors - and the field reader only looked at `.message`. So the duplicate-term refine produced an error that rendered nowhere and a form that refused to submit: a Save button that did nothing at all. Sharing the authoring content rules with the template dialog last commit extended that silence to a second surface, where duplicates had simply saved before. Reading the nested message fixes both, including the authoring form, where it has been silent since the refine was written. The rest is consolidation of three rounds of patches: - `freshData()` names the rule both seed effects were open-coding, with the library reasoning in one place instead of the same eight comment lines in two files. Both effects are now two lines and read as one intent. - A prefill that will not decode no longer latches as seeded. It seeded nothing, so calling it seeded left the author on a defaults-only form with the id still in the URL and still sent by every publish - required slots checked against content they never saw. It is a dead end with the same two ways out as a failed fetch. - The edit dialog is keyed per target, so "mounted per target" is structural rather than a consequence of the dialog being modal. That is what makes a boolean latch correct there, and AGENTS.md now says which latch belongs where instead of asserting one rule for both. - The dialog's "returned no content" branch is gone: `GetFrameTemplate` cannot produce that state, no test exercised it, and the error alert already covers every failure that is reachable. - The picker no longer blames the registry for being unreachable. A client cannot tell that from a registry that answered with an error, and the case that prompted the finding - a database missing the templates table - is the second. - The built-ins content check was a call that could never fire. Replaced with a reflective test in the idiom internal/mcp already uses, which fails with instructions the day `builtinFile` gains a prefill key. - The scaffold header carries the same two commands in three fewer lines, since these comments persist into published content by design.
…rationales `refetchOnMount: "always"` guarantees a template's prefill arrives a round trip after the first render, and `reset` replaces form state wholesale - so the create form was editable for exactly as long as it took to discard anything typed into it. It now withholds the form the way the edit dialog already did. The test harnesses that render "past the picker" say what that means in the app: a template that was chosen and fetched. Three comments claimed constraints the code does not have, which is the same defect the previous round corrected in AGENTS.md, one level down: - Two `setTimeout(…, 0)` wrappers cited react-hooks/set-state-in-effect. The rule reports at most one violation per effect and an earlier disable had already spent it in both, so neither deferral was doing anything; verified by unwrapping them and running the linter. AGENTS.md records the trap, since the linter's silence is what misled. - The seed effect spelled the "does this template matter" condition itself while the fetch and every publish read `publishTemplateID`, which the surrounding comment claims cannot happen. - "Try again" was offered for a prefill that will not decode, where the stored bytes are the input and a retry redraws the same screen. Both effects now depend on the row they use rather than on the three query fields `freshData` reads, so adding a condition to that helper cannot leave a hand-maintained dependency array silently under-specified.
The web dialog treats "a row I do not have" as "still fetching", so a GetFrameTemplate success carrying no template would strand it on a loading skeleton with nothing to report. Deleting its defensive branch was right - the RPC cannot produce that - but nothing said so or held it there. Now the method documents it and a test asserts it across every id shape a caller can send, including the ones that error. The retry offered on a failed template load was also unpinned: removing the button entirely, or offering it for a prefill that will not decode, both left the suite green. Its presence and its absence are now asserted where each belongs.
…cised The five-row table added for it asserted nothing on three of its rows: an id that errors satisfies "if success, then non-nil" trivially, and the error codes for those three shapes are already pinned exactly by TestGetFrameTemplate. Under a mutation that empties only the success path, those rows passed regardless. Folded into that test's success cases instead, where it fails with a message rather than the nil dereference the existing field reads would have produced, and the doc comment now claims only what is checked.
8d6a6c3 to
ed03e07
Compare
|
Pushed fixes for all five. I reviewed the fixes rather than trusting them, and that turned up more than the original five. Worth knowing about, in rough order of severity:
Open question for you: the three new failure screens are |
Closes #60.
Frame templates give an author a running start: a named starter that prefills content and marks each slot required, recommended, or optional.
What a template is, and is not
A template is an authoring affordance local to this implementation. It is not a Frame and not a Frame Spec concept: it is copied once at creation and never referenced again.
docs/adr/0001-frame-templates-are-not-frames.mdrecords why, including why modelling templates as Frames cannot work here (rbac.Canopens with a cross-org deny, so built-ins in a shared org would be invisible to everyone).A template carries two things: prefill content, and a per-slot rule with a level and a note. The note is what the seeded form shows in place of the generic hint, and what an AI client would ask as a question.
Surfaces
frame_templates. Both kinds resolve into one list behind the same read RPCs, so no client can tell them apart.frames template list,frames template initto write a fill-in scaffold, andframes publish --templateto check it.make testis unaffected, wired into thee2e-sandboxjob.The enforcement decision, stated plainly
Requirements are checked only on the publish that names a template id. Nothing is recorded on the Frame. So
requiredis an authoring aid, not a control: an author can empty a required section on their next publish and nothing objects, and any caller that omits the id skips the checks entirely. Real governance would need a persistent model, which is a different design. The ADR records the alternatives considered and what each would have cost.Unblocks
This settles the modelling question #61 was waiting on. Templates are exposed to AI clients as tools only, never as MCP resources:
nebari-frame://addresses Frame content, and an authoring affordance in that namespace would look like something a client can inherit from or resolve.#82 tracks browser-level coverage of the authoring and template flows, which is the one gap this branch does not close.
Verification
All 22 Go packages and 189 web tests pass with
-race. The template lifecycle, required-slot refusal, built-in immutability, and the copy-once guarantee each have tests; the e2e suite covers the same properties against a real deployment.Two notes for reviewers:
frames publishstores the author's exact bytes, so the CLI scaffold's guidance comments persist into stored content. That is deliberate and documented; the alternative, printing guidance to a terminal that then scrolls away, loses it.