fix: auto-resolve question tool in non-interactive contexts#937
Conversation
`Question.ask()` awaits an Effect Deferred that only resolves on a TUI click. When `altimate-code run` is invoked as a subprocess (Claude Code's Bash tool, CI, plugin host) and a skill that uses `question` fires, nobody can ever click — the deferred awaits forever and the parent eventually TaskStops the subprocess. The symptom is indistinguishable from a hang: 0% CPU, no log activity, no error. In non-interactive contexts (no TTY, or explicit env-var opt-in), auto-resolve `question` with a conservative-by-default policy and flag the auto-answer in the tool result so the calling LLM can adapt instead of treating it as a real user choice. Resolution policy (env-var controlled): - Detect non-interactive: `!process.stdin.isTTY`. Overrides: ALTIMATE_FORCE_INTERACTIVE=1 — keep the original interactive Deferred path even when isTTY is false. ALTIMATE_NON_INTERACTIVE=1 — force non-interactive even when isTTY is true (useful for tests + CI assertions). - Default in non-TTY (ALTIMATE_AUTO_ANSWER=last): pick the option whose label/description contains a safe keyword (skip, cancel, no, abort, profile only, decline, deny, stop); fall back to the last option (UX convention: safer/cancel typically sits at end). - ALTIMATE_AUTO_ANSWER=first / =skip / =<exact label>: explicit overrides for callers who want a specific behavior. Tool result prefix reflects mode — "Running in non-interactive mode (no TTY). Auto-answered with safe defaults: ..." vs the original "User has answered your questions: ..." — so the agent knows the choice was not a real user answer. Tests: 6 new bun:test cases covering safe-keyword selection, last-option fallback, each ALTIMATE_AUTO_ANSWER mode, and the prefix wording. Existing 2 legacy tests gated with ALTIMATE_FORCE_INTERACTIVE=1 so they preserve their original intent under non-TTY CI. Closes #936
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughQuestionTool now detects non-interactive runs via environment variables ( ChangesNon-interactive auto-answer support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opencode/src/tool/question.ts (2)
39-57: 💤 Low valueConsider logging when ALTIMATE_AUTO_ANSWER doesn't match any known mode or option label.
When
ALTIMATE_AUTO_ANSWERis set to a value that doesn't match "skip", "first", "last", or any option label (case-insensitive), the function silently returns empty arrays (Unanswered). This is safe but could make debugging harder if a user misspells a mode or label.Consider logging a warning in this case to help users diagnose configuration issues.
📝 Optional: Add debug logging for unmatched modes
// exact label match for explicit answers, e.g. ALTIMATE_AUTO_ANSWER="Profile only" const match = q.options.find((o) => o.label.toLowerCase() === mode) + if (!match && mode !== "skip" && mode !== "first" && mode !== "last") { + console.warn(`ALTIMATE_AUTO_ANSWER="${mode}" did not match any option label; returning Unanswered`) + } return match ? [match.label] : []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/tool/question.ts` around lines 39 - 57, The autoAnswer function silently returns empty answers when ALTIMATE_AUTO_ANSWER contains an unknown mode or when the explicit-label match fails; add a warning log to aid debugging: detect when mode is not "skip"/"first"/"last" and no q.options label matches the lowercase mode, and emit a single warning (e.g., console.warn or the module's logger) including the provided ALTIMATE_AUTO_ANSWER value and question id/label (use Question.Info properties) so users know the env value didn't match any known mode or option; place this check inside autoAnswer just before returning an empty array for unmatched cases and reference autoAnswer, ALTIMATE_AUTO_ANSWER, and q.options in the change.
86-92: 💤 Low valueConsider mode-specific prefix wording for accuracy.
The prefix says "Auto-answered with safe defaults" but this is only accurate when
ALTIMATE_AUTO_ANSWERis "last" (default). When the mode is "first" or an exact label match, the selection isn't necessarily using safe defaults—it's just picking the first option or the specified label.While the key information ("Running in non-interactive mode") is accurate and sufficient, you might consider mode-specific wording for precision:
- "last" → "Auto-answered with safe defaults"
- "first" → "Auto-selected first option"
- exact match → "Auto-selected option: {mode}"
- "skip" → "Auto-skipped (Unanswered)"
This is a minor clarity improvement; the current wording is acceptable since the primary goal is signaling non-interactive execution.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/tool/question.ts` around lines 86 - 92, The prefix message built in question.ts (variable prefix) misstates the non-interactive auto-answer behavior by always saying "Auto-answered with safe defaults"; update the logic that sets prefix (referencing isNonInteractive() and the ALTIMATE_AUTO_ANSWER mode) to choose mode-specific wording: "Auto-answered with safe defaults" for "last", "Auto-selected first option" for "first", "Auto-selected option: {mode}" for exact label matches, and "Auto-skipped (Unanswered)" for "skip", while keeping the existing "Running in non-interactive mode" text and preserving the interactive branch "User has answered your questions".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/src/tool/question.ts`:
- Around line 39-57: The autoAnswer function silently returns empty answers when
ALTIMATE_AUTO_ANSWER contains an unknown mode or when the explicit-label match
fails; add a warning log to aid debugging: detect when mode is not
"skip"/"first"/"last" and no q.options label matches the lowercase mode, and
emit a single warning (e.g., console.warn or the module's logger) including the
provided ALTIMATE_AUTO_ANSWER value and question id/label (use Question.Info
properties) so users know the env value didn't match any known mode or option;
place this check inside autoAnswer just before returning an empty array for
unmatched cases and reference autoAnswer, ALTIMATE_AUTO_ANSWER, and q.options in
the change.
- Around line 86-92: The prefix message built in question.ts (variable prefix)
misstates the non-interactive auto-answer behavior by always saying
"Auto-answered with safe defaults"; update the logic that sets prefix
(referencing isNonInteractive() and the ALTIMATE_AUTO_ANSWER mode) to choose
mode-specific wording: "Auto-answered with safe defaults" for "last",
"Auto-selected first option" for "first", "Auto-selected option: {mode}" for
exact label matches, and "Auto-skipped (Unanswered)" for "skip", while keeping
the existing "Running in non-interactive mode" text and preserving the
interactive branch "User has answered your questions".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2e6c3623-6d27-4e80-adc6-f385f03948eb
📒 Files selected for processing (2)
packages/opencode/src/tool/question.tspackages/opencode/test/tool/question.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The earlier non-interactive policy in this branch scanned option label text for "safe" keywords (skip/cancel/no/abort/...) and fell back to the last option. That tried to recover semantics the LLM already knew at construction time, and false-positived on substrings — "no" matched inside "Snowflake", "Annotate", "Knowledge", "Honor". The fix: don't guess. Return Unanswered for every question when no TTY is present and let the agent decide. The agent has full context — it knows what action it was about to take and why it asked. It can pick a safe path from that context or report that user input is required. Pretending a decision was made that wasn't is the worse failure mode. Changes: - Drop SAFE_KEYWORDS and the label-text scan entirely. - Default non-interactive behavior returns [] (renders as "Unanswered" via the existing format()). - Cache isNonInteractive() once at execute() entry so the result prefix can't disagree with the path that produced the answer. - Non-interactive prefix tells the agent how to proceed AND names the escape hatch (ALTIMATE_AUTO_ANSWER=first|last|<label>) so it can surface it to the user when reporting back. - Keep =first / =last / =<exact label> as explicit user opt-ins for callers who genuinely want a default. Drop =skip — it's the new default. Addresses cubic-dev-ai review feedback on #937 by removing the heuristic that needed the word-boundary fix in the first place. Tests: 9 pass / 0 fail in packages/opencode/test/tool/question.test.ts. Refs #936 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
New commit: redesigned around the actual problemPushed Delete the label-text heuristic entirely. The LLM that constructed the question already knew at construction time which option meant "cancel" — the keyword scan was trying to recover information that was never lost, and false-positived on substrings ( What this commit does
Addresses cubic review feedback
Tests + verification
Honest tradeoffThe prior policy (auto-pick "last" or safe-keyword) tried to make forward progress when no human is present. The new policy says: forward progress on behalf of someone who didn't choose is the worse failure mode — let the agent (which has full context) decide. The escape hatch in the prefix gives the user explicit knobs if they disagree. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Centralized-test bot failures — likely shared infra, not PR-causedThe Strong signal these are shared fault-injection-harness failures, not regressions introduced by this PR. Flagging here so the pattern is on record across the three PRs; happy to dig into the harness config if the bot owner wants. |
suryaiyer95
left a comment
suryaiyer95
left a comment
There was a problem hiding this comment.
One workflow-affecting concern (verified against the codebase) — posting just this; the rest were quality/description nits.
| function isNonInteractive(): boolean { | ||
| if (process.env["ALTIMATE_FORCE_INTERACTIVE"] === "1") return false | ||
| if (process.env["ALTIMATE_NON_INTERACTIVE"] === "1") return true | ||
| return !process.stdin.isTTY |
There was a problem hiding this comment.
Likely regression for server / IDE (headless-but-interactive) mode — !process.stdin.isTTY is the wrong signal for "no human is listening."
Question.ask() does not only resolve on a TUI keypress. Answers also arrive over HTTP: Question.reply({ requestID, answers }) is exposed at POST /question/:requestID/reply (packages/opencode/src/server/routes/question.ts), with GET /question to list pending ones. So when altimate-code runs as a server with a frontend/IDE client (VS Code / JetBrains / web), process.stdin.isTTY is false, yet a real human can answer via the route.
With this guard, that existing interactive flow is misclassified as non-interactive: execute() short-circuits to autoAnswer() → returns Unanswered immediately and never publishes the question for the client to reply to. The UI user loses the ability to answer, out of the box — ALTIMATE_FORCE_INTERACTIVE=1 exists but server deployments won't have it set.
Suggestion: gate on whether an answer channel actually exists, not on TTY. E.g. treat it as interactive when the server/question clients are connected (a pending-question listener is registered), and reserve the auto-answer path for true headless runs (run subprocess / CI). At minimum, default server mode to interactive and require explicit ALTIMATE_NON_INTERACTIVE=1 to opt into auto-answer, so the altimate-code run-as-subprocess fix doesn't also silently disable the HTTP reply path.
There was a problem hiding this comment.
Thanks for catching this — your point was the core insight that drove the entire redesign of this PR. Addressed across f981199, d2a85165, and 764291c0.
Specifically:
tool/question.tsno longer touchesprocess.stdin.isTTYat all. Detection is now opt-in viaALTIMATE_NON_INTERACTIVE, andrunis the only entrypoint that sets it.serve,web,acp, andworkspace-servedeliberately leave it unset, so theirPOST /question/:requestID/replypath stays live for connected IDE/web clients exactly as you described.cli/cmd/run.tssets the env var only whenargs.attachis not set — attach-mode runs the agent on the remote server, so the local env var would just be noise and could mislead other host-local tools.tool/bash.tsstripsALTIMATE_NON_INTERACTIVEfrommergedEnvbefore spawning children, so a nestedaltimate-code serve(or any server entrypoint) spawned from arunsession doesn't inherit the flag and silently disable its own HTTP reply path. This was a latent regression of the same class you flagged — it would have hit the same IDE/web users.
The block comment at tool/question.ts:6-38 captures the rationale and explicitly mentions PR #937 review for future maintainers. Tests at test/tool/question.test.ts lock in the default-interactive contract (the suryaiyer95-named regression class) plus FORCE_INTERACTIVE precedence and NON_INTERACTIVE=0 opt-out.
There's one remaining architectural item your review surfaced indirectly: session/llm.ts:299 only consults agent.permission, not the merged session.permission, so the question: deny rule in run.ts is intent-marker only. Tracked as a follow-up — out of scope here because the env-var path now handles the actual behavior correctly.
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 1 finding(s)
- 1 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/tool/question.ts (L34)
[🟠 MEDIUM] There's a potential null pointer exception here. Depending on the runtime environment (e.g., embedded environments or child processes without standard streams), process.stdin could be undefined. Using optional chaining process.stdin?.isTTY is a safer approach and adheres to the null checks guideline.
Suggested change:
return !process.stdin?.isTTY
| function isNonInteractive(): boolean { | ||
| if (process.env["ALTIMATE_FORCE_INTERACTIVE"] === "1") return false | ||
| if (process.env["ALTIMATE_NON_INTERACTIVE"] === "1") return true | ||
| return !process.stdin.isTTY |
There was a problem hiding this comment.
[🟠 MEDIUM] There's a potential null pointer exception here. Depending on the runtime environment (e.g., embedded environments or child processes without standard streams), process.stdin could be undefined. Using optional chaining process.stdin?.isTTY is a safer approach and adheres to the null checks guideline.
Suggested change:
| return !process.stdin.isTTY | |
| return !process.stdin?.isTTY |
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo supported files changed. |
Code Review —
|
Five fixes flowing from the multi-model consensus review on the prior revision. Touches the `run` entrypoint, the bash tool, the question tool, and its tests. - `run.ts:447`: `process.stdin.isTTY` was still dereferenced without a guard. The prior commit closed null-safety in `question.ts` only; this closes the same risk in the `run` entrypoint. Addresses the dev-punia-altimate review comment in full. - `run.ts` handler: gate the `ALTIMATE_NON_INTERACTIVE=1` assignment on `!args.attach`. In attach mode the agent runs on the remote server, so the local env var is a no-op and only pollutes the local process env for other tools that may consult it. - `bash.ts`: strip `ALTIMATE_NON_INTERACTIVE` from `mergedEnv` before `spawn`. Without this, child processes spawned by the bash tool (e.g. nested `altimate-code serve`) would inherit the flag from the parent `run` process and silently disable their own HTTP question- reply path — the exact regression the prior commit is fixing for other surfaces. - `question.test.ts`: regression tests for `ALTIMATE_FORCE_INTERACTIVE=1` precedence over `NON_INTERACTIVE=1` and for `ALTIMATE_NON_INTERACTIVE=0` honored as explicit opt-out. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Update — addressing review findings (commit d2a8516)Ran a multi-model consensus review (Claude + GPT 5.4 Codex + Gemini 3.1 Pro + Kimi K2.5 + MiniMax M2.7 + GLM-5.1, 1 convergence round). Resolved 5 findings in this push; 2 deferred to follow-ups. Fixed in this commit
Test count: 11 → 13. All passing locally ( Deferred to follow-upsThese were flagged in the consensus review but are pre-existing architectural concerns out of scope for this PR:
Convergence review artifactsStored at
|
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/test/tool/question.test.ts (1)
119-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHarden env isolation for the non-interactive suite.
Line 119 sets
ALTIMATE_NON_INTERACTIVE=1, butALTIMATE_FORCE_INTERACTIVEis not cleared in this suite. If that var is pre-set by the parent environment, these tests can silently run the interactive path and lose non-interactive coverage.Suggested fix
beforeEach(() => { + delete process.env["ALTIMATE_FORCE_INTERACTIVE"] process.env["ALTIMATE_NON_INTERACTIVE"] = "1" askSpy = spyOn(QuestionModule.Question, "ask").mockImplementation(async () => []) }) afterEach(() => { + delete process.env["ALTIMATE_FORCE_INTERACTIVE"] delete process.env["ALTIMATE_NON_INTERACTIVE"] delete process.env["ALTIMATE_AUTO_ANSWER"] askSpy.mockRestore() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/tool/question.test.ts` around lines 119 - 127, The beforeEach hook sets ALTIMATE_NON_INTERACTIVE but does not clear ALTIMATE_FORCE_INTERACTIVE, which could cause these non-interactive tests to silently run in interactive mode if that variable is pre-set in the parent environment. Add a line in the beforeEach function to explicitly delete process.env["ALTIMATE_FORCE_INTERACTIVE"] to ensure proper test isolation, and also add deletion of this variable to the afterEach function for complete environment cleanup alongside the existing ALTIMATE_NON_INTERACTIVE and ALTIMATE_AUTO_ANSWER deletions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/cli/cmd/run.ts`:
- Around line 457-460: The stdin-read condition at line 459 in the run.ts file
is too broad and attempts to read stdin whenever process.stdin is not a TTY (or
is undefined), even when a message has already been provided as a positional
argument. Modify the condition to check not only that process.stdin is available
and not a TTY, but also that no message was already provided by the user before
attempting to read from Bun.stdin.text(). This prevents the process from
blocking indefinitely on stdin when message content has already been supplied.
---
Outside diff comments:
In `@packages/opencode/test/tool/question.test.ts`:
- Around line 119-127: The beforeEach hook sets ALTIMATE_NON_INTERACTIVE but
does not clear ALTIMATE_FORCE_INTERACTIVE, which could cause these
non-interactive tests to silently run in interactive mode if that variable is
pre-set in the parent environment. Add a line in the beforeEach function to
explicitly delete process.env["ALTIMATE_FORCE_INTERACTIVE"] to ensure proper
test isolation, and also add deletion of this variable to the afterEach function
for complete environment cleanup alongside the existing ALTIMATE_NON_INTERACTIVE
and ALTIMATE_AUTO_ANSWER deletions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf81a5f5-387e-436f-ba73-ba864cd4bc88
📒 Files selected for processing (4)
packages/opencode/src/cli/cmd/run.tspackages/opencode/src/tool/bash.tspackages/opencode/src/tool/question.tspackages/opencode/test/tool/question.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/tool/question.ts
| // altimate_change start — null-safe stdin access. process.stdin can be | ||
| // undefined in embedded/child runtimes (see dev-punia review on PR #937). | ||
| if (!process.stdin?.isTTY) message += "\n" + (await Bun.stdin.text()) | ||
| // altimate_change end |
There was a problem hiding this comment.
Stdin-read guard is too broad and can reintroduce subprocess hangs.
At Line 459, the condition reads stdin for any non-TTY (and even when process.stdin is undefined), which can block forever on inherited stdin despite a provided positional message. This is the same hang class this PR series is fixing.
Suggested fix
- if (!process.stdin?.isTTY) message += "\n" + (await Bun.stdin.text())
+ if (process.stdin?.isTTY === false && message.trim().length === 0) {
+ message += "\n" + (await Bun.stdin.text())
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/src/cli/cmd/run.ts` around lines 457 - 460, The stdin-read
condition at line 459 in the run.ts file is too broad and attempts to read stdin
whenever process.stdin is not a TTY (or is undefined), even when a message has
already been provided as a positional argument. Modify the condition to check
not only that process.stdin is available and not a TTY, but also that no message
was already provided by the user before attempting to read from
Bun.stdin.text(). This prevents the process from blocking indefinitely on stdin
when message content has already been supplied.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…view Two small fixes flagged by coderabbit + cubic on the prior commit (d2a8516). - `run.ts:459`: the `!process.stdin?.isTTY` null-safety guard turned a crash (undefined stdin) into a stall (we still entered the branch and awaited `Bun.stdin.text()` on a stream that would never EOF). Use `process.stdin && !process.stdin.isTTY` so undefined stdin skips the read entirely — the only sensible behavior in an embedded runtime where there is no stdin to read from. - `question.test.ts`: the `tool.question non-interactive auto-answer` describe block sets `ALTIMATE_NON_INTERACTIVE=1` in beforeEach but didn't clear `ALTIMATE_FORCE_INTERACTIVE`. A parent-shell export could silently flip the suite to the interactive path and lose non-interactive coverage. Delete in both hooks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Follow-up — addressing coderabbit + cubic findings on
|
| Finding | Location | Change |
|---|---|---|
| coderabbit + cubic | cli/cmd/run.ts:459 |
!process.stdin?.isTTY → process.stdin && !process.stdin.isTTY. Undefined stdin now skips the read entirely — only sensible behavior when there is no stdin to read from. |
| coderabbit (outside-diff) | test/tool/question.test.ts:119-127 |
tool.question non-interactive auto-answer block now deletes ALTIMATE_FORCE_INTERACTIVE in both beforeEach and afterEach. Without this, a parent-shell FORCE_INTERACTIVE=1 would silently flip the suite to the interactive path and lose all non-interactive coverage. |
Not adopted from the suggestions
CodeRabbit additionally suggested that the stdin-read should be skipped when a positional message argument was already provided. That would change current semantics — altimate-code run "summarize" < data.txt is a supported pattern that intentionally combines positional + piped content. Out of scope for this fix.
Tests
13/13 pass on the updated file. All previously-passing question/bash tool tests still pass.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. cc @sahrizvi |
Resolved conflict on packages/opencode/src/cli/cmd/run.ts by keeping the assembleStdinMessage call. PR #937's null-safety against undefined process.stdin is folded into readStdinIfAvailable in a follow-up commit on top of this merge.
Addresses suryaiyer95's review of `cc222db2a`: - Default first-byte timeout bumped from 100ms to 500ms. The previous value was aggressive against realistic slow-to-first-byte producers (DB queries that need to plan, decompression headers, network calls with DNS+TLS handshake). 500ms is generous for those without sacri- ficing the inherited-idle wedge fix. - New `ALTIMATE_STDIN_TIMEOUT_MS=N` env override so a user hitting an even slower pipeline can bump it without code changes. - Stderr note when fd 0 looked like real input (FIFO / file / socket) but readStdin came back empty. Silent drop was the original UX failure that motivated this whole review thread. The note fires on the dominant inherited-idle path too; that's intentional since stderr is captured but not surfaced for most subprocess callers (Claude Code's Bash tool, CI), and a single line of noise per call is worth the explicitness for the slow-producer case. - `process.stdin` null-safety folded into `readStdinIfAvailable`: in embedded / child runtimes (dev-punia review on PR #937) accessing `process.stdin.isTTY` could throw. Treat undefined stdin as "no input to read". - 6 new injection tests: env override + invalid env fallback, warning fires/doesn't fire across each gate, undefined-stdin guard. - E2E thresholds adjusted for the 500ms timeout (idle-exit assertion bumped to 1500ms, post-timeout sleep bumped to 1200ms).
4 participants
Closes #936
Summary
packages/opencode/src/tool/question.tscallsQuestion.ask(), which awaits an EffectDeferredthat only resolves on a TUI click. Whenaltimate-code runis invoked as a subprocess (Claude Code's Bash tool, CI,subprocess.run, plugin host) and a skill that uses thequestiontool fires, nobody can ever click — the deferred awaits forever and the parent eventuallyTaskStops the subprocess. Symptom: 0% CPU, no log activity, no error, indistinguishable from a hang.This PR short-circuits the
questiontool in non-interactive contexts with a conservative-by-default auto-answer policy.Resolution policy
!process.stdin.isTTY. Overrides:ALTIMATE_FORCE_INTERACTIVE=1— keep the original interactive Deferred path even when isTTY is false.ALTIMATE_NON_INTERACTIVE=1— force non-interactive even when isTTY is true (useful for tests + CI assertions).ALTIMATE_AUTO_ANSWER=last): pick the option whose label/description contains a safe keyword (skip,cancel,no,abort,profile only,decline,deny,stop); fall back to the last option (UX convention: safer/cancel typically sits at the end).ALTIMATE_AUTO_ANSWER=first— always pick first option.ALTIMATE_AUTO_ANSWER=skip— returnUnansweredfor all questions.ALTIMATE_AUTO_ANSWER="<exact label>"— exact-match an option's label (case-insensitive)."Running in non-interactive mode (no TTY). Auto-answered with safe defaults: ..."vs the original"User has answered your questions: ..."— so the agent knows the choice was not a real user answer and can adapt strategy.Why not just always pick "cancel"?
Picking cancel/abort blindly fails open in the opposite direction: skills that ask permission to do reasonable work would always get a no, breaking the user's actual intent. The safe-keyword scan tries to match the question author's intent (these are typically "may I do destructive thing X?" prompts) without blocking legitimate flows.
Where this lives
Two reasonable choices:
packages/opencode/src/tool/question.tsshort-circuits before callingQuestion.ask().Question.ask()itself, so any caller ofQuestion.ask(not just the tool) benefits.Option 1 is what this PR ships; the scope is contained and the diff is reviewable. Option 2 is the deeper, more invasive change and may be the right long-term home. Happy to refactor if reviewers prefer.
Test plan
bun test test/tool/question.test.ts→ 8 pass, 0 fail, 12 expect() calls. Verified locally before pushing.ALTIMATE_FORCE_INTERACTIVE=1inbeforeEachso they preserve their original intent under non-TTY CI).ALTIMATE_AUTO_ANSWER=first,=skip,=<exact label>, non-interactive prefix wording.questionfrom Claude Code's Bash tool — expect completion in seconds with auto-answered output, not a hang.Diff size
The patch is roughly 70 lines of source change plus tests. Larger than #935 (the stdin-wedge guard) because the resolution policy has real branching to implement; still bounded to one tool file.
Risk
Low. Default behavior under TTY is unchanged (the new branch only fires when
!isTTYorALTIMATE_NON_INTERACTIVE=1). The non-TTY auto-answer surfaces explicitly in the tool result, so a downstream agent treating the choice as a real user click is impossible.ALTIMATE_FORCE_INTERACTIVE=1provides an escape hatch for any consumer that wants the original behavior even in non-TTY.Links
data-parityskill's PII consent question wedge)plugin-skill-experiments/03-issues-and-fixes.mdIssue chore(deps): Bump @gitlab/gitlab-ai-provider from 3.6.0 to 4.1.0 #5run.ts— same surface area, different code path)Summary by cubic
Fixes hangs in the
questiontool for headless runs by treatingaltimate-code runas non-interactive, defaulting to Unanswered, and clearly labeling the mode. Interactive server flows remain unchanged; addresses #936.ALTIMATE_NON_INTERACTIVE=1enables non-interactive;ALTIMATE_FORCE_INTERACTIVE=1forces interactive;ALTIMATE_NON_INTERACTIVE=0opts out.runsetsALTIMATE_NON_INTERACTIVE=1by default (skipped with--attach).ALTIMATE_AUTO_ANSWER=first,=last, or="<exact label>".ALTIMATE_AUTO_ANSWER.bashtool stripsALTIMATE_NON_INTERACTIVEfrom child env to avoid breaking nested server reply paths.runstdin read is now fully null-safe: useprocess.stdin && !process.stdin.isTTYand skip the read when stdin is undefined to prevent stalls.FORCE_INTERACTIVEoverNON_INTERACTIVE), explicit opt-out (NON_INTERACTIVE=0), non-interactive prefix, and suite isolation fixes.Written for commit 764291c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests