Skip to content

fix: QA rounds 3-6 — B58-B64 fixed, 10 tmux flows, promptable agent switching #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
e6qu merged 5 commits into from
Mar 22, 2026
Merged

fix: QA rounds 3-6 — B58-B64 fixed, 10 tmux flows, promptable agent switching #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6facfde
fix: plugin hook error handling (B58-B59) + BUGS.md cleanup
Mar 22, 2026
d51ec8f
docs: QA round 3 — zero new bugs, 5 more false positives verified
Mar 22, 2026
d9ef520
fix: B60 objective markdown injection + sweep error logging + integra…
Mar 22, 2026
2eea983
docs: add history editing prompts, slash commands, and verification g…
Mar 22, 2026
b6de1ed
fix: QA rounds 5-6 — B61-B64 fixes, 10 tmux flows, promptable agent s…
Mar 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 86 additions & 65 deletions BUGS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,103 +10,124 @@ All bugs tracked here. Do not create per-package bug files.
| --- | ----- | ---- | -------- | -------- | ----- |
| S3 | Untrusted `.opencode/` autoloading (MCP + plugins) | High | `mcp/`, `plugin/` | [#6361](https://github.com/anomalyco/opencode/issues/6361), [#7163](https://github.com/anomalyco/opencode/issues/7163) | Warning log added; full trust prompt planned |

## Fixed — Security (4)

| # | Issue | Sev | Fix |
| --- | ----- | ---- | --- |
| S1 | `Filesystem.contains()` symlink bypass | Crit | Added `realpathSync()` resolution before lexical check |
| S2 | `exec()` command injection in github.ts | High | Replaced `exec()` with `spawn()` + argument array |
| S4 | Server unauthenticated on non-loopback | Med | Server throws if bound to non-loopback without `OPENCODE_SERVER_PASSWORD` |
| S5 | Read tool exposes .env files | Med | Sensitive file deny-list; `always: []` for sensitive files forces permission prompt |

## Open — Bugs (0)

_No open bugs._

## Fixed — Bugs (QA deep dive, PR #32)

| # | Issue | Sev | Fix |
| --- | ----- | --- | --- |
| B53 | `CAS.deleteBySession()` race condition | High | Wrapped SELECT + DELETE in `Database.transaction()` |
| B54 | `CAS.deleteOrphans()` deletes shared CAS entries | High | Added EditGraphNode reference check before deleting |
| B55 | `EditGraph.checkout()` inconsistent on partial failure | High | Wrapped undo loop + head update in `Database.transaction()` |
| B56 | `EditGraph.deleteBySession()` not atomic | Med | Wrapped in `Database.transaction()` |
| B57 | `filterEdited()` synthetic placeholder reuses part ID | Med | Changed to `PartID.ascending()` for unique synthetic ID |

## Open — Edge Cases (1)

| # | Issue | Sev | Location | Notes |
| --- | ----- | --- | -------- | ----- |
| E1 | `sweep()` clock skew: `turnWhenSet > currentTurn` | Low | `context-edit/index.ts:622-625` | Negative elapsed → never sweeps. Only possible from a bug upstream — turn counter is monotonic. |

## False Positives — Edge Cases (5)

Investigated and determined to be correct behavior or non-issues.
| E1 | `sweep()` clock skew: `turnWhenSet > currentTurn` | Low | `context-edit/index.ts:622-625` | Negative elapsed → never sweeps. Turn counter is monotonic — only possible from upstream bug. |

| Issue | Verdict |
|-------|---------|
| E2: `EditGraph.getHead()` returns undefined vs null | **Correct** — `undefined` is standard TS for "not present"; all callers use `!head` which handles both |
| E3: First commit creates self-referential branch | **Intentional** — `branches: { main: nodeID }` is standard DAG initialization; "main" → first node is correct |
| E4: `Objective.extract()` concurrent race | **False positive** — prompt loop serializes calls per session; concurrency cannot occur |
| E5: `SideThread.create()` duplicate ID not caught | **Correct** — `Identifier.ascending()` is unique (timestamp+counter+random); DB error on collision is the right behavior (fail loudly) |
| E6: SHA-256 collision in CAS not detected | **Intentional** — SHA-256 has no known collisions; `onConflictDoNothing()` was explicitly chosen (B43 fix) |

## Open — Code Quality (5)

Found during QA bug hunt (static analysis). Not crashes, but code quality issues.
## Open — Code Quality (4)

| # | Issue | Sev | Location | Notes |
| --- | ----- | --- | -------- | ----- |
| Q1 | 95 empty `.catch(() => {})` blocks across 29 files | Low | Various | Most intentional (file ops), ~10 mask real errors in `config.ts`, `lsp/client.ts`, `sdk.tsx` |
| Q2 | 17 TODO/FIXME/HACK comments | Low | 13 files | Track as tech debt; key ones: copilot lost type safety (#374), process.env vs Env.set (#300, #524) |
| Q3 | `console.log` in TUI production code | Low | `cli/cmd/tui/` | **FIXED** in this PR — replaced 18 calls with `Log.create()` |
| Q4 | Copilot SDK lost chunk type safety | Med | `provider/sdk/copilot/chat/openai-compatible-chat-language-model.ts:374` | TODO says "MUST FIX" — type safety lost on Chunk due to error schema |
| Q5 | `process.env` used directly instead of `Env.set` | Low | `provider/provider.ts:300,524` | Env.set only updates shallow copy, not process.env — architectural issue |

## Open — Bugs (0)

_No open bugs._
| Q1 | 95 empty `.catch(() => {})` blocks across 29 files | Low | Various | Most intentional (file ops), ~10 mask real errors |
| Q2 | 17 TODO/FIXME/HACK comments | Low | 13 files | Tech debt; key: copilot type safety (#374), process.env vs Env.set (#300, #524) |
| Q4 | Copilot SDK lost chunk type safety | Med | `provider/sdk/copilot/chat/openai-compatible-chat-language-model.ts:374` | Upstream TODO "MUST FIX" |
| Q5 | `process.env` used directly instead of `Env.set` | Low | `provider/provider.ts:300,524` | Architectural issue |

---

## Deferred (1)
## Fixed — Security (4)

| # | Issue | Sev | Location | Notes |
| --- | ------------------------------ | --- | ----------------- | --------------------------------------------------------------------------- |
| B51 | ID generator counter not atomic | Low | `id/id.ts:25-27` | Fine single-threaded; documented with comment. Fix if worker threads added. |
| # | Issue | Sev | Fix |
| --- | ----- | ---- | --- |
| S1 | `Filesystem.contains()` symlink bypass | Crit | Added `realpathSync()` before lexical check |
| S2 | `exec()` command injection in github.ts | High | Replaced `exec()` with `spawn()` + argument array |
| S4 | Server unauthenticated on non-loopback | Med | Server throws without `OPENCODE_SERVER_PASSWORD` |
| S5 | Read tool exposes .env files | Med | Sensitive file deny-list; forced permission prompt |

---
## Fixed — Bugs (QA, PRs #32-#33)

## Fixed (51)
| # | Issue | Sev | Fix |
| --- | ----- | --- | --- |
| B53 | `CAS.deleteBySession()` race condition | High | `Database.transaction()` |
| B54 | `CAS.deleteOrphans()` deletes shared entries | High | EditGraphNode reference check |
| B55 | `EditGraph.checkout()` partial failure | High | `Database.transaction()` |
| B56 | `EditGraph.deleteBySession()` not atomic | Med | `Database.transaction()` |
| B57 | `filterEdited()` synthetic ID collision | Med | `PartID.ascending()` |
| B58 | `pluginGuard()` uncaught Plugin.trigger() errors | High | try-catch → EditResult error |
| B59 | `pluginNotify()` silent Plugin.trigger() errors | Med | try-catch → log.warn |
| B60 | Objective markdown injection into system prompt | Med | Escaped newlines + markdown chars in objective text |
| B61 | MCP `add()` inconsistent return type | Med | All branches now return `{ status: s.status }` (Record) |
| B62 | Text part timing start overwritten at stream end | Low | Preserve `currentText.time?.start` in processor.ts |
| B63 | Unguarded `JSON.parse` on ripgrep output | Low | flatMap with try-catch, log.warn on malformed lines |
| B64 | Untracked file line count off-by-one | Low | `content.trimEnd().split("\n").length` |
| Q3 | `console.log` in TUI production code | Low | 18 calls → `Log.create()` (PR #31) |

## Fixed — Bugs (PRs #10-#22)

56 bugs fixed. Full details in git history. By severity: 5 Crit, 15 High, 19 Med, 12 Low.

51 bugs fixed across PRs #10, #12, #16-#22. Full details in git history.
---

**By severity:** 5 Critical, 15 High, 19 Medium, 12 Low
## Deferred (1)

**By category:**
- CAS/EditGraph: B1, B10, B23, B41-B43, B45
- Session/prompt pipeline: B7, B15-B16, B21-B22, B47-B49
- Circuit breaker/verify: B25-B31
- Evaluator/refine: B32-B35, B40
- Utilities: B2, B11, B13-B14, B50, B52
- Side threads/skills: B4-B6, B8-B9, B24, B36-B39
- Upstream backports: B17-B20
- Other: B3, B12, B44, B46
| # | Issue | Sev | Location | Notes |
| --- | ----- | --- | -------- | ----- |
| B51 | ID generator counter not atomic | Low | `id/id.ts:25-27` | Fine single-threaded; fix if worker threads added. |

---

## False Positives / Intentional (6)
## False Positives / Intentional (49)

| Issue | Resolution |
|-------|------------|
| Issue | Verdict |
|-------|---------|
| Fork-based ephemeral: message IDs point to deleted session | Intentional — results serialized immediately |
| Skill template returns Promise not string | By design — all consumers `await` |
| Provider/config state map key inconsistency | False positive — consistent keying by directory |
| Bus subscription cleanup gap | False positive — unsubscribe + finalizer both clean up |
| `CAS.deleteBySession()` race with store | False positive — deletion is idempotent |
| E2: `EditGraph.getHead()` returns undefined vs null | Correct TS idiom — all callers use `!head` |
| E3: First commit self-referential branch | Intentional DAG initialization |
| E4: `Objective.extract()` concurrent race | False positive — prompt loop serializes calls |
| E5: `SideThread.create()` duplicate ID | Correct — DB error on collision is right (fail loudly) |
| E6: SHA-256 collision in CAS | Intentional — `onConflictDoNothing()` per B43 |
| V1: Circuit breaker timing race | Edge case — benign; breaker prevents execution during cooldown |
| R4: Refine session cleanup | False positive — finally block cleans all sessions |
| E1-fork: Fork session failure leaks | Already fixed in B21 |
| PM1: edit/write use `always: ["*"]` | By design — "remember answer for type", not auto-approve |
| PM2: bash doesn't ask edit permission | By design — bash has own permission level |
| Protected message window timing race | False positive — part marked hidden regardless; filter runs next prompt |
| Side thread system prompt staleness | False positive — thread list queried fresh from DB per prompt |
| Sweep transaction silent failure | Fixed — added try-catch with log.error (this PR) |
| `updatePart()` creates orphaned parts if message deleted | False positive — FK constraint `message_id → MessageTable.id` prevents orphaned inserts |
| Script paths with spaces in skill/scripts.ts | False positive — array-based `Process.text()` doesn't split on spaces |
| Truncation boundary at exact maxBytes | False positive — `>` comparison is correct (include at limit, truncate above) |
| Compaction during active prompt | False positive — `BusyError` prevents concurrent runs |
| filterEdited + sweep modify same part | False positive — orthogonal concerns (edit vs lifecycle), no conflict |
| Nested `Database.use()` in `checkout()` transaction | False positive — `Database.use()` reuses transaction context via ALS `ctx.use()` |
| Uninitialized `casHash` in hide/replace/externalize | False positive — `Database.transaction()` callback is synchronous, always assigns before outer scope |
| `CAS.store` race in `externalize()` | False positive — both store and get run synchronously within same transaction |
| `filterEdited` synthetic part losing agent metadata | False positive — message spread `...msg` preserves role/agent; part is just text placeholder |
| `annotate()` losing `casHash` from previous edit | False positive — spread `...part.edit` preserves all existing fields including casHash |
| Side-thread `update()` read-after-write staleness | False positive — SQLite ops are synchronous; `get()` sees committed data |
| Provider `find("create")!` non-null assertion | Inside try-catch; degrades to `InitError` with cause — confusing but not a crash |
| Permission `Map.delete` during iteration | False positive — safe per JS Map spec; deleted entries not revisited |
| Permission data `null ?? []` fallback | False positive — `??` correctly handles both `null` and `undefined` |
| Retry `JSON.parse` without string check | False positive — entire block wrapped in try-catch returning undefined |
| Instruction state Map unbounded growth | False positive — `clear()` called per message; states keyed by directory (few entries) |
| Provider sort `findIndex` returning -1 | False positive — desc sort puts -1 last, non-matching models sort after all priority models |
| Bash tool double-kill on timeout+abort | False positive — timeout `.catch(() => {})` is intentional; double-kill is idempotent |
| Edit tool sync stat then async read TOCTOU | False positive — `Filesystem.stat()` is synchronous; TOCTOU benign (caught by readText) |
| Share sync queue data loss on rapid calls | False positive — Map mutation visible to timeout closure; all merged data sent |
| `side-thread` hint ignored by sweeper | By design — side-thread is a classification hint for `/focus`, not auto-cleanup |
| Compaction loses edit metadata on replay | False positive — replay only replays user messages; user messages can't be edited (ownership check) |
| `Database.effect()` async fire-and-forget | Intentional — effects fire after DB commit; `Bus.publish` async rejection is benign since DB state is already correct |
| Share sync timeout accumulation | False positive — exactly 1 timeout per sessionID; existing entry merges data, no new timer created |
| Git `--numstat` undefined filepath on split | False positive — git `--numstat` always produces 3 tab-separated fields |
| `Bus.publish` unhandled promise rejection | Intentional fire-and-forget pattern; `void Bus.publish(...)` used throughout codebase |
| Processor metadata overwrite during text-delta | False positive — metadata is additive; `if (value.providerMetadata)` guard prevents null overwrites |
| MCP OAuth transport deleted before add() | Edge case — user must restart OAuth flow anyway; catch returns error status |
| MCP silent kill in disposer hides orphans | Intentional — kill failures are benign (process already exited); logged elsewhere |

---

## Notes

**TUI Testing:** Use `testRender()` from `@opentui/solid` for unit tests. tmux-based integration harness at `test/cli/tui/tmux-tui-test.ts` for E2E flows.
**TUI Testing:** `testRender()` for components, tmux harness at `test/cli/tui/tmux-tui-test.ts` for E2E. 10 flows pass (home, command-palette, agent-cycle, submit-message, cost-dialog, slash-command, multi-agent-verify, slash-classify, slash-threads, slash-history).

**SAST:** Pre-commit + CI run `scripts/sast-check.sh` (no eval, no Function, no secrets, no console.log).
51 changes: 51 additions & 0 deletions docs/agents.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,57 @@ Complete conversation rewrite agent. Asks user to confirm objective before proce

Invoked via `/focus-rewrite-history` command. Always asks for confirmation before rewriting user messages.

## Promptable Mode Switching

Build and Plan agents can be switched via natural language prompts — the same way Claude Code supports "enter plan mode". The LLM calls the appropriate tool, the user confirms, and the TUI updates automatically.

### How it works

| Direction | Tool | Permission | TUI Update |
|-----------|------|------------|------------|
| Build → Plan | `plan_enter` | Build agent has `plan_enter: "allow"` | `local.agent.set("plan")` |
| Plan → Build | `plan_exit` | Plan agent has `plan_exit: "allow"` | `local.agent.set("build")` |

**Flow:**
1. User types a natural language prompt, OR the agent decides autonomously that switching would be beneficial
2. The current agent calls `plan_enter` (Build → Plan) or `plan_exit` (Plan → Build)
3. A confirmation dialog appears asking the user to approve the switch
4. On approval, a synthetic user message is created with `agent: "plan"` or `agent: "build"`, switching the active agent
5. The TUI watcher in `session/index.tsx` detects the completed tool call and updates the agent display
6. Subsequent messages use the new agent's system prompt and permissions

### Autonomous switching

Agents can decide to switch modes based on their own reasoning — they do not need the user to explicitly ask. The Build agent will proactively switch to Plan when it determines a task is complex enough to benefit from planning. The Plan agent will switch to Build when planning is complete and implementation should begin. Agents can switch back and forth as many times as needed during a session.

**Build → Plan (autonomous):** The Build agent realizes mid-implementation that the task is more complex than expected, involves multiple files, or requires architectural decisions. It calls `plan_enter` to step back and plan first.

**Plan → Build (autonomous):** The Plan agent completes the plan file, has no remaining questions, and determines the plan is ready. It calls `plan_exit` to begin implementation.

### Example prompts (user-triggered)

**Switch to Plan mode:**
```
Let's plan this before implementing.
Enter plan mode.
I need to think through the architecture first.
```

**Switch back to Build mode:**
```
The plan looks good, let's implement it.
Start building.
Exit plan mode and execute the plan.
```

### Implementation details

- **Tools:** `plan_enter` and `plan_exit` defined in `src/tool/plan.ts`
- **TUI watcher:** `src/cli/cmd/tui/routes/session/index.tsx:221-236` listens for tool completions
- **Permissions:** Build agent allows `plan_enter`; Plan agent allows `plan_exit` (cross-permissions)
- **Confirmation:** Both tools use `Question.ask()` to get user consent before switching
- **Plan file:** Stored at `$XDG_DATA_HOME/opencode/plans/<session-slug>.md`

## Modified Agents

### build / plan
Expand Down
68 changes: 68 additions & 0 deletions docs/context-editing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,74 @@ Park and list project-level side threads. Threads survive across sessions.

---

## How to Elicit History Editing

The context editing system is available to the agent when `context_edit` is in the tool set. The agent can use it autonomously, or you can prompt it directly.

### Direct prompts to trigger editing

**Hide stale content:**
```
Hide the file read output from 3 turns ago — it's outdated since we edited the file.
```

**Replace incorrect information:**
```
The grep result from earlier is wrong — replace it with a note saying "file was restructured".
```

**Externalize verbose output:**
```
Externalize that long test output — just keep a summary of what passed and failed.
```

**Mark for automatic cleanup:**
```
Mark that debug logging as discardable — it's only useful for the next 2 turns.
```

**Park a side thread:**
```
Park that security issue we noticed — it's not related to our current task.
```

### Slash commands

| Command | What it does |
|---------|-------------|
| `/focus` | Runs the classifier agent to label messages by topic, then externalizes stale output and parks off-topic threads. Requires the focus agent to be enabled in config. |
| `/focus-rewrite-history` | Full conversation rewrite with user confirmation. The agent reviews all messages, classifies them, and rewrites the history to focus on the current objective. Disabled by default — enable in agent config. |
| `/btw <question>` | Ask a side question without polluting the main conversation. Runs in a forked ephemeral session. |
| `/reset-context` | Restore all edited parts to their originals from CAS. Undo all context edits. |
| `/classify` | Run the classifier agent to see how messages are labeled (main/side/mixed). Read-only, no side effects. |
| `/threads` | List all parked side threads for this project. |
| `/history` | Show the edit history (linear log from HEAD). |
| `/tree` | Show the full edit DAG with branches. |

### Enabling focus agents

By default, the focus and focus-rewrite-history agents are disabled. Enable them in your `opencode.json`:

```jsonc
{
"agent": {
"focus": {}, // remove "disable": true
"focus-rewrite-history": {} // remove "disable": true
}
}
```

### How history editing is verified

Integration tests prove the editing pipeline works end-to-end:
- **hide → filterEdited**: secret content removed from LLM context, CAS preserves original, synthetic placeholder created
- **unhide**: original content restored from CAS
- **mark → sweep**: discardable content auto-cleaned after N turns

See `test/context-edit/integration.test.ts` for the proof tests.

---

## See Also

- [schema.md](schema.md) — database tables (cas_object, edit_graph_node/head, side_thread, PartBase extensions)
Expand Down
Loading
Loading