Address porting feedback from claude#5031
Merged
Merged
Conversation
WalkthroughAligns Go datetime serialization/parsing with Python pynamodb formats, makes GitHub organization SSRF validation opt-in via a new parity flag, refactors GitHub validation wiring, updates multiple API handlers (signature creation response, manager ACL flow, GitHub-not-found responses), and adds ChangesPynamoDB DateTime Serialization / Parsing
GitHub Validation, Parity Flag, and API Handler Behavior Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Signed-off-by: Łukasz Gryglicki <lgryglicki@cncf.io>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR captures and applies porting/parity fixes identified during an EasyCLA Python→Go audit, focusing on aligning legacy Go (cla-backend-legacy) behavior and DynamoDB serialization with the legacy Python backend, and documenting the audit results.
Changes:
- Updated legacy Go datetime formatting/parsing to match pynamodb’s canonical
...%f%zshape (.ffffff+0000) across writers and parsers. - Adjusted several legacy v1 endpoints for closer Python parity (e.g., POST /v1/signature response shape, GitHub org “not found” status handling, CLA manager add side effects).
- Relaxed
/v1/github/validateURL validation to mirror Python behavior and added an audit/report markdown documenting findings and fixes.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
unicron-fix-porting-report.md |
Adds a detailed audit report documenting parity findings, fixes applied, and open questions. |
cla-backend-legacy/internal/store/user_permissions.go |
Updates pynamodb-style datetime formatter used for user-permissions records. |
cla-backend-legacy/internal/store/projects.go |
Extends pynamodb datetime parsing layouts to accept +0000 (no-colon) offsets. |
cla-backend-legacy/internal/store/dynamo_conv_reverse.go |
Aligns time.Time → DynamoDB string fallback formatting with pynamodb canonical format. |
cla-backend-legacy/internal/legacy/github/service.go |
Changes ValidateOrganization to be permissive like Python and keeps response-size limiting. |
cla-backend-legacy/internal/api/handlers.go |
Applies multiple parity fixes: datetime formatting usage, POST /v1/signature response/event_summary, CLA manager add behavior, GitHub org not-found status behavior. |
.gitignore |
Ignores CLAUDE.md. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cla-backend-legacy/internal/api/handlers.go (1)
4306-4359:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDeduplicate the ACL before reusing it downstream.
Now that duplicate adds intentionally continue,
sigACLcan contain the samelfidtwice. The stored item is deduped, but the email payload and response are still built from the duplicated slice, so repeated adds can return duplicate managers and duplicate entries in the “existing managers” email list.Suggested fix
- sigACL = append(sigACL, lfid) - sig["signature_acl"] = &types.AttributeValueMemberSS{Value: uniqueStringsPreserveOrder(sigACL)} + sigACL = uniqueStringsPreserveOrder(append(sigACL, lfid)) + sig["signature_acl"] = &types.AttributeValueMemberSS{Value: sigACL}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cla-backend-legacy/internal/api/handlers.go` around lines 4306 - 4359, The sigACL slice can contain duplicates because we allow repeated adds; before using sigACL to send emails or build the response, dedupe it (e.g., call the existing uniqueStringsPreserveOrder helper on sigACL) and assign the result back to sigACL so the values passed to sendCLAManagerEmailBestEffort(ctx, "added", lfid, companyID, claGroupID, sigACL) and buildClaManagersResponse(ctx, sigACL) are de-duplicated; ensure this happens after appending lfid and before those two calls so stored signature_acl remains correct and downstream consumers receive a unique list.
🧹 Nitpick comments (1)
unicron-fix-porting-report.md (1)
10-10: ⚡ Quick winRemove machine-specific absolute path from the report.
Line 10 hardcodes a local path that won’t resolve for other contributors and unnecessarily exposes local environment details. Prefer a repo-relative reference or plain filename.
✏️ Suggested doc tweak
-- Plan: `/home/morgi/.claude/plans/use-the-most-advanced-nifty-jellyfish.md` +- Plan: `use-the-most-advanced-nifty-jellyfish.md` (local absolute path removed)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@unicron-fix-porting-report.md` at line 10, Replace the machine-specific absolute path `/home/morgi/.claude/plans/use-the-most-advanced-nifty-jellyfish.md` in unicron-fix-porting-report.md (line with "Plan: ...") with a repo-relative path or just the filename `use-the-most-advanced-nifty-jellyfish.md` (or a relative path like `docs/plans/use-the-most-advanced-nifty-jellyfish.md`) so the reference is portable and does not expose local environment details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cla-backend-legacy/internal/api/handlers.go`:
- Around line 3402-3425: The create handler is building a manual resp map that
diverges from the canonical serializer (causing type mismatches like ints vs
serialized items); replace the manual resp construction in handlers.go with the
same serializer used elsewhere (call store.ItemToInterfaceMap or the existing
canonical signature serialization helper on the created signature item) and then
add/override the fields that must be set (signature_id, signature_project_id,
signature_reference_id/type, signature_type,
signature_signed/approved/embargo_acked, signature_return_url/sign_url,
date_created/date_modified using formatPynamoDateTimeUTC(now), version="v1"),
and include signature_user_ccla_company_id only when userCclaCompanyID != "" so
the create response matches the read/persisted shape.
In `@cla-backend-legacy/internal/legacy/github/service.go`:
- Around line 36-48: Restore SSRF protections for the endpoint before calling
http.NewRequestWithContext: validate and parse the URL string (endpoint) to
require the https scheme, reject localhost and private/reserved IP ranges (IPv4
and IPv6), and only allow either a defined allowlist (github.com,
api.github.com, raw.githubusercontent.com) or explicitly reject non-allowed
hosts; perform this check in the same function where http.NewRequestWithContext
is called (referencing endpoint and http.NewRequestWithContext) and return a
400/403 on invalid URLs. Also stop creating a new local http.Client (client :=
&http.Client{...}) and use the existing s.httpClient (as used by CheckNamespace
and GetNamespace), ensuring the shared client has an appropriate timeout set.
---
Outside diff comments:
In `@cla-backend-legacy/internal/api/handlers.go`:
- Around line 4306-4359: The sigACL slice can contain duplicates because we
allow repeated adds; before using sigACL to send emails or build the response,
dedupe it (e.g., call the existing uniqueStringsPreserveOrder helper on sigACL)
and assign the result back to sigACL so the values passed to
sendCLAManagerEmailBestEffort(ctx, "added", lfid, companyID, claGroupID, sigACL)
and buildClaManagersResponse(ctx, sigACL) are de-duplicated; ensure this happens
after appending lfid and before those two calls so stored signature_acl remains
correct and downstream consumers receive a unique list.
---
Nitpick comments:
In `@unicron-fix-porting-report.md`:
- Line 10: Replace the machine-specific absolute path
`/home/morgi/.claude/plans/use-the-most-advanced-nifty-jellyfish.md` in
unicron-fix-porting-report.md (line with "Plan: ...") with a repo-relative path
or just the filename `use-the-most-advanced-nifty-jellyfish.md` (or a relative
path like `docs/plans/use-the-most-advanced-nifty-jellyfish.md`) so the
reference is portable and does not expose local environment details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef504bac-15a1-4810-8196-c9c28bfcad7d
📒 Files selected for processing (7)
.gitignorecla-backend-legacy/internal/api/handlers.gocla-backend-legacy/internal/legacy/github/service.gocla-backend-legacy/internal/store/dynamo_conv_reverse.gocla-backend-legacy/internal/store/projects.gocla-backend-legacy/internal/store/user_permissions.gounicron-fix-porting-report.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cla-backend-legacy/internal/api/handlers.go`:
- Around line 4310-4313: The in-memory sigACL can contain duplicates after you
removed the early return (because you append(lfid) even when already present),
so deduplicate sigACL before it is reused: after you perform append(sigACL,
lfid) update sigACL to a unique list (e.g., with a temporary map/set) and then
pass that deduped slice into sendCLAManagerEmailBestEffort(...) and
buildClaManagersResponse(ctx, sigACL); ensure the same deduped variable is used
for any subsequent storage or response logic to avoid returning the same manager
twice.
- Around line 191-200: RequestEmployeeSignatureV2 is still serializing
date_created/date_modified (and any other signature timestamps written in that
handler) using time.RFC3339Nano, causing mixed timestamp formats; update
RequestEmployeeSignatureV2 to use the existing formatPynamoDateTimeUTC(t
time.Time) helper for all timestamp fields it writes (replace calls that use
t.Format(time.RFC3339Nano) with formatPynamoDateTimeUTC(t) for date_created,
date_modified, event_time/document_creation_date equivalents) so signatures use
the same six-microsecond +0000 format as pynamodb.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc15c477-98a2-4346-9524-996d6e9138f2
📒 Files selected for processing (3)
cla-backend-legacy/internal/api/handlers.gocla-backend-legacy/internal/legacy/github/service.gocla-backend-legacy/internal/parity/flags.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cla-backend-legacy/internal/api/handlers.go (1)
295-300:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLowercase the validation error string to unblock staticcheck.
Line 299 still uses a capitalized error message, and
staticcheckis already failing withST1005.Suggested fix
- return "", fmt.Errorf("Invalid URL specified") + return "", fmt.Errorf("invalid URL specified")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cla-backend-legacy/internal/api/handlers.go` around lines 295 - 300, In validateURL change the returned error message to be lowercase to satisfy staticcheck ST1005: update the fmt.Errorf call in function validateURL so the error string starts with a lowercase letter (e.g., "invalid URL specified") while keeping the same semantics and function signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cla-backend-legacy/internal/api/handlers.go`:
- Around line 295-300: In validateURL change the returned error message to be
lowercase to satisfy staticcheck ST1005: update the fmt.Errorf call in function
validateURL so the error string starts with a lowercase letter (e.g., "invalid
URL specified") while keeping the same semantics and function signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 374a8503-f560-4497-92f5-91ec704db092
📒 Files selected for processing (1)
cla-backend-legacy/internal/api/handlers.go
mlehotskylf
approved these changes
May 5, 2026
lukaszgryglicki
added a commit
that referenced
this pull request
May 7, 2026
Address porting feedback from claude
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
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.
No description provided.