Add CLI options to configure framework services#1
Add CLI options to configure framework services#1google-labs-jules[bot] wants to merge 1 commit intomainfrom
Conversation
- Added support for `--no-vkb`, `--no-constraints`, `--no-transcript`, `--no-logging`, and `--no-health` flags in `bin/coding` - Updated `scripts/launch-claude.sh` and `scripts/launch-copilot.sh` to forward service arguments - Modified `scripts/start-services-robust.js` to conditionally start services based on CLI flags - Default behavior remains unchanged (all services start by default)
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. New to Jules? Learn more at jules.google/docs. |
There was a problem hiding this comment.
Pull request overview
This PR adds CLI flags to control which framework services start during the coding command initialization. While the implementation correctly parses and forwards the flags through the shell script chain, there is a critical design flaw: it allows users to skip services marked as REQUIRED, which contradicts the robust startup logic that normally blocks execution when required services fail.
Key Changes:
- Added five new CLI flags:
--no-vkb,--no-constraints,--no-transcript,--no-logging,--no-health - Implemented argument forwarding from
bin/codingthrough launch scripts to the Node.js service starter - Modified service startup logic to conditionally skip services based on flags
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| bin/coding | Added parsing for service control flags and exports them via SERVICE_ARGS_STR environment variable |
| scripts/launch-copilot.sh | Reconstructs SERVICE_ARGS array from environment variable and forwards to start-services.sh |
| scripts/launch-claude.sh | Reconstructs SERVICE_ARGS array from environment variable and forwards to start-services.sh |
| start-services.sh | Forwards all arguments to the Node.js robust service starter |
| scripts/start-services-robust.js | Parses service flags and conditionally skips services, but allows bypassing required services which breaks startup validation logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| }); | ||
| } | ||
| } else { | ||
| console.log('⏭️ Skipping Live Logging Coordinator (--no-logging)'); |
There was a problem hiding this comment.
Same issue as with --no-transcript: the Live Logging Coordinator is marked as a REQUIRED service (line 100 in SERVICE_CONFIGS) but can be skipped with --no-logging. This bypasses the startup failure logic that would normally block if a required service fails to start.
See comment on lines 527-550 for the full explanation and recommendation.
| console.log('⏭️ Skipping Live Logging Coordinator (--no-logging)'); | |
| if (SERVICE_CONFIGS.liveLoggingCoordinator.required) { | |
| console.error('❌ ERROR: Live Logging Coordinator is a REQUIRED service and cannot be skipped with --no-logging.'); | |
| process.exit(1); | |
| } else { | |
| console.log('⏭️ Skipping Live Logging Coordinator (--no-logging)'); | |
| } |
| } | ||
|
|
||
| # Reconstruct SERVICE_ARGS array from env var | ||
| SERVICE_ARGS=($SERVICE_ARGS_STR) |
There was a problem hiding this comment.
The array reconstruction SERVICE_ARGS=($SERVICE_ARGS_STR) is missing quotes around the variable expansion. This can cause issues if SERVICE_ARGS_STR is empty or contains special characters. While the current use case (simple flag names) works, this is a bash anti-pattern.
Recommendation: Use read -ra SERVICE_ARGS <<< "$SERVICE_ARGS_STR" for safer array reconstruction, or quote the expansion as SERVICE_ARGS=("$SERVICE_ARGS_STR") if treating as a single element, or use proper array serialization if multiple elements with spaces are expected.
| SERVICE_ARGS=($SERVICE_ARGS_STR) | |
| read -ra SERVICE_ARGS <<< "$SERVICE_ARGS_STR" |
|
|
||
|
|
||
| # Reconstruct SERVICE_ARGS array from env var | ||
| SERVICE_ARGS=($SERVICE_ARGS_STR) |
There was a problem hiding this comment.
The array reconstruction SERVICE_ARGS=($SERVICE_ARGS_STR) is missing quotes around the variable expansion. This can cause issues if SERVICE_ARGS_STR is empty or contains special characters. While the current use case (simple flag names) works, this is a bash anti-pattern.
Recommendation: Use read -ra SERVICE_ARGS <<< "$SERVICE_ARGS_STR" for safer array reconstruction, or quote the expansion as SERVICE_ARGS=("$SERVICE_ARGS_STR") if treating as a single element, or use proper array serialization if multiple elements with spaces are expected.
| SERVICE_ARGS=($SERVICE_ARGS_STR) | |
| read -ra SERVICE_ARGS <<< "$SERVICE_ARGS_STR" |
| if (!SERVICE_FLAGS.skipTranscript) { | ||
| try { | ||
| const transcriptResult = await startServiceWithRetry( | ||
| SERVICE_CONFIGS.transcriptMonitor.name, | ||
| SERVICE_CONFIGS.transcriptMonitor.startFn, | ||
| SERVICE_CONFIGS.transcriptMonitor.healthCheckFn, | ||
| { | ||
| required: SERVICE_CONFIGS.transcriptMonitor.required, | ||
| maxRetries: SERVICE_CONFIGS.transcriptMonitor.maxRetries, | ||
| timeout: SERVICE_CONFIGS.transcriptMonitor.timeout | ||
| } | ||
| ); | ||
| results.successful.push(transcriptResult); | ||
| await registerWithPSM(transcriptResult, 'scripts/enhanced-transcript-monitor.js'); | ||
| } catch (error) { | ||
| results.failed.push({ | ||
| serviceName: SERVICE_CONFIGS.transcriptMonitor.name, | ||
| error: error.message, | ||
| required: true | ||
| }); | ||
| } | ||
| } else { | ||
| console.log('⏭️ Skipping Transcript Monitor (--no-transcript)'); | ||
| } |
There was a problem hiding this comment.
The --no-transcript and --no-logging flags allow skipping services that are marked as REQUIRED in SERVICE_CONFIGS (lines 67 and 100). When these services fail to start, the code correctly blocks startup (lines 697-706), but when they're skipped via flags, they're not added to results.failed, allowing startup to proceed.
This creates an inconsistency: either these services should not be skippable, or they should not be marked as required. Based on the PR description saying these flags "provide flexibility," it seems the intent is to allow skipping them, but this contradicts their required status.
Recommendation: Either remove --no-transcript and --no-logging from the allowed flags, or change the service configs to mark these as optional when the corresponding skip flags are set.
This change introduces CLI options to the
codingcommand, allowing users to configure which parts of the agent coding framework to include during startup.New flags:
--no-vkb: Skips VKB Server startup--no-constraints: Skips Constraint Monitor startup--no-transcript: Skips Transcript Monitor startup--no-logging: Skips Live Logging Coordinator startup--no-health: Skips Health Verifier and Statusline Monitor startupBy default, all services are started. These options provide flexibility for users who may want to run a lighter-weight environment or troubleshoot specific components.
PR created automatically by Jules for task 1422269814196160073 started by @davidraehles