Skip to content

Fix show lookups for supported Basecamp item URLs - #1017

Merged
robzolkos merged 4 commits into
mainfrom
fix/show-basecamp-url-coverage
Nov 8, 2026
Merged

robzolkos merged 4 commits into
mainfrom
fix/show-basecamp-url-coverage

Conversation

@robzolkos

@robzolkos robzolkos commented Nov 8, 2026 •

Copy link
Copy Markdown
Collaborator

Pasting a supported Basecamp item URL into basecamp show should return the item with its full content, rather than fail on an incorrect endpoint or stop at a sparse recording.

This fixes inbox-forward routing, recognizes current reply/card-table/column wire types, and supports the native reply, subtask, Google-document and cloud-file paths already modeled by the URL parser. Typed and generic lookups share canonical endpoint mapping; recurring occurrences use validated parent/date context to build a relative API request.

URL lookups now reject untrusted hosts, conflicting accounts, wrong projects and wrong nested chat/forward parents. Project checks retain the original sparse recording's bucket when enrichment returns a different representation, and missing parent context is carried into the richer response. Numerically equivalent IDs remain accepted. Recording fragments still select their referenced child.

Verification

  • Committed regression tests cover endpoint/content selection, actual wire types, occurrence refetch context, aliases, numeric ID equivalence and URL rejection boundaries.
  • Full local bin/ci passes.
  • Real local BC3 acceptance: 309/309 cases pass — 273 positive cases across 29 resource/context families and 36 deliberate rejections. The harness rebuilds the CLI, compares identity/type/content with dedicated local API responses, and cleans up its short-lived token and loopback server. No mocked API or production/beta requests.
  • Three independent Codex CLI reviews (correctness, URL/account/host security, regression/coverage), plus two follow-ups after addressing the numeric-ID and remote-review findings. Actual review model: gpt-6.1-sol; Codex CLI 0.166.0.

The real-server harness uses an isolated retained local development database and is not committed; the deterministic regression tests run with the normal repository suite. This does not add arbitrary web-page support, new product families, or an ONCE/SQLite verification claim. Real URL acceptance runs use --no-comments; existing tests retain comment-enrichment coverage.

Ready for Rob’s final review once the remote checks are green. Please leave merging to Rob.

@robzolkos
robzolkos requested a review from a team as a code owner November 8, 2026 15:18
Copilot AI balanced review requested due to automatic review settings November 8, 2026 15:18
@github-actions github-actions Bot added commands CLI command implementations tests Tests (unit and e2e) labels Nov 8, 2026
Copilot AI previously approved these changes Nov 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approved

No active review issues are recorded for the changes to basecamp show URL routing and content retrieval.

No open issues 🎉

What changed in this PR

Fixes basecamp show so supported Basecamp item URLs resolve through the correct endpoints and return full content rather than sparse recordings.

Changes:

  • Unifies typed and generic endpoint mapping, adding native item aliases and current wire types.
  • Adds validated occurrence refetching and rejects mismatched URL hosts, accounts, projects, and nested parents.
  • Adds regression coverage for routing, content retrieval, URL rejection, and numerically equivalent IDs.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File Description
internal/​urlarg/​urlarg.go Preserves the parent forward ID from nested reply URLs.
internal/​commands/​show.go Corrects endpoint routing, expands supported types, and validates URL context.
internal/​commands/​show_urls_test.go Adds URL routing, full-content, occurrence, and validation regression tests.
internal/​commands/​show_test.go Updates endpoint expectations and reply/chat fixtures.

Review effort: Balanced

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 4 files

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread internal/commands/show_test.go Resolved
Comment thread internal/commands/show.go Resolved
Comment thread internal/commands/show.go Resolved
Comment thread internal/commands/show_urls_test.go Resolved
Copilot AI balanced review requested due to automatic review settings November 8, 2026 15:28
Copilot AI dismissed their stale review, a newer Copilot review was requested November 8, 2026 15:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Valid nested reply and chat-line URLs can be rejected because enrichment discards the sparse recording's matching parent ID when the dedicated response omits parent.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Preserve nested reply parent when enrichment omits it

internal/​commands/​show.go:282

Valid nested reply and chat-line URLs are rejected when the dedicated response omits parent, because enrichment discards the sparse recording's matching parent ID. Preserve that parent when the richer response omits it; the same issue affects the chat check at line 285. Add nested-URL cases using the parent-less responses already covered by TestShowRefetchesRealReplyContent and TestShowLineRefetchesViaParent.

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 8, 2026 15:41

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread internal/commands/show_urls_test.go Resolved
Copilot AI previously approved these changes Nov 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approved

No active review issues are reported for the URL-routing and full-content retrieval changes.

No open issues 🎉

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 8, 2026 15:49
Copilot AI dismissed their stale review, a newer Copilot review was requested November 8, 2026 15:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approved

No active review issues are reported for the URL lookup and context-validation changes.

No open issues 🎉

Review effort: Balanced

@robzolkos
robzolkos merged commit 7eba5aa into main Nov 8, 2026
32 checks passed
@robzolkos
robzolkos deleted the fix/show-basecamp-url-coverage branch November 8, 2026 16:37
jeremy added a commit that referenced this pull request Nov 9, 2026
…950575

* origin/main:
  Fix show lookups for supported Basecamp item URLs (#1017)
  Stamp v0.25.0 (#1016)
  Put the gist of the writing instructions in every agent session (#1014)
  Stamp v0.24.3 (#1013)
  Read a slow keyring instead of calling its credential missing (#1011)
@robzolkos robzolkos mentioned this pull request Nov 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands CLI command implementations tests Tests (unit and e2e)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants