42 KiB

Raw Blame History

CLAVITOR-PRINCIPLES.md Review Report

Date: 2026-04-08
Reviewer: Claude Code
Scope: Full document review across correctness, conciseness, completeness, viability, security, consistency, clarity, actionability, maintainability, audibility, compliance, and localization.

Executive Summary

The document is exceptionally well-constructed for a security-critical project. It demonstrates strong architectural thinking, clear threat modeling, and actionable daily practices. However, several issues ranging from minor inconsistencies to potential security blind spots warrant attention.

Metric	Score	Notes
Security	9/10	Comprehensive threat model, one minor gap in Threat C analysis
Correctness	8/10	Technical details accurate, some terminology drift
Completeness	7/10	Missing incident response, versioning, and escalation procedures
Conciseness	7/10	Some redundancy and over-explanation
Consistency	8/10	Generally consistent, minor formatting drift
Viability	8/10	Mostly viable, some idealistic rules may create friction
Clarity	9/10	Exceptionally clear with concrete examples
Actionability	9/10	Daily checklist is exemplary
Maintainability	8/10	Good, but needs deprecation tracking
Audibility	9/10	Excellent grep-driven verification (see F036 on LLM-era evolution)
Compliance	6/10	GDPR mentioned but narrowly; missing SOC2, PCI-DSS, data retention, cross-border transfer
Localization	1/10	Not addressed — no i18n/l10n guidance for multi-language UI

Findings

F001: Redundancy — "Foundation First" duplicated

Location: Lines 10-13 and 20-34 (restatement)
Severity: Low
Metric: Conciseness

The "Foundation First — No Mediocrity. Ever." principle appears with significant overlap: first as an introductory statement (lines 10-13), then expanded in Part 1 (lines 20-34). The second version is more complete; the first is redundant.

Recommended Action:
Remove the introductory paragraph (lines 10-13) and rely solely on the Part 1 version, which is more complete. Alternatively, reduce the intro to a single sentence: "Foundation First — see Part 1."

F002: Missing — "Quick fixes are not fixes" lacks counter-example

Location: Line 28
Severity: Medium
Metric: Completeness

The principle states "A 'temporary' hack that ships is permanent" but doesn't provide the positive pattern. Developers need to know what TO do when urgent fixes are needed.

Recommended Action:
Add a positive pattern example:

When urgent: fix the symptom with a comment containing // TEMP-FIX: <ticket> and 
a mandatory fix-by date. No TEMP-FIX older than 72 hours may exist in main.

F003: Inconsistency — "No migrations during early-stage work" vs "MigrateDB" exists

Location: Lines 79-85, 645-650 (checklist)
Severity: Medium
Metric: Consistency

The principle states "Do not write migration code" but the daily checklist (C3) acknowledges MigrateDB exists "for the existing alternate_for and verified_at columns (legacy from earlier work)." This creates a "do as I say, not as I do" situation.

Recommended Action:
Either: (a) Remove the legacy migration code and update C3, or (b) add explicit text to the principle acknowledging the grandfathered exception and stating when the exemption expires (e.g., "Remove MigrateDB when legacy columns are phased out by [date]").

F004: Viability Risk — "No env vars for fundamental paths" may block valid use cases

Location: Lines 54-55, 636-642 (checklist)
Severity: Medium
Metric: Viability

The KISS principle states "No env vars when a hardcoded relative path is correct" with examples ./vaults/ and WL3. While simplicity is valuable, this hardcoding prevents:

Container deployments with volume mounts at specific paths
Multi-tenant hosting with separate data directories
Compliance requirements requiring data on encrypted filesystems at specific mount points

Recommended Action:
Add a clarification: "Exception: When the deployment environment (containers, compliance mounts) requires configurable paths, a single CLAVITOR_DATA_ROOT env var is acceptable, but no per-path fragmentation (DATA_DIR, WL3_DIR, VAULT_DIR, etc.)."

F005: Technical Inaccuracy — "Two crypto implementations of the same primitive" check is misleading

Location: Lines 676-688 (checklist D2)
Severity: Medium
Metric: Correctness

The checklist says to run diff clavis-vault/cmd/clavitor/web/crypto.js clavis-cli/crypto/crypto.js and expects "zero diff (or... intentionally different... documented)." This is misleading because:

The CLI crypto.js may need target-specific adapters (BearSSL vs WebCrypto)
The paths suggest different directory structures that may require different module exports
A zero-diff requirement would prevent any platform-specific optimizations

Recommended Action:
Change the check to:
"Expected: core encryption/decryption logic (encrypt_field, decrypt_field, HKDF) is byte-identical. Platform-specific wrappers/adapters are permitted if documented in clavis-crypto/README.md and don't affect the output format."

