fix(OPENFRAM-006-16): CU-86akbhh82 3 review findings across 2 files - #133
flamingo[bot] wants to merge 2 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| self.events_tx | ||
| .try_send(Event::ClientError(ClientError::Other(other.to_string()))) | ||
| .ok(); | ||
| return Err(error); | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
🦩 🔴 handle_auth_error / auth_url_callback: reconnect loop silently reuses server list on parse failure without dropping obsolete auth state
In handle_auth_error, the finding is about the parse-failure/no-retry path leaving the connector looping with stale server state and only a generic error surfacing. The primary lever available in this function without inventing new fields is to ensure callers no longer silently keep looping on Ok(false). I did not add new state-reset logic inside handle_auth_error itself (there is no "last known-good" snapshot field to restore from, and inventing one would risk correctness), but I fixed the caller side (connect(), see item 2) so that a handle_auth_error returning false after a parse failure now terminates the loop with a real, propagated ConnectError instead of silently falling through forever. This directly resolves the "inconsistent retry state" symptom (infinite fall-through) but does not add explicit rollback of self.servers to a snapshot, since no such snapshot exists in the current struct — a complete fix would require adding a last_known_good_servers: Vec<(ServerAddr, usize)> field and restoring it here, which is a larger structural change I flag as a residual risk.
🤖 Prompt for AI agents
In async-nats/src/connector.rs around line 179, review and complete this code-review fix: handle_auth_error / auth_url_callback: reconnect loop silently reuses server list on parse failure without dropping obsolete auth state.
What the draft fix changed: In `handle_auth_error`, the finding is about the parse-failure/no-retry path leaving the connector looping with stale server state and only a generic error surfacing. The primary lever available in this function without inventing new fields is to ensure callers no longer silently keep looping on `Ok(false)`. I did not add new state-reset logic inside `handle_auth_error` itself (there is no "last known-good" snapshot field to restore from, and inventing one would risk correctness), but I fixed the caller side (`connect()`, see item 2) so that a `handle_auth_error` returning `false` after a parse failure now terminates the loop with a real, propagated `ConnectError` instead of silently falling through forever. This directly resolves the "inconsistent retry state" symptom (infinite fall-through) but does not add explicit rollback of `self.servers` to a snapshot, since no such snapshot exists in the current struct — a complete fix would require adding a `last_known_good_servers: Vec<(ServerAddr, usize)>` field and restoring it here, which is a larger structural change I flag as a residual risk.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| self.events_tx | ||
| .try_send(Event::ClientError(ClientError::Other(other.to_string()))) | ||
| .ok(); | ||
| return Err(error); | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
🦩 🟠 connect() error loop discards MaxReconnects/AuthorizationViolation errors as fire-and-forget events instead of returning them
In Connector::connect(), the ConnectErrorKind::AuthorizationViolation arm and the other (catch-all) arm previously fell through to loop again indefinitely after only doing try_send(...).ok(). Both arms now return Err(error) (propagating the original ConnectError, preserving its source/kind) after emitting the same tracing/event side effects, so a genuinely fatal error (e.g., bad TLS config, or an auth violation with no working callback) is returned to the caller instead of spin-looping forever. This also resolves finding 1's "silently reuses server list" symptom, since the loop can no longer fall through in the parse-failure case.
🤖 Prompt for AI agents
In async-nats/src/connector.rs around line 150, review and complete this code-review fix: connect() error loop discards MaxReconnects/AuthorizationViolation errors as fire-and-forget events instead of returning them.
What the draft fix changed: In `Connector::connect()`, the `ConnectErrorKind::AuthorizationViolation` arm and the `other` (catch-all) arm previously fell through to loop again indefinitely after only doing `try_send(...).ok()`. Both arms now `return Err(error)` (propagating the original `ConnectError`, preserving its `source`/`kind`) after emitting the same tracing/event side effects, so a genuinely fatal error (e.g., bad TLS config, or an auth violation with no working callback) is returned to the caller instead of spin-looping forever. This also resolves finding 1's "silently reuses server list" symptom, since the loop can no longer fall through in the parse-failure case.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| echo "::notice::merge has conflicts — skipping, will retry next run" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "::group::Verify fork-only auth_url_callback feature survived the merge" | ||
| if ! grep -R --include="*.rs" -l "auth_url_callback" . > /dev/null; then | ||
| echo "::error::auth_url_callback fork feature is missing after merging upstream/main — aborting sync, not opening a PR" | ||
| git checkout origin/main | ||
| git branch -D "$BRANCH" | ||
| exit 1 | ||
| fi | ||
| echo "::endgroup::" | ||
|
|
||
| - name: Build and test fork feature gate | ||
| run: | | ||
| if ! grep -R --include="*.rs" -l "auth_url_callback" . > /dev/null; then | ||
| echo "::notice::auth_url_callback not found — skipping build gate (handled above)" | ||
| exit 0 | ||
| fi | ||
| cargo check --workspace --all-features | ||
| cargo test --workspace --all-features -- auth_url_callback | ||
|
|
||
| - name: Push branch and open PR | ||
| run: | | ||
| git push -f origin "$BRANCH" | ||
|
|
||
| gh pr create --repo "$GITHUB_REPOSITORY" --base main --head "$BRANCH" \ | ||
| --title "Sync from Fork" \ | ||
| --body "Automatic weekly sync from \`nats-io/nats.rs@main\`." | ||
| --body "Automatic weekly sync from \`nats-io/nats.rs@main\`. Verified that the fork-only \`auth_url_callback\` feature is present and builds/tests successfully after the merge." | ||
|
|
There was a problem hiding this comment.
🦩 🔴 async-nats/nats.rs contains no auth_url_callback fork feature; sync workflow merges upstream unconditionally
In the Sync step of the sync job in .github/workflows/sync-upstream.yml, added a post-merge verification gate that greps the repo (.rs files) for auth_url_callback after git merge --no-ff --no-edit upstream/main succeeds; if the string is absent, the workflow logs an ::error::, resets the branch back to origin/main, deletes the local sync branch, and exits with failure instead of proceeding to push/PR. Added a new Build and test fork feature gate step running cargo check --workspace --all-features and cargo test --workspace --all-features -- auth_url_callback scoped to run only when the marker is present, and moved the git push/gh pr create into a final Push branch and open PR step so the PR is only opened after both the presence check and the build/test gate pass. This is a heuristic, grep-based proxy for "the fork feature still exists/compiles" since I have no visibility into the actual source files, function names, or test names in this repo — a complete fix would require knowing the real file(s)/API surface implementing auth_url_callback (e.g., a specific struct/trait method in async-nats or nats.rs) so the check and test invocation target it precisely rather than a raw string grep, and would require confirming cargo test ... -- auth_url_callback actually maps to real test names in this crate.
🤖 Prompt for AI agents
In .github/workflows/sync-upstream.yml around line 30, review and complete this code-review fix: async-nats/nats.rs contains no auth_url_callback fork feature; sync workflow merges upstream unconditionally.
What the draft fix changed: In the `Sync` step of the `sync` job in `.github/workflows/sync-upstream.yml`, added a post-merge verification gate that greps the repo (`.rs` files) for `auth_url_callback` after `git merge --no-ff --no-edit upstream/main` succeeds; if the string is absent, the workflow logs an `::error::`, resets the branch back to `origin/main`, deletes the local sync branch, and exits with failure instead of proceeding to push/PR. Added a new `Build and test fork feature gate` step running `cargo check --workspace --all-features` and `cargo test --workspace --all-features -- auth_url_callback` scoped to run only when the marker is present, and moved the `git push`/`gh pr create` into a final `Push branch and open PR` step so the PR is only opened after both the presence check and the build/test gate pass. This is a heuristic, grep-based proxy for "the fork feature still exists/compiles" since I have no visibility into the actual source files, function names, or test names in this repo — a complete fix would require knowing the real file(s)/API surface implementing `auth_url_callback` (e.g., a specific struct/trait method in `async-nats` or `nats.rs`) so the check and test invocation target it precisely rather than a raw string grep, and would require confirming `cargo test ... -- auth_url_callback` actually maps to real test names in this crate.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 3 review findings across 2 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
Warning
This PR edits CI-executable files (workflows, build/manifest definitions). A same-repo PR can run a modified workflow with a write-scoped token as soon as it opens — review those hunks FIRST, before anything else in this PR.
async-nats/src/connector.rs:179async-nats/src/connector.rs:150.github/workflows/sync-upstream.yml:30What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
4fa2224e-8528-443d-a51a-823c78308977Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akbhh82 NATS async-nats and CI review findings (9 PRs)