fix(controls): compensating controls could not be created or applied - #354
Merged
Conversation
The create form sent values no row could hold, so the feature had zero rows
on every tenant.
- control_type offered preventive/detective/corrective/compensating. The
database CHECK allows segmentation/identity/runtime/detection/other —
zero overlap, so every create failed.
- reduction_factor was entered and sent as a percent ("20"), but the column
is DECIMAL(3,2) CHECK (>= 0 AND <= 1). The list then rendered a stored
0.30 as "0.3%", so read and write disagreed with each other as well as
with the database.
- Recording a test sent PATCH to a route registered as POST, so the Test
button returned 405 and never worked.
- Every catch was bare (`catch {}`), so the API's error message was
discarded and the user saw only "Failed to create control".
There was also no way to link a control to an asset, and the asset link is
the only thing that makes a control affect scoring — so even a control that
saved successfully did nothing.
Both vocabularies and the percent<->fraction conversion now live in
src/features/controls/vocabulary.ts, and the Selects are rendered from it,
so the form cannot offer a value the backend rejects. The form speaks
percent (which is how operators think about it) and converts at the API
boundary; whole percents map exactly onto the two-decimal storable set, so
the round trip is lossless.
The create dialog states plainly what a control does today: it caps a
protected asset's findings at P2 rather than P1. The percentage is recorded
and shown as rationale but does not scale the result further, and the copy
no longer implies otherwise.
Control-type badges lost their hardcoded light/dark colour pairs — a
category is not a risk level, and a neutral badge keeps the page out of the
dark-mode drift the UI review flagged.
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.
Pairs with openctemio/api#404. Merge that first — this PR sends the values it validates.
The defect
compensating_controlshas 0 rows on every tenant, including tenants where the module is released and enabled. Not because nobody tried, but because the create form could not produce a valid row.control_typepreventive|detective|corrective|compensating(defaultcompensating)segmentation|identity|runtime|detection|otherreduction_factor"20"DECIMAL(3,2) CHECK (>= 0 AND <= 1)Zero overlap on the type, and
20is 20x the ceiling on the factor. Every create hit a CHECK violation, came back as a 500, and the page showed "Failed to create control" because everycatchwas bare and discarded the server's message.Three further faults in the same page:
{reduction_factor}%, so a stored0.30displayed as "0.3%". Read and write disagreed with each other as well as with the database.PATCHto/{id}/test, but the route is registered asPOST— a 405, swallowed by the same bare catch.What changed
One source of truth —
src/features/controls/vocabulary.tsholds both vocabularies and the unit conversion, and every<Select>is rendered by mapping over it. The form can no longer offer a value the backend rejects, because there is no second list to drift from.Asset linking — a new
LinkAssetsDialog(searchable, multi-select, modelled on the existingbulk-add-assets-dialog) posting toPOST /{id}/assets. This is the change that makes the feature do anything at all.Honest errors —
getErrorMessage(error, fallback)replaces the bare catches, so the API's new 400s ("control_type must be one of: …") actually reach the user. This page was the outlier; 382 other call sites already did this.Honest copy — the create dialog now says what a control actually does today:
That last clause is deliberate.
reduction_factoris presentational: the classifier is binary onIsProtected, so a 5% and a 95% control produce an identical P2. Rather than delete the field or quietly imply it does more than it does, the UI states the truth. Making the factor band the outcome would change risk scoring for every tenant and belongs in its own change.The unit decision
The form speaks percent; the API keeps the 0–1 fraction. Converted at the single API boundary via
percentToFactor/factorToPercent.The wire format is not negotiable — it is what the column stores. Percent in the UI is right because it is how operators talk about risk reduction and it was already the field's label. The conversion is safe rather than a new source of drift: the column is
DECIMAL(3,2), so whole percents map exactly onto the storable set. A test asserts the round trip is lossless for all 100 values and that every one satisfies the CHECK.Also dropped the
controlTypeColorsmap. Control type is a category, not a risk level, so a neutral badge is more honest — and it removes five hardcoded light/dark colour pairs from a page the UI review flagged for growing dark-mode drift.Verification
tsc --noEmitvitest run(full suite)npm run build(after a realnpm ci)bash scripts/check-palette-drift.sh origin/developno new hardcoded palette classesprettier --check/eslintThe drift guards were proven red. Reverting the page to the legacy default (
control_type: 'compensating'), the raw percent, and the raw-fraction renderer fails exactly the three tests that should fail:The page test asserts the actual request body the form puts on the wire satisfies both CHECK constraints — not just that a constant matches — so it fails if the form ever regresses regardless of how the vocabulary is wired.