Skip to content

fix(tools/fossil): address the requested changes in review feedback of Fossil SCM integration#784

Merged
potiuk merged 3 commits into
apache:mainfrom
onlyarnav:fossil-scm-fixes
Jul 8, 2026
Merged

fix(tools/fossil): address the requested changes in review feedback of Fossil SCM integration#784
potiuk merged 3 commits into
apache:mainfrom
onlyarnav:fossil-scm-fixes

Conversation

@onlyarnav

Copy link
Copy Markdown
Contributor

Summary

Addresses follow-up review feedback on the merged Fossil SCM integration (#736):

  • Parser Unit Tests: Added unit tests in tools/vcs/tests/test_vcs.py to cover FossilBackend.status and FossilBackend.log timeline output parsing.
  • CLI Argument Hardening: Added the -- separator before field name/value inputs in ticket commands to prevent options confusion.
  • Resource Leaks: Wrapped SQLite connections using contextlib.closing to ensure handles are closed on exceptions.
  • Safety warning: Swapped bare except Exception: pass catches with warning prints to stderr.

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving — this cleanly addresses the four review-feedback items on the Fossil integration:

  • SQLite connection-leak fix via contextlib.closing(...) in find_repo_db / query_db — connections now close on the exception path too (parameterized queries preserved).
  • Tighter exception handling in ticket.py: except Exceptionexcept FossilError, warning to stderr instead of a silent pass.
  • -- options-terminator hardening (with -R moved before --) — the right defensive pattern for the ticket add/set arg vectors.
  • Good added test coverage for the vcs status/log parser (EDITED/ADDED/DELETED→M/A/D, extras→?, the VCSError→empty-status path, timeline + grep, bracket-in-comment case).

No credential/URL/injection surface introduced — list-form subprocess.run (no shell), clean_env(), parameterized SQL, DB-sourced artifact UUIDs. SPDX headers present.

One non-blocking follow-up worth a quick manual check: the -- hardening assumes Fossil's own arg parser honors -- in ticket add/set; the tests are mocked, so that isn't exercised against a real fossil binary. Thanks @onlyarnav.


This review was drafted by an AI-assisted tool and confirmed by a Magpie maintainer. The maintainer approving this PR has read the findings and signed off.

More on how Magpie handles maintainer review: CONTRIBUTING.md.

@potiuk

potiuk commented Jul 8, 2026

Copy link
Copy Markdown
Member

I pushed a small follow-up (rebased on latest main): the three nested mock_run closures in tools/vcs/tests/test_vcs.py were missing type annotations, which workspace-mypy rejects (no-untyped-def) — annotated them as (args: list[str], cwd: str | None = None, **kwargs: object) -> str. mypy/ruff/pytest for tools/vcs pass locally and CI is now green. Merging. Thanks @onlyarnav.

@potiuk potiuk merged commit 73d94cd into apache:main Jul 8, 2026
10 checks passed
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.

2 participants