F006: Missing — No incident response procedure

Location: Entire document
Severity: High
Metric: Completeness

The document thoroughly describes prevention but contains zero incident response guidance. Given the security-critical nature, this is a significant gap:

What happens when Threat A is detected? (agent locked, then what?)
Who is notified of a harvester pattern?
What's the SLA for PRF unlock after a false positive lockout?
How is a compromised vault file rotated?

Recommended Action:
Add Part 7: Incident Response with sections for:

Agent lockout recovery procedure
Compromised credential token rotation
Audit log review cadence
Escalation contacts/owner notification
Emergency PRF unlock process (if owner loses authenticator)

F007: Security Gap — Threat C analysis misses "steal at creation" vector

Location: Lines 274-298
Severity: High
Metric: Security

Threat C discusses stolen CVT client credentials, but misses a critical sub-vector: credential theft at creation time. When an agent is created:

Browser generates L2
Browser encrypts with L0, creates type-0x01 token
This token briefly exists in browser memory before being sent to agent

A malicious browser extension or XSS could exfiltrate the token during this window. The per-agent IP whitelist helps post-creation, but doesn't address theft-at-creation.

Recommended Action:
Add to Threat C:
"Sub-vector C2 — Credential theft at creation: The type-0x01 token exists transiently in browser memory during agent enrollment. Defense: agent tokens should be delivered through a side-channel (QR code display + camera scan by agent device, or file download + scp) rather than clipboard or direct HTTP response to browser. Document the secure transfer mechanism in the agent enrollment flow."

F008: Consistency — "Must never" lists use inconsistent formatting

Location: Part 3 subprojects
Severity: Low
Metric: Consistency

The "Must never" sections across subprojects use inconsistent formatting:

clavis-vault: bullet list with "- Have functions named..." (line 358)
clavis-cli: bullet list with "- Bridge to Go..." (line 405)
clavis-chrome: bullet list with "- Store L2 or L3..." (line 456)
clavitor.ai/admin: bullet list with "- Hold any decryption material..." (line 491)

Some use "- Verb..." (action-oriented), others use "- No/None..." (prohibition-oriented). clavis-vault has a "Must never" that starts with "Have" which is confusing ("Must never have functions named..." vs "Must never: have functions named...")

Recommended Action:
Standardize all "Must never" bullets to start with action verbs in negative form:

"Implement crypto operations in Go" (not "Bridge to Go for any...")
"Store L2 or L3 in extension storage" (not "Use a different field...")

F009: Missing — No definition of "early-stage"

Location: Lines 79-85, 362
Severity: Medium
Metric: Completeness

The document references "early-stage work" multiple times as justification for no migrations, no backwards compatibility, and willingness to delete vaults. However, "early-stage" is never defined. This creates ambiguity about when the rule flips.

Recommended Action:
Add a definition in Part 1 or a footnote:
"'Early-stage' means: no paying customers, no production deployments outside the core team's infrastructure, and version < 1.0. The project remains in early-stage until [specific criteria: e.g., first paid subscription, 100 active vaults, or v1.0 release]. When early-stage ends, the principles on migrations and backwards compatibility will be revised."

F010: Audibility Gap — Checklist A6 (keys never logged) is insufficient

Location: Lines 587-593
Severity: Medium
Metric: Audibility, Security

The grep for A6 checks for obvious key material (l1Key, master, wrappedL3, prf, p1Bytes) but misses:

Variable naming variations (l1_key, l1KeyBytes, masterKeyBytes)
Hex encoding patterns that might reveal keys in logs
Indirect logging via error wrapping (errors.Wrap(err, "failed with master key: " + key))

Recommended Action:
Enhance A6 with additional patterns:

# Check for variable naming variations
grep -rnE "(l1[_-]?key|l1[_-]?bytes|master[_-]?key|wrapped[_-]?l3|prf[_-]?output|p1[_-]?bytes)" \
  clavis-vault/api/ clavis-vault/lib/ | grep -iE "log\.|fmt\.Print|errors\.|AuditEvent"

# Check for hex encoding of key-sized data (32 bytes = 64 hex chars)
grep -rnE "%x.*\[32\]|%x.*\[64\]|hex\.EncodeToString.*key|hex\.EncodeToString.*master" \
  clavis-vault/api/ clavis-vault/lib/

F011: Viability Risk — "No docstrings on code you didn't change" may reduce maintainability

Location: Lines 104-105
Severity: Low
Metric: Viability, Maintainability

The KISS principle discourages adding docstrings to unchanged code. While this prevents PR bloat, it may result in persistently under-documented legacy code. As the codebase grows and original authors leave, this creates knowledge silos.

