fix(ops): cipher-api Known Roads + compose fixes - #1154
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 49 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request consolidates the GHCR registry from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.claude/hooks/damage-control/patterns.yaml (1)
883-886: Consider using more specific path prefixes to limit scope.The
chitSafePathscheck uses substring matching (seewrite-tool-damage-control.pyline 125:if safe_normalized in normalized_fwd). The bare string"pr-kits"would match any path containing that substring anywhere, including potentially sensitive paths likesome/path/pr-kits/secrets.env.For tighter scoping while still enabling the intended PR-kit compose edits:
♻️ More specific path patterns
# Integration compose overrides — registry reference fixes (GHCR org consolidation) - - "docker-compose.integrations" + - "docker-compose.integrations.images.yml" # PR-kit compose files — integration starter templates - - "pr-kits" + - "integrations/pr-kits"This maintains the ability to edit
pmoves/integrations/pr-kits/*/docker-compose.pmoves-net.ymlfiles while preventing accidental matches on unrelated paths containing "pr-kits".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/damage-control/patterns.yaml around lines 883 - 886, The current patterns.yaml entry uses a bare substring "pr-kits" which the chitSafePaths check (see write-tool-damage-control.py and the conditional "if safe_normalized in normalized_fwd") will match anywhere and can accidentally allow sensitive files; change the pattern to a more specific path prefix that only matches the intended compose files (for example use the repo-relative prefix used by your integrations like "pmoves/integrations/pr-kits/" or "pmoves/integrations/pr-kits/*/docker-compose.pmoves-net.yml") so that write-tool-damage-control.py's substring check only permits the intended PR-kit compose edits.pmoves/docker-compose.yml (1)
2169-2169: Consider using environment variable override pattern for consistency.The
OLLAMA_BASE_URLis hardcoded while other environment variables in this service use the${VAR:-default}pattern (e.g.,NEO4J_USER=${NEO4J_USER:-neo4j}). For consistency and flexibility in different deployment scenarios:♻️ Suggested change for consistency
- - OLLAMA_BASE_URL=http://pmoves-ollama:11434 + - OLLAMA_BASE_URL=${OLLAMA_BASE_URL:-http://pmoves-ollama:11434}Based on learnings: "teams pre-wire environment variables using the ${VAR:-} empty-default pattern for forward compatibility."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/docker-compose.yml` at line 2169, The OLLAMA_BASE_URL env var is hardcoded; update its declaration to follow the existing override pattern by using the shell parameter expansion syntax so it can be pre-wired or overridden (e.g., change the value to use ${OLLAMA_BASE_URL:-http://pmoves-ollama:11434}) — locate the OLLAMA_BASE_URL entry in the service env block and replace the literal URL with the ${VAR:-default} form to match other variables like NEO4J_USER.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.claude/hooks/damage-control/patterns.yaml:
- Around line 883-886: The current patterns.yaml entry uses a bare substring
"pr-kits" which the chitSafePaths check (see write-tool-damage-control.py and
the conditional "if safe_normalized in normalized_fwd") will match anywhere and
can accidentally allow sensitive files; change the pattern to a more specific
path prefix that only matches the intended compose files (for example use the
repo-relative prefix used by your integrations like
"pmoves/integrations/pr-kits/" or
"pmoves/integrations/pr-kits/*/docker-compose.pmoves-net.yml") so that
write-tool-damage-control.py's substring check only permits the intended PR-kit
compose edits.
In `@pmoves/docker-compose.yml`:
- Line 2169: The OLLAMA_BASE_URL env var is hardcoded; update its declaration to
follow the existing override pattern by using the shell parameter expansion
syntax so it can be pre-wired or overridden (e.g., change the value to use
${OLLAMA_BASE_URL:-http://pmoves-ollama:11434}) — locate the OLLAMA_BASE_URL
entry in the service env block and replace the literal URL with the
${VAR:-default} form to match other variables like NEO4J_USER.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16d03994-f981-4a4f-930b-2af1743afe30
📒 Files selected for processing (10)
.claude/hooks/damage-control/patterns.yamlpmoves/Makefilepmoves/docker-compose.integrations.images.ymlpmoves/docker-compose.ymlpmoves/docs/AGENTS/AGNOTE4482PHI.t1.mdpmoves/docs/operations/CONTAINER_REBUILD_MANIFEST.mdpmoves/integrations/pr-kits/firefly/docker-compose.pmoves-net.ymlpmoves/integrations/pr-kits/jellyfin/docker-compose.pmoves-net.ymlpmoves/integrations/pr-kits/open-notebook/docker-compose.pmoves-net.ymlpmoves/integrations/pr-kits/wger/docker-compose.pmoves-net.yml
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| - NEO4J_USER=${NEO4J_USER:-neo4j} | ||
| - NEO4J_PASSWORD=${NEO4J_PASSWORD:-neo4j} | ||
| - NATS_URL=${NATS_URL:-nats://nats:pmoves@nats:4222} | ||
| - OLLAMA_BASE_URL=http://pmoves-ollama:11434 |
There was a problem hiding this comment.
🔥 The Roast: Every other service in this file uses ${OLLAMA_URL:-http://pmoves-ollama:11434} (line 1746, 1908) or ${OLLAMA_BASE_URL:-http://pmoves-ollama:11434} (line 2676) — but cipher-api gets a hardcoded value with no env var override. It's like this one service got invited to the party but wasn't told the dress code. The PR description literally says TensorZero routing is "deferred" — how exactly do you plan to route it if the URL is baked into YAML?
🩹 The Fix:
| - OLLAMA_BASE_URL=http://pmoves-ollama:11434 | |
| - OLLAMA_BASE_URL=${OLLAMA_BASE_URL:-http://pmoves-ollama:11434} |
📏 Severity: suggestion
| .PHONY: up-cipher | ||
| up-cipher: ## Start Cipher Memory (knowledge-graph memory for Claude Code / agents) | ||
| @$(DC) --profile agents up -d --build cipher-api | ||
| @echo "✔ Cipher Memory ready at http://localhost:$${CIPHER_PORT:-8096}" |
There was a problem hiding this comment.
🔥 The Roast: docker compose up -d returns as soon as the container is created, not when it's healthy. Printing "✔ Cipher Memory ready" right after is like shouting "dinner's served!" the moment you put the pizza in the oven. You literally created a cipher-health target 4 lines below this — use it! Also, port 8096 is Jellyfin's default host port too, so when both are running, someone's getting a surprise.
🩹 The Fix: Either call cipher-health after the up -d (with a retry loop), or soften the message to "starting..." and point users to make cipher-health. Also consider defaulting CIPHER_PORT to something other than 8096 to avoid the Jellyfin collision.
📏 Severity: warning
Code Review Roast 🔥Verdict: 2 Issues Found (carried forward) | Recommendation: Track as follow-up (PR already merged) Overview
Issue Details (click to expand)
🏆 Best part: The incremental commit is genuinely impressive in scope. The 💀 Worst part: The 2 carried-forward issues from the first review commit are still sitting there untouched. The hardcoded 📊 Overall: This PR evolved from a focused ops fix (GHCR registry + healthcheck IPv6) into a substantial feature drop — fleet remote access runbooks, publish pipeline state machine with tests, room schema extensions, submodule bumps, and a mountain of documentation. The new code is clean. The old warnings are still warnings. Like a house that got a beautiful new addition but still hasn't fixed the leaky faucet in the original bathroom. Carried-Forward Issues (unchanged from previous review)
Files Reviewed (27 files in incremental + 10 from prior review)New files reviewed (incremental):
Carried forward from prior review:
Fix these issues in Kilo Cloud Reviewed by glm-5.1 (incremental from 2e390de) · incremental tokens Reviewed by glm-5.1 · 416,244 tokens |
Alpine BusyBox wget resolves `localhost` to IPv6 [::1] first, but Node.js 0.0.0.0 binding only listens on IPv4. Changed healthcheck URL from localhost to 127.0.0.1. Added OLLAMA_BASE_URL=http://pmoves-ollama:11434 to cipher-api env so the container can reach the in-network Ollama instance. This is the interim direct connection; TensorZero-routed config with cloud fallback is a separate PR (needs TAC tree + cipher.yml rework). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cipher-api was in the agents compose profile but missing from all make targets — any attempt to start it required raw docker compose (which bypasses the env-file chain and fails on CHIT_PROD). - Added cipher-api to up-agents-stack service list - New up-cipher target (--build for fresh image) - New cipher-health target (curl health probe) Closes Known Roads gap: cipher-api now has a canonical make path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
z890-claude session: PR #1148 review/merge, 5/6 manifest images rebuilt (tensorzero, botz, flute-gw, publisher-discord, cipher-api), 3 Dockerfile blockers resolved, n8n runners activated (Python+JS), 28 healthy containers (up from 20/23). CLAIM/RELEASE/ACK signed for PHI-4482-T1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2e390de to
f3d3970
Compare
Docker Hardening ValidationHardening Validation ReportValidated: Sat Mar 28 21:09:45 UTC 2026Services CheckedPMOVES.AI Docker Hardening Validation[INFO] Checking: pmoves/docker-compose.hardened.yml [INFO] Validating: hi-rag-gateway-v2 [INFO] Validating: extract-worker [INFO] Validating: langextract [INFO] Validating: presign [INFO] Validating: render-webhook [INFO] Validating: retrieval-eval [INFO] Validating: pdf-ingest [INFO] Validating: jellyfin-bridge [INFO] Validating: invidious-companion-proxy [INFO] Validating: ffmpeg-whisper [INFO] Validating: media-video [INFO] Validating: media-audio [INFO] Validating: hi-rag-gateway-v2-gpu [INFO] Validating: hi-rag-gateway-gpu [INFO] Validating: deepresearch [INFO] Validating: supaserch [INFO] Validating: publisher-discord [INFO] Validating: mesh-agent [INFO] Validating: nats-echo-req [INFO] Validating: nats-echo-res [INFO] Validating: publisher [INFO] Validating: analysis-echo [INFO] Validating: graph-linker [INFO] Validating: comfy-watcher [INFO] Validating: grayjay-plugin-host [INFO] Validating: agent-zero [INFO] Validating: archon [INFO] Validating: channel-monitor [INFO] Validating: pmoves-yt [INFO] Validating: notebook-sync [INFO] Validating: supabase_service_role_key [INFO] Validating: supabase_jwt_secret ====================================== |
1 participant
Summary
localhostto127.0.0.1— Alpine BusyBox wget resolves to IPv6[::1]first, but Node.js0.0.0.0only binds IPv4OLLAMA_BASE_URL=http://pmoves-ollama:11434so cipher-api can reach in-network Ollama (interim — TensorZero-routed config with cloud fallback is a separate PR)cipher-apitoup-agents-stack, newup-cipher+cipher-healthmake targets — closes Known Roads gap where cipher-api had no canonical make pathContext
Cipher-api was in the
agentscompose profile but missing from allup-*make targets. Starting it required rawdocker composewhich bypasses the--env-filechain and fails onCHIT_PROD_PASSPHRASE. This PR gives it a proper Known Roads path.Deferred (separate PR)
Pmoves-cipher/memAgent/cipher.ymlLLM provider config — needs TensorZero routing (not direct Ollama), TAC tree documentation, cloud fallback chainPmoves-cipher/src/core/mcp/client.tsMCP SDK TS2353 fix — submodule-level PRTest plan
make -C pmoves up-cipherbuilds and starts cipher-apimake -C pmoves cipher-healthreturns healthymake -C pmoves up-agents-stackincludes cipher-api in startupwget http://127.0.0.1:3000/health)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores