Repository navigation
feat(tools/discourse): add a read-only Discourse topic bridge - #1755
MakerFlyFly wants to merge 4 commits into
Conversation
Projects discussing design and governance on Discourse need structured topic context alongside their existing mail and forge sources. Read a topic and its bounded post stream with a stdlib-only CLI. Preserve rendered bodies and server body-replacement markers, and fail explicitly on missing requested posts or malformed responses. Keep optional API credentials within the configured HTTPS forum origin and path. Wire the package into the workspace, adapter inventory and specifications while declaring partial mail-archive coverage. This is the first implementation slice. Additional read commands, confirmed writes and the remaining archive verbs are follow-up work. Generated-by: Codex (GPT-6)
github-actions
Bot
added
family:tools
Kaap10
left a comment
Kaap10
left a comment
There was a problem hiding this comment.
Thank you for this contribution! This is an exceptionally clean, well-tested, and secure initial slice for #307.
Review Summary
Issue #307 is actively tracked and directly relevant to the project roadmap (Discourse context is needed by projects like Python core, Rust, NixOS, Julia, Docker, and ASF podlings). Delivering this in scoped, incremental PRs is an effective strategy.
Key Highlights & Strengths
- Strict Standard-Library Discipline:
- Zero external runtime dependencies; uses standard library
urllib,http.client,dataclasses, andjson.
- Zero external runtime dependencies; uses standard library
- Robust Security Boundaries (RFC-AI-0004 & AGENTS.md):
- Enforces GET-only at the transport and redirect levels.
SameOriginRedirectHandlerensures credentials (Api-Key,Api-Username) never cross origin or escape the configured forum subpath.- Rejects directory traversal (
..), backslashes, control characters, and ambiguous percent-encoding (%2f,%5c,%25) prior to dispatch. - Forum post content (
cookedHTML) is explicitly treated as untrusted external data.
- Comprehensive Error Handling & Edge Cases:
- Fails fast on missing post IDs rather than returning incomplete thread data.
- Rejects unsupported mega-topics cleanly (
isMegaTopic: true). - Normalizes both legacy string tags and modern Discourse tag objects.
- Complete Repository Integration:
- Correctly registered across
pyproject.toml,uv.lock,.github/labeler.yml,docs/adapters/registry.md,docs/labels-and-capabilities.md,docs/vendor-neutrality.md, andtools/spec-loop/specs/adapters.md. - All 20 CI checks passed green on GitHub Actions.
- Correctly registered across
Follow-Up Design Notes for Future PRs (PR2–PR7)
- Rate-Limiting (HTTP 429) Backoff: The client currently surfaces
Retry-Afteron 429 without unbounded retries. In PR2/PR3 (category lists and full-text searches), consider an optional bounded backoff policy for large sweeps. - Downstream HTML Handling: Downstream skills reading
body_format: "html"should ensure proper sanitization/stripping before rendering into terminal or plain-text markdown summaries.
| from typing import Any | ||
|
|
||
| DEFAULT_TIMEOUT_SECONDS = 30 | ||
|
|
There was a problem hiding this comment.
Strictly enforcing request.get_method() == 'GET' before opening the connection ensures read-only discipline across initial requests and redirects.
| if ( | ||
| origin(destination) != self._origin | ||
| or destination.username is not None | ||
| or not _is_under_path(raw_path, self._raw_path) |
There was a problem hiding this comment.
The single UTF-8 decode pass and explicit check against %, \, control chars, and .. prevents directory traversal and path confusion before requests reach urllib.
|
|
||
| def normalize_topic( | ||
| raw: dict[str, Any], posts: list[dict[str, Any]], config: DiscourseConfig, *, truncated: bool | ||
| ) -> dict[str, Any]: |
There was a problem hiding this comment.
Failing immediately when a requested post chunk is missing from the response avoids returning silently incomplete threads to downstream skills.
| "deleted": bool(post.get("deleted_at") or post.get("user_deleted")), | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
Relaying body_format: "html" alongside body_hidden preserves the server's rendered representation while giving consuming skills unambiguous format metadata.
| | `ponymail` | shipping | [`tools/ponymail/`](../ponymail/) (will be renamed to `tools/mail-archive-ponymail/` in PR3) | ASF's `lists.apache.org` deployment; PMC LDAP OAuth for private lists; anonymous read for public lists; load-bearing for the `security-issue-import`, `security-issue-sync`, and `security-issue-invalidate` skills today. | | ||
| | `hyperkitty` | placeholder | not implemented | Mailman 3's archive UI. Same conceptual surface as PonyMail (per-list archive, search by month + query, per-thread permalink) but a different URL grammar and a different JSON API. | | ||
| | `discourse` | placeholder | not implemented | Topic-based forum, with mailing-list-mode bridging. The verbs map onto Discourse's `/search.json` and `/t/<slug>/<id>.json` endpoints. | | ||
| | `discourse` | partial | [`tools/discourse/`](../discourse/) | Topic-read foundation, not a complete/selectable archive backend. `topic view` reuses `thread_data` fields in forum stream order, which may differ from chronological contract order. Mega topics without an ID stream are unsupported; output does not classify visibility. All contract verbs remain follow-up work in [#307](https://github.com/apache/magpie/issues/307). The verbs map onto Discourse's `/search.json` and `/t/<slug>/<id>.json` endpoints. | |
There was a problem hiding this comment.
Documenting Coverage: partial and clarifying that this is not yet a complete/selectable backend prevents adopters from premature configuration before PR7 completes the archive contract.
|
Thx for your review! I'll keep your suggestions in mind for the follow-up PRs |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
A good first slice of #307: the bridge is GET-only, refuses a redirect to another origin before it can forward the API key, insists on https://, rejects header injection through the credentials, and fails loudly on a missing post instead of returning a partial thread; declaring Coverage: partial until PR7 is the right call. One parsing ambiguity in topic URLs below.
Smaller observations
- See inline comments on
tools/discourse/src/magpie_discourse/topics.py:55.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
| raise DiscourseError(f"Topic URL is not under the configured forum path {base.path}: {ref}") | ||
| topic_path = path[len(base.path) :] | ||
| match = _SHORT_TOPIC_PATH.match(topic_path) or _TOPIC_PATH.match(topic_path) | ||
| if not match or int(match.group("id")) < 1: |
There was a problem hiding this comment.
The short pattern wins, so /t/2026/42 is topic 2026 / post 42. On a forum that allows an all-digit slug that same URL is topic 42 with slug 2026, and this returns topic 2026 without an error (the id check compares against the same parse). Please disambiguate (check the returned slug) or reject the ambiguous form, with a test.
There was a problem hiding this comment.
Got it. Thx for your explanation. I will fix that and add a regression test
There was a problem hiding this comment.
Still working on it, I found some issues with the fix and am looking into them.
There was a problem hiding this comment.
Thanks for flagging this. I investigated it against Discourse's own URL generation and routing. Its slug generator excludes numeric-only generated slugs, and the numeric topic/post route is registered before the slug/topic route. Therefore /t/2026/42 resolves to topic 2026, post 42 in Discourse itself. Private-email notifications can also use native short links such as /t/42/7.
The follow-up restores that route precedence so the bridge reads the same topic as the forum. The earlier rejection introduced a regression for notification links. This is why the fix retains the native interpretation instead of adding a slug lookup or rejecting the short form.
I added CLI regressions for these routes, ASCII-only numeric positions, base paths and canonical URL round trips, and clarified the error for bare non-ASCII numeric IDs. All 279 mocked package tests pass locally, along with ruff and mypy. I have completed manual review of the changes.
Two numeric path segments can identify a numeric slug and topic id or a topic id and post number. Prioritizing the short form can silently fetch a different topic, and comparing the response id cannot detect that. Reject the ambiguous form before HTTP, including escaped numeric slugs. Keep explicit topic ids and unambiguous topic URLs, and document the deliberate compatibility limit without adding network disambiguation. Exercise the CLI, subpath deployments, share links and numeric-slug post URLs with mocked HTTP. Preserve the existing JSON and read-only scope. Generated-by: Codex (GPT-6)
Restore Discourse's numeric topic/post route precedence so notification links such as /t/42/7 resolve to topic 42. Normal Discourse slug generation rejects numeric-only slugs. Use ASCII digits for topic ids and post numbers, and cover native short links, encoded slugs, Unicode input and canonical URL round trips. Align the README and adapter specification with the supported routes. Generated-by: Codex (GPT-6)
Report an ASCII-digit requirement for bare non-ASCII numeric references before attempting URL parsing. Preserve native topic URL routing, Unicode slugs, positive-integer validation and request-free input failures. Cover mixed digits, superscripts and whitespace while checking the CLI error text and retaining the existing credential and request assertions. Generated-by: Codex (GPT-6)
Summary
magpie-discourse topic view <id|url> [--max-posts N]command for structured forum context.contract:mail-archivewithCoverage: partial; this is not yet a complete/selectable archive backend.Proposed delivery plan
We intend to deliver the broader issue as separate, independently useful PRs.
Each includes its implementation, tests and required documentation/integration.
The sequence and details remain open to maintainer feedback.
Only PR1 is implemented here.
Skill integration, mega-topic pagination and response-byte/total-duration budgets remain follow-up design work.
Type of change
Test plan
prek run --all-files --verbose, including offline lychee and all four workspace hooks: 40 hooks passed; 51 workspace members, 9,440 passed, 361 conditional skips, 4 deselected and 26 passing subtests.truncated: true. No forum writes. This input-validation follow-up uses mocked HTTP; the earlier live smoke test is retained as historical evidence.eee77046on the existinge6e5c2efeature baseline; they are not a full test run on the latest upstream main.27268f28is clean; the merged tree was not tested.eee77046; updated-head GitHub CI will be assessed separately.RFC-AI-0004 compliance
Linked issues
Refs #307.
This first slice does not close the broader request.
Notes for reviewers
/t/2026/42reads topic 2026 and/t/42/7reads topic 42. Normal slug generation rejects numeric-only slugs. Topic IDs and post numbers use ASCII digits; Unicode slugs remain supported. See the Discourse routes and slug generation.cookedbody, including posts beyond the output cap. Explicit empty strings, whitespace and placeholders are preserved; missing/null/invalid bodies fail the whole read.body_hiddenrelays the optional booleancooked_hiddenmarker, defaulting to false. It is independent ofhidden; false proves neither original text nor public visibility.truncatedmeans only the post-count limit; it is not a byte or total-time budget. Mega topics without an ordinary ID stream are explicitly unsupported.AI disclosure
AI-generated PR description.
Claude Code assisted the initial prototype; Codex (GPT-6) assisted revisions, tests, validation and PR preparation.
The author has manually reviewed the implementation and all follow-up fixes through
eee77046and takes responsibility for this contribution.