Recommended Action:
Add a clarifying exception:
"No docstrings on code you didn't change, UNLESS you needed to spend >10 minutes understanding what the code does. In that case, add the docstring you wish existed — pay it forward."

F012: Technical Inaccuracy — Threat B time calculation uses Grover's incorrectly

Location: Line 267
Severity: Low
Metric: Correctness

The text states: "32-byte AES-256-GCM keys are not brute-forceable. ~10⁵⁷ years at 10¹² guesses/sec; Grover's quantum speedup leaves 2¹²⁸ effective security."

Grover's algorithm reduces the effective key space by a square root, not to 2¹²⁸. For AES-256, Grover's provides a sqrt(2²⁵⁶) = 2¹²⁸ speedup, meaning the effective security is reduced to that of a 128-bit classical key, not "2¹²⁸ effective security." The phrasing is confusing.

Recommended Action:
Correct to:
"32-byte AES-256-GCM keys are not brute-forceable. ~10⁵⁷ years at 10¹² guesses/sec; Grover's quantum algorithm provides a square-root speedup, reducing effective security to that of a 128-bit classical cipher — still computationally infeasible (~10²⁹ years)."

F013: Missing — No guidance on principle deprecation

Location: Lines 768-771
Severity: Medium
Metric: Completeness

Part 5 mentions marking principles deprecated but doesn't specify the mechanism:

What format should the deprecation notice take?
Who authorizes deprecation?
How long should deprecated principles remain before removal?
Should deprecated principles move to an appendix or stay in place?

Recommended Action:
Add to Part 5:

### Deprecation format
When a principle is obsolete:
1. Add **DEPRECATED** in bold at the start of the principle header
2. Include: date deprecated, reason, replacement principle (if any), and authorizing person (e.g., "Deprecated 2026-04-08: Early-stage ended. Migration code now required. — Johan")
3. Deprecated principles remain in place for 90 days, then move to Appendix A: Deprecated Principles
4. A deprecated principle's grep checks in Part 4 are commented out with a reason

F014: Redundancy — "No backwards-compat shims" duplicates "Don't add what's not needed"

Location: Lines 87-96 vs 98-105
Severity: Low
Metric: Conciseness

The "No backwards-compat shims" section lists specific anti-patterns (flags, rename hacks, re-exports, removed comments, commented-out code). The following section "Don't add what's not needed" has overlapping scope. Both mention dead code/comments.

Recommended Action:
Merge the two sections under "Don't add what's not needed" and structure as:

### Don't add what's not needed (and remove what isn't used)

- No backwards-compatibility shims: ...
- No error handling for impossible situations: ...
[etc]

F015: Security Gap — No guidance on secure deletion of keys from memory

Location: Part 2, threat model sections
Severity: Medium
Metric: Security, Completeness

The document extensively discusses where keys live (L0, L1, L2, L3) and what never holds them, but provides no guidance on secure deletion of key material from memory after use. In Go, this is particularly important because:

The garbage collector may move/copy memory
memset or clear doesn't guarantee overwriting the physical memory
Sensitive data may remain in memory after session close

