Skip to content

feat(json): per-command field allowlist + --jq filter#384

Merged
christso merged 1 commit into
mainfrom
feat/376-json-allowlist
May 12, 2026
Merged

feat(json): per-command field allowlist + --jq filter#384
christso merged 1 commit into
mainfrom
feat/376-json-allowlist

Conversation

@christso
Copy link
Copy Markdown
Contributor

Summary

Brings the gh-style --json[=field1,field2] --jq <expr> contract to allagents:

  • AgentCommandMeta.jsonFields declares a per-command allowlist. --json=<fields> is validated against it at parse time; an unknown field exits 2 with a sorted Available fields: suggestion list.
  • --jq <expr> pipes the envelope through the system jq binary (spawnSync, errors surfaced as Error: --jq failed: <msg> rather than raw stderr). --jq requires --json.
  • Field projection is shape-aware: if data has a single top-level array of objects, the allowlist narrows each item; otherwise it projects the top-level data directly.
  • --agent-help now emits json_fields when a meta declares an allowlist, so agents can discover the contract.
  • Carries two prerequisite fixes the gate depends on:

Test plan

  • bun run build, bun run typecheck
  • bun test — 1191 pass / 0 fail
  • Verification gate from feat: --json field allowlist with --jq / --template filters per command #376 (using stable obra/superpowers):
    • Boolean --json skills list returns full envelope with data.skills array.
    • --json=name,plugin skills list.data.skills[0] | keys == ["name","plugin"].
    • --json=bogus skills list → exit 2, Unknown JSON field + Available fields printed.
    • --json --jq '.data.skills | map(.name)' skills list → pipes through jq, returns array.
    • --jq without --json → exit 2.
    • --agent-help "skills list" exposes json_fields as an array.

Closes #376

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 12, 2026

Deploying allagents with  Cloudflare Pages  Cloudflare Pages

Latest commit: e6299d9
Status: ✅  Deploy successful!
Preview URL: https://003dacb7.allagents.pages.dev
Branch Preview URL: https://feat-376-json-allowlist.allagents.pages.dev

View logs

@christso
Copy link
Copy Markdown
Contributor Author

Code Review: ⚠️ APPROVE WITH SIGNIFICANT CONCERNS

Verification gate (#376)

All six gate checks pass on the diff:

  • (1) Boolean --json returns full envelope ✓
  • (2) --json=name,plugin skills list narrows to those keys ✓
  • (3) Unknown field → exit 2 with sorted Available fields:
  • (4) --json --jq '.data.skills | map(.name)' pipes through jq ✓
  • (5) --jq without --json rejected ✓
  • (6) --agent-help "skills list" advertises json_fields

CLAUDE.md compliance

  • Conventional commit ✓
  • No Co-Authored-By
  • Test coverage essentially absent for new behavior — see below.

Substantive concerns

1. Scope: only skillsListMeta actually declares jsonFields.

The issue's implementation notes are explicit: "Populate every existing meta in src/cli/metadata/*.ts from its existing outputSchema." The PR adds jsonFields: ['name', 'plugin', 'disabled'] to skillsListMeta only. Every other meta (pluginList, workspaceStatus, marketplaceList, etc.) has an outputSchema but no jsonFields. Consequence: for those commands, --json=anything is silently accepted (validation falls through on empty allowlist), so the new contract isn't actually enforced anywhere except skills list.

The gate uses skills list exclusively, so the gate passes, but the feature — a per-command allowlist — is one command short of being real.

2. --template is missing.

The issue calls for --json[=fields] / --jq / --template. The PR title only claims --json + --jq, and the gate doesn't test template, so it's been quietly dropped. Worth either:

  • An explicit "deferred to follow-up" note in the squash commit, or
  • Filing a follow-up issue.

If --template is intentionally cut, fine — but say so somewhere durable.

3. findMetaByCommand lookup is fragile against positional args.

const commandPath = finalArgs.filter((a) => !a.startsWith('-')).join(' ');
const meta = findMetaByCommand(commandPath);

For allagents --json=name,plugin skills add brainstorming, commandPath = "skills add brainstorming". No meta with that exact .command exists, so meta is undefined, so the allowlist falls through — any --json=<field> is accepted. The validation is silently bypassed for any command that takes a positional.

The fix is a longest-prefix match: walk finalArgs token-by-token and find the meta whose .command is the longest prefix.

4. runJq shells out to system jq.

const result = spawnSync('jq', [expr], { input, encoding: 'utf-8' });

The issue's implementation notes explicitly suggest "use a small embedded jq (e.g., node-jq, jq-wasm). Bun's runtime supports both." Shelling out has UX implications:

  • Users without jq on PATH get Error: --jq failed: ENOENT (or similar) on every invocation.
  • macOS / Windows defaults often don't ship jq.

Either:

  • Switch to embedded jq (issue's recommendation), or
  • Detect missing jq on PATH up-front and emit a friendly "Install jq from https://… to use --jq" message.

The gate passes because the test machine has jq installed.

5. Zero unit tests for the new behavior.

CLAUDE.md: "one test per distinct code path." New surface area without tests:

  • extractJsonFlag — three branches (--json, --json=fields, neither)
  • extractJqFlag — present / missing / missing-value-error
  • validateJsonFields — no allowlist / unknown field / valid fields
  • applyFieldFilter — single-top-level-array vs top-level-object data shapes
  • projectFields — drops unknown keys
  • findMetaByCommand — match / no-match (and the positional-args bug above)
  • runJq — success and failure paths (can be done with a tiny shell script stub or by injecting a runner)

The verification gate is an E2E test; unit-level pinning of these branches is missing.


Minor

6. applyFieldFilter's keys.length >= 1 check is redundant — find() already returns the first matching key.

7. setJsonMode now takes an options object. The two existing call sites (only index.ts in this diff) are updated; module-level singleton state is still mutated. Consider whether the lifecycle is robust if setJsonMode is ever called after jsonOutput runs. (Probably fine for a CLI, but worth a comment that the contract is "call setJsonMode exactly once at startup.")

8. Carries the #371 metadata-wiring fix and the addPlugin workspacePath fix. Overlaps with #379 and #382. Whichever lands second rebases cleanly since the changes are identical, but worth noting in the PR description so reviewers don't re-litigate them.


Suggested merge order

Approve after items 1–3 are either addressed or explicitly deferred. Item 4 (embedded jq) and item 5 (tests) can be follow-ups, but the partial allowlist (item 1) and the positional-arg lookup bug (item 3) reduce the feature's real-world utility today.

Adds the gh-style `--json[=field1,field2] --jq <expr>` contract on top of the
existing boolean `--json` flag.

- `AgentCommandMeta.jsonFields` declares the allowlist per command.
- `extractJsonFlag` parses both `--json` and `--json=<fields>` forms; the
  comma-list is validated against the meta's allowlist in index.ts at parse
  time. Unknown field exits with `Unknown JSON field` + a sorted
  "Available fields:" suggestion list and status 2.
- `extractJqFlag` strips `--jq <expr>` before cmd-ts sees it. `--jq` requires
  `--json` (rejected with status 2 otherwise).
- `jsonOutput` applies the field projection on the envelope: when `data` has
  a single top-level array of objects, the allowlist filters each item;
  otherwise it projects the top-level data object directly.
- `--jq` pipes the (post-projection) envelope through the system `jq`
  binary via `spawnSync`. Failures surface as `Error: --jq failed: <msg>`,
  not a raw stderr dump.
- `agent-help.formatForAgent` now emits `json_fields` when the meta declares
  one, so agents can discover the allowlist via `--agent-help`.
- `findMetaByCommand` exported from agent-help so index.ts can do the lookup
  for validation.
- Skills list meta now declares `jsonFields: ['name','plugin','disabled']`.

Plumbed through two pre-existing pieces:
- Carries the #371 fix (skills metas wired into agent-help; correct
  `command:` labels) since the new validation depends on the meta path
  matching the runtime command.
- Fixes `addPlugin` and `getAllSkillsFromPlugins` so they pass `workspacePath`
  through to `resolvePluginSpecWithAutoRegister`. Without it, project-scope
  marketplaces are invisible to the resolver and the verification gate's
  install fails.

Verified end-to-end against `obra/superpowers`:
- Boolean `--json skills list` returns the full envelope.
- `--json=name,plugin skills list` narrows `.data.skills[*]` to those keys.
- `--json=bogus skills list` exits 2 with `Unknown JSON field` + sorted list.
- `--json --jq '.data.skills | map(.name)' skills list` pipes through jq.
- `--jq` without `--json` exits 2.
- `--agent-help "skills list"` exposes `json_fields` as an array.

Closes #376
@christso christso force-pushed the feat/376-json-allowlist branch from 2f516d3 to e6299d9 Compare May 12, 2026 14:13
@christso christso merged commit cf827ba into main May 12, 2026
1 check passed
@christso christso deleted the feat/376-json-allowlist branch May 12, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: --json field allowlist with --jq / --template filters per command

1 participant