[PR #150] [MERGED] fix: treat screenshot failure as non-critical in browser tool #192

New Issue

2026-06-06T22:09:37-04:00

yindo commented

2026-06-06 22:09:37 -04:00

📋 Pull Request Information

Original PR: https://github.com/vxcontrol/pentagi/pull/150
Author: @mason5052
Created: 2/26/2026
Status: ✅ Merged
Merged: 3/2/2026
Merged by: @asdek

Base: feature/next_release ← Head: fix/browser-graceful-screenshot-failure

📝 Commits (1)

c68f933 fix: treat screenshot failure as non-critical in browser tool

📊 Changes

2 files changed (+205 additions, -10 deletions)

View changed files

📝 backend/pkg/tools/browser.go (+14 -10)
📝 backend/pkg/tools/browser_test.go (+191 -0)

📄 Description

Description of the Change

Problem

The browser tool's ContentMD(), ContentHTML(), and Links() methods run content fetch and screenshot capture concurrently. When the screenshot request fails for any reason (scraper unavailable, timeout, image too small), the entire operation returns an error -- discarding valid, successfully-fetched page content.

The AI agent then receives an error like:

browser tool 'markdown' handled with error: failed to fetch screenshot by url '...': ...

...and interprets the URL as unreachable, wasting tool calls on retries or fallback approaches, even though the content was sitting in memory.

Additionally, getHTML() was using the wrong minimum content size constant (minMdContentSize = 50 bytes instead of minHtmlContentSize = 300 bytes), accepting HTML responses that are too small to be useful.

The file also mixed Go's stdlib log.Println with logrus structured logging used everywhere else in the codebase.

Solution

Three fixes in backend/pkg/tools/browser.go:

Graceful screenshot degradation: In ContentMD(), ContentHTML(), and Links(), screenshot failure is now logged as a warning and the content is returned with an empty screenshot name. This is consistent with wrapCommandResult() which already ignores the PutScreenshot return value (_, _ = b.scp.PutScreenshot(...)).
Fix getHTML() size check: Changed from minMdContentSize (50) to minHtmlContentSize (300).
Consistent logging: Replaced 3 log.Println calls with logrus.WithField("url", url).Info(...) and removed the unused "log" import.

Closes #149

Type of Change

Bug fix (non-breaking change which fixes an issue)
Test update

Areas Affected

AI Agents (Researcher/Developer/Executor)
Security Tools Integration

Testing and Verification

Test Configuration

PentAGI Version: v1.2.0 (master @ e97bbe5)
Docker Version: N/A (unit tests only)
Host OS: Windows 11
LLM Provider: N/A

Test Steps

Added newTestScraper() helper that creates an httptest.Server simulating the scraper service with configurable screenshot behavior ("ok", "fail", "small")
Added 6 new test cases covering all three browser actions:
- TestContentMD_ScreenshotFailure_ReturnsContent -- scraper returns 500 on /screenshot, content is still returned
- TestContentHTML_ScreenshotFailure_ReturnsContent -- same for HTML
- TestLinks_ScreenshotFailure_ReturnsContent -- same for links
- TestContentMD_ScreenshotSmall_ReturnsContent -- screenshot below minImgContentSize, content still returned
- TestContentMD_BothSucceed_ReturnsContentAndScreenshot -- happy path, both content and screenshot file verified
- TestGetHTML_UsesCorrectMinContentSize -- content of 60 bytes (> 50, < 300) is correctly rejected with minHtmlContentSize

Test Results

Before fix: ContentMD/ContentHTML/Links return error when screenshot fails, discarding valid content
After fix: Content is returned; screenshot failure logged as warning with empty screenshot name
Before fix: getHTML() accepts 60-byte HTML (passes minMdContentSize check)
After fix: getHTML() correctly rejects content below minHtmlContentSize (300 bytes)

Security Considerations

No security impact. This change improves reliability by not discarding valid data on non-critical side-effect failure. No new dependencies or permission changes.

Performance Impact

Negligible. The only change in the success path is the addition of a warning log on screenshot failure, which is a fast operation. Content fetch performance is unchanged.

Documentation Updates

README.md updates
API documentation updates
Configuration documentation updates
GraphQL schema updates
Other

No documentation changes required. The fix is internal behavior -- the browser tool API surface remains the same.

Deployment Notes

Backward-compatible. No new environment variables or configuration changes. The only observable difference is that the browser tool now returns content even when screenshots fail, which is the expected behavior described in #149.

Checklist

Code Quality

My code follows the project's coding standards
I have added/updated necessary documentation
I have added tests to cover my changes
All new and existing tests pass
I have run go fmt and go vet (for Go code)

Security

I have considered security implications
Changes maintain or improve the security model
Sensitive information has been properly handled

Compatibility

Changes are backward compatible
Dependencies are properly updated

Documentation

Documentation is clear and complete
Comments are added for non-obvious code

Additional Notes

This PR continues the browser tool hardening work from PR #120 (resource leak prevention). The wrapCommandResult() method already treats screenshot storage as optional (_, _ = b.scp.PutScreenshot(...)), so returning content without a screenshot is consistent with the existing design intent.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/vxcontrol/pentagi/pull/150 **Author:** [@mason5052](https://github.com/mason5052) **Created:** 2/26/2026 **Status:** ✅ Merged **Merged:** 3/2/2026 **Merged by:** [@asdek](https://github.com/asdek) **Base:** `feature/next_release` ← **Head:** `fix/browser-graceful-screenshot-failure` --- ### 📝 Commits (1) - [`c68f933`](https://github.com/vxcontrol/pentagi/commit/c68f9335e06ae39606c4376a777a19cf624882ca) fix: treat screenshot failure as non-critical in browser tool ### 📊 Changes **2 files changed** (+205 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `backend/pkg/tools/browser.go` (+14 -10) 📝 `backend/pkg/tools/browser_test.go` (+191 -0) </details> ### 📄 Description ### Description of the Change #### Problem The browser tool's `ContentMD()`, `ContentHTML()`, and `Links()` methods run content fetch and screenshot capture concurrently. When the screenshot request fails for any reason (scraper unavailable, timeout, image too small), the entire operation returns an error -- **discarding valid, successfully-fetched page content**. The AI agent then receives an error like: ``` browser tool 'markdown' handled with error: failed to fetch screenshot by url '...': ... ``` ...and interprets the URL as unreachable, wasting tool calls on retries or fallback approaches, even though the content was sitting in memory. Additionally, `getHTML()` was using the wrong minimum content size constant (`minMdContentSize` = 50 bytes instead of `minHtmlContentSize` = 300 bytes), accepting HTML responses that are too small to be useful. The file also mixed Go's stdlib `log.Println` with `logrus` structured logging used everywhere else in the codebase. #### Solution Three fixes in `backend/pkg/tools/browser.go`: 1. **Graceful screenshot degradation**: In `ContentMD()`, `ContentHTML()`, and `Links()`, screenshot failure is now logged as a warning and the content is returned with an empty screenshot name. This is consistent with `wrapCommandResult()` which already ignores the `PutScreenshot` return value (`_, _ = b.scp.PutScreenshot(...)`). 2. **Fix getHTML() size check**: Changed from `minMdContentSize` (50) to `minHtmlContentSize` (300). 3. **Consistent logging**: Replaced 3 `log.Println` calls with `logrus.WithField("url", url).Info(...)` and removed the unused `"log"` import. Closes #149 ### Type of Change - [x] Bug fix (non-breaking change which fixes an issue) - [x] Test update ### Areas Affected - [x] AI Agents (Researcher/Developer/Executor) - [x] Security Tools Integration ### Testing and Verification #### Test Configuration ```yaml PentAGI Version: v1.2.0 (master @ e97bbe5) Docker Version: N/A (unit tests only) Host OS: Windows 11 LLM Provider: N/A ``` #### Test Steps 1. Added `newTestScraper()` helper that creates an `httptest.Server` simulating the scraper service with configurable screenshot behavior ("ok", "fail", "small") 2. Added 6 new test cases covering all three browser actions: - `TestContentMD_ScreenshotFailure_ReturnsContent` -- scraper returns 500 on /screenshot, content is still returned - `TestContentHTML_ScreenshotFailure_ReturnsContent` -- same for HTML - `TestLinks_ScreenshotFailure_ReturnsContent` -- same for links - `TestContentMD_ScreenshotSmall_ReturnsContent` -- screenshot below `minImgContentSize`, content still returned - `TestContentMD_BothSucceed_ReturnsContentAndScreenshot` -- happy path, both content and screenshot file verified - `TestGetHTML_UsesCorrectMinContentSize` -- content of 60 bytes (> 50, < 300) is correctly rejected with `minHtmlContentSize` #### Test Results - Before fix: `ContentMD/ContentHTML/Links` return error when screenshot fails, discarding valid content - After fix: Content is returned; screenshot failure logged as warning with empty screenshot name - Before fix: `getHTML()` accepts 60-byte HTML (passes `minMdContentSize` check) - After fix: `getHTML()` correctly rejects content below `minHtmlContentSize` (300 bytes) ### Security Considerations No security impact. This change improves reliability by not discarding valid data on non-critical side-effect failure. No new dependencies or permission changes. ### Performance Impact Negligible. The only change in the success path is the addition of a warning log on screenshot failure, which is a fast operation. Content fetch performance is unchanged. ### Documentation Updates - [ ] README.md updates - [ ] API documentation updates - [ ] Configuration documentation updates - [ ] GraphQL schema updates - [ ] Other No documentation changes required. The fix is internal behavior -- the browser tool API surface remains the same. ### Deployment Notes Backward-compatible. No new environment variables or configuration changes. The only observable difference is that the browser tool now returns content even when screenshots fail, which is the expected behavior described in #149. ### Checklist #### Code Quality - [x] My code follows the project's coding standards - [x] I have added/updated necessary documentation - [x] I have added tests to cover my changes - [x] All new and existing tests pass - [x] I have run `go fmt` and `go vet` (for Go code) #### Security - [x] I have considered security implications - [x] Changes maintain or improve the security model - [x] Sensitive information has been properly handled #### Compatibility - [x] Changes are backward compatible - [x] Dependencies are properly updated #### Documentation - [x] Documentation is clear and complete - [x] Comments are added for non-obvious code ### Additional Notes This PR continues the browser tool hardening work from PR #120 (resource leak prevention). The `wrapCommandResult()` method already treats screenshot storage as optional (`_, _ = b.scp.PutScreenshot(...)`), so returning content without a screenshot is consistent with the existing design intent. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

yindo added the pull-request label 2026-06-06 22:09:37 -04:00

yindo closed this issue

2026-06-06 22:09:38 -04:00

yindo referenced this issue

2026-06-06 22:10:02 -04:00

[PR #268] [CLOSED] feat: queue input for running automation flows #283

yindo referenced this issue

2026-06-06 22:10:07 -04:00

[PR #292] [MERGED] docs: explain assistant flow management for active flows #301

Sign in to join this conversation.