Skip to content

fix(workflows): render gate show_file contents in the interactive prompt#2810

Open
doquanghuy wants to merge 10 commits into
github:mainfrom
doquanghuy:fix/2809-gate-show-file
Open

fix(workflows): render gate show_file contents in the interactive prompt#2810
doquanghuy wants to merge 10 commits into
github:mainfrom
doquanghuy:fix/2809-gate-show-file

Conversation

@doquanghuy
Copy link
Copy Markdown
Contributor

@doquanghuy doquanghuy commented Jun 2, 2026

Description

Closes #2809.

The built-in gate step accepts a show_file config field. The value was read, template-expanded, and stored in output["show_file"], but its contents were never displayed at the interactive prompt — the operator was asked to approve/reject without seeing the referenced file.

This renders show_file inside the interactive gate prompt, before the options. A missing, undecodable, or invalid path degrades to a short one-line notice instead of raising, so a misconfigured path never breaks the prompt.

What changed

  • New GateStep._compose_prompt(message, show_file) folds the show_file material (a path header plus its contents) into the displayed message. GateStep._prompt(message, options) keeps its existing two-argument contract and simply renders the (possibly multi-line) message inside the gate box — display data does not widen the interactive seam, so stubbing _prompt in tests stays stable.
  • New GateStep._read_show_file(...) reads the file as UTF-8, caps output at MAX_SHOW_FILE_LINES, and returns a (could not read file: …) / (file is empty) notice on error/empty/invalid-path (OSError, UnicodeDecodeError, ValueError) instead of raising.
  • All show_file-derived strings that reach the terminal — the displayed path, each file line, and the read-error notice — are stripped of C0 control characters (_sanitize_for_display) so neither file contents nor the path can inject ANSI escape sequences. The file is still opened with the original path.
  • A non-string message (e.g. a YAML numeric literal) is coerced to str for display.

Compatibility

  • Non-interactive (PAUSED) path is unchanged — the file is not read when stdin is not a TTY.
  • Exit codes, on_reject handling, and resume semantics are unchanged.
  • Gates without show_file produce byte-identical output for the common single-line message; a multi-line message is now boxed on every line (previously continuation lines fell outside the box).

Testing

  • Tested locally with uv run specify --help
  • uv sync --extra test && uv run pytest — green
  • Tested with a sample project (if applicable)

Tests in tests/test_workflows.py::TestGateStep cover: interactive rendering of show_file contents; missing / empty / oversized (truncated) / invalid-path files degrading to a notice; control-character stripping of both file contents and the displayed path; non-string message rendering; and the non-interactive path PAUSING and preserving output["show_file"] without reading the file. Gate tests force stdin determinism by rebinding the gate module's sys reference (no process-wide mutation).

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (describe below)

Used Claude to trace the bug in gate/__init__.py, draft the fix and tests, and write this PR body. The behaviour was reproduced and the diff reviewed locally before submission.

@doquanghuy doquanghuy requested a review from mnriem as a code owner June 2, 2026 10:10
@doquanghuy doquanghuy force-pushed the fix/2809-gate-show-file branch from b43191b to 5412c47 Compare June 2, 2026 11:28
@mnriem mnriem requested a review from Copilot June 2, 2026 21:44
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the built-in workflow gate step so that a configured show_file is actually rendered in the interactive prompt, allowing operators to review the referenced file contents before choosing approve/reject. It adds a bounded, non-throwing file-read helper and extends test coverage around the interactive/non-interactive behaviors.

Changes:

  • Render show_file contents inside the interactive gate prompt (before the options).
  • Add _read_show_file() helper that caps output length and degrades to a one-line notice on read/decode errors.
  • Add targeted tests covering interactive rendering, missing files, non-interactive pause behavior, empty files, and truncation.
Show a summary per file
File Description
src/specify_cli/workflows/steps/gate/__init__.py Passes show_file into the gate prompt and safely renders file contents with truncation/error notices.
tests/test_workflows.py Adds regression tests for interactive rendering and safe behavior across missing/empty/large files and non-interactive runs.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

Comment thread src/specify_cli/workflows/steps/gate/__init__.py Outdated
The gate step read and recorded `show_file` but never displayed its
contents at the interactive prompt, so the operator approved/rejected
without seeing the referenced file. Render the file inside the prompt
when stdin is a TTY, with a graceful notice for missing/unreadable
files. Non-interactive PAUSED behaviour, exit codes, resume semantics,
and no-`show_file` output are unchanged.

Closes github#2809.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@doquanghuy doquanghuy force-pushed the fix/2809-gate-show-file branch from 5412c47 to bf2f25c Compare June 3, 2026 04:14
@doquanghuy
Copy link
Copy Markdown
Contributor Author

Thanks @copilot — fixed in the latest push.

show_file is now coerced to str after template evaluation (and for any non-string literal), so a single-expression template that resolves to a non-string (e.g. a number from a prior step) is rendered instead of being silently skipped. The now-redundant isinstance(show_file, str) guard at the prompt was simplified to if show_file:. Added a test covering a templated show_file that resolves to a non-string.

uv run pytest green (3313 passed, 40 skipped); ruff check src/ clean.

@mnriem ready for another look when you have a moment.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

Comment thread src/specify_cli/workflows/steps/gate/__init__.py Outdated
doquanghuy and others added 2 commits June 4, 2026 00:05
…le reads

The gate prompt rendered show_file by passing it as a third positional
argument to _prompt. A test that stubs _prompt with a two-argument lambda
(test_gate_abort_still_halts_with_continue_on_error) then failed once the
branch caught up to main, because the call site passed three arguments to
the two-argument stub.

Compose the show_file material into the displayed message in execute() and
keep _prompt to its (message, options) contract. Display data no longer
widens the interactive seam, so stubbing _prompt stays stable and future
review material can be added without breaking callers. _prompt now renders
a multi-line message inside the gate box.

Also catch ValueError in _read_show_file so a path the OS rejects outright
(e.g. an embedded NUL byte) degrades to a notice instead of crashing the
prompt, matching the helper's stated contract.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@doquanghuy
Copy link
Copy Markdown
Contributor Author

Pushed a fix that also brings the branch up to date with main.

CI failure (the merge-time TypeError). main grew test_gate_abort_still_halts_with_continue_on_error, which stubs GateStep._prompt with a two-argument lambda. This PR had widened _prompt to take show_file as a third positional argument, so the stubbed call blew up once the branch met main.

Rather than patch the stub, I kept _prompt at its original (message, options) contract and compose the show_file material into the displayed message in execute() (new _compose_prompt helper; _prompt now renders a multi-line message inside the gate box). Display data no longer widens the interactive seam, so stubbing _prompt stays stable and future review material can be added without breaking callers — i.e. this class of failure can't recur.

Copilot's open note. _read_show_file now also catches ValueError, so a path the OS rejects outright (e.g. an embedded NUL byte, which Path.open raises before any I/O) degrades to the (could not read file: …) notice instead of crashing the prompt. Added a regression test.

Local: full uv run pytest green, ruff check src/ clean, gate suite + the previously-failing test pass.

@mnriem ready for another look when you have a moment.

The multi-line render loop split the message on newlines, which assumes a
str. A non-string message (e.g. a YAML numeric literal) previously rendered
fine through the old f-string and would now raise on .split. Coerce with
str() to preserve that tolerance, and add a regression test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 7

Comment thread tests/test_workflows.py Outdated
Comment thread tests/test_workflows.py Outdated
Comment thread tests/test_workflows.py Outdated
Comment thread src/specify_cli/workflows/steps/gate/__init__.py Outdated
Comment thread src/specify_cli/workflows/steps/gate/__init__.py
Comment thread tests/test_workflows.py
Comment thread tests/test_workflows.py
… typing

Address review feedback on the gate tests and helper:

- Swap the gate module's sys.stdin for a fixed-isatty stub (shared
  _StubStdin / _force_gate_stdin helpers) instead of setattr on
  sys.stdin.isatty, which is not assignable under some pytest capture
  modes. This also forces the non-interactive tests to a non-TTY so they
  cannot block on input() when run in a real terminal.
- The non-interactive show_file test now hard-fails if _read_show_file is
  called, proving the file is not read on the PAUSED path.
- _compose_prompt accepts a non-string message (e.g. a YAML numeric
  literal) and always returns str via str(message), keeping its annotation
  and docstring accurate; the redundant coercion in _prompt is removed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@doquanghuy
Copy link
Copy Markdown
Contributor Author

Pushed 06bcd43 addressing Copilot's latest review.

  • Test robustness (5 comments). All gate tests now swap the gate module's sys.stdin for a fixed-isatty stub via a shared _force_gate_stdin(monkeypatch, tty=…) helper, mirroring test_gate_abort_still_halts_with_continue_on_error, instead of setattr on sys.stdin.isatty (which isn't assignable under some pytest capture modes). The non-interactive tests now force a non-TTY so they can't go interactive or hang in a real terminal, and test_non_interactive_show_file_still_pauses_without_reading hard-fails if _read_show_file is called — proving the file isn't read on the PAUSED path.
  • _compose_prompt typing. It now coerces with str(message) (annotated message: object) and always returns a str, matching the docstring, whether or not show_file is set; the redundant coercion in _prompt was removed.
  • Multi-line note. The per-line boxing is byte-identical for single-line messages (the normal case) and corrects multi-line messages (continuation lines are now inside the box rather than falling outside it). Detail in the thread.

Full uv run pytest green (3353 passed), ruff check clean.

@mnriem ready for another look when you have a moment.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 2

Comment thread tests/test_workflows.py
Comment thread src/specify_cli/workflows/steps/gate/__init__.py
…s non-TTY

Address review feedback:

- _read_show_file strips C0 control characters (except tab) from each line,
  so a show_file containing ANSI escape sequences (e.g. \x1b[2J) cannot
  clear the screen or spoof the prompt/options when rendered to a terminal.
- Add an autouse fixture on TestGateStep that defaults every gate test to a
  non-TTY stdin, so no test can drop into the interactive prompt and block
  on input() when the suite runs under a real TTY. Interactive tests opt
  back in via _force_gate_stdin(tty=True); the now-redundant explicit
  non-TTY calls were removed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@doquanghuy
Copy link
Copy Markdown
Contributor Author

Pushed 4d52dd9 addressing Copilot's latest review.

  • ANSI injection via show_file. _read_show_file now strips C0 control characters (except tab) from each line, so a referenced file containing ANSI escapes (e.g. \x1b[2J) can't clear the screen or spoof the prompt/options. Sanitizing at the read boundary covers every rendering. New test_read_show_file_strips_control_chars.
  • Deterministic gate tests. Added an autouse fixture on TestGateStep defaulting all gate tests to a non-TTY stdin, so none can drop into the interactive prompt and block on input() under a real TTY; interactive tests opt back in via _force_gate_stdin(tty=True).

Full uv run pytest green (3354 passed), ruff check clean.

@mnriem ready for another look when you have a moment.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

Comment thread tests/test_workflows.py Outdated
_force_gate_stdin rebinds the gate module's `sys` name to a stand-in whose
stdin has a fixed isatty() and which delegates every other attribute to the
real sys, instead of mutating the process-wide sys.stdin. This keeps the
patch local to the gate module and leaves real stdin untouched. The gate
abort test, which used the same process-wide swap, now shares the helper, so
the pattern exists in exactly one place.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@doquanghuy
Copy link
Copy Markdown
Contributor Author

Pushed 8b653d7 addressing Copilot's note: _force_gate_stdin now rebinds the gate module's sys reference to a delegating stand-in (fixed-isatty stdin, every other attribute proxied to the real sys) rather than mutating process-wide sys.stdin, so the patch stays local to the gate module. The one other test using the same swap now shares the helper, so the pattern exists in exactly one place. Full uv run pytest green (3354 passed), ruff clean. @mnriem ready for another look.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 2

Comment thread src/specify_cli/workflows/steps/gate/__init__.py
Comment thread src/specify_cli/workflows/steps/gate/__init__.py
…content

Control characters were stripped from show_file *contents* but the path was
still printed verbatim as the header (`f"{show_file}:"`) and echoed in the
read-error notice, so a show_file path containing ANSI escapes could still
inject terminal sequences. Centralize stripping in `_sanitize_for_display`
and apply it to every show_file-derived string that reaches the terminal —
the displayed path, each file line, and the error notice — while still
opening the file with the original path. Add a test for path sanitization.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@doquanghuy
Copy link
Copy Markdown
Contributor Author

Pushed 4f6f843 + refreshed the PR description for Copilot's two notes. (1) The displayed show_file path (header line and the read-error notice) is now sanitized too, not just the file contents — all routed through a single _sanitize_for_display helper; the file is still opened with the raw path. New test_compose_prompt_sanitizes_show_file_path. (2) Rewrote What changed to describe the actual _compose_prompt design (_prompt keeps its (message, options) contract). Full uv run pytest green (3355 passed), ruff clean. @mnriem ready for another look.

Reuse the existing _CONTROL_CHARS regex directly at the three display sites
instead of wrapping it in a one-line helper.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

Comment thread src/specify_cli/workflows/steps/gate/__init__.py Outdated
…play

The control-char class skipped LF (so an embedded newline in a show_file
path could break the boxed layout) and the C1 range (so \x9b CSI and other
8-bit controls survived). Widen the class to [\x00-\x08\x0a-\x1f\x7f-\x9f]
(still keeping tab).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@doquanghuy
Copy link
Copy Markdown
Contributor Author

Pushed 3905fbc: the gate _CONTROL_CHARS class now also strips LF and the C1 range ([\x00-\x08\x0a-\x1f\x7f-\x9f], tab still kept), closing the newline-in-path and \x9b/CSI gaps Copilot flagged. One-line regex change; extended the existing path test to cover them. Full uv run pytest green (3355 passed), ruff clean. @mnriem ready for another look.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Gate show_file is recorded but never displayed at the interactive prompt

3 participants