Add vendor assessment agent with structured output#982
Add vendor assessment agent with structured output#982aureliensibiril wants to merge 59 commits intogetprobo:mainfrom
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.
| dialer := &tls.Dialer{ | ||
| NetDialer: &net.Dialer{Timeout: 10 * time.Second}, | ||
| Config: &tls.Config{ | ||
| InsecureSkipVerify: true, //nolint:gosec // cert inspector; verification happens manually below |
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>
Few-shot <example> blocks in six vetting sub-agent prompts (analyzer, compliance, code_security, ai_risk, incident_response, regulatory_compliance) used a semicolon-delimited "key: value" format in their <output> tags. The actual model output for those agents is enforced as JSON via the OutputType schema, so the examples contradicted the enforced contract and could bias the model toward emitting invalid JSON during the synthesis turn. Convert every example <output> to real JSON matching the sub-agent's output schema. No semantic changes to the examples themselves. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The final vendor_info_extractor step used to share the orchestrator's 20-minute AssessmentTimeout context, so a slow orchestrator could leave the extractor with no budget to run. Observed on a Pylon assessment where the orchestrator consumed ~19 minutes of sub-agent work and the extractor then failed immediately with "context deadline exceeded" — losing the full markdown report that had just been produced. Detach the extractor from the assessment context and give it a dedicated 5-minute budget via context.WithoutCancel + a fresh WithTimeout. The extractor has no tools and emits a single structured JSON output, so five minutes is more than enough even when Anthropic forces the streaming path. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
WebSearchTool reimplemented the exact URL, query params, HTTP call, body parse and slice-to-limit flow that searxngSearch (sibling file, same package) already provided. Delegate to the helper and drop the duplicated HTTP block and its now-unused imports. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The AssessVendor GraphQL resolver and the MCP NewAssessVendorOutput builder both converted []probo.Subprocessor into their respective generated slice type with identical field-for-field loops. Extract NewVendorSubprocessors in each types package and call it from both sides. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The earlier LLM config rewrite dropped the EvidenceDescriberConfig struct (Interval, StaleAfter, MaxConcurrency) together with its top-level Config field, but probod.go still dereferences impl.cfg.EvidenceDescriber for the worker tuning knobs, so the tree fails to build. Reintroduce the struct and field to restore the compile; LLM parameters stay under AgentsConfig.EvidenceDescriber. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Decouple probo.Service from the concrete *vetting.Assessor so tests can swap in a deterministic stub implementation. The interface lives next to AssessVendorRequest in vendor_service.go; the real *vetting.Assessor satisfies it by shape, so callers in probod are unaffected. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Wire a deterministic VendorAssessor implementation that returns a fixed Result, plus an AgentsConfig flag (stub-vendor-assessor) that selects it over the real vetting.NewAssessor at startup. Intended strictly for end-to-end tests where running the real LLM and browser pipeline is both expensive and flaky; the flag must never be enabled in production. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Enable stub-vendor-assessor in the e2e config so the assessVendor mutation returns deterministic output without reaching the network, then cover the new GraphQL endpoint with cases for owner/admin success, viewer forbidden, cross-tenant access denied, and the optional procedure field. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
0ec970e to
2678fc3
Compare
Previously the stub shipped inside the production probod binary and was selected at runtime by an AgentsConfig flag. A misconfigured YAML could therefore silently replace the real assessor with a fixture that returns "Stub Vendor" for every assessVendor call. Move the stub into a file tagged //go:build e2e, introduce a matching prod twin tagged //go:build !e2e, and drop the StubVendorAssessor config field. The e2e target now builds a dedicated bin/probod-e2e with -tags=e2e so the fixture is physically absent from the production binary. Also fix the stub's privacy URL to use url.JoinPath instead of naive string concat. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
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.
TypedTool was an unused near-duplicate of FunctionTool that only added automatic output marshalling. Merge the two into a single FunctionTool constructor and drop the error return: the JSON schema is generated from a compile-time Go type, so any failure is a programmer bug and panicking surfaces it earlier than a runtime error dance. Simplify all tool factories to return agent.Tool directly and collapse the three toolsets to slice literals, eliminating the CollectTools helper and the error-checking boilerplate at every call site. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Promote the previously-hardcoded maxEmptyOutputRetries constant to an Agent field with a matching WithMaxEmptyOutputRetries functional option so callers that want to tighten or relax the synthesis-turn retry budget no longer need to fork the package. The default (2) keeps the previous behavior. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Bump the synthesis-turn entry and empty-output retry logs from Info to Warn so the fallback path surfaces in production telemetry when the structured-output loop needs tuning; normal tool dispatch and LLM lifecycle logs stay at Info. Expand the inline append(...) / llm.Message literal pairs to one argument per line per project style, and rewrite the empty-retry branch comment to spell out the Anthropic extended-thinking trigger so the next reader does not have to reverse-engineer it. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Extract the shared generated-code and SPA-bundle prerequisites of the probod and probod-e2e targets into a PROBOD_BIN_DEPS variable so both targets stay in sync. Drive the test-e2e target through the PROBOD_E2E_BIN variable for the same reason. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
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.
Use $(PROBOD_BIN) and $(PROBOD_E2E_BIN) as target LHS so overriding those variables at invocation time resolves to a real rule instead of an unknown target. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Revert the JSON tag renames from `agents`/`default` back to `llm`/`defaults` so existing `config.yaml` files keep parsing after this branch lands. The Go field names (`Agents`, `Default`) remain for internal readability; only the deployment-facing keys are restored. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Deployments that do not configure an LLM provider for the `vendor-assessor` agent now get a DisabledVendorAssessor that returns ErrVendorAssessmentDisabled. The console resolver surfaces that sentinel as a stable UNAVAILABLE GraphQL error instead of falling through to the generic INTERNAL handler. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Replace the build-tag-gated stub/prod pair with a single vendor_assessor.go file. When `llm.vendor-assessor.provider` is not set, buildVendorAssessor returns probo.DisabledVendorAssessor instead of constructing the real LLM/browser pipeline. Unlike the other agents, vendor-assessor does not inherit the default provider -- its pipeline is expensive and must be enabled explicitly by operators. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Remove the `bin/probod-e2e` target and `-tags=e2e` coverage variant -- the stub they relied on is gone. `make test-e2e` now builds the regular `bin/probod` and the vendor test asserts on the UNAVAILABLE error returned by DisabledVendorAssessor. Happy path payload shape moves to pkg/probo unit tests. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Relocate pkg/agents/vetting -> pkg/vetting so the vendor assessment suite sits alongside other product domains (pkg/probo, pkg/iam, pkg/trust) instead of under a grab-bag `pkg/agents` folder that confusingly shadowed the pkg/agent framework. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Fold the one-shot changelog generation out of pkg/agents into DocumentService.generateChangelog. Thread the 4 LLM config fields (client, model, temperature, max_tokens) through to TenantService so DocumentService can access them via s.svc.* instead of going through the legacy agents.Agent wrapper. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Match the pattern used by pkg/vetting: the system prompt lives in pkg/probo/prompts/changelog_generator.txt and is embedded via `//go:embed`. Keeps long LLM prompts out of .go source files where they're awkward to edit and diff. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
All 3 files that used to live here have been relocated: vetting moved to pkg/vetting, changelog logic folded into pkg/probo/DocumentService, and the legacy Agent wrapper deleted outright. The pkg/agent framework now stands alone without the confusingly named sibling package. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
gofmt caught two import blocks where `pkg/vetting` was still in the slot that matched its old `pkg/agents/vetting` path. Move them into alphabetical order. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Add a commented-out `vendor-assessor` block under `llm` in the sample dev config so operators know the key exists. The feature is gated on an explicit provider and returns UNAVAILABLE when unset. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
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