From 450e6c7e42507b454a1dc128c1be2873fe594d74 Mon Sep 17 00:00:00 2001 From: Nguyen Manh <0xmanhnv@gmail.com> Date: Tue, 4 Aug 2026 09:24:22 +0000 Subject: [PATCH] fix(validation): evidence from a simulation loses its link to the run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit triggerValidationEvidence passed nil for the simulation run id, so every validation_evidence row written by the agent-completes-command path landed with simulation_run_id NULL. On the live database that is all 5 rows, and all 5 are executor_kind=safe-check — every one of them produced BY a simulation, and none traceable back to one. The field is exposed on the evidence API (validation_handler.go), so "which evidence did this run produce?" has always answered empty. The value was never missing. payload.SimulationRunID is the same field the sibling triggerSimulationFinalize already reads to decide which run to finalize; the two paths sitting side by side in this file simply disagreed about whether it existed. Taking it from the command payload rather than from the agent's submission means they now agree by construction, instead of depending on an agent remembering to echo the id back. An unparseable id logs and drops the link rather than the evidence: losing a validation result would turn a provenance problem into a correctness one. The existing mock declared the parameter as `_ *shared.ID` — it discarded the argument, which is why no test could ever have caught this. A mock that ignores a parameter cannot fail when that parameter is wrong. It now captures it, and the new tests cover the link, the no-simulation case (a fabricated link would be worse than an absent one) and the malformed case. Verified fail-before/pass-after by restoring the nil: "evidence was ingested with a nil simulation run id even though the command payload carries one". --- .../infra/http/handler/command_handler.go | 25 +++- .../handler/command_simulation_link_test.go | 118 ++++++++++++++++++ .../handler/command_validation_hook_test.go | 12 +- 3 files changed, 153 insertions(+), 2 deletions(-) create mode 100644 internal/infra/http/handler/command_simulation_link_test.go diff --git a/internal/infra/http/handler/command_handler.go b/internal/infra/http/handler/command_handler.go index 40fb9183..9ec1b91e 100644 --- a/internal/infra/http/handler/command_handler.go +++ b/internal/infra/http/handler/command_handler.go @@ -614,10 +614,33 @@ func (h *CommandHandler) triggerValidationEvidence(cmd *commanddom.Command) { } tenantID := cmd.TenantID + // Carry the simulation link onto the evidence. It was passed as nil here, + // so every row this path wrote had simulation_run_id NULL — on the live + // database all 5, all executor_kind=safe-check, i.e. all produced BY a + // simulation and none traceable back to one. The API exposes the field, so + // "which evidence did this run produce?" answered empty. + // + // The value comes from the command payload, not from the agent: it is the + // same field the sibling triggerSimulationFinalize already reads to decide + // which run to finalize, so the two paths now agree by construction instead + // of by an agent remembering to echo it back. + var simRunID *shared.ID + if payload.SimulationRunID != "" { + if id, sErr := shared.IDFromString(payload.SimulationRunID); sErr == nil { + simRunID = &id + } else { + // Don't fail the evidence over it — a malformed id costs the link, + // not the finding update. + h.logger.Warn("validate command carries an unparseable simulation_run_id", + "command_id", cmd.ID.String(), + "simulation_run_id", payload.SimulationRunID) + } + } + go func() { bgCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - if _, err := h.validationIngest.Ingest(bgCtx, tenantID, findingID, nil, ev); err != nil { + if _, err := h.validationIngest.Ingest(bgCtx, tenantID, findingID, simRunID, ev); err != nil { h.logger.Error("failed to record validation evidence", "command_id", cmd.ID.String(), "finding_id", payload.FindingID, diff --git a/internal/infra/http/handler/command_simulation_link_test.go b/internal/infra/http/handler/command_simulation_link_test.go new file mode 100644 index 00000000..53795955 --- /dev/null +++ b/internal/infra/http/handler/command_simulation_link_test.go @@ -0,0 +1,118 @@ +package handler + +import ( + "encoding/json" + "testing" + + "github.com/openctemio/api/internal/app/validation" + commanddom "github.com/openctemio/api/pkg/domain/command" + "github.com/openctemio/api/pkg/domain/shared" + "github.com/openctemio/api/pkg/logger" +) + +// triggerValidationEvidence passed nil for the simulation run id, so every row +// this path wrote had simulation_run_id NULL. On the live database that was all +// 5 of them — every one executor_kind=safe-check, i.e. every one produced BY a +// simulation and none traceable back to one. The API exposes the field, so +// "which evidence did this run produce?" answered empty. +// +// The value was never missing: payload.SimulationRunID is the same field the +// sibling triggerSimulationFinalize already reads to decide which run to +// finalize. The two paths simply disagreed about whether it existed. +// +// Note the old mock signature was `_ *shared.ID` — it discarded the argument, +// which is exactly why no test could see this. A mock that ignores a parameter +// cannot fail when the parameter is wrong. + +func simulationValidateCommand(t *testing.T, tenantID, findingID shared.ID, simRunID string) *commanddom.Command { + t.Helper() + + payload, err := json.Marshal(validation.ValidateCommandPayload{ + FindingID: findingID.String(), + SimulationRunID: simRunID, + ExecutorKind: "safe-check", + Technique: "T1046", + Target: validation.ValidateTargetPayload{Type: "domain", Address: "example.com"}, + }) + if err != nil { + t.Fatalf("marshal payload: %v", err) + } + + cmd, err := commanddom.NewCommand(tenantID, commanddom.CommandTypeValidate, + commanddom.CommandPriorityNormal, payload) + if err != nil { + t.Fatalf("new command: %v", err) + } + result, err := json.Marshal(validation.ValidateResultPayload{ + Outcome: "not_detected", Summary: "port closed", + }) + if err != nil { + t.Fatalf("marshal result: %v", err) + } + cmd.Complete(result) + return cmd +} + +func TestTriggerValidationEvidence_CarriesTheSimulationLink(t *testing.T) { + ing := &captureIngester{} + h := &CommandHandler{logger: logger.NewNop()} + h.SetValidationIngest(ing) + + simRunID := shared.NewID() + cmd := simulationValidateCommand(t, shared.NewID(), shared.NewID(), simRunID.String()) + + h.triggerValidationEvidence(cmd) + waitFor(t, func() bool { calls, _, _, _ := ing.snapshot(); return calls == 1 }) + + got := ing.simRun() + if got == nil { + t.Fatal("evidence was ingested with a nil simulation run id even though the " + + "command payload carries one. The row lands with simulation_run_id NULL " + + "and the run it came from can never be traced to it") + } + if *got != simRunID { + t.Errorf("simulation_run_id = %s, want %s", got, simRunID) + } +} + +// Validation that is not part of a simulation must stay unlinked — inventing a +// link would be worse than missing one. +func TestTriggerValidationEvidence_NoSimulationStaysNil(t *testing.T) { + ing := &captureIngester{} + h := &CommandHandler{logger: logger.NewNop()} + h.SetValidationIngest(ing) + + cmd := simulationValidateCommand(t, shared.NewID(), shared.NewID(), "") + + h.triggerValidationEvidence(cmd) + waitFor(t, func() bool { calls, _, _, _ := ing.snapshot(); return calls == 1 }) + + if got := ing.simRun(); got != nil { + t.Errorf("simulation_run_id = %s for a command with no simulation; a "+ + "fabricated link is worse than an absent one", got) + } +} + +// A malformed id costs the link, not the finding update. Dropping the evidence +// would turn a cosmetic provenance problem into a lost validation result. +func TestTriggerValidationEvidence_MalformedSimulationIDStillIngests(t *testing.T) { + ing := &captureIngester{} + h := &CommandHandler{logger: logger.NewNop()} + h.SetValidationIngest(ing) + + cmd := simulationValidateCommand(t, shared.NewID(), shared.NewID(), "not-a-uuid") + + h.triggerValidationEvidence(cmd) + waitFor(t, func() bool { calls, _, _, _ := ing.snapshot(); return calls == 1 }) + + if got := ing.simRun(); got != nil { + t.Errorf("an unparseable simulation_run_id produced a link: %s", got) + } + calls, _, _, ev := ing.snapshot() + if calls != 1 { + t.Fatalf("ingest calls = %d, want 1: the evidence must survive a bad link", calls) + } + if ev.Outcome != validation.Outcome("not_detected") { + t.Errorf("outcome = %q, want not_detected — the verdict must be unaffected", ev.Outcome) + } +} diff --git a/internal/infra/http/handler/command_validation_hook_test.go b/internal/infra/http/handler/command_validation_hook_test.go index 12fdaa47..cd8c18ea 100644 --- a/internal/infra/http/handler/command_validation_hook_test.go +++ b/internal/infra/http/handler/command_validation_hook_test.go @@ -19,18 +19,28 @@ type captureIngester struct { tenantID shared.ID finding shared.ID ev validation.Evidence + // simRunID was previously discarded by this mock, which is why it could not + // see that the handler always passed nil. + simRunID *shared.ID } -func (c *captureIngester) Ingest(_ context.Context, tenantID, findingID shared.ID, _ *shared.ID, ev validation.Evidence) (validation.IngestResult, error) { +func (c *captureIngester) Ingest(_ context.Context, tenantID, findingID shared.ID, simRunID *shared.ID, ev validation.Evidence) (validation.IngestResult, error) { c.mu.Lock() defer c.mu.Unlock() c.calls++ c.tenantID = tenantID c.finding = findingID + c.simRunID = simRunID c.ev = ev return validation.IngestResult{StatusChanged: true}, nil } +func (c *captureIngester) simRun() *shared.ID { + c.mu.Lock() + defer c.mu.Unlock() + return c.simRunID +} + func (c *captureIngester) snapshot() (int, shared.ID, shared.ID, validation.Evidence) { c.mu.Lock() defer c.mu.Unlock()