Add vendor assessment agent with structured output#982
Conversation
bde166f to
3c2639a
Compare
cd1d947 to
9c38610
Compare
ccba179 to
d640d96
Compare
29c89db to
a061840
Compare
a061840 to
c7ba96e
Compare
|
@cubic-dev-ai review this PR |
@aureliensibiril I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
11 issues found across 101 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed. We prioritized the most important files first.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pkg/agents/vetting/prompts/code_security.txt">
<violation number="1" location="pkg/agents/vetting/prompts/code_security.txt:73">
P2: Example outputs are not valid JSON, conflicting with the enforced JSON schema and likely causing invalid outputs when the model follows the examples.</violation>
</file>
<file name="pkg/agents/vetting/prompts/analyzer.txt">
<violation number="1" location="pkg/agents/vetting/prompts/analyzer.txt:66">
P2: Examples contradict the JSON output requirement; they show semicolon-delimited text instead of valid JSON, which can lead the model to emit invalid JSON and fail schema enforcement.</violation>
</file>
<file name="pkg/agents/vetting/prompts/compliance.txt">
<violation number="1" location="pkg/agents/vetting/prompts/compliance.txt:45">
P2: Examples contradict the JSON‑schema requirement and show non‑JSON output formats, which can lead the model to emit invalid JSON and fail schema validation.</violation>
</file>
<file name="pkg/agent/tools/browser/fetch_robots.go">
<violation number="1" location="pkg/agent/tools/browser/fetch_robots.go:92">
P2: Disallow parsing lowercases the entire line and uses the lowercased remainder as the path, which changes case-sensitive URL paths and can misreport disallowed entries.</violation>
</file>
<file name="pkg/agent/tools/search/diff_documents.go">
<violation number="1" location="pkg/agent/tools/search/diff_documents.go:66">
P2: Oversized documents are incorrectly reported as having no differences, and the tool suppresses the "too large" diagnostic output.</violation>
</file>
<file name="pkg/agent/tools/internal/netcheck/netcheck.go">
<violation number="1" location="pkg/agent/tools/internal/netcheck/netcheck.go:34">
P1: `IsPublicIP` does not block all multicast addresses; it only blocks link-local multicast, allowing other multicast ranges to be treated as public.</violation>
</file>
<file name="pkg/agent/tools/browser/click.go">
<violation number="1" location="pkg/agent/tools/browser/click.go:57">
P1: Click-triggered navigation is not revalidated, allowing bypass of initial domain/URL SSRF checks.</violation>
</file>
<file name="pkg/agents/vetting/prompts/ai_risk.txt">
<violation number="1" location="pkg/agents/vetting/prompts/ai_risk.txt:69">
P2: Examples under `<examples>` are not valid JSON despite the prompt requiring schema-enforced JSON output; the semicolon-separated format can bias the model toward invalid JSON and break strict validation.</violation>
</file>
<file name="pkg/agents/vetting/prompts/incident_response.txt">
<violation number="1" location="pkg/agents/vetting/prompts/incident_response.txt:59">
P2: Examples contradict the JSON output requirement; they use a semicolon-delimited key:value list rather than valid JSON, which can cause the model to emit invalid output for the enforced schema.</violation>
</file>
<file name="pkg/agent/tools/browser/extract_text.go">
<violation number="1" location="pkg/agent/tools/browser/extract_text.go:70">
P2: Text size is capped only after full-page extraction, so huge pages can still cause large transfer/allocation overhead before truncation.</violation>
</file>
<file name="pkg/agents/vetting/assessment.go">
<violation number="1" location="pkg/agents/vetting/assessment.go:196">
P2: Research browser is created without any allowed-domain restriction; browser tool permits any http/https URL when no allowedDomains are set, enabling SSRF-style access to internal endpoints.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Three reinforcements on the browser navigation path, all surfaced by cubic code review on PR getprobo#982: - netcheck.IsPublicIP now rejects the full multicast range (ip.IsMulticast) rather than only link-local multicast, so addresses in 224.0.0.0/4 and 239.0.0.0/8 can no longer slip through the SSRF guard. - Browser.checkURL now runs netcheck.ValidatePublicURL on every URL, even when no allowed-domain list is set. The research browser in the vendor assessment is intentionally allowed to roam the public web, but it must still refuse URLs that resolve to loopback, private, or link-local IPs. - ClickElementTool reads the post-click location and feeds it back through Browser.checkURL. A click that triggers navigation to a different host (JS-initiated redirect, malicious <a href>, vendor page hijack) used to extract text from whatever page the browser ended up on; that path could bypass the initial checkURL call and read internal endpoints. The post-click revalidation closes that gap. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Three defects flagged by cubic code review on PR getprobo#982: - fetch_robots_txt lowercased the entire Disallow line before reading the path value, corrupting case-sensitive paths (e.g. /Admin/ reported as /admin/). Match the sitemap handling and read the path off the original-case raw line. - extract_page_text pulled the full document.body.innerText over the DevTools protocol before truncating on the Go side, so a huge page could burn bandwidth and memory well beyond maxTextLength. Slice the string in JS at 4x maxTextLength code units first (safe upper bound for UTF-16 code units per Go rune) before transferring, then finish the rune-exact truncation in Go. - diff_documents silently dropped the "documents too large for detailed diff" message when either side exceeded the 5000-line LCS cap, returning HasDifferences=false and an empty UnifiedDiff. Add a tooLarge flag on the internal diffOutput and surface the message via ErrorDetail so the caller can distinguish "no differences" from "too large to compare". Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
CodeQL flagged InsecureSkipVerify in check_ssl_certificate on PR getprobo#982. The tool is a cert INSPECTOR: we intentionally connect to servers whose certificates may be expired, self-signed, or otherwise invalid because reporting on that state is the entire purpose of the tool. The handshake's built-in verification is disabled, then the code manually runs x509.Verify on the returned chain and reports the result in the Valid field. No credentials or confidential data are ever sent over the connection. Document the intent inline and add a //nolint:gosec directive so the scanner stops flagging this path. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
There was a problem hiding this comment.
1 issue found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pkg/agent/tools/browser/click.go">
<violation number="1" location="pkg/agent/tools/browser/click.go:74">
P1: Post-click URL validation is performed only after click-triggered navigation, so SSRF-blocked destinations may still be contacted before rejection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Three reinforcements on the browser navigation path, all surfaced by cubic code review on PR getprobo#982: - netcheck.IsPublicIP now rejects the full multicast range (ip.IsMulticast) rather than only link-local multicast, so addresses in 224.0.0.0/4 and 239.0.0.0/8 can no longer slip through the SSRF guard. - Browser.checkURL now runs netcheck.ValidatePublicURL on every URL, even when no allowed-domain list is set. The research browser in the vendor assessment is intentionally allowed to roam the public web, but it must still refuse URLs that resolve to loopback, private, or link-local IPs. - ClickElementTool reads the post-click location and feeds it back through Browser.checkURL. A click that triggers navigation to a different host (JS-initiated redirect, malicious <a href>, vendor page hijack) used to extract text from whatever page the browser ended up on; that path could bypass the initial checkURL call and read internal endpoints. The post-click revalidation closes that gap. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Three defects flagged by cubic code review on PR getprobo#982: - fetch_robots_txt lowercased the entire Disallow line before reading the path value, corrupting case-sensitive paths (e.g. /Admin/ reported as /admin/). Match the sitemap handling and read the path off the original-case raw line. - extract_page_text pulled the full document.body.innerText over the DevTools protocol before truncating on the Go side, so a huge page could burn bandwidth and memory well beyond maxTextLength. Slice the string in JS at 4x maxTextLength code units first (safe upper bound for UTF-16 code units per Go rune) before transferring, then finish the rune-exact truncation in Go. - diff_documents silently dropped the "documents too large for detailed diff" message when either side exceeded the 5000-line LCS cap, returning HasDifferences=false and an empty UnifiedDiff. Add a tooLarge flag on the internal diffOutput and surface the message via ErrorDetail so the caller can distinguish "no differences" from "too large to compare". Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
CodeQL flagged InsecureSkipVerify in check_ssl_certificate on PR getprobo#982. The tool is a cert INSPECTOR: we intentionally connect to servers whose certificates may be expired, self-signed, or otherwise invalid because reporting on that state is the entire purpose of the tool. The handshake's built-in verification is disabled, then the code manually runs x509.Verify on the returned chain and reports the result in the Valid field. No credentials or confidential data are ever sent over the connection. Document the intent inline and add a //nolint:gosec directive so the scanner stops flagging this path. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
a9325bc to
0ec970e
Compare
Three reinforcements on the browser navigation path, all surfaced by cubic code review on PR getprobo#982: - netcheck.IsPublicIP now rejects the full multicast range (ip.IsMulticast) rather than only link-local multicast, so addresses in 224.0.0.0/4 and 239.0.0.0/8 can no longer slip through the SSRF guard. - Browser.checkURL now runs netcheck.ValidatePublicURL on every URL, even when no allowed-domain list is set. The research browser in the vendor assessment is intentionally allowed to roam the public web, but it must still refuse URLs that resolve to loopback, private, or link-local IPs. - ClickElementTool reads the post-click location and feeds it back through Browser.checkURL. A click that triggers navigation to a different host (JS-initiated redirect, malicious <a href>, vendor page hijack) used to extract text from whatever page the browser ended up on; that path could bypass the initial checkURL call and read internal endpoints. The post-click revalidation closes that gap. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Three defects flagged by cubic code review on PR getprobo#982: - fetch_robots_txt lowercased the entire Disallow line before reading the path value, corrupting case-sensitive paths (e.g. /Admin/ reported as /admin/). Match the sitemap handling and read the path off the original-case raw line. - extract_page_text pulled the full document.body.innerText over the DevTools protocol before truncating on the Go side, so a huge page could burn bandwidth and memory well beyond maxTextLength. Slice the string in JS at 4x maxTextLength code units first (safe upper bound for UTF-16 code units per Go rune) before transferring, then finish the rune-exact truncation in Go. - diff_documents silently dropped the "documents too large for detailed diff" message when either side exceeded the 5000-line LCS cap, returning HasDifferences=false and an empty UnifiedDiff. Add a tooLarge flag on the internal diffOutput and surface the message via ErrorDetail so the caller can distinguish "no differences" from "too large to compare". Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
CodeQL flagged InsecureSkipVerify in check_ssl_certificate on PR getprobo#982. The tool is a cert INSPECTOR: we intentionally connect to servers whose certificates may be expired, self-signed, or otherwise invalid because reporting on that state is the entire purpose of the tool. The handshake's built-in verification is disabled, then the code manually runs x509.Verify on the returned chain and reports the result in the Valid field. No credentials or confidential data are ever sent over the connection. Document the intent inline and add a //nolint:gosec directive so the scanner stops flagging this path. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
0ec970e to
2678fc3
Compare
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="GNUmakefile">
<violation number="1" location="GNUmakefile:131">
P2: `test-e2e` hardcodes `bin/probod-e2e` instead of using `PROBOD_E2E_BIN`, so overriding the variable can run the wrong or stale binary.</violation>
<violation number="2" location="GNUmakefile:214">
P2: The new `bin/probod-e2e` target duplicates `bin/probod` prerequisites, creating dependency-list drift risk and fragile build maintenance.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 37 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="GNUmakefile">
<violation number="1" location="GNUmakefile:143">
P2: `test-e2e` prerequisite was changed to a configurable target name, but no matching rule exists for overridden `PROBOD_E2E_BIN` values, causing Make target resolution failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Three reinforcements on the browser navigation path, all surfaced by cubic code review on PR getprobo#982: - netcheck.IsPublicIP now rejects the full multicast range (ip.IsMulticast) rather than only link-local multicast, so addresses in 224.0.0.0/4 and 239.0.0.0/8 can no longer slip through the SSRF guard. - Browser.checkURL now runs netcheck.ValidatePublicURL on every URL, even when no allowed-domain list is set. The research browser in the vendor assessment is intentionally allowed to roam the public web, but it must still refuse URLs that resolve to loopback, private, or link-local IPs. - ClickElementTool reads the post-click location and feeds it back through Browser.checkURL. A click that triggers navigation to a different host (JS-initiated redirect, malicious <a href>, vendor page hijack) used to extract text from whatever page the browser ended up on; that path could bypass the initial checkURL call and read internal endpoints. The post-click revalidation closes that gap. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Three defects flagged by cubic code review on PR getprobo#982: - fetch_robots_txt lowercased the entire Disallow line before reading the path value, corrupting case-sensitive paths (e.g. /Admin/ reported as /admin/). Match the sitemap handling and read the path off the original-case raw line. - extract_page_text pulled the full document.body.innerText over the DevTools protocol before truncating on the Go side, so a huge page could burn bandwidth and memory well beyond maxTextLength. Slice the string in JS at 4x maxTextLength code units first (safe upper bound for UTF-16 code units per Go rune) before transferring, then finish the rune-exact truncation in Go. - diff_documents silently dropped the "documents too large for detailed diff" message when either side exceeded the 5000-line LCS cap, returning HasDifferences=false and an empty UnifiedDiff. Add a tooLarge flag on the internal diffOutput and surface the message via ErrorDetail so the caller can distinguish "no differences" from "too large to compare". Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
CodeQL flagged InsecureSkipVerify in check_ssl_certificate on PR getprobo#982. The tool is a cert INSPECTOR: we intentionally connect to servers whose certificates may be expired, self-signed, or otherwise invalid because reporting on that state is the entire purpose of the tool. The handshake's built-in verification is disabled, then the code manually runs x509.Verify on the returned chain and reports the result in the Valid field. No credentials or confidential data are ever sent over the connection. Document the intent inline and add a //nolint:gosec directive so the scanner stops flagging this path. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
28978a6 to
36eb8c3
Compare
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The rebase onto main split the evidence-describer config into two: an LLMAgentConfig under agents.evidence-describer and a worker-tuning EvidenceDescriberConfig (Interval, StaleAfter, MaxConcurrency) at the top level. Bootstrap was only populating the LLM half, leaving Interval at zero — which panics the probod boot with "non-positive interval for NewTicker" when the evidence description worker starts. Add the missing fields with the same defaults main uses. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
e55b7f0 to
50c0be6
Compare
Summary
Toolsetinterface,TypedTool[In, Out]factory, and result helpers topkg/agentfor reusable agent toolingagent.WithOutputTypeResponseFormatinto the Anthropic provider (OutputConfig.Format), enabling API-level structured output — previously silently droppedChanges by area
Agent framework (
pkg/agent)ToolsetAPI withWithToolsets(...)TypedTool[In, Out]withResultJSON/ResultError(f)helpersWithOutputType(...)for structured JSON output enforcementWithThinking(...)for extended thinking budgetLLM providers (
pkg/llm)ResponseFormat→OutputConfig.Format(JSON schema enforcement)Vendor assessment (
pkg/agents/vetting)jsonschematags inoutput_types.goWithOutputTypefor schema-enforced JSON responsesTask priority removal
rankfield from Task schema and UITest plan
make test MODULE=./pkg/agent/...— all tests passmake test MODULE=./pkg/agents/vetting/...— output type schema tests passgo vet/go build— clean on all affected packagesrun.sh