Default actuator output to JSONL - #80
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe CLI now emits JSONL by default, uses ChangesActuator JSONL contract
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User as CLI caller
participant Actuator as xyph-actuator.ts
participant Entry as runActuator
participant Runtime as Graph runtime
participant Commands as Commander commands
participant Output as JSONL or human output
User->>Actuator: invoke command
Actuator->>Entry: delegate argv
alt help request
Entry->>Commands: create help-only context and parse help
else normal request
Entry->>Runtime: resolve runtime
Runtime-->>Entry: runtime context
Entry->>Commands: register and parse commands
end
Commands-->>Entry: success or Commander error
Entry->>Output: emit JSONL or human-rendered result
Possibly related PRs
✅ Pre-merge checks override appliedThe pre-merge checks have been overridden successfully. You can now proceed with the merge. Overridden by ❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/cli/actuatorEntry.ts (1)
242-244: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove command registration inside the try-catch block
defaultRegisterCommands(line 244) sits outside the try-catch (lines 246–262), so registration errors — Proxy throws from the help-only context, failed dynamic imports, or any register function accessing a missing property — propagate as unhandled rejections without structured logging. Moving registration inside the try block ensures all setup-and-parse errors are logged consistently viaserializeErrorand handled through the same error path.♻️ Proposed refactor
const program = createActuatorProgram(); program.exitOverride(); - - await (options.registerCommands ?? defaultRegisterCommands)(program, ctx); try { + await (options.registerCommands ?? defaultRegisterCommands)(program, ctx); await program.parseAsync(argv, { from: 'node' }); logger.info('actuator session ended cleanly'); return 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/actuatorEntry.ts` around lines 242 - 244, Move the command registration call using registerCommands/defaultRegisterCommands inside the existing try-catch in the actuator entry flow, before command parsing or execution. Ensure registration failures use the same serializeError logging and error-handling path as setup and parse failures, without changing the existing registration selection or context.
🤖 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 `@docs/CLI-plan.md`:
- Around line 320-322: Update the preceding interactive handoff example in the
CLI documentation to include the --humanize option on the xyph handoff command.
Keep the message and headless JSONL example unchanged, ensuring the interactive
example explicitly requests human-oriented output.
- Around line 194-213: Add an appropriate language identifier, such as console,
to the fenced code blocks containing CLI examples near the JSONL example and the
corresponding section around line 320. Update only the fence annotations while
preserving the example content.
---
Nitpick comments:
In `@src/cli/actuatorEntry.ts`:
- Around line 242-244: Move the command registration call using
registerCommands/defaultRegisterCommands inside the existing try-catch in the
actuator entry flow, before command parsing or execution. Ensure registration
failures use the same serializeError logging and error-handling path as setup
and parse failures, without changing the existing registration selection or
context.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ebecb2b1-f287-4124-8e76-63f22438865b
⛔ Files ignored due to path filters (9)
docs/diagrams/teardown-argv-scan.svgis excluded by!**/*.svgdocs/diagrams/teardown-cli-context.svgis excluded by!**/*.svgdocs/diagrams/teardown-domain-entities.svgis excluded by!**/*.svgdocs/diagrams/teardown-hexagonal-arch.svgis excluded by!**/*.svgdocs/diagrams/teardown-jsonl-protocol.svgis excluded by!**/*.svgdocs/diagrams/teardown-jsonl-types.svgis excluded by!**/*.svgdocs/diagrams/teardown-schema.svgis excluded by!**/*.svgdocs/diagrams/teardown-system-architecture.svgis excluded by!**/*.svgdocs/diagrams/teardown-system-mind-map.svgis excluded by!**/*.svg
📒 Files selected for processing (20)
docs/CLI-plan.mddocs/EXECUTIVE_SUMMARY.mddocs/TEARDOWN.mddocs/canonical/AGENT_PROTOCOL.mddocs/diagrams/teardown-argv-scan.mmddocs/diagrams/teardown-argv-scan.mmd.sha256docs/diagrams/teardown-jsonl-protocol.mmddocs/diagrams/teardown-jsonl-protocol.mmd.sha256docs/diagrams/teardown-system-architecture.mmddocs/diagrams/teardown-system-architecture.mmd.sha256docs/diagrams/teardown-system-mind-map.mmddocs/diagrams/teardown-system-mind-map.mmd.sha256src/cli/actuatorEntry.tssrc/cli/commands/wizards.tssrc/cli/index.tssrc/cli/runtimeEntry.tstest/acceptance/cycles/0022-durable-diagnostic-logging/DurableDiagnosticLogging.test.tstest/unit/ActuatorEntry.test.tstest/unit/runtimeEntry.test.tsxyph-actuator.ts
![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: cbf73a9dfe
ℹ️ 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".
|
@codex update: addressed the valid review findings in follow-up commits. Commit aba4877 moves actuator command registration into the existing try/catch and fixes the CLI-plan fence/handoff examples. Commit 3cf6788 emits a terminal JSONL error envelope for Commander parse failures and adds focused regression coverage. I am intentionally not preserving |
|
To use Codex here, create an environment for this repo. |
|
@codex self-audit finding before fix:
|
|
To use Codex here, create an environment for this repo. |
Code Lawyer Activity Summary
Validation:
|
|
@coderabbitai ignore pre-merge checks |
ℹ️ Actions performedPre-merge checks override executed successfully. Overridden by Approval outcome: Approval skipped: request-changes workflow disabled. |
|
@codex self-audit findings before fix:
|
|
To use Codex here, create an environment for this repo. |
Code Lawyer Activity Summary
Validation:
@codex please confirm the follow-up fixes preserve the intended agent-first JSONL actuator contract. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3919c64d41
ℹ️ 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".
Code Lawyer Follow-up Activity Summary
Validation:
@codex please confirm the new actuator startup/help behavior now satisfies the default JSONL contract. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a19fca06c
ℹ️ 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".
| * command line, because output mode determines which CLI context is built. | ||
| */ | ||
| export function parseHumanizeFlagFromArgv(argv: readonly string[]): boolean { | ||
| return argv.includes('--humanize'); |
There was a problem hiding this comment.
For free-text options such as comment --message, Commander consumes a following dash-prefixed token as the required option value (the tests already rely on this for --message -h), but this raw pre-scan flips the entire invocation to human mode whenever that value is exactly --humanize. An agent command like xyph-actuator comment comment:x --on task:x --message --humanize therefore stops emitting the default JSONL terminal envelope even though the user supplied --humanize as data, not as the output-mode switch. Apply the same token-consumption classification used by help detection before treating --humanize as global.
Useful? React with 👍 / 👎.
| i += 1; | ||
| continue; | ||
| } | ||
| if (HELP_FLAGS.has(arg)) return true; |
There was a problem hiding this comment.
When a root option precedes the help subcommand, for example xyph-actuator --humanize help status or xyph-actuator --as agent.x help status, the scan skips/ignores the option tokens but never recognizes the later help token. That leaves helpOnly false, so runActuator resolves graph runtime before Commander can render help; from a directory without a configured git repo this returns a startup error instead of help, breaking the lazy-help contract. Treat help as a help request after skipped global/value options too.
Useful? React with 👍 / 👎.
| { registerShowCommands }, | ||
| { registerAgentCommands }, | ||
| { registerDoctorCommands }, | ||
| { registerApiCommands }, |
There was a problem hiding this comment.
Because the new entrypoint registers api under the default JSONL actuator, the existing TTY guard in src/cli/commands/api.ts:17-19 is now reachable in machine mode but still writes plain text to stderr and exits without any terminal JSONL error. In the no-stdin case (xyph-actuator api run interactively, or by a tool that forgot to pipe request records), JSONL consumers get no parseable failure envelope and stderr is contaminated, so this rejection should go through the structured error path instead.
Useful? React with 👍 / 👎.
| try { | ||
| let runtime: ResolvedGraphRuntime | null = null; | ||
| if (!helpOnly) { | ||
| runtime = (options.resolveRuntime ?? ((runtimeCwd: string): ResolvedGraphRuntime => resolveGraphRuntime({ cwd: runtimeCwd })))(cwd); |
There was a problem hiding this comment.
When tests or embedded callers pass RunActuatorOptions.homeDir to isolate XYPH user config, the log path and identity resolution honor that directory but graph runtime discovery still reads the process home directory. For non-help invocations from a cwd without local graph config, this can select the wrong graph or emit a startup error even though the supplied homeDir contains the intended .xyph/config; forward options.homeDir to resolveGraphRuntime here.
Useful? React with 👍 / 👎.
1 participant
Summary
--jsonswitch with opt-in--humanizeoutput.Validation
npm run buildnpm run lintnpm run test:localbash scripts/check-diagrams.shnpm run test:localand diagram freshness checks successfully.Notes
scripts/render-diagrams.shrefreshed the committed diagram artifacts. It also reported existing render failures forvision-layer-stack.mmdandwasm-target-lowerer.mmd; those are outside this PR's actuator-output change, andscripts/check-diagrams.shpasses.Summary by CodeRabbit
New Features
--humanizefor human-friendly rendering (and interactive wizard flows now align with it).Bug Fixes
Documentation
--jsonframing and reflect the--humanize/default JSONL contract.