- Notifications
You must be signed in to change notification settings - Fork927
fix(coderd/agentapi): make sub agent slugs more unique#18581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The incorrect assumption that slugs were unique per-agent was made whenthe subagent API was implemented. Whilst this PR doesn't completelyenforce that, we instead compute a stable hash to prefix the slug thatshould provide a reasonable level of probability that the slug will beunique.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR addresses the incorrect assumption of unique subagent slugs per agent by prefixing each slug with a stable CRC32 hash computed from the subagent name and original slug.
- Updated test expectations in subagent_test.go to check for the new hash-prefixed slug format.
- Modified the slug generation in subagent.go to compute and append a CRC32 hash to the original slug.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| coderd/agentapi/subagent_test.go | Updated test cases to expect new hash-prefixed slugs. |
| coderd/agentapi/subagent.go | Added CRC32-based hash computation to generate unique slugs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Except for my confidence in crc32, otherwise LGTM 👍
coderd/agentapi/subagent.go Outdated
| // there is no database-layer enforcement of this constraint. | ||
| // We can get around this by creating a slug that *should* be | ||
| // unique (at least highly probable). | ||
| slugHash := fmt.Sprintf("%08x", crc32.ChecksumIEEE([]byte(subAgent.Name+"/"+app.Slug))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wonder if we should use crypto/sha256 or hash/fnv instead? Crc32 isn’t particularly good for collision prevention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sounds good! I've made the change in3e9f2f3.
Now we use a sha256 hash, encode it with base32 encoding, truncate it to 8 characters, then lower case it.
6c713d5 intomainUh oh!
There was an error while loading.Please reload this page.
The incorrect assumption that slugs were unique per-agent was made when the subagent API was implemented. Whilst this PR doesn't completely enforce that, we instead compute a stable hash to prefix the slug that should provide a reasonable level of probability that the slug will be unique.