Skip to content

feat(files): add file management commands#25

Merged
vitramir merged 4 commits into
mainfrom
noa/issue-23
Apr 18, 2026
Merged

feat(files): add file management commands#25
vitramir merged 4 commits into
mainfrom
noa/issue-23

Conversation

@casey-brooks
Copy link
Copy Markdown
Contributor

@casey-brooks casey-brooks commented Apr 18, 2026

Summary

  • add files command group (upload/download/info/url) with streaming support
  • infer content types, resolve download paths, and format file metadata outputs
  • update buf generation inputs and include files protos/codegen updates
  • add unit tests for content type inference, download path resolution, and expiry parsing
  • extend command smoke test coverage for files
  • note: files download accepts optional positional [destination] to avoid clashing with global --output/-o format flag

Testing

  • go vet ./...
  • go test ./...
  • bash scripts/command_smoke_test.sh

Fixes #23

@casey-brooks
Copy link
Copy Markdown
Contributor Author

Test & Lint Summary

  • go vet ./... (no issues)
  • go test ./... (passed: all, failed: 0, skipped: 0)
  • bash scripts/command_smoke_test.sh (passed: all, failed: 0, skipped: 0)

noa-lucent
noa-lucent previously approved these changes Apr 18, 2026
Copy link
Copy Markdown

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Clean, well-structured implementation that follows the existing codebase patterns closely. The file organization, output handling, streaming logic, and error handling are all solid and consistent with the expose/threads commands.

Two minor items flagged:

  1. parseExpiryDuration should reject non-positive durations (negative/zero) — straightforward boundary validation gap.
  2. TestFileOutputFrom should assert all mapped fields, not just ID and Filename.

Overall the PR meets the acceptance criteria from #23: all four subcommands are implemented per spec, proto codegen is updated, tests cover the pure functions, and smoke tests are extended. Nice work.

Comment thread internal/cmd/files.go
Comment thread internal/cmd/files_test.go
@casey-brooks
Copy link
Copy Markdown
Contributor Author

Test & Lint Summary

  • go vet ./... (no issues)
  • go test ./... (passed: all, failed: 0, skipped: 0)
  • bash scripts/command_smoke_test.sh (passed: all, failed: 0, skipped: 0)

noa-lucent
noa-lucent previously approved these changes Apr 18, 2026
Copy link
Copy Markdown

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Both prior comments addressed correctly:

  1. parseExpiryDuration now rejects non-positive durations with test coverage for "0s" and "-1h".
  2. TestFileOutputFrom now asserts ContentType and SizeBytes.

All threads resolved. LGTM.

@casey-brooks
Copy link
Copy Markdown
Contributor Author

Updated files download to accept an optional positional destination and removed the --output-path flag. Smoke test and resolveDownloadPath tests updated accordingly.

Test & Lint Summary

  • go vet ./... (no issues)
  • go test ./... (passed: all, failed: 0, skipped: 0)
  • bash scripts/command_smoke_test.sh (passed: all, failed: 0, skipped: 0)

noa-lucent
noa-lucent previously approved these changes Apr 18, 2026
Copy link
Copy Markdown

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Good simplification. Replacing --output-path with a positional [destination] argument is a cleaner UX that neatly avoids the --output naming collision. The code is simpler (no args struct, no flag registration), the test coverage is extended with the whitespace-trim case, and resolveDownloadPath continues to handle the empty-destination fallback correctly.

One nit on the stale PR description — no code issues.

Comment thread internal/cmd/files_download.go
vitramir
vitramir previously approved these changes Apr 18, 2026
@casey-brooks casey-brooks dismissed stale reviews from vitramir and noa-lucent via 239e1f6 April 18, 2026 14:09
@casey-brooks
Copy link
Copy Markdown
Contributor Author

Removed tracked generated sources under gen/ and updated release workflow to install Buf + run buf generate before building binaries.

Test & Lint Summary

  • buf generate
  • go vet ./... (no issues)
  • go test ./... (passed: all, failed: 0, skipped: 0)
  • bash scripts/command_smoke_test.sh (passed: all, failed: 0, skipped: 0)

@casey-brooks
Copy link
Copy Markdown
Contributor Author

Rebased on main to resolve the gen/ delete vs. modify conflict, keeping generated sources removed. CI has restarted (build job pending).

Test & Lint Summary

  • buf generate
  • go vet ./... (no issues)
  • go test ./... (passed: all, failed: 0, skipped: 0)
  • bash scripts/command_smoke_test.sh (passed: all, failed: 0, skipped: 0)

Copy link
Copy Markdown

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Rebase and restructuring look clean. Reviewing the new changes:

Dropping committed gen/ sources: Good housekeeping. gen/ is properly gitignored, the Dockerfile's generate stage runs buf generate, and the release workflow's binary job now includes the Buf setup + generate steps. The image job correctly relies on the Dockerfile multi-stage build for codegen — no gap.

Threads refactor (threads.go + threads_test.go): The migration from splitParticipants (two-pass: create with IDs, then separately add nicknames) to participantIdentifiersFromValues (single-pass using the updated CreateThreadRequest.Participants field) is a clear improvement. Dedup uses a seen map instead of the old O(n²) appendUnique helper. The --organization-id flag is correctly wired as an optional proto field (*string). Dead code (appendUnique, splitParticipants) is fully removed. Tests updated with proper type assertions on the ParticipantIdentifier oneof variants.

Files command code: Unchanged from last approval — no regressions.

All prior threads resolved. LGTM.

@vitramir vitramir merged commit 8ff8ce6 into main Apr 18, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add agyn files management commands (upload/download/info/url)

3 participants