docs(spec): M2 Spec Kit feature slice (proactive picker → Console hand-off) - #5144
Open
mlehotskylf wants to merge 29 commits into
Open
docs(spec): M2 Spec Kit feature slice (proactive picker → Console hand-off)#5144mlehotskylf wants to merge 29 commits into
mlehotskylf wants to merge 29 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 |
mlehotskylf
force-pushed
the
docs/easycla-ss-m2-speckit
branch
from
August 4, 2026 13:35
4990e8e to
3f3efd6
Compare
Contributor
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. |
Contributor
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>
mlehotskylf
force-pushed
the
docs/easycla-ss-m2-speckit
branch
from
August 4, 2026 13:41
3f3efd6 to
b88e3ac
Compare
Contributor
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
Contributor
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>
Contributor
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`.
Contributor
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.
…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>
Contributor
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).
PR #5144 review (copilot): open question 2 described the ECLA invalidation contract as a "per-signature flag flip", the exact semantic FR-008 rejects. A reader working from the ledger alone would build a bare signature_approved flip, which auto_create_ecla re-processing silently reactivates -- passing tests, then breaking on the next Approved List edit. Restate it as the persistent self-exclusion honored by Approved-List re-validation and PR gating, and point at FR-008b's reason/actor field as that marker so the two are not built as separate concerns. Open question 4 had the same drift in milder form: "Recommended ... small delta" understated a deliverable that plan.md treats as required and non-cuttable for the GitHub sign-entry path. Marked required, with the Gerrit-only fallback named. Root cause is structural: the ledger restates settled semantics in its own words, so it drifts when FRs are hardened. Audited items 1, 3 and 5 -- those remain consistent with FR-003, FR-010 and FR-001. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Contributor
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 (7)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:98
- The PR description says fallback enrichment will use the existing v1
updateUserAPI, but this verified finding says that API is unusable and a new ownership-checked binding operation is required. Update the PR description so reviewers and downstream planning see the actual additional backend scope.
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/plan.md:16
- This still labels the status and no-PR ICLA backend changes as optional, although FR-010 requires the status extension and open question 4 says the no-PR shape is mandatory for GitHub sign entry. It also omits the now-required ICLA invalidation revision and identity-binding operation, materially understating the implementation stack.
**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
- The scale estimate repeats the stale optional/backend scope: identity binding is another new upstream operation, while the ICLA revision, status extension, and no-PR ICLA shape are all mandatory elsewhere in this spec. Listing is the only extension here still awaiting endpoint selection.
**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/spec.md:60
- This status truth table cannot satisfy acceptance scenario 6. Approved List edits normally set
signature_approved=falsesynchronously (docs/MY_CLAS_API.md:280-285), so a user removed from the list would be labeled Invalidated rather than Needs attention. Conversely,covered=falsealso includes a sanctioned company or missing CCLA, where “Request approval” cannot resolve the problem. Require reasoned coverage/invalidation states and expose the link only for the applicable reason instead of deriving both from these two booleans.
- **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:51
GET /v4/my-clasdeliberately retains signed ICLAs aftersignature_approvedbecomes false (docs/MY_CLAS_API.md:291-296), so “Each signed ICLA row” also exposes Invalidate for already-invalidated records. A repeated call could overwrite the new timestamp/reason and emit duplicate email/event side effects. Limit the action to non-invalidated rows and define the endpoint as idempotent or as rejecting repeated invalidation.
- **FR-007**: Each signed ICLA row MUST offer Invalidate with a confirmation modal (copy per mockup; irreversible; requires typing INVALIDATE to confirm). The invalidated record is kept for audit — past contributions remain attributed to it. The existing `PUT /v4/cla-group/{claGroupID}/user/{userID}/icla` cannot be reused **unchanged** (all verified): (a) it performs **no ownership check** — the revised EasyCLA endpoint itself MUST verify the authenticated identity owns the target signature (same requirement as the new ECLA endpoint), since the v4 API is independently reachable and bypassing the SS route would otherwise allow invalidating another contributor's ICLA; the SS server MUST also enforce self-only invalidation as defense in depth (same enforcement-point pattern as M1); (b) it resolves by `(claGroupID, userID)` and silently invalidates `sigs[0]` when several signatures match — invalidation MUST target the specific `signatureID` shown in the row (M1 lists distinct signatures); (c) its side effects are admin-framed — the email template says a Project Manager invalidated the ICLA and the logged event data (`SignatureProjectInvalidatedEventData`) records a project-deletion reason. The backend flow MUST become actor- and reason-aware (self-service email per FR-008a; event data reflecting a contributor self-invalidation).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:42
- The requirement promises pasted repository links while GitHub and Gerrit are in scope, but open question 5 defines URL parsing and lookup only for GitHub. Implementers cannot tell whether pasted Gerrit URLs must work. Explicitly restrict pasted links to GitHub in this requirement and the user story, or define the Gerrit URL resolver.
- **FR-001**: My CLAs MUST offer a "Sign CLA" action opening a modal with one search box matching project names, CLA group names, and linked repo/org sources (GitHub, Gerrit) as search metadata, and accepting a pasted repo link (resolved to its CLA Group — see open question 5). GitLab-backed CLA groups are excluded until SS ships GitLab account linking (see Scope boundaries / open question 1). No org/repo selection step — the signing unit is the CLA Group. *([NEEDS CLARIFICATION]: listing endpoint.)*
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:36
- Question 1 is not resolved: its own section still leaves the identity-binding operation’s shape, create-vs-enrich branches, and gateway authentication for clarification, and question 5 still needs a listing endpoint. Calling it resolved can cause the clarify phase to skip required design 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).
Verified the M2 spec/plan claims against lfx-self-serve@main and cla-backend-go. The backend claims all hold, but three assumptions were stale or wrong and would have propagated into every downstream Spec Kit artifact: - Source Code paths: M1 shipped as a Profile tab (app/modules/profile/clas/, route /profile/clas, myClasEnabledGuard), not the app/modules/my-clas/ module both M1's and M2's plans predicted. Corrected every path and added the shared/ surfaces. - Ownership enforcement: framed as new backend work, but M1 already built the boundary — authorizeIdentity (v2/my_clas/service.go:383) validates identity keys against the caller's own records and their platform identities. FR-007(a)/FR-008 now require reusing it rather than adding a parallel check. - Constitution Check: cited "the unratified .specify/memory/ constitution.md template", but no .specify/ directory exists in either repo. Noted that scaffolding is a separate, non-blocking change. Also recorded three findings the slice had missed: - GET /v4/my-clas/identities already ships (getMyIdentities) and returns the authorized identity set — the multi-account GitHub picker needs no new read endpoint. - FR-010 requires relaxing M1's valid-only row filter and the boolean-derived status mapping in cla.service.ts, not just adding a column; the BFF currently discards the rows the column must show. - The auto_create_ecla re-approval fires precisely on !SignatureApproved || !SignatureSigned (signatures/service.go:898), confirming FR-008's durability caveat is the live code path. Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Traces both critical-path spikes to their exact code locations and
records the outcomes in a new Phase 0 research.md, per M1's folder
shape. Reviewed cla-backend-go and easycla-contributor-console.
Spike 1 — no-PR GitHub ICLA (open question 4, risk 2): RESOLVED.
Two independent dependencies, one per repo, not one:
- Console findActiveSignature() hard-stops with "restart from your
pull request" (individual-dashboard.component.ts:49-66) and also
sources return_url from the active signature.
- Backend errors on missing pull_request_id/repository_id
(v2/sign/service.go:2890,2903), called unconditionally at :1414.
Gerrit avoids both rather than satisfying them, so the precedent
bounds the delta but proves less than assumed: acl =
github:{user.GithubID} (:1433) must still be set, making the
GitHub-ID binding load-bearing rather than a matching optimization.
Fix is a widened branch in each repo plus an explicit proactive
signal — no DocuSign/schema/webhook change. Gerrit-only fallback
now saves little; recommend keeping GitHub in M2.
Spike 2 — ECLA durable self-exclusion (open question 2, risk 1):
mechanism RESOLVED. UserIsApproved takes only (user, cclaSignature)
(signatures/service.go:1607) and cannot see the employee signature,
so a marker on that record cannot be honored inside it. The marker
must instead be checked at three call sites: the auto_create_ecla
re-validation guard (:898), ProcessEmployeeSignature's approval
branch (:1578), and eclaCoveredByCurrentApprovalList
(v2/my_clas/service.go:214). Threading the signature into
UserIsApproved was considered and rejected. InvalidateProjectRecord
writes only signature_approved + note (repository.go:2089-2130) and
is already signature-ID-keyed, confirming FR-008b's rationale and
making its new attributes additive to the same atomic write.
Also corrects one inaccurate verified fact and records a new gap:
ClaDashboardComponent does not fetch the project, it reads it from
browser storage after removing the key (cla-dashboard.component.ts
:57,63), so a cold deep link renders both ICLA and ECLA buttons
disabled. Tracked as plan.md risk 5; affects FR-002 for both
agreement types, including the ECLA path previously believed to
work proactively as-is. Flagged as needing a dev-Console smoke test
— the one spike conclusion not verified against a running system.
Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
The previous commit recorded a supposed FR-002 gap: that ClaDashboardComponent reads its project from browser storage rather than fetching it, leaving both ICLA and ECLA buttons disabled on a cold deep link. That was wrong. Retracted. The project fetch lives in the embedded child component, not the container: cla-dashboard.component.html:3 wires <app-project-title> with a successEmitter into setProject(), and ProjectTitleComponent.getProject() fetches by ID and emits the result (project-title.component.ts:46-58). The container's storage read is a fallback the child overwrites. The decision screen is deep-linkable today, as the spec originally stated; no Console-side fetch is needed and plan.md's risk 5 is removed. The error came from reading the component's .ts without its template. Recorded that as a method note in research.md — component-level claims about this Console need the .html read alongside the .ts. Two real dependencies surfaced by the same trace are kept: - ProjectTitleComponent.getProject() sources projectId from storage, not the route; the container writes it from the route param in ngOnInit before the child's ngAfterViewInit reads it, so ordering holds, but the hand-off requires writable browser storage. This is a pre-existing Console property (it already warns about private windows), not something M2 introduces. - getUser() fails with "There is an invalid user ID in the URL" when USER_ID is absent (project-title.component.ts:84-101), so a placeholder or unresolved ID breaks the hand-off. This reinforces FR-003's server-side userID resolution. Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (10)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/research.md:100
- The proposed three guards miss another reactivation path:
CreateProjectCompanyEmployeeSignaturedirectly callsValidateProjectRecordfor any existing unapproved ECLA (signatures/repository.go:2510-2512). If the preceding lookup misses because of eventual consistency or a create race, this path can still re-enable a self-excluded signature. Guard this path too, or makeValidateProjectRecorditself refuse to clear a self-exclusion marker; otherwise FR-008 is not durable.
**Option A — exclusion checked at the call sites (recommended).** Keep `UserIsApproved` as the pure Approved-List predicate it is, and have both consumers consult the employee signature's self-exclusion marker *before* treating approval as coverage:
- `signatures/service.go:898` — add the marker to the re-validation guard so a self-excluded record is skipped rather than re-approved.
- `ProcessEmployeeSignature` `:1578` — require `!selfExcluded` alongside `userApproved` before `hasSigned = true`.
- `v2/my_clas/service.go:214` — `eclaCoveredByCurrentApprovalList` already sits at the third consumer; the marker feeds FR-010's status directly.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/research.md:53
- The explicit proactive signal has no defined way to reach this component. FR-002's hand-off URL contains only
claGroupIDanduserID, but the Console must choose the proactive branch before it creates the signing request; without a query/state signal it still executesfindActiveSignature()and dead-ends. Define how the hand-off conveys proactive mode and how the Console propagates it into the backend request.
**Introduce an explicit no-PR (proactive) ICLA mode rather than special-casing on absent metadata.** Concretely:
1. **Backend** (`v2/sign/service.go`): make the github branch tolerate missing PR metadata *by intent, not by accident* — when the request carries no PR context, skip `getIndividualSignatureCallbackURL` and fall back to the caller-supplied `return_url`, mirroring the Gerrit path's shape. Prefer an explicit signal in the request (e.g. a proactive flag or a distinct `return_url_type`) over inferring it from an empty metadata map, so a genuine PR-flow bug can't silently degrade into a callback-less signature.
2. **Keep `acl = github:{user.GithubID}`.** This is orthogonal to PR context and is the identity binding the PR check later matches on — it must not be dropped in the proactive path. This is why FR-003/FR-004's GitHub-ID binding is load-bearing: `user.GithubID` must already be on the record before signing, or the proactive signature is bound to an empty ACL.
3. **Console** (`individual-dashboard.component.ts`): generalize the `hasGerrit` branch into a "no active signature required" condition covering the proactive entry, and source `return_url` from the stored redirect in that mode. The existing structure already supports this — it's a widening of an existing branch, not a new code path.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:60
approved && !coveredis broader than an Approved List mismatch.eclaCoveredByCurrentApprovalListalso returns false for a sanctioned/unknown company, a missing current CCLA, and evaluation errors (v2/my_clas/service.go:678-705), so this mapping labels all of those rows “Needs attention”; FR-011 then gives each a Request approval link that cannot resolve the underlying problem. Expose a reason/status distinction, or restrict that link and copy to the actual Approved List-mismatch case.
- **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). SS-side, this also requires relaxing M1's valid-only row filter and the boolean-derived status mapping in `server/services/cla.service.ts` (see Verified facts, 2026-08-11) — the column cannot show Invalidated rows that the BFF currently discards.
- **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:91
- This endpoint is not a usable multi-account picker model. It returns a flat, sorted set of unrelated
github-id:*andgithub-username:*strings, so IDs cannot be paired with display names; moreover platform-linked accounts contribute only usernames, while GitHub IDs are emitted only from existing EasyCLA records (v2/my_clas/service.go:326-348,575-588). A newly linked account—the case requiring record creation—therefore has no selectable ID for FR-003. Use M1's structured Auth0 identities or extend the endpoint to return structured account pairs.
- **The multi-account GitHub picker needs no new read endpoint.** `GET /v4/my-clas/identities` (`getMyIdentities`, `v2/my_clas/service.go:301`; handler `v2/my_clas/handlers.go:97`; swagger `cla.v2.yaml:2837`) already returns the deduplicated identity set the My CLAs API authorizes — the picker's candidate list (open question 1).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:81
- This still marks the status extension and no-PR request shape as conditional, although FR-010 requires the former and risk 2 says the latter is required for the in-scope GitHub flow. Leaving them “per clarify” allows implementation to omit core acceptance paths.
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/plan.md:36
- The question-state summary is internally inconsistent: question 1 still has a
[NEEDS CLARIFICATION]binding-operation shape, while questions 2–4 are explicitly resolved and now execution deliverables. Spec Kit could otherwise skip the unresolved identity contract and re-clarify settled 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/research.md:4
- The PR description explicitly lists
research.mdunder “Deliberately not included” and says the planning flow will generate it, but this change adds a hand-authored Phase 0 research document. Update the PR scope/description or remove this file so reviewers and Spec Kit have one source of truth about the artifact set.
# Phase 0 Research: M2 — backend spikes
**Input**: [spec.md](spec.md) open questions 2 and 4 | [plan.md](plan.md) Complexity Tracking risks 1 and 2
**Date**: 2026-08-11 | **Status**: both spikes resolved; recommendations below feed `/speckit.clarify` and the contracts
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:16
- The no-PR ICLA shape and status extension are no longer optional clarify outcomes: FR-010 mandates the status field, and the resolved proactive-signing spike makes the no-PR shape required for GitHub. Keeping “possibly” here contradicts the plan's settled scope.
**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/research.md:6
- Questions 2 and 4 are now marked resolved, so they are no longer the two
[NEEDS CLARIFICATION]items. The actual remaining clarification markers are the identity-binding operation (question 1) and listing endpoint (question 5); this introduction sends/speckit.clarifyto the wrong work.
These are the two `[NEEDS CLARIFICATION]` items that block M2's sign-entry and ECLA-invalidation paths. Neither was answerable from the requirements text — both needed the code traced. Every claim below cites the file and line it was read from (`cla-backend-go` and `easycla-contributor-console` at `dev`/`main`, 2026-08-11).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:48
research.mdis present and marked complete, so describing it as a future/speckit.planoutput is stale and conflicts with this directory's actual contents.
├── research.md # Phase 0 output — TBD by /speckit.plan (confirms remaining open items)
| New swagger-first endpoint in `cla-backend-go` for self-service ECLA invalidation, per FR-008/008a/008b: | ||
|
|
||
| - **Targeting**: by `signatureID` (the row's own ID, per FR-007's precedent). `InvalidateProjectRecord` is already signature-ID-keyed (`:2112-2116`), so no repository change is needed for targeting — only for the additional attributes. | ||
| - **Ownership**: resolve through M1's existing `authorizeIdentity` (`v2/my_clas/service.go:383`) rather than a new check — see plan.md's Security gate. |
…atus Closes the last two M2 open questions and folds in a full read of the Final/v16 mockup. All five open questions are now resolved; the only outstanding item is a measurement, not a clarification. Open question 1 (identity binding) — resolved by building no binding operation. SS runs a read-only pre-flight against the already-shipped GET /v4/my-clas/identities, gates per repo-source type, and delegates binding to the Console's existing GitHub OAuth. Rationale: user.GithubID becomes the merge-gating signature ACL (v2/sign/service.go:1433), and a second writer risks two systems disagreeing about whose GitHub ID it is — the hazard commit d0a4f81 already had to guard. Verified the two sides compare as numeric GitHub IDs (users/repository.go:719-728; cla.service.ts:193-196), which also surfaced that SS carries githubIds as an array while an ACL binds one, so a picker is required. Open question 5 (search) — resolved: a new endpoint is required. Both existing CLA-group listings are foundation/project-SFID-scoped and GetCLAGroupByName is exact-match-only. Records a latent bug: GetCLAGroups logs SearchField/SearchTerm/FullMatch then builds a projection with no filter (project/repository/repository.go:529-538), so it is an unfiltered full-table Scan whose search parameters do nothing — meaning search on this table has never been measured. Scope is also four sources per the mockup, not one. Specifies measure-first with no bespoke cache or new datastore in M2, escalating to OpenSearch rather than a cache if needed; the cardinality assumption behind that is recorded as explicitly unverified (no lfproduct-* AWS profile available locally). Mockup read in full — three items the artifacts were missing: - The ECLA "Download PDF" discrepancy does not exist; the mockup already renders it disabled with a "Covered by CCLA" tooltip. Removed from the blocker list (no PM input needed) and specified as FR-011a. - "Request approval" is a real row action no FR covered, conditional on the Needs-attention state. Added to FR-011. - Exact status vocabulary forces FR-010a: the three statuses need three independent response fields, since row.Valid collapses approved-ness and coverage (v2/my_clas/service.go:218) and telling self- from manager-invalidation needs FR-008b's provenance. This makes the status work joint-largest with the ECLA endpoint and couples the two. Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/research.md:61
- This retained constraint contradicts the adopted design later in this file and FR-003/FR-004: it says SS binds the identity before hand-off, while the final decision explicitly forbids SS identity writes and delegates binding to the Console. Since this research feeds the generated contracts, remove the superseded statement or rewrite it to match the final identity flow.
The no-PR shape must not require a live GitHub OAuth session — identity is bound server-side by SS before hand-off. Nothing in the traced code conflicts with this: neither `postIndivdualRequestSignature` nor the backend's proactive path needs a GitHub token, only `user.GithubID` already stored on the record.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:33
- This security gate still cannot pass. The Console copies
:userIdfrom the editable URL into the signing request, while/v4/request-individual-signaturehassecurity: []and accepts the body’s requireduser_id; server-side resolution before redirect does not bind the later request to the authenticated LF principal. Require the proactive Console/backend path to authenticate the signing request and verify that the routed user belongs to that principal before issuing DocuSign.
- **Security**: `userID` derived server-side; the v4 API is independently reachable by any authenticated caller, so backend signature-ownership enforcement is **mandatory** for both the revised ICLA endpoint and the new ECLA endpoint (the revised call still takes a target signature/user identifier — bypassing the SS route must not permit invalidating another contributor's agreement). **This is a reuse, not a new mechanism** (verified): M1 already built the ownership boundary in `v2/my_clas/service.go` — `authorizeIdentity` (`:383`) verifies every requested identity key against the caller's own EasyCLA records *and* their platform user-service identities, dropping unverifiable keys into `skippedIdentities`. M1's SS controller records the resulting contract: "EasyCLA re-verifies each key belongs to the caller and owns the signature, so the upstream endpoint — not this controller — is the ownership authorization boundary." Both M2 invalidation endpoints MUST resolve ownership through that same helper rather than introducing a parallel check. The SS server retains self-only enforcement and impersonation blocking (existing middleware) as defense in depth; no arbitrary-user lookup exposed. PASS with these requirements carried into contracts.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:52
- The specified picker has no way to route the selected account.
GET /v4/my-clas/identitiesreturns only a flat list of strings, not a GitHub-ID-to-EasyCLA-userID mapping. When linked accounts correspond to different EasyCLA records, selecting one does not determine the{userID}required by FR-002. Define a server-side lookup from the validated selected GitHub ID to exactly one EasyCLA user record, or extend the response contract with that mapping.
- **Multi-identity rule**: SS carries `githubIds` as an **array** (multiple linked GitHub identities are possible) while a signature ACL binds exactly **one**. When more than one is linked, the flow MUST ask which account the contributor will contribute with — no silent auto-pick — matching the mockup's authorize-the-account note. Single-identity users never see the picker.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:33
authorizeIdentitydoes not verify signature ownership: it only filters identity keys and has no signature-ID argument. The existing PDF path enforces ownership only after that helper by resolving the authorized user records and searching their signatures for the target ID (v2/my_clas/service.go:246-265). Require the invalidation endpoints to reuse that complete lookup-and-match pattern; callingauthorizeIdentityalone would still leave the target signature unchecked.
- **Security**: `userID` derived server-side; the v4 API is independently reachable by any authenticated caller, so backend signature-ownership enforcement is **mandatory** for both the revised ICLA endpoint and the new ECLA endpoint (the revised call still takes a target signature/user identifier — bypassing the SS route must not permit invalidating another contributor's agreement). **This is a reuse, not a new mechanism** (verified): M1 already built the ownership boundary in `v2/my_clas/service.go` — `authorizeIdentity` (`:383`) verifies every requested identity key against the caller's own EasyCLA records *and* their platform user-service identities, dropping unverifiable keys into `skippedIdentities`. M1's SS controller records the resulting contract: "EasyCLA re-verifies each key belongs to the caller and owns the signature, so the upstream endpoint — not this controller — is the ownership authorization boundary." Both M2 invalidation endpoints MUST resolve ownership through that same helper rather than introducing a parallel check. The SS server retains self-only enforcement and impersonation blocking (existing middleware) as defense in depth; no arbitrary-user lookup exposed. PASS with these requirements carried into contracts.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/research.md:4
- The PR description explicitly says
research.mdis deliberately not included and that the remaining items are still for/speckit.clarify, but this PR adds a hand-written research artifact that declares all five questions resolved. Update the description so reviewers and Spec Kit consumers receive the actual artifact scope and status.
This issue also appears on line 61 of the same file.
# Phase 0 Research: M2 — backend spikes
**Input**: [spec.md](spec.md) open questions 2 and 4 (Spikes 1–2) and 1 and 5 (Addendum) | [plan.md](plan.md) Complexity Tracking risks 1–5
**Date**: 2026-08-11, addendum 2026-08-12 | **Status**: **all five spec open questions resolved**; one measurement outstanding (CLA-group cardinality, FR-001a). Recommendations below feed the contracts.
| - **FR-004**: Before a GitHub hand-off, Self Serve MUST run a **read-only identity pre-flight** and MUST NOT perform any identity write: | ||
| - **Source**: the already-shipped `GET /v4/my-clas/identities` (`getMyIdentities`, `v2/my_clas/service.go:301`) — no new endpoint. | ||
| - **Gate per repo-source type, not globally**: a Gerrit-backed CLA Group requires no GitHub identity and MUST NOT be gated; a GitHub-backed one requires a bound GitHub ID. GitLab follows the same shape by config when SS ships GitLab linking. | ||
| - **On a missing identity, do not block**: hand off to the Console's existing GitHub-OAuth entry, which binds the ID exactly as it does today. The FR-003 constraint holds — SS performs no write. |
… history The three M2 artifacts had accumulated their own changelog: ~25 "RESOLVED" markers, 3 retractions, 3 supersessions, and phrases like "an earlier reading" / "previously mis-scoped". Requirements were buried under the narrative of how they came to be, and the same findings (the dead search filter, the cache rationale, the Download PDF non-discrepancy) were re-argued in three or four places. Restructured to state what is true now: - spec.md: "Open questions" (where all five were closed) becomes "Decisions", one paragraph each. Verified facts grouped by topic instead of by the date they were checked. 5859 -> ~3100 words. - plan.md: risks reordered by actual size, renumbered contiguously, cut-order guidance kept. 2550 -> ~1830 words. - research.md: four named spikes replacing spikes-plus-addendum; duplicated code cites removed. 2959 -> ~2480 words. No requirement, decision, or code citation dropped: all 16 FRs and all 57 file:line references verified present after the rewrite. For scale, the M1 sibling already on dev is a 560-word spec and an 828-word plan. Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Suppressed comments (5)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:49
- The claimed fallback is not present in the current Console. Its auth service performs LF Auth0 login, the decision/individual components keep using the routed
userId, and the onlyupdateUsermethod is unused; no GitHub authorization step binds a missing GitHub ID beforerequest-individual-signature. A contributor with no bound identity therefore reaches signing with an empty GitHub ACL instead of being bound “as it does today.” Make an ownership-verified binding flow an explicit M2 deliverable, or block this path until linking is complete.
- **A missing identity MUST NOT block**: hand off to the Console's GitHub-OAuth entry, which binds it as it does today.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:81
RepositoryNameIndexcannot perform suffix matching: its query usesrepository_name = :valueas the GSI key condition (repositories/repository.go:423-447). Parse a pasted URL to the canonical repository name and use an exact lookup, or use a different index/search mechanism; otherwise the documented fallback cannot be implemented on this GSI.
- **If it fails** — or if per-keystroke **repo-URL** search is needed at full fidelity, where cardinality is genuinely much larger — narrow repo-URL matching to exact/suffix match on the `RepositoryNameIndex` GSI first, and escalate to OpenSearch rather than a cache. Reasoning in [research.md](research.md).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:68
- This status predicate makes acceptance scenario 7 unreachable for existing approval-list removals. Those paths call
InvalidateProjectRecord, which setssignature_approved=false(signatures/repository.go:4139,4168,4177), so a contributor who no longer matches criteria is classified as Invalidated, not Needs attention. Define precedence using coverage plus invalidation provenance (including legacy rows with empty provenance), or change the approval-list mutation semantics.
This issue also appears on line 81 of the same file.
- **FR-010**: Each row MUST show one of three statuses with the mockup's exact labels and notes: **Valid** (approved ∧ covered); **Needs attention** (approved ∧ ¬covered) with *"No longer matches {company}'s approval criteria."*; **Invalidated** (¬approved), and when self-invalidated *"You confirmed you no longer work at {company}."* Source: extend `GET /v4/my-clas`, which already computes the coverage evaluation per ECLA row. SS-side this also requires relaxing M1's valid-only row filter and its boolean-derived status mapping (see Verified facts).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:70
- The existing request-authorization screen is not directly deep-linkable.
ClaRequestAuthorizationComponent.ngOnInit()readsSELECTED_COMPANYfrom browser storage, and submission also readsPROJECT; both are populated only after traversingCorporateDashboardComponent. A fresh link from My CLAs therefore renders without a company/managers and cannot submit. Require the Console deep link to carry identifiers and hydrate this state, or route through a supported initialization path.
- **FR-011**: "Needs attention" ECLA rows MUST link "Request approval →" into the Console's existing request-authorization flow as a deep link only. Per the mockup this action is **conditional on that state** and MUST be removed when the row becomes Invalidated. No SS-side approval-request logic.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/research.md:1
- This added artifact contradicts the PR description, which says
research.mdis deliberately not included and will be generated by Spec Kit; the description also says decisions 2 and 4 remain open, while this file says all five are settled. Update the PR description or the artifact set so reviewers and downstream Spec Kit runs have one consistent scope.
# Phase 0 Research: M2 — backend spikes
|
|
||
| - **FR-001**: My CLAs MUST offer a "Sign CLA" action opening a modal with one search box over **four sources**: project name, CLA-group name, org name with repo-source provenance (GitHub/GitLab/Gerrit, displayed), and repo URL (a pasted link resolved to its CLA Group). Results MUST come from a **new unscoped search endpoint** — no existing endpoint covers these four sources. Rows with several linked orgs MUST collapse them into an expandable "N linked orgs" affordance. There is no org/repo selection step: the signing unit is the CLA Group. | ||
| - **FR-001a**: The search endpoint MUST meet a per-keystroke budget of **< 300 ms p95 server time** (with ~200 ms client debounce and a server-side result cap) **without introducing a cache or new datastore in M2**. Implementation order: (a) serve from DynamoDB with the search term applied server-side; (b) fix the dead search filter (see Verified facts); (c) measure p95 at real cardinality before adding any layer. If (a)–(c) prove insufficient, the escalation is a **platform search index (OpenSearch)**, not a hand-rolled cache — rationale in [research.md](research.md). Exact-name lookups MAY reuse the `project-name-lower-search-index` GSI, but a GSI MUST NOT be forced to serve substring matching across four fields. | ||
| - **FR-002**: On selection, SS MUST hand off to the Console's existing decision-screen URL — `{console}/#/cla/project/{claGroupID}/user/{userID}` — without `?redirect=`. The ICLA/ECLA choice, legal guidance, and `project_icla_enabled`/`project_ccla_enabled` gating stay in the Console. |
| - **FR-001**: My CLAs MUST offer a "Sign CLA" action opening a modal with one search box over **four sources**: project name, CLA-group name, org name with repo-source provenance (GitHub/GitLab/Gerrit, displayed), and repo URL (a pasted link resolved to its CLA Group). Results MUST come from a **new unscoped search endpoint** — no existing endpoint covers these four sources. Rows with several linked orgs MUST collapse them into an expandable "N linked orgs" affordance. There is no org/repo selection step: the signing unit is the CLA Group. | ||
| - **FR-001a**: The search endpoint MUST meet a per-keystroke budget of **< 300 ms p95 server time** (with ~200 ms client debounce and a server-side result cap) **without introducing a cache or new datastore in M2**. Implementation order: (a) serve from DynamoDB with the search term applied server-side; (b) fix the dead search filter (see Verified facts); (c) measure p95 at real cardinality before adding any layer. If (a)–(c) prove insufficient, the escalation is a **platform search index (OpenSearch)**, not a hand-rolled cache — rationale in [research.md](research.md). Exact-name lookups MAY reuse the `project-name-lower-search-index` GSI, but a GSI MUST NOT be forced to serve substring matching across four fields. | ||
| - **FR-002**: On selection, SS MUST hand off to the Console's existing decision-screen URL — `{console}/#/cla/project/{claGroupID}/user/{userID}` — without `?redirect=`. The ICLA/ECLA choice, legal guidance, and `project_icla_enabled`/`project_ccla_enabled` gating stay in the Console. | ||
| - **FR-003**: SS MUST resolve the EasyCLA `userID` server-side from the session identity; never from client input. SS MUST NOT write a GitHub identity onto an EasyCLA user record — `user.GithubID` becomes the signature ACL that gates merges, so binding stays owned by the Console's existing GitHub OAuth. For Gerrit, `GET /v4/user-from-token` alone is correct. |
Legal/stakeholder review on 2026-08-14 removed self-service invalidation entirely (ICLA + ECLA). Replaces it with CLA-manager-routed removal requests for ECLAs, informational-only ICLA rows, signed-under identity display, a Manage-in-CCLA-Console link for managers, and a new system-set read-only Revoked status for sanctions/OFAC cases, matching the reshaped scope in lfx-self-serve#1229 and its linked tasks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (6)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:49
- FR-003 no longer defines how the selected GitHub account determines the routed EasyCLA record.
GET /v4/my-clas/identitiesreturns only a flat set of identity strings (v2/my_clas/service.go:298-356), whileuser-from-tokenresolves by LF username/email; neither maps a picked GitHub ID to itsuserID. For users whose linked accounts live on different EasyCLA records, the hand-off can therefore use the wrong record and create the ACL from that record's GitHub ID. Require an authenticated, platform-aware resolver that verifies the selected GitHub ID and returns its correspondinguserID.
- **FR-003**: SS MUST resolve the EasyCLA `userID` server-side from the session identity; never from client input. SS MUST NOT write a GitHub identity onto an EasyCLA user record — `user.GithubID` becomes the signature ACL that gates merges, so binding stays owned by the Console's existing GitHub OAuth. For Gerrit, `GET /v4/user-from-token` alone is correct.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:40
- This security gate still cannot pass based only on server-side redirect construction. The
userIDremains editable in the URL, and the Console ultimately calls unauthenticatedPOST /v4/request-individual-signature(security: []) with a client-supplieduser_id; its handler uses that ID directly. Require the proactive signing request to authenticate the caller and bind/verify the target user end-to-end (or use a signed, one-time hand-off token), rather than treating redirect-time resolution as sufficient.
- **Security**: `userID` derived server-side. The manager-resolution/notification endpoint MUST verify the caller owns the ECLA signature it's requesting removal/approval for (reusing `authorizeIdentity`, `v2/my_clas/service.go:383`) so it can't be used to probe or spam notifications for another contributor's agreement — but it has no invalidation blast radius, since it writes no signature or Approved-List state. **PASS — smaller security surface than the retired design.**
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:95
- This excludes the write-path work required by the linked M2 revocation task. The current signature model has no revocation fields and
InvalidateProjectRecordwrites onlysignature_approvedandnote; lfx-self-serve#1370 explicitly requires the sanctions and CLA-manager removal paths to write the new timestamp/provenance. If M2 only reads the marker, no newly revoked row can provideRevoked · <date>, and revalidation remains able to reactivate it. Include those writer changes in this plan and risk estimate.
1. **Status response shape** — the largest single item, unchanged in size by the 2026-08-14 revision, just re-termed. The upstream evaluation exists but the response shape does not: `row.Valid = sig.SignatureApproved && covered` (`v2/my_clas/service.go:218`) makes Needs attention unrepresentable, and telling **Revoked** apart from ordinary non-approval needs a third input. FR-010a requires three independent fields, the third being the revocation-provenance marker (FR-010b) — written by the sanctions-screening path and the corporate console, not by M2, but M2 must read and expose it. SS-side delta confirmed: M1's BFF filters non-valid rows and derives status from the boolean.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:77
- The three-state mapping is not exhaustive for the legacy records this requirement says must be tolerated. A signed pre-M2 signature with
signature_approved=falseand empty revocation metadata is neither Valid, Needs attention (both require approved), nor Revoked, yet relaxing M1's valid-only filter will return it. Define a fallback status/backfill rule or keep these rows filtered so every returned row can satisfy FR-010.
- **FR-010**: Each row MUST show one of three statuses with the mockup's exact labels and notes: **Valid** (approved ∧ covered ∧ ¬revoked); **Needs attention** (approved ∧ ¬covered ∧ ¬revoked) with *"No longer matches {company}'s approval criteria."*; **Revoked** (system-revoked, e.g. sanctions/OFAC screening) — **read-only**, showing `Revoked · <date>` with **no user actions** (no Request Removal, no Request approval, no download). Source: extend `GET /v4/my-clas`, which already computes the coverage evaluation per ECLA row. SS-side this also requires relaxing M1's valid-only row filter and its boolean-derived status mapping (see Verified facts). Copy MUST avoid "Canceled/Invalid" — the agreed term is **Revoked**.
- **FR-010a**: The `GET /v4/my-clas` response MUST carry **three independent fields** rather than one boolean: (a) approved-ness, (b) coverage, (c) revocation provenance (FR-010b's reason/actor, system-set only). Today approved-ness and coverage are collapsed into `row.Valid`, which makes **Needs attention** unrepresentable. `Valid` MAY be retained as a derived convenience but MUST NOT be the only signal. This is the largest single M2 change to the read path and a prerequisite for FR-010 and FR-011.
- **FR-010b**: Revoked signatures MUST carry structured metadata: a **revocation timestamp** and a **revocation reason/actor** (e.g. `sanctions_screening`, `cla_manager`, system), additive on the signatures table. This is the durable marker that keeps a revoked ECLA excluded from `auto_create_ecla`/Approved-List re-validation — a bare `signature_approved` flip is not durable (see Verified facts). Written only by the system revocation path and by CLA-manager-initiated removals in the corporate console — **never** by a Self-Serve write, since none exists. Signatures revoked before M2, or invalidated under the pre-2026-08-14 self-service design, carry no such metadata, so consumers MUST tolerate empty values.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:68
- The notification contract is contradictory: this line permits either email or an in-app mechanism, but FR-008a immediately requires that managers “MUST be notified by email,” while the plan repeats that delivery is an implementation choice. Resolve this to one mandatory behavior before contract/task generation; otherwise an in-app-only implementation can satisfy FR-008 while violating FR-008a.
- **FR-008**: Each valid ECLA row MUST offer **Request Removal**, opening a shared **Contact CLA Manager** modal (removal mode): an optional message and Send. Copy MUST make clear the manager completes the removal ("This starts the process to invalidate it on your behalf"). Sending MUST NOT invalidate, flip, or otherwise mutate any signature or Approved-List state in Self Serve or `cla-backend-go` — it only notifies. **Delivery mechanism is an implementation detail, not a contract**: the simplest correct option is a **plain email** to the resolved CLA manager(s) (no in-app manager-list UI required); an in-app manager picker is not mandated if email is simpler to ship. This requires a **new** `cla-backend-go` endpoint (or reuse of an existing manager-notification path, if one exists) to resolve the CLA manager(s) for a company + CLA group and send the notification.
- **FR-008a**: The CLA manager(s) MUST be notified by email when a contributor sends a Request Removal or Request approval (FR-011) — this email **is** the notification mechanism referenced in FR-008, not an addition to it. The template carries the contributor's signed-under identity (FR-007), project/CLA group, company, the optional message, and the request type (removal vs. approval). There is **no contributor-facing invalidation email** — self-service invalidation does not exist.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:47
- A DynamoDB
Scanfilter does not provide the stated bounded search:FilterExpressionis applied after items are read, andLimitcaps evaluated items rather than matching results. Thus fixing the dead filter does not bound latency/RCUs, while applying the result cap directly can omit valid matches outside the evaluated page. Specify complete pagination until the result cap is filled and make the cardinality/latency measurement a gate, or require an indexed search strategy if the scan cannot meet the budget.
- **FR-001a**: The search endpoint MUST meet a per-keystroke budget of **< 300 ms p95 server time** (with ~200 ms client debounce and a server-side result cap) **without introducing a cache or new datastore in M2**. Implementation order: (a) serve from DynamoDB with the search term applied server-side; (b) fix the dead search filter (see Verified facts); (c) measure p95 at real cardinality before adding any layer. If (a)–(c) prove insufficient, the escalation is a **platform search index (OpenSearch)**, not a hand-rolled cache — rationale in [research.md](research.md). Exact-name lookups MAY reuse the `project-name-lower-search-index` GSI, but a GSI MUST NOT be forced to serve substring matching across four fields.
…sting request pattern Mockup v17 Final (post-legal-review) changes two things about the GitHub hand-off that the artifacts described loosely: - The account picker is shown whenever one or more GitHub identities are linked -- no silent auto-pick, even for a single account. - Zero linked accounts blocks continuing with an empty state linking to Identities, rather than falling through to the Console. Propagated through FR-004, acceptance scenario 2, plan.md risk 6, and research.md spike 4. FR-007 now states explicitly that the signed-under identity renders inline inside the Signed cell, not as a new table column, matching the prototype. FR-008 previously left delivery "an implementation detail" and guessed that an existing manager-notification path might exist. One does: the Approved List request flow (addCclaAllowlistRequest). Verified in cla-backend-go -- it resolves managers from the CCLA signature ACL, persists a request row to cla-*-ccla-whitelist-requests with a status field, sends a templated email over SNS-backed utils.SendEmail(), and logs an audit event. It is not fire-and-forget, so "same as the Approved List flow" means all four steps. FR-008 now names that precedent with file-level pointers, FR-008b requires the request record as a receipt (explicitly descoping the precedent's approve/reject half), and new FR-008c covers the case the precedent handles silently: an empty or stale signature_acl resolves to zero recipients, which must not surface as a Send button that goes nowhere. Also corrects the now-inaccurate "no new storage/datastore" claims in plan.md, which predate the FR-008b request record. Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
Suppressed comments (5)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:154
- The end-to-end identity security question is still unresolved despite this statement. The fragment
userIDis editable after SS derives it, and/v4/request-individual-signatureis unauthenticated and acceptsuser_idfrom its body (swagger/cla.v2.yaml:4376-4387). Require the Console/backend to authenticate the signing request and verify that the routed user belongs to the LF session; otherwise server-side derivation at redirect does not prevent initiating for another record.
Phase 1 (`data-model.md`, `contracts/`, `quickstart.md`) is not blocked on clarification. One **measurement** is outstanding: CLA-group cardinality (see Performance assumption). Contracts to write: the four-source search endpoint (FR-001/FR-001a), the extended `GET /v4/my-clas` three-field response (FR-010a/FR-010b), the CLA-manager-resolution + notification endpoint for Request Removal/Request approval (FR-008/FR-008a), and the hand-off URL + pre-flight read (FR-002/FR-003/FR-004). No invalidation-endpoint contract is needed — that design is retired.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:78
- The three-state truth table omits signed records with
approved=falseand no revocation metadata, even though FR-010b explicitly says pre-M2 invalidations have empty metadata.GetMyClasreturns every signed record regardless of approval (v2/my_clas/service.go:171-221), so relaxing M1's valid-only filter exposes rows that match neither Valid, Needs attention, nor Revoked and have no revocation date. Define whether these legacy rows are excluded, backfilled/mapped with an unknown date, or represented by another state.
- **FR-010**: Each row MUST show one of three statuses with the mockup's exact labels and notes: **Valid** (approved ∧ covered ∧ ¬revoked); **Needs attention** (approved ∧ ¬covered ∧ ¬revoked) with *"No longer matches {company}'s approval criteria."*; **Revoked** (system-revoked, e.g. sanctions/OFAC screening) — **read-only**, showing `Revoked · <date>` with **no user actions** (no Request Removal, no Request approval, no download). Source: extend `GET /v4/my-clas`, which already computes the coverage evaluation per ECLA row. SS-side this also requires relaxing M1's valid-only row filter and its boolean-derived status mapping (see Verified facts). Copy MUST avoid "Canceled/Invalid" — the agreed term is **Revoked**.
- **FR-010a**: The `GET /v4/my-clas` response MUST carry **three independent fields** rather than one boolean: (a) approved-ness, (b) coverage, (c) revocation provenance (FR-010b's reason/actor, system-set only). Today approved-ness and coverage are collapsed into `row.Valid`, which makes **Needs attention** unrepresentable. `Valid` MAY be retained as a derived convenience but MUST NOT be the only signal. This is the largest single M2 change to the read path and a prerequisite for FR-010 and FR-011.
- **FR-010b**: Revoked signatures MUST carry structured metadata: a **revocation timestamp** and a **revocation reason/actor** (e.g. `sanctions_screening`, `cla_manager`, system), additive on the signatures table. This is the durable marker that keeps a revoked ECLA excluded from `auto_create_ecla`/Approved-List re-validation — a bare `signature_approved` flip is not durable (see Verified facts). Written only by the system revocation path and by CLA-manager-initiated removals in the corporate console — **never** by a Self-Serve write, since none exists. Signatures revoked before M2, or invalidated under the pre-2026-08-14 self-service design, carry no such metadata, so consumers MUST tolerate empty values.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:70
- The zero-manager branch is not executable as written: the UI removes Send, but the request record is required to be written, leaving no user action or endpoint call that triggers persistence. Define an explicit blocked-case submission/report action that writes the receipt and returns “no recipients,” or remove the persistence requirement when submission is blocked; writing on modal open would create duplicate requests without consent.
- **FR-008c**: CLA manager(s) MUST be resolved from the **CCLA signature ACL** for the company + CLA group (`SignatureACL`, hydrated to full users with emails — see Verified facts), the same source both existing request flows use. **When zero managers resolve** (empty or stale `signature_acl` — the existing Approved-List flow merely logs a warning and bails), the UI MUST NOT present a Send action that silently goes nowhere: the modal MUST show an explanatory state telling the contributor no CLA manager is currently reachable for this company and pointing them at LF support. The request record MUST still be written, so the dead end is diagnosable. This applies equally to Request Removal and Request approval (FR-011).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:67
- This “no manager-list UI” contract conflicts with linked M2 tasks #1574 and #1372, which require the resolved CLA manager(s) list in the shared modal, while the PR description also leaves email versus in-app delivery as an implementation choice. That changes both the endpoint response and UI scope while this spec claims alignment with those tickets. Reconcile the source of truth before Spec Kit generates contracts/tasks.
- **FR-008**: Each valid ECLA row MUST offer **Request Removal**, opening a shared **Contact CLA Manager** modal (removal mode): an optional message and Send. Copy MUST make clear the manager completes the removal ("This starts the process to invalidate it on your behalf"). Sending MUST NOT invalidate, flip, or otherwise mutate any signature or Approved-List state in Self Serve or `cla-backend-go` — it only notifies. **Delivery is by email to the resolved CLA manager(s)** — no in-app manager-list UI or manager picker is required. This requires a **new** `cla-backend-go` endpoint, which MUST follow the shape of the existing **Approved-List request** flow (`addCclaAllowlistRequest`, `POST /company/{companyID}/ccla-whitelist-requests/{projectID}` — see Verified facts) rather than inventing a new pattern: resolve managers from the CCLA signature ACL, persist a request record, send a templated email, and log an audit event. Note that "same as the Approved-List flow" means **all four** of those steps — that flow is *not* fire-and-forget, and an email-only implementation would diverge from the named precedent.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:96
- This “no in-app manager list” scope contradicts linked M2 tasks #1574 and #1372, which require the resolved manager(s) list in the modal. Because that changes the manager-resolution response and frontend work, the plan cannot claim to follow those tasks while omitting it; reconcile the ticket/mockup contract before generating tasks.
2. **CLA-manager resolution + notification endpoint** (FR-008/FR-008a) — the one genuinely new backend piece, and **smaller than the retired ECLA-invalidation endpoint it replaces**, largely because it has a working precedent to copy rather than a design to invent. Follow `addCclaAllowlistRequest`: resolve managers from the company/CLA-group CCLA signature ACL (`GetProjectCompanySignatures` → `SignatureACL`), persist a request row, send a templated email via SNS-backed `utils.SendEmail()`, log an audit event. Delivery is email; no in-app manager list. No Approved-List mutation, no signature write, no durable-exclusion marker to design (unlike the retired design) — the marker in risk 1 is written elsewhere, not by this endpoint. **Descoped from the precedent**: approve/reject endpoints and any request-status UI — the manager finishes in the CCLA console. Ownership check still required: the caller must own the ECLA they're requesting removal/approval for. **Watch the zero-manager case** (FR-008c): an empty/stale `signature_acl` resolves to no recipients, and the precedent silently warns-and-bails.
| - **FR-001**: My CLAs MUST offer a "Sign CLA" action opening a modal with one search box over **four sources**: project name, CLA-group name, org name with repo-source provenance (GitHub/GitLab/Gerrit, displayed), and repo URL (a pasted link resolved to its CLA Group). Results MUST come from a **new unscoped search endpoint** — no existing endpoint covers these four sources. Rows with several linked orgs MUST collapse them into an expandable "N linked orgs" affordance. There is no org/repo selection step: the signing unit is the CLA Group. | ||
| - **FR-001a**: The search endpoint MUST meet a per-keystroke budget of **< 300 ms p95 server time** (with ~200 ms client debounce and a server-side result cap) **without introducing a cache or new datastore in M2**. Implementation order: (a) serve from DynamoDB with the search term applied server-side; (b) fix the dead search filter (see Verified facts); (c) measure p95 at real cardinality before adding any layer. If (a)–(c) prove insufficient, the escalation is a **platform search index (OpenSearch)**, not a hand-rolled cache — rationale in [research.md](research.md). Exact-name lookups MAY reuse the `project-name-lower-search-index` GSI, but a GSI MUST NOT be forced to serve substring matching across four fields. | ||
| - **FR-002**: On selection, SS MUST hand off to the Console's existing decision-screen URL — `{console}/#/cla/project/{claGroupID}/user/{userID}` — without `?redirect=`. The ICLA/ECLA choice, legal guidance, and `project_icla_enabled`/`project_ccla_enabled` gating stay in the Console. | ||
| - **FR-003**: SS MUST resolve the EasyCLA `userID` server-side from the session identity; never from client input. SS MUST NOT write a GitHub identity onto an EasyCLA user record — `user.GithubID` becomes the signature ACL that gates merges, so binding stays owned by the Console's existing GitHub OAuth. For Gerrit, `GET /v4/user-from-token` alone is correct. |
| - **FR-008a**: The CLA manager(s) MUST be notified by email when a contributor sends a Request Removal or Request approval (FR-011) — this email **is** the notification mechanism referenced in FR-008, not an addition to it. The template carries the contributor's signed-under identity (FR-007), project/CLA group, company, the optional message, and the request type (removal vs. approval). There is **no contributor-facing invalidation email** — self-service invalidation does not exist. | ||
| - **FR-008b**: Removal and approval requests are **not** recorded as invalidation metadata on the signature (there is no self-service invalidation to attribute). Only the system-set Revoked state carries structured metadata — see FR-010b. Requests ARE, however, persisted as their own **request record** (who asked, which signature/CLA group, request type, timestamp, optional message) plus an audit event, matching the Approved-List precedent. This record is a **receipt, not a workflow**: M2 ships no approve/reject endpoints and no request-status UI on either side — the manager completes the removal in the CCLA console. The record exists so that "did this request ever reach a manager?" is answerable by support and by audit, which a fire-and-forget email cannot do for what is a legally significant action. | ||
| - **FR-008c**: CLA manager(s) MUST be resolved from the **CCLA signature ACL** for the company + CLA group (`SignatureACL`, hydrated to full users with emails — see Verified facts), the same source both existing request flows use. **When zero managers resolve** (empty or stale `signature_acl` — the existing Approved-List flow merely logs a warning and bails), the UI MUST NOT present a Send action that silently goes nowhere: the modal MUST show an explanatory state telling the contributor no CLA manager is currently reachable for this company and pointing them at LF support. The request record MUST still be written, so the dead end is diagnosable. This applies equally to Request Removal and Request approval (FR-011). | ||
| - **FR-009**: No SS-side invalidation writes exist, so there is nothing for the impersonation-readonly middleware to block for this flow. (Retained from history: impersonation write-blocking remains the pattern for any future contributor-facing write Self Serve does own.) |
|
|
||
| - **FR-010**: Each row MUST show one of three statuses with the mockup's exact labels and notes: **Valid** (approved ∧ covered ∧ ¬revoked); **Needs attention** (approved ∧ ¬covered ∧ ¬revoked) with *"No longer matches {company}'s approval criteria."*; **Revoked** (system-revoked, e.g. sanctions/OFAC screening) — **read-only**, showing `Revoked · <date>` with **no user actions** (no Request Removal, no Request approval, no download). Source: extend `GET /v4/my-clas`, which already computes the coverage evaluation per ECLA row. SS-side this also requires relaxing M1's valid-only row filter and its boolean-derived status mapping (see Verified facts). Copy MUST avoid "Canceled/Invalid" — the agreed term is **Revoked**. | ||
| - **FR-010a**: The `GET /v4/my-clas` response MUST carry **three independent fields** rather than one boolean: (a) approved-ness, (b) coverage, (c) revocation provenance (FR-010b's reason/actor, system-set only). Today approved-ness and coverage are collapsed into `row.Valid`, which makes **Needs attention** unrepresentable. `Valid` MAY be retained as a derived convenience but MUST NOT be the only signal. This is the largest single M2 change to the read path and a prerequisite for FR-010 and FR-011. | ||
| - **FR-010b**: Revoked signatures MUST carry structured metadata: a **revocation timestamp** and a **revocation reason/actor** (e.g. `sanctions_screening`, `cla_manager`, system), additive on the signatures table. This is the durable marker that keeps a revoked ECLA excluded from `auto_create_ecla`/Approved-List re-validation — a bare `signature_approved` flip is not durable (see Verified facts). Written only by the system revocation path and by CLA-manager-initiated removals in the corporate console — **never** by a Self-Serve write, since none exists. Signatures revoked before M2, or invalidated under the pre-2026-08-14 self-service design, carry no such metadata, so consumers MUST tolerate empty values. |
| **Project Type**: web application (existing monorepo) + swagger-first backend slice. | ||
| **Performance Goals**: My CLAs interactive < 2s p95 including status; **sign-entry search < 300 ms p95 server time** (per-keystroke, with ~200 ms client debounce and a result cap — FR-001a); hand-off redirect adds no perceptible latency. The search budget MUST be met without a cache or new datastore — see the spec's Performance assumption for the measure-first sequence and the unverified cardinality it rests on. | ||
| **Constraints**: SS makes **no invalidation writes at all** — the only contributor-facing write is the Request-Removal/Request-approval notification send (FR-008), which mutates no signature or Approved-List state; no signing-initiation calls (FR-005); the one implicit write is `GET /v4/user-from-token`'s lookup-or-create side effect (FR-003); server-side identity derivation throughout; PR-check link untouched (FR-006); feature-flagged dark launch; 3-week budget. | ||
| **Scale/Scope**: all LFX contributors; extends 1 existing page, ~4 server routes (search, `userID` resolution/hand-off + pre-flight, manager-notification send), **2 new upstream endpoints** (four-source search; CLA-manager resolution + notification) and **1 revision** (`GET /v4/my-clas` three-field status + signed-under identity). No SS-owned identity-binding operation, no invalidation endpoint, and no cache. One additive request-record table for FR-008b, modeled on the existing Approved-List request table. |
| 3. **Four-source search endpoint** — no existing endpoint fits (both listings are SFID-scoped; `GetCLAGroupByName` is exact-match-only), and the mockup searches four sources with a per-keystroke expectation. Includes a latent bug to fix: `GetCLAGroups` logs its search parameters then builds a projection with no filter (`project/repository/repository.go:529-538`), so search on this table has never been measured. **Deliberately no cache or new datastore** (FR-001a): serve from DynamoDB, fix the filter, measure p95; escalate to OpenSearch only if measurement demands it. **Blocking measurement**: the CLA-group row count, unobtainable in the authoring session (no `lfproduct-*` AWS profile locally; `lfx-*` SSO tokens expired) — an explicit unverified assumption, not a finding. | ||
| 4. **Proactive-ICLA gap** — spiked and **smaller than feared** ([research.md](research.md) Spike 1). Two independent dependencies, one per repo: the Console's `findActiveSignature()` hard-stop (`individual-dashboard.component.ts:49-66`) and the backend's PR-metadata requirement (`v2/sign/service.go:2890,2903`). Fix is a widened branch condition in each plus an explicit proactive request signal — no DocuSign/schema/webhook change. Required for the GitHub sign-entry path. Caveat: `acl = github:{user.GithubID}` (`:1433`) must still be set, so the GitHub-ID binding must land before signing. | ||
| 5. **Signed-under identity display** (FR-007) — small, additive: the signer identity and its repo-source type are already tied to the signature record; expose them in the `GET /v4/my-clas` response and render inline. No new endpoint. | ||
| 6. **Account authorization** — **retired by a scope decision, not by new code.** M2 builds no identity-binding operation: SS runs a read-only pre-flight against `GET /v4/my-clas/identities` and delegates binding to the Console's existing GitHub OAuth (FR-003/FR-004). Residual work: a Gerrit/GitHub conditional, an account-picker modal shown whenever one or more GitHub identities are linked (no auto-select, even for one), and an empty-state block (with a link to Identities) when none are linked. Identity types verified compatible (numeric IDs on both sides). |
|
|
||
| - **The three call sites above are still exactly where the durability fix belongs** — they're now guarding the system-set **Revoked** marker (FR-010b) instead of a self-service one. | ||
| - **The attribute shape survives**: a revocation timestamp + reason/actor (now values like `sanctions_screening` / `cla_manager`, not `self`), atomically alongside `InvalidateProjectRecord`'s existing write. `InvalidateProjectRecord` does **not** set `date_modified`, so the new timestamp is still what makes the date recoverable. | ||
| - **What does not survive**: there is no M2-owned write path. The marker is written by the sanctions-screening job and by CLA-manager actions in the corporate console — both outside this milestone's code. M2 only needs to **read** the marker (for FR-010's status field) and honor it at the three call sites so a revoked ECLA doesn't silently get re-approved by `auto_create_ecla`. |
| No Spec Kit scaffolding exists in either repo (there is no `.specify/` directory in `easycla` or `lfx-self-serve`). M1's artifacts were authored in Spec-Kit *shape* by hand; M2 follows that convention. Committing `.specify/` is a separate change and not a prerequisite. Default gates: | ||
|
|
||
| - **Simplicity**: no new services and no cache; extends M1's existing tab and `cla` server seam; hand-off is the Console's existing deep link; the retired self-service invalidation design (revised `invalidateICLA`, new ECLA-invalidation endpoint) is **not built at all** — M2 now makes zero invalidation writes. The one genuinely new backend piece is the CLA-manager resolution + notification endpoint (FR-008), which adds **one request-record table** modeled directly on the existing `cla-*-ccla-whitelist-requests` (FR-008b) — a receipt for audit/support, with no status workflow. **PASS — simpler than the pre-2026-08-14 design.** | ||
| - **Security**: `userID` derived server-side. The manager-resolution/notification endpoint MUST verify the caller owns the ECLA signature it's requesting removal/approval for (reusing `authorizeIdentity`, `v2/my_clas/service.go:383`) so it can't be used to probe or spam notifications for another contributor's agreement — but it has no invalidation blast radius, since it writes no signature or Approved-List state. **PASS — smaller security surface than the retired design.** |
|
|
||
| No constitution violations. Schedule risks against the 3-week budget, in order: | ||
|
|
||
| 1. **Status response shape** — the largest single item, unchanged in size by the 2026-08-14 revision, just re-termed. The upstream evaluation exists but the response shape does not: `row.Valid = sig.SignatureApproved && covered` (`v2/my_clas/service.go:218`) makes Needs attention unrepresentable, and telling **Revoked** apart from ordinary non-approval needs a third input. FR-010a requires three independent fields, the third being the revocation-provenance marker (FR-010b) — written by the sanctions-screening path and the corporate console, not by M2, but M2 must read and expose it. SS-side delta confirmed: M1's BFF filters non-valid rows and derives status from the boolean. |
|
|
||
| ### Why the write path was rejected rather than designed | ||
|
|
||
| The open question was the shape of a backend-owned identity-binding operation (create-vs-enrich branches, gateway auth). **Recommendation adopted: build neither branch.** SS runs a read-only pre-flight against the shipped `GET /v4/my-clas/identities`, gates per repo-source type (Gerrit needs no GitHub ID), and when the identity is missing hands off to the Console's existing GitHub OAuth. |
… the contributor pick recipients FR-008 claimed no in-app manager-list UI was required. Re-reading mockup v17 Final shows the opposite: the modal has a "CLA manager(s)" field label and a #mgrManagerList container, and openMgrModal() renders one checkbox per resolved manager, checked by default, with a toggleMgrManager() handler. The mock data has multi-manager groups, and all three modal hints read "the CLA manager(s) below" -- the list is part of the copy contract. So recipient selection is a real requirement, not an optional nicety: Send must notify only the selected managers, which also means the resolution result has to be returned to the client rather than consumed server-side. Also specifies the zero-selected case (unchecking everyone), which the prototype does not cover: Send must not silently deliver nothing. Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:53
- The selected account cannot reliably drive this hand-off.
GetMyIdentitiesemits numericgithub-idvalues only from existing EasyCLA records; identities loaded from Platform user-service contribute usernames and emails, not numeric IDs (v2/my_clas/service.go:301-347,525-589). The URL then carries onlyuserID, and signing builds the ACL from that routed user's storedGithubID(v2/sign/service.go:1431-1435). A newly linked account or a selection bound to another EasyCLA record can therefore produce an empty/wrong ACL. Require a numeric-ID source and resolve the selected ID to the matching EasyCLA user before hand-off, or define an explicit Console-owned binding step.
- **FR-003**: SS MUST resolve the EasyCLA `userID` server-side from the session identity; never from client input. SS MUST NOT write a GitHub identity onto an EasyCLA user record — `user.GithubID` becomes the signature ACL that gates merges, so binding stays owned by the Console's existing GitHub OAuth. For Gerrit, `GET /v4/user-from-token` alone is correct.
- **FR-004**: Before a GitHub hand-off, SS MUST run a **read-only identity pre-flight** against the shipped `GET /v4/my-clas/identities`:
- **Gate per repo-source type**: a Gerrit-backed group MUST NOT be gated; a GitHub-backed one requires a bound GitHub ID. GitLab follows the same shape by config.
- **When one or more GitHub identities are linked**, SS MUST show an account-picker modal (per mockup v17 Final) so the contributor chooses which linked GitHub account to sign with before continuing to the Console — no silent auto-pick, even for a single linked account.
- **When zero GitHub identities are linked**, the picker MUST show an empty state that blocks continuing ("No GitHub accounts are connected to your profile yet") with a link to Identities to link one; it MUST NOT fall through to the Console. This is a stricter gate than a Gerrit-backed group, which needs no account step at all.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:95
- This excludes the writer work that the M2 revocation-metadata task requires. lfx-self-serve#1370 explicitly requires the new fields to be written by sanctions screening and CLA-manager removals, while the current
InvalidateProjectRecordstill writes onlysignature_approvedandnote(signatures/repository.go:2088-2129). If those writer paths remain outside M2, the new Revoked state cannot be produced durably. Include the writer updates in this plan or remove Revoked from M2 pending the owning delivery.
1. **Status response shape** — the largest single item, unchanged in size by the 2026-08-14 revision, just re-termed. The upstream evaluation exists but the response shape does not: `row.Valid = sig.SignatureApproved && covered` (`v2/my_clas/service.go:218`) makes Needs attention unrepresentable, and telling **Revoked** apart from ordinary non-approval needs a third input. FR-010a requires three independent fields, the third being the revocation-provenance marker (FR-010b) — written by the sanctions-screening path and the corporate console, not by M2, but M2 must read and expose it. SS-side delta confirmed: M1's BFF filters non-valid rows and derives status from the boolean.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:40
- The security gate is still incomplete. The hand-off
userIDis editable in the URL fragment, whilePOST /v4/request-individual-signaturehassecurity: []and its handler trustsinput.UserIDwithout an authenticated principal (swagger/cla.v2.yaml:4376-4387,v2/sign/handlers.go:164-182). Resolving the initial redirect server-side therefore does not prevent a modified Console request from initiating signing for another user. Require the initiation endpoint to authenticate and bind the requested user to the session (or to a signed, single-use hand-off token) before marking this gate PASS.
- **Security**: `userID` derived server-side. The manager-resolution/notification endpoint MUST verify the caller owns the ECLA signature it's requesting removal/approval for (reusing `authorizeIdentity`, `v2/my_clas/service.go:383`) so it can't be used to probe or spam notifications for another contributor's agreement — but it has no invalidation blast radius, since it writes no signature or Approved-List state. **PASS — smaller security surface than the retired design.**
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:70
- This zero-manager flow has no operation that can create the required receipt: the UI must remove Send, while the planned upstream operation is the combined manager-resolution/notification send. Merely opening the modal cannot currently persist a request record. Specify whether resolution itself creates an idempotent zero-manager receipt or provide a separate submit/report action; otherwise the diagnosability requirement is unreachable.
- **FR-008c**: CLA manager(s) MUST be resolved from the **CCLA signature ACL** for the company + CLA group (`SignatureACL`, hydrated to full users with emails — see Verified facts), the same source both existing request flows use. **When zero managers resolve** (empty or stale `signature_acl` — the existing Approved-List flow merely logs a warning and bails), the UI MUST NOT present a Send action that silently goes nowhere: the modal MUST show an explanatory state telling the contributor no CLA manager is currently reachable for this company and pointing them at LF support. The request record MUST still be written, so the dead end is diagnosable. This applies equally to Request Removal and Request approval (FR-011).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:78
- The three-state mapping leaves legacy records unclassifiable. This file explicitly says pre-M2 invalidations can have
approved=falsewith empty revocation provenance, but such a row is neither Valid nor Needs attention under FR-010 and cannot be Revoked with the required provenance/date. Once M1's valid-only filter is relaxed, these rows need a defined fallback or a backfill strategy.
- **FR-010**: Each row MUST show one of three statuses with the mockup's exact labels and notes: **Valid** (approved ∧ covered ∧ ¬revoked); **Needs attention** (approved ∧ ¬covered ∧ ¬revoked) with *"No longer matches {company}'s approval criteria."*; **Revoked** (system-revoked, e.g. sanctions/OFAC screening) — **read-only**, showing `Revoked · <date>` with **no user actions** (no Request Removal, no Request approval, no download). Source: extend `GET /v4/my-clas`, which already computes the coverage evaluation per ECLA row. SS-side this also requires relaxing M1's valid-only row filter and its boolean-derived status mapping (see Verified facts). Copy MUST avoid "Canceled/Invalid" — the agreed term is **Revoked**.
- **FR-010a**: The `GET /v4/my-clas` response MUST carry **three independent fields** rather than one boolean: (a) approved-ness, (b) coverage, (c) revocation provenance (FR-010b's reason/actor, system-set only). Today approved-ness and coverage are collapsed into `row.Valid`, which makes **Needs attention** unrepresentable. `Valid` MAY be retained as a derived convenience but MUST NOT be the only signal. This is the largest single M2 change to the read path and a prerequisite for FR-010 and FR-011.
- **FR-010b**: Revoked signatures MUST carry structured metadata: a **revocation timestamp** and a **revocation reason/actor** (e.g. `sanctions_screening`, `cla_manager`, system), additive on the signatures table. This is the durable marker that keeps a revoked ECLA excluded from `auto_create_ecla`/Approved-List re-validation — a bare `signature_approved` flip is not durable (see Verified facts). Written only by the system revocation path and by CLA-manager-initiated removals in the corporate console — **never** by a Self-Serve write, since none exists. Signatures revoked before M2, or invalidated under the pre-2026-08-14 self-service design, carry no such metadata, so consumers MUST tolerate empty values.
|
|
||
| **2026-08-14 legal/stakeholder review**: self-service invalidation is removed entirely, for both ICLA and ECLA. There are **no user-initiated invalidation writes** from the Me lens. ICLAs are informational-only here — no removal action of any kind. ECLA "removal" is a **request to the CLA manager**, who performs the actual removal in the corporate console; sanctions-based revocation is system-set (see Status below). Request Removal and Request approval share one modal component, differing only in mode. | ||
|
|
||
| - **FR-008**: Each valid ECLA row MUST offer **Request Removal**, opening a shared **Contact CLA Manager** modal (removal mode): the **resolved CLA manager(s)**, an optional message, and Send. Copy MUST make clear the manager completes the removal ("This starts the process to invalidate it on your behalf"). Sending MUST NOT invalidate, flip, or otherwise mutate any signature or Approved-List state in Self Serve or `cla-backend-go` — it only notifies. **Delivery is by email**, but the modal MUST **display the resolved manager(s) and let the contributor select recipients**: per mockup v17 Final each manager renders as a checkbox checked by default and can be unchecked (`mgrManagerList` / `toggleMgrManager`), and all three modal hints read "the CLA manager(s) **below**". Send MUST notify only the **selected** managers. The zero-selected case MUST NOT send silently — either disable Send or require at least one recipient (the prototype does not cover this). This requires a **new** `cla-backend-go` endpoint, which MUST follow the shape of the existing **Approved-List request** flow (`addCclaAllowlistRequest`, `POST /company/{companyID}/ccla-whitelist-requests/{projectID}` — see Verified facts) rather than inventing a new pattern: resolve managers from the CCLA signature ACL, persist a request record, send a templated email, and log an audit event. Note that "same as the Approved-List flow" means **all four** of those steps — that flow is *not* fire-and-forget, and an email-only implementation would diverge from the named precedent. |
… when none checked Resolves the zero-selected case FR-008 previously left open. Decision: Send is disabled whenever no CLA manager is checked, rather than allowing a send that silently reaches nobody. Also requires the backend to reject an empty recipient list, so the rule does not depend on the disabled control being the only guard. Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 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:49
- Server-side resolution before redirect does not bind the later signing request to that user. The
userIDremains editable in the URL fragment, whilePOST /v4/request-individual-signatureis unauthenticated and trustsinput.user_id; a modified hand-off can therefore initiate signing for another EasyCLA record. Require an authenticated ownership check in the Console/backend flow or a short-lived signed hand-off token that binds the user and CLA group.
- **FR-003**: SS MUST resolve the EasyCLA `userID` server-side from the session identity; never from client input. SS MUST NOT write a GitHub identity onto an EasyCLA user record — `user.GithubID` becomes the signature ACL that gates merges, so binding stays owned by the Console's existing GitHub OAuth. For Gerrit, `GET /v4/user-from-token` alone is correct.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:55
- A mismatch cannot safely be log-only. The signing service builds the signature ACL from the routed EasyCLA record's
GithubID, not from the account selected in Self Serve, and the hand-off URL carries no selected-account identifier. Continuing can therefore bind the signature to a different or empty GitHub ID. Resolve the routed record by the selected numeric ID, or block until the mismatch is reconciled.
- **Mismatches are log-only in M2**: a GitHub ID bound to a different EasyCLA record MUST NOT trigger reconciliation (M3 item).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/plan.md:87
- Excluding the writers from M2 leaves the Revoked state unproducible. The current sanctions path persists only company-level
is_sanctioned/sanction_origin; it does not write per-signature timestamp/provenance, while M2 task #1370 explicitly requires both the sanctions and manager-removal paths to write those fields. Scope those writer changes into this plan, or change the status contract to use an existing durable source.
Secondary repo: `linuxfoundation/easycla` `cla-backend-go` — new CLA-manager resolution + notification endpoint (FR-008/FR-008a), swagger-first (`swagger/cla.v2.yaml` → `make swagger` → handler/service). No invalidation endpoint is built. Schema impact is **additive only** per FR-010b (revocation timestamp + reason/actor, written by the sanctions-screening path and corporate-console manager actions, not by M2) — no migrations, but the attributes are read by v1/v2 paths, so consumers must tolerate empty values on pre-M2 records. Also: the `GET /v4/my-clas` status + signed-under-identity extension, and the no-PR ICLA request shape in `v2/sign` (+ a matching `easycla-contributor-console` tweak).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:147
- The proactive return target is undefined. This plan explicitly omits
?redirect=, and the proactive flow has no PR-created active signature, so reading the Console's storedredirectcan yield no value or a stale value from an earlier flow. Define and validate a deterministic proactive return URL and ensure the Console stores it before initiating signing.
4. **Proactive ICLA active-signature gap** — widen two branch conditions with an *explicit* proactive request signal (not inferred from absent metadata, so PR-flow bugs can't silently degrade), sourcing `return_url` from the stored redirect. The two dependencies are the Console's `findActiveSignature()` hard-stop (`individual-dashboard.component.ts:49-66`) and the backend's PR-metadata requirement (`v2/sign/service.go:2890,2903`). No DocuSign, schema, or webhook change. The GitHub-ID binding must land *before* signing.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:70
- This zero-manager requirement has no executable path: the UI must omit Send and FR-008 requires the backend to reject an empty recipient list, yet this line requires a request record to be written. Define a separate attempt/receipt operation for the zero-manager case, or allow the request submission to persist and return a no-manager result without treating it as a notification send.
- **FR-008c**: CLA manager(s) MUST be resolved from the **CCLA signature ACL** for the company + CLA group (`SignatureACL`, hydrated to full users with emails — see Verified facts), the same source both existing request flows use. **When zero managers resolve** (empty or stale `signature_acl` — the existing Approved-List flow merely logs a warning and bails), the UI MUST NOT present a Send action that silently goes nowhere: the modal MUST show an explanatory state telling the contributor no CLA manager is currently reachable for this company and pointing them at LF support. The request record MUST still be written, so the dead end is diagnosable. This applies equally to Request Removal and Request approval (FR-011).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:78
- Pre-M2 invalidated signatures have no revocation metadata, so after relaxing the valid-only filter an
approved=falserow with an empty marker satisfies none of the three status formulas. This contradicts the requirement that every row has exactly one status. Define whether these legacy rows remain hidden, map to a dated/undated Revoked state, or receive a backfill.
- **FR-010b**: Revoked signatures MUST carry structured metadata: a **revocation timestamp** and a **revocation reason/actor** (e.g. `sanctions_screening`, `cla_manager`, system), additive on the signatures table. This is the durable marker that keeps a revoked ECLA excluded from `auto_create_ecla`/Approved-List re-validation — a bare `signature_approved` flip is not durable (see Verified facts). Written only by the system revocation path and by CLA-manager-initiated removals in the corporate console — **never** by a Self-Serve write, since none exists. Signatures revoked before M2, or invalidated under the pre-2026-08-14 self-service design, carry no such metadata, so consumers MUST tolerate empty values.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:69
- A request row alone cannot answer whether the request reached a manager. The named precedent persists before sending, swallows template/SNS errors, and
utils.SendEmailonly confirms SNS publication—not email delivery. Persist notification attempt/outcome (and a provider/message ID or failure) with retry semantics, or narrow this claim to “the request was recorded.”
- **FR-008b**: Removal and approval requests are **not** recorded as invalidation metadata on the signature (there is no self-service invalidation to attribute). Only the system-set Revoked state carries structured metadata — see FR-010b. Requests ARE, however, persisted as their own **request record** (who asked, which signature/CLA group, request type, timestamp, optional message) plus an audit event, matching the Approved-List precedent. This record is a **receipt, not a workflow**: M2 ships no approve/reject endpoints and no request-status UI on either side — the manager completes the removal in the CCLA console. The record exists so that "did this request ever reach a manager?" is answerable by support and by audit, which a fire-and-forget email cannot do for what is a legally significant action.
|
|
||
| **2026-08-14 legal/stakeholder review**: self-service invalidation is removed entirely, for both ICLA and ECLA. There are **no user-initiated invalidation writes** from the Me lens. ICLAs are informational-only here — no removal action of any kind. ECLA "removal" is a **request to the CLA manager**, who performs the actual removal in the corporate console; sanctions-based revocation is system-set (see Status below). Request Removal and Request approval share one modal component, differing only in mode. | ||
|
|
||
| - **FR-008**: Each valid ECLA row MUST offer **Request Removal**, opening a shared **Contact CLA Manager** modal (removal mode): the **resolved CLA manager(s)**, an optional message, and Send. Copy MUST make clear the manager completes the removal ("This starts the process to invalidate it on your behalf"). Sending MUST NOT invalidate, flip, or otherwise mutate any signature or Approved-List state in Self Serve or `cla-backend-go` — it only notifies. **Delivery is by email**, but the modal MUST **display the resolved manager(s) and let the contributor select recipients**: per mockup v17 Final each manager renders as a checkbox checked by default and can be unchecked (`mgrManagerList` / `toggleMgrManager`), and all three modal hints read "the CLA manager(s) **below**". Send MUST notify only the **selected** managers, and **Send MUST be disabled whenever no CLA manager is checked** — at least one recipient is required (decided 2026-08-17; the prototype does not cover this case). The backend MUST reject a request with an empty recipient list rather than relying on the disabled control alone. This requires a **new** `cla-backend-go` endpoint, which MUST follow the shape of the existing **Approved-List request** flow (`addCclaAllowlistRequest`, `POST /company/{companyID}/ccla-whitelist-requests/{projectID}` — see Verified facts) rather than inventing a new pattern: resolve managers from the CCLA signature ACL, persist a request record, send a templated email, and log an audit event. Note that "same as the Approved-List flow" means **all four** of those steps — that flow is *not* fire-and-forget, and an email-only implementation would diverge from the named precedent. |
…rified backend behavior Four pills (Valid / Needs attention / Invalidated / Revoked) mapped from signature_signed, signature_approved, company sanction status, and live approval-list coverage, with open decisions marked: the ECLA approved=false state (manager removal — exists, contrary to assumption), the missing Revoked date source, and the revoked token missing from lfx-self-serve#1423. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (9)
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:53
- This empty state is a dead end for first-time GitHub signers. M2 explicitly adds no EasyCLA identity-binding write, and the Console's OAuth can only bind the account after hand-off, but this requirement forbids that hand-off. Linking an account in SS does not populate
user.GithubID(FR-003), so the user still cannot satisfy the numeric-ID gate. Either permit a controlled Console OAuth hand-off or include an ownership-checked binding path.
- **When zero GitHub identities are linked**, the picker MUST show an empty state that blocks continuing ("No GitHub accounts are connected to your profile yet") with a link to Identities to link one; it MUST NOT fall through to the Console. This is a stricter gate than a Gerrit-backed group, which needs no account step at all.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:49
- Server-side resolution before redirect does not bind the later signing request to that identity. The fragment
userIDremains editable, while/v4/request-individual-signaturehassecurity: []and readsinput.user_id; a modified hand-off can therefore initiate signing for another record. The M2 contract must require end-to-end authenticated ownership enforcement (or an equivalent signed, one-time hand-off), not only trusted URL construction.
- **FR-003**: SS MUST resolve the EasyCLA `userID` server-side from the session identity; never from client input. SS MUST NOT write a GitHub identity onto an EasyCLA user record — `user.GithubID` becomes the signature ACL that gates merges, so binding stays owned by the Console's existing GitHub OAuth. For Gerrit, `GET /v4/user-from-token` alone is correct.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:67
- The backend must validate every selected recipient against a freshly resolved
SignatureACL, not merely reject an empty list. The named precedent accepts an arbitraryRecipientEmailoverride and sends to it (approval_list/service.go:315-332); copying that behavior would let an agreement owner use this endpoint to email arbitrary addresses. Define recipient IDs rather than client-supplied addresses and reject any ID outside the current manager set.
- **FR-008**: Each valid ECLA row MUST offer **Request Removal**, opening a shared **Contact CLA Manager** modal (removal mode): the **resolved CLA manager(s)**, an optional message, and Send. Copy MUST make clear the manager completes the removal ("This starts the process to invalidate it on your behalf"). Sending MUST NOT invalidate, flip, or otherwise mutate any signature or Approved-List state in Self Serve or `cla-backend-go` — it only notifies. **Delivery is by email**, but the modal MUST **display the resolved manager(s) and let the contributor select recipients**: per mockup v17 Final each manager renders as a checkbox checked by default and can be unchecked (`mgrManagerList` / `toggleMgrManager`), and all three modal hints read "the CLA manager(s) **below**". Send MUST notify only the **selected** managers, and **Send MUST be disabled whenever no CLA manager is checked** — at least one recipient is required (decided 2026-08-17; the prototype does not cover this case). The backend MUST reject a request with an empty recipient list rather than relying on the disabled control alone. This requires a **new** `cla-backend-go` endpoint, which MUST follow the shape of the existing **Approved-List request** flow (`addCclaAllowlistRequest`, `POST /company/{companyID}/ccla-whitelist-requests/{projectID}` — see Verified facts) rather than inventing a new pattern: resolve managers from the CCLA signature ACL, persist a request record, send a templated email, and log an audit event. Note that "same as the Approved-List flow" means **all four** of those steps — that flow is *not* fire-and-forget, and an email-only implementation would diverge from the named precedent.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:70
- No action described here can create the required diagnostic record: when zero managers resolve, the UI exposes no Send action, while FR-008b creates records only when a request is sent. Specify an explicit backend call/transition that persists the zero-manager attempt without sending email; otherwise this acceptance requirement is unreachable.
- **FR-008c**: CLA manager(s) MUST be resolved from the **CCLA signature ACL** for the company + CLA group (`SignatureACL`, hydrated to full users with emails — see Verified facts), the same source both existing request flows use. **When zero managers resolve** (empty or stale `signature_acl` — the existing Approved-List flow merely logs a warning and bails), the UI MUST NOT present a Send action that silently goes nowhere: the modal MUST show an explanatory state telling the contributor no CLA manager is currently reachable for this company and pointing them at LF support. The request record MUST still be written, so the dead end is diagnosable. This applies equally to Request Removal and Request approval (FR-011).
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:78
- This required marker has no writer in the M2 plan: the plan and research repeatedly say M2 only reads it and that it is “written elsewhere,” while the verified current code writes sanctions only on the company. Without adding the sanctions and corporate-console signature writers to the deliverable, rows cannot provide the required revocation timestamp/provenance or durable exclusion. Include those writer changes (and their tests) in M2 or relax the Revoked acceptance criteria.
- **FR-010b**: Revoked signatures MUST carry structured metadata: a **revocation timestamp** and a **revocation reason/actor** (e.g. `sanctions_screening`, `cla_manager`, system), additive on the signatures table. This is the durable marker that keeps a revoked ECLA excluded from `auto_create_ecla`/Approved-List re-validation — a bare `signature_approved` flip is not durable (see Verified facts). Written only by the system revocation path and by CLA-manager-initiated removals in the corporate console — **never** by a Self-Serve write, since none exists. Signatures revoked before M2, or invalidated under the pre-2026-08-14 self-service design, carry no such metadata, so consumers MUST tolerate empty values.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/status-matrix.md:57
- E7 also violates FR-010's “each row MUST show one of three statuses” contract by introducing a fourth wire state with no pill. The linked #1423 explicitly requires
unknown, so this matrix is aligned with that ticket but the authoritative parent spec is not. Addunknownand its degradation semantics to FR-010/FR-010a (or remove it from the matrix) before generating contracts.
| E7 | true | true | no | false — company record missing, no approved+signed CCLA, or evaluation/lookup failure | *(no pill)* — render **—** | `unknown` / `unknown` | Per #1423: degrade the row, not the whole request; no invented copy, no Request approval |
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/spec.md:52
- The picker selection is not connected to
userIDresolution./my-clas/identitiescan aggregate several EasyCLA records, but FR-003 only says to resolve from the session and FR-004 makes cross-record mismatches log-only; continuing can therefore hand off the first/LF-derived record while the user selected another GitHub ID, producing an ACL for the wrong account. Require the server to validate the selected numeric ID against the caller and resolve the EasyCLA record bound to that ID.
- **When one or more GitHub identities are linked**, SS MUST show an account-picker modal (per mockup v17 Final) so the contributor chooses which linked GitHub account to sign with before continuing to the Console — no silent auto-pick, even for a single linked account.
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/status-matrix.md:17
- This introduces a fourth user-facing status that the parent spec and PR scope omit: FR-010 and the acceptance scenarios say every row uses Valid / Needs attention / Revoked. Since #1732 confirms Invalidated ICLA is intended, the authoritative spec must add its derivation, date behavior, and actions instead of leaving implementers with incompatible three- and four-state contracts.
This issue also appears on line 57 of the same file.
| **Invalidated** | ICLA only | An admin invalidated the ICLA (`signature_approved=false`) | `Invalidated · <date>` from #1732; undated on pre-existing records | None (contributor remediation is an open legal question — see decisions) |
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/status-matrix.md:55
- The same manager-removal outcome has two incompatible mappings. This row maps approval-list removal to Needs attention, but FR-010b says CLA-manager removals write the Revoked marker; FR-010 also defines Needs attention only when
approved=true. Resolve whether manager removal is Revoked or Needs attention and update the marker writer and formulas consistently before this matrix is consumed.
| E5 | true | **false** | no | — | **Needs attention** ⚠️ | `needs_attention` / `not_on_approval_list` | Proposed, not yet confirmed — see decisions. Cause: manager removed the contributor from the approval list, which flips `approved=false` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds Milestone 2 as a self-contained Spec Kit feature folder —
specs/001-easycla-ss-integration-fable/m2-sign-cla-handoff/— mirroring the existingm1-my-cla/layout. This is the artifact Spec Kit (/speckit.plan→/speckit.tasks→/speckit.implement) consumes to plan and implement M2.Separate from the program-level design PR (#5132), which holds the six milestone briefs, overview, and 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
M2 extends M1's My CLAs page (Me lens) with four additive capabilities, per the M2 mockup v17 Final (source of truth):
GET /v4/my-clas; M2 exposes it instead of collapsing it into a single boolean.Guardrails: Self Serve runs no signing ceremony, makes no signing-initiation calls, and makes no invalidation writes of any kind — for either ICLA or ECLA. Removal and invalidation always happen in the corporate console, performed by a CLA manager, or are system-set by sanctions screening. The Console is not cut over or retired, and the PR-check remediation link is unchanged (no SSM flip).
Ticket-level scope lives in lfx-self-serve#1229 and its tasks (linuxfoundation/lfx-self-serve#1249, #1253, #1256, #1369, #1370, #1372, #1573, #1574, #1575).
Contents
spec.md— M2 feature spec: user story (P2), acceptance scenarios, FR-001…FR-011a, success criteria, scope boundaries, a "Verified Console/backend facts" section, and resolved design decisions.plan.md— implementation plan: summary, technical context, constitution check, project structure (extends M1'smy-clasmodule +claserver seam), complexity tracking against the ~3-week budget.research.md— Phase 0 spikes: no-PR ICLA signing, revocation-marker durability, CLA-group search, identity binding.Key verified findings baked into the spec
userID(it errors on a missing one) and writable browser storage.GET /v4/user-from-tokenresolves by LF username → email only, and records it creates carry no GitHub identity;userIDresolution must be platform-aware.user_github_idas a string but stores and queries it as a number via thegithub-id-indexGSI; SS'snormalizeGithubId()produces the same shape.GET /v4/my-clas/identitiesalready returns the deduplicated identity set, andauthorizeIdentityis already the ownership boundary.addCclaAllowlistRequest) resolves managers from the CCLA signature ACL (GetProjectCompanySignatures→SignatureACL, hydrated to users with emails), persists a request row with a status field, sends a templated email via SNS-backedutils.SendEmail(), and logs an audit event. M2 copies those four steps and descopes the approve/reject half — the request record is a receipt for audit and support, not a workflow.signature_acl; when it is empty or stale the existing flow logs a warning and bails. FR-008c requires the modal to say no CLA manager is reachable and point at LF support, while still writing the request record so the case is diagnosable.GET /v4/my-clasalready computes the per-ECLA coverage evaluation and then collapses it (row.Valid = sig.SignatureApproved && covered) — the reason the status work needs three independent fields, and why SS must relax an M1 filter that drops every non-valid row.GetCLAGroups's search parameters are dead code — it acceptsSearchField/SearchTerm, logs them, then builds a projection with no filter, making it an unfiltered full-tableScan. Fixing that is part of FR-001a.invalidateICLAis untouched by M2 — it remains a CLA-manager/admin-facing endpoint. Its existing gaps are not M2's to fix, since no self-service invalidation is built.cla-*-ccla-whitelist-requests. No migrations, but consumers must tolerate empty values on pre-M2 records.Deliberately not included
data-model.mdandcontracts/are not hand-written — the Spec Kit planning flow generates them. Genuinely open items (CLA-manager resolution/notification endpoint execution, the no-PR ICLA request shape, the listing-endpoint choice) are marked for/speckit.clarify.There is also no
.specify/scaffolding in either repo; these artifacts follow Spec Kit shape by hand, as M1's did. Initializing it is a separate change and does not block M2.Note on links
The upward links (
../spec.md,../02-milestone-sign-icla-fable.md) point at program-level docs in #5132 and resolve once that PR merges. Them1-my-cla/sibling folder is already ondev.🤖 Generated with Claude Code