[PM-37255] feat: Integrate fill-assist targeting rules into autofill parser#7066
[PM-37255] feat: Integrate fill-assist targeting rules into autofill parser#7066aj-rosado wants to merge 33 commits into
Conversation
…assist-data-layer
…emoved unnecessary deserialization tests
…assist-data-layer
…ow updates Updating code to schema with required values
…assist-data-layer
aj-rosado
added
the
ai-review-vnext
github-actions
Bot
added
the
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the end-to-end integration of fill-assist targeting rules into Code Review DetailsNo new findings at or above the reporting threshold. Notes on previously raised threads (verified during this pass):
Verified behavior:
|
aj-rosado
changed the title
|
|
||
| // Get inline information if available | ||
| val isInlineAutofillEnabled = settingsRepository.isInlineAutofillEnabled | ||
| Timber.d("Autofill request isInlineEnabled=$isInlineAutofillEnabled -- ${fillRequest?.id}") |
There was a problem hiding this comment.
Why are we removing this?
There was a problem hiding this comment.
I have added a TON of debug logs. When removing probably removed this one with the extra ones by mistake. Will re-add
| * when the feature flag is enabled and the host rules cover the current partition type; | ||
| * otherwise returns the heuristic [autofillViews]. | ||
| */ | ||
| private fun resolveEffectiveViews( |
There was a problem hiding this comment.
What do you think about making this an extension function?
private fun List<AutofillView>.toEffectiveViews(
assistStructure: AssistStructure,
uri: String?,
focusedView: AutofillView,
urlBarWebsite: String?,
): List<AutofillView> {| ?.takeUnless { it.startsWith("androidapp://") } | ||
| ?.toUri() | ||
| ?.host | ||
| ?.takeIf { featureFlagManager.getFeatureFlag(FlagKey.FillAssistTargetingRules) } |
There was a problem hiding this comment.
Can we just return early instead of putting this check in the chain?
if (!featureFlagManager.getFeatureFlag(FlagKey.FillAssistTargetingRules)) {
return autofillViews
}There was a problem hiding this comment.
Next PR in chain will remove that. Are you ok if we keep it here for now to avoid more conflicts when merging?
There was a problem hiding this comment.
Why is it being removed in that PR?
There was a problem hiding this comment.
Moved the validation to outside this method
val effectiveViews = if (isFillAssistEnabled) {
resolveEffectiveViews(
assistStructure = assistStructure,
autofillViews = autofillViews,
uri = uri,
focusedView = focusedView,
urlBarWebsite = urlBarWebsite,
)
} else {
autofillViews
}
| val urlBarWebsite = traversalDataList | ||
| .flatMap { it.urlBarWebsites } | ||
| .firstOrNull() | ||
| val autofillViews = traversalDataList.toAutofillViews(urlBarWebsite = urlBarWebsite) |
There was a problem hiding this comment.
The new buildFillAssistViews is awfully similar to the existing logic for parsing the assist structure. It's hard for me to tell if it is possible but can we optimize this to parse the structure only once?
There was a problem hiding this comment.
It is possible but as It adds complexity to the traverse, not sure if it a real improvement.
I have created a draft PR with a possible solution for this.
#7144
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7066 +/- ##
==========================================
- Coverage 86.76% 86.16% -0.61%
==========================================
Files 868 913 +45
Lines 63975 65677 +1702
Branches 9585 9630 +45
==========================================
+ Hits 55511 56591 +1080
- Misses 5023 5637 +614
- Partials 3441 3449 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ?.host | ||
| ?.takeIf { featureFlagManager.getFeatureFlag(FlagKey.FillAssistTargetingRules) } | ||
| ?.let { host -> | ||
| fillAssistManager.getFillAssistRules()?.hostRules?.get(host.removePrefix("www.")) |
There was a problem hiding this comment.
🧹 DEBT: getFillAssistRules() is invoked on the autofill parse hot path (once per parse() — i.e. per field focus / fill request). Its implementation re-reads the SharedPreferences string and fully re-deserializes the entire rules blob every call:
// FillAssistDiskSourceImpl
override fun getFillAssistRules(serverUrl: String): FillAssistRules? =
getString(FILL_ASSIST_RULES_KEY.appendIdentifier(serverUrl))
?.let { json.decodeFromStringOrNull(it) }Since the cached blob can contain rules for many hosts, this JSON parse repeats on every autofill interaction with no in-memory memoization. Consider caching the deserialized FillAssistRules in memory (invalidated on sync) so the parse path avoids repeated full deserialization. Gated behind the feature flag, so not urgent — but worth addressing before broad rollout.
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 1m 20s —— View job
Verifying the caching update
- Reviewed the
cache fill assist rulescommit (1d3381f) - Confirmed the hot-path re-deserialization is resolved
- Checked test coverage
✅ Resolved. The DEBT concern is addressed. getFillAssistRules() now memoizes the deserialized FillAssistRules in the cachedRules field and short-circuits before touching disk (FillAssistManagerImpl.kt:146), so the full JSON blob is no longer re-parsed on every parse() call. Invalidation is wired correctly:
- reset on server-config change (
initblock,:78) - reset after a sync stores new rules (
:137)
Test coverage is solid — the three new cases cover the cache hit (disk read exactly = 1), re-read after server-config change, and returning the latest rules after a sync.
One carry-over note: this newly-added cachedRules var is written from three contexts (init/unconfined, sync/io, and getFillAssistRules on the binder thread) without @Volatile. That's the same publication concern already tracked in the sibling ♻️ DEBT thread on this field — marking @Volatile there would also cover this cache. No action needed here beyond that existing thread.
| } answers { | ||
| mutableFillAssistFlagFlow.value | ||
| } | ||
| every { getFeatureFlagFlow(FlagKey.ManageDevices) } returns mutableFillAssistFlagFlow |
There was a problem hiding this comment.
📝 SUGGESTED: This stub wires the fill-assist flow to FlagKey.ManageDevices, but AutofillParserImpl only ever calls getFeatureFlag(FlagKey.FillAssistTargetingRules) — never getFeatureFlagFlow. This looks like a copy-paste leftover; it's dead setup and misleadingly ties the test's flag flow to an unrelated flag. Recommend removing this line.
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 58s —— View job
Verified — FlagKey.ManageDevices copy-paste leftover resolved ✅
Confirmed the update. The stub now wires to the correct flag:
private val mutableFillAssistFlagFlow = MutableStateFlow(false)
private val featureFlagManager: FeatureFlagManager = mockk {
every {
getFeatureFlag(FlagKey.FillAssistTargetingRules)
} answers {
mutableFillAssistFlagFlow.value
}
every {
getFeatureFlagFlow(FlagKey.FillAssistTargetingRules)
} returns mutableFillAssistFlagFlow
}The misleading tie to the unrelated FlagKey.ManageDevices is gone, and the MutableStateFlow now cleanly backs the flag value that the tests toggle (mutableFillAssistFlagFlow.value = true at lines 1004/1064/1124/1197). Original concern resolved.
One residual nit (non-blocking): AutofillParserImpl only ever calls getFeatureFlag(...) — never getFeatureFlagFlow(...) (confirmed at AutofillParserImpl.kt:220). So the getFeatureFlagFlow stub (lines 94–95) is still unexercised setup and could be dropped. Purely cosmetic — no action required.
| import com.bitwarden.vault.FolderView | ||
| import com.x8bit.bitwarden.data.auth.datasource.disk.AuthDiskSource | ||
| import com.x8bit.bitwarden.data.auth.manager.UserLogoutManager | ||
| import com.x8bit.bitwarden.data.autofill.manager.FillAssistManager |
There was a problem hiding this comment.
📝 SUGGESTED: Import out of order — autofill.manager.FillAssistManager is placed between the two auth.manager imports (auth sorts before autofill). Run IDE optimize-imports so it lands after UserStateManager. Same misplacement exists in VaultManagerModule.kt. Minor (detekt ImportOrdering is disabled, so CI won't catch it) but deviates from the codebase's sorted-import convention.
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 2m 17s —— View job
Import fix review
- Read
VaultSyncManagerImpl.ktimports - Read
VaultManagerModule.ktimports - Assess whether the update was correct
- Fix the regression and push
f677f80 (Fixing fillAssistManager import) removed the FillAssistManager import entirely from VaultSyncManagerImpl.kt instead of reordering it. But FillAssistManager is still referenced at VaultSyncManagerImpl.kt:83 (constructor param) and :346 (fillAssistManager.syncIfNecessary()), and it lives in a different package (com.x8bit.bitwarden.data.autofill.manager) — so this would have broken compilation.
I pushed a fix (2219e6f) that:
VaultSyncManagerImpl.kt— re-added the import in its correctly sorted position (afterauth.repository.util.userSwitchingChangesFlow, beforeplatform.datasource.disk.SettingsDiskSource, sinceautofillsorts afterauthand beforeplatform).VaultManagerModule.kt— this file still had the import misplaced between thevault.manager.*imports (line 47). Moved it up to afterauth.manager.UserStateManager, matching the sorted convention.
Both imports now land where IDE optimize-imports would put them.
💡 Recommend re-running a local build (
./gradlew app:assembleStandardDebugor the compile task) before merging to confirm — the removed import would have surfaced as a hard compile error, so it's worth a green build check.
| private val LOGIN_FILL_ASSIST_CATEGORIES: List<String> = listOf( | ||
| "account-login", | ||
| "account-creation", | ||
| "account-update", | ||
| ) | ||
|
|
||
| /** | ||
| * A list of categories from Fill Assist that are used for [AutofillView.Card] | ||
| */ | ||
| private val CARD_FILL_ASSIST_CATEGORIES: List<String> = listOf( | ||
| "payment-card", | ||
| ) |
There was a problem hiding this comment.
❓ QUESTION: These hardcoded category allowlists are the gate that decides whether fill-assist replaces heuristics for a given partition. Any login-related category the CDN emits that isn't listed here will silently fall back to heuristics. Notably, FillAssistViewNodeExtensionsTest uses the category "account-registration" (with a newPassword field), which is not in LOGIN_FILL_ASSIST_CATEGORIES — so a real registration form served with that category would never trigger fill-assist even though the field mapping supports it.
Is the complete set of server categories confirmed to be exactly account-login/account-creation/account-update (+ payment-card)? If new categories can appear, consider deriving the partition mapping from the field keys present in the rule rather than a fixed category allowlist, to avoid silent gaps as the schema evolves.
There was a problem hiding this comment.
If a new category appears we will need to manually add it. As this falls back to heuristics is not a problem.
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 42s —— View job
Verified — hardcoded category allowlist behavior confirmed ✅
Confirmed the author's answer. The concern raised was that any login-related category the CDN emits that isn't in LOGIN_FILL_ASSIST_CATEGORIES / CARD_FILL_ASSIST_CATEGORIES would silently fall back to heuristics.
Re-reading the gating logic at AutofillParserImpl.kt:226-241, the answer holds:
- When
coversCurrentPartitionisfalse(i.e. an unlisted category),toEffectiveViewsreturnsthis— the heuristic autofill views. That's the existing, working production path, so an unrecognized category degrades gracefully rather than breaking autofill. Not a correctness issue. - Adding a new category is a one-line change to the relevant allowlist, which is easy to catch and extend when the schema evolves.
On the specific "account-registration" note from the original comment: I checked FillAssistViewNodeExtensionsTest.kt:114 — that category is only exercised against buildFillAssistViews / traverseForFillAssist (the field-mapping/traversal layer), not the toEffectiveViews category gate. So there's no test asserting fill-assist coverage for a category that the gate would reject; the two layers are tested independently and there's no misleading assertion. My original example conflated the two layers.
Resolving as answered — the fall-back-to-heuristics design is a deliberate and safe choice. 👍
|
|
||
| // In-memory cache to avoid re-deserializing the full rules blob on every autofill parse. | ||
| // Populated lazily on first read; updated in-place when a sync stores new rules. | ||
| private var cachedRules: FillAssistRules? = null |
There was a problem hiding this comment.
♻️ DEBT: cachedRules is a plain var written from three concurrency contexts without a memory barrier.
Details and fix
cachedRules is written from:
- the
initblock onunconfinedScope(cachedRules = nullon server-config change), sync()onioScope(cachedRules = nullafter storing new rules),getFillAssistRules()(?.also { cachedRules = it }), which runs on the binder thread that the autofill framework invokesparse()on.
Because the field is neither @Volatile nor guarded by a lock, a cachedRules = null reset performed on one thread may not be visible to the parser thread, so a parse can serve stale rules after a server-config change until the JVM happens to publish the write.
The impact is benign (transient stale rules or an extra disk read, no crash), but a cheap fix restores the intended invalidation semantics:
@Volatile
private var cachedRules: FillAssistRules? = nullThere was a problem hiding this comment.
@claude reverted this change, we are getting the data from sharedPreferences and not disk, this cache is not really necessary
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 56s —— View job
Agreed — reverting the cache is the right call ✅
- Re-read
FillAssistManagerImpl.getFillAssistRules()(post-revert) - Traced the read path into
FillAssistDiskSourceImpl - Re-evaluated the original DEBT concern
You're right, and thanks for pushing back — my original DEBT note conflated two different costs.
On disk I/O: getFillAssistRules() → FillAssistDiskSourceImpl.getFillAssistRules() → getString(...) (FillAssistDiskSourceImpl.kt:35). SharedPreferences loads its backing file into an in-memory map once and serves getString from there — so there's no per-call disk read. My phrasing "re-reads the SharedPreferences string" implied I/O that isn't happening. You're correct on that.
On the residual cost: the only thing that still repeats per call is the json.decodeFromStringOrNull(...) deserialization. But parse() fires per field-focus / fill request — a user-initiated interaction, not a tight loop — so a single JSON parse per autofill interaction is negligible in practice. The in-memory cache was buying a micro-optimization on a low-frequency path.
Net: reverting is the better trade-off. It removes the added complexity and eliminates the @Volatile/publication concern from the sibling ♻️ DEBT thread on cachedRules (that field no longer exists, so that thread is moot too). No action needed here.
If deserialization ever does show up on a profile once this is broadly rolled out, an in-memory cache can be reintroduced then — but it's not warranted now. Resolving as answered. 👍
The previous fix removed the FillAssistManager import from VaultSyncManagerImpl.kt entirely, breaking compilation since the class is still referenced. Re-add it in its correctly sorted position, and also correct the misplaced import in VaultManagerModule.kt. Co-authored-by: aj-rosado <aj-rosado@users.noreply.github.com>
This reverts commit 2219e6f.
| val HtmlInfo?.isInputField: Boolean get() = this?.tag == "input" | ||
|
|
||
| /** | ||
| * Whether this [HtmlInfo] matches the given [SelectorClause]. |
There was a problem hiding this comment.
The link here is broken, can we make it [FillAssistRules.SelectorClause]
| clause.name == null && | ||
| clause.type == null && | ||
| clause.role == null | ||
| fun hasAttr(key: String, value: String) = attrs.any { it.first == key && it.second == value } |
There was a problem hiding this comment.
The formatter really wants a newline before the inline function
| clause.type == null && | ||
| clause.role == null | ||
| fun hasAttr(key: String, value: String) = attrs.any { it.first == key && it.second == value } | ||
| return listOf( |
| val data = toAutofillViewData(autofillId = id, website = website) | ||
| matchingEntries.firstNotNullOfOrNull { (key, _) -> | ||
| key.toAutofillViewForFieldKey( | ||
| data, |
There was a problem hiding this comment.
This should either have a name or be 1 line.
key.toAutofillViewForFieldKey(data)OR
key.toAutofillViewForFieldKey(
data = data,
)| "cardholderName" -> AutofillView.Card.CardholderName(data = data) | ||
| "cardExpirationDate" -> AutofillView.Card.ExpirationDate(data = data) | ||
| "cardExpirationMonth" -> AutofillView.Card.ExpirationMonth(data = data, monthValue = null) | ||
| "cardExpirationYear" -> AutofillView.Card.ExpirationYear(data = data, yearValue = null) |
There was a problem hiding this comment.
Can we make some consts for these magic strings?
| clause.id to "id", | ||
| clause.name to "name", | ||
| clause.type to "type", | ||
| clause.role to "role", |
There was a problem hiding this comment.
Can we make these consts?
| private val authDiskSource: AuthDiskSource, | ||
| private val vaultDiskSource: VaultDiskSource, | ||
| private val vaultSdkSource: VaultSdkSource, | ||
| private val fillAssistManager: FillAssistManager, |
There was a problem hiding this comment.
❌ CRITICAL: FillAssistManager is referenced but not imported — this will not compile.
Details and fix
The most recent commit (643be12 "Revert 'Restore FillAssistManager import and fix import ordering'") removed the import com.x8bit.bitwarden.data.autofill.manager.FillAssistManager line from this file, but the constructor parameter here (line 83) and the fillAssistManager.syncIfNecessary() call (line 343) still reference the type.
FillAssistManager lives in com.x8bit.bitwarden.data.autofill.manager, a different package from this file's com.x8bit.bitwarden.data.vault.manager, so it cannot resolve without an explicit import — this is an unresolved reference / compile error.
Restore the import (in alphabetical position, after ...auth.repository.util.userSwitchingChangesFlow):
import com.x8bit.bitwarden.data.autofill.manager.FillAssistManagerThere was a problem hiding this comment.
Claude finished @david-livefront's task in 31s —— View job
Verified — FillAssistManager import restored ✅
- Read
VaultSyncManagerImpl.ktimports - Read
VaultManagerModule.ktimports - Confirmed both are correctly sorted
Confirmed — the CRITICAL compile issue is resolved.
VaultSyncManagerImpl.kt:23 — import com.x8bit.bitwarden.data.autofill.manager.FillAssistManager is present and correctly sorted (after auth.repository.util.userSwitchingChangesFlow, before platform.datasource.disk.SettingsDiskSource). The constructor param (:84) now resolves.
VaultManagerModule.kt:21 — same import present and correctly sorted (after auth.manager.UserStateManager, before platform.datasource.disk.SettingsDiskSource), matching the sorted convention flagged in the sibling thread.
Both land where IDE optimize-imports would put them. The unresolved-reference / compile error is gone. 👍
| import com.x8bit.bitwarden.data.vault.manager.VaultLockManagerImpl | ||
| import com.x8bit.bitwarden.data.vault.manager.VaultMigrationManager | ||
| import com.x8bit.bitwarden.data.vault.manager.VaultMigrationManagerImpl | ||
| import com.x8bit.bitwarden.data.autofill.manager.FillAssistManager |
There was a problem hiding this comment.
🎨 SUGGESTED: Import is out of alphabetical order — the same 643be12 revert moved it here, between the vault.manager.* imports.
Details
com.x8bit.bitwarden.data.autofill.manager.FillAssistManager belongs with the other data.auth/data.autofill imports near the top of the block (before data.platform.*), not between VaultMigrationManagerImpl and VaultSyncManager. This compiles fine (detekt ImportOrdering is disabled), but it's inconsistent with the rest of the file's ordering.
| return mapOf( | ||
| clause.id to HTML_ATTR_ID, | ||
| clause.name to HTML_ATTR_NAME, | ||
| clause.type to HTML_ATTR_TYPE, | ||
| clause.role to HTML_ATTR_ROLE, | ||
| ) | ||
| .all { (value, key) -> value == null || hasAttr(key, value) } |
There was a problem hiding this comment.
Map keyed on the clause attribute values silently drops constraints when two values collide.
Details and fix
The map keys here are the clause values (clause.id, clause.name, ...), not the attribute names. When two of these values are equal, the map collapses them and only the last entry survives — so one of the attribute checks is silently skipped.
This is not a contrived case: id and name are frequently identical in real HTML forms (e.g. <input id="email" name="email"> → selector input#email[name='email'] yields clause.id == "email" and clause.name == "email"). The map becomes {"email" -> "name"}, so the id constraint is never verified and a node with a matching name but a different id will incorrectly match.
Because matchesSelectorClause is documented as untestable in unit tests, this cannot be caught by the existing suite. Keying on the attribute name instead avoids the collapse:
fun matchesAttr(value: String?, key: String) = value == null || hasAttr(key, value)
return matchesAttr(clause.id, HTML_ATTR_ID) &&
matchesAttr(clause.name, HTML_ATTR_NAME) &&
matchesAttr(clause.type, HTML_ATTR_TYPE) &&
matchesAttr(clause.role, HTML_ATTR_ROLE)2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-37255
📔 Objective
Wires the fill-assist targeting-rules pipeline end-to-end into the Android autofill framework.
When the
FillAssistTargetingRulesfeature flag is enabled and rules are available for the current host, the autofill parser replaces heuristic field detection with site-specific CSS-selector-based matching. Unmatched nodes are excluded entirely — there is no heuristic fallback when rules are active.What's included:
FillAssistApi,FillAssistService,FillAssistManifestJson,FillAssistFormsJson: fetches a versioned manifest and forms JSON from the fill-assist CDN endpoint.FillAssistManagerImpl,FillAssistDiskSource: parses CSS selectors intoFillAssistRules(tag, id, name, type, role constraints per field), caches rules on disk, syncs on server-config change with a 6-hour re-fetch throttle.>>>Shadow DOM notation, space-separated CSS descendant selectors (split on whitespace outside[…]attribute brackets to preserve attribute values that contain spaces),#idshorthand, and[attr='value']/[attr="value"]attribute selectors.AutofillParserImpllooks up host rules for the focused view's URI and callsAssistStructure.buildFillAssistViewsto replace the heuristic view list when rules match.FillAssistViewNodeExtensions.traverseForFillAssisttraverses theAssistStructuretree;HtmlInfoExtensions.matchesSelectorClausedoes the per-node attribute comparison (kept inHtmlInfoExtensionsalongsidehints()sinceHtmlInfo.attributesusesandroid.util.Pairand is untestable in unit tests).FillAssistManagerTest,FillAssistServiceTest,FillAssistViewNodeExtensionsTest(all previously commented-out tests now passing viamockkStatic(HtmlInfo::matchesSelectorClause)), andAutofillParserTestsupdated for the new constructor parameters.