fix: question tool 'factory is not a function' error

[striderZA]

Bug

The OCGS question extension called ctx.ui.custom({type:'picker',...}) with a plain config object, but Pi's custom() API expects a factory function (tui, theme, kb, done) => component as the first argument. This threw factory is not a function at runtime.

Fix

Rewrote the TUI path using Pi's factory pattern from examples/extensions/question.ts:

• Factory function returns { render, handleInput, invalidate }
• Numbered options list with keyboard navigation (↑↓ enter esc)
• Inline editor for custom typed answers ("Type something.")
• Option descriptions shown below each item
• renderCall and renderResult for proper TUI rendering
• Preserved all OCGS features: header param, decision logging to production/session-logs/, promptSnippet/promptGuidelines

Closes #(bug-report)

[github-actions]

PR Review: fix/question-tool-factory-pattern

Summary

Rewrites the TUI path from a plain config object to Pi's factory function pattern. The architecture is sound and the implementation follows the Pi extension examples correctly. One definite bug found.

---

Critical Bug

Line 329 — Redundant ternary (dead code / logic error)

const answerText = result.wasCustom ? result.answer : result.answer;

Both branches evaluate to result.answer. This is either leftover from a refactor or was meant to do something like result.wasCustom ? "${result.answer}" : result.answer. As-is, the ternary is a no-op. It doesn't cause a runtime failure since logDecision receives answerText which is correct either way, but it's misleading and should be cleaned up.

---

Code Quality Observations

Issue	Line(s)	Severity	Suggestion
Redundant ternary	329	Medium	Remove the variable and pass result.answer directly to logDecision, or make the distinction meaningful
logDecision not wrapped in try-catch	29-36	Low	If production/session-logs/ is unwritable, the entire execute throws instead of gracefully degrading
renderResult accesses result.content[0] without length guard	389	Low	Add result.content?.length check before indexing to be defensive against empty arrays
editor.onSubmit mixed arrow+function style	153	Low	Consistent style preference — the rest of the file uses named functions
_kb param unused in factory	136	Info	Already prefixed with _, which is correct. No change needed
No tests for the TUI component	N/A	Info	Consider adding a story-level test for the non-TUI fallback path at minimum

---

What's Done Well

• Factory pattern is correct: The return value { render, handleInput, invalidate } matches Pi's expected contract
• Keyboard navigation: up/down/enter/esc are handled correctly with clamping on optionIndex
• Inline editor integration: properly pairs Editor with Pi's TUI, delegates input via editor.handleInput, and esc returns to option selection
• cachedLines invalidation: avoids unnecessary re-renders while staying responsive to input
• Decision logging: writes to production/session-logs/agent-decisions.jsonl with timestamps — good provenance
• renderCall and renderResult: properly implemented, showing numbered options and clean result display
• Fallback for non-TUI mode: degrades gracefully with a text summary

---

Suggested Changes (in priority order)

1. Line 329: Remove the dead ternary — replace with result.answer:

   logDecision({
     question: params.question,
     header: params.header,
     options: simpleOptions,
     answer: result.answer,
     wasCustom: result.wasCustom,
   });

2. Wrap logDecision in try-catch to prevent filesystem errors from crashing the extension.

3. (Optional) Guard renderResult against empty content arrays.

---

Verdict

Approved with minor fix required — the core issue (factory pattern mismatch) is correctly resolved, the component is well-structured, and the bug on line 329 is trivial to fix. Ship after addressing the dead ternary.

opencode session  |  github run

[github-actions]

PR Review: fix/question-tool-factory-pattern

Summary +402/-72 in index.ts — rewrites TUI path to Pi's factory pattern. Architecture is correct, but there's a critical runtime bug.

---

🔴 Critical Bug: answerText is undefined (line 333)

The variable answerText is referenced on line 333 but never declared anywhere in the file:

// line 329-335
logDecision({
  question: params.question,
  header: params.header,
  options: simpleOptions,
  answer: answerText,    // <-- ReferenceError at runtime
  wasCustom: result.wasCustom,
});

This will throw ReferenceError: answerText is not defined on any successful non-cancelled answer. Should be result.answer instead.

---

🟡 Medium Issues

#	Issue	Line(s)	Suggestion
1	logDecision can throw	29–36	Wrap in try-catch — if production/session-logs/ is unwritable (e.g. read-only FS in CI), the entire execute throws instead of gracefully falling back
2	renderResult accesses result.content[0] without guard	387	Add result.content?.length check — if content array is empty, this silently returns undefined text

---

🟢 Minor / Nitpicks

• Line 153: editor.onSubmit = (value) => { ... } uses an arrow function while the rest of the file uses named function declarations. Inconsistent style.
• Line 258: If isOther && editMode, the label shows ✎ — nice touch.

---

✅ What's Good

• Factory pattern returns { render, handleInput, invalidate } — matches Pi's expected contract correctly.
• Keyboard navigation is proper: up/down clamping, enter/esc routing.
• cachedLines invalidation avoids unnecessary re-renders.
• logDecision writes to agent-decisions.jsonl with timestamps — good provenance tracking.
• Non-TUI fallback degrades gracefully with text summary.
• renderCall and renderResult produce clean formatted output.

---

Verdict

Changes requested — the answerText ReferenceError on line 333 must be fixed before this can ship. It's a one-line fix (answerText → result.answer). Recommend fixing before merge.

opencode session  |  github run