Uh oh!
There was an error while loading. Please reload this page.
Add basecamp files replace: publish a new version of an uploaded file - #634
Conversation
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.
There was a problem hiding this comment.
Pull request overview
Adds files replace to publish new upload versions while preserving the upload’s identity and history.
Changes:
- Implements attachment staging and version creation with description and filename options.
- Adds unit and smoke coverage.
- Updates CLI metadata, documentation, API coverage, and surface snapshots.
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.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/commands/files.go | Implements and registers files replace. |
internal/commands/files_test.go | Tests replacement flow, URLs, and descriptions. |
internal/commands/commands.go | Adds the catalog action. |
e2e/smoke/smoke_files_write.bats | Adds the live replacement smoke test. |
e2e/smoke/smoke_lifecycle.bats | Marks alias paths out of scope. |
skills/basecamp/SKILL.md | Documents usage and semantics. |
API-COVERAGE.md | Records endpoint coverage. |
.surface | Regenerates the CLI surface snapshot. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:fe92090ed7
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…overage - 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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/commands/files.go:1755
--projectaccepts project names, so interpolating it directly makes both breadcrumbs invalid for values such as"My Project"(--project My Projectbecomes two arguments).refis also the original URL and may contain shell metacharacters such as&. Shell-quote both dynamic arguments, or construct the hints from resolved numeric IDs, before embedding them in copyable commands.
ref := args[0]
scope := ""
if *project != "" {
scope = fmt.Sprintf(" --project %s", *project)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:3bf8723b01
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…crumb 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
internal/commands/files.go:1801
- Go
%qis not shell escaping: unquoted project names can contain;,$(), or backticks, and the double quotes used for whitespace still allow command substitution. Because these breadcrumbs are presented as copy-pasteable shell commands, always POSIX single-quote the project value (matchingconfig.ShellQuote) rather than only checking for whitespace.
if strings.ContainsAny(project, " \t") {
return fmt.Sprintf(" --project %q", project)
}
return " --project " + project
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:99813373d4
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
jeremy
commented
Aug 12, 2026
Review-loop note, for the record: this was the third round on the same two
If a fourth variation of either class appears, it's evidence the closure |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
internal/commands/files.go:1805
- The documented POSIX escape sequence is malformed: it ends with a curly double quote and does not match the
'\''sequence emitted below. Correct the example so future changes do not copy an invalid quoting form.
// spelled '\”. This is an encoding applied to every embedded value, not a
Uh oh!
There was an error while loading. Please reload this page.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
internal/commands/files.go:1812
- The documented POSIX representation for an embedded single quote is malformed here; it should match the
'\''sequence implemented below so this security-sensitive explanation remains accurate.
// spelled '\”. This is an encoding applied to every embedded value, not a
internal/commands/files.go:1739
resolveLocalImagesvalidates and uploads each image in one reverse-order pass. With a description such as a missing image followed by a valid local image, it uploads the valid image first and only then returns the local validation error, leaving staged data even though replacement never runs. This contradicts the preflight/no-request contract documented by this change. Validate every local image before uploading any of them (and add a mixed valid/missing regression case), then perform resolution.
if descHTML, err = resolveLocalImages(cmd, app, descHTML); err != nil {
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:cc7a0a094c
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
internal/commands/files.go:1812
- The documented POSIX single-quote escape is malformed; write
\'\''so the comment accurately matches the replacement performed below.
// spelled '\”. This is an encoding applied to every embedded value, not a
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:16211fa680
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/commands/files.go:1822
- The shell-escaping example ends with a curly double quote and does not show the sequence that
shellQuoteactually emits. Correct it to'\''so the helper's documentation accurately explains embedded single-quote handling.
// spelled '\”. This is an encoding applied to every embedded value, not a
Uh oh!
There was an error while loading. Please reload this page.
…or-roundtrip * origin/main: (96 commits) ci: bump the github-actions group with 6 updates (#639) Reject three more doomed invocations before draining stdin (#645) Stdin `-` support everywhere sensible; usage error for stray `-` elsewhere (#641) Add hey-cli Windows signing secrets to the release env manifest (#642) deps: bump the go-dependencies group with 5 updates (#638) Update nix flake and plugin version for v0.9.1 ci: bump the github-actions group with 4 updates (#633) Add basecamp files replace: publish a new version of an uploaded file (#634) Add basecamp files versions — HELD, blocked on the SDK (#622) Update nix flake and plugin version for v0.9.0 Make the Codex probe's timeout actually bound doctor (#629) Make the lockstep check catch stale agreement and .yaml workflows (#628) Keep refreshing opencode's other spelling (#627) Lint the release the same way we lint everything else (#625) Install the skill where opencode actually looks (#624) Take the communiques out of the source tree (#623) Correct the API coverage claim: 183/184, not 100% (#621) Stop echoing back step fields the caller never changed (#620) Drive the circuit breaker's clock from tests, not sleep() (#619) Tell agents the truth about card column moves (#618) ...
Closes#404 — the write half of the upload-versions story. #622's
files versionsreads the history;files replace <upload-id|url> <file>writes toit, over SDK v0.14.0's
UploadsService.CreateVersion: stage the file as anattachment (the same two-step flow as
uploads create), then POST it to theupload's versions endpoint.
The upload keeps its id, URL and comments; the previous file becomes a past
version. That's the issue's use case verbatim: a release script ships each
build to the same published link instead of minting duplicates. (#404
proposed PUT-with-sgid on the upload itself — bc3 has since shipped the
dedicated versions endpoint, which is what this rides.)
Semantics follow the endpoint's presence rules:
notifyon the wire)--descriptionis given; anexplicit
--description ""clears it (tri-state pinned byTestFilesReplaceDescriptionTriState, including that omission stays offthe wire)
--base-namerenames without touching the extensionfiles new-versionCompleteness bar: catalog action, API-COVERAGE row, SKILL.md examples +
semantics note,
.surfaceregenerated (406 lines — command + alias acrossthe tree), unit tests (two-step flow order, URL acceptance, tri-state), a
real write smoke (
files replaceon the suite's own created upload, thenfiles versionsasserting exactly one current) plus out-of-scope markers forthe alias leaves.
Live-verified against production:
uploads create→files replace→files versionsshows both filenames with exactly onecurrent: true, sameupload id throughout. Verification upload trashed after.
bin/cigreen at the head (real exit 0).Summary by cubic
Add
basecamp files replaceto publish a new version of an uploaded file while keeping the same upload ID/URL and comments. This delivers the write half of #404 and pairs withfiles versionsto support stable links for new builds.New Features
basecamp files replace <upload-id|url> <file>(alias:new-version) via SDKUploadsService.CreateVersion(stage attachment, then POST to versions).--descriptionis set (empty clears);--base-namerenames without changing the extension.Bug Fixes
--descriptionbefore staging any data.--projectwith fallback to the root-level--projectwhen the group flag is unset; shell-quote all embedded values (bothfiles replaceandfiles versions) so emitted commands are safe and round-trip through a shell.$outputdirectly; tests pin--base-namepresence/omission behavior.Written for commit a0a5ec2. Summary will update on new commits.