Recommended Action:
Add to Part 2 (after Cardinal Rule #3) or to clavis-crypto subproject:

### Secure key lifecycle in memory

When a key is no longer needed (session close, credential operation complete):
- Use `memset_explicit` (C11) or `runtime.memclrNoHeapPointers` (Go) to clear buffers
- In Go, sensitive byte slices should be allocated from `mmap` with `PROT_READ|PROT_WRITE` 
  and `MADV_DONTDUMP` to exclude from core dumps
- The browser's `sessionStorage` clear is automatic on tab close; no additional action needed
- CLI: clear key buffers before exit; use `explicit_bzero` on sensitive stack buffers

F016: Correctness — Part 4 Section F uses imprecise commands

Location: Lines 729-742
Severity: Low
Metric: Correctness, Audibility

Check F1 says to run go test ./lib/... ./api/... but this doesn't capture all tests in the monorepo. F2 says make build but the Makefile might have other targets that matter. F3 is described as "not greppable" but could include guidance.

Recommended Action:
Update F1:

cd clavis-vault && go test ./...
cd clavitor.ai/admin && go test ./...
# Add any other test directories

Update F2:

cd clavis-vault && make dev  # validates JS as part of dev build
# or for CI: make build

Add to F3:

**F3. New code has tests.**

git diff --name-only HEAD~1 | grep -E ".go$" | while read f; do test_file="$(dirname $f)/$(basename $f .go)_test.go" [ -f "$test_file" ] || echo "WARNING: $f has no test file" done

F017: Viability Risk — "Restart rule" is not actionable

Location: Lines 32-34
Severity: Medium
Metric: Viability, Actionability

The "restart rule" states: "When the foundation is wrong: start over. Not 'refactor slightly'... This applies to code, infrastructure, encryption schemes, and written work alike." This is philosophically correct but operationally vague:

What constitutes "wrong" enough to restart?
How much sunk cost is too much to restart?
What's the approval process for a restart vs refactor?

Recommended Action:
Add decision criteria:

### When to restart vs. refactor

**Restart when** (any of):
- The core abstraction is incorrect (e.g., server holding L2 material)
- Three or more fundamental fixes are needed for one feature
- The code contradicts its own stated invariants
- Security review reveals architectural vulnerability

**Refactor when**:
- Implementation is messy but abstraction is sound
- Performance issues but semantics are correct
- Code duplication but logic is correct

**Approval**: Restart decisions require explicit user confirmation. State: "The foundation is wrong because [reason]. I recommend restarting [component]. Sunk cost: [commits/lines]. Estimated rebuild: [time]. Proceed?"

F018: Clarity Issue — "Don't add what's not needed" mixes multiple concerns

Location: Lines 98-105
Severity: Low
Metric: Clarity, Consistency

This section combines four unrelated concerns:

Error handling for impossible situations (line 100)
Validation at trusted boundaries (lines 101-102)
Feature flags for hypothetical futures (line 103)
Docstrings on unchanged code (lines 104-105)

These are distinct principles with different rationales. Combining them reduces clarity.

Recommended Action:
Split into four subsections or bullet points with clear headers:

- **No error handling for impossible situations.** Trust internal code.
- **No validation at trusted internal boundaries.** Validate only at the system perimeter...
- **No feature flags for hypothetical futures.** Build what's asked, no more.
- **Minimal documentation on unchanged code.** Only comment where logic isn't self-evident.

F019: Security Gap — Threat A doesn't address "slow harvest" evasion

Location: Lines 240-252
Severity: Medium
Metric: Security, Completeness

Threat A defenses include rate limits (3/min, 10/hr) and two-strike lockdown. However, a sophisticated attacker could:

Harvest at 2/min (below threshold)
Use time-of-day patterns (only during business hours)
Randomize intervals to avoid detection

The rate limits are reactive, not proactive. There's no mention of behavioral analysis or anomaly detection.

Recommended Action:
Add to Threat A:

**Advanced evasion consideration:** Rate limits are a first line. Future enhancement:
anomaly detection on access patterns (time-of-day consistency, geographic consistency,
entry access distribution). Document that current rate limits are baseline defense,
and audit logs should be reviewed for slow-and-low patterns below rate limits.

F020: Correctness — Check E2 description is slightly inaccurate

Location: Lines 703-717
Severity: Low
Metric: Correctness

The check states "Every AgentCanAccess call should be followed by an agentReadEntry call in the same code path." However, the correct sequence is:

AgentCanAccess (authorization check)
[Handler-specific logic]
agentReadEntry (rate limit/strike tracking)

They're not necessarily "followed by" in an immediate sense — there may be intermediate logic.

Recommended Action:
Correct to:

Every credential-read handler must call both `AgentCanAccess` (before accessing 
data) and `agentReadEntry` (after decryption, before returning). Verify by ensuring
each handler contains both function names in the proper order.

F021: Completeness — No guidance on test fixture security

Location: Check A1 mentions "Test fixtures may use the term in test data"
Severity: Medium
Metric: Security, Completeness

Check A1 notes that test fixtures may contain master_key in test data. This is a dangerous blanket exception without guidance on:

Whether test keys should be distinguishable from real keys
Whether test fixtures should be encrypted/committed
How to ensure test keys don't leak into production

Recommended Action:
Add to Part 4 Section F or as a new subsection in Part 2:

### Test fixture key material
Test data containing key material:
- Must use the prefix `TEST_` in all variable/field names
- Must use obviously fake values (e.g., `master_key = bytes.Repeat([]byte{0xAB}, 32)`)
- Must not be derived from any production key material
- Must not be committed to the repository in binary form

F022: Redundancy — Audit logging mentioned in multiple places

Location: Lines 108-115, 114 (repeated in example), and implicitly throughout
Severity: Low
Metric: Conciseness

Audit logging is mentioned as a general principle (lines 108-115), with code example (line 114), and implied in various threat defenses. The code example is also repetitive — it shows the full function call twice in different contexts.

Recommended Action:
In Part 1, keep the principle but move the full code example to a single location (perhaps Part 3 clavis-vault section where it's most relevant) and reference it:

Audit format: `lib.AuditLog()` — see `clavis-vault` section for usage example.

F023: Viability — Part 6 "When you have to violate a principle" lacks escalation path

Location: Lines 775-794
Severity: Medium
Metric: Viability

Part 6 describes what to do when principles conflict (surface to user, pick higher consequence, document, add TODO). However, it doesn't address what happens if:

The user disagrees with the agent's assessment of which principle has higher consequence
The violation is discovered post-hoc (code already committed)
Multiple principles seem equally consequential

Recommended Action:
Add to Part 6:

### Escalation and post-hoc discovery

**User disagreement:** If the user insists on a lower-consequence principle over a higher one 
(e.g., speed over security), escalate to project owner (Johan) before proceeding. 
Document the override in the PR description.

**Post-hoc discovery:** If you find a committed violation during daily review:
1. Assess if it's actively dangerous (security veto) — if yes, escalate immediately
2. If not dangerous, add to TODO and address in current sprint
3. Never leave a violation undocumented for >24 hours

**Equal consequence conflicts:** When two principles of equal tier conflict, prefer the 
one that preserves user agency and data integrity.

F024: Consistency — Missing period in final line

Location: Line 798
Severity: Very Low
Metric: Consistency

The final line "Foundation First. No mediocrity. Ever." has inconsistent punctuation. "Ever" has no period but the others do.

Recommended Action:
Add period: "Foundation First. No mediocrity. Ever." → "Foundation First. No mediocrity. Ever."

F025: Completeness — No explicit versioning of the principles document itself

Location: Entire document
Severity: Medium
Metric: Completeness, Maintainability

The document doesn't have a version number or date. When an agent reads this "end-to-end" each morning, how do they know if principles have changed since yesterday? Without versioning, agents cannot:

Track which principles are new/changed
Review the delta rather than the full document
Correlate violations with principle versions

Recommended Action:
Add a document header:

---
Document Version: 1.0
Last Updated: 2026-04-08
Change Log: See Appendix B (or git log)
---

Or use the existing date in Part 5: "Reference the incident or insight that surfaced the principle (commit hash, issue number, or session date)" — add principle document version to that reference.

F026: Audibility — Check B1 pattern may have false negatives

Location: Lines 605-611
Severity: Low
Metric: Audibility

Check B1 searches for catch.*plaintext or catch.*ciphertext or catch.*=.*null. However, a malicious silent fallback could use:

catch (e) { return data; } (returns encrypted or undefined)
catch { decrypted = ""; } (empty string, not null)
finally blocks that silently restore old values

Recommended Action:
Enhance B1:

# Check for any catch that doesn't show user-visible failure
grep -rn "catch.*{" clavis-vault/cmd/clavitor/web/ | \
  grep -v "decryption failed\|user-visible\|alert\|console.error"

Or add manual review note: "All catch blocks in decryption paths must be reviewed manually — automated grep may miss subtle fallbacks."

F027: Security — Missing explicit prohibition on key material in crash dumps

Location: Threat B section (lines 253-272)
Severity: Medium
Metric: Security

Threat B discusses backups, logs, metrics, error messages, audit rows, and debug dumps, but doesn't explicitly mention crash dumps/core files. These are distinct from "debug dumps" and may persist in /var/crash/ or system directories.

Recommended Action:
Add to Threat B:

- **No key material in crash dumps.** Disable core dumps for Clavitor processes
  (`prctl(PR_SET_DUMPABLE, 0)` on Linux, `PROC_UNSET_DYLD_TRACES` on macOS) or 
  use `MADV_DONTDUMP` on memory pages holding key material.

F028: Clarity — "Three threats, no others" claim may be overstated

Location: Line 227-232
Severity: Low
Metric: Clarity, Correctness

The document states: "The three threats below are the entire universe Clavitor defends against." This is a strong claim. However, the threat model excludes:

Supply chain attacks (malicious dependency)
Social engineering (owner tricked into unlocking)
Side-channel attacks (timing, power analysis)
Infrastructure compromise (compromised Tailscale node)

While some of these may be out of scope, claiming "entire universe" is overstated.

Recommended Action:
Change to:

The three threats below are the **primary** universe Clavitor defends against 
at the application layer. Infrastructure, supply chain, and social engineering 
are handled through operational security practices outside this codebase.

F029: Correctness — Key tier table has formatting inconsistency

Location: Lines 189-195
Severity: Very Low
Metric: Consistency

The key tier table uses inconsistent formatting:

L0, L1, L2, L3, P1 use code formatting
master_key uses plain text (no code formatting)
Column "What" describes byte counts for all except P1

Recommended Action:
Standardize:

| Tier | What | Server may hold |
|------|------|-----------------|
| L0   | 4 bytes — vault file routing | Yes (sliced from `L1[:4]`) |
| L1   | 8 bytes — vault content encryption | Yes (in bearer token) |
| L2   | 16 bytes — agent decryption key | **NEVER** |
| L3   | 32 bytes — hardware-only key | **NEVER** |
| P1   | 8 bytes — public lookup token | Yes (browser computes & sends) |
| Master | 32 bytes — full PRF output | **NEVER** |

F030: Completeness — No mention of compiler optimizations affecting security

Location: Part 2, security sections
Severity: Medium
Metric: Security, Completeness

The document doesn't address compiler optimizations that may undermine security:

Go's compiler may optimize away clear() calls on slices that are never read again
C compilers may dead-store-eliminate memset calls
This affects the secure deletion of key material

Recommended Action:
Add to Cardinal Rule #2 or #3:

**Compiler considerations:** Standard `memset` or `clear()` may be optimized away.
Use `runtime.memclrNoHeapPointers` (Go) or `memset_explicit` (C11) or 
`OPENSSL_cleanse` (if using OpenSSL) to ensure memory is actually cleared.

Location: Lines 301-313 (Cardinal Rule #5), scattered references
Severity: Medium
Metric: Compliance

The document extensively discusses GDPR (specifically the "GDPR-out-of-scope by design" property of WL3), but fails to address other compliance frameworks that may apply to a credential vault service:

SOC 2 Type II: Controls for availability, confidentiality, processing integrity
PCI-DSS: If the vault stores payment card credentials
HIPAA: If used in healthcare contexts (ePHI access credentials)
CCPA/CPRA: California privacy laws (broader than GDPR in some aspects)
FedRAMP: If any government use is anticipated
ISO 27001: Information security management

The narrow GDPR focus may lead to architectural decisions that satisfy one framework while violating others.

Recommended Action:
Add Part 8: Compliance Frameworks or expand Cardinal Rule #5:

### Cardinal Rule #5 — Data minimization for compliance

WL3 is designed for **global** compliance minimization, not just GDPR:
- No PII in WL3 (satisfies GDPR, CCPA, LGPD, PIPEDA)
- No payment card data in vault (PCI-DSS scope exclusion)
- Audit logs retained per strictest applicable jurisdiction (often 7 years)
- Encryption at rest satisfies SOC 2 CC6.1, ISO 27001 A.10.1.2

When adding features, ask: which compliance framework requires this? If none, don't add it.

F032: Compliance Gap — No data retention policy guidance

Location: Part 2, audit sections
Severity: High
Metric: Compliance

The document states "we never delete" vault data (line 512) until GDPR request, but doesn't specify:

How long audit logs must be retained
When deleted entries are truly purged (vs marked deleted)
How backups are rotated and when they're destroyed
Cross-border data transfer mechanisms (EU data in US POPs)

This creates compliance risk across multiple jurisdictions with differing retention requirements.

Recommended Action:
Add to Part 2 or Part 8:

### Data retention

| Data type | Retention | Rationale |
|-----------|-----------|-----------|
| Vault content | Until customer deletion + 30 days | GDPR Art. 17, business continuity |
| Audit logs | 7 years | SOX, SOC 2, various national requirements |
| Failed auth attempts | 90 days | Threat analysis, then purge |
| Backups | 30 days rolling | Disaster recovery, then secure wipe |
| Telemetry | 90 days | Operational metrics only |

Secure deletion: Use `shred` (files), `DBAN` patterns (disks), or `crypto/rand` overwrite 
for SQLite `VACUUM` operations before file removal.

F033: Compliance Gap — No cross-border data transfer mechanism

Location: Not addressed
Severity: Medium
Metric: Compliance

For EU customers with data in US POPs (or vice versa), the document doesn't address:

Standard Contractual Clauses (SCCs)
Data Processing Agreements (DPAs)
Adequacy decisions (EU-US Data Privacy Framework)

Cardinal Rule #5 claims GDPR-out-of-scope, but if the central admin DB is in a different jurisdiction than the POP, there are still transfer implications.

Recommended Action:
Add to Part 8:

### Cross-border transfers

WL3 files are jurisdiction-agnostic (no PII), but metadata flows between POPs and central 
require SCCs in place. The vault slot registry (central) and WL3 files (POPs) are always 
in the same jurisdiction for a given customer. Multi-region customers are explicitly 
opt-in with transfer mechanism documented at enrollment.

F034: Localization — No internationalization guidance exists

Location: Not addressed in any subproject section
Severity: High
Metric: Localization

The document contains zero guidance on:

UI string externalization (browser, mobile, CLI)
Right-to-left (RTL) language support (Arabic, Hebrew)
Date/time/number localization
Locale-aware credential detection (different TLDs, country-specific form patterns)
Translation workflows (who translates, how are updates propagated)

This is a significant gap for a product with browser extensions and mobile apps that will inevitably face non-English users.

Recommended Action:
Add subproject sections for localization or a dedicated Part 9:

## Part 9 — Localization (i18n/l10n)

### Principle: English-first, but locale-ready

- All user-facing strings live in `locales/en.json` (single source of truth)
- Browser/extension: Use `browser.i18n.getMessage()` with `_locales/<lang>/messages.json`
- CLI: Use gettext `.po` files, respect `$LANG` and `$LC_MESSAGES`
- Mobile: Use platform-native i18n (Android resources, iOS Localizable.strings)
- Vault web UI: Serve locale bundle based on `Accept-Language`, fallback to English

### Hard vetos:
- No hardcoded strings in source code (except log messages, which are English-only)
- No user-facing string concatenation (`"Hello, " + username`) — use parameterized templates
- No locale-specific crypto or validation logic (locale is display-only)
- No translation of credential field names (username/password are universal)

### RTL considerations:
- Browser extension UI must support RTL layouts (`dir="rtl"`)
- Vault web UI CSS uses logical properties (`inline-start`, not `left`)

### Credential detection:
- TLD matching must respect IDN (internationalized domain names)
- Form field detection uses localized label heuristics (maintained per-locale)

F035: Localization — Form field detection without locale awareness

Location: Implicit in browser extensions
Severity: Medium
Metric: Localization

Browser extensions that detect login forms by field names/placeholders will fail on non-English sites. A French site's "Nom d'utilisateur" or Japanese "ユーザー名" won't match hardcoded "Username" heuristics.

Recommended Action:
Add to clavis-chrome/firefox/safari sections:

**Must never:**
- Detect forms using English-only field name patterns
- Rely solely on `id="username"` (common but not universal)

**Must:**
- Use W3C `autocomplete` attributes as primary signal
- Maintain locale-specific keyword lists for heuristics (German: "Benutzername", French: "identifiant")
- Support Cyrillic, CJK, and Arabic script in field detection

F036: Methodology Critique — Grep-based checks are legacy in LLM era

Location: Part 4 (entire section)
Severity: Medium
Metric: Audibility, Maintainability

The daily checklist relies heavily on grep for verification (A1-A7, B1-B3, C1-C4, D2, E1-E3). While grep is reliable for exact patterns, it:

Cannot understand semantic equivalence (e.g., master_key vs mKey vs key[:32])
Produces false negatives on obfuscated or indirect code paths
Requires manual maintenance as code evolves (pattern drift)
Cannot evaluate the "spirit" of a principle, only the "letter"

With LLMs available for code review, grep-based checks are a local maximum. The daily reviewer should be running semantic analysis, not just pattern matching.

Recommended Action:
Update the Part 4 introduction:

## Part 4 — Daily review verification

**Methodology:** These checks use `grep` for fast, deterministic screening. 
However, grep catches patterns, not intent. The daily reviewer must also:
1. Run the automated grep checks (catches obvious violations)
2. Run semantic review on changed files using available analysis tools (catches obfuscated violations)
3. Manually review any ambiguous findings

**Goal:** Zero violations that an intelligent reviewer would catch, not just zero grep matches.

Consider replacing some greps with semantic checks:

A6 (keys in logs): Use taint analysis to find key material flowing to log functions
B1 (silent decryption fallback): Use control-flow analysis to find catch blocks that don't re-throw or return error indicators
D1 (DRY violations): Use clone detection to find similar code blocks

Summary of Recommendations by Priority

Critical (Security/Viability/Compliance Blockers)

ID	Finding	Action
F007	Threat C missing "steal at creation"	Add C2 sub-vector with defense
F006	No incident response procedure	Add Part 7: Incident Response
F034	No internationalization guidance exists	Add Part 9: Localization

High (Should Address Soon)

ID	Finding	Action
F003	Migration code inconsistency	Resolve MigrateDB or document exception
F004	Hardcoded paths blocking valid use cases	Add env var exception clause
F015	No secure deletion guidance	Add memory clearing guidance
F017	Restart rule not actionable	Add decision criteria
F023	Violation escalation path missing	Add to Part 6
F032	No data retention policy guidance	Add retention table and purge procedures
F035	Form detection without locale awareness	Add locale-specific keyword lists

Medium (Quality Improvements)

ID	Finding	Action
F002	Quick fix guidance incomplete	Add positive pattern
F005	Crypto diff check misleading	Clarify byte-identical vs adapters
F009	"early-stage" undefined	Add explicit definition
F010	A6 grep insufficient	Add pattern variations
F019	Threat A slow harvest gap	Add evasion consideration
F021	Test fixture security vague	Add fixture guidance
F025	No document versioning	Add version header
F027	Missing crash dump prohibition	Add to Threat B
F028	"Entire universe" overstatement	Weaken to "primary"
F031	GDPR is the only framework mentioned	Add Part 8: Compliance Frameworks
F033	No cross-border data transfer mechanism	Add SCC/DPA guidance
F036	Grep-based checks are legacy in LLM era	Update methodology to include semantic review

Low (Polish/Consistency)

ID	Finding	Action
F001	Foundation First duplicated	Remove intro or reduce
F008	"Must never" formatting inconsistent	Standardize bullet style
F011	~~Docstring guidance too strict~~	✅ Fixed — now allows docstrings when you spend >10 min understanding
F012	~~Grover's algorithm explanation wrong~~	✅ Fixed — corrected quantum security explanation
F013	~~Deprecation mechanism unclear~~	✅ Fixed — added explicit format, 90-day period, approval requirement
F014	~~Two sections overlap~~	✅ Fixed — merged backwards-compat into "Don't add what's not needed"
F016	~~F check commands imprecise~~	✅ Fixed — made "awesome" with bash scripts, emojis, pass/fail
F017	~~Restart rule not actionable~~	✅ Fixed — added explicit user approval requirement
F018	~~Mixed concerns in section~~	✅ REVERSED — made error handling mandatory with unique codes
F019	~~Threat A slow harvest gap~~	✅ Fixed — added >12 credentials/24hr = dark-red flag
F020	~~E2 description inaccurate~~	✅ Fixed — corrected sequence description
F021	~~Test fixture security vague~~	✅ Fixed — 32 identical bytes rule added
F022	~~Audit example repetitive~~	✅ Fixed — consolidated to single location
F023	~~Violation escalation path missing~~	✅ Fixed — added permanent ban & escalation rules
F024	~~Missing period~~	✅ Fixed — already had periods
F025	~~No document versioning~~	✅ Fixed — added version header
F026	~~B1 may have false negatives~~	✅ Fixed — added LLM manual review requirement
F027	~~Missing crash dump prohibition~~	✅ Fixed — added to Threat B
F028	~~"Entire universe" overstatement~~	✅ Fixed — changed to "primary universe"
F029	~~Key table formatting~~	✅ Fixed — standardized Master row
F022	Audit example repetitive	Consolidate to one location
F024	Missing period	Add punctuation
F026	B1 may have false negatives	Enhance or add manual note
F029	Key table formatting	Standardize master_key
F030	No compiler optimization warning	Add to security section

Final Assessment

CLAVITOR-PRINCIPLES.md is an exemplary security architecture document that successfully balances prescriptive guidance with practical verifiability. The daily checklist mechanism (Part 4) is particularly strong. The document demonstrates mature threat modeling and clear understanding of the project's unique security requirements (browser-as-trust-anchor, server-as-dumb-store).

Primary concerns:

Security gaps: The missing "steal at creation" vector (F007) and lack of incident response procedures (F006) are operational blind spots.
Compliance narrowness: GDPR-only focus (F031) and missing data retention policies (F032) create enterprise adoption friction.
Localization vacuum: Complete absence of i18n/l10n guidance (F034) will block international market entry and create technical debt when localization is eventually needed.

Regarding grep vs. LLMs (F036): The user correctly notes that grep-based verification is a legacy approach when semantic analysis is available. The daily checklist should evolve from pattern-matching ("does this string exist?") to intent-analysis ("does this code path violate the principle's spirit?"). This is not a criticism of the document's current state—grep is reliable and deterministic—but a recognition that the methodology should modernize as tooling improves.

Regarding F018 (Error handling reversal): The user explicitly reversed the "No error handling for impossible situations" principle. Instead of trusting internal code, the new rule is mandatory error handling with unique codes for every branch. This is a significant shift from "trust" to "verify and log" — every if needs an else, even for theoretically unreachable code paths. When the "impossible" happens in production, you'll know exactly which impossible thing occurred (e.g., ERR-12345: L3 unavailable in decrypt).

Recommendation: Address Critical and High priority items before the next significant feature development cycle. Localization (F034) in particular should be addressed before UI work expands, as retrofitting i18n is significantly more expensive than designing it in from the start. Medium and Low priority items can be addressed incrementally during principle review sessions.

The document should be considered fit for purpose for a single-language, GDPR-focused, early-stage product with noted improvements required for enterprise and international readiness.

Report generated: 2026-04-08
Review methodology: Static analysis against correctness, conciseness, completeness, viability, security, consistency, clarity, actionability, maintainability, audibility, compliance, and localization criteria.

42 KiB Raw Blame History