fix: require Metadata.CorrelationID at Intent construction - #192
Conversation
…ow-up) Rusty's post-merge review on #191 found a real gap: Intent is documented (both in its own field comment and NewIntent's doc comment) as the anchor for the intent -> proposal -> risk decision -> request/order -> fill correlation chain, but NewIntent only required Metadata.EventID to be non-zero, not Metadata.CorrelationID. A caller could construct a valid Intent with no correlation identifier, leaving every later stage with nothing to correlate back to -- exactly the gap the docs claimed didn't exist. Since Intent is the first/anchor stage, nothing later can retroactively assign a CorrelationID an intent never had. NewIntent now requires Metadata.CorrelationID non-zero, matching the documented contract, with a regression test (TestNewIntentRejectsZeroMetadataCorrelationID). Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_014JmhCNQ3Nh3veVXzZifMa9
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, matches documented intent invariants, and is covered by an added targeted regression test.
Pull request overview
This PR tightens order.NewIntent validation to match existing documentation by requiring Metadata.CorrelationID (in addition to Metadata.EventID) so the intent → downstream order lifecycle correlation chain is guaranteed to be anchored at intent creation.
Changes:
- Require non-zero
Metadata.CorrelationIDinorder.NewIntentvalidation. - Update intent tests to supply a correlation ID in fixtures and add a regression test for rejecting zero correlation IDs.
- Add a test helper to generate
id.CorrelationIDvalues.
File summaries
| File | Description |
|---|---|
| order/intent.go | Enforces non-zero Metadata.CorrelationID and updates constructor documentation accordingly. |
| order/intent_test.go | Updates fixtures for the new requirement and adds a regression test for missing correlation IDs. |
| order/helpers_test.go | Adds mustCorrelationID helper for generating correlation IDs in tests. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
rustyeddy
left a comment
rustyeddy
left a comment
There was a problem hiding this comment.
Reviewed latest head. This is the right narrow fix for the #191 correlation-gap finding: NewIntent now requires both Metadata.EventID and Metadata.CorrelationID, the constructor docs match the invariant, the shared fixture supplies a real correlation ID, and there is a focused regression test proving a zero CorrelationID is rejected. I don't see a new blocker in these changes. Looks ready to merge.
What changed
order.NewIntentnow requiresMetadata.CorrelationIDto benon-zero, in addition to the existing
Metadata.EventIDrequirement.Why
Rusty's post-merge review comment on #191 found a real gap:
Intentis documented — both in its own field comment and
NewIntent's doccomment — as the anchor for the
intent -> proposal -> risk decision -> request/order -> fillcorrelation chain, but the constructor onlyenforced a non-zero
EventID, notCorrelationID. A caller couldtherefore construct a valid
Intentwith no correlation identifier,leaving every later stage with nothing to correlate back to. Since
Intentis the first/anchor stage of that chain, nothing downstreamcan retroactively assign a
CorrelationIDan intent never had — thefix is to require it now, matching what the docs already claimed.
Test plan
go build ./...,go vet ./...,gofmt -l .all cleango test ./... -racepasses across the whole repositoryTestNewIntentRejectsZeroMetadataCorrelationIDorderpackage coverage remains 100.0%Addresses feedback on #191
🤖 Generated with Claude Code
https://claude.ai/code/session_014JmhCNQ3Nh3veVXzZifMa9