fix(security): resolve all remaining P2 submodule issues - #848
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughUpdates two submodule pointers, refactors production audit and P2 submodule tracker docs to mark all P2 items resolved with expanded narratives, and enhances local runner tooling to support platform-aware Docker socket mounting and a PAT/short-lived token selection flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as Runner CLI
participant TokenSvc as access_token
participant GitHub as GitHub (PAT/API)
participant Docker as docker_run
participant Daemon as Docker Daemon
CLI->>TokenSvc: request token (repo, lane)
TokenSvc->>GitHub: fetch PAT (if available)
GitHub-->>TokenSvc: PAT or no-PAT (short token)
TokenSvc-->>CLI: (token, is_pat)
CLI->>Docker: docker_run(image, token, is_pat)
Docker->>Daemon: mount socket (_docker_socket_mount())
Docker->>Daemon: start container with env (ACCESS_TOKEN or RUNNER_TOKEN)
Daemon-->>Docker: container started
Docker-->>CLI: run complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md (1)
749-801:⚠️ Potential issue | 🟠 MajorThis checklist is now out of sync with the dashboard state.
The section still instructs operators to close AB-4/5/6 and recover AB-9, but the dashboard now says active blockers are
0and AB-9 is resolved. Please rewrite this into a current verification checklist for the Mar 10 baseline, or move the old commands into a clearly marked historical section.As per coding guidelines, "pmoves/docs/**: Check docs for operational accuracy: Keep status claims aligned with evidence in runbooks and smokes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md` around lines 749 - 801, The Validation Checklist and Resolution Sequence in pmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md still list AB-4, AB-5, AB-6 and AB-9 as active blockers; update the document for the Mar 10 baseline by either (a) removing or marking those AB-* steps as resolved and replacing them with a short current verification checklist that reflects "active blockers: 0" (e.g., a small set of smoke/health checks and a note pointing to evidence) or (b) moving the original AB-4/AB-5/AB-6/AB-9 commands under a clearly labeled "Historical remediation commands" section; ensure you edit the "Validation Checklist" and "Resolution Sequence" headings and mention AB-9 as resolved, and keep the original commands under a historical heading if preserving them for auditability.pmoves/docs/security/P2_SUBMODULE_TRACKER.md (1)
55-59:⚠️ Potential issue | 🟡 MinorUpdate the resolution process to cover
CLOSEDoutcomes too.This procedure still says every P2 requires a submodule PR and a
FIXEDstatus, but this tracker now closes items as wontfix, accepted risk, false positive, and stale/already-clean. Please document those non-code closure paths here as well so future updates follow the same rules the table is already using.As per coding guidelines, "pmoves/docs/**: Check docs for operational accuracy: Keep status claims aligned with evidence in runbooks and smokes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/docs/security/P2_SUBMODULE_TRACKER.md` around lines 55 - 59, The document currently mandates a submodule PR and marking status as FIXED but omits non-code closure flows; update P2_SUBMODULE_TRACKER.md to document CLOSED outcomes (wontfix, accepted risk, false positive, stale/already-clean) by adding a new clause alongside steps 1–4 that: (1) enumerates each CLOSED reason (wontfix, accepted risk, false positive, stale/already-clean), (2) states that a submodule PR and parent gitlink update are only required when a code change is produced (i.e., when status is FIXED), (3) requires the tracker update to set status=CLOSED with the closure reason, approver name, date, and brief evidence or rationale (link to assessment or ticket comments), and (4) instructs to attach or link any acceptance/risk decisions or validation runbook evidence for auditability so future reviewers can verify the non-code closure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PMOVES-Open-Notebook`:
- Line 1: Replace the permissive shell expansion
${SURREAL_PASSWORD:-changeme_surreal} used in the production compose/service
configuration with the fail-when-missing expansion ${SURREAL_PASSWORD:?required}
so the service will abort startup if SURREAL_PASSWORD is not set; keep the
insecure "changeme_surreal" value only in an explicit dev-only override (e.g., a
dev compose file or .env.example) rather than in production
compose/configuration.
In `@pmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md`:
- Around line 6-8: The docs header currently contains the literal placeholder
"Commit: pending"; replace that placeholder in PRODUCTION_AUDIT_DASHBOARD.md
with the exact parent commit SHA (or, if the final commit isn't available yet,
the final submodule-update commit or a PR reference like "#1234") so the March
10 snapshot is reproducible; update the "Commit:" line to the chosen SHA/PR
reference, commit the change to the branch, and push so the baseline is pinned.
In `@pmoves/integrations/archon`:
- Line 1: The PR currently updates the gitlink for pmoves/integrations/archon
without showing what changed; add the upstream commit range or a diff/compare
output so reviewers can verify the bump — specifically include the commit range
178ebb9e6a054520979dd448d0f5675a1a55cac7..e965b4f50d1c1362a0442cd1ab99b2d790fa3f82
(or the generated git diff/compare URL and a short summary of notable changes)
in the PR description or as a checked-in changelog entry referencing
pmoves/integrations/archon so reviewers can inspect the exact upstream changes.
---
Outside diff comments:
In `@pmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md`:
- Around line 749-801: The Validation Checklist and Resolution Sequence in
pmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md still list AB-4, AB-5, AB-6 and AB-9
as active blockers; update the document for the Mar 10 baseline by either (a)
removing or marking those AB-* steps as resolved and replacing them with a short
current verification checklist that reflects "active blockers: 0" (e.g., a small
set of smoke/health checks and a note pointing to evidence) or (b) moving the
original AB-4/AB-5/AB-6/AB-9 commands under a clearly labeled "Historical
remediation commands" section; ensure you edit the "Validation Checklist" and
"Resolution Sequence" headings and mention AB-9 as resolved, and keep the
original commands under a historical heading if preserving them for
auditability.
In `@pmoves/docs/security/P2_SUBMODULE_TRACKER.md`:
- Around line 55-59: The document currently mandates a submodule PR and marking
status as FIXED but omits non-code closure flows; update P2_SUBMODULE_TRACKER.md
to document CLOSED outcomes (wontfix, accepted risk, false positive,
stale/already-clean) by adding a new clause alongside steps 1–4 that: (1)
enumerates each CLOSED reason (wontfix, accepted risk, false positive,
stale/already-clean), (2) states that a submodule PR and parent gitlink update
are only required when a code change is produced (i.e., when status is FIXED),
(3) requires the tracker update to set status=CLOSED with the closure reason,
approver name, date, and brief evidence or rationale (link to assessment or
ticket comments), and (4) instructs to attach or link any acceptance/risk
decisions or validation runbook evidence for auditability so future reviewers
can verify the non-code closure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb583a30-9a2c-445a-847e-71f98a6dae37
📒 Files selected for processing (4)
PMOVES-Open-Notebookpmoves/docs/PRODUCTION_AUDIT_DASHBOARD.mdpmoves/docs/security/P2_SUBMODULE_TRACKER.mdpmoves/integrations/archon
| @@ -1 +1 @@ | |||
| Subproject commit e9ccae69e49b7b6dd388c3956202eedc0fb27e4c | |||
| Subproject commit 0533c8aff5e73ee288c0b83ff49beb304df82f29 | |||
There was a problem hiding this comment.
Fail closed instead of defaulting SurrealDB to changeme_surreal.
Per the PR notes, this gitlink brings in ${SURREAL_PASSWORD:-changeme_surreal}. That still allows the service to boot with a known credential whenever the env var is missing, so the “security fix” remains one misconfiguration away from a default-secret exposure. Please switch the production compose path to ${SURREAL_PASSWORD:?required} and keep any insecure default only in an explicit dev-only override.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PMOVES-Open-Notebook` at line 1, Replace the permissive shell expansion
${SURREAL_PASSWORD:-changeme_surreal} used in the production compose/service
configuration with the fail-when-missing expansion ${SURREAL_PASSWORD:?required}
so the service will abort startup if SURREAL_PASSWORD is not set; keep the
insecure "changeme_surreal" value only in an explicit dev-only override (e.g., a
dev compose file or .env.example) rather than in production
compose/configuration.
| **Last Updated:** 2026-03-10 (all P2 items resolved — 0 open) | ||
| **Branch:** `main` | ||
| **Commit:** pending (P2 final resolution) |
There was a problem hiding this comment.
Replace the pending commit placeholder before this becomes the new baseline.
For a dashboard described as the single source of truth, Commit: pending makes the Mar 10 snapshot hard to reproduce. Please pin this to the actual parent SHA or, at minimum, the final submodule-update commit/PR reference once it exists.
As per coding guidelines, "pmoves/docs/**: Check docs for operational accuracy: Keep status claims aligned with evidence in runbooks and smokes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md` around lines 6 - 8, The docs
header currently contains the literal placeholder "Commit: pending"; replace
that placeholder in PRODUCTION_AUDIT_DASHBOARD.md with the exact parent commit
SHA (or, if the final commit isn't available yet, the final submodule-update
commit or a PR reference like "#1234") so the March 10 snapshot is reproducible;
update the "Commit:" line to the chosen SHA/PR reference, commit the change to
the branch, and push so the baseline is pinned.
| @@ -1 +1 @@ | |||
| Subproject commit 178ebb9e6a054520979dd448d0f5675a1a55cac7 | |||
| Subproject commit e965b4f50d1c1362a0442cd1ab99b2d790fa3f82 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Gitlink diff for pmoves/integrations/archon =="
git diff --submodule=log -- pmoves/integrations/archon || true
echo
echo "== Submodule URL from .gitmodules =="
git config -f .gitmodules --get submodule.pmoves/integrations/archon.url || true
echo
if [ -d "pmoves/integrations/archon/.git" ] || [ -f "pmoves/integrations/archon/.git" ]; then
echo "== Commit range inside submodule checkout =="
git -C pmoves/integrations/archon log --oneline --decorate --no-merges \
178ebb9e6a054520979dd448d0f5675a1a55cac7..e965b4f50d1c1362a0442cd1ab99b2d790fa3f82
else
echo "Submodule checkout is not available locally."
echo "Please include the upstream compare/log for:"
echo "178ebb9e6a054520979dd448d0f5675a1a55cac7..e965b4f50d1c1362a0442cd1ab99b2d790fa3f82"
fiRepository: POWERFULMOVES/PMOVES.AI
Length of output: 379
Include the upstream diff for the pmoves/integrations/archon submodule bump.
In a security PR, gitlink-only changes are difficult to review. Please include the upstream commit range (178ebb9e6a054520979dd448d0f5675a1a55cac7..e965b4f50d1c1362a0442cd1ab99b2d790fa3f82) or comparison output so reviewers can verify whether this bump is intentional, correct, and related to the P2 closure objectives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/integrations/archon` at line 1, The PR currently updates the gitlink
for pmoves/integrations/archon without showing what changed; add the upstream
commit range or a diff/compare output so reviewers can verify the bump —
specifically include the commit range
178ebb9e6a054520979dd448d0f5675a1a55cac7..e965b4f50d1c1362a0442cd1ab99b2d790fa3f82
(or the generated git diff/compare URL and a short summary of notable changes)
in the PR description or as a checked-in changelog entry referencing
pmoves/integrations/archon so reviewers can inspect the exact upstream changes.
Local Validation Results + CI Runner FixP2 Validation (all passing)
CI Runner FixAll 3 self-hosted Linux runners were offline due to expired
Root cause fix: Switched from Cross-platform fix: Docker socket mount path now detects Windows vs Linux ( Build jobs should now start processing. 🏗️ |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pmoves/tools/local_cert_runners.py (2)
174-176: Add return type hint for consistency with coding guidelines.The function is missing a return type annotation. Per coding guidelines, type hints are preferred.
✨ Suggested fix
def docker_run( repo: str, image: str, lane: RunnerLane, token: str, *, is_pat: bool = True, -) -> None: +) -> None:Actually, the signature already shows
-> Noneis missing. Add it:def docker_run( - repo: str, image: str, lane: RunnerLane, token: str, *, is_pat: bool = True, -) -> None: + repo: str, image: str, lane: RunnerLane, token: str, *, is_pat: bool = True, +) -> None:As per coding guidelines: "prefer type hints in Python 3.11+".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tools/local_cert_runners.py` around lines 174 - 176, The docker_run function signature is missing an explicit return type; update the definition of docker_run(repo: str, image: str, lane: RunnerLane, token: str, *, is_pat: bool = True) to include the return type annotation -> None so it conforms to the project's typing guidelines (ensure the function name docker_run is updated accordingly).
245-246: Missing blank line between function definitions.PEP 8 recommends two blank lines between top-level function definitions. There's no blank line between
_runner_log_argsand_selected_lanes.✨ Suggested fix
except json.JSONDecodeError: pass return ["--log-driver", "json-file"] + + def _selected_lanes(names: list[str] | None) -> tuple[RunnerLane, ...]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tools/local_cert_runners.py` around lines 245 - 246, Add the missing blank line between the top-level functions: insert one additional empty line after the end of _runner_log_args (the function that returns ["--log-driver", "json-file"]) so there are two blank lines separating it from the next top-level function _selected_lanes(names: list[str] | None) -> tuple[RunnerLane, ...], complying with PEP8's two-blank-line rule for function definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pmoves/tools/local_cert_runners.py`:
- Around line 174-176: The docker_run function signature is missing an explicit
return type; update the definition of docker_run(repo: str, image: str, lane:
RunnerLane, token: str, *, is_pat: bool = True) to include the return type
annotation -> None so it conforms to the project's typing guidelines (ensure the
function name docker_run is updated accordingly).
- Around line 245-246: Add the missing blank line between the top-level
functions: insert one additional empty line after the end of _runner_log_args
(the function that returns ["--log-driver", "json-file"]) so there are two blank
lines separating it from the next top-level function _selected_lanes(names:
list[str] | None) -> tuple[RunnerLane, ...], complying with PEP8's
two-blank-line rule for function definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42c915a8-7fba-4d6c-80e1-76ced2d1303c
📒 Files selected for processing (1)
pmoves/tools/local_cert_runners.py
583957f to
0c441fe
Compare
…rmed open Checked all 11 open P2 items against current submodule SHAs on main: - 6 CLOSED: BoTZ export syntax, PMOVES.YT MinIO creds, Pipecat MCP allowlist, A2UI env.shared export, A2UI NATS auth, A2UI env.tier-ui.sh - 1 IMPROVED: Open-Notebook /healthz alias added (/metrics still absent) - 4 CONFIRMED OPEN: Open-Notebook SurrealDB root:root, Pipecat metrics, tensorzero RUSTSEC advisories, tensorzero example creds (all Tier 2/3) Also corrected Trivy triage claim: dashboard previously said CVE pins were "added to Dockerfiles" — corrected to "triaged with remediation plan" (archon pins not yet committed, deepresearch status TBD). P2 tracker: 4 open + 1 improved / 17 total (down from 11 open) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…status Submodule update (archon): added Dockerfile.server pip upgrade step for crawl4ai>=0.8.0 (CVE-2026-26216) and langchain-core>=1.2.5 (CVE-2025-68664). Dashboard corrections: - deepresearch: ray==2.52.0 + vllm==0.14.1 were already committed in pmoves/services/deepresearch/Dockerfile:17-20 (previous correction over-stated they were missing) - archon: now COMPLETE with Dockerfile override - Trivy critical CVEs: 2/2 affected images remediated Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Open-Notebook: SurrealDB creds parameterized (#3), /metrics endpoint added (#5). Pipecat metrics (#10), tensorzero RUSTSEC (#13), tensorzero example secrets (#14) closed with documented justification. P2 tracker: 0 open / 17 total (all resolved) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace RUNNER_TOKEN (expires in ~1h) with ACCESS_TOKEN (PAT-based auto-registration) in local_cert_runners.py. Containers now auto-fetch fresh registration tokens on restart, preventing the perpetual 404 registration loop that caused all 3 Linux runners to go offline. Also fix cross-platform Docker socket mount path for Windows/WSL and skip GPU passthrough on Windows (requires nvidia-container-toolkit). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…stry Add RUNNER_PAT to bootstrap registry for reboot-survivable runner authentication. Update MAKE_TARGETS.md and github-runner-workflows.md to document the ACCESS_TOKEN strategy, env var cascade, and troubleshooting steps. Source env.shared in Make targets so PAT is available without manual shell export. Validated against upstream myoung34/github-runner docs: - ACCESS_TOKEN with repo scope is correct for repo-scoped runners - Container auto-deregisters on stop via signal trap - Credentials are un-exported after setup (workflow-safe) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
0c441fe to
c2a38ff
Compare
2 participants
Summary
root:rootcredentials replaced with${SURREAL_PASSWORD:-changeme_surreal}env var substitution/metricsPrometheus endpoint added viaprometheus_client+make_asgi_app()deny.toml${VAR:?required}fail-closed patternP2 tracker: 0 open / 17 total (all resolved)
Changes
PMOVES-Open-Notebookgitlink updated (3 commits: SurrealDB creds, /metrics endpoint, security posture docs)pmoves/docs/security/P2_SUBMODULE_TRACKER.md— all items marked resolved with justificationpmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md— updated to reflect 0 open P2sTest plan
make -C pmoves docs-reconcile-checkshows 0 open P2sdocker-compose.ymluses${SURREAL_PASSWORD:-changeme_surreal}(noroot:root)api/main.pyimportsprometheus_clientand mounts/metricsgrep -r "root:root" PMOVES-Open-Notebook/docker-compose*.ymlreturns nothing🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores