diff --git a/docs/decisions/0026-exported-api-review.md b/docs/decisions/0026-exported-api-review.md new file mode 100644 index 0000000..6fb7334 --- /dev/null +++ b/docs/decisions/0026-exported-api-review.md @@ -0,0 +1,392 @@ +# 0026: Exported API review against Google Go style + +Status: E1–E6 accepted and E7 rejected on 2026-09-27; +implementation proceeds as one stacked PR per item. +Each numbered recommendation can be approved, redirected or rejected independently. + +## Scope and method + +The review covers every exported identifier in the five public packages: `edge`, `identity`, `authn`, `iamproof` and `authz`, as reported by `go doc -all` at the base of this branch. +Internal packages and `examples/` are evidence of consumer use, not review subjects. + +The basis is Google's Go style guide, read in full, and the primary sources it links: + +- [Style guide](https://google.github.io/styleguide/go/guide) (normative and canonical), [style decisions](https://google.github.io/styleguide/go/decisions) (normative) and [best practices](https://google.github.io/styleguide/go/best-practices) (auxiliary). + Where they differ, the guide takes precedence over decisions, and decisions over best practices. +- [Effective Go](https://go.dev/doc/effective_go), [Go Doc Comments](https://go.dev/doc/comment), [Go Code Review Comments](https://go.dev/wiki/CodeReviewComments), [Package names](https://go.dev/blog/package-names), [Organizing Go code](https://go.dev/blog/organizing-go-code), [Working with errors in Go 1.13](https://go.dev/blog/go1.13-errors), [Contexts and structs](https://go.dev/blog/context-and-structs), [Go concurrency patterns: Context](https://go.dev/blog/context) and [Testable examples](https://go.dev/blog/examples). + +Effective Go describes itself as a 2009 document that is not actively updated; +newer, more specific sources win where they disagree. + +The module has no release tag and an empty release manifest. +Renames and package moves therefore break no published consumer, but that stops being true after 0.1.0. +Each recommendation below was checked against the accepted decision records; +where one reopens an accepted decision, that is stated explicitly. + +## Summary + +| ID | Recommendation | Breaking | Priority | +| --- | --- | --- | --- | +| E1 | Move the action-header selector out of the root `edge` package | Yes | High | +| E2 | Give the action sentinels consistent names | Yes | High (with E1) | +| E3 | Rename `identity.WithCaller` to `identity.NewContext` | Yes | Medium | +| E4 | Type the `InvocationError` operation and limit names | Mostly source-compatible | Medium | +| E5 | Add `String` methods to the `identity` enumerations | No | Medium | +| E6 | Complete doc comments for sentinels, links, callback parameters and paired results | No | Medium | +| E7 | Move the Cognito verifier into its own package | Yes | Low; owner judgment | + +Items considered and not recommended are listed after E7, with reasons, so the review can be checked for omissions. + +## E1. Move the action-header selector out of the root `edge` package + +### Evidence + +`action.go` imports only `errors`, `net/http` and `strings`. +[Decision 0011](0011-request-header-processing.md) says the selector "needs no context, AWS event types, or JSON processing", yet placed it in the root package. +The root package is otherwise the Lambda transport adapter, and imports `github.com/aws/aws-lambda-go/events`. + +As a result, a native HTTP server that only needs the selector links the whole Lambda adapter. +`go list -deps ./examples/dispatcher/native` includes `github.com/aws/aws-lambda-go/events` and the root adapter package, although the native entry point never touches Lambda; +its only use of `edge` is `edge.NewActionHeader` in `examples/dispatcher/internal/orders/app.go`. + +### Recommendation + +Create a standard-library-only package for action selection and move `ActionHeader`, its constructor and its sentinels there unchanged in behavior. +Proposed shape: + +```go +package actionheader + +var ( + ErrMissing = errors.New("actionheader: missing action") + ErrAmbiguous = errors.New("actionheader: ambiguous action") + ErrInvalid = errors.New("actionheader: invalid action") +) + +type Selector struct{ /* unexported */ } + +func NewSelector(name string) (Selector, error) +func (s Selector) Parse(headers http.Header) (string, error) +``` + +### Reasoning + +- The style guide's [maintainability](https://google.github.io/styleguide/go/guide#maintainability) principle: maintainable code "minimizes its dependencies". + The only dependency the selector needs is `net/http`. +- [Package size](https://google.github.io/styleguide/go/best-practices#package-size) suggests merging packages when a user "must import both packages in order to use either in any meaningful way". + The inverse applies here: selector users need nothing else from the adapter, and adapter users do not need the selector. + A conceptually distinct idea gets "its own small package". +- [Package names](https://go.dev/blog/package-names) warns that a package whose name does not make a meaningful prefix for its contents has the wrong boundary. + `edge.ActionHeader` in a Lambda-adapter package is exactly that case. +- AGENTS.md asks that transport, identity, authentication and authorization stay separate. + Action selection is request dispatch, not Lambda transport. + +Naming: `action` would be the most natural package name, but it collides with the local variable `action` used throughout consumer code and examples. +[Package names](https://google.github.io/styleguide/go/decisions#package-names) says to avoid "package names that are likely to be shadowed by commonly used local variable names". +`actionheader` avoids that. +Inside it, the exported names drop the repeated context ([package vs. exported symbol name](https://google.github.io/styleguide/go/decisions#package-vs-exported-symbol-name)): `actionheader.ErrMissing` rather than `actionheader.ErrActionMissing`. +The constructor is `NewSelector` rather than `New`, because the returned type is not named after the package ([Package names](https://go.dev/blog/package-names#naming-package-contents): `time.NewTimer`, not `time.New`). + +### Alternatives + +- Keep the selector in `edge`. + This costs nothing now, but it permanently couples native HTTP consumers to the Lambda adapter's dependencies. + A later move would break published consumers. +- Put the selector in `authz`. + [Decision 0021](0021-authorization-rules.md) keeps `authz` free of HTTP concerns, so this would blur an accepted boundary. +- Put the selector in `authn`. + Selecting an action is not authentication. + +### Consequences + +This reopens part of decision 0011, which said: "Do not add an edge.Action type, global action context key, or action package at this stage." +That sentence was about action _metadata_ ownership; +the proposed package moves only the existing selector, adds no action type or context key, and keeps metadata consumer-owned. +The owner should still confirm that the move is compatible with 0011's intent. + +Examples, guides, the support matrix and the external consumer check need their imports updated. +Behavior, errors and tests move unchanged apart from names. + +## E2. Give the action sentinels consistent names + +### Evidence + +Across the module, sentinel names put the adjective first: `edge.ErrInvalidEvent`, `edge.ErrUnsupportedEvent`, `edge.ErrInvalidInvocation`, `authn.ErrMissingCredentials`, `authn.ErrInvalidCredentials` and `identity.ErrInvalidCaller`. +The three action sentinels put the noun first: `ErrActionMissing`, `ErrActionAmbiguous` and `ErrActionInvalid`. + +### Recommendation + +With E1: `actionheader.ErrMissing`, `ErrAmbiguous` and `ErrInvalid`, as shown above. +Without E1: rename them in `edge` to `ErrMissingAction`, `ErrAmbiguousAction` and `ErrInvalidAction`. + +### Reasoning + +[Consistency](https://google.github.io/styleguide/go/guide#consistency) notes it "can be very jarring … if the same concept has many names", and consistency within a package is the most immediate level. +[Maintainability](https://google.github.io/styleguide/go/guide#maintainability) asks for predictable names: a user who knows `ErrInvalidEvent` should be able to guess `ErrInvalidAction`. + +### Consequences + +This is a rename only; the error strings and meanings are unchanged. + +## E3. Rename `identity.WithCaller` to `identity.NewContext` + +### Evidence + +Every other exported `With…` function in the module is a functional option: +`identity.WithIAMAccountID`, `WithIAMPrincipalID`, `WithGatewayScopes`, `WithClaimsBudget`, `edge.WithGatewayIdentity`, `WithResponseHeaderBudget`, `WithIdentityClaimsBudget`, `WithStreamErrorReporter`, `iamproof.WithTimeout` and `WithTransport`. +`identity.WithCaller(ctx, caller) (context.Context, error)` is the one exception. +In `identity`'s godoc it sits beside four same-looking option constructors. + +### Recommendation + +Rename it `NewContext`, keeping its signature and semantics, so it pairs with the existing `FromContext`: + +```go +func NewContext(ctx context.Context, caller Caller) (context.Context, error) +func FromContext(ctx context.Context) Caller +``` + +### Reasoning + +The Go blog's context article defines this exact pair for a package that transports a value in a context: `NewContext` "returns a new Context that carries a provided" value, and `FromContext` extracts it ([Go concurrency patterns: Context](https://go.dev/blog/context)). +[Predictable names](https://google.github.io/styleguide/go/guide#maintainability): a reader who sees `FromContext` expects `NewContext`. +Within this module, the `With` prefix has come to signal "option"; one exception weakens that signal ([consistency](https://google.github.io/styleguide/go/guide#consistency)). + +### Alternatives + +Keep `WithCaller`. +The standard library's `context.WithValue` also uses `With` for a derived context, so the current name is defensible; +the recommendation rests on the module-local convention and the blog's canonical pair, not on a rule. + +### Consequences + +Update `authn`'s handler, examples, guides and decision 0012's cross-reference. +The error-returning signature stays, because conflict detection is a security invariant (decision 0012). + +## E4. Type the `InvocationError` operation and limit names + +### Evidence + +`(*InvocationError).Operation() string` returns one of eight words: validate, decode, request, identity, response, encode, cleanup or stream. +`Limit() (name string, maximum int64, ok bool)` returns one of five names: response_headers, buffered_body, buffered_envelope, stream_metadata or identity_claims. +Both sets exist only in prose; +internally they are string literals passed to `invocationError` and `limitError`. +A consumer that branches on them has to copy literals, and a typo compiles and silently never matches. + +### Recommendation + +```go +type Operation string + +const ( + OperationValidate Operation = "validate" + OperationDecode Operation = "decode" + // … one constant per documented operation +) + +type Resource string + +const ( + ResourceResponseHeaders Resource = "response_headers" + ResourceBufferedBody Resource = "buffered_body" + // … one constant per documented limit +) + +func (e *InvocationError) Operation() Operation +func (e *InvocationError) Limit() (resource Resource, maximum int64, ok bool) +``` + +### Reasoning + +[Error structure](https://google.github.io/styleguide/go/best-practices#error-structure): information callers need programmatically "should ideally be presented structurally", with `os.PathError` as the model. +The accessors already exist; +typed constants make the documented value sets part of the checked API instead of prose. +[Working with errors in Go 1.13](https://go.dev/blog/go1.13-errors#errors-and-package-apis) asks packages to document which error properties callers may rely on; +exported constants are the most direct form of that. +[Constant names](https://google.github.io/styleguide/go/decisions#constant-names): name by role (`ResourceBufferedBody`), not by value. + +### Alternatives + +- Export untyped string constants and keep the `string` results. + That is fully source-compatible but gives no type checking. +- Keep prose only. + +### Consequences + +Comparisons against untyped string literals (`e.Operation() == "decode"`) keep compiling. +Code that passes a result where a `string` is required needs a conversion. +The diagnostic text of `Error()` is unchanged. + +## E5. Add `String` methods to the `identity` enumerations + +### Evidence + +`Kind`, `Source`, `ClaimKind`, `Representation` and `IAMPrincipalType` have no `String` method. +`Caller.String` builds its text with `strconv.Itoa`, so a caller prints as `identity.Caller{kind:2,source:4}`. +Any `%v` of these values in logs or test failures prints a bare number. + +### Recommendation + +Add a `String` method to each type that returns a stable lowercase word (for example `iam`, `verified_iam_proof`, `gateway_text`), and `Kind(7)`-style text for values outside the declared set. +Make `Caller.String` use them. + +### Reasoning + +[Useful test failures](https://google.github.io/styleguide/go/decisions#useful-test-failures): "It should be possible to diagnose a test's failure without reading the test's source." +Consumers comparing `Kind` or `Source` in their own tests currently get numbers. +[Clarity](https://google.github.io/styleguide/go/guide#clarity): the reader should not need to reverse-engineer a value. +Effective Go's printing section uses a `String` method for exactly this purpose. +These values carry no identifiers or claims, so the change keeps `identity`'s rule that diagnostics omit facts. + +### Consequences + +`Caller.String` output changes. +The package already documents diagnostics as non-contractual, so this is not a compatibility break. +Add tests for every declared value and one undeclared value. + +## E6. Complete doc comments for sentinels, links, callback parameters and paired results + +### Evidence + +- `authn` (8), `iamproof` (5) and `authz` (5) sentinels share a single group sentence each, while `edge` and `identity` document every sentinel. + The meaning of each `authn` sentinel, including its [0019](0019-http-authentication-selector.md) HTTP mapping (400, 401, 431, 500 or 503), is currently written down only in the decision record and the guide. +- Go doc links (`[Name]`) appear 7 times in `edge`, 2 in `identity`, once in `authn` and never in `iamproof` or `authz`. + Names such as `ErrInvalidCredentials` and `Token.Value` are therefore not links on pkg.go.dev. +- The callback types `authn.VerifyFunc` and `authz.CheckFunc` have unnamed parameters, so godoc shows `func(context.Context, string)` without saying that the string is the credential. +- `identity.Claim.Bool` returns `(bool, bool)`. + +### Recommendation + +- Give each sentinel its own doc comment that states when it is returned and, for `authn`, the HTTP status its `*Error` maps to. +- Use doc links for cross-references to identifiers in the same and other module packages. +- Name the callback parameters, for example `func(ctx context.Context, credential string) (identity.Caller, error)`. +- Name `Claim.Bool`'s results `(value, ok bool)`. + +### Reasoning + +- [Doc comments](https://google.github.io/styleguide/go/decisions#doc-comments): "All top-level exported names must have doc comments." + [Go Doc Comments](https://go.dev/doc/comment#var) does accept a group comment for a set of sentinels, so the current form is valid upstream Go; + the recommendation follows Google's stricter rule and this module's own `edge`/`identity` practice. +- [Documentation conventions: errors](https://google.github.io/styleguide/go/best-practices#errors): "Document significant error sentinel values … so that callers can anticipate" the conditions they can handle. +- [Go Doc Comments: doc links](https://go.dev/doc/comment#doclinks) makes cross-references navigable, and [Preview](https://google.github.io/styleguide/go/best-practices#preview) recommends checking the rendered result. +- [Named result parameters](https://google.github.io/styleguide/go/decisions#named-result-parameters): "If a function returns two or more parameters of the same type, adding names can be useful." + The same reasoning applies to parameters of func types, which godoc renders without any other context. + +### Consequences + +These are documentation and naming changes only, with no behavior or compatibility change. +Parameter and result names are not part of a Go type's identity. + +## E7. Move the Cognito verifier into its own package + +### Evidence + +`authn` exports `CognitoConfig`, `CognitoClient`, `CognitoVerifier` and `NewCognitoVerifier`. +The repeated prefix marks a sub-domain: local RS256 and JWKS verification sits inside a package whose other exports are HTTP credential selection. +The module's other verifier, IAM proofs, is already its own package (`iamproof`). + +### Recommendation + +Move it to `authn/cognito`: + +```go +package cognito + +type Config struct{ /* unchanged fields */ } +type AppClient struct{ ClientID, Audience string; AllowUnbound bool } +type Verifier struct{ /* unexported */ } + +func NewVerifier(cfg Config) (*Verifier, error) +func (v *Verifier) Verify(ctx context.Context, token string) (identity.Caller, error) +func (v *Verifier) Warm(ctx context.Context) error +``` + +The new package imports `authn` only for `ErrInvalidCredentials`, `ErrUnavailable` and `ErrInvalidConfiguration`. +It keeps returning them, so `Verifier.Verify` still plugs into `authn.Config.Bearer` directly. +`authn` does not import `cognito`, so there is no cycle. + +### Reasoning + +- [Repetition](https://google.github.io/styleguide/go/decisions#repetition) and [avoid repetition](https://google.github.io/styleguide/go/best-practices#avoid-repetition): a shared prefix on several names is context that belongs in the package name (`cognito.Verifier`, not `authn.CognitoVerifier`). +- [Package size](https://google.github.io/styleguide/go/best-practices#package-size): "When something is conceptually distinct, giving it its own small package can make it easier to use." +- The two verifiers would have symmetric layouts. +- `AppClient` instead of `Client` avoids a reader confusing the policy type with an AWS SDK Cognito client. + [Package names](https://go.dev/blog/package-names#bad-package-names) advises against names that collide with packages commonly used alongside yours. + +### Alternatives + +Keep the current layout. +[Decision 0020](0020-cognito-verification.md) placed the verifier in `authn` to add it "without changing these boundaries", and did not compare it with a separate package. +The current layout means one fewer import for Bearer users and no churn. + +### Consequences + +This is the most discretionary item: a structural improvement, not a correctness issue. +It changes import paths and names for every Cognito consumer, and its benefit is mostly godoc organization and naming. + +## Considered, not recommended + +- **Keep `(*authn.Authenticator).Handler(next) (http.Handler, error)`.** + Returning a plain `http.Handler` and panicking on a nil `next` would give the conventional `func(http.Handler) http.Handler` middleware shape, and [when to panic](https://google.github.io/styleguide/go/best-practices#when-to-panic) notes that the standard library panics on API misuse. + But every constructor in this module reports invalid input as an error (`edge.New` rejects nil handlers the same way), and [decision 0019](0019-http-authentication-selector.md) approved this signature. + Consistency wins; a consumer can adapt it in two lines. +- **Keep the explicit `iamproof.ErrInvalidProof` to `authn.ErrInvalidCredentials` adapter.** + Passing `proofVerifier.Verify` directly compiles and turns invalid proofs into 503 responses. + That is the module's clearest remaining misuse trap, and [simplicity](https://google.github.io/styleguide/go/guide#simplicity) accepts extra code "so that the end user of the API may more easily call the API correctly". + However, decision 0019 A3 weighed and rejected the alternatives: making `iamproof` import `authn` "reverses its transport-independent boundary", and a classification protocol adds machinery. + The review found no new evidence against that trade-off, so it is listed only for completeness. +- **Keep a single `edge.Option` type, even though `New` rejects `WithStreamErrorReporter` at run time.** + A separate `StreamingOption` type would catch the misuse at compile time, but it adds a type and duplicated option constructors for a single streaming-only option ([least mechanism](https://google.github.io/styleguide/go/guide#least-mechanism)). + Revisit this if more streaming-only options appear. +- **Keep the per-package configuration styles.** + `authn.Config` and `CognitoConfig` are option structs; + `edge`, `identity` and `iamproof` use variadic options. + This matches the selection criteria in [option structure](https://google.github.io/styleguide/go/best-practices#option-structure) and [variadic options](https://google.github.io/styleguide/go/best-practices#variadic-options). + Structs fit when every caller supplies several required values (verifiers, issuer, clients). + Options fit when most callers use defaults. + Options already take values rather than using presence as a signal (`WithGatewayIdentity(enabled bool)`), as that section asks. +- **No `authz.Must` helper.** + Composing rules needs one error check per constructor, but rule inputs normally come from runtime configuration. + [Must functions](https://google.github.io/styleguide/go/decisions#must-functions) are for program start-up with known-good inputs, "not on things like user input". +- **Keep `iamproof.NewGenerator` and `NewVerifier` taking `(region, audience string, …)` positionally.** + [Function argument lists](https://google.github.io/styleguide/go/best-practices#function-argument-lists) warns that adjacent same-typed parameters are easy to swap. + Here the risk is contained: the region must match a region pattern and resolve to a standard STS endpoint, so swapped arguments fail construction unless the audience is itself region-shaped. + +## Already conformant + +- Package names are short, lowercase single words without stutter. + The `go-lambda-edge` path with package `edge` follows the common `go-` repository convention; + documentation already shows the import. +- Constructors follow `New` for a package's primary type (`edge.New`, `authn.New`) and `NewT` otherwise. + There are no `Get` prefixes, and initialisms are consistent (`AccountID`, `PrincipalARN`, `ClientID`, `CacheTTL`). +- Functions return concrete types and accept small function types instead of exported interfaces ([interfaces](https://google.github.io/styleguide/go/decisions#interfaces)). + Error results are typed `error`, with documented `*InvocationError`, `*authn.Error` and `*identity.ClaimsLimitError` ([returning errors](https://google.github.io/styleguide/go/decisions#returning-errors)). +- `context.Context` is always the first parameter and is never stored in an exported struct ([contexts](https://google.github.io/styleguide/go/decisions#contexts)). +- There is no mutable package-level state ([global state](https://google.github.io/styleguide/go/best-practices#global-state)). +- Concurrency, cleanup, zero values and context behavior are documented wherever they are non-obvious ([documentation conventions](https://google.github.io/styleguide/go/best-practices#conventions)). + Comma-ok results replace in-band sentinels ([in-band errors](https://google.github.io/styleguide/go/decisions#in-band-errors)). +- Every public package has runnable examples. + +## Related observation: comment line length + +The previous change moved comments to one sentence per line. +[Go Doc Comments](https://go.dev/doc/comment) explicitly accepts semantic linefeeds. +Google's [comment line length](https://google.github.io/styleguide/go/decisions#comment-line-length) decision prefers wrapping long comment lines and calls a whole paragraph on one line a poor reading experience, while setting no column width. +One sentence per line avoids that failure mode, but 46 of the 1,020 `//` comment lines exceed 120 characters (one exceeds 200). +An optional follow-up could add clause-level breaks (SemBr rule 2) to those sentences, without returning to column wrapping. + +## Open questions + +1. Does E1 conflict with the intent of decision 0011's "no action package" sentence, or only with its wording? +2. For E1, is `actionheader` acceptable, or does the owner prefer another name that avoids shadowing `action`? +3. Should approved items land as one API-change PR before 0.1.0, or as one stacked PR per item? + +## Resolution + +On 2026-09-27 the owner approved E1–E6, rejected E7 and asked for one stacked PR per item. +The proposed names stand: `actionheader` with `Selector`, `NewSelector`, `ErrMissing`, `ErrAmbiguous` and `ErrInvalid`, and `identity.NewContext`. +The Cognito verifier stays in `authn`. + +E1 moves the selector and renames its type and constructor; +E2 then renames the moved sentinels, so each PR is a single kind of change. +This is compatible with decision 0011's intent: the new package holds only the existing selector, and action metadata remains consumer-owned. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 03218c3..b68c58a 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -33,3 +33,4 @@ Keep related decisions small enough to review and implement independently. | [0023: Consumer readiness](0023-consumer-readiness.md) | R1–R5 accepted; local implementation complete; external release prerequisites open | What complete consumer example, test support, deferred scope and release preparation should follow the implemented packages? | | [0024: Release automation](0024-release-automation.md) | Accepted; PR-only validation and organization credentials; App installation verified, release execution pending | How should release PRs, validation, credentials and initial versioning work? | | [0025: Main-branch protection](0025-main-branch-protection.md) | Proposed; awaiting owner review | Should the existing four CI checks and PR workflow be enforced before merging to main? | +| [0026: Exported API review](0026-exported-api-review.md) | E1–E6 accepted, E7 rejected; implementation in progress | Which exported API changes does Google's Go style guidance support before the first release? | diff --git a/docs/plan.md b/docs/plan.md index 23d56ec..aab0596 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -63,6 +63,7 @@ Decision 0021 accepts authorization rules and the consumer-owned resolution/disp | Package | Responsibility | | --- | --- | | `edge` (root) | Invocation adapter, payload decoding, HTTP translation and optional gateway identity; metadata accessors deferred | +| `actionheader` | Strict single-value action selection from a configured request header; standard library only (decision 0026 E1) | | `identity` | Validated callers, claim representation, context transport; standard library only | | `authn` | Explicit IAM-proof/OAuth selection, Cognito verification and JWKS caching | | `iamproof` | Exported client proof generator and bounded online STS verifier | @@ -368,6 +369,22 @@ The workflow now reads both values from secrets. Token creation and Release Please execution still need a successful run after merge. No release was published, and the license and deployment qualifications remain open. +### 7. Pre-release exported API alignment + +[Decision 0026](decisions/0026-exported-api-review.md) reviewed every exported identifier against Google's Go style guidance. +The owner approved E1–E6 and rejected E7 on 2026-09-27, with one stacked PR per item. +Behavior, error semantics and trust boundaries do not change; +these are placement, naming, typing and documentation changes made before the first release. + +- [ ] E1: move the action-header selector to the standard-library-only `actionheader` package as `Selector`/`NewSelector`. +- [ ] E2: rename the moved sentinels to `ErrMissing`, `ErrAmbiguous` and `ErrInvalid`. +- [ ] E3: rename `identity.WithCaller` to `identity.NewContext`. +- [ ] E4: type the `InvocationError` operation and limit names with exported constants. +- [ ] E5: add `String` methods to the `identity` enumerations. +- [ ] E6: complete sentinel docs, doc links, callback parameter names and `Claim.Bool` result names. + +Acceptance: each PR passes the full AGENTS.md validation set, and examples, guides and the external consumer check use the new names. + ## Findings carried forward from the Beakley review These are requirements to test independently, not instructions to preserve old implementation structure.