feat(chatmock): add live prompt controls and reasoning compat#3
Conversation
Add a file-backed prompt manager with local admin HTTP endpoints for live prompt inspection, config updates, and reloads. Update Docker wiring for stable prompt roots and an isolated test stack, and accept standard chat-completions reasoning_effort at the proxy boundary with route coverage for both feature sets.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 39 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a PromptManager-based live prompt reload system, runtime prompt configuration and admin HTTP endpoints (inspect, reload, update config), refactors instruction resolution to use the manager, expands environment/docker support for per-service prompt settings and a test compose, and adds tests and docs covering the feature. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as Flask App
participant PM as PromptManager
participant FS as File System
Client->>App: POST /admin/prompts/config<br/>{prompt_dir: "..."}
App->>App: Validate loopback & token
App->>PM: update_config(payload)
PM->>FS: Write prompt-config.json
PM->>PM: Resolve & validate prompt paths
PM->>PM: Clear cache & reload()
PM->>FS: Read prompt.md & codex files
PM-->>App: Return updated state
App-->>Client: 200 {config, cached_files}
Note over Client,FS: Subsequent /v1/chat requests use cached prompts from PM
sequenceDiagram
participant Client
participant App as Flask App
participant PM as PromptManager
participant Cache as Memory Cache
Client->>App: POST /v1/chat/completions
App->>PM: get_base_instructions()
PM->>Cache: Return cached prompt text
Cache-->>PM: prompt.md content
PM-->>App: Instructions string
App->>App: resolve_request_reasoning_param(payload, model)
App->>App: Build chat request with instructions + reasoning_param
App-->>Client: 200 completion response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a live prompt switching and reloading mechanism. It adds a PromptManager to handle prompt configurations and caching, along with a set of local-only admin HTTP endpoints (/admin/prompts, /admin/prompts/reload, /admin/prompts/config) to manage these prompts at runtime. The Docker configuration has been updated to support this feature, and a new isolated test stack has been added. Feedback includes concerns regarding the loopback IP check in Docker environments, missing error handling in the reload endpoint, redundant file I/O in the configuration update logic, and the lack of atomic file writes for configuration persistence.
| remote_addr = request.remote_addr | ||
| if remote_addr not in (None, "127.0.0.1", "::1"): | ||
| return jsonify({"error": {"message": "Admin routes are local-only"}}), 403 |
There was a problem hiding this comment.
The remote_addr check for 127.0.0.1 or ::1 will likely fail when running ChatMock inside a Docker container (using the provided docker-compose.yml) and attempting to access the admin API from the host machine. In bridge networking mode, the source IP seen by the container is typically the bridge gateway IP (e.g., 172.17.0.1), not the loopback address. This makes the admin endpoints inaccessible as documented in DOCKER.md. Consider allowing the Docker gateway IP or providing an option to trust requests when a valid CHATMOCK_ADMIN_TOKEN is provided, regardless of the source IP.
| @app.post("/admin/prompts/reload") | ||
| def admin_prompts_reload(): | ||
| denied = _require_local_admin() | ||
| if denied is not None: | ||
| return denied | ||
| prompt_manager.reload() | ||
| return jsonify(prompt_manager.as_dict()) |
There was a problem hiding this comment.
The admin_prompts_reload endpoint does not catch potential exceptions from prompt_manager.reload(), such as FileNotFoundError or ValueError (e.g., if a prompt file was deleted or became empty since the last config update). This could result in an unhandled 500 error. It should implement error handling similar to admin_prompts_config.
| def update_config(self, config: dict[str, Any]) -> PromptConfigState: | ||
| with self._lock: | ||
| current = self.get_state() | ||
| prompt_dir = config.get("prompt_dir", current.prompt_dir) | ||
| if "prompt_dir" in config and "base_prompt_path" not in config: | ||
| base_prompt_path = None | ||
| else: | ||
| base_prompt_path = config.get("base_prompt_path", current.base_prompt_path) | ||
| if "prompt_dir" in config and "codex_prompt_path" not in config: | ||
| codex_prompt_path = None | ||
| else: | ||
| codex_prompt_path = config.get("codex_prompt_path", current.codex_prompt_path) | ||
| merged = { | ||
| "prompt_dir": prompt_dir, | ||
| "base_prompt_path": base_prompt_path, | ||
| "codex_prompt_path": codex_prompt_path, | ||
| } | ||
| normalized = self._normalize_config(merged) | ||
| self._write_config(normalized) | ||
| return self.reload() |
There was a problem hiding this comment.
The update_config method is inefficient as it triggers multiple redundant file reads. It calls _normalize_config (which reads both prompt files), then _write_config, and finally reload(). reload() itself calls _normalize_config again (reading files again) and then explicitly reads the files one more time to update the memory cache. For a single configuration update, the prompt files are read up to 6 times. Consider refactoring to read and validate the files once and reuse the content.
| def _write_config(self, config: dict[str, str | None]) -> None: | ||
| self._config_path.parent.mkdir(parents=True, exist_ok=True) | ||
| payload = { | ||
| "prompt_dir": config.get("prompt_dir"), | ||
| "base_prompt_path": config["base_prompt_path"], | ||
| "codex_prompt_path": config["codex_prompt_path"], | ||
| } | ||
| self._config_path.write_text(json.dumps(payload, indent=2) + "\n", encoding="utf-8") |
There was a problem hiding this comment.
The _write_config method writes directly to the configuration file. If the process is interrupted or the disk becomes full during this operation, the configuration file could be left in a corrupted state. It is safer to write to a temporary file in the same directory and then use os.replace() to atomically update the target file.
|
@codex review |
|
/gemini review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@chatmock/app.py`:
- Around line 81-100: The admin endpoints admin_prompts_reload and
admin_prompts_config must validate JSON input and convert prompt I/O errors into
structured 400 responses: for admin_prompts_config call
request.get_json(silent=False) inside a try/except to catch JSON parsing errors
(BadRequest) and return jsonify({"error": {"message": "Invalid JSON body"}}),
400; then assert the parsed payload is a dict (reject lists/null/other types
similarly). For both admin_prompts_reload (which may trigger file reads) and
admin_prompts_config wrap prompt_manager.reload() and
prompt_manager.update_config(payload) in a try/except that catches
FileNotFoundError, ValueError, OSError/IOError/PermissionError and returns a
structured 400 JSON error with the exception message (instead of letting them
surface as 500). Ensure you reference request.get_json, prompt_manager.reload,
and prompt_manager.update_config when making these changes.
- Around line 63-66: The _require_local_admin function currently rejects any
non-local remote_addr; update it to accept requests from known Docker
bridge/gateway addresses and an admin token: add a small allowlist (e.g., common
bridge prefixes like "172.17.", "172.18." or specific addresses such as
"172.17.0.1") and check request.remote_addr against that allowlist inside
_require_local_admin, and also allow access when a valid CHATMOCK_ADMIN_TOKEN
(env var) is provided via an authorization header or X-Admin-Token header;
ensure you still return the same JSON 403 when neither condition is met and keep
the function name _require_local_admin unchanged.
In `@chatmock/config.py`:
- Around line 157-167: The reload() method (used by
read_base_instructions()/BASE_INSTRUCTIONS at import time) currently writes
defaults to disk via _write_config when no persisted config exists, causing file
creation during module import; change reload to be read-only: when
_load_persisted_config() returns None, compute seed/normalized (using
_normalize_config or _resolve_legacy_defaults) but do NOT call _write_config;
instead expose a separate explicit persistence path (e.g., a new
persist_defaults or an optional save flag on reload) so callers (like
create_app) can decide when to write; apply the same non-writing behavior to the
similar logic referenced at the other occurrence (lines ~262-263) to avoid
implicit file creation during import.
In `@chatmock/reasoning.py`:
- Around line 53-57: The code is overwriting an existing nested reasoning.effort
with an unvalidated top-level reasoning_effort; change build_reasoning_param so
you only assign merged_overrides["effort"] when payload.get("reasoning_effort")
is valid (e.g., non-empty and matches the allowed effort values/format used
elsewhere) and otherwise leave existing reasoning_overrides["effort"] intact;
locate the block using variables standard_effort, reasoning_overrides and
function build_reasoning_param and add a validation check before setting
merged_overrides["effort"] so invalid strings like "bogus" do not clobber a
valid nested "reasoning": {"effort": "..."} entry.
In `@chatmock/responses_api.py`:
- Line 8: The code currently eagerly creates a global PromptManager via
get_prompt_manager(), making the fallback branches that should use explicit
BASE_INSTRUCTIONS or GPT5_CODEX_INSTRUCTIONS unreachable; update the resolution
logic so functions that accept optional PROMPT_MANAGER and instruction args
(BASE_INSTRUCTIONS, GPT5_CODEX_INSTRUCTIONS) choose in this order: 1) explicit
instruction argument if provided, 2) provided PROMPT_MANAGER if non-null, and
only 3) call get_prompt_manager() to create/use the global PromptManager as a
last resort; modify the initialization so get_prompt_manager() is not called
unconditionally at module import and ensure helper logic in the functions that
reference PROMPT_MANAGER and PromptManager honors this
explicit-instruction-first rule.
In `@DOCKER.md`:
- Around line 28-45: Update the curl examples that call the admin endpoints (GET
/admin/prompts, POST /admin/prompts/reload, POST /admin/prompts/config) to
include the required admin token header when CHATMOCK_ADMIN_TOKEN is set; add
the header -H "X-ChatMock-Admin-Token: $CHATMOCK_ADMIN_TOKEN" (or mention the
required header name) to the POST/GET examples shown and update the surrounding
prose to note these endpoints require the header when CHATMOCK_ADMIN_TOKEN is
configured.
In `@docs/superpowers/plans/2026-04-22-live-prompt-reload.md`:
- Around line 127-148: The plan currently references CHATGPT_LOCAL_PROMPT_* env
names but the implementation and .env.example/docker-compose expose
service-facing CHATMOCK_* variables; update the plan's Step 2 to use the actual
implemented names (e.g., CHATMOCK_LOCAL_PROMPT_DIR,
CHATMOCK_LOCAL_PROMPT_BASE_PATH, CHATMOCK_LOCAL_PROMPT_CODEX_PATH,
CHATMOCK_LOCAL_PROMPT_CONFIG) and instruct maintainers to add those exact vars
to .env.example and DOCKER.md (and note any optional admin token env if
implemented) so docs and compose variables remain consistent.
In `@tests/test_routes.py`:
- Around line 111-210: These admin tests can fail if CHATMOCK_ADMIN_TOKEN is set
in CI; update the tests (e.g., test_admin_prompts_returns_current_prompt_state,
test_admin_prompts_reload_refreshes_cached_prompt_contents,
test_admin_prompts_config_switches_prompt_directory_live) to pass a
deterministic admin_token into create_app (call create_app(...,
admin_token="test-token")) and include the matching X-ChatMock-Admin-Token
header on the admin requests that expect 200 (GET /admin/prompts, POST
/admin/prompts/reload, POST /admin/prompts/config); leave
test_admin_prompts_rejects_non_loopback_access unchanged (no header) so it still
verifies 403. Ensure the header name and token string match between create_app
and the client requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86304a62-e140-4b0f-8a43-ea3b39592450
📒 Files selected for processing (12)
.env.exampleDOCKER.mdchatmock/app.pychatmock/config.pychatmock/reasoning.pychatmock/responses_api.pychatmock/routes_openai.pydocker-compose.test.ymldocker-compose.ymldocs/superpowers/plans/2026-04-22-live-prompt-reload.mddocs/superpowers/specs/2026-04-22-live-prompt-reload-design.mdtests/test_routes.py
There was a problem hiding this comment.
Code Review
This pull request implements a live prompt reload system, enabling runtime switching of prompt directories and content via a new local-only admin API. Key changes include the introduction of a PromptManager for configuration persistence and caching, updated Docker Compose configurations for isolated testing, and new reasoning parameter resolution logic. Review feedback identifies a potential connectivity issue for admin routes within Docker bridge networks, suggests improved error handling for JSON parsing, and recommends atomic file operations for configuration updates.
|
|
||
| def _require_local_admin(): | ||
| remote_addr = request.remote_addr | ||
| if remote_addr not in (None, "127.0.0.1", "::1"): |
There was a problem hiding this comment.
The loopback check using request.remote_addr will block legitimate requests from the host machine when running inside a standard Docker bridge network (where the host appears as the gateway IP, e.g., 172.17.0.1). This makes the admin endpoints unusable as documented in DOCKER.md unless the container is run in host networking mode. Consider allowing an override via an environment variable or relaxing the check if a valid ADMIN_TOKEN is provided.
| if remote_addr not in (None, "127.0.0.1", "::1"): | |
| if remote_addr not in (None, "127.0.0.1", "::1") and os.getenv("CHATMOCK_ALLOW_ADMIN_EXTERNAL", "false").lower() != "true": |
| def _load_persisted_config(self) -> dict[str, Any] | None: | ||
| if not self._config_path.exists(): | ||
| return None | ||
| data = json.loads(self._config_path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
json.loads() will raise a JSONDecodeError if the configuration file is corrupted or contains invalid JSON, causing the application to crash on startup or during a reload. It is safer to catch this exception and raise a more descriptive error or fall back to defaults.
| data = json.loads(self._config_path.read_text(encoding="utf-8")) | |
| try: | |
| data = json.loads(self._config_path.read_text(encoding="utf-8")) | |
| except (json.JSONDecodeError, UnicodeDecodeError) as exc: | |
| raise ValueError(f"Prompt config at {self._config_path} is not valid JSON: {exc}") from exc |
| "base_prompt_path": config["base_prompt_path"], | ||
| "codex_prompt_path": config["codex_prompt_path"], | ||
| } | ||
| self._config_path.write_text(json.dumps(payload, indent=2) + "\n", encoding="utf-8") |
There was a problem hiding this comment.
Writing the configuration file directly is not atomic. If the process crashes or the disk becomes full during the write, the configuration file could be left in a corrupted state. Using a temporary file and Path.replace() ensures an atomic update.
| self._config_path.write_text(json.dumps(payload, indent=2) + "\n", encoding="utf-8") | |
| temp_path = self._config_path.with_suffix(".tmp") | |
| temp_path.write_text(json.dumps(payload, indent=2) + "\n", encoding="utf-8") | |
| temp_path.replace(self._config_path) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 354be5871f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if remote_addr not in (None, "127.0.0.1", "::1"): | ||
| return jsonify({"error": {"message": "Admin routes are local-only"}}), 403 |
There was a problem hiding this comment.
The local-only guard currently treats request.remote_addr is None as authorized, so in deployments where upstream/proxy wiring omits REMOTE_ADDR, non-local callers can still hit /admin/prompts* endpoints. Because these routes mutate runtime prompt config, the guard should only allow explicit loopback addresses and reject missing address metadata.
Useful? React with 👍 / 👎.
| denied = _require_local_admin() | ||
| if denied is not None: | ||
| return denied | ||
| prompt_manager.reload() |
There was a problem hiding this comment.
/admin/prompts/reload calls prompt_manager.reload() without handling FileNotFoundError/ValueError, so a bad on-disk prompt edit causes a 500 instead of a recoverable 4xx JSON error. This breaks operator workflows (including scripted reloads) for an expected validation failure case.
Useful? React with 👍 / 👎.
| payload = request.get_json(silent=True) or {} | ||
| if not isinstance(payload, dict): |
There was a problem hiding this comment.
Using request.get_json(silent=True) or {} converts an empty JSON array ([]) into {}, so /admin/prompts/config accepts an invalid payload shape as a successful no-op instead of returning 400. This weakens request validation and can hide client-side payload bugs.
Useful? React with 👍 / 👎.
Address CodeRabbit, Gemini, and Codex review feedback by tightening admin auth and error handling, making prompt config persistence explicit and atomic, validating top-level reasoning_effort before override, and aligning tests/docs with the implemented prompt env and token behavior.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a live prompt reload feature, allowing the ChatMock service to switch prompt directories and refresh prompt content via a new local-only admin API without requiring a restart. It introduces a PromptManager to handle runtime configuration persistence and caching, updates Docker Compose for flexible prompt mounting, and adds support for isolated test stacks. Feedback was provided regarding the prompt path resolution logic, specifically to ensure a fallback to the base prompt when a specific codex variant is missing from a directory, maintaining consistency with legacy behavior.
|
|
||
| if normalized_dir: | ||
| normalized_base = normalized_base or str(Path(normalized_dir) / "prompt.md") | ||
| normalized_codex = normalized_codex or str(Path(normalized_dir) / "prompt_gpt5_codex.md") |
There was a problem hiding this comment.
The current implementation of normalized_codex derivation when prompt_dir is provided lacks the fallback logic present in the legacy _resolve_static_prompt_config. If prompt_gpt5_codex.md is missing from the directory, it will result in a FileNotFoundError later in the validation process instead of falling back to the base prompt file. This can be fixed by only setting normalized_codex if the file actually exists, allowing the subsequent check at line 146 to handle the fallback to normalized_base.
| normalized_codex = normalized_codex or str(Path(normalized_dir) / "prompt_gpt5_codex.md") | |
| codex_path = Path(normalized_dir) / "prompt_gpt5_codex.md" | |
| normalized_codex = normalized_codex or (str(codex_path) if codex_path.exists() else None) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b0afd30ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(expected_token, str) and expected_token and provided_token == expected_token: | ||
| return None |
There was a problem hiding this comment.
_require_local_admin returns success as soon as the token matches, so requests from non-local IPs can reach /admin/prompts/* whenever they know CHATMOCK_ADMIN_TOKEN. This contradicts the documented “local-only” guarantee and exposes prompt reconfiguration if the service is bound beyond loopback (for example 0.0.0.0 in Docker or host networking). The locality check should run before any token-based allow path.
Useful? React with 👍 / 👎.
| prompt_config_path=prompt_config_path, | ||
| reset=True, | ||
| ) | ||
| prompt_manager.persist_defaults() |
There was a problem hiding this comment.
create_app() now always calls persist_defaults(), which writes a prompt config file on every boot. In read-only or restricted deployments (e.g., CHATGPT_LOCAL_HOME/CODEX_HOME not writable), this raises PermissionError/OSError and prevents the server from starting, even though serving requests only needs in-memory prompts. This introduces a startup hard-failure path that did not exist before.
Useful? React with 👍 / 👎.
Address follow-up Codex and Gemini review feedback by failing closed when REMOTE_ADDR is missing, avoiding startup prompt-config writes, and preserving base-prompt fallback when a prompt directory has no codex variant file. Add regression coverage for the missing-address guard and codex-prompt fallback path.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements live prompt switching and reloading through a new local-only admin API and a PromptManager class. It also introduces an isolated Docker Compose test stack and refines the resolution of reasoning parameters. Feedback suggests using the ipaddress module for more robust local address verification in the admin guard and renaming a variable in PromptManager._normalize_config to avoid redefinition.
| allowed_bridge_prefixes = ("172.17.", "172.18.", "172.19.", "172.20.") | ||
| allowed_bridge_addresses = {"172.17.0.1", "172.18.0.1"} | ||
| is_allowed_bridge = isinstance(remote_addr, str) and ( | ||
| remote_addr in allowed_bridge_addresses | ||
| or remote_addr.startswith(allowed_bridge_prefixes) | ||
| ) |
There was a problem hiding this comment.
The hardcoded Docker bridge IP prefixes and addresses are brittle. Docker networks can use various private IP ranges (e.g., 10.0.0.0/8, 192.168.0.0/16) depending on the host configuration and existing networks. Using the ipaddress module to verify if remote_addr is a private or loopback address would be more robust. Alternatively, ensure that CHATMOCK_ALLOW_ADMIN_EXTERNAL is well-documented for users in non-standard network setups.
| codex_path = Path(normalized_dir) / "prompt_gpt5_codex.md" | ||
| normalized_codex = normalized_codex or (str(codex_path) if codex_path.exists() else None) |
There was a problem hiding this comment.
The variable name codex_path is used here as a candidate path and then redefined later in the same method (line 151) as the final resolved path. This redefinition can be confusing for maintainers. Consider using a more descriptive name for the candidate path to improve readability.
| codex_path = Path(normalized_dir) / "prompt_gpt5_codex.md" | |
| normalized_codex = normalized_codex or (str(codex_path) if codex_path.exists() else None) | |
| codex_candidate = Path(normalized_dir) / "prompt_gpt5_codex.md" | |
| normalized_codex = normalized_codex or (str(codex_candidate) if codex_candidate.exists() else None) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5843866cab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| allowed_bridge_prefixes = ("172.17.", "172.18.", "172.19.", "172.20.") | ||
| allowed_bridge_addresses = {"172.17.0.1", "172.18.0.1"} | ||
| is_allowed_bridge = isinstance(remote_addr, str) and ( | ||
| remote_addr in allowed_bridge_addresses | ||
| or remote_addr.startswith(allowed_bridge_prefixes) |
There was a problem hiding this comment.
The local-only guard currently treats any client in 172.17.*–172.20.* as trusted (remote_addr.startswith(...)), which means sibling containers on those bridge networks can reach /admin/prompts/* when no CHATMOCK_ADMIN_TOKEN is configured (the default path in this patch). That expands access beyond loopback and undermines the advertised local-only admin surface; the check should be limited to explicit host gateway addresses (or require a token regardless of source).
Useful? React with 👍 / 👎.
Address the latest Codex and Gemini review feedback by limiting the Docker-network admin allowlist to explicit host gateway addresses, documenting the external override for non-standard networks, and clarifying the prompt-dir codex fallback variable naming.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces live prompt switching and reloading capabilities through a new local-only admin API, allowing runtime configuration changes without container restarts. Key changes include the addition of a PromptManager for handling prompt caching and persistence, the integration of admin endpoints in the Flask application, and updates to the Docker Compose configuration to support a more flexible prompt mounting strategy and an isolated test stack. The review feedback highlights a security concern regarding timing attacks on the admin token comparison and a performance optimization to avoid redundant disk I/O when validating and reloading prompt files.
| or request.headers.get("Authorization", "").removeprefix("Bearer ").strip() | ||
| ) | ||
| if isinstance(expected_token, str) and expected_token: | ||
| if provided_token != expected_token: |
There was a problem hiding this comment.
The token comparison uses a standard equality operator, which is susceptible to timing attacks. While this is a mock tool, it is best practice to use a constant-time comparison for security tokens.
| if provided_token != expected_token: | |
| if not hmac.compare_digest(provided_token, expected_token): |
| _read_prompt_text(base_path) | ||
| _read_prompt_text(codex_path) |
There was a problem hiding this comment.
The prompt files are read here for validation, and then read again immediately after in the reload method. This results in redundant disk I/O. Consider having _normalize_config return the file contents if they are already read, or deferring the validation to the actual loading step.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe0e209a80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not (is_local or is_allowed_bridge or allow_admin_external): | ||
| return jsonify({"error": {"message": "Admin routes are local-only"}}), 403 |
There was a problem hiding this comment.
When CHATMOCK_ALLOW_ADMIN_EXTERNAL=true, _require_local_admin skips the source-IP restriction for every remote client, but token validation is still optional (ADMIN_TOKEN can be unset). In deployments that bind to a non-loopback host (for example 0.0.0.0) and enable this flag to support nonstandard Docker gateway IPs, unauthenticated callers can hit /admin/prompts/* and change live prompt configuration. Require a non-empty admin token (or a stricter allowlist) whenever external admin access is enabled.
Useful? React with 👍 / 👎.
| volumes: | ||
| chatmock_live_auth: | ||
| external: true | ||
| name: chatmock_chatmock_data |
There was a problem hiding this comment.
The isolated test override hardcodes chatmock_chatmock_data as an external volume name, so the documented docker compose ... -p chatmock-test ... workflow fails whenever the live stack was started with a different project name (-p or COMPOSE_PROJECT_NAME), because that external volume will not exist. Parameterizing this name avoids breaking the test stack in common Compose setups.
Useful? React with 👍 / 👎.
Address the latest Codex and Gemini review feedback by requiring a token for external admin access, using constant-time token comparison, removing redundant prompt validation reads, and making the isolated test stack's live-auth volume configurable.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5f8f97744
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._write_config(normalized) | ||
| return self.reload() |
There was a problem hiding this comment.
update_config writes the new config to disk before proving the new prompt files are loadable. Because _normalize_config only checks file existence, an update to unreadable or empty prompt files raises later in reload(), so the API returns 400 but leaves an invalid persisted config behind; the process continues with old in-memory prompts until restart, then startup can fail when reloading that bad config. Reload/validate first, then persist only on success to keep disk state consistent with active state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request implements a live prompt reload system, allowing users to switch prompt directories and refresh content via a new admin API without restarting the service. It introduces a PromptManager for state persistence and caching, updates the Docker Compose configuration for better isolation, and adds comprehensive documentation and tests. Feedback highlights the fragility of hardcoded Docker bridge IP addresses in the admin security layer and identifies a potential race condition in the configuration persistence logic, suggesting the use of unique temporary files.
| remote_addr = request.remote_addr | ||
| allow_admin_external = os.getenv("CHATMOCK_ALLOW_ADMIN_EXTERNAL", "false").lower() == "true" | ||
| allowed_local_addresses = {"127.0.0.1", "::1"} | ||
| allowed_bridge_addresses = {"172.17.0.1", "172.18.0.1"} |
There was a problem hiding this comment.
Hardcoding specific Docker bridge gateway addresses like 172.17.0.1 and 172.18.0.1 is fragile, as Docker network ranges can vary significantly depending on the host configuration and the number of existing networks. While the CHATMOCK_ALLOW_ADMIN_EXTERNAL escape hatch exists, consider allowing a configurable list of CIDR ranges or trusted gateway addresses via an environment variable to make this more robust for diverse container environments.
| "base_prompt_path": config["base_prompt_path"], | ||
| "codex_prompt_path": config["codex_prompt_path"], | ||
| } | ||
| temp_path = self._config_path.with_suffix(".tmp") |
There was a problem hiding this comment.
Using a fixed .tmp suffix for the configuration file write can lead to race conditions if multiple threads or processes (e.g., multiple Gunicorn workers) attempt to update the configuration simultaneously. Although the PromptManager uses an internal RLock, this only protects against concurrent access within a single instance. For better safety, use tempfile.NamedTemporaryFile to ensure a unique temporary filename for each write operation.
| temp_path = self._config_path.with_suffix(".tmp") | |
| import tempfile | |
| with tempfile.NamedTemporaryFile('w', dir=self._config_path.parent, delete=False, encoding='utf-8') as tf: | |
| tf.write(json.dumps(payload, indent=2) + "\n") | |
| temp_path = Path(tf.name) | |
| temp_path.replace(self._config_path) |
Address the latest Gemini and Codex review feedback by adding configurable trusted admin IP/CIDR support, requiring a token when external admin access is enabled, and validating prompt config updates before persisting them.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a live prompt switching and reloading mechanism for the ChatMock service. Key changes include the addition of a PromptManager to handle prompt caching and runtime configuration persistence, and new admin HTTP endpoints for inspecting, reloading, and updating prompt settings. The update also includes an isolated Docker Compose test stack and enhanced reasoning parameter resolution. Feedback suggests optimizing the admin access check by pre-parsing trusted networks outside the request handler and improving the efficiency of JSON configuration writes.
| def _require_local_admin(): | ||
| remote_addr = request.remote_addr | ||
| allow_admin_external = os.getenv("CHATMOCK_ALLOW_ADMIN_EXTERNAL", "false").lower() == "true" | ||
| allowed_local_addresses = {"127.0.0.1", "::1"} | ||
| allowed_bridge_addresses = {"172.17.0.1", "172.18.0.1"} | ||
| trusted_remote_spec = os.getenv("CHATMOCK_ADMIN_TRUSTED_IPS", "").strip() | ||
|
|
||
| def _is_in_trusted_ranges(value: str | None) -> bool: | ||
| if not isinstance(value, str) or not value or not trusted_remote_spec: | ||
| return False | ||
| try: | ||
| remote_ip = ipaddress.ip_address(value) | ||
| except ValueError: | ||
| return False | ||
| for token in (item.strip() for item in trusted_remote_spec.split(",")): | ||
| if not token: | ||
| continue | ||
| try: | ||
| if "/" in token: | ||
| if remote_ip in ipaddress.ip_network(token, strict=False): | ||
| return True | ||
| elif remote_ip == ipaddress.ip_address(token): | ||
| return True | ||
| except ValueError: | ||
| continue | ||
| return False |
There was a problem hiding this comment.
The _require_local_admin function parses environment variables and IP ranges on every request. This is inefficient, especially for the CHATMOCK_ADMIN_TRUSTED_IPS list which involves creating ip_network objects repeatedly. These configurations should be read and parsed once during application initialization (within the create_app scope but outside the request handler).
allow_admin_external = os.getenv("CHATMOCK_ALLOW_ADMIN_EXTERNAL", "false").lower() == "true"
trusted_networks = []
for token in (t.strip() for t in os.getenv("CHATMOCK_ADMIN_TRUSTED_IPS", "").split(",") if t.strip()):
try:
trusted_networks.append(ipaddress.ip_network(token, strict=False))
except ValueError:
continue
def _require_local_admin():
remote_addr = request.remote_addr
allowed_local_addresses = {"127.0.0.1", "::1"}
allowed_bridge_addresses = {"172.17.0.1", "172.18.0.1"}
def _is_in_trusted_ranges(value: str | None) -> bool:
if not isinstance(value, str) or not value or not trusted_networks:
return False
try:
remote_ip = ipaddress.ip_address(value)
return any(remote_ip in net for net in trusted_networks)
except ValueError:
return False| with tempfile.NamedTemporaryFile( | ||
| "w", | ||
| dir=self._config_path.parent, | ||
| delete=False, | ||
| encoding="utf-8", | ||
| ) as temp_file: | ||
| temp_file.write(json.dumps(payload, indent=2) + "\n") | ||
| temp_path = Path(temp_file.name) |
There was a problem hiding this comment.
In _write_config, using json.dump directly to the file object is more efficient than json.dumps followed by write. Additionally, setting sort_keys=True ensures a consistent file content which is better for version control and debugging.
| with tempfile.NamedTemporaryFile( | |
| "w", | |
| dir=self._config_path.parent, | |
| delete=False, | |
| encoding="utf-8", | |
| ) as temp_file: | |
| temp_file.write(json.dumps(payload, indent=2) + "\n") | |
| temp_path = Path(temp_file.name) | |
| with tempfile.NamedTemporaryFile( | |
| "w", | |
| dir=self._config_path.parent, | |
| delete=False, | |
| encoding="utf-8", | |
| ) as temp_file: | |
| json.dump(payload, temp_file, indent=2, sort_keys=True) | |
| temp_file.write("\n") | |
| temp_path = Path(temp_file.name) | |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/test_routes.py (1)
232-245:⚠️ Potential issue | 🟡 MinorPin admin token env for the trusted-IP success test.
Line 243 expects
200, butcreate_app()still inheritsCHATMOCK_ADMIN_TOKEN/CHATGPT_LOCAL_ADMIN_TOKEN; if either is set in CI, this request returns403without the header.🧪 Proposed fix
- `@patch.dict`("os.environ", {"CHATMOCK_ADMIN_TRUSTED_IPS": "10.0.0.0/8"}) + `@patch.dict`( + "os.environ", + { + "CHATMOCK_ADMIN_TRUSTED_IPS": "10.0.0.0/8", + "CHATMOCK_ADMIN_TOKEN": "", + "CHATGPT_LOCAL_ADMIN_TOKEN": "", + }, + ) def test_admin_prompts_allows_configured_trusted_ip_range(self) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_routes.py` around lines 232 - 245, The test test_admin_prompts_allows_configured_trusted_ip_range fails in CI when leftover CHATMOCK_ADMIN_TOKEN/CHATGPT_LOCAL_ADMIN_TOKEN env vars require an Authorization header; update the test to pin those env vars so auth is not required (e.g., extend the `@patch.dict` call to include CHATMOCK_ADMIN_TOKEN and CHATGPT_LOCAL_ADMIN_TOKEN as empty or a known value), then call create_app as before and assert the 200 response; this change should be made in the test_admin_prompts_allows_configured_trusted_ip_range test to ensure create_app() does not inherit external admin tokens.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@chatmock/app.py`:
- Around line 133-138: The handler currently only catches BadRequest from
request.get_json(silent=False) so UnsupportedMediaType escapes and yields a 415;
update the try/except in chatmock/app.py to also catch
werkzeug.exceptions.UnsupportedMediaType (alongside BadRequest) and return the
same jsonify({"error": {"message": "Invalid JSON body"}}), 400 response; ensure
UnsupportedMediaType is imported and handled in the same except block or as a
separate except that returns the identical 400 JSON so request.get_json and
payload validation (payload variable) produce consistent 400 responses for
invalid request bodies.
In `@chatmock/config.py`:
- Around line 28-54: Update environment-variable handling so the new CHATMOCK_*
names are honored before falling back to legacy names: in
default_prompt_config_path(), check CHATMOCK_PROMPT_CONFIG (trimmed) first, then
CHATGPT_LOCAL_PROMPT_CONFIG; in _resolve_static_prompt_config(), read
CHATMOCK_PROMPT_DIR and CHATMOCK_PROMPT_CONFIG (and CHATMOCK_PROMPT_CODEX_PATH
if applicable) and fallback to CHATGPT_LOCAL_PROMPT_DIR /
CHATGPT_LOCAL_PROMPT_BASE_PATH / CHATGPT_LOCAL_PROMPT_CODEX_PATH when the
CHATMOCK_* vars are unset; keep the same return types and semantics of
default_prompt_config_path and _resolve_static_prompt_config.
In `@tests/test_routes.py`:
- Around line 115-121: Replace hardcoded "test-token" literals in the tests with
a shared fixture or constant and reference that instead: define a neutral-named
variable/fixture (e.g., admin_token or ADMIN_TOKEN) and use it when calling
create_app(admin_token=...) and when building headers ("X-ChatMock-Admin-Token":
admin_token) so all occurrences in tests/test_routes.py (the create_app calls
and headers usage surrounding create_app, test_client and header creation) use
the single shared value rather than inline string literals to avoid Ruff S106.
---
Duplicate comments:
In `@tests/test_routes.py`:
- Around line 232-245: The test
test_admin_prompts_allows_configured_trusted_ip_range fails in CI when leftover
CHATMOCK_ADMIN_TOKEN/CHATGPT_LOCAL_ADMIN_TOKEN env vars require an Authorization
header; update the test to pin those env vars so auth is not required (e.g.,
extend the `@patch.dict` call to include CHATMOCK_ADMIN_TOKEN and
CHATGPT_LOCAL_ADMIN_TOKEN as empty or a known value), then call create_app as
before and assert the 200 response; this change should be made in the
test_admin_prompts_allows_configured_trusted_ip_range test to ensure
create_app() does not inherit external admin tokens.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53e3ea33-8012-49f4-b3bf-97b82b4f1895
📒 Files selected for processing (9)
.env.exampleDOCKER.mdchatmock/app.pychatmock/config.pychatmock/reasoning.pychatmock/responses_api.pydocker-compose.test.ymldocs/superpowers/plans/2026-04-22-live-prompt-reload.mdtests/test_routes.py
✅ Files skipped from review due to trivial changes (1)
- docker-compose.test.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- chatmock/reasoning.py
- chatmock/responses_api.py
- docs/superpowers/plans/2026-04-22-live-prompt-reload.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a4ea84f6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(expected_token, str) and expected_token: | ||
| if not isinstance(provided_token, str) or not hmac.compare_digest(provided_token, expected_token): | ||
| return jsonify({"error": {"message": "Invalid admin token"}}), 403 |
There was a problem hiding this comment.
The new admin routes are writable (POST /admin/prompts/config and /admin/prompts/reload), but this check only enforces a token when ADMIN_TOKEN is configured, so loopback callers are unauthenticated by default. Because CORS currently reflects arbitrary origins in chatmock/http.py, a user merely visiting a malicious website can issue cross-origin requests to http://127.0.0.1:8000/admin/prompts/* and mutate runtime prompt config. This introduces a local CSRF-style privilege bypass for production defaults unless a token is always required (or these routes are exempted from permissive CORS).
Useful? React with 👍 / 👎.
Address the latest CodeRabbit follow-up by honoring CHATMOCK_* prompt env vars before legacy names, treating UnsupportedMediaType as the same invalid JSON path, and stabilizing admin tests with a shared token constant plus pinned token env in the trusted-IP case.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a live prompt reloading mechanism and an isolated test stack. A new PromptManager handles cached prompt instructions and runtime configuration persistence, while chatmock/app.py now includes admin endpoints for managing these prompts with IP-based and token-based security. The Docker setup has been updated to mount a unified prompts directory, and new documentation and tests have been added to support these features. Feedback suggests tightening the Authorization header extraction to strictly enforce the "Bearer " prefix.
| provided_token = ( | ||
| request.headers.get("X-ChatMock-Admin-Token") | ||
| or request.headers.get("X-Admin-Token") | ||
| or request.headers.get("Authorization", "").removeprefix("Bearer ").strip() |
There was a problem hiding this comment.
The Authorization header extraction is quite permissive. While removeprefix("Bearer ") handles standard Bearer tokens, it also allows tokens to be passed without the prefix if they don't start with "Bearer ". Consider strictly enforcing the "Bearer " prefix if that is the intended authentication scheme for the Authorization header.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0edbc00fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _read_prompt_text(path: Path) -> str: | ||
| content = path.read_text(encoding="utf-8") | ||
| if not isinstance(content, str) or not content.strip(): | ||
| raise ValueError(f"Prompt file {path} is empty") |
There was a problem hiding this comment.
_read_prompt_text now raises on blank files, and both startup (read_gpt5_codex_instructions) and runtime reloads (PromptManager._load_prompt_contents) call it for prompt_gpt5_codex.md. This turns an empty codex variant into a hard failure (startup exception or /admin/prompts/reload/config 400) instead of inheriting base instructions, which breaks setups that intentionally keep the codex file present but blank to use prompt.md defaults.
Useful? React with 👍 / 👎.
1 participant
Summary
reasoning_efforton/v1/chat/completionsand normalize it into ChatMock's internal Responses-style reasoning payloadVerification
./.venv/bin/python -m pytest tests/test_routes.py -k "admin_prompts or prompt_reload" -v./.venv/bin/python -m pytest tests/test_routes.py -k "reasoning_effort or admin_prompts" -v./.venv/bin/python -m pytest tests/test_routes.py -vdocker compose -f docker-compose.yml -f docker-compose.test.yml -p chatmock-test configdocker compose -f docker-compose.yml -f docker-compose.test.yml -p chatmock-test up -d chatmock chatmock-clawmemcurl -fsS http://127.0.0.1:18000/healthcurl -fsS http://127.0.0.1:18001/healthdocker exec chatmock-test sh -lc 'echo CODEX_HOME=$CODEX_HOME; ls -l /live-auth/auth.json /data 2>/dev/null || true'docker compose -f docker-compose.yml -f docker-compose.test.yml -p chatmock-test down -vNotes
CHATMOCK_ADMIN_TOKEN/live-authSummary by CodeRabbit
New Features
Documentation
Tests
Chores