fix(security/docs): SessionHandle Drop + free-swallow + recovery_qr docs + dev-c launcher (Stream A) #3

Open
alee wants to merge 0 commits from feature/arch-followup-stream-a-security-polish into main
Owner

Summary

Stream A of the architecture-review-followup. Small security-flavored polish PR addressing four findings from the 2026-05-04 audit:

  • P1.1impl Drop for SessionHandle so .free() becomes a real cleanup safety net (Rust fix). Native test exercises construct → drop → registry empty; companion wasm-bindgen-test in tests/session_drop.rs for symmetry. Recorded audit of every .free() callsite under extension/src/.
  • JS free-swallow — removed try { current.free() } catch { /* already freed */ } at extension/src/service-worker/session.ts so failures propagate. Vitest covers no-op / called-once-with-idempotency / propagating-throw.
  • P1.7crates/relicario-core/src/recovery_qr.rs brought up to the documentation density of crypto.rs / backup.rs / tar_safe.rs. Module `//!` header (3 sections), ASCII layout diagram pinned by static-assert, doc-comments on all four public items, `production_params()` fn replaced with a top-level `RECOVERY_PRODUCTION_PARAMS` const, named slice-range constants, audit-H1 length-prefix one-liner.
  • P1.8 — `tools/relay/start.sh` 4-window expansion. `DEV_C_PROMPT` discovery + manual / tmux / kitty fourth-window invocations + summary-print updates. (`queue.test.ts` assertion was already fixed in `061facd`.)

Synthesis references

  • `docs/superpowers/reviews/2026-05-04-architecture-review.md` — P1.1, P1.7, P1.8
  • `docs/superpowers/specs/2026-05-04-security-polish-design.md` — Plan A

.free() callsite audit

`grep -rn "\.free\b" extension/src/` returns exactly one functional callsite:

```
extension/src/service-worker/session.ts:32: current.free();
```

Two additional matches (`session.ts:12, 14`) are documentation references inside the new doc-block. Phase 1's `impl Drop` makes this single `.free()` call cleanup the SESSIONS registry entry directly; calling `wasm.lock(handle.value)` first is now redundant belt-and-suspenders and the SW intentionally does not.

Lifecycle audit — both paths reach `clearCurrent()`:

  • Path A (explicit user lock): `extension/src/service-worker/router/popup-only.ts:59`
  • Path B (inactivity timeout): `extension/src/service-worker/index.ts:54` (inside the `sessionTimer.onExpired(...)` callback)

Test plan

  • `cargo test` — full workspace green (relicario-core 194, relicario-cli 54, relicario-wasm 6, relicario-server verify_commit 4); zero failures
  • `cargo build -p relicario-wasm --target wasm32-unknown-unknown` — clean, ~2.3s
  • `cargo clippy -p relicario-core --all-targets --no-deps` and `-p relicario-wasm` — both clean
  • `cargo doc -p relicario-core --no-deps --document-private-items` — warning count back to baseline 6 (Phase 3's intra-doc-link warning was fixed in `f8296fa`)
  • `cd extension && bun run build` — 1 pre-existing 4.17 MiB WASM size warning, 0 errors
  • `cd tools/relay && bun test` — 5 pass / 0 fail
  • `bash -n tools/relay/start.sh` — silent (manual banner + tmux/kitty modes verified by inspection; no automated coverage exists for the launcher)

Pre-existing test failures (acknowledged)

`cd extension && bun run test` reports 17 failed / 335 passed (52 files). The 17 failures are pre-existing on the kickoff baseline `bd3d53f` (verified by checking out the baseline and running the same suite cleanly: same exact 17 reds). Stream A added 3 new passes (`session.test.ts`) and zero regressions.

Failing-test clusters (all unrelated to Stream A scope):

  • `src/service-worker/tests/devices.test.ts` — `revokeDevice removes by name` (1)
  • `src/service-worker/router/tests/router.test.ts` — `register_this_device > treats generate_device_keypair() as an object, not a JSON string` (1)
  • `src/popup/components/tests/devices.test.ts` — devices view rendering + register flow (6)
  • `src/popup/components/tests/settings.test.ts` — Sync now (3) + Display color-picker section (6)

Per PM directive (Option A), these are documented here and left to a follow-up stream.

Phase commits

  • `1e858e1` Phase 1 — `impl Drop for SessionHandle` + native test + wasm-bindgen-test
  • `03d0781` Phase 2 — JS swallow removal + vitest + audit
  • `229e483` Phase 3 — `recovery_qr.rs` documentation pass
  • `f8296fa` Phase 3 follow-up — drop intra-doc link to private const (quality-review fix)
  • `0c9387f` Phase 4 — `start.sh` fourth-window for dev-c
## Summary Stream A of the architecture-review-followup. Small security-flavored polish PR addressing four findings from the 2026-05-04 audit: - **P1.1** — `impl Drop for SessionHandle` so `.free()` becomes a real cleanup safety net (Rust fix). Native test exercises construct → drop → registry empty; companion wasm-bindgen-test in `tests/session_drop.rs` for symmetry. Recorded audit of every `.free()` callsite under `extension/src/`. - **JS free-swallow** — removed `try { current.free() } catch { /* already freed */ }` at `extension/src/service-worker/session.ts` so failures propagate. Vitest covers no-op / called-once-with-idempotency / propagating-throw. - **P1.7** — `crates/relicario-core/src/recovery_qr.rs` brought up to the documentation density of `crypto.rs` / `backup.rs` / `tar_safe.rs`. Module \`//!\` header (3 sections), ASCII layout diagram pinned by static-assert, doc-comments on all four public items, \`production_params()\` fn replaced with a top-level \`RECOVERY_PRODUCTION_PARAMS\` const, named slice-range constants, audit-H1 length-prefix one-liner. - **P1.8** — \`tools/relay/start.sh\` 4-window expansion. \`DEV_C_PROMPT\` discovery + manual / tmux / kitty fourth-window invocations + summary-print updates. (\`queue.test.ts\` assertion was already fixed in \`061facd\`.) ## Synthesis references - \`docs/superpowers/reviews/2026-05-04-architecture-review.md\` — P1.1, P1.7, P1.8 - \`docs/superpowers/specs/2026-05-04-security-polish-design.md\` — Plan A ## .free() callsite audit \`grep -rn "\\.free\\b" extension/src/\` returns exactly **one** functional callsite: \`\`\` extension/src/service-worker/session.ts:32: current.free(); \`\`\` Two additional matches (\`session.ts:12, 14\`) are documentation references inside the new doc-block. Phase 1's \`impl Drop\` makes this single \`.free()\` call cleanup the SESSIONS registry entry directly; calling \`wasm.lock(handle.value)\` first is now redundant belt-and-suspenders and the SW intentionally does not. Lifecycle audit — both paths reach \`clearCurrent()\`: - **Path A** (explicit user lock): \`extension/src/service-worker/router/popup-only.ts:59\` - **Path B** (inactivity timeout): \`extension/src/service-worker/index.ts:54\` (inside the \`sessionTimer.onExpired(...)\` callback) ## Test plan - [x] \`cargo test\` — full workspace green (relicario-core 194, relicario-cli 54, relicario-wasm 6, relicario-server verify_commit 4); zero failures - [x] \`cargo build -p relicario-wasm --target wasm32-unknown-unknown\` — clean, ~2.3s - [x] \`cargo clippy -p relicario-core --all-targets --no-deps\` and \`-p relicario-wasm\` — both clean - [x] \`cargo doc -p relicario-core --no-deps --document-private-items\` — warning count back to baseline 6 (Phase 3's intra-doc-link warning was fixed in \`f8296fa\`) - [x] \`cd extension && bun run build\` — 1 pre-existing 4.17 MiB WASM size warning, 0 errors - [x] \`cd tools/relay && bun test\` — 5 pass / 0 fail - [x] \`bash -n tools/relay/start.sh\` — silent (manual banner + tmux/kitty modes verified by inspection; no automated coverage exists for the launcher) ## Pre-existing test failures (acknowledged) \`cd extension && bun run test\` reports **17 failed / 335 passed (52 files)**. The 17 failures are pre-existing on the kickoff baseline \`bd3d53f\` (verified by checking out the baseline and running the same suite cleanly: same exact 17 reds). Stream A added 3 new passes (\`session.test.ts\`) and zero regressions. Failing-test clusters (all unrelated to Stream A scope): - \`src/service-worker/__tests__/devices.test.ts\` — \`revokeDevice removes by name\` (1) - \`src/service-worker/router/__tests__/router.test.ts\` — \`register_this_device > treats generate_device_keypair() as an object, not a JSON string\` (1) - \`src/popup/components/__tests__/devices.test.ts\` — devices view rendering + register flow (6) - \`src/popup/components/__tests__/settings.test.ts\` — Sync now (3) + Display color-picker section (6) Per PM directive (Option A), these are documented here and left to a follow-up stream. ## Phase commits - \`1e858e1\` Phase 1 — \`impl Drop for SessionHandle\` + native test + wasm-bindgen-test - \`03d0781\` Phase 2 — JS swallow removal + vitest + audit - \`229e483\` Phase 3 — \`recovery_qr.rs\` documentation pass - \`f8296fa\` Phase 3 follow-up — drop intra-doc link to private const (quality-review fix) - \`0c9387f\` Phase 4 — \`start.sh\` fourth-window for dev-c
alee added 14 commits 2026-05-09 02:32:52 +00:00
Three-reviewer architecture audit (DEV-A: core, DEV-B: cli/server/wasm,
DEV-C: extension/relay) plus PM synthesis. Lens: make the codebase
readable for a smart developer who doesn't know Rust but wants to learn
by tinkering.

Top synthesis findings (P1):
- SessionHandle has no impl Drop; .free() is a cleanup no-op (cross-cutting Rust+JS)
- cli/main.rs is a 2641-line monolith with no submodule boundaries
- setup.ts (1220 LOC) bypasses the SW and orchestrates WASM directly
- vault.ts (1027 LOC) owns shell + sidebar + list + drawer + routing
- shared/state.ts is fully any-typed
- recovery_qr.rs is undocumented vs. rest of crypto-adjacent core
- duplicated SW router helpers (storage + itemToManifestEntry)
- pure parsers (parse_month_year, base32_decode_lenient) belong in core
- 16x duplicated git invocation boilerplate with one-line errors

CLI/extension parity: 22/23 capabilities ✓; only true gap is `relicario
status` (no get_vault_status); `detach` is partial via update_item.

Also fixes tools/relay/queue.test.ts:54 to match the dev-c role
expansion already in queue.ts (was failing 1/4; now 5/5 pass).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Catches the workspace and the extension manifests up to the v0.5.x
release line (was still showing 0.2.0).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
GLYPH_TYPE_IDENTITY changed from ⌬ to ◍ so it's visually distinct from
GLYPH_DEVICES (also ⌬). Adds a CSS rule asserting [hidden] over the
.form-actions display:flex so the fullscreen sticky save bar can hide
the inner action row by attribute.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
queue.ts and server.ts now know about dev-c alongside pm/dev-a/dev-b
so the four-role coordination paradigm works end-to-end. start.sh
opens a fourth window for dev-c. call.py and call.ts are HTTP shims
that agents can use when the MCP relay tools aren't registered in
their session (the kickoff prompts reference call.py by path as a
fallback).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds the four kickoff prompts that drove the 2026-05-04 whole-codebase
architecture audit (PM + DEV-A/B/C reviewers), the planning prompt
that converts the synthesis into three implementation plans, and the
PM + DEV-A/B/C kickoff prompts for executing those plans in parallel.

Also updates the existing v0.5.1-* prompts with the relay-server
fallback section that references the new tools/relay/call.py shim.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
.gitea_env_vars is local Gitea config that shouldn't be checked in
(synthesis open decision #8). The .dev-c-content.md hidden file is a
raw subagent-output draft from the 2026-05-04 review — the canonical
notes are already in dev-c-notes.md alongside its peers.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Plan A (security & docs polish, S): SessionHandle impl Drop + JS .free()
audit + recovery_qr.rs documentation + relay launcher dev-c expansion.
Independent of B/C; ships first.

Plan B (CLI restructure, M-L): split cli/main.rs (2641 LOC) into commands/
folder + prompt.rs + parse.rs; helpers::git_run captures stderr; Vault::
after_manifest_change centralizes the groups-cache discipline; canonical
ParamsFile; batched purge; migrate parse_month_year/base32_decode_lenient/
guess_mime to relicario-core with WASM re-exports.

Plan C (extension restructure, L): typed StateHost (precondition); extract
service-worker/storage.ts; setup.ts SW migration via create_vault/
attach_vault messages + step-registry pattern; vault.ts split into
shell/sidebar/list/drawer/form-wrapper with vault_locked channel
unified through shared/state.ts; P2 cluster (timer reset, gitHost clear,
teardown helper, allSettled, MutationObserver debounce); get_vault_status
closes the relicario status parity gap.

Cross-boundary cites verified: Plan B Phase 8 WASM exports are the seam
Plan C consumes (deferred to a future plan); Plan A owns the .free() swallow
removal that Plan C respects without redoing.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
TL;DR-first guide to the PM/Senior-Dev paradigm: how to invoke
/multi-agent-kickoff, how the launcher's three modes (manual/tmux/kitty)
work, the in-memory queue + per-role inbox semantics, the call.py /
call.ts fallback shims, message kinds, conventions, and troubleshooting.

Lives next to the kickoff prompts in docs/superpowers/coordination/ so
the workflow's docs and outputs share one home.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Generated by /multi-agent-kickoff for the three architecture-review
followup plans. PM coordinates; Dev-A owns Plan A (security & docs polish,
S, ships first); Dev-B owns Plan B (CLI restructure, M-L); Dev-C owns
Plan C (extension restructure, L).

Each dev prompt forces cd into its worktree (per project memory rule),
includes the relay tool calls + Python shim fallback, scopes hard-rules
to the planning subagents' flagged judgment calls, and ships an opinionated
PR title + body template that mirrors the plan's Done criteria.

PM prompt enforces the cross-plan boundaries: A is independent; B Phase 8
WASM exports are a seam C does not consume in this train; A owns the
.free() swallow removal and Drop impl; if both B and C touch wasm.d.ts,
B sequences first.

Launcher discovers these via `ls -t coordination/*-<role>-prompt.md | head -1`
so they take precedence over previous kickoff sets.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Closes the P1.1 defense-in-depth gap: wasm-bindgen's auto-generated
.free() previously dropped the SessionHandle wrapper (a u32) without
removing the SESSIONS HashMap entry, leaving the master key and
image_secret in WASM linear memory until JS explicitly called
lock(handle). Drop now wires .free() to session::remove, and the
new native test pins the contract.

Refs: docs/superpowers/specs/2026-05-04-security-polish-design.md (Phase 1)
Refs: docs/superpowers/reviews/2026-05-04-architecture-review.md (P1.1)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Phase 1 added impl Drop for SessionHandle on the Rust side so .free()
now actually removes the SESSIONS registry entry. The JS-side
try { current.free() } catch { /* already freed */ } swallow was
hiding the fact that .free() wasn't doing the cleanup at all;
post-Phase-1 it has to go so failures surface instead of being lost.

.free() callsite audit: exactly one match under extension/src/ — the
SW session.ts line this commit edits. Lifecycle audit: clearCurrent()
is reached via (a) popup lock → router popup-only.ts and (b)
session-timer expiry → service-worker/index.ts.

Refs: docs/superpowers/specs/2026-05-04-security-polish-design.md (Phase 2)
Refs: docs/superpowers/reviews/2026-05-04-architecture-review.md (P1.1, DEV-C P2 service-worker)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Phase 3 of the security-polish series. Brings recovery_qr.rs up to
the documentation density of crypto.rs / imgsecret.rs / backup.rs /
tar_safe.rs. No runtime behaviour change: just module-level //! header
explaining the format + KDF domain separation + parameter-pinning
rationale, an ASCII diagram of the 109-byte payload layout pinned by
a static assertion, doc-comments on the four public items, and named
slice-range constants for the offset arithmetic.

production_params() is replaced with a top-level const so the "pinned,
do not change once shipped" property is visible at every use site.

Refs: docs/superpowers/specs/2026-05-04-security-polish-design.md (Phase 3)
Refs: docs/superpowers/reviews/2026-05-04-architecture-review.md (P1.7)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Phase 3 code-quality review caught that the [`RECOVERY_PRODUCTION_PARAMS`]
form in the module header introduced a new rustdoc warning (the const is
module-private, so the link only resolves under --document-private-items).
Drop the brackets so it renders as plain backticks — same visual, no
broken link, no need to widen visibility.

Refs: docs/superpowers/specs/2026-05-04-security-polish-design.md (Phase 3)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Phase 4 of the security-polish series. The relay was expanded from 3
roles (pm + dev-a + dev-b) to 4 (adds dev-c) in dd0010d, but the
launcher script never followed — it still opened only 3 windows and
the manual-mode banner said "3 new terminals". Add DEV_C_PROMPT
discovery alongside the existing PM/Dev-A/Dev-B lines, and a fourth
window/tab/terminal in each of the three modes (manual / tmux / kitty)
plus the corresponding banner and summary-print updates.

The queue.test.ts assertion update part of P1.8 already shipped in
061facd — this commit closes the launcher half.

Refs: docs/superpowers/specs/2026-05-04-security-polish-design.md (Phase 4)
Refs: docs/superpowers/reviews/2026-05-04-architecture-review.md (P1.8)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This branch is already included in the target branch. There is nothing to merge.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feature/arch-followup-stream-a-security-polish:feature/arch-followup-stream-a-security-polish
git checkout feature/arch-followup-stream-a-security-polish
Sign in to join this conversation.
No Reviewers
No Label
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: alee/relicario#3