Skip to content

docs(rules): add Security section to self_review_checklist (follow-up to #3359)#3787

Closed
realmeylisdev wants to merge 0 commit into
mainfrom
chore/lessons-capture-nsec-leak-pattern
Closed

docs(rules): add Security section to self_review_checklist (follow-up to #3359)#3787
realmeylisdev wants to merge 0 commit into
mainfrom
chore/lessons-capture-nsec-leak-pattern

Conversation

@realmeylisdev
Copy link
Copy Markdown
Contributor

Description

Small follow-up to PR #3784 (Phase 1 of #3359). Captures the
investigation lesson in the project's pre-PR checklist so future
reviewers see the rule before it bites again.

Two additions to .claude/rules/self_review_checklist.md:

  1. Testing bullet — flags tests that pin a security-sensitive
    leak as a feature (e.g. expect(verifier, endsWith('.nsec1...'))).
    The pre-fix BYOK leak was hidden behind exactly this shape for the
    lifetime of the imported keycast_flutter package.
  2. Security section — points reviewers at the new
    nsec-leak-guard CI script and asks the local-custody question
    for any new credential / secret handling.

Related Issue: Relates to #3359 (already closed by PR #3784 merging will close it)

No code changes. No tests changes. Docs only.

Type of Change

  • 📝 Documentation

Test plan

  • Diff is 20-line addition to a single Markdown file; no behavior change
  • Headings preserve the existing checklist structure (item under "Testing", new "Security" section between "Testing" and the divider)

Comment thread .claude/rules/self_review_checklist.md Outdated
Comment on lines +120 to +139
- [ ] No test pins a security-sensitive leak as a feature. Before
asserting on the *contents* of a payload (`endsWith('.nsec1...')`,
`body['nsec'] contains 'nsec1'`, `verifier contains <secret>`), ask
"if this assertion fires, is something *good* happening?" The
pre-fix BYOK leak (PR #3784) was pinned for two years by exactly
this shape. When the right answer is "no, the secret should never
be there," flip the assertion to a negative regression guard.

Security:

- [ ] No private-key material crosses the network or storage
boundary. The `nsec-leak-guard` CI job
(`mobile/tools/ci/grep_for_nsec_payload.sh`) catches the exact
`body['nsec']` / `'nsec':` shapes that landed PR #3784, but the
same threat model applies to any new credential / secret
handling. If you're touching an OAuth flow, signer, key import /
export path, or anything that handles a `nsec1` / `nsec_` /
private-key-bearing string, ask: "is the local-custody invariant
preserved end-to-end? Does the server need this to be a secret,
or would a pubkey + signed challenge suffice?"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that, but right now I’m not completely happy with the large number of rules we have. The problem is that they’re sent with every request, which quickly fills up the context window, even in cases where those rules aren’t really needed. That’s why I like skills, especially the review skill, since things like this can be cleaned up once the code scaffold is in place.

It’s probably not a blocker at the moment, even though the rules are quite extensive, but in the future it might make sense to rethink what actually needs to be included in every session and what should only run during code review.

@realmeylisdev
Copy link
Copy Markdown
Contributor Author

Thanks — your framing clicked. The cleanest move here is probably to drop the self_review_checklist.md addition from this PR rather than land it. The 20 new lines are review-time content (a test-pinning anti-pattern + a security implementation question), so they'd land more naturally inside /review-before-commit than in the always-loaded checklist you flagged as already heavy.

Wrote up the broader trajectory at mobile/docs/brainstorm/2026-05-01-claude-rules-context-cost-brainstorm.md plus a concrete first migration step (state_management.md/state-management-help skill) as a follow-up plan. About to force-push the branch back to its parent so the diff goes empty; happy to close once you've had a look. The security / test-pin lessons reland in /review-before-commit once that migration's underway — sound good?

@realmeylisdev realmeylisdev force-pushed the chore/lessons-capture-nsec-leak-pattern branch from 2a2808b to 4818b07 Compare May 1, 2026 13:32
@NotThatKindOfDrLiz
Copy link
Copy Markdown
Member

@realmeylisdev If this is now closed, can we go ahead and delete the branch or are you going to cherrypick from it elsewhere? Just want to keep the repo as clean as we can. TYSM!

@realmeylisdev realmeylisdev deleted the chore/lessons-capture-nsec-leak-pattern branch May 1, 2026 14:55
@realmeylisdev
Copy link
Copy Markdown
Contributor Author

Already deleted — branch was force-pushed back to parent before close so it had zero commits on it; nothing to cherrypick literally. The content (Security section + test-pinning bullet) relands in a future /review-before-commit skill migration per the PR closing comment. Thanks for the nudge to clean up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants