Skip to content

feat: add URL detection and integrity hash parsing to harness package - #1095

Merged
ggallen merged 1 commit into
mainfrom
feat/url-detection-integrity-hash
May 26, 2026
Merged

feat: add URL detection and integrity hash parsing to harness package#1095
ggallen merged 1 commit into
mainfrom
feat/url-detection-integrity-hash

Conversation

@ggallen

@ggallen ggallen commented May 18, 2026

Copy link
Copy Markdown
Member

Summary

  • Add IsURL, IsAbsPath, IsRelPath helpers for classifying resource references
  • Add ParseIntegrityHash for extracting and validating #sha256=... fragments from URLs
  • Foundation for ADR-0038 universal harness access — no callers yet

Test plan

  • Unit tests for all functions with edge cases (userinfo, path traversal, hex validation)
  • go test ./internal/harness/... passes
  • make lint passes

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown

Site preview

Preview: https://9c414d60-site.fullsend-ai.workers.dev

Commit: 242828836d38af12d9a0d9c637023baacb096981

@fullsend-ai-review

fullsend-ai-review Bot commented May 18, 2026

Copy link
Copy Markdown

Review

Findings

No findings.

Previous run

Review

Findings

No findings.

Clean utility addition — URL validation has appropriate defense-in-depth (HTTPS-only, userinfo rejection, belt-and-suspenders @ check), ParseIntegrityHash correctly validates hash format and rejects path traversal, and the test suite covers edge cases well. Plan doc updated to match implementation.

Previous run (2)

Review

Findings

No findings.

Previous run (3)

Review

Findings

Low

  • [correctness] internal/harness/url.go:43ParseIntegrityHash godoc does not mention that uppercase hex characters (A-F) are rejected. SHA-256 hashes are commonly rendered with mixed case, so callers may be surprised when a valid-looking hash like ABCDEF01... returns hasHash=false. Consider documenting the lowercase-only requirement in the godoc, or accepting uppercase with strings.ToLower normalization.

Info

  • [correctness] internal/harness/url.go:39IsRelPath("") returns true for an empty string. This is tested and intentional, but empty string is not a meaningful file path. Future callers that pass user input through IsRelPath without an empty-string guard may misclassify missing values as relative paths.

Comment thread internal/harness/url.go
Comment thread internal/harness/url.go
@fullsend-ai-review

Copy link
Copy Markdown

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

  • #1099 — ParseIntegrityHash godoc does not mention that uppercase hex characters (A-F) are rejected. SHA-256 hashes are commonly rendered with mixed case, so callers may be surprised when a valid-looking hash returns hasHash=false.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels May 19, 2026
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels May 26, 2026
Add utility functions for detecting URLs, absolute/relative paths,
and parsing SHA256 integrity hashes from URL fragments. Includes
comprehensive tests covering HTTPS validation, scheme rejection,
hash extraction, and edge cases.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Greg Allen <gallen@redhat.com>
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels May 26, 2026
@ggallen
ggallen added this pull request to the merge queue May 26, 2026
Merged via the queue into main with commit 94db499 May 26, 2026
9 of 11 checks passed
@ggallen
ggallen deleted the feat/url-detection-integrity-hash branch May 26, 2026 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants