Add create run logs#68
Conversation
|
Claude finished @ramilamparo's task in 1m 17s —— View job Review: PR #68 — Add create run logs
Result: Approved — clean, well-structured addition that follows all established codebase patterns. No security, performance, or code quality issues found. One minor inline suggestion about adding a |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
AndrianBdn
left a comment
AndrianBdn
left a comment
There was a problem hiding this comment.
I think it would be great to explain what is run log - it is term we coined ourself, there is no alternatives as far as I know. AI system / user could confuse this with something else, so both in SKILL.md / README.md
Clarify what run-level logs are used for in --help, README, and SKILL.md command tree.
|
Claude finished @ramilamparo's task in 2m 3s —— View job Review: PR #68 — Add create run logs
Result: Approved — clean, well-structured addition that follows all established codebase patterns. No security, performance, or code quality issues found. The new commit addresses @AndrianBdn's feedback by adding a clear explanation of what run-level logs are in both README.md and SKILL.md. One minor observation (not blocking) noted in the review about |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Inline runs API methods into the returned object literal and drop unused long-form aliases (createRun, listRuns, etc.) to match the pattern used by folders/tcases. - Require non-empty comment in CreateRunLogRequestSchema and add tests covering both --comment and --body paths.
|
Claude finished @ramilamparo's task in 5m 3s —— View job Review: PR #68 — Add create run logs
Result: Approved — clean, well-structured addition that follows all established codebase patterns. The latest commit addresses prior feedback ( |
There was a problem hiding this comment.
Clean, well-structured addition that follows all established codebase patterns. The latest commit addresses the prior .min(1) and documentation feedback nicely.
Review summary:
- Code quality: Good. One minor nit about
import typeforCreateRunLogRequest. The refactor from named functions to inline object methods is clean and removes the unnecessary aliases. - Security: No vulnerabilities. Path params are Zod-validated, auth uses the standard middleware, body is JSON-serialized safely. HTML in
--commentis by design (server-side rendering responsibility). - Performance: No concerns — single POST request with O(1) work.
- Test coverage: Adequate — covers happy path, body input variants, validation errors (including the new empty-comment rejection), and a live integration test. Three minor suggestions posted inline.
| QueryPlansSchema, | ||
| CloneRunRequestSchema, | ||
| type ListRunTCasesRequest, | ||
| CreateRunLogRequest, |
There was a problem hiding this comment.
Nit: CreateRunLogRequest is only used as a type annotation (body as CreateRunLogRequest), so it should be type CreateRunLogRequest to match the existing pattern on line 7 (type ListRunTCasesRequest).
Alternatively, you could use body as Parameters<typeof api.runs.createLog>[2] like the create and clone endpoints do, which would make this import unnecessary entirely.
| test('rejects empty --comment', async () => { | ||
| const exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => { | ||
| throw new Error('process.exit') | ||
| }) | ||
| const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) | ||
| try { | ||
| await expect( | ||
| runCommand('--project-code', 'PRJ', '--run-id', '1', '--comment', '') | ||
| ).rejects.toThrow('process.exit') | ||
| const errorOutput = errorSpy.mock.calls.map((c) => c.join(' ')).join('\n') | ||
| expect(errorOutput).toMatch(/--comment.*must not be empty/) | ||
| } finally { | ||
| exitSpy.mockRestore() | ||
| errorSpy.mockRestore() | ||
| } | ||
| }) |
There was a problem hiding this comment.
Nit: This manually reimplements the expectValidationError pattern (spy on process.exit, spy on console.error, assert, restore) that's already imported and used just above. Could simplify to:
test('rejects empty --comment', async () => {
await expectValidationError(
() => runCommand('--project-code', 'PRJ', '--run-id', '1', '--comment', ''),
/--comment.*must not be empty/
)
})| @@ -0,0 +1,130 @@ | |||
| import { HttpResponse, http, type PathParams } from 'msw' | |||
There was a problem hiding this comment.
Nit (repeat from prior review — still unaddressed): The ['runs', 'logs', 'create'] command path introduces runs logs as a new intermediate subcommand group. Consider adding a corresponding entry in missing-subcommand-help.spec.ts:
{ args: ['api', 'runs', 'logs'], label: '`api runs logs`' },Minor — the builder handles this generically, so it's unlikely to break, but it would keep the help-text test coverage consistent with other intermediate groups.
2 participants
No description provided.