praxis/.ciagent/REVIEW.md

Praxis v0.3 — Multi-Persona Code Review (P0 Pre-Execution + P1 Mastery Core)

Reviewer: ci-code-reviewer persona Scope: all v0.3 changes (P0 pre-execution grill amendments + P1 mastery core + VC issuance, SLICE-01 → SLICE-09) Lenses: Correctness, Testing, Security, Performance, Maintainability, Adversarial Date: 2026-08-04 Authority: PLAN.md + REQUIREMENTS.md + VERIFY.md (APPROVE_WITH_NOTES) + GRILL-v0.3.md (4 MUST) + PERSONAS.md (v0.3 roster) Test baseline: 238 passed, 10 skipped (matches VERIFY.md L2.1) Final verdict: APPROVE_WITH_NOTES — 0 P0 fixes applied; 5 P1 flags + 2 P2 notes for post-hoc review

---

Review Methodology

Each focus file from the task brief was read in full and cross-referenced against its covering tests, the grill MUST conditions, and the VERIFY.md findings. The 4 grill MUST conditions were independently re-verified in code (not just trusting VERIFY.md). SQL was audited for parameterization. The IRT and scenario-selection code were checked for the claimed O(1) / O(n) complexity. The VC crypto path was checked for argument-order correctness in PyNaCl calls (VerifyKey.verify(smessage, signature) — confirmed correct at issuer.py:156).

---

Per-Persona Findings

1. Correctness (lead-developer + backend-engineer lens)

server/mastery/mastery_score.py — gate logic

• Gate logic (D-032): check_gate at mastery_score.py:78-86 implements distinct_passed_count >= 3 AND path_score >= 3.5 — correct. Constants _GATE_REQUIRED_DISTINCT = 3 and _GATE_REQUIRED_SCORE = 3.5 are module-level (single source of truth).
• Conjunctive floor: compute_scenario_score at mastery_score.py:48-54 enforces every criterion ≥ 2 (or the criterion's conjunctive_floor if higher) AND mean ≥ 3.0. Professionalism floor (≥2) is honored via rubric_schema.RubricCriterion.conjunctive_floor.
• Determinism: Pure function, no I/O, round(total, 6) for stable float comparison. Verified by test_mastery_integration.py::test_mastery_flow_is_deterministic.
• Verdict: ✅ correct.

server/mastery/irt.py — theta update + cold-start

• P_success: 1 / (1 + exp(-(θ−b))) — standard 1PL/Rasch logistic. Correct.
• update_theta: Kalman-like Gaussian-approximation update at irt.py:38-55:
  • prior_precision = 1/σ², info = P(1−P) (Fisher information for Bernoulli), new_precision = prior_precision + info, new_σ² = 1/new_precision, new_θ = θ + new_σ² × (outcome − P).
  • This is the standard 1PL Bayesian update. Correct. σ² shrinks monotonically as observations accumulate.
• Cold-start (R-IRT-01): select_scenario at irt.py:57-90 falls back to difficulty-based matching when observations < 5. Target difficulty = round(θ + logit(target_p)) clamped to [1,5]. Sound.
• Verdict: ✅ correct. O(1) per update_theta call (verified — single math computation, no loops).

server/vc/issuer.py — JCS + Ed25519

• JCS canonicalization: canonicaljson.encode_canonical_json at issuer.py:103-104 — RFC 8785-aligned, deterministic. Tested by test_vc_issuer.py::test_jcs_canonicalization_determinism + test_jcs_key_ordering_is_sorted.
• eddsa-jcs-2022 proof: _compute_hash_data at issuer.py:118-125 = SHA256(canonical_proof) || SHA256(canonical_doc). Signed with signing_key.sign(hash_data).signature (detached signature). Correct per the cryptosuite spec.
• verify_proof: at issuer.py:141-159 reconstructs the same hash and calls verify_key.verify(hash_data, sig). PyNaCl's VerifyKey.verify(smessage, signature) arg order is correct (verified against the library signature: verify(self, smessage, signature=None)). Raises BadSignatureError on mismatch → caught → returns False.
• Tamper detection: re-canonicalizes the unsecured doc (without proof) + proof options (without proofValue) — any byte flip in the payload changes the canonical bytes → hash mismatch → verify fails. Tested by test_vc_issuer.py::test_tamper_detection_flipped_byte_fails + test_vc_integration.py::test_tamper_payload_verify_fails.
• Verdict: ✅ correct. 19 VC tests pass.

server/vc/status_list.py — bitstring revocation

• set/get_status: bit-twiddling at status_list.py:35-52 is correct (byte_pos = idx >> 3, bit_pos = idx & 7).
• get_status bounds check: status_list.py:50 returns False if byte_pos >= len(buf) — defensive, good.
• allocate_slot: O(n) scan over the allocation bitstring at status_list.py:54-72. For _MIN_BITS = 131072 (16KB), this is fine in practice (pilot scale). Expansion path (doubling) at status_list.py:66-72 is correct.
• REQ-NFR-VC-02 (revocation latency): status list fetched from SQLite on every verify call (verification.py:47-48) — no cache. Confirmed.
• Verdict: ✅ correct.

server/session_recorder.py — mastery flow wiring

• Sequencing: run_mastery_flow at session_recorder.py:154-311 correctly sequences: extract → score → IRT update → progress upsert → gate event record → VC issuance.
• scoring_inconclusive path: at session_recorder.py:185-192 short-circuits all downstream steps and surfaces retry_advised: True. No score, no gate event, no progress change, no IRT update. Grill Axis 4 MUST #3 satisfied. Tested by test_mastery_integration.py::test_mastery_flow_scoring_inconclusive_no_score_no_gate_event.
• VC issuance: session_recorder.py:276-293 — path_complete = gate_open and new_week >= 6; on True, lazy-imports server.vc.issuer.issue_credential. ImportError swallowed (SLICE-09-independent ship); Exception logged (issuance failure doesn't crash mastery flow). Grill Axis 8 MUST satisfied.
• Outer guard: _run_mastery_flow_guarded at session_recorder.py:148-152 wraps the whole flow in try/except — mastery failure never crashes session end. Good isolation.
• P1 finding (P1-4, carried from VERIFY.md): compute_path_score at session_recorder.py:209-211 uses only the current session's score, not the cumulative mean over all passing sessions. The gate still works (distinct-count is the primary gate; the score threshold is secondary and the current-session score is a reasonable proxy). The in-code comment at session_recorder.py:212-213 acknowledges this. Flag for v0.4: fold in prior passing scores from mastery_progress.scenarios_passed_json.
• Verdict: ✅ correct (with P1-4 noted).

2. Testing (backend-engineer + lead-developer lens)

Grill MUST conditions — independently re-verified in code

#	Grill MUST	Test evidence (verified in code)	Verdict
Axis 3 #1	VC interop test exists	tests/test_vc_interop.py (153 LOC): JCS canonicalization is valid JSON, signature is 64-byte base64, W3C VC 2.0 schema conformance (@context, type, issuer, validFrom/validUntil, credentialSubject, credentialTier, proof fields). Staging-gated test_full_w3c_vc_interop_validation for extended self-check.	✅ covered (P1-3: live external-verifier run is post-hoc)
Axis 3 #2	Key-rotation drill test exists	tests/test_vc_key_rotation_drill.py::test_key_rotation_operational_drill — issues N with key A, rotates to B, issues M with B, verifies all, revokes one each. Plus test_vc_integration.py::test_key_rotation_old_vc_still_verifies.	✅ covered
Axis 4 #1	credentialTier: "formative" in payload	test_vc_issuer.py::test_credential_tier_is_formative_in_payload asserts both payload-level and credentialSubject-level. test_vc_integration.py::test_issue_and_verify_valid asserts response credentialTier == "formative".	✅ covered
Axis 4 #3	scoring_inconclusive fallback	test_mastery_integration.py::test_mastery_flow_scoring_inconclusive_no_score_no_gate_event — 3 bad-quote responses → inconclusive, no ability/progress/gate-event rows. test_evidence_extractor_integration.py covers the extractor-level inconclusive path.	✅ covered

4/4 grill MUST conditions tested. Matches VERIFY.md L2.5.

Untested critical paths

• P1 gap (new finding): HTTP route wiring untested. The /vc/verify/{credential_id} route at server/__main__.py:124-136 is NOT tested via FastAPI TestClient / ASGI transport. The underlying verify_credential() function is well-tested (test_vc_integration.py, test_vc_key_rotation_drill.py), but the route registration, 404-on-not-found behavior, and the _store.init() call in the route handler are untested. A route-registration regression (e.g., route mounted after StaticFiles catch-all at __main__.py:146, shadowing the API route) would not be caught. Recommended: add one httpx.AsyncClient + ASGI transport test that hits GET /vc/verify/<unknown> → 404 and GET /vc/verify/<valid> → 200 with the formative tier.
• P2 gap: status list expansion path untested. BitstringStatusList.allocate_slot at status_list.py:66-72 doubles the bitstring when all slots are full. This expansion branch is not exercised by any test (pilot scale never fills 131072 slots). Low risk, but worth a unit test that forces expansion with a tiny _MIN_BITS override.
• P2 gap: get_status on uninitialized list. If get_status(idx) is called before any set_status or allocate_slot, _load initializes an all-zero bitstring → returns False. This is correct behavior but untested explicitly.

3. Security (security-engineer lens)

server/vc/verification.py — public endpoint injection

• credential_id injection: The credential_id path parameter at __main__.py:125 flows to store.get_credential(cred_id) at store.py:372-381, which uses a parameterized query (WHERE id = ?). No SQL injection. FastAPI does not apply a regex constraint on the path param, but SQLite handles arbitrary strings safely (returns None for non-matching ids → 404).
• No PII leak: verification.py:53-73 returns only {valid, status, issuer, credential{id,type,validFrom,validUntil}, mastery{skill,level,path,rubricScore,scenariosPassed,completedWeeks}, credentialTier, verifiedAt}. credentialSubject.id is urn:uuid:<learner_ref> (opaque). No email/name/phone/address. Confirmed.
• Verdict: ✅ secure (no injection vector).

server/mastery/evidence_extractor.py — LLM prompt injection

• Vector: transcript turns injected verbatim into the user message at evidence_extractor.py:86. A malicious learner could attempt prompt injection in spoken turns ("ignore previous instructions...").
• Mitigations (all verified in code):
  1. System prompt is fixed and authoritative (evidence_extractor.py:78-84).
  2. Output is JSON-schema-validated (_parse_evidence_json at evidence_extractor.py:96-119 rejects non-list, unknown criterion_id, schema-invalid items).
  3. Fuzzy-match gate at evidence_extractor.py:180 — an injected "quote" that isn't in the transcript is rejected. This is the strongest mitigation: even if the LLM obeys an injection, the forged quote must actually appear in the learner's spoken turns to pass.
• Verdict: ✅ secure. The fuzzy-match gate blocks the highest-impact injection (faking evidence to boost a score).

db/store.py — SQL injection in new async methods

• Audit: all 14 v0.3 async methods (get_ability, upsert_ability, get_progress, upsert_progress, record_gate_event, list_gate_events, init_issuer_key, get_active_signing_key_row, get_public_key_row, set_issuer_key_superseded, insert_credential, get_credential, set_credential_status, get_status_list, upsert_status_list) use ? placeholder parameterization. No f-string SQL, no string concatenation in queries. Grep for f".*SELECT|f".*INSERT|f".*UPDATE|f".*WHERE in server/ and db/ returned zero matches.
• Verdict: ✅ no SQL injection.

4. Performance (backend-engineer lens)

server/mastery/irt.py — O(1) verification

• update_theta: 1 division, 1 multiplication, 1 exp, 1 subtraction — O(1). Confirmed. REQ-NFR-IRT-01 (<100ms) trivially satisfied (sub-microsecond).
• P_success: O(1).
• select_scenario cold-start: O(n) over path scenarios (n ≈ 6 in v0.3). Fine.
• Verdict: ✅ O(1) per update as required.

server/scenarios/library.py — select_for_theta O(n) verification

• select_for_theta at library.py:143-167: single for e in entries loop with abs(e.difficulty - target_b) — O(n), NOT O(n²). No nested loops. list_by_path at library.py:126-133 is also O(n) (one pass, though it calls self.get(e.id) per entry which is cached after first load).
• Minor note (P2): list_by_path at library.py:129-130 calls self.get(e.id) (which loads + caches the scenario YAML) for every entry just to read s.path. For n=6 this is negligible, but for a large library this could be optimized by storing path in the IndexEntry itself (the manifest already has it). Not a v0.3 concern.
• Verdict: ✅ O(n), not O(n²).

5. Maintainability (lead-developer lens)

server/mastery/ module organization

• Clean separation: rubric_schema.py (model), rubric_loader.py (I/O), rubric_scorer.py (deterministic scoring), evidence_extractor.py (LLM extraction), mastery_score.py (gate logic), irt.py (IRT engine). Each module is single-responsibility, <120 LOC, typed, with __all__ exports.
• Verdict: ✅ well-organized.

server/vc/ module organization

• Clean separation: issuer.py (payload + signing + issuance), issuer_keys.py (key management + encryption), status_list.py (revocation), verification.py (public verify + revoke). CREDENTIAL_TIER = "formative" is a module-level constant in issuer.py:34 — single source of truth.
• Minor coupling smell (P2): issuer_keys._fetch_private_key_enc at issuer_keys.py:92-99 reaches into store._connect() (a private method) instead of using a public store.get_private_key_enc(key_id) method. This couples issuer_keys to PraxisStore's internal connection management. Not a bug, but a small abstraction leak. Recommended: add a public store.get_issuer_key_row(key_id) method that returns the full row.
• Verdict: ✅ well-organized (with P2 coupling note).

6. Adversarial (security-engineer + red-team lens)

/vc/verify public endpoint — rate-limiting

• P1 (carried from VERIFY.md P1-1): Endpoint is public + unauthenticated (D-043, by design — third-party verifiers must reach it). No rate limiting in v0.3. A flood of verify requests would each hit SQLite (get_credential + get_public_key_row + get_status_list = 3 queries per verify). Acceptable for pilot (single-deploy, low traffic). Flag for v0.4: add slowapi rate-limit (60 req/min/IP) on /vc/verify/*.

Issuer key management — PRAXIS_VC_ISSUER_KEY fallback

• P1 (carried from VERIFY.md P1-2): _load_root_key at issuer_keys.py:25-31 silently falls back to nacl.utils.random(...) if PRAXIS_VC_ISSUER_KEY is unset. On a deploy where the env var is missing:

  • First boot: init_issuer_key generates a key, encrypts with the random root key, stores ciphertext. Issuance works within this process.
  • Restart: new random root key → get_active_signing_key decrypts the old ciphertext with the new key → nacl.secret.SecretBox.decrypt raises CryptoError → issuance fails with a confusing error.
  • Old VCs still verify (public key is stored unencrypted) — no data loss, no security hole.
  • This is a P1 operational footgun, not a P0. The failure mode is "new issuance breaks after restart" not "credentials become invalid" or "keys leak." Recommended v0.4 fix: fail fast at startup if PRAXIS_VC_ISSUER_KEY is unset (raise RuntimeError), or persist the root key to a secrets manager on first init.

• No other adversarial vectors found. Issuance is server-side only (learner code never calls issue_credential directly — only session_recorder.run_mastery_flow after gate-open). Key rotation marks old keys superseded, not deleted — old VCs verify against archived public keys. Tested by test_vc_key_rotation_drill.py.

---

P0 Fixes Applied

None. No P0 (critical bug / security hole) fixes were required. The codebase passes all 238 tests, all 4 grill MUST conditions are satisfied and tested, all SQL is parameterized, the VC crypto path is correct (PyNaCl arg order verified), the IRT and gate logic are mathematically sound, and the scoring_inconclusive fallback correctly avoids silent fail-to-zero.

The two issues flagged as P1 in VERIFY.md (rate-limiting, root-key fallback) were re-confirmed as P1, not P0:

• Rate-limiting: acceptable for pilot scale, no security hole (public verify is read-only, no PII leak).
• Root-key fallback: operational footgun, not a security hole (old VCs remain valid; only new issuance breaks after restart with missing env).

---

P1+ Flags (post-hoc review — non-blocking for v0.1.4 ship)

ID	Flag	Severity	Location	Recommended action	Origin
P1-1	/vc/verify public + unauthenticated, no rate limiting → DoS vector (3 SQLite queries per verify)	P1	server/vc/verification.py, server/__main__.py:124	v0.4: add slowapi rate-limit (60 req/min/IP) on /vc/verify/*. Acceptable for pilot.	VERIFY.md P1-1 (re-confirmed)
P1-2	_load_root_key() silent random fallback when PRAXIS_VC_ISSUER_KEY unset → cross-restart issuance breaks silently (old VCs still verify)	P1	server/vc/issuer_keys.py:25-31	v0.4: fail fast at startup if env unset (raise RuntimeError), or persist root key to secrets manager.	VERIFY.md P1-2 (re-confirmed)
P1-3	VC interop test validates W3C schema + crypto format but does not invoke a live external W3C verifier (grill Axis 3 MUST #1 strictest bar)	P1	tests/test_vc_interop.py:128-153	Before v0.3 milestone ship (v0.1.5): schedule staging run with @digitalcredentials/vc or digitalbazaar/vc-verifier. Schema + format validation is sufficient for v0.1.4 patch ship.	VERIFY.md P1-3 (re-confirmed)
P1-4	compute_path_score uses only current session's score, not cumulative mean over all passing sessions	P1	server/session_recorder.py:209-211	v0.4: fold in prior passing scores from mastery_progress.scenarios_passed_json. Gate still works (distinct-count is primary).	VERIFY.md P1-4 (re-confirmed)
P1-5 (new)	HTTP route /vc/verify/{credential_id} wiring untested (no TestClient/ASGI test) — route registration, 404 behavior, _store.init() in handler not exercised	P1	server/__main__.py:124-136, tests/	v0.4 (or before v0.1.5): add one httpx.AsyncClient + ASGI transport test: GET /vc/verify/<unknown> → 404, GET /vc/verify/<valid> → 200 with credentialTier: formative. Catches route-shadowing regressions (StaticFiles catch-all at __main__.py:146 could shadow API routes if ordering changes).	New finding
P2-1	No max-transcript-length guard in evidence extraction → long sessions could exceed model context window	P2	server/mastery/evidence_extractor.py:75-93	Future: truncation or chunking for >30-min sessions. Not a v0.3 blocker.	VERIFY.md P2-1 (carried)
P2-2 (new)	BitstringStatusList.allocate_slot expansion branch (doubling when full) untested; issuer_keys._fetch_private_key_enc reaches into store._connect() (private method) — abstraction leak	P2	server/vc/status_list.py:66-72, server/vc/issuer_keys.py:92-99	Future: add a forced-expansion unit test with tiny _MIN_BITS; add a public store.get_issuer_key_row(key_id) method to remove the private-method coupling.	New finding

---

Final Verdict: APPROVE_WITH_NOTES

v0.3 (P0 + P1) is verified across all 6 persona lenses:

• ✅ Correctness: gate logic (D-032 ≥3 distinct AND ≥3.5), IRT Kalman update, JCS+Ed25519 signing/verification, status list bit-twiddling, mastery flow wiring, scoring_inconclusive short-circuit — all correct. PyNaCl VerifyKey.verify(smessage, signature) arg order confirmed.
• ✅ Testing: 238 passed / 10 skipped. 4/4 grill MUST conditions independently re-verified as tested. P1-5 flags the untested HTTP route wiring (function-level tests are sufficient for v0.1.4).
• ✅ Security: no SQL injection (all 14 new async methods parameterized), no PII leak on /vc/verify, LLM prompt injection mitigated by fuzzy-match gate. P1-1 (rate-limit) and P1-2 (root-key fallback) re-confirmed as P1, not P0.
• ✅ Performance: irt.update_theta is O(1); library.select_for_theta is O(n) (not O(n²)); status_list.allocate_slot is O(n) over 131072 bits (acceptable).
• ✅ Maintainability: server/mastery/ and server/vc/ are cleanly separated, single-responsibility, typed, <120 LOC per module. Minor P2 coupling note on issuer_keys._fetch_private_key_enc.
• ✅ Adversarial: issuance is server-side only (gated by mastery flow); key rotation archives (not deletes) old keys; public verify is read-only with no PII. P1-1/P1-2 are the only attack-surface flags, both acceptable for pilot.

0 P0 fixes applied. No critical bugs or security holes found. The 5 P1 flags + 2 P2 notes are non-blocking and tracked for v0.4 / the v0.1.5 milestone ship. The v0.1.4 patch ship is unblocked.

Recommended next steps:

1. Proceed to P2 (final audit + milestone ship).
2. Before v0.1.5: schedule the live external-verifier interop run (P1-3) + add the HTTP route test (P1-5).
3. v0.4: address P1-1 (rate-limit), P1-2 (root-key fail-fast), P1-4 (path-score cumulative mean).

---

---ci---
phase: 2
milestone: v0.3
status: review
requirements_covered:
  - REQ-MAST-01
  - REQ-MAST-02
  - REQ-MAST-03
  - REQ-MAST-04
  - REQ-SCEN-02
  - REQ-SCEN-03
  - REQ-SCEN-04
  - REQ-PATH-02
  - REQ-NFR-MAST-01
  - REQ-NFR-MAST-02
  - REQ-NFR-VC-01
  - REQ-NFR-VC-02
  - REQ-NFR-IRT-01
requirements_total: 13
requirements_covered_count: 13
requirements_pending_count: 0
grill_must_satisfied: 4
grill_must_total: 4
grill_must_tested: 4
p0_fixes_applied: 0
p1_flags: 5
p2_notes: 2
verdict: APPROVE_WITH_NOTES
personas_run:
  - correctness
  - testing
  - security
  - performance
  - maintainability
  - adversarial
tests_passed: 238
tests_skipped: 10
---