Add basecampAuthRoutableUrl trait and guard its consumers#282
Add basecampAuthRoutableUrl trait and guard its consumers#282
Conversation
github-actions
Bot
added
typescript
Spec Change ImpactChanges:
SDK Regeneration:All SDKs need to be regenerated due to the addition of the trait. Breaking Change:No breaking changes detected — no operations, types, or resources were removed. Checklist for SDK Updates:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b87f4076f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR formalizes the “authenticated API-host hop → redirected signed URL hop” download contract by introducing a Smithy trait, annotating affected download_url fields, and adding a CI guard to prevent Go SDK regressions where the signed-download primitive is called without the authenticated first hop.
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.
Changes:
- Add
@basecampAuthRoutableUrlSmithy trait and apply it toUpload.download_urlandCampfireLineAttachment.download_url. - Add a shell-based CI check to deny-list
fetchSignedDownloadcall sites outsidego/pkg/basecamp/download.go, and wire it intomake check. - Update the TypeScript UploadsService test fixture to use an API-host-shaped
download_url.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
spec/basecamp.smithy |
Imports and applies the new trait to the two download_url members. |
spec/basecamp-traits.smithy |
Defines and documents the basecampAuthRoutableUrl trait and OpenAPI extension mapping. |
openapi.json |
Adds x-basecamp-auth-routable-url to the two affected download_url schemas. |
typescript/src/generated/openapi-stripped.json |
Carries through the same OpenAPI extension into the TS-generated artifact. |
scripts/check-auth-routable-consumers.sh |
Adds a CI guard to restrict hop-2 primitive usage to the correct Go download flow. |
Makefile |
Adds auth-routable-check target and includes it in the omnibus check. |
typescript/tests/services/uploads.test.ts |
Fixes fixture URL shape away from example.com to an API-host-shaped URL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/check-auth-routable-consumers.sh">
<violation number="1" location="scripts/check-auth-routable-consumers.sh:11">
P2: Scope the exemption to the one allowed call in `fetchAPIDownload`, not the whole file. Right now any extra `fetchSignedDownload` call added later in `download.go` would bypass the CI guard.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a765ed589f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review on PR #282: scripts/check-auth-routable-consumers.sh now enforces two rules instead of just a file-level exemption: 1. No fetchSignedDownload references in non-test Go code outside download.go (unchanged). 2. Exactly one method-call site (`.fetchSignedDownload(`) inside download.go. The function declaration lacks the leading `.` so it's excluded; a second call site silently bypassing rule 1 now fails the guard and surfaces in review (cubic-dev-ai). spec/basecamp-traits.smithy drops the hardcoded `client.go:499` line reference for `Client.followPagination` — line numbers drift, the symbol/file is enough (copilot-pull-request-reviewer).
Address PR #282 review: the previous filter `grep -v '/download.go:'` matched any filename suffix, so a forbidden `fetchSignedDownload` call in, e.g., `go/pkg/basecamp/oauth/download.go` would have been silently allowlisted. Anchor to the exact path `^go/pkg/basecamp/download.go:` (chatgpt-codex-connector).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3169ef0ea1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
PR #282 review follow-ups: Run the guard in CI (chatgpt-codex-connector). A lint wired only into make check does not protect pull requests — the existing GitHub Actions test-go job invokes individual checks directly, not the omnibus target. Add an auth-routable consumer check step alongside the existing check-service-drift.sh step in test.yml. The test-go job already runs with contents: read permissions and other shell-based checks; this follows the same pattern and introduces no new privileges or secrets. Portabilize the grep patterns (copilot-pull-request-reviewer). Rule 1 previously used `grep -r --include=... --exclude=...` and `\b` — both GNU/BSD grep extensions outside POSIX. Switch to `git grep` with a pathspec (`:!*_test.go` excludes test files; only tracked files are scanned, which also naturally ignores build artifacts) and a POSIX word-boundary pattern `(^|[^[:alnum:]_])`. Rule 2 already used `\.` so no change was needed there. Both failure paths re-verified after the switch: external reference (vaults.go), subpath escape (oauth/download.go), and second call site in download.go.
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
github-actions
Bot
added
github-actions
PR #282 follow-up review: Widen Rule 1's pattern to catch method-value uses of fetchSignedDownload (chatgpt-codex-connector). The previous pattern required the identifier to be followed by `(`, so `fn := c.fetchSignedDownload` outside download.go would capture the method value and evade the guard; calling `fn(ctx, url)` later would bypass the two-hop invariant without tripping CI. New pattern matches the identifier followed by any non-word char or end-of-line, which covers direct calls, method-value captures, and bare-identifier mentions (the last being the plan's intentional soft signal for stale doc examples). Verified with a simulated capture. List both fixture path shapes in the trait docs (copilot-pull-request-reviewer). The trait is applied to both Upload.download_url and CampfireLineAttachment.download_url, whose real fixtures use different path shapes: Upload uses `/{accountId}/blobs/{blob}/download/{filename}` (spec/fixtures/uploads/ get.json) while CampfireLineAttachment uses `/{accountId}/buckets/{bucketId}/uploads/{id}/download/{filename}` (spec/fixtures/campfires/upload_line_get.json). The docs previously showed only the second shape, misleading Upload test authors.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bd2dd27ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/check-auth-routable-consumers.sh">
<violation number="1" location="scripts/check-auth-routable-consumers.sh:19">
P2: Rule 1 no longer limits search scope to Go files, so non-Go files can incorrectly fail the consumer guard.</violation>
<violation number="2" location="scripts/check-auth-routable-consumers.sh:20">
P1: `git grep` pathspecs are relative to the current working directory. In CI, the `test-go` job runs this script from `working-directory: go` (invoked as `../scripts/check-auth-routable-consumers.sh`), so the pathspec `go/pkg/basecamp/` resolves to `go/go/pkg/basecamp/` — which doesn't exist. This makes Rule 1 a silent no-op (finds zero matches, passes vacuously) and the `grep -v` filter on `go/pkg/basecamp/download.go:` also never matches. The guard is completely ineffective in CI.
Normalize to the repo root at the top of the script before any path-dependent operations.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Marks Upload.download_url and CampfireLineAttachment.download_url as API-host URLs that require the two-hop authenticated-then-signed fetch rather than pre-signed external URLs. The trait's /// block carries the full consumer contract (hop mechanics, test-fidelity requirements, retry semantics, pagination exemption, public-vs-internal primitive division) so hand-rolled helpers can implement the flow without reading SDK source. Guards regression with scripts/check-auth-routable-consumers.sh — a call-site deny-list that fails if fetchSignedDownload is invoked outside go/pkg/basecamp/download.go. After PR #278, the only legitimate caller is fetchAPIDownload, which runs the authenticated first hop before the signed fetch; any other caller is either re-inventing the two-hop flow or skipping hop 1. Wired into make check via auth-routable-check. Fixes the TypeScript uploads test fixture that still used example.com, so the spec-shape-only test no longer teaches the wrong URL shape. openapi.json diff is exactly two new x-basecamp-auth-routable-url: {} entries on the Upload and CampfireLineAttachment download_url property schemas; no generator in any SDK consumes field-level x-basecamp-* extensions, so no downstream code churn.
Address review on PR #282: scripts/check-auth-routable-consumers.sh now enforces two rules instead of just a file-level exemption: 1. No fetchSignedDownload references in non-test Go code outside download.go (unchanged). 2. Exactly one method-call site (`.fetchSignedDownload(`) inside download.go. The function declaration lacks the leading `.` so it's excluded; a second call site silently bypassing rule 1 now fails the guard and surfaces in review (cubic-dev-ai). spec/basecamp-traits.smithy drops the hardcoded `client.go:499` line reference for `Client.followPagination` — line numbers drift, the symbol/file is enough (copilot-pull-request-reviewer).
Address PR #282 review: the previous filter `grep -v '/download.go:'` matched any filename suffix, so a forbidden `fetchSignedDownload` call in, e.g., `go/pkg/basecamp/oauth/download.go` would have been silently allowlisted. Anchor to the exact path `^go/pkg/basecamp/download.go:` (chatgpt-codex-connector).
PR #282 review follow-ups: Run the guard in CI (chatgpt-codex-connector). A lint wired only into make check does not protect pull requests — the existing GitHub Actions test-go job invokes individual checks directly, not the omnibus target. Add an auth-routable consumer check step alongside the existing check-service-drift.sh step in test.yml. The test-go job already runs with contents: read permissions and other shell-based checks; this follows the same pattern and introduces no new privileges or secrets. Portabilize the grep patterns (copilot-pull-request-reviewer). Rule 1 previously used `grep -r --include=... --exclude=...` and `\b` — both GNU/BSD grep extensions outside POSIX. Switch to `git grep` with a pathspec (`:!*_test.go` excludes test files; only tracked files are scanned, which also naturally ignores build artifacts) and a POSIX word-boundary pattern `(^|[^[:alnum:]_])`. Rule 2 already used `\.` so no change was needed there. Both failure paths re-verified after the switch: external reference (vaults.go), subpath escape (oauth/download.go), and second call site in download.go.
PR #282 follow-up review: Widen Rule 1's pattern to catch method-value uses of fetchSignedDownload (chatgpt-codex-connector). The previous pattern required the identifier to be followed by `(`, so `fn := c.fetchSignedDownload` outside download.go would capture the method value and evade the guard; calling `fn(ctx, url)` later would bypass the two-hop invariant without tripping CI. New pattern matches the identifier followed by any non-word char or end-of-line, which covers direct calls, method-value captures, and bare-identifier mentions (the last being the plan's intentional soft signal for stale doc examples). Verified with a simulated capture. List both fixture path shapes in the trait docs (copilot-pull-request-reviewer). The trait is applied to both Upload.download_url and CampfireLineAttachment.download_url, whose real fixtures use different path shapes: Upload uses `/{accountId}/blobs/{blob}/download/{filename}` (spec/fixtures/uploads/ get.json) while CampfireLineAttachment uses `/{accountId}/buckets/{bucketId}/uploads/{id}/download/{filename}` (spec/fixtures/campfires/upload_line_get.json). The docs previously showed only the second shape, misleading Upload test authors.
Step 5 previously required test stub URLs to use the configured API base host. After rebasing onto main, that conflicts with two already- merged conventions: - Go's `download_test.go` (PR #278) uses `storage.3.basecamp.com` for ~16 primitive-level tests so the SDK's host-rewrite step is visible in test setup. - The TS `UploadsService.download` test added by PR #281 follows the same pattern at the service level. Both correctly assert the auth-hop sequence (Bearer on hop 1, none on hop 2), which is the actual load-bearing guard against the PR #278 bug shape — that bug passed because the test never asserted the auth hop fired, not because of the host choice. Relax step 5 to make the auth-hop assertion the explicit MUST, document both URL host conventions as acceptable (with API-host preferred for fixture-shape mirroring), and keep the schema-shape-only exemption.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f79674757c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR #282 review (Codex P1, cubic-dev-ai P1+P2, Copilot ×2): The CI test-go job runs every shell step from working-directory: go and invokes the script as `../scripts/check-auth-routable-consumers.sh`. With cwd=go/, the previous script's `git grep` pathspec `go/pkg/basecamp/` resolves to `go/go/pkg/basecamp/` (silent no-op, guard passes vacuously) and the literal path `go/pkg/basecamp/download.go` hits "No such file or directory" (Rule 2 fails for the wrong reason, breaking CI before tests run). Reproduced locally by cd'ing into go/ and running the script directly. Resolve paths from the script location instead, matching the same SCRIPT_DIR/REPO_DIR pattern check-service-drift.sh already uses: - `git -C "$REPO_DIR" grep ...` for Rule 1 (pathspec resolves at REPO_DIR, not caller cwd). - Absolute "$DOWNLOAD_GO" for Rule 2's grep on a single file. Also scope Rule 1's pathspec back to `*.go` files via `:(glob)go/pkg/basecamp/**/*.go`, matching the previous --include='*.go' behavior. The earlier git-grep refactor dropped the Go-only restriction, which let non-Go files (JSON, docs) be subject to the soft-signal heuristic (cubic-dev-ai P2). Verified: passes from repo root, from go/ (CI cwd simulation), and from /tmp; both failure paths still pinpoint correctly when invoked from go/; a JSON file mentioning fetchSignedDownload no longer trips Rule 1.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Rebased onto
mainafter #278 merged.Summary
basecampAuthRoutableUrlSmithy trait and apply toUpload.download_urlandCampfireLineAttachment.download_url. The trait's///block documents the two-hop authenticated-then-signed fetch contract so hand-rolled helpers can implement it without reading SDK source.scripts/check-auth-routable-consumers.sh— a call-site deny-list that fails CI iffetchSignedDownloadis invoked outsidego/pkg/basecamp/download.go, preventing the bug shape fixed in Fix 401 on file downloads #278 from returning. Wired intomake checkvia a newauth-routable-checktarget.typescript/tests/services/uploads.test.tsfixture URL that still usedexample.com, so the shape-only unmarshaling test no longer teaches the wrong URL shape.openapi.jsondiff is exactly two new"x-basecamp-auth-routable-url": {}entries on theUploadandCampfireLineAttachmentdownload_urlproperty schemas. No generator in any SDK consumes field-levelx-basecamp-*extensions, so there is no downstream code churn.Test plan
make smithy-validatepassesmake smithy-checkpasses —git diff openapi.jsonshows exactly twox-basecamp-auth-routable-url: {}additions and nothing elsemake behavior-model-checkandmake url-routes-checkpass (no-op, as expected)ts-generate,rb-generate,py-generate,kt-generate-services,swift-generate,go generate) produce no meaningful code changes (only the extension passthrough intotypescript/src/generated/openapi-stripped.json)make go-check-drift,make py-check-drift,make kt-check-driftpassmake go-test,make ts-test,make conformancepass./scripts/check-auth-routable-consumers.shexits 0;git ls-files --stageshows mode100755make auth-routable-checkexits 0; failure path validated by injecting a violatingfetchSignedDownloadcall invaults.goand confirming the script exits non-zero with a clear message before revertingmake check(omnibus) passes end-to-end