fix(governance): close actor_mapping pseudonymization bypass#152
Open
jgbr1el93 wants to merge 27 commits into
Open
fix(governance): close actor_mapping pseudonymization bypass#152jgbr1el93 wants to merge 27 commits into
jgbr1el93 wants to merge 27 commits into
Conversation
…n bypass fix [TDD-RED] Encodes post-fix contract from Taura Security pentest finding (28/04/2026): actor_mapping identity fields become append-only; pseudonymization is irreversible per actor_id; mutation attempts surface as HTTP 409 via a new ErrActorMappingImmutable sentinel. Failing tests cover: - Service: CreateOrGetActorMapping (renamed from UpsertActorMapping) for AC1 (new actor), AC2 (idempotent no-op), AC3/AC4 (different email/name), AC5 (pentest PoC: PUT over [REDACTED]). - Repository sqlmock: INSERT ... ON CONFLICT DO NOTHING RETURNING contract, post-INSERT SELECT comparison path, ErrActorMappingImmutable on divergence. - HTTP handler: PUT mapped to 200 (create / idempotent) or 409 (mutation / redacted overwrite attempt). Build is intentionally RED until Gate 0.2 (TDD-GREEN) introduces: - command.ErrActorMappingImmutable - ActorMappingUseCase.CreateOrGetActorMapping - actormapping.ErrActorMappingImmutable - INSERT ... ON CONFLICT DO NOTHING SQL + compare-or-fail logic - Handler error-to-409 mapping
…eation [TDD-GREEN] Addresses Taura Security pentest finding (28/04/2026): pseudonymization of actor mappings could be silently reverted via subsequent PUT requests. The fix introduces: - ErrActorMappingImmutable sentinel in domain/errors, re-exported by the adapter and service layers so errors.Is works across the layer boundary without violating depguard service-no-adapters - INSERT ... ON CONFLICT DO NOTHING + transaction-scoped SELECT compare at the SQL layer, replacing the COALESCE-based upsert that allowed plaintext PII to overwrite [REDACTED] values; the same-transaction compare closes the TOCTOU window - CreateOrGetActorMapping service method documenting the new append-only semantics; UpsertActorMapping kept as a thin wrapper for the existing HTTP handler - 409 Conflict response on PUT attempts that mutate display_name or email, surfaced via the slug governance_actor_mapping_immutable - Idempotent no-op for PUT with identical payload (HTTP PUT semantics preserved, updated_at unchanged) - Swagger documentation of the 409 response Defense-in-depth: validation enforced at both service and SQL layers. Identity-mutation failures use HandleSpanBusinessErrorEvent so they don't inflate 5xx dashboards. The previous test TestUpsert_PreservesExistingFieldsWhenInputIsNil exercised the vulnerable COALESCE behavior; it is replaced by TestUpsert_PartialPayloadRejectedWhenStoredFieldsDiffer covering the new contract.
…iants Adds three property-based fuzz targets covering the helpers that gate the pseudonymization-bypass fix (Taura Security pentest, 28/04/2026): - FuzzStringPtrEqual asserts reflexivity, symmetry, and the documented nil/empty/byte-equality semantics. Any drift here would silently allow a mutation through the conflict path of insertOrCompare. - FuzzActorMappingPIIDiffers asserts reflexivity, symmetry, and the nil-arg "differs" contract — the invariants the immutability check depends on. - FuzzNewActorMapping asserts the XOR(success, error) contract of the constructor and validates the trimming/length sentinels never panic under adversarial UTF-8, NUL bytes, or overlong inputs. Each target ran for 10s with no findings (10 seed inputs total, ~2.5M executions combined).
…nvariants Gate 5 of fix-actor-mapping-pseudonymization-bypass. Encodes domain invariants from the Taura Security pentest finding (28/04/2026) as testing/quick property checks (MaxCount=1000 per property). Properties: - IsRedacted is well-defined (biconditional vs hand-coded reference), partial-redaction always false, nil receiver returns false. - Pseudonymization is irreversible — after a row enters [REDACTED], no Upsert/CreateOrGetActorMapping puts plaintext PII back on disk until DELETE clears the row. - CreateOrGetActorMapping is idempotent for identical args. - Mutation of display_name OR email on an existing row returns ErrActorMappingImmutable and leaves the stored row untouched. Uses a hand-rolled statefulFakeRepository that mirrors the post-fix INSERT...ON CONFLICT DO NOTHING + SELECT + compare semantics from the postgres adapter, driven by a deterministic LCG-decoded trace generator. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…y and TOCTOU resistance Gate 6 WRITE phase for fix-actor-mapping-pseudonymization-bypass. Adds 9 testcontainers-backed integration tests in the actor_mapping adapter package covering the post-fix INSERT ... ON CONFLICT DO NOTHING + SELECT compare contract end-to-end against real PostgreSQL: AC1 NewActor_PersistsAndReturnsEntity AC2 IdempotentSameValues_NoMutation (verifies updated_at pinned) AC3/4 DifferentPayload_ReturnsImmutable (row preserved) AC5 OverRedacted_ReturnsImmutable (plaintext-over-redacted PoC) AC5 OverRedacted_ConcurrentPlaintextAttacks_AllFail (race PoC) AC7 DeleteAfterPseudonymize_Succeeds AC8 ConcurrentDifferentPayloads_NoMutation (load-bearing race) AC8 ConcurrentIdenticalPayloads_AllSucceedIdempotently AC8 ConcurrentFreshActor_OneInsertsRestSeeWinner (TOCTOU on fresh ID) Tests use the shared tests/integration.TestHarness which spins Postgres 17 + Valkey 8 + RabbitMQ via testcontainers, runs the embedded migrations, and provides a tenant-scoped InfrastructureProvider. Tests authored only; orchestrator will execute at cycle-end.
…contract Two pre-existing tests in tests/integration/governance/actor_mapping_test.go encoded the PRE-FIX vulnerable behaviour of the actor_mapping upsert path (COALESCE-based UPDATE on conflict) and would fail under the new immutability contract introduced to close the pseudonymization-bypass finding. - Removed TestIntegration_Governance_ActorMapping_UpsertUpdatesExisting: redundant with TestIntegration_ActorMappingImmutability_DifferentPayload_ReturnsImmutable in internal/governance/adapters/postgres/actor_mapping/actor_mapping_immutability_integration_test.go, which already pins "existing row + different payload → ErrActorMappingImmutable + row unchanged" end-to-end. - Reframed TestIntegration_Governance_ActorMapping_UpsertPreservesExistingFieldWhenOmitted into TestIntegration_Governance_ActorMapping_UpsertPartialPayloadIsRejected: the new path has no COALESCE escape hatch, so a partial PUT (different display_name, nil email) against an existing row must surface ErrActorMappingImmutable and leave the persisted row untouched. This documents the no-COALESCE contract from the package-level integration harness. Pure test-side adjustment; no production code touched.
…r DB failure injection
Authors five Toxiproxy-driven chaos scenarios under build tag `chaos` to
verify the post-fix Upsert path (INSERT ON CONFLICT DO NOTHING + in-tx
SELECT-compare) keeps its immutability guarantee when Postgres misbehaves:
1. Connection drop mid-transaction against an existing row → caller
errors out, row UNCHANGED.
2. Connection drop / cancellation around a fresh-actor INSERT → either
the full payload landed or the row is absent; never partial.
3. Pseudonymized [REDACTED] row + 1s latency + concurrent plaintext
attackers → zero successes, row remains [REDACTED]/[REDACTED].
4. PG proxy fully disabled → repo returns a wrapped error, no panic,
no nil-without-error, no hang (10s watchdog).
5. Disable / re-enable proxy → idempotent re-PUT recovers, persisted
row matches the submitted payload exactly.
Tests WRITE-only at this gate; execution is deferred to cycle-end per
gate cadence. Reuses tests/chaos harness (ChaosHarness, InjectPGResetPeer,
InjectPGLatency, DisablePGProxy, DirectDB) — no new infrastructure.
The actor_mapping immutable handler was using LogSpanError for the 409 Conflict path, which calls libOpentelemetry.HandleSpanError and marks the span as failed. A 409 raised by the post-fix immutability contract is a client-side mistake (the persisted row is untouched), not an infrastructure or server fault — it MUST not pollute the error budget. Adds sharedhttp.LogSpanBusinessEvent which records the event via libOpentelemetry.HandleSpanBusinessErrorEvent (no error attribution) and still emits the SafeError log line. Switches writeConflict to use it and updates the docstring to match. Refs Gate 8 review finding H-1.
Adds CodeGovernanceActorMappingImmutable = "MTCH-0604" to the product error code table and wires the governance_actor_mapping_immutable slug into the shared HTTP error catalog so 409 responses from the actor mapping immutability path surface the stable product code instead of the generic conflict fallback. Clients can now branch on MTCH-0604 to distinguish the pseudonymization-bypass rejection from other conflict modes. Refs Gate 8 review finding M-1.
H-2: The sqlmock regex helpers used .* between SQL tokens, which would silently accept arbitrary SQL injected between, say, DO NOTHING and RETURNING. Replaces .* with \s+ where the gap is whitespace, and bounds the column-list and VALUES segments with [^)]* so injection inside parentheses is also rejected. Removes the dead `regexp` import that only existed to satisfy a `var _ = regexp.MustCompile` compile-time guard. M-2: The three immutability handler tests asserted only the HTTP status code, not the response body. Decodes the JSON body and verifies the product code equals constant.CodeGovernanceActorMappingImmutable (MTCH-0604) and the message describes the immutability constraint. This closes the loophole where the handler could return 409 with a generic "conflict" payload and the test would still pass. Also drops the dead `var _ = http.StatusOK` compile-time guard from the handler test file and updates the file-header comment to "Pins post-fix contract" (the GREEN phase has shipped — the TDD-RED header is stale). Pretty-prints the fuzz seed corpus via gofmt (no behavior change). Refs Gate 8 review findings H-2, M-2, L1, L2.
The pseudonymization-irreversibility property tracked `redacted` by consulting `snap.IsRedacted()` after every operation, which collapsed the invariant into a tautology: the model state was derived from the repository state, so the assertion `if redacted: require snap.IsRedacted()` was always trivially true. Reworks the state oracle so `redacted` is set ONLY by opPseudonymize on an existing row and cleared ONLY by opDelete or the delete step of opRecreate. The repository snapshot is now used exclusively for invariant assertions, never to derive the oracle's flag. The property now genuinely fails if a future regression allows an Upsert/Create call to overwrite a [REDACTED] row. Refs Gate 8 review finding H-3.
…e bucket floor H-4: The five actor_mapping chaos tests called truncateActorMappingDirect without acquiring the shared chaos harness mutex (h.testMu). If the chaos suite is ever run with t.Parallel enabled, the tests would cross- contaminate each other's Toxiproxy injections and direct-DB state. Exposes a new ChaosHarness.LockTest(t) helper that acquires h.testMu and registers t.Cleanup to release. The five actor_mapping chaos tests now call h.LockTest(t) at the top so they are mutually exclusive with each other (and with ResetDatabase-using tests) regardless of -parallel. H-5: PseudonymizedRowUnderLatency_StillRejectsAttacker tolerated runs where 100% of attacker calls fell into the timeout/otherErrors buckets, masking a real bypass behind a degenerate environment. Adds a require.GreaterOrEqual(immutableHits, 1) assertion so at least one attacker MUST observe ErrActorMappingImmutable for the scenario to count as a valid pass. Refs Gate 8 review findings H-4, H-5.
The previous TestActorMapping_Update PUT-ed an existing actor_id with a
different display_name and asserted the row was updated — that
asserted the pre-fix mutation behavior, which the post-fix contract now
rejects with 409.
Replaces the test with two scenarios that pin the new contract:
* TestActorMapping_IdempotentSamePayload — second PUT with identical
display_name/email returns 200 and echoes the same row.
* TestActorMapping_MutationReturnsConflict — second PUT with a
different display_name returns 409 with the
governance_actor_mapping_immutable product code (MTCH-0604); the
persisted row is unchanged.
Uses the existing client.APIError.IsConflict / ProductCode helpers; no
changes to tests/client/governance.go are required.
Refs Gate 8 review finding C-1.
…mutability M-3: The ActorMappingRepository interface doc still described Upsert as "creates or updates an actor mapping" — that wording matches the pre-fix mutable contract. Updates both the type-level and Upsert-level doc comments to reflect the post-fix append-only contract: identity fields cannot be mutated in place; mismatching payloads return ErrActorMappingImmutable; pseudonymization and right-to-erasure remain the only state transitions. M-4: Migration 000001 still carries the original COMMENT ON TABLE text claiming the table is "Mutable". Adds migration 000033 to update the COMMENT to describe the append-only contract and the governance_actor_mapping_immutable (MTCH-0604) rejection. Migration 000001 itself is left untouched per the standard rule against editing applied migrations. The .down.sql restores the pre-fix COMMENT text. Refs Gate 8 review findings M-3, M-4.
M-1: remove dead actorID empty-string reject branch in actor_mapping_immutability_property_test idempotency test. actorID is built from a uint16 seed so it can never be empty; the guard was dead code flagged by ring:test-reviewer. M-2: collapse dead conditional in the irreversibility oracle's opCreate/opRecreate arm. Both branches of "if ok && !redacted else if ok" performed the same assignment; rewrite as a single "if ok" with explicit comment about redacted-flag ownership (set by opPseudonymize, cleared by opDelete/opRecreate-delete) to keep the oracle independent of repo state per commit 9a2dd57. M-3: update ActorMapping entity docstring to reflect the post-fix append-only contract. Old text claimed the table was "mutable by design for GDPR compliance"; new text documents append-only identity fields, the ErrActorMappingImmutable / MTCH-0604 / HTTP 409 rejection, and the two allowed state transitions (Pseudonymize and DELETE), with a pointer to migration 000033. M-4 (skipped): CHANGELOG entry was not added manually because the repo uses semantic-release with @semantic-release/changelog plugin; manual edits would be overwritten. The release-engineering gap (no breaking-change: footer in any branch commit, so semver may emit only a patch bump for a public-API breaking change) is surfaced for the release owner as a follow-up; it is out of Gate 8 cleanup scope. Refs Gate 8 review (ring:codereview iteration 1).
…ping-pseudonymization-bypass Persists the dev-cycle state file and task spec as audit artifacts for the actor_mapping pseudonymization bypass fix. Cycle summary: - Source: docs/tasks/fix-actor-mapping-pseudonymization-bypass.md - Origin: Pentest Taura Security 28/04/2026 - Task: T-001 (1 subtask: ST-001-01) — completed - Total commits in branch: 17 (16 + this artifact commit) - Gates closed (lean backend flow): * Gate 0 (Implementation): TDD-RED 430e81b, TDD-GREEN 4f595b1, plus fuzz/property/integration/chaos/e2e/observability/migration work consolidated under the lean flow's Gate 0 quality umbrella * Gate 8 (Codereview): 10/10 reviewers PASS, iteration 1, 4 MEDIUMs resolved (M-1/M-2/M-3 fixed in commit 2cbee71, M-4 deferred to release engineering) * Gate 9 (Validation): user APPROVED, all 10 ACs verified * Gate 0.5D (Migration Safety): PASS — migration 000033 is documentation-only (COMMENT ON TABLE), zero schema mutation, fully reversible State file is preserved as the cycle audit record. Refs: /ring:dev-cycle resume session 2026-05-14.
Final cycle close for fix-actor-mapping-pseudonymization-bypass-2026-05-13. - status: in_progress → completed - feedback_loop_completed: false → true - task T-001: in_progress → completed - completed_at: 2026-05-14T15:55:00Z ring:dev-report dispatch deferred for context economy; can be re-run via /ring:dev-report when continuous-improvement metrics are desired. All gates closed (lean backend flow): - Gate 0 (Implementation): ✓ - Gate 8 (Codereview, 10/10 PASS): ✓ - Gate 9 (Validation, user APPROVED): ✓ - Gate 0.5D (Migration safety): ✓
Applies all 7 findings from the local CodeRabbit review against the fix-actor-mapping-pseudonymization-bypass branch before opening PR. Grupo A — Swagger field name (3 findings, 1 root cause): - handlers_actor_mapping.go: replace display_name with displayName in the Swagger @description and doc comment for UpsertActorMapping. The request JSON field is camelCase per the DTO, so the snake_case description would have misled SDK consumers. - Regenerated docs/swagger/{docs.go, swagger.json, swagger.yaml} via make generate-docs. Grupo B — Test improvements (3 findings): - actor_mapping_immutability_fuzz_test.go: replace uuid.NewString() with a fixed UUID constant fuzzFixedActorID. Random UUIDs per fuzz iteration violate the project rule of deterministic test data and would make any fuzz failure non-reproducible. - actor_mapping_chaos_test.go:482-484: assert.Error/assert.Nil → require.Error/require.Nil for the graceful-degradation contract (Upsert with PG unreachable MUST return an error, and MUST NOT also return a non-nil entity). Subsequent assertions should not run if this invariant is violated. - actor_mapping_chaos_test.go:393: assert.Equal → require.Equal for the zero-plaintext-overwrite security invariant under chaos latency injection. This is the load-bearing assertion for the entire pseudonymization-bypass fix; subsequent assertions should fail-fast. Grupo C — Chaos harness API hygiene (1 nitpick): - tests/chaos/harness.go: rename ChaosHarness.LockTest → LockHarnessForTest for clarity (avoids "test the lock" misreading). - tests/chaos/common.go: add atomic.Bool testLockHeld field on ChaosHarness. Both LockHarnessForTest and ResetDatabase now CompareAndSwap before acquiring testMu, failing fast via t.Fatalf if the harness is already locked by the same test (instead of deadlocking on double-acquire). All 5 callers in actor_mapping_chaos_test.go updated to the new name. Verification: - go vet ./... clean (including chaos build tag) - go test -tags=unit ./internal/governance/... all green - go test -tags=unit ./tests/... all green - grep display_name in docs/swagger/ → 0 results - grep LockTest in tests/chaos/ → 0 results - grep uuid.NewString in fuzz file → 0 results No production logic changed. Only Swagger doc text, test data sources, and assertion strictness. Refs CodeRabbit local review 2026-05-14, 7 findings, all resolved.
Addresses CodeRabbit re-review findings #5 and #6 by documenting the chaos test suite's intentional single-threaded model, rather than refactoring the testLockHeld CAS-before-mutex pattern. Background: commit 0e6e076 "test(chaos): serialize actor_mapping chaos tests..." established that the chaos suite runs with -p 1 (see Makefile target test-chaos). Under this model, LockHarnessForTest and ResetDatabase never run concurrently from different tests; the testLockHeld flag exists only to detect the same-test double-lock scenario (a test that mistakenly calls both helpers, which would deadlock on testMu). The current CAS-before-mutex pattern fails fast via t.Fatalf when the flag is already set, instead of blocking on the mutex. This is correct under serial execution and is now explicitly documented as deliberate. If the chaos suite is ever changed to permit concurrent test execution, the docstring flags that the CAS pattern must be revisited (the current pattern would incorrectly fail legitimate concurrent tests instead of serializing them through proper mutex contention). Changes: - tests/chaos/harness.go: append SERIAL-EXECUTION INVARIANT block to LockHarnessForTest docstring (full rationale + revisit-trigger note) and a shorter cross-reference paragraph to ResetDatabase docstring. - No code changes; testLockHeld CAS and testMu logic untouched. Findings #1, #2, #3, #4 deliberately deferred: - #1 (state metrics zeroed): metadata, not entrega - #2/#3 (task spec post-hoc edits): task spec is historical spec - #4 (reflectKindCheck): conscious decision from Gate 8 iter 1
Addresses CodeRabbit round-3 finding #1 (minor potential_issue) by appending the explicit error code MTCH-0604 to the @failure 409 annotation on UpsertActorMapping. The 409 response in swagger.json now reads "Actor mapping identity is immutable (MTCH-0604)", making the API contract self-documenting for consumers parsing only the OpenAPI spec (without needing to inspect ErrorResponse examples). Files: - handlers_actor_mapping.go: 1-line annotation update - docs/swagger/{docs.go,swagger.json,swagger.yaml}: regenerated via make generate-docs Other CodeRabbit round-3 findings deliberately skipped: - #2 (CRITICAL, fuzz build tag): CodeRabbit suggested changing //go:build unit → //go:build fuzz in actor_mapping_fuzz_test.go. This conflicts with the Matcher project's local convention: all 23 other fuzz_test.go files use //go:build unit (verified via grep) and there is no test-fuzz Makefile target. The suggestion comes from a generic Ring standard; local convention prevails. Changing the tag would break fuzz execution under `make test-unit` and create inconsistency with the rest of the codebase. - #3 (trivial, reflectKindCheck): repeated nitpick from Gate 8 iter 1 and round 2; conscious decision retained. - #4 (trivial, task spec isolation level clarification): task spec is a historical planning document; post-hoc edits skipped per consistent policy.
CI lint failure on Go Analysis / Lint (matcher) job (run 25940398927) flagged the docstring at actor_mapping.go:63 as "File is not properly formatted (gci)". Root cause: the Immutability contract list items (a) UPDATE... / b) DELETE... used 7 spaces of indentation after the // prefix; golangci-lint's gci/gofumpt enforcement wants 5 spaces to align with the parent list bullet style. Reduced indentation from 7 spaces to 5 spaces on both bullets. No content change — only whitespace inside the comment block. Local make lint earlier in the cycle had stale cache; running make lint-fix now reproduces and corrects the issue. Verified make lint clean post-fix (0 issues).
Applies CodeRabbit Cloud finding C (trivial nitpick, quick win) on PR #150. The TestActorMapping_IdempotentSamePayload e2e journey now also asserts that `CreatedAt` and `UpdatedAt` remain byte-identical between the first and second PUT responses, strengthening the no-op contract for AC2 (idempotent PUT with identical payload). Previously the test only verified that identity fields (displayName, email) round-tripped intact. Adding the timestamp assertions guards against a regression where the repository would touch updated_at on a no-op write — e.g. if the INSERT ... ON CONFLICT DO NOTHING path were ever changed to ON CONFLICT DO UPDATE with COALESCE again. The integration-tier check already pins this at second precision (actor_mapping_immutability_integration_test.go); this e2e assertion adds end-to-end timestamp stability through the full HTTP + API client stack. Other CodeRabbit Cloud findings deliberately deferred (see PR replies): - A (typed input struct for CreateOrGetActorMapping): inconsistent with sibling commands in the same file that use primitive params; would require broader cleanup PR - B (manual stub for ActorMappingRepository): MockActorMappingRepository is already generated by mockgen and used by sibling tests; refactor would create divergence - D (ErrActorMappingImmutable in entity file): sentinel already lives in domain/errors/errors.go parallel to ErrActorMappingNotFound, with type-aliases at service and adapter layers; adding a fourth declaration site duplicates without resolving
…arity Applies CodeRabbit Cloud follow-up nitpick on PR #150 (review at 2026-05-15T21:17:53Z). Extends the timestamp assertions in TestActorMapping_IdempotentSamePayload with: 1. Non-empty assertions on the first PUT response (createdAt, updatedAt) to guard against a false-positive where both responses happen to carry zero-valued timestamps. 2. GET response timestamp parity vs the first PUT response, asserting that GET preserves the same CreatedAt/UpdatedAt the row was inserted with — i.e., the read path does not synthesize new timestamps and the row was never silently rewritten between PUT and GET. Combined with the existing PUT-vs-PUT timestamp equality (commit 0441fe8), the e2e idempotency journey now pins the full chain: - First PUT returns non-zero timestamps - Second PUT echoes the same timestamps (no-op write contract) - GET returns those same timestamps (read path is faithful)
Addresses CodeRabbit Cloud review 4301505026 — the "Arquivos afetados" section of the task spec listed paths/symbols speculated during planning that ended up diverging from the implementation: - ErrActorMappingImmutable moved from services/command/commands.go to domain/errors/errors.go (matches existing governance error pattern; re-exported via type-aliases at service and adapter layers) - IdentityEquals helper was not added to the entity; comparison logic lives in the adapter as actorMappingPIIDiffers / stringPtrEqual - New shared helpers (LogSpanBusinessEvent), constants (CodeGovernanceActorMappingImmutable), and error catalog entries (defGovernanceActorMappingImmutable) were not anticipated by the original task spec but materialized during implementation - Test file inventory now lists the actual filenames (*_immutable_test.go, *_immutability_*_test.go) instead of the original speculative names The task spec now serves as an accurate map of the implementation for future maintainers reading docs/tasks/.
Replaces the literal breaking-change footer-token (uppercase, colon-
suffixed) in the M-4 deferral_reason field of
docs/ring:dev-cycle/current-cycle.json with a lowercase, hyphenated
alternative ("breaking-change conventional-commits footer", no colon).
The original phrasing was descriptive prose, not a declaration — but
semantic-release pattern-matches the canonical footer literal anywhere
it appears, regardless of context. This change is defense-in-depth so
the token never sneaks into release tooling via file contents or
future tooling that greps file bodies.
No behavioral change; cycle history audit intent preserved.
Refs incident 2026-05-15 (accidental v3.0.0-beta.1 emission from #150's
squash merge body); reverted by #151.
WalkthroughThis PR enforces append-only immutability for actor-mapping identity fields by changing persistence to INSERT ... ON CONFLICT DO NOTHING RETURNING with a transactional select-and-compare on conflict, introduces ErrActorMappingImmutable, maps it to HTTP 409 (MTCH-0604), updates Swagger/migrations/docs, and adds comprehensive tests (unit, property, fuzz, sqlmock, integration, e2e, chaos). ChangesActor Mapping Pseudonymization Bypass Fix
Comment |
Contributor
🔒 Security Scan Results —
|
| Stage | Status | Blocking? |
|---|---|---|
| Filesystem Scan | ✅ Clean | — |
| Docker Image Scan | ✅ Clean | — |
| Docker Hub Health Score | ✅ Clean | — |
| Pre-release Version Check | ✅ Clean | — |
Trivy
Filesystem Scan
✅ No vulnerabilities or secrets found.
Docker Image Scan
✅ No vulnerabilities found.
Docker Hub Health Score Compliance
✅ Policies — 4/4 met
| Policy | Status |
|---|---|
| Default non-root user | ✅ Passed |
| No fixable critical/high CVEs | ✅ Passed |
| No high-profile vulnerabilities | ✅ Passed |
| No AGPL v3 licenses | ✅ Passed |
Pre-release Version Check
✅ No unstable version pins found.
Contributor
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 78.9% ✅ PASS |
| Threshold | 70% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/matcher/cmd/generate-casdoor |
31.2% |
github.com/LerianStudio/matcher/cmd/health-probe |
50.0% |
github.com/LerianStudio/matcher/cmd/matcher |
47.0% |
github.com/LerianStudio/matcher/cmd/migration-preflight |
58.6% |
github.com/LerianStudio/matcher/internal/auth |
87.8% |
github.com/LerianStudio/matcher/internal/bootstrap |
72.5% |
github.com/LerianStudio/matcher/internal/configuration/adapters/audit |
96.6% |
github.com/LerianStudio/matcher/internal/configuration/adapters/http/dto |
95.5% |
github.com/LerianStudio/matcher/internal/configuration/adapters/http |
86.9% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/common |
57.1% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/context |
92.3% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/fee_rule |
98.6% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/field_map |
84.3% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/match_rule |
90.9% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/schedule |
78.0% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/source |
94.6% |
github.com/LerianStudio/matcher/internal/configuration/domain/entities |
93.9% |
github.com/LerianStudio/matcher/internal/configuration/domain/repositories/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/configuration/domain/value_objects |
100.0% |
github.com/LerianStudio/matcher/internal/configuration/ports/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/configuration/services/command |
87.4% |
github.com/LerianStudio/matcher/internal/configuration/services/metrics |
87.5% |
github.com/LerianStudio/matcher/internal/configuration/services/query |
100.0% |
github.com/LerianStudio/matcher/internal/configuration/services/worker |
92.5% |
github.com/LerianStudio/matcher/internal/discovery/adapters/fetcher |
88.7% |
github.com/LerianStudio/matcher/internal/discovery/adapters/http/dto |
81.8% |
github.com/LerianStudio/matcher/internal/discovery/adapters/http |
88.5% |
github.com/LerianStudio/matcher/internal/discovery/adapters/m2m |
94.0% |
github.com/LerianStudio/matcher/internal/discovery/adapters/postgres/connection |
89.3% |
github.com/LerianStudio/matcher/internal/discovery/adapters/postgres/extraction |
77.4% |
github.com/LerianStudio/matcher/internal/discovery/adapters/postgres/schema |
87.8% |
github.com/LerianStudio/matcher/internal/discovery/adapters/redis |
87.7% |
github.com/LerianStudio/matcher/internal/discovery/domain/entities |
86.4% |
github.com/LerianStudio/matcher/internal/discovery/domain/repositories |
0.0% |
github.com/LerianStudio/matcher/internal/discovery/domain/value_objects |
100.0% |
github.com/LerianStudio/matcher/internal/discovery/extractionpoller |
91.2% |
github.com/LerianStudio/matcher/internal/discovery/schemacache |
66.9% |
github.com/LerianStudio/matcher/internal/discovery/services/command |
75.1% |
github.com/LerianStudio/matcher/internal/discovery/services/metrics |
86.7% |
github.com/LerianStudio/matcher/internal/discovery/services/query |
79.4% |
github.com/LerianStudio/matcher/internal/discovery/services/syncer |
78.1% |
github.com/LerianStudio/matcher/internal/discovery/services/worker |
71.0% |
github.com/LerianStudio/matcher/internal/exception/adapters/audit |
69.1% |
github.com/LerianStudio/matcher/internal/exception/adapters/http/connectors |
84.4% |
github.com/LerianStudio/matcher/internal/exception/adapters/http/dto |
100.0% |
github.com/LerianStudio/matcher/internal/exception/adapters/http |
88.7% |
github.com/LerianStudio/matcher/internal/exception/adapters/postgres/comment |
79.8% |
github.com/LerianStudio/matcher/internal/exception/adapters/postgres/dispute |
93.6% |
github.com/LerianStudio/matcher/internal/exception/adapters/postgres/exception |
92.0% |
github.com/LerianStudio/matcher/internal/exception/adapters/redis |
81.1% |
github.com/LerianStudio/matcher/internal/exception/adapters/resolution |
91.7% |
github.com/LerianStudio/matcher/internal/exception/adapters |
100.0% |
github.com/LerianStudio/matcher/internal/exception/domain/dispute |
99.1% |
github.com/LerianStudio/matcher/internal/exception/domain/entities |
100.0% |
github.com/LerianStudio/matcher/internal/exception/domain/repositories/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/exception/domain/services |
89.5% |
github.com/LerianStudio/matcher/internal/exception/domain/value_objects |
100.0% |
github.com/LerianStudio/matcher/internal/exception/ports/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/exception/ports |
100.0% |
github.com/LerianStudio/matcher/internal/exception/services/command |
90.0% |
github.com/LerianStudio/matcher/internal/exception/services/query |
98.7% |
github.com/LerianStudio/matcher/internal/governance/adapters/audit |
84.2% |
github.com/LerianStudio/matcher/internal/governance/adapters/http/dto |
98.5% |
github.com/LerianStudio/matcher/internal/governance/adapters/http |
91.5% |
github.com/LerianStudio/matcher/internal/governance/adapters/postgres/actor_mapping |
95.5% |
github.com/LerianStudio/matcher/internal/governance/adapters/postgres/archive_metadata |
83.3% |
github.com/LerianStudio/matcher/internal/governance/adapters/postgres |
95.7% |
github.com/LerianStudio/matcher/internal/governance/domain/entities |
100.0% |
github.com/LerianStudio/matcher/internal/governance/domain/hashchain |
88.5% |
github.com/LerianStudio/matcher/internal/governance/domain/repositories/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/governance/services/command |
76.4% |
github.com/LerianStudio/matcher/internal/governance/services/query |
100.0% |
github.com/LerianStudio/matcher/internal/governance/services/worker |
85.1% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/http/dto |
80.0% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/http |
94.7% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/parsers |
95.9% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/postgres/common |
67.8% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/postgres/job |
95.7% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/postgres/transaction |
95.8% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/rabbitmq |
77.9% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/redis |
80.6% |
github.com/LerianStudio/matcher/internal/ingestion/domain/entities |
96.8% |
github.com/LerianStudio/matcher/internal/ingestion/domain/repositories/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/ingestion/domain/value_objects |
97.1% |
github.com/LerianStudio/matcher/internal/ingestion/services/command |
81.7% |
github.com/LerianStudio/matcher/internal/ingestion/services/metrics |
100.0% |
github.com/LerianStudio/matcher/internal/ingestion/services/query |
83.6% |
github.com/LerianStudio/matcher/internal/matching/adapters/http/dto |
95.0% |
github.com/LerianStudio/matcher/internal/matching/adapters/http |
92.2% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/adjustment |
95.1% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/exception_creator |
95.8% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/fee_schedule |
93.6% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/fee_variance |
95.3% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/match_group |
90.4% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/match_item |
98.7% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/match_run |
96.5% |
github.com/LerianStudio/matcher/internal/matching/adapters/rabbitmq |
72.8% |
github.com/LerianStudio/matcher/internal/matching/adapters/redis |
92.8% |
github.com/LerianStudio/matcher/internal/matching/domain/entities |
98.6% |
github.com/LerianStudio/matcher/internal/matching/domain/enums |
100.0% |
github.com/LerianStudio/matcher/internal/matching/domain/repositories/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/matching/domain/services |
88.0% |
github.com/LerianStudio/matcher/internal/matching/domain/value_objects |
100.0% |
github.com/LerianStudio/matcher/internal/matching/ports/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/matching/services/command |
84.5% |
github.com/LerianStudio/matcher/internal/matching/services/metrics |
87.7% |
github.com/LerianStudio/matcher/internal/matching/services/query |
97.2% |
github.com/LerianStudio/matcher/internal/reporting/adapters/http/dto |
82.2% |
github.com/LerianStudio/matcher/internal/reporting/adapters/http |
91.4% |
github.com/LerianStudio/matcher/internal/reporting/adapters/postgres/dashboard |
92.3% |
github.com/LerianStudio/matcher/internal/reporting/adapters/postgres/export_job |
91.1% |
github.com/LerianStudio/matcher/internal/reporting/adapters/postgres/report |
88.4% |
github.com/LerianStudio/matcher/internal/reporting/adapters/redis |
89.9% |
github.com/LerianStudio/matcher/internal/reporting/adapters/storage |
73.8% |
github.com/LerianStudio/matcher/internal/reporting/domain/entities |
97.5% |
github.com/LerianStudio/matcher/internal/reporting/domain/repositories/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/reporting/ports/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/reporting/services/command |
69.5% |
github.com/LerianStudio/matcher/internal/reporting/services/metrics |
93.3% |
github.com/LerianStudio/matcher/internal/reporting/services/query/exports |
84.6% |
github.com/LerianStudio/matcher/internal/reporting/services/query |
82.1% |
github.com/LerianStudio/matcher/internal/reporting/services/streamingpayload |
100.0% |
github.com/LerianStudio/matcher/internal/reporting/services/worker |
83.4% |
github.com/LerianStudio/matcher/internal/shared/adapters/cross |
63.6% |
github.com/LerianStudio/matcher/internal/shared/adapters/custody |
89.1% |
github.com/LerianStudio/matcher/internal/shared/adapters/http |
90.9% |
github.com/LerianStudio/matcher/internal/shared/adapters/m2m |
65.2% |
github.com/LerianStudio/matcher/internal/shared/adapters/outboxtelemetry |
81.1% |
github.com/LerianStudio/matcher/internal/shared/adapters/postgres/common |
87.6% |
github.com/LerianStudio/matcher/internal/shared/adapters/rabbitmq |
92.5% |
github.com/LerianStudio/matcher/internal/shared/domain/exception |
95.4% |
github.com/LerianStudio/matcher/internal/shared/domain/fee |
93.3% |
github.com/LerianStudio/matcher/internal/shared/domain |
95.4% |
github.com/LerianStudio/matcher/internal/shared/infrastructure/testutil |
45.8% |
github.com/LerianStudio/matcher/internal/shared/objectstorage/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/shared/objectstorage |
67.4% |
github.com/LerianStudio/matcher/internal/shared/observability/metrics |
80.0% |
github.com/LerianStudio/matcher/internal/shared/observability/outboxmetrics |
92.8% |
github.com/LerianStudio/matcher/internal/shared/observability/workermetrics |
95.6% |
github.com/LerianStudio/matcher/internal/shared/observability |
71.4% |
github.com/LerianStudio/matcher/internal/shared/ports/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/shared/ports |
46.8% |
github.com/LerianStudio/matcher/internal/shared/sanitize |
96.0% |
github.com/LerianStudio/matcher/internal/shared/testutil |
98.8% |
github.com/LerianStudio/matcher/internal/shared/utils |
100.0% |
github.com/LerianStudio/matcher/internal/streaming/bootstrap |
77.3% |
github.com/LerianStudio/matcher/internal/streaming/catalog |
91.0% |
github.com/LerianStudio/matcher/internal/streaming/emission |
93.5% |
github.com/LerianStudio/matcher/internal/testutil |
95.4% |
github.com/LerianStudio/matcher/pkg/chanutil |
100.0% |
github.com/LerianStudio/matcher/pkg |
95.8% |
github.com/LerianStudio/matcher/tests/chaos |
84.1% |
github.com/LerianStudio/matcher/tests/client |
63.3% |
github.com/LerianStudio/matcher/tests/integration/ratelimit |
100.0% |
Generated by Go PR Analysis workflow
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ring`:dev-cycle/current-cycle.json:
- Line 46: The file has inconsistent review iteration counts:
accumulated_metrics.review_iterations and metrics.review_iterations are 0 while
the review gate shows "iterations": 1 with reviewer verdicts; update those two
fields (accumulated_metrics.review_iterations and metrics.review_iterations) to
reflect the actual number of review iterations (set to 1) or compute them from
the review gate entries so the state file is internally consistent with the
"review" gate data and verdict counts.
- Line 163: The review flags a semver risk for M-4: the PUT 409 behavior change
is a breaking API change but the current deferral_reason advocates shipping it
as a patch via the auto-generated CHANGELOG.md; update the release metadata so
the change is properly handled—either mark M-4 as a breaking change and update
the release strategy to emit a major bump or, if keeping pre-launch patch
policy, add explicit, prominent release notes and upgrade guidance in
CHANGELOG.md and the release process documentation explaining the 409 behavior
and client migration steps; ensure the identifier "M-4" and the auto-generated
"CHANGELOG.md" entry are updated to reflect whichever path you choose.
- Line 165: Update the _protocol_deviation entry to formalize the conditions
that permit skipping a full 10-reviewer re-run: add explicit criteria
referencing automated verification gates (e.g., go vet and go test results),
allowable severity/scope limits (cosmetic/documentation-only changes), and that
all original reviewers issued PASS in the initial iteration; cite the specific
fixes (M-1/M-2/M-3) and note M-4 as out-of-scope, and state that these criteria
must be met before re-review can be skipped to ensure consistent auditability.
In `@docs/tasks/fix-actor-mapping-pseudonymization-bypass.md`:
- Around line 12-14: Update the reproduction steps to use the PUT
upsert/create-or-get HTTP method consistently: change Step 1 from "POST
/v1/governance/actor-mappings/{ID}" to "PUT /v1/governance/actor-mappings/{ID}"
so it matches the contract and the later steps that call "PUT
/v1/governance/actor-mappings/{ID}", and ensure the subsequent step referencing
"POST /v1/governance/actor-mappings/{ID}/pseudonymize" remains correct (or
adjust to the actual pseudonymize route if different) so the flow between "PUT
/v1/governance/actor-mappings/{ID}", "PUT /v1/governance/actor-mappings/{ID}"
(update), and "/pseudonymize" is consistent.
In
`@internal/governance/adapters/postgres/actor_mapping/actor_mapping_immutability_fuzz_test.go`:
- Around line 159-160: The fuzz test currently rejects cases with invalid UTF-8
for display/email even when those fields are absent; update the validation so
utf8.ValidString(display) and utf8.ValidString(email) are called only when the
corresponding presence indicator for that field is set (e.g., check the
display-present and email-present flags or pointer/non-nil markers used in this
test) instead of unconditionally returning nil; locate the checks around the
variables display and email in actor_mapping_immutability_fuzz_test.go and guard
each utf8.ValidString call with the field's presence check so absent fields can
contain invalid bytes and fuzz paths are not skipped.
In
`@internal/governance/adapters/postgres/actor_mapping/actor_mapping_immutability_integration_test.go`:
- Around line 131-134: The test currently compares only epoch seconds
(originalCreatedAt.Unix() vs result.CreatedAt.Unix() and same for UpdatedAt),
which misses sub-second differences; update the assertions in
actor_mapping_immutability_integration_test.go to assert full timestamp equality
instead of Unix seconds (e.g., use time.Time equality via
originalCreatedAt.Equal(result.CreatedAt) or assert.True with Equal on the
time.Time values) for both the CreatedAt and UpdatedAt checks in the
idempotent-path assertions (the ones referencing originalCreatedAt,
originalUpdatedAt and result.CreatedAt/result.UpdatedAt), and apply the same
change to the other occurrence around lines 470-471 so sub-second changes are
detected.
In `@internal/governance/domain/repositories/actor_mapping_repository.go`:
- Around line 23-30: The Upsert comment is misleading about "exact" matching
because the repository implementation treats nil and "" as equivalent; update
the docblock for Upsert in actor_mapping_repository.go to explicitly state that
identity fields (display_name, email) are compared with nil and empty-string
considered equal (i.e., a nil field and an empty string in the payload are
treated as matching the stored value), and mention this behavior affects
idempotency checks and tests that construct payloads for Upsert so callers/tests
can account for the nil/"" equivalence when expecting ErrActorMappingImmutable
or idempotent success.
In `@internal/governance/services/command/actor_mapping_commands.go`:
- Line 112: Create a dedicated input struct named CreateOrGetActorMappingInput
and update the method signature of CreateOrGetActorMapping to accept this struct
(e.g., ctx context.Context, in *CreateOrGetActorMappingInput) instead of
individual parameters; the struct should contain actorID string and pointer
fields for displayName and email to preserve current semantics, and update all
call sites and internal references inside
ActorMappingUseCase.CreateOrGetActorMapping to read from in.ActorID,
in.DisplayName, in.Email for type safety and documentation.
In
`@internal/governance/services/command/actor_mapping_immutability_property_test.go`:
- Around line 501-506: The errors.Is check against ErrActorMappingImmutable
using err2 is unreachable because err2 is already asserted nil earlier in the
test; remove the dead block that checks errors.Is(err2,
ErrActorMappingImmutable) (and its accompanying defensive comment) from the
actor_mapping_immutability_property_test.go test, or if you prefer to keep the
defensive intent, move the errors.Is check to occur before the nil assertion so
that ErrActorMappingImmutable can be detected on the real error value
(referencing the err2 variable and ErrActorMappingImmutable symbol).
In `@tests/chaos/harness.go`:
- Around line 401-403: The fatal message on CompareAndSwap failure is
misleadingly scoped to “same test”; update the error text used in the
h.testLockHeld.CompareAndSwap(false, true) failure (and the similar case at
443-445) to indicate global harness lock contention rather than per-test (e.g.,
mention "ChaosHarness lock already held" and reference the operations
ResetDatabase/LockHarnessForTest), so diagnostics correctly reflect that the CAS
protects a harness-wide lock, not an individual test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20935f00-7132-4288-b63e-c5d97036d1e4
📒 Files selected for processing (32)
docs/ring:dev-cycle/current-cycle.jsondocs/swagger/docs.godocs/swagger/swagger.jsondocs/swagger/swagger.yamldocs/tasks/fix-actor-mapping-pseudonymization-bypass.mdinternal/governance/adapters/http/handlers_actor_mapping.gointernal/governance/adapters/http/handlers_actor_mapping_immutable_test.gointernal/governance/adapters/postgres/actor_mapping/actor_mapping.postgresql.gointernal/governance/adapters/postgres/actor_mapping/actor_mapping.postgresql_test.gointernal/governance/adapters/postgres/actor_mapping/actor_mapping_immutability_fuzz_test.gointernal/governance/adapters/postgres/actor_mapping/actor_mapping_immutability_integration_test.gointernal/governance/adapters/postgres/actor_mapping/actor_mapping_immutable_sqlmock_test.gointernal/governance/adapters/postgres/actor_mapping/errors.gointernal/governance/domain/entities/actor_mapping.gointernal/governance/domain/entities/actor_mapping_fuzz_test.gointernal/governance/domain/entities/actor_mapping_property_test.gointernal/governance/domain/errors/errors.gointernal/governance/domain/repositories/actor_mapping_repository.gointernal/governance/services/command/actor_mapping_commands.gointernal/governance/services/command/actor_mapping_immutability_property_test.gointernal/governance/services/command/actor_mapping_immutable_test.gointernal/shared/adapters/http/error_catalog.gointernal/shared/adapters/http/handler_helpers.gointernal/shared/adapters/http/handler_helpers_test.gomigrations/000033_actor_mapping_immutable_comment.down.sqlmigrations/000033_actor_mapping_immutable_comment.up.sqlpkg/constant/errors.gotests/chaos/actor_mapping_chaos_test.gotests/chaos/common.gotests/chaos/harness.gotests/e2e/journeys/actor_mapping_test.gotests/integration/governance/actor_mapping_test.go
Addresses 6 actionable findings from the CodeRabbit Cloud review of the re-submitted security fix. All are quality refinements on artifacts we authored; no behavior change to the security contract itself. Code fixes: - fuzz test buildActorMapping (#5): validate UTF-8 only on fields the caller marks as present. The previous guard rejected the entire mapping when an absent field happened to carry invalid bytes — those bytes are never read in the absent branch, so the rejection only shrank the valid input space without protecting any assertion. - integration test idempotency assertions (#6): switch from .Unix() epoch-seconds comparison to time.Time.Equal(). Sub-second drift was previously absorbed by the seconds-precision rounding; the tighter comparison now surfaces any spurious updated_at write introduced by a future regression. Applied at both call sites in the AC2 and AC8-identical-payload tests. - chaos harness fatal message (#10): the t.Fatalf text on the testLockHeld CAS used to say "called twice on the same test", but the CAS is harness-scoped, so any second acquirer (including a future parallel test if the suite is ever de-serialized) would land in the same branch. Rephrase the message to reference the chaos suite serial-execution invariant and the docstring that explains it. Applied to both ResetDatabase and LockHarnessForTest. Documentation: - ActorMappingRepository.Upsert contract (#7): the docstring said identity fields must match "exactly", but the adapter helpers (stringPtrEqual / actorMappingPIIDiffers) intentionally treat nil-pointer and empty-string as equivalent because the DB stores NULL for both. Document the equivalence explicitly in the contract so future maintainers don't read "exactly" as byte-strict including nil-vs-empty. - Task spec PoC step 1 (#4): the pentest reproduction in docs/tasks/fix-actor-mapping-pseudonymization-bypass.md said "POST /v1/governance/actor-mappings/{ID} creates mapping". The actual route is PUT — there is no POST create endpoint. Corrected with a clarifying parenthetical. - Cycle state metrics (#1): docs/ring:dev-cycle/current-cycle.json had accumulated_metrics.review_iterations: 0 despite the cycle documenting Gate 8 iteration 1 plus 4 MEDIUM findings resolved. Updated to review_iterations: 1 and populated issues_by_severity with the actual counts (4 MEDIUM, 22 LOW) so the state file truthfully audits the cycle. Findings #2, #3, #8, #9 are tracked as deferred with replies on the PR threads — they cover policy decisions (pre-launch patch posture), process documentation that was already conscious, sibling-method consistency, and an intentional defensive check.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/tasks/fix-actor-mapping-pseudonymization-bypass.md`:
- Around line 50-53: The doc mismatch: either update the docs to point to the
actual source-of-truth where the sentinel errors are implemented
(internal/governance/domain/errors/errors.go) or move the sentinel definitions
to the claimed location (internal/governance/services/command/commands.go);
specifically ensure the doc lists the sentinel errors ErrActorMappingImmutable
and ErrActorMappingPseudonymized at the real file and also document that the
HTTP handler maps ErrActorMappingImmutable to a 409 response with an explanatory
payload (handler behavior referencing ErrActorMappingImmutable).
In `@internal/governance/services/command/actor_mapping_immutable_test.go`:
- Around line 22-25: Remove the stale TDD-RED comment that starts "All tests in
this file are expected to FAIL until Gate 0.2 (TDD-GREEN)..." in
actor_mapping_immutable_test.go; replace it with a short, accurate note or
delete it entirely so the file no longer claims tests are expected to fail—this
will prevent misleading maintainers about the current passing status of the
tests.
- Around line 64-66: Tests currently instantiate a gomock controller and
mocks.NewMockActorMappingRepository (symbols gomock.NewController and
mocks.NewMockActorMappingRepository) but per guidelines ActorMappingRepository
(interface with Upsert, GetByActorID, PseudonymizeWithTx, Delete) should use a
manual mock; replace the gomock setup in all affected tests by creating a small
local/mock struct that implements ActorMappingRepository with fields to record
calls and configurable return values (e.g., UpsertFunc, GetByActorIDFunc,
PseudonymizeWithTxFunc, DeleteFunc) and use that mock instance in place of
mocks.NewMockActorMappingRepository so each test can set the desired behavior
and assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76846fdd-a8c8-476b-aae3-a98d706ed1d7
📒 Files selected for processing (6)
CHANGELOG.mddocs/ring:dev-cycle/current-cycle.jsondocs/tasks/fix-actor-mapping-pseudonymization-bypass.mdinternal/governance/services/command/actor_mapping_commands.gointernal/governance/services/command/actor_mapping_immutability_property_test.gointernal/governance/services/command/actor_mapping_immutable_test.go
Comment on lines
+50
to
+53
| 4. **Novos erros de domínio** em `internal/governance/services/command/commands.go` (ou arquivo dedicado): | ||
| - `ErrActorMappingImmutable` (mutação tentada) | ||
| - `ErrActorMappingPseudonymized` (subtipo opcional para clareza — pode ser apenas msg diferente do mesmo sentinel) | ||
| 5. **Mapeamento HTTP:** handler converte `ErrActorMappingImmutable` em **HTTP 409** com payload de erro explicativo. |
There was a problem hiding this comment.
Align the sentinel-error location with the implemented source of truth.
Line 50 says the new domain errors live in internal/governance/services/command/commands.go, but the implementation section points to internal/governance/domain/errors/errors.go. Please keep this section consistent to avoid misdirecting future changes.
🧰 Tools
🪛 LanguageTool
[style] ~52-~52: Evite abreviações de internet. Considere escrever “mensagem” por extenso.
Context: ...opcional para clareza — pode ser apenas msg diferente do mesmo sentinel) 5. **Mapea...
(INTERNET_ABBREVIATIONS)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/tasks/fix-actor-mapping-pseudonymization-bypass.md` around lines 50 -
53, The doc mismatch: either update the docs to point to the actual
source-of-truth where the sentinel errors are implemented
(internal/governance/domain/errors/errors.go) or move the sentinel definitions
to the claimed location (internal/governance/services/command/commands.go);
specifically ensure the doc lists the sentinel errors ErrActorMappingImmutable
and ErrActorMappingPseudonymized at the real file and also document that the
HTTP handler maps ErrActorMappingImmutable to a 409 response with an explanatory
payload (handler behavior referencing ErrActorMappingImmutable).
Comment on lines
+22
to
+25
| // All tests in this file are expected to FAIL until Gate 0.2 (TDD-GREEN) | ||
| // implements: (a) the sentinel ErrActorMappingImmutable, (b) the new | ||
| // CreateOrGetActorMapping method, and (c) the service-layer guard that | ||
| // detects mutation attempts against an existing actor_mapping row. |
There was a problem hiding this comment.
Remove the stale TDD-RED expectation text.
Lines 22-25 say these tests are expected to fail until implementation, but this file is now part of the passing suite. Keeping that note is misleading for future maintenance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/governance/services/command/actor_mapping_immutable_test.go` around
lines 22 - 25, Remove the stale TDD-RED comment that starts "All tests in this
file are expected to FAIL until Gate 0.2 (TDD-GREEN)..." in
actor_mapping_immutable_test.go; replace it with a short, accurate note or
delete it entirely so the file no longer claims tests are expected to fail—this
will prevent misleading maintainers about the current passing status of the
tests.
Comment on lines
+64
to
+66
| ctrl := gomock.NewController(t) | ||
| repo := mocks.NewMockActorMappingRepository(ctrl) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "ActorMappingRepository interface:"
rg -n -A30 -B2 'type\s+ActorMappingRepository\s+interface' internal/governance/domain/repositories/actor_mapping_repository.go
echo
echo "gomock usage in actor_mapping_immutable_test.go:"
rg -n 'gomock|NewMockActorMappingRepository' internal/governance/services/command/actor_mapping_immutable_test.goRepository: LerianStudio/matcher
Length of output: 3182
🏁 Script executed:
cat internal/governance/domain/repositories/actor_mapping_repository.goRepository: LerianStudio/matcher
Length of output: 2735
Replace gomock with a manual mock for ActorMappingRepository.
The ActorMappingRepository interface has 4 methods (Upsert, GetByActorID, PseudonymizeWithTx, Delete), which falls below the 5-method threshold for gomock usage. Per coding guidelines, use simple manual mocks for smaller contracts.
Apply to all test functions: lines 64–65, 107–108, 151–152, 177–178, 204–205, 228–229.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/governance/services/command/actor_mapping_immutable_test.go` around
lines 64 - 66, Tests currently instantiate a gomock controller and
mocks.NewMockActorMappingRepository (symbols gomock.NewController and
mocks.NewMockActorMappingRepository) but per guidelines ActorMappingRepository
(interface with Upsert, GetByActorID, PseudonymizeWithTx, Delete) should use a
manual mock; replace the gomock setup in all affected tests by creating a small
local/mock struct that implements ActorMappingRepository with fields to record
calls and configurable return values (e.g., UpsertFunc, GetByActorIDFunc,
PseudonymizeWithTxFunc, DeleteFunc) and use that mock instance in place of
mocks.NewMockActorMappingRepository so each test can set the desired behavior
and assertions.
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
3 participants
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.
Summary
Re-submission of the actor_mapping pseudonymization bypass fix (originally PR #150, reverted by PR #151 due to process violation that triggered an accidental v3.0.0-beta.1 major release).
This branch has been rebased onto current develop and rewritten to eliminate every literal conventional-commits footer token from commit bodies and from in-repo files, ensuring the squash merge produces a patch bump only (
fix(governance):→vX.Y.Z+1-beta.N). The project is pre-launch — no major version bumps are expected.displayName,email) onactor_mappingbecome append-only after creation. Mutation attempts return 409 Conflict with product code MTCH-0604.INSERT … ON CONFLICT DO NOTHING+ transactionalSELECTcompare), service guard, HTTP error mapping.Background
Before this fix:
POST /v1/governance/actor-mappings/{ID}creates mapping with PII (displayName,email)POST /v1/governance/actor-mappings/{ID}/pseudonymizeredacts both fields to[REDACTED]PUTwith plaintext PII would silently overwrite the[REDACTED]row viaON CONFLICT DO UPDATE … COALESCE(...)— pseudonymization was reversibleAfter this fix:
ON CONFLICT DO UPDATEtoON CONFLICT DO NOTHING+ in-transactionSELECTcompare. Pseudonymization is irreversible byactor_id.What changed since the reverted PR #150
The functional content is byte-for-byte the same fix that was reviewed and validated previously. Differences from the original branch:
docs/ring:dev-cycle/current-cycle.json— same token neutralization in the M-4 deferral_reason JSON string (defense-in-depth).529f8d3d) — picks up the auto-generated v2.1.2-beta.1 CHANGELOG entry; no CHANGELOG.md conflicts.b292a239already on develop.Production scope
Production files touched are 100% under
internal/governance/actor_mapping*plus purely additive changes to 3 shared files (1 new error catalog entry, 1 new HTTP helper, 1 new constant). Other features (matching,ingestion,exception,reporting,discovery,configuration,auth) are not touched.internal/governance/services/command/actor_mapping_commands.goerrors.Is(err, ErrActorMappingImmutable)→ span business event +SafeErrorinternal/governance/adapters/postgres/actor_mapping/actor_mapping.postgresql.goINSERT … ON CONFLICT DO NOTHING+ tx-localSELECT+ payload compare viaactorMappingPIIDiffers/stringPtrEqualinternal/governance/adapters/http/handlers_actor_mapping.gowriteConflict+LogSpanBusinessEvent;@Failure 409swagger annotationinternal/governance/domain/errors/errors.goErrActorMappingImmutable(source of truth, type-aliased in adapter + service)internal/shared/adapters/http/error_catalog.go,pkg/constant/errors.goMTCH-0604/governance_actor_mapping_immutablemigrations/000033_actor_mapping_immutable_comment.{up,down}.sqlCOMMENT ON TABLEdocumenting the contract (documentation-only, fully reversible)Acceptance criteria
All 10 ACs implemented and verified via multi-layer tests (sqlmock, property, fuzz, integration, chaos, e2e):
email→ 409, dados inalteradosdisplayName→ 409, dados inalterados[REDACTED]preservado (cenário do PoC do pentest)POST /pseudonymizecontinua redigindo ambos os campos[REDACTED]HandleSpanBusinessErrorEvent+SafeActorIDPrefix(não polui dashboards 5xx)ErrorResponse+MTCH-0604Review trail (previously closed)
The original PR #150 went through:
Re-review by a human Lerian maintainer is requested before merge of THIS PR.
Behavior change for API consumers
fix(governance):→ patch via semantic-release.Test plan
email→ 409 withMTCH-0604/pseudonymize→ 204; row becomes[REDACTED][REDACTED]preservedmake migrate-up && make migrate-down VERSION=32)v3.0.0jump on next semantic-release run after merge — should bump from currentv2.1.2-beta.1to the next patch betaRefs