Add basecamp files versions - #622
Merged
Merged
Conversation
github-actions
Bot
added
commands
This was referenced Aug 4, 2026
Implements basecamp files versions <upload-id> over
UploadsService.ListVersions, with the full completeness-bar treatment:
catalog entry, API-COVERAGE row, skill mention, .surface, unit tests, and
smoke coverage (including the vaults/docs alias leaves).
Do not merge. The SDK method mistypes its response, so the command emits
misleading data — see COMMUNIQUE-sdk-upload-versions-response.md. A live
read-only probe of GET /uploads/{id}/versions.json returns version *events*
(action, details, recording_id, created_at, creator), but the OpenAPI spec
models ListUploadVersionsResponseContent as []Upload, so the SDK decodes
events into Upload structs: title, filename, status and download_url come
back blank, action/details/recording_id are dropped, and the id is the event
id, which does not resolve as an upload.
The command itself is correct and ready; it lands once the SDK returns the
right type.
… ones it did not go.mod moves v0.12.0 -> v0.14.0 via make bump-sdk (provenance in lockstep). Compiler-caught, fixed to the migration guide's letter: schedule update pointer fields (nil = leave alone), gauges list options + result wrappers, UpcomingScheduleResponse field rename, UpdateStepRequest.DueOn and UpdateUploadRequest.Description pointers, TodolistGroups().Update routed to the merge-safe todolists endpoint, dead FetchCommentThread deleted with the Recordings().Get it called. Silent breaks, from an exhaustive audit of MIGRATING's no-signal and runtime-failure tables against every CLI call site: - TimelineEvent.CreatedAt nil-deref panics in both TUI hub feeds (the CLI command path had the guard; the hubs did not) - SearchResult.CreatedAt nil-deref panic in workspace search - exit codes: 422s (validation -> 9) and 507s (limit_exceeded -> 10) no longer collapse into the shared table's api_error default of 7; the circuit breaker states its decision for both instead of inheriting it - schedule create/update revalidate timestamps locally (RFC 3339 or bare date) now that the SDK forwards them verbatim - webhooks update: an all-blank --types no longer serializes as a non-nil empty list that clears the subscription roster - document update routes explicit clears through Replace, whose omitted fields the server nulls; partial updates ride the SDK's merge-safe composite instead of a hand-rolled fetch-and-merge Test stubs updated for composite fetches, the inbox_forwards route, and the versions endpoint's real event shape.
jeremy
force-pushed
the
feat/files-versions
branch
from
August 12, 2026 00:41
6d91314 to
bed3c10
Compare
jeremy
marked this pull request as ready for review
August 12, 2026 00:41
github-actions
Bot
added
tui
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (4)
internal/output/codes.go:51
- These arms do not affect the process exit code.
output.Erroris an alias ofclioutput.Error(internal/output/errors.go:14), soapiErr.ExitCode()ininternal/cli/root.go:391,440invokes the shared module’s method and its ownExitCodeFor; this wrapper has no production callers. Consequently, 422 and 507 errors still exit with 7. Route root exits through this wrapper (with anErrorregression test), or update the shared error implementation.
func ExitCodeFor(code string) int {
switch code {
case CodeValidation:
return ExitValidation
case CodeLimitExceeded:
return ExitLimit
}
return clioutput.ExitCodeFor(code)
internal/commands/files.go:1926
- When both
--title ""and--content ""are supplied, both clear flags are true and this request remains all-nil. SDKDocuments().Replacerejects an all-nil request before sending it, so the CLI cannot clear both fields together. Send at least one explicit empty field in this case; the other omitted field is cleared by the replace endpoint.
req := &basecamp.ReplaceDocumentRequest{}
internal/commands/files.go:1573
- Explicit
--page 0, negative pages, and negative limits bypass these checks and become zero-valued SDK options, which fetch all versions instead of rejecting invalid pagination. Validatelimit < 0and an explicitly setpage < 1before the combination checks, as the other listing commands do.
// Validate flag combinations
if all && limit > 0 {
return output.ErrUsage("--all and --limit are mutually exclusive")
}
if page > 0 && (all || limit > 0) {
return output.ErrUsage("--page cannot be combined with --all or --limit")
}
if page > 1 {
return output.ErrUsage("only --page 1 is supported; use --all to fetch everything")
}
API-COVERAGE.md:215
- This row cannot be marked fully implemented at SDK v0.14.0: that release also adds
CreateUploadVersion, while this PR only exposesListVersionsand explicitly defers the write command to cli#404. It also conflicts with the SDK-sync completeness bar inAGENTS.md:112-130, which requires a command for every new SDK method. Keep uploads partial and update the summary/counts, or include the write command.
| uploads | 8 | `files`, `uploads` | ✅ | BC4 | - | list, show, create, update, download, versions (`files versions <id>`); trash/archive/restore go through `recordings`. Create supports `--visible-to-clients` (root vault only) |
![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: bed3c109e7
ℹ️ 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".
The go.sum change invalidated it; the value is the actual hash CI's fixed-output derivation computed and printed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
API-COVERAGE.md:215
- This row cannot claim complete upload coverage after the v0.14 bump: that release adds
UploadsService.CreateVersion, but this PR explicitly defers the corresponding write command to #404. It also leaves this file's summary at lines 14–23 sayingListVersionsis still blocked. Keep the section incomplete until the write operation lands, and update the summary/counts now that the read endpoint is unblocked.
| uploads | 8 | `files`, `uploads` | ✅ | BC4 | - | list, show, create, update, download, versions (`files versions <id>`); trash/archive/restore go through `recordings`. Create supports `--visible-to-clients` (root vault only) |
internal/resilience/hooks.go:181
- Add cases for
basecamp.CodeValidationandbasecamp.CodeLimitExceededto the existingTestIsCircuitBreakerErrortable. Without them, the newly important guarantee that 422/507 responses do not open the circuit is not covered.
case basecamp.CodeValidation, basecamp.CodeLimitExceeded:
// A 422 is the caller's input; a 507 is a plan limit no retry can
// satisfy. Neither says the server is unhealthy. (A 507 tripped the
// circuit before SDK v0.14.0 only because it arrived as CodeAPI.)
return false
internal/commands/schedule_test.go:89
- The phrase “refuses one missing required fields” is grammatically incorrect; use “refuses requests missing required fields.”
// UpdateEntry is a merge-safe composite since SDK v0.13.0: it GETs
// the entry first and refuses one missing required fields, so the
// stub must serve a complete entry rather than `{}`.
internal/commands/schedule.go:871
- Cover all three new validation paths in
schedule_test.go: RFC 3339 acceptance, bare-date acceptance, and malformed-value rejection (for create and update entry points as appropriate). Existing tests only pass RFC 3339 timestamps, so the new date support and local usage error can regress unnoticed.
if _, err := time.Parse(time.RFC3339, value); err == nil {
return nil
}
if _, err := time.Parse("2006-01-02", value); err == nil {
return nil
internal/output/codes.go:49
- Add the new validation and limit cases to
TestExitCodeForininternal/output/output_test.go. That existing table currently stops atCodeAmbiguous, so the local compatibility mapping that must produce exit codes 9 and 10 is untested.
case CodeValidation:
return ExitValidation
case CodeLimitExceeded:
return ExitLimit
…adcrumbs, refuse a double clear - root.go's two exit branches called the shared module's Error.ExitCode(), which flattens the new validation/limit_exceeded codes to 7 — route both through output.ExitCodeFor, whose local arms know them (and whose fallback is the shared table, so nothing else moves) - versions breadcrumbs reuse the caller's own reference and carry an explicit --project, so the follow-up show/download commands keep their scope instead of prompting (or failing headless) - clearing both --title and --content is now a local usage error: a Replace omitting both fields is rejected by the SDK and BC3 anyway, so fail before spending a request
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.
Suppressed comments (4)
internal/commands/files.go:1573
- Explicit
--page 0or a negative page currently bypasses this check and is treated as the default unbounded listing; a negative--limitis likewise ignored and fetches every version. That contradicts the documented “only page 1” contract and can unexpectedly issue an unbounded request. Reject these values and add them to the pagination test table.
if page > 1 {
return output.ErrUsage("only --page 1 is supported; use --all to fetch everything")
}
API-COVERAGE.md:215
- Marking uploads as implemented leaves this file’s summary contradictory: lines 9–10 still report 49 implemented sections and one blocker, while lines 14–23 still say versions is held on the old SDK shape. Update those totals to 50 implemented/184 endpoints and 0 blocked, and remove or rewrite the stale blocker paragraph.
| uploads | 8 | `files`, `uploads` | ✅ | BC4 | - | list, show, create, update, download, versions (`files versions <id>`); trash/archive/restore go through `recordings`. Create supports `--visible-to-clients` (root vault only) |
internal/resilience/hooks.go:181
- The existing
TestIsCircuitBreakerErrortable exercises the other classifications but not these newly added branches. Add validation and limit-exceeded errors with expectedfalseresults so a future fallback to the generic 5xx path cannot make 507 trip the circuit again.
case basecamp.CodeValidation, basecamp.CodeLimitExceeded:
// A 422 is the caller's input; a 507 is a plan limit no retry can
// satisfy. Neither says the server is unhealthy. (A 507 tripped the
// circuit before SDK v0.14.0 only because it arrived as CodeAPI.)
return false
internal/commands/schedule.go:873
- This new validation contract has no tests even though
schedule_test.gocovers the surrounding create/update behavior. Add table cases for an RFC 3339 timestamp, a bare date, and malformed input, and verify both create and update reject malformed values before sending a request.
func validateScheduleTimestamp(flag, value string) error {
if _, err := time.Parse(time.RFC3339, value); err == nil {
return nil
}
if _, err := time.Parse("2006-01-02", value); err == nil {
return nil
}
return output.ErrUsage(fmt.Sprintf("Invalid --%s: %q is neither an RFC 3339 timestamp (2026-06-01T09:00:00Z) nor a date (2026-06-01)", flag, value))
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c48966425d
ℹ️ 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".
| newFilesShowCmd(&project), | ||
| newFilesVersionsCmd(&project), | ||
| newFilesUpdateCmd(&project), | ||
| newFilesDownloadCmd(&project), |
There was a problem hiding this comment.
Adding this leaf to NewFilesCmd also registers docs versions and vaults versions, because NewDocsCmd and NewVaultsCmd clone the entire files command tree; .surface confirms both commands are public. However, CommandCategories adds versions only to the files action list, so basecamp commands incorrectly omits the action for docs and vaults. Either catalog those inherited actions or avoid registering the leaf under those aliases.
Useful? React with 👍 / 👎.
Comment on lines
+1565
to
+1569
| if all && limit > 0 { | ||
| return output.ErrUsage("--all and --limit are mutually exclusive") | ||
| } | ||
| if page > 0 && (all || limit > 0) { | ||
| return output.ErrUsage("--page cannot be combined with --all or --limit") |
There was a problem hiding this comment.
When callers pass --limit=-1 or --page=-1, these > 0 checks accept the value, and the later option builder omits it, silently turning the request into the default fetch-all operation. This makes invalid pagination input potentially return far more history than requested and also lets negative values evade the mutual-exclusion checks; reject negative limits and require an explicitly supplied page to be exactly 1 before calling the SDK.
Useful? React with 👍 / 👎.
| output.WithBreadcrumbs( | ||
| output.Breadcrumb{ | ||
| Action: "show", | ||
| Cmd: fmt.Sprintf("basecamp files show %s%s", ref, scope), |
There was a problem hiding this comment.
When the command is invoked with a valid project name containing spaces, such as --in "My Project", this breadcrumb renders it as the unquoted suffix --project My Project, so copying the advertised follow-up command fails with an extra argument. Preserving the raw URL has the same problem for query strings containing shell metacharacters such as &; quote both the reference and scope value, or resolve the project to its numeric ID before constructing these pasteable commands.
Useful? React with 👍 / 👎.
jeremy
added a commit
that referenced
this pull request
Aug 12, 2026
…#634) * Add basecamp files replace: publish a new version of an uploaded file Closes the write half of the upload-versions story (#404): files versions (#622) reads the history, files replace writes to it, over the SDK v0.14.0 CreateVersion method — stage the file as an attachment, then POST it to the upload's versions endpoint. The upload keeps its id, URL and comments; the previous file becomes a past version, which is the point: a release script can ship each build to the same published link instead of a new duplicate. Defaults follow the endpoint's presence semantics: nobody is notified, and the description carries forward unless --description is given (an explicit empty string clears it). --base-name renames without touching the extension. The issue predates the dedicated endpoint — it proposed PUT-with-sgid on the upload itself, which bc3 has since superseded with POST /versions.json. Live-verified against production: uploads create -> files replace -> files versions shows both filenames with exactly one current version; the verification upload was trashed after. * Review fixes: safe smoke assertion, scoped breadcrumbs, --base-name coverage - the smoke test's current-version count went through bash -c with API JSON interpolated into single quotes — an apostrophe in a creator name breaks it, and worse could execute; assert_json_value reads $output directly - replace's breadcrumbs now reuse the caller's reference and carry --project, same contract as versions' (download resolves a project first) - --base-name is pinned both ways: present it travels as base_name, omitted it stays off the wire * Fail every locally-detectable problem before staging, and quote breadcrumb scopes - a pasted URL naming a different account is refused up front: extractID keeps only the number, which would silently retarget the session account's same-numbered upload on a mutating request - upload IDs must be positive before anything is transferred - the description's local image references resolve before the replacement blob is staged, so a deterministic local failure no longer costs the whole transfer - breadcrumb --project values with whitespace are quoted (both replace and versions), so emitted commands round-trip through a shell All four pinned by TestFilesReplaceRejectsBeforeStaging (zero requests on the wire) and the existing breadcrumb tests. * Close the quoting and URL-identity classes, not their next variations Third review round on the same two classes — the instrument was wrong, not under-tuned: - Breadcrumbs now pass every embedded value (the caller's reference AND the project scope) through shellQuote: inert strings bare, everything else single-quoted, the one POSIX form in which nothing substitutes. An encoding at the boundary instead of another metacharacter case. - A URL argument must identify an UPLOAD in the session's account: account and path type both validated before anything stages, so a same-numbered todo URL can no longer retarget the mutation. TestShellQuote pins the encoding (including query-string URLs, embedded single quotes and command substitution); the wrong-type URL joins TestFilesReplaceRejectsBeforeStaging's zero-requests table. * Complete the URL identity predicate: a listing is not an upload Copilot found the hole the class-closure note promised to treat as a bug in the closure reasoning: collection URLs (/vaults/456/uploads, /buckets/456/uploads) parse as type uploads while their extracted ID is the parent's, so the type-and-account guard alone could still retarget a same-numbered upload. The predicate now requires a non-collection URL with a recording ID — type, account, and identity, all three. Pinned by the vault-scoped collection case in TestFilesReplaceRejectsBeforeStaging. Also gofmt on the TestShellQuote table, which is what CI's lint tripped on. * Breadcrumb scope falls back to the root-level --project The root command binds --project separately (app.Flags.Project); the breadcrumb scope only consulted the files-group flag, so a root-level scope vanished from the emitted commands. scopeProject mirrors the fetch paths' precedence — group flag, then root flag, deliberately not the config default, which a copier's own config supplies. Applied to both replace and versions; pinned by TestScopeProject. * Adopt the host trust gate, and preflight description images Two adoptions of controls the repo already had, at call sites that lacked them: - A URL-shaped argument must pass hostutil.IsTrustedBasecampHost before the identity checks: the URL router is host-agnostic, so a look-alike on an attacker-controlled host carrying the configured account's own IDs passed account/type/identity and would have retargeted a real upload — the confused-deputy case the comment and chat commands already gate. - resolveLocalImages validates every image reference before uploading any: processing in reverse meant a description with a missing image first and a valid image later uploaded the valid one, then failed, stranding the attachment. Helper-level fix, so every caller gets it. Both pinned in TestFilesReplaceRejectsBeforeStaging (zero requests); the mixed-image case is red-proven — against the pre-fix helper it fails with a staged upload on the wire.
jeremy
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements
basecamp files versions <upload-id>over the SDK's typedUploadsService.ListVersions, and absorbs the SDK v0.12.0 → v0.14.0 bump thatunblocked it.
Why this was held, and what unheld it
The SDK's
ListVersionsused to type the response as[]Uploadwhile the APIreturns version events — the command printed blank fields. basecamp-sdk#683
(released in v0.14.0, live-canaried against production first) retyped it as
[]UploadVersion: the event'saction/created_at/creator, plus the file itrecorded nested under
uploadwith a per-versiondownload_url.The command
files versionsflattens events into display rows:action,created,creator,filename,byte_size,current,download_url. The rowidisthe version event's id — no files command accepts it, so the breadcrumbs
address the upload by the id the user gave. Exactly one row is
current; anevent whose recordable was deleted gets no file columns rather than zero
values. The unit fixture is the production response shape, captured live.
Live-verified against production (account 2914079, sandbox vault): a real
upload with a replaced file prints both real filenames, correct byte sizes,
exactly one
current: true, and per-version download URLs. Upload trashedafter.
The SDK bump it rides on (v0.12.0 → v0.14.0, two breaking releases)
Compiler-caught breaks fixed per MIGRATING: schedule-update pointer fields,
gauges list options/result wrappers,
RecurringScheduleEntryOccurrencesrename,
UpdateStepRequest.DueOn/UpdateUploadRequest.Descriptionpointers,
TodolistGroups().Update→ merge-safe todolists endpoint, deadFetchCommentThreaddeleted with the removedRecordings().Getit called.Silent breaks, from an exhaustive audit of MIGRATING's no-signal/runtime
tables against every CLI call site:
TimelineEvent.CreatedAtin the TUI hubfeeds (plus one on
SearchResult.CreatedAtin workspace search) — guardedvalidation→ 9 and 507limit_exceeded→ 10, insteadof both collapsing into the shared table's
api_errordefault of 7; theshared
github.com/basecamp/cli/outputmodule still needs the same arms(local override in
internal/output/codes.gountil then)trip it as a 5xx
api_error; a plan limit is not server unhealth)date) now that the SDK forwards them verbatim
--typesno longer serializes as[]andclears the subscription roster
Replace(omittedfields null server-side); partial updates ride the SDK's merge-safe
composite instead of the old hand-rolled fetch-and-merge
Also release-note-worthy, no code change:
--page Nnow actually returnspage N (fourteen services silently ignored it before), and JSON envelopes
drop
0001-01-01placeholder timestamps where the API omits them.bin/cigreen at the head, SDK provenance in lockstep viamake bump-sdk.API-COVERAGE.md uploads row back to ✅. Follow-up: cli#404 (
files new-versionwrite command) ridesCreateVersionin a separate PR.