Repository navigation
Conversation
These conventions previously lived in a separate repo and were symlinked into the working copy by a SessionStart hook, so they were invisible to anyone reading this repository and drifted from the code they describe. Checking them in puts them under review alongside the code. Each rule carries `paths:` frontmatter where it applies to a subtree, so it only enters an agent's context when the files it governs are in play. Three facts were stale on arrival and are corrected here: - The e2e rule listed OpenID providers on 11105-11107; a fourth provider on 11108 has since been added. It now points at OPENID_PORTS and friends in scripts/dev-e2e-setup instead of restating the list. - It also documented a `sso_discoverable_domains` init arg that no longer exists — SSO discovery is domain-based now. Dropped. - The backend rule hardcoded the arm64-linux PocketIC asset. The release ships all four arch/OS combos, so it now says to match the host. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 219c7fc. Security OverviewDetected Code Changes
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR checks in AI coding-agent conventions directly into the repository under .claude/rules/ (scoped via paths: frontmatter where appropriate), making them visible in code review and easier to keep in sync with code changes. It also updates repo hygiene so the rules are tracked while other local agent state remains ignored.
Changes:
- Add five topic-focused rule documents under
.claude/rules/(workflow, exhaustive variant handling, backend/Rust, frontend, and Playwright e2e). - Update
.gitignoreto stop ignoring the entire.claude/directory and instead ignore everything except.claude/rules/. - Reduce
CLAUDE.mdto an index pointing at the checked-in rule files.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
CLAUDE.md |
Converted to a short index of the checked-in agent rule docs. |
.gitignore |
Ignores .claude/* local state while explicitly un-ignoring .claude/rules/ for check-in. |
.claude/rules/workflow.md |
Adds workflow conventions (formatting/linting, PR title/description conventions, CI/review loop guidance). |
.claude/rules/handle-all-type-variants.md |
Adds guidance to handle all union/variant cases across Rust/TS/Candid boundaries. |
.claude/rules/backend.md |
Adds Rust-canister conventions (no traps, clippy flags, PocketIC integration-test setup notes). |
.claude/rules/frontend.md |
Adds frontend conventions (design tokens, component reuse, i18n/Svelte/TS patterns, utilities, UX copy). |
.claude/rules/e2e-testing.md |
Adds Playwright e2e/dev-stack guidance and references to the script-defined OpenID port constants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MRmarioruci
reviewed
Aug 3, 2026
MRmarioruci
reviewed
Aug 3, 2026
MRmarioruci
reviewed
Aug 3, 2026
MRmarioruci
reviewed
Aug 3, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The documentation contains incorrect error-handling guidance and incomplete test and lint instructions that could lead to unaudited canister state changes or unreliable verification.
11 open issues
Review effort: Max
Reorganise .claude/rules/ by topic and add the conventions reviewers have repeatedly asked for across the repository's merged, closed and open PRs: code style, security, candid API evolution, stable memory and upgrades, testing, i18n and copy, and scripts/CI. workflow.md becomes pull-requests.md. Address the review on this PR: generalise the comment guidance beyond Tailwind and apply it to all code, drop the full-stack and CI polling sections, and forbid AI attribution footers.
sea-snake
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The change risks accidental commits of nested .claude local state and includes guidance that omits CI checks or contradicts supported security behavior.
13 open issues
11 resolved since last review
Review effort: Max
| - "src/internet_identity_frontend/**" | ||
| - "src/internet_identity_interface/**" | ||
| - "src/asset_util/**" | ||
| - "src/canister_tests/**" |
|
|
||
| - On the IC, returning `Err` commits every write made before it; only a trap rolls back. Run every check that can fail before the first write, and never `?` after a write. | ||
| - Every `await` is a commit point and lets other messages run. Re-read and re-check (authorization, existence, generation) anything captured before it; never write back a value loaded before it. A lock or "in progress" flag written before an `await` needs a recovery path. | ||
| - Don't check, await, then write (lazy salts, get-or-create with `raw_rand`). Initialize at install, or refuse. |
| ## Stack gotchas | ||
|
|
||
| - With `NO_HOT_RELOAD=1` and `SEPARATE_FRONTEND_CANISTER=1` the browser loads assets from the canister. `.svelte` changes take effect only after `dev-e2e-setup` reinstalls it. | ||
| - Re-running `dev-e2e-setup` does not re-apply changed init args to an installed canister. Run `--teardown` first. |
| - Rust: `cargo fmt`, then `cargo clippy` with the flags in `backend.md`. | ||
| - JS/TS/Svelte: `npm run format`, `npm run lint -- --fix`, `npm run check`. CI stops at the first failure, so fix all three. | ||
| - After a `.did` change: `npm run generate` (see `candid-api.md`). | ||
| - Revert any `src/frontend/src/lib/locales/*.po` diff (see `i18n-and-copy.md`). |
| - Parse URLs with `new URL()` / the `url` crate and compare exact origins (`new URL(x).origin === x`). Never concatenate URL parts or use substring checks. | ||
| - Match a hostname against an allowlist exactly or on a dot boundary (`h === d || h.endsWith("." + d)`), never a bare `endsWith(d)`. | ||
| - Accept only the canonical form of security-relevant input (lowercase origins without default port or userinfo, uncompressed keys); a caller sending another form is refused, not silently normalized. Operator configuration (install args) is canonicalized (trim, ASCII-lowercase) once, before it is persisted. | ||
| - Every origin used as an outcall target or derivation input goes through the canonicalizing validator and requires `https`. Plain `http` is allowed only for loopback hosts behind an explicit dev flag. |
| ## Delegations and keys | ||
|
|
||
| - Length-prefix every variable-length field when hashing a seed. | ||
| - Keep delegation seeds minimal and justify every input. Set delegation `targets` where the canister signs, and keep them when rebuilding a chain. |
|
|
||
| - Length-prefix every variable-length field when hashing a seed. | ||
| - Keep delegation seeds minimal and justify every input. Set delegation `targets` where the canister signs, and keep them when rebuilding a chain. | ||
| - Delegation lifetimes that cross the IC are at least 5 minutes, because of replica clock drift. |
| - "src/internet_identity/src/main.rs" | ||
| - "src/internet_identity/tests/integration/upgrade.rs" | ||
| - "src/internet_identity/tests/integration/rollback.rs" | ||
| - "src/internet_identity/stable_memory/**" |
Comment on lines
+19
to
+20
| - Stored records are minicbor `#[cbor(map)]` with numeric keys (`#[n(k)]`). Rust field names can be renamed freely; the keys cannot. | ||
| - A new field on a stored type must be `Option` or `#[cbor(default)]`. Existing records don't have the key, and a required field traps every read of them. This took staging down once (#4481). |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The proposed rules could reject supported sign-ins, override delegation lifetime limits, and break compatibility with existing Candid-backed stable data.
13 open issues
1 resolved since last review
Review effort: Max
| ## Stack gotchas | ||
|
|
||
| - With `NO_HOT_RELOAD=1` and `SEPARATE_FRONTEND_CANISTER=1` the browser loads assets from the canister. `.svelte` changes take effect only after `dev-e2e-setup` reinstalls it. | ||
| - Re-running `dev-e2e-setup` rebuilds a canister only when files in its own directory (or `src/frontend`) are newer than its wasm. Changes to shared crates (`src/internet_identity_interface`, `src/asset_util`) and changed init args are not picked up: run `--teardown` first. |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved guidance could break persisted-state compatibility and supported authentication flows, while the E2E teardown workaround still allows tests to run stale Wasm.
13 open issues
Review effort: Max
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Critical inaccuracies in the guidance could break persisted-state compatibility and extend delegation lifetimes beyond caller or grant-expiration limits.
13 open issues
Review effort: Max
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved guidance could break persisted-state decoding and supported authentication flows, while the E2E teardown workaround still permits tests to run against stale builds.
13 open issues
Review effort: Max
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved errors in the guidance could reject supported authentication flows, break stable-state compatibility, and leave end-to-end tests running stale Wasm builds.
13 open issues
Review effort: Max
This branch has not been deployed
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.
The conventions coding agents follow in this repo have been maintained in a separate repository and symlinked into each developer's working copy. That keeps them invisible to anyone reading this repository, and they drift from the code they describe. Checking them in puts rule changes through code review alongside the code they govern.
The rules are distilled from the review history of this repository: the discussions on all 4,289 pull requests (merged, closed and open) were read and the guidance reviewers gave repeatedly, or stated as a principle, was kept. Each candidate was checked against the current tree; guidance from the lit-html, dfx and selenium era that no longer applies was dropped.
Changes
Organise
.claude/rules/by topic. Each file carriespaths:frontmatter where it applies to a subtree, so it only enters an agent's context when those files are in play:pull-requests.mdcode-style.mdhandle-all-type-variants.mdsecurity.mdsrc/**backend.mdawait, layering, Rust style, clippy, PocketICcandid-api.md*.did, interface crate, bindings,docs/ii-spec.mdxstable-memory.mdfrontend.mdsrc/frontend/**i18n-and-copy.md<Trans>/$t, UI copytesting.mde2e-testing.mdscripts/dev-e2e*scripts-and-ci.mdscripts/**,.github/**, manifestsWhere review history and the earlier rules disagreed, the rules follow what the code and the most recent reviews do:
Storable::from_bytes(which cannot return an error) and HTTP outcall transforms, plus deliberate rollback. It adds that returningErrdoes not roll back earlier writes, so fallible checks run before the first write.InternalCanisterError.This PR's own review:
code-style.md).requestReviews; the REST call answered 200 but added nobody.Un-ignore the rules directory in
.gitignore:.claude/becomes.claude/*plus!.claude/rules/, since a directory-level ignore stops git descending. Everything else Claude Code writes there stays ignored.Reduce
CLAUDE.mdto an index of the rules directory, and note thatFLOWS.mdxis out of date.