fix(docs): canonicalize NATS URLs to authenticated form in documentation - #1375
Merged
Merged
Conversation
Replace `nats://localhost:4222` with `nats://nats:pmoves@nats:4222` across documentation, Makefile defaults, and context files. Resolves remaining unauthenticated NATS references in: - Context documentation (.claude/context/nats-subjects.md) - Makefile inline script defaults (supaserch-smoke, deepresearch-smoke) - Example manifests and operation guides Code files were already fixed in prior convergence wave. This completes the P0 NATS authentication migration from AGNOTE4482. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR updates NATS connection endpoints across documentation, configuration, and examples to use authenticated, container-network addresses ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f23428aad
ℹ️ 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".
| .PHONY: deepresearch-smoke | ||
| deepresearch-smoke: ## Publish a sample DeepResearch request and wait for a result (requires worker running) | ||
| @ENV_FILE="$(CURDIR)/env.shared" bash -lc '. ./scripts/with-env.sh "$$ENV_FILE" && PYTHONPATH="$(CURDIR)/.." python3 tools/deepresearch_smoke.py --nats nats://localhost:4222 --timeout $${DEEPRESEARCH_SMOKE_TIMEOUT:-60}' | ||
| @ENV_FILE="$(CURDIR)/env.shared" bash -lc '. ./scripts/with-env.sh "$$ENV_FILE" && PYTHONPATH="$(CURDIR)/.." python3 tools/deepresearch_smoke.py --timeout $${DEEPRESEARCH_SMOKE_TIMEOUT:-60}' |
There was a problem hiding this comment.
This change drops the explicit --nats override, so tools/deepresearch_smoke.py now inherits NATS_URL from with-env.sh (defaulting to nats://nats:pmoves@nats:4222), but this Make target runs on the host shell rather than inside the compose network. In the default setup, host DNS cannot resolve nats, so make deepresearch-smoke fails before publishing any request; previously the hardcoded localhost URL avoided that. Please keep a host-reachable authenticated URL here (for example nats://nats:pmoves@127.0.0.1:4222) or explicitly map Docker-only aliases for host execution.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pmoves/services/agent-zero/skills/.template/cookbook/examples.md (1)
188-188: Prefer env-based NATS URL in template examples (avoid hardcoded credentials).At Line 188, hardcoding a credential-bearing URL in a reusable template can spread insecure patterns; use
os.getenv("NATS_URL", "...")instead.As per coding guidelines, "pmoves/services/**: Focus on PMOVES secret hardening conventions: Prefer central env helpers and *_FILE secret loading paths."Suggested snippet update
+import os ... - await nc.connect("nats://nats:pmoves@nats:4222") + await nc.connect(os.getenv("NATS_URL", "nats://nats:pmoves@nats:4222"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/agent-zero/skills/.template/cookbook/examples.md` at line 188, Replace the hardcoded credential-bearing NATS URL used in the nc.connect call with an environment-driven value: read NATS URL from the central env helper or os.getenv (and support *_FILE secret loading per PMOVES conventions) and pass that variable to nc.connect instead of the literal "nats://nats:pmoves@nats:4222"; ensure the template example falls back to a safe default only if appropriate and references the project-wide env helper or secret-file loader so credentials are not embedded in the template.pmoves/services/agent-zero/skills/.template/prompts/feature-branch.md (1)
95-103: Make the template use an env-overridable NATS URL.This template currently teaches new feature code to hardcode the broker URL. Keep the authenticated default, but read
NATS_URLfirst so generated skills work across host, compose, and CI environments.♻️ Proposed template update
import asyncio +import os from nats.aio.client import Client as NATS async def publish_event(subject: str, data: dict): """Publish event to NATS message bus.""" nc = NATS() try: - await nc.connect(servers=["nats://nats:pmoves@nats:4222"]) + await nc.connect( + servers=[os.getenv("NATS_URL", "nats://nats:pmoves@nats:4222")] + ) await nc.publish(subject, json.dumps(data).encode())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/agent-zero/skills/.template/prompts/feature-branch.md` around lines 95 - 103, The publish_event function hardcodes the NATS broker URL; change it to read an environment variable NATS_URL (falling back to "nats://nats:pmoves@nats:4222") before calling nc.connect so generated skills can be configured per-host; add the necessary import for os (and ensure json is imported where publish_event uses json.dumps), then use that env-derived URL in the nc.connect(servers=[...]) call inside publish_event to replace the literal string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pmoves/Makefile`:
- Line 2447: The default NATS URL used by publish_and_wait() is a compose-only
hostname ("nats://nats:pmoves@nats:4222") which can fail when the Python runs on
the host; update the os.getenv("NATS_URL", "...") call inside publish_and_wait
to use a host-reachable default (e.g. "nats://nats:pmoves@127.0.0.1:4222" or
"nats://nats:pmoves@localhost:4222") so host-run smoke targets can resolve and
authenticate, and mirror the same change for the other occurrence of the
NATS_URL default in this diff range.
In `@pmoves/services/graph-linker/README.md`:
- Line 73: The Docker run example uses hostname-based service discovery (neo4j,
nats) but omits joining a shared Docker network; update the README example (the
docker run command shown) to include a --network option and mention creating or
using an existing network (e.g., docker network create <name>) so the neo4j and
nats hostnames resolve correctly; keep the example consistent by naming the
network (e.g., pmoves-network) and add a brief note that containers must be
started on that same network.
---
Nitpick comments:
In `@pmoves/services/agent-zero/skills/.template/cookbook/examples.md`:
- Line 188: Replace the hardcoded credential-bearing NATS URL used in the
nc.connect call with an environment-driven value: read NATS URL from the central
env helper or os.getenv (and support *_FILE secret loading per PMOVES
conventions) and pass that variable to nc.connect instead of the literal
"nats://nats:pmoves@nats:4222"; ensure the template example falls back to a safe
default only if appropriate and references the project-wide env helper or
secret-file loader so credentials are not embedded in the template.
In `@pmoves/services/agent-zero/skills/.template/prompts/feature-branch.md`:
- Around line 95-103: The publish_event function hardcodes the NATS broker URL;
change it to read an environment variable NATS_URL (falling back to
"nats://nats:pmoves@nats:4222") before calling nc.connect so generated skills
can be configured per-host; add the necessary import for os (and ensure json is
imported where publish_event uses json.dumps), then use that env-derived URL in
the nc.connect(servers=[...]) call inside publish_event to replace the literal
string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2a9c8a3-e23b-4fb5-ae71-b91186dbcc69
📒 Files selected for processing (13)
pmoves/.claude/context/nats-subjects.mdpmoves/Makefilepmoves/docs/AGENTS/ALIGNED_IMPLEMENTATION_ROADMAP.mdpmoves/docs/PINOKIO_EXAMPLE_MANIFESTS.mdpmoves/docs/PINOKIO_PACKAGING_GUIDE.mdpmoves/docs/PMOVES.AI PLANS/SMOKETESTS.mdpmoves/docs/operations/SMOKETESTS.mdpmoves/docs/submodules/SUBMODULE_GEOMETRIC_INTEGRATION_SURVEY.mdpmoves/services/agent-zero/skills/.template/cookbook/examples.mdpmoves/services/agent-zero/skills/.template/prompts/feature-branch.mdpmoves/services/graph-linker/README.mdpmoves/services/hf-mcp-server/README.mdpmoves/tests/functional/test_a2ui_bridge_integration.py
| supaserch-smoke: ## Publish SupaSerch request via NATS and verify HTTP fallback responds | ||
| @echo "→ Publishing SupaSerch smoke request on supaserch.request.v1" | ||
| @bash -lc $'$(LOAD_ENV_SHARED) python3 - <<"PY"\nimport asyncio\nimport json\nimport os\nimport sys\nimport uuid\nimport urllib.request\nfrom urllib.error import URLError, HTTPError\n\nfrom nats.aio.client import Client as NATS\n\n\nasync def publish_and_wait() -> dict[str, object]:\n request_id = f"supaserch-smoke-{uuid.uuid4().hex[:8]}"\n nc = NATS()\n url = os.getenv("NATS_URL", "nats://localhost:4222")\n await nc.connect(url)\n loop = asyncio.get_running_loop()\n future: asyncio.Future | None = loop.create_future()\n\n async def handler(msg):\n nonlocal future\n try:\n data = json.loads(msg.data.decode("utf-8"))\n except json.JSONDecodeError:\n return\n if data.get("request_id") != request_id:\n return\n if future and not future.done():\n future.set_result(data)\n\n sid = await nc.subscribe("supaserch.result.v1", cb=handler)\n payload = {\n "request_id": request_id,\n "query": "supaserch smoke verification",\n "correlation_id": request_id,\n "trigger": "make supaserch-smoke",\n }\n await nc.publish("supaserch.request.v1", json.dumps(payload).encode("utf-8"))\n await nc.flush()\n try:\n result = await asyncio.wait_for(future, timeout=10)\n except asyncio.TimeoutError:\n print("✖ Did not receive supaserch.result.v1 within 10s")\n await nc.unsubscribe(sid)\n await nc.drain()\n sys.exit(1)\n await nc.unsubscribe(sid)\n await nc.drain()\n fallback = result.get("fallback", {}) if isinstance(result, dict) else {}\n if fallback.get("status") != "ok":\n print("✖ NATS fallback status not ok:", json.dumps(fallback))\n sys.exit(1)\n via = fallback.get("via", "unknown")\n latency = fallback.get("latency_ms", 0)\n print(f"✔ NATS round-trip complete (via {via}, latency {latency} ms)")\n return result\n\n\nresult = asyncio.run(publish_and_wait())\nhost_port = os.getenv("SUPASERCH_HOST_PORT", os.getenv("SUPASERCH_PORT", "8099"))\nhttp_url = f"http://localhost:{host_port}/v1/search?q=supaserch+smoke+http"\ntry:\n with urllib.request.urlopen(http_url, timeout=8) as resp:\n body = json.loads(resp.read().decode("utf-8"))\nexcept (HTTPError, URLError, TimeoutError) as exc:\n print(f"✖ HTTP fallback request failed: {exc}")\n sys.exit(1)\n\nfallback = body.get("fallback", {}) if isinstance(body, dict) else {}\nif fallback.get("status") != "ok":\n print("✖ HTTP fallback status not ok:", json.dumps(fallback))\n sys.exit(1)\n\nvia = fallback.get("via", "unknown")\nlatency = fallback.get("latency_ms", 0)\nprint(f"✔ HTTP fallback responded (via {via}, latency {latency} ms)")\nPY' | ||
| @bash -lc $'$(LOAD_ENV_SHARED) python3 - <<"PY"\nimport asyncio\nimport json\nimport os\nimport sys\nimport uuid\nimport urllib.request\nfrom urllib.error import URLError, HTTPError\n\nfrom nats.aio.client import Client as NATS\n\n\nasync def publish_and_wait() -> dict[str, object]:\n request_id = f"supaserch-smoke-{uuid.uuid4().hex[:8]}"\n nc = NATS()\n url = os.getenv("NATS_URL", "nats://nats:pmoves@nats:4222")\n await nc.connect(url)\n loop = asyncio.get_running_loop()\n future: asyncio.Future | None = loop.create_future()\n\n async def handler(msg):\n nonlocal future\n try:\n data = json.loads(msg.data.decode("utf-8"))\n except json.JSONDecodeError:\n return\n if data.get("request_id") != request_id:\n return\n if future and not future.done():\n future.set_result(data)\n\n sid = await nc.subscribe("supaserch.result.v1", cb=handler)\n payload = {\n "request_id": request_id,\n "query": "supaserch smoke verification",\n "correlation_id": request_id,\n "trigger": "make supaserch-smoke",\n }\n await nc.publish("supaserch.request.v1", json.dumps(payload).encode("utf-8"))\n await nc.flush()\n try:\n result = await asyncio.wait_for(future, timeout=10)\n except asyncio.TimeoutError:\n print("✖ Did not receive supaserch.result.v1 within 10s")\n await nc.unsubscribe(sid)\n await nc.drain()\n sys.exit(1)\n await nc.unsubscribe(sid)\n await nc.drain()\n fallback = result.get("fallback", {}) if isinstance(result, dict) else {}\n if fallback.get("status") != "ok":\n print("✖ NATS fallback status not ok:", json.dumps(fallback))\n sys.exit(1)\n via = fallback.get("via", "unknown")\n latency = fallback.get("latency_ms", 0)\n print(f"✔ NATS round-trip complete (via {via}, latency {latency} ms)")\n return result\n\n\nresult = asyncio.run(publish_and_wait())\nhost_port = os.getenv("SUPASERCH_HOST_PORT", os.getenv("SUPASERCH_PORT", "8099"))\nhttp_url = f"http://localhost:{host_port}/v1/search?q=supaserch+smoke+http"\ntry:\n with urllib.request.urlopen(http_url, timeout=8) as resp:\n body = json.loads(resp.read().decode("utf-8"))\nexcept (HTTPError, URLError, TimeoutError) as exc:\n print(f"✖ HTTP fallback request failed: {exc}")\n sys.exit(1)\n\nfallback = body.get("fallback", {}) if isinstance(body, dict) else {}\nif fallback.get("status") != "ok":\n print("✖ HTTP fallback status not ok:", json.dumps(fallback))\n sys.exit(1)\n\nvia = fallback.get("via", "unknown")\nlatency = fallback.get("latency_ms", 0)\nprint(f"✔ HTTP fallback responded (via {via}, latency {latency} ms)")\nPY' |
Contributor
There was a problem hiding this comment.
Keep host-run smoke targets on a host-reachable NATS URL.
Both recipes execute Python on the host, but the new default uses the compose-only hostname nats. Unless the operator exports an override, these smokes can fail DNS resolution before publishing. Either run them inside the compose network or use a host-reachable authenticated fallback.
🐛 Proposed fix using a host-reachable authenticated smoke URL
- url = os.getenv("NATS_URL", "nats://nats:pmoves@nats:4222")
+ url = (
+ os.getenv("NATS_SMOKE_URL")
+ or os.getenv("HOST_NATS_URL")
+ or "nats://nats:pmoves@127.0.0.1:4222"
+ )- `@ENV_FILE`="$(CURDIR)/env.shared" bash -lc '. ./scripts/with-env.sh "$$ENV_FILE" && PYTHONPATH="$(CURDIR)/.." python3 tools/deepresearch_smoke.py --timeout $${DEEPRESEARCH_SMOKE_TIMEOUT:-60}'
+ `@ENV_FILE`="$(CURDIR)/env.shared" bash -lc '. ./scripts/with-env.sh "$$ENV_FILE" && \
+ NATS_SMOKE_URL="$${NATS_SMOKE_URL:-$${HOST_NATS_URL:-nats://nats:pmoves@127.0.0.1:4222}}" \
+ PYTHONPATH="$(CURDIR)/.." python3 tools/deepresearch_smoke.py \
+ --timeout $${DEEPRESEARCH_SMOKE_TIMEOUT:-60} \
+ --nats "$$NATS_SMOKE_URL"'Also applies to: 2594-2594
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/Makefile` at line 2447, The default NATS URL used by
publish_and_wait() is a compose-only hostname ("nats://nats:pmoves@nats:4222")
which can fail when the Python runs on the host; update the
os.getenv("NATS_URL", "...") call inside publish_and_wait to use a
host-reachable default (e.g. "nats://nats:pmoves@127.0.0.1:4222" or
"nats://nats:pmoves@localhost:4222") so host-run smoke targets can resolve and
authenticate, and mirror the same change for the other occurrence of the
NATS_URL default in this diff range.
| ```bash | ||
| docker build -t pmoves-graph-linker . | ||
| docker run -e NEO4J_URL=bolt://neo4j:7687 -e NATS_URL=nats://nats:4222 pmoves-graph-linker | ||
| docker run -e NEO4J_URL=bolt://neo4j:7687 -e NATS_URL=nats://nats:pmoves@nats:4222 pmoves-graph-linker |
Contributor
There was a problem hiding this comment.
Docker run example is incomplete for hostname-based service discovery.
At Line 73, nats/neo4j hostnames require a shared Docker network; without --network, this command is likely to fail.
Suggested doc fix
-docker run -e NEO4J_URL=bolt://neo4j:7687 -e NATS_URL=nats://nats:pmoves@nats:4222 pmoves-graph-linker
+docker run --network pmoves_bus \
+ -e NEO4J_URL=bolt://neo4j:7687 \
+ -e NATS_URL=nats://nats:pmoves@nats:4222 \
+ pmoves-graph-linker📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| docker run -e NEO4J_URL=bolt://neo4j:7687 -e NATS_URL=nats://nats:pmoves@nats:4222 pmoves-graph-linker | |
| docker run --network pmoves_bus \ | |
| -e NEO4J_URL=bolt://neo4j:7687 \ | |
| -e NATS_URL=nats://nats:pmoves@nats:4222 \ | |
| pmoves-graph-linker |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/graph-linker/README.md` at line 73, The Docker run example
uses hostname-based service discovery (neo4j, nats) but omits joining a shared
Docker network; update the README example (the docker run command shown) to
include a --network option and mention creating or using an existing network
(e.g., docker network create <name>) so the neo4j and nats hostnames resolve
correctly; keep the example consistent by naming the network (e.g.,
pmoves-network) and add a brief note that containers must be started on that
same network.
4 tasks
POWERFULMOVES
added a commit
that referenced
this pull request
Apr 23, 2026
All 110 NATS URLs in non-test code now use authenticated form: nats://nats:pmoves@nats:4222 - PR #1375 completed docs migration (13 files, +16/-16) - Code files migrated in prior convergence waves - Verified 2026-04-23: 0 unauthenticated URLs in production code - Closed obsolete PR #1376 (wrong hostname, already superseded) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
9 tasks
This was referenced May 2, 2026
POWERFULMOVES
added a commit
that referenced
this pull request
Jun 2, 2026
…2 non-urgent) (#1690) Lane 1 (Archon fork-sync): DONE — fork synced to upstream main 0.4.1, bun 1.3.14-slim base, Archon #15 merged, gitlinks reconciled #1674, vendored pin retired, branch protection right-sized for solo operator. Lane 2 (NATS-auth): scope-and-report overturned the "~17 urgent files" framing. env.shared.example already emits the authed NATS_URL and all production wiring reads it via os.getenv — zero consumers of the bare nats://nats:4222 constant. DoX already normalizes both forms (#1375/#1292); pmoves_health's literal is __main__ example code; the ~30 triple copies have drifted (md5 differs) so there is no clean single-source fix. Decision: no 30-file hardened-branch sweep for a non-issue; any fallback hardening is a separate low-priority refactor at the canonical pmoves-cipher-mcp package. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nats://localhost:4222withnats://nats:pmoves@nats:4222across documentation, Makefile defaults, and context filesFiles Changed
.claude/context/nats-subjects.md- Context referenceMakefile- Inline script defaults (supaserch-smoke, deepresearch-smoke)Context
Code files were already fixed in prior convergence wave. This completes the NATS authentication migration — all references now use the authenticated URL format.
Test Plan
localhost:4222remains inpmoves/docs/configs🤖 Generated with Claude Code
Summary by CodeRabbit