fix(controls): compensating controls were impossible to create and inert - #404
Merged
Conversation
Compensating controls had 0 rows on every tenant because the feature was broken end to end, in four independent places. 1. Create could never succeed. control_type/status/reduction_factor went straight into the INSERT unvalidated, so a value outside the CHECK constraints surfaced as a 500 with "internal error" instead of a 400 saying what was wrong. Validation now mirrors the CHECKs, sourced from the pkg/domain/compensatingcontrol constants (which already carried the correct vocabulary and had zero importers). 2. Even a VALID create returned 500. Create/Update/RecordTest scanned their own RETURNING clause into plain strings, but description, test_result, test_evidence and created_by are NULL on a fresh row — "converting NULL to string is unsupported", raised AFTER the INSERT committed. The caller saw a failure while the control really had been created. All five read sites now share one null-safe scanControl. 3. Compensating controls could not affect priority on the path that control changes actually drive. Linking an asset publishes a reclassify sweep (LinkAssets -> Reclassifier.reclassifyAsset -> ClassifyFinding), but ClassifyFinding never consulted the control lookup — only the batch and explain paths did. The fan-out built to make control changes move priority fed a classifier that could not see controls. The lookup is now applied through one shared helper used by all three paths. 4. RecordTest with test_result='fail' left status='active', so the API reported a failed control as active (scoring was saved only by the effective-control SQL separately excluding failures). It now deactivates, matching the domain rule. An empty/unknown test_result is a 400, not a 500. reduction_factor stays a 0-1 fraction on the wire — it is what the column (DECIMAL(3,2), CHECK 0..1) stores. A 0 factor is now rejected at create: the classifier only treats an asset as protected when the factor is > 0, so a control saved with the column default would have been a silent no-op. Note on scope: item 3 changes classification outcomes, but only for assets an operator has deliberately linked to an effective control. It applies the existing binary IsProtected rule on a path that was missing it; it does not make reduction_factor band the outcome. The factor remains presentational (a 0.05 and a 0.95 control both yield P2) — banding would change scoring for every tenant and needs its own change and its own argument.
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
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.
The defect
compensating_controlshas 0 rows on every tenant, including tenants where the module is released and enabled. The feature was unreachable and, had it been reachable, inert. Four independent faults, each sufficient on its own.1. Create could never succeed — and failed dishonestly
Createputcontrol_type,statusandreduction_factorstraight into the INSERT with no validation beyond a non-empty name. The live CHECK constraints allowsegmentation | identity | runtime | detection | other; the create form offeredpreventive | detective | corrective | compensating— zero overlap. A constraint violation then fell into the generic error arm and became500 {"message":"internal error"}, so a plain client input mistake looked like a server fault.Validation now mirrors the CHECKs and returns 400 with the allowed values. The allowlist is sourced from
pkg/domain/compensatingcontrol, which already carried exactly the right vocabulary as typed constants and had zero importers — the handler is raw SQL and bypassed it entirely.2. Even a valid create returned 500
Found while writing the integration test.
Create/Update/RecordTestscanned their ownRETURNINGclause into plainstringfields, butdescription,test_result,test_evidenceandcreated_byare all NULL on a freshly inserted row:The error is raised after the INSERT has already committed — the caller got a 500 while the control really had been created.
List/Getwere already null-safe; the three write paths were not. All five read sites now go through onescanControl.3. Controls could not affect priority on the path control changes actually drive
This is why the feature would still have been a note even once creatable.
Linking an asset to a control publishes a reclassify sweep, and that sweep runs:
ClassifyFindingnever consulted the control lookup. OnlyEnrichAndClassifyBatchand the read-onlyExplainFindingdid. So the B2 fan-out — machinery built specifically so control changes reflect in priority — fed the one classifier that could not see controls.ExplainFindingwould report "Compensating controls present (reduction: 30%)" on a finding thatClassifyFindinghad just written as P1; the explain endpoint and the live classifier disagreed. The inline copy inpriority_explanation.goeven carried the comment "same as the live classify path", which was false.All three paths now go through one shared helper, so a fourth path cannot silently diverge.
4. A failed test left the control reporting itself as active
RecordTestwithtest_result='fail'leftstatus='active'. Scoring survived only because the effective-control SQL separately excludestest_result='fail', but the API response was misleading. It now deactivates, matching the domain rule incompensatingcontrol.RecordTest. Deactivation is deliberately one-way — a later passing test does not silently re-activate a control an operator disabled for other reasons. An empty or unknowntest_result(which violated thetest_resultCHECK and 500'd) is now a 400.The
reduction_factorunitKept as a 0–1 fraction on the wire, because that is what the column stores:
DECIMAL(3,2) CHECK (>= 0 AND <= 1). The UI change (openctemio/ui#TBD) keeps a percent-labelled field and divides by 100 at the API boundary — integer percents map exactly onto the storable set at scale 2, so the conversion is lossless and the wire contract is unchanged.A 0 factor is now rejected at create. The column defaults to
0.0and the classifier only marks an asset protected when the factor is> 0, so a control saved without one was accepted and then did nothing.Scope note — this does change classification, narrowly
Item 3 changes classification outcomes, but only for assets an operator has deliberately linked to an effective control, and there are currently zero such controls anywhere. It applies the existing binary
IsProtectedrule on a path that was missing it.It does not make
reduction_factorband the outcome. The factor is still presentational — a 0.05 control and a 0.95 control both produce P2, and the value only reaches a reason string. Making it band the result would change risk scoring for every tenant and deserves its own change and its own argument.Verification
go build ./...go vet ./...go test ./...make lint-ci(golangci-lint v1.64.8 + vet + staticcheck)Both new tests were proven red against the unfixed code:
applyControlProtectioncall fromClassifyFinding→ClassifyFinding must consult the compensating control lookupCreateto the name-only check → the legacy payload reachesdb.QueryRowContextinstead of being rejectedThe integration tests run against the real schema and assert the CHECK constraints directly, so the Go allowlist cannot drift from the database without a test failing.