docs(adr): correct nats-core + nats-jetstream evaluation#2104
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the architecture decision records regarding the NATS client library. It corrects a previous misinterpretation of the modular NATS client family's capabilities and provides a comprehensive technical analysis of the migration path. The decision to remain on the current monolithic nats-py client is maintained, with a clear, trigger-based plan for future re-evaluation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🧰 Additional context used📓 Path-based instructions (1){README.md,docs/**/*.md}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (7)📚 Learning: 2026-05-16T18:36:19.195ZApplied to files:
📚 Learning: 2026-05-16T18:36:31.446ZApplied to files:
📚 Learning: 2026-05-16T18:36:35.250ZApplied to files:
📚 Learning: 2026-05-16T18:36:31.446ZApplied to files:
📚 Learning: 2026-05-16T18:36:35.250ZApplied to files:
📚 Learning: 2026-05-16T18:36:35.250ZApplied to files:
📚 Learning: 2026-05-16T18:36:35.250ZApplied to files:
🔇 Additional comments (1)
WalkthroughThis PR updates the architectural decision document to correct a prior misread about the nats-core Python client library. The original ADR incorrectly stated that nats-core lacks JetStream, KV store, and durable consumer support; in fact, these features are provided by companion packages (nats-jetstream, nats-key-value, etc.) in a deliberately modular design. The changes restate the decision to remain on nats-py==2.14.0 with a scoped filterwarnings workaround, document current package availability and versions, inventory SynthOrg's actual NATS/JetStream/KV usage, compare API surface deltas between nats-py and the modular family, and replace a prior fixed-date checkpoint with explicit trigger-based revisit conditions (Triggers A–C) plus a calendar revisit date of 2026-08-01. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Code Review
This pull request updates the NATS client library architectural decision record to correct a previous misinterpretation of the modular nats-io/nats.py client family. The changes include a detailed comparison of API surfaces, a live package state analysis, and a new trigger-based revisit strategy. Feedback focuses on ensuring consistency with the project's dependency pinning strategy by recommending exact version pins for future releases instead of version ranges in the documentation.
| **Decision:** Stay on `nats-py` (pinned `==2.14.0`). File upstream PR to replace deprecated `asyncio.iscoroutinefunction` with `inspect.iscoroutinefunction`. Maintain scoped `filterwarnings` as workaround until upstream fix lands. | ||
| > **Correction note:** the 2026-04-11 evaluation rejected `nats-core` on the basis that it "lacks JetStream, KV store, and durable consumers". That was a misread of the modular `nats-io/nats.py` client family (see caspervonb's [PR #1228 comment](https://github.com/Aureliolo/synthorg/pull/1228#issuecomment-4271799539) and issue [#2037](https://github.com/Aureliolo/synthorg/issues/2037)). The conclusion (stay on `nats-py`) holds; the reasoning below is the corrected evaluation. | ||
|
|
||
| **Decision:** Stay on `nats-py==2.14.0` with the scoped `filterwarnings` entry. Unpin to `nats-py>=2.15` and drop the filter the moment that release ships (`nats-io/nats.py` PR [#932](https://github.com/nats-io/nats.py/pull/932), merged 2026-05-13, contains the `inspect.iscoroutinefunction` swap). |
There was a problem hiding this comment.
The project consistently uses exact version pins for dependencies in pyproject.toml (e.g., nats-py==2.14.0). The ADR suggests using a range (>=2.15), which deviates from this convention. It is better to specify an exact pin for the future release to maintain consistency.
**Decision:** Stay on `nats-py==2.14.0` with the scoped `filterwarnings` entry. Bump the pin to `nats-py==2.15.0` and drop the filter the moment that release ships (`nats-io/nats.py` PR [#932](https://github.com/nats-io/nats.py/pull/932), merged 2026-05-13, contains the `inspect.iscoroutinefunction` swap).| **Trigger-based revisit** (replaces the prior fixed 2026-06-10 checkpoint): | ||
|
|
||
| **Verification checkpoint (2026-06-10):** on this date run the checklist below and update this section with the outcome (mark each item Done / Not done / Outcome). | ||
| 1. **Trigger A -- `nats-py 2.15+` is released:** bump the pin in `pyproject.toml` to `nats-py>=2.15`, drop the scoped `filterwarnings` entry, run `uv run python -m pytest tests/ -m integration -k nats` to confirm no deprecation warnings fire, and update this section with the resolution outcome. This closes the Python 3.14 thread without any further migration consideration. |
There was a problem hiding this comment.
To maintain consistency with the project's dependency pinning strategy, specify an exact version for the future nats-py release.
1. **Trigger A -- `nats-py 2.15+` is released:** bump the pin in `pyproject.toml` to `nats-py==2.15.0` (or the latest stable version), drop the scoped `filterwarnings` entry, run `uv run python -m pytest tests/ -m integration -k nats` to confirm no deprecation warnings fire, and update this section with the resolution outcome. This closes the Python 3.14 thread without any further migration consideration.Apply Gemini Code Assist suggestions on docs/architecture/decisions.md: swap nats-py>=2.15 -> nats-py==2.15.0 to match the repo's exact-pin dependency convention (pyproject.toml:108 pins nats-py==2.14.0). Lines 118 (Decision block) and 173 (Trigger A).
1b1ee7b to
bbed413
Compare
Summary
Corrects the "NATS Client Library" section of
docs/architecture/decisions.md. The 2026-04-11 ADR (PR #1228, closing #1217) concluded thatnats-core"lacks JetStream, KV store, and durable consumers" and used that to justify staying onnats-py==2.14.0. caspervonb pointed out on that PR that the conclusion is based on a misread of the modular client family:nats-coreis the protocol layer, and JetStream/KV live in companion packages (nats-jetstreamandnats-key-value), mirroring thenats.jsv3 split.The rewritten section records:
nats-io/nats.pymain):nats-core 0.2.0,nats-jetstream 0.3.0(both Beta on PyPI),nats-key-value 0.1.0(Beta, workspace-only, not yet on PyPI), andnats-py 2.14.0still the current monolith.nats-io/nats.pyPR #932 (theinspect.iscoroutinefunctionfix for Python 3.14) merged 2026-05-13 but has not been tagged for release yet.nats-core.src/synthorg/communication/bus/and the upstream source files. Includes a confirmed regression risk forpublish_batch:nats-jetstream 0.3.0does not exposepublish_async/publish_async_completedon eitherJetStreamorStream.nats-py>=2.15when that release ships, (B) re-evaluate migration whennats-key-valueis on PyPI AND the family is at 1.0, (C) re-evaluate urgency ifnats-pyis silent for six months. Plus a 2026-08-01 calendar revisit so a quiet period does not let the trigger drift.Decision: stay on
nats-py==2.14.0with the scopedfilterwarningsentry, per user-confirmed Path A on 2026-05-24. No follow-up migration issue filed; acceptance criterion 4 of #2037 is satisfied vacuously by the user's choice and durably captured by the Trigger-based revisit section.Scope
Docs-only. No changes to
src/synthorg/communication/bus/**,pyproject.toml, or the scopedfilterwarningsentry.Test plan
git diff main...HEADconfirms onlydocs/architecture/decisions.mdis touched.markdownlint,trim trailing whitespace,fix end of files,no em-dashes,codespell) clean on commit.lychee,doc numeric macros, etc.) clean on push.docs-consistencyagent: every file path, API call, error type, and pin location confirmed accurate (see review coverage below).Review coverage
Pre-reviewed by 3 agents on the corrected ADR:
docs-consistency: zero findings; every concrete claim in the ADR verified against the codebase.comment-quality-rot: one Minor finding (migration framing carried over from the prior ADR text in the Context paragraph); fixed in1b1ee7b1e.issue-resolution-verifier: all four acceptance criteria of Re-evaluate nats-core + nats-jetstream migration: original ADR was based on misread packaging #2037 marked RESOLVED.Closes #2037