feat(preflight): deterministic submodule lane + showtime verify pages - #627
Conversation
feat(cipher): Cipher Memory MCP bridge + Claude config unification
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a manifest and Make targets for deterministic submodule-layer validation; introduces multiple new command-line tools for bootstrapping, submodule validation/orchestration, secrets hydration/audit, runner management/mapping, and Showtime endpoint verification; updates docs and build infra to wire these tools into audit and bringup flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Validator as Submodule<br/>Validator
participant Manifest
participant Git
participant Filesystem
participant PythonCheck as Python<br/>Compiler
participant Reports as JSON/MD
User->>Validator: run submodule-layer-validate
Validator->>Manifest: load manifest & policy
Validator->>Git: parse .gitmodules & status
loop per submodule
Validator->>Filesystem: check required/top-level files
Filesystem-->>Validator: file results
Validator->>Git: check remote commit reachability
Git-->>Validator: reachability result
Validator->>PythonCheck: run python_compile_check (if enabled)
PythonCheck-->>Validator: compile results
end
Validator->>Reports: aggregate findings, write JSON & MD
Reports-->>User: artifacts written
sequenceDiagram
participant User
participant Showtime as Showtime<br/>Verify
participant Endpoints
participant Docker
participant Renderer
User->>Showtime: run showtime-links
Showtime->>Endpoints: load endpoint definitions
par Health checks and service snapshot
Showtime->>Endpoints: HTTP health probes
Endpoints-->>Showtime: status codes/errors
Showtime->>Docker: compose services snapshot
Docker-->>Showtime: service list
end
Showtime->>Showtime: filter worker services
Showtime->>Renderer: render JSON / MD / HTML
Renderer-->>User: write artifacts (and optionally open HTML)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pmoves/docs/NEXT_STEPS.md (1)
333-338:⚠️ Potential issue | 🟡 MinorDuplicate content: the entire block from Line 333 onward is a stale copy of earlier sections.
Lines 333–459 repeat the
# PMOVES v5 • NEXT_STEPSheader and most of theImmediate,n8n Flow Operations,Backlog Snapshot,Later, andNext Session Focussections already present above, but with stale timestamps (2025-09-26,2025-10-05) and missing the newer entries. This looks like a leftover from a prior merge. Remove the duplicate block to avoid confusion and keep a single authoritative section.As per coding guidelines, "Refresh both
docs/ROADMAP.mdanddocs/NEXT_STEPS.mdwhen significant features ship, priorities move, or a new sprint starts; adjust their_Last updatedtimestamps".
🤖 Fix all issues with AI agents
In `@pmoves/docs/MAKE_TARGETS.md`:
- Around line 190-191: Update the "bringup-showtime" bullet so it lists the JSON
evidence artifact as well as the HTML/MD outputs: mention that the target emits
pmoves/docs/SHOWTIME_VERIFY_LINKS.html|.md and
pmoves/docs/evidence/showtime_links.json (to match the existing showtime-links
entry). Locate the text containing the existing strings
"SHOWTIME_VERIFY_LINKS.html|.md" and adjust the sentence to explicitly include
"pmoves/docs/evidence/showtime_links.json" so both descriptions are consistent.
In `@pmoves/tools/bootstrap_light_env.py`:
- Around line 30-35: The --requirements argument currently uses action="append"
with default=["tools/requirements-lite.txt"], causing user values to be appended
rather than replace; change the parser.add_argument call for "--requirements" to
use default=None (keep action="append"), and in the entry point (e.g., main() or
where args are processed) add a conditional that sets args.requirements =
["tools/requirements-lite.txt"] if args.requirements is None so defaulting
happens post-parse and user-specified values replace the default.
In `@pmoves/tools/chit_manifest_sync.py`:
- Line 10: The module-level bare import "import yaml" in
pmoves/tools/chit_manifest_sync.py can raise ImportError if PyYAML isn't
installed; either add a pmoves/tools/requirements-lite.txt listing PyYAML or
wrap the import in a try/except like manifest_audit.py does and raise SystemExit
with a clear message instructing the user to install PyYAML (include the package
name and optional install command), so that the import failure is handled
gracefully at startup.
In `@pmoves/tools/runner_lane_map.py`:
- Around line 363-371: The code unconditionally sets strict_fail =
bool(unmapped) which makes unmapped lanes cause exit 2 even when args.strict is
false; change the logic so strict_fail is only driven by unmapped/other checks
when args.strict is true. Specifically, initialize strict_fail = False, then if
args.strict set strict_fail = bool(unmapped) and, if args.enforce_phase, OR in
bool(phase_failures) else OR in bool(unavailable) (using the same symbols:
strict_fail, args.strict, args.enforce_phase, unmapped, phase_failures,
unavailable) so the non-zero exit is only returned when --strict is passed.
In `@pmoves/tools/runtime_secrets_hydrate.py`:
- Around line 160-168: The current or-chain for MEILI_MASTER_KEY (and similarly
FIREFLY_APP_KEY/FIREFLY_ACCESS_TOKEN) lets stub values like "changeme"
short-circuit and prevents secrets.token_urlsafe from being used; update the
logic used by set_if_missing to treat placeholder values as missing by
filtering/normalizing env_values and container-found values before the or-chain
(e.g., trim and compare against known placeholders like "changeme" or empty
string), or introduce a small helper (used by _find_container_env_value result
and env_values.get(...).strip()) that returns None for placeholders so that
secrets.token_urlsafe(24) will be selected when appropriate; adjust usages for
MEILI_MASTER_KEY and FIREFLY_APP_KEY to use this sanitation.
🧹 Nitpick comments (16)
pmoves/tools/secrets_hardening_audit.py (1)
27-36: Consider skipping additional high-volume directories incandidate_files().The function already skips
.git, butrglob("*")will still traversenode_modules,__pycache__,.venv*,dist,build, etc. For a large monorepo this can noticeably slow the audit. Adding a few more skip patterns would help.♻️ Suggested skip set
def candidate_files() -> Iterable[Path]: allowed = {".md", ".py", ".sh", ".yaml", ".yml", ".json", ".txt"} + skip_dirs = {".git", "node_modules", "__pycache__", ".venv", ".venv-pmoves", "dist", "build"} for path in REPO_ROOT.rglob("*"): if not path.is_file(): continue - if ".git" in path.parts: + if skip_dirs & set(path.parts): continue if path.suffix.lower() not in allowed: continue yield pathpmoves/tools/runtime_secrets_hydrate.py (1)
33-54:_write_env_filereads the file even whenupdatesis empty.Minor: if
updatesis empty, the function still reads, re-indexes, and rewrites the file. The caller guards this (if not updates: return 0), so this is not triggered today, but a defensive early return would make the function safer for future callers.pmoves/mk/preflight.mk (1)
115-122:showtime-links*targets use$(PYTHON)whilesubmodule-layer-validate*targets use$(PRECHECK_PY).On Windows,
$(PRECHECK_PY)resolves topy -3while$(PYTHON)may differ. Ifshowtime-linkscan be invoked standalone during preflight (outside bring-up), consider using$(PRECHECK_PY)for consistency, or document that these targets require a full Python environment.pmoves/tools/showtime_watch.py (1)
32-42:probe()opens URLs without scheme validation.
urlopenacceptsfile://and other non-HTTP schemes. SinceENDPOINTSis imported fromflight_check_retroand should only containhttp:///https://URLs, this is low risk. A one-line guard would harden it against accidental misconfiguration upstream.🛡️ Optional scheme guard
def probe(url: str, timeout: float = 2.5) -> tuple[bool, int]: + if not url.startswith(("http://", "https://")): + return False, 0 try: with urlopen(url, timeout=timeout) as resp:pmoves/tools/bootstrap_light_env.py (1)
56-57: Baresubprocess.run(cmd, check=True)gives poor error UX on failure.When a subprocess fails, the raw
CalledProcessErrortraceback is printed. For a user-facing bootstrap tool, consider catching it and printing a friendlier message with the failed command and return code.pmoves/tools/chit_manifest_sync.py (1)
214-219:--checkmode is sensitive to alias ordering, not just content.
yaml.safe_dump(next_manifest, sort_keys=False)preserves insertion order. If the existing v1 manifest was hand-edited with aliases in a different order,--checkwill report "OUT-OF-SYNC" even with no semantic difference. This is acceptable if the tool is the sole writer of the v1 manifest, but worth noting for operators who might hand-edit.pmoves/tools/local_cert_runners.py (2)
86-110: Runner token visible viadocker inspectand Docker socket grants full host Docker access.Two security posture notes:
RUNNER_TOKENpassed via-eis readable indocker inspect. Consider using Docker secrets or a file-mount if the environment requires stronger isolation.- Mounting
/var/run/docker.sockgives the runner container full control over the host Docker daemon. This is standard for self-hosted GHA runners but should be documented as a conscious trust decision.Neither is a blocker — both are typical trade-offs for local-cert runners — but worth calling out for operational awareness.
56-79: Token fallback chain is sound; consider logging which source was used.The
registration_tokenfunction checks lane-specific env → shared env →gh apicall. For operator debugging, it would help to print which token source was selected (without printing the token itself).pmoves/tools/showtime_verify_links.py (1)
23-24: Fragilesys.pathmanipulation for sibling import.Inserting into
sys.pathat import time and importingflight_check_retro.ENDPOINTScouples this script to a specific directory layout and produces a confusing error if the module is missing. Consider a guarded import with a clear error message.♻️ Proposed guarded import
sys.path.insert(0, str(Path(__file__).resolve().parent)) -from flight_check_retro import ENDPOINTS # type: ignore +try: + from flight_check_retro import ENDPOINTS # type: ignore +except ImportError as exc: + raise SystemExit( + f"Cannot import ENDPOINTS from flight_check_retro — " + f"ensure the module exists in {Path(__file__).resolve().parent}: {exc}" + ) from excpmoves/tools/runner_lane_map.py (2)
130-148:load_runnersdoesn't paginate — repos with >100 runners will be silently truncated.Unlike the
cmd_statusfunction inlocal_cert_runners.pywhich uses--paginate, this function requests a single page of 100 runners. Consider usinggh api --paginateor iterating pages to avoid silent truncation.♻️ Proposed fix using gh --paginate
def load_runners(repo: str) -> list[Runner]: - cmd = ["gh", "api", f"repos/{repo}/actions/runners?per_page=100"] - proc = subprocess.run(cmd, capture_output=True, text=True, check=False) + cmd = ["gh", "api", "--paginate", f"repos/{repo}/actions/runners?per_page=100", "--jq", ".runners"] + proc = subprocess.run(cmd, capture_output=True, text=True, check=False) if proc.returncode != 0: msg = proc.stderr.strip() or proc.stdout.strip() or "unknown gh api error" raise RuntimeError(msg) - payload = json.loads(proc.stdout) + items: list[dict] = [] + for line in proc.stdout.strip().splitlines(): + if line.strip(): + items.extend(json.loads(line)) runners: list[Runner] = [] - for item in payload.get("runners", []): + for item in items:Alternatively, keep the current approach if you're confident the runner count stays under 100.
90-100: Regex-based YAML discovery is adequate but limited.
RUNS_ON_LIST_REonly matchesruns-on: [...]on a single line. Multi-line array syntax, matrix expressions, and bare-stringruns-on:are skipped. This is an acceptable trade-off for tooling that only targets self-hosted multi-label lanes, but worth documenting the limitation.pmoves/tools/tooling_script_audit.py (2)
439-463: Redundantstartswithcheck.
lower.startswith("pmoves")on Line 451 already matches strings starting with"pmoves-", making the second condition redundant.♻️ Simplified condition
- if not (lower.startswith("pmoves") or lower.startswith("pmoves-")): + if not lower.startswith("pmoves"):
90-115:parse_gitmodulesis duplicated across 4 tools with different signatures and implementations.Found in
tooling_script_audit.py,submodule_layer_runall.py,submodule_layer_validate.py, andsubmodule_sitrep.py. However, they are not "nearly identical"—they have different return types (name+path tuples, name+path+url tuples, and dictionaries) and implementations (regex parsing vs. configparser). Evaluate whether a shared utility is warranted; if extracted, account for the different return types and data requirements.pmoves/tools/submodule_layer_runall.py (1)
70-76:read_summarysilently returns(1, 0)on any parse failure — consider distinguishing real errors from missing files.If the validator crashes before writing the JSON, this function defaults to 1 error, which is reasonable. However, it might mask the root cause. Consider logging the exception or differentiating "file not found" from "malformed JSON" for operator debugging.
pmoves/tools/submodule_layer_validate.py (2)
221-237:py_compile.compilewrites.pycfiles into submodule__pycache__directories as a side effect.Each compiled file creates (or updates) a
.pycin__pycache__/under the submodule tree. While these are typically gitignored, it pollutes the working tree. You can avoid this by redirecting the bytecode output.♻️ Suppress .pyc side effect
+import os ... def python_compile_check(module_root: Path, max_files: int) -> tuple[str, str]: + # Prevent py_compile from writing .pyc files into submodule trees + old_dont_write = os.environ.get("PYTHONDONTWRITEBYTECODE") + os.environ["PYTHONDONTWRITEBYTECODE"] = "1" py_files: list[Path] = [] ... try: for path in py_files: py_compile.compile(str(path), doraise=True) except py_compile.PyCompileError as exc: + if old_dont_write is None: + os.environ.pop("PYTHONDONTWRITEBYTECODE", None) + else: + os.environ["PYTHONDONTWRITEBYTECODE"] = old_dont_write return "fail", str(exc) + if old_dont_write is None: + os.environ.pop("PYTHONDONTWRITEBYTECODE", None) + else: + os.environ["PYTHONDONTWRITEBYTECODE"] = old_dont_write return "pass", f"compiled-files={len(py_files)}"Or more simply, redirect the
.pycoutput to a temp file:try: for path in py_files: - py_compile.compile(str(path), doraise=True) + py_compile.compile(str(path), cfile=os.devnull, doraise=True) except py_compile.PyCompileError as exc:
165-177:remote_commit_reachableuses substring match onls-remoteoutput.Line 174:
any(commit in line for line in ...)does a substring match. While full 40-char SHA collisions are extremely unlikely, an exact-prefix match would be more precise.♻️ Use startswith for exact prefix match
- matched = any(commit in line for line in proc.stdout.splitlines()) + matched = any(line.startswith(commit) or line.split("\t", 1)[0] == commit for line in proc.stdout.splitlines())
| - Emits clickable verification artifacts (`pmoves/docs/SHOWTIME_VERIFY_LINKS.html|.md`) with pages/UI/API links and helper worker container states. | ||
| - Tuning knobs: `SHOWTIME_INTERVAL`, `SHOWTIME_MAX_SECONDS`, `SHOWTIME_WATCH=0`. |
There was a problem hiding this comment.
bringup-showtime description omits the JSON evidence artifact.
Line 190 mentions HTML and MD artifacts but the tool also produces pmoves/docs/evidence/showtime_links.json. The showtime-links entry at Line 198 correctly lists all three. Consider aligning for consistency.
🤖 Prompt for AI Agents
In `@pmoves/docs/MAKE_TARGETS.md` around lines 190 - 191, Update the
"bringup-showtime" bullet so it lists the JSON evidence artifact as well as the
HTML/MD outputs: mention that the target emits
pmoves/docs/SHOWTIME_VERIFY_LINKS.html|.md and
pmoves/docs/evidence/showtime_links.json (to match the existing showtime-links
entry). Locate the text containing the existing strings
"SHOWTIME_VERIFY_LINKS.html|.md" and adjust the sentence to explicitly include
"pmoves/docs/evidence/showtime_links.json" so both descriptions are consistent.
| parser.add_argument( | ||
| "--requirements", | ||
| action="append", | ||
| default=["tools/requirements-lite.txt"], | ||
| help="Requirements file(s) relative to pmoves/ (repeatable).", | ||
| ) |
There was a problem hiding this comment.
action="append" with a list default will append to it, not replace it.
With default=["tools/requirements-lite.txt"] and action="append", passing --requirements custom.txt yields ["tools/requirements-lite.txt", "custom.txt"] instead of the expected ["custom.txt"]. This is a well-known argparse gotcha.
🐛 Proposed fix: use `default=None` and set the default post-parse
parser.add_argument(
"--requirements",
action="append",
- default=["tools/requirements-lite.txt"],
+ default=None,
help="Requirements file(s) relative to pmoves/ (repeatable).",
)Then in main():
args = parse_args()
+ if args.requirements is None:
+ args.requirements = ["tools/requirements-lite.txt"]
venv_path = resolve_under_pmoves(args.venv)🤖 Prompt for AI Agents
In `@pmoves/tools/bootstrap_light_env.py` around lines 30 - 35, The --requirements
argument currently uses action="append" with
default=["tools/requirements-lite.txt"], causing user values to be appended
rather than replace; change the parser.add_argument call for "--requirements" to
use default=None (keep action="append"), and in the entry point (e.g., main() or
where args are processed) add a conditional that sets args.requirements =
["tools/requirements-lite.txt"] if args.requirements is None so defaulting
happens post-parse and user-specified values replace the default.
| from pathlib import Path | ||
| from typing import Any, Dict, Iterable, List, Mapping, Sequence, Tuple | ||
|
|
||
| import yaml |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all requirements files in pmoves and check for PyYAML
echo "=== Searching for requirements files containing PyYAML ==="
find pmoves -type f -name '*requirements*' -o -name 'pyproject.toml' -o -name 'setup.py' | head -20Repository: POWERFULMOVES/PMOVES.AI
Length of output: 1239
🏁 Script executed:
#!/bin/bash
# Check if PyYAML or yaml is listed in requirements files
echo "=== Checking pmoves/tools/ for requirements files ==="
ls -la pmoves/tools/ | grep -i req
echo ""
echo "=== Checking pmoves/ for requirements files ==="
ls -la pmoves/ | grep -i req
echo ""
echo "=== Full search for requirements files ==="
find pmoves -name '*requirements*' -type fRepository: POWERFULMOVES/PMOVES.AI
Length of output: 3786
🏁 Script executed:
#!/bin/bash
# Search for PyYAML in all files
echo "=== Searching for PyYAML references ==="
rg -i 'pyyaml|PyYAML' pmoves/ --type py
echo ""
echo "=== Checking requirements files content ==="
find pmoves -name '*requirements*' -type f -exec echo "File: {}" \; -exec cat {} \;Repository: POWERFULMOVES/PMOVES.AI
Length of output: 50381
🏁 Script executed:
#!/bin/bash
# More specific search for requirements-lite in pmoves/tools
echo "=== Looking for requirements-lite.txt in pmoves/tools ==="
find pmoves/tools -name '*requirements*' -type f
echo ""
echo "=== Checking if pyproject.toml in pmoves has PyYAML ==="
grep -i 'pyyaml\|yaml' pmoves/pyproject.toml || echo "PyYAML not found in pyproject.toml"Repository: POWERFULMOVES/PMOVES.AI
Length of output: 216
🏁 Script executed:
#!/bin/bash
# Check what files are in pmoves/tools directory
echo "=== Contents of pmoves/tools directory ==="
ls -la pmoves/tools/
echo ""
echo "=== Check chit_manifest_sync.py for any setup comments ==="
head -30 pmoves/tools/chit_manifest_sync.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 7741
🏁 Script executed:
#!/bin/bash
# Check for bootstrap scripts that might install dependencies
echo "=== Looking for bootstrap-lite or lightweight environment setup ==="
find pmoves -name '*bootstrap*' -o -name '*lite*' -type f | grep -v '.pyc' | head -20
echo ""
echo "=== Check if manifest_audit.py shows how PyYAML is handled ==="
grep -A5 -B5 'PyYAML' pmoves/tools/manifest_audit.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 1746
🏁 Script executed:
#!/bin/bash
# Check if there's any shared requirements or dependencies documentation
echo "=== Look for any requirements files at pmoves root or similar patterns ==="
find pmoves -maxdepth 2 -name 'requirements*.txt' -o -name '*.txt' | grep -i req
echo ""
echo "=== Check bootstrap_light_env.py to understand dependency model ==="
head -50 pmoves/tools/bootstrap_light_env.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 1575
🏁 Script executed:
#!/bin/bash
# Check if requirements-lite.txt actually exists in pmoves/tools
echo "=== Does pmoves/tools/requirements-lite.txt exist? ==="
ls -la pmoves/tools/requirements-lite.txt 2>&1
echo ""
echo "=== Full path check ==="
test -f pmoves/tools/requirements-lite.txt && echo "File exists" || echo "File DOES NOT exist"
echo ""
echo "=== Check what bootstrap_light_env.py does when it's missing ==="
grep -A10 'def.*requirements\|def.*install' pmoves/tools/bootstrap_light_env.py | head -40Repository: POWERFULMOVES/PMOVES.AI
Length of output: 1004
🏁 Script executed:
#!/bin/bash
# Let's see the full bootstrap_light_env.py to understand the dependency model
wc -l pmoves/tools/bootstrap_light_env.py
tail -100 pmoves/tools/bootstrap_light_env.pyRepository: POWERFULMOVES/PMOVES.AI
Length of output: 3066
Create pmoves/tools/requirements-lite.txt and include PyYAML, or add error handling for the missing import.
This tool imports yaml (PyYAML) at module scope (line 10) without fallback error handling. The file pmoves/tools/requirements-lite.txt does not exist, and PyYAML is not listed in pmoves/pyproject.toml. Runtime import failure will occur if PyYAML is not pre-installed. Compare with pmoves/tools/manifest_audit.py, which wraps the yaml import in try/except and raises a clear SystemExit message when the dependency is missing—this tool should follow the same pattern or ensure the dependency is declared.
🤖 Prompt for AI Agents
In `@pmoves/tools/chit_manifest_sync.py` at line 10, The module-level bare import
"import yaml" in pmoves/tools/chit_manifest_sync.py can raise ImportError if
PyYAML isn't installed; either add a pmoves/tools/requirements-lite.txt listing
PyYAML or wrap the import in a try/except like manifest_audit.py does and raise
SystemExit with a clear message instructing the user to install PyYAML (include
the package name and optional install command), so that the import failure is
handled gracefully at startup.
| strict_fail = bool(unmapped) | ||
| if args.strict: | ||
| if args.enforce_phase: | ||
| strict_fail = strict_fail or bool(phase_failures) | ||
| else: | ||
| strict_fail = strict_fail or bool(unavailable) | ||
| if strict_fail: | ||
| return 2 | ||
| return 0 |
There was a problem hiding this comment.
Unmapped lanes always cause exit 2, even without --strict.
strict_fail = bool(unmapped) on Line 363 is evaluated unconditionally, so unmapped lanes cause a non-zero exit regardless of --strict. The --strict help text says "Exit non-zero on unmapped lanes or unavailable live lanes", implying both checks should be gated.
🐛 Proposed fix to gate unmapped check behind --strict
- strict_fail = bool(unmapped)
- if args.strict:
+ strict_fail = False
+ if args.strict:
+ strict_fail = bool(unmapped) or bool(unavailable)
if args.enforce_phase:
- strict_fail = strict_fail or bool(phase_failures)
- else:
- strict_fail = strict_fail or bool(unavailable)
+ strict_fail = strict_fail or bool(phase_failures)
if strict_fail:
return 2
return 0🤖 Prompt for AI Agents
In `@pmoves/tools/runner_lane_map.py` around lines 363 - 371, The code
unconditionally sets strict_fail = bool(unmapped) which makes unmapped lanes
cause exit 2 even when args.strict is false; change the logic so strict_fail is
only driven by unmapped/other checks when args.strict is true. Specifically,
initialize strict_fail = False, then if args.strict set strict_fail =
bool(unmapped) and, if args.enforce_phase, OR in bool(phase_failures) else OR in
bool(unavailable) (using the same symbols: strict_fail, args.strict,
args.enforce_phase, unmapped, phase_failures, unavailable) so the non-zero exit
is only returned when --strict is passed.
| # Pull runtime-emitted labels from running containers when available. | ||
| set_if_missing( | ||
| "MEILI_MASTER_KEY", | ||
| _find_container_env_value( | ||
| containers, name_tokens=("meili",), keys=("MEILI_MASTER_KEY", "MEILI_ENV") | ||
| ) | ||
| or env_values.get("MEILI_MASTER_KEY", "").strip() | ||
| or secrets.token_urlsafe(24), | ||
| ) |
There was a problem hiding this comment.
Placeholder values in env file are never replaced by secrets.token_urlsafe() due to short-circuit evaluation.
The or chain at Lines 163–167 includes env_values.get("MEILI_MASTER_KEY", "").strip() before secrets.token_urlsafe(24). If the env file already contains MEILI_MASTER_KEY=changeme, the or chain short-circuits at step 2 with "changeme" (truthy), so the token is never generated. Then set_if_missing receives "changeme", detects it as a placeholder, but re-sets the same placeholder value — effectively a no-op.
The same pattern applies to FIREFLY_APP_KEY (Lines 170–176) if FIREFLY_ACCESS_TOKEN contains a placeholder.
🐛 Proposed fix: filter out placeholder values before the `or` chain
+ def _non_placeholder(val: str) -> str:
+ """Return val only if it is not a placeholder, else empty string."""
+ v = val.strip()
+ return v if v and not _looks_placeholder(v) else ""
+
set_if_missing(
"MEILI_MASTER_KEY",
_find_container_env_value(
containers, name_tokens=("meili",), keys=("MEILI_MASTER_KEY", "MEILI_ENV")
)
- or env_values.get("MEILI_MASTER_KEY", "").strip()
+ or _non_placeholder(env_values.get("MEILI_MASTER_KEY", ""))
or secrets.token_urlsafe(24),
)
set_if_missing(
"FIREFLY_APP_KEY",
_find_container_env_value(
containers, name_tokens=("firefly", "wealth"), keys=("FIREFLY_APP_KEY", "APP_KEY")
)
- or env_values.get("FIREFLY_ACCESS_TOKEN", "").strip()
+ or _non_placeholder(env_values.get("FIREFLY_ACCESS_TOKEN", ""))
or secrets.token_urlsafe(24),
)🤖 Prompt for AI Agents
In `@pmoves/tools/runtime_secrets_hydrate.py` around lines 160 - 168, The current
or-chain for MEILI_MASTER_KEY (and similarly
FIREFLY_APP_KEY/FIREFLY_ACCESS_TOKEN) lets stub values like "changeme"
short-circuit and prevents secrets.token_urlsafe from being used; update the
logic used by set_if_missing to treat placeholder values as missing by
filtering/normalizing env_values and container-found values before the or-chain
(e.g., trim and compare against known placeholders like "changeme" or empty
string), or introduce a small helper (used by _find_container_env_value result
and env_values.get(...).strip()) that returns None for placeholders so that
secrets.token_urlsafe(24) will be selected when appropriate; adjust usages for
MEILI_MASTER_KEY and FIREFLY_APP_KEY to use this sanitation.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pmoves/docs/AGENTS/CODEX_OPERATOR_HOME.md (1)
2-2:⚠️ Potential issue | 🟡 MinorUpdate the “Last updated” date to reflect this change.
The doc now includes a new link, so the date should reflect the current edit window (2026-02-16). As per coding guidelines, keep status claims aligned with evidence in runbooks and smokes.
✏️ Suggested fix
-_Last updated: 2026-02-14_ +_Last updated: 2026-02-16_
🤖 Fix all issues with AI agents
In `@pmoves/docs/AGENTS/CODEX_CIPHER_MEMORY_IMPLEMENTATION_MAP.md`:
- Around line 65-77: Replace the definitive cleanliness claims under the
"Clean/near-clean worktrees" and "Dirty worktrees requiring triage" sections
with guidance to verify state at runtime—e.g., change entries like
`PMOVES.AI-hardened-audit`, `PMOVES.AI`, and `PMOVES.AI-slice-cipher` to a
checklist or operator note that instructs readers to run git status/inspect
merge state (or link to the runbook/smoke tests) rather than asserting they are
clean/dirty; update the section header text and any bullet wording to indicate
these are examples or require verification instead of factual snapshots.
| ### Clean/near-clean worktrees | ||
| - `PMOVES.AI-hardened-audit` (clean) | ||
| - `PMOVES.AI-hardened-ci` (clean) | ||
| - `PMOVES.AI-slice-ci-ghcr` (clean) | ||
| - `PMOVES.AI-submodule-audit` (clean after artifact cleanup) | ||
|
|
||
| ### Dirty worktrees requiring triage | ||
| - `PMOVES.AI` (root branch): large mixed change-set (code, docs, workflows, submodules) | ||
| - `PMOVES.AI-main-audit`: many submodule pointer edits and integration updates pending commit policy | ||
| - `PMOVES.AI-slice-cipher`: includes unresolved merge conflicts (`UU`) in: | ||
| - `.gitignore` | ||
| - `pmoves/docker-compose.yml` | ||
|
|
There was a problem hiding this comment.
Avoid asserting live worktree cleanliness in docs.
These status claims are environment-specific and will drift quickly; they should be phrased as a checklist (“verify clean/dirty via git status”) or moved to a local/operator note instead of a definitive snapshot. As per coding guidelines, keep status claims aligned with evidence in runbooks and smokes.
✏️ Suggested rewrite
-### Clean/near-clean worktrees
-- `PMOVES.AI-hardened-audit` (clean)
-- `PMOVES.AI-hardened-ci` (clean)
-- `PMOVES.AI-slice-ci-ghcr` (clean)
-- `PMOVES.AI-submodule-audit` (clean after artifact cleanup)
-
-### Dirty worktrees requiring triage
-- `PMOVES.AI` (root branch): large mixed change-set (code, docs, workflows, submodules)
-- `PMOVES.AI-main-audit`: many submodule pointer edits and integration updates pending commit policy
-- `PMOVES.AI-slice-cipher`: includes unresolved merge conflicts (`UU`) in:
- - `.gitignore`
- - `pmoves/docker-compose.yml`
+### Worktree hygiene checklist (verify locally)
+- Run `git status --short` in each worktree.
+- Flag any worktree with `UU` or large mixed change-sets for triage.
+- Prefer documenting local findings in your operator notes or runbook evidence.🤖 Prompt for AI Agents
In `@pmoves/docs/AGENTS/CODEX_CIPHER_MEMORY_IMPLEMENTATION_MAP.md` around lines 65
- 77, Replace the definitive cleanliness claims under the "Clean/near-clean
worktrees" and "Dirty worktrees requiring triage" sections with guidance to
verify state at runtime—e.g., change entries like `PMOVES.AI-hardened-audit`,
`PMOVES.AI`, and `PMOVES.AI-slice-cipher` to a checklist or operator note that
instructs readers to run git status/inspect merge state (or link to the
runbook/smoke tests) rather than asserting they are clean/dirty; update the
section header text and any bullet wording to indicate these are examples or
require verification instead of factual snapshots.
3 participants
Summary
Changes
pmoves/tools/submodule_layer_runall.pypmoves/tools/showtime_verify_links.pypmoves/mk/preflight.mk:submodule-layer-validate-allsubmodule-layer-validate-all-strictshowtime-linksshowtime-links-openshowtime-links-strictpmoves/docs/MAKE_TARGETS.mdpmoves/docs/NEXT_STEPS.mdLocal validation
python -m py_compile pmoves/tools/submodule_layer_validate.py pmoves/tools/submodule_layer_runall.py pmoves/tools/showtime_verify_links.pymake -C pmoves submodule-layer-validate-all ARGS="--allow-uninitialized --skip-python-compile"make -C pmoves submodule-layer-validate-all-strict ARGS="--allow-uninitialized --skip-python-compile"(expected strict fail on warnings in current uninitialized audit worktree)make -C pmoves showtime-linksmake -C pmoves showtime-links-strict(expected fail when required endpoint is down)Notes
pmoves/docs/evidence/*) and is not part of this PR.Summary by CodeRabbit
New Features
Documentation