Repository navigation
feat(tools/discourse): add a read-only Discourse topic bridge - #1755
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)
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.
|
Thx for your review! I'll keep your suggestions in mind for the follow-up PRs |
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.
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)
potiuk
left a comment
There was a problem hiding this comment.
Thank you for checking the topic-URL point against Discourse itself: its slug generator refuses all-digit slugs ("they would be indistinguishable from id's", lib/slug.rb), so /t/2026/42 can only mean topic 2026, post 42, and following Discourse's route precedence is the right reading. The README and spec now say so, and the new tests pin the precedence, the round trip of the canonical URL, and the refusal of non-ASCII digits before any request. With that, this first slice of #307 is ready.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Magpie handles maintainer review:
Contributing guide.
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.