Skip to content

[PM-40995] fix: Allow fill-assist to rescue autofill requests when heuristics find no fields - #7214

Open
aj-rosado wants to merge 1 commit into
mainfrom
PM-40995/fill-assist-fallback-when-no-heuristics-found
Open

[PM-40995] fix: Allow fill-assist to rescue autofill requests when heuristics find no fields#7214
aj-rosado wants to merge 1 commit into
mainfrom
PM-40995/fill-assist-fallback-when-no-heuristics-found

Conversation

@aj-rosado

Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-40995

📔 Objective

AutofillParserImpl.parseInternal returned AutofillRequest.Unfillable immediately whenever the built-in heuristic field-classifier found zero non-Unused fields on a page — before the app ever checked whether fill-assist (server-curated CSS-selector-based field targeting) was enabled or had matching rules for that host. This meant fill-assist could never rescue a page the heuristics failed to parse, defeating its purpose on pages designed to evade naive heuristics (e.g. generically-named, hint-less input fields). Confirmed via a live repro against a test page with no autofill suggestion offered at all.

This PR:

  • Lets parseInternal fall back to an Unused-inclusive candidate view when heuristics find nothing, but only when fill-assist is actually enabled (feature flag + setting) — the disabled path is unchanged in both behavior and cost.
  • Gives toEffectiveViews's Unused case (previously dead code, since a heuristic-classified focused view could never be Unused) real behavior: it checks against both login and card fill-assist categories, since heuristics gave no signal for which one applies.
  • Adds a conservative guard so fill-assist is only trusted for the Unused case when it actually matches a field on the page — an empty match falls back to heuristics instead of being treated as authoritative, so an unrelated host rule (e.g. one covering a different pathname/category on the same host) can't silently claim a page it has no real match for. The pre-existing Card/Login branches keep their existing "authoritative even if empty" behavior since those already carry real heuristic confirmation.
  • Fixes a pre-existing bug in the shared test setup()'s default toAutofillViewData mock stub, uncovered by the new tests: it used firstArg()/secondArg() for the autofillId/website params, but since it mocks an extension function, the receiver occupies argument position 0 at the JVM level, so the stub was assigning the ViewNode itself into the autofillId field. Fixed by shifting to secondArg()/thirdArg().
  • Adds test coverage for the rescue path, the conservative empty-match guard, and confirmation the rescue path stays inert when fill-assist is disabled.

@aj-rosado aj-rosado added the ai-review Request a Claude code review label Jul 27, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug labels Jul 27, 2026
@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the fill-assist rescue path added to AutofillParserImpl.parseInternal, the new toEffectiveViews handling for the Unused case, the supporting refactors (selectCandidateAutofillViews, firstFocusedOrNull), and the accompanying test changes. This is a security-sensitive autofill change at a trust boundary, so the analysis focused on whether the new path could cause credentials to be offered on the wrong page.

The rescue path is well-guarded: it activates only when the feature flag and user setting are both enabled, host rules exist for the resolved host, and fill-assist actually matches a field on the page. The conservative empty-match guard for the Unused case correctly prevents an unrelated host rule from silently claiming a form it has no real match for, while preserving the existing "authoritative even if empty" behavior for the heuristic-confirmed Card/Login branches. The test-stub fix (firstArg/secondArgsecondArg/thirdArg for a mocked extension function) is correct, and the three new tests cover the rescue path, the empty-match guard, and the disabled-feature inert path.

Code Review Details

No findings. The change is well-reasoned, conservatively guarded, and adequately tested.

Note: Codecov reports 0% patch coverage, but this appears to be an artifact of the heavy extension-function mocking in these tests rather than a genuine coverage gap — the new tests do exercise parseInternal end to end.

@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.42%. Comparing base (845e20b) to head (f62b943).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...twarden/data/autofill/parser/AutofillParserImpl.kt 96.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7214      +/-   ##
==========================================
- Coverage   86.19%   85.42%   -0.78%     
==========================================
  Files         888      962      +74     
  Lines       64776    66801    +2025     
  Branches     9691     9745      +54     
==========================================
+ Hits        55836    57065    +1229     
- Misses       5470     6260     +790     
- Partials     3470     3476       +6     
Flag Coverage Δ
app-data 17.42% <96.00%> (-0.29%) ⬇️
app-ui-auth-tools 18.61% <0.00%> (-0.14%) ⬇️
app-ui-platform 16.66% <0.00%> (+0.14%) ⬆️
app-ui-vault 27.89% <0.00%> (+0.56%) ⬆️
authenticator 6.13% <0.00%> (+<0.01%) ⬆️
lib-core-network-bridge 4.12% <0.00%> (-0.01%) ⬇️
lib-data-ui 1.18% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aj-rosado
aj-rosado marked this pull request as ready for review July 27, 2026 14:06
@aj-rosado
aj-rosado requested review from a team and david-livefront as code owners July 27, 2026 14:06
@aj-rosado aj-rosado added t:bug Change Type - Bug and removed t:bug Change Type - Bug labels Jul 27, 2026
is AutofillView.Login -> rule.category in LOGIN_FILL_ASSIST_CATEGORIES
is AutofillView.Unused -> false
is AutofillView.Unused -> rule.category in LOGIN_FILL_ASSIST_CATEGORIES ||
rule.category in CARD_FILL_ASSIST_CATEGORIES

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since this is a multi-line statement, can we make sure it is wrapped in curly braces:

is AutofillView.Unused -> {
    rule.category in LOGIN_FILL_ASSIST_CATEGORIES ||
        rule.category in CARD_FILL_ASSIST_CATEGORIES
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants