fix(nats): derive CloudEvents subject from metadata instead of hardcoded TODO

[TRIVENI206]

…ded TODO

Summary

This PR replaces the hardcoded CloudEvents subject ("TODO") in the NATS eventing driver with a dynamically derived value from message metadata.

The subject is now determined using the following priority:

• entity_id
• repository_id or artifact_id
• project_id

If none of these values are present, a fallback value ("minder") is used.

This improves event filtering in NATS JetStream and aligns with the CloudEvents specification, while keeping the change minimal and avoiding unrelated refactoring for easier review.

Fixes #6250

Testing

• Built the NATS module using:
  go build ./internal/events/nats/...

• Ran tests for the NATS module:
  go test ./internal/events/nats/...

• Verified that:

  • The code compiles successfully
  • No tests in the NATS module are broken
  • The subject is now set dynamically instead of being hardcoded

[CLAassistant]

All committers have signed the CLA.

[evankanderson]

This seems to be conflicting with the work discussed in #6251 (and runs into the same question as to whether we can get by with simply entity_id given our simplification and standardization on entities last year.

[coveralls]

Coverage is 58.618% — TRIVENI206:fix-nats-subject into mindersec:main. No base build found for mindersec:main.

[TRIVENI206]

Thanks for the feedback!

I based the implementation on multiple metadata fields for broader compatibility, but I see the point about standardizing on entity_id.

Would you prefer simplifying this to use only entity_id (with a fallback if missing), or keeping the current priority-based approach?

Happy to update the PR accordingly.

[evankanderson]

I'd like to use only entity_ids, but only if that is correct. I haven't yet pulled in enough of the surrounding code to know that for sure. Some analysis and/or example cases from a running Minder instance would help.

[TRIVENI206]

Thanks for the guidance!

I looked into how metadata is constructed across the codebase and found that entity_id is consistently used as the primary identifier. There are also indications (e.g., comments about replacing other identifiers) that the system is standardizing around entity_id.

Based on this, I’ve simplified the implementation to rely solely on entity_id (with a fallback), keeping the change aligned with the current design direction.

I’ve also verified that the NATS module builds successfully and tests pass locally .

Please let me know if this matches your expectations.

[evankanderson]

I agree with your assessment -- looking through the internal/providers/github/webhook code, it appears that things tend to bottom out in HandleEntityAndDoMessage, which has an entity and an originator.

It appears that there are also messages around app installation which won't have an entity ID associated with them, but the fixed "minder" string should work just as well as the fixed "TODO" string.

[evankanderson]

This is a stupid little Go trick, but you can use the default-zero return value from map lookup combined with cmp.Or (return first non-zero value) to write this as:

Suggested change

	subject := ""
	if val, ok := msg.Metadata["entity_id"]; ok && val != "" {
	subject = val
	} else {
	subject = "minder"
	}
	event.SetSubject(subject)
	event.SetSubject(cmp.Or(msg.Metadata["entity_id"], "minder"))

[evankanderson]

(You don't have to do the "stupid Go trick", I just wanted to show some of the interesting language patterns)

[TRIVENI206]

Thanks for the suggestion — that’s a neat Go pattern!

I’m happy to update the code to use cmp.Or if you’d prefer that style, or keep the current version for readability. Please let me know which you’d like.

[evankanderson]

I'm fine merging this as-is after CI completes; I'm headed out on a run, but will do a pass this evening (maybe two)

[evankanderson]

It looks like you may need to fix a bad merge on this PR; after that, I'm happy to approve.

[evankanderson]

This file looks like it might be a bad merge -- try git checkout upstream/main internal/db/domain.go

[evankanderson]

I think this has been fixed at head; try git pull upstream main after reverting this change.

[evankanderson]

You still seem to have some merge changes to clean up. You may want to record your changes, and then reset to head and cherry-pick your changes in.

[evankanderson]

You don't need to check for presence if you're already checking that val is non-zero-valued.

Suggested change

	if val, ok := msg.Metadata["entity_id"]; ok && val != "" {
	if val := msg.Metadata["entity_id"]; val != "" {