Skip to content

fix(safety): retain query confirmation without configured AI - #742

Merged
cevheri merged 4 commits into
libredb:mainfrom
2160039878-cyber:fix/query-safety-without-ai-703
Sep 10, 2026
Merged

fix(safety): retain query confirmation without configured AI#742
cevheri merged 4 commits into
libredb:mainfrom
2160039878-cyber:fix/query-safety-without-ai-703

Conversation

@2160039878-cyber

@2160039878-cyber 2160039878-cyber commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Description

When a provider has no required API key, Query Safety Check keeps a plain confirmation explaining that the statement may change data, objects or permissions. A distinct LLM_UNCONFIGURED code selects this path. Invalid provider/model/URL settings retain LLM_CONFIG and remain visible, as do authentication and service errors.

Closes #703.

Type of Change

  • Bug fix (non-breaking)
  • Documentation and test updates

Changes Made

  • Tag missing required credentials at the existing LLM validation and API-key checks. Map that reason to LLM_UNCONFIGURED, retaining HTTP 503 and the existing error response shape; no message matching is used.
  • Suppress only that code in Query Safety. The local dangerous-statement gate, displayed query, cancellation and explicit execution controls are retained.
  • Update README and docs/FEATURES.md with the narrower missing-credentials exception, and list the new response code in docs/API_DOCS.md.

Testing

  • TDD: both new explicit-confirmation cases failed against the original source because it displayed the configuration error.
  • Review TDD: all six missing-credentials / invalid-configuration UI cases and both real validation-to-response credential cases fail against the previous implementation.
  • bun run test:components --pass-with-no-tests -t 'QuerySafetyDialog|isDangerousQuery': 121 matching tests passed, 0 failed.
  • bun run test:unit --isolate --pass-with-no-tests -t 'createErrorResponse|ApiErrorCode|BaseLLMProvider|validateConfig': 55 passed, 0 failed.
  • bun run test:api --isolate --pass-with-no-tests -t 'POST /api/ai/query-safety': 9 passed, 0 failed.
  • Covers missing Gemini/OpenAI credentials, invalid provider names, missing models or custom URLs, Ollama's model-not-found response, HTTP error classification and unchanged explicit confirmation. Existing LLM_AUTH and other failure paths remain visible.
  • Passed locally: format, lint, typecheck, knip, readme:check, chart:check, channels:showcase:check, security:check, production build, build:lib and attw.
  • Full local bun run test / coverage and E2E were not completed: Windows host lacks Helm/chart dependencies, Docker is unavailable, and existing SQLite cleanup tests encounter Windows file-lock errors. Official Linux CI must verify the full suite and 100% line-coverage gate.

Environment: Windows, Node.js 24.18.1, Bun 1.4.2. Both builds ran from a clean checkout of implementation commit 415fc4d with real local dependencies. The subsequent API documentation entry passed readme:check; CI validates the final submitted commit.

Checklist

  • Reviewed the diff and updated documentation and regression tests.
  • Required CI test job passed the 100% line-coverage gate; all 20 checks passed on 11ad27f (2 normal fork skips). Verified CI run.

Additional Notes

AI-assisted implementation and test execution using Codex. Existing LLMConfigError callers without a reason retain LLM_CONFIG; only errors explicitly tagged as missing credentials use the new code. No new dependencies or environment variables. All existing error messages and HTTP statuses are retained.

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cevheri cevheri added the loop:needs-moderator-action Flagged by the maintainer loop: suspicious content or a decision only a human can make label Sep 9, 2026
@cevheri

cevheri commented Sep 9, 2026

Copy link
Copy Markdown
Member

The behaviour is right for the case #703 describes, and your two new cases do fail against the pre-change source, so the guard is pinned rather than decorative. One thing needs narrowing before this can land.

LLM_CONFIG is not only "no API key". src/lib/llm/utils/config.ts also raises it for Invalid provider: gemni. Valid options: ... after a typo in LLM_PROVIDER, for Model name is required for ollama provider., and for Custom provider requires LLM_API_URL ...; providers/ollama.ts raises it at stream time for Model not found. Make sure "llama3" is pulled in Ollama. I added those three as cases in your own test file on cf105cc and all three passed a "message is not rendered" assertion, so someone who did configure AI now sees the generic warning with no sign that their setup is broken.

Please key the silence on the unconfigured case alone, for example a distinct code emitted only when the provider has no credentials at all, and leave the rest of LLM_CONFIG visible.

Also docs/FEATURES.md:91 still states that the Query Safety dialog renders an AI failure inline. That needs to move in the same PR.

@2160039878-cyber

Copy link
Copy Markdown
Contributor Author

Fixed in 415fc4d. Missing required credentials now carry a distinct LLM_UNCONFIGURED code from the existing validation/API-key checks; the dialog leaves all LLM_CONFIG errors visible. README and docs/FEATURES.md describe that exception.

The regression cases cover invalid provider names, missing models, missing custom URLs and Ollama's model-not-found response, plus the absent-key confirmation and real validation-to-HTTP mapping. Six UI cases and both credential mapping cases fail on the previous implementation. Current targeted validation is 121 component, 55 unit and 9 API tests, all passing, plus the repository's static checks and both builds. The PR body records the commands and local full-suite limitations.

2160039878-cyber and others added 2 commits September 10, 2026 03:21
…hed setup

Silencing every missing-credentials error hid a real case. LLM_PROVIDER set
without LLM_API_KEY looked exactly like no AI at all, while the same omission
for ollama or custom stayed visible, so the same mistake was reported for two
providers and swallowed for the other two.

Naming a provider is a statement of intent to use AI, so LLMConfig now carries
providerExplicit, resolveConfig sets it from the environment or an override,
and unconfiguredReason is the single place that decides which of the two cases
a missing credential is. Also pins the gemini and openai reason, which no test
covered: dropping the argument left the suite green.
@cevheri
cevheri merged commit 6ee1f35 into libredb:main Sep 10, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

loop:needs-moderator-action Flagged by the maintainer loop: suspicious content or a decision only a human can make

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] query safety check - ability to remove Gemini error message

2 participants