fix(cli): tie iii engine lifecycle and skip prompts in non-TTY context#528
Open
jonathanzhan1975 wants to merge 4 commits into
Open
fix(cli): tie iii engine lifecycle and skip prompts in non-TTY context#528jonathanzhan1975 wants to merge 4 commits into
jonathanzhan1975 wants to merge 4 commits into
Conversation
|
@jonathanzhan1975 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds TTY- and preference-aware CLI prompt gating; implements a --background spawn mode that detaches/unrefs the engine and alters stdio/handler registration; introduces isDropStaleIndexEnabled() wired into index logic; extends memory_recall validation and proxying to /agentmemory/search with format/token_budget; quotes plugin hook node commands and updates tests. ChangesCLI Prompting and Background Process Mode
MCP standalone: memory_recall and memory_smart_search
Config flag and index wiring
Plugin hook manifests and tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cli.ts (2)
429-432: 💤 Low valueRedundant TTY check —
!process.stdin.isTTYon line 432 is unreachable.Line 429 already returns early when
!process.stdin.isTTY, so the same condition on line 432 can never be true. Theprefs.skipNpxHintaddition is the meaningful change; consider removing the duplicate TTY check for clarity.♻️ Suggested simplification
- if (prefs.skipGlobalInstall || prefs.skipNpxHint || !process.stdin.isTTY) return; + if (prefs.skipGlobalInstall || prefs.skipNpxHint) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.ts` around lines 429 - 432, The second redundant TTY check is unreachable because the file already returns when !process.stdin.isTTY; edit the conditional that currently reads "if (prefs.skipGlobalInstall || prefs.skipNpxHint || !process.stdin.isTTY) return;" to remove the duplicate "!process.stdin.isTTY" check so it simply returns when prefs.skipGlobalInstall or prefs.skipNpxHint are true, keeping the earlier "if (!process.stdin.isTTY) return;" and using the existing prefs variable (prefs.skipGlobalInstall, prefs.skipNpxHint) to determine the later early exit.
517-519: 💤 Low valueSame redundancy:
!process.stdin.isTTYon line 519 is already handled by line 517.♻️ Suggested simplification
- if (prefs.skipConsoleInstall || !process.stdin.isTTY) return state; + if (prefs.skipConsoleInstall) return state;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.ts` around lines 517 - 519, The second TTY check is redundant; after the earlier guard "if (!process.stdin.isTTY || process.env['CI']) return state" we already know stdin is a TTY. Update the conditional "if (prefs.skipConsoleInstall || !process.stdin.isTTY) return state" to remove the redundant check so it reads "if (prefs.skipConsoleInstall) return state" (leave the readPrefs() call and surrounding logic intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/cli.ts`:
- Around line 429-432: The second redundant TTY check is unreachable because the
file already returns when !process.stdin.isTTY; edit the conditional that
currently reads "if (prefs.skipGlobalInstall || prefs.skipNpxHint ||
!process.stdin.isTTY) return;" to remove the duplicate "!process.stdin.isTTY"
check so it simply returns when prefs.skipGlobalInstall or prefs.skipNpxHint are
true, keeping the earlier "if (!process.stdin.isTTY) return;" and using the
existing prefs variable (prefs.skipGlobalInstall, prefs.skipNpxHint) to
determine the later early exit.
- Around line 517-519: The second TTY check is redundant; after the earlier
guard "if (!process.stdin.isTTY || process.env['CI']) return state" we already
know stdin is a TTY. Update the conditional "if (prefs.skipConsoleInstall ||
!process.stdin.isTTY) return state" to remove the redundant check so it reads
"if (prefs.skipConsoleInstall) return state" (leave the readPrefs() call and
surrounding logic intact).
Co-authored-by: 이민재 <19909783+honor2030@users.noreply.github.com>
Co-authored-by: honor2030 <19909783+honor2030@users.noreply.github.com>
…oken_budget (rohitg00#507) (rohitg00#516) * fix(mcp): route memory_recall to /agentmemory/search and forward format/token_budget memory_recall and memory_smart_search were sharing the smart-search endpoint, which always returns compact mode and silently drops the format and token_budget parameters that the tool schema advertises. Split the cases so memory_recall hits /agentmemory/search (which honors format) while memory_smart_search keeps its own endpoint. Default format to "full" for memory_recall so the documented behavior matches the wire call. Signed-off-by: serhiizghama <zmrser@gmail.com> * test(mcp): cover memory_recall endpoint, format forwarding, and defaults Two new proxy tests for issue rohitg00#507: one asserts memory_recall calls POST /agentmemory/search with the format and token_budget fields, and never falls through to smart-search; the other pins the default format to "full" when the caller omits it. Signed-off-by: serhiizghama <zmrser@gmail.com> --------- Signed-off-by: serhiizghama <zmrser@gmail.com>
jonathanzhan1975
force-pushed
the
fix/cli-iii-lifecycle
branch
from
3317d30 to
fc91308
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli.ts`:
- Around line 663-667: The signal handlers in cli.ts currently call
process.exit() after cleanupChild(), which can prematurely terminate the process
before other signal handlers in src/index.ts run (e.g., indexPersistence.save()
and sdk.shutdown()). Remove the forced process.exit() call inside the loop that
registers process.on for "SIGINT" and "SIGTERM"; instead simply invoke
cleanupChild() (or await it if it becomes async) and return so the normal signal
propagation and the index.ts handlers (indexPersistence.save, sdk.shutdown) can
run, or if you need to ensure ordering, invoke those shutdown helpers explicitly
from this handler rather than calling process.exit().
In `@src/mcp/standalone.ts`:
- Around line 123-126: The code currently lowercases args["format"] and assigns
it to v.format but accepts any non-empty string; instead enforce a whitelist of
allowed formats (e.g., const allowedFormats = ['json','yaml', ...]) and after
computing fmt = args["format"]?.trim().toLowerCase() validate that fmt is one of
allowedFormats; if not, return or throw a validation error (fail fast at the MCP
boundary) rather than assigning v.format. Update the block that reads fmt/sets
v.format (referencing args["format"], fmt, and v.format) to perform this
whitelist check and produce a clear validation error when the value is invalid.
🪄 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: 12d3e765-c203-4b9f-8038-026f64a8fbe2
📒 Files selected for processing (9)
plugin/hooks/hooks.codex.jsonplugin/hooks/hooks.jsonsrc/cli.tssrc/config.tssrc/index.tssrc/mcp/standalone.tstest/codex-plugin.test.tstest/env-loader.test.tstest/mcp-standalone-proxy.test.ts
✅ Files skipped from review due to trivial changes (2)
- plugin/hooks/hooks.json
- plugin/hooks/hooks.codex.json
Comment on lines
+663
to
+667
| for (const sig of ["SIGINT", "SIGTERM"] as const) { | ||
| process.on(sig, () => { | ||
| cleanupChild(); | ||
| process.exit(); | ||
| }); |
There was a problem hiding this comment.
Avoid forcing process.exit() in the engine cleanup signal hook.
Line 666 can terminate the process before src/index.ts signal handlers run, which skips graceful worker shutdown (indexPersistence.save(), sdk.shutdown()).
🔧 Proposed fix
for (const sig of ["SIGINT", "SIGTERM"] as const) {
process.on(sig, () => {
cleanupChild();
- process.exit();
+ // Let downstream signal handlers (worker shutdown) run.
+ // Only force exit if this is the sole listener.
+ if (process.listenerCount(sig) === 1) process.exit(0);
});
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli.ts` around lines 663 - 667, The signal handlers in cli.ts currently
call process.exit() after cleanupChild(), which can prematurely terminate the
process before other signal handlers in src/index.ts run (e.g.,
indexPersistence.save() and sdk.shutdown()). Remove the forced process.exit()
call inside the loop that registers process.on for "SIGINT" and "SIGTERM";
instead simply invoke cleanupChild() (or await it if it becomes async) and
return so the normal signal propagation and the index.ts handlers
(indexPersistence.save, sdk.shutdown) can run, or if you need to ensure
ordering, invoke those shutdown helpers explicitly from this handler rather than
calling process.exit().
Comment on lines
+123
to
+126
| const fmt = args["format"]; | ||
| if (typeof fmt === "string" && fmt.trim()) { | ||
| v.format = fmt.trim().toLowerCase(); | ||
| } |
There was a problem hiding this comment.
Whitelist accepted format values at validation boundary.
Line 123-126 lowercases format but accepts any non-empty string; invalid values are forwarded downstream instead of failing fast in the MCP boundary validator.
🔧 Proposed fix
const fmt = args["format"];
if (typeof fmt === "string" && fmt.trim()) {
- v.format = fmt.trim().toLowerCase();
+ const normalized = fmt.trim().toLowerCase();
+ const allowedFormats = new Set(["full", "compact"]);
+ if (!allowedFormats.has(normalized)) {
+ throw new Error("format must be one of: full, compact");
+ }
+ v.format = normalized;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp/standalone.ts` around lines 123 - 126, The code currently lowercases
args["format"] and assigns it to v.format but accepts any non-empty string;
instead enforce a whitelist of allowed formats (e.g., const allowedFormats =
['json','yaml', ...]) and after computing fmt =
args["format"]?.trim().toLowerCase() validate that fmt is one of allowedFormats;
if not, return or throw a validation error (fail fast at the MCP boundary)
rather than assigning v.format. Update the block that reads fmt/sets v.format
(referencing args["format"], fmt, and v.format) to perform this whitelist check
and produce a clear validation error when the value is invalid.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root cause of #303 is the TTY heuristic causing iii.exe to be orphaned in non-TTY context; also interactive prompts block headless launches.
This fix:
backgroundModep.confirm()calls in the startup install prompts with a!process.stdin.isTTYcheckFixes #303.
Summary by CodeRabbit
Bug Fixes
New Features
Chores
Tests