#199 test fix for uploaded files#201
Conversation
The only additions are: - EncryptKeycode() in encrypt.go (PGP-armored keycode encryption) - PublicKey, PublicKeysResponse, Recipient types in misc.go - GetPublicKeys(), UploadKeycode(), UploadKeycodes() in package.go (all using packageCode in URLs) - The UploadKeycodes() call in the upload flow before FinalizePackage()
| fmt.Printf("File uploads complete\n") | ||
|
|
||
| fmt.Println("Uploading keycodes...") | ||
| if err := p.UploadKeycodes(); err != nil { |
There was a problem hiding this comment.
Not a blocker: wondering whether a keycode upload failure should be fatal here. If SendSafely has no PGP public keys configured for this package (empty PublicKeys slice), UploadKeycodes returns nil and we proceed to finalize. That seems right. But if there are recipients and all keycodes fail, we abort before FinalizePackage. Is that the intended behavior, or would it be better to finalize anyway and let the caller decide? I don't know the SendSafely semantics well enough to say.
There was a problem hiding this comment.
Fixed in f36be86: keycode upload failures are now non-fatal. We log a warning and still proceed to FinalizePackage(), since the recipient keycode link is a best-effort notification and its failure doesn't affect the already-uploaded file data. This also avoids the previous behavior of aborting entirely when only one of several recipients' keycodes failed to encrypt/upload.
| if _, err := io.WriteString(w, keyCode); err != nil { | ||
| return "", fmt.Errorf("failed to write keycode: %w", err) | ||
| } | ||
| w.Close() |
There was a problem hiding this comment.
w.Close() and armorWriter.Close() (line 92) return errors that are dropped here. In practice these write to a bytes.Buffer which cannot fail, so this is probably safe. The existing encrypt() above does the same thing. Would it make sense to check both and return an error, at least for defensive consistency? Or is the pattern intentional given the buffer-backed writer?
There was a problem hiding this comment.
Fixed in f36be86: both w.Close() and armorWriter.Close() errors are now checked and returned instead of being dropped, for defensive consistency.
eugeneckim
left a comment
eugeneckim
left a comment
There was a problem hiding this comment.
go_debug.txt (763 lines) and yb-support-tool (binary, 13 MB) are both included in the commit. Neither should be version-controlled. The debug file ends with a DENIED response from the keycode upload endpoint — is there a successful end-to-end run somewhere that confirms the full flow works? Happy to be corrected if the package expiry explains the DENIED.
…oad failure, check Close() errors in EncryptKeycode, untrack debug/binary files - root.go: keycode upload failures are now non-fatal so we still finalize the package, since the recipient link is best-effort and unrelated to the already-uploaded file data - encrypt.go: check errors from w.Close() and armorWriter.Close() in EncryptKeycode instead of dropping them - Remove accidentally committed yb-support-tool/go_debug.txt and yb-support-tool/yb-support-tool binary, add .gitignore to prevent recurrence Co-authored-by: Cursor <cursoragent@cursor.com>
|
Re: the review noting On the DENIED response in that debug log: I removed the file rather than dig through it, so I can't personally confirm whether that reflects package expiry or an unverified end-to-end flow. @eugeneckim can you confirm whether a full successful run has been done since, or should we hold off merging until that's verified? |
|
Update: ran a live end-to-end test of the upload flow from this branch (build f36be86) against production ( So the full flow works end-to-end on this branch. That said, this doesn't fully rule out the original DENIED response you saw, it's possible that was tied to a specific package/key state that this run didn't hit. But the happy path is confirmed working. |
2 participants
The only additions are: