Skip to content

refactor(llm): derive valid providers and expose direct env reads - #718

Merged
cevheri merged 1 commit into
libredb:mainfrom
2160039878-cyber:fix/llm-config-source-673
Sep 9, 2026
Merged

refactor(llm): derive valid providers and expose direct env reads#718
cevheri merged 1 commit into
libredb:mainfrom
2160039878-cyber:fix/llm-config-source-673

Conversation

@2160039878-cyber

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

Copy link
Copy Markdown
Contributor

Description

The LLM configuration duplicates its provider list and hides four environment reads from the documentation guard. Derive the provider list from DEFAULT_MODELS and read the four LLM_* variables directly, preserving resolution and validation behavior.

Type of Change

  • Code refactoring

Related Issue

Closes #673

Changes Made

  • Reuse Object.keys(DEFAULT_MODELS) in provider resolution, validation, and the invalid-provider message.
  • Remove getEnvVar and update the environment guard's coverage-boundary description.
  • Add a provider-validation case derived from the model map. Replace the old assertion that LLM names are undiscovered with a positive discovery assertion; dynamic seed credentials remain outside the guard.

Testing

bun run test:unit -t 'environment variable documentation|accepts every provider in DEFAULT_MODELS': 10 passed / 0 failed after the refactor. The provider case accepts every key derived from Object.keys(DEFAULT_MODELS) and rejects an unknown provider; the new discovery assertion failed before the direct reads were introduced.

Mutation proof: temporarily removed all six active/commented LLM_MODEL= assignments from .env.example, then ran bun run test:unit -t 'environment variable documentation': 8 passed / 1 failed, reporting LLM_MODEL as undocumented. All six must be removed because the existing extractor intentionally accepts commented examples. Restored .env.example byte-for-byte; it has no diff in this PR.

Full Linux validation used the repository's unchanged CI workflow on submitted commit d87c448832369537d6256f82948556bbd6737b22: run and logs.

  • bun run test:coverage: 14,715 tests passed, all 391 core test files and 34 component isolation groups passed.
  • bun run coverage:check: 46,321 / 46,321 lines (100%).
  • Format, lint, typecheck, knip, README/chart/channel/security drift guards, application/library builds, package type-resolution checks, Helm lint, and Go launcher checks passed.
  • Browser E2E 65 passed, subpath E2E 1 passed, PostgreSQL functional smoke 1 passed, packaged tarball/npx E2E 3 passed each, Node 24/26 engine smoke passed.
  • Secret Scan, Dependency Scan, and Image Scan passed. The manual secret scan fetched and scanned all fork history and branches, including this commit.

The fork run's overall status is red solely because SonarCloud Analysis lacks the upstream token/project access. All test/build jobs passed. CLAUDE.md explicitly excludes SonarCloud from required checks, and the upstream workflow skips it for fork PRs. Upstream required workflows still need normal maintainer approval.

Full validation ran on GitHub-hosted Linux: this Windows host cannot run the container-based checks because Docker Desktop fails at inference-manager initialization, and its native full component runner encounters SQLite cleanup EBUSY. No complete native-Windows pass is claimed.

Checklist

  • Issue acceptance criteria checked; actual validation results recorded above.
  • Full tests and the 100% line-coverage gate passed on the submitted commit.
  • Diff reviewed; no database-provider changes requiring the provider triad.

Additional Notes

AI-assisted implementation, review, and validation. This branch starts independently from main and contains only this issue's change.

CI follow-up

The fork-run SonarCloud 401 is tracked in #732 and fixed by #733. The inherited condition admitted fork-owned pushes and fork-local PRs to the canonical SonarCloud project. The dedicated CI fix run now succeeds: all nine executable test/build jobs pass, and SonarCloud is scoped to the canonical repository. That run tests CI fix commit 80a318b; this PR's exact-head verification remains the original run linked above, whose nine executable jobs passed. Upstream Actions still await maintainer approval.

@cevheri cevheri added enhancement New feature or request ai Artificial intelligence labels Sep 9, 2026
@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 commented Sep 9, 2026

Copy link
Copy Markdown
Member

Checked against #673. All four "Done when" items hold. Both validProviders arrays are gone, getEnvVar is gone with four direct reads in its place, and the boundary paragraph is updated in both places it lives: the header and the comment inside the boundary test.

Two things I verified beyond that.

Behaviour is preserved where it is easiest to lose. Object.keys(DEFAULT_MODELS) yields gemini, openai, ollama, custom in that order, identical to the arrays you deleted, so validateConfig's "Valid options: ..." text is unchanged.

I also added the control your mutation run was missing. Deleting all six LLM_MODEL= lines turns the guard red with exactly ["LLM_MODEL"] on your branch, as you reported. Running the same deletion against main's config.ts leaves that test green, because the name is invisible to the extractor there. That pair is the real proof: shut before, open now.

The part I want to call out is that you noticed all six lines had to go, commented per-provider examples included, because isDocumented strips a leading #. The issue told you to delete one line. Following it literally would have produced a green run and the wrong conclusion, and you went and read the guard instead of trusting the instruction. That is the habit that makes a refactor like this trustworthy, and it is worth more than the diff.

Merging shortly.

@cevheri
cevheri merged commit c309623 into libredb:main Sep 9, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai Artificial intelligence enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The LLM config module keeps its valid-provider list twice by hand, and getEnvVar hides four variables from the env-documentation guard

2 participants