Skip to content

Commit 18dd708

Browse files
committed
fix(feedback): address 3 self-review findings
1. **webbrowser.open return value silently dropped on headless boxes** The command printed "Opening GitHub in your browser..." and exited 0 even when webbrowser.open returned False (no GUI, no $BROWSER, CI runner, container without DISPLAY). Now we capture the return value, surface "no browser available — copy the URL above" to stderr, and only print "Opened GitHub in your browser." after a confirmed True return. 2. **_openkb_version was the third copy of the same logic** openkb/__init__.py exports __version__ via importlib.metadata with fallback "0.0.0+unknown"; openkb/agent/chat.py wraps it as _openkb_version(); cli.py was re-implementing it with fallback "unknown" — already drifted. Replaced with the same one-liner used in chat.py: `from openkb import __version__; return __version__`. Now all three call sites agree. 3. **type prompt hung in non-TTY contexts (regression of PR #48)** PR #48 introduced _stdin_is_tty() specifically because adding a prompt without a TTY check broke automation pipelines. PR #53's second prompt (asking for feedback type) repeated the same pattern. Now it skips the prompt when stdin isn't a TTY and falls through to `--type other`. Pipes / CI work without flags now: echo "..." | openkb feedback "msg" # works, type=other, no label 4 new regression tests: - test_feedback_skips_type_prompt_when_stdin_is_not_a_tty - test_feedback_warns_when_webbrowser_open_returns_false - test_feedback_confirms_when_webbrowser_open_succeeds - test_openkb_version_helper_matches_package_version 331 tests pass (327 prior + 4 new).
1 parent 9ca0270 commit 18dd708

2 files changed

Lines changed: 100 additions & 19 deletions

File tree

openkb/cli.py

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1150,15 +1150,15 @@ def status(ctx):
11501150

11511151

11521152
def _openkb_version() -> str:
1153-
"""Return the installed openkb package version, or 'unknown' if it
1154-
can't be resolved (e.g. running from an editable checkout that isn't
1155-
on PYTHONPATH as a distribution).
1153+
"""Return the installed openkb package version.
1154+
1155+
Delegates to ``openkb.__version__`` so the chat REPL, feedback issue
1156+
body, and any future caller all surface the same fallback string
1157+
(``0.0.0+unknown`` from ``openkb/__init__.py``). Mirrors
1158+
``openkb.agent.chat._openkb_version``.
11561159
"""
1157-
try:
1158-
from importlib.metadata import version
1159-
return version("openkb")
1160-
except Exception:
1161-
return "unknown"
1160+
from openkb import __version__
1161+
return __version__
11621162

11631163

11641164
def _collect_feedback_diagnostics(ctx) -> dict[str, str]:
@@ -1253,13 +1253,20 @@ def feedback(ctx, message, feedback_type, print_url, no_diagnostics):
12531253
return
12541254

12551255
if feedback_type is None:
1256-
feedback_type = click.prompt(
1257-
"Type",
1258-
default="other",
1259-
type=click.Choice(_FEEDBACK_TYPES),
1260-
show_default=True,
1261-
show_choices=True,
1262-
)
1256+
# Skip the prompt in non-TTY contexts (CI / piped stdin) so
1257+
# ``echo "msg" | openkb feedback`` doesn't hang on the second
1258+
# prompt after consuming all piped input for the message body.
1259+
# Mirrors the ``_stdin_is_tty()`` gate added in PR #48.
1260+
if _stdin_is_tty():
1261+
feedback_type = click.prompt(
1262+
"Type",
1263+
default="other",
1264+
type=click.Choice(_FEEDBACK_TYPES),
1265+
show_default=True,
1266+
show_choices=True,
1267+
)
1268+
else:
1269+
feedback_type = "other"
12631270

12641271
diagnostics = {} if no_diagnostics else _collect_feedback_diagnostics(ctx)
12651272
url = _build_feedback_url(message, feedback_type, diagnostics)
@@ -1268,14 +1275,25 @@ def feedback(ctx, message, feedback_type, print_url, no_diagnostics):
12681275
click.echo(url)
12691276
return
12701277

1271-
click.echo("Opening GitHub in your browser to file the issue.")
1272-
click.echo("If nothing happens, copy this URL into a browser yourself:")
1278+
click.echo("Copy this URL into a browser if the auto-open below fails:")
12731279
click.echo(f" {url}")
12741280

12751281
import webbrowser
12761282
try:
1277-
webbrowser.open(url)
1283+
opened = webbrowser.open(url)
12781284
except Exception as exc:
12791285
# webbrowser.open rarely raises but be defensive — the printed URL
12801286
# above is the fallback path.
12811287
click.echo(f" (browser auto-open failed: {exc})", err=True)
1288+
return
1289+
1290+
# ``webbrowser.open`` returns False on headless boxes (no GUI, no
1291+
# ``BROWSER`` env) without raising. Without this check we'd silently
1292+
# print "Opened" and the user would think the issue was filed.
1293+
if opened:
1294+
click.echo("Opened GitHub in your browser.")
1295+
else:
1296+
click.echo(
1297+
" (no browser available — copy the URL above to file the issue)",
1298+
err=True,
1299+
)

tests/test_feedback.py

Lines changed: 64 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,8 @@ def test_feedback_no_diagnostics_strips_block_from_body():
195195
def test_feedback_prompts_for_type_when_not_given_via_flag():
196196
"""If --type isn't on the command line, prompt the user."""
197197
runner = CliRunner()
198-
with patch("webbrowser.open") as mock_open:
198+
with patch("webbrowser.open") as mock_open, \
199+
patch("openkb.cli._stdin_is_tty", return_value=True):
199200
result = runner.invoke(
200201
cli, ["feedback", "--print-url", "missing-type prompt test"],
201202
input="feature\n",
@@ -205,3 +206,65 @@ def test_feedback_prompts_for_type_when_not_given_via_flag():
205206
# The URL should be tagged with the chosen type (mapped to 'enhancement').
206207
url_line = [ln for ln in result.output.splitlines() if "issues/new" in ln][-1]
207208
assert "labels=enhancement" in url_line
209+
210+
211+
# ---------------------------------------------------------------------------
212+
# Regressions from the self-review on PR #53
213+
# ---------------------------------------------------------------------------
214+
215+
216+
def test_feedback_skips_type_prompt_when_stdin_is_not_a_tty():
217+
"""In CI / piped contexts the second prompt would hang or abort
218+
confusingly — the command must fall through to a default."""
219+
runner = CliRunner()
220+
with patch("webbrowser.open"), \
221+
patch("openkb.cli._stdin_is_tty", return_value=False):
222+
result = runner.invoke(
223+
cli, ["feedback", "--print-url", "non-tty feedback"],
224+
)
225+
226+
assert result.exit_code == 0, result.output
227+
# Non-TTY → falls back to "other", which has no label param
228+
url_line = [ln for ln in result.output.splitlines() if "issues/new" in ln][-1]
229+
assert "labels=" not in url_line
230+
# And the title should NOT have a type prefix
231+
assert "%5Bother%5D" not in url_line # urlencoded "[other]"
232+
233+
234+
def test_feedback_warns_when_webbrowser_open_returns_false():
235+
"""`webbrowser.open` returns False on headless boxes without raising —
236+
the command must surface that to the user, not silently pretend
237+
success."""
238+
runner = CliRunner()
239+
with patch("webbrowser.open", return_value=False) as mock_open:
240+
result = runner.invoke(
241+
cli, ["feedback", "--type", "bug", "headless test"],
242+
)
243+
244+
assert result.exit_code == 0, result.output
245+
mock_open.assert_called_once()
246+
# The success-confirmation message must NOT appear
247+
assert "Opened GitHub in your browser" not in result.output
248+
# The user must see a clear "no browser available" indication
249+
assert "no browser available" in result.output
250+
251+
252+
def test_feedback_confirms_when_webbrowser_open_succeeds():
253+
runner = CliRunner()
254+
with patch("webbrowser.open", return_value=True):
255+
result = runner.invoke(
256+
cli, ["feedback", "--type", "bug", "happy path"],
257+
)
258+
259+
assert result.exit_code == 0, result.output
260+
assert "Opened GitHub in your browser" in result.output
261+
262+
263+
def test_openkb_version_helper_matches_package_version():
264+
"""`_openkb_version` in cli.py must delegate to `openkb.__version__`
265+
so the chat REPL and the feedback issue body never disagree on the
266+
fallback string."""
267+
from openkb import __version__
268+
from openkb.cli import _openkb_version
269+
270+
assert _openkb_version() == __version__

0 commit comments

Comments
 (0)