59 lines
4.5 KiB
Markdown
59 lines
4.5 KiB
Markdown
# 📋 Code Review Report — OpenCode Adapter Iteration-3 (OPENCODE_PERMISSION fix) (Job 8d04f9f3)
|
||
|
||
- **Reviewer**: `reviewer-hermes-01` (role: reviewer) — reviewer chain: plan Rev.2/Rev.3 (9c6b3390, 90aa4208), implementation NOT PASS (08041d33), fix PASS (8cc8f403), quoting regression NOT PASS (6a3d73d4)
|
||
- **Reviewed diff**: same 23-file OpenCode integration; the delta from the prior iteration is precisely the blocking fix — the `OPENCODE_PERMISSION` export rewritten to the empty-guard form — plus a behavior-level test replacing the prior form-assertion.
|
||
- **Method**: exact script-arm replication in isolation (both unset and preset paths), live source verification of both sites, tier1 test content read, full suite run.
|
||
|
||
---
|
||
|
||
## 1. Prior Blocking Defect — Fix Verification
|
||
|
||
**Prior finding (6a3d73d4)**: `export OPENCODE_PERMISSION="${OPENCODE_PERMISSION:-{\"*\":\"allow\"}}"` corrupted user-supplied values — a preset `{"*":"deny"}` became `{"*":"deny"}}` (invalid JSON) on both create and resume paths, because the `\"` escapes inside an unquoted `${:-}` default are stripped when the variable is set, and the default's closing `}` collides with the JSON's.
|
||
|
||
**Fix verified in live source** — both sites now use the exact empty-guard I specified:
|
||
- create_session.sh:216–219 (inside the `opencode)` spawn arm): `if [ -z "${OPENCODE_PERMISSION:-}" ]; then export OPENCODE_PERMISSION='{"*":"allow"}'; fi`
|
||
- resume_session.sh:129–133 (pre-`new-session`, agent-gated): identical guard
|
||
|
||
**Isolation replication of the exact fixed arm** (both paths):
|
||
```
|
||
unset → '{"*":"allow"}' → json.loads VALID
|
||
preset → '{"*":"deny"}' → json.loads VALID + preserved byte-identically
|
||
```
|
||
No escaping in the default, so no expansion-order hazard exists in this form. The user-override contract promised by `.mam.env.example` and the brief is now honored.
|
||
|
||
## 2. Test Upgrade — Form-Assertion Replaced by Behavior Test
|
||
|
||
Prior iteration's tier1 test asserted the export line verbatim (a form-assertion that green-lit the broken quoting). The new test (test_tier1_unit.py L991–1028) exercises **both paths behaviorally**:
|
||
- unset path: env scrubbed of `OPENCODE_PERMISSION` → guard+export → output parses as `{"*": "allow"}`
|
||
- preset path: `OPENCODE_PERMISSION='{"*":"deny"}'` injected → guard+export → output parses as `{"*": "deny"}` — **proving the user value passes through intact**
|
||
|
||
Run for both create and resume script arms. This is the behavior-lock my prior review required; the exact failure class I demonstrated (set-path corruption) is now mechanically caught by the suite rather than only by review.
|
||
|
||
## 3. Carry-Over State — Re-Confirmed Unchanged (no regression in the fix round)
|
||
|
||
| Prior-verified item | Status |
|
||
|---|---|
|
||
| Real-schema probe (`directory`/`time_created` ms) in adapter + drift-C | ✅ unchanged from the 8cc8f403-verified state (probe lines L118–119/145/213–223; reconcile L379–394) |
|
||
| `_resolve_db` CLI invocation + sandbox-safe env (`HOME`/`HOME_DIR`/`XDG_DATA_HOME` overrides) | ✅ unchanged |
|
||
| run_loop resolver arms (opencode + grok) | ✅ unchanged |
|
||
| status.sh `resume_on_disk` dispatch (both functions, 5 agents) | ✅ unchanged |
|
||
| conftest real-schema seeding + adaptive seeder | ✅ unchanged |
|
||
| `test_opencode_real_schema_directory_and_time_created_ms` | ✅ present |
|
||
| `test_opencode_resolve_db_cli_path` | ✅ present |
|
||
| update_yaml_resumed.sh opencode elif | ✅ present |
|
||
| paste-skip membership (lib.sh:2125) | ⚠️ still deferred to live TUI smoke test — unchanged posture, correctly gated per plan §4 |
|
||
|
||
## 4. Verification Summary
|
||
|
||
| Check | Result |
|
||
|---|---|
|
||
| Full suite | ✅ **447 passed, 0 failures** (734.0 s) |
|
||
| Prior blocking defect (quoting corruption) | ✅ fixed at both sites; both paths proven in isolation and by behavior test |
|
||
| Form-assertion → behavior-assertion upgrade | ✅ done, preset-path preservation asserted |
|
||
| All other refinement items | ✅ unchanged from verified state, no collateral edits |
|
||
|
||
## 5. Verdict
|
||
|
||
The single blocking item from 6a3d73d4 is fixed exactly as specified — the empty-guard form with no escape hazards, at both the create spawn arm and the resume pre-spawn gate — and, more importantly, the test that let the regression through has been converted from a form-assertion to a behavior test that would have caught it. Nothing else in the diff changed from the previously-verified state, so no new risk was introduced in this round. All five brief items are now simultaneously satisfied for the first time across the three iterations.
|
||
|
||
[VERDICT: PASS] |