feat: getActiveModels helper for cache-aware discovery - #10
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e7d201b07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export async function getActiveModels(cache, discover) { | ||
| // BUG: fire-and-forget refresh — returns possibly-stale cache without awaiting discovery | ||
| if (cache.isEmpty()) { | ||
| discover(); // missing await — race: caller gets empty snapshot |
There was a problem hiding this comment.
Await discovery before returning active models
When the cache is empty and discover populates it asynchronously, this fire-and-forget call lets getActiveModels resolve with cache.list() before discovery has updated the cache, so callers can receive an empty or stale model list despite awaiting this helper. Awaiting discover() before listing the cache avoids the race and also propagates discovery failures instead of leaving them unhandled.
Useful? React with 👍 / 👎.
project820
left a comment
There was a problem hiding this comment.
🤖 Psycho-842 [BOT] review
⚖️ Verdict: NEEDS REVISION — the empty-cache path returns before asynchronous discovery has populated the cache.
Summary
This PR adds an async getActiveModels helper in a new fixture file.
On its primary empty-cache path, it starts discovery without awaiting it, then immediately returns the current (empty or stale) snapshot.
The repository checks pass, but they do not exercise this new .mjs helper.
Findings (by severity)
- 🔴 P1 High —
src/gate-e2e-fixture.mjs:5:discover()is fire-and-forget while the next line returnscache.list(). With an empty cache and asynchronous discovery, an awaitedgetActiveModels()resolves to[]before discovery writes the models; discovery failures are also detached from the caller. Await discovery before listing (and add a regression test for the empty-cache path and rejection propagation). The async gremlin is not waiting for anyone.
Response to bot findings
chatgpt-codex-connectorreported the same missing-await race on line 5. Measured reproduction confirmed it: the helper returned[]while discovery was still pending, and the cache became["model-A"]only later. The concern is therefore retained as a P1 finding rather than dismissed.
📋 Detailed analysis (measured CI, diff, and verification)
- Reviewed HEAD:
0e7d201b07e243a511199a8e3466219019afbe51; mergeable:MERGEABLE; merge state:UNSTABLE. - Diff measured: +8/−0 across 1 file (
src/gate-e2e-fixture.mjs).git diff --check origin/main...HEADproduced no whitespace errors. - Tip CI measured via the check-runs API:
verifycompleted withsuccesson this exact HEAD. The commit-status API reported no legacy statuses. - Files actually opened:
src/gate-e2e-fixture.mjs,package.json, and.github/workflows/ci.yml. - Caller trace:
git grep -nE 'getActiveModels|gate-e2e-fixture'found only this helper's definition; there are no in-repository callers to validate the intended contract against. - Local verification:
npm ci && npm test && npm run typecheck && npm run buildcompleted successfully (19 test files, 245 tests passed; typecheck and build passed). The CI workflow also runs those commands. - Targeted execution with an empty cache and a 25 ms async
discoverreturned[]immediately withdiscoveredImmediately: false; after 35 ms the cache contained["model-A"]. This directly reproduces the race. - Unresolved review threads: one Codex thread on
src/gate-e2e-fixture.mjs:5, addressed above. - Cross-reference: no matching issues were found for
getActiveModels; the only matching PR is this PR. The other open PR (#9) changesdocs/psycho842-card-test.md, so it does not overlap this file. No revert history touched this file. - This is a COMMENT review only, not an approval; user final sign-off remains required.
🛠️ NEEDS REVISION
🦀 Reviewed by Hermes agent BOT 🦞
Adds a small async helper. Should be safe — cache-first with background refresh. Will close after gate test.