[Factory Experiment] connect: use deterministic BTreeSet::first for peer/IP selection - #164
Conversation
Replace s.iter().next() with s.first() in connect_name and connect_imid to make the selection strategy explicit and documented. BTreeSet::first() returns the minimum element per Ord, which is deterministic across runs. Add test_connect_name_multi_imid to exercise name → multi-IMID resolution. Add test_connect_imid_multi_ip to exercise IMID → multi-IP resolution.
|
CI flagged some linting issues (& test failures) however were easy to fix by just prompting the model to fix them. Gas Town on Kilo doesn't set up a true dev environment afaict, so it can't run commands like Anyway this should be good to review now, same comment about not being able to assign @ejj-agent to review it. cc @ejj |
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_connect_name_multi_imid() { |
There was a problem hiding this comment.
I'm convince-able but I think this is probably overkill.
I'd suggest instead writing a pure helper function for name_to_imid and imid_to_ip that returns the first element or an anyhow error. Then you can write a very simple unit test for that to ensure the first one is chosen.
There was a problem hiding this comment.
OK let me see if the factory can get to a satisfactory result just by acting on this comment
|
btw @timanglade I think the code in this is pretty good, had a comment but its a reasonable starting point. |
ejj-agent
left a comment
ejj-agent
left a comment
There was a problem hiding this comment.
AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving. This uses the same AI-review process Ethan uses to review his own code.
I reviewed the PR for correctness, simplicity, diff hygiene, and tests. I don't see any additional substantive concerns beyond the existing discussion about whether the new connection-level tests are more integration-heavy than needed for this deterministic selection change.
…pers Replace the heavy integration tests for multi-IMID and multi-IP selection with simple unit tests on the pure helper functions.
ac468a3 to
d653b76
Compare
ejj-agent
left a comment
ejj-agent
left a comment
There was a problem hiding this comment.
AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving. This uses the same AI-review process Ethan uses to review his own code.
Helpers are well-scoped and address the prior reviewer suggestion. The behavior change is effectively a no-op (BTreeSet::iter().next() already returned the minimum), but the explicit .first() plus docstring makes the intent legible. One minor test gap noted inline; otherwise the diff is clean.
| } | ||
|
|
||
| #[test] | ||
| fn test_resolve_imid_to_ip_first() { |
There was a problem hiding this comment.
AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving.
This test puts only one IP in the BTreeSet, so it doesn't actually exercise the "select minimum of multiple candidates" behavior the test name implies — any selection strategy would pass. Mirror test_resolve_name_to_imid_first here: insert two IPs and assert the minimum is returned.
3 participants
Part of an experiment with setting up an automated software factory. Authored by Gas Town on Kilo, using DeepSeek V4 Pro / DeepSeek V4 Flash running on Fireworks. The factory was only fed the content of IM-58, then left completely unsupervised.
Summary
.iter().next()with.first()onBTreeSetinconnect_nameandconnect_imidDetails
The previous code used
s.iter().next()on BTreeSet collections to select a peer when multiple IMIDs or IPs were available. While BTreeSet is inherently ordered, using.first()makes the intent explicit and guarantees deterministic selection.