feat(ENG-823): setup and config field validation

[chandrajeet-singh]

Context

Standalone-hub half of the ENG-823 setup/config field-validation work (RFC step 7; sibling PRs: connect#1304 — canonical registry + build-time checks, unified-cloud#3212 — dashboard/widget, unified-cloud-api#8612 — server observe telemetry). The branch carries the parent-ticket name (eng-821); commits and this title use the implementation ticket ENG-823 — same pairing as every sibling PR in the chain.

What changes for users

• Client-side field validation: setup/config fields carrying Falcon validation: (pattern or format) or legacy V2 rules (html-pattern, domain) are enforced in the hub form via the Zod schema — error message on the field, Connect gated until valid. Saved-secret placeholders are exempt so reconnect is never blocked.
• Fail-open paths (deliberate): an unknown format, an uncompilable pattern, or a pattern flagged by the ReDoS lint renders the field without validation (today's behaviour) rather than broken or hanging — each path is loud (console.warn / lint) but never blocks a customer.
• Count-only friction metric: stackone-hub:field-validation-failed DOM CustomEvent (connector + field + rule kind — never the value), deduped once per field per form session. No listener ships in this repo by design (customer-embedded package, no analytics dependency).
• CI gate: npm test now runs the format conformance gate plus the vitest suite (previously the spec file never executed anywhere).

Known gaps (recorded deliberately)

• Legacy domain interpolation (pre-existing): the historical .*<pattern>.com* rule interpolates the author pattern unescaped; preserved byte-for-byte for no-V2-regression, now additionally guarded by the ReDoS lint in compileRegex. Tightening the semantics is a V2-retirement concern, not this PR.
• Vector-check blind spot: the conformance gate's vectors and pinned canonical sources are hub-local snapshots — they catch pattern↔vector drift and language-identical source drift, but an upstream format added after the snapshot is not caught automatically and cannot be: connect is a private repo, so no unauthenticated live comparison is possible from this public repo's CI. Adding a format upstream requires a manual sync of the registry copy, vectors and pinned sources together (documented in the gate's header).

Test plan

npm test — format conformance gate (accept/reject vectors, reverse coverage, pinned canonical sources, live-canonical comparison when reachable) + 19 vitest specs covering the Falcon/legacy resolvers, saved-secret carve-out, recorder lifetime across schema rebuilds, and the ReDoS guard (both exponential classes return in <1s). tsc --noEmit and biome check clean.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[Copilot]

Pull request overview

This PR adds client-side validation support for connector setup/config fields in the integration picker, including Falcon-style validation (format/pattern) while preserving legacy (v2) connector validation behavior. It also adds a small CI-style guard to keep a local copy of format regexes aligned with the canonical @stackone/core accept/reject vectors.

Changes:

• Extend ConnectorConfigField.validation to support both legacy and Falcon validation contracts (including format-based validation).
• Add a local FORMAT_PATTERNS registry and rule-resolution helpers to apply validations in the Zod schema builder.
• Add a npm test script that runs a vector-check script to prevent regex drift.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/modules/integration-picker/utils/zodSchema.ts	Introduces format/pattern validation resolution and applies the resolved regex rule to Zod field schemas.
src/modules/integration-picker/types.ts	Adds typed validation contracts for legacy vs Falcon connectors and updates ConnectorConfigField.validation.
scripts/check-format-vectors.ts	Adds a script to validate the local format regex registry against known accept/reject vectors.
package.json	Updates npm test to run the new vector-check script via tsx.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[StuBehan]

Reviewed against RFC 2026-037 (step 7). The step-7 scope itself looks right: the discriminated union matches the RFC's branch shapes, the two resolvers are properly independent, the Falcon fallback-message order matches the spec table exactly, and I verified the legacy path is behaviour-preserving — the "No V2 regression" AC holds. I also diffed FORMAT_PATTERNS, the vectors, and FormatName against connect@packages/core/src/connector/ and all three copies are byte-faithful, so the D3 copy is correct as of today.

Verified locally on a clean install of the PR head: npm run build, npm run lint, npm test, npx biome check scripts/, and tsc -p tsconfig.json all pass.

Four things I'd want resolved before merge:

1. D3's CI guard doesn't actually run. The RFC says three separate times that a drifted copy "fails that repo's CI" via the hub's npm test, and both new comment blocks repeat it. But .github/workflows/node-ci.yml runs npm ci → npm run build → npm run lint and never npm test. The check works — it's just never executed. One line in the workflow.

2. The guard also can't catch the drift it exists for. Picking up @copilot's point on the script: the hub's vector table is a local copy iterated by key, so a format added to core with no corresponding hub update produces no failure here. That matters more than it looks, because the fall-through in resolveFalconRule is silent — an unrecognised format yields no rule at all, so a field the author declared as validated renders completely unvalidated. Confirmed: format: 'hostname' accepts "literally anything". D3 notes this mechanism exists precisely because "the copies had genuinely drifted three ways… while every stale vector copy still passed CI" — this is that hole again.

3. Saved secrets will block reconnect as soon as a connector uses this. The RFC's risks table says "Pre-existing accounts not re-validated — existing accounts unaffected. Validation runs only as the user types." That doesn't hold on this surface: secret fields are pre-filled with the redacted sentinel, and RHF computes isValid eagerly from defaultValues, so validation runs before the user touches anything. Details inline — I think this is an RFC assumption that needs correcting, not just a code fix.

4. The step-9 telemetry probably shouldn't be in this PR. As implemented it can't satisfy step 9 (no connector identifier, dedupe doesn't hold, no collector), and the RFC's rollout ordering has 9 trailing 1–8. Splitting it keeps step 7 shippable now — the frontends have to be deployed before step 8 authors anything.

Nits: branch says eng-821, title and commit say ENG-823. PR body is empty — worth linking the RFC and noting this is step 7. The RFC also describes this surface as mode: 'onChange' twice while it's actually onTouched — your comment has it right, so the RFC is the stale one.

[StuBehan]

Switching my earlier review to request-changes to gate the merge — full detail is already in the threads, nothing new here.

The two I'd want fixed first are the saved-secret reconnect blocker and the per-keystroke telemetry event. Neither breaks anything on merge, since no connector authors setup/config validation: yet — but the RFC's rollout ordering has all three frontends deployed before step 8 authors any, so there's no natural forcing function to come back to them. Easier to fix now than to find it live behind a customer's embedded hub.

Happy to be talked out of the telemetry one specifically if you'd rather split step 9 out and land step 7 as-is — that resolves it too.

[chandrajeet-singh]

Thanks @StuBehan — addressed the threads:

• D3 CI guard: npm test now runs in node-ci.yml, and check-format-vectors.ts has the reverse-coverage assertion (a registry format with no vectors now fails CI) — closes the drift hole. Synced the datetime vectors too.
• Saved-secret reconnect blocker: testWithMetric short-circuits isSecretPlaceholder(val), so a pre-filled redacted secret no longer fails validation / gates Connect / emits a spurious event. Flagged the RFC "existing accounts unaffected" risk row for amendment.
• Unknown format: now console.warns before failing open, instead of silently rendering unvalidated.
• Regex try/catch: added compileRegex — an uncompilable pattern degrades to no rule instead of throwing during render and taking down the hub via the error boundary.
• && val guard: expanded the comment on why it's load-bearing (zod-4 accumulation).
• Telemetry: the event now carries the connector id and dedupes (the fields memo is stable per keystroke). Happy to split step 9 into its own PR and land step 7 as-is if you'd prefer — your call.

Added tests for the secret short-circuit, unknown-format fail-open, uncompilable-pattern degrade, and widened datetime. Ready for another pass 🙏

[StuBehan]

The isSecretPlaceholder guard is the right fix for the reconnect case, the console.warn on unknown format closes the silent fail-open, and the && val comment is now clear about why it is load-bearing. Thanks for those.

Reopening one thread: the keystroke dedupe is still broken. It was marked resolved, but the code did not change and the comment was reworded to assert the behaviour we do not have. Detail inline. Also, this is currently conflicting with main, so it needs a rebase before it can go in at step 3 of the chain.

[jerann]

Adversarial pre-review, run alongside the related connect#1304 / unified-cloud#3212 / unified-cloud-api#8612 cluster. hub isn't one of the repos the review canon covers, so this is entirely independent probing rather than canon-check matches — all 4 findings below are reproduced, not theoretical.

Needs a change

• FORMAT_PATTERNS.url reintroduces the exact ReDoS pattern connect#1304 just fixed for this exact reason — reproduced a 700ms+ hang on a 40k-character input. src/modules/integration-picker/utils/zodSchema.ts:19 (commented inline)
• compileRegex's try/catch only catches invalid regex syntax at construction time, not catastrophic backtracking at match time, despite the comment above it claiming it "guard[s] here too" — reproduced a 15s+ hang on ^(a+)+$. src/modules/integration-picker/utils/zodSchema.ts:41 (commented inline)
• uri accepts trailing-garbage values connect#1304's canonical pattern rejects, and this repo's own conformance-vector list is missing exactly the reject cases that would catch it. src/modules/integration-picker/utils/zodSchema.ts:20 (commented inline)
• The new npm test CI gate resolves tsx via npx -y rather than an actual pinned dependency — confirmed tsx only appears as an optional peer dependency of a vitest sub-dependency in the lockfile, not as an installed package, so it's fetched from the registry outside npm ci's integrity checks on every run. package.json:23 (commented inline)

Worth knowing
This PR's regex/type mirror is checked directly against connect#1304's actual diff (still open): 4 of 6 pieces match exactly, but url and uri have already drifted — see the inline comments. Since #1304 can still change before merging, worth re-diffing this file against it right before this PR merges, not just at authoring time.

— eng-review · canon c225d039 (hub not in canon — these are all independent findings) · automated pre-review, not a human review

[chandrajeet-singh]

Round-2 findings all addressed in 0750906 (inline replies on each thread):

• url/uri drift from connect#1304: synced to the canonical hardened forms (linear url, trailing-garbage-rejecting uri) and added the uri reject vectors that would have caught the drift in CI. The email sync had landed earlier; these two were missed.
• compileRegex's false ReDoS claim: fixed with a real guard, not a reworded comment — hasCatastrophicBacktrackingRisk is ported from connect-sdk as a local copy (same no-@stackone-deps model as the FORMAT_PATTERNS copy) and gates compileRegex, failing open. Tests pin that ^(a+)+$ and (a|a)+ are dropped in <1s instead of hanging the tab, and that safe legacy rules still apply.
• Unpinned tsx in CI: now a pinned devDependency resolved from the lockfile; npx -y dropped from npm test.

On the "worth knowing" note about re-diffing against connect#1304 before merge: agreed — the registry copy, the vectors, and now the lint copy all name their canonical sources, and I'll re-diff all three if #1304 changes again before this merges. All 19 tests + the vector check pass, tsc and biome clean, npm ci --dry-run verified against the regenerated lockfile.

[jerann]

Round-2 automated pre-review - hub is now covered by the canon (40 routed candidates against this diff, up from independent-probing-only in round 1). Independently re-verified all 4 round-1 findings (the ReDoS url pattern, the compileRegex guard that didn't actually guard, uri accepting trailing garbage, unpinned tsx in CI) as genuinely fixed at this head - not re-flagging any of them. 2 changes needed, 1 suggestion below.

Needs a change

• The format-vector conformance check (and the comments describing it) overclaim what it catches - it only compares hub's patterns against hub's own local vector copy, never against connect's actual canonical source, so a future upstream addition or a safety-preserving-but-unsafe regex regression would pass silently. This already happened once in this PR's own history: at commit 3224b95, uri was still the loose .+$ form with only 3 local reject vectors (no trailing-garbage cases), and CI stayed green the whole time. scripts/check-format-vectors.ts:11 (commented inline) (test-045a, test-045b, test-045c, proc-073b)

• The PR description is empty despite the diff changing user-visible behaviour (new client-side validation, a new fail-open path, a new CustomEvent, a new CI gate), so two known gaps - the pre-existing domain-rule interpolation and the vector-check blind spot above - are recorded nowhere a reviewer would see them. PR description (proc-038a, proc-013d)

Suggestions

• Branch is feat(eng-821)/setup-and-config-field-validation; PR title and 7 of 9 commit subjects say eng-823. Pick one so it doesn't misfile in a ticket search. PR title / branch name (proc-079a)

— eng-review · canon 4b2291af · automated pre-review, not a human review

[chandrajeet-singh]

Addressed the comments and run the eng-review locally -

[StuBehan]

The recorder now genuinely outlives schema rebuilds, and the two regression tests pin the lifetime rather than the comment. isSecretPlaceholder is the right guard and is tested against the real sentinel shape. npm test running in CI closes the last of my July list.

One thing left, on the format gate: layer 3 cannot fire and its message misattributes why. Inline. Not a merge blocker on its own, but I would not want it merged as-is and then trusted.

[chandrajeet-singh]

eng-review (automated pre-review) ran against this PR at 732736e. Two canon items surfaced; both were reviewed and dispositioned as intentional, so there is nothing to change.

Reviewed, accepted as intentional

• The format-conformance gate (scripts/check-format-vectors.ts) is one-directional: a format ADDED to connect's canonical registry cannot fail hub CI, because both sides of every layer are hub-local snapshots and connect is private (no authenticated live compare from public CI). Accepted for now, mitigated by the documented manual-sync contract in the gate header (bump registry copy + vectors + pinned sources together). (test-045a, test-045b, test-045c)
• Ticket-ID split: branch feat(eng-821)/... vs title/commits feat(ENG-823). Intentional and already documented in the PR body (branch = parent ticket, commits/title = implementation ticket). (proc-079a)

No blocking findings. The validation core (fail-open regex compile, saved-secret carve-out, ReDoS heuristic) verified clean; biome check clean on all touched files; the new gate is wired into npm test -> node-ci.yml.

-- eng-review · summary · canon b8dcf50c · automated pre-review, not a human review

[jerann]

Automated pre-review re-run against 732736e. Every round-1/round-2 finding and every human review thread was independently re-verified in code at this head, not trusted from commit messages or replies. 6 changes needed (2 are new bugs beyond the canon checks), 1 suggestion.

Worth knowing before merging: StuBehan's Aug 7 review is still CHANGES_REQUESTED — the Aug 14 follow-up was a plain comment, not an approval, so this PR doesn't have a clearing review yet.

Needs a change

• 2 of 3 fail-open validation paths are silent, contradicting the PR description's "each path is loud" claim — also worth updating the description to match. src/modules/integration-picker/utils/zodSchema.ts:55 (commented inline) (bug-d862f45f, proc-038b)

• Saved-secret sentinel still fails validation on type: 'number' fields — same bug class as the July reconnect-blocker thread, missed on this branch. src/modules/integration-picker/utils/zodSchema.ts:208 (commented inline) (arch-179a, arch-179b)
• The new ReDoS lint doesn't catch the quadratic shape already live in 2 legacy V2 patterns (5 connectors, 10 fields) today. src/modules/integration-picker/utils/regexSafety.ts:15 (commented inline) (bug-3d935788)
• scripts/check-format-vectors.ts — the drift guard itself — ships outside both the lint and tsc globs. package.json:23 (commented inline) (proc-039a)
• RFC step-9 telemetry is still in this step-7-scoped PR. This was already raised and the split was offered on Jul 27 but not done — worth resolving one way or the other before merge. src/modules/integration-picker/utils/zodSchema.ts (proc-013a)

Suggestions

• types.ts:68's comment claims a build-time backstop that isn't on connect's main yet (it's in unmerged connect#1304), and its "nil in practice" claim is true but uncounted. src/modules/integration-picker/types.ts:68 (commented inline) (conv-119b, mind-024b)
• Branch is feat(eng-821)/..., title and all 9 commits say ENG-823 — already explained in the PR body, just flagging so it doesn't misfile in a ticket search. PR title / branch name (proc-079a)

— eng-review · summary · canon e3900a98 · automated pre-review, not a human review

[jerann]

Bug: 2 of the 3 fail-open paths here are silent, contradicting this PR's own description ("each path is loud"). Only the unknown-format branch (line 104) warns; a ReDoS-flagged pattern (line 57) and an uncompilable pattern (line 62) both drop the connector-declared rule with zero log output.

Proof: git grep console. -- zodSchema.ts regexSafety.ts at this head -> 1 hit (line 104). Expected 3 loud paths, actual 1.

Precedent: same "field renders with no validation and nobody notices" failure raised on this thread: #183 (comment) — closed for the format case only.

— eng-review · bug-d862f45f · canon e3900a98 · automated pre-review, not a human review

[chandrajeet-singh]

Fixed in a2d3606. compileRegex now warns on both fail-open paths — the ReDoS-flagged pattern and the uncompilable pattern — matching the unknown-format branch. git grep console. -- zodSchema.ts now returns 3 loud paths. + test asserting both warn.

[jerann]

Bug: This lint only catches exponential backtracking (nested quantifiers / alternation), not the quadratic "adjacent unbounded quantifier" shape this same PR just rewrote email/url to avoid, because "this pattern runs on every keystroke."

Proof: probed 2 legacy V2 patterns already live in 5 connectors (10 fields, e.g. sapsuccessfactors) — both lint-clean and both quadratic: 920ms / 2006ms on a 40k-char non-match, where the sibling rewrite in this file runs in <1ms on the same input.

Precedent: this file's own comment claims it "re-guards ... the legacy TS path that never had it" — it doesn't, for this shape.

— eng-review · bug-3d935788 · canon e3900a98 · automated pre-review, not a human review

[chandrajeet-singh]

Fixed in a2d3606. The star-height lint (canonical connect-sdk copy) catches exponential only — I kept it byte-faithful rather than diverge from the source of truth. The quadratic "adjacent unbounded quantifier" shape is now handled the way the canonical docstring recommends: bounding the input. Author patterns run only up to MAX_PATTERN_INPUT_LENGTH (512); over that they fail-open. FORMAT_PATTERNS are canonical + linear and never capped. The comment here now states the exponential-only scope and points at the cap. + test.

[jerann]

Change: This secret-placeholder short-circuit never runs for type: 'number' fields — their required/optional checks (lines 189-197, unchanged by this PR) return before reaching here.

Proof: the saved-secret sentinel fails /^\d+$/.test(...), so a number field with a saved secret (e.g. a usage_limit-style field with no secret key) gates Connect on reconnect — the exact failure mode already fixed here for string fields.

Precedent: #183 (comment) — same bug, one branch missed.

— eng-review · arch-179a,arch-179b · canon e3900a98 · automated pre-review, not a human review

[chandrajeet-singh]

Fixed in a2d3606. The number branch now admits the redacted sentinel (isSecretPlaceholder(val) || /^\d+$/.test(val)) before returning, so a saved secret no longer gates Connect on reconnect. Restructured it to return early — number carries no validation: per the connector schema (AUTHENTICATION_FIELD_SCHEMA exposes it only on text/password/text_area), so it never reaches the rule section. + test.

[jerann]

Change: This script isn't covered by CI's lint or type-check gates — biome lint/check run on ./src ./dev only, and tsconfig.json's include is ["src"]. It runs via tsx, which strips types without checking them.

Proof: scripts/check-format-vectors.ts (182 lines — the drift guard itself) is outside both globs, so it ships with no lint and no type-check.

Suggestion: add scripts to the lint glob and to a tsc project (or its own tsconfig).

— eng-review · proc-039a · canon e3900a98 · automated pre-review, not a human review

[chandrajeet-singh]

Fixed in a2d3606. ./scripts is now in both biome globs (lint/check) and scripts is in tsconfig.include; the lint script also runs tsc --noEmit, so check-format-vectors.ts (and verify-build.mjs) are now both linted and type-checked in CI. npm run lint is green.

[jerann]

Change: This comment (and the matching one at zodSchema.ts:148 and the test docstring) states the schema "rebuilds per keystroke" — it doesn't. useIntegrationPicker.ts's fields memo deps are [connectorData, selectedIntegration, accountData, hubData]; formData isn't among them, so the memo is stable across keystrokes.

Proof: the recorder fix itself is correct and verified (it does outlive schema rebuilds) — only the stated reason for needing it is wrong.

Precedent: second time this thread's justification didn't match the code: #183 (comment)

— eng-review · proc-074b · canon e3900a98 · automated pre-review, not a human review

[chandrajeet-singh]

Fixed in a2d3606. Corrected the comment (and the matching one at zodSchema.ts and the test docstring): the schema rebuilds when the connector or account data changes (the fields memo deps [connectorData, selectedIntegration, accountData, hubData]), not per keystroke. The recorder-lifetime fix is unchanged — owning it in the component keeps the dedupe across those rebuilds.

[jerann]

Suggestion: Two small accuracy gaps in this comment. "Exposure is nil in practice" is true (0 of 1695 setup/config fields across all 503 connectors use validation on a select field) but doesn't say so — worth the count. And the claimed backstop ("connect-sdk rejects ... at build time") doesn't exist on connect's main yet — AuthenticationField has no validation member at all there; that rejection ships in unmerged connect#1304.

Precedent: #183 (comment)

— eng-review · conv-119b,mind-024b · canon e3900a98 · automated pre-review, not a human review

[chandrajeet-singh]

Fixed in a2d3606. Comment now cites the count (0 of 1695 setup/config fields across 503 connectors declare validation on a select) and corrects the backstop: the build-time rejection of validation-on-select ships in connect#1304 (unmerged), not connect main yet.

[chandrajeet-singh]

Deep review against the connect repo (source of truth) — a2d3606

Reviewed the hub validation against connect's canonical implementation (AUTHENTICATION_FIELD_SCHEMA, FORMAT_PATTERNS, regexSafety) and fixed the open findings (replies inline). CI is green: npm run build, verify:build, npm run lint (biome + tsc --noEmit), npm test (23 tests + vector check).

Kept consistent with connect (not diverged)

• FORMAT_PATTERNS / FormatName / vectors stay byte-faithful to @stackone/core — the local copy is unchanged.
• regexSafety.ts stays a faithful copy of connect-sdk's star-height lint. It catches exponential backtracking only (by design); the quadratic shape is handled the way the canonical docstring recommends — bounding input length (MAX_PATTERN_INPUT_LENGTH) — rather than forking the lint.
• Field types: validation is applied only where the schema exposes it (text/password/text_area); number returns before the rule section, matching the schema having no number type.

@StuBehan — your four blockers

1. CI guard now runs — npm test is in node-ci.yml.
2. Guard catches drift — the reverse-coverage check (fail if a registry key has no vectors) is in check-format-vectors.ts; the script is now also lint + type-checked in CI.
3. Saved-secret reconnect — fixed for string fields earlier and now for number fields too (the branch admits the sentinel before the numeric check).
4. Step-9 telemetry — as implemented it carries the connector key, dedupes per field for the session, and emits a stackone-hub:field-validation-failed CustomEvent for hosts to collect. Happy to split it out if you'd still prefer to land step 7 alone — your call.

🤖 Generated with Claude Code