Skip to content

fix(DATAGO-121653): reject prompt imports with invalid command characters#1542

Open
solace-jkhalife wants to merge 6 commits into
mainfrom
jkhalife/DATAGO-121653/duplicate_prompt_imports
Open

fix(DATAGO-121653): reject prompt imports with invalid command characters#1542
solace-jkhalife wants to merge 6 commits into
mainfrom
jkhalife/DATAGO-121653/duplicate_prompt_imports

Conversation

@solace-jkhalife
Copy link
Copy Markdown
Contributor

What is the purpose of this change?

When importing a prompt with a command field containing characters outside [a-zA-Z0-9_-] (e.g. spaces, dots, @), the import would silently succeed but the resulting row would be dropped from all list responses — making the prompt invisible after import. The fix validates the command pattern at the API layer and in the import dialog UI before the request is submitted.

How was this change implemented?

  • Backend (routers/prompts.py): Added a regex check against ^[a-zA-Z0-9_-]+$ on the command field in the import_prompt endpoint. Returns HTTP 400 with the offending value in the error message if validation fails. Only applied when preserve_command=true; ignored otherwise.
  • Frontend (PromptImportDialog.tsx): Added COMMAND_PATTERN regex to the Zod form schema for the command field. Added initialCommandInvalid state so that files with an invalid command immediately open the field in editable mode (same UX as conflict handling). Added isValid to the isEnabled gate on the Import button. Fixed setValue calls to pass shouldValidate: true so react-hook-form recalculates isValid after programmatic population.
  • Tests: Backend integration tests covering rejection of 6 invalid command patterns and pass-through when preserve_command=false. Frontend unit tests covering editable input appearance, button disabled state, button re-enable after correction, and read-only display for valid commands.

Key Design Decisions

The validation pattern (^[a-zA-Z0-9_-]+$) mirrors the existing PromptGroupResponse filter — the same characters that cause silent drops from list queries.

How was this change tested?

  • Manual testing: import a prompt JSON with a command containing a space or dot — should see 400 / inline error; import with a valid command — should succeed
  • Unit tests: 5 new frontend tests in PromptImportDialog.test.tsx (all passing)
  • Integration tests: 2 new backend tests in test_prompts_validation_and_auth.py (all passing on SQLite)
  • Known limitations: PostgreSQL integration tests require Docker (not run locally)

Is there anything the reviewers should focus on/be aware of?

@solace-jkhalife solace-jkhalife self-assigned this May 15, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 15, 2026

✅ FOSSA Guard: Licensing (SolaceLabs_solace-agent-mesh) • PASSED

Compared against main (3118fa717678ea15ae464ccb7375d26973943573) • 0 new, 9 total (9 in base)

Scan Report | View Details in FOSSA

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 15, 2026

✅ FOSSA Guard: Vulnerability (SolaceLabs_solace-agent-mesh) • PASSED

Compared against main (3118fa717678ea15ae464ccb7375d26973943573) • 0 new, 14 total (14 in base)

Scan Report | View Details in FOSSA

@solace-jkhalife solace-jkhalife marked this pull request as ready for review May 19, 2026 12:44
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR prevents “invisible” prompts after import by validating the command/chat-shortcut field against the same allowed character set used by prompt-list responses, rejecting invalid values both server-side (HTTP 400) and client-side (form validation).

Changes:

  • Backend: validate imported command against ^[a-zA-Z0-9_-]+$ when preserveCommand is enabled, returning 400 on mismatch.
  • Frontend: add command regex validation to the import dialog and gate the Import action on overall form validity; adjust RHF setValue calls to trigger validation.
  • Tests: add backend integration tests for invalid commands + preserveCommand=false behavior; add frontend unit tests for dialog UX and button enablement.

Reviewed changes

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

File Description
src/solace_agent_mesh/gateway/http_sse/routers/prompts.py Adds backend command pattern validation during prompt import.
client/webui/frontend/src/lib/components/prompts/PromptImportDialog.tsx Adds client-side command regex validation, initial invalid-state handling, and Import button gating on form validity.
client/webui/frontend/src/stories/Prompts/PromptImportDialog.test.tsx Adds unit tests covering invalid/valid command UI states and Import button enablement.
tests/integration/apis/persistence/prompts/test_prompts_validation_and_auth.py Adds integration coverage for rejecting invalid commands on import and ignoring command when preserveCommand is false.

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

Comment thread client/webui/frontend/src/lib/components/prompts/PromptImportDialog.tsx Outdated
Comment thread src/solace_agent_mesh/gateway/http_sse/routers/prompts.py Outdated
@sonarqube-solacecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Collaborator

@lgh-solace lgh-solace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Can you check out the below comments?

Please review and fix if confirmed

  • The same $-before-newline quirk likely affects the create/update path too. Python's re (without re.MULTILINE) treats $ as matching either end-of-string or right before a trailing \n, and Pydantic v2's string pattern uses those semantics. The Field constraints at prompt_dto.py:69 and prompt_dto.py:93 still use pattern="^[a-zA-Z0-9_-]+$" — did you consider whether POST /groups / PATCH /groups/{id} with "command": "valid-cmd\n" slips through the same way the import path used to? A quick integration probe would confirm. If it does, the cleanest fix is a field_validator using re.fullmatch against a shared constant — which would also collapse the three regex copies.

Other areas to consider

  • One regex, three copies. re.fullmatch literal at prompts.py:1257, Pydantic pattern at prompt_dto.py:69 / :93, and COMMAND_PATTERN at PromptImportDialog.tsx:12. The new code-comment already calls out the coupling ("must match PromptGroupResponse") — extracting one Python constant + one FE constant alongside PROMPT_FIELD_LIMITS would prevent the next drift.
  • promptImportCommandSchema is unreferenced and duplicates the dialog's form schema. prompt-import.schema.ts:62-64 is dead code, and the regex change was made to the inline schema in the dialog instead. Worth deleting it or moving the dialog's promptImportFormSchema into the schema file so the rule lives in one place.
  • Backend truncates before it validates at prompts.py:1251-1257 — a 51-char command with an invalid char at position 51 will be sliced off and pass silently. Worth either swapping the order or adding a # why note.
  • PromptImportDialog.test.tsx:11-21 polyfills Blob.prototype.text per-file. If jsdom really lacks it in this version, this belongs in vitest.setup.ts once rather than copied into every new dialog test — PromptBuilderChat.test.tsx doesn't need it, which suggests it may not be necessary here either.
    11).

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread client/webui/frontend/src/lib/schemas/prompt-import.schema.ts
Comment thread src/solace_agent_mesh/gateway/http_sse/routers/prompts.py
Comment thread src/solace_agent_mesh/gateway/http_sse/routers/prompts.py
Comment thread src/solace_agent_mesh/gateway/http_sse/routers/dto/prompt_dto.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants