Repository navigation
fix(dashboard): validate manifest mode fields - #6914
Conversation
Prefix the fragment filename with the PR number so fragments sort usefully, unwrap the hard-wrapped body to one sentence per line, and add the (#6914) reference so the release flow doesn't also emit a generated line for this PR.
houko
left a comment
There was a problem hiding this comment.
Reviewed the dashboard manifest-validation changes: the new client-side checks (cron-required for periodic schedules, JSON-Schema shape/number-safety for json_schema response format) are well-tested and correctly routed through the existing validateManifestForm → spawnMutation flow — no inline fetch added. Accessibility wiring (aria-invalid/aria-describedby/aria-required, auto-opening invalid sections) checks out against the new tests. Left one comment on whether the equivalent constraints should also be enforced server-side, since manifests can reach the daemon without going through this validator.
Generated by Claude Code
| if (!form.model.model.trim()) errors.push("model.model"); | ||
| if (form.schedule.mode === "periodic" && !form.schedule.cron.trim()) { | ||
| errors.push("schedule.cron"); | ||
| } | ||
| if (form.response_format.mode === "json_schema") { | ||
| const schema = form.response_format.schema.trim(); | ||
| if (!schema || parseSupportedJsonSchema(schema) === undefined) { | ||
| errors.push("response_format.schema"); |
There was a problem hiding this comment.
Both new checks here (empty cron for a periodic schedule, and the JSON-Schema shape/number-safety checks in parseSupportedJsonSchema) are enforced only in this validator, which the create-agent form calls before spawnMutation.mutate({ manifest_toml }) in AgentsPage.tsx. That's the right place for UX, but a manifest can also reach the daemon by other paths — PUT/raw manifest_toml via PATCH /api/agents/{id} (routes/agents/lifecycle.rs), or an operator hand-editing agent.toml — none of which go through this TS validator.
On the Rust side today:
ScheduleMode::Periodic { cron: String }(librefang-types/src/agent.rs) has no non-empty check. An empty cron reachingparse_cron_to_secs(librefang-kernel/src/background.rs:668) falls through to the "unparseable" branch and silently defaults to a 300s interval with awarn!log — it doesn't reject.ResponseFormat::JsonSchema { schema: serde_json::Value, .. }(librefang-types/src/config/types.rs) accepts any JSON value, includingnullor non-object/boolean schemas, with no server-side shape check.
Neither is a crash or security hole, but per the repo's "client-side-only validation is not a real security/correctness boundary" guidance, is a server-side guard (even just rejecting on manifest parse/apply) intentionally out of scope for this PR, or worth a follow-up issue? Given this PR is scoped to the dashboard, I'm flagging rather than fixing — the fix would live in librefang-types/librefang-kernel, a different crate than the rest of this change.
Generated by Claude Code
There was a problem hiding this comment.
Non-blocking nit, not covered by the earlier review on this PR: commit acf33fcdc78ea88eb7cbfa4704a073e5a39cbbdd ("docs: fix changelog fragment format for #6914") has a hard-wrapped message body ("...fragments sort\nusefully, unwrap the hard-wrapped body..."), breaking mid-sentence rather than at sentence boundaries. CLAUDE.md's prose-wrapping rule covers commit message bodies too (only the subject line gets the git-display-truncation exception). Not asking for a history rewrite here — flagging only, since the fix would need an amend + force-push on an already-open PR.
Everything else looks solid: the changelog fragment itself is correctly named/sectioned/formatted, the validation logic (parseSupportedJsonSchema, hasUnsupportedJsonNumber) correctly detects precision-losing numbers by scanning the raw JSON text rather than the already-rounded parsed value, and the TagInput fix now clears the input on a duplicate submission instead of only on a successful add.
Generated by Claude Code
Both blocks hard-wrapped mid-sentence at ~76 columns, which the project's prose-wrapping convention forbids for new prose.
Changes
Tests
pnpm test --run(94 files, 990 tests passed)pnpm lintpnpm typecheckpnpm buildgit diff --checkReview
Independent review found no remaining Critical, Important, or Minor issues and marked both implementation commits Ready to merge.