feat(cart-recovery): add abandonment reason mapping in hooks#718
feat(cart-recovery): add abandonment reason mapping in hooks#718MonishJuspay wants to merge 1 commit into
Conversation
- Converts LLM-friendly labels to consistent snake_case identifiers before storing in Clairvoyance metadata, so Nautilus receives values suitable for database storage and querying.
WalkthroughA new mapping constant translates LLM-generated abandonment-reason display labels into database machine codes. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds normalization for Breeze Buddy abandoned checkout metadata so LLM-provided “abandonment reason” labels are converted to a consistent identifier before being persisted on the lead’s meta_data.outcome.
Changes:
- Introduces a display-label → machine-code mapping for
abandonment_reason. - Applies the mapping inside
UpdateOutcomeInDatabaseHookbefore writingfinal_dataintometa_data["outcome"]. - Adds debug/warning logs around mapping behavior.
| # Mapping from LLM-friendly display labels to database-friendly machine codes | ||
| # Used for abandoned checkout recovery "abandonment_reason" field | ||
| ABANDONMENT_REASON_DISPLAY_TO_CODE = { | ||
| # High-Intent Reasons | ||
| "Payment failed": "PAYMENT_FAILED", | ||
| "Payment method unavailable": "PAYMENT_METHOD_UNAVAILABLE", | ||
| "Technical issue": "TECHNICAL_ISSUE", |
There was a problem hiding this comment.
The PR description says the hook converts labels to consistent snake_case identifiers, but this mapping emits SCREAMING_SNAKE_CASE values (e.g., "PAYMENT_FAILED"). Please either change the mapped codes to the intended snake_case format, or update the PR description/comments/constant name to match the chosen convention so downstream consumers (Nautilus/db queries) are consistent.
| if display_value in ABANDONMENT_REASON_DISPLAY_TO_CODE: | ||
| machine_code = ABANDONMENT_REASON_DISPLAY_TO_CODE[display_value] | ||
| final_data["abandonment_reason"] = machine_code | ||
| logger.debug( | ||
| f"Mapped abandonment_reason from '{display_value}' to '{machine_code}' " | ||
| f"for lead {context.lead.id}" | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| f"Unknown abandonment_reason display value '{display_value}' " | ||
| f"for lead {context.lead.id}. Using as-is." |
There was a problem hiding this comment.
display_value comes directly from LLM args and may be non-string (or even unhashable like a list/dict). In that case display_value in ABANDONMENT_REASON_DISPLAY_TO_CODE will raise TypeError and prevent the DB update. Consider guarding with isinstance(display_value, str) (and optionally normalizing via .strip()), otherwise skip mapping/log a structured warning.
| if display_value in ABANDONMENT_REASON_DISPLAY_TO_CODE: | |
| machine_code = ABANDONMENT_REASON_DISPLAY_TO_CODE[display_value] | |
| final_data["abandonment_reason"] = machine_code | |
| logger.debug( | |
| f"Mapped abandonment_reason from '{display_value}' to '{machine_code}' " | |
| f"for lead {context.lead.id}" | |
| ) | |
| else: | |
| logger.warning( | |
| f"Unknown abandonment_reason display value '{display_value}' " | |
| f"for lead {context.lead.id}. Using as-is." | |
| if isinstance(display_value, str): | |
| normalized_display_value = display_value.strip() | |
| if normalized_display_value in ABANDONMENT_REASON_DISPLAY_TO_CODE: | |
| machine_code = ABANDONMENT_REASON_DISPLAY_TO_CODE[ | |
| normalized_display_value | |
| ] | |
| final_data["abandonment_reason"] = machine_code | |
| logger.debug( | |
| f"Mapped abandonment_reason from '{display_value}' to '{machine_code}' " | |
| f"for lead {context.lead.id}" | |
| ) | |
| else: | |
| logger.warning( | |
| f"Unknown abandonment_reason display value '{display_value}' " | |
| f"for lead {context.lead.id}. Using as-is." | |
| ) | |
| else: | |
| logger.warning( | |
| f"Invalid abandonment_reason type '{type(display_value).__name__}' " | |
| f"for lead {context.lead.id}. Skipping display-to-code mapping." |
| else: | ||
| logger.warning( | ||
| f"Unknown abandonment_reason display value '{display_value}' " | ||
| f"for lead {context.lead.id}. Using as-is." | ||
| ) |
There was a problem hiding this comment.
The else branch logs a warning for any non-matching value, which will also fire when the value is already a machine code (or is None/empty). This can create noisy logs during normal operation. Consider: (1) skipping mapping entirely when the value is falsy, and (2) treating values already in ABANDONMENT_REASON_DISPLAY_TO_CODE.values() as already-normalized (no warning), only warning for truly unknown non-empty strings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/ai/voice/agents/breeze_buddy/template/hooks.py (1)
1-457:⚠️ Potential issue | 🟡 MinorFix the Black formatting failure.
CI reports
black --check --diff .would reformat this file, so this PR is currently blocked on formatting. Please run Black with the repository configuration before merging. As per coding guidelines, format code using Black with line-length=88.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/ai/voice/agents/breeze_buddy/template/hooks.py` around lines 1 - 457, Run Black to fix formatting issues in this file and commit the changes: format the module containing classes UpdateOutcomeInDatabaseHook, ExternalHTTPHook, and HookRegistry using Black with the repository config (black --line-length=88 or simply run black .) so the file passes `black --check --diff .`; ensure the formatted file is added to the PR and pushed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/ai/voice/agents/breeze_buddy/template/hooks.py`:
- Around line 238-251: The current block that maps
final_data["abandonment_reason"] assumes a string and persists unknown/free-form
values; first guard the value type by checking
isinstance(final_data["abandonment_reason"], str) before doing the membership
test against ABANDONMENT_REASON_DISPLAY_TO_CODE, and if it's not a string or not
in the mapping, replace it with a safe sentinel (e.g., None or an explicit
"UNKNOWN_ABANDONMENT" marker) instead of leaving the original list/dict/string
to be persisted; update the logger calls in this branch to reflect
dropping/mapping to the sentinel and reference final_data,
ABANDONMENT_REASON_DISPLAY_TO_CODE, and context.lead.id so reviewers can find
the change.
---
Outside diff comments:
In `@app/ai/voice/agents/breeze_buddy/template/hooks.py`:
- Around line 1-457: Run Black to fix formatting issues in this file and commit
the changes: format the module containing classes UpdateOutcomeInDatabaseHook,
ExternalHTTPHook, and HookRegistry using Black with the repository config (black
--line-length=88 or simply run black .) so the file passes `black --check --diff
.`; ensure the formatted file is added to the PR and pushed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c4f5f20-36cf-48ef-a04c-5170767a48d6
📒 Files selected for processing (1)
app/ai/voice/agents/breeze_buddy/template/hooks.py
| if "abandonment_reason" in final_data: | ||
| display_value = final_data["abandonment_reason"] | ||
| if display_value in ABANDONMENT_REASON_DISPLAY_TO_CODE: | ||
| machine_code = ABANDONMENT_REASON_DISPLAY_TO_CODE[display_value] | ||
| final_data["abandonment_reason"] = machine_code | ||
| logger.debug( | ||
| f"Mapped abandonment_reason from '{display_value}' to '{machine_code}' " | ||
| f"for lead {context.lead.id}" | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| f"Unknown abandonment_reason display value '{display_value}' " | ||
| f"for lead {context.lead.id}. Using as-is." | ||
| ) |
There was a problem hiding this comment.
Validate before mapping and do not persist unknown LLM values as-is.
abandonment_reason is Any; a list/dict value can raise in the membership check, and unknown strings are logged then persisted by the metadata loop. Drop invalid/unknown values or map them to an explicit sentinel before storage.
Proposed validation guard
if "abandonment_reason" in final_data:
display_value = final_data["abandonment_reason"]
- if display_value in ABANDONMENT_REASON_DISPLAY_TO_CODE:
+ if not isinstance(display_value, str):
+ logger.warning(
+ f"Invalid abandonment_reason type for lead {context.lead.id}. "
+ "Dropping field."
+ )
+ final_data.pop("abandonment_reason", None)
+ elif display_value in ABANDONMENT_REASON_DISPLAY_TO_CODE:
machine_code = ABANDONMENT_REASON_DISPLAY_TO_CODE[display_value]
final_data["abandonment_reason"] = machine_code
logger.debug(
f"Mapped abandonment_reason from '{display_value}' to '{machine_code}' "
f"for lead {context.lead.id}"
)
+ elif display_value in set(ABANDONMENT_REASON_DISPLAY_TO_CODE.values()):
+ logger.debug(
+ f"abandonment_reason already normalized for lead {context.lead.id}"
+ )
else:
logger.warning(
- f"Unknown abandonment_reason display value '{display_value}' "
- f"for lead {context.lead.id}. Using as-is."
+ f"Unknown abandonment_reason value for lead {context.lead.id}. "
+ "Dropping field."
)
+ final_data.pop("abandonment_reason", None)Based on learnings, Breeze Buddy LLM payloads should be predetermined and must not persist sensitive or free-form data through this path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if "abandonment_reason" in final_data: | |
| display_value = final_data["abandonment_reason"] | |
| if display_value in ABANDONMENT_REASON_DISPLAY_TO_CODE: | |
| machine_code = ABANDONMENT_REASON_DISPLAY_TO_CODE[display_value] | |
| final_data["abandonment_reason"] = machine_code | |
| logger.debug( | |
| f"Mapped abandonment_reason from '{display_value}' to '{machine_code}' " | |
| f"for lead {context.lead.id}" | |
| ) | |
| else: | |
| logger.warning( | |
| f"Unknown abandonment_reason display value '{display_value}' " | |
| f"for lead {context.lead.id}. Using as-is." | |
| ) | |
| if "abandonment_reason" in final_data: | |
| display_value = final_data["abandonment_reason"] | |
| if not isinstance(display_value, str): | |
| logger.warning( | |
| f"Invalid abandonment_reason type for lead {context.lead.id}. " | |
| "Dropping field." | |
| ) | |
| final_data.pop("abandonment_reason", None) | |
| elif display_value in ABANDONMENT_REASON_DISPLAY_TO_CODE: | |
| machine_code = ABANDONMENT_REASON_DISPLAY_TO_CODE[display_value] | |
| final_data["abandonment_reason"] = machine_code | |
| logger.debug( | |
| f"Mapped abandonment_reason from '{display_value}' to '{machine_code}' " | |
| f"for lead {context.lead.id}" | |
| ) | |
| elif display_value in set(ABANDONMENT_REASON_DISPLAY_TO_CODE.values()): | |
| logger.debug( | |
| f"abandonment_reason already normalized for lead {context.lead.id}" | |
| ) | |
| else: | |
| logger.warning( | |
| f"Unknown abandonment_reason value for lead {context.lead.id}. " | |
| "Dropping field." | |
| ) | |
| final_data.pop("abandonment_reason", None) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/ai/voice/agents/breeze_buddy/template/hooks.py` around lines 238 - 251,
The current block that maps final_data["abandonment_reason"] assumes a string
and persists unknown/free-form values; first guard the value type by checking
isinstance(final_data["abandonment_reason"], str) before doing the membership
test against ABANDONMENT_REASON_DISPLAY_TO_CODE, and if it's not a string or not
in the mapping, replace it with a safe sentinel (e.g., None or an explicit
"UNKNOWN_ABANDONMENT" marker) instead of leaving the original list/dict/string
to be persisted; update the logger calls in this branch to reflect
dropping/mapping to the sentinel and reference final_data,
ABANDONMENT_REASON_DISPLAY_TO_CODE, and context.lead.id so reviewers can find
the change.
Summary by CodeRabbit