4.5 KiB
📋 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_PERMISSIONexport 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]