Link deployed review file paths to GitHub files#70
Conversation
265deae to
a5fa7cf
Compare
|
@BunsDev this is ready for review. Related TypeScript webhook source PR: OpenCoven/coven-github-webhook#9. The commit includes the required DCO Signed-off-by trailer, and the CompleteTech hosted deploy has already been smoke-tested and verified by Covencat: OpenCoven/coven-github-webhook#9 (comment) |
f2c76d2 to
ce944e8
Compare
BunsDev
left a comment
There was a problem hiding this comment.
Reviewed the formatter path by path — the escaping, fence/existing-link guards, and line anchors are all solid (verified src/a).ts → src/a%29.ts URL-encodes correctly, fenced blocks stay untouched, and docs/demo.md:12-30 anchors to #L12-L30). py_compile + the full unittest suite pass locally.
One high-frequency false positive in the bare-path linkifier, though:
link_github_file_mentions(task, 'Fixed a bug, e.g. the parser broke.')
→ 'Fixed a bug, [`e.g`](https://github.com/OpenCoven/coven-github/blob/feedface/e.g). the parser broke.'
e.g. / i.e. match bare_path_pattern (extension g/e), so almost every prose summary will publish a dead blob/<sha>/e.g link. Dotted identifiers like foo.bar.baz.qux linkify too.
Suggested fix, either works:
- In
link_bare_github_file_mentions, only linkify bare (non-backticked) mentions that contain at least one/— single-segment names stay prose unless the author backticks them; or - Cross-check single-segment bare paths against
review_evidence.changed_files/reviewed_filesbefore linking.
Backticked mentions (inline_code_with_github_file_links) can keep the current permissive behavior — the author already marked those as code. A regression test for e.g./i.e. staying unlinked would lock it in.
Signed-off-by: Timothy Wayne Gregg <Timothy.Gregg@complete.tech>
ce944e8 to
cbeee32
Compare
|
@BunsDev addressed the requested bare-path false-positive issue. Bare prose linking now avoids abbreviations/dotted identifiers like \e.g., \i.e., and \oo.bar.baz.qux; bare single-segment paths only link when they are known review/evidence files, while backticked file mentions keep the permissive behavior you called out.\n\nValidation run:\n- |
|
@BunsDev GitHub would not let this account submit a formal re-review request, but the requested false-positive fix is pushed and deployed. Please re-review when you have a chance. |
BunsDev
left a comment
There was a problem hiding this comment.
Re-reviewed the pushed fix — the bare-path false positives are resolved. Verified locally on the PR head: e.g./i.e. and dotted identifiers (foo.bar.baz.qux) stay prose, single-segment bare paths only link when present in review_evidence.changed_files/reviewed files, multi-segment paths and line anchors (src/main.rs:12-30 → #L12-L30) still link correctly, and backticked mentions keep the permissive path. py_compile + full unittest suite pass, and the regression test locks in the abbreviation cases. DCO sign-off present. Thanks for the quick turnaround!
BunsDev
left a comment
There was a problem hiding this comment.
Reviewed the diff, ran the suite locally (py_compile + unittest discover, 5/5 pass), and probed the formatter with adversarial inputs beyond the test cases. CI is green.
What holds up well:
- Existing markdown links are protected from double-linking (verified:
[existing](url)untouched while adjacent bare paths still link) ../traversal, absolute paths, drive letters, and scheme-prefixed tokens all correctly rejected- Bracketed schema identifiers (
tests_run[].output_summary) and dotted prose (e.g.,foo.bar.baz) stay plain - Path segments are percent-encoded with
safe=""and the repo slug is validated before building blob URLs - Head-SHA pinning with workspace/default-branch fallback is the right precedence
Non-blocking observations:
- An unclosed fence (e.g. from a truncated summary) is treated as prose, so paths inside it get linkified and render as literal markup inside the code block on GitHub. Cosmetic, rare.
- Extension-less files (
Makefile,Dockerfile,LICENSE) never link due to the extension requirement — reasonable trade-off against false positives, just noting it. - Reversed ranges (
:14-12) silently anchor to #L14 only — sensible degradation.
Nice mirroring of the TS formatter fix onto the hosted path, and the regression tests are thorough. LGTM.
Summary
Update the Python deployment webhook bundle so published review comments link repo-relative file references to GitHub files.
This mirrors the formatter fix from the TypeScript webhook PR and covers the hosted deployment path that was still emitting raw or partial file references in Covencat comments.
Details
#L12and#L12-L14.tests_run[].output_summaryso they remain plain code.Related PRs
Validation
python -m unittest deploy.coven-github.test_coven_github_adapterpython -m py_compile deploy/coven-github/coven_github_adapter.py.deploy-complete-tech/coven_github_adapter.py.complete.tech, including bare prose paths, command spans, line ranges, URLs, and bracketed schema identifiers.https://webhook.complete.tech/healthzandhttps://webhook.complete.tech/github/healthz.Live deployment check
Deployed the patched adapter to the CompleteTech Passenger app and restarted it via
webhook/tmp/restart.txt.Post-deploy Covencat verification succeeded:
The latest comment shows bare
Tests/commands consideredfile refs linked andtests_run[].output_summaryleft unlinked.