Skip to content

fix(submit): report failure when registry rejects submission - #70

Merged
knoxgraeme merged 1 commit into
mainfrom
claude/elegant-curie-1SYr6
Jun 7, 2026
Merged

knoxgraeme merged 1 commit into
mainfrom
claude/elegant-curie-1SYr6

Conversation

@knoxgraeme

Copy link
Copy Markdown
Owner

Summary

Fixes #68

When submitSkillsToRegistry returns failed submissions (e.g. HTTP 429 non-JSON from the registry), the JSON output incorrectly reported success: true with exit_code: 0. Automation relying on these signals would silently miss failed submissions.

Root cause (src/commands/submit.ts): the else branch that pushes to failed[] never set jsonOutput.success = false, and the JSON exit always called outputJsonAndExit(EXIT_CODES.SUCCESS) unconditionally.

Changes:

  • Set jsonOutput.success = false when any submission lands in failed[]
  • Exit with EXIT_CODES.NETWORK_ERROR (3) in both JSON and TTY modes when the registry rejects the submission
  • Add src/__tests__/submit.test.ts covering argument validation and the JSON failure contract

Before:

{ "success": true, "failed": [{ "skill": "...", "reason": "Registry returned non-JSON response (HTTP 429): ..." }], "exit_code": 0 }

After:

{ "success": false, "failed": [{ "skill": "...", "reason": "Registry returned non-JSON response (HTTP 429): ..." }], "exit_code": 3 }

Test plan

  • All 7 new submit.test.ts tests pass
  • success: false + non-zero exit_code for invalid args in JSON mode
  • Non-zero exit for non-existent repo (covers failure path before registry)
  • Existing test suite unaffected

https://claude.ai/code/session_01CogjFAAGo379Vv3dPLPkzP


Generated by Claude Code

When `submitSkillsToRegistry` returns failed submissions (e.g. HTTP 429
non-JSON response from the registry), the JSON output incorrectly reported
`success: true` with `exit_code: 0`. Automation relying on these signals
would incorrectly treat the submission as successful.

- Set `jsonOutput.success = false` when any submission lands in `failed[]`
- Exit with `EXIT_CODES.NETWORK_ERROR` (3) in both JSON and TTY modes when
  the registry rejects the submission
- Add `submit.test.ts` covering argument validation and the JSON failure contract

Fixes #68

https://claude.ai/code/session_01CogjFAAGo379Vv3dPLPkzP

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 06039a0602

ℹ️ 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".

Comment on lines +53 to +58
const { stdout, exitCode } = invokeCli([
'--json',
'submit',
'skillfish-test-nonexistent-owner-xyz/nonexistent-repo-xyz',
'--yes',
]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid live GitHub calls in submit tests

This test invokes the real CLI with a syntactically valid repo, so it proceeds into discovery and hits GitHub (fetchDefaultBranch calls https://api.github.com/repos/...). In CI runs without network, behind a proxy, or when GitHub rate-limits the runner, the test can fail or time out even though the JSON failure contract is unchanged; mock the discovery/registry path instead of depending on a live nonexistent repository.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

The concern is valid, but the existing test infrastructure (invokeCli via execFileSync) spawns a separate process, so vi.mock can't intercept network calls made inside it. Adding proper mocking would require a different test architecture (e.g. an in-process test runner or an HTTP interceptor like MSW). The same live-call pattern already exists in add.test.ts (line 33–38). Happy to track adding proper test mocking infrastructure as a follow-up improvement rather than blocking this fix.


Generated by Claude Code

@caioribeiroclw-pixel

Copy link
Copy Markdown

Thanks — this covers the failure semantics I hit in #68 from the reporter side.

I checked this branch locally and the important behavior is now aligned with what agents/CI need: JSON mode does not report a failed submission as success: true, and the command exits non-zero when the submission path fails.

Validation I ran on 06039a0:

  • npm run typecheck
  • npm test → 242/242 passing
  • npm run build
  • git diff --check

I’m going to close my duplicate PR #69 so review can converge here.

@knoxgraeme
knoxgraeme merged commit d8b9e28 into main Jun 7, 2026
6 checks passed
@knoxgraeme
knoxgraeme deleted the claude/elegant-curie-1SYr6 branch June 7, 2026 02:46
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.

submit --json reports success on registry 429 failure

3 participants