playwright bot#14
Closed
JacobCoffee wants to merge 64 commits into
Closed
Conversation
Member
Author
|
i think i have addressed or fix all comments, please recheck when you get time |
sethmlarson
approved these changes
Nov 25, 2025
sethmlarson
left a comment
sethmlarson
left a comment
Collaborator
There was a problem hiding this comment.
Had two comments, otherwise LGTM.
When timestamp extraction fails, raise ValueError instead of silently returning datetime.now(). This ensures scraping failures are surfaced rather than producing potentially incorrect data. Addresses review feedback from @sethmlarson.
StanFromIreland
left a comment
StanFromIreland
left a comment
Member
There was a problem hiding this comment.
I went through the non-demo non-test things, and it looks really great! Some little comments, mostly nits:-)
| git config user.email "bot@python.org" | ||
| git add state.json | ||
| git diff --quiet || git commit -m "Update comment processing state [skip ci]" | ||
| git push || true No newline at end of file |
Member
There was a problem hiding this comment.
This is just speculation, but might this introduce a race condition?
| @@ -0,0 +1 @@ | |||
| """Scripts for PSRT GHSA bot.""" | |||
Member
There was a problem hiding this comment.
Suggested change
| """Scripts for PSRT GHSA bot.""" | |
| """Scripts for the PSRT GHSA bot.""" |
| - python/psrt team membership TODO: make this configurable, and allow list of org/teams? | ||
| like, what if we want PSRT to be able to responds across all PSF repos? (psf, python, pycon, pypi?) | ||
| or maybe we just say "team is $TEAM, and this bot works in $ORG as long as you are member of | ||
| that $TEAM" so we leave the user mgmt to the org admins. yeah.. probably that. |
Member
There was a problem hiding this comment.
Suggested change
| that $TEAM" so we leave the user mgmt to the org admins. yeah.. probably that. | |
| that $TEAM" so we leave the user management to the org admins. |
| ghsa_id: GHSA identifier | ||
|
|
||
| Returns: | ||
| AuthorizationResult indicating if user is authorized and a rason |
Member
There was a problem hiding this comment.
Suggested change
| AuthorizationResult indicating if user is authorized and a rason | |
| AuthorizationResult indicating if user is authorized and a reason |
| @@ -0,0 +1,312 @@ | |||
| """Command execution engine for PSRT GHSA Bot. | |||
|
|
|||
| TODO: Maybe we should look into easily extensiblke commands | |||
Member
There was a problem hiding this comment.
Suggested change
| TODO: Maybe we should look into easily extensiblke commands | |
| TODO: Maybe we should look into easily extensible commands |
Member
There was a problem hiding this comment.
This would make more sense in the repo root, no?
| - If Playwright action runs before cron action, should we have playwright kick it off | ||
| so that the right groups are assigned? | ||
|
|
||
| - When someone duplicate a coammand we shouldnt run it twice: |
Member
There was a problem hiding this comment.
Suggested change
| - When someone duplicate a coammand we shouldnt run it twice: | |
| - When someone duplicate a command we shouldn’t run it twice: |
| ], | ||
| ) | ||
|
|
||
| # set these higher so they arent noiys.. |
Member
There was a problem hiding this comment.
Suggested change
| # set these higher so they arent noiys.. | |
| # set these higher so they aren’t noisy.. |
| dependencies = [ | ||
| "cvelib>=1.4.0", | ||
| "githubkit[auth-app]>=0.13.5", | ||
| # we might could put this into a dep group to not load it every time we install |
Member
There was a problem hiding this comment.
Suggested change
| # we might could put this into a dep group to not load it every time we install | |
| # We could put this into a dep group to not load it every time we install |
Member
There was a problem hiding this comment.
This should be cleared, I don't think we need the state for jolt-org...?
Collaborator
|
Since this repository may be moving to be public soon I'm going to start closing some PRs/issues that are no longer necessary (or hopefully won't be if GitHub does deliver new GHSA features). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Base playwright:
Closes #9, Closes #3, Closes #4, Closes #7