Conversation
|
👋 patrickhuie19, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
erikburt
left a comment
erikburt
left a comment
There was a problem hiding this comment.
I’ve left more detailed comments inline, but at this size it’s difficult to fully vet in a single pass.
Some higher-level feedback:
- I think this probably belongs in its own repo. It’s not just a single action, it’s a collection of related actions and workflows, and because of that, seems like a suite/product in itself. Having it's own repository would make ownership and versioning a lot clearer. Can expand on this further if needed.
- I’m happy to help set up a new repo.
- If it must live in this repository, then all the code should live in the top-level
apps/medicdirectory probably, rather than.github/actions/medic.
- You're most of the way there, but each major medic feature should likely be its own independently versioned action, with shared functionality pulled into a common library.
- For example comment-parser, doesn't have an action.yml but is does have it's own
dist.libis not an actual lib, it's a shared folder. Although, probably not big enough to warrant it being a separate library yet, still helps with organization imo.
- For example comment-parser, doesn't have an action.yml but is does have it's own
- This kind of goes with the above, but right now, the code is tough to reason about and maintain. I think it would benefit from more structural cleanup. Clearer boundaries between individual functionality, tests utilities, common functions, etc.
- Documentation would be helpful as well
| - name: Checkout medic | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: smartcontractkit/medic-test | ||
| path: _medic | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20' | ||
|
|
||
| - name: Parse comment | ||
| id: parse | ||
| run: node _medic/.github/actions/medic/dist/comment-parser/index.js | ||
| env: | ||
| INPUT_GITHUB-TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| INPUT_COMMENT-BODY: ${{ github.event.comment.body }} | ||
| INPUT_ISSUE-NUMBER: ${{ github.event.issue.number }} | ||
| INPUT_COMMENTER-LOGIN: ${{ github.event.comment.user.login }} | ||
|
|
There was a problem hiding this comment.
Seems like you properly split up the other functions/actions into their own action.ymls, but this one should be as well.
| id: check | ||
| run: | | ||
| echo "Found /medic generate conflict command" | ||
| echo "should_generate=true" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
I recommend using ... | tee -a "${GITHUB_OUTPUT}" everywhere so you can see what values are actually being set.
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Authenticate to GCP | ||
| uses: google-github-actions/auth@v2 |
| export_environment_variables: true | ||
|
|
||
| - name: Auto retry failed workflow | ||
| uses: smartcontractkit/medic-test/.github/actions/medic/workflow-retry-auto@main |
There was a problem hiding this comment.
ideally this is versioned and tagged, not pointing to main. could add reusable workflows in a later PR.
| - name: Checkout medic | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: smartcontractkit/medic-test | ||
| path: _medic |
There was a problem hiding this comment.
Using old repo all over the place. And based on below comment, this shouldn't be necesary.
| const AnthropicVertex = mod.default; | ||
| client = new AnthropicVertex({ | ||
| projectId, | ||
| region: process.env.CLOUD_ML_REGION || 'us-east5', |
There was a problem hiding this comment.
Ideally this is an input, mixing random env vars and inputs can be confusing (same with ANTHROPIC_VERTEX_PROJECT_ID) above.
|
|
||
| try { | ||
| const response = await octokit.request( | ||
| 'GET /repos/{owner}/{repo}/actions/jobs/{job_id}/logs', |
There was a problem hiding this comment.
Why not use octokit.rest.actions.downloadJobLogsForWorkflowRun. It also accepts the request object afaict.
| const execPromise = exec.exec( | ||
| 'claude', | ||
| ['--dangerously-skip-permissions', '-p', prompt, '--output-format', 'stream-json', '--verbose'], | ||
| { |
There was a problem hiding this comment.
This technically leaks credentials in 2 ways.
This exec call inherits env vars, so INPUT_GITHUB-TOKEN is leaked to Claude. Also when using actions/checkout in a workflow, credentials are also saved to the repo unless the action is called with persist-credentials: false.
| @@ -0,0 +1,292 @@ | |||
| /** | |||
There was a problem hiding this comment.
I think files like this should probably live in __tests__ or in a e2e folder of some sort.
| import { PROMPT_TEMPLATE } from './lib/prompts.js'; | ||
|
|
||
| process.stdout.write(PROMPT_TEMPLATE); |
2 participants
Bringing https://github.com/smartcontractkit/medic-test/pulls to this repo. Next step is to use the reusable workflows in core.