Use hosted MCP by default and harden deploy runtime paths#36
Conversation
|
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 48 minutes and 2 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. 📝 WalkthroughWalkthroughThis PR switches the default FPF MCP surface from a local stdio server to a hosted remote MCP endpoint, adds deployment staging, introduces runtime path-resolution utilities, updates runtime path handling and status behavior, and revises docs/configs to make local full-surface runtime an optional expert mode. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![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 transitions the default Codex integration from a local stdio-based MCP server to a hosted public endpoint, while maintaining a local "full-surface" runtime for expert development via the FPF_MCP_SURFACE=full environment variable. Key changes include the addition of a path resolution utility, deployment scripts for staging runtime artifacts, and comprehensive documentation updates. Feedback is provided regarding an inconsistency in the path resolution logic for absolute paths, which could affect fallback behavior.
| const absolutePath = resolve(trimmedPath); | ||
| return { | ||
| path: absolutePath, | ||
| root: kind === 'directory' ? absolutePath : dirname(absolutePath), |
There was a problem hiding this comment.
There is an inconsistency in how root is calculated for absolute paths versus relative paths when the target is a directory. For relative paths discovered in the loop, root is the base directory where the relative path was found. For absolute paths, if kind is 'directory', root is currently set to the path itself. This inconsistency can lead to unexpected behavior if the resulting root is used as a fallbackRoot for subsequent resolutions. Setting it to the parent directory ensures consistency with the relative resolution logic.
| root: kind === 'directory' ? absolutePath : dirname(absolutePath), | |
| root: dirname(absolutePath), |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR switches the default Codex/MCP configuration to a hosted public MCP endpoint and introduces more robust runtime asset path discovery + deploy staging so hosted Mastra deployments can locate FPF-spec.md and runtime artifacts reliably.
Changes:
- Add
resolveRuntimePath()and updateFpfRuntimeto discover source/artifact roots across cwd/module ancestors. - Add deploy staging script and wiring (
predeploy/deploy) to stageFPF-spec.mdandsnapshot.jsonfor hosted deploys. - Update MCP tool descriptions and docs/configs to describe hosted public MCP as the default, with a local full-surface expert runtime as optional.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/runtime-path-resolution.test.ts | Adds unit coverage for bundled/module-root fallback path resolution. |
| src/runtime/runtime.ts | Uses new runtime path resolver for source/artifact discovery; status now attempts refresh when snapshot is missing. |
| src/runtime/path-resolution.ts | Implements runtime path discovery across ancestor roots of cwd and module location. |
| src/mcp/tools.ts | Updates tool descriptions/labels to reflect “compiler-backed” runtime and “full-surface” expert tooling. |
| server.json | Reframes local server config as optional expert runtime; sets FPF_MCP_SURFACE=full. |
| scripts/prepare-deploy.sh | Adds staging script to copy runtime-read assets into src/mastra/public. |
| package.json | Adds predeploy/deploy scripts and updates package description. |
| manifest.json | Updates description and sets FPF_MCP_SURFACE=full in bun runtime command. |
| docs/mcp-interface.md | Documents hosted default endpoint and local full-surface option. |
| docs/drr/DRR-0001-mcp-first-class-interface.md | Updates decision record to note hosted URL as default transport. |
| README.md | Updates setup instructions for hosted default and local full-surface expert runtime. |
| AGENTS.md | Updates expert-tool instructions to require FPF_MCP_SURFACE=full. |
| .mcp.json | Adds project-scoped hosted MCP configuration. |
| .gitignore | Ignores generated deploy-staged src/mastra/public directory. |
| .codex/config.toml | Switches Codex config to hosted MCP URL by default. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 1. Ensure the snapshot exists locally | ||
| if [ ! -f "$INDEX/snapshot.json" ]; then | ||
| echo "Snapshot not found — building index..." | ||
| bun "$ROOT/src/cli.ts" refresh-index |
There was a problem hiding this comment.
prepare-deploy.sh attempts to build the snapshot with bun ... refresh-index, but the CLI command implemented in src/cli.ts is refresh (not refresh-index). As written, this will fall into the default help path and won’t create .runtime/fpf-index/snapshot.json, causing deploy staging to fail when the snapshot is missing. Update the command to invoke the existing refresh CLI command (optionally with --force if you want to guarantee regeneration).
| bun "$ROOT/src/cli.ts" refresh-index | |
| bun "$ROOT/src/cli.ts" refresh |
![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: f7bbf7a0b6
ℹ️ 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".
| # 1. Ensure the snapshot exists locally | ||
| if [ ! -f "$INDEX/snapshot.json" ]; then | ||
| echo "Snapshot not found — building index..." | ||
| bun "$ROOT/src/cli.ts" refresh-index |
There was a problem hiding this comment.
When snapshot.json is missing (fresh checkout or clean CI worker), this script runs bun "$ROOT/src/cli.ts" refresh-index, but the CLI only implements refresh/status/query/... and returns a non-zero exit for unknown commands. Because set -e is enabled, predeploy aborts here and bun run deploy cannot stage required runtime artifacts unless the snapshot was built manually beforehand.
Useful? React with 👍 / 👎.
|
@claude[agent] review |
|
@codex[agent] 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: 5
🧹 Nitpick comments (1)
src/runtime/runtime.ts (1)
201-203: Preserve refresh failure visibility instatus().Lines 201-203 swallow all refresh errors, which makes troubleshooting missing/stale snapshots harder.
🔧 Proposed refactor
if (!existingSnapshot) { - await this.refresh(false).catch(() => undefined); + await this.refresh(false).catch((error) => { + const message = error instanceof Error ? error.message : String(error); + console.warn(`[FpfRuntime.status] refresh(false) failed: ${message}`); + }); existingSnapshot = await this.loadSnapshot(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/runtime.ts` around lines 201 - 203, In status(), the current await this.refresh(false).catch(() => undefined) silences all refresh errors; remove the catch so refresh errors surface (or catch and rethrow with context) so callers can see failures; specifically update the status() block that sets existingSnapshot (the code that calls this.refresh(false) before calling this.loadSnapshot()) to either await this.refresh(false) directly or catch(err) { throw new Error(`refresh failed in status(): ${err?.message || err}`) } and then proceed to await this.loadSnapshot() so refresh failures are not swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/drr/DRR-0001-mcp-first-class-interface.md`:
- Around line 33-35: Update the doc to narrow the "default Codex transport"
statement to explicitly say the hosted public MCP URL exposes only the public
3-tool surface (not full-surface capabilities), and modify the later rationale
language that references "exact docs, retrieval traces, and refresh" to note
those require the optional full-surface entry point (e.g., enabling
FPF_MCP_SURFACE=full and running src/mastra/stdio.ts). Ensure mentions of
"default" and "Codex registration" refer only to the 3-tool public surface and
add a brief note linking the full-surface capability to the FPF_MCP_SURFACE env
var and stdio.ts entrypoint.
In `@manifest.json`:
- Line 22: The manifest currently forces expert surface by embedding the env var
in the runtime command string ("bun": "FPF_MCP_SURFACE=full bun
src/mastra/stdio.ts"); remove the hard-coded FPF_MCP_SURFACE=full from that
"bun" command (or make it conditional/controlled by an external env variable) so
the manifest no longer forces expert mode for every local start; update the
"bun" entry to run plain "bun src/mastra/stdio.ts" or to reference the existing
environment variable without forcing its value.
In `@README.md`:
- Around line 131-140: Update the Codex Setup notes to explain that if
`.codex/config.toml` points to the hosted MCP URL (the `mcp_servers.fpf_memory`
entry), starting a local full-surface server with `FPF_MCP_SURFACE=full bun run
mcp` will not expose expert tools because Codex still connects to the hosted
endpoint; instruct readers to either temporarily change the
`mcp_servers.fpf_memory` URL in `.codex/config.toml` to `http://localhost:1234`
(or the local port) when running the local MCP, or alternatively start the
direct stdio launcher with `FPF_MCP_SURFACE=full bun src/mastra/stdio.ts` so
expert-tool sessions are available without editing `.codex/config.toml`.
In `@scripts/prepare-deploy.sh`:
- Around line 19-22: The script currently only rebuilds the index if
"$INDEX/snapshot.json" is missing, allowing stale snapshots to be deployed;
change the logic in scripts/prepare-deploy.sh to ensure the index is refreshed
when the spec changed by comparing timestamps or always running the refresh:
either run bun "$ROOT/src/cli.ts" refresh-index unconditionally before staging,
or perform a freshness check like if [ ! -f "$INDEX/snapshot.json" ] || [
"$ROOT/docs/FPF-spec.md" -nt "$INDEX/snapshot.json" ]; then bun
"$ROOT/src/cli.ts" refresh-index; fi so that "$INDEX/snapshot.json" is rebuilt
when FPF-spec.md is newer.
In `@src/runtime/path-resolution.ts`:
- Around line 20-26: In resolveRuntimePath, reject empty/blank rawPath values
immediately after trimming to avoid resolve(root, '') collapsing to the project
root; check trimmedPath === '' (after const trimmedPath = rawPath.trim()) and
throw a descriptive Error (including the options.kind or the original rawPath)
so callers fail fast instead of silently returning the root path.
---
Nitpick comments:
In `@src/runtime/runtime.ts`:
- Around line 201-203: In status(), the current await
this.refresh(false).catch(() => undefined) silences all refresh errors; remove
the catch so refresh errors surface (or catch and rethrow with context) so
callers can see failures; specifically update the status() block that sets
existingSnapshot (the code that calls this.refresh(false) before calling
this.loadSnapshot()) to either await this.refresh(false) directly or catch(err)
{ throw new Error(`refresh failed in status(): ${err?.message || err}`) } and
then proceed to await this.loadSnapshot() so refresh failures are not swallowed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 104e16aa-f8fe-4f82-b150-6d66ce1d971e
📒 Files selected for processing (15)
.codex/config.toml.gitignore.mcp.jsonAGENTS.mdREADME.mddocs/drr/DRR-0001-mcp-first-class-interface.mddocs/mcp-interface.mdmanifest.jsonpackage.jsonscripts/prepare-deploy.shserver.jsonsrc/mcp/tools.tssrc/runtime/path-resolution.tssrc/runtime/runtime.tstests/runtime-path-resolution.test.ts
| 3. The current default Codex transport is the hosted public MCP URL. | ||
| 4. The repo also keeps a local stdio entry point for optional full-surface expert/dev work: | ||
| `FPF_MCP_SURFACE=full bun src/mastra/stdio.ts` |
There was a problem hiding this comment.
Narrow the stated default contract to the public 3-tool surface.
After making hosted/public the default here, the later rationale still says Codex can ask for exact docs, retrieval traces, and refresh. Those capabilities now require the optional full-surface path, so the DRR overstates what the default Codex registration actually exposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/drr/DRR-0001-mcp-first-class-interface.md` around lines 33 - 35, Update
the doc to narrow the "default Codex transport" statement to explicitly say the
hosted public MCP URL exposes only the public 3-tool surface (not full-surface
capabilities), and modify the later rationale language that references "exact
docs, retrieval traces, and refresh" to note those require the optional
full-surface entry point (e.g., enabling FPF_MCP_SURFACE=full and running
src/mastra/stdio.ts). Ensure mentions of "default" and "Codex registration"
refer only to the 3-tool public surface and add a brief note linking the
full-surface capability to the FPF_MCP_SURFACE env var and stdio.ts entrypoint.
| "transport": ["stdio", "http"], | ||
| "runtime": { | ||
| "bun": "bun src/mastra/stdio.ts" | ||
| "bun": "FPF_MCP_SURFACE=full bun src/mastra/stdio.ts" |
There was a problem hiding this comment.
Don’t force expert surface in the manifest runtime command.
Line 22 hard-enables full surface for every manifest-driven local start, which removes opt-in gating for expert tools.
🔧 Proposed fix
- "bun": "FPF_MCP_SURFACE=full bun src/mastra/stdio.ts"
+ "bun": "bun src/mastra/stdio.ts"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "bun": "FPF_MCP_SURFACE=full bun src/mastra/stdio.ts" | |
| "bun": "bun src/mastra/stdio.ts" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@manifest.json` at line 22, The manifest currently forces expert surface by
embedding the env var in the runtime command string ("bun":
"FPF_MCP_SURFACE=full bun src/mastra/stdio.ts"); remove the hard-coded
FPF_MCP_SURFACE=full from that "bun" command (or make it conditional/controlled
by an external env variable) so the manifest no longer forces expert mode for
every local start; update the "bun" entry to run plain "bun src/mastra/stdio.ts"
or to reference the existing environment variable without forcing its value.
| The DRR records the MCP-first boundary choice; the current Codex default is the hosted public MCP. | ||
|
|
||
| Equivalent `~/.codex/config.toml` entry: | ||
|
|
||
| ```toml | ||
| [mcp_servers.fpf_memory] | ||
| command = "bun" | ||
| args = ["src/mastra/stdio.ts"] | ||
| cwd = "/absolute/path/to/fpf-memory" | ||
| required = false | ||
| startup_timeout_sec = 15 | ||
| tool_timeout_sec = 60 | ||
| url = "https://fpf-memory-remote-20260414.server.mastra.cloud/api/mcp/fpf_memory/mcp" | ||
| ``` | ||
|
|
||
| This repo now also ships the same project-scoped configuration at `.codex/config.toml`. Once the project is trusted, Codex can load the `fpf_memory` server directly from the repo without copying the snippet into your user config. | ||
|
|
||
| Local development can keep using the Bun shortcut: | ||
|
|
||
| ```bash | ||
| bun run mcp | ||
| ``` | ||
| This repo ships the same project-scoped configuration at `.codex/config.toml` and `.mcp.json`. Once the project is trusted, Codex can load the hosted `fpf_memory` server directly from the repo. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for file in .codex/config.toml .mcp.json; do
if [ -f "$file" ]; then
printf '\n== %s ==\n' "$file"
sed -n '1,160p' "$file"
fi
doneRepository: venikman/fpf-memory
Length of output: 448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, find README.md
README=$(find . -name "README.md" -type f | head -1)
if [ -z "$README" ]; then
echo "README.md not found"
exit 1
fi
echo "=== File: $README ==="
echo "Line count: $(wc -l < "$README")"
echo -e "\n=== Lines 125-165 (context around 131-140 and 148-158) ==="
sed -n '125,165p' "$README" | cat -n -v
# Also search for mentions of local MCP configuration
echo -e "\n=== Search for 'local' or 'stdio' in README ==="
grep -n -i -E "(local|stdio|FPF_MCP_SURFACE)" "$README" || echo "No matches found"Repository: venikman/fpf-memory
Length of output: 6525
Add local MCP reconfiguration step for expert-tool Codex sessions.
The Codex Setup section (lines 131–140) configures the hosted public MCP. Lines 148–158 then instruct starting FPF_MCP_SURFACE=full bun run mcp to access expert tools. However, if Codex is configured via .codex/config.toml to use the hosted URL (as shown in lines 131–140), starting a local full-surface server does not expose expert tools—Codex will continue connecting to the hosted endpoint where only public tools (ask_fpf, query_fpf_spec, get_fpf_index_status) are available.
The section must clarify that expert-tool sessions require either:
- Temporarily pointing
.codex/config.tomltohttp://localhost:1234(or the port where local MCP runs), or - Using the direct stdio launcher (
FPF_MCP_SURFACE=full bun src/mastra/stdio.ts) without MCP client reconfiguration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 131 - 140, Update the Codex Setup notes to explain
that if `.codex/config.toml` points to the hosted MCP URL (the
`mcp_servers.fpf_memory` entry), starting a local full-surface server with
`FPF_MCP_SURFACE=full bun run mcp` will not expose expert tools because Codex
still connects to the hosted endpoint; instruct readers to either temporarily
change the `mcp_servers.fpf_memory` URL in `.codex/config.toml` to
`http://localhost:1234` (or the local port) when running the local MCP, or
alternatively start the direct stdio launcher with `FPF_MCP_SURFACE=full bun
src/mastra/stdio.ts` so expert-tool sessions are available without editing
`.codex/config.toml`.
| export function resolveRuntimePath( | ||
| rawPath: string, | ||
| options: RuntimePathResolutionOptions = {}, | ||
| ): ResolvedRuntimePath { | ||
| const trimmedPath = rawPath.trim(); | ||
| const kind = options.kind ?? 'any'; | ||
|
|
There was a problem hiding this comment.
Reject blank runtime paths up front.
src/runtime/runtime.ts preserves empty env vars via ??, so FPF_SPEC_SOURCE_PATH= or FPF_RUNTIME_ARTIFACT_DIR= reaches this helper as "". From there resolve(root, '') collapses to the root itself, which can quietly redirect artifact writes into the project/bundle root instead of failing fast.
Suggested fix
export function resolveRuntimePath(
rawPath: string,
options: RuntimePathResolutionOptions = {},
): ResolvedRuntimePath {
const trimmedPath = rawPath.trim();
+ if (trimmedPath.length === 0) {
+ throw new Error('Runtime path must not be empty');
+ }
const kind = options.kind ?? 'any';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function resolveRuntimePath( | |
| rawPath: string, | |
| options: RuntimePathResolutionOptions = {}, | |
| ): ResolvedRuntimePath { | |
| const trimmedPath = rawPath.trim(); | |
| const kind = options.kind ?? 'any'; | |
| export function resolveRuntimePath( | |
| rawPath: string, | |
| options: RuntimePathResolutionOptions = {}, | |
| ): ResolvedRuntimePath { | |
| const trimmedPath = rawPath.trim(); | |
| if (trimmedPath.length === 0) { | |
| throw new Error('Runtime path must not be empty'); | |
| } | |
| const kind = options.kind ?? 'any'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/path-resolution.ts` around lines 20 - 26, In resolveRuntimePath,
reject empty/blank rawPath values immediately after trimming to avoid
resolve(root, '') collapsing to the project root; check trimmedPath === ''
(after const trimmedPath = rawPath.trim()) and throw a descriptive Error
(including the options.kind or the original rawPath) so callers fail fast
instead of silently returning the root path.
There was a problem hiding this comment.
Fixed in 3536a1c — added an early guard: if (trimmedPath.length === 0) throw new Error('Runtime path must not be empty'). This prevents FPF_SPEC_SOURCE_PATH="" or FPF_RUNTIME_ARTIFACT_DIR="" from silently resolving to the project root.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7bbf7a0b6
ℹ️ 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".
| # 1. Ensure the snapshot exists locally | ||
| if [ ! -f "$INDEX/snapshot.json" ]; then | ||
| echo "Snapshot not found — building index..." | ||
| bun "$ROOT/src/cli.ts" refresh-index |
There was a problem hiding this comment.
When snapshot.json is missing (the default in a clean clone, since .runtime is not tracked), this script calls bun src/cli.ts refresh-index, but the CLI only implements refresh/status/query/etc. As a result, predeploy exits non-zero under set -e, so bun run deploy fails before staging artifacts, blocking first-time or CI deploys unless a snapshot already exists locally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3536a1c — changed refresh-index → refresh (the correct CLI subcommand), and also changed the script to always run refresh before staging so stale snapshots can't be deployed when FPF-spec.md changes.
There was a problem hiding this comment.
Already fixed in 3536a1c — changed refresh-index to refresh (the correct CLI subcommand), and also made the refresh unconditional so stale snapshots can't slip through.
…reshness, error visibility - scripts/prepare-deploy.sh: fix CLI subcommand (refresh-index → refresh) - scripts/prepare-deploy.sh: always refresh before staging to prevent stale snapshots - src/runtime/path-resolution.ts: use dirname(absolutePath) consistently for root - src/runtime/path-resolution.ts: reject empty/blank rawPath with descriptive error - src/runtime/runtime.ts: log swallowed refresh errors in status() for debuggability Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/runtime.ts`:
- Around line 54-57: The code currently passes artifactDir directly to
resolveRuntimePath which allows its ancestor-scan to bind to a .fpf-runtime
outside the current source; instead, resolve artifactDir against
sourceResolution.root (i.e., compute the absolute path using
sourceResolution.root + artifactDir) before calling resolveRuntimePath so
discovery starts from the intended directory. Update the call that constructs
artifactResolution (the resolveRuntimePath invocation) to use the
resolved/absolute artifact path derived from sourceResolution.root and
artifactDir (leave fallbackRoot as-is or remove if no longer needed) to prevent
binding to unrelated parent runtime artifacts.
- Around line 200-207: status() currently only calls refresh(false) when
loadSnapshot() returns undefined, so snapshots that exist but fail
snapshotNeedsRebuild() are incorrectly treated as fresh; update the logic in
status() to, after loading existingSnapshot via loadSnapshot(), call
snapshotNeedsRebuild(existingSnapshot) and if it returns true run await
this.refresh(false) (with the same error handling) and then reload
existingSnapshot = await this.loadSnapshot(); ensure subsequent logic uses the
reloaded snapshot so fresh is computed from a snapshot that passed
snapshotNeedsRebuild() (refer to methods loadSnapshot, snapshotNeedsRebuild,
refresh, and status).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e6054c6-7c4b-4c12-a876-cd21023b7c97
📒 Files selected for processing (3)
scripts/prepare-deploy.shsrc/runtime/path-resolution.tssrc/runtime/runtime.ts
✅ Files skipped from review due to trivial changes (1)
- src/runtime/path-resolution.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/prepare-deploy.sh
| let existingSnapshot = await this.loadSnapshot(); | ||
| if (!existingSnapshot) { | ||
| await this.refresh(false).catch((error) => { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| console.warn(`[FpfRuntime.status] refresh(false) failed: ${message}`); | ||
| }); | ||
| existingSnapshot = await this.loadSnapshot(); | ||
| } |
There was a problem hiding this comment.
status() should treat incompatible snapshots as stale too.
This branch only refreshes when loadSnapshot() returns undefined, but refresh() also rebuilds snapshots that fail snapshotNeedsRebuild(). With an older on-disk schema, Line 216 can still report fresh: true even though the next query will rebuild—or fail if the source file is unavailable in the deployed image.
⚙️ Suggested fix
- let existingSnapshot = await this.loadSnapshot();
- if (!existingSnapshot) {
+ let existingSnapshot = await this.loadSnapshot();
+ if (!existingSnapshot || snapshotNeedsRebuild(existingSnapshot)) {
await this.refresh(false).catch((error) => {
const message = error instanceof Error ? error.message : String(error);
console.warn(`[FpfRuntime.status] refresh(false) failed: ${message}`);
});
existingSnapshot = await this.loadSnapshot();
}
@@
- fresh: existingSnapshot?.sourceHash === currentSourceHash,
+ fresh:
+ Boolean(existingSnapshot) &&
+ !snapshotNeedsRebuild(existingSnapshot) &&
+ existingSnapshot.sourceHash === currentSourceHash,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/runtime.ts` around lines 200 - 207, status() currently only calls
refresh(false) when loadSnapshot() returns undefined, so snapshots that exist
but fail snapshotNeedsRebuild() are incorrectly treated as fresh; update the
logic in status() to, after loading existingSnapshot via loadSnapshot(), call
snapshotNeedsRebuild(existingSnapshot) and if it returns true run await
this.refresh(false) (with the same error handling) and then reload
existingSnapshot = await this.loadSnapshot(); ensure subsequent logic uses the
reloaded snapshot so fresh is computed from a snapshot that passed
snapshotNeedsRebuild() (refer to methods loadSnapshot, snapshotNeedsRebuild,
refresh, and status).
Code ReviewOverviewThis PR shifts the Codex-facing default from local stdio MCP to a hosted HTTP endpoint, gates expert tools behind Strengths
Issues & RisksBehavior change in
|
…apshot freshness in status() - Pre-compute absolute artifact path via resolve(sourceResolution.root, artifactDir) before passing to resolveRuntimePath, preventing ancestor-walk from binding to unrelated .runtime/fpf-index directories in parent checkouts. - Extend status() to also trigger refresh when snapshot exists but fails snapshotNeedsRebuild() (incompatible schema), not just when snapshot is missing. - Update fresh computation to include snapshotNeedsRebuild() check, ensuring status never reports fresh:true for structurally incompatible snapshots. Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
|
Thanks for the thorough review. Here's a summary of what's been addressed in this PR and what I'd suggest as follow-ups: Addressed in commits 3536a1c + f60c747:
Suggested follow-ups (not in scope for this PR):
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f60c747d3c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| cp "$ROOT/FPF-spec.md" "$PUBLIC/FPF-spec.md" | ||
| cp "$INDEX/snapshot.json" "$PUBLIC/.runtime/fpf-index/snapshot.json" |
There was a problem hiding this comment.
prepare-deploy.sh rebuilds the snapshot via bun src/cli.ts refresh, which reads FPF_SPEC_SOURCE_PATH / FPF_RUNTIME_ARTIFACT_DIR, but the staging step then always copies from hard-coded FPF-spec.md and .runtime/fpf-index/snapshot.json. In environments that set non-default runtime paths, this stages a different source/snapshot pair than the one just refreshed, which can produce hash mismatches or stale data in the deployed runtime.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid observation — the staging cp commands use hardcoded default paths while refresh would respect env var overrides. In practice this script is only invoked in the CI deploy pipeline where defaults apply, so there's no mismatch today. Worth hardening as a follow-up if non-default runtime paths are ever used in deploy.
Summary
Verification
Summary by CodeRabbit
New Features
Documentation
Chores
Tests