Describe evidence with structured assessment output#1107
Describe evidence with structured assessment output#1107aureliensibiril wants to merge 21 commits into
Conversation
There was a problem hiding this comment.
2 issues found across 15 files
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/probo/evidence_assessment_worker.go">
<violation number="1" location="pkg/probo/evidence_assessment_worker.go:168">
P2: `assessment` is set before the commit transaction, so the failure path can persist assessment data together with `FAILED` status.</violation>
</file>
<file name="pkg/evidencedescriber/prompt.txt">
<violation number="1" location="pkg/evidencedescriber/prompt.txt:33">
P2: The readable example contradicts the non-speculation rule by assigning SOC2/ISO27001 to a generic security screenshot, which can cause framework hallucinations in outputs.</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 9 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/coredata/evidence_assessment.go">
<violation number="1" location="pkg/coredata/evidence_assessment.go:28">
P2: `SetAssessment` misses typed-nil values and can persist JSON `null` instead of clearing the assessment.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
997b474 to
0916c25
Compare
There was a problem hiding this comment.
1 issue found across 10 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/probo/evidence_assessment_worker.go">
<violation number="1" location="pkg/probo/evidence_assessment_worker.go:202">
P1: `failEvidence` now writes the full `evidences` row from a stale in-memory snapshot, which can clobber concurrent updates. Update only failure-related columns (`assessment_status`, `assessment_processing_started_at`, `updated_at`) in this path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
7c0ac9b to
478198b
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 478198b. Configure here.
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
9af66c2 to
b3cc7fd
Compare
Introduces an optional extended-thinking token budget on LLMAgentConfig so agents that benefit from reasoning over multi-part files can opt in via JSON config without a code change. ResolveAgent propagates the default like the other model parameters. Only providers and models supported by pkg/agent.WithThinking will honour the value. Signed-off-by: Aurélien Sibiril <[email protected]>
The evidence describer now produces a typed EvidenceAssessment (system, setting, scope, captured_at, frameworks, issues, confidence, readable, rejection_reason, summary) instead of a free-text description. The worker persists the full record as JSONB in evidences.assessment and derives Evidence.Description from Assessment.Summary, preserving the existing GraphQL contract. The column description_status is renamed to assessment_status (and the backing enum type alongside it) because the worker now tracks the status of the whole assessment, not just a description. The existing four-state machine (PENDING -> PROCESSING -> COMPLETED | FAILED) and stale-recovery behaviour are unchanged. Agent construction follows the pattern introduced by pkg/vetting: a memoised output schema decorated with an enum on the confidence field, and an optional extended-thinking budget threaded through LLMAgentConfig. Frontend surface is deliberately unchanged; exposing the new fields in GraphQL or the CLI is left as a follow-up. Signed-off-by: Aurélien Sibiril <[email protected]>
The enum-injection workaround for jsonschema-go was duplicated between pkg/vetting and the new evidence describer. Move it to pkg/agent as a method on OutputType so every agent that needs string enums on structured outputs can opt in with one call. Signed-off-by: Aurélien Sibiril <[email protected]>
pgx rejects empty byte slices as invalid JSON, so callers need to send SQL NULL when the raw message is empty. The guard was inlined in Connector.Insert / Connector.Update and added again as a one-off method on Evidence. Collapse all three sites onto a single Arg method on the underlying type. Signed-off-by: Aurélien Sibiril <[email protected]>
coredata owns the DB round-trip, not the LLM-domain schema. The EvidenceAssessment struct and its confidence enum belong with the agent that produces them. coredata exposes generic SetAssessment(any) / AssessmentInto(dst any) helpers that work over json.RawMessage, keeping the persistence layer schema- agnostic. Also: - Scope the PENDING -> PROCESSING transition in the worker by evidence.ID instead of NoScope, matching the later Update calls on the same row. - Wrap the 'no file attached' and the three pgx Exec errors in evidence.go (Upsert, Update, ResetStaleAssessmentProcessing) with cannot-prefixed messages, as required by the error convention. These were pre-existing bare returns preserved from the old evidence_description_worker; cleaning them up now since the same functions are touched here. - Replace the Evidence.assessmentArg helper with the shared Assessment.Arg method from the previous commit. Signed-off-by: Aurélien Sibiril <[email protected]>
assessAndCommit mutates evidence.Description and evidence.Assessment on the shared pointer before writing. If the commit transaction errors out, Process falls through to failEvidence, which used to call the full Evidence.Update and therefore wrote the in-memory mutations (new assessment, new description) back with FAILED status. Replace that with a targeted MarkAssessmentFailed statement that only updates assessment_status, assessment_processing_started_at, and updated_at, so the failure path can never overwrite the canonical columns with partial results. Signed-off-by: Aurélien Sibiril <[email protected]>
A typed-nil pointer (e.g. (*evidencedescriber.EvidenceAssessment)(nil)) wrapped in an interface does not compare equal to untyped nil, so the fast path in SetAssessment was missing it. json.Marshal would then emit the literal 'null', which we would store as 4 bytes of JSONB instead of SQL NULL. Check the marshalled output for "null" and clear the column in that case too. Signed-off-by: Aurélien Sibiril <[email protected]>
The readable example listed ['SOC2', 'ISO27001'] for a Google Workspace 2SV screenshot that names no framework on the file. This contradicted the prompt's own 'do not speculate' rule for the frameworks field and risked training the agent to hallucinate framework tags on any security-looking screenshot. Empty the array in the example and add a parenthetical reminder that explains why. Signed-off-by: Aurélien Sibiril <[email protected]>
The package, agent name, struct (Describer -> Assessor), method (Describe -> Assess), config key (evidence-describer -> evidence-assessor), and worker log scope all still said 'describer' while the output is EvidenceAssessment and the column is evidences.assessment. Align the vocabulary end-to-end so a grep for 'describer' finds nothing. Also: - s/EvidenceFileId/EvidenceFileID/ for Go acronym convention. - Move MarkAssessmentFailed from a method on Evidence (used only e.ID) to a top-level helper taking the ID directly. - Detect typed-nil in Evidence.SetAssessment via reflect so a (*T)(nil) caller no longer persists JSON 'null' instead of SQL NULL. Rename AssessmentInto -> GetAssessment. - Pass evidence by value to assessAndCommit so a failed commit cannot leak partial in-memory mutations to failEvidence. - Trim the prompt: field-by-field descriptions already live on the jsonschema struct tags and were duplicated verbatim. Signed-off-by: Aurélien Sibiril <[email protected]>
The single migration bundled an additive ADD COLUMN, two RENAME COLUMN statements, and a RENAME TYPE. Unrelated DDL changes in one file make rollback and history review harder. Split into three files, each with a single concern: - add evidences.assessment column - rename description_status / description_processing_started_at - rename the evidence_description_status enum type Signed-off-by: Aurélien Sibiril <[email protected]>
Implementing the standard database/sql/driver.Valuer interface lets pgx convert empty JSONB values to SQL NULL automatically, removing the per-call-site Arg() footgun where forgetting the helper would let pgx reject the empty []byte as invalid JSON. Aligns with the existing pattern used by every enum type in this package. Signed-off-by: Aurélien Sibiril <[email protected]>
The worker already has a loaded Evidence at the failure path, so the bespoke MarkEvidenceAssessmentFailed SQL was redundant with Evidence.Update. Setting the three status fields on the Claim-state value copy and calling Update keeps one write path. Signed-off-by: Aurélien Sibiril <[email protected]>
Marshalling the value first and rejecting the literal "null" output clears the field for both untyped-nil and typed-nil pointers without the reflect.Kind switch. Signed-off-by: Aurélien Sibiril <[email protected]>
Spell out the variable name and break Run's ctx and message slice across lines, matching the layout used by the vendor info extractor in pkg/vetting. Signed-off-by: Aurélien Sibiril <[email protected]>
The map-of-fields signature was overkill for the common single-enum case and forced an awkward single-key literal at call sites. Take field name and values as positional args; chain calls when more than one field needs decorating. Also folds the per-package output-type helper inline since the call is now one line. Signed-off-by: Aurélien Sibiril <[email protected]>
Reusing Evidence.Update wrote every column from a snapshot held across the LLM call, which would clobber any concurrent edit to description, assessment, url, etc. landed in that window. Add an Evidence.MarkAssessmentFailed method that touches only the three status columns; the worker calls it instead. Signed-off-by: Aurélien Sibiril <[email protected]>
Leftover from the evidencedescriber → evidenceassessor rename: the error wrapping the constructor failure still said "describer". Signed-off-by: Aurélien Sibiril <[email protected]>
Regenerate pkg/llm/registry_gen.go from the live OpenRouter catalog so the current model lineup is available -- notably Claude Opus 4.8 (plus 4.8-fast and 4.7) and the GPT-5.x family. The previous snapshot topped out at Claude Opus 4.6 / GPT-5.4. Signed-off-by: Aurélien Sibiril <[email protected]>
Close a stale-recovery race on the assessment state machine. The PROCESSING-only guard on SetAssessmentFailed / SetAssessmentCompleted could let a worker whose claim had been recycled clobber a fresh worker's live claim, leaving the row stuck FAILED and dropping a good result. Both terminal transitions now also match the claim's assessment_processing_started_at, so a superseded claim is a no-op; SetAssessmentFailed returns whether it transitioned and the worker logs the superseded case. Stop the assessor from sending a temperature alongside extended thinking. Anthropic rejects that combination and the assessor was the only agent setting both, so every Anthropic run would 400. Temperature is now sent only when thinking is off, mirroring pkg/vetting. Add coredata integration tests covering the claim-ownership guard, the happy-path transitions and stale recovery (skipped without a test database). Also document that legacy COMPLETED rows carry a NULL assessment, correct the now-stale strict-schema comment in pkg/vetting, de-duplicate the worker's file-load error wrap, and refresh a doc example that still referenced the removed evidencedescriber package. Signed-off-by: Aurélien Sibiril <[email protected]>
Finish the describer -> assessor rename on the operator surface. The env vars feeding the renamed config were still named for the describer (AGENT_EVIDENCE_DESCRIBER_* and EVIDENCE_DESCRIBER_*), so an operator using the assessor names was silently ignored. They are now AGENT_EVIDENCE_ASSESSOR_* / EVIDENCE_ASSESSOR_*, with matching .env.example entries (fixing the missing AGENT_ prefix) and bootstrap coverage for the worker-tuning block, which was previously untested. Pin the assessor to a current vision-capable model (gpt-5.4-mini) rather than inheriting the generic AGENT_DEFAULT (gpt-4o). Evidence assessment is vision-first -- it reads screenshots, PDFs and console exports -- so a stale default directly hurt assessment quality. Signed-off-by: Aurélien Sibiril <[email protected]>
b3cc7fd to
a28df40
Compare
The rebase onto main pulled in a stricter .golangci.yml (wsl_v5), which flagged missing blank lines across the evidence assessor changes; add the required whitespace. Also build the evidence assessor before the worker context cancels are created, so evidenceassessor.New's fallible early-return path no longer trips govet's lostcancel on those cancels. Signed-off-by: Aurélien Sibiril <[email protected]>
|
I have opened a new PR based on this one: #1371 |

Summary
EvidenceAssessment(system, setting, scope, captured_at, frameworks, issues, confidence, readable, rejection_reason, summary) validated by a JSON schema instead of free-text. The worker persists the record as JSONB in a newevidences.assessmentcolumn and derivesEvidence.DescriptionfromAssessment.Summary, preserving the existing GraphQLEvidence.description: Stringcontract.evidences.description_status/description_processing_started_at(+ the backing enum type) toassessment_status/assessment_processing_started_at— the worker now tracks the status of the whole assessment, not just a description. The four-state machine (PENDING → PROCESSING → COMPLETED | FAILED) and stale-recovery behavior are unchanged.Thinking *intextended-thinking budget toLLMAgentConfig, propagated viaResolveAgentand wired into the describer. Providers that don't honour extended thinking ignore it.The agent follows the same structured-output + enum-decoration pattern introduced for the vendor assessor in
pkg/vetting; no orchestrator or sub-agent topology was ported since evidence describing has a single input artifact.Out of scope (follow-up)
UPDATE evidences SET assessment_status='PENDING', description=NULL WHERE assessment IS NULL AND type='FILE'post-deploy if desired.Test plan
make migrate-upand confirmpsql -c "\\d evidences"showsassessment jsonb,assessment_status,assessment_processing_started_atgo test ./pkg/coredata/... ./pkg/evidencedescriber/...green (new unit tests coverSetAssessment/GetAssessmentround-trip and the confidence-enum schema decoration)assessment_status = COMPLETED,descriptionmatchesassessment->>'summary',assessment->>'confidence' IN ('HIGH','MEDIUM','LOW'),assessment->>'readable' = 'true'assessment_status = COMPLETED,assessment->>'readable' = 'false',rejection_reasonpopulated,descriptionequals the rejection reasonllm.evidence-describer.thinking = 4000and confirm the describer still produces valid output on supported modelsNotes for reviewers
FOR UPDATE SKIP LOCKED, so an older binary on a stale pod would simply fail its next poll; acceptable given the async nature of the worker. Do not co-deploy the old binary.confidenceis guarded by a unit test that asserts the generated JSON schema still contains the enum — ifagent.NewOutputTypeever reshapes the schema, that test fails loudly rather than silently shipping an un-constrained schema.Summary by cubic
Switch evidence processing to a structured
EvidenceAssessmentstored in JSONB and replace the describer with a new assessor worker. Adds claim-safe state transitions, renames config/env toevidence-assessor, pins a vision-capable default model, and supports an optional thinking budget.Tests
+601-14Coredata
+427-170evidences.assessment(JSONB) and rename status columns/enum to “assessment”.Evidence.SetAssessment/GetAssessmentstore SQL NULL for empty/typed-nil JSON;jsonRawMessageOrNullnow implementsdriver.Valuer.EvidenceAssessmentStatus; guard PROCESSING → terminal transitions byassessment_processing_started_at; addEvidence.MarkAssessmentFailedto update status-only.EvidenceFileId→EvidenceFileIDand update CRUD/queries.Service
+939-1041pkg/evidencedescriberwithpkg/evidenceassessoremitting typedEvidenceAssessment; mirrorsummarytoEvidence.description.evidence_assessment_workerwith stale-claim-safe transitions; failure path updates only status columns.agent.OutputType.DecorateEnum; refreshpkg/llmregistry; skip temperature when extended thinking is set.evidence-assessor; pin default model togpt-5.4-mini; add optionalThinkingonLLMAgentConfig.AssessmentStatusandEvidenceFileID; minor linter and context-order fixes to satisfy wsl_v5 and avoid lostcancel.GraphQL API
+2-2Evidence.descriptionas-is; map file ID viaEvidenceFileID.Agents
+1-1pkg/evidenceassessor.Other
+6-2AGENT_EVIDENCE_ASSESSOR_*/EVIDENCE_ASSESSOR_*; add interval/stale/concurrency and thinking examples.Written for commit 7783c31. Summary will update on new commits.
Note
Medium Risk
Introduces database migrations (new JSONB column and status/type renames) and changes the background worker flow that updates evidence rows, so deploy/migration ordering and data/state transitions need validation.
Overview
Switches evidence processing from a free-text “describer” to a structured
EvidenceAssessmentproduced by a newpkg/evidenceassessoragent, storing the full payload inevidences.assessment(JSONB) while continuing to populate legacyevidences.descriptionfromassessment.summary.Renames evidence processing state tracking from
description_status/description_processing_started_at(and the underlying enum type) toassessment_status/assessment_processing_started_at, updates all evidence CRUD/queries accordingly, and adds safer failure handling viaSetAssessmentFailedto avoid overwriting concurrent edits.Adds
OutputType.DecorateEnumfor JSON-schema enum injection (used by both vendor vetting and the new assessor) and extends LLM agent config with optionalThinkingtokens; also tweaks JSON binding for connector/evidence JSON fields to treat empty values as SQLNULL.Reviewed by Cursor Bugbot for commit 478198b. Bugbot is set up for automated code reviews on this repo. Configure here.