Repository navigation
Conversation
irfano
added
type: enhancement
|
|
github-actions
Bot
added
the
review: recommended
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review - Found 4 potential issues
The change itself is well structured: the first-message context build is extracted cleanly, the log snapshot is bounded (levels, 30-minute window, 20 lines, 500 chars/line, 4000 chars total), runs through SensitiveDataSanitizer plus the existing personal-data scrub, and carries a prompt-injection notice. Hilt wiring checks out (WooLog is provided by AppConfigModule, Clock by ApplicationModule), and the test coverage for the provider is thorough.
Findings (details inline):
[fix here]SensitiveDataSanitizer: the new invisible-character strip runs after the secret redactions, so a zero-width/format character inside a key name defeats the redaction and the strip then normalises the leaked value into clean text.[follow-up]WooLog.getCurrentLogEntries()only triggers a flush and does not wait for it, so the most recent entries (often the failure that brought the merchant to the chat) can be missing from the snapshot. Pre-existing, but this PR now depends on freshness.[nit]dateInYearOfallocates twoCalendars per entry for a whole day's log file; the current year can be computed once.[nit]COLON_SECRET_REGEXuses\S+and swallows trailing punctuation (the test encodeshunter2,becoming[redacted]with the comma gone).
PR housekeeping
- The PR title and description were not included in the staged review context, so the title/reasoning/issue-link and testing-instructions checks could not be performed this pass; please make sure the description links the issue and lists steps to start an AI chat from the login flow and confirm the
# Recent app logssection is present (and excerpt-free for the unexpected-response entry point). - Applied repo
AGENTS.mdguidance (Kotlin conventions, companion object placement, no!!, BDD test naming): no deviations found in this diff.
Automatic review · claude-fable-5-1 · Workflow run
How to reply to a finding
Reply on this review (or inline at the line the finding refers to) with one of:
@claude addressed- I made the change. Bot verifies against the next diff before marking resolved.@claude rejected: <reason>- Will not fix; reason gets quoted on the next review.@claude not-applicable- Finding does not apply (wrong file, already covered elsewhere, etc.).
The bot honours these on the next review pass.
| matchResult.value.replace(QUOTED_ARGUMENT_REGEX, "'[redacted]'") | ||
| } | ||
| .replace(WHITESPACE_REGEX, " ") | ||
| .replace(INVISIBLE_CHARACTER_REGEX, "") |
There was a problem hiding this comment.
AI Code Review [fix here]
Issue: The invisible-character strip is the last step, after every secret regex has already run. A key name containing a format/control character (e.g. pass\u200Bword=hunter2 or access_\u00ADtoken: xyz) does not match KEY_VALUE_SECRET_REGEX / COLON_SECRET_REGEX / JSON_SECRET_REGEX, and this final replace then turns it into a clean password=hunter2 in the output. Since this sanitizer now feeds app-log lines and store-response excerpts into an external AI service, the ordering undoes the protection it is meant to add.
Suggestion: Strip the invisible characters first, but keep line breaks so COOKIE_HEADER_REGEX ([^\n\r,]+) and STACK_TRACE_REGEX still see them, then run the redactions and the whitespace collapse:
return value.orEmpty()
.replace(INVISIBLE_CHARACTER_REGEX, "")
.replace(BEARER_TOKEN_REGEX, "Bearer [redacted]")
// ... existing redactions ...
.replace(WHITESPACE_REGEX, " ")
.trim()
// Cc minus the line/tab separators that later patterns rely on
private val INVISIBLE_CHARACTER_REGEX = Regex("""[\p{Cf}\p{Cs}]|[\p{Cc}&&[^\r\n\t]]""")And add a test such as given a zero-width character inside a secret key, when sanitizing, then the value is still redacted.
| private val KEY_VALUE_SECRET_REGEX = Regex( | ||
| """(?i)(^|[?&;\s])((?:$SECRET_KEYS)=)[^&;\s,"'}]+""" | ||
| ) | ||
| private val COLON_SECRET_REGEX = Regex("""(?i)\b((?:$SECRET_KEYS)\s*:\s*)(?:"[^"]*"|\S+)""") |
There was a problem hiding this comment.
AI Code Review [nit]
Issue: (?:"[^"]*"|\S+) consumes any trailing punctuation that touches the value, so password: hunter2, API Key: ... becomes password: [redacted] API Key: ... (the comma is lost, as the new test asserts). It is harmless for secrecy but makes redacted log lines harder to read, and it is inconsistent with the quoted branch, which keeps the comma.
Suggestion: Exclude the same separators the other patterns stop at, e.g. (?:"[^"]*"|[^\s,;]+), and update the expected string in SensitiveDataSanitizerTest to password: [redacted], API Key: [redacted], access_token:[redacted].
There was a problem hiding this comment.
@claude rejected: A secret can contain commas, so the value is hidden up to the next space, the same as Zendesk_SSR_Ability.
| suspend fun buildRecentAppLogs(keepStoreResponseExcerpts: Boolean): String? = withContext(dispatchers.io) { | ||
| val now = clock.millis() | ||
| val since = Date(now - RECENT_LOGS_PERIOD_MS) | ||
| val lines = wooLog.getCurrentLogEntries() |
There was a problem hiding this comment.
AI Code Review [follow-up]
Issue: WooLog.getCurrentLogEntries() calls fileLogger.forceFlush(), which only emits on writeTrigger and returns immediately; the actual write happens later in appCoroutineScope, and getCurrentLogFileContent() reads the file right away. Entries logged since the last periodic flush (up to 5 s in the foreground) can therefore be missing, and the most valuable line here is usually the error that just sent the merchant to the chat. This is pre-existing behaviour shared with EncryptedLogsFileProvider and the Zendesk logs, so it can be a separate PR, but this feature is the first that depends on the snapshot being current.
Suggestion: Make forceFlush() wait for the write to complete (e.g. send a CompletableDeferred alongside the trigger that processLogs completes, or expose a suspend fun flushAndRead() that drains the channel and writes synchronously before reading). Alternatively, have buildRecentAppLogs merge the still-buffered entries with the file contents.
There was a problem hiding this comment.
@claude rejected: By the time the merchant opens the chat and sends the first message, the error that led there is already in the file.
|
|
||
| // The log file doesn't store the year, so entries read back from today's file are dated 1970. | ||
| private fun LogEntry.dateInYearOf(now: Long): Date { | ||
| val year = Calendar.getInstance().apply { timeInMillis = now }.get(Calendar.YEAR) |
There was a problem hiding this comment.
AI Code Review [nit]
Issue: dateInYearOf is called once per entry in the filter, and each call creates two Calendar instances (the first only to read the current year). getCurrentLogEntries() returns the whole current log file, which can be thousands of entries, so this is avoidable churn on the IO dispatcher. There is also a harmless edge around New Year, where a Dec 31 entry read on Jan 1 gets dated a year ahead and is kept; worth a word in the comment so nobody "fixes" it later.
Suggestion: Compute the year once in buildRecentAppLogs and pass it in, e.g.:
val year = Calendar.getInstance().apply { timeInMillis = now }.get(Calendar.YEAR)
...
.filter { it.level in RECENT_LOG_LEVELS && !it.logDate.inYear(year).before(since) }
private fun Date.inYear(year: Int): Date =
Calendar.getInstance().apply { time = this@inYear; set(Calendar.YEAR, year) }.time
Description
Fixes WOOMOB-4227
Built on top of #16752. When a merchant starts an AI support chat during login, the first message now also sends recent lines from the app log under
# Recent app logs. Odie gets them as context, the merchant doesn't see them.What is sent:
How it is cleaned, following
Zendesk_SSR_Ability(fbhepr%2Skers%2Sjcpbz%2Sjc%2Qpbagrag%2Syvo%2Snv%2Sgbbyf%2Snovyvgl.mraqrfx%2Qffe.cuc%3Se%3Qs73nr645%23766-og):The shared sanitizer now has the same rules, so the store response excerpt from WOOMOB-3905 is cleaned the same way. The site address and last error in the login context now go through it too.
The chat opened from the "Unable to log in" alert already sends the store response excerpt with the error details, so it's cut from the log lines there.
Test Steps
Each scenario checks the first message the app sends to Odie. ApiFaker answers it with a bot reply that says what the request had.
Setup
adb push tunnel-503.json odie-logs-excerpt.json odie-logs.json odie-no-logs.json /data/local/tmp/.JSON files
tunnel-503.json, the error the Jetpack tunnel returns when the store answers with a page that isn't JSON:{"error":"unknown_error","message":"An unknown error occurred","data":{"status":503,"raw_body":"<html><head><title>Maintenance</title></head><body><h1>Briefly unavailable for scheduled maintenance. Check back in a minute.</h1></body></html>"}}odie-logs-excerpt.json:{"chat_id":1,"session_id":"test","bot_slug":"wpcom-mobile-woo-support","bot_version":"1","messages":[{"message_id":2,"role":"bot","content":"Odie received recent app logs with the store response excerpt"}]}odie-logs.json:{"chat_id":1,"session_id":"test","bot_slug":"wpcom-mobile-woo-support","bot_version":"1","messages":[{"message_id":2,"role":"bot","content":"Odie received recent app logs"}]}odie-no-logs.json:{"chat_id":1,"session_id":"test","bot_slug":"wpcom-mobile-woo-support","bot_version":"1","messages":[{"message_id":2,"role":"bot","content":"Odie received no recent app logs"}]}Odie commands
Store check command
"Unable to log in" alert
Odie received recent app logs.Help on the store list
This needs a store in the list whose site can't be reached, like an expired Jurassic Ninja site.
Odie received recent app logs with the store response excerpt.After login
Odie received no recent app logs.Images/gif
The bot replies are fake ApiFaker responses. They say whether the request had the recent app logs.
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.