docs(spec): M2 Spec Kit feature slice (proactive picker → Console hand-off) - #5144
docs(spec): M2 Spec Kit feature slice (proactive picker → Console hand-off)#5144mlehotskylf wants to merge 18 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request adds the Milestone 2 specification and implementation plan for feature-flagged CLA selection, Contributor Console handoff, ICLA/ECLA invalidation, status display, ownership enforcement, and impersonation protection. ChangesM2 Self Serve CLA handoff
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
4990e8e to
3f3efd6
Compare
There was a problem hiding this comment.
Pull request overview
Intended to add the M2 Spec Kit feature slice, but the diff also includes unrelated M1 API, SSS, email, test, and operational changes.
Changes:
- Adds M2 signing hand-off specification and plan.
- Adds the My CLAs API and documentation.
- Changes SSS screening, email normalization, utilities, and tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
utils/search_aws_log_group.sh |
Improves AWS/jq error handling. |
utils/my_clas.sh |
Adds My CLAs API utility. |
utils/lookup_all_logs.sh |
Fails fast on lookup errors. |
utils/local-fbsd-api.sh |
Adds local FreeBSD runner. |
utils/get_user_svc.sh |
Adds user-service lookup utility. |
utils/get_ddb_user_identities.sh |
Adds DynamoDB identity lookup. |
utils/downcase_emails.sh |
Adds email normalization script. |
tests/functional/cypress/e2e/v4/cla-manager.cy.ts |
Changes request failure handling. |
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md |
Defines the M2 feature. |
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md |
Plans M2 implementation. |
specs/001-easycla-ss-integration-fable/m1-my-cla/spec.md |
Adds M1 specification. |
specs/001-easycla-ss-integration-fable/m1-my-cla/plan.md |
Adds M1 plan. |
specs/001-easycla-ss-integration-fable/m1-my-cla/data-model.md |
Documents M1 models. |
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md |
Documents upstream contracts. |
docs/MY_CLAS_API.md |
Documents My CLAs endpoints. |
CLAUDE.md |
Adds repository guidance. |
cla-backend-legacy/internal/api/handlers.go |
Adds SSS switch and normalization. |
cla-backend-go/v2/user-service/client.go |
Adds contextual user lookups. |
cla-backend-go/v2/sign/service.go |
Integrates SSS switch and email changes. |
cla-backend-go/v2/sign/service_sss_test.go |
Tests SSS behavior. |
cla-backend-go/v2/sign/handlers.go |
Updates compliance errors. |
cla-backend-go/v2/my_clas/service.go |
Implements My CLAs logic. |
cla-backend-go/v2/my_clas/service_test.go |
Tests My CLAs service. |
cla-backend-go/v2/my_clas/repository.go |
Adds My CLAs data access. |
cla-backend-go/v2/my_clas/handlers.go |
Adds My CLAs handlers. |
cla-backend-go/users/repository.go |
Normalizes stored emails. |
cla-backend-go/swagger/common/my-identity-list.yaml |
Defines identity response. |
cla-backend-go/swagger/common/my-cla.yaml |
Defines CLA response item. |
cla-backend-go/swagger/common/my-cla-pdf.yaml |
Defines PDF response. |
cla-backend-go/swagger/common/my-cla-list.yaml |
Defines CLA list response. |
cla-backend-go/swagger/cla.v2.yaml |
Adds My CLAs API paths. |
cla-backend-go/signatures/repository.go |
Normalizes removed emails. |
cla-backend-go/emails/prefill_test.go |
Tests CLA Group prefill. |
cla-backend-go/emails/docusign_templates.go |
Updates signed-document emails. |
cla-backend-go/emails/docusign_templates_test.go |
Tests template rendering. |
cla-backend-go/config/ssm.go |
Loads the SSS switch. |
cla-backend-go/config/config.go |
Adds SSS enabled configuration. |
cla-backend-go/cmd/server.go |
Wires My CLAs and SSS. |
cla-backend-go/cmd/s3_upload/main.go |
Updates service construction. |
cla-backend-go/approval_list/repository.go |
Normalizes requester email. |
.gitignore |
Updates generated-file exclusions. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:63
- This source map does not match the completed M1 Me-lens seam it says M2 will extend. In the current
lfx-self-serve, M1 is implemented underapp/modules/profile/clas, registered as/profile/clasinmodules/profile/profile.routes.ts, and surfaced throughProfileLayoutComponent/buildProfileTabs;MainLayoutComponentandapp.routes.tsdo not own that tab. Following this plan would create a separate top-level surface instead of extending the existing Profile/Me hub. Update the structure to name the actual profile module, profile routes, tab builder/sidebar, and feature guard integration points.
├── app/modules/ # NEW "Sign a CLA" Me-lens surface (picker)
│ └── sign-cla/ # CLA-Group + org/repo + sign-type selection, hand-off
├── app/layouts/main-layout/main-layout.component.ts # EDIT: add Me-lens entry behind flag
├── app/app.routes.ts # EDIT: register the picker route (lens: 'me', flag-guarded)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:27
- FR-002 makes org/repo narrowing inside the Self Serve picker mandatory, but the corresponding open question on line 47 still allows deferring that selection to the Console. Those outcomes are mutually exclusive, so the acceptance criteria cannot be finalized or tested until one is selected. Either remove the defer-to-Console option or make this requirement conditional on the clarification result.
- **FR-002**: The picker MUST let the user select a CLA Group, and where a CLA Group spans multiple GitHub orgs/repos, MUST let the user narrow to a specific org/repo. *(Interaction depth is a [NEEDS CLARIFICATION] — see Open questions.)*
…d-off) Extract Milestone 2 into its own implementable Spec Kit folder (m2-sign-cla-handoff/), mirroring the existing m1-my-cla/ layout, based on main so it is self-contained and independent of the program-level design PR (#5132). Reflects the 2026-08-04 revision (per Heather/PM): M2 is an additive, PR-independent "Sign a CLA" picker in the Me lens that hands off to the existing Contributor Console for signing — for both ICLA and CCLA paths. SS runs no signing ceremony, makes no signing-initiation calls, and does not cut over or retire the Console; the PR-check remediation link is unchanged. Scaffolds spec.md + plan.md only. The hand-off contract, org/repo picker scope, and CLA-Group discovery endpoint are left as explicit open questions for the Spec Kit planning flow to resolve — research.md/data-model.md/ contracts/ are intentionally not hand-written. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
3f3efd6 to
b88e3ac
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (5)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:56
- The source plan omits required Contributor Console work. In the current Console,
IndividualDashboardComponent.findActiveSignature()fails when no active signature exists and tells the user to restart from a pull request. Since FR-001 requires a no-PR flow and FR-005 forbids Self Serve from initiating signing, changing onlylfx-self-servecannot make the ICLA path completable; includeeasycla-contributor-consolechanges (and any supporting contract/backend work) to bootstrap this proactive flow.
Primary repo: `linuxfoundation/lfx-self-serve`
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:63
- These frontend paths do not match the completed M1 seam this plan says it extends. M1 is implemented under
app/modules/profile/clas, registered inapp/modules/profile/profile.routes.ts, and exposed through the profile layout/sidebar—not throughapp/modules/my-clas,main-layout, orapp.routes.ts. Update this tree against the current repository so generated tasks target real integration points.
├── app/modules/ # NEW "Sign a CLA" Me-lens surface (picker)
│ └── sign-cla/ # CLA-Group + org/repo + sign-type selection, hand-off
├── app/layouts/main-layout/main-layout.component.ts # EDIT: add Me-lens entry behind flag
├── app/app.routes.ts # EDIT: register the picker route (lens: 'me', flag-guarded)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:28
- This constitution path is absent from the current branch, and PR #5132 also does not add the
.specifytooling. As written, the plan claims to have evaluated a file that will not exist after these changes merge.
`.specify/memory/constitution.md` is the unratified template — no project-specific gates exist. Default gates applied:
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:17
- The documented test stack is stale: the current
lfx-self-serveapp runs Vitest for unit tests and Playwright for E2E; it does not use Jest/Karma or Cypress. Correcting this prevents the generated tasks from targeting nonexistent harnesses.
This issue also appears in the following locations of the same file:
- line 28
- line 56
- line 60
**Testing**: lfx-self-serve conventions — Jest/Karma unit tests for services/components, server route tests, Cypress/E2E per repo norms (verify exact harness during implementation).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:27
- FR-002 already mandates that org/repo narrowing occurs in the Self Serve picker, but acceptance scenario 3 and the open question retain “defer to the Console” as a valid outcome; FR-008 likewise assumes org/repo is passed during hand-off. These normative requirements cannot all hold if clarification chooses deferral. Make them conditional on the design decision or remove the deferral alternative before generating tasks.
- **FR-002**: The picker MUST let the user select a CLA Group, and where a CLA Group spans multiple GitHub orgs/repos, MUST let the user narrow to a specific org/repo. *([NEEDS CLARIFICATION]: interaction depth.)*
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (6)
cla-backend-go/v2/my_clas/service_test.go (1)
23-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding error paths to
fakeRepo.Every
fakeRepomethod returns a nil error, so no test covers the failure branches. Two branches carry real behavior:
authorizeIdentitymust fail closed whenGetUsersByLFUsernamereturns an error.GetMyClasmust propagate aGetUserCLASignatureserror instead of returning a partial list.Add an error field per method and two short tests.
🤖 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-go/v2/my_clas/service_test.go` around lines 23 - 70, Extend fakeRepo with configurable errors for GetUsersByLFUsername and GetUserCLASignatures, returning the corresponding error from those methods. Add focused tests verifying authorizeIdentity fails closed when LF username lookup errors and GetMyClas propagates signature lookup errors without returning partial results.cla-backend-go/v2/user-service/client.go (2)
295-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape the URL components.
lfUsernameis placed in a query string anduserSFIDis placed in a path segment, both with plainfmt.Sprintf. Neither value is escaped. TodaylfUsernamecomes from the authenticated principal anduserSFIDcomes from the user-service response, so this is not exploitable from a request parameter. Escaping keeps the URLs correct if either source changes.♻️ Proposed refactor
- url := fmt.Sprintf("https://%s/user-service/v1/users?username=%s", usc.apiGwURL, lfUsername) + url := fmt.Sprintf("https://%s/user-service/v1/users?username=%s", usc.apiGwURL, neturl.QueryEscape(lfUsername))- url := fmt.Sprintf("https://%s/user-service/v1/users/%s/identities?pageSize=%d&offset=%d", usc.apiGwURL, userSFID, pageSize, offset) + url := fmt.Sprintf("https://%s/user-service/v1/users/%s/identities?pageSize=%d&offset=%d", usc.apiGwURL, neturl.PathEscape(userSFID), pageSize, offset)Import
net/urlasneturlto avoid shadowing by the localurlvariables.Also applies to: 362-362
🤖 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-go/v2/user-service/client.go` around lines 295 - 296, Escape the dynamic URL components in the user lookup request and the related request around userSFID. Use net/url imported as neturl, encode lfUsername as a query parameter and userSFID as a path segment before constructing each URL, while preserving the existing request behavior.
306-307: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse one
http.Clientinstead of creating one per call.Both new functions create
&http.Client{Timeout: 30 * time.Second}on each invocation. Each client gets its own default transport, so connections are never reused and idle connections accumulate per call. Declare one package-level client and share it.♻️ Proposed refactor
// package level, next to the other package vars var userServiceHTTPClient = &http.Client{Timeout: 30 * time.Second}- httpClient := &http.Client{Timeout: 30 * time.Second} - response, err := httpClient.Do(request) + response, err := userServiceHTTPClient.Do(request)Apply the same change in
ListUserIdentitiesat line 358.🤖 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-go/v2/user-service/client.go` around lines 306 - 307, Create a single package-level http.Client variable with the 30-second timeout at the top of the client.go file alongside other package-level declarations. Replace the inline httpClient instantiation at line 306-307 with a reference to this shared package-level client. Apply the same change to the httpClient creation in the ListUserIdentities function at line 358 to reuse the single shared client instance across both functions.cla-backend-go/v2/my_clas/repository.go (1)
137-188: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider bounding the users-table scan cost.
GetUsersBySecondaryEmailsruns a full scan ofcla-<stage>-usersfor every request that includessecondaryEmail. The scan reads all items and returns full items even though only a few attributes are used bytoUserModels.Two cheap improvements:
- Add a
ProjectionExpressionfor the attributestoUserModelsreads, to cut read units and payload size.- Add a metric or log of the consumed capacity, so the cost of this endpoint stays visible in production.
The swagger description already warns callers, and the service only scans values the caller owns, so this is a cost concern rather than an abuse path.
🤖 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-go/v2/my_clas/repository.go` around lines 137 - 188, Update GetUsersBySecondaryEmails to request only the attributes consumed by toUserModels via a DynamoDB ProjectionExpression, using the existing expression attribute-name handling as needed. Enable consumed-capacity reporting on the ScanInput and record the returned capacity through the repository’s established metric or logging mechanism for each scan page.cla-backend-go/v2/my_clas/handlers.go (1)
28-33: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSensitive Data Exposure (CWE-532): Insertion of Sensitive Information into Log File
Reachability: External · Exploitability: Difficult
Reachability path
● Entry cla-backend-go/cmd/server.go:486 Configure │ ▼ ● Sink cla-backend-go/v2/my_clas/handlers.goRemove
authUserEmailfrom the My CLAs log fields if application logs are subject to PII controls.The field is emitted on warning and error paths in all three handlers. The success paths do not write these fields. Apply the same change at lines 62–67 and 102–107.
🤖 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-go/v2/my_clas/handlers.go` around lines 28 - 33, Remove the authUserEmail field from the logrus.Fields definitions in all three My CLAs handlers, including the blocks near GetMyClas and the corresponding sections around lines 62–67 and 102–107. Keep the remaining functionName, XREQUESTID, and authentication username fields unchanged.cla-backend-go/v2/my_clas/service.go (1)
509-523: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueCase-variant Ownership Widening (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)
Reachability: External · Exploitability: Theoretical
Reachability path
● Entry cla-backend-legacy/internal/api/handlers.go:54 Handlers: Ported building blocks (incrementally used by endpoints as they are rewritten from Python). │ ▼ ● Sink cla-backend-go/v2/my_clas/service.goDefensive refactor: Remove the caller-supplied spelling from the approved-username list.
Append only the canonical variants proven to belong to the user; this keeps the ownership guarantee exact and avoids future case-sensitive identity collisions.
🤖 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-go/v2/my_clas/service.go` around lines 509 - 523, Update appendAllowedUsernames so approved usernames append only the canonical variants returned by canon, removing the caller-supplied value from the destination list while preserving skipped handling and deduplication.
🤖 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-go/approval_list/repository.go`:
- Line 90: Validate the normalized requester email in the request-writing flow
before constructing attributes or calling PutItem: trim whitespace and reject an
empty result with the repository’s established validation error pattern. Only
invoke addStringSliceAttribute for a non-empty value, preserving the lowercased
email behavior.
In `@cla-backend-go/config/config.go`:
- Around line 146-148: Update the comment for the Enabled field to accurately
describe behavior when the SSS kill switch is disabled: persisted SSS-origin
is_sanctioned blocks do not block compliance, while manual or admin-origin
blocks still do. Keep the documented behavior aligned with both compliance
implementations and the existing test expectations.
In `@cla-backend-go/signatures/repository.go`:
- Line 3345: Normalize every entry in params.RemoveEmailApprovalList with
TrimSpace and ToLower before passing it to buildApprovalAttributeList and
approvalList.ApprovalList, while preserving the existing normalized lookup
behavior. Update the removal-path updateApprovalTable call to pass
params.RemoveEmailApprovalList instead of params.AddEmailApprovalList.
- Line 3345: The email normalization in the loop over
params.RemoveEmailApprovalList converts whitespace-only entries to empty
strings, which causes GetProjectCompanyEmployeeSignatures to execute without an
email filter and return all company employee signatures instead of a filtered
result. This leads to unintended removal of signatures. Add a validation check
immediately after the email = strings.ToLower(strings.TrimSpace(email)) line to
skip the iteration if email is empty before the goroutine is spawned, preventing
the unfiltered query and subsequent invalidateSignatures call.
In `@cla-backend-go/users/repository.go`:
- Around line 390-393: After calling normalizeEmails() and assigning the result
to the normalized variable, check if the list is empty before assigning to
expressionAttributeValues. When normalized is empty, either add a REMOVE
operation for the `#UES` attribute to clear user_emails instead of assigning the
empty SS, or validate and reject the input before the UpdateItem call rather
than attempting to send an empty string set to DynamoDB.
- Around line 390-393: Update the Debug logging in the Save function, including
the log statement near normalizeEmails and the other sensitive-field logs
identified in the comment, so email addresses and related PII are not emitted.
Log only non-sensitive metadata such as collection counts, while preserving the
existing query-building behavior.
In `@cla-backend-go/v2/my_clas/service.go`:
- Around line 225-227: Update the sorting logic around result.Clas to parse each
SignedOn value and compare the parsed timestamps chronologically before any
FormatTimeString processing. Define and consistently apply behavior for invalid
or unsupported timestamps, then format the values afterward without using the
formatted strings for ordering.
In `@cla-backend-go/v2/user-service/client.go`:
- Around line 361-404: Bound the pagination loop around the user-identity
listing flow by tracking the number of pages and enforcing a finite maximum page
count. Also break immediately when identityList.Data is empty, while preserving
the existing short-page termination condition and response handling.
In `@docs/MY_CLAS_API.md`:
- Around line 599-608: Update the documented access policy for non-admin
GetMyClas and GetMyClaPdf to require immutable githubId/gitlabId corroboration
for historical records and signed PDFs, or explicitly record approved privacy
and security sign-off for the alias-based exception before release; do not
present the current-possession behavior as an unconditional accepted product
decision.
In
`@specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md`:
- Around line 3-5: Replace the superseded M1 contract in
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md:3-5
with the authoritative /v4/my-clas, /v4/my-clas/{signatureID}/pdf, and
/v4/my-clas/identities definitions. Update
specs/001-easycla-ss-integration-fable/m1-my-cla/data-model.md:5-5 for current
signature fields, ECLA classification, response fields, and validity; remove
obsolete Self Serve aggregation and authorization requirements from
specs/001-easycla-ss-integration-fable/m1-my-cla/plan.md:8-10; and align
specs/001-easycla-ss-integration-fable/m1-my-cla/spec.md:21-27 with valid,
secondary email, GitLab/Gerrit support, and the current endpoint scope.
In `@tests/functional/cypress/e2e/v4/cla-manager.cy.ts`:
- Line 710: The failOnStatusCode property in the CLA Manager creation request is
unconditionally set to false, which masks unexpected HTTP errors. Make this
setting conditional based on the allowFail parameter so that failOnStatusCode is
false only when allowFail is true, allowing the test to properly fail on
unexpected status codes when allowFail is false.
In `@utils/downcase_emails.sh`:
- Around line 1-2: Add the required Linux Foundation copyright header
immediately after the shebang in utils/downcase_emails.sh lines 1-2, followed by
the MIT SPDX identifier. Add the Linux Foundation copyright header after the
shebang in utils/lookup_all_logs.sh lines 1-2, preserving its existing
applicable SPDX header.
- Around line 10-26: Update the update-item operation in the normalization loop
to condition the write on user_emails still matching the scanned item’s original
user_emails.SS value, while preserving the normalized :e update. Capture
conditional-check failures separately from successful updates and report the
affected uid for manual retry instead of treating them as successful writes.
- Line 10: Update the script around the DynamoDB scan to set a restrictive
umask, create the JSON output with mktemp, and store its path for reuse instead
of writing directly to the stage-named file. Register an EXIT trap to remove the
temporary file on termination, and ensure the scan command writes to that
temporary path while preserving its existing query options.
In `@utils/get_user_svc.sh`:
- Around line 51-59: Update the profile lookup curl invocation in
get_user_svc.sh to include the -f/--fail flag alongside the existing silent and
show-error options. Ensure HTTP 4xx/5xx responses cause curl to fail so the
surrounding profile and pagination flows do not treat error responses as empty
results.
In `@utils/local-fbsd-api.sh`:
- Around line 5-10: Update the setup and build flow in the local fBSD launch
script to enable strict shell failure handling and run the `cd cla-backend-go`
plus `go build` commands in a subshell. Ensure any failed directory change or
build terminates the script before sourcing environment files or executing the
binary.
---
Nitpick comments:
In `@cla-backend-go/v2/my_clas/handlers.go`:
- Around line 28-33: Remove the authUserEmail field from the logrus.Fields
definitions in all three My CLAs handlers, including the blocks near GetMyClas
and the corresponding sections around lines 62–67 and 102–107. Keep the
remaining functionName, XREQUESTID, and authentication username fields
unchanged.
In `@cla-backend-go/v2/my_clas/repository.go`:
- Around line 137-188: Update GetUsersBySecondaryEmails to request only the
attributes consumed by toUserModels via a DynamoDB ProjectionExpression, using
the existing expression attribute-name handling as needed. Enable
consumed-capacity reporting on the ScanInput and record the returned capacity
through the repository’s established metric or logging mechanism for each scan
page.
In `@cla-backend-go/v2/my_clas/service_test.go`:
- Around line 23-70: Extend fakeRepo with configurable errors for
GetUsersByLFUsername and GetUserCLASignatures, returning the corresponding error
from those methods. Add focused tests verifying authorizeIdentity fails closed
when LF username lookup errors and GetMyClas propagates signature lookup errors
without returning partial results.
In `@cla-backend-go/v2/my_clas/service.go`:
- Around line 509-523: Update appendAllowedUsernames so approved usernames
append only the canonical variants returned by canon, removing the
caller-supplied value from the destination list while preserving skipped
handling and deduplication.
In `@cla-backend-go/v2/user-service/client.go`:
- Around line 295-296: Escape the dynamic URL components in the user lookup
request and the related request around userSFID. Use net/url imported as neturl,
encode lfUsername as a query parameter and userSFID as a path segment before
constructing each URL, while preserving the existing request behavior.
- Around line 306-307: Create a single package-level http.Client variable with
the 30-second timeout at the top of the client.go file alongside other
package-level declarations. Replace the inline httpClient instantiation at line
306-307 with a reference to this shared package-level client. Apply the same
change to the httpClient creation in the ListUserIdentities function at line 358
to reuse the single shared client instance across both functions.
🪄 Autofix
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: 87870c4c-e5ee-4c24-ba79-049c40eeac83
📒 Files selected for processing (41)
.gitignoreCLAUDE.mdcla-backend-go/approval_list/repository.gocla-backend-go/cmd/s3_upload/main.gocla-backend-go/cmd/server.gocla-backend-go/config/config.gocla-backend-go/config/ssm.gocla-backend-go/emails/docusign_templates.gocla-backend-go/emails/docusign_templates_test.gocla-backend-go/emails/prefill_test.gocla-backend-go/signatures/repository.gocla-backend-go/swagger/cla.v2.yamlcla-backend-go/swagger/common/my-cla-list.yamlcla-backend-go/swagger/common/my-cla-pdf.yamlcla-backend-go/swagger/common/my-cla.yamlcla-backend-go/swagger/common/my-identity-list.yamlcla-backend-go/users/repository.gocla-backend-go/v2/my_clas/handlers.gocla-backend-go/v2/my_clas/repository.gocla-backend-go/v2/my_clas/service.gocla-backend-go/v2/my_clas/service_test.gocla-backend-go/v2/sign/handlers.gocla-backend-go/v2/sign/service.gocla-backend-go/v2/sign/service_sss_test.gocla-backend-go/v2/user-service/client.gocla-backend-legacy/internal/api/handlers.godocs/MY_CLAS_API.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/data-model.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/plan.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/spec.mdspecs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.mdspecs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.mdtests/functional/cypress/e2e/v4/cla-manager.cy.tsutils/downcase_emails.shutils/get_ddb_user_identities.shutils/get_user_svc.shutils/local-fbsd-api.shutils/lookup_all_logs.shutils/my_clas.shutils/search_aws_log_group.sh
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (14)
cla-backend-go/approval_list/repository.go (1)
90-90: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -e printf '%s\n' '--- repository.go ---' sed -n '1,180p' cla-backend-go/approval_list/repository.go printf '%s\n' '--- addStringSliceAttribute definitions and uses ---' rg -n -C 4 'addStringSliceAttribute|user_emails|requesterEmail' cla-backend-goRepository: linuxfoundation/easycla
Length of output: 31945
🏁 Script executed:
#!/bin/bash set -e printf '%s\n' '--- helper ---' cat -n cla-backend-go/approval_list/helpers.go printf '%s\n' '--- AddCclaApprovalRequest callers ---' rg -n -C 8 'AddCclaApprovalRequest|requester_email|requesterEmail' cla-backend-go/approval_list cla-backend-go | head -n 260 printf '%s\n' '--- module and local AWS SDK availability ---' rg -n 'aws-sdk-go|go [0-9]+\.[0-9]+' cla-backend-go/go.mod go.mod 2>/dev/null || true find . -path '*aws-sdk-go*attributevalue*' -o -path '*aws-sdk-go*dynamodbattribute*' | head -n 40Repository: linuxfoundation/easycla
Length of output: 26332
🌐 Web query:
AWS DynamoDB String Set empty string member ValidationException💡 Result:
In Amazon DynamoDB, you cannot store an empty Set (including String Sets, Number Sets, and Binary Sets). Attempting to perform a PutItem or UpdateItem operation with an empty set will result in a ValidationException [1][2][3]. It is important to distinguish between two related concepts: 1. Empty Sets are Not Allowed: DynamoDB explicitly prohibits sets that contain no elements [2][3]. If you attempt to persist a set with no members, the service will reject the request with a ValidationException, stating that a string set (or number/binary set) may not be empty [4][5]. 2. Empty Strings Within a Set: While the set itself cannot be empty, DynamoDB does permit the inclusion of empty string values as members within a non-empty String Set [4][6][3]. To resolve a ValidationException caused by an empty set, you should either: - Ensure the set contains at least one valid element before sending the request [7]. - If the attribute is optional and the set is empty, remove the attribute from the item entirely before sending the request to DynamoDB [7]. Note that while Lists and Maps allow empty structures, Sets strictly require at least one member to be considered valid by the DynamoDB API [8][3].
Citations:
- 1: aws/aws-sdk-js-v3#5266
- 2: https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_PutItem.html
- 3: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/HowItWorks.NamingRulesDataTypes.html
- 4: https://stackoverflow.com/questions/61689685/dynamodbmapper-unable-to-create-a-string-set
- 5: getmoto/moto#8022
- 6: https://stackoverflow.com/questions/78117417/aws-dynamodb-does-not-save-empty-string-set-properly
- 7: https://dynobase.dev/dynamodb-errors/dynamodb-string-set-cannot-be-empty/
- 8: aws/aws-sdk-js#3235
🏁 Script executed:
#!/bin/bash set -e printf '%s\n' '--- AWS SDK module cache ---' find "${GOMODCACHE:-/root/go/pkg/mod}" -path '*aws-sdk-go*' -type f 2>/dev/null | head -n 20 || true printf '%s\n' '--- SDK v1.36.27 AttributeValue validation ---' curl -fsSL https://raw.githubusercontent.com/aws/aws-sdk-go/v1.36.27/service/dynamodb/api.go | rg -n -C 12 'func \(s \*AttributeValue\) Validate|SS.*Validate|StringSet|empty' printf '%s\n' '--- approval-list request argument definitions and validation ---' sed -n '1,190p' cla-backend-go/approval_list/service.go rg -n -C 8 'ContributorEmail|ContributorName|Request.*Access|approval.*request|allowlist|approval_list' cla-backend-go/approval_list cla-backend-go/swagger | head -n 320Repository: linuxfoundation/easycla
Length of output: 50379
🌐 Web query:
site:docs.aws.amazon.com/amazondynamodb/latest/developerguide/HowItWorks.NamingRulesDataTypes.html empty string values set💡 Result:
In most programming contexts and data structures, an empty string is treated as a valid, distinct string value, and thus it can be contained within a set [1]. Because sets enforce the uniqueness of their elements, an empty string is considered a unique element that is distinct from other strings or null values [1]. For example, in systems like Amazon DynamoDB, empty string values are explicitly permitted within string sets [1]. While DynamoDB does not support empty sets themselves, it allows a string set to include an empty string as one of its members [1]. In common programming languages such as Python, Java, or JavaScript, the standard Set data structure similarly treats an empty string as a valid element [1]. You can include an empty string in a set without it being interpreted as an indicator of an empty set or a null value. Example of adding an empty string to a set: # Python my_set = {"apple", ""} print("" in my_set) # Returns True // JavaScript let mySet = new Set(["apple", ""]); console.log(mySet.has("")); // Returns true // Java Set mySet = new HashSet<>; mySet.add(""); System.out.println(mySet.contains("")); // Returns true
Citations:
🏁 Script executed:
#!/bin/bash set -e printf '%s\n' '--- approval-list service request path ---' sed -n '1,175p' cla-backend-go/approval_list/service.go printf '%s\n' '--- ContributorEmail declarations and validation ---' rg -n -C 10 'ContributorEmail|contributor_email|contributorEmail' cla-backend-go/approval_list cla-backend-go/gen cla-backend-go/swagger | head -n 280 printf '%s\n' '--- request schema required fields ---' rg -n -C 8 'ContributorEmail|contributor_email|contributor-email' cla-backend-go/swagger cla-backend-go/gen | head -n 220Repository: linuxfoundation/easycla
Length of output: 24118
🏁 Script executed:
#!/bin/bash set -e python3 - <<'PY' inputs = ["", " ", "\t\n", " User@Example.COM "] for raw in inputs: normalized = raw.strip().lower() attribute = {"SS": [normalized]} print({"input": repr(raw), "normalized": repr(normalized), "attribute": attribute, "set_member_count": len(attribute["SS"])}) PYRepository: linuxfoundation/easycla
Length of output: 564
Reject an empty normalized requester email before writing the request.
strings.TrimSpaceconverts empty input to"", and the helper createsSS: [""]. DynamoDB accepts this non-empty string set, soPutItemcan persist an invaliduser_emailsvalue. Return a validation error beforePutItem.🤖 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-go/approval_list/repository.go` at line 90, Validate the normalized requester email in the request-writing flow before constructing attributes or calling PutItem: trim whitespace and reject an empty result with the repository’s established validation error pattern. Only invoke addStringSliceAttribute for a non-empty value, preserving the lowercased email behavior.cla-backend-go/config/config.go (1)
146-148: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the persisted SSS-origin behavior correctly.
The comment states that persisted
is_sanctionedvalues still block. Both compliance implementations returnfalsefor an SSS-origin persisted block whensssEnabledis false. The new test also requires that behavior. Limit this statement to manual or admin blocks, or change the runtime behavior.🤖 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-go/config/config.go` around lines 146 - 148, Update the comment for the Enabled field to accurately describe behavior when the SSS kill switch is disabled: persisted SSS-origin is_sanctioned blocks do not block compliance, while manual or admin-origin blocks still do. Keep the documented behavior aligned with both compliance implementations and the existing test expectations.cla-backend-go/signatures/repository.go (1)
3345-3345: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C 8 'buildApprovalAttributeList|RemoveEmailApprovalList|ApprovalList =|invalidateSignatures|updateApprovalTable' cla-backend-goRepository: linuxfoundation/easycla
Length of output: 50379
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- repository update path ---' sed -n '3298,3365p' cla-backend-go/signatures/repository.go printf '%s\n' '--- list removal helper ---' rg -n -C 8 'func RemoveItemsFromList|RemoveItemsFromList' cla-backend-go printf '%s\n' '--- approval-table implementation ---' rg -n -C 12 'func .*updateApprovalTable|updateApprovalTable' cla-backend-go/signatures printf '%s\n' '--- user search implementation ---' rg -n -C 10 'func .*SearchUsers|SearchUsers\(' cla-backend-go | head -200Repository: linuxfoundation/easycla
Length of output: 47562
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- exact removal semantics ---' sed -n '110,155p' cla-backend-go/utils/utils.go printf '%s\n' '--- approval-table removal path ---' sed -n '3860,3925p' cla-backend-go/signatures/repository.go printf '%s\n' '--- user search query construction ---' sed -n '1255,1345p' cla-backend-go/users/repository.go printf '%s\n' '--- converter behavior probe ---' python3 - <<'PY' existing = ["alice@example.com"] remove = [" Alice@Example.com "] updated = [value.strip() for value in existing] result = [value for value in updated if value not in remove] print({"existing": existing, "remove": remove, "updated": updated, "result": result}) PYRepository: linuxfoundation/easycla
Length of output: 6907
Normalize and pass the removal email list to every consumer.
utils.RemoveItemsFromListuses exact equality." Alice@Example.com "does not remove storedalice@example.com; line 3345 only normalizes the goroutine-local lookup. Normalize the removal slice beforebuildApprovalAttributeListandapprovalList.ApprovalList.- The removal call to
updateApprovalTablepassesparams.AddEmailApprovalList. Passparams.RemoveEmailApprovalListso removal-only requests deactivate the correct records.🤖 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-go/signatures/repository.go` at line 3345, Normalize every entry in params.RemoveEmailApprovalList with TrimSpace and ToLower before passing it to buildApprovalAttributeList and approvalList.ApprovalList, while preserving the existing normalized lookup behavior. Update the removal-path updateApprovalTable call to pass params.RemoveEmailApprovalList instead of params.AddEmailApprovalList.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C 8 'SearchUsers|criteria\.UserEmail|GetProjectCompanyEmployeeSignatures|invalidateSignatures' cla-backend-goRepository: linuxfoundation/easycla
Length of output: 50380
🏁 Script executed:
# Get context around line 3345 in signatures/repository.go sed -n '3330,3360p' cla-backend-go/signatures/repository.go # Get the GetProjectCompanyEmployeeSignatures function sed -n '2200,2250p' cla-backend-go/signatures/repository.go # Get the invalidateSignatures function signature and implementation rg -n 'func.*invalidateSignatures' cla-backend-go/signatures/repository.go -A 25Repository: linuxfoundation/easycla
Length of output: 5556
🏁 Script executed:
# Verify SearchUsers returns non-nil for empty input sed -n '1255,1275p' cla-backend-go/users/repository.go # Verify the condition check in the goroutine sed -n '3344,3365p' cla-backend-go/signatures/repository.go # Check buildApprovalAttributeList to see if it normalizes rg -n 'buildApprovalAttributeList' cla-backend-go/signatures/repository.go -A 10Repository: linuxfoundation/easycla
Length of output: 6297
Reject blank removal entries before starting the removal goroutines.
Line 3345 normalizes whitespace-only input to an empty string. When
"",SearchUsers("user_emails", "", false)returns a non-nil empty result (not an error or nil), so the condition on line 3354 does not trigger early return. The code then callsGetProjectCompanyEmployeeSignatureswithcriteria.UserEmail = "". Since the filter is only applied whencriteria.UserEmail != ""(line 2220), the query executes without the email filter and may return all employee signatures for the company and project. These unfiltered signatures are then passed toinvalidateSignatures, causing unintended removals.Add a check to reject empty normalized emails before the goroutine starts:
for _, email := range params.RemoveEmailApprovalList { email = strings.ToLower(strings.TrimSpace(email)) if email == "" { log.WithFields(f).Debugf("skipping empty removal entry") continue } go func(email string) {🤖 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-go/signatures/repository.go` at line 3345, The email normalization in the loop over params.RemoveEmailApprovalList converts whitespace-only entries to empty strings, which causes GetProjectCompanyEmployeeSignatures to execute without an email filter and return all company employee signatures instead of a filtered result. This leads to unintended removal of signatures. Add a validation check immediately after the email = strings.ToLower(strings.TrimSpace(email)) line to skip the iteration if email is empty before the goroutine is spawned, preventing the unfiltered query and subsequent invalidateSignatures call.cla-backend-go/users/repository.go (1)
390-393: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C 6 'normalizeEmails|AttributeValue.*SS|UpdateItem|user_emails' cla-backend-go/users/repository.goRepository: linuxfoundation/easycla
Length of output: 6571
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- Save implementation ---' sed -n '300,465p' cla-backend-go/users/repository.go printf '%s\n' '--- normalizeEmails implementation ---' sed -n '888,925p' cla-backend-go/users/repository.go printf '%s\n' '--- user email model and Save callers ---' rg -n -C 4 'type User|Emails \[\]string|\.Save\(|Save\(.*User|user\.Emails' cla-backend-go/users cla-backend-go -g '*.go' | head -n 260 printf '%s\n' '--- tests mentioning email updates ---' rg -n -C 5 'normalizeEmails|user_emails|Emails' cla-backend-go -g '*_test.go' | head -n 260Repository: linuxfoundation/easycla
Length of output: 42023
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- UserUpdate definition and API schema references ---' rg -n -C 8 'type UserUpdate|UserUpdate struct|emails.*json|Emails.*json|emails:' cla-backend-go -g '*.go' -g '*.yaml' -g '*.yml' -g '*.json' | head -n 320 printf '%s\n' '--- update-user handler validation and generated parameters ---' sed -n '1,165p' cla-backend-go/users/handlers.go printf '%s\n' '--- module SDK version ---' rg -n 'aws-sdk-go|go [0-9]' cla-backend-go/go.mod go.mod 2>/dev/null || true printf '%s\n' '--- focused structural behavior probe ---' python3 - <<'PY' def normalize(emails): if emails is None: return None seen = set() out = [] for email in emails: email = email.strip().lower() if not email or email in seen: continue seen.add(email) out.append(email) return out for value in (None, [], [""], [" ", "\t"], ["A@example.org", " a@example.org "]): normalized = normalize(value) print(repr(value), "=>", repr(normalized), "non_nil_branch=", value is not None, "empty_SS=", value is not None and len(normalized) == 0) PYRepository: linuxfoundation/easycla
Length of output: 23625
🌐 Web query:
AWS DynamoDB UpdateItem AttributeValue SS empty set validation error💡 Result:
In Amazon DynamoDB, set type attributes (String Set [SS], Number Set [NS], and Binary Set [BS]) cannot be empty [1][2][3]. If you attempt to update an item with an empty set, DynamoDB will reject the request with a ValidationException [1][2][4]. This restriction is a fundamental design constraint of the DynamoDB service [5][4]. The service requires that set attributes contain at least one element [1][2]. To resolve or avoid this validation error, consider the following approaches: 1. Remove the attribute: If the set is empty, it is often appropriate to remove the attribute from the item entirely rather than setting it to an empty collection [6][4]. 2. Validate before sending: Implement logic in your application or client-side code to check if the set is empty before performing the UpdateItem operation [6]. If the set is empty, skip the update or remove the attribute using a REMOVE expression [6]. 3. Ensure at least one element: If the business logic requires the attribute to exist, ensure that the set contains at least one valid element before sending the request [6]. Note that this behavior is specific to set types. While String and Binary scalar attributes also have length requirements (must be greater than zero), set types are uniquely restricted from being empty [1][2][5]. Attempting to send an AttributeValue structure that is empty or does not contain a supported, non-empty data type will trigger a ValidationException [4].
Citations:
- 1: https://docs.aws.amazon.com/cli/latest/reference/dynamodb/update-item.html
- 2: https://docs.aws.amazon.com/boto3/latest/reference/services/dynamodb/client/update_item.html
- 3: https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_AttributeValueUpdate.html
- 4: https://stackoverflow.com/questions/42203545/amazonserviceexception-supplied-attributevalue-is-empty-must-contain-exactly-o
- 5: aws/aws-sdk-net#3297
- 6: https://dynobase.dev/dynamodb-errors/dynamodb-string-set-cannot-be-empty/
Handle empty normalized email lists before assigning
AttributeValue.SS.When
user.Emailsis non-nil butnormalizeEmailsreturns no values,Savesends an emptySSto DynamoDB. DynamoDB rejects empty string sets, so the update fails and cannot clearuser_emails. UseREMOVE#UES`` when an empty list means clear; otherwise reject the input beforeUpdateItem.🤖 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-go/users/repository.go` around lines 390 - 393, After calling normalizeEmails() and assigning the result to the normalized variable, check if the list is empty before assigning to expressionAttributeValues. When normalized is empty, either add a REMOVE operation for the `#UES` attribute to clear user_emails instead of assigning the empty SS, or validate and reject the input before the UpdateItem call rather than attempting to send an empty string set to DynamoDB.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# First, read the exact code at the specified lines cat -n cla-backend-go/users/repository.go | sed -n '380,400p'Repository: linuxfoundation/easycla
Length of output: 1405
🏁 Script executed:
# Check the context of this function and normalizeEmails implementation ast-grep outline cla-backend-go/users/repository.goRepository: linuxfoundation/easycla
Length of output: 2707
🏁 Script executed:
# Search for log level configuration in the codebase rg -i "log.*level|debug" cla-backend-go --type go -B 2 -A 2 | head -100Repository: linuxfoundation/easycla
Length of output: 7545
🏁 Script executed:
# Look for server/initialization configuration fd config -e yaml -e yml -e go | head -20Repository: linuxfoundation/easycla
Length of output: 288
🏁 Script executed:
# Check if there are any log redaction or sanitization mechanisms rg -i "redact|sanitiz|mask" cla-backend-go --type goRepository: linuxfoundation/easycla
Length of output: 2230
🏁 Script executed:
# Examine the config.go file for log level settings cat -n cla-backend-go/config/config.go | head -100Repository: linuxfoundation/easycla
Length of output: 4094
🏁 Script executed:
# Search for logrus or log level initialization rg -i "logrus|setlevel|debug" cla-backend-go --type go | grep -i "level\|init\|new" | head -30Repository: linuxfoundation/easycla
Length of output: 3346
🏁 Script executed:
# Check server.go for log initialization and configuration cat -n cla-backend-go/cmd/server.go | sed -n '1,100p'Repository: linuxfoundation/easycla
Length of output: 5137
🏁 Script executed:
# Look for environment variables controlling log levels rg -i "log_level|loglevel" cla-backend-go --type go -B 2 -A 2Repository: linuxfoundation/easycla
Length of output: 526
🏁 Script executed:
# Check if Debug logs are sent to DataDog or other persistent services rg -i "datadog|cloudwatch|stderr|stdout" cla-backend-go --type go | head -40Repository: linuxfoundation/easycla
Length of output: 3882
🏁 Script executed:
# Check the complete logging/logger.go to understand log level defaults and output cat -n cla-backend-go/logging/logger.goRepository: linuxfoundation/easycla
Length of output: 6979
🏁 Script executed:
# Look for log output configuration (file, stdout, etc.) rg "AddHook|SetOutput" cla-backend-go --type go -B 2 -A 2Repository: linuxfoundation/easycla
Length of output: 161
🏁 Script executed:
# Check if logs are sent to Datadog or other centralized services rg "Hook|Formatter" cla-backend-go/logging --type go -B 2 -A 2Repository: linuxfoundation/easycla
Length of output: 1681
🏁 Script executed:
# Check for deployment/infrastructure configuration that sets LOG_LEVEL find . -type f \( -name "*.yml" -o -name "*.yaml" -o -name "*.tf" -o -name "*.json" -o -name "*.env*" \) | xargs grep -l "LOG_LEVEL" 2>/dev/null | head -20Repository: linuxfoundation/easycla
Length of output: 161
🏁 Script executed:
# Check for any log redaction or sanitization at the application level rg "email|Email" cla-backend-go/logging --type go -B 2 -A 2Repository: linuxfoundation/easycla
Length of output: 161
🏁 Script executed:
# Verify if there's any configuration in the actual Save function that calls this code cat -n cla-backend-go/users/repository.go | sed -n '331,470p'Repository: linuxfoundation/easycla
Length of output: 7207
Sensitive Data Exposure (CWE-532): Insertion of Sensitive Information into Log File
Reachability: External
Redact the email list before logging, or log only the count.
Line 391 logs the full normalized email addresses at Debug level using
%v. The default log level in the codebase is Debug (logging/logger.go:77), and no deployment-levelLOG_LEVELoverride is configured. In production Lambda or container environments, Debug logs are typically captured and retained in CloudWatch Logs or similar log aggregation services, creating a persistent PII copy. Replace the log statement with a redacted version—for example, log only the count of emails (len(normalized)) or redact the addresses to[email address]format. This pattern applies to other Debug logs in the Save function that also log sensitive fields (lines 369, 376, 383, 398, 405, 412, 419, 426).🤖 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-go/users/repository.go` around lines 390 - 393, Update the Debug logging in the Save function, including the log statement near normalizeEmails and the other sensitive-field logs identified in the comment, so email addresses and related PII are not emitted. Log only non-sensitive metadata such as collection counts, while preserving the existing query-building behavior.cla-backend-go/v2/my_clas/service.go (1)
225-227: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Inspect FormatTimeString to confirm the output format is lexicographically sortable. set -euo pipefail rg -n --type=go -C15 'func FormatTimeString\s*\(' cla-backend-go/utils # Show the layout constants it uses. rg -n --type=go 'time\.RFC3339|2006-01-02|Jan 2|Layout' cla-backend-go/utils | head -40Repository: linuxfoundation/easycla
Length of output: 3377
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- utils parsing ---' cat -n cla-backend-go/utils/utils.go | sed -n '40,115p' printf '%s\n' '--- service context ---' cat -n cla-backend-go/v2/my_clas/service.go | sed -n '180,245p' printf '%s\n' '--- SignedOn declarations and assignments ---' rg -n --type=go -C4 'SignedOn|signedOn|FormatTimeString|my-cla-list|sorted by signedOn' cla-backend-go/v2 cla-backend-go | head -240 printf '%s\n' '--- contract references ---' rg -n -i --glob '*.{yaml,yml,go,md}' 'cla-list|signedOn|signed.on|chronolog|sort' . | head -240Repository: linuxfoundation/easycla
Length of output: 48068
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- timestamp writers ---' rg -n --type=go -C10 'func .*AddSignedOn|AddSignedOn\(|SignatureCreated|DateCreated:|date_created' cla-backend-go/v2/signatures cla-backend-go/signatures cla-backend-go/v2/sign cla-backend-go/v2/my_clas | head -280 printf '%s\n' '--- my-cla sorting tests and fixtures ---' cat -n cla-backend-go/v2/my_clas/service_test.go | sed -n '1,220p' rg -n --type=go -C8 'GetMyCla|ResultCount|SignedOn|signedOn' cla-backend-go/v2/my_clas/service_test.go printf '%s\n' '--- API contract and response model ---' cat -n cla-backend-go/swagger/common/my-cla-list.yaml | sed -n '1,65p' cat -n docs/MY_CLAS_API.md | sed -n '235,255p;305,380p'Repository: linuxfoundation/easycla
Length of output: 50379
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- AddSignedOn implementation ---' rg -n --type=go -C18 'func \(.*\) AddSignedOn' cla-backend-go/signatures printf '%s\n' '--- all direct signed_on assignments near write paths ---' rg -n --type=go -C3 'SignedOn:\s|signed_on.*currentTime|AddSignedOn' cla-backend-go/signatures cla-backend-go/v2/sign cla-backend-go/v2/dynamo_events | head -220 printf '%s\n' '--- contract wording ---' cat -n cla-backend-go/swagger/common/my-cla-list.yaml | sed -n '25,38p' cat -n docs/MY_CLAS_API.md | sed -n '238,248p'Repository: linuxfoundation/easycla
Length of output: 19120
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from datetime import datetime, timezone # Model the relevant Go behavior: # ParseDateTime accepts RFC3339-like values, then FormatTimeString emits # time.RFC3339, which has second precision and UTC normalization. values = [ "2025-01-01T00:00:00.900000Z", "2025-01-01T00:00:00.100000Z", ] formatted = [ datetime.fromisoformat(v.replace("Z", "+00:00")) .astimezone(timezone.utc) .strftime("%Y-%m-%dT%H:%M:%SZ") for v in values ] print("raw chronological order:", values[0] > values[1]) print("formatted values:", formatted) print("formatted comparator distinguishes them:", formatted[0] != formatted[1]) assert values[0] != values[1] assert formatted[0] == formatted[1] PYRepository: linuxfoundation/easycla
Length of output: 305
Sort by parsed time before formatting.
FormatTimeStringremoves subsecond precision and returns the original string for unsupported values. String sorting can therefore violate chronological order. Sort by parsed timestamps and define the invalid-value behavior before formatting.🤖 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-go/v2/my_clas/service.go` around lines 225 - 227, Update the sorting logic around result.Clas to parse each SignedOn value and compare the parsed timestamps chronologically before any FormatTimeString processing. Define and consistently apply behavior for invalid or unsupported timestamps, then format the values afterward without using the formatted strings for ordering.cla-backend-go/v2/user-service/client.go (1)
361-404: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound the pagination loop.
The loop only stops when a page returns fewer than
pageSizerecords. If the upstream ignoresoffsetand keeps returning exactly 100 records, the loop repeats until the request context is cancelled. The 30-second timeout applies per request, not to the loop.Add a maximum page count, and stop when a page returns no records.
🛡️ Proposed guard
const pageSize = int64(100) + const maxPages = 100 var identities []*models.UserIdentity - for offset := int64(0); ; offset += pageSize { + for page := 0; page < maxPages; page++ { + offset := int64(page) * pageSize url := fmt.Sprintf("https://%s/user-service/v1/users/%s/identities?pageSize=%d&offset=%d", usc.apiGwURL, userSFID, pageSize, offset) @@ identities = append(identities, identityList.Data...) - if int64(len(identityList.Data)) < pageSize { + if int64(len(identityList.Data)) < pageSize { break } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const pageSize = int64(100) const maxPages = 100 var identities []*models.UserIdentity for page := 0; page < maxPages; page++ { offset := int64(page) * pageSize url := fmt.Sprintf("https://%s/user-service/v1/users/%s/identities?pageSize=%d&offset=%d", usc.apiGwURL, userSFID, pageSize, offset) request, reqErr := http.NewRequestWithContext(ctx, "GET", url, nil) if reqErr != nil { log.WithFields(f).WithError(reqErr).Warn("problem building new request") return nil, reqErr } request.Header.Set("X-API-KEY", usc.apiKey) request.Header.Set("Authorization", "Bearer "+tok) request.Header.Set("Content-Type", "application/json") response, doErr := httpClient.Do(request) if doErr != nil { log.WithFields(f).WithError(doErr).Warn("problem listing user identities") return nil, doErr } data, readErr := io.ReadAll(response.Body) closeErr := response.Body.Close() if closeErr != nil { log.WithFields(f).WithError(closeErr).Warn("error closing body") } if readErr != nil { log.WithFields(f).WithError(readErr).Warn("problem reading the user identities response") return nil, readErr } if response.StatusCode != http.StatusOK { log.WithFields(f).Warnf("problem listing user identities - status: %d", response.StatusCode) return nil, fmt.Errorf("listing user identities for user %s failed with status: %d", userSFID, response.StatusCode) } var identityList models.UserIdentityList if unmarshalErr := json.Unmarshal(data, &identityList); unmarshalErr != nil { log.WithFields(f).WithError(unmarshalErr).Warn("problem decoding the user identities response") return nil, unmarshalErr } identities = append(identities, identityList.Data...) if int64(len(identityList.Data)) < pageSize { break } }🤖 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-go/v2/user-service/client.go` around lines 361 - 404, Bound the pagination loop around the user-identity listing flow by tracking the number of pages and enforcing a finite maximum page count. Also break immediately when identityList.Data is empty, while preserving the existing short-page termination condition and response handling.docs/MY_CLAS_API.md (1)
599-608: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '383,520p' cla-backend-go/v2/my_clas/service.go rg -n "authorizeIdentity|GithubID|GithubUsername|historical|reassign|recycl|GetMyClas" cla-backend-go/v2/my_clas docs/MY_CLAS_API.mdRepository: linuxfoundation/easycla
Length of output: 19136
🏁 Script executed:
sed -n '280,380p' cla-backend-go/v2/my_clas/service.go sed -n '440,575p' cla-backend-go/v2/my_clas/service_test.go sed -n '120,220p' cla-backend-go/v2/my_clas/service.goRepository: linuxfoundation/easycla
Length of output: 12677
🏁 Script executed:
sed -n '200,290p' cla-backend-go/v2/my_clas/service.go sed -n '55,115p' cla-backend-go/v2/my_clas/handlers.go rg -n "Pdf|pdf|Download|download" cla-backend-go/v2/my_clasRepository: linuxfoundation/easycla
Length of output: 10902
Historical CLA Disclosure Through Reassigned Identity (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)
Reachability: External · Exploitability: Moderate
Require privacy/security approval or immutable identity corroboration before release.
The non-admin
GetMyClasandGetMyClaPdfpaths authorize email and SCM username values from the authenticated LF account’s identities. A reassigned alias can expose the previous holder’s historical records and signed ICLA PDF. Require immutablegithubId/gitlabIdcorroboration, or document explicit privacy and security approval for this exception.🤖 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 `@docs/MY_CLAS_API.md` around lines 599 - 608, Update the documented access policy for non-admin GetMyClas and GetMyClaPdf to require immutable githubId/gitlabId corroboration for historical records and signed PDFs, or explicitly record approved privacy and security sign-off for the alias-based exception before release; do not present the current-possession behavior as an unconditional accepted product decision.specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md (1)
3-5: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use one authoritative M1 contract after PR
#5125.These artifacts still contain pre-PR
#5125endpoints, client-side authorization, old signature classification, and old status models while their banners say those designs are superseded.
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md#L3-L5: replace the old endpoint contract with/v4/my-clas,/v4/my-clas/{signatureID}/pdf, and/v4/my-clas/identities.specs/001-easycla-ss-integration-fable/m1-my-cla/data-model.md#L5-L5: update the signature fields, ECLA classification, response fields, and validity model.specs/001-easycla-ss-integration-fable/m1-my-cla/plan.md#L8-L10: remove obsolete Self Serve aggregation and authorization requirements.specs/001-easycla-ss-integration-fable/m1-my-cla/spec.md#L21-L27: align requirements withvalid, secondary email, GitLab/Gerrit support, and current endpoint scope.📍 Affects 4 files
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md#L3-L5(this comment)specs/001-easycla-ss-integration-fable/m1-my-cla/data-model.md#L5-L5specs/001-easycla-ss-integration-fable/m1-my-cla/plan.md#L8-L10specs/001-easycla-ss-integration-fable/m1-my-cla/spec.md#L21-L27🤖 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 `@specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md` around lines 3 - 5, Replace the superseded M1 contract in specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md:3-5 with the authoritative /v4/my-clas, /v4/my-clas/{signatureID}/pdf, and /v4/my-clas/identities definitions. Update specs/001-easycla-ss-integration-fable/m1-my-cla/data-model.md:5-5 for current signature fields, ECLA classification, response fields, and validity; remove obsolete Self Serve aggregation and authorization requirements from specs/001-easycla-ss-integration-fable/m1-my-cla/plan.md:8-10; and align specs/001-easycla-ss-integration-fable/m1-my-cla/spec.md:21-27 with valid, secondary email, GitLab/Gerrit support, and the current endpoint scope.tests/functional/cypress/e2e/v4/cla-manager.cy.ts (1)
710-710: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore conditional use of
allowFailforfailOnStatusCode.This line sets
failOnStatusCode: falseunconditionally. This removes the ability to fail the test on unexpected HTTP error status codes. If the test relies onallowFailto distinguish expected failures from unexpected ones, this change masks real errors in the CLA Manager creation request.Restore the previous conditional behavior, unless the intent is to always ignore status codes for this specific request.
🔧 Proposed fix
- failOnStatusCode: false, + failOnStatusCode: allowFail ? false : true,🤖 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 `@tests/functional/cypress/e2e/v4/cla-manager.cy.ts` at line 710, The failOnStatusCode property in the CLA Manager creation request is unconditionally set to false, which masks unexpected HTTP errors. Make this setting conditional based on the allowFail parameter so that failOnStatusCode is false only when allowFail is true, allowing the test to properly fail on unexpected status codes when allowFail is false.utils/downcase_emails.sh (3)
1-2: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required source headers to both shell scripts.
The CI header checker applies to shell files.
utils/downcase_emails.sh#L1-L2: add the Linux Foundation copyright header and# SPDX-License-Identifier: MITafter the shebang.utils/lookup_all_logs.sh#L1-L2: add the Linux Foundation copyright header after the shebang and preserve the applicable SPDX header.As per coding guidelines, source files under
cla-backend-goand applicable utility scripts must include the required Linux Foundation copyright header, and new source files must include the MIT SPDX identifier.📍 Affects 2 files
utils/downcase_emails.sh#L1-L2(this comment)utils/lookup_all_logs.sh#L1-L2🤖 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 `@utils/downcase_emails.sh` around lines 1 - 2, Add the required Linux Foundation copyright header immediately after the shebang in utils/downcase_emails.sh lines 1-2, followed by the MIT SPDX identifier. Add the Linux Foundation copyright header after the shebang in utils/lookup_all_logs.sh lines 1-2, preserving its existing applicable SPDX header.Source: Coding guidelines
10-10: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
cat -n utils/downcase_emails.shRepository: linuxfoundation/easycla
Length of output: 1932
Sensitive Data Exposure (CWE-312): Cleartext Storage of Sensitive Information
Reachability: Internal
Write user email data to a secure temporary file with automatic cleanup.
Line 10 writes all user email addresses to
${STAGE}_user_emails.jsonwithout mode restrictions and without cleanup. Email addresses are personally identifiable information. A shared workstation or CI artifact process can expose this data through the persistent file.Apply
umask 077before theaws dynamodb scancommand, create a temporary file usingmktemp, and register anEXITtrap to remove the file when the script terminates:Proposed fix
+umask 077 +email_dump="$(mktemp "${TMPDIR:-/tmp}/${STAGE}_user_emails.XXXXXX.json")" +trap 'rm -f "$email_dump"' EXIT -aws dynamodb scan ... > "${STAGE}_user_emails.json" -cat "${STAGE}_user_emails.json" | jq -c ... +aws dynamodb scan ... > "$email_dump" +jq -c '...' < "$email_dump"🤖 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 `@utils/downcase_emails.sh` at line 10, Update the script around the DynamoDB scan to set a restrictive umask, create the JSON output with mktemp, and store its path for reuse instead of writing directly to the stage-named file. Register an EXIT trap to remove the temporary file on termination, and ensure the scan command writes to that temporary path while preserving its existing query options.
10-26: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent concurrent email updates from being overwritten.
The scan and update form an unguarded read-modify-write. If the application changes
user_emailsafter Line 10, Line 23 replaces that newer value with the stale normalized set.Add a condition that requires
user_emailsto equal the scanned value. Report conditional failures for manual retry.Proposed fix
uid=$(jq -r '.user_id.S' <<<"$item") + oldss=$(jq -c '.user_emails.SS' <<<"$item") newss=$(jq -c '[.user_emails.SS[] | ascii_downcase | gsub("^\\s+|\\s+$";"") | select(length > 0)] | unique' <<<"$item") + values=$(jq -cn --argjson old "$oldss" --argjson new "$newss" \ + '{":old":{"SS":$old},":e":{"SS":$new}}') ... --update-expression 'SET user_emails = :e' \ - --expression-attribute-values "{\":e\":{\"SS\":$newss}}" && echo "ok" + --condition-expression 'user_emails = :old' \ + --expression-attribute-values "$values" && echo "ok"As per path instructions, operational scripts can mutate DynamoDB in the AWS environment selected by the active credentials, including production.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.aws dynamodb scan --profile "$PROFILE" --region "$REGION" --table-name "$TABLE" --projection-expression 'user_id, user_emails' --filter-expression 'attribute_exists(user_emails)' --output json > "${STAGE}_user_emails.json" cat "${STAGE}_user_emails.json" | jq -c '.Items[] | select(.user_emails.SS != null) | ([.user_emails.SS[] | ascii_downcase | gsub("^\\s+|\\s+$";"") | select(length > 0)] | unique) as $n | select(($n | length > 0) and ($n != (.user_emails.SS | sort)))' \ | while IFS= read -r item; do uid=$(jq -r '.user_id.S' <<<"$item") oldss=$(jq -c '.user_emails.SS' <<<"$item") newss=$(jq -c '[.user_emails.SS[] | ascii_downcase | gsub("^\\s+|\\s+$";"") | select(length > 0)] | unique' <<<"$item") # lower + trim + drop-empty + dedupe values=$(jq -cn --argjson old "$oldss" --argjson new "$newss" \ '{":old":{"SS":$old},":e":{"SS":$new}}') if [ "$newss" = "[]" ] then echo "skip $uid (no valid emails after normalize)" >&2 continue fi echo "user $uid -> $newss" if [ "$APPLY" = "1" ] then aws dynamodb update-item --profile "$PROFILE" --region "$REGION" --table-name "$TABLE" \ --key "{\"user_id\":{\"S\":\"$uid\"}}" \ --update-expression 'SET user_emails = :e' \ --condition-expression 'user_emails = :old' \ --expression-attribute-values "$values" && echo "ok"🤖 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 `@utils/downcase_emails.sh` around lines 10 - 26, Update the update-item operation in the normalization loop to condition the write on user_emails still matching the scanned item’s original user_emails.SS value, while preserving the normalized :e update. Capture conditional-check failures separately from successful updates and report the affected uid for manual retry instead of treating them as successful writes.Source: Path instructions
utils/get_user_svc.sh (1)
51-59: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
cd /repo && pwdRepository: linuxfoundation/easycla
Length of output: 1932
🏁 Script executed:
# First, check if the file exists and examine lines 51-59 and 70-73 if [ -f "utils/get_user_svc.sh" ]; then echo "=== Lines 51-59 ===" sed -n '51,59p' utils/get_user_svc.sh echo "" echo "=== Lines 70-73 ===" sed -n '70,73p' utils/get_user_svc.sh else echo "File not found" fiRepository: linuxfoundation/easycla
Length of output: 1932
🏁 Script executed:
# Create a simple script to verify curl behavior with different flags python3 - <<'PY' import subprocess import sys # According to curl documentation: # -s: silent mode (no progress meter) # -S: show errors even in silent mode # -f: fail on server errors (HTTP 4xx, 5xx) # Test 1: Check curl man page or help for -f flag behavior result = subprocess.run(['curl', '--help'], capture_output=True, text=True) help_text = result.stdout + result.stderr # Find the -f flag documentation for line in help_text.split('\n'): if ' -f,' in line or line.strip().startswith('-f'): print(f"curl -f documentation: {line}") # Also check explicit behavior description print("\n--- Key curl behavior ---") print("curl -sS: silent mode, but still return 0 for HTTP error codes") print("curl -fsS: fail (non-zero exit) on HTTP 4xx/5xx, silent mode") print("\nThe -f flag is critical for catching HTTP errors in scripts.") PYRepository: linuxfoundation/easycla
Length of output: 1932
🏁 Script executed:
# Get the full context of the file around those lines head -80 utils/get_user_svc.sh | tail -35Repository: linuxfoundation/easycla
Length of output: 1932
🏁 Script executed:
# Also check git log for Signed-off-by requirement cd /repo && git log --oneline -10 -- utils/get_user_svc.sh 2>/dev/null || echo "No git history available"Repository: linuxfoundation/easycla
Length of output: 1932
🌐 Web query:
curl -f flag fail on HTTP errors exit code documentation💡 Result:
When using curl, the command line flag to make it return a non-zero exit code on HTTP errors (specifically for HTTP response codes 400 or greater) is -f or --fail [1][2]. When this flag is used, if the server returns an HTTP status code indicating an error (400 or above), curl will exit with error code 22 [3][1]. By default, curl does not consider HTTP response codes to be failures and will continue processing or return a success exit code (0) even if the server returns an error page [1][2]. Key details regarding this behavior: 1. No Output: When using --fail, curl does not output the response body when an HTTP error occurs [1][2]. 2. Alternative for Content: If you want to receive an error code 22 on HTTP failures while still saving or outputting the response body (the error page returned by the server), use --fail-with-body instead of --fail [4][1][2]. 3. Scope: Note that --fail is not a global option and is contained by --next [4][2]. If you are performing multiple transfers, you may need to ensure the flag is applied correctly to the relevant URLs. 4. Limitations: The --fail option is not entirely fail-safe; in some cases, such as certain authentication scenarios (e.g., response codes 401 and 407), the request may still appear to succeed in curl's logic [1][2]. If you are writing scripts, checking for exit code 22 is the standard way to detect these specific HTTP-based failures when the --fail flag is active [3].
Citations:
- 1: https://man.archlinux.org/man/curl.1
- 2: https://curl.se/docs/manpage.html
- 3: https://everything.curl.dev/cmdline/exitcode.html
- 4: https://man7.org/linux/man-pages/man1/curl.1.html
🌐 Web query:
curl -sS vs -fsS difference HTTP error handling💡 Result:
In the context of curl, HTTP error handling is governed by a distinction between transport-level failures and application-level HTTP status codes [1][2]. By default, curl only considers transport-level issues (such as inability to resolve a host, timeout, or lost connection) as failures [1][2]. If a server successfully processes a request and returns an HTTP status code in the 4xx or 5xx range, curl treats this as a successful transfer, meaning it does not return a non-zero exit code [3][1][4]. To change this behavior and force curl to exit with a non-zero status when receiving 4xx or 5xx HTTP responses, you must explicitly enable error handling via command-line flags [1][5]: 1. --fail (-f): Instructs curl to exit with error code 22 if the server returns an HTTP status code of 400 or higher [1][6][5]. When used, curl suppresses the response body, which is often useful if you only need to verify success [6][2]. 2. --fail-with-body: Similar to --fail, this also causes curl to exit with error code 22 for HTTP errors >= 400, but it preserves and outputs the response body [6][5][2]. This is preferred for debugging or when the server's error response contains necessary information [2]. At the library level (libcurl), the behavior is similar. The library does not trigger an error for HTTP 4xx/5xx responses unless the
CURLOPT_FAILONERRORoption is set to 1L [3]. When enabled, libcurl will return the error codeCURLE_HTTP_RETURNED_ERROR[3][7]. In summary, the key difference is that curl separates the success of the underlying HTTP protocol exchange (which is always successful if a response is received) from the meaning of the HTTP status code itself (which may represent an application-level error) [1][2]. Users must explicitly bridge this gap if they want HTTP errors to trigger shell exit codes [8][2].Citations:
- 1: https://everything.curl.dev/http/response.html
- 2: https://www.simplified.guide/curl/http-fail-error
- 3: https://github.com/curl/everything-curl/blob/master/libcurl-http/responses.md
- 4: https://everything.curl.dev/cmdline/exitcode.html
- 5: https://curl.se/docs/manpage.html
- 6: https://man.archlinux.org/man/curl.1
- 7: https://github.com/curl/curl/blob/master/lib/strerror.c
- 8: curl/curl#3789
Add curl -f flag to fail on HTTP errors.
By default,
curl -sSreturns exit code 0 for HTTP 4xx and 5xx responses. The profile lookup then printsno user-service profile foundand exits 0 instead of failing. The pagination loop also treats error responses as the final empty page.Add the
-fflag to make curl exit with code 22 on HTTP errors >= 400.Proposed fix
-profile="$(curl -sS -G -XGET "${hdr[@]}" --data-urlencode "username=${lfid}" "${GW}/user-service/v1/users")" +if ! profile="$(curl -fsS -G -XGET "${hdr[@]}" \ + --data-urlencode "username=${lfid}" \ + "${GW}/user-service/v1/users")" +then + echo "user-service profile request failed" >&2 + exit 3 +fi ... - page="$(curl -sS -XGET "${hdr[@]}" "${GW}/user-service/v1/users/${sfid}/identities?pageSize=100&offset=${offset}")" + if ! page="$(curl -fsS -XGET "${hdr[@]}" \ + "${GW}/user-service/v1/users/${sfid}/identities?pageSize=100&offset=${offset}")" + then + echo "user-service identities request failed at offset ${offset}" >&2 + exit 3 + fi🤖 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 `@utils/get_user_svc.sh` around lines 51 - 59, Update the profile lookup curl invocation in get_user_svc.sh to include the -f/--fail flag alongside the existing silent and show-error options. Ensure HTTP 4xx/5xx responses cause curl to fail so the surrounding profile and pagination flows do not treat error responses as empty results.utils/local-fbsd-api.sh (1)
5-10: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop when directory setup or the build fails.
If
cdorgo buildfails, the script continues. It can source the wrong environment file and execute an existing stale binary.Use strict mode and perform the build in a subshell.
Proposed fix
#!/bin/bash +set -euo pipefail ... -cd cla-backend-go -go build -o bin/cla-fbsd main.go -cd .. +(cd cla-backend-go && go build -o bin/cla-fbsd main.go) source setenv.sh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.set -euo pipefail (cd cla-backend-go && go build -o bin/cla-fbsd main.go) source setenv.sh # source setenv-prod.sh.secret GH_ORG_VALIDATION=false PORT=8080 ./cla-backend-go/bin/cla-fbsd🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 5-5: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[info] 7-7: Use a ( subshell ) to avoid having to cd back.
(SC2103)
[info] 8-8: Not following: setenv.sh was not specified as input (see shellcheck -x).
(SC1091)
🤖 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 `@utils/local-fbsd-api.sh` around lines 5 - 10, Update the setup and build flow in the local fBSD launch script to enable strict shell failure handling and run the `cd cla-backend-go` plus `go build` commands in a subshell. Ensure any failed directory change or build terminates the script before sourcing environment files or executing the binary.Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md`:
- Line 20: Resolve the selection contract across FR-002, Acceptance Scenario 3,
and Open Question 1: choose either Self Serve narrowing the CLA Group to a
relevant org/repo or deferring that selection to Contributor Console. Update all
three references consistently so the requirement, scenario, and open question
describe the same behavior, and remove the ambiguity.
- Line 43: Update the scope wording to use the required “Approved List”
terminology, replacing the hyphenated “approval-list” reference while preserving
the existing M4 exclusion meaning. Do not introduce “whitelist” or alter any
other scope items.
- Line 9: Preserve the blockquote continuity at the blank line on line 9 of the
specification by prefixing that blank line with “>”, or remove the blank line
entirely so both notes render as one blockquote.
- Line 14: Standardize the corporate agreement terminology across the user
story, acceptance scenarios, and FR-004 by choosing one canonical sign-type
value consistent with project_ccla_enabled. If ECLA remains user-facing,
explicitly define its mapping to CCLA for the agreement picker and Contributor
Console hand-off, and apply that mapping consistently throughout the
specification.
- Line 33: Update FR-008 and the hand-off contract specification to require
server-side identity derivation from the Console session, never client-supplied
user IDs or query parameters. Define carried context such as CLA Group ID, sign
type, and org/repo as either session-bound through a server-validated one-time
signed token or non-identifying metadata, and verify the Console session model
enforces ownership before opening the signing flow.
🪄 Autofix
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: 396bf8f3-cc63-4e00-acd5-75c6e024fe95
📒 Files selected for processing (2)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.mdspecs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md
🚧 Files skipped from review as they are similar to previous changes (1)
- specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md
…ied facts
Verified against easycla-contributor-console and cla-backend-go:
- Hand-off reuses the Console's existing decision-screen deep link
(/#/cla/project/{claGroupID}/user/{userID}, ?redirect= optional) — no new
hand-off contract. The ICLA/ECLA choice, legal guidance, and flag gating
stay in the Console; SS does not re-implement them.
- Drop org/repo selection: the signing unit is the CLA Group; org/repo names
are at most search metadata in the picker.
- userID resolved server-side via existing GET /v4/user-from-token
(lookup-or-create by LF username/email).
Adds constraints: simple/straightforward design, independently deliverable
in ~2 weeks.
Rewrites open questions from code evidence:
1. GitHub identity binding (primary) — proactive ICLA must land on a user
record the PR check can match (GitHub ID → username → email); user-from-token
records carry no GitHub identity. Recommended: require M1's GitHub-account
linking for the ICLA path.
2. Proactive-ICLA active-signature gap — Console + backend assume PR-derived
context on the GitHub ICLA path (active-signature record, repository_id/
pull_request_id for the DocuSign callback); Gerrit path proves a no-PR
shape works. ECLA path has no PR dependency.
3. CLA-Group listing endpoint (unchanged).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md (1)
39-42: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the success criterion cover the signing outcome.
SC-002 measures only arrival at the decision screen. It can pass while Scenario 3 fails during identity binding or active-signature creation. Add a criterion for ICLA completion and PR-check matching, or state that M2 success stops at the hand-off.
🤖 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 `@specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md` around lines 39 - 42, Update success criterion SC-002 in the Success Criteria section to cover the full signing outcome, including successful ICLA completion and matching PR checks after identity binding and active-signature creation; alternatively, explicitly redefine M2 success as ending at the Contributor Console hand-off.
🤖 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 `@specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md`:
- Line 19: Resolve the M2 specification inconsistency by either adding GitHub
identity binding and no-PR active-signature support to the M2 requirements and
flow, or removing/defering Scenario 3 and FR-008 with explicit interim behavior.
Update the affected sections consistently, including the contributor handoff
flow and all referenced requirements, so proactive ICLA signing can produce a
PR-matchable completed record.
---
Nitpick comments:
In `@specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md`:
- Around line 39-42: Update success criterion SC-002 in the Success Criteria
section to cover the full signing outcome, including successful ICLA completion
and matching PR checks after identity binding and active-signature creation;
alternatively, explicitly redefine M2 success as ending at the Contributor
Console hand-off.
🪄 Autofix
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: e83e6017-54a3-4ed6-a8ff-902e3a515d4b
📒 Files selected for processing (2)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.mdspecs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md
🚧 Files skipped from review as they are similar to previous changes (1)
- specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:32
- FR-002/FR-003 silently resolve two questions that the PR description and linked program PR #5132 still call unresolved: whether org/repo narrowing is required and what the no-PR Console hand-off contract is. The parent spec currently requires selection/pre-scoping by sign type and allows org/repo narrowing, whereas this slice removes both without recording a subsequent product decision. Keep these as clarification items, or update the PR description and parent artifacts with the decision that supersedes them before treating this as the extracted implementation contract.
- **FR-002**: The picker MUST let the user find and select a CLA Group — a simple searchable list (project/CLA Group names). No org/repo selection step: the signing unit is the CLA Group, and the Console hand-off URL carries only `claGroupID` + `userID`. Org/repo names MAY be used as search metadata to help users find the right CLA Group, nothing more.
- **FR-003**: On selection, Self Serve MUST hand off to the Contributor Console's existing decision-screen URL — `{console}/#/cla/project/{claGroupID}/user/{userID}` — the same shape the PR-check link uses, without the optional `?redirect=` param (there is no PR to return to). The ICLA/ECLA choice, its legal guidance text, and the `project_icla_enabled`/`project_ccla_enabled` gating stay in the Console; SS MUST NOT re-implement them.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:63
- These Angular paths describe the pre-implementation M1 plan rather than the current Self Serve layout. M1 landed as
app/modules/profile/clas, registered inapp/modules/profile/profile.routes.ts, with its feature-gated tab wired through the profile layout/sidebar; there is noapp/modules/my-classurface to mirror. Targetingmain-layout.component.tsand top-levelapp.routes.tswould bypass the established Profile & Account integration, so update the structure after deciding whether Sign a CLA is a profile child or another current Me-lens surface.
├── app/modules/sign-cla/ # NEW Me-lens page: searchable CLA Group list → hand-off
├── app/layouts/main-layout/main-layout.component.ts # EDIT: add Me-lens entry behind flag
├── app/app.routes.ts # EDIT: register the route (lens: 'me', flag-guarded)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:79
- An ECLA-only fallback does not satisfy this PR's stated M2 scope. The spec's acceptance scenario 3 and FR-008 require proactive ICLA completion, and the PR description explicitly includes both ICLA and CCLA paths; deferring the no-PR ICLA change would ship a picker whose Individual path is known to fail. Make resolution of this gap a launch blocker, or formally revise the milestone scope and acceptance criteria with PM approval.
2. **Proactive-ICLA gap** (spec open question 2) — Console + backend delta; decide in/out of M2 at `/speckit.clarify`. ECLA works proactively with zero changes, so a worst-case fallback exists but weakens the milestone.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:23
- “No EasyCLA writes” is incompatible with FR-004 because
user-from-tokenis explicitly lookup-or-create and may also refresh stored identity fields. Narrow this prohibition to signing-related writes while explicitly allowing the required provisioning operation.
**Constraints**: no EasyCLA writes and no signing-initiation calls from SS (FR-005); server-side identity derivation only (never trust client-supplied user IDs); PR-check remediation link untouched (FR-006); feature-flagged dark launch; 2-week delivery budget.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:18
- The storage statement conflicts with the required
GET /v4/user-from-tokenlookup-or-create flow. That middleware updates an existing identity and callsCreateUserwhen no record exists (cla-backend-go/cmd/server.go:933-1068), so EasyCLA DynamoDB is not untouched. Distinguish “no SS-owned storage/schema” from the permitted EasyCLA user-provisioning write.
**Storage**: none in SS (stateless: list CLA Groups, resolve `userID`, redirect). EasyCLA DynamoDB + S3 untouched.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:19
- The current M1 CLA server service/controller tests use Vitest (
cla.service.spec.tsandclas.controller.spec.ts), not Jest/Karma. Leaving the harness unresolved sends/speckit.taskstoward the wrong tooling even though the exact adjacent tests are already available; name Vitest for server tests and reference the current Angular harness separately.
**Testing**: lfx-self-serve conventions — Jest/Karma unit tests for services/components, server route tests, Cypress/E2E per repo norms (verify exact harness during implementation).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:12
- This says there are two clarification items, but the spec defines three and the plan later tracks CLA-Group listing as open question 3. Include the listing source here so the summary does not omit a required Phase 0 decision.
The SS-side build is deliberately thin: one page, one or two server routes, one redirect. The milestone's real design work is the two `/speckit.clarify` items in [spec.md](spec.md): **GitHub identity binding** (a proactive ICLA must land on a user record the PR check can match — recommended: require M1's GitHub-account linking for the ICLA path) and the **proactive-ICLA active-signature gap** (Console + backend assume PR-derived context on the GitHub ICLA path; the Gerrit path proves a no-PR shape already works). Both are scoped and evidenced in the spec's "Verified Console/backend facts" section; neither is resolved here.
…count auth
Incorporate the M2 UI mockup (easyclav2-migration-planning, MyCLAs v8) as the
UI source of truth. M2 is now M1's My CLAs page extended with:
- "+ Sign a CLA" inline search (project / CLA group / repo source incl.
GitLab & Gerrit) → hand-off to the Console decision screen, unchanged
from the prior spec, plus a pre-hand-off account-authorization step
(mockup's note) — adopted as the direction for the identity-binding
question; per-platform mechanics remain open.
- CLA invalidation per row: ICLA via existing PUT /v4/cla-group/{id}/user/
{id}/icla (verified: handler has no ownership check → SS enforces
self-only); ECLA ("Confirm you no longer work at {company}?") needs a new
backend endpoint — flagged as the main schedule risk.
- Invalidation blocked server-side during SS impersonation, reusing the
existing impersonation-readonly middleware (FR-009).
- Status column (Valid / Needs attention / Invalidated) with
"Request approval →" deep link into the Console; approval-criteria
evaluation source is an open question.
Scope decision (2026-08-05): all mockup features are M2; schedule risks
noted in plan.md with independent degradation order if the 2-week budget
forces a cut.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:9
- This is not currently an extraction of the linked program-level M2 contract. PR #5132's revised User Story 2/FR-011–012 requires Self Serve to choose the sign type and, where applicable, org/repo before handing off, and it does not include invalidation or status. This slice instead delegates sign-type choice, excludes org/repo selection, and adds two substantial backend-backed features. Align the parent PR and this slice before Spec Kit consumes competing definitions of M2.
This is the extracted, implementable slice for Milestone 2. Program-wide context, assumptions, and resolved decisions live in the parent spec; this file is what `/speckit.plan`, `/speckit.tasks`, and `/speckit.implement` operate on.
> **Scope (revised 2026-08-04, per Heather/PM; UI per mockup v8)**: M2 extends M1's **My CLAs** page with three additions — a proactive "Sign a CLA" entry that hands off to the existing Contributor Console, per-CLA **invalidation**, and a richer **status** column. Self Serve never runs the DocuSign ceremony; nothing is cut over or retired; the PR-check remediation link is unchanged.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:42
GET /v4/user-from-tokencannot by itself satisfy FR-004: it resolves only LF username/email and may create an LF-only record, while M1 deliberately aggregates platform-linked records because commit attribution uses GitHub/GitLab/Gerrit identities. Passing that LF-only user ID to the Console can bind the new signature to a record that contribution checks do not resolve. Make user-ID selection depend on the authorized platform identity/binding contract rather than mandating this endpoint before that contract is settled.
- **FR-003**: Self Serve MUST resolve the EasyCLA `userID` server-side from the session identity via the existing `GET /v4/user-from-token` endpoint (lookup-or-create). No client-supplied user IDs.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:85
- The evaluation source is already implemented:
GET /v4/my-clasevaluates each ECLA against its current CCLA and approval lists and returnsvalid(v2/my_clas/service.go:214-219,665-714). The actual missing contract is a backend-derived reason that distinguishes invalidation from an approval-criteria failure for the explanatory note. Reframe this question accordingly to avoid designing a duplicate evaluation endpoint.
3. **Status evaluation** — source for "no longer matches approval criteria": extend `GET /v4/my-clas` vs. a separate check; must not require SS to re-implement approval-list logic. Second schedule risk.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:82
- The proactive ICLA entry cannot ride existing endpoints: this spec establishes that the GitHub ICLA path requires a PR-created active-signature record and repository/PR metadata. Deferring the no-PR Console/backend delta while retaining sign entry would make the required ICLA path fail, so the fallback must treat that delta as mandatory or explicitly narrow the shipped hand-off to ECLA.
If the budget forces a cut, the mockup's pieces degrade independently: sign entry + ICLA invalidation ride existing endpoints; ECLA invalidation and status enrichment are the deferrable slices — decide at `/speckit.plan`.
Review Feedback Addressed (round 2)Commit: d2c322f All five copilot-pull-request-reviewer findings were verified against Changes Made
Threads Resolved5 of 5 unresolved threads addressed in this iteration. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Suppressed comments (10)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:50
- This invalidation is not durable while the Approved List remains unchanged. When
auto_create_eclais enabled, a later Approved List update processes every still-listed employee and callsValidateProjectRecordfor an unapproved ECLA (signatures/service.go:890-910), silently reactivating the record the contributor invalidated. Define either a durable self-opt-out honored by auto-create/authorization or removal of the matching Approved List entry; otherwise “ends their coverage” is not guaranteed.
- **FR-008**: Each valid ECLA row MUST offer Invalidate framed per the mockup — "This confirms you're no longer covered under {company}'s Corporate CLA (CCLA) for {project}, and marks your ECLA as invalid" (typed INVALIDATE confirmation). The company's Approved List is not mutated; the company's CLA managers are notified so they can update it. *([NEEDS CLARIFICATION]: no self-service ECLA-invalidation endpoint exists — new `cla-backend-go` work; see Open questions.)*
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:81
- Status evaluation is not retired by exposing the existing boolean.
covered=falsealso represents sanctioned companies and missing/inactive CCLAs, so the UI cannot derive the required explanatory note or decide whether “Request approval” applies. Keep this as backend work to expose a machine-readable coverage reason.
4. **Status evaluation** (open question 3) — retired: `GET /v4/my-clas` already computes the coverage evaluation per ECLA row; M2 exposes it as a status field instead of collapsing it into `Valid`.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:56
- A single
covered=falsevalue cannot support these explanations. The current evaluator also returns false for a sanctioned company and for a missing/inactive CCLA (v2/my_clas/service.go:678-695), not only for an Approved List mismatch; mapping all of those cases to the same status would show misleading copy and remediation. Expose a reason code so the UI can distinguish the failure modes.
- **FR-010**: Each row MUST show a status: **Valid** (approved and covered); **Needs attention** with an explanatory note (ECLA signed but no longer matching the company's Approved List criteria — approved but not covered); **Invalidated** (not approved). Source: extend `GET /v4/my-clas`, which already computes the coverage evaluation per ECLA row and currently collapses it into a single boolean (resolved — see open question 3).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:57
- The existing request-authorization route is not independently deep-linkable. Its component reads the selected company and project from Console browser storage; a user arriving directly from Self Serve has neither value, so managers are not loaded and submission dereferences missing state. Require a Console change or route through the dashboard with the needed context.
- **FR-011**: "Needs attention" ECLA rows MUST link "Request approval →" into the Contributor Console's existing request-authorization flow (deep link; no new SS flow).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:41
- The linked parent milestone in PR #5132 still requires Self Serve to choose ICLA/ECLA and optionally select an org/repo before hand-off, while this requirement leaves both choices to the Console. Since both documents are presented as authoritative program context, they would give Spec Kit contradictory implementation scope. Update the parent milestone or explicitly mark its clauses as superseded before these links resolve.
- **FR-002**: On selection, Self Serve MUST hand off to the Contributor Console's existing decision-screen URL — `{console}/#/cla/project/{claGroupID}/user/{userID}` — without the optional `?redirect=` param. The ICLA/ECLA choice, legal guidance, and `project_icla_enabled`/`project_ccla_enabled` gating stay in the Console; SS MUST NOT re-implement them.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:4
- The PR description identifies mockup v8 as the source of truth, but this file switches the acceptance baseline to Final/v16 and says v8 is superseded. Update the PR description or reconcile the document so reviewers and Spec Kit use the same UI contract.
**UI mockup (source of truth)**: [EasyCLA_MyCLAs_Full_Prototype_Final.html](https://github.com/linuxfoundation/easyclav2-migration-planning/blob/main/Mockups/M2/EasyCLA_MyCLAs_Full_Prototype_Final.html) (internally v16; supersedes v8/v10)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:94
- This narrowed resolver only handles GitHub repository names/URLs, but FR-001 and the user story also promise Gerrit-backed repository search and pasted repository links. Without a Gerrit resolution path, that accepted input cannot select a Gerrit CLA Group. Define the Gerrit equivalent or explicitly narrow the earlier requirements to GitHub URLs.
5. **CLA-Group listing (narrowed 2026-08-08)** — search = CLA-group/project **names** from the existing project↔CLA-group mappings, plus an **exact repo-name/URL resolver**: the user types a repo name or pastes a GitHub URL, SS parses `owner/repo` and resolves it via the existing `RepositoryNameIndex` GSI (`GitHubGetRepositoryByName`) → `cla_group_id`. No fuzzy full-text search over repo names. Remaining question is only which existing listing endpoint to reuse for the name search.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:8
- This summary assumes the existing request-authorization flow can be deep-linked, but that route depends on company/project objects already stored by the Console's corporate dashboard. A direct Self Serve link starts without that browser state and cannot load managers or submit. Include the required Console routing/state-loading delta in the plan.
Extend M1's **My CLAs** page (Me lens) per the mockup: (1) a "Sign CLA" modal search (project / CLA group / repo source / pasted repo link) that resolves the user's EasyCLA `userID` server-side (existing `GET /v4/user-from-token`) and — after an account-authorization step for the platform they'll contribute with — redirects to the Contributor Console's existing decision-screen URL (`{console}/#/cla/project/{claGroupID}/user/{userID}`); (2) per-row **CLA invalidation** with confirmation modals (ICLA via a revised `invalidateICLA` flow — signature-ID-targeted, actor-aware side effects, SS-side ownership enforcement; ECLA needs a new backend endpoint), blocked server-side during impersonation via SS's existing impersonation-readonly middleware; (3) a **status** column (Valid / Needs attention / Invalidated) with a "Request approval →" deep link into the Console for ECLAs that no longer match Approved List criteria.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:69
- The status extension is no longer conditional: FR-010 requires a richer status and open question 3 is marked resolved. Leaving it under “Conditional” can omit the API field that the UI depends on. Move the
GET /v4/my-classtatus/reason extension into the mandatory backend work.
This issue also appears on line 81 of the same file.
Secondary repo: `linuxfoundation/easycla` `cla-backend-go` — new self-service ECLA-invalidation endpoint, swagger-first (`swagger/cla.v2.yaml` → `make swagger` → handler/service), no schema changes expected. Conditional (per clarify): status-evaluation extension to `GET /v4/my-clas`; no-PR ICLA request shape in `v2/sign` (+ matching `easycla-contributor-console` tweak).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:77
- These settled semantics do not prevent reactivation. With
auto_create_ecla, a later Approved List update revalidates an unapproved employee signature that still matches the unchanged list (signatures/service.go:890-910). Track the durable self-opt-out/removal decision as a design risk; otherwise the new endpoint cannot guarantee that self-invalidation ends coverage.
1. **ECLA-invalidation endpoint** (spec open question 2) — the one guaranteed new backend piece; semantics settled (per-signature flag flip, no Approved List mutation, notifications, backend ownership check) — execution risk only.
…round-2 fixes - spec.md + plan.md: delivery budget ~2 weeks -> ~3 weeks (per PM) - spec.md acceptance scenario 3: signature-ID-targeted invalidation with self-invalidation event reason (was "existing endpoint" — contradicted the revised FR-007) - plan.md: summary resolution wording matches platform-aware FR-003; constitution Simplicity gate reflects the ICLA-flow revision; open-question count references updated (1/3 resolved, 5 narrowed) Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:56
- A single
approved && !coveredstatus cannot support the required explanatory note or decide whether “Request approval” is applicable. The current helper returnsfalsenot only for an Approved List mismatch, but also for a missing/sanctioned company and a missing signed CCLA (v2/my_clas/service.go:678-696); in those cases requesting approval cannot restore coverage. Require a machine-readable coverage reason/action (or distinct statuses) from/v4/my-clasrather than only exposing the collapsed boolean asNeeds attention.
- **FR-010**: Each row MUST show a status: **Valid** (approved and covered); **Needs attention** with an explanatory note (ECLA signed but no longer matching the company's Approved List criteria — approved but not covered); **Invalidated** (not approved). Source: extend `GET /v4/my-clas`, which already computes the coverage evaluation per ECLA row and currently collapses it into a single boolean (resolved — see open question 3).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:16
- The status response extension and no-PR ICLA shape are not optional clarify outcomes for the stated scope. FR-010 requires the former, and the plan's own lines 78 and 83 confirm the latter is required for the GitHub acceptance path. Calling both “possibly” needed understates the required Go work.
**Language/Version**: TypeScript (Angular 20.3 frontend + Node 22 / Express 4 SSR server) in `lfx-self-serve`; Go 1.25 in `cla-backend-go` for the ECLA-invalidation endpoint (and possibly status evaluation / no-PR ICLA shape, per clarify outcomes).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:24
- This scope estimate still treats the status extension as optional and omits the required no-PR ICLA backend delta. Both are mandatory for FR-010 and the GitHub sign-entry acceptance path, so they need to be included in the backend scope rather than left to clarification.
**Scale/Scope**: all LFX contributors; extends 1 existing Me-lens page, ~3–4 server routes (CLA-Group search, `userID` resolution/hand-off, ICLA invalidate, ECLA invalidate), 1 new upstream endpoint (ECLA invalidation) + possibly a status/listing extension per clarify outcomes.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:69
- These two backend changes are mandatory, not conditional: FR-010 requires extending
/v4/my-clas, and lines 78/83 establish that no-PR ICLA support is required for the in-scope GitHub path. Leaving them under “Conditional” contradicts the completed risk analysis and can cause the implementation plan to omit required work.
Secondary repo: `linuxfoundation/easycla` `cla-backend-go` — new self-service ECLA-invalidation endpoint, swagger-first (`swagger/cla.v2.yaml` → `make swagger` → handler/service), no schema changes expected. Conditional (per clarify): status-evaluation extension to `GET /v4/my-clas`; no-PR ICLA request shape in `v2/sign` (+ matching `easycla-contributor-console` tweak).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:86
- This open question simultaneously says the per-platform mechanism is still to be clarified and then adopts that mechanism in the next bullet (M1 Auth0 linking for GitHub, none for Gerrit, configuration for GitLab). State only the remaining decision—such as the end-to-end binding contract and
updateUsergateway authorization—so/speckit.clarifydoes not revisit an adopted matrix or mistakenly treat all mechanics as settled.
1. **Account-authorization mechanics (direction adopted)** — the mockup resolves the *what*: users authorize their GitHub/GitLab/Gerrit account before the Console opens. To clarify: the *how* per platform (Auth0 identity linking as in M1 vs. an OAuth step in the flow), and how the authorized identity gets bound to the EasyCLA user record used in the hand-off (existing GitHub-anchored record vs. enriching the LF-created one). Sub-decisions already reasoned through (2026-08-07):
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:36
- Question 1 is not resolved: the spec still leaves the end-to-end account-binding mechanics and
updateUsergateway authorization for clarification. Labeling it resolved here contradicts the open-question ledger and can let the post-design security re-check pass without that contract.
**Post-Phase-1 re-check**: pending — re-run after `/speckit.clarify` confirms the remaining open items in [spec.md](spec.md) (questions 1 and 3 resolved; 5 narrowed; 2 and 4 are execution deliverables).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:80
- Account authorization cannot be marked retired while the spec still requires clarification of the binding contract and verification that SS can call the authenticated v1
updateUserendpoint. Keep this as a narrowed risk until those mechanics are confirmed.
3. **Account-authorization mechanics** (open question 1) — retired: Gerrit needs no step (same LF SSO), GitLab is conditional on SS shipping GitLab linking (config flip, M2 doesn't block), GitHub reuses M1's linking + picker; first-timer enrichment is a call to the existing v1 `updateUser` API.
Address unresolved review comments from copilot-pull-request-reviewer: - spec.md FR-008: note the durability caveat — a per-signature flag flip is not durable while the user stays on the Approved List (auto_create_ecla re-approves via ValidateProjectRecord, signatures/ service.go:895-900; ProcessEmployeeSignature re-authorizes, :1562-1588). The new ECLA endpoint must apply a persistent self-exclusion, not just flip signature_approved. - spec.md FR-007(a): backend ownership enforcement is now mandatory on the revised ICLA endpoint (v4 is independently reachable); SS self-only check retained as defense in depth. - plan.md Constitution Check (Security): make backend signature-ownership mandatory for both the revised ICLA and new ECLA endpoints, SS as defense in depth. - spec.md: add supersession note — this slice is the authoritative M2 contract where it diverges from the program-level M2 brief in #5132. Resolves 4 review threads. Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Review Feedback AddressedCommit: d5a115f Changes Made
Threads Resolved4 of 4 unresolved threads addressed in this iteration. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Suppressed comments (7)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:17
- This constraint conflicts with FR-008. A plain
signature_approvedflip is revalidated, while Approved List mutation is prohibited, so durable self-invalidation needs persisted state unless an existing durable mechanism is identified. Permit the narrowly scoped self-exclusion state required by FR-008.
- **Simple and straightforward** — no new services, no new state, no bespoke contracts where an existing one works. Prefer reusing what the Console and EasyCLA backend already do.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:93
- This “per-signature flag flip” contradicts FR-008, which correctly establishes that such a flip is automatically reversed while the employee remains listed. The settled semantics must include the persistent self-exclusion honored by revalidation and PR gating, or implementation may follow this stale requirement and fail to end coverage.
2. **ECLA invalidation endpoint** — new `cla-backend-go` API (swagger-first) for an employee ending their own CCLA coverage. Semantics settled per FR-008/FR-008a and the invalidation preamble: per-signature flag flip, record kept for audit, no Approved List mutation, CLA managers + user notified, backend-side ownership check (don't replicate `invalidateICLA`'s missing check). Main remaining backend deliverable alongside open question 4.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:94
- The resolved formula repeats the status-classification bug in FR-010. Approved List removal currently flips
signature_approvedto false, soInvalidated = ¬approvedcannot represent the required “Needs attention” state for a signed ECLA that no longer matches the list. The endpoint needs a distinct invalidation reason/marker rather than deriving all three statuses from these two booleans.
3. **Status evaluation — RESOLVED (2026-08-08)**: extend `GET /v4/my-clas`. The Approved List evaluation already runs there — `eclaCoveredByCurrentApprovalList` (mirroring the PR-gating `UserIsApproved`) is computed per ECLA row and then collapsed into `row.Valid = signature_approved && covered` (`v2/my_clas/service.go`). M2 exposes the distinction as a status field instead of collapsing it: **Valid** = approved ∧ covered; **Needs attention** = approved ∧ ¬covered (ECLA); **Invalidated** = ¬approved. Known carried-over caveat: GitLab group membership can't be evaluated (needs per-group OAuth) and defers to the flag — acceptable, and moot while GitLab is deferred.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:69
- “No schema changes expected” is incompatible with FR-008 unless an existing durable exclusion field is identified. Since
signature_approvedis revalidated and Approved List mutation is out of scope, the plan must account for a possible signature model/schema extension rather than excluding it.
Secondary repo: `linuxfoundation/easycla` `cla-backend-go` — new self-service ECLA-invalidation endpoint, swagger-first (`swagger/cla.v2.yaml` → `make swagger` → handler/service), no schema changes expected. Conditional (per clarify): status-evaluation extension to `GET /v4/my-clas`; no-PR ICLA request shape in `v2/sign` (+ matching `easycla-contributor-console` tweak).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:58
- This status mapping does not satisfy acceptance scenario 6 for normal Approved List removals. That flow calls
InvalidateProjectRecordand setssignature_approved=false(signatures/repository.go:4131-4178), so the proposed rule labels the row “Invalidated,” not “Needs attention.” Define status using an explicit invalidation/revocation reason or marker so loss of current coverage can be distinguished from user invalidation.
This issue also appears on line 94 of the same file.
- **FR-010**: Each row MUST show a status: **Valid** (approved and covered); **Needs attention** with an explanatory note (ECLA signed but no longer matching the company's Approved List criteria — approved but not covered); **Invalidated** (not approved). Source: extend `GET /v4/my-clas`, which already computes the coverage evaluation per ECLA row and currently collapses it into a single boolean (resolved — see open question 3).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:36
- The ledger incorrectly says question 1 is resolved. The spec still explicitly leaves per-platform mechanics for clarify (line 88) and gateway/ownership behavior for the enrichment call to clarify (line 92). Mark it as partially resolved so Spec Kit does not skip required identity-binding work.
**Post-Phase-1 re-check**: pending — re-run after `/speckit.clarify` confirms the remaining open items in [spec.md](spec.md) (questions 1 and 3 resolved; 5 narrowed; 2 and 4 are execution deliverables).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:96
- This resolver only covers GitHub
owner/reponames and URLs, but FR-001 and the user story also include Gerrit-backed repository sources and pasted repository links. A Gerrit URL cannot resolve throughRepositoryNameIndex; define its resolver or explicitly restrict pasted-link support to GitHub throughout the scope.
5. **CLA-Group listing (narrowed 2026-08-08)** — search = CLA-group/project **names** from the existing project↔CLA-group mappings, plus an **exact repo-name/URL resolver**: the user types a repo name or pastes a GitHub URL, SS parses `owner/repo` and resolves it via the existing `RepositoryNameIndex` GSI (`GitHubGetRepositoryByName`) → `cla_group_id`. No fuzzy full-text search over repo names. Remaining question is only which existing listing endpoint to reuse for the name search.
…dback Address follow-up review comments from copilot-pull-request-reviewer: - plan.md risk 1: replace the "per-signature flag flip" parenthetical with the durable self-exclusion mechanism, consistent with FR-008; make it part of the ECLA endpoint deliverable. - plan.md risk 3 & spec.md account-auth sub-decision: retract the "first-timer enrichment is a call to existing v1 updateUser, no new backend work" claim. Verified against users/handlers.go:84-107 — the githubUsername branch returns 400 for an unseen username and, when found, Saves without verifying the linked identity belongs to the caller. A small backend-owned, ownership-checked identity-binding operation is required. Resolves 3 review threads. Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Review Feedback AddressedCommit: 8059c4e Changes Made
Threads Resolved3 of 3 unresolved threads addressed in this iteration. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:59
- The status/action mapping loses required coverage reasons. The current evaluator also returns
covered=falsefor a sanctioned or missing company and for a missing active CCLA (cla-backend-go/v2/my_clas/service.go:678-695), not only for an Approved List mismatch. Under FR-010 these rows become “Needs attention,” and FR-011 then shows a request-approval link that cannot fix them. Require a reason-aware API result and only show that link for approval-list failures.
- **FR-010**: Each row MUST show a status: **Valid** (approved and covered); **Needs attention** with an explanatory note (ECLA signed but no longer matching the company's Approved List criteria — approved but not covered); **Invalidated** (not approved). Source: extend `GET /v4/my-clas`, which already computes the coverage evaluation per ECLA row and currently collapses it into a single boolean (resolved — see open question 3).
- **FR-011**: "Needs attention" ECLA rows MUST link "Request approval →" into the Contributor Console's existing request-authorization flow (deep link; no new SS flow).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:93
- This reintroduces the flag-only invalidation semantics that FR-008 explicitly rejects as non-durable. An Approved List update can reapprove a signature whose only change is
signature_approved=false; the open-question contract must require the persistent self-exclusion described in FR-008.
2. **ECLA invalidation endpoint** — new `cla-backend-go` API (swagger-first) for an employee ending their own CCLA coverage. Semantics settled per FR-008/FR-008a and the invalidation preamble: per-signature flag flip, record kept for audit, no Approved List mutation, CLA managers + user notified, backend-side ownership check (don't replicate `invalidateICLA`'s missing check). Main remaining backend deliverable alongside open question 4.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:36
- Question 1 is not resolved: the spec still marks the identity-binding operation and its gateway authentication
[NEEDS CLARIFICATION]on spec.md:92, and risk 3 below says it is “not fully retired.” Calling it resolved can let the post-Phase-1 gate pass before that security contract exists.
**Post-Phase-1 re-check**: pending — re-run after `/speckit.clarify` confirms the remaining open items in [spec.md](spec.md) (questions 1 and 3 resolved; 5 narrowed; 2 and 4 are execution deliverables).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:69
- These backend changes are not conditional under the stated M2 scope. FR-010 requires the
GET /v4/my-clasextension, and the verified GitHub ICLA gap plus risk 2 require the no-PR request shape unless GitHub is explicitly removed from M2. Leaving both under “Conditional” can produce an implementation plan that cannot satisfy the acceptance scenarios.
Secondary repo: `linuxfoundation/easycla` `cla-backend-go` — new self-service ECLA-invalidation endpoint, swagger-first (`swagger/cla.v2.yaml` → `make swagger` → handler/service), no schema changes expected. Conditional (per clarify): status-evaluation extension to `GET /v4/my-clas`; no-PR ICLA request shape in `v2/sign` (+ matching `easycla-contributor-console` tweak).
Add FR-008b: both invalidation flows must record an invalidation timestamp and a reason/actor value on the signature record, alongside a contributor-framed signature.note. date_modified is not a usable invalidation date -- any write bumps it and InvalidateProjectRecord does not set it, so the invalidation date cannot be reconstructed from the record today. The reason/actor value should be the same field FR-008's durable self-exclusion relies on; the Approved-List re-validation marker and the audit signal are one concept, not two overlapping fields. Correct plan.md's "no schema changes expected" claim: the signatures table gains additive attributes (no migrations), read by v1/v2 paths, so consumers must tolerate empty values on pre-M2 records. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:94
- This summary restores the flag-only behavior that FR-008 explicitly rejects. A per-signature flag flip can be undone by Approved List re-validation, so the settled semantics must include the persistent self-exclusion honored by both re-validation and PR gating.
2. **ECLA invalidation endpoint** — new `cla-backend-go` API (swagger-first) for an employee ending their own CCLA coverage. Semantics settled per FR-008/FR-008a and the invalidation preamble: per-signature flag flip, record kept for audit, no Approved List mutation, CLA managers + user notified, backend-side ownership check (don't replicate `invalidateICLA`'s missing check). Main remaining backend deliverable alongside open question 4.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:32
- This gate's “one genuinely new backend piece” claim conflicts with risk 3, which requires a new backend-owned identity-binding operation in addition to ECLA invalidation. Include that operation in the simplicity assessment rather than passing the gate on an incomplete backend scope.
- **Simplicity**: no new services, storage, or state; extends M1's page and module; hand-off is the Console's existing deep link; invalidation reuses the existing impersonation middleware and revises (not replaces) the existing ICLA flow. The ECLA endpoint is the one genuinely new backend piece; the ICLA revision and no-PR request shape are bounded deltas to existing flows. PASS.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:36
- Question 1 is not resolved: the spec still marks the identity-binding operation's shape and gateway authentication as needing clarification, and risk 3 calls the account mechanics “not fully retired.” Leaving this re-check list as “1 and 3 resolved” can cause the remaining security contract to be skipped.
**Post-Phase-1 re-check**: pending — re-run after `/speckit.clarify` confirms the remaining open items in [spec.md](spec.md) (questions 1 and 3 resolved; 5 narrowed; 2 and 4 are execution deliverables).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:69
- These backend changes are no longer conditional under the current spec. FR-010 requires extending
GET /v4/my-clas, and risk 2 says the no-PR ICLA shape is required for the core GitHub sign-entry path. Mark both as required in the source structure so implementation planning cannot omit them during clarification.
Secondary repo: `linuxfoundation/easycla` `cla-backend-go` — new self-service ECLA-invalidation endpoint, swagger-first (`swagger/cla.v2.yaml` → `make swagger` → handler/service). Schema impact is limited to **additive** attributes on the signatures table per FR-008b (invalidation timestamp + reason/actor, the latter doubling as FR-008's durable self-exclusion marker) — no migrations, but the attributes are read by v1/v2 code paths, so consumers must tolerate empty values on pre-M2 records. Conditional (per clarify): status-evaluation extension to `GET /v4/my-clas`; no-PR ICLA request shape in `v2/sign` (+ matching `easycla-contributor-console` tweak).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:52
- The invalidation eligibility is internally inconsistent. The user story says every signed CLA row offers Invalidate, while this requirement limits ECLAs to the Valid state; a signed, approved ECLA in the newly defined Needs attention state is therefore unspecified. State explicitly whether Needs attention ECLAs can be self-invalidated so the UI and endpoint authorization use the same rule.
This issue also appears on line 94 of the same file.
- **FR-008**: Each valid ECLA row MUST offer Invalidate framed per the mockup — "This confirms you're no longer covered under {company}'s Corporate CLA (CCLA) for {project}, and marks your ECLA as invalid" (typed INVALIDATE confirmation). The company's Approved List is not mutated; the company's CLA managers are notified so they can update it. **Durability caveat (verified):** a plain per-signature flag flip is not durable while the user stays on the Approved List — when `auto_create_ecla` is set, a later Approved List update re-processes every still-listed employee and calls `ValidateProjectRecord` to re-approve unapproved employee signatures (`signatures/service.go:895-900`), and `ProcessEmployeeSignature` re-authorizes any existing employee signature whenever `UserIsApproved` succeeds (`:1562-1588`). The new endpoint MUST therefore apply a **persistent self-exclusion** the Approved-List re-validation and PR gating both honor (or otherwise change eligibility), not merely flip `signature_approved`. *([NEEDS CLARIFICATION]: no self-service ECLA-invalidation endpoint exists — new `cla-backend-go` work; the durable self-exclusion mechanism is part of that work; see Open questions.)*
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:24
- The scope count omits mandatory backend work already identified by this plan: risk 3 requires a backend-owned identity-binding operation, FR-007 requires the ICLA invalidation revision, and risk 2 makes the no-PR ICLA delta mandatory for GitHub. Counting only one upstream endpoint understates the cross-repository implementation scope and the three-week schedule risk.
This issue also appears in the following locations of the same file:
- line 32
- line 36
- line 69
**Scale/Scope**: all LFX contributors; extends 1 existing Me-lens page, ~3–4 server routes (CLA-Group search, `userID` resolution/hand-off, ICLA invalidate, ECLA invalidate), 1 new upstream endpoint (ECLA invalidation) + possibly a status/listing extension per clarify outcomes.
| - **Per-platform matrix**: *Gerrit* — no account step ever: Gerrit authenticates via the same LF SSO as Self Serve, so the LF identity from `user-from-token` already is the signing identity. *GitHub* — one-time Auth0 account link via M1's existing flow; resolved by GitHub ID. *GitLab* — **conditional** (decided 2026-08-07): the sign entry requires SS's GitLab account linking, which SS may ship before M2 completes. If it lands in time, GitLab enables in M2 by config (same pattern as GitHub; EasyCLA user records already carry `user_gitlab_id`/`user_gitlab_username`); if not, GitLab CLA groups (~2 projects) stay excluded from the "Sign CLA" entry and enable as a follow-up. Either way the account step MUST be platform-parametrized — GitLab is a config flip, never a redesign. M2 does not block on it. | ||
| - **Multiple linked GitHub accounts**: EasyCLA user records carry a *single* GitHub identity, so two linked GH accounts typically mean two EasyCLA records — an ICLA signed on one does **not** cover commits from the other by GitHub-ID match (only the fragile email fallback). When >1 GitHub account is linked, the sign flow MUST ask "which account will you contribute with?" and resolve the EasyCLA record by that GitHub ID; no silent auto-pick. Single-account users never see the picker. | ||
| - **No live platform session required at sign time**: a GitHub login is needed exactly once, to complete the Auth0 account link. The hand-off and signing work off the server-resolved `userID` — the Console decision screen is deep-linkable and the DocuSign ceremony needs nothing from GitHub. This is a constraint on open question 4: the no-PR ICLA request shape must accept the routed user without demanding a fresh GitHub OAuth, or it reintroduces the session dependency the proactive path removes. | ||
| - **Record binding + first-timer enrichment (resolved 2026-08-08)**: resolve the EasyCLA record by the (chosen) linked GitHub ID first; if none exists, fall back to `GET /v4/user-from-token` (lookup-or-create) and **enrich** the record with the linked GitHub ID/username so the PR check matches by GitHub ID rather than the fragile email fallback. The existing v1 `updateUser` API is **not** a usable enrichment primitive (verified): its `githubUsername` branch returns 400 for a previously-unseen username and, when a username exists, updates that record without verifying the linked identity belongs to the authenticated caller (`users/handlers.go:84-107`). Enrichment therefore requires a **small backend-owned, ownership-checked identity-binding operation** — not new business logic, but not zero backend work either. *([NEEDS CLARIFICATION]: shape of that binding operation and its gateway auth.)* |
…tHub ID PR #5144 review (copilot): the FR-003 fallback chain was internally inconsistent. It asserts one-GitHub-identity-per-EasyCLA-record, then prescribed enriching whatever record user-from-token returns. createUserFromRequest (cmd/server.go:1004-1042) resolves by LF username then LF email and returns any match -- including a record already bound to a different GitHub identity. In the documented multi-account case (A and B linked, record bound to A, user selects B), enrichment would overwrite A's binding and silently break GitHub-ID matching for A's existing commits. Constrain enrichment to unbound fallback records; require a separate record bound to the selected GitHub ID otherwise. This widens the binding operation's scope to include create-record-bound-to-GitHub-ID, since user-from-token's CreateUser path fires only when no record matches LF username/email and so cannot produce the second record. Also note refreshStoredUserIdentity touches only user_name/lf_email/ date_modified, so this is an M2-introduced risk rather than a pre-existing overwrite bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:32
- This PASS contradicts the plan's own required work: FR-008b adds persistent signature attributes, and line 80 requires a new backend-owned identity-binding operation. Reframe the gate as “no SS-owned state/new service” and assess the added EasyCLA state and backend operations rather than asserting there is no new state and only one new backend piece.
- **Simplicity**: no new services, storage, or state; extends M1's page and module; hand-off is the Console's existing deep link; invalidation reuses the existing impersonation middleware and revises (not replaces) the existing ICLA flow. The ECLA endpoint is the one genuinely new backend piece; the ICLA revision and no-PR request shape are bounded deltas to existing flows. PASS.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:36
- Question 1 is not resolved: spec line 98 still marks the identity-binding operation's create/enrich shape and gateway authentication as needing clarification. Calling it resolved can cause
/speckit.clarifyto skip a required security-sensitive contract.
**Post-Phase-1 re-check**: pending — re-run after `/speckit.clarify` confirms the remaining open items in [spec.md](spec.md) (questions 1 and 3 resolved; 5 narrowed; 2 and 4 are execution deliverables).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:98
- The PR description still says fallback enrichment uses the existing v1
updateUserAPI and lists only questions 2 and 4 as remaining backend deliverables, while this requirement correctly establishes thatupdateUseris unusable and a new ownership-checked binding operation remains undefined. Update the PR description so reviewers and Spec Kit consumers do not miss this additional backend/security deliverable.
The existing v1 `updateUser` API is **not** a usable enrichment primitive (verified): its `githubUsername` branch returns 400 for a previously-unseen username and, when a username exists, updates that record without verifying the linked identity belongs to the authenticated caller (`users/handlers.go:84-107`). Enrichment therefore requires a **backend-owned, ownership-checked identity-binding operation** covering both bind-to-unbound-record and create-record-bound-to-GitHub-ID. *([NEEDS CLARIFICATION]: shape of that binding operation, its create-vs-enrich branches, and its gateway auth.)*
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:60
- The three-state mapping cannot infer the stated reason from the existing boolean.
eclaCoveredByCurrentApprovalListalso returnsfalsefor a sanctioned/unknown company, a missing CCLA, and approval-evaluation errors (v2/my_clas/service.go:678-705); those rows would be mislabeled as an Approved List mismatch and incorrectly offered “Request approval.” Extend the API with a coverage reason (or an unavailable/error outcome) and expose the approval link only for an actual approval-list mismatch.
- **FR-010**: Each row MUST show a status: **Valid** (approved and covered); **Needs attention** with an explanatory note (ECLA signed but no longer matching the company's Approved List criteria — approved but not covered); **Invalidated** (not approved). Source: extend `GET /v4/my-clas`, which already computes the coverage evaluation per ECLA row and currently collapses it into a single boolean (resolved — see open question 3).
- **FR-011**: "Needs attention" ECLA rows MUST link "Request approval →" into the Contributor Console's existing request-authorization flow (deep link; no new SS flow).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:81
- Status evaluation is not retired: the current endpoint exposes only
approvedand aggregatevalid, whilecovered=falseconflates approval-list mismatch with sanctions, missing CCLA/company data, and evaluation failure. Keep this as backend work to return a reason/status that can drive the explanatory note and conditionally show “Request approval.”
4. **Status evaluation** (open question 3) — retired: `GET /v4/my-clas` already computes the coverage evaluation per ECLA row; M2 exposes it as a status field instead of collapsing it into `Valid`.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:103
- This says the only unresolved item is the listing endpoint, but the resolver is defined only for GitHub URLs even though FR-001 accepts an unqualified pasted repo link and includes Gerrit-backed groups. Either restrict pasted links explicitly to GitHub or specify Gerrit URL resolution before marking the resolver decision narrowed.
5. **CLA-Group listing (narrowed 2026-08-08)** — search = CLA-group/project **names** from the existing project↔CLA-group mappings, plus an **exact repo-name/URL resolver**: the user types a repo name or pastes a GitHub URL, SS parses `owner/repo` and resolves it via the existing `RepositoryNameIndex` GSI (`GitHubGetRepositoryByName`) → `cla_group_id`. No fuzzy full-text search over repo names. Remaining question is only which existing listing endpoint to reuse for the name search.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:16
- The Go work is not merely “possibly” needed for the no-PR ICLA shape: risk 2 states it is required for the core GitHub sign-entry path. The technical context also omits the mandatory ICLA invalidation revision and identity-binding operation, materially understating the implementation scope.
This issue also appears in the following locations of the same file:
- line 32
- line 36
**Language/Version**: TypeScript (Angular 20.3 frontend + Node 22 / Express 4 SSR server) in `lfx-self-serve`; Go 1.25 in `cla-backend-go` for the ECLA-invalidation endpoint (and possibly status evaluation / no-PR ICLA shape, per clarify outcomes).
|
|
||
| The existing v1 `updateUser` API is **not** a usable enrichment primitive (verified): its `githubUsername` branch returns 400 for a previously-unseen username and, when a username exists, updates that record without verifying the linked identity belongs to the authenticated caller (`users/handlers.go:84-107`). Enrichment therefore requires a **backend-owned, ownership-checked identity-binding operation** covering both bind-to-unbound-record and create-record-bound-to-GitHub-ID. *([NEEDS CLARIFICATION]: shape of that binding operation, its create-vs-enrich branches, and its gateway auth.)* | ||
| > Note: `refreshStoredUserIdentity` (called on both fallback hit paths) touches only `user_name`/`lf_email`/`date_modified` — it does not mutate GitHub fields, so this is an M2-introduced risk, not a pre-existing overwrite bug. | ||
| 2. **ECLA invalidation endpoint** — new `cla-backend-go` API (swagger-first) for an employee ending their own CCLA coverage. Semantics settled per FR-008/FR-008a and the invalidation preamble: per-signature flag flip, record kept for audit, no Approved List mutation, CLA managers + user notified, backend-side ownership check (don't replicate `invalidateICLA`'s missing check). Main remaining backend deliverable alongside open question 4. |
What
Extracts Milestone 2 into its own self-contained Spec Kit feature folder —
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/— based ondev, mirroring the existingm1-my-cla/layout. This is the artifact Spec Kit (/speckit.plan→/speckit.tasks→/speckit.implement) will consume to plan and implement M2.Intentionally separate from the program-level design PR (#5132), which keeps all six milestone briefs + overview + top-level
spec.md. Splitting by artifact type (program design vs. implementable feature slice) keeps this PR small and free of cross-doc conflicts.Scope reflected (revised through 2026-08-08, per Heather/PM; UI per mockup Final/v16)
M2 extends M1's My CLAs page (Me lens) with three additive capabilities, per the M2 mockup Final/v16 (source of truth):
GET /v4/my-clas(M1) — M2 exposes it instead of collapsing it into a single boolean.Guardrails: Self Serve runs no signing ceremony, makes no signing-initiation calls, and never talks to DocuSign; the Console is not cut over or retired; the PR-check remediation link is unchanged (no SSM flip).
Contents
spec.md— M2 feature spec: user story (P2), acceptance scenarios, FR-001…011 (incl. FR-008a notifications), success criteria, scope boundaries, a "Verified Console/backend facts" section, and the open-question ledger (1 and 3 resolved, 5 narrowed; 2 and 4 are the two remaining backend deliverables).plan.md— implementation plan: summary, technical context, constitution check, project structure (extends M1'smy-clasmodule +claserver seam), complexity tracking against the ~3-week budget.Key verified findings baked into the spec
invalidateICLAendpoint cannot be reused unchanged for self-service: no ownership check,sigs[0]on multiple matches (must be signature-ID-targeted), manager-framed email and project-deletion event data (must become actor-/reason-aware).GET /v4/user-from-tokenresolves by LF username → email only; userID resolution must be platform-aware (GitHub ID first, fallback + enrichment via the existing v1updateUserAPI).Deliberately not included
research.md/data-model.md/contracts/are not hand-written — the Spec Kit planning flow generates them. The remaining genuinely open items (ECLA-invalidation endpoint execution, the no-PR ICLA request shape, the listing-endpoint choice) are marked for/speckit.clarify.Note on links
The upward links (
../spec.md,../02-milestone-sign-icla-fable.md) point at program-level docs that live in #5132 (targetingdev) and resolve once that PR merges. Them1-my-cla/sibling folder is already ondev.🤖 Generated with Claude Code