-
Notifications
You must be signed in to change notification settings - Fork 1
feat: vip_agentforce_post_ingestion_failed hook to catch failed ingestions.
#8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: vip-2448-ingest-api-vip_agentforce_transform_post-filter
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a failure handling mechanism for post ingestion by adding a new vip_agentforce_post_ingestion_failed action hook. The hook allows consumers to catch and respond to ingestion failures, specifically distinguishing between actual failures (transform errors or API errors) and intentional skips (filter rejections).
Key Changes:
- Added
Ingestion_Failuredata class to encapsulate failure information including failure code, post object, and WP_Error details - Implemented
vip_agentforce_post_ingestion_failedaction hook that fires only on actual failures (transform or API errors) - Enhanced error reporting with backtraces and detailed error data for debugging
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
vip-agentforce.php |
Added autoload for new Ingestion_Failure class |
modules/ingestion/class-ingestion-failure.php |
New data class encapsulating failure information with helper methods for failure type detection |
modules/ingestion/class-ingestion.php |
Added failure action firing, new send_to_api() method stub, and enhanced error handling with backtraces |
tests/phpunit/test-ingestion.php |
Added comprehensive tests for failure action behavior covering skip vs. failure scenarios, backtrace inclusion, and to_array() method |
tests/phpunit/test-ingestion-api-failure.php |
New test file for API failure scenarios using test double to simulate API errors |
tests/phpunit/doubles/class-ingestion-with-failing-api.php |
Test double extending Ingestion class to simulate API failures for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [ | ||
| 'post_id' => $post_id, | ||
| // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_debug_backtrace -- Intentional for error tracing. | ||
| 'backtrace' => debug_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS, 5 ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdullah-kasim What data does the backtrace gives us to better debug? The origin function that triggered the ingestion?
I wonder if we could also rely on some ingest post parameter to detect where it's coming from and rely on the backtrace only when there's an exception.
| ); | ||
|
|
||
| self::fire_ingestion_failure( | ||
| new Ingestion_Failure( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion (not required): Move the Ingestion_Failure object creation with WP Error under the fire_ingestion_failure to keep code cleaner, this way we could pass the failure code and post_id only, an fill the rest there.
| ] | ||
| ); | ||
|
|
||
| self::fire_ingestion_failure( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdullah-kasim One other thing I've been wondering (for both the ingestion failures) is that if there's a config error we might get DDoSed via ingestion failures.
I do wonder
- Should we instantiate as little as possible here, and let the action do the work of instantiating object as needed?
- Do we need some kind of protection for when this scenario happens (or SF is down)? I'm referring to having some protection -before- the action is triggered.
Regarding point 2, I know it could also make sense to do it as the action level, but I do wonder if it would be better/more performant, to catch this earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also true for the logging part, we should be aware that this might raise too much logging data when an error happens at this level.
Description
Part of VIP-2449. Requires #7 to be merged.
We're opting for
post_ingestion_failedrather thanpost_ingestion_completeas I couldn't come up with a use-case to trigger on all completion.Pre-review checklist
Please make sure the items below have been covered before requesting a review:
Pre-deploy checklist
Steps to Test
Click here for testing steps
# Manual Testing: Ingestion Failure ActionPrerequisites
vip dev-env start/wp/log/debug.logerror_log()output: appears in terminal (not debug.log)vip-agentforce.php:Test 1: Action Does NOT Fire on Successful Ingestion
Goal: Verify the failure action does not fire when ingestion succeeds.
vip-agentforce.php:Expected: No "UNEXPECTED FAILURE" message. Verify success in debug.log:
Test 2: Action Does NOT Fire on Filter Rejection (Skip)
Goal: Verify the failure action does not fire when ingestion is skipped.
vip-agentforce.phpwith:vip dev-env shell -- wp post create --post_title="Skip Test" --post_status=publishExpected: No "UNEXPECTED FAILURE" message. Verify skip in debug.log:
Test 3: Action DOES Fire on Transform Failure
Goal: Verify the failure action fires when transform returns null.
vip-agentforce.phpwith:vip dev-env shell -- wp post create --post_title="Transform Fail Test" --post_status=publishExpected:
Test 4: Action DOES Fire on API Failure
Goal: Verify the failure action fires when API returns failure.
vip-agentforce.phpwith:Expected:
Failure Codes Reference
is_transform_failure()is_api_error()transform_failedIngestion_Failure::CODE_TRANSFORM_FAILEDapi_errorIngestion_Failure::CODE_API_ERRORNote: Filter rejection (skip) does NOT trigger the failure action.
Cleanup
Remove all test filters from
vip-agentforce.phpafter testing.