Repository navigation
input(): forward prompt through Conductor and flush pending stdout - #254
Conversation
Completes the py-slang side of source-academy#190: input() was already blocking correctly via conductor.requestInput(), but discarded its prompt argument entirely and never flushed stdout buffered by earlier print()s before blocking. - MiscBuiltins.input now accepts 0-1 args (@Validate(0, 1, ...)), writes the prompt to stdout (matching CPython's input(prompt) behavior) and forwards it to receiveInput/conductor.requestInput. - createInputStream gained a setNextPrompt side-channel so its pull()-driven ReadableStream can pass a per-call prompt through to requestInput(), since pull() itself has no visibility into per-call arguments. - receiveInput now flushes any buffered stdout before blocking, so print()s issued earlier in the same chunk aren't stuck in the buffer while the popup waits. - Context.streams gained an optional flushStdout hook, wired up in PyCseEvaluator.evaluateChunk. - Added src/tests/input.test.ts covering both the with- and without-prompt paths. Depends on the (as yet unpublished) conductor protocol change adding requestInput(prompt?). Points @sourceacademy/conductor at the local checkout via a portal: resolution for now; repin to a real published version once that lands upstream. Note: the portal symlink currently breaks this repo's jest.config.js (its moduleNameMapper/ transformIgnorePatterns assume a real node_modules directory, not a symlink resolving outside rootDir) - verified tests by temporarily copying the built conductor package into node_modules instead; no action needed once a real release replaces the portal link.
There was a problem hiding this comment.
Code Review
This pull request adds support for prompts in the Python input() built-in function, ensuring that any buffered stdout is flushed before blocking on stdin, and passing the prompt to the conductor. A critical issue was identified in createInputStream where the default highWaterMark of 1 on ReadableStream triggers an eager pull() upon initialization, causing the first prompt to be bypassed. Setting highWaterMark: 0 is recommended to ensure input is only pulled on-demand.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
ReadableStream defaults to highWaterMark: 1, which makes it eagerly call pull() as soon as it's constructed - i.e. during evaluator init, before any Python input() call has set a prompt. That fired a phantom conductor.requestInput(undefined) and left each real prompt to be consumed one input() call late (off-by-one). Setting highWaterMark: 0 makes pull() only run in direct response to reader.read(), matching the on-demand behavior the code already assumed. Caught by gemini-code-assist review on PR source-academy#254; added a regression test using the real createInputStream (the existing input.test.ts mocked stdin directly and couldn't have caught this).
…to fix/py-slang-190-input-popup
|
@coderabbitai, any review? |
|
✅ Action performedReview finished.
|
WalkthroughAdds prompt support to the Python input() builtin: a new InputStreamContext type with setNextPrompt, updated createInputStream/receiveInput sequencing that flushes stdout before requesting input, wiring of flushStdout through evaluator and runner streams, updated input() builtin behavior, new tests, and an unrelated package.json dependency resolution override. ChangesPrompt-aware input() flow
Dependency Resolution Override
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PythonProgram
participant MiscBuiltinsInput
participant ReceiveInput
participant StdinStream
participant Conductor
PythonProgram->>MiscBuiltinsInput: input(prompt)
MiscBuiltinsInput->>MiscBuiltinsInput: displayOutput(prompt)
MiscBuiltinsInput->>ReceiveInput: receiveInput(context, prompt)
ReceiveInput->>ReceiveInput: flushStdout()
ReceiveInput->>StdinStream: setNextPrompt(prompt)
StdinStream->>Conductor: requestInput(pendingPrompt)
Conductor-->>StdinStream: response
StdinStream-->>ReceiveInput: value
ReceiveInput-->>MiscBuiltinsInput: typed value
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@package.json`:
- Around line 85-87: The package.json resolutions block still pins
`@sourceacademy/conductor` to a machine-specific portal:/home/vakshay/conductor
path, which must be removed before merge. Update the resolutions entry in
package.json to point to a published release or a git URL pinned to a specific
commit/tag, using the `@sourceacademy/conductor` resolution key so the project
builds reproducibly in CI and for other developers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04140fee-f461-466b-b00e-f2c8246f2735
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
package.jsonsrc/conductor/PyCseEvaluator.tssrc/engines/cse/context.tssrc/engines/cse/streams.tssrc/runner.tssrc/stdlib/misc.tssrc/tests/input.test.tssrc/tests/utils.ts
source-academy/conductor#40 merged and got tagged 0.6.0 (e105c9c), so the temporary portal: dev pin isn't needed anymore. Verified: tsc --noEmit clean, input.test.ts (3/3) and the full stdlib.test.ts suite (1812/1812) all pass against the real published dependency.
…to fix/py-slang-190-input-popup
- requestInput mock in the createInputStream test was declared async
with no await inside, tripping @typescript-eslint/require-await.
It does no actual async work, so drop async and return
Promise.resolve(...) directly.
- Replace two `{} as any` dummy ExprNS.Call args (each with an
eslint-disable comment that wasn't actually suppressing anything)
with `{} as ExprNS.Call`, matching the real parameter type.
martin-henz
left a comment
There was a problem hiding this comment.
Looks good. This is a nice example for using the channel communication between an evaluator and the frontend.
Summary
Closes #190 on the py-slang side.
input()already blocked correctly viaconductor.requestInput()(the CSE machine's async generator loop suspends naturally on thatawait), but two things were missing:MiscBuiltins.inputdiscarded its prompt argument entirely and calledrequestInput()with zero args.print()s before blocking, so context printed just before aninput()call in the same chunk wouldn't reach the user until the whole chunk finished.Changes:
MiscBuiltins.inputnow validates 0-1 args (@Validate(0, 1, "input", true)), writes the prompt to stdout before blocking (matching CPython'sinput(prompt), which prints the prompt with no trailing newline), and forwards it throughreceiveInput(context, prompt).createInputStreamgained asetNextPromptside-channel so itspull()-drivenReadableStreamcan pass a per-call prompt toconductor.requestInput(prompt)—pull()itself has no visibility into per-call arguments.receiveInputnow flushes any buffered stdout (via a new optionalcontext.streams.flushStdouthook, wired up inPyCseEvaluator.evaluateChunk) before blocking.src/tests/input.test.tscovering both the with- and without-prompt paths.Depends on
This depends on an unreleased Conductor protocol change adding
requestInput(prompt?: string)/IHostPlugin.receiveInputRequest: source-academy/conductor#40. Until that merges and a new tag is cut, this branch points@sourceacademy/conductorat a localportal:dev resolution inpackage.json/yarn.lock— that needs to be repinned to the real published tag before merge, and this PR should probably stay in draft/blocked until then.Note for reviewers: the
portal:symlink currently breaks this repo'sjest.config.jslocally (itsmoduleNameMapper/transformIgnorePatternsfor@sourceacademy/conductorassume a realnode_modulesdirectory, not a symlink resolving outsiderootDir) — verified correctness by temporarily swapping in a physical copy of the built conductor package for test runs. This resolves itself once the portal pin is replaced with a real release.Test plan
tsc --noEmitpasses cleanjest src/tests/input.test.tspasses (2/2)jest src/tests/stdlib.test.tssuite passes (1812/1812), confirming no regressions