feat: per-company invoice classifier configuration#170
Merged
Conversation
Add a Settings → Services page where company owners can override the global env-var classifier config with per-company values (URL, API token, confidence thresholds). This enables dedicated ML models per company while preserving env-var defaults for local development. - New `classifier_configs` table (company-scoped, unique per company) - `ServiceConfig` context with CRUD and env_defaults helper - `ClassifierConfig` schema with conditional validation (required fields only when override is enabled) - LiveView with health check before save, collapsible API docs, and env-var placeholders showing current defaults - Owner/admin-only access via `:manage_services` permission - ADR-0049 documents the design decisions and rejected alternatives Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds DB-backed per-company invoice classifier configuration with encrypted API token, env-var fallbacks, a permission-gated Services settings LiveView at /c/:company_id/settings/services including async health probes and confirmation flow; runtime clients remain reading global env vars for now. Changes
Sequence DiagramsequenceDiagram
participant User
participant LiveView as Services LiveView
participant ServiceConfig as ServiceConfig Context
participant DB as Database
participant Health as Health Probe
participant Encrypt as Encryption Service
User->>LiveView: GET /c/:company_id/settings/services (mount)
LiveView->>ServiceConfig: get_or_create_classifier_config(company_id)
ServiceConfig->>DB: SELECT ... WHERE company_id
alt not found
ServiceConfig->>DB: INSERT default classifier_config
DB-->>ServiceConfig: inserted row
end
ServiceConfig-->>LiveView: ClassifierConfig
LiveView->>ServiceConfig: env_defaults()
ServiceConfig-->>LiveView: defaults
LiveView->>Health: async GET <url>/health (initial probe)
Health-->>LiveView: {:ok} or {:error, reason}
User->>LiveView: Submit "Save" with attrs
LiveView->>Health: async pre-save GET <url>/health
Health-->>LiveView: {:ok} or {:error, reason}
alt health ok
LiveView->>ServiceConfig: update_classifier_config(attrs, user_id)
ServiceConfig->>Encrypt: encrypt(api_token) (if present)
Encrypt-->>ServiceConfig: encrypted_blob
ServiceConfig->>DB: UPDATE classifier_configs SET ...
DB-->>ServiceConfig: updated row
ServiceConfig-->>LiveView: {:ok, updated_config}
LiveView->>User: flash success
else health failed
LiveView->>User: show "Save anyway?" confirmation
User->>LiveView: confirm_save
LiveView->>ServiceConfig: update_classifier_config(...)
ServiceConfig->>DB: UPDATE ...
DB-->>ServiceConfig: updated row
ServiceConfig-->>LiveView: {:ok, updated_config}
LiveView->>User: flash success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
priv/repo/migrations/20260423052425_update_service_configurations_default_disabled.exs (1)
1-11: Consider squashing intermediate migrations before merge.This migration modifies
service_configurations, but the subsequent migration (20260423053709) drops this table entirely and replaces it withclassifier_configs. As noted in ADR-0049, these intermediate migrations are "harmless but noisy."For cleaner migration history, consider squashing the three migrations into a single migration that directly creates
classifier_configs(only if no production data exists inservice_configurationsyet).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@priv/repo/migrations/20260423052425_update_service_configurations_default_disabled.exs` around lines 1 - 11, The migration module UpdateServiceConfigurationsDefaultDisabled updates service_configurations to set enabled=false but a later migration removes that table and adds classifier_configs, so squash these intermediate migrations by combining UpdateServiceConfigurationsDefaultDisabled and the subsequent migrations into one migration that directly creates classifier_configs (and removes any updates to service_configurations); ensure you only do this squash if there is no production data in service_configurations yet, otherwise keep history intact; update the combined migration to create the classifier_configs schema and remove or omit any UPDATE statements touching service_configurations (remove functions up/down in UpdateServiceConfigurationsDefaultDisabled and replace with the single create/drop logic for classifier_configs).lib/ksef_hub/service_config.ex (2)
47-65: UseTrackedRepo.updatefor audit trail.Similar to the insert case, updating classifier config is a user-visible settings change that should be tracked in the activity log.
♻️ Proposed change
- Repo.update(changeset) + TrackedRepo.update(changeset)As per coding guidelines: "Every context mutation affecting user-visible state must emit an activity event using
TrackedRepoinstead ofRepo"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/service_config.ex` around lines 47 - 65, In update_classifier_config/2 replace the direct Repo.update(changeset) call with the tracked repository so the change emits an activity event (use TrackedRepo.update instead of Repo.update); keep the same changeset construction (including the api_token encryption logic in update_classifier_config and ClassifierConfig.changeset) and add any required alias/import for TrackedRepo in this module so compilation succeeds.
26-40: Consider usingTrackedRepofor audit trail.Per coding guidelines, context mutations affecting user-visible state should use
TrackedRepofor automatic activity event emission. Creating a classifier config is a user-visible settings change.♻️ Proposed change
+ alias KsefHub.ActivityLog.TrackedRepo def get_or_create_classifier_config(company_id) do case get_classifier_config(company_id) do nil -> %ClassifierConfig{company_id: company_id} |> Ecto.Changeset.change() - |> Repo.insert(on_conflict: :nothing, conflict_target: :company_id) + |> TrackedRepo.insert(on_conflict: :nothing, conflict_target: :company_id)As per coding guidelines: "Every context mutation affecting user-visible state must emit an activity event using
TrackedRepoinstead ofRepo"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/service_config.ex` around lines 26 - 40, The get_or_create_classifier_config function is creating a user-visible settings change using Repo.insert and Repo.get_by!, so replace those calls with TrackedRepo variants to ensure activity events are emitted; specifically, update the insert path that currently calls Repo.insert(on_conflict: :nothing, conflict_target: :company_id) to use TrackedRepo.insert (preserving the on_conflict and conflict_target options and the Ecto.Changeset from %ClassifierConfig), and replace the fallback Repo.get_by!(ClassifierConfig, company_id: company_id) with TrackedRepo.get_by!(ClassifierConfig, company_id: company_id) so the creation path uses the tracked repository for audit trails.lib/ksef_hub_web/live/settings_live/services.ex (3)
396-409: Handle trailing slashes in URL and narrow the rescue clause.String concatenation doesn't account for URLs with trailing slashes (e.g.,
http://example.com/becomeshttp://example.com//health). Also, the broadrescuecan mask unexpected errors.Proposed fix
`@spec` check_url_health(String.t()) :: :ok | {:error, term()} defp check_url_health(url) do + health_url = URI.merge(url, "/health") |> to_string() + case Req.get( - url: url <> "/health", + url: health_url, receive_timeout: 5_000, retry: false, connect_options: [timeout: 3_000] ) do {:ok, %{status: 200}} -> :ok {:ok, %{status: status}} -> {:error, {:http_error, status}} {:error, reason} -> {:error, reason} end -rescue - e -> {:error, Exception.message(e)} end
Req.getalready returns{:error, reason}for network/timeout issues, so the rescue is typically unnecessary unless guarding against malformed URLs—in which case, validate earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/settings_live/services.ex` around lines 396 - 409, In check_url_health, avoid concatenating "/health" directly to url (which creates "//health" if url has a trailing slash) by normalizing the URL before calling Req.get (e.g., strip a trailing slash or use a proper URI join) and remove the broad rescue; instead either let Req.get return {:error, reason} or rescue only specific exceptions (e.g., ArgumentError) and return {:error, reason} from that narrow rescue so unexpected errors are not masked; update check_url_health to call Req.get with the normalized URL and eliminate the catch‑all rescue clause.
252-273: Clarify token clearing behavior.Both
niland""are treated as "keep existing" per the comment. However, the context function atlib/ksef_hub/service_config.ex:47-65supports clearing via"". This means users have no UI path to clear a configured token—they can only overwrite it with a new value.If this is intentional, consider adding a "Clear token" button or checkbox. If clearing should work, pass
""through instead of deleting it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/settings_live/services.ex` around lines 252 - 273, The current handle_event("save", %{"classifier" => params}, socket) block treats both nil and "" api_token values as "keep existing", which prevents the service_config logic (which supports clearing via "") from clearing tokens; change the params normalization so only nil removes the key (i.e., remove Map.delete(params, "api_token") for the "" branch and allow the empty string to pass through) so do_save/resolve_check_url and the downstream service_config behavior can receive "" to clear the token (or alternatively implement a dedicated "Clear token" UI control if intentional).
241-294: Consider adding@specto callback function clauses.The
handle_event/3andhandle_info/2clauses lack explicit@spec. While thePhoenix.LiveViewbehaviour defines these, per coding guidelines all functions should have type specifications.Example for handle_event
`@impl` true `@spec` handle_event(String.t(), map(), Phoenix.LiveView.Socket.t()) :: {:noreply, Phoenix.LiveView.Socket.t()} def handle_event("validate", %{"classifier" => params}, socket) do # ... end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/settings_live/services.ex` around lines 241 - 294, Add explicit `@spec` annotations for the LiveView callbacks: declare a typespec for handle_event/3 (e.g. `@spec` handle_event(String.t(), map(), Phoenix.LiveView.Socket.t()) :: {:noreply, Phoenix.LiveView.Socket.t()}) and for handle_info/2 (e.g. `@spec` handle_info(term(), Phoenix.LiveView.Socket.t()) :: {:noreply, Phoenix.LiveView.Socket.t()}) and place each `@spec` immediately above the first clause of the corresponding function (referencing handle_event/3 and handle_info/2) so all clauses satisfy the module's coding guidelines.test/ksef_hub_web/live/settings_live/services_test.exs (1)
116-133: Consider testing save via LiveView form submission.This test calls the context function directly rather than exercising the LiveView save flow. This duplicates coverage already present in
service_config_test.exsand doesn't verify the LiveView form handling,updated_by_idinjection, or flash messages.Suggested approach
Test the actual LiveView save flow instead:
test "saves classifier config via LiveView form", %{conn: conn, company: company, user: user} do {:ok, view, _html} = live(conn, ~p"/c/#{company.id}/settings/services") # Submit form with valid data (may need to mock health check) view |> form("form[phx-submit=save]", classifier: %{ enabled: true, url: "http://custom:9000", category_confidence_threshold: "0.85", tag_confidence_threshold: "0.90" }) |> render_submit() # Assert flash or verify persisted config config = ServiceConfig.get_classifier_config(company.id) assert config.url == "http://custom:9000" assert config.updated_by_id == user.id end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/settings_live/services_test.exs` around lines 116 - 133, Replace the direct context call test "saves classifier config via context" with a LiveView integration test that opens the services page (use live(conn, ~p"/c/#{company.id}/settings/services")), submits the classifier form via the view using form("form[phx-submit=save]", classifier: ...) and render_submit(), then verify persistence via ServiceConfig.get_classifier_config(company.id) and assert fields including url, category_confidence_threshold/tag_confidence_threshold (as floats) and updated_by_id == user.id; if the LiveView triggers external checks, stub/mocks those health checks before submission so the form can submit successfully and assert any flash if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/sidecar-services.md`:
- Around line 148-149: Update the documentation to clarify that although the
admin UI exposes per-company classifier overrides (stored in
classifier_configs), the classification pipeline does not yet read those DB
overrides and still uses global Application.get_env settings; reference the
invoice classifier implementation (lib/ksef_hub/invoice_classifier.ex) and
ADR-0049 to state the per-company override UI is available but the runtime
wiring/integration is pending so DB values do not currently take precedence over
env vars.
In `@lib/ksef_hub/service_config/classifier_config.ex`:
- Around line 49-56: Remove :updated_by_id from the cast list inside
ClassifierConfig.changeset so this programmatic field is not accepted from
input; instead, in the service layer implement update_classifier_config/2 (or
update_classifier_config/3 if you prefer passing the id) to call
ClassifierConfig.changeset(attrs) and then set the actor explicitly with
Ecto.Changeset.put_change(:updated_by_id, updated_by_id) before persisting;
update any callers to pass the updated_by_id into update_classifier_config/2 so
the value is set server-side rather than via form input.
In
`@priv/repo/migrations/20260423053709_replace_service_configurations_with_classifier_configs.exs`:
- Around line 4-24: The migration currently uses change/0 and calls drop
table(:service_configurations), which is not reversible; replace change/0 with
explicit up/0 and down/0 functions: move the existing drop
table(:service_configurations), create table(:classifier_configs, primary_key:
false) ... and create unique_index(:classifier_configs, [:company_id]) into
up/0, and implement down/0 to drop :classifier_configs and recreate the original
:service_configurations table (including id :binary_id primary key, service_name
:string null: false, url :string, settings :map, enabled :boolean, timestamps()
and the unique_index on [:service_name]) so rollbacks succeed.
In `@test/ksef_hub_web/live/settings_live/services_test.exs`:
- Around line 140-154: The test currently calls
ServiceConfig.update_classifier_config(...) for config and other_config without
capturing results, which can hide failures; update the test to capture the
return tuples from ServiceConfig.update_classifier_config (for both the first
config and other_config) into variables and assert they indicate success (e.g.,
match {:ok, _updated} or equivalent) so failures surface, referencing
ServiceConfig.update_classifier_config and
ServiceConfig.get_or_create_classifier_config in the test.
In `@test/ksef_hub/service_config_test.exs`:
- Around line 137-149: The calls to ServiceConfig.update_classifier_config/2
discard their return values so failures like {:error, changeset} are hidden;
change the two calls for config_a and config_b to capture and assert their
results (e.g., pattern-match or assert {:ok, _} =
ServiceConfig.update_classifier_config(config_a, ...)) so any error is surfaced
immediately; reference the function ServiceConfig.update_classifier_config/2 and
the variables config_a and config_b when making the change.
---
Nitpick comments:
In `@lib/ksef_hub_web/live/settings_live/services.ex`:
- Around line 396-409: In check_url_health, avoid concatenating "/health"
directly to url (which creates "//health" if url has a trailing slash) by
normalizing the URL before calling Req.get (e.g., strip a trailing slash or use
a proper URI join) and remove the broad rescue; instead either let Req.get
return {:error, reason} or rescue only specific exceptions (e.g., ArgumentError)
and return {:error, reason} from that narrow rescue so unexpected errors are not
masked; update check_url_health to call Req.get with the normalized URL and
eliminate the catch‑all rescue clause.
- Around line 252-273: The current handle_event("save", %{"classifier" =>
params}, socket) block treats both nil and "" api_token values as "keep
existing", which prevents the service_config logic (which supports clearing via
"") from clearing tokens; change the params normalization so only nil removes
the key (i.e., remove Map.delete(params, "api_token") for the "" branch and
allow the empty string to pass through) so do_save/resolve_check_url and the
downstream service_config behavior can receive "" to clear the token (or
alternatively implement a dedicated "Clear token" UI control if intentional).
- Around line 241-294: Add explicit `@spec` annotations for the LiveView
callbacks: declare a typespec for handle_event/3 (e.g. `@spec`
handle_event(String.t(), map(), Phoenix.LiveView.Socket.t()) :: {:noreply,
Phoenix.LiveView.Socket.t()}) and for handle_info/2 (e.g. `@spec`
handle_info(term(), Phoenix.LiveView.Socket.t()) :: {:noreply,
Phoenix.LiveView.Socket.t()}) and place each `@spec` immediately above the first
clause of the corresponding function (referencing handle_event/3 and
handle_info/2) so all clauses satisfy the module's coding guidelines.
In `@lib/ksef_hub/service_config.ex`:
- Around line 47-65: In update_classifier_config/2 replace the direct
Repo.update(changeset) call with the tracked repository so the change emits an
activity event (use TrackedRepo.update instead of Repo.update); keep the same
changeset construction (including the api_token encryption logic in
update_classifier_config and ClassifierConfig.changeset) and add any required
alias/import for TrackedRepo in this module so compilation succeeds.
- Around line 26-40: The get_or_create_classifier_config function is creating a
user-visible settings change using Repo.insert and Repo.get_by!, so replace
those calls with TrackedRepo variants to ensure activity events are emitted;
specifically, update the insert path that currently calls
Repo.insert(on_conflict: :nothing, conflict_target: :company_id) to use
TrackedRepo.insert (preserving the on_conflict and conflict_target options and
the Ecto.Changeset from %ClassifierConfig), and replace the fallback
Repo.get_by!(ClassifierConfig, company_id: company_id) with
TrackedRepo.get_by!(ClassifierConfig, company_id: company_id) so the creation
path uses the tracked repository for audit trails.
In
`@priv/repo/migrations/20260423052425_update_service_configurations_default_disabled.exs`:
- Around line 1-11: The migration module
UpdateServiceConfigurationsDefaultDisabled updates service_configurations to set
enabled=false but a later migration removes that table and adds
classifier_configs, so squash these intermediate migrations by combining
UpdateServiceConfigurationsDefaultDisabled and the subsequent migrations into
one migration that directly creates classifier_configs (and removes any updates
to service_configurations); ensure you only do this squash if there is no
production data in service_configurations yet, otherwise keep history intact;
update the combined migration to create the classifier_configs schema and remove
or omit any UPDATE statements touching service_configurations (remove functions
up/down in UpdateServiceConfigurationsDefaultDisabled and replace with the
single create/drop logic for classifier_configs).
In `@test/ksef_hub_web/live/settings_live/services_test.exs`:
- Around line 116-133: Replace the direct context call test "saves classifier
config via context" with a LiveView integration test that opens the services
page (use live(conn, ~p"/c/#{company.id}/settings/services")), submits the
classifier form via the view using form("form[phx-submit=save]", classifier:
...) and render_submit(), then verify persistence via
ServiceConfig.get_classifier_config(company.id) and assert fields including url,
category_confidence_threshold/tag_confidence_threshold (as floats) and
updated_by_id == user.id; if the LiveView triggers external checks, stub/mocks
those health checks before submission so the form can submit successfully and
assert any flash if desired.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a80b414-5908-461c-b6ee-b20665534192
📒 Files selected for processing (19)
.env.exampledocs/adr/0049-per-company-classifier-config.mddocs/architecture.mddocs/infrastructure.mddocs/sidecar-services.mdlib/ksef_hub/authorization.exlib/ksef_hub/invoice_classifier.exlib/ksef_hub/service_config.exlib/ksef_hub/service_config/classifier_config.exlib/ksef_hub_web/components/settings_components.exlib/ksef_hub_web/live/settings_live/services.exlib/ksef_hub_web/router.expriv/repo/migrations/20260422215003_create_service_configurations.exspriv/repo/migrations/20260423052425_update_service_configurations_default_disabled.exspriv/repo/migrations/20260423053709_replace_service_configurations_with_classifier_configs.exstest/ksef_hub/invoice_classifier/client_test.exstest/ksef_hub/invoice_extractor/client_test.exstest/ksef_hub/service_config_test.exstest/ksef_hub_web/live/settings_live/services_test.exs
- Remove :updated_by_id from changeset cast; set server-side via put_change to prevent form input from overriding the actor - Make replacement migration reversible with explicit up/down - Assert return values of update_classifier_config in isolation tests - Fix trailing-slash bug in health check URL construction - Clarify docs that classifier pipeline doesn't yet read DB overrides Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/live/settings_live/services.ex`:
- Around line 10-11: The LiveView currently calls ServiceConfig.ClassifierConfig
directly for building and validating changesets, coupling the web layer to
internal module details; add a context helper
ServiceConfig.change_classifier_config(struct_or_params, attrs \\ %{}) that
returns the appropriate changeset (for new and existing ClassifierConfig) and
replace all direct calls to ServiceConfig.ClassifierConfig.* in this LiveView
(including the form-building and validation points where changesets are created)
with ServiceConfig.change_classifier_config/2 so the LiveView only calls the
ServiceConfig context API (e.g., replace any
ServiceConfig.ClassifierConfig.changeset(...) or new/empty struct usage with
ServiceConfig.change_classifier_config(classifier_config_or_parent, attrs)).
- Around line 265-267: The async health probes started with
Task.Supervisor.async_nolink(KsefHub.TaskSupervisor, fn -> {:pre_save_health,
params, check_url_health(url)} end) are racing and older task replies can
overwrite `@health` and pending_params; fix by storing the Task.async ref(s)
returned by Task.Supervisor.async_nolink in the LiveView assigns (e.g.,
:active_health_ref or a map of refs keyed by intent) when starting a probe, then
in handle_info/2 that handles the {:pre_save_health, ...} reply check the
incoming ref against the stored active ref(s) and drop the message if it doesn't
match, updating `@health/pending_params` only when the refs match; apply the same
pattern for the other probe locations mentioned (lines ~286-289 and 299-315) to
ensure stale tasks are ignored.
- Around line 253-259: The current params normalization removes the "" token
entirely so ServiceConfig.update_classifier_config/2 never receives a deliberate
empty-string clear request; change the logic so that only absent (nil) keys are
deleted but preserve "" values (or implement an explicit clear control like
"clear_api_token" and send "" only when that flag is set) — update the block
handling params["api_token"] (the Map.delete branches) so that "" is forwarded
to ServiceConfig.update_classifier_config/2 as the clear-token signal instead of
being removed unless you add and check a dedicated clear flag.
In `@lib/ksef_hub/service_config.ex`:
- Around line 48-75: The update_classifier_config function must stop reading
updated_by_id from the caller-supplied attrs and must use TrackedRepo to persist
changes so an activity event is emitted: change the function signature to accept
actor metadata (e.g., updated_by_id or actor) separately from attrs, remove
Map.get(attrs, "updated_by_id") / Map.get(attrs, :updated_by_id) usage, build
the changeset with ClassifierConfig.changeset and the api_token encryption logic
unchanged, then call TrackedRepo.update(changeset, actor: <actor-metadata>)
instead of Repo.update(changeset) so the actor is recorded server-side and the
required activity event is emitted.
- Around line 70-72: The current clause pattern-matches on
Encryption.encrypt(token) which will crash if it returns {:error, reason};
change the logic in the token when is_binary(token) branch to use a with (or
case) to call Encryption.encrypt(token) and, on {:ok, encrypted}, call
Ecto.Changeset.put_change(changeset, :api_token_encrypted, encrypted), but on
{:error, reason} return the original changeset with an added error via
Ecto.Changeset.add_error(changeset, :api_token_encrypted, "encryption failed:
#{inspect(reason)}") so encryption failures become normal changeset errors
instead of raising.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 972be5e2-b8d5-4785-994d-5dd5a7ee4fff
📒 Files selected for processing (7)
docs/sidecar-services.mdlib/ksef_hub/service_config.exlib/ksef_hub/service_config/classifier_config.exlib/ksef_hub_web/live/settings_live/services.expriv/repo/migrations/20260423053709_replace_service_configurations_with_classifier_configs.exstest/ksef_hub/service_config_test.exstest/ksef_hub_web/live/settings_live/services_test.exs
🚧 Files skipped from review as they are similar to previous changes (4)
- test/ksef_hub_web/live/settings_live/services_test.exs
- docs/sidecar-services.md
- lib/ksef_hub/service_config/classifier_config.ex
- priv/repo/migrations/20260423053709_replace_service_configurations_with_classifier_configs.exs
Comment on lines
+70
to
+72
| token when is_binary(token) -> | ||
| {:ok, encrypted} = Encryption.encrypt(token) | ||
| Ecto.Changeset.put_change(changeset, :api_token_encrypted, encrypted) |
There was a problem hiding this comment.
Handle encryption failures without crashing the save flow.
Line 71 pattern-matches on Encryption.encrypt/1. If encryption returns {:error, reason}, this raises and turns a recoverable validation/persistence error into a LiveView crash. Convert that failure into a normal changeset error instead.
Possible fix
token when is_binary(token) ->
- {:ok, encrypted} = Encryption.encrypt(token)
- Ecto.Changeset.put_change(changeset, :api_token_encrypted, encrypted)
+ with {:ok, encrypted} <- Encryption.encrypt(token) do
+ Ecto.Changeset.put_change(changeset, :api_token_encrypted, encrypted)
+ else
+ {:error, reason} ->
+ Ecto.Changeset.add_error(
+ changeset,
+ :api_token,
+ "could not be encrypted: #{inspect(reason)}"
+ )
+ endAs per coding guidelines, "Use with for multi-step operations that can fail in Elixir code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/service_config.ex` around lines 70 - 72, The current clause
pattern-matches on Encryption.encrypt(token) which will crash if it returns
{:error, reason}; change the logic in the token when is_binary(token) branch to
use a with (or case) to call Encryption.encrypt(token) and, on {:ok, encrypted},
call Ecto.Changeset.put_change(changeset, :api_token_encrypted, encrypted), but
on {:error, reason} return the original changeset with an added error via
Ecto.Changeset.add_error(changeset, :api_token_encrypted, "encryption failed:
#{inspect(reason)}") so encryption failures become normal changeset errors
instead of raising.
…aces - Add ServiceConfig.change_classifier_config/2 so LiveView no longer calls ClassifierConfig.changeset directly - Use TrackedRepo.update with actor opts for activity log emission instead of Repo.update with updated_by_id in form attrs - Track active_health_ref to discard stale async health probe replies - Revert .env.example to main state Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/ksef_hub_web/live/settings_live/services.ex (1)
259-265:⚠️ Potential issue | 🟠 MajorKeep the explicit blank-token clear path.
Deleting
""here meansServiceConfig.update_classifier_config/3never receives the clear-token signal it already supports. From this screen, once a company token is stored, there is no way to remove it without turning the whole override off.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/settings_live/services.ex` around lines 259 - 265, The current params normalization removes empty-string values, which prevents ServiceConfig.update_classifier_config/3 from receiving the explicit clear-token signal; change the params handling in the api_token branch so nil still deletes the key but "" is preserved (do not Map.delete params when params["api_token"] == ""), ensuring the params map passes an explicit "" value to ServiceConfig.update_classifier_config/3 so the clear-token path remains reachable.
🧹 Nitpick comments (1)
test/ksef_hub_web/live/settings_live/services_test.exs (1)
90-166: Add a regression test for the failed-probe edit/resubmit flow.The current suite never covers “health check fails → user edits the form → Save anyway / resubmit”, which is exactly where stale
pending_paramscan slip through. A LiveView test for that path would lock down the confirmation-state handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/settings_live/services_test.exs` around lines 90 - 166, Add a LiveView test in the existing "saving" describe block that reproduces the failed-probe → edit → resubmit path: arrange a classifier config and stub/mock the health check (probe) to fail on the first save, mount the live view via live(conn, ~p"/c/#{company.id}/settings/services"), submit the form (use form("form[phx-submit=save]", classifier: ...)) to trigger the failing probe and assert the probe-failure error is rendered, then simulate the user editing the form (e.g., change the url field with render_change) and resubmit (render_submit), finally assert the ServiceConfig persisted the new value (via ServiceConfig.get_or_create_classifier_config or ServiceConfig.reload) to ensure the edited value, not stale pending_params, was saved; reference symbols: live/1, form("form[phx-submit=save]"), render_change, render_submit, ServiceConfig.get_or_create_classifier_config, and ServiceConfig.update_classifier_config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/live/settings_live/services.ex`:
- Around line 293-295: The handler handle_event("check_health", _params, socket)
is using the persisted socket.assigns.config instead of the current unsaved form
inputs; change it to derive the config from the live form/changeset before
calling check_health_async (e.g. use socket.assigns.changeset |>
Ecto.Changeset.apply_changes() or read socket.assigns.form data to build the
temp config) and pass that temporary config (and current env_defaults from the
form if applicable) into check_health_async, then assign health: :checking and
active_health_ref as before.
- Around line 419-430: Reject private or internal targets before calling
check_url_health by parsing the URL (use URI.parse) and validating scheme is
http/https, resolving the hostname to one or more IPs (e.g., :inet.getaddr /
:inet_res.lookup) and refusing any address in loopback, link-local, RFC1918
(10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16), carrier-grade (100.64.0.0/10), IPv6
loopback/ULA/ULA equivalents, and known metadata addresses like 169.254.169.254;
also prevent following redirects by setting Req option redirect: false so probes
can't chain to internal targets. Implement this check inside or called from
check_url_health (and name the helper e.g., host_allowed?/1 or ip_allowed?/1)
and return {:error, :disallowed_target} before making the Req.get call when the
host/IP is disallowed.
- Around line 248-255: In handle_event("validate", %{"classifier" => params},
socket) clear any pending confirmation state so stale pending_params aren't
used: when building the changeset and assigning form (to_form(changeset, as:
:classifier)) also reset pending_params (set to nil or %{}) and
pending_confirmation (false) on the socket assigns; update the assign(socket,
...) call to include those keys so editing the form removes the previous failed
pre-save probe state before validation.
---
Duplicate comments:
In `@lib/ksef_hub_web/live/settings_live/services.ex`:
- Around line 259-265: The current params normalization removes empty-string
values, which prevents ServiceConfig.update_classifier_config/3 from receiving
the explicit clear-token signal; change the params handling in the api_token
branch so nil still deletes the key but "" is preserved (do not Map.delete
params when params["api_token"] == ""), ensuring the params map passes an
explicit "" value to ServiceConfig.update_classifier_config/3 so the clear-token
path remains reachable.
---
Nitpick comments:
In `@test/ksef_hub_web/live/settings_live/services_test.exs`:
- Around line 90-166: Add a LiveView test in the existing "saving" describe
block that reproduces the failed-probe → edit → resubmit path: arrange a
classifier config and stub/mock the health check (probe) to fail on the first
save, mount the live view via live(conn,
~p"/c/#{company.id}/settings/services"), submit the form (use
form("form[phx-submit=save]", classifier: ...)) to trigger the failing probe and
assert the probe-failure error is rendered, then simulate the user editing the
form (e.g., change the url field with render_change) and resubmit
(render_submit), finally assert the ServiceConfig persisted the new value (via
ServiceConfig.get_or_create_classifier_config or ServiceConfig.reload) to ensure
the edited value, not stale pending_params, was saved; reference symbols:
live/1, form("form[phx-submit=save]"), render_change, render_submit,
ServiceConfig.get_or_create_classifier_config, and
ServiceConfig.update_classifier_config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2edc116c-2db9-4b65-80ab-6cc2a5eeb7a8
📒 Files selected for processing (3)
lib/ksef_hub/service_config.exlib/ksef_hub_web/live/settings_live/services.extest/ksef_hub_web/live/settings_live/services_test.exs
Comment on lines
+248
to
+255
| def handle_event("validate", %{"classifier" => params}, socket) do | ||
| changeset = | ||
| socket.assigns.config | ||
| |> ServiceConfig.change_classifier_config(params) | ||
| |> Map.put(:action, :validate) | ||
|
|
||
| {:noreply, assign(socket, form: to_form(changeset, as: :classifier))} | ||
| end |
There was a problem hiding this comment.
Clear the pending confirmation state when the form changes.
After a failed pre-save probe, pending_params still holds the old submission. If the user edits the form, the warning remains visible and "Save anyway" will persist the stale params from the previous attempt, not the current form state.
Possible fix
def handle_event("validate", %{"classifier" => params}, socket) do
changeset =
socket.assigns.config
|> ServiceConfig.change_classifier_config(params)
|> Map.put(:action, :validate)
- {:noreply, assign(socket, form: to_form(changeset, as: :classifier))}
+ {:noreply,
+ assign(socket,
+ form: to_form(changeset, as: :classifier),
+ confirm_save: false,
+ pending_params: nil
+ )}
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub_web/live/settings_live/services.ex` around lines 248 - 255, In
handle_event("validate", %{"classifier" => params}, socket) clear any pending
confirmation state so stale pending_params aren't used: when building the
changeset and assigning form (to_form(changeset, as: :classifier)) also reset
pending_params (set to nil or %{}) and pending_confirmation (false) on the
socket assigns; update the assign(socket, ...) call to include those keys so
editing the form removes the previous failed pre-save probe state before
validation.
- Add host_allowed?/1 SSRF guard to check_url_health that rejects loopback, RFC1918, link-local, carrier-grade NAT, and IPv6 private addresses before making HTTP requests; disable redirect following - check_health button now probes the URL from the live form, not the persisted config - validate event clears pending_params and confirm_save so editing the form after a failed probe doesn't leave stale state - Preserve "" api_token in save params so the clear-token path in update_classifier_config/3 remains reachable - Add LiveView test for failed-probe → edit → resubmit flow Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract resolve_ip/1 helper so resolved_ip_allowed?/1 stays within credo's max nesting depth of 2. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/live/settings_live/services.ex`:
- Around line 254-259: The form-change branches should clear any in-flight
health probe so stale replies aren't applied; update the assigns where you clear
the form/pending_params/confirm_save (e.g., the assign call that sets form:
to_form(changeset, as: :classifier), pending_params: nil, confirm_save: false)
to also set active_health_ref: nil, and make the same change in the other
similar block (the one around lines 314-339) so handle_info/2 no longer treats
old probe replies as current.
- Around line 336-339: The current branch lets any non-:ok health_result flow to
the "Save anyway" override; change the case on health_result in the function
handling the save so that disallowed internal targets cannot be overridden: keep
the :ok -> do_save(socket, params) path, add an explicit clause for the
disallowed/internal-target result (e.g., :disallowed_target or whatever sentinel
the health check returns) that returns {:noreply, assign(socket, confirm_save:
false, pending_params: nil, error: :disallowed_target)} (or similar) and only
allow confirm_save: true for recoverable errors (e.g., network/timeouts). Use
host_allowed?/1 earlier or in that branch to double-check the URL and ensure
do_save, confirm_save and pending_params are not set when the host is
private/loopback.
- Around line 447-463: The current host validation uses :inet.getaddr/2 (in the
block where host = String.to_charlist(uri.host) and case :inet.getaddr(host,
:inet) do) which only checks a single resolved IP; change this to use
:inet.getaddrs/2 (or inet_res:lookup/3) to obtain all A/AAAA addresses for the
host and then iterate over the full list, calling ip4_private?/1 or
ip6_private?/1 for each address; if any resolved address is private/internal,
return {:error, :disallowed_target}, otherwise return :ok. Ensure both IPv4 and
IPv6 lists are fully validated and reuse the existing ip4_private?/1 and
ip6_private?/1 helpers and the same uri.scheme/uri.host checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 733acbfd-3cd5-4ad6-8d3d-32610d872835
📒 Files selected for processing (2)
lib/ksef_hub_web/live/settings_live/services.extest/ksef_hub_web/live/settings_live/services_test.exs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ksef_hub_web/live/settings_live/services_test.exs
Comment on lines
+254
to
+259
| {:noreply, | ||
| assign(socket, | ||
| form: to_form(changeset, as: :classifier), | ||
| pending_params: nil, | ||
| confirm_save: false | ||
| )} |
There was a problem hiding this comment.
Invalidate the active probe when the form changes.
validate clears the confirmation state, but it leaves active_health_ref intact. If an earlier "save" or "check_health" task finishes after the user edits the form, handle_info/2 still treats that reply as current and can either repopulate the warning with old params or call do_save/2 for the previous submission.
Suggested fix
{:noreply,
assign(socket,
form: to_form(changeset, as: :classifier),
+ health: nil,
+ active_health_ref: nil,
pending_params: nil,
confirm_save: false
)}Also applies to: 314-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub_web/live/settings_live/services.ex` around lines 254 - 259, The
form-change branches should clear any in-flight health probe so stale replies
aren't applied; update the assigns where you clear the
form/pending_params/confirm_save (e.g., the assign call that sets form:
to_form(changeset, as: :classifier), pending_params: nil, confirm_save: false)
to also set active_health_ref: nil, and make the same change in the other
similar block (the one around lines 314-339) so handle_info/2 no longer treats
old probe replies as current.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
lib/ksef_hub_web/live/settings_live/services.ex (3)
336-339:⚠️ Potential issue | 🟠 MajorDo not offer
Save anywayfor blocked targets.
{:error, :disallowed_target}is a hard validation failure, not a transient outage. This branch currently lets a private/loopback URL be persisted by clicking Save anyway.Possible fix
case health_result do :ok -> do_save(socket, params) - _error -> {:noreply, assign(socket, confirm_save: true, pending_params: params)} + {:error, :disallowed_target} -> + changeset = + socket.assigns.config + |> ServiceConfig.change_classifier_config(params) + |> Ecto.Changeset.add_error(:url, "must point to a public host") + + {:noreply, + assign(socket, + form: to_form(changeset, as: :classifier), + confirm_save: false, + pending_params: nil + )} + + _error -> + {:noreply, assign(socket, confirm_save: true, pending_params: params)} end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/settings_live/services.ex` around lines 336 - 339, The current case on health_result treats all non-:ok results the same and sets confirm_save to true, which allows a "Save anyway" for hard validation failures; update the case to specially handle the {:error, :disallowed_target} result (from health_result) so it does not set confirm_save or allow do_save — instead assign an appropriate error state on the socket (e.g. set an error message or validation flag) and return {:noreply, socket} for that branch; keep the existing behavior for transient errors (the fallback branch) to set confirm_save: true and pending_params as before.
254-259:⚠️ Potential issue | 🟠 MajorInvalidate the in-flight probe when the form changes.
Clearing only
pending_params/confirm_savestill leaves the previousactive_health_reflive. If an earlier"save"probe returns after the user edits the form,handle_info/2can still apply or even persist the old params.Possible fix
{:noreply, assign(socket, form: to_form(changeset, as: :classifier), + health: nil, + active_health_ref: nil, pending_params: nil, confirm_save: false )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/settings_live/services.ex` around lines 254 - 259, When resetting the form (the assign(...) call that sets form, pending_params and confirm_save), also invalidate any in-flight probe by clearing the live reference stored as active_health_ref and cancelling any monitor/timer for it; specifically set active_health_ref: nil in that assign and if you store a monitor or timer ref (the value used to track the in-flight "save" probe), call Process.demonitor(monitor_ref, [:flush]) or :erlang.cancel_timer(timer_ref) before clearing so a late handle_info/2 for the old probe cannot apply or persist stale params.
456-478:⚠️ Potential issue | 🔴 CriticalValidate every resolved A/AAAA record, not just the first one.
:inet.getaddr/2only checks one resolved IP. A hostname with mixed public/private DNS answers can passresolved_ip_allowed?/1while the HTTP client connects to a different private record, which reopens the SSRF path.Does Erlang/OTP `:inet.getaddr/2` return only a single resolved address, and should `:inet.getaddrs/2` (or `:inet_res.lookup/3`) be used to validate all A/AAAA records before allowing an HTTP target?
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/ksef_hub_web/live/settings_live/services.ex`:
- Around line 336-339: The current case on health_result treats all non-:ok
results the same and sets confirm_save to true, which allows a "Save anyway" for
hard validation failures; update the case to specially handle the {:error,
:disallowed_target} result (from health_result) so it does not set confirm_save
or allow do_save — instead assign an appropriate error state on the socket (e.g.
set an error message or validation flag) and return {:noreply, socket} for that
branch; keep the existing behavior for transient errors (the fallback branch) to
set confirm_save: true and pending_params as before.
- Around line 254-259: When resetting the form (the assign(...) call that sets
form, pending_params and confirm_save), also invalidate any in-flight probe by
clearing the live reference stored as active_health_ref and cancelling any
monitor/timer for it; specifically set active_health_ref: nil in that assign and
if you store a monitor or timer ref (the value used to track the in-flight
"save" probe), call Process.demonitor(monitor_ref, [:flush]) or
:erlang.cancel_timer(timer_ref) before clearing so a late handle_info/2 for the
old probe cannot apply or persist stale params.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b171058-8df0-45ea-ba2e-8acc731696ba
📒 Files selected for processing (1)
lib/ksef_hub_web/live/settings_live/services.ex
… IPs - Validate handler now nils active_health_ref so in-flight probe replies are discarded when the user edits the form - SSRF-blocked URLs (disallowed_target) show a flash error and never offer "Save anyway"; only recoverable errors allow the override - Resolve ALL A/AAAA records via :inet.getaddrs/2 and reject the host if any address is private - Update test to verify SSRF blocking behavior Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Key design decisions (ADR-0049)
runtime.exsconfig is untouched; DB overrides only apply when a company explicitly enables themNew files
lib/ksef_hub/service_config.exlib/ksef_hub/service_config/classifier_config.exlib/ksef_hub_web/live/settings_live/services.exdocs/adr/0049-per-company-classifier-config.mdTest plan
/c/:id/settings/services, toggle override, save with health check🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Permissions
Documentation
Tests