feat: schema introspection, integration tests, monolith removal#35
Merged
feat: schema introspection, integration tests, monolith removal#35
Conversation
📝 WalkthroughWalkthroughA legacy, monolithic Partiful CLI was removed and replaced with a slimmer command structure. New additions include a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 Generate unit tests (beta)
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 (3)
src/commands/schema.js (1)
3-80: Consider making schema metadata a single source of truth.
SCHEMASis hand-maintained and can drift from real command option definitions, which weakens agent introspection accuracy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/schema.js` around lines 3 - 80, SCHEMAS is a hand-maintained metadata object that can drift from actual CLI option definitions; replace it with a single source of truth by deriving schema data programmatically from the real command registration logic (e.g., where commands and options are defined/registered) instead of hardcoding SCHEMAS, or centralize command option declarations into a shared descriptor used both to register commands and to build this schema. Locate SCHEMAS in src/commands/schema.js and change callers to read the derived descriptor (or generate SCHEMAS at startup by introspecting the command registry), ensuring functions or modules that register commands (the command registration function/class and option definitions) become the canonical source used to construct the schema metadata and remove duplicate, manually maintained entries.tests/events-integration.test.js (2)
8-29: Extract shared CLI execution helpers to reduce duplication.
run/runRaware duplicated withtests/guests-integration.test.js, which invites drift in env/timeout behavior across suites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events-integration.test.js` around lines 8 - 29, The run and runRaw helper functions are duplicated across test files; extract them into a single shared helper module (e.g., exportRunHelpers) and update tests to import and use those helpers to avoid drift. Specifically, move the implementations of run and runRaw into a new common test helper (exporting functions named run and runRaw), ensure the helper sets the same env/timeout defaults and supports passing opts, and update tests/events-integration.test.js and tests/guests-integration.test.js to import { run, runRaw } from that shared helper instead of defining them locally.
111-115:versiontest is too weak to catch contract regressions.Current assertion only checks non-empty stdout. It should validate exit code and JSON shape/fields (
status,data.version,data.cli,data.node).💡 Proposed test hardening
describe('version command', () => { it('returns version info', () => { - const { stdout } = runRaw(['version']); - expect(stdout).toBeTruthy(); + const { stdout, exitCode } = runRaw(['version']); + expect(exitCode).toBe(0); + const out = JSON.parse(stdout.trim()); + expect(out.status).toBe('success'); + expect(out.data.cli).toBe('partiful'); + expect(out.data.version).toBeTruthy(); + expect(out.data.node).toMatch(/^v/); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events-integration.test.js` around lines 111 - 115, The current 'version command' test only asserts non-empty stdout; update the test that uses runRaw to also assert the process exit code is 0 and parse stdout as JSON, then validate the JSON shape includes top-level keys 'status' and 'data' and that data contains 'version', 'cli', and 'node' fields (check they are non-empty strings or expected types); locate the test block with describe('version command') and the it(...) that calls runRaw(['version']) and replace the single truthy assertion with these additional checks (exitCode, parsed JSON structure and field existence/type assertions).
🤖 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/cli.js`:
- Around line 40-45: The version command's action calls jsonOutput(...) directly
and ignores global output options; update the program.command('version') action
to merge or pass optsWithGlobals() into the jsonOutput call (e.g., call
optsWithGlobals() and supply its output handling/options to jsonOutput) so that
partiful version respects --output and other global flags; locate the action
handler around program.command('version') and change the jsonOutput invocation
accordingly.
In `@src/commands/schema.js`:
- Around line 92-96: The lookup using SCHEMAS[path] is vulnerable to
prototype-chain keys; change the check to explicitly test own properties (e.g.,
use Object.prototype.hasOwnProperty.call(SCHEMAS, path) or Reflect.has with
Object.hasOwn) instead of truthiness of SCHEMAS[path], and only call jsonError
when the schema is not an own property; still retrieve available keys with
Object.keys(SCHEMAS) for the error message and then assign schema =
SCHEMAS[path] after the ownership check so code that references schema (and
calls jsonError) behaves correctly.
---
Nitpick comments:
In `@src/commands/schema.js`:
- Around line 3-80: SCHEMAS is a hand-maintained metadata object that can drift
from actual CLI option definitions; replace it with a single source of truth by
deriving schema data programmatically from the real command registration logic
(e.g., where commands and options are defined/registered) instead of hardcoding
SCHEMAS, or centralize command option declarations into a shared descriptor used
both to register commands and to build this schema. Locate SCHEMAS in
src/commands/schema.js and change callers to read the derived descriptor (or
generate SCHEMAS at startup by introspecting the command registry), ensuring
functions or modules that register commands (the command registration
function/class and option definitions) become the canonical source used to
construct the schema metadata and remove duplicate, manually maintained entries.
In `@tests/events-integration.test.js`:
- Around line 8-29: The run and runRaw helper functions are duplicated across
test files; extract them into a single shared helper module (e.g.,
exportRunHelpers) and update tests to import and use those helpers to avoid
drift. Specifically, move the implementations of run and runRaw into a new
common test helper (exporting functions named run and runRaw), ensure the helper
sets the same env/timeout defaults and supports passing opts, and update
tests/events-integration.test.js and tests/guests-integration.test.js to import
{ run, runRaw } from that shared helper instead of defining them locally.
- Around line 111-115: The current 'version command' test only asserts non-empty
stdout; update the test that uses runRaw to also assert the process exit code is
0 and parse stdout as JSON, then validate the JSON shape includes top-level keys
'status' and 'data' and that data contains 'version', 'cli', and 'node' fields
(check they are non-empty strings or expected types); locate the test block with
describe('version command') and the it(...) that calls runRaw(['version']) and
replace the single truthy assertion with these additional checks (exitCode,
parsed JSON structure and field existence/type assertions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32dff257-5277-4cd0-8264-3d79d5ef67f8
📒 Files selected for processing (5)
partifulsrc/cli.jssrc/commands/schema.jstests/events-integration.test.jstests/guests-integration.test.js
💤 Files with no reviewable changes (1)
- partiful
![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: 0c8300ecaa
ℹ️ 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".
src/cli.js
Outdated
Comment on lines
+43
to
+44
| .action(() => { | ||
| jsonOutput({ version: program.version(), cli: 'partiful', node: process.version }); |
There was a problem hiding this comment.
The new version subcommand writes JSON via jsonOutput(...) without passing cmd.optsWithGlobals(), so global options like --output are ignored here even though other commands honor them. In scripted usage (for example, partiful version --output version.json), this unexpectedly writes to stdout instead of the target file and breaks workflows that depend on consistent global flag behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/commands/schema.js (2)
32-49: Consider adding missingdescriptionfields for consistency.Several parameters in
events.update,events.cancel, andcontacts.list --limitlackdescriptionfields, while other schemas include them. Since this command is designed for agent introspection, consistent descriptions improve usability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/schema.js` around lines 32 - 49, The schema entries for 'events.update' and 'events.cancel' (and the 'contacts.list' --limit parameter) are missing parameter description fields; update the parameter objects for eventId, --title, --date, --end-date, --location, --description, --capacity in 'events.update' and eventId in 'events.cancel' to include concise description strings, and add a description to the --limit parameter of 'contacts.list' to match existing schemas; locate and edit the parameter definitions in the 'events.update' and 'events.cancel' blocks and the 'contacts.list' parameter definition to add these description properties.
1-1: Unused import:EXIT.
EXITis imported but never referenced in this file. Remove it to avoid dead code.-import { jsonOutput, jsonError, EXIT } from '../lib/output.js'; +import { jsonOutput, jsonError } from '../lib/output.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/schema.js` at line 1, Remove the unused EXIT import from the module import line so the top-level import only brings in jsonOutput and jsonError; update the import statement that currently references EXIT (import { jsonOutput, jsonError, EXIT } from '../lib/output.js') to drop EXIT to eliminate the dead/unreferenced symbol.tests/helpers.js (1)
21-22: Consider: stderr is not captured on success path.The success case returns only
stdout, while the error case returns bothstdoutandstderr. For consistency and easier debugging, consider capturing stderr on success as well.♻️ Optional improvement for consistency
To capture stderr on success, you'd need to use
spawnSyncinstead ofexecFileSync:import { spawnSync } from 'child_process'; export function runRaw(args, opts = {}) { const result = spawnSync('node', [CLI, ...args], { encoding: 'utf-8', env: { ...process.env, PARTIFUL_TOKEN: 'fake-token', ...opts.env }, timeout: 10000, }); return { stdout: result.stdout || '', stderr: result.stderr || '', exitCode: result.status ?? 1, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers.js` around lines 21 - 22, The success path in runRaw (tests/helpers.js) only returns stdout while the error path includes stderr, so change runRaw to use spawnSync('node', [CLI, ...args], {...}) instead of execFileSync and return an object with stdout, stderr and exitCode (use result.stdout || '' and result.stderr || '' and result.status ?? 1) so both success and failure consistently expose stderr; update imports to pull spawnSync from 'child_process' and preserve existing env, timeout and encoding options.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/helpers.js`:
- Around line 23-24: The catch block in tests/helpers.js currently returns
exitCode: e.status which is null when a process is killed by a signal (e.g.,
timeout); update the catch to return a deterministic numeric exitCode: if
e.status is a number use it, otherwise if e.signal is present map the signal
name (e.g., 'SIGTERM','SIGKILL') to the conventional exit code (128 + signal
number, e.g., SIGTERM -> 143, SIGKILL -> 137) and return that; ensure the change
is made in the catch that returns { stdout: e.stdout || '', stderr: e.stderr ||
'', exitCode: ... } so tests that assert numeric exit codes get a stable value.
---
Nitpick comments:
In `@src/commands/schema.js`:
- Around line 32-49: The schema entries for 'events.update' and 'events.cancel'
(and the 'contacts.list' --limit parameter) are missing parameter description
fields; update the parameter objects for eventId, --title, --date, --end-date,
--location, --description, --capacity in 'events.update' and eventId in
'events.cancel' to include concise description strings, and add a description to
the --limit parameter of 'contacts.list' to match existing schemas; locate and
edit the parameter definitions in the 'events.update' and 'events.cancel' blocks
and the 'contacts.list' parameter definition to add these description
properties.
- Line 1: Remove the unused EXIT import from the module import line so the
top-level import only brings in jsonOutput and jsonError; update the import
statement that currently references EXIT (import { jsonOutput, jsonError, EXIT }
from '../lib/output.js') to drop EXIT to eliminate the dead/unreferenced symbol.
In `@tests/helpers.js`:
- Around line 21-22: The success path in runRaw (tests/helpers.js) only returns
stdout while the error path includes stderr, so change runRaw to use
spawnSync('node', [CLI, ...args], {...}) instead of execFileSync and return an
object with stdout, stderr and exitCode (use result.stdout || '' and
result.stderr || '' and result.status ?? 1) so both success and failure
consistently expose stderr; update imports to pull spawnSync from
'child_process' and preserve existing env, timeout and encoding options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b69aa11-580e-479b-aaff-ab58dc1e2fb3
📒 Files selected for processing (5)
src/cli.jssrc/commands/schema.jstests/events-integration.test.jstests/guests-integration.test.jstests/helpers.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/cli.js
- tests/guests-integration.test.js
- tests/events-integration.test.js
Comment on lines
+23
to
+24
| } catch (e) { | ||
| return { stdout: e.stdout || '', stderr: e.stderr || '', exitCode: e.status }; |
There was a problem hiding this comment.
Edge case: e.status is null when process is killed by signal (e.g., timeout).
If the 10s timeout triggers, execFileSync kills the process with SIGTERM and e.status will be null. This could cause unexpected behavior in test assertions checking for specific exit codes. Consider handling this case explicitly.
🛡️ Suggested improvement
} catch (e) {
- return { stdout: e.stdout || '', stderr: e.stderr || '', exitCode: e.status };
+ // e.status is null when killed by signal (e.g., timeout)
+ return { stdout: e.stdout || '', stderr: e.stderr || '', exitCode: e.status ?? 1 };
}📝 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.
Suggested change
| } catch (e) { | |
| return { stdout: e.stdout || '', stderr: e.stderr || '', exitCode: e.status }; | |
| } catch (e) { | |
| // e.status is null when killed by signal (e.g., timeout) | |
| return { stdout: e.stdout || '', stderr: e.stderr || '', exitCode: e.status ?? 1 }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/helpers.js` around lines 23 - 24, The catch block in tests/helpers.js
currently returns exitCode: e.status which is null when a process is killed by a
signal (e.g., timeout); update the catch to return a deterministic numeric
exitCode: if e.status is a number use it, otherwise if e.signal is present map
the signal name (e.g., 'SIGTERM','SIGKILL') to the conventional exit code (128 +
signal number, e.g., SIGTERM -> 143, SIGKILL -> 137) and return that; ensure the
change is made in the catch that returns { stdout: e.stdout || '', stderr:
e.stderr || '', exitCode: ... } so tests that assert numeric exit codes get a
stable value.
This was referenced Mar 25, 2026
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
1 participant
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.
Changes
partiful schema [path]) — agents can introspect command parameters as JSONpartiful version) — returns structured version infosrc/architectureRemaining backlog: text blast endpoint reverse-engineering.
Summary by CodeRabbit
Revert
New Features
schemacommand to inspect available command parameters and configurationsversioncommand to display CLI version and environment informationTests