Skip to content

feat: Implement github_create_issue in write_tools.py + test#2

Merged
mabry1985 merged 1 commit into
mainfrom
feat/github-plugin-1ez.4
Jun 21, 2026
Merged

feat: Implement github_create_issue in write_tools.py + test#2
mabry1985 merged 1 commit into
mainfrom
feat/github-plugin-1ez.4

Conversation

@mabry1985

Copy link
Copy Markdown
Contributor

I'll start by reading the canonical instructions and the files I need to modify.I'll start by reading the canonical instructions and the files I need to modify.Let me look at read_tools.py for the implemented-tool conventions and the existing tests for style.Let me read the existing test files to match conventions.Now I have everything I need. Let me implement the changes. First, update the import in write_tools.py:Now replace the stub body of github_create_issue:Let me check the test config for async support before writing the test.asyncio_mode = "auto" is set and pytest-asyncio is a dev dep, so async test functions run directly. The tool calls run_gh/check_gh_error via the ghplugin.write_tools module namespace, so I'll patch there. Now I'll write the test:Let me verify my work by running the lint and test gate.Ruff passes. The pytest failures are environmental — the homebrew pytest lacks pytest-asyncio, so asyncio_mode=auto is unrecognized (the 6 sync tests pass). Let me confirm the new tests actually pass with the real dev deps in a clean venv.All 11 tests pass (6 existing + 5 new) and ruff check/format both pass. Let me clean up the temp venv.Done. Here's what I implemented:

write_tools.py

  • Updated the module-top import to from .gh_cli import bad_repo, check_gh_error, run_gh.
  • Replaced the github_create_issue stub body with a real gh issue create invocation:
    • Validates repo via bad_repo() first (short-circuits).
    • Builds ["issue", "create", "--repo", repo, "--title", title, "--body", body].
    • Splits labels on comma, strips each, skips blanks, and appends a --label <name> pair per label.
    • Runs via run_gh, checks failures via check_gh_error, and returns out.strip() (the issue URL).
    • Replaced the TODO(team) docstring line with Returns the new issue URL. (kept it a plain string literal per §6).
    • github_comment and github_create_pr were left untouched.

tests/test_write_tools.py (new)

Mocks run_gh in the ghplugin.write_tools namespace (where the tool resolves it) and asserts:

  • success returns the stripped issue URL, with the correct gh issue create argv (repo/title/body);
  • no labels → no --label flag;
  • comma-separated labels split into separate --label flags, stripped, blanks dropped;
  • invalid repo returns the bad_repo error and never shells out;
  • a nonzero gh exit returns the check_gh_error string.

Verification

  • ruff check . → all checks passed; ruff format --check . → 8 files already formatted.
  • pytest -q in a clean 3.12 venv with requirements-dev.txt11 passed (6 existing + 5 new). (The system pytest lacked pytest-asyncio, hence the need for the venv to honor asyncio_mode=auto.)

All acceptance criteria are satisfied.

@protoquinn

protoquinn Bot commented Jun 21, 2026

Copy link
Copy Markdown

👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow.

@protoquinn protoquinn Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

QA Audit — PR #2 | feat: Implement github_create_issue in write_tools.py + test

VERDICT: WARN (CI test still queued — provisional; final verdict on terminal CI)


CI Status

  • test: queued

Diff Review

  • write_tools.py: Replaced stub with gh issue create impl. Imports check_gh_error + run_gh; validates repo via bad_repo(), builds argv, returns issue URL or error. Clean, follows existing patterns.
  • tests/test_write_tools.py (new): 5 async tests — success path, no-labels, comma labels with blanks, bad-repo short-circuit, gh error propagation. All patch in correct namespace (ghplugin.write_tools).

Observations

  • LOW: labels.split(",") would AttributeError on explicit labels=None (default "" makes this unlikely). Consider (labels or "").split(",").
  • LOW: body=None would produce --body None in argv. Same rarity — default "" covers normal calls.

No blocking findings. Implementation is sound, tests are thorough.

— Quinn, QA Engineer

@protoquinn

protoquinn Bot commented Jun 21, 2026

Copy link
Copy Markdown

Submitted COMMENT review on #2.

@mabry1985 mabry1985 merged commit cc917d2 into main Jun 21, 2026
1 check passed
@mabry1985 mabry1985 deleted the feat/github-plugin-1ez.4 branch June 21, 2026 19:51
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.

1 participant