Skip to content

fix(go-core): align thread participants#146

Merged
rowan-stein merged 1 commit into
mainfrom
noa/issue-145
May 25, 2026
Merged

fix(go-core): align thread participants#146
rowan-stein merged 1 commit into
mainfrom
noa/issue-145

Conversation

@casey-brooks
Copy link
Copy Markdown
Contributor

@casey-brooks casey-brooks commented May 25, 2026

Summary

Tests

  • go test ./...
  • go vet ./...
  • go test -tags "e2e svc_agents_orchestrator" -run '^TestNoDuplicateWorkloads$' ./tests attempted locally and reached the expected local environment boundary (dial agents:50051: context deadline exceeded) because the Kubernetes E2E stack is not running in this workspace.

@casey-brooks
Copy link
Copy Markdown
Contributor Author

Update for #145

Implemented the thread contract and cleanup fixes.

Summary

  • createThread now uses participants (ParticipantIdentifier) instead of deprecated participant_ids.
  • createThread filters out the caller identity from initial participants, so the initiator is not sent in the participants list.
  • TestNoDuplicateWorkloads cleanup now deletes the created agent env and archives the thread before deleting the agent, preventing FK cleanup failures after early errors.

Test & lint summary

  • go test ./...: 0 failed; packages reported no test files.
  • go vet ./...: passed with no errors.
  • go test -tags "e2e svc_agents_orchestrator" -run '^TestNoDuplicateWorkloads$' ./tests: attempted locally; failed at environment boundary because the local Kubernetes E2E stack is not running (dial agents:50051: context deadline exceeded).

Copy link
Copy Markdown

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I found one blocking issue in the shared thread helper: it silently proceeds when the initiator identity is missing from the outgoing context, which can reintroduce the invalid participant list. Please make that identity requirement explicit/fail-fast before merge.

Comment thread suites/go-core/tests/main_test.go
@casey-brooks
Copy link
Copy Markdown
Contributor Author

Update after review

Addressed Noa's strict initiator identity requirement.

Summary

  • createThread now calls requireIdentityIDFromOutgoingContext, which fails loudly if outgoing identity metadata is missing or empty.
  • The participant list is only built after a non-empty initiator identity is present, so the helper cannot silently send the initiator in the participants list when context metadata is missing.

Test & lint summary

  • go test ./...: 0 failed; packages reported no test files.
  • go vet ./...: passed with no errors.
  • go test -tags "e2e svc_agents_orchestrator" -run '^TestNoDuplicateWorkloads$' ./tests: attempted locally; failed at environment boundary because the local Kubernetes E2E stack is not running (dial agents:50051: context deadline exceeded).

@rowan-stein
Copy link
Copy Markdown
Collaborator

Heads up: I merged threads PR that fixes the underlying contract (initiator auto-included + cascade deletes): agynio/threads#36. This may supersede the need to change go-core createThread behavior in e2e PR #146.

Copy link
Copy Markdown

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed with the merged threads fix in mind. Since agynio/threads#36 now fixes the underlying CreateThread contract and cascade cleanup, the createThread compatibility/filtering change in this PR should be removed so the E2E suite exercises the fixed service behavior. Cleanup hardening in the dedup test can remain if still useful.

Comment thread suites/go-core/tests/main_test.go Outdated
@casey-brooks
Copy link
Copy Markdown
Contributor Author

Update after re-review

Adjusted PR #146 now that agynio/threads#36 is merged.

Summary

  • Reverted the createThread helper changes so it matches main behavior again and passes participant_ids through unchanged.
  • Removed the initiator filtering/strict metadata logic from the shared helper so E2E exercises the fixed threads service behavior directly.
  • Kept the TestNoDuplicateWorkloads cleanup hardening: delete agent env, archive thread, then delete agent.

Test & lint summary

  • go test ./...: 0 failed; packages reported no test files.
  • go vet ./...: passed with no errors.
  • go test -tags "e2e svc_agents_orchestrator" -run '^TestNoDuplicateWorkloads$' ./tests: attempted locally; failed at environment boundary because the local Kubernetes E2E stack is not running (dial agents:50051: context deadline exceeded).

Copy link
Copy Markdown

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review complete. The createThread workaround has been reverted, so E2E now exercises the fixed threads service behavior from agynio/threads#36. The remaining changes are limited to TestNoDuplicateWorkloads cleanup hardening, and CI is green on the current head SHA.

@rowan-stein rowan-stein merged commit 058ee78 into main May 25, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

go-core TestNoDuplicateWorkloads: thread create invalid participants (initiator included)

3 participants