Conversation
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 introduces a new Highlights
Ignored Files
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. Footnotes
|
- Add Zod schemas at API level for all request types with validateRequest() that throws RequestValidationError, caught by apiHandler for clean error output - Add handleValidationError() + buildArgumentMap() to map API field names to CLI argument names (e.g., title → --title) for user-friendly error messages - Move validation schemas from command-level to API-level as single source of truth; command handlers pass args directly to API functions - Add customFields, parameterValues, and filledTCaseTitleSuffixParams to test-case create/update commands with proper validation (parameterValues restricted to template type) - Fix jsonResponse null crash, expand ResultStatus with custom statuses, relax test-cases update check, extract shared superRefine for run schemas - Add tests for new fields (mocked + live) and batch-create validation errors
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e484fb2a50
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/api/run.ts
Outdated
There was a problem hiding this comment.
Renamed the file to runs.ts to keep it uniform with the others.
skills/SKILL.md
Outdated
There was a problem hiding this comment.
New skill can be added with npx skills add Hypersequent/qas-cli
| - name: Run live API tests | ||
| env: | ||
| QAS_TEST_URL: ${{ secrets.QAS_TEST_URL }} | ||
| QAS_TEST_TOKEN: ${{ secrets.QAS_TEST_TOKEN }} | ||
| QAS_TEST_USERNAME: ${{ secrets.QAS_TEST_USERNAME }} | ||
| QAS_TEST_PASSWORD: ${{ secrets.QAS_TEST_PASSWORD }} | ||
| QAS_DEV_AUTH: ${{ secrets.QAS_DEV_AUTH }} | ||
| run: npm run test:live |
There was a problem hiding this comment.
Live tests are added that allows running automated tests with a real live qasphere instance. QAS_TEST_USERNAME and QAS_TEST_PASSWORD are required because we don't have a public API yet for deleting projects, which is required for clean-ups after the tests. So we use login credentials and call a non-public API instead.
| "typescript": "^5.8.3", | ||
| "typescript-eslint": "^8.31.1", | ||
| "vitest": "^3.1.2" | ||
| "vitest": "^4.1.0" |
There was a problem hiding this comment.
Updated vitest to allow specifying tags. Which is useful for live tests and excluding them on normal testing.
There was a problem hiding this comment.
I was testing with claude and found some bugs
⏺ Bugs Found
- Trailing slash on QAS_URL breaks all API commands — QAS_URL="https://host.com/" produces double-slash URLs
(https://host.com//api/...) returning 404 HTML. createApi() doesn't strip trailing slashes like
ResultUploadCommandHandler does. - folders bulk-create --folders expects wrong JSON format — Help says '[{"path": ["Parent", "Child"]}]'
(array), but validation requires '{"folders": [{"path": ["Parent", "Child"]}]}' (object wrapper). - test-cases list --include help suggests invalid values — Help example says "tags,requirements" but server
rejects these with a validation error. Valid values are undocumented in the CLI. - runs tcases list --limit silently ignored — Passing --limit 1 still returns all 20 test cases. The CLI
accepts the arg but the API endpoint doesn't paginate. - test-cases list --page 0 crashes with raw Go error — Server requires page >= 1 but CLI doesn't validate.
Error shown: Field validation for 'Page' failed on the 'gt' tag. - settings update-statuses color help/example are wrong — Help says "color": "#FF9800" (hex), but API
requires named colors: blue, gray, red, orange, yellow, green, teal, indigo, purple, pink.
skills/SKILL.md
Outdated
There was a problem hiding this comment.
Reorganize this skill to be more brief.
AI should be able to ask for help on particular command to know more details.
Otherwise, at the very least repeating the headers of the table for each API doesn't seem like a good idea.
Maybe I am wrong about this, and it doesn't matter because of context lengths of new models.
There was a problem hiding this comment.
Updated the command tables with a tree instead. According to claude the skill takes up 1800-2000 tokens.
There was a problem hiding this comment.
Its possible to also move the api files inside the folders to have code that changes frequently lives together.
There was a problem hiding this comment.
I'm not sure what you mean?
README.md
Outdated
| ## Command: `api` | ||
|
|
||
| The `api` command provides direct access to the QA Sphere public API from the command line. Outputting JSON to stdout for easy scripting and piping. | ||
|
|
||
| ``` | ||
| qasphere api <resource> <action> [options] | ||
| ``` | ||
|
|
||
| ### Conventions | ||
|
|
||
| - **JSON output** — All commands print JSON to stdout; errors go to stderr. | ||
| - **`--project-code`** — Specified per-command (not global). Some endpoints (audit-logs, users, settings) are org-scoped and don't need it. | ||
| - **Comma-separated arrays** — Options like `--milestone-ids`, `--tags`, `--priorities`, `--folders` accept comma-separated values: `--tags 1,2,3`. |
There was a problem hiding this comment.
I think this documentation is too dense. We can have detailed documentation on docs.qasphere.com but here we should just write the commands without much explanation because. a lof of it should be self-explanatory.
e.g. qasphere api custom-fields list is going to list custom fields.
Explaining this is not necessary here.
There was a problem hiding this comment.
Replaced the markdown tables with a tree instead.
| export default { | ||
| // Reusable fields shared across commands | ||
| 'project-code': `Project code identifying the QA Sphere project.`, | ||
|
|
||
| list: { | ||
| command: 'List all tags in a project.', | ||
| epilog: apiDocsEpilog('tag', 'list-project-tags'), | ||
| }, |
There was a problem hiding this comment.
Shouldn't the command example also show the project argument?
% ./build/bin/qasphere.js api tags --help
qasphere.js api tags
Manage tags
Commands:
qasphere.js api tags list List all tags in a project.
Options:
-h, --help Show help [boolean]
--version Show version number [boolean]
--verbose Show verbose errors [boolean]
apple@Apples-MacBook-Pro qas-cli % ./build/bin/qasphere.js api tags list
Missing required argument: project-code
There was a problem hiding this comment.
qasphere api tags will just show the available sub commands. The subcommand list is needed to show full argument options qasphere api tags list -h.
Fixed.
Fixed. Argument should now only accept an array.
Updated help text to include possible values.
Looks like the API doesn't pagination --limit. I've removed the limit argument.
Could not reproduce. Possibly you've tested older code?
Fixed |
|
Claude finished @ramilamparo's task in 9m 13s —— View job Reviewing PR #58: Add public API commands
Reviewed the full diff (~420K chars, 100 files) with 4 specialized subagents (code quality, security, performance, test coverage). All 12 previously raised issues remain resolved. The latest 3 commits address reviewer feedback well (specific validators, step schema fix, 1 minor issue posted as inline comment ( 1 still-open thread from prior review: Cleanup: Minimized 13 outdated bot review summaries, deleted 4 progress comments. |
|
Claude finished @ramilamparo's task in 11m 14s —— View job Reviewing PR #58: Add public API commands
Reviewed the full diff (~424K chars, 100 files) with 4 specialized subagents (code quality, security, performance, test coverage). All 12 previously raised issues from prior reviews remain resolved. 1 new finding posted as inline comment: 2 still-open threads from prior reviews:
Cleanup: Minimized 14 outdated bot review summaries. |
|
Claude finished @ramilamparo's task in 4m 21s —— View job Reviewing PR #58: Add public API commands
Reviewed the full diff (~424K chars, 100 files) with 4 specialized subagents. All 12 previously raised issues from prior reviews remain resolved. Posted 2 new inline comments and minimized 15 outdated bot review summaries. New findings:
2 still-open threads from prior reviews:
|
Reviewing PR #58: Add public API commands
|
There was a problem hiding this comment.
Review Summary
Reviewed the full diff (~424K chars, 100 files) with 4 specialized subagents (code quality, security, performance, test coverage). All 12 previously raised issues from prior reviews have been resolved. No new critical, high, or medium issues found.
2 still-open minor threads from prior reviews:
precondition-text/precondition-idmissing fromupdateFields/buildArgumentMapin both test-cases create (line 311) and update (line 433) handlers — affects error message completeness and validation error path mappingfiles uploadmissing file existence check —readFileSyncgives raw Node.jsENOENTinstead of a user-friendly error
Both are low-severity quality improvements, not blockers.
What's been fixed well across the 28 commits:
- Path parameter injection mitigated via
validateResourceId/validateProjectCode/validateIntId ResultStatustype now includes custom statuses- DRY: body-merging extracted to
mergeBodyWithOverrides, query plan validation to sharedvalidateQueryPlans listRunTCasesresponse unwrapping fixedtest-cases updatecheck rewritten for flexibility- Trailing slash on
QAS_URLhandled .int().positive()added to numeric array schemasjsonResponsenull-safe check added
|
Claude finished @ramilamparo's task in 3m 41s —— View job Reviewing PR #58: Add public API commands
Reviewed the full diff (~424K chars, 100 files) with 4 specialized subagents (code quality, security, performance, test coverage). All 12 previously raised issues from prior reviews have been resolved. Posted 2 new inline comments and minimized 16 outdated bot review summaries. New findings:
2 still-open threads from prior reviews:
|
There was a problem hiding this comment.
Review Summary
Reviewed the full diff (~424K chars, 100 files) with 4 specialized subagents (code quality, security, performance, test coverage). All 12 previously raised issues from prior reviews have been resolved.
2 new inline comments posted:
shared-preconditions.ts:26— Return type doesn't handlenullthat the API returns for empty lists (confirmed by live test)runs.ts:11—import typefor type-onlyFolderimport (nit)
2 still-open threads from prior reviews:
precondition-text/precondition-idmissing fromupdateFieldsin test-cases create/update handlers — affects error messages and validation error mappingfiles uploadmissing file existence check —readFileSyncgives raw Node.jsENOENTinstead of user-friendly error
Test coverage observation (non-blocking): Validation error paths remain lightly tested for several commands (settings update-statuses, folders bulk-create, test-plans create, files upload, results create). The runs create and test-cases create specs are good models for coverage.
No security or performance issues found.
| const validated = params ? validateRequest(params, ListSharedPreconditionsRequestSchema) : {} | ||
| return fetcher( | ||
| appendSearchParams(`/api/public/v0/project/${projectCode}/shared-precondition`, validated) | ||
| ).then((r) => jsonResponse<SharedPrecondition[]>(r)) |
There was a problem hiding this comment.
Return type doesn't account for null — live test confirms API returns null for empty lists
The live test at src/tests/api/shared-preconditions/list.spec.ts:49 types the result as SharedPrecondition[] | null and comments "API returns null for empty list". But the API function here types the return as SharedPrecondition[] without handling null.
If the CLI calls printJson(result) on a null return, it prints null instead of []. More importantly, any TypeScript caller trusting the return type will assume it's always an array and could crash on .length, .map(), etc.
Suggested fix:
.then((r) => jsonResponse<SharedPrecondition[] | null>(r))
.then((r) => r ?? [])This is consistent with how other list endpoints handle nullable responses (e.g., projects.list() uses .then((r) => r.projects ?? [])).
| sortOrderParam, | ||
| validateRequest, | ||
| } from './schemas' | ||
| import { Folder } from './folders' |
There was a problem hiding this comment.
Nit: import type for type-only import
Folder is used only as a type annotation (in the RunTCase interface). The project convention is to use import type for type-only imports (e.g., src/tests/api/shared-preconditions/list.spec.ts:3).
import type { Folder } from './folders'
Summary
Adds a new
qasphere apicommand that provides direct CLI access to the full QA Sphere public API. Users can now manage projects, test runs, test cases, results, and other resources programmatically without writing custom API integration code.What's included
projects,runs,test-cases,results,folders,milestones,tags,requirements,shared-steps,shared-preconditions,custom-fields,audit-logs,settings,test-plans,files,users@filenamereferences[0].tcaseIds: not allowed for "live" runsQAS_URL/QAS_TOKENloaded only when the API is called, so CLI validation errors surface firstnpx skills add Hypersequent/qas-cliFolder structure
Other changes
audit-logs,custom-fields,milestones,requirements,results,settings,shared-preconditions,shared-steps,tags,test-plans,users,file— plus expandedruns,tcases,projects,foldersCLAUDE.mdupdated with full architecture docs for the API commandREADME.mdupdated with usage documentationSKILL.mdadded for AI coding agent supportnpm audit fixapplied to dependencies (high vulnerability issues were being reported)Testing
src/tests/api/organized by resource, one spec per action (e.g.,projects/list.spec.ts,runs/create.spec.ts){ tags: ['live'] }test-helper.tswithuseMockServer(),runCli(), and atestfixture that provides project lifecycle managementglobal-setup.ts) handles live API authentication🤖 Generated with Claude Code