refactor: typed event payload structs + roundtrip tests (PR F of cleanup)#192
Merged
Conversation
…nup) Server-only refactor; no SDK or proto changes. Promotes the projector decoder Raw structs to exported payload structs in a shared internal/eventtypes/payloads/ package. Handler emit sites now construct typed structs instead of map[string]any literals. Catches handler/projector schema drift at compile time — exactly the bug class behind issue #135's partial-write hazard. One enabler change: store.Event.Data type is broadened from map[string]any to any so call sites can pass typed structs. AppendEvent's existing JSON marshal accepts any JSON-marshalable value; legacy map-literal call sites continue to compile unchanged. Files created: - internal/eventtypes/payloads/{user,device,action,assignment, execution,lps_password,luks_key}.go — 36 typed payload structs across 7 stream types - internal/eventtypes/payloads/doc.go — package docs - internal/eventtypes/payloads/payloads_test.go — 32 roundtrip tests (one per payload), each marshal → unmarshal → assert.Equal Files modified: - internal/store/store.go — Event.Data: any - internal/projectors/{user,device,action,assignment,execution, lps_password,luks_key}.go — decoders use payloads.X structs - ~30 handler emit sites converted to typed payloads: - internal/api/user_handler.go — 11 sites - internal/api/device_handler.go — 6 sites - internal/api/registration_handler.go — DeviceRegistered - internal/api/certificate_handler.go — DeviceCertRenewed - internal/api/system_actions.go — AssignmentCreated + UserSystemActionLinked - internal/control/inbox_worker.go — 5 execution sites + introduced commandOutputPayload helper Deferred to PR G (final tech-debt audit): ~50 emit sites still use map[string]any. They continue to work because Event.Data is now any, but they don't get the compile-time schema-drift guarantee. Mechanical follow-up: add payload structs in remaining files (action.go, internal_handler.go, idp/sso/auth handlers, SCIM, role/group handlers, settings, totp, terminal, compliance) and update emit sites. Note: lps_password and luks_key payload structs preserve the masked LogValue() method from the projector originals so neither password nor passphrase can leak into slog output. Part of the 2026.06 cleanup-release sweep. PR G (final audit) follows.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces a shared typed payloads package, broadens Event.Data to any, updates handlers to emit typed payload structs, refactors projectors to unmarshal those types, and adds comprehensive JSON roundtrip tests. ChangesTyped Event Payloads Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/eventtypes/payloads/lps_password.go (1)
26-39: ⚡ Quick winAdd explicit redaction tests for secret
LogValue()outputs.Please add focused tests asserting
LpsPasswordRotated.LogValue()(and the siblingLuksKeyRotated.LogValue()) never emit raw credential values. This is a small guardrail with strong security payoff.🤖 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/eventtypes/payloads/lps_password.go` around lines 26 - 39, Add unit tests that construct LpsPasswordRotated and LuksKeyRotated instances with known secret values and call their LogValue() methods, then assert the returned slog.Value does not contain the original secret and that the respective fields ("password" for LpsPasswordRotated, "key" or equivalent for LuksKeyRotated) equal the explicit redaction token (e.g. "[REDACTED]"). Locate the LogValue implementations on LpsPasswordRotated and LuksKeyRotated and in tests extract/format the slog.Value or traverse its group entries to verify absence of the raw secret and presence of the redacted string; fail the test if the raw credential is found or the redaction token differs. Ensure tests cover both positive (redacted) and negative (would-be leak) checks using concrete sample secrets.internal/eventtypes/payloads/payloads_test.go (2)
421-460: ⚡ Quick winAdd explicit redaction tests for the secret-bearing payloads.
These cases only verify JSON round-trip. They do not pin the masking contract for
LpsPasswordRotated/LuksKeyRotated, so a future regression could still leakPasswordorPassphraseinto structured logs while this file stays green. A focusedLogValue()/logging assertion here would cover one of the most security-sensitive guarantees in the PR.🤖 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/eventtypes/payloads/payloads_test.go` around lines 421 - 460, Add focused redaction tests that assert LpsPasswordRotated and LuksKeyRotated do not expose secrets when converted to log-friendly form: for each type (payloads.LpsPasswordRotated and payloads.LuksKeyRotated) construct a value containing a clear secret in Password/Passphrase, call the struct's logging representation (LogValue() or the method used by your logging helpers) and assert the returned map/fields do not contain the raw secret and instead contain a redacted marker (or omitted), then keep the existing JSON round-trip checks; reference the types LpsPasswordRotated, LuksKeyRotated and their LogValue()/logging helper to locate where to add the new assertions.
462-484: ⚡ Quick winCover
RawCommandOutputdirectly, not justCommandOutput.
TestRoundtrip_CommandOutputvalidates the struct tags, but it never goes throughpayloads.RawCommandOutput, so it would not catch a regression where the helper starts producing a quoted JSON string or stops omitting nil output. A small helper-focused test would lock down the exact nested-wire contract used byExecutionTerminalandExecutionTimedOut.🤖 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/eventtypes/payloads/payloads_test.go` around lines 462 - 484, TestRoundtrip_CommandOutput only exercises payloads.CommandOutput struct tags and misses the actual nested-wire contract produced/consumed by payloads.RawCommandOutput; add a focused test that constructs a payloads.RawCommandOutput (using the same helper/path that ExecutionTerminal and ExecutionTimedOut use), marshal it to JSON and unmarshal back, and assert the wire shape exactly matches the legacy keys (stdout, stderr, exit_code), that nil/empty fields are omitted as expected and that the value is a JSON object (not a quoted JSON string). Reference payloads.RawCommandOutput and the execution payloads ExecutionTerminal/ExecutionTimedOut when locating the helper to produce the RawCommandOutput.
🤖 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.
Nitpick comments:
In `@internal/eventtypes/payloads/lps_password.go`:
- Around line 26-39: Add unit tests that construct LpsPasswordRotated and
LuksKeyRotated instances with known secret values and call their LogValue()
methods, then assert the returned slog.Value does not contain the original
secret and that the respective fields ("password" for LpsPasswordRotated, "key"
or equivalent for LuksKeyRotated) equal the explicit redaction token (e.g.
"[REDACTED]"). Locate the LogValue implementations on LpsPasswordRotated and
LuksKeyRotated and in tests extract/format the slog.Value or traverse its group
entries to verify absence of the raw secret and presence of the redacted string;
fail the test if the raw credential is found or the redaction token differs.
Ensure tests cover both positive (redacted) and negative (would-be leak) checks
using concrete sample secrets.
In `@internal/eventtypes/payloads/payloads_test.go`:
- Around line 421-460: Add focused redaction tests that assert
LpsPasswordRotated and LuksKeyRotated do not expose secrets when converted to
log-friendly form: for each type (payloads.LpsPasswordRotated and
payloads.LuksKeyRotated) construct a value containing a clear secret in
Password/Passphrase, call the struct's logging representation (LogValue() or the
method used by your logging helpers) and assert the returned map/fields do not
contain the raw secret and instead contain a redacted marker (or omitted), then
keep the existing JSON round-trip checks; reference the types
LpsPasswordRotated, LuksKeyRotated and their LogValue()/logging helper to locate
where to add the new assertions.
- Around line 462-484: TestRoundtrip_CommandOutput only exercises
payloads.CommandOutput struct tags and misses the actual nested-wire contract
produced/consumed by payloads.RawCommandOutput; add a focused test that
constructs a payloads.RawCommandOutput (using the same helper/path that
ExecutionTerminal and ExecutionTimedOut use), marshal it to JSON and unmarshal
back, and assert the wire shape exactly matches the legacy keys (stdout, stderr,
exit_code), that nil/empty fields are omitted as expected and that the value is
a JSON object (not a quoted JSON string). Reference payloads.RawCommandOutput
and the execution payloads ExecutionTerminal/ExecutionTimedOut when locating the
helper to produce the RawCommandOutput.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a4c26867-1a07-4c1e-aa79-f257fd29b5ba
📒 Files selected for processing (23)
internal/api/certificate_handler.gointernal/api/device_handler.gointernal/api/registration_handler.gointernal/api/system_actions.gointernal/api/user_handler.gointernal/control/inbox_worker.gointernal/eventtypes/payloads/action.gointernal/eventtypes/payloads/assignment.gointernal/eventtypes/payloads/device.gointernal/eventtypes/payloads/doc.gointernal/eventtypes/payloads/execution.gointernal/eventtypes/payloads/lps_password.gointernal/eventtypes/payloads/luks_key.gointernal/eventtypes/payloads/payloads_test.gointernal/eventtypes/payloads/user.gointernal/projectors/action.gointernal/projectors/assignment.gointernal/projectors/device.gointernal/projectors/execution.gointernal/projectors/lps_password.gointernal/projectors/luks_key.gointernal/projectors/user.gointernal/store/store.go
) Three nitpick test additions from CR review: - TestLpsPasswordRotated_LogValueMasksPassword: locks the security guarantee — routing the payload through slog (e.g. slog.Warn("...", "payload", payload)) must NEVER emit the raw Password value. Asserts both 'NotContains(secret)' and 'Contains([REDACTED])'. - TestLuksKeyRotated_LogValueMasksPassphrase: sibling for LUKS. - TestRawCommandOutput_NilOmitted + TestRawCommandOutput_ProducesObject: locks the wire contract used by ExecutionTerminal / ExecutionTimedOut. Nil input produces nil so omitempty fires; non-nil input produces a JSON object (NOT a quoted string), with the exact legacy keys (stdout, stderr, exit_code), and round-trips cleanly through ExecutionTerminal.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/eventtypes/payloads/payloads_test.go`:
- Around line 477-484: The test only checks presence of legacy keys but allows
extra keys; after unmarshalling into asMap use a strict check on the key set
(e.g. require.Len(t, asMap, 3) and keep/assert the three contains for "stdout",
"stderr", "exit_code" or assert that the map keys exactly equal that slice) to
enforce the legacy wire shape; apply the same change to the second occurrence
around the asMap check at the other block (the one noted as also applying to
lines 514-520) so both tests fail if any extra keys are present.
- Around line 486-493: The test only checks RawCommandOutput(nil) == nil but
should also assert the parent payload omits the field when marshaled; update
TestRawCommandOutput_NilOmitted to construct an ExecutionTerminal (and/or
ExecutionTimedOut) payload with the command output set to
payloads.RawCommandOutput(nil) (or left nil), json.Marshal that payload, and
assert the resulting JSON does not contain the command output field name (so
omitempty behavior is enforced); reference the payloads.RawCommandOutput helper
and the ExecutionTerminal/ExecutionTimedOut types when adding the
marshal-and-assert step.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3ef33c7-58f6-4a63-b050-a3c5fd937306
📒 Files selected for processing (1)
internal/eventtypes/payloads/payloads_test.go
…atches on PR #192) Two more nitpick test tightenings from CR review: - TestRoundtrip_CommandOutput + TestRawCommandOutput_ProducesObject: added assert.Len(asMap, 3) so an extra key (regression where a new field gets added to CommandOutput) would fail the test. - TestRawCommandOutput_NilOmitted: added end-to-end omitempty contract assertion via ExecutionTerminal — marshal a payload with Output: RawCommandOutput(nil) and assert the wire JSON does NOT contain the 'output' key. Without this the helper could regress to []byte('null') and we'd only catch it via downstream projector failures.
4 tasks
6 tasks
This was referenced Jul 2, 2026
feat(lps): sealed password transport — agent seals to control X25519 key, gateway relays opaque
#483
Merged
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
1 participant
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
Server-only refactor; no SDK or proto changes.
Promotes the projector decoder
Rawstructs to exported payload structs in a sharedinternal/eventtypes/payloads/package. Handler emit sites now construct typed structs instead ofmap[string]anyliterals. Catches handler/projector schema drift at compile time — exactly the bug class behind issue #135's partial-write hazard.One enabler change
store.Event.Datatype is broadened frommap[string]anytoanyso call sites can pass typed structs.AppendEvent's existing JSON marshal accepts any JSON-marshalable value; legacy map-literal call sites continue to compile unchanged.Files created
internal/eventtypes/payloads/{user,device,action,assignment,execution,lps_password,luks_key}.go— 36 typed payload structs across 7 stream typesinternal/eventtypes/payloads/doc.go— package docs (field-tag rules, omitempty/pointer rationale)internal/eventtypes/payloads/payloads_test.go— 32 roundtrip tests (one per payload), eachmarshal → unmarshal → assert.EqualFiles modified
internal/store/store.go—Event.Data: anyinternal/projectors/{user,device,action,assignment,execution,lps_password,luks_key}.go— decoders usepayloads.Xstructsinternal/api/user_handler.go— 11 sitesinternal/api/device_handler.go— 6 sitesinternal/api/registration_handler.go— DeviceRegisteredinternal/api/certificate_handler.go— DeviceCertRenewedinternal/api/system_actions.go— AssignmentCreated + UserSystemActionLinkedinternal/control/inbox_worker.go— 5 execution sites + introducedcommandOutputPayloadhelperDeferred to PR G
~50 emit sites still use
map[string]any. They continue to work becauseEvent.Datais nowany, but they don't get the compile-time schema-drift guarantee. PR G (final tech-debt audit) will sweep them mechanically.Security note
lps_passwordandluks_keypayload structs preserve the maskedLogValue()method from the projector originals so neither password nor passphrase can leak into slog output.Test plan
go build ./...,go vet ./...,staticcheck ./...,gofmt -l .all clean.Part of the 2026.06 cleanup-release sweep. PR G (final tech-debt audit + remaining emit-site sweep + map[string]string sweep) follows.
Summary by CodeRabbit