feat(internet-identity): target @icp-sdk/auth v9 - #384
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Skill Validation ReportValidating skill: /home/runner/work/icskills/icskills/skills/internet-identityStructure
Frontmatter
Tokens
Markdown
Tokens
Content Analysis
Contamination Analysis
Result: 2 warnings Project Checks |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rapper Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
marc0olo
left a comment
There was a problem hiding this comment.
Review: internet-identity skill for @icp-sdk/auth v9
Must fix
- The new React example breaks under StrictMode, which is the default development configuration an agent will scaffold into.
const [authClient] = useState(() => new AuthClient())holds one instance for the component's life, while the effect's cleanup callsdispose(). StrictMode mounts, unmounts and remounts, so the cleanup runs once on the way through and leaves the client permanently disposed:#disposedis set indispose()(src/client/auth-client.ts:669) and never cleared, the state subscription is torn down in the same method, and the nextsignIn()throwsSupersededError("This client was disposed while signing in")from#assertCurrent(:783).subscribe()stops firing too, so the screen never updates again. Because the client lives inuseState, the remount cannot replace it. Two shapes work: create the client inside the same effect that disposes it (a remount then gets a fresh one), or hold it at module scope and dispose only where a view is genuinely torn down rather than at the app root. Worth a comment either way, since the reason is not obvious from the API. - The three changed eval cases were not run.
improve-ic-skillStep 7A treats running every added or changed case with baseline as not optional, and asks for the results in the PR; the description reportsnpm run build,check-project.jsandskill-validator checkonly. Note that CONTRIBUTING contradicts itself here (the "Updating an Existing Skill" path calls eval results recommended, while section 8 calls them required), so this may be the looser rule being followed; worth settling separately. It matters concretely for case 1 ("Local II URL"), which looks mis-scoped in exactly the way Step 7A warns about: the prompt asks for "just the value and a one-line explanation", while the expectations now require explaining thatcanisterIdis the same locally as on mainnet. A run would show whether that case passes with the skill, rather than landing a regression test that is already broken.
Suggestions
- Add eval coverage for the two changes most likely to regress. Step 6 asks for a case per new pitfall, and the current diff adds behaviors for the provider pair but nothing for the two rewrites where a v8-trained model will confidently produce the old answer: the session bounds (an agent that learned
maxTimeToLive: BigInt(8) * BigInt(3_600_000_000_000)will keep emitting it, and mistake 3 exists to stop that) anddispose()(new lifecycle requirement, and the React example above is where agents will get it wrong). - "Older API notes", 8.x bullet on
maxTimeToLive: it reads "bounded a delegation and defaulted to 8 hours", where the upstream v9 guide says v8 "quietly capped every sign-in at eight hours". A cap and a default behave differently for anyone who passed a longer value on v8, and this is the sentence an agent will quote when asked what changed. - Vanilla example is now inconsistent with the React one: it calls
authClient.subscribe(...)without keeping the returned unsubscribe and never disposes the client, while the React block does both and explains why. One line ininit()would keep the two teaching the same lifecycle.
Verified
- Every v9 API claim against
dfinity/icp-js-authat tag9.0.0: the no-argument constructor the examples rely on (options: AuthClientCreateOptions = {},:385), a string orURLthrowingTypeError(:395),authorizeUrlandcanisterIdrequired together (:409), mainnet defaultshttps://id.ai/authorizeandrdmx6-jaaaa-aaaaa-aaadq-cai(:42-43),getIdentity()throwingSessionNotHeldErrorrather than returning an anonymous identity (:545-557), the fourSessionStatusstates and which carry a principal (:296-304),subscribe()anddispose()(:650,:665),nonce: () => Promise<Uint8Array>(:1055), and both session bounds optional (:248-259). - The seven-day and thirty-day figures against Internet Identity itself:
DEFAULT_SESSION_IDLE_NSis7 * DAY_NSandMIN_SESSION_IDLE_NSis10 * MINUTE_NS(src/internet_identity/src/storage/account.rs:62,70),DEFAULT_SESSION_TTL_NSandMAX_SESSION_TTL_NSare both30 * DAY_NS(src/internet_identity/src/sessions.rs:30). - The
useSyncExternalStorecontract holds:subscribe()returns an unsubscribe, andgetStatus()returns the same object until something changes, which is what the store's snapshot identity requires. - Prerequisites:
@icp-sdk/auth@9.0.0still declares@icp-sdk/core: ^5as its peer range, so>= 5.3.0remains correct and needs no bump alongside the major. - Evals: nothing stale was left behind. No case still asserts the eight-hour delegation or a bare
identityProviderstring, and case 11 ("local vs mainnet II usage") still reads correctly under v9. - Frontmatter and the description are untouched, so trigger evals need no re-run.
- CI
checks / validatepasses.
Replaces the React walkthrough with the two shapes that matter, one client for the page or one per component, and the call each needs. Adds eval cases for the session bounds and for disposing a client a component owns, and splits a compound expectation that could never score honestly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — both must-fixes and both suggestions are in, as of 0968d74. I had not loaded StrictMode. Confirmed exactly as you describe: Digging into your second shape turned up a harder constraint: Rather than pick a React shape, the section is now framework-free and teaches the two lifetimes and the call each one needs:
The React block, Evals. Ran with baseline, and case 2 turned out not to be mis-scoped:
Case 5 first scored 3/4 on a behavior I had not touched, because it asserted two things at once ("signIn() did not reject and the identity is passed to HttpAgent"); split into two behaviors it is 5/5 with the skill and 3/5 without. The two new cases are the ones you asked for. Their baselines are worth reading: without the skill, case 20 produces 8.x bullet. Fixed to "capped": v8 clamped, it did not merely default. On CONTRIBUTING contradicting itself about whether eval results are required or recommended: agreed, worth settling separately, and I have no stake in which way it lands. |
marc0olo
left a comment
There was a problem hiding this comment.
Follow-up review: internet-identity skill for @icp-sdk/auth v9
Fixed
- The StrictMode trap is gone, by removing the React block rather than patching it. Teaching the two client lifetimes and the call each one needs is the more durable shape for a skill that gets loaded into any framework.
new AuthClient()outside a browser: confirmed independently,Error: Could not find local storage.on Node with auth 9.0.0 and core 5.4.0, so the new "the client is browser-only" sentence is both correct and load-bearing. That constraint is a better reason for the framework-free section than either shape I proposed.- Evals run with baseline, results posted. Case 2 was not mis-scoped after all: 4/4 with the skill against a 1/4 baseline says so, and my concern there was wrong. Splitting case 5's compound behavior into two is a real improvement on its own. The case 20 baseline (
AuthClient.login(), "valid for 8 hours",maxTimeToLivepresented as the delegation's lifetime) is exactly the v8 answer mistake 3 exists to prevent, which is the best argument for that case existing. - Two new eval cases cover the session bounds and
dispose(), which were the two gaps. - 8.x bullet now says v8 capped every sign-in at eight hours rather than defaulting to them.
Still needs work
Non-blocking, and about the shape of the new snippet rather than what it teaches. prompt and hint are genuinely constructor options (auth-client.ts:180, :190), and both lifetimes are described correctly. Read as straight-line code, though, the example subscribes and unsubscribes on the next line, then constructs a client and disposes it on the next line, which is a shape an agent can reproduce literally. principal in hint: principal is undeclared, and prompt: 'none' appears here for the first time with no word on what a silent re-issue is or when an app wants one, in a skill with no shared-sessions section to point at. A // on teardown: marker, or one named function per lifetime, would keep the lesson and lose the ambiguity.
The arrangement stakeholders are asking for: one derivation origin, one cookie domain, and every page picking up a sibling's sign-in through a /reauth route. Adds the two pitfalls that make it silently wrong, and three eval cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Read straight through, the previous snippet subscribed and unsubscribed on adjacent lines, and constructed a client and disposed it on adjacent lines. Each lifetime is now a function that hands back its own teardown, principal is a parameter rather than undeclared, and prompt/hint points at the section that explains a silent re-issue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in d05aeaf. All three points were fair, and the third got cheaper to fix in the meantime: the skill now has a shared-sessions section, so Each lifetime is a function that hands back its own teardown, so nothing reads as "subscribe, then immediately unsubscribe": // Page-lifetime: one client for the app, nothing to dispose. Views come and go,
// so each hands back the teardown for its own listener.
const authClient = new AuthClient();
function watchHeader() {
const unsubscribe = authClient.subscribe(() => render(authClient.getStatus()));
return unsubscribe; // when the header goes; the client carries on
}
// Component-lifetime: the client belongs to the view, so it goes with the view.
function openReauthDialog(principal) {
const client = new AuthClient({ prompt: "none", hint: principal });
return () => client.dispose(); // covers its subscription, and is not a sign-out
}
Case 21 (disposing a client a component owns) still scores 3/3 with the skill against 1/3 baseline after the rewrite. |
A sibling can sign in while a page is already up. Redirecting then would throw away whatever the user was doing, so the status change is offered behind a button instead. Adds an eval for the after-load path and one for getting it backwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
marc0olo
left a comment
There was a problem hiding this comment.
Follow-up review: internet-identity skill for @icp-sdk/auth v9
Re-reviewing at 7a11224.
Fixed
- The lifecycle snippet. Each lifetime is now a named function that hands back its own teardown, so nothing reads as subscribe-then-unsubscribe or construct-then-dispose,
principalis a parameter rather than a free variable, andprompt: "none"now points at the shared-sessions section instead of appearing unexplained. That third point did get cheaper once the section existed, as you say.
Still needs work
The new shared-subdomain section carries the same two issues as dfinity/developer-docs#395, which is where I found them.
transport: "redirect"has a prerequisite the section does not mention. Internet Identity delivers a redirect response only to a callback declared by the callback's own origin at/.well-known/ii-auth-callbacks, and this is enforced by the URL transport rather than only the MCP flow:src/frontend/src/lib/utils/transport/url.ts:55importsmatchDeclaredCallback, andauthCallbacks.tssays the name "is deliberately not MCP-specific". Validation fails closed, so a generated app that follows this section deploys and then never returns from/reauth. The requirements: a byte-exact entry forhttps://<origin>/reauth, no fragment,application/jsonwithAccess-Control-Allow-Origin(II reads it cross-origin), under the size cap, no redirects, and one file per app origin rather than one on the derivation origin. This skill already writes the_headersblock forii-alternative-originsandii-app-metadata, so the same treatment fits. The upstream caveat is worth a line too: a declared callback must terminate locally, since the response arrives in the URL fragment and a3xxcarrying none re-attaches it.- Mistake 14 overstates what happens without
hint. "It may answer for a different account and overwrite the shared record with it" is not the behavior. Where Internet Identity holds more than one session and the request names none it refuses, withInteractionRequiredError.reasonofaccount_selection_required. Where a mint answers for an unexpected account the client raisesAccountMismatchError, documented as "neither gone nor retryable" with "the caller's own record is what has to give way", so the record gives way rather than taking on the wrong account. Passinghintis still the right advice; what omitting it costs is the resume, not the user's identity. Worth getting exact here, since a pitfall entry is the thing an agent repeats back. - The four new eval cases have no run results. Same Step 7A point as the first round, now for the cases added in
5538d64and7a11224(shared sign-in, plus the three adversarial ones). The adversarial ones are the interesting read here: a baseline that has never seen this section is unlikely to produceCookieStateStorage, ahint, ortransport: 'redirect'at all, so the deltas should be large, and the "silent re-issue without hint" case is worth watching given the point above. - If
ii-auth-callbackslands, the "Shared sign-in across sibling subdomains" case wants an expectation for it. As written it asks what the derivation origin has to serve, and the callback allow-list is served by each app origin instead, so the case can pass while the generated app cannot complete a sign-in.
…int costs A redirect sign-in is delivered only to a callback the returning origin declares at /.well-known/ii-auth-callbacks, one file per app origin, so a /reauth route without it never comes back. Omitting hint costs the resume rather than the user's identity: the provider refuses with account_selection_required, and a mint for an unexpected account is rejected as AccountMismatchError. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both confirmed in source, and the callback one was a real hole: a generated app would have deployed and never returned from
Also fixed on the docs side, all four: the hardcoded Eval results, which I owed you from the previous round as well:
Case 22 now asserts the callback document too, and your last point was exactly right: the case had been asking what the derivation origin serves, so it passed while the generated app could not have completed a sign-in. It now asks which origin serves each well-known document, and that expectation failed on the first run after I added it — the model produced Case 24 gained a behavior asserting the failure is a refused resume rather than a wrong identity, so the corrected wording is now tested rather than just written. |
marc0olo
left a comment
There was a problem hiding this comment.
Follow-up review: internet-identity skill for @icp-sdk/auth v9
Re-reviewing at bd956ff.
Fixed
ii-auth-callbacksis documented per app origin, with the entry shape, the exact-match and no-fragment rules, the_headersblock alongside the two the skill already had, the fails-closed behavior, and the terminate-locally caveat. Mistake 14 names it too, so it is reachable from the pitfall list and not only from the walkthrough.- The
hintwording now says a provider holding more than one session refuses withaccount_selection_required, and that a mint for an unexpected account is rejected asAccountMismatchError, so what omitting it costs is the resume rather than the user's identity. That matches the source. - Eval results posted for all four new cases, with baselines. Case 22 at 9/9 against 1/9 is the useful number, and correcting the prompt after the callback expectation failed on its first run, rather than accepting the score, is the right way round. Case 24 gaining a behavior that asserts the refusal means the corrected wording is now tested rather than only written.
Still needs work
One item, and it is the skill contradicting itself rather than anything about v9.
- The
/reauthsnippet uses bare top-levelawait, twice:await authClient.signIn({ ... })andawait authClient.signOut()sit at module scope inside anif. Line 148 of this same file says the init pattern "wraps async setup in a function so this code works with any bundler target (Vite defaults to es2020 which lacks top-level await)", eval case 1 asserts "All await calls are inside async functions, no bare top-level await at module scope", and another case exists purely to teach the fix forTop-level await is not available in the configured target environment. So an agent that follows this section produces a frontend that does not build on Vite's default target, and the skill's own pitfall is what would have prevented it. Wrapping the body inasync function reauth() { ... }with a call after it settles it, and matches how the sign-in flow above is written.
marc0olo
left a comment
There was a problem hiding this comment.
@sea-snake pre-approving to unblock when the fix lands
The route's awaits sat at module scope, which the skill's own init pattern and eval case 1 both say not to do: Vite's default target has no top-level await, so a frontend following this section would not build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in 0823cb9. You are right that this was the skill contradicting itself rather than anything about v9: line 148 tells an agent to wrap async setup because Vite's default target has no top-level await, eval case 1 asserts it, and then the section I added put two awaits at module scope. The route is now The same snippet had landed in dfinity/developer-docs#395 with the same problem, and you had already approved that one, so I fixed it there too (769672e) rather than leave a copy of it in the docs. Evals after the change: case 22 still 9/9 against 1/9, and case 1, the one that asserts no bare top-level await, 5/5 against 1/5. |
Out of scope for this PR, which is the internet-identity skill. The template's own owners decide when it moves off ^7.1.0 and whether it goes to v9 or v10. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@icp-sdk/authv9 shipped. The skill teaches 7.x, and two of the patterns it hands an agent no longer run at all. It also gains the arrangement stakeholders are asking for: one sign-in shared across sibling subdomains.skills/internet-identity/SKILL.mdPrerequisites now say
>= 9.0.0.Mistakes that break your build
/authorizein theidentityProviderURL". In 9.x the option is not a URL: it is{ authorizeUrl, canisterId }, both required together, and a string throws aTypeError. The/authorizeadvice is kept, since the URL is still used verbatim, and the bullet now also says to omit the option entirely for mainnet.maxTimeToLivebounds the session now,maxTimeToIdleends an unused one, and the delegation calls are signed with is short-lived and replaced automatically. Leave both unset unless the app has a policy of its own.SessionNotHeldError:getIdentity()throws it rather than returning an anonymous identity when a sign-in exists that this origin holds no credential for.derivationOrigin. The subdomains then hold different principals, the shared record names an account the reading origin can never hold, and/reauthbounces the user forever.hint, or on the default transport. Withouthintthe provider may answer for a different account; and the re-issue runs on page load with no user gesture, so the default window transport is popup-blocked.Sign-in flow.
new AuthClient()with no options, since mainnet II is the default and the skill already recommends mainnet II from local dev. The 8-hourmaxTimeToLiveis gone,signOut()is described as ending the session for every tab, and the init block subscribes togetStatus()rather than telling the caller to reload or reset UI state by hand.New: the client's lifecycle. One client for the page or one per component, both reading the same sign-in;
subscribe()returns an unsubscribe for a view that stops rendering while the client carries on, anddispose()for a client a view owns. Framework-free, and it notes the client is browser-only so a server-rendering framework must client-render whatever owns it.New: sharing a sign-in across sibling subdomains. The three steps, building on the existing multi-origin section rather than repeating it: one shared
derivationOriginauthorized by that origin's alternative-origins document; the same cookie domain throughCookieStateStorage, which means trusting every origin under it; and a/reauthroute that answerssigned-in-elsewherewith a second client (transport: "redirect",prompt: "none",hint), handlingInteractionRequiredErrorby signing out so a stale record cannot loop. Every page runs the load check, not only protected ones, or a visitor already signed in on a sibling sees a signed-out header. Links the client's shared sessions guide for the rest.Local fallback spells out the provider pair. Attributes flow:
noncewas passed as a promise; it has been a callback since v8, so that example could not have run. "5.x API notes" → "Older API notes", with an 8.x section listing what 9.x changed and a link to the upgrade guide.evaluations/internet-identity.jsonThree existing cases retargeted (the provider pair,
authorizeUrl, the construction shape) and five added: session bounds, disposing a client a component owns, and the three shared-subdomain cases.One existing case was also repaired rather than retargeted: case 5 asserted two things in one behavior ("signIn() did not reject and the identity is passed to HttpAgent"), so it could never score honestly; split in two, it goes from 3/4 to 5/5.
Eval results, with baseline
Cases 1, 2 and 5 are the ones this PR changed; 3, 6 and 12 are the untouched cases whose sections were rewritten, re-run as a regression check; the rest are new.
Case 22's baseline is the one worth reading: without the skill an agent writes a single
AuthClientcallingclient.login()in a hidden popup, with noCookieStateStorage, noprompt: 'none'and nohint— a plausible-looking implementation that cannot work.Case 6 scores 3/4 with the skill on a behavior this PR did not touch: it asks for a backend-minted nonce and server-side retention until consumed, while the skill only teaches the first. Same compound-expectation shape as case 5. Worth a follow-up, either splitting the behavior or adding retention to mistake 9.
Verification
npm run validatepasses (31 skills, warnings unchanged frommain),npm run buildcompletes, and every eval case this PR added or changed was run with baseline, as above.🤖 Generated with Claude Code