fix: honest status reporting, routines crash, and setup-field safety - #599
Merged
Conversation
Addresses the first customer test report (41 findings, 3 blockers) against v0.58.0. Three construction errors accounted for most of it: status is reported rather than checked, errors are shown rather than explained, and actions are offered whose precondition the UI cannot establish. LLM provider status (OM-02/03/04/08/18/33/34/35) - New providerCredentialVerifier: 4-state no_key|unverified|verified|invalid, wire-format-keyed probe, cached, verdicts bound to a key fingerprint so a restart cannot resurrect a verdict for a replaced key. - 2xx alone is not enough: requires a JSON content-type and a bounded model-list body, so a proxy block page cannot read as "verified". - Only 401 means a bad key; 403 (region/permission) reports unverified. - redirect: 'error' so x-api-key is never forwarded to a redirect target. - GET /admin/providers stays network- and write-free; probing happens on an explicit POST /:id/verify or a key save. - /setup now records that its ping happened instead of discarding it; the desktop wizard finally reads the keyVerified result it has always written. - Deterministic provider ordering (OM-10b). Routines page crash (OM-14/19/32) - Root cause: t.rich passed function handlers against messages declaring ICU arguments rather than tags. In a Server Component the raw function reaches the Flight serializer, which emits an error row carrying a digest inside a 200 response. Present at 17 call sites; routines was the only server component among them, hence the only hard crash. - Payload guards, digest shown on the error page, chat-specific recovery no longer offered on unrelated routes, and a CI guard for the whole class. Plugin status (OM-16/24/36) - Plugins reported ACTIVE with every credential removed. Readiness is now derived from declared required fields versus stored values, plus the existing circuit-breaker status that never reached the UI. Setup-field safety (OM-17) - A masked field under an email field, with no validation, invited a real account password. Adds a standing caution at every secret input, plus manifest-declared format validation enforced server-side in every write path, anchored to match HTML pattern semantics. - Patterns come from untrusted manifests: an allowlist grammar rejects catastrophic shapes at load, and every match runs in a worker under a 50 ms budget, so no pattern can stall the event loop. Also: post-install next steps, skill scan verdict surfaced at import, raw provider errors and request IDs replaced by classified codes, Enter sends in chat (with IME handling), nav dropdown hover/click decoupled, dead tab link, CLI install instructions no longer hidden when the CLI is missing, /help page, onboarding rebuilt as visible steps with LLM access as step 1.
This was referenced Aug 3, 2026
Closed
Contributor
Author
|
Follow-ups filed for everything this PR deliberately leaves open, so none of it gets rediscovered later:
|
This was referenced Aug 3, 2026
Weegy
added a commit
that referenced
this pull request
Aug 3, 2026
Two defects in the setup-field validation shipped in #599, both found by writing the first real plugin manifest against it. Neither was visible from inside this repo. 1. The pattern allowlist refused `{n,}` while allowing `+`, which is the same thing. The first realistic pattern anyone wrote against the feature — `[A-Za-z]{2,}` for an email TLD — was rejected for no safety reason and had to be respelled. Shape is now judged purely by the group-content rules that already govern `+` and `*`, so every spelling of a quantifier is treated alike. Each hostile pattern in the existing table is now also asserted in its `{n,}` spelling, so the counted form cannot open a door the `+` form keeps shut. The size cap had to be extended to `{n,}`'s minimum, which nothing needed to bound while the construct was illegal — otherwise `a{100000,}` would have become legal the moment the shape check stopped rejecting it. The client carried a hand-mirrored copy of the same rule, and there the bug was worse: a rejected pattern means no native `pattern=` attribute and a check that fails open, so the operator would have typed a bad value, seen nothing, and hit a 400 from a validation the client had silently opted out of. Both halves are fixed together. 2. Every `400 runtime.setup_field_invalid` returned the English hint whatever the UI language, because the locale parameter was never passed. English help text in a German UI is one of the named contributing factors of the finding this feature exists to prevent. The client now resolves the hint from the field it already holds, keyed on the violation's field, and falls back to the server string when the field is unknown. No API change: `hint` stays the English fallback for clients that have no manifest. The unused locale parameter is gone rather than left implying a threading that does not exist. Middleware has no locale plumbing at all — no Accept-Language read anywhere, and the locale cookie never leaves the Next.js layer. Threading one would mean the server picking a language for a client it cannot see, which is the same untranslatable-string-through-the-API mistake in a different costume.
Weegy
added a commit
that referenced
this pull request
Aug 12, 2026
…ncurrency (#664) Three independent hygiene items left over from PR #599. Items 4 and 5 of the issue are product/sign-off decisions, not code, and are untouched. 1. `@formatjs/icu-messageformat-parser` is imported by the t.rich CI guard in web-ui/app/_lib/i18n-parity.test.ts but resolved only transitively through next-intl -> intl-messageformat. Declared as a devDependency at ^3.5.16, the version already in the tree, so the lockfile gains only the declaration. 2. web-ui/CLAUDE.md claimed `npm run i18n:check` fails on mismatched ICU/tag placeholders. It does not: `i18n:check` runs only scripts/i18n-validate.mjs (key sets, empty/non-string values, forbidden HTML, de===en warnings). The placeholder and rich-text-tag parity check is real but lives in the vitest guard, which CI runs separately. The checklist now names both commands and what each one actually catches — the routines crash that motivated #599 was exactly a placeholder mismatch that passed `i18n:check` with all keys present. 3. `test` pins --test-concurrency=4. Node derives the default from the core count, so a 16-core dev machine ran 16 files at once while a 4-vCPU CI runner runs ~4; the suite starts many short-lived Express servers on localhost and the issue records three consecutive runs each failing a DIFFERENT test, two with connection-level `fetch failed`, all passing in isolation. The pin makes the number the same everywhere. Rationale is in the CI comment next to the step, since package.json cannot carry one. Measured on a 16-core machine: unpinned 36.5s, pinned 48.0s (+31%), both green at 6174 tests. On a 4-vCPU runner the pin is close to a no-op, so the cost lands on large dev machines only. Note this run did NOT reproduce the flake — the pin is prevention based on the issue's evidence, not a fix proven here.
Weegy
added a commit
that referenced
this pull request
Aug 12, 2026
…ay why a key is unverified (#672) The two decisions left over from #605. Neither was a code bug on its own — both were surfaces that withheld what the operator needed to act. ## OM-06 — store "install" vs. admin "connected" The issue asked whether a provider is ONE concept or TWO. The code had already answered: a catalogue entry (registry + models) and a credential (vault, own verification lifecycle) are genuinely different things, and a provider can be known-but-unconfigured or configured-but-blocked. Merging them would erase real states. So the concepts stay; the surface is fixed. `install_available` was `install_state === 'available'`, which only says "not already installed". It never asked whether the CAPABILITY was taken — while `InstallService.create` refuses exactly that with 409 `install.capability_already_provided`. The store therefore advertised a button the server was guaranteed to reject, which is what the report saw: "Jetzt installieren" for a provider the admin area already listed as connected, with nothing linking the two. Same check, same helper, one turn earlier. `findActiveProviderCollision` starts from a catalog lookup, so it is blind to a hub-only entry — which is precisely the case that failed. Factored out `findProvidesCollision`, which takes the `provides` list the registry summary already carries; `findActiveProviderCollision` is now a thin wrapper so the two cannot drift. `blocked_by_active_provider` is structured rather than folded into `blocking_reasons` (server-authored English the client can only print), because the operator's next step is to CONFIGURE the provider they already have — and no client can build that link by parsing prose. The store detail page now renders that link. ## /setup 403 — ratified, and made legible Reviewed rather than rubber-stamped. #599 did NOT weaken the gate: a bare 403 becomes `unverified` with `reason: 'forbidden'`, never `verified`. Only an explicit `authentication_error` marker still earns `invalid`. That is a correction of a false accusation, not a relaxation — a region block is no evidence a key is bad, and the old behaviour locked out region-restricted operators with valid keys. Keeping it. What it left behind was a bare `UNVERIFIED` chip covering both "your key is fine, your region is blocked" and "the provider was down". The verdict already carried `reason`, and `ProviderVerificationReason`'s own comment says it exists so "a future UI can map it to a localized string without a second server change". This is that UI: `verifyReason` on the DTO, a closed code->key map in the panel, en+de copy for all six reasons. Unknown codes render nothing rather than leaking a raw code at the operator. ## Mutation checks Every behaviour is proven to fail, not assumed to: - collision helper: always-null, stop-ignoring-inactive, stop-ignoring-self - store wiring: ignore the collision, drop the structured field - UI: drop the line, render for any status, leak an unknown code One first attempt reported green because the perl edit never landed; each mutation now asserts its own anchor before running. ## Test-stub bug this exposed `fakeRegistry`/`fakeInstalled` in registryInstallMerge.test.ts omit `list()`, which `InstalledRegistry` requires — hidden by `as unknown as`. The new check calls it, so four remote-plugin tests 500'd. Stubs corrected rather than the production path made defensive: a stub that does not implement its interface is the defect.
Weegy
added a commit
that referenced
this pull request
Aug 13, 2026
…e it (#682) OM-17, structural fix. The reported near-miss: a tester typed their real Google account password into `gw_sa_private_key`. Nothing careless about it — the form asked them to hand-transcribe two values out of a service-account key file into an email field and a masked field stacked directly beneath it, which is the visual pattern of a login. #599 made that mistake DETECTABLE (caution copy + `pattern` validation). Uploading the file removes the opportunity to make it. The tester proposed exactly this, and they were right: it is structural, and it removes a step rather than adding one. ## The type is mirrored in SIX places, not three The issue named three (`admin-v1.ts`'s union, `manifestLoader.isSetupFieldType`, `agentSpec`'s `.strict()` z.enum). Two more only surface when you follow the value: `SUPPORTED_TYPES` in `installService.ts` — where a missing member does NOT error, `isSupportedType` just skips the field and the upload vanishes from the install wizard with no diagnostic — and `InstallSetupField`'s own shape, which is what `extractSetupSchema` returns and therefore what the new route reads. A sixth lives in web-ui (`storeTypes.ts`). All six carry `json_file` now, each with a comment naming the others. ## Server-side extraction `src/plugins/setupJsonFile.ts` — the client posts the file's TEXT, the server parses it, validates it, explodes it into the keys named in `extracts`, and stores only those. The browser is never trusted to decide which bytes become `gw_sa_private_key`. Guard rails, each pinned by a test: - the size cap is applied to the RAW text, before `JSON.parse` runs; - `expect` rejects the wrong file BEFORE any value is extracted (an OAuth client secret and a service-account key look alike at a glance); - a missing/empty/non-string value is a readable error, never a silently empty secret — that outcome would look like a successful setup and fail later, far from the cause; - `Object.hasOwn` on every path segment, so `$.constructor` cannot reach a prototype property; - the raw document is never returned, logged or persisted. Paths are a deliberate subset (`$.a.b`), not JSONPath: service-account files are flat, and the full grammar would add a dependency plus an evaluation surface for no case anyone has. An unsupported path fails loudly instead of matching nothing. ## The write path is SHARED, not copied `POST /installed/:id/secrets/from-json` extracts, then hands the derived values to the same `applySetupValues` the typed `PATCH …/secrets` now uses. The security claim of #603 is that an extracted value is treated exactly like a typed one — same `pattern` validation, same vault/config split, same reactivation, same response (key NAMES only, never a value). Two implementations of that would be two implementations that can drift, and the drift would be invisible until a `json_file` field quietly skipped a check the typed path applies. The extraction map comes from the MANIFEST, never the request: a caller-supplied `extracts` would let anyone write any vault key from any file. `coerce()` refuses a value submitted under a `json_file` key rather than ignoring it — silently dropping it would let a client believe it had stored a credential. The install form skips the field for the same reason, so the two sides agree. ## UI The install wizard renders a file picker. Falling through to the text input would be worse than not shipping the type: it would ask the operator to paste a raw key into a field, which is the transcription step this removes. The post-install `CredentialsEditor` omits `json_file` fields — every row there is a text input, and the DERIVED fields are ordinary secrets listed as usual, so post-install editing still works. Offering the upload there too is a follow-up. ## The errorHelp guard earned its keep The new codes were first built with a template (`runtime.json_file_${failure.code}`), which the coverage guard cannot see — the codes would have shipped with no operator copy while the suite stayed green. Now a `JSON_FILE_ERROR_CODES` table holds literals, exhaustive over `JsonFileFailureCode` so a new failure kind cannot ship without a wire code, and the forwarder is registered in the guard. 11 new help entries in both locales; the two spec-level ones say plainly that the fault is in the plugin, not the operator's file — that distinction is the difference between "try another file" and "report this". Verification: `test/setupJsonFile.test.ts` 12/12, related middleware suites 45/45, web-ui 693/693, `tsc --noEmit` clean on both sides, `typecheck:test` ratchet 406 = baseline, eslint 0 errors. `TemplateInstantiateForm.test.tsx` failed once in a full-suite run and passes both in isolation (22/22) and on a full-suite re-run (693/693) — the repo's known cross-file pollution, not this change. Not included: the Google Workspace manifest itself lives in a separate repo, and post-install upload in the credentials editor. Closes #603
Weegy
added a commit
that referenced
this pull request
Aug 13, 2026
Resolves three conflicts after 90 commits on main: - publish-images.yml: union. Keeps this branch's `build_args: ''` for web-ui (MIDDLEWARE_URL is runtime-only now) and main's dev-runner / dev-runner-daemon matrix entries (epic #470 W1). - messages/{en,de}.json: takes main's side for all 11 t.rich keys per locale. Both branches fixed the same ICU-vs-tag bug; main's fix (#599) shipped a month ago and the components on main now render `{chunks}` (routines/page.tsx, login/page.tsx, RoutineTemplateEditor.tsx, ...), so the content must live in the message. This branch's empty-tag form (`<toolName></toolName>`) would render an empty <code> element — a regression against main. Verified by reading every chunk handler; the test suite does NOT cover this class (see PR notes). The RuntimeReadinessBanner keys this branch adds survive the merge unchanged in both locales.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Our first customer test round (TE Printline, 1 Aug 2026, v0.58.0 on macOS, German UI) produced 41 findings — 3 blockers, 11 high. The tester spent 76 minutes and ended with no working AI access, a crashed section, and this note:
That sentence is the actual bug. This PR fixes the three construction errors behind most of the report:
Corrections to the report's own root-cause analysis
The report was unusually rigorous and flagged its own hypotheses as hypotheses. Three of them turned out to be wrong, and it matters:
.filteratroutines/page.tsx:28-29t.richpassed a function handler against a message declaring an ICU argument ({toolName}) instead of a tag. The raw function reaches the RSC Flight serializer, which emits an error row carrying a digest inside a 200 response — explaining the opaque digest, the "all requests are 200" observation, and why reload never helped. Verified by running the Flight serializer directly under--conditions react-server. Present at 17 call sites in 8 files;routines/page.tsxis the only Server Component among them, which is exactly why it is the only page that hard-crashes. The:28-29defect is real and is hardened too, but no path reaches it.<select>that work without React" are ReactonChangehandlers writing a cookie +router.refresh(). If the language and theme switchers genuinely work, React is alive. A real hover/click race is fixed here, but this does not fully explain the reported symptom — see Open below.Button busyexists; it is just sub-100 ms on a local probe. The real defect:generatedAtwas already on the wire with zero render sites.Also: the report says one bootstrap site seeds
ANTHROPIC_API_KEYfrom the environment. There are three.Changes
Blocker · LLM provider status — OM-02/03/04/08/18/33/34/35, OM-10b, OM-10
isConnected()was a vault lookup: any non-empty string meant "connected". Meanwhile a correct, complete key probe sat indesktop/src/ipc.tswritingstate.keyVerified— a value read nowhere in the entire repo.middleware/src/platform/providerCredentialVerifier.ts:no_key | unverified | verified | invalid, one wire-format-keyed probe covering anthropic/openai/mistral/minimax and every openai-compatible provider, cached (TTL 300 s), shaped after the existingcliBackendDetector.200 text/htmlblock page cannot render as green.unverified— telling a customer in a blocked region that their working key is invalid would be a new version of the same problem.redirect: 'error':fetchstripsAuthorizationacross origins but not custom headers, so a followed redirect would have forwarded the rawx-api-key./admin/providersstays network-free and write-free; probing happens on an explicitPOST /:id/verifyor a key save.connectedis retained as a derived field for wire back-compat./setupnow records that its ping happened instead of throwing the result away.reactivate()re-registered models at the end of an append-ordered array (OM-10b).Seeding from
process.envis deliberately left in place — removing it would break existing installs. The key still lands in the vault; it now reads as ungeprüft rather than verbunden, which is the actual fix.Blocker · Routines unreachable — OM-14/19/32, OM-41
All 17
t.richsites converted to tag syntax; payload guards so a malformed body surfaces the page's own error card instead of crashing the render; thedigestis now displayed so support and customer share a reference; chat-specific recovery ("reset local chat data") no longer offered on unrelated routes; kg-lifecycle 404s explained as "needs the Postgres backend" instead of a silent empty page. A CI guard now fails the build if at.richhandler has no matching<tag>— in every locale, not justen.Blocker-adjacent · Plugin status — OM-16/24/36
Google Workspace reported "Installiert · AKTIV" after every credential was deleted.
install_statecame from registry presence alone, while the circuit-breaker's'errored'status never reached the UI — the source comment said so outright. Readiness is now derived from declared required fields versus stored values, plus that runtime status.PluginInstallStateis deliberately not widened (20+ call sites branch on=== 'installed');readinessis additive and optional.High · A masked field invited a real password — OM-17
The tester entered their work email and their actual Google account password into
gw_sa_client_email/gw_sa_private_key, and the system accepted both with "gespeichert". They wrote: "Wenn ich an dieser Stelle ein Login sehe, dann ist das kein Anwenderfehler." They are right.PATCH …/secretspreviously validated only the shape, never the value), anchored to match HTMLpattern=semantics so"my password is 1234"cannot satisfy[0-9]{4}.Remaining findings
Post-install next steps modelled on the skill-import flow the report praised (OM-06/07); consistent plugin counts and a genuine mis-bucketing bug fixed (OM-27); scan verdict surfaced at import (OM-25); classified error codes instead of raw provider JSON and request IDs, with legacy rows redacted on read (OM-26);
generatedAtrendered (OM-22); CLI install instructions no longer hidden precisely when the CLI is missing (OM-11); symmetric tab callback (OM-05/38); Enter sends in chat with IME handling —isComposinghad zero occurrences across all five composers (OM-21/37); nav hover/click decoupled (OM-20/40);/helppage (OM-09); onboarding rebuilt as visible steps with LLM access as step 1, gated on a verified key (OM-01/12); numerals, initials, truncation, theme labels (OM-39/31/30/23).Verification
An adversarial cross-vendor review found 12 defects in the first implementation, including a bypassable ReDoS screen (
^(a|a)+$passed the filter and blocked the event loop for 1739 ms at 26 characters), validation that failed open, a proxy block page reading as "verified", and 403 mapped to "invalid". All are fixed in this branch.Every guard was mutation-tested — reverted, observed red, restored. The clearest evidence: with the execution bound removed, a single test ran for 65.5 seconds, which is the unbounded backtracking blowup measured directly.
Open / deliberately out of scope
aria-expandedafter a syntheticmouseenter, headerscrollWidthvsclientWidthat the 1100 px window, and whetherAuthBadgeever leaves its loading skeleton.pattern+pattern_hint. That manifest lives in neither this repo noromadia-byte5-plugins— it ships via hub.omadia.ai. The platform half lands here first and benefits every plugin; the standing caution protects users today regardless.de === en, 80 jargon strings, 58 hardcoded literals, 9 window titles). Separate issues. The highest-leverage piece is already in:i18n:checknow warns onde === en, making the debt CI-visible.json_fileupload field, localized manifestlabel/help, and the agent-based help bot (which needs a byte5-operated LLM path — it has to work when the customer's key is broken).Review notes
--test-concurrencyon a loaded machine: three consecutive runs each failed a different test, two with connection-levelfetch failed. All pass in isolation and at--test-concurrency=4. Consistent with the pre-existing flakiness, but not proven against a stashed baseline.desktop/src/ipc.tsreceived the same probe semantics but cannot be typechecked in this worktree (desktop/node_modulesabsent); it is syntax-verified and mirrored by tested middleware code.@formatjs/icu-messageformat-parser(used by the new i18n guard) resolves transitively via next-intl and should be added to web-uidevDependenciesin a follow-up —package.jsonwas left untouched to avoid a merge hazard./setupbehaviour change worth conscious sign-off: a bare 403 now passes with a warning instead of hard-blocking. A region-restricted operator with a valid key was previously locked out of setup entirely.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.