Conversation
Add structured JSON output support for agent and CI/CD consumption: Phase 1 (mutating): sync, install, update, uninstall, collect Phase 2 (read-only): target list, status, diff Key design decisions: - Per-command JSON schema (no universal envelope) - Shared helper: writeJSON() with recursive nil slice→[] conversion - --json implies --force for mutating commands (skip prompts) - TUI/spinners suppressed; progress goes to stderr, JSON to stdout - 10 integration tests covering valid JSON, dry-run, nil slices Also includes post-review cleanup: - ensureEmptySlices now recurses into nested structs/slices - Unified mode resolution via getTargetMode() (removed 4 duplicates) - sync --json now creates backups (behavioral parity with non-JSON) - targetList no longer double-parses --json flag
- Replace hasFlag loop with slices.Contains (stdlib) - Deduplicate checkRepoDirty → delegate to isRepoDirty - Add nil-receiver guard to diffProgress.stop() for consistency - Return explicit error for status --json in project mode - Use consistent error message in sync --json source-not-found path
- Expand description from 160 to 708 chars with broader trigger keywords (targets, backup/restore, trash, hub, symlink/copy, include/exclude filters) - Add Skill Hubs recipe section (hub add/list/remove/default/index) - Add Scripting & CI/CD recipe section (12 --json commands) - Add --json column to Quick Lookup table - Fix backup row: correctly show ✗ for project mode (global-only) - Split tui/ui rows: tui/upgrade are ✗, ui supports -p - Add hub to command list - Update AI Caller Rules: hardcoded secret detection, --no-tui commands, 12 --json commands - Add --json flag docs to reference files (install, update, uninstall, status, diff, sync, collect) Eval results: 100% pass rate across 5 test cases (up from 80% in iteration 1). Key fix: Quick Lookup backup ✗ resolved false project-mode claims.
Runbooks can now leverage structured JSON output for verification instead of brittle text matching. Adds --json quick reference table (12 commands), jq assertion patterns, and updated checklist/templates.
Structured JSON output for 8 commands (sync, install, update, uninstall, collect, target list, status, diff), bringing total --json coverage to all 12 major CLI commands.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the command-line interface by integrating structured JSON output across a wider range of commands. The primary goal is to facilitate automation and integration with other tools, such as Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a --json flag to 8 CLI commands, providing structured, machine-readable output for automation and CI/CD pipelines. The implementation is extensive, touching many command files and adding new JSON helper functions and integration tests. My review focuses on improving code maintainability by reducing duplication in several command files where logic for JSON and non-JSON output is separated. I've also identified a high-severity issue where some commands still write non-JSON text to stdout in JSON mode, which is being masked by a workaround in the new tests. Additionally, I've suggested moving a shared utility function to a more appropriate location and improving error handling in the new JSON output helper. Overall, this is a great feature addition, and addressing these points will make the implementation more robust and maintainable.
Note: Security Review did not run due to the size of the PR.
Address code review feedback from PR #67: - Move all dry-run diagnostic messages (internal/sync) from stdout to stderr so they don't pollute JSON output - Suppress UI output (spinners, headers, warnings) in uninstall --json mode by adding jsonOutput guards across all phases - Return nil after writeJSONError to prevent dual output (JSON + plain text error) in install and sync commands - Handle json.MarshalIndent error in writeJSONError with stderr fallback - Merge duplicated discovery logic (JSON vs non-JSON branches) in sync, sync_project, diff, and collect using nullable spinner pattern - Remove checkRepoDirty alias in status.go, call isRepoDirty directly - Remove extractJSON test workaround — tests now assert stdout is valid JSON directly, enforcing the clean-stdout contract
writeJSONError now returns a jsonSilentError sentinel that main() recognises — it exits with code 1 without printing plain-text output to stdout. This fixes a regression where --json errors (config load failure, missing source dir, discovery failure) returned exit 0. All writeJSONError call sites changed from the broken pattern: writeJSONError(err); return nil // exit 0, wrong to: return writeJSONError(err) // exit 1, correct Also applied the same fix to update.go error paths which had the older pattern of writeJSONError(err) followed by return err (which would print both JSON and plain text). Added regression tests for sync and install error paths verifying both non-zero exit code and valid JSON error output on stdout.
Type assertion would break if the error gets wrapped via fmt.Errorf with %w. errors.As handles wrapped errors correctly.
The --json addition accidentally removed the guard that rejects unexpected positional arguments in project mode. Restore it so `skillshare status --project foo` correctly returns an error instead of silently ignoring the extra argument.
install --json: logo, spinners, and step output leaked to stdout, corrupting machine-parseable JSON. Add suppressUIToStderr() around the entire install flow, restoring stdout before writing JSON. diff --project --json: spinner and progress display polluted stdout. Skip spinner and progress creation in JSON mode, matching the existing cmdDiffGlobal pattern. uninstall --json: five error paths (discovery failure, no skills found, glob detection, validation, preflight) returned plain-text errors instead of JSON error envelopes. Wrap each with writeJSONError(). Add 8 integration tests (5 new failure cases + 3 confirming existing behavior) with an assertPureJSON helper that validates stdout contains only valid JSON with no leading/trailing noise.
… --json suppressUIToStderr was redirecting UI to stderr, but in environments where stdout and stderr share the same terminal (e.g. docker exec), all the spinner/progress/header output was still visible alongside the JSON. Replace with suppressUIToDevnull which opens os.DevNull for both os.Stdout and ui.ProgressWriter, producing zero visible noise in --json mode. Also add status --json support for project mode, reusing the same JSON schema as global mode (source, skill_count, tracked_repos, targets, audit, version).
When no explicit analyzer filter was configured, status --json reported an empty analyzers array. Now ResolvePolicy backfills the full list of built-in analyzer IDs so the JSON output reflects what actually runs. Add Registry.IDs() helper with deduplication (markdownLinkAnalyzer and staticAnalyzer share the "static" ID).
updateTrackedRepo and updateRegularSkill now return (updateResult, error) instead of just error, so callers (including project mode and JSON output) can accurately report updated/skipped/securityFailed/pruned counts. Adapt cmdUpdateProject, cmdUpdateProjectBatch, updateAllProjectSkills, and list_project.go TUI action to the new signatures.
collect --json and sync --json returned the raw error after writing JSON, causing main() to print a duplicate plain-text error. Wrap with jsonSilentError so main() exits non-zero silently. Also fix sync --json skipping backup output that would corrupt stdout.
- diff.go: align JSON struct field tags - CHANGELOG: fix jq example (.errors → .details) - analyzer_metadata: align map literal and test struct formatting
…semantics - Extract writeJSONResult() to replace 5 identical write+silent-error trailers - Replace local isRepoDirty() with existing git.IsDirty() (8 call sites) - Use defer for suppressUIToDevnull restore instead of manual calls at each return site in install.go and update.go (prevents stdout leak on new paths) - Fix ResolvePolicy always filling EnabledAnalyzers, which broke the nil-means-all contract for ForPolicy; add EffectiveAnalyzers() for display - Simplify collectOutputJSON double map allocation - Remove unnecessary string conversion in writeJSONError
Extract buildTrackedRepoJSON() that runs git.IsDirty concurrently via goroutines + WaitGroup instead of sequential subprocess spawns per repo. Also deduplicates the tracked-repo JSON loop between global and project status paths, and optimizes skill counting to a single pass with a map.
JSON mode stdout was already pure JSON, but stderr still emitted progress lines (e.g. "Discovering skills...", "Found N local skill(s)") which broke `2>&1 | jq .` pipelines. Remove the else-branch stderr writes so --json mode is fully silent except for stdout JSON output.
Keep sync dry-run diagnostics on stdout by default to avoid non-JSON behavior regressions. Introduce sync.DiagOutput so JSON mode can silence diagnostics via io.Discard at command entry and restore after execution. Update search JSON integration assertion to match fully silent progress behavior.
Address code review feedback from PR #67: - Move all dry-run diagnostic messages (internal/sync) from stdout to stderr so they don't pollute JSON output - Suppress UI output (spinners, headers, warnings) in uninstall --json mode by adding jsonOutput guards across all phases - Return nil after writeJSONError to prevent dual output (JSON + plain text error) in install and sync commands - Handle json.MarshalIndent error in writeJSONError with stderr fallback - Merge duplicated discovery logic (JSON vs non-JSON branches) in sync, sync_project, diff, and collect using nullable spinner pattern - Remove checkRepoDirty alias in status.go, call isRepoDirty directly - Remove extractJSON test workaround — tests now assert stdout is valid JSON directly, enforcing the clean-stdout contract
1 participant
v0.16.12 adds structured JSON output to 8 more commands, bringing total
--jsoncoverage to all 12 major CLI commands. Every command now has a machine-readable output mode for agent consumption and CI/CD pipelines:--jsoncommands — sync, install, update, uninstall, collect, target list, status, diff--jsonon mutating commands implies--force(skips prompts)No breaking changes. Drop-in upgrade from v0.16.11.