feat(provider): Akahu integration#1921
Open
failing wants to merge 12 commits into
Open
Conversation
# Conflicts: # config/locales/views/settings/en.yml
superagent-security
Bot
added
the
contributor:verified
Contributor
|
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 full Akahu provider integration: routes, controller flows (link/setup/sync), AkahuItem/AkahuAccount models, importer/processors for accounts and transactions, Provider::Akahu client and adapter, provider metadata change to multi-kind, DB migration/schema updates, reconciliation updates for Akahu pending flags, UI panels/partials, and tests. ChangesAkahu provider integration
Estimated code review effort: Possibly related issues:
Possibly related PRs:
Suggested reviewers:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
superagent-security
Bot
added
the
pr:flagged
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d67dceec04
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
test/models/provider/metadata_test.rb (1)
8-12: ⚡ Quick winAvoid hard-coding exclusivity for multi-kind providers.
This test encodes a product assumption (“only akahu can be multi-kind”) that will create false failures when another provider adds multiple kinds.
💡 Suggested patch
- test "akahu is the only provider with multiple kinds" do - providers_with_multiple_kinds = Provider::Metadata::REGISTRY.select { |_provider_key, metadata| metadata[:kinds].size > 1 } - - assert_equal [ :akahu ], providers_with_multiple_kinds.keys + test "akahu supports multiple kinds" do + providers_with_multiple_kinds = Provider::Metadata::REGISTRY.select { |_provider_key, metadata| metadata[:kinds].size > 1 } + + assert_includes providers_with_multiple_kinds.keys, :akahu end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/models/provider/metadata_test.rb` around lines 8 - 12, The test "akahu is the only provider with multiple kinds" hard-codes exclusivity; update it to assert that :akahu is present among providers with multiple kinds instead of being the sole one. Modify the assertion against Provider::Metadata::REGISTRY (the providers_with_multiple_kinds calculation) to use an inclusion assertion (e.g., assert_includes providers_with_multiple_kinds.keys, :akahu) and, if desired, also assert that providers_with_multiple_kinds is not empty to preserve the intent that multi-kind providers exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/akahu_items_controller.rb`:
- Around line 25-27: Replace the fallback assignment that uses ||= (which won't
override an empty string) by checking blank? on the model's name and assign a
localized default; specifically, after building `@akahu_item` via
Current.family.akahu_items.build(akahu_item_params) change the logic to set
`@akahu_item.name` = t('akahu_items.default_name') only when
`@akahu_item.name.blank`? so it covers empty strings and uses the existing i18n
key.
- Around line 44-52: The destroy action currently calls
`@akahu_item.unlink_all`!(dry_run: false) and proceeds to call
`@akahu_item.destroy_later` and redirect with a success notice even when unlinking
raises; change the rescue so that on any exception from unlink_all! you do NOT
call `@akahu_item.destroy_later` and instead redirect (or render) to
settings_providers_path with an error/alert message and an appropriate status
(e.g. :unprocessable_entity or :see_other) so the item is not destroyed when
unlinking fails; update the destroy method to only call destroy_later and
success redirect when unlink_all! completes without error and surface the
exception class/message (from e) in the error log and user-facing alert.
In `@app/models/akahu_account.rb`:
- Around line 47-51: The fallback literal "Akahu account" used when building
display_name should be replaced with an i18n lookup (use
t('akahu_account.fallback') or similar) so the user-facing string is localized;
update the display_name expression to call the t() helper instead of the
hard-coded string and add the corresponding key (e.g. akahu_account.fallback:
"Akahu account") to config/locales/en.yml. Ensure you use presence fallbacks
exactly as before (snapshot[:name].presence || connection[:name].presence ||
t(...)) and run tests/locale checks after adding the new key.
- Line 59: The account_id assignment currently forces a string by calling .to_s
on (snapshot[:_id].presence || snapshot[:id].presence), which converts nil to an
empty string; change the assignment for account_id to keep nil when no id is
present by removing the .to_s call so it uses (snapshot[:_id].presence ||
snapshot[:id].presence) directly; update the AkahuAccount creation/assignment
site where account_id is set to avoid persisting "" and rely on presence to
return nil when absent.
In `@app/models/akahu_item.rb`:
- Around line 87-99: The method sync_status_summary currently returns hard-coded
English strings; replace these with i18n lookups using the Rails t() helper and
pluralization/interpolation: call t('akahu_item.sync_status.no_accounts') when
total_accounts.zero?, t('akahu_item.sync_status.all_synced', count:
linked_count) for the linked-only branch (use Rails pluralization in the locale
file so %{count} and plural forms are handled), and
t('akahu_item.sync_status.partial', linked: linked_count, unlinked:
unlinked_count) for the else branch; update the corresponding locale keys
(akahu_item.sync_status.*) to include pluralization and interpolation, and make
the same change for the similar logic referenced around lines 124-133.
In `@app/models/akahu_item/importer.rb`:
- Around line 94-109: In fetch_pending_transactions_by_account, broaden the
exception handling so non-Akahu errors (at least JSON::ParserError and a generic
StandardError) are rescued and the method still returns the consistent shape {
success: false, by_account: Hash.new { |h,k| h[k] = [] }, error: e.message };
keep the existing Provider::Akahu::AkahuError rescue or replace it with a
multi-rescue clause, log the actual exception message (e.message) and class, and
ensure the by_account default uses the same Hash.new { |hash, key| hash[key] =
[] } construction used elsewhere.
- Around line 169-174: The current filtering in new_posted_transactions only
checks existing_posted_keys and allows duplicates within the same
posted_transactions batch; change the logic in the block that builds
new_posted_transactions (referencing posted_transactions,
transaction_storage_key, new_posted_transactions) to maintain a mutable
seen_keys set initialized from existing_posted_keys, and for each tx compute
key, skip if key.blank? or already in seen_keys, otherwise add the key to
seen_keys and accept the tx so duplicates in the same fetch are filtered out.
In `@app/models/family/akahu_connectable.rb`:
- Around line 12-15: The method create_akahu_item! currently hard-codes the
user-facing name "Akahu Connection"; update akahu_items.create! inside
create_akahu_item! to use I18n translation instead (e.g.,
I18n.t('<feature_namespace>.create_akahu_item.default_name')) and add the
corresponding key to your locale files organized hierarchically by feature
(e.g., family.akahu.create_akahu_item.default_name = "Akahu Connection") so
translations can be provided and maintained.
In `@app/models/provider/akahu_adapter.rb`:
- Around line 16-17: The provider card strings are hardcoded in the Akahu
adapter (the name and description entries) and must be replaced with i18n
lookups using the t() helper (e.g., use t('providers.akahu.name') and
t('providers.akahu.description') where the current name/description are set) and
add corresponding keys under a hierarchical locale namespace (providers -> akahu
-> name/description) in the locale YAML so translations load properly; update
any tests or fixtures that assert the literal strings to use the locale keys or
translated text.
In `@app/models/provider/akahu.rb`:
- Around line 12-19: The initialize method assigns `@base_url` directly which
risks SSRF; validate and normalize base_url (in initialize and any other places
that set `@base_url`) by parsing via URI, ensuring scheme is http or https and the
host exactly matches a trusted whitelist or falls under a trusted hostname
pattern, and if validation fails fallback to DEFAULT_BASE_URL or raise
AkahuError (e.g., use URI.parse(base_url.to_s.strip) and check
uri.scheme.in?(%w[http https]) and TRUSTED_HOSTS.include?(uri.host)); update the
`@base_url` assignment to use the validated value and raise
AkahuError.new("Invalid Akahu base_url", :configuration_error) if an explicit
invalid value is provided.
- Around line 164-177: The code currently includes raw provider payloads via
response.body in raised AkahuError messages and Rails.logger.error; remove
direct exposure of response.body and replace it with non-sensitive diagnostics
(e.g. response.code, response.message, and a short sanitized indicator or
truncated length/hash) when constructing errors or logs. Update the AkahuError
raises in the error handling block (references: AkahuError, response.body,
response.code, response.message, Rails.logger.error) to omit response.body, log
only minimal metadata, and if you need a trace for debugging produce a
sanitized/truncated or hashed version of the body instead of the raw payload.
In `@app/views/settings/providers/_akahu_panel.html.erb`:
- Line 6: The view is hardcoding the link label "Akahu" inside the link_to call;
replace that literal with a translation lookup so the panel is fully localized.
Update the call used within t("settings.providers.akahu_panel.step_1_html",
link: link_to(...)) to pass link_to(t("settings.providers.akahu") /* or an
appropriate i18n key with a default */, "https://my.akahu.nz/developers",
target: "_blank", rel: "noopener noreferrer", class: "text-primary font-medium
underline") so the link text comes from i18n rather than a hardcoded string.
In `@db/migrate/20260522120000_create_akahu_items_and_accounts.rb`:
- Line 11: The akahu_items.status column currently has a default but allows
NULL; update the migration so the schema enforces non-nullability by declaring
the column with null: false (e.g., in the create_table block for akahu_items
change the t.string :status line to include null: false) so the database
prevents NULLs for status and preserves sync/state integrity.
In `@db/schema.rb`:
- Line 1409: The unique index
idx_recurring_txns_on_family_merchant_amount_currency is too broad and blocks
valid recurring rows across different accounts; update the index definition to
scope uniqueness by account (e.g., include account_id in the indexed columns) or
remove the uniqueness constraint if per-account uniqueness isn't required —
locate the index named idx_recurring_txns_on_family_merchant_amount_currency and
change its columns to include account_id (or convert it to a non-unique index)
so duplicates are only prevented within the same account.
---
Nitpick comments:
In `@test/models/provider/metadata_test.rb`:
- Around line 8-12: The test "akahu is the only provider with multiple kinds"
hard-codes exclusivity; update it to assert that :akahu is present among
providers with multiple kinds instead of being the sole one. Modify the
assertion against Provider::Metadata::REGISTRY (the
providers_with_multiple_kinds calculation) to use an inclusion assertion (e.g.,
assert_includes providers_with_multiple_kinds.keys, :akahu) and, if desired,
also assert that providers_with_multiple_kinds is not empty to preserve the
intent that multi-kind providers exist.
🪄 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: b0ac9059-cbfd-4903-8077-f3ccf15cea87
📒 Files selected for processing (53)
app/components/settings/provider_card.rbapp/controllers/accounts_controller.rbapp/controllers/akahu_items_controller.rbapp/controllers/settings/providers_controller.rbapp/helpers/settings_helper.rbapp/javascript/controllers/providers_filter_controller.jsapp/models/account/provider_import_adapter.rbapp/models/akahu_account.rbapp/models/akahu_account/processor.rbapp/models/akahu_account/transactions/processor.rbapp/models/akahu_entry/processor.rbapp/models/akahu_item.rbapp/models/akahu_item/importer.rbapp/models/akahu_item/provided.rbapp/models/akahu_item/sync_complete_event.rbapp/models/akahu_item/syncer.rbapp/models/akahu_item/unlinking.rbapp/models/data_enrichment.rbapp/models/entry_search.rbapp/models/family.rbapp/models/family/akahu_connectable.rbapp/models/family/syncer.rbapp/models/provider/akahu.rbapp/models/provider/akahu_adapter.rbapp/models/provider/metadata.rbapp/models/provider_merchant.rbapp/models/transaction.rbapp/models/transaction/search.rbapp/models/user.rbapp/views/accounts/index.html.erbapp/views/akahu_items/_akahu_item.html.erbapp/views/akahu_items/select_accounts.html.erbapp/views/akahu_items/select_existing_account.html.erbapp/views/akahu_items/setup_accounts.html.erbapp/views/settings/providers/_akahu_panel.html.erbapp/views/settings/providers/show.html.erbconfig/locales/views/akahu_items/en.ymlconfig/locales/views/settings/en.ymlconfig/routes.rbdb/migrate/20260522120000_create_akahu_items_and_accounts.rbdb/schema.rbtest/components/settings/provider_card_test.rbtest/controllers/akahu_items_controller_test.rbtest/controllers/settings/providers_controller_test.rbtest/models/akahu_account/processor_test.rbtest/models/akahu_account_test.rbtest/models/akahu_entry/processor_test.rbtest/models/akahu_item/importer_test.rbtest/models/akahu_item/syncer_test.rbtest/models/family/syncer_test.rbtest/models/provider/akahu_adapter_test.rbtest/models/provider/akahu_test.rbtest/models/provider/metadata_test.rb
superagent-security
Bot
added
pr:verified
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/models/family/akahu_connectable.rb (1)
12-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat blank
item_namethe same as missing.
||preserves"", so callers that pass a blank string still skip the translated default and raise on the model validation instead.Suggested fix
akahu_item = akahu_items.create!( - name: item_name || I18n.t("family.akahu.create_akahu_item.default_name"), + name: item_name.presence || I18n.t("family.akahu.create_akahu_item.default_name"), app_token: app_token, user_token: user_token )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/family/akahu_connectable.rb` around lines 12 - 16, The current create_akahu_item! method treats a blank string as a provided name because it uses ||, so update the name assignment to treat blank strings as missing by using Rails presence (e.g., use item_name.presence or equivalent) when building the akahu_items.create! call in create_akahu_item! so the I18n.t("family.akahu.create_akahu_item.default_name") is used for nil or blank names; leave the rest of the method and akahu_items.create! call unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/akahu_items_controller.rb`:
- Around line 48-50: In the rescue block in the destroy action (the rescue => e
handling around Akahu unlink), stop exposing e.message in the user-facing flash:
keep the detailed info in the server log via Rails.logger.warn("Akahu unlink
during destroy failed: #{e.class} - #{e.message}") but change the redirect_to
call to use a generic/localized alert (e.g. t(".unlink_failed") without the
error interpolation) or a dedicated generic key, so no exception class/message
is rendered to users.
---
Duplicate comments:
In `@app/models/family/akahu_connectable.rb`:
- Around line 12-16: The current create_akahu_item! method treats a blank string
as a provided name because it uses ||, so update the name assignment to treat
blank strings as missing by using Rails presence (e.g., use item_name.presence
or equivalent) when building the akahu_items.create! call in create_akahu_item!
so the I18n.t("family.akahu.create_akahu_item.default_name") is used for nil or
blank names; leave the rest of the method and akahu_items.create! call
unchanged.
🪄 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: f48a5b2b-1ec4-4661-b938-67a0695e8302
📒 Files selected for processing (15)
app/controllers/akahu_items_controller.rbapp/models/akahu_account.rbapp/models/akahu_item.rbapp/models/akahu_item/importer.rbapp/models/akahu_item/provided.rbapp/models/family/akahu_connectable.rbapp/models/provider/akahu.rbapp/models/provider/akahu_adapter.rbapp/models/provider_connection_status.rbapp/views/settings/providers/_akahu_panel.html.erbconfig/locales/views/akahu_items/en.ymldb/migrate/20260522120000_create_akahu_items_and_accounts.rbdb/migrate/20260522121000_remove_legacy_recurring_transactions_family_unique_index.rbdb/schema.rbtest/controllers/akahu_items_controller_test.rb
✅ Files skipped from review due to trivial changes (1)
- config/locales/views/akahu_items/en.yml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/controllers/akahu_items_controller.rb (1)
227-229: 💤 Low valueConsider the same pattern here for consistency.
Line 229 interpolates
e.messageinto the user-facing alert, similar to what was just fixed in thedestroyaction. While ActiveRecord validation messages are often user-appropriate, they can sometimes expose model/attribute names that aren't user-friendly.Suggested fix
rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved => e Rails.logger.error("Akahu account setup failed: #{e.class} - #{e.message}") - redirect_to accounts_path, alert: t(".creation_failed", error: e.message), status: :see_other + redirect_to accounts_path, alert: t(".creation_failed"), status: :see_other end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/akahu_items_controller.rb` around lines 227 - 229, The rescue block in AkahuItemsController that currently does rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved => e and redirects with alert: t(".creation_failed", error: e.message) should be changed to log the full error (include e.class and e.backtrace) via Rails.logger.error and stop interpolating raw e.message into the user-facing alert; instead pass a generic/sanitized I18n message (e.g. t(".creation_failed") or t(".creation_failed", error: some_sanitized_text)) so users see a friendly message while developers get the detailed error in the logs—update the rescue in the create/account-setup flow (the rescue block in AkahuItemsController) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/controllers/akahu_items_controller.rb`:
- Around line 227-229: The rescue block in AkahuItemsController that currently
does rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved => e and
redirects with alert: t(".creation_failed", error: e.message) should be changed
to log the full error (include e.class and e.backtrace) via Rails.logger.error
and stop interpolating raw e.message into the user-facing alert; instead pass a
generic/sanitized I18n message (e.g. t(".creation_failed") or
t(".creation_failed", error: some_sanitized_text)) so users see a friendly
message while developers get the detailed error in the logs—update the rescue in
the create/account-setup flow (the rescue block in AkahuItemsController)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2825b34f-b8f6-46da-8e3e-5c1704599978
📒 Files selected for processing (2)
app/controllers/akahu_items_controller.rbconfig/locales/views/akahu_items/en.yml
✅ Files skipped from review due to trivial changes (1)
- config/locales/views/akahu_items/en.yml
Contributor
|
@failing @jjmata hi there, some suggestions for this PR after running an analysis from my end: Concrete blockers:
Suggested validation:
It's also quite a large PR (55 files changed) - I've personally submitted a few large ones myself, so I understand the approach of trying to keep complete featuresets together, but it can make review a lot harder - in the future, I suggest breaking apart into smaller, more palatable bites. Happy to help answer any questions or thoughts - cheers 👍🏻 |
Author
Thanks for taking a look!
Added the relevant tests. Acknowledge it's quite a big PR. Will take on your feedback for splitting it up next time. Cheers! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/controllers/akahu_items_controller.rb (1)
63-64:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThese flows still collapse multiple Akahu connections into one.
credentialed_akahu_itemjust grabs the first active credentialed item in the family, sopreload_accounts,select_accounts, andlink_accountscan operate on the wrong connection as soon as a family has more than one Akahu item. These endpoints need an explicitakahu_item_idand should load that specific record fromCurrent.family.Also applies to: 73-74, 90-90, 249-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/akahu_items_controller.rb` around lines 63 - 64, The endpoints are incorrectly using credentialed_akahu_item (which returns the first active item) causing multiple Akahu connections to collapse; change these flows (preload_accounts, select_accounts, link_accounts and the occurrences at the other mentioned locations) to require an akahu_item_id param and load the specific AkahuItem from Current.family (e.g. Current.family.akahu_items.find_by!(id: params[:akahu_item_id])) instead of calling credentialed_akahu_item, validate existence and credentials on that loaded record, and use that instance for all subsequent operations and responses.app/models/akahu_entry/processor.rb (1)
121-129:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the note labels written into transaction notes.
These prefixes are persisted and surfaced back to users, so they should not be hard-coded English strings.
As per coding guidelines, "Always use
t()helper for user-facing strings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/akahu_entry/processor.rb` around lines 121 - 129, The notes method currently hard-codes English prefixes ("Reference:", "Particulars:", "Code:", "Other account:"); change those to use Rails i18n by replacing each literal with t(...) calls (e.g. t('akahu_entry.notes.reference'), t('akahu_entry.notes.particulars'), t('akahu_entry.notes.code'), t('akahu_entry.notes.other_account')) inside the parts array in the notes method, leaving data/meta interpolation unchanged, and add corresponding keys to the locale YAML so translations are available; keep the existing presence/join logic and ensure you call t() with appropriate scope/namespace matching other akahu_entry translations.app/models/akahu_item.rb (1)
49-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize failed processor results before returning them.
This path still forwards
data[:error]/errorsand the fullresulthash back to callers. If a processor packs raw provider payloads or secret-bearing messages into its failure result, that bypasses the generic exception handling you added in the rescue branch. Return a generic translated error here and keep the detailed payload server-side only.Also applies to: 140-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/akahu_item.rb` around lines 49 - 50, The branch in AkahuItem that returns raw failure payloads (the Hash path checking result.with_indifferent_access[:success] == false) must not forward result or provider error fields; instead return only akahu_account_id, success: false and a generic translated error message (e.g. via I18n.t or a dedicated processor failure message) and remove or omit the full result/errors field. Update the same pattern in the other failing-return block referenced (around lines 140–150) so both places sanitize processor results and keep detailed payloads server-side only.
♻️ Duplicate comments (1)
app/models/akahu_item/unlinking.rb (1)
21-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t swallow unlink failures here.
AkahuItemsController#destroyonly stops destroying the item whenunlink_all!raises, but this rescue converts every unlink error intoresult[:error]and keeps going. That means partial unlink failures still end indestroy_laterplus a success notice, leaving cleanup incomplete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/akahu_item/unlinking.rb` around lines 21 - 34, The current rescue in the unlinking logic swallows errors so AkahuItemsController#destroy never sees failures; change the handler so unlink failures re-raise instead of silently continuing: in the method that runs the ActiveRecord::Base.transaction / links.each loop (the unlink_all! logic in unlinking.rb), keep the logging of the exception (Rails.logger.warn with provider_account.id and link_ids) but after setting result[:error] either raise the original exception (raise e) or remove the rescue entirely so the error bubbles to AkahuItemsController#destroy; this ensures unlink_all! failures stop destruction and are handled by the controller.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/akahu_account/processor.rb`:
- Around line 45-46: The current log in AkahuAccount::Processor includes
e.message which can leak provider details; change the Rails.logger.error call to
log only stable metadata (e.g., akahu_account.id and the exception class) and
remove e.message, and separately send the full exception to your error tracker
(e.g., call Sentry.capture_exception(e, extra: { akahu_account_id:
akahu_account.id }) or Raven.capture_exception with the akahu_account id) so raw
details go to Sentry only while logs contain non-sensitive context.
In `@app/models/akahu_item/syncer.rb`:
- Around line 62-63: Remove the raw exception text from the syncer logs: in
AkahuItem::Syncer replace the Rails.logger.error interpolation that includes
e.message with a generic message (e.g., "AkahuItem::Syncer - Unexpected sync
error: #{e.class}") and rely on your error-tracking (capture_exception) for full
details; keep the collect_health_stats(sync, errors: [...]) call as-is so
monitoring records the failure without exposing provider payloads.
---
Outside diff comments:
In `@app/controllers/akahu_items_controller.rb`:
- Around line 63-64: The endpoints are incorrectly using credentialed_akahu_item
(which returns the first active item) causing multiple Akahu connections to
collapse; change these flows (preload_accounts, select_accounts, link_accounts
and the occurrences at the other mentioned locations) to require an
akahu_item_id param and load the specific AkahuItem from Current.family (e.g.
Current.family.akahu_items.find_by!(id: params[:akahu_item_id])) instead of
calling credentialed_akahu_item, validate existence and credentials on that
loaded record, and use that instance for all subsequent operations and
responses.
In `@app/models/akahu_entry/processor.rb`:
- Around line 121-129: The notes method currently hard-codes English prefixes
("Reference:", "Particulars:", "Code:", "Other account:"); change those to use
Rails i18n by replacing each literal with t(...) calls (e.g.
t('akahu_entry.notes.reference'), t('akahu_entry.notes.particulars'),
t('akahu_entry.notes.code'), t('akahu_entry.notes.other_account')) inside the
parts array in the notes method, leaving data/meta interpolation unchanged, and
add corresponding keys to the locale YAML so translations are available; keep
the existing presence/join logic and ensure you call t() with appropriate
scope/namespace matching other akahu_entry translations.
In `@app/models/akahu_item.rb`:
- Around line 49-50: The branch in AkahuItem that returns raw failure payloads
(the Hash path checking result.with_indifferent_access[:success] == false) must
not forward result or provider error fields; instead return only
akahu_account_id, success: false and a generic translated error message (e.g.
via I18n.t or a dedicated processor failure message) and remove or omit the full
result/errors field. Update the same pattern in the other failing-return block
referenced (around lines 140–150) so both places sanitize processor results and
keep detailed payloads server-side only.
---
Duplicate comments:
In `@app/models/akahu_item/unlinking.rb`:
- Around line 21-34: The current rescue in the unlinking logic swallows errors
so AkahuItemsController#destroy never sees failures; change the handler so
unlink failures re-raise instead of silently continuing: in the method that runs
the ActiveRecord::Base.transaction / links.each loop (the unlink_all! logic in
unlinking.rb), keep the logging of the exception (Rails.logger.warn with
provider_account.id and link_ids) but after setting result[:error] either raise
the original exception (raise e) or remove the rescue entirely so the error
bubbles to AkahuItemsController#destroy; this ensures unlink_all! failures stop
destruction and are handled by the controller.
🪄 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: 1ba2242e-498e-4b6c-bb9b-ea7108edb8fe
📒 Files selected for processing (16)
app/controllers/akahu_items_controller.rbapp/models/akahu_account/processor.rbapp/models/akahu_account/transactions/processor.rbapp/models/akahu_entry/processor.rbapp/models/akahu_item.rbapp/models/akahu_item/importer.rbapp/models/akahu_item/syncer.rbapp/models/akahu_item/unlinking.rbapp/models/provider/akahu.rbapp/views/akahu_items/_akahu_item.html.erbapp/views/settings/providers/_akahu_panel.html.erbconfig/locales/views/akahu_items/en.ymltest/controllers/akahu_items_controller_test.rbtest/models/akahu_account_test.rbtest/models/akahu_item_importer_error_messages_test.rbtest/models/akahu_item_unlinking_test.rb
✅ Files skipped from review due to trivial changes (1)
- config/locales/views/akahu_items/en.yml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/akahu_item.rb (1)
47-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep raw processor messages out of the logs too.
The returned hash is now sanitized, but Line 55 still logs
e.message. In the same failure cases these tests model, that message can contain tokens or account numbers, so the sensitive data still lands in server logs. Log stable context only (AkahuItem/account ids + exception class) and keep the raw message out of normal logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/akahu_item.rb` around lines 47 - 56, The rescue block inside AkahuItem's akahu_accounts processing currently logs e.message which may contain sensitive tokens; update the rescue in the AkahuItem loop (where AkahuAccount::Processor.new(...).process is called) to log only stable context (AkahuItem id, akahu_account.id) and the exception class (e.class) — remove e.message from the Rails.logger.error call — and keep the returned sanitized hash unchanged.
🧹 Nitpick comments (1)
test/controllers/akahu_items_controller_test.rb (1)
112-139: ⚡ Quick winAdd external
return_torejection coverage forlink_accounts.This test validates connection selection correctly, but
link_accountsalso receivesreturn_tofrom the setup flow. Add one regression asserting absolute external URLs (for example,https://evil.example/accounts) are rejected and redirect falls back toaccounts_path.Proposed test addition
+ test "link accounts rejects absolute external return paths" do + second_item = AkahuItem.create!( + family: `@family`, + name: "Secondary Akahu", + app_token: "second-app-token", + user_token: "second-user-token" + ) + second_account = second_item.akahu_accounts.create!( + name: "Secondary Checking", + account_id: "acc_secondary", + currency: "NZD" + ) + + post link_accounts_akahu_items_url, params: { + akahu_item_id: second_item.id, + account_ids: [ second_account.id ], + accountable_type: "Depository", + return_to: "https://evil.example/accounts" + } + + assert_redirected_to accounts_path + endAs per coding guidelines: "Write minimal, effective tests. Only test critical and important code paths."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/akahu_items_controller_test.rb` around lines 112 - 139, Update the "link accounts uses requested Akahu connection" test to also pass a malicious absolute external return_to (e.g. "https://evil.example/accounts") in the post to link_accounts_akahu_items_url and assert that the controller rejects it by redirecting to accounts_path instead of the external URL; keep the existing assertions that one Account and one AccountProvider are created and that AccountProvider.order(:created_at).last.provider_id equals second_account.id, and assert that `@akahu_account.reload.current_account` remains nil to cover the open-redirect fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/models/akahu_account_test.rb`:
- Around line 76-80: The failure stub for
AkahuAccount::Processor.any_instance.stubs(:process) currently omits a :result
key so the test's refute result.key?(:result) is vacuous; update the stubbed
return to include a representative sanitized result payload (e.g., a hash with
the expected keys/structure used by process_accounts) under the :result key so
the sanitization/stripping behavior is exercised—modify the return in the
AkahuAccount::Processor.any_instance.stubs(:process) call to include :result =>
{ ...representative sanitized fields... } alongside success: false, error:
raw_message, errors: [...].
---
Outside diff comments:
In `@app/models/akahu_item.rb`:
- Around line 47-56: The rescue block inside AkahuItem's akahu_accounts
processing currently logs e.message which may contain sensitive tokens; update
the rescue in the AkahuItem loop (where AkahuAccount::Processor.new(...).process
is called) to log only stable context (AkahuItem id, akahu_account.id) and the
exception class (e.class) — remove e.message from the Rails.logger.error call —
and keep the returned sanitized hash unchanged.
---
Nitpick comments:
In `@test/controllers/akahu_items_controller_test.rb`:
- Around line 112-139: Update the "link accounts uses requested Akahu
connection" test to also pass a malicious absolute external return_to (e.g.
"https://evil.example/accounts") in the post to link_accounts_akahu_items_url
and assert that the controller rejects it by redirecting to accounts_path
instead of the external URL; keep the existing assertions that one Account and
one AccountProvider are created and that
AccountProvider.order(:created_at).last.provider_id equals second_account.id,
and assert that `@akahu_account.reload.current_account` remains nil to cover the
open-redirect fallback behavior.
🪄 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: 3fec8495-adc8-4967-bef7-7d19382894b8
📒 Files selected for processing (14)
app/controllers/akahu_items_controller.rbapp/models/akahu_account/processor.rbapp/models/akahu_entry/processor.rbapp/models/akahu_item.rbapp/models/akahu_item/syncer.rbapp/models/provider/akahu_adapter.rbapp/views/akahu_items/select_accounts.html.erbconfig/locales/views/akahu_items/en.ymltest/controllers/akahu_items_controller_test.rbtest/models/akahu_account/processor_test.rbtest/models/akahu_account_test.rbtest/models/akahu_entry/processor_test.rbtest/models/akahu_item/syncer_test.rbtest/models/provider/akahu_adapter_test.rb
✅ Files skipped from review due to trivial changes (1)
- config/locales/views/akahu_items/en.yml
superagent-security
Bot
added
pr:flagged
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
jjmata
reviewed
May 24, 2026
jjmata
reviewed
May 24, 2026
superagent-security
Bot
added
pr:verified
superagent-security
Bot
added
pr:flagged
github-project-automation
Bot
moved this to Todo
in Next/future release train (v0.7.2) - June 2026
DS Drift Patrol — 1 finding
Rule 3 —
|
| t.jsonb :raw_payload | ||
| t.jsonb :raw_institution_payload | ||
| t.text :app_token | ||
| t.text :user_token |
There was a problem hiding this comment.
P2: Akahu API tokens are added as plaintext database columns
Akahu app/user tokens are stored in plain text columns.
Encrypt Akahu token fields and ensure they are never logged or rendered.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="db/migrate/20260522120000_create_akahu_items_and_accounts.rb">
<violation number="1" location="db/migrate/20260522120000_create_akahu_items_and_accounts.rb:18">
<priority>P2</priority>
<title>Akahu API tokens are added as plaintext database columns</title>
<evidence>The migration creates `akahu_items.app_token` and `akahu_items.user_token` as plain `text` columns. These tokens are used as Akahu credentials for financial-account access, so a database dump or SQL read would expose reusable provider credentials.</evidence>
<recommendation>Store Akahu credentials with the app's encrypted credential mechanism (for example Active Record encryption or the same encrypted field pattern used for other provider secrets), rotate any tokens created during testing, and avoid returning or logging these fields anywhere.</recommendation>
</violation>
</file>
|
Two blockers before this can move past 1. Open redirect (security) 2. DS Drift — Generated by Claude Code |
superagent-security
Bot
added
pr:verified
DS Drift Patrol — 2 findingsRule 4 — DS::Disclosure summary indentation (#1855 review) Two new
Fix: add <div class="flex items-center justify-between gap-2 w-full">Generated by Claude Code |
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
5 participants
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
Akahu is a NZ open banking aggregator with the ability to to connect to various banks and other financial institutions.
Changes
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests