fix: replace generic "Response not valid" with actionable error messages for better observability#41769
Open
fix: replace generic "Response not valid" with actionable error messages for better observability#41769
Conversation
…ror messages The catch-all in executePluginActionSaga discarded the original error (Axios timeout, network failure, server validation rejection) and replaced it with the opaque "Response not valid" message, making production debugging impossible for customers in view mode. Client: - Add extractExecutionErrorMessage() to categorise Axios transport errors vs server envelope errors vs unknown failures - Surface the specific message through the runActionSaga error cascade instead of falling through to "An unexpected error occurred" - Trigger execution path inherits the fix automatically Server: - Replace result.setRequest(null) in view mode with sanitizeRequestForViewMode() that retains actionId, requestedAt, and httpMethod while stripping sensitive fields (body, headers, URL, params) - Upgrade execution error logging from debug/println/printStackTrace to structured SLF4J warn/error with action identity
Contributor
WalkthroughAdds a client-side error message extractor and integrates it into action execution error flows; replaces ad-hoc prints/stacktraces with SLF4J logging on server plugins/utilities; and introduces request sanitization for view-mode action execution results with accompanying tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RestAPIActivateUtils.java (1)
151-152: ⚡ Quick winAdd action context and exception details to this warning log.
Line [151] logs a generic message only; include action identity (e.g., actionId) and the caught exception so parse failures are correlatable in production logs.
Proposed logging improvement
- log.warn( - "Response declared Content-Type application/json but body is not valid JSON. Falling back to string representation."); + log.warn( + "Invalid JSON response for actionId={} with Content-Type={}; falling back to string body.", + actionExecutionRequest.getActionId(), + contentType, + e);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RestAPIActivateUtils.java` around lines 151 - 152, The warning currently logged in RestAPIActivateUtils only states the JSON parse failed; update the log.warn call to include the action identity (e.g., actionId or action.getId()) and the caught exception object so failures are traceable—modify the catch block where log.warn("Response declared Content-Type application/json but body is not valid JSON. ...") is called to pass a formatted message containing the actionId (or action identifier variable used in this method) and include the exception (e.g., e) as the throwable parameter to the logger.app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java (1)
468-495: ⚡ Quick winStrengthen this test by seeding every sensitive field before sanitization.
You assert
executionParametersis stripped, but it isn’t initialized here. Set it (and optionallyquery/properties) pre-sanitize so regressions are caught.Proposed test hardening
original.setHeaders(Map.of("Authorization", "Bearer token")); original.setRequestParams(List.of("sensitive-param")); + original.setExecutionParameters(List.of("secret-exec-param")); + original.setQuery("SELECT * FROM sensitive_table"); + original.setProperties(Map.of("debug", "secret")); @@ assertEquals(null, sanitized.getRequestParams()); assertEquals(null, sanitized.getExecutionParameters()); + assertEquals(null, sanitized.getQuery()); + assertEquals(null, sanitized.getProperties());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java` around lines 468 - 495, The test sanitizeRequestForViewMode_retainsSafeFieldsAndStripsSensitiveOnes should seed all sensitive fields before calling actionExecutionSolution.sanitizeRequestForViewMode so the assertions actually verify they are stripped; update the setup for the ActionExecutionRequest (variable original) to set executionParameters (e.g., a Map), and also populate other sensitive containers like query and properties if present on the ActionExecutionRequest class, then call actionExecutionSolution.sanitizeRequestForViewMode(result) and keep the existing assertEquals(null, ...) checks for getExecutionParameters(), getQuery(), getProperties() along with url/body/headers/requestParams to ensure regressions are caught.
🤖 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/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java`:
- Around line 229-233: The error log in RestApiPlugin is currently emitting raw
request details via datasourceConfiguration.getUrl() and
actionConfiguration.getPath() which can leak sensitive data; update the
log.error call inside the REST execution failure handling to omit or mask
URL/path and instead log safe action-level metadata (e.g.,
actionConfiguration.getName(), actionConfiguration.getId(),
datasourceConfiguration.getId()) or a redacted/masked URL string produced by a
maskUrl helper; ensure you still pass the exception object to log the stack
trace but remove the unredacted URL/path parameters from the formatted message.
---
Nitpick comments:
In
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RestAPIActivateUtils.java`:
- Around line 151-152: The warning currently logged in RestAPIActivateUtils only
states the JSON parse failed; update the log.warn call to include the action
identity (e.g., actionId or action.getId()) and the caught exception object so
failures are traceable—modify the catch block where log.warn("Response declared
Content-Type application/json but body is not valid JSON. ...") is called to
pass a formatted message containing the actionId (or action identifier variable
used in this method) and include the exception (e.g., e) as the throwable
parameter to the logger.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java`:
- Around line 468-495: The test
sanitizeRequestForViewMode_retainsSafeFieldsAndStripsSensitiveOnes should seed
all sensitive fields before calling
actionExecutionSolution.sanitizeRequestForViewMode so the assertions actually
verify they are stripped; update the setup for the ActionExecutionRequest
(variable original) to set executionParameters (e.g., a Map), and also populate
other sensitive containers like query and properties if present on the
ActionExecutionRequest class, then call
actionExecutionSolution.sanitizeRequestForViewMode(result) and keep the existing
assertEquals(null, ...) checks for getExecutionParameters(), getQuery(),
getProperties() along with url/body/headers/requestParams to ensure regressions
are caught.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5164f543-f807-4e34-b36d-488d284cdc57
📒 Files selected for processing (7)
app/client/src/sagas/ActionExecution/PluginActionSaga.tsapp/client/src/sagas/ActionExecution/errorUtils.test.tsapp/client/src/sagas/ActionExecution/errorUtils.tsapp/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RestAPIActivateUtils.javaapp/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java
…ry-param secrets Keep httpMethod + path (safe metadata); drop datasourceConfiguration.getUrl() which could contain API keys in query parameters.
sondermanish
previously approved these changes
May 1, 2026
Collaborator
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/25207896325. |
|
Deploy-Preview-URL: https://ce-41769.dp.appsmith.com |
…n for unexpected errors Action execution failures are typically caused by the customer's upstream API (timeout, connection reset, invalid JSON), not by Appsmith itself. Logging these at warn/error creates noise in production and can trigger false alerts. - RestApiPlugin: log.error → log.debug (upstream REST failures) - executionExceptionMapper: log.warn → log.debug (duplicated by handler) - RestAPIActivateUtils: log.warn → log.debug (content-type mismatch) - executionExceptionHandler: kept at log.warn (rare, genuinely unexpected) Self-hosted operators who want to monitor upstream failure rates can lower the log level to DEBUG — making this opt-in rather than default.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java`:
- Around line 886-890: The WARN log in ActionExecutionSolutionCEImpl (inside the
execution error handling path) currently logs only error.getMessage(), losing
stack trace; update the log.warn call that references actionDTO.getName() and
actionDTO.getId() to pass the Throwable (error) as the last argument so the
logger preserves the full exception and stack trace when WARN/INFO level is
used.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d3ca22a-e80f-4493-a233-5573af6c0311
📒 Files selected for processing (3)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RestAPIActivateUtils.javaapp/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java
Comment on lines
+886
to
+890
| log.warn( | ||
| "Handling execution error for action '{}' (id: {}): {}", | ||
| actionDTO.getName(), | ||
| actionDTO.getId(), | ||
| error.getMessage()); |
Contributor
There was a problem hiding this comment.
Include throwable in WARN log to preserve stack trace
At Line 886, the warn log currently emits only error.getMessage(). In production (where DEBUG is often off), this drops stack-trace context for unexpected failures.
Suggested patch
- log.warn(
- "Handling execution error for action '{}' (id: {}): {}",
- actionDTO.getName(),
- actionDTO.getId(),
- error.getMessage());
+ log.warn(
+ "Handling execution error for action '{}' (id: {}): {}",
+ actionDTO.getName(),
+ actionDTO.getId(),
+ error.getMessage(),
+ error);📝 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.
Suggested change
| log.warn( | |
| "Handling execution error for action '{}' (id: {}): {}", | |
| actionDTO.getName(), | |
| actionDTO.getId(), | |
| error.getMessage()); | |
| log.warn( | |
| "Handling execution error for action '{}' (id: {}): {}", | |
| actionDTO.getName(), | |
| actionDTO.getId(), | |
| error.getMessage(), | |
| error); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java`
around lines 886 - 890, The WARN log in ActionExecutionSolutionCEImpl (inside
the execution error handling path) currently logs only error.getMessage(),
losing stack trace; update the log.warn call that references actionDTO.getName()
and actionDTO.getId() to pass the Throwable (error) as the last argument so the
logger preserves the full exception and stack trace when WARN/INFO level is
used.
sondermanish
approved these changes
May 1, 2026
subrata71
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
runActionSagaerror cascade so specific transport error messages are shown in the debugger console instead of falling through to "An unexpected error occurred"result.setRequest(null)in view mode withsanitizeRequestForViewMode()that retains safe metadata (actionId, requestedAt, httpMethod) while stripping sensitive fields (body, headers, URL, params)System.out.println/printStackTrace()to structured SLF4J with action identityFixes https://linear.app/appsmith/issue/APP-15199/replace-generic-response-not-valid-error-with-actionable-error
Log level rationale
Action execution failures are typically caused by the customer's upstream API (timeout, connection reset, invalid JSON content-type), not by Appsmith itself. Logging these at
WARN/ERRORlevel would create production noise proportional to upstream error rates and could trigger false alerts in monitoring systems.We use
log.debugfor upstream-caused failures so self-hosted operators can opt in by lowering the log level — making this an explicit choice rather than default noise. The one exception isexecutionExceptionHandler, which stays atlog.warnbecause it only fires for errors that escape the plugin's own error handling — genuinely unexpected conditions that operators should know about.RestApiPlugin(upstream REST failure)debugActionExecutionResult.executionExceptionMapper(error transformation)debugexecutionExceptionHandlerwhich runs right after. Only fires for rare non-timeout/non-stale errors that escape the plugin.executionExceptionHandler(catch-all)warnRestAPIActivateUtils(JSON parse fallback)debugThe key improvement over the previous code is structured SLF4J logging with action identity (replacing
System.out.println,error.printStackTrace(), and unstructuredlog.debug), not the log level itself. When operators DO enable debug logging, they now get actionable context instead of anonymous stack traces.Context
Enterprise customer (Plum) reported production debugging is impossible — "Response not valid" errors cannot be traced to specific actions in view mode. The root cause is a client-side catch-all that discards all error context and a server-side request stripping that removes all correlation metadata.
Slack thread: https://theappsmith.slack.com/archives/C0341RERY4R/p1776882142131629
Linear (support): V2-3907
Test plan
extractExecutionErrorMessagecovering all error categoriessanitizeRequestForViewModeverifying sensitive field strippingTip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/25208490026
Commit: ce142c5
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 01 May 2026 10:02:25 UTC
Summary by CodeRabbit
Bug Fixes
Tests