feat(neo4j): Neo4j Submodule Integration Enhancement - #898
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughNeo4j management undergoes consolidation by delegating inline Makefile targets to a submodule, updating backup/restore paths from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/CLAUDE.md:
- Around line 216-239: Fix the Neo4j service entry in .claude/CLAUDE.md: correct
the typo in the header "n**Neo4j**" to "**Neo4j**", remove the duplicated Neo4j
block so there is a single canonical entry, and update the exposed ports list by
removing port "2004" (keep only 7474 HTTP and 7687 Bolt) to match the actual
docker-compose exposure; ensure the entry still includes the profile command,
API and health endpoints, submodule note, and references to
pmoves_core/PMOVES-Neo4j symbols so the operator map remains accurate.
In `@pmoves/docs/NEO4J_INTEGRATION_GUIDE.md`:
- Around line 70-73: The Grafana URL under the "Grafana Dashboard" section is
incorrect; update the documented URL from http://localhost:3000 to the actual
default http://localhost:3002 so readers are pointed to the correct port (edit
the "Grafana Dashboard" block, replacing the URL line).
- Around line 54-60: The restore example uses make -C pmoves which runs with
pmoves/ as the working directory, but the BACKUP variable is set to
pmoves/backups/... which is one directory too deep; update the neo4j restore
example (the make invocation referencing the neo4j-restore target and BACKUP
variable) to use BACKUP=backups/neo4j_YYYYMMDD_HHMMSS.dump or an absolute path
so the path resolves correctly when running make -C pmoves.
In `@pmoves/Makefile`:
- Around line 2821-2853: The neo4j-backup Makefile target uses
backup_dir="pmoves/backups" which, when invoked via make -C pmoves, expands to
pmoves/pmoves/backups (wrong host path), and the fallback export writes
file:///backups/neo4j_${timestamp}.cypher but the docker cp and validation still
expect /backups/neo4j.dump and neo4j_${timestamp}.dump causing validation to
always fail; fix by making backup_dir relative to the current working directory
(e.g., backup_dir="backups" or use $(CURDIR)/backups) and update the docker cp /
validation logic in the neo4j-backup target to handle both possible container
filenames (/backups/neo4j.dump and /backups/neo4j_${timestamp}.cypher) and copy
the actual fallback filename into $$backup_dir/neo4j_${timestamp}.cypher (and
then check that exact path) so both primary dump and fallback cypher export are
correctly copied and validated.
- Around line 2873-2875: The Makefile restore flow is removing the wrong Docker
volume name; update the docker volume removal command invoked after
neo4j-local-down to remove the named volume with the hyphen (project-prefixed)
used by the compose service (i.e., change the volume name from the incorrect
"pmoves_neo4jdata" to "pmoves_neo4j-data"), keeping the surrounding silence/||
true behavior and leaving the neo4j-local-down invocation intact so the restore
runs against a truly fresh Neo4j volume.
- Around line 2810-2813: The neo4j-local-down Makefile target currently uses
$(DC) --profile neo4j-local down -v --remove-orphans which tears down the whole
project and removes named volumes; change it to perform a service-scoped
shutdown instead: call the Docker Compose command referenced by $(DC) to stop
the specific Neo4j service (and optionally remove that single service container
and its associated anonymous volumes) rather than using down -v --remove-orphans
so unrelated PMOVES services and global volumes are not affected; update the
neo4j-local-down target to stop/remove only the "neo4j" service via the Compose
CLI for the neo4j-local profile.
- Around line 2745-2794: The appended neo4j-* make targets (e.g., neo4j-up,
neo4j-down, neo4j-restart, neo4j-logs, neo4j-migrate, neo4j-seed,
neo4j-bootstrap, neo4j-status) duplicate earlier definitions and use the wrong
submodule path; remove the duplicate block (or replace the earlier one) and
update every invocation that uses -C PMOVES-Neo4j and any ls references like
PMOVES-Neo4j/db/... to use the correct relative path ../PMOVES-Neo4j so make -C
resolves to the sibling submodule, and ensure only one set of neo4j-* targets
remains.
In `@pmoves/monitoring/grafana/dashboards/neo4j-overview.json`:
- Around line 20-301: The dashboard panels (uid "neo4j-overview", titles like
"Neo4j Database Size", "Heap Memory Usage", "Transaction Rate", "Page Cache
Performance") all query metrics with the neo4j_* prefix but Prometheus is not
configured to scrape Neo4j; add a scrape job to your Prometheus scrape_configs
to target the Neo4j exporter endpoint (port 2004) or the Neo4j metrics endpoint,
with appropriate job_name and relabeling so metrics with neo4j_* are ingested;
update the Prometheus config used by your deployment and reload Prometheus so
the dashboard no longer shows “No data.”
In `@pmoves/scripts/backup-neo4j.sh`:
- Around line 58-71: Load the PMOVES shared env before using NEO4J_PASSWORD
(source pmoves/env.shared or equivalent) so NEO4J_PASSWORD is not defaulted to
"changeme"; ensure the backup target inside the container uses a path the
compose mounts (e.g., /data or /data/backups) instead of /backups, create the
directory if needed inside the container before running neo4j-admin, and update
the docker cp source to match that path (use "$CONTAINER_NAME:/data/neo4j.dump"
or "$CONTAINER_NAME:/data/backups/neo4j.dump") while keeping variables
BACKUP_FILE, BACKUP_DIR, and CONTAINER_NAME consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aba17c7f-4775-4707-8e28-63dc7b988281
📒 Files selected for processing (7)
.claude/CLAUDE.md.gitmodulesPMOVES-Neo4jpmoves/Makefilepmoves/docs/NEO4J_INTEGRATION_GUIDE.mdpmoves/monitoring/grafana/dashboards/neo4j-overview.jsonpmoves/scripts/backup-neo4j.sh
This commit addresses all 10 CodeRabbit review comments on the Neo4j submodule integration enhancement PR. Critical Fixes (4): 1. Remove duplicate neo4j-* Makefile targets (lines 643-695) - Old targets referenced non-existent pmoves/integrations/neo4j - Kept canonical targets with corrected submodule paths 2. Fix destructive neo4j-local-down target - Changed from 'down -v --remove-orphans' (tears down everything) - To 'stop neo4j' + 'rm -f neo4j' (only affects Neo4j container) 3. Fix backup directory path resolution - Changed backup_dir from 'pmoves/backups' to 'backups' - Fixed with 'make -C pmoves' path resolution - Improved fallback validation for both .dump and .cypher exports 4. Fix backup-neo4j.sh script - Added env.shared loading for NEO4J_PASSWORD - Changed container path from /backups to /data/backups (mounted volume) - Added directory creation inside container before dump Major Fixes (4): 5. Remove duplicate Neo4j entry in CLAUDE.md - Fixed typo: 'n**Neo4j**' → '**Neo4j**' - Removed duplicate documentation block - Removed port 2004 from exposed ports (internal metrics only) 6. Fix restore volume name - Changed from 'pmoves_neo4jdata' to 'pmoves_neo4j-data' - Matches actual volume name in docker-compose.yml 7. Add Prometheus scrape config for Neo4j - Added neo4j job to prometheus.yml - Scrapes metrics endpoint on port 2004 - 15s scrape interval 8. Fix backup directory resolution (Makefile) - Already addressed in critical fix #3 Minor Fixes (2): 9. Fix Grafana URL in documentation - Changed from http://localhost:3000 to http://localhost:3002 - Matches actual Grafana port configuration 10. Fix restore example path in integration guide - Changed from pmoves/backups/ to backups/ - Correct path resolution with 'make -C pmoves' Additional Improvements: - Removed duplicate neo4j-status target (line 1827) - All Neo4j submodule targets now use ../PMOVES-Neo4j path - Improved error messages in backup/restore logic - Better fallback handling for cypher export Testing: - All Makefile targets verified non-duplicate - Backup paths resolve correctly from pmoves/ directory - Prometheus scrape config validates YAML syntax - Documentation links and URLs corrected Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Docker Hardening ValidationHardening Validation ReportValidated: Fri Mar 13 06:14:17 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 ====================================== |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| content={ | ||
| "ok": False, | ||
| "error": str(e) | ||
| } |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
In general, to fix this kind of issue you should avoid returning raw exception text or stack traces to the client. Instead, log the detailed error (including the exception and, if needed, its traceback) on the server, then return a generic, non-sensitive error message in the HTTP response. Optionally, you can include a stable, non-sensitive error code or correlation ID to help track the error in logs without exposing internals.
For this file, the best fix with minimal functional change is:
- Keep the existing
logger.error(...)calls so detailed information is still logged server-side. - Change the JSON error responses in both
triage_endpointandaccuracy_endpointto:- Stop including
str(e)in theerrorfield. - Replace it with a generic message like
"Internal server error"and, if desired, a simple context string (e.g.,"Triage failed"/"Accuracy calculation failed").
- Stop including
Concretely:
- In
triage_endpoint(around lines 387–395), modify theJSONResponsecontent so"error": str(e)becomes a generic message. - In
accuracy_endpoint(around lines 419–426), make the same change. - No new imports or helper methods are required; we reuse the existing
logger.
| @@ -390,7 +390,7 @@ | ||
| status_code=500, | ||
| content={ | ||
| "ok": False, | ||
| "error": str(e) | ||
| "error": "Internal server error while triaging issue." | ||
| } | ||
| ) | ||
|
|
||
| @@ -422,7 +422,7 @@ | ||
| status_code=500, | ||
| content={ | ||
| "ok": False, | ||
| "error": str(e) | ||
| "error": "Internal server error while calculating accuracy." | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (12)
.claude/context/nats-subjects.md-786-800 (1)
786-800:⚠️ Potential issue | 🟠 MajorRemove duplicate
github.issue.labeled.v1entry with conflicting schema.The subject
github.issue.labeled.v1is defined twice (lines 786-792 and 794-800) with incompatible payload schemas:
- First entry:
{"labels": [...], "confidence": 0.85, "method": "semantic", ...}- Second entry:
{"label": "bug", ...}(singular, no confidence/method)The actual implementation (
pmoves/services/github-issue-triage/app.py:273-286) matches only the first schema. The duplicate creates ambiguity for consumers.🗑️ Proposed fix
**`github.issue.labeled.v1`** - **Direction:** Published by github-issue-triage → Consumed by monitoring - **Purpose:** Label application events after successful triage - **Payload:** ```json {"repo": "PMOVES.AI", "issue_number": 123, "labels": ["bug"], "confidence": 0.85, "method": "semantic", "timestamp": "2026-03-13T00:00:00Z"}
-
github.issue.labeled.v1
-- Direction: Published by github-issue-triage → Consumed by monitoring
-- Purpose: Label application events
-- Payload:
- {"repo": "PMOVES.AI", "issue_number": 123, "label": "bug", "timestamp": "2026-03-13T00:00:00Z"}
</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.claude/context/nats-subjects.md around lines 786 - 800, Remove the
duplicated subject definition for github.issue.labeled.v1 in the nats subjects
doc and keep the schema that matches the implementation in
pmoves/services/github-issue-triage/app.py (the version containing "labels"
array plus "confidence" and "method"); specifically delete the second
conflicting entry (the one with a singular "label" and missing
confidence/method) so the doc only exposes the canonical payload: {"repo",
"issue_number", "labels", "confidence", "method", "timestamp"} and avoid
ambiguity for consumers.</details> </blockquote></details> <details> <summary>.claude/context/nats-subjects.md-778-784 (1)</summary><blockquote> `778-784`: _⚠️ Potential issue_ | _🟠 Major_ **Remove undocumented subject `github.issue.triage.v1`.** This subject is never published by the actual implementation. The code in `pmoves/services/github-issue-triage/app.py:273-286` only publishes `github.issue.labeled.v1` events after successful triage—there is no separate `github.issue.triage.v1` subject. <details> <summary>🗑️ Proposed fix</summary> ```diff -**`github.issue.triage.v1`** -- **Direction:** Published by github-issue-triage → Consumed by monitoring -- **Purpose:** Issue triage results -- **Payload:** - ```json - {"repo": "PMOVES.AI", "issue_number": 123, "labels": ["bug", "high-priority"], "confidence": 0.85, "timestamp": "2026-03-13T00:00:00Z"} - ``` -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/context/nats-subjects.md around lines 778 - 784, Remove the undocumented NATS subject entry "github.issue.triage.v1" from .claude/context/nats-subjects.md because the implemented publisher in pmoves/services/github-issue-triage/app.py (see the triage publish logic around the block that emits "github.issue.labeled.v1" at lines ~273-286) only emits "github.issue.labeled.v1"; update the documentation to reflect the actual published subject "github.issue.labeled.v1" and delete the extraneous example payload for "github.issue.triage.v1".pmoves/services/github-issue-triage/hirag_client.py-58-85 (1)
58-85:⚠️ Potential issue | 🟠 MajorFix query payload and response parsing to match Hi-RAG v2 API.
The payload uses incorrect field names and response parsing looks for wrong keys:
- Payload should use
kinstead oftop_k,use_rerankinstead ofrerank- Response contains
hitsnotresults🔧 Proposed fix
async def query( self, query_text: str, top_k: int = 10, rerank: bool = True, - filters: Optional[Dict[str, Any]] = None + filters: Optional[Dict[str, Any]] = None, + namespace: str = "default" ) -> Optional[List[Dict[str, Any]]]: ... try: payload = { "query": query_text, - "top_k": top_k, - "rerank": rerank + "namespace": namespace, + "k": top_k, + "use_rerank": rerank } - if filters: - payload["filters"] = filters + # Note: Hi-RAG v2 uses entity_types for filtering, not generic filters + if filters and "entity_types" in filters: + payload["entity_types"] = filters["entity_types"] async with httpx.AsyncClient(timeout=self.timeout) as client: response = await client.post( self.query_url, json=payload, headers={"Content-Type": "application/json"} ) response.raise_for_status() data = response.json() # Extract results - Hi-RAG v2 returns "hits" - if "results" in data: - return data["results"] + if "hits" in data: + return data["hits"] + elif "results" in data: + # Fallback for v1 compatibility + return data["results"] elif isinstance(data, list): return data else: - logger.warning(f"Unexpected Hi-RAG response format: {data.keys()}") + logger.warning(f"Unexpected Hi-RAG response format: {list(data.keys())}") return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/github-issue-triage/hirag_client.py` around lines 58 - 85, The query payload and response parsing must be updated to Hi-RAG v2: in the method that builds the payload (the block using variables top_k, rerank, filters and self.query_url) rename payload keys to use "k" instead of "top_k" and "use_rerank" instead of "rerank" (keep adding filters as before), and after posting parse response.json() for "hits" (return data["hits"] when present) and fall back to returning the list if data is a list; also update the logger.warning to reference the actual keys or structure when the format is unexpected (use the existing response variable and data variable names to locate and change logic).pmoves/monitoring/prometheus/prometheus.yml-46-50 (1)
46-50:⚠️ Potential issue | 🟠 MajorDon't add the Neo4j scrape job until the exporter is actually enabled.
The compose snippet in this PR only exposes 7474/7687 and does not enable Neo4j's Prometheus exporter, so
neo4j:2004/metricswill stay down. Wire up the exporter inpmoves/docker-compose.ymlfirst, then land this scrape job.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/monitoring/prometheus/prometheus.yml` around lines 46 - 50, The prometheus scrape job "job_name: neo4j" should not be added until the Neo4j Prometheus exporter is actually enabled; remove or guard the scrape block (the block containing job_name: neo4j, targets: ["neo4j:2004"], metrics_path and scrape_interval) from pmoves/monitoring/prometheus/prometheus.yml and instead first add the exporter configuration to pmoves/docker-compose.yml (expose the exporter port and link it to the neo4j service). After wiring the exporter into docker-compose.yml and verifying it listens on the expected port, reintroduce the "job_name: neo4j" scrape block pointing to the exporter endpoint.pmoves/services/github-issue-triage/app.py-43-48 (1)
43-48:⚠️ Potential issue | 🟠 MajorRemove the credentialed NATS fallback.
NATS_URLfalls back tonats://nats:pmoves@nats:4222, which bakes a plaintext credential pattern into the service and bypasses the shared secret-loading flow. This should come from the central env helper and fail closed when the secret is missing.As per coding guidelines,
pmoves/services/**: "Prefer central env helpers and *_FILE secret loading paths. Flag direct critical-secret reads and plaintext fallbacks."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/github-issue-triage/app.py` around lines 43 - 48, The NATS_URL variable currently falls back to a hardcoded credentialed URL; remove that plaintext fallback and instead load NATS_URL via the central env-secret helper (use the project's shared env helper/secret loader and its *_FILE support) so the service fails closed when the secret is missing; update the NATS_URL assignment (the NATS_URL symbol in this file) to call the central helper and do not provide a credentialed default value.pmoves/services/github-branch-cleanup/app.py-221-247 (1)
221-247:⚠️ Potential issue | 🟠 MajorPass the token from
get_stale_branches()and URL-encode branch names to handle refs likefeature/foo.
get_stale_branches()callsget_branch_commit_date()once per branch in a loop, and each call mints a fresh token viaget_github_token(). This is wasteful and unnecessary. Additionally, branch names containing slashes are not URL-encoded in the/branches/{branch_name}endpoint, causing 404s for common patterns likefeature/foowhich then silently fail with aNonereturn. Refactor to mint the token once inget_stale_branches()and pass it toget_branch_commit_date(), and useurllib.parse.quote()to safely encode the branch name in the URL path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/github-branch-cleanup/app.py` around lines 221 - 247, get_branch_commit_date currently calls get_github_token() for each branch and embeds branch_name raw into the URL; change its signature to accept a token parameter (e.g., token: str) and URL-encode branch_name with urllib.parse.quote() when constructing the GET URL, and update get_stale_branches to call get_github_token() once, pass that token into each get_branch_commit_date(repo, branch_name, token) call so the token is reused and branch refs like "feature/foo" won't 404; ensure headers still use the passed token and preserve response.raise_for_status()/date parsing behavior in get_branch_commit_date.pmoves/services/github-branch-cleanup/app.py-161-170 (1)
161-170:⚠️ Potential issue | 🟠 MajorFix
/metricsendpoint to return proper Prometheus format.The endpoint returns
generate_latest()without specifying the media type, which breaks Prometheus scraping. ImportResponseandCONTENT_TYPE_LATESTfromfastapi.responsesandprometheus_client:from fastapi.responses import Response from prometheus_client import Counter, Histogram, Gauge, generate_latest, CONTENT_TYPE_LATEST `@app.get`("/metrics") async def metrics(): """Prometheus metrics endpoint.""" return Response(generate_latest(), media_type=CONTENT_TYPE_LATEST)The
/healthzendpoint is correct as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/github-branch-cleanup/app.py` around lines 161 - 170, The /metrics endpoint currently returns generate_latest() without a Prometheus content-type; update the metrics() handler to return a fastapi Response with media_type set to CONTENT_TYPE_LATEST and import the needed symbols: add imports for Response from fastapi.responses and CONTENT_TYPE_LATEST from prometheus_client, and change the metrics() function to return Response(generate_latest(), media_type=CONTENT_TYPE_LATEST) (leave health_check() unchanged).pmoves/services/github-issue-triage/app.py-346-361 (1)
346-361:⚠️ Potential issue | 🟠 MajorFix
/metricsendpoint to return proper Prometheus media type.The
/metricsendpoint returnsgenerate_latest()without wrapping it in aResponseobject with the correct media type. This breaks Prometheus scraping. Follow the pattern used in other services: importCONTENT_TYPE_LATESTfromprometheus_clientand returnResponse(content=generate_latest(), media_type=CONTENT_TYPE_LATEST).Similarly, consider wrapping the
/healthzendpoint in aResponsefor consistency with the PMOVES health/metrics convention, though the JSON response is acceptable as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/github-issue-triage/app.py` around lines 346 - 361, The /metrics endpoint currently returns generate_latest() directly which lacks the Prometheus media type; update the metrics() function to import CONTENT_TYPE_LATEST from prometheus_client and return a FastAPI Response using Response(content=generate_latest(), media_type=CONTENT_TYPE_LATEST); while here the health_check() JSON is acceptable, consider returning it via Response for consistency if desired—make changes in the metrics() function (and imports) referencing generate_latest and metrics to ensure Prometheus scraping works.pmoves/docker-compose.yml-2177-2177 (1)
2177-2177:⚠️ Potential issue | 🟠 MajorDon't bake the default NATS credential into these new services.
Both blocks fall back to
nats://nats:pmoves@nats:4222, which reintroduces a known broker password wheneverNATS_URLis unset. Pull the URL from the tier env files and fail closed instead.As per coding guidelines "Flag hardcoded credentials/default secrets in environment blocks."
Also applies to: 2214-2214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/docker-compose.yml` at line 2177, The environment block currently embeds a hardcoded NATS credential via the default in the NATS_URL fallback (NATS_URL=${NATS_URL:-nats://nats:pmoves@nats:4222}); remove the default so the service uses the external variable only (e.g. NATS_URL=${NATS_URL}) and ensure the compose service references the appropriate tier env_file that contains the approved broker URL, and enforce fail-closed behavior by causing docker-compose to error when NATS_URL is unset (no default). Apply the same change to the other occurrence that uses the same fallback.pmoves/docker-compose.yml-2186-2188 (1)
2186-2188:⚠️ Potential issue | 🟠 MajorHost port
9096already collides elsewhere in this compose file.
github-branch-cleanuppublishes9096:9096, butgrayjay-plugin-hostlater maps${GRAYJAY_PLUGIN_HOST_PORT:-9096}:8080. Enabling both profiles will fail with a port-allocation error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/docker-compose.yml` around lines 2186 - 2188, The compose file maps host port 9096 twice (service github-branch-cleanup uses "9096:9096" while grayjay-plugin-host maps "${GRAYJAY_PLUGIN_HOST_PORT:-9096}:8080"), causing port collisions when both profiles are enabled; update one of the services to avoid the conflict—either change the github-branch-cleanup service's host mapping to use a configurable env var (e.g. ${GITHUB_BRANCH_CLEANUP_PORT:-9096}:9096) so it can be overridden, or pick a different default host port (e.g. 9097) for github-branch-cleanup or grayjay-plugin-host; ensure the referenced symbols github-branch-cleanup and grayjay-plugin-host are updated consistently wherever their ports are documented or consumed.pmoves/docker-compose.yml-2216-2220 (1)
2216-2220:⚠️ Potential issue | 🟠 Major
BOTZ_MCP_URLpoints at a port the target service never opens.
botz-gatewayin this compose file exposes and health-checks port8054, but this default sendsgithub-issue-triagetobotz-gateway:8102. Those calls will connection-refuse unless the gateway is actually moved to8102.🔧 Suggested fix
- - BOTZ_MCP_URL=${BOTZ_MCP_URL:-http://botz-gateway:8102} + - BOTZ_MCP_URL=${BOTZ_MCP_URL:-http://botz-gateway:8054}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/docker-compose.yml` around lines 2216 - 2220, Environment variable BOTZ_MCP_URL is set to an incorrect port (8102) while the botz-gateway service exposes/health-checks port 8054; update the default BOTZ_MCP_URL value to use port 8054 (BOTZ_MCP_URL=${BOTZ_MCP_URL:-http://botz-gateway:8054}) so github-issue-triage and any callers point at the actual exposed port of botz-gateway; locate the BOTZ_MCP_URL entry in the docker-compose service env block and change the port accordingly.pmoves/Makefile-2691-2735 (1)
2691-2735:⚠️ Potential issue | 🟠 MajorGuard the submodule before delegating to it.
These targets assume
../PMOVES-Neo4jis already populated. When that directory is still an uninitialized submodule checkout,make -C ../PMOVES-Neo4j ...just fails with an opaque path error. Add a preflight check here, or extendsubmodule-integrity*to requirePMOVES-Neo4jbefore delegation.🔧 Suggested guard
neo4j-up: ## Start Neo4j submodule stack `@echo` "→ Starting Neo4j from submodule..." + `@test` -f ../PMOVES-Neo4j/Makefile || { echo "Initialize PMOVES-Neo4j first: git submodule update --init --recursive PMOVES-Neo4j"; exit 1; } @$(MAKE) --no-print-directory -C ../PMOVES-Neo4j up🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/Makefile` around lines 2691 - 2735, All neo4j-* targets (neo4j-up, neo4j-down, neo4j-restart, neo4j-logs, neo4j-migrate, neo4j-seed, neo4j-bootstrap, neo4j-status) must guard that the ../PMOVES-Neo4j submodule is initialized/populated before delegating; add a small preflight check at the top of each target that verifies the directory exists and contains expected files (e.g., test for ../PMOVES-Neo4j/.git or a known path like ../PMOVES-Neo4j/Makefile or db/migrations) and print a clear error and exit if missing, or alternatively invoke the existing submodule-integrity* target (if present) before calling make -C ../PMOVES-Neo4j so delegation never runs against an uninitialized submodule.
🟡 Minor comments (2)
pmoves/services/github-issue-triage/labeling_rules.py-116-116 (1)
116-116:⚠️ Potential issue | 🟡 MinorDead code:
text_loweris assigned but never used.The variable
text_loweris created but the pattern matching on line 127 usesissue_text(original case). Either remove the unused variable or use it consistently in the pattern matching.🧹 Proposed fix - remove unused variable
- text_lower = issue_text.lower() - # Score each category🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/github-issue-triage/labeling_rules.py` at line 116, The variable text_lower is assigned but never used; either remove that assignment or use it for case-insensitive matching by replacing occurrences of issue_text in the pattern checks with text_lower (or pass text_lower into the regex/search calls) so matching is consistent; specifically update the pattern-matching logic that currently references issue_text to use text_lower for case-insensitive comparisons or delete the unused text_lower assignment if you intend to keep case-sensitive checks.pmoves/docs/NEO4J_INTEGRATION_GUIDE.md-113-115 (1)
113-115:⚠️ Potential issue | 🟡 MinorUse the compose volume key in the reset example.
The service definition uses the
neo4j-datavolume key, sodocker volume rm pmoves_neo4jdatawill not match the generated name. Point readers at the compose volume key, or tell them to resolve it withdocker volume ls, so the reset step actually works.As per coding guidelines,
pmoves/docs/**: "Check docs for operational accuracy: Keep status claims aligned with evidence in runbooks and smokes. Flag command drift across Windows/WSL/Linux instructions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/docs/NEO4J_INTEGRATION_GUIDE.md` around lines 113 - 115, The reset example uses a hardcoded Docker volume name that won't match the compose volume key; update the reset instructions that show the three commands (the docker logs / docker volume rm / make -C pmoves neo4j-local-up block) to either reference the compose volume key "neo4j-data" (so users run docker volume rm for the correct compose-generated name) or add a preceding step instructing users to resolve the exact volume with docker volume ls and remove that listed volume; ensure the text explicitly mentions the compose volume key "neo4j-data" and/or the docker volume ls fallback so the reset command works on all platforms.
🧹 Nitpick comments (2)
pmoves/services/github-issue-triage/labeling_rules.py (1)
36-81: Pattern lists as class attributes are fine here (read-only usage).Ruff flags these as mutable class attribute defaults (RUF012). However, since these lists are never mutated—only read during
__init__to compile regex patterns—this is safe in practice. The patterns act as class-level constants.For stricter compliance, you could use tuples or
ClassVar[tuple[str, ...]]annotations, but this is optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/github-issue-triage/labeling_rules.py` around lines 36 - 81, The class-level pattern lists (BUG_PATTERNS, FEATURE_PATTERNS, DOCUMENTATION_PATTERNS, PERFORMANCE_PATTERNS, SECURITY_PATTERNS, REFACTOR_PATTERNS) should be made immutable to satisfy RUF012; convert each list to a tuple (or annotate with ClassVar[tuple[str, ...]]) so they remain class-level constants and are not mutable defaults, then update any usage that assumes list semantics (if any) to work with tuples or explicitly convert to a list where mutation is required.pmoves/services/github-branch-cleanup/README.md (1)
21-27: Consider adding a language specifier to the code fence.The architecture diagram code block lacks a language identifier, which triggers a markdownlint warning (MD040). Adding
textorplaintextwould satisfy linters.📝 Proposed fix
-``` +```text GitHub Webhook → n8n → NATS: github.webhook.pr.v1 → github-branch-cleanup (port 8100) → Agent Zero MCP (token minting) → GitHub API (branch operations) → NATS: github.branch.deleted.v1</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@pmoves/services/github-branch-cleanup/README.mdaround lines 21 - 27, The
fenced ASCII architecture diagram in the README lacks a language specifier
causing MD040; update the triple-backtick fence for that diagram to include a
language liketextorplaintext(i.e., changetotext) so the block
becomes a labeled code fence and the markdown linter warning is resolved.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@pmoves/Makefile:
- Around line 2777-2787: The neo4j dump/load commands are using invalid auth
flags and missing/incorrect parameters: update the docker exec invocations that
call neo4j-admin (references: neo4j-admin database dump and neo4j-admin database
load, variables $$container_name, $$backup_dir, $$timestamp) to remove
--username/--password (neo4j-admin is an offline local admin), add the required
positional database name (e.g., "neo4j") to the database dump command, and
change the load flag from --from= to --from-path= so the load uses
--from-path=/backups/neo4j_$${timestamp}.dump (or .cypher as appropriate); keep
the surrounding docker cp and echo/ls behavior unchanged.In
@pmoves/scripts/backup-neo4j.sh:
- Around line 78-82: The neo4j-admin invocation in the backup script is invalid:
neo4j-admin database dumprequires a positional name, does not
accept--username/--password, and must operate on an offline DB; it cannot
dump a running server via docker exec. Fix pmoves/scripts/backup-neo4j.sh by
either (A) shutting down the Neo4j container (use CONTAINER_NAME to stop the
container or stop the service inside it), then runneo4j-admin database dump <database>writing to CONTAINER_BACKUP_DIR (remove --username/--password), or
(B) if you have Enterprise and need an online backup, replace the command with
neo4j-admin database backup ...(use Enterprise backup flags) and adjust logic
accordingly; ensure you reference CONTAINER_NAME, CONTAINER_BACKUP_DIR, and the
correct database name when updating the script.In
@pmoves/services/github-branch-cleanup/app.py:
- Around line 458-462: The f-string passed to logger.info in app.py is malformed
because the expression {len(deleted_branches} is missing the closing
brace/parenthesis; update the log message in the logger.info call (where
deleted_branches, protected_skipped and duration are referenced) to use
correctly closed expressions, e.g. ensure {len(deleted_branches)} and
{len(protected_skipped)} and keep duration formatted as {duration:.2f}s so the
file parses.- Around line 495-517: The branch auto-delete currently triggers for any PR
action in ["closed","merged"] but GitHub never emits "merged" — update the guard
in the PR handler to only proceed when action == "closed" AND the pull request
payload indicates it was merged (check pr.get("merged") or
pr.get("pull_request", {}).get("merged") as appropriate), then continue with the
existing protected-branch check (config.is_protected_branch) and deletion flow
(delete_branch, branches_deleted_total.labels(...).inc(), logging); ensure you
do not delete branches for closed-but-unmerged PRs.- Around line 81-88: The stale_days field must be validated on both model and
request-parsing surfaces to avoid treating all branches as stale when days <= 0:
add a positive constraint to the Pydantic field (e.g., in
CleanupRequest.stale_days use gt=0 or equivalent) and validate the query
parameter handler for GET /api/stale-branches (reject or coerce non-positive
values) so config.validate() isn't the only guard; additionally, before
constructing timedelta(stale_days) in the cleanup logic, enforce or assert a
positive integer (raise an HTTP 400 or return a clear error) when stale_days <=
0 so only valid positive day counts reach the deletion logic.- Line 25: The package entrypoint is missing a main.py so running python -m
app fails; add a main.py to the app package that imports and calls the
existing initialization (e.g., import app and call the startup function or
create the ASGI app), or alternatively change the Dockerfile CMD to run the
module/file directly (python app.py) or start uvicorn with the app object
(uvicorn app:app --host 0.0.0.0 --port 8100); update either the package by
adding main.py that invokes the app startup or adjust the Dockerfile CMD to
one of the provided direct execution options.In
@pmoves/services/github-branch-cleanup/Dockerfile:
- Around line 29-39: The Dockerfile copies builder site-packages into
/root/.local then switches to USER pmoves, so the non-root user can't access
installed packages; change the COPY to copy --from=builder /root/.local
/home/pmoves/.local (or copy directly from builder to /home/pmoves/.local),
ensure the directory exists and is chowned to pmoves (mkdir -p
/home/pmoves/.local && chown -R pmoves:pmoves /home/pmoves/.local), and update
the ENV PATH to prepend /home/pmoves/.local/bin instead of /root/.local/bin
before switching to USER pmoves (references: the COPY --from=builder line, ENV
PATH line, and USER pmoves).In
@pmoves/services/github-issue-triage/app.py:
- Around line 209-223: The fallback branch currently treats the output of
LabelingRules.classify_issue() as a dict (pattern_result['label'],
['confidence'], ['reasoning']) which will break because it returns a
ClassificationResult object; update the fallback to use attributes instead
(e.g., check pattern_result.label, then set labels = [pattern_result.label],
confidence = pattern_result.confidence, method = "pattern", and reasoning =
pattern_result.reasoning) so the pattern-based path uses the object properties
rather than dict indexing.- Around line 249-255: The code is reading repository from issue_data but GitHub
sends repository at the top level; update the lookup to read repo from
data.get('repository', {}) .get('full_name') (with optional fallback to
issue_data.get('repository', {}) for safety), keep using issue_data =
data.get('issue', {}) and issue_number = issue_data.get('number'), and adjust
the missing-repo check to use the corrected repo variable so normal
opened/edited events are not dropped; modify the block around action,
issue_data, repo, and issue_number in app.py accordingly.In
@pmoves/tests/test_issue_triage.py:
- Line 6: Import paths with hyphens are invalid; update the test to import
LabelingRules and ClassificationResult from a module path without hyphens.
Rename the package directory github-issue-triage to github_issue_triage (or
change it to use underscores) and then change the import in
pmoves/tests/test_issue_triage.py to use
pmoves.services.github_issue_triage.labeling_rules import LabelingRules,
ClassificationResult; also update the other failing import occurrences
referenced (lines 122, 131, 146, 157) to the underscore-based package name, or
alternatively, if you cannot rename the directory, load the module via
importlib.spec_from_file_location/import_module using the file path and then
access LabelingRules and ClassificationResult dynamically.
Major comments:
In @.claude/context/nats-subjects.md:
- Around line 786-800: Remove the duplicated subject definition for
github.issue.labeled.v1 in the nats subjects doc and keep the schema that
matches the implementation in pmoves/services/github-issue-triage/app.py (the
version containing "labels" array plus "confidence" and "method"); specifically
delete the second conflicting entry (the one with a singular "label" and missing
confidence/method) so the doc only exposes the canonical payload: {"repo",
"issue_number", "labels", "confidence", "method", "timestamp"} and avoid
ambiguity for consumers.- Around line 778-784: Remove the undocumented NATS subject entry
"github.issue.triage.v1" from .claude/context/nats-subjects.md because the
implemented publisher in pmoves/services/github-issue-triage/app.py (see the
triage publish logic around the block that emits "github.issue.labeled.v1" at
lines ~273-286) only emits "github.issue.labeled.v1"; update the documentation
to reflect the actual published subject "github.issue.labeled.v1" and delete the
extraneous example payload for "github.issue.triage.v1".In
@pmoves/docker-compose.yml:
- Line 2177: The environment block currently embeds a hardcoded NATS credential
via the default in the NATS_URL fallback
(NATS_URL=${NATS_URL:-nats://nats:pmoves@nats:4222}); remove the default so the
service uses the external variable only (e.g. NATS_URL=${NATS_URL}) and ensure
the compose service references the appropriate tier env_file that contains the
approved broker URL, and enforce fail-closed behavior by causing docker-compose
to error when NATS_URL is unset (no default). Apply the same change to the other
occurrence that uses the same fallback.- Around line 2186-2188: The compose file maps host port 9096 twice (service
github-branch-cleanup uses "9096:9096" while grayjay-plugin-host maps
"${GRAYJAY_PLUGIN_HOST_PORT:-9096}:8080"), causing port collisions when both
profiles are enabled; update one of the services to avoid the conflict—either
change the github-branch-cleanup service's host mapping to use a configurable
env var (e.g. ${GITHUB_BRANCH_CLEANUP_PORT:-9096}:9096) so it can be overridden,
or pick a different default host port (e.g. 9097) for github-branch-cleanup or
grayjay-plugin-host; ensure the referenced symbols github-branch-cleanup and
grayjay-plugin-host are updated consistently wherever their ports are documented
or consumed.- Around line 2216-2220: Environment variable BOTZ_MCP_URL is set to an
incorrect port (8102) while the botz-gateway service exposes/health-checks port
8054; update the default BOTZ_MCP_URL value to use port 8054
(BOTZ_MCP_URL=${BOTZ_MCP_URL:-http://botz-gateway:8054}) so github-issue-triage
and any callers point at the actual exposed port of botz-gateway; locate the
BOTZ_MCP_URL entry in the docker-compose service env block and change the port
accordingly.In
@pmoves/Makefile:
- Around line 2691-2735: All neo4j-* targets (neo4j-up, neo4j-down,
neo4j-restart, neo4j-logs, neo4j-migrate, neo4j-seed, neo4j-bootstrap,
neo4j-status) must guard that the ../PMOVES-Neo4j submodule is
initialized/populated before delegating; add a small preflight check at the top
of each target that verifies the directory exists and contains expected files
(e.g., test for ../PMOVES-Neo4j/.git or a known path like
../PMOVES-Neo4j/Makefile or db/migrations) and print a clear error and exit if
missing, or alternatively invoke the existing submodule-integrity* target (if
present) before calling make -C ../PMOVES-Neo4j so delegation never runs against
an uninitialized submodule.In
@pmoves/monitoring/prometheus/prometheus.yml:
- Around line 46-50: The prometheus scrape job "job_name: neo4j" should not be
added until the Neo4j Prometheus exporter is actually enabled; remove or guard
the scrape block (the block containing job_name: neo4j, targets: ["neo4j:2004"],
metrics_path and scrape_interval) from
pmoves/monitoring/prometheus/prometheus.yml and instead first add the exporter
configuration to pmoves/docker-compose.yml (expose the exporter port and link it
to the neo4j service). After wiring the exporter into docker-compose.yml and
verifying it listens on the expected port, reintroduce the "job_name: neo4j"
scrape block pointing to the exporter endpoint.In
@pmoves/services/github-branch-cleanup/app.py:
- Around line 221-247: get_branch_commit_date currently calls get_github_token()
for each branch and embeds branch_name raw into the URL; change its signature to
accept a token parameter (e.g., token: str) and URL-encode branch_name with
urllib.parse.quote() when constructing the GET URL, and update
get_stale_branches to call get_github_token() once, pass that token into each
get_branch_commit_date(repo, branch_name, token) call so the token is reused and
branch refs like "feature/foo" won't 404; ensure headers still use the passed
token and preserve response.raise_for_status()/date parsing behavior in
get_branch_commit_date.- Around line 161-170: The /metrics endpoint currently returns generate_latest()
without a Prometheus content-type; update the metrics() handler to return a
fastapi Response with media_type set to CONTENT_TYPE_LATEST and import the
needed symbols: add imports for Response from fastapi.responses and
CONTENT_TYPE_LATEST from prometheus_client, and change the metrics() function to
return Response(generate_latest(), media_type=CONTENT_TYPE_LATEST) (leave
health_check() unchanged).In
@pmoves/services/github-issue-triage/app.py:
- Around line 43-48: The NATS_URL variable currently falls back to a hardcoded
credentialed URL; remove that plaintext fallback and instead load NATS_URL via
the central env-secret helper (use the project's shared env helper/secret loader
and its *_FILE support) so the service fails closed when the secret is missing;
update the NATS_URL assignment (the NATS_URL symbol in this file) to call the
central helper and do not provide a credentialed default value.- Around line 346-361: The /metrics endpoint currently returns generate_latest()
directly which lacks the Prometheus media type; update the metrics() function to
import CONTENT_TYPE_LATEST from prometheus_client and return a FastAPI Response
using Response(content=generate_latest(), media_type=CONTENT_TYPE_LATEST); while
here the health_check() JSON is acceptable, consider returning it via Response
for consistency if desired—make changes in the metrics() function (and imports)
referencing generate_latest and metrics to ensure Prometheus scraping works.In
@pmoves/services/github-issue-triage/hirag_client.py:
- Around line 58-85: The query payload and response parsing must be updated to
Hi-RAG v2: in the method that builds the payload (the block using variables
top_k, rerank, filters and self.query_url) rename payload keys to use "k"
instead of "top_k" and "use_rerank" instead of "rerank" (keep adding filters as
before), and after posting parse response.json() for "hits" (return data["hits"]
when present) and fall back to returning the list if data is a list; also update
the logger.warning to reference the actual keys or structure when the format is
unexpected (use the existing response variable and data variable names to locate
and change logic).
Minor comments:
In@pmoves/docs/NEO4J_INTEGRATION_GUIDE.md:
- Around line 113-115: The reset example uses a hardcoded Docker volume name
that won't match the compose volume key; update the reset instructions that show
the three commands (the docker logs / docker volume rm / make -C pmoves
neo4j-local-up block) to either reference the compose volume key "neo4j-data"
(so users run docker volume rm for the correct compose-generated name) or add a
preceding step instructing users to resolve the exact volume with docker volume
ls and remove that listed volume; ensure the text explicitly mentions the
compose volume key "neo4j-data" and/or the docker volume ls fallback so the
reset command works on all platforms.In
@pmoves/services/github-issue-triage/labeling_rules.py:
- Line 116: The variable text_lower is assigned but never used; either remove
that assignment or use it for case-insensitive matching by replacing occurrences
of issue_text in the pattern checks with text_lower (or pass text_lower into the
regex/search calls) so matching is consistent; specifically update the
pattern-matching logic that currently references issue_text to use text_lower
for case-insensitive comparisons or delete the unused text_lower assignment if
you intend to keep case-sensitive checks.
Nitpick comments:
In@pmoves/services/github-branch-cleanup/README.md:
- Around line 21-27: The fenced ASCII architecture diagram in the README lacks a
language specifier causing MD040; update the triple-backtick fence for that
diagram to include a language liketextorplaintext(i.e., change ``` towarning is resolved. In `@pmoves/services/github-issue-triage/labeling_rules.py`: - Around line 36-81: The class-level pattern lists (BUG_PATTERNS, FEATURE_PATTERNS, DOCUMENTATION_PATTERNS, PERFORMANCE_PATTERNS, SECURITY_PATTERNS, REFACTOR_PATTERNS) should be made immutable to satisfy RUF012; convert each list to a tuple (or annotate with ClassVar[tuple[str, ...]]) so they remain class-level constants and are not mutable defaults, then update any usage that assumes list semantics (if any) to work with tuples or explicitly convert to a list where mutation is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID:
0f582f28-5b51-4bca-88ba-b44c9056b356📒 Files selected for processing (21)
.claude/CLAUDE.md.claude/context/nats-subjects.mdpmoves/Makefilepmoves/docker-compose.ymlpmoves/docs/NEO4J_INTEGRATION_GUIDE.mdpmoves/monitoring/prometheus/prometheus.ymlpmoves/scripts/backup-neo4j.shpmoves/services/github-branch-cleanup/Dockerfilepmoves/services/github-branch-cleanup/README.mdpmoves/services/github-branch-cleanup/__init__.pypmoves/services/github-branch-cleanup/app.pypmoves/services/github-branch-cleanup/config.pypmoves/services/github-branch-cleanup/requirements.txtpmoves/services/github-issue-triage/Dockerfilepmoves/services/github-issue-triage/README.mdpmoves/services/github-issue-triage/app.pypmoves/services/github-issue-triage/hirag_client.pypmoves/services/github-issue-triage/labeling_rules.pypmoves/services/github-issue-triage/requirements.txtpmoves/tests/test_branch_cleanup.pypmoves/tests/test_issue_triage.py✅ Files skipped from review due to trivial changes (1)
- pmoves/services/github-branch-cleanup/init.py
| if docker exec $$container_name neo4j-admin database dump \ | ||
| --to-path=/backups \ | ||
| --overwrite-destination=true \ | ||
| --username=neo4j \ | ||
| --password="$${NEO4J_PASSWORD:-changeme}" 2>/dev/null; then \ | ||
| docker cp $$container_name:/backups/neo4j.dump "$$backup_dir/neo4j_$${timestamp}.dump" 2>/dev/null && \ | ||
| echo "✅ Backup created: $$backup_dir/neo4j_$${timestamp}.dump" && \ | ||
| ls -lh "$$backup_dir/neo4j_$${timestamp}.dump"; \ | ||
| elif docker exec $$container_name cypher-shell -u neo4j -p "$${NEO4J_PASSWORD:-changeme}" \ | ||
| "CALL apoc.export.cypher.all(null, 'file:///backups/neo4j_$${timestamp}.cypher', {})"; then \ | ||
| docker cp $$container_name:/backups/neo4j_$${timestamp}.cypher "$$backup_dir/neo4j_$${timestamp}.cypher" 2>/dev/null && \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the Makefile at the specified lines
head -2787 pmoves/Makefile | tail -20Repository: POWERFULMOVES/PMOVES.AI
Length of output: 1187
🏁 Script executed:
# Also check the second location mentioned (2824-2829)
head -2829 pmoves/Makefile | tail -10Repository: POWERFULMOVES/PMOVES.AI
Length of output: 484
🌐 Web query:
Neo4j neo4j-admin database dump command syntax arguments username password
💡 Result:
neo4j-admin database dump is an offline, local admin command and does not accept username/password arguments.
Syntax (Neo4j Ops Manual):
neo4j-admin database dump [-h] [--expand-commands] [--verbose]
[--overwrite-destination[=true|false]]
[--additional-config=<file>]
[--to-path=<path> | --to-stdout]
<database>[1]
Example:
neo4j-admin database dump neo4j --to-path=/var/backups/neo4j --overwrite-destination=true[1]
If you need an authenticated, online-safe backup, that’s a different command flow (e.g., neo4j-admin database backup for online backups in Enterprise), not database dump. [2]
Sources: Neo4j Operations Manual (offline dump syntax/options) [1], Neo4j neo4j-admin command reference (dump vs backup) [2].
🌐 Web query:
Neo4j neo4j-admin database load --from-path authentication
💡 Result:
neo4j-admin database load --from-path=… does not authenticate to Neo4j with a username/password. It’s a local admin command that loads store files from an on-disk (or cloud-bucket) archive; it must be run with OS-level permissions (Neo4j recommends running it as the neo4j OS user) and you must stop the target database if you’re overwriting it. [1]
What can require “authentication”:
- Cloud storage access (S3/GCS/Azure):
--from-pathcan point to bucket URIs, and then you must provide cloud credentials (AWS/GCP/Azure), not Neo4j credentials. [1] - Neo4j login after restore: if you restored the
systemdatabase (where native users/roles live in modern Neo4j), your instance’s users/passwords may change accordingly; if you’re locked out, follow Neo4j’s password recovery flow (temporarily disable auth and reset the password insystem). [2]
Sources:
[1] Neo4j Operations Manual – “Restore a database dump” (neo4j-admin database load, --from-path, overwrite, cloud paths, run as neo4j user)
[2] Neo4j Operations Manual – “Recover admin user and password” (disable auth + reset via system)
Fix neo4j-admin backup/restore syntax – missing database argument and invalid authentication flags.
The neo4j-admin database dump and neo4j-admin database load commands are offline, local admin commands that do not accept --username or --password arguments. Additionally:
database dumprequires a positional<database>argument (e.g.,neo4j) and is missing it.database loaduses--from=instead of the correct--from-path=parameter.
Current code
# Lines ~2777-2787 (backup)
docker exec $$container_name neo4j-admin database dump \
--to-path=/backups \
--overwrite-destination=true \
--username=neo4j \
--password="$${NEO4J_PASSWORD:-changeme}"
# Lines ~2824-2829 (restore)
docker exec $$container_name neo4j-admin database load \
--from=/backups/neo4j.dump \
--overwrite-destination=true \
--username=neo4j \
--password="$${NEO4J_PASSWORD:-changeme}"These commands will fail immediately with syntax errors. Correct syntax requires removing the auth flags and adding the database name to dump, and using --from-path= for load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/Makefile` around lines 2777 - 2787, The neo4j dump/load commands are
using invalid auth flags and missing/incorrect parameters: update the docker
exec invocations that call neo4j-admin (references: neo4j-admin database dump
and neo4j-admin database load, variables $$container_name, $$backup_dir,
$$timestamp) to remove --username/--password (neo4j-admin is an offline local
admin), add the required positional database name (e.g., "neo4j") to the
database dump command, and change the load flag from --from= to --from-path= so
the load uses --from-path=/backups/neo4j_$${timestamp}.dump (or .cypher as
appropriate); keep the surrounding docker cp and echo/ls behavior unchanged.
| if docker exec "$CONTAINER_NAME" neo4j-admin database dump \ | ||
| --to-path="$CONTAINER_BACKUP_DIR" \ | ||
| --overwrite-destination=true \ | ||
| --username=neo4j \ | ||
| --password="$NEO4J_PASSWORD" 2>/dev/null; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the full script context
find . -name "backup-neo4j.sh" -type fRepository: POWERFULMOVES/PMOVES.AI
Length of output: 99
🏁 Script executed:
# Also check the Neo4j version or any version pinning in the codebase
fd -e "Dockerfile\|docker-compose\|env\|requirements" | head -20Repository: POWERFULMOVES/PMOVES.AI
Length of output: 49
🏁 Script executed:
# Search for neo4j references in the codebase
rg "neo4j" --type sh -A 2 -B 2Repository: POWERFULMOVES/PMOVES.AI
Length of output: 12758
🏁 Script executed:
cat -n pmoves/scripts/backup-neo4j.shRepository: POWERFULMOVES/PMOVES.AI
Length of output: 4063
🌐 Web query:
neo4j-admin database dump command syntax flags username password
💡 Result:
neo4j-admin database dump (Neo4j 5.x+) syntax & flags
Syntax (offline database dump):
neo4j-admin database dump [-h] [--expand-commands] [--verbose]
[--overwrite-destination[=true|false]]
[--additional-config=<file>]
[--to-path=<path> | --to-stdout]
<database>[1]
Key flags
--to-path=<path>: destination directory for the dump file (or a supported cloud-storage URI). [1]--to-stdout: stream the dump to stdout (for piping). [1]--overwrite-destination[=true|false]: overwrite an existing dump file in the destination (defaultfalse). [1]--additional-config=<file>: supply extra config overrides. [1]--expand-commands: allow command expansion in config value evaluation. [1]--verbose,-h/--help. [1]
Username/password?
neo4j-admin database dumpdoes not take Neo4j auth flags (no--username/--password, no-u/-p) because it operates on the local store files (the database must not be mounted/running). [1]
Example
bin/neo4j-admin database dump neo4j --to-path=/full/path/to/dumps[1]
Notes
- Dumps contain database data only; users/roles (RBAC) metadata is not included. [2]
Sources: Neo4j Operations Manual (offline backup / dump command reference) [1] and backup-restore overview notes on dump contents [2].
🌐 Web query:
neo4j-admin database dump running server offline requirements
💡 Result:
neo4j-admin database dump is an offline dump: the target database must be offline (not mounted), so you cannot dump a database that’s mounted in a running Neo4j server. [1]
What “offline” means / requirements
- Database must be stopped/unmounted before running the dump. [1]
- Local-only operation:
- Enterprise: can run the command from an online or offline DBMS, but the database you’re dumping must be stopped; if you’re in a cluster, the database must be stopped on the specific server you run it on. [1]
- Community: can run the command only when the DBMS is offline (i.e., stop the Neo4j service). [1]
- File/dir permissions: ensure the dump target directory exists and has safe permissions (Neo4j warns the target directory must not be world-readable/executable). [1]
- Common operational pattern (incl. Kubernetes): put Neo4j into offline maintenance mode, run dump(s), then bring it back online. [2]
If you need a backup while the server stays running
- Use
neo4j-admin database backup(Enterprise) for online backups from a running server (requires the backup service to be configured). [3]
Sources
- Neo4j Operations Manual – Back up an offline database https://neo4j.com/docs/operations-manual/current/backup-restore/offline-backup/
- Neo4j Operations Manual (Kubernetes) – Dump and load databases (offline) https://neo4j.com/docs/operations-manual/current/kubernetes/operations/dump-load/
- Neo4j Operations Manual – Back up an online database https://neo4j.com/docs/operations-manual/current/backup-restore/online-backup/
🌐 Web query:
neo4j-admin database dump database argument required
💡 Result:
That error means you didn’t provide the required positional <database> argument.
In Neo4j 5+ / current docs, the syntax is:
neo4j-admin database dump ... <database> (database name is not given via --database) [1].
Examples:
# Dump the default database
bin/neo4j-admin database dump neo4j --to-path=/full/path/to/dumps# Or stream to stdout
bin/neo4j-admin database dump neo4j --to-stdout > neo4j.dumpIf you’re following older Neo4j 3.x/4.x instructions, the legacy form was:
neo4j-admin dump --database=MyDatabase --to=backup.dump[2]
Sources: [1] [2]
This neo4j-admin database dump invocation is invalid.
neo4j-admin database dump requires a positional <database> argument (e.g., neo4j), does not accept --username/--password flags, and cannot run against a database mounted in a running server. The database must be offline. Either stop the Neo4j service before dumping, or use neo4j-admin database backup for online backups (Enterprise only). (neo4j.com/docs/operations-manual/current/backup-restore/offline-backup/)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/scripts/backup-neo4j.sh` around lines 78 - 82, The neo4j-admin
invocation in the backup script is invalid: `neo4j-admin database dump` requires
a positional <database> name, does not accept `--username/--password`, and must
operate on an offline DB; it cannot dump a running server via docker exec. Fix
pmoves/scripts/backup-neo4j.sh by either (A) shutting down the Neo4j container
(use CONTAINER_NAME to stop the container or stop the service inside it), then
run `neo4j-admin database dump <database>` writing to CONTAINER_BACKUP_DIR
(remove --username/--password), or (B) if you have Enterprise and need an online
backup, replace the command with `neo4j-admin database backup ...` (use
Enterprise backup flags) and adjust logic accordingly; ensure you reference
CONTAINER_NAME, CONTAINER_BACKUP_DIR, and the correct database name when
updating the script.
| import httpx | ||
| from prometheus_client import Counter, Histogram, Gauge, generate_latest | ||
|
|
||
| from .config import config |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the github-branch-cleanup directory exists and list its contents
fd -t f "github-branch-cleanup" --type d | head -5Repository: POWERFULMOVES/PMOVES.AI
Length of output: 105
🏁 Script executed:
# Look for the Dockerfile in the github-branch-cleanup service
find . -path "*/github-branch-cleanup/Dockerfile" -type fRepository: POWERFULMOVES/PMOVES.AI
Length of output: 117
🏁 Script executed:
# Search for app.py in the github-branch-cleanup directory
find . -path "*/github-branch-cleanup/app.py" -type fRepository: POWERFULMOVES/PMOVES.AI
Length of output: 113
🏁 Script executed:
# List all files in the github-branch-cleanup directory
find . -path "*/github-branch-cleanup/*" -type f | head -20Repository: POWERFULMOVES/PMOVES.AI
Length of output: 373
🏁 Script executed:
cat ./pmoves/services/github-branch-cleanup/DockerfileRepository: POWERFULMOVES/PMOVES.AI
Length of output: 1174
🏁 Script executed:
head -30 ./pmoves/services/github-branch-cleanup/app.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 933
🏁 Script executed:
cat ./pmoves/services/github-branch-cleanup/__init__.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 260
🏁 Script executed:
# Check if __main__.py exists
ls -la ./pmoves/services/github-branch-cleanup/__main__.py 2>&1Repository: POWERFULMOVES/PMOVES.AI
Length of output: 165
🏁 Script executed:
# Look at other services to see their structure and entrypoints
find ./pmoves/services -maxdepth 2 -name "Dockerfile" -type f | head -5Repository: POWERFULMOVES/PMOVES.AI
Length of output: 291
🏁 Script executed:
# Check another service's Dockerfile to see the typical entrypoint pattern
find ./pmoves/services -maxdepth 2 -name "Dockerfile" -type f | head -1 | xargs -I {} sh -c 'echo "=== {} ===" && cat {}'Repository: POWERFULMOVES/PMOVES.AI
Length of output: 593
🏁 Script executed:
# Check the structure of a few other services to see how they handle entrypoints
for service in a2ui-nats-bridge analysis-echo agent-zero; do
echo "=== $service ==="
ls -la ./pmoves/services/$service/ | grep -E "\.py$|__init__|__main__"
doneRepository: POWERFULMOVES/PMOVES.AI
Length of output: 380
🏁 Script executed:
# Check if any service uses relative imports with python -m entrypoint
grep -r "from \\..*import" ./pmoves/services/*/app.py 2>/dev/null | head -5Repository: POWERFULMOVES/PMOVES.AI
Length of output: 140
🏁 Script executed:
# Compare entrypoints across services
for dockerfile in $(find ./pmoves/services -maxdepth 2 -name "Dockerfile" | head -10); do
echo "=== $dockerfile ==="
grep -E "CMD|ENTRYPOINT" "$dockerfile"
doneRepository: POWERFULMOVES/PMOVES.AI
Length of output: 1658
🏁 Script executed:
# Let me check if there's any documentation or patterns about how this directory should be structured
cat ./pmoves/services/github-branch-cleanup/README.md 2>/dev/null | head -30Repository: POWERFULMOVES/PMOVES.AI
Length of output: 1381
🏁 Script executed:
# Check if any service that uses "python -m" with a module name also has __init__.py
find ./pmoves/services -name "__init__.py" -exec dirname {} \; | while read dir; do
dockerfile="$dir/Dockerfile"
if [ -f "$dockerfile" ]; then
cmd=$(grep "^CMD" "$dockerfile" | head -1)
if echo "$cmd" | grep -q "python -m"; then
echo "=== $(basename $dir) ==="
echo "CMD: $cmd"
ls -la "$dir"/__main__.py 2>&1 | grep -E "No such|__main__"
fi
fi
doneRepository: POWERFULMOVES/PMOVES.AI
Length of output: 49
This entrypoint requires __main__.py to exist in the package.
pmoves/services/github-branch-cleanup/Dockerfile runs python -m app, which requires the app package (identified by __init__.py) to have a __main__.py module for Python to execute. Since __main__.py is missing, the service fails to start before any code runs.
The solution is either:
- Add
__main__.pywith an entrypoint that calls the app initialization - Use a direct file execution:
CMD ["python", "app.py"] - Use uvicorn directly:
CMD ["uvicorn", "app:app", "--host", "0.0.0.0", "--port", "8100"]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/github-branch-cleanup/app.py` at line 25, The package
entrypoint is missing a __main__.py so running python -m app fails; add a
__main__.py to the app package that imports and calls the existing
initialization (e.g., import app and call the startup function or create the
ASGI app), or alternatively change the Dockerfile CMD to run the module/file
directly (python app.py) or start uvicorn with the app object (uvicorn app:app
--host 0.0.0.0 --port 8100); update either the package by adding __main__.py
that invokes the app startup or adjust the Dockerfile CMD to one of the provided
direct execution options.
| dry_run: bool = Field( | ||
| default=True, | ||
| description="If true, only report what would be deleted" | ||
| ) | ||
| stale_days: int = Field( | ||
| default=30, | ||
| description="Days of inactivity to consider branch stale" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and examine the relevant sections
fd github-branch-cleanup -type fRepository: POWERFULMOVES/PMOVES.AI
Length of output: 237
🏁 Script executed:
wc -l pmoves/services/github-branch-cleanup/app.py
head -100 pmoves/services/github-branch-cleanup/app.py | tail -40Repository: POWERFULMOVES/PMOVES.AI
Length of output: 1057
🏁 Script executed:
# Also check around line 357-360 for the second location
sed -n '350,370p' pmoves/services/github-branch-cleanup/app.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 662
🏁 Script executed:
# Search for the CleanupRequest class and list_stale_branches endpoint
grep -n "class CleanupRequest\|def list_stale_branches\|def config\|class.*Config" pmoves/services/github-branch-cleanup/app.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 137
🏁 Script executed:
# Check if config.validate() is called and what validations exist
grep -n "config\.\|validate\|stale_days" pmoves/services/github-branch-cleanup/app.py | head -20Repository: POWERFULMOVES/PMOVES.AI
Length of output: 1326
🏁 Script executed:
# Let's find the config module and check config.validate()
fd github-branch-cleanup -type d
find pmoves/services/github-branch-cleanup -name "config*" -o -name "*.py" | head -10Repository: POWERFULMOVES/PMOVES.AI
Length of output: 397
🏁 Script executed:
# Check the imports and config definition
head -40 pmoves/services/github-branch-cleanup/app.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 1145
🏁 Script executed:
# Look for config validation logic
grep -n "class.*Config\|def validate" pmoves/services/github-branch-cleanup/app.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 49
🏁 Script executed:
# Let's see the full config.validate() implementation
sed -n '1,150p' pmoves/services/github-branch-cleanup/app.py | grep -A 20 "def validate"Repository: POWERFULMOVES/PMOVES.AI
Length of output: 49
🏁 Script executed:
# Read the config.py file
cat pmoves/services/github-branch-cleanup/config.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 2692
🏁 Script executed:
# Look at how stale_days is used in the cleanup logic
sed -n '280,330p' pmoves/services/github-branch-cleanup/app.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 1657
🏁 Script executed:
# Check the POST endpoint that uses CleanupRequest
grep -n "post\|cleanup" pmoves/services/github-branch-cleanup/app.py | head -15Repository: POWERFULMOVES/PMOVES.AI
Length of output: 802
🏁 Script executed:
# Find the cleanup endpoint and see how stale_days is handled
sed -n '330,380p' pmoves/services/github-branch-cleanup/app.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 1470
Validate stale_days on both API surfaces to prevent unintended deletion of all branches.
config.validate() only protects the env default (BRANCH_STALE_DAYS). Callers can send 0 or negative values via CleanupRequest.stale_days or GET /api/stale-branches?days=..., which makes every non-protected branch immediately stale. When days is 0 or negative, timedelta(days=stale_days) produces a threshold of today or in the future, causing all commits to be considered stale and subject to deletion when dry_run=False.
🛡️ Suggested guardrails
-from fastapi import FastAPI, HTTPException, BackgroundTasks
+from fastapi import FastAPI, HTTPException, BackgroundTasks, Query
...
stale_days: int = Field(
default=30,
+ ge=1,
description="Days of inactivity to consider branch stale"
)
...
async def list_stale_branches(
repo: str,
- days: int = 30
+ days: int = Query(30, ge=1)
) -> BranchListResponse:Also applies to: 357-360
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/github-branch-cleanup/app.py` around lines 81 - 88, The
stale_days field must be validated on both model and request-parsing surfaces to
avoid treating all branches as stale when days <= 0: add a positive constraint
to the Pydantic field (e.g., in CleanupRequest.stale_days use gt=0 or
equivalent) and validate the query parameter handler for GET /api/stale-branches
(reject or coerce non-positive values) so config.validate() isn't the only
guard; additionally, before constructing timedelta(stale_days) in the cleanup
logic, enforce or assert a positive integer (raise an HTTP 400 or return a clear
error) when stale_days <= 0 so only valid positive day counts reach the deletion
logic.
| logger.info( | ||
| f"Cleanup completed: {len(deleted_branches} deleted, " | ||
| f"{len(protected_skipped)} protected skipped, " | ||
| f"duration={duration:.2f}s" | ||
| ) |
There was a problem hiding this comment.
Fix the malformed f-string.
len(deleted_branches) is missing the closing ) / }, so this file does not parse.
🐛 Proposed fix
logger.info(
- f"Cleanup completed: {len(deleted_branches} deleted, "
+ f"Cleanup completed: {len(deleted_branches)} deleted, "
f"{len(protected_skipped)} protected skipped, "
f"duration={duration:.2f}s"
)📝 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.
| logger.info( | |
| f"Cleanup completed: {len(deleted_branches} deleted, " | |
| f"{len(protected_skipped)} protected skipped, " | |
| f"duration={duration:.2f}s" | |
| ) | |
| logger.info( | |
| f"Cleanup completed: {len(deleted_branches)} deleted, " | |
| f"{len(protected_skipped)} protected skipped, " | |
| f"duration={duration:.2f}s" | |
| ) |
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 459-459: Expected ,, found }
(invalid-syntax)
[warning] 459-459: missing closing quote in string literal
(invalid-syntax)
[warning] 462-462: f-string: unterminated string
(invalid-syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/github-branch-cleanup/app.py` around lines 458 - 462, The
f-string passed to logger.info in app.py is malformed because the expression
{len(deleted_branches} is missing the closing brace/parenthesis; update the log
message in the logger.info call (where deleted_branches, protected_skipped and
duration are referenced) to use correctly closed expressions, e.g. ensure
{len(deleted_branches)} and {len(protected_skipped)} and keep duration formatted
as {duration:.2f}s so the file parses.
| # Only process closed/merged PRs | ||
| if action not in ["closed", "merged"]: | ||
| return | ||
|
|
||
| # Get branch name | ||
| branch = pr.get("head", {}).get("ref", "") | ||
| if not branch: | ||
| return | ||
|
|
||
| # Check if branch should be auto-deleted | ||
| # (Typically: user branches, not release/main) | ||
| if config.is_protected_branch(branch): | ||
| logger.info(f"Branch {branch} is protected, skipping auto-delete") | ||
| return | ||
|
|
||
| # Delete branch (respect DRY_RUN global setting) | ||
| if config.DRY_RUN: | ||
| logger.info(f"[DRY RUN] Would auto-delete branch after PR: {branch}") | ||
| else: | ||
| success = await delete_branch(repo, branch) | ||
| if success: | ||
| branches_deleted_total.labels(repo=repo).inc() | ||
| logger.info(f"Auto-deleted branch after PR: {branch}") |
There was a problem hiding this comment.
Only auto-delete merged PR branches.
GitHub does not emit a merged action; merged PRs arrive as action == "closed" with pull_request.merged == true. The current guard deletes branches for closed-but-unmerged PRs too.
🐛 Proposed fix
- if action not in ["closed", "merged"]:
+ if action != "closed" or not pr.get("merged", False):
return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/github-branch-cleanup/app.py` around lines 495 - 517, The
branch auto-delete currently triggers for any PR action in ["closed","merged"]
but GitHub never emits "merged" — update the guard in the PR handler to only
proceed when action == "closed" AND the pull request payload indicates it was
merged (check pr.get("merged") or pr.get("pull_request", {}).get("merged") as
appropriate), then continue with the existing protected-branch check
(config.is_protected_branch) and deletion flow (delete_branch,
branches_deleted_total.labels(...).inc(), logging); ensure you do not delete
branches for closed-but-unmerged PRs.
| # Pattern-based classification (fallback or enhancement) | ||
| pattern_result = labeling_rules.classify_issue(issue_text) | ||
|
|
||
| # Merge results (prefer semantic if confident) | ||
| if method == "semantic" and confidence >= LABEL_CONFIDENCE_THRESHOLD: | ||
| # Use semantic result | ||
| pass | ||
| else: | ||
| # Use pattern-based result | ||
| if pattern_result['label']: | ||
| labels = [pattern_result['label']] | ||
| confidence = pattern_result['confidence'] | ||
| method = "pattern" | ||
| reasoning = pattern_result['reasoning'] | ||
|
|
There was a problem hiding this comment.
Use ClassificationResult attributes here.
LabelingRules.classify_issue() is consumed as result.label / result.confidence in the new tests. Indexing it like a dict here will raise and break every pattern fallback.
🐛 Proposed fix
- if method == "semantic" and confidence >= LABEL_CONFIDENCE_THRESHOLD:
- # Use semantic result
- pass
- else:
- # Use pattern-based result
- if pattern_result['label']:
- labels = [pattern_result['label']]
- confidence = pattern_result['confidence']
- method = "pattern"
- reasoning = pattern_result['reasoning']
+ if method != "semantic" or confidence < LABEL_CONFIDENCE_THRESHOLD:
+ if pattern_result.label:
+ labels = [pattern_result.label]
+ confidence = pattern_result.confidence
+ method = "pattern"
+ reasoning = pattern_result.reasoning🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/github-issue-triage/app.py` around lines 209 - 223, The
fallback branch currently treats the output of LabelingRules.classify_issue() as
a dict (pattern_result['label'], ['confidence'], ['reasoning']) which will break
because it returns a ClassificationResult object; update the fallback to use
attributes instead (e.g., check pattern_result.label, then set labels =
[pattern_result.label], confidence = pattern_result.confidence, method =
"pattern", and reasoning = pattern_result.reasoning) so the pattern-based path
uses the object properties rather than dict indexing.
| action = data.get('action') | ||
| issue_data = data.get('issue', {}) | ||
| repo = issue_data.get('repository', {}).get('full_name') | ||
| issue_number = issue_data.get('number') | ||
|
|
||
| if not repo or not issue_number: | ||
| logger.warning(f"Invalid webhook event: missing repo or issue_number") |
There was a problem hiding this comment.
Read repository from the top-level webhook payload.
GitHub issue webhooks send repository alongside issue, not inside it. With the current lookup, normal opened / edited events hit the missing repo guard and get dropped.
🐛 Proposed fix
- issue_data = data.get('issue', {})
- repo = issue_data.get('repository', {}).get('full_name')
+ issue_data = data.get('issue', {})
+ repo = data.get('repository', {}).get('full_name')
issue_number = issue_data.get('number')🧰 Tools
🪛 Ruff (0.15.5)
[error] 255-255: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/github-issue-triage/app.py` around lines 249 - 255, The code
is reading repository from issue_data but GitHub sends repository at the top
level; update the lookup to read repo from data.get('repository', {})
.get('full_name') (with optional fallback to issue_data.get('repository', {})
for safety), keep using issue_data = data.get('issue', {}) and issue_number =
issue_data.get('number'), and adjust the missing-repo check to use the corrected
repo variable so normal opened/edited events are not dropped; modify the block
around action, issue_data, repo, and issue_number in app.py accordingly.
| """ | ||
|
|
||
| import pytest | ||
| from pmoves.services.github-issue-triage.labeling_rules import LabelingRules, ClassificationResult |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the test file exists and examine the problematic lines
if [ -f "pmoves/tests/test_issue_triage.py" ]; then
echo "=== File exists. Examining lines 1-10 and around other line numbers ==="
cat -n pmoves/tests/test_issue_triage.py | head -20
echo ""
echo "=== Lines around 122 ==="
cat -n pmoves/tests/test_issue_triage.py | sed -n '120,125p'
echo ""
echo "=== Lines around 131 ==="
cat -n pmoves/tests/test_issue_triage.py | sed -n '129,135p'
echo ""
echo "=== Lines around 146 ==="
cat -n pmoves/tests/test_issue_triage.py | sed -n '144,150p'
echo ""
echo "=== Lines around 157 ==="
cat -n pmoves/tests/test_issue_triage.py | sed -n '155,160p'
else
echo "File does not exist"
fiRepository: POWERFULMOVES/PMOVES.AI
Length of output: 2189
🏁 Script executed:
# Check the directory structure for the service package
echo "=== Checking pmoves/services directory structure ==="
if [ -d "pmoves/services" ]; then
ls -la pmoves/services/
echo ""
find pmoves/services -maxdepth 2 -type d | head -20
else
echo "pmoves/services directory does not exist"
fiRepository: POWERFULMOVES/PMOVES.AI
Length of output: 5026
🏁 Script executed:
# Search for files with "github" in the name to understand the actual structure
echo "=== Searching for github-related paths ==="
find . -type d -name "*github*" 2>/dev/null
find . -type f -path "*github*" 2>/dev/null | head -20Repository: POWERFULMOVES/PMOVES.AI
Length of output: 1534
🏁 Script executed:
# Verify Python's behavior with hyphenated module names
python3 << 'EOF'
# Test if Python can import from a module path with hyphens
import sys
import importlib.util
# Hyphenated names are indeed invalid in Python imports
test_cases = [
"some-module",
"github-issue-triage",
"valid_module"
]
for name in test_cases:
try:
# Try to load the name as a module
spec = importlib.util.find_spec(name)
print(f"'{name}': Valid Python module name")
except (ValueError, ImportError, SyntaxError) as e:
print(f"'{name}': Invalid - {type(e).__name__}: {e}")
EOFRepository: POWERFULMOVES/PMOVES.AI
Length of output: 195
🏁 Script executed:
# Try to parse the test file as Python to see if there are syntax errors
python3 << 'EOF'
import ast
import sys
try:
with open('pmoves/tests/test_issue_triage.py', 'r') as f:
code = f.read()
ast.parse(code)
print("SUCCESS: File parses without syntax errors")
except SyntaxError as e:
print(f"SYNTAX ERROR: {e}")
print(f"Line {e.lineno}: {e.text}")
print(f"Offset: {' ' * (e.offset - 1) if e.offset else ''}^")
except Exception as e:
print(f"ERROR: {type(e).__name__}: {e}")
EOFRepository: POWERFULMOVES/PMOVES.AI
Length of output: 260
🏁 Script executed:
# Also check the actual structure of the github-issue-triage package
echo "=== Contents of github-issue-triage directory ==="
ls -la pmoves/services/github-issue-triage/
echo ""
echo "=== Check if labeling_rules.py exists ==="
if [ -f "pmoves/services/github-issue-triage/labeling_rules.py" ]; then
echo "File exists"
head -5 pmoves/services/github-issue-triage/labeling_rules.py
else
echo "File does not exist"
fiRepository: POWERFULMOVES/PMOVES.AI
Length of output: 820
Fix invalid import syntax in test module.
Python import paths cannot contain hyphens. The directory pmoves/services/github-issue-triage/ must be renamed to use underscores (e.g., github_issue_triage/) for the imports to parse correctly, or the imports must be rewritten using dynamic import mechanisms like importlib.
Also applies to: lines 122, 131, 146, 157
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 6-6: Expected import, found -
(invalid-syntax)
[warning] 6-6: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/test_issue_triage.py` at line 6, Import paths with hyphens are
invalid; update the test to import LabelingRules and ClassificationResult from a
module path without hyphens. Rename the package directory github-issue-triage to
github_issue_triage (or change it to use underscores) and then change the import
in pmoves/tests/test_issue_triage.py to use
pmoves.services.github_issue_triage.labeling_rules import LabelingRules,
ClassificationResult; also update the other failing import occurrences
referenced (lines 122, 131, 146, 157) to the underscore-based package name, or
alternatively, if you cannot rename the directory, load the module via
importlib.spec_from_file_location/import_module using the file path and then
access LabelingRules and ClassificationResult dynamically.
Atomic Commit Review - PR Status UpdateDate: 2026-03-13 SummaryThis PR has been analyzed as part of the atomic commit and PR merge strategy review. Conflict Analysis
Merge RecommendationStatus: Ready to merge after status refresh
See Also
|
This commit addresses all 10 CodeRabbit review comments on the Neo4j submodule integration enhancement PR. Critical Fixes (4): 1. Remove duplicate neo4j-* Makefile targets (lines 643-695) - Old targets referenced non-existent pmoves/integrations/neo4j - Kept canonical targets with corrected submodule paths 2. Fix destructive neo4j-local-down target - Changed from 'down -v --remove-orphans' (tears down everything) - To 'stop neo4j' + 'rm -f neo4j' (only affects Neo4j container) 3. Fix backup directory path resolution - Changed backup_dir from 'pmoves/backups' to 'backups' - Fixed with 'make -C pmoves' path resolution - Improved fallback validation for both .dump and .cypher exports 4. Fix backup-neo4j.sh script - Added env.shared loading for NEO4J_PASSWORD - Changed container path from /backups to /data/backups (mounted volume) - Added directory creation inside container before dump Major Fixes (4): 5. Remove duplicate Neo4j entry in CLAUDE.md - Fixed typo: 'n**Neo4j**' → '**Neo4j**' - Removed duplicate documentation block - Removed port 2004 from exposed ports (internal metrics only) 6. Fix restore volume name - Changed from 'pmoves_neo4jdata' to 'pmoves_neo4j-data' - Matches actual volume name in docker-compose.yml 7. Add Prometheus scrape config for Neo4j - Added neo4j job to prometheus.yml - Scrapes metrics endpoint on port 2004 - 15s scrape interval 8. Fix backup directory resolution (Makefile) - Already addressed in critical fix #3 Minor Fixes (2): 9. Fix Grafana URL in documentation - Changed from http://localhost:3000 to http://localhost:3002 - Matches actual Grafana port configuration 10. Fix restore example path in integration guide - Changed from pmoves/backups/ to backups/ - Correct path resolution with 'make -C pmoves' Additional Improvements: - Removed duplicate neo4j-status target (line 1827) - All Neo4j submodule targets now use ../PMOVES-Neo4j path - Improved error messages in backup/restore logic - Better fallback handling for cypher export Testing: - All Makefile targets verified non-duplicate - Backup paths resolve correctly from pmoves/ directory - Prometheus scrape config validates YAML syntax - Documentation links and URLs corrected Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
b9851cc to
772a9e6
Compare
Add central service for TTS synthesis and casting to Google Cast devices.
Components:
- service.py: HTTP API (port 8060) with 8 endpoints
- flute_client.py: Flute-Gateway TTS client (prosodic synthesis)
- device_manager.py: Cast device discovery and management
- Docker deployment with security hardening
API Endpoints:
- GET /healthz - Health check
- GET /metrics - Prometheus metrics
- GET /devices - List discovered devices
- POST /cast/discover - Trigger device discovery
- POST /cast/speech - TTS synthesis + casting
- POST /cast/audio - Cast audio file
- POST /cast/stop - Stop playback
- GET /cast/status - Device status
NATS Events:
- voice.cast.completed.v1 - Cast successful
- voice.cast.request.v1 - Cast request received
- voice.cast.failed.v1 - Cast failed
Prometheus Metrics:
- cast_tts_requests_total{method, status}
- cast_tts_latency_seconds
- cast_device_discoveries_total
Integration:
- Flute-Gateway (port 8055/8056) for prosodic TTS
- Ultimate-TTS Studio (port 7861) for fallback
- NATS (port 4222) for event coordination
Related: #898
Register Google Cast integration as Agent Zero plugin. Plugin Features: - 6 MCP tool definitions (cast_discover, cast_list, cast_speech, etc.) - 5 NATS event subjects (voice.cast.*, device.cast.*) - Service endpoint registration (health, metrics) - Configuration schema (FLUTE_GATEWAY_URL, ULTIMATE_TTS_URL, NATS_URL) Related: #898
Documentation files: - pmoves/docs/voice/cast-integration.md: Complete API reference, architecture overview, troubleshooting, performance tuning - pmoves/docs/voice/QUICKSTART_CAST.md: 5-minute setup guide - IMPLEMENTATION_SUMMARY_CAST.md: Implementation details and status - .gitignore: Added temp audio files for Cast TTS Topics covered: - Architecture (4-tier: Agent Zero → Flute-Gateway → Cast TTS Gateway → Devices) - MCP tools reference (6 tools) - Cast TTS Gateway API (8 endpoints) - NATS event integration - Voice agent pipeline - Configuration reference - Troubleshooting guide - Performance tuning - Advanced usage Related: #898
Add central service for TTS synthesis and casting to Google Cast devices.
Components:
- service.py: HTTP API (port 8060) with 8 endpoints
- flute_client.py: Flute-Gateway TTS client (prosodic synthesis)
- device_manager.py: Cast device discovery and management
- Docker deployment with security hardening
API Endpoints:
- GET /healthz - Health check
- GET /metrics - Prometheus metrics
- GET /devices - List discovered devices
- POST /cast/discover - Trigger device discovery
- POST /cast/speech - TTS synthesis + casting
- POST /cast/audio - Cast audio file
- POST /cast/stop - Stop playback
- GET /cast/status - Device status
NATS Events:
- voice.cast.completed.v1 - Cast successful
- voice.cast.request.v1 - Cast request received
- voice.cast.failed.v1 - Cast failed
Prometheus Metrics:
- cast_tts_requests_total{method, status}
- cast_tts_latency_seconds
- cast_device_discoveries_total
Integration:
- Flute-Gateway (port 8055/8056) for prosodic TTS
- Ultimate-TTS Studio (port 7861) for fallback
- NATS (port 4222) for event coordination
Related: #898
Register Google Cast integration as Agent Zero plugin. Plugin Features: - 6 MCP tool definitions (cast_discover, cast_list, cast_speech, etc.) - 5 NATS event subjects (voice.cast.*, device.cast.*) - Service endpoint registration (health, metrics) - Configuration schema (FLUTE_GATEWAY_URL, ULTIMATE_TTS_URL, NATS_URL) Related: #898
Documentation files: - pmoves/docs/voice/cast-integration.md: Complete API reference, architecture overview, troubleshooting, performance tuning - pmoves/docs/voice/QUICKSTART_CAST.md: 5-minute setup guide - IMPLEMENTATION_SUMMARY_CAST.md: Implementation details and status - .gitignore: Added temp audio files for Cast TTS Topics covered: - Architecture (4-tier: Agent Zero → Flute-Gateway → Cast TTS Gateway → Devices) - MCP tools reference (6 tools) - Cast TTS Gateway API (8 endpoints) - NATS event integration - Voice agent pipeline - Configuration reference - Troubleshooting guide - Performance tuning - Advanced usage Related: #898
Add dual-output TTS processor for Google Cast integration.
Components:
- CastAudioOutputProcessor: Dual TTS output (local + Cast)
- CastAudioOnlyProcessor: Cast-only output (no local audio)
Features:
- Integrates with Cast TTS Gateway API (port 8060)
- Non-blocking async HTTP calls
- Device selection via default_device parameter
- Error handling with fallback to local TTS
- Voice configuration support
- Graceful cleanup on cancellation
Usage:
Dual-output mode:
cast = CastAudioOutputProcessor(
cast_gateway_url="http://localhost:8060",
default_device="Brysons Speakers speaker"
)
pipeline = Pipeline([vibevoice_tts, cast, ...])
Cast-only mode:
cast_only = CastAudioOnlyProcessor(
cast_gateway_url="http://localhost:8060"
)
pipeline = Pipeline([cast_only, ...])
File: pmoves/services/flute-gateway/pipecat/processors/cast_audio.py
Lines: ~280
Related: #898, Phase 2 Component 1/4
Add NATS subscriber for automatic casting of agent voice responses. Components: - VoiceFollowCastAgent: Main agent class - NotebookClient: Open-Notebook integration for logging - CLI interface: Run as standalone service Features: - Subscribes to voice.agent.response.v1 and agent.response.v1 - Extracts text from multiple envelope formats - Calls Cast TTS Gateway API for synthesis - Optional Open-Notebook logging - Environment-based configuration - Graceful error handling NATS Subjects: - voice.agent.response.v1 (preferred) - agent.response.v1 (fallback) Environment Config: - CAST_FOLLOW_NATS_URL (default: nats://127.0.0.1:4222) - CAST_TTS_GATEWAY_URL (default: http://localhost:8060) - CAST_DEFAULT_DEVICE (default: None → auto-select) - CAST_FOLLOW_VOICE (default: default) - CAST_NOTEBOOK_ENABLED (default: false) - OPEN_NOTEBOOK_API_URL (optional) - OPEN_NOTEBOOK_API_TOKEN (optional) Usage: export CAST_TTS_GATEWAY_URL="http://localhost:8060" export CAST_DEFAULT_DEVICE="Brysons Speakers speaker" python pmoves/tools/voice_follow_cast_agent.py File: pmoves/tools/voice_follow_cast_agent.py Lines: ~390 Related: #898, Phase 2 Component 2/4
Add standalone utility for logging Cast events to Open-Notebook.
Components:
- CastNotebookLogger: Main logger class
- CLI interface: Manual logging from command line
- Rich formatting: Markdown-formatted event content
Features:
- Log successful/failed Cast events
- Human-readable event formatting
- Metadata support
- Multiple notebook support
- Graceful error handling
Event Types:
- voice_cast_completed
- voice_cast_failed
Environment:
- OPEN_NOTEBOOK_API_URL (default: http://localhost:5055)
- OPEN_NOTEBOOK_API_TOKEN (required)
- CAST_NOTEBOOK_DEFAULT (default: cast-logs)
Usage:
from pmoves.tools.cast_notebook_logger import CastNotebookLogger
logger = CastNotebookLogger(
api_base="http://localhost:5055",
token="your-token"
)
logger.log_cast_completed(
device="Brysons Speakers speaker",
text="Hello world",
voice="default",
duration_ms=2500
)
CLI:
python cast_notebook_logger.py \
--event voice_cast_completed \
--device "Brysons Speakers speaker" \
--text "Hello world" \
--duration 2500
File: pmoves/tools/cast_notebook_logger.py
Lines: ~280
Related: #898, Phase 2 Component 3/4
Add Google Cast support to Flute-Gateway voice agent pipelines.
Changes:
- Import CastAudioOutputProcessor and CastAudioOnlyProcessor
- Add Cast configuration to VoiceAgentConfig:
* enable_cast: Enable/disable Cast output
* cast_gateway_url: Cast TTS Gateway URL
* cast_device: Default Cast device name
* cast_only_mode: Cast-only mode (disable local audio)
- Add Cast parameters to build_voice_agent_pipeline():
* enable_cast: Function-level override
* cast_gateway_url: Gateway URL override
* cast_device: Device name override
* cast_only_mode: Cast-only mode override
- Integrate Cast processor into pipeline (after TTS)
- Support dual-output mode (local + Cast)
- Support cast-only mode (Cast output only)
Pipeline Flow:
Standard: VAD → STT → LLM → TTS → Audio Output
With Cast: VAD → STT → LLM → TTS → Cast Audio Output
Cast-only: VAD → STT → LLM → Cast Audio Output
Configuration Priority:
Function params > config params > defaults
Usage:
# Dual-output mode (local + Cast)
pipeline = await build_voice_agent_pipeline(
transport, config,
enable_cast=True,
cast_device="Brysons Speakers speaker"
)
# Cast-only mode (no local audio)
pipeline = await build_voice_agent_pipeline(
transport, config,
enable_cast=True,
cast_only_mode=True,
cast_device="Brysons Speakers speaker"
)
# Via config
config = VoiceAgentConfig(
persona="assistant",
enable_cast=True,
cast_device="Brysons Speakers speaker"
)
Modified: pmoves/services/flute-gateway/pipecat/pipelines/voice_agent.py
Lines: ~50 modified
Related: #898, Phase 2 Component 4/4
…ity queue
Add Phase 3 multi-room audio capabilities to Cast TTS Gateway.
Components:
- groups.py: CastDeviceGroup and CastGroupManager for device grouping
- concurrent.py: ConcurrentCaster for parallel multi-device casting
- queue.py: CastPriorityQueue for priority-based announcement queuing
Features:
- Device groups: Create/list/delete logical groupings of Cast devices
- Concurrent casting: Parallel audio playback to multiple devices
- Priority queue: Ordered announcement queue (urgent/high/normal/low)
- Error isolation: Individual device failures don't stop others
- Progress tracking: Per-device results and aggregate metrics
API Endpoints:
- POST /cast/groups - Create device group
- GET /cast/groups - List all groups
- DELETE /cast/groups/{name} - Delete group
- PUT /cast/groups/{name} - Update group
- POST /cast/speech - Support 'group' parameter for multi-room
- GET /cast/queue - Inspect priority queue
- DELETE /cast/queue - Clear all announcements
- DELETE /cast/queue/{id} - Remove specific announcement
Usage:
# Create device group
curl -X POST http://localhost:8060/cast/groups \
-d '{"name": "All Speakers", "devices": ["Brysons Speakers speaker", "Den speaker"]}'
# Cast to group (multi-room)
curl -X POST http://localhost:8060/cast/speech \
-d '{"text": "Multi-room test", "group": "All Speakers"}'
# Priority announcement
curl -X POST http://localhost:8060/cast/speech \
-d '{"text": "Urgent alert", "priority": "urgent", "group": "All Speakers"}'
Related: #898, Phase 3 Complete
3 participants
Neo4j Submodule Integration Enhancement
Summary
Complete Neo4j submodule integration following PMOVES-supabase pattern with root-level submodule, comprehensive monitoring, and automated backup/restore.
🎯 Key Changes
1. Submodule Structure
PMOVES-Neo4j(waspmoves/integrations/neo4j)2. Infrastructure Files
Integration Guide:
pmoves/docs/NEO4J_INTEGRATION_GUIDE.mdGrafana Dashboard:
pmoves/monitoring/grafana/dashboards/neo4j-overview.jsonBackup Automation:
pmoves/scripts/backup-neo4j.shpmoves/backups/3. Makefile Targets
Submodule Delegation:
neo4j-up,neo4j-down,neo4j-restart: Lifecycle managementneo4j-migrate VERSION=003: Run migrationsneo4j-seed SEED=001_person_aliases.csv: Load seed dataneo4j-bootstrap: Initialize Neo4j (migrations + seeds)Profile-based Integration (following PMOVES-supabase pattern):
neo4j-local-up: Start with neo4j-local profileneo4j-local-down: Stop and clean up volumesneo4j-local-status: Check container statusBackup/Restore:
neo4j-backup: Create timestamped backupneo4j-restore BACKUP=...: Restore from backup4. Context Updates
.claude/CLAUDE.mdGET http://localhost:7474/db/neo4j/healthhttp://localhost:7474PMOVES-Neo4j/CLAUDE.md🔄 Migration System
001_init.cypher002_chit_geometry.cypher003_consciousness_taxonomy.cypher🧪 Testing
📊 Monitoring
🔐 Security
pmoves/env.shared(gitignored)📚 Documentation
pmoves/docs/NEO4J_INTEGRATION_GUIDE.mdPMOVES-Neo4j/CLAUDE.mdpmoves/docs/NEO4J_SUBMODULE_INTEGRATION_COMPLETE.mdpmoves/docs/NEO4J_SUBMODULE_PROMOTION.md🔗 Integrations
pmoves_cipher_store,pmoves_cipher_search(separate repo)✅ Atomic Commits
🎁 Benefits
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Summary by CodeRabbit
Documentation
New Features
Chores