Skip to content

Commit fe29981

Browse files
authored
Refactor/debt pass 1 (#137)
* Add refactor safety-net tests * Fix AGENTS face-lock reference * Centralize container format constants * Guard crypto self-test flag * Label object recognition evaluation modules * Centralize state directory setup * Cache TUI vessel entropy inspection * Annotate vault core API * Log silent local failure paths * Document threaded runtime contracts * Annotate AI gate and enable mypy * Add refactor follow-up proposals * Add refactor completion report
1 parent a902b11 commit fe29981

25 files changed

Lines changed: 1085 additions & 150 deletions

AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ Use this context for changes involving `main.py`, command syntax, CLI output, co
199199
Use this context for changes involving ORB matching, camera capture, object cue registration, match ambiguity, stable multi-frame matching, face template enrollment, or UI lock behavior:
200200

201201
- `src/phasmid/ai_gate.py`
202-
- `src/phasmid/face_lock.py`
202+
- `src/phasmid/web_server.py` (UI face-lock session handling)
203203
- `docs/SPECIFICATION.md`, especially section 12
204204
- `docs/THREAT_MODEL.md`
205205
- related tests in `tests/`

docs/archive/REFACTOR_PROPOSALS.md

Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,239 @@
1+
# Refactor Proposals
2+
3+
This document records proposal-only items from the June 2026 refactor pass.
4+
No code changes are authorized by this document. Items that affect stored data,
5+
security gates, operator-visible behavior, or deployment posture require owner
6+
approval and matching specification or threat-model updates before
7+
implementation.
8+
9+
## D2: `kdf_subkeys.py` Staged v4 Design
10+
11+
Motivation: `kdf_subkeys.py` is not wired into the v3 runtime path, but its
12+
presence can be mistaken for an active container-format migration.
13+
14+
Design: Choose one of three owner-approved outcomes: keep it with an explicit
15+
"design artifact, not wired" module header; move it under an archive namespace;
16+
or remove it together with its direct tests and retention-matrix entries.
17+
18+
Migration steps: If kept, add the header and a short note in the retention
19+
matrix. If archived or removed, update imports, `tests/test_kdf_subkeys.py`,
20+
`tests/test_terminology.py`, and `tests/TEST_RETENTION_MATRIX.md` in the same
21+
change.
22+
23+
Test impact: Run terminology tests, retention-matrix checks, full default and
24+
optional suites, and any claim-coverage checks that mention future key
25+
derivation work.
26+
27+
Compatibility risk: Low if only documented. High if implemented as a format
28+
change, because any v4 subkey migration would affect stored local data and
29+
container compatibility.
30+
31+
Gate: Q1.
32+
33+
## D3: Dead Recognition Module Disposition
34+
35+
Motivation: `face_sample_matcher.py` and `object_cue_policy_gate.py` appear
36+
unused by runtime imports, while similarly named modules make the active object
37+
cue path harder to identify.
38+
39+
Design: After owner approval, either delete the unused modules or retain them
40+
with clear "not wired into runtime" headers and retention-matrix rationale.
41+
Keep `lightweight_object_matcher.py` and `recognition_benchmark.py` as
42+
evaluation components.
43+
44+
Migration steps: For deletion, remove the module files, remove or relocate
45+
their tests, update `tests/test_terminology.py`, and update
46+
`tests/TEST_RETENTION_MATRIX.md`. For retention, add headers and matrix notes.
47+
48+
Test impact: Run optional recognition tests, terminology tests, scenario tests,
49+
and full suites. Also grep importers before deletion.
50+
51+
Compatibility risk: Low for documentation. Medium for deletion because
52+
external experiments may import these modules even if runtime code does not.
53+
54+
Gate: Q2.
55+
56+
## D7: `web_server.py` Decomposition
57+
58+
Motivation: `web_server.py` combines app setup, global mutable state, public
59+
routes, operator routes, maintenance routes, emergency routes, metadata
60+
handling, and security gates in one large module.
61+
62+
Design: Introduce an explicit `AppState` container and split routes into
63+
routers by surface: public store/retrieve, operator pages, maintenance,
64+
emergency, and metadata. Keep route paths, request shapes, response bodies,
65+
headers, and neutral filenames unchanged.
66+
67+
Migration steps: First add `AppState` without moving routes. Then move one
68+
router at a time behind compatibility imports. Preserve module-level aliases
69+
used by tests until all tests are migrated. Avoid combining this with UI text
70+
or security-gate changes.
71+
72+
Test impact: `tests/test_web_server.py`, webui leakage tests, restricted-action
73+
tests, Field Mode scenario tests, and full default/optional suites need to run
74+
after every router move.
75+
76+
Compatibility risk: High. This surface is adjacent to Web mutation tokens,
77+
restricted confirmation, hidden routes, Field Mode visibility, response
78+
headers, and capture-visible strings.
79+
80+
Gate: Owner review required before implementation.
81+
82+
## D11: Audit Durability And Chain-State Scaling
83+
84+
Motivation: audit appends currently do not fsync per record or use
85+
inter-process locking, and the next sequence state is derived by reading the
86+
whole log.
87+
88+
Design: Evaluate three options: accept current append semantics and document
89+
the local durability limit; add optional fsync controlled by configuration; or
90+
add advisory file locking plus a compact chain-state sidecar. Any option must
91+
preserve existing record shape and `verify_log_integrity()` compatibility.
92+
93+
Migration steps: Add characterization tests first. If adding a sidecar, make it
94+
rebuildable from `events.log` and never required to verify older logs. If
95+
adding fsync, measure write latency on target hardware before enabling it by
96+
default.
97+
98+
Test impact: audit record-shape tests, multi-record chain verification,
99+
tamper-detection tests, operations export tests, and Pi Zero 2 W performance
100+
validation.
101+
102+
Compatibility risk: Medium for optional fsync; high for sidecar or lock
103+
semantics if they change field-device behavior.
104+
105+
Gate: Q5.
106+
107+
## D14: `operations.py` Split
108+
109+
Motivation: `operations.py` mixes state verification, audit verification,
110+
redacted export, and doctor-like status routines that are reachable from CLI
111+
commands.
112+
113+
Design: Split into `state_ops.py`, `audit_ops.py`, and `export_ops.py`, with
114+
`operations.py` retained as a compatibility shim that re-exports public
115+
functions.
116+
117+
Migration steps: Add new modules with copied functions and no behavior change.
118+
Update internal imports one call site at a time. Keep CLI imports stable until
119+
tests prove equivalence. Remove the shim only in a later owner-approved pass.
120+
121+
Test impact: `tests/test_operations.py`, CLI tests for verification/export
122+
commands, audit tests, state-store tests, and claim coverage.
123+
124+
Compatibility risk: Medium because shell-visible command behavior and
125+
diagnostic wording are capture-visible surfaces.
126+
127+
Gate: Owner review required before implementation.
128+
129+
## D17: `webui_service.py` Lifecycle Redesign
130+
131+
Motivation: WebUI lifecycle management combines process-group termination,
132+
pid-file probing, socket checks, `lsof` fallback, and a daemon timer. Repeated
133+
probes may be expensive and failures are difficult to diagnose.
134+
135+
Design: Introduce a small lifecycle state object that records the last probe
136+
time, process handle, pid-file result, socket result, and startup failure
137+
details. Cache probe results briefly for TUI ticks while keeping explicit
138+
start/stop checks fresh.
139+
140+
Migration steps: Add tests around current start/stop/pid-file behavior. Add
141+
state recording without changing decisions. Then add short-lived caching only
142+
for passive status reads.
143+
144+
Test impact: TUI service tests, WebUI start/stop tests, pid-file cleanup tests,
145+
and manual smoke testing of local WebUI startup.
146+
147+
Compatibility risk: Medium to high because lifecycle behavior is
148+
operator-facing and may affect cleanup after restricted local actions.
149+
150+
Gate: Owner review required before implementation.
151+
152+
## D18: `FileAttemptLimiter` Concurrent Update Locking
153+
154+
Motivation: the file-backed attempt limiter uses read-modify-write state that
155+
can lose updates under concurrent processes.
156+
157+
Design: Consider advisory `flock` around the read/update/write section, with a
158+
fallback path on platforms without `flock`. Keep existing lockout thresholds,
159+
timestamps, response shape, and neutral failure behavior unchanged.
160+
161+
Migration steps: Add a concurrency characterization test with mocked time and a
162+
temporary state file. Implement locking in the narrow file-update section.
163+
Document platform fallback behavior if needed.
164+
165+
Test impact: attempt-limiter tests, WebUI restricted-action tests, CLI access
166+
tests, and scenario tests for neutral failure behavior.
167+
168+
Compatibility risk: Medium because this is a security control and any timing or
169+
locking change could affect deployed operator workflows.
170+
171+
Gate: Owner review required before implementation.
172+
173+
## D19: `LocalStateCipher` Key Derivation
174+
175+
Motivation: `LocalStateCipher` derives local state encryption keys using
176+
SHA-256 over configured or local key material, while vault payload keys use
177+
Argon2id.
178+
179+
Design: Do not change code in a refactor pass. If strengthening is desired,
180+
design a versioned local-state envelope with explicit migration and rollback
181+
behavior. The old format must remain readable until migration is complete.
182+
183+
Migration steps: Define a new state blob version, add compatibility readers,
184+
write new blobs only after explicit migration, and document recovery behavior
185+
if migration is interrupted.
186+
187+
Test impact: local-state crypto tests, object cue store tests, nonce tests,
188+
state-store tests, and field-device migration tests using pre-existing state
189+
fixtures.
190+
191+
Compatibility risk: High. Changing this directly can make existing `.state`
192+
blobs unreadable and would alter local recovery requirements.
193+
194+
Gate: Owner review plus threat-model/specification update required.
195+
196+
## D20: `approval_flow.py` And `roles.py` Integration Or Archive
197+
198+
Motivation: approval and role modules are implemented and tested, but runtime
199+
integration is not active beyond configuration flags. This can imply a
200+
partially delivered authorization feature.
201+
202+
Design: Owner must choose one path: integrate behind an explicit documented
203+
operator workflow; retain as staged work with clear module headers; or archive
204+
until a future issue. Integration must not weaken existing restricted
205+
confirmation or typed confirmation requirements.
206+
207+
Migration steps: For integration, update specification, threat model, CLI/TUI
208+
flows, and tests before enabling behavior. For retention/archive, add headers
209+
and retention-matrix notes only.
210+
211+
Test impact: approval-flow tests, roles tests, restricted-action tests, CLI/TUI
212+
tests, and WebUI mutation-token tests.
213+
214+
Compatibility risk: High if integrated, because this is authorization-adjacent
215+
behavior. Low if only documented as staged work.
216+
217+
Gate: Q4.
218+
219+
## D21: `_ui_unlocked()` And `_guard_page()`
220+
221+
Motivation: `_ui_unlocked()` currently returns `True`, making `_guard_page()`
222+
effectively non-enforcing while documentation still references UI face-lock
223+
session behavior.
224+
225+
Design: Treat this as a product decision, not a refactor. If intentionally
226+
disabled, simplify and document the current posture. If a regression, fix under
227+
a bug issue with tests for lock, unlock, timeout, and capture-visible wording.
228+
229+
Migration steps: Reconcile `docs/THREAT_MODEL.md`, `docs/SPECIFICATION.md`,
230+
AGENTS guidance, WebUI tests, and any operator documentation. Then implement
231+
the chosen behavior in a single-purpose change.
232+
233+
Test impact: WebUI route guard tests, face-lock session tests if re-enabled,
234+
Field Mode visibility tests, and source-leakage tests.
235+
236+
Compatibility risk: High because UI face lock is operator-visible and can be
237+
mistaken for vault encryption if documented poorly.
238+
239+
Gate: Q3.

0 commit comments

Comments
 (0)