feat: check release tag and assets for license in LE-03.02 - #468
feat: check release tag and assets for license in LE-03.02#468Satarupa22-SD wants to merge 5 commits into
Conversation
Signed-off-by: Satarupa22-SD <satarupa2212@gmail.com>
|
Moving to draft just while checks are failing, feel free to mark it as ready for review when the CI goes green |
- data/payload.go: use promoted LicenseAtRef selector (staticcheck QF1008) - steps_test.go: discard fmt.Fprintf return in test stub (errcheck) Signed-off-by: Eddie Knight <knight@linux.com>
jmeridth
left a comment
jmeridth
left a comment
There was a problem hiding this comment.
🤖 (summary) This is a well-built PR in the places that are hardest to get right: the data layer distinguishes 404 from error, the caching is deliberate and commented, the tests are genuinely load-bearing (we mutation-tested them), and the merge base is current. The findings are all in the verdict ladder, and the top one is the interesting kind: a behavior your own tests pin correctly but that punishes the control's recommended practice. Four models reviewed this; the inline comments are the survivors after cross-checking, several confirmed against the live GitHub API.
| return gemara.Failed, fmt.Sprintf("A license exists on the default branch, but none was found in the released source code at tag %q", latest.TagName), gemara.High | ||
| } | ||
| return gemara.Failed, fmt.Sprintf("No license was found in the released source code at tag %q", latest.TagName), gemara.High | ||
| default: |
There was a problem hiding this comment.
🤖 (medium) A failed tag lookup lands here, logs a Warn, and falls through to Passed on default-branch evidence, which the comment above rightly calls untrustworthy. GitHub signals rate-limit exhaustion with 403 (not 429), retry.go treats 403 as permanent, and the error is cached, so a rate-limited bulk scan disables exactly the tamper check issue #70 asks for and reports Passed. The failed /releases fetch above correctly yields NeedsReview; the same class of failure should get the same disposition here.
There was a problem hiding this comment.
🤖 Partially fixed at b1c332c: the 403 path now correctly lands NeedsReview (mutation-checked), and a tag failure with a license asset present does too. But a generic failure (say a 500, which isTransientError also does not retry) still logs a warning and falls through to Passed/Medium on the HEAD evidence, and steps_test.go pins that. The inconsistency this comment flagged remains for that class: a failed /releases fetch gets NeedsReview while a failed tag lookup gets Passed. Is the generic-failure Passed deliberate? If not, aligning it is a one-line disposition change plus the test expectation. Minor related nit: every 403 is messaged as "rate limited", including permission-denied; the disposition is right either way, just the wording.
3 participants
fixes #70
CC: @jmeridth