Part of duplicate code analysis: #12573
Summary
internal/delegation repeats the same 9-field mapping from an Identity/request struct to a CreateOrConfirmRequest-shaped value in three separate places: once when building an Identity from a request in store.go, once when re-validating a restored Identity in recovery.go, and again implicitly via bindingEquals in identity.go. Every time a new field is added to the delegation binding tuple (e.g., RunID, EnclaveBackend, EnclaveEntryID, InvocationID, Repository, ToolPolicy, SchemaHash, AdmittedDefaultBranchSHA, InvocationExpiresAt), all three call sites must be updated in lockstep, or the envelope-subset validation, idempotency-binding check, and restart-recovery validation can silently diverge.
Duplication Details
Pattern: Repeated enumeration of the delegation "binding tuple" fields across construction, comparison, and revalidation
- Severity: Medium
- Occurrences: 3
- Locations:
internal/delegation/store.go (~lines 180-194, identity construction inside createOrConfirmAt)
internal/delegation/identity.go (lines 111-120, bindingEquals)
internal/delegation/recovery.go (~lines 157-173, validateRestoredIdentity reconstructing a CreateOrConfirmRequest from a restored Identity)
- Code Sample:
// store.go — building the Identity from req
identity := &Identity{
ExecutorBearer: bearer,
RunID: req.RunID,
EnclaveBackend: req.EnclaveBackend,
EnclaveEntryID: req.EnclaveEntryID,
InvocationID: req.InvocationID,
Repository: req.Repository,
ToolPolicy: req.ToolPolicy,
SchemaHash: req.SchemaHash,
AdmittedDefaultBranchSHA: req.AdmittedDefaultBranchSHA,
...
}
// identity.go — bindingEquals compares the same field set
func (id *Identity) bindingEquals(req CreateOrConfirmRequest) bool {
return id.RunID == req.RunID &&
id.EnclaveBackend == req.EnclaveBackend &&
id.EnclaveEntryID == req.EnclaveEntryID &&
id.InvocationID == req.InvocationID &&
id.Repository == req.Repository &&
id.ToolPolicy == req.ToolPolicy &&
id.SchemaHash == req.SchemaHash &&
id.AdmittedDefaultBranchSHA == req.AdmittedDefaultBranchSHA &&
id.InvocationExpiresAt.Equal(req.InvocationExpiresAt)
}
// recovery.go — validateRestoredIdentity rebuilds a CreateOrConfirmRequest from the same fields
return (&Store{envelope: envelope}).validateAgainstEnvelope(CreateOrConfirmRequest{
RunID: identity.RunID,
EnclaveBackend: identity.EnclaveBackend,
EnclaveEntryID: identity.EnclaveEntryID,
InvocationID: identity.InvocationID,
Repository: identity.Repository,
ToolPolicy: identity.ToolPolicy,
SchemaHash: identity.SchemaHash,
AdmittedDefaultBranchSHA: identity.AdmittedDefaultBranchSHA,
RequestedTTL: identity.ExpiresAt.Sub(identity.CreatedAt),
InvocationExpiresAt: identity.InvocationExpiresAt,
IdempotencyKey: identity.IdempotencyKey,
}, identity.CreatedAt)
Impact Analysis
- Maintainability: High risk over time — this is the core security-relevant "binding tuple" for the delegation ADR (identity ⇄ request equivalence). Any future field added to the envelope/request/identity model (a likely event as the ADR evolves) must be added correctly in all three spots or the system can silently accept a restart-restored identity whose binding no longer matches what a fresh request would produce, or fail to detect an idempotency-key binding mismatch.
- Bug Risk: High relative to severity — because the three copies are structurally similar but not textually identical (different struct literal, comparison expression, and reconstruction direction), an automated diff/grep won't catch a missed field the way it would for an exact-duplicate function body.
- Code Bloat: Moderate — ~30 combined lines encode the same 9-field tuple three different ways.
Refactoring Recommendations
-
Introduce a single "binding tuple" type or a request/identity conversion method
- Extract common functionality to:
internal/delegation/identity.go — add func requestFromIdentity(id *Identity) CreateOrConfirmRequest (used by recovery.go instead of hand-rolling the struct literal) and consider deriving bindingEquals from requestFromIdentity(id) == req (if all fields are comparable) or from a small ordered slice of field-getter pairs to make the equivalence check and the two directions of conversion share one field list.
- Estimated effort: 2-3 hours including test updates
- Benefits: A single field list to update when the delegation binding tuple changes; recovery validation and live-request validation are guaranteed to use the same field mapping; reduces the chance that restart recovery accepts identities that a live request would reject.
-
Add a table-driven test that fails if a new field is added to Identity/CreateOrConfirmRequest without updating all three sites
- Use reflection-based struct-field-count assertions (or golden struct literals) in
identity_test.go to catch drift early, as a stopgap if full consolidation is deferred.
Implementation Checklist
Parent Issue
See parent analysis report: #12573
Related to #12573
Generated by Duplicate Code Detector · copilot · auto · 64.2 AIC · ⊞ 13.5K · ◷
Part of duplicate code analysis: #12573
Summary
internal/delegationrepeats the same 9-field mapping from anIdentity/request struct to aCreateOrConfirmRequest-shaped value in three separate places: once when building anIdentityfrom a request instore.go, once when re-validating a restoredIdentityinrecovery.go, and again implicitly viabindingEqualsinidentity.go. Every time a new field is added to the delegation binding tuple (e.g.,RunID,EnclaveBackend,EnclaveEntryID,InvocationID,Repository,ToolPolicy,SchemaHash,AdmittedDefaultBranchSHA,InvocationExpiresAt), all three call sites must be updated in lockstep, or the envelope-subset validation, idempotency-binding check, and restart-recovery validation can silently diverge.Duplication Details
Pattern: Repeated enumeration of the delegation "binding tuple" fields across construction, comparison, and revalidation
internal/delegation/store.go(~lines 180-194, identity construction insidecreateOrConfirmAt)internal/delegation/identity.go(lines 111-120,bindingEquals)internal/delegation/recovery.go(~lines 157-173,validateRestoredIdentityreconstructing aCreateOrConfirmRequestfrom a restoredIdentity)Impact Analysis
Refactoring Recommendations
Introduce a single "binding tuple" type or a request/identity conversion method
internal/delegation/identity.go— addfunc requestFromIdentity(id *Identity) CreateOrConfirmRequest(used byrecovery.goinstead of hand-rolling the struct literal) and consider derivingbindingEqualsfromrequestFromIdentity(id) == req(if all fields are comparable) or from a small ordered slice of field-getter pairs to make the equivalence check and the two directions of conversion share one field list.Add a table-driven test that fails if a new field is added to
Identity/CreateOrConfirmRequestwithout updating all three sitesidentity_test.goto catch drift early, as a stopgap if full consolidation is deferred.Implementation Checklist
requestFromIdentityhelper (or equivalent) shared byrecovery.gobindingEqualsto reuse the same field-list source of truth where feasibleinternal/delegation(store_test.go, recovery_test.go, identity_test.go if present)Parent Issue
See parent analysis report: #12573
Related to #12573