Merged
Conversation
Prevents test utilities from being compiled and published with the services/slack package while keeping them importable by any test.
… issues - Terminate orphaned containers on error in Postgres and Mattermost setup - Validate Docker Hub HTTP status before decoding response - Add json:"-" tag to SlackChannel.Type to clarify it is computed - Only set DeleteAt on user export when user is actually deleted
fmartingr
force-pushed
the
e2e-testing-imports
branch
from
8c4cde8 to
b23da00
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end Slack import testing: programmatic Slack export builder, testcontainer orchestration (Docker network, Postgres, Mattermost), a TestHelper with mmctl-based import/validation, consolidated Slack models with DeleteAt propagation, many unit/integration tests, and Makefile test-flag support. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as E2E Test
participant Builder as SlackExportBuilder
participant Helper as TestHelper
participant Net as Docker Network
participant PG as Postgres Container
participant MM as Mattermost Container
participant MMCTL as mmctl (inside container)
Test->>Builder: Build Slack export ZIP
Builder-->>Test: export.zip
Test->>Helper: SetupHelper(t)
Helper->>Net: CreateTestNetwork()
Helper->>PG: CreatePostgresContainer(network)
PG-->>Helper: postgres ready
Helper->>MM: CreateMattermostContainer(network)
MM-->>Helper: mattermost ready (SiteURL, Admin)
Test->>Helper: ImportBulkData(ctx, export.zip)
Helper->>MMCTL: copy file into container & run import
MMCTL->>MM: POST import job
MM-->>MMCTL: job id
Helper->>MM: poll import job status
MM-->>Helper: job complete
Helper-->>Test: import finished
Test->>MM: GET users/channels/posts (via Helper API)
MM-->>Test: imported entities returned
Test->>Helper: TearDown()
Helper->>MM: Stop container
Helper->>PG: Stop container
Helper->>Net: Remove network
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
Makefile (1)
11-11: Use$(GO_TEST_FLAGS)for consistency with the rest of the Makefile.Line 75 uses
${GO_TEST_FLAGS}(brace syntax) while every other variable reference in the file uses$(...). Both are valid in GNU Make, but mixing styles is a minor inconsistency.♻️ Proposed fix
- $(GO) test -race -v $(GO_PACKAGES) -count=1 ${GO_TEST_FLAGS} + $(GO) test -race -v $(GO_PACKAGES) -count=1 $(GO_TEST_FLAGS)Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 11, Replace the brace-style variable reference ${GO_TEST_FLAGS} with the consistent $(GO_TEST_FLAGS) syntax in the Makefile so all variable usages follow the same style; search for occurrences of ${GO_TEST_FLAGS} and update them to $(GO_TEST_FLAGS) (the variable name to update is GO_TEST_FLAGS).commands/transform_slack_e2e_test.go (2)
21-23:uniqueTeamNamehas a narrow but real collision window.
time.Now().UnixNano()%10000wraps around every ~10 µs. Different call-site prefixes ("e2e","posts","mentions","deleted","consist") make intra-run collisions effectively impossible, but rapid back-to-back test runs in CI (e.g., parallelgo test ./...jobs sharing the same Mattermost instance) could produce the same team name and cause flaky failures. Using a monotonically incrementing atomic counter orcrypto/rand/UUID avoids the modular collision entirely.♻️ Proposed alternative using `crypto/rand`
import ( + "crypto/rand" + "encoding/hex" "fmt" - "time" ... ) func uniqueTeamName(prefix string) string { - return fmt.Sprintf("t%s%d", prefix, time.Now().UnixNano()%10000) + b := make([]byte, 4) + _, _ = rand.Read(b) + return fmt.Sprintf("t%s%s", prefix, hex.EncodeToString(b)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/transform_slack_e2e_test.go` around lines 21 - 23, The uniqueTeamName function currently uses time.Now().UnixNano()%10000 which has a small collision window; replace that suffix with a non-colliding value (e.g., a UUID or crypto/rand-derived hex string, or a package-level atomic counter) to eliminate wraparound collisions. Update uniqueTeamName to generate and append the new unique suffix (using functions from crypto/rand or uuid.New) instead of the modulo timestamp so concurrent/parallel test runs cannot produce the same name.
64-66: Reusing a globalcommands.RootCmdacross subtests is not ideal architecture, but flag state is correctly reset.While
c := commands.RootCmdcreates a pointer to the global singleton and each subtest mutates it viaSetArgs()andExecute(), Cobra correctly re-parses all flag values on eachExecute()call. The command functiontransformSlackCmdFonly reads flags viaGetString()andGetBool()without inspectingChangedstate, so flag value bleeding does not occur. This pattern works correctly but could be improved by either creating a fresh command copy for each test or resetting the command state explicitly between subtests for better test isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/transform_slack_e2e_test.go` around lines 64 - 66, Tests currently reuse the global commands.RootCmd across subtests which risks shared state; change the tests to avoid mutating the global singleton by creating a fresh command instance per subtest (e.g., clone or recreate the root command) or explicitly reset its state before each subtest; ensure each subtest calls SetArgs(...) and Execute() on a new/isolated command object rather than commands.RootCmd so transformSlackCmdF continues to read flags via GetString/GetBool without cross-test bleeding.testhelper/testhelper.go (4)
423-438: Consider whetherValidationResultneeds JSON tags.
ValidationResulthas JSON struct tags but appears to only be used in-process (returned fromValidateImportFile, consumed byValidateImportFileOrFail). If it's never serialized to JSON, the tags are unnecessary clutter. If future JSON serialization is planned, this is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/testhelper.go` around lines 423 - 438, ValidationResult currently includes JSON struct tags but is only used in-process by ValidateImportFile and ValidateImportFileOrFail; remove the unnecessary `json:"..."` tags from the ValidationResult struct (type ValidationResult) to reduce clutter if there is no JSON serialization path, and run/adjust any tests that reference the tags or rely on struct field tags; if you discover any serialization code that depends on those tags, instead leave them and add a comment explaining why they must stay.
62-65: Postgres connection string logged — may contain credentials.Line 65 logs the full connection string which includes username and password (from
CreatePostgresContainer). While this is test code, CI logs are often stored and visible to others.Suggested fix
- t.Logf("PostgreSQL started with connection string: %s", postgresConnStr) + t.Logf("PostgreSQL started")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/testhelper.go` around lines 62 - 65, The test currently logs the full Postgres connection string (postgresConnStr) which may contain credentials; change the code in testhelper.go so it stops logging the raw postgresConnStr returned by CreatePostgresContainer and instead either log a non-sensitive piece of information (e.g., host/port extracted from postgresConnStr) or a masked/placeholder string such as "<REDACTED_CONN_STR>"; ensure you still append postgresTearDown to th.tearDowns and keep CreatePostgresContainer usage unchanged, but remove or replace the t.Logf("PostgreSQL started with connection string: %s", postgresConnStr) call to avoid exposing credentials.
326-374: Exponential backoff rendersimportJobMaxAttemptsmisleading.The constant is named
importJobMaxAttempts = 60and used asimportJobMaxAttempts * importJobPollInterval(= 60s deadline), but with exponential backoff (1s → 2s → 4s → 8s → 10s cap), only ~8-10 polls fit in that window — not 60. The name implies 60 attempts. Consider renaming to reflect it's a timeout:Suggested fix
- importJobMaxAttempts = 60 - importJobPollInterval = 1 * time.Second + importJobTimeout = 60 * time.Second + importJobPollInterval = 1 * time.SecondAnd at line 328:
- deadline := time.After(importJobMaxAttempts * importJobPollInterval) + deadline := time.After(importJobTimeout)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/testhelper.go` around lines 326 - 374, The constant importJobMaxAttempts is misleading because exponential backoff makes it a timeout, not a max attempts count; in waitForImportJobCompletion change the constant to a timeout-style name (e.g., importJobTimeout of type time.Duration) and compute deadline := time.After(importJobTimeout) (or alternatively preserve importJobMaxAttempts as an integer and change the logic to cap attempts rather than time), and update any references to importJobMaxAttempts and importJobPollInterval accordingly so the code and names reflect a timeout-based loop with exponential backoff (refer to function waitForImportJobCompletion, constant importJobMaxAttempts, and variable importJobPollInterval).
536-597: Double-close on the error path iftempFile.Close()at line 591 fails.If
zipWriter.Close()at line 587 succeeds buttempFile.Close()at line 591 fails, the named return setsretErr, causing the deferred cleanup at lines 546-550 to callzipWriter.Close()again (already closed), and lines 537-542 to calltempFile.Close()again. While Go'sos.File.Closeon an already-closed file just returnsos.ErrClosed(no panic), the logic is fragile.A cleaner approach: set a flag or nil out the writers after explicit close to guard the defers, or consolidate the cleanup:
Simplified approach
func wrapJSONLInZip(jsonlPath string) (zipPath string, retErr error) { tempFile, err := os.CreateTemp("", "import-*.zip") if err != nil { return "", fmt.Errorf("failed to create temp file: %w", err) } tempPath := tempFile.Name() + closed := false defer func() { if retErr != nil { - tempFile.Close() + if !closed { + tempFile.Close() + } os.Remove(tempPath) } }() zipWriter := zip.NewWriter(tempFile) - defer func() { - if retErr != nil { - zipWriter.Close() - } - }() // ... (rest of the function) if err := zipWriter.Close(); err != nil { return "", fmt.Errorf("failed to close zip writer: %w", err) } if err := tempFile.Close(); err != nil { os.Remove(tempPath) return "", fmt.Errorf("failed to close temp file: %w", err) } + closed = true return tempPath, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/testhelper.go` around lines 536 - 597, The deferred cleanup can double-close tempFile and zipWriter when explicit Close() calls set retErr; update the logic so deferred functions only close resources if they are still non-nil/active: change the defer bodies that currently reference tempFile and zipWriter to check for nil (or maintain boolean flags like closedTemp/closedZip) before calling Close()/Remove, and after successfully calling zipWriter.Close() and tempFile.Close() in the normal path, set the corresponding variables to nil (or set the closed flags) so the defers skip them; ensure error returns still set retErr normally but do not cause duplicate Close calls on the same resources (refer to retErr, tempFile, zipWriter, and the two defer blocks).testhelper/slack_fixtures.go (2)
251-268:filepath.Relreturns OS-specific separators; ZIP entries require forward slashes.On Windows,
filepath.Relreturns backslash-separated paths (e.g.,general\2025-01-01.json), but ZIP entries must use forward slashes per the ZIP specification. This would cause tests to fail on Windows or produce ZIPs that other tools can't read correctly.Suggested fix
relPath, err := filepath.Rel(sourceDir, path) if err != nil { return err } + // ZIP spec requires forward slashes + relPath = filepath.ToSlash(relPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/slack_fixtures.go` around lines 251 - 268, filepath.Rel can return OS-specific separators which breaks ZIP entries; after computing relPath (from filepath.Rel) normalize it to ZIP standard by replacing OS-specific separators with "/" before using it in archive.Create or assigning to header.Name (used with zip.FileInfoHeader). For directories (info.IsDir), ensure you append a trailing "/" to the normalized relPath when calling archive.Create; for files, set header.Name to the normalized relPath. Update any uses of relPath in this block to use the normalized value so ZIP entries are always forward-slash separated.
102-141:allChannels()is called twice during validation — minor optimization opportunity.
validate()callsallChannels()at line 111 and again at line 116, each time allocating a new slice. For test code this is negligible, but it's easy to fix.Suggested fix
func (b *SlackExportBuilder) validate() error { // Build lookup maps for quick validation userIDs := make(map[string]bool) for _, user := range b.users { userIDs[user.Id] = true } channelNames := make(map[string]bool) - for _, channel := range b.allChannels() { + allCh := b.allChannels() + for _, channel := range allCh { channelNames[channel.Name] = true } // Validate channel creators and members reference existing users - for _, channel := range b.allChannels() { + for _, channel := range allCh {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/slack_fixtures.go` around lines 102 - 141, The validate() function calls b.allChannels() twice which allocates two slices; call b.allChannels() once, store the result in a local variable (e.g., channels := b.allChannels()), use that variable to build channelNames and to iterate when validating channel.Creator and channel.Members, and leave the posts loop unchanged; update references from b.allChannels() to the new local variable in validate().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testhelper/containers.go`:
- Around line 190-201: The call in CreatePostgresContainer uses
network.WithNetwork(..., &testcontainers.DockerNetwork{Name: networkName}) which
is incorrect because it supplies a partial DockerNetwork struct; replace that
option with network.WithNetworkName(postgresAliasSlice, networkName) (i.e., use
network.WithNetworkName(aliases []string, networkName string)) when invoking
postgres.Run so the container is attached to the user-defined network by name
and the "postgres" alias is set correctly; update the arguments to postgres.Run
to remove the partial DockerNetwork usage and pass the alias slice and
networkName to network.WithNetworkName.
In `@testhelper/testhelper.go`:
- Around line 44-82: SetupHelper currently appends resources to th.tearDowns but
if a later step fails the caller's defer th.TearDown() may never run; register
an early cleanup with t.Cleanup to guarantee accumulated teardowns run on test
failure (e.g., right after creating th call t.Cleanup(func(){ th.TearDown() })),
and ensure TearDown is safe to call multiple times/when tearDowns is empty by
making it idempotent (clear th.tearDowns after executing them). This uses
SetupHelper, TestHelper, th.tearDowns, t.Cleanup, and TearDown to locate and
change the code.
---
Nitpick comments:
In `@commands/transform_slack_e2e_test.go`:
- Around line 21-23: The uniqueTeamName function currently uses
time.Now().UnixNano()%10000 which has a small collision window; replace that
suffix with a non-colliding value (e.g., a UUID or crypto/rand-derived hex
string, or a package-level atomic counter) to eliminate wraparound collisions.
Update uniqueTeamName to generate and append the new unique suffix (using
functions from crypto/rand or uuid.New) instead of the modulo timestamp so
concurrent/parallel test runs cannot produce the same name.
- Around line 64-66: Tests currently reuse the global commands.RootCmd across
subtests which risks shared state; change the tests to avoid mutating the global
singleton by creating a fresh command instance per subtest (e.g., clone or
recreate the root command) or explicitly reset its state before each subtest;
ensure each subtest calls SetArgs(...) and Execute() on a new/isolated command
object rather than commands.RootCmd so transformSlackCmdF continues to read
flags via GetString/GetBool without cross-test bleeding.
In `@Makefile`:
- Line 11: Replace the brace-style variable reference ${GO_TEST_FLAGS} with the
consistent $(GO_TEST_FLAGS) syntax in the Makefile so all variable usages follow
the same style; search for occurrences of ${GO_TEST_FLAGS} and update them to
$(GO_TEST_FLAGS) (the variable name to update is GO_TEST_FLAGS).
In `@testhelper/slack_fixtures.go`:
- Around line 251-268: filepath.Rel can return OS-specific separators which
breaks ZIP entries; after computing relPath (from filepath.Rel) normalize it to
ZIP standard by replacing OS-specific separators with "/" before using it in
archive.Create or assigning to header.Name (used with zip.FileInfoHeader). For
directories (info.IsDir), ensure you append a trailing "/" to the normalized
relPath when calling archive.Create; for files, set header.Name to the
normalized relPath. Update any uses of relPath in this block to use the
normalized value so ZIP entries are always forward-slash separated.
- Around line 102-141: The validate() function calls b.allChannels() twice which
allocates two slices; call b.allChannels() once, store the result in a local
variable (e.g., channels := b.allChannels()), use that variable to build
channelNames and to iterate when validating channel.Creator and channel.Members,
and leave the posts loop unchanged; update references from b.allChannels() to
the new local variable in validate().
In `@testhelper/testhelper.go`:
- Around line 423-438: ValidationResult currently includes JSON struct tags but
is only used in-process by ValidateImportFile and ValidateImportFileOrFail;
remove the unnecessary `json:"..."` tags from the ValidationResult struct (type
ValidationResult) to reduce clutter if there is no JSON serialization path, and
run/adjust any tests that reference the tags or rely on struct field tags; if
you discover any serialization code that depends on those tags, instead leave
them and add a comment explaining why they must stay.
- Around line 62-65: The test currently logs the full Postgres connection string
(postgresConnStr) which may contain credentials; change the code in
testhelper.go so it stops logging the raw postgresConnStr returned by
CreatePostgresContainer and instead either log a non-sensitive piece of
information (e.g., host/port extracted from postgresConnStr) or a
masked/placeholder string such as "<REDACTED_CONN_STR>"; ensure you still append
postgresTearDown to th.tearDowns and keep CreatePostgresContainer usage
unchanged, but remove or replace the t.Logf("PostgreSQL started with connection
string: %s", postgresConnStr) call to avoid exposing credentials.
- Around line 326-374: The constant importJobMaxAttempts is misleading because
exponential backoff makes it a timeout, not a max attempts count; in
waitForImportJobCompletion change the constant to a timeout-style name (e.g.,
importJobTimeout of type time.Duration) and compute deadline :=
time.After(importJobTimeout) (or alternatively preserve importJobMaxAttempts as
an integer and change the logic to cap attempts rather than time), and update
any references to importJobMaxAttempts and importJobPollInterval accordingly so
the code and names reflect a timeout-based loop with exponential backoff (refer
to function waitForImportJobCompletion, constant importJobMaxAttempts, and
variable importJobPollInterval).
- Around line 536-597: The deferred cleanup can double-close tempFile and
zipWriter when explicit Close() calls set retErr; update the logic so deferred
functions only close resources if they are still non-nil/active: change the
defer bodies that currently reference tempFile and zipWriter to check for nil
(or maintain boolean flags like closedTemp/closedZip) before calling
Close()/Remove, and after successfully calling zipWriter.Close() and
tempFile.Close() in the normal path, set the corresponding variables to nil (or
set the closed flags) so the defers skip them; ensure error returns still set
retErr normally but do not cause duplicate Close calls on the same resources
(refer to retErr, tempFile, zipWriter, and the two defer blocks).
Reset all flag values and Changed state on the shared RootCmd before each Execute() call, preventing flag pollution across subtests. Also fix duplicate step numbering in test comments.
There was a problem hiding this comment.
🧹 Nitpick comments (5)
commands/transform_slack_e2e_test.go (4)
36-38: Low entropy inuniqueTeamNamemay cause collisions in CI.
time.Now().UnixNano()%10000yields only 4 digits of variation. On systems with coarse clock resolution or parallel CI jobs, team name collisions are possible. Consider using a wider modulus or incorporatingt.Name().Suggested improvement
func uniqueTeamName(prefix string) string { - return fmt.Sprintf("t%s%d", prefix, time.Now().UnixNano()%10000) + return fmt.Sprintf("t%s%d", prefix, time.Now().UnixNano()%1000000) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/transform_slack_e2e_test.go` around lines 36 - 38, The uniqueTeamName function uses time.Now().UnixNano()%10000 which provides only 4 digits of entropy and can collide in CI; update uniqueTeamName to increase entropy (e.g., use a larger modulus like 1e9, append a per-test identifier such as t.Name() or the test PID, or generate a short random/UUID suffix via crypto/rand) so names are effectively unique in parallel runs; modify the uniqueTeamName function to combine the prefix, the widened timestamp/random suffix, and optionally t.Name() to avoid collisions.
268-281: Channel mention conversion is not verified.The
ExportWithMentionsfixture includes a<#C002|random>channel mention (from the relevant snippet), but the test only asserts user mentions and@here. Consider also verifying that channel mentions are converted correctly (e.g.,~random).Suggested addition
- var foundUserMention, foundHereMention bool + var foundUserMention, foundHereMention, foundChannelMention bool for _, postID := range posts.Order { post := posts.Posts[postID] // Slack <@U002> should be converted to `@jane.smith` if strings.Contains(post.Message, "@jane.smith") { foundUserMention = true } // Slack <!here> should be converted to `@here` if strings.Contains(post.Message, "@here") { foundHereMention = true } + // Slack <#C002|random> should be converted to ~random + if strings.Contains(post.Message, "~random") { + foundChannelMention = true + } } assert.True(t, foundUserMention, "user mention should be converted to `@jane.smith`") assert.True(t, foundHereMention, "@here mention should be present") + assert.True(t, foundChannelMention, "channel mention should be converted to ~random")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/transform_slack_e2e_test.go` around lines 268 - 281, The test currently checks user and `@here` conversions by scanning posts.Order and posts.Posts and asserting foundUserMention and foundHereMention; add a similar check for the channel mention from the ExportWithMentions fixture by scanning post.Message for the converted channel token (e.g., "~random"), introduce a foundChannelMention boolean (or reuse a descriptive variable) and set it when strings.Contains(post.Message, "~random") is true, then add an assertion (assert.True) to ensure the channel mention was converted.
331-340: Consider merging this intoTestTransformSlackE2Eas a subtest to avoid spinning up a second Docker environment.
TestTransformSlackE2ETeamConsistencycreates an entirely separate Mattermost + Postgres stack just for one team-consistency check. Since the assertions here (channel TeamId, user team membership) are complementary to the existing subtests, folding this in as anothert.RuninsideTestTransformSlackE2Ewould cut the E2E suite runtime roughly in half without sacrificing isolation — each subtest already uses a unique team name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/transform_slack_e2e_test.go` around lines 331 - 340, TestTransformSlackE2ETeamConsistency spins up a separate Docker stack; merge it into TestTransformSlackE2E as a subtest instead: move the body of TestTransformSlackE2ETeamConsistency into a t.Run call inside TestTransformSlackE2E, remove its separate ctx/testhelper SetupHelper/TearDown, reuse the parent ctx and th from TestTransformSlackE2E, and ensure the subtest still uses a unique team name and the same assertions (channel TeamId and user team membership); delete the now-empty TestTransformSlackE2ETeamConsistency function.
79-84: Reusing the globalcommands.RootCmdacross subtests is fragile; consider documenting the no-parallel constraint more visibly.Each subtest mutates the package-level
RootCmdsingleton (viaSetArgsandExecute). This works only because subtests are sequential. If a future contributor addst.Parallel(), it will race on the shared command. TheresetCobraFlagshelper documents this partially, but the contract isn't visible at the call sites.Also,
defer os.Remove("transform-slack.log")is repeated in every subtest with a hardcoded relative path. Consider extracting this to a constant or performing a single cleanup viat.Cleanupon the parent test.Suggested cleanup
Define a constant at file level and register cleanup once in the parent:
+const transformSlackLogFile = "transform-slack.log" + func TestTransformSlackE2E(t *testing.T) { if testing.Short() { t.Skip("Skipping integration test in short mode") } th := testhelper.SetupHelper(t) defer th.TearDown() + t.Cleanup(func() { os.Remove(transformSlackLogFile) }) t.Run("basic import creates users and channels in Mattermost", func(t *testing.T) { ... - defer os.Remove("transform-slack.log") ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/transform_slack_e2e_test.go` around lines 79 - 84, The test reuses the package-level commands.RootCmd and mutates it via SetArgs/Execute (with resetCobraFlags), which is fragile for parallel subtests; fix by ensuring tests do not race: either create a fresh cobra.Command instance per subtest (clone or construct a new RootCmd) and call SetArgs/Execute on that instance instead of commands.RootCmd, or explicitly document and enforce the no-parallel contract by removing any t.Parallel() and registering a single t.Cleanup in the parent test that removes the log; also replace repeated defer os.Remove("transform-slack.log") with a file name constant (e.g., transformSlackLog) at file scope and call t.Cleanup(func(){ os.Remove(transformSlackLog) }) once in the parent so all subtests share the single cleanup.go.mod (1)
15-16: Consider isolating E2E test dependencies in a separate sub-module.Adding
testcontainers-goand its Docker/Moby/OpenTelemetry/containerd transitive closure (lines 21–103) to the maingo.modinflates the main module's dependency graph by ~30 new indirect packages. This means everygo build,go vet, andgovulncheckrun against the root module now has to resolve and scan all container-runtime deps, and any future CVE in the Docker/Moby/OTel stack will appear in the main module's audit.The idiomatic Go approach for heavy test-only infrastructure is a sub-module (e.g.,
e2e/go.mod) that maintains its own dependency graph and is excluded from the main module'sgo build ./....♻️ Suggested structure
mmetl/ ├── go.mod # main module — no testcontainers dep ├── go.sum ├── ... └── e2e/ ├── go.mod # module github.com/mattermost/mmetl/e2e ├── go.sum # testcontainers + all docker deps isolated here └── ...The E2E tests would live under
e2e/and be run with a dedicated CI step (cd e2e && go test ./...), keeping the main module graph clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 15 - 16, The root go.mod currently includes heavy test-only deps (github.com/testcontainers/testcontainers-go and modules/postgres) which pull in many container/runtime transitive packages; create a separate sub-module (e.g., e2e/) with its own go.mod (module path like github.com/yourrepo/e2e) and move all E2E tests and testcontainers imports into that directory, remove testcontainers entries from the root go.mod, and update CI to run tests via cd e2e && go test ./... so the main module (root go.mod) no longer carries the testcontainers dependency graph.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@commands/transform_slack_e2e_test.go`:
- Around line 36-38: The uniqueTeamName function uses
time.Now().UnixNano()%10000 which provides only 4 digits of entropy and can
collide in CI; update uniqueTeamName to increase entropy (e.g., use a larger
modulus like 1e9, append a per-test identifier such as t.Name() or the test PID,
or generate a short random/UUID suffix via crypto/rand) so names are effectively
unique in parallel runs; modify the uniqueTeamName function to combine the
prefix, the widened timestamp/random suffix, and optionally t.Name() to avoid
collisions.
- Around line 268-281: The test currently checks user and `@here` conversions by
scanning posts.Order and posts.Posts and asserting foundUserMention and
foundHereMention; add a similar check for the channel mention from the
ExportWithMentions fixture by scanning post.Message for the converted channel
token (e.g., "~random"), introduce a foundChannelMention boolean (or reuse a
descriptive variable) and set it when strings.Contains(post.Message, "~random")
is true, then add an assertion (assert.True) to ensure the channel mention was
converted.
- Around line 331-340: TestTransformSlackE2ETeamConsistency spins up a separate
Docker stack; merge it into TestTransformSlackE2E as a subtest instead: move the
body of TestTransformSlackE2ETeamConsistency into a t.Run call inside
TestTransformSlackE2E, remove its separate ctx/testhelper SetupHelper/TearDown,
reuse the parent ctx and th from TestTransformSlackE2E, and ensure the subtest
still uses a unique team name and the same assertions (channel TeamId and user
team membership); delete the now-empty TestTransformSlackE2ETeamConsistency
function.
- Around line 79-84: The test reuses the package-level commands.RootCmd and
mutates it via SetArgs/Execute (with resetCobraFlags), which is fragile for
parallel subtests; fix by ensuring tests do not race: either create a fresh
cobra.Command instance per subtest (clone or construct a new RootCmd) and call
SetArgs/Execute on that instance instead of commands.RootCmd, or explicitly
document and enforce the no-parallel contract by removing any t.Parallel() and
registering a single t.Cleanup in the parent test that removes the log; also
replace repeated defer os.Remove("transform-slack.log") with a file name
constant (e.g., transformSlackLog) at file scope and call t.Cleanup(func(){
os.Remove(transformSlackLog) }) once in the parent so all subtests share the
single cleanup.
In `@go.mod`:
- Around line 15-16: The root go.mod currently includes heavy test-only deps
(github.com/testcontainers/testcontainers-go and modules/postgres) which pull in
many container/runtime transitive packages; create a separate sub-module (e.g.,
e2e/) with its own go.mod (module path like github.com/yourrepo/e2e) and move
all E2E tests and testcontainers imports into that directory, remove
testcontainers entries from the root go.mod, and update CI to run tests via cd
e2e && go test ./... so the main module (root go.mod) no longer carries the
testcontainers dependency graph.
- Rename test to "handles posts from multiple missing users" since no reactions are tested - Add zipWriterClosed guard in wrapJSONLInZip to prevent double-close when tempFile.Close() fails after zipWriter.Close() succeeds
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
testhelper/testhelper.go (1)
45-50:⚠️ Potential issue | 🟠 MajorRegister teardown with
t.Cleanupduring setup to prevent leaked Docker resources on setup failure.If setup fails after some resources are created,
require.NoErroraborts before callers can reliably deferTearDown, leaving containers/network behind. Also makeTearDownidempotent so repeated calls are safe.Suggested fix
func SetupHelper(t *testing.T) *TestHelper { th := &TestHelper{ t: t, tearDowns: make([]TearDownFunc, 0), AdminPassword: "testpassword123", } + t.Cleanup(th.TearDown) ctx, cancel := context.WithTimeout(context.Background(), setupTimeout) defer cancel() @@ func (th *TestHelper) TearDown() { ctx := context.Background() - // Tear down in reverse order - for i := len(th.tearDowns) - 1; i >= 0; i-- { + tearDowns := th.tearDowns + th.tearDowns = nil // idempotent + for i := len(tearDowns) - 1; i >= 0; i-- { - if err := th.tearDowns[i](ctx); err != nil { + if err := tearDowns[i](ctx); err != nil { th.t.Logf("Error during teardown: %v", err) } } }Also applies to: 56-71, 111-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/testhelper.go` around lines 45 - 50, The SetupHelper function currently builds TestHelper without registering teardown with t.Cleanup and TearDown may not be idempotent; modify SetupHelper to call t.Cleanup(th.TearDown) immediately after creating th so teardown always runs if setup fails, and update the TestHelper.TearDown method (and any helper functions referenced by TearDown) to be safe to call multiple times (idempotent) by checking/clearing resource pointers/flags and ignoring already-cleaned resources; apply the same t.Cleanup registration pattern to other setup constructors noted (lines around the other occurrences) so all setup paths register cleanup.
🧹 Nitpick comments (1)
testhelper/slack_fixtures_test.go (1)
203-203: Avoid hard-coding dated post filenames in ZIP assertions.These assertions couple tests to an internal naming detail (
2025-01-01.json) instead of the contract (channel folder contains JSON post files). If filename generation changes, these tests fail without a behavior regression.Refactor idea
- if file.Name == "general/2025-01-01.json" { + if strings.HasPrefix(file.Name, "general/") && strings.HasSuffix(file.Name, ".json") {// helper used by tests func findChannelPostFile(files []*zip.File, channel string) *zip.File { for _, f := range files { if strings.HasPrefix(f.Name, channel+"/") && strings.HasSuffix(f.Name, ".json") { return f } } return nil }Also applies to: 355-355, 1008-1008
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/slack_fixtures_test.go` at line 203, Replace the hard-coded filename equality (e.g., the check comparing file.Name == "general/2025-01-01.json") with a more resilient check that looks for any JSON post file inside the channel folder; add and use a helper like findChannelPostFile that scans zip files for strings.HasPrefix(f.Name, channel+"/") && strings.HasSuffix(f.Name, ".json") and return the matching *zip.File, then update the assertions at the locations referencing the dated filenames (including the occurrences around the original check and the other noted lines) to use this helper and assert the returned file is non-nil and has the expected contents instead of relying on an exact filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testhelper/testhelper.go`:
- Around line 509-524: The isZipFile function is too permissive by only checking
the first two bytes ("PK"); update it to read a full 4-byte header (use
io.ReadFull or check read length) and compare against known ZIP signatures such
as PK\x03\x04 (0x50 0x4B 0x03 0x04), PK\x05\x06 (0x50 0x4B 0x05 0x06) and
PK\x07\x08 (0x50 0x4B 0x07 0x08), return false on short reads or read errors,
and keep the existing defer file.Close() behavior; modify the isZipFile function
accordingly to perform these explicit 4-byte comparisons.
- Line 65: The test currently logs the full PostgreSQL connection string via
t.Logf which can leak credentials; change the logging to emit only non-sensitive
metadata: parse postgresConnStr to extract and log host and port (or
alternatively log the test container name if available) and remove any
username/password/database fields from outputs; update the call site that uses
t.Logf("PostgreSQL started with connection string: %s", postgresConnStr) to
instead log only the extracted host:port or container identifier.
---
Duplicate comments:
In `@testhelper/testhelper.go`:
- Around line 45-50: The SetupHelper function currently builds TestHelper
without registering teardown with t.Cleanup and TearDown may not be idempotent;
modify SetupHelper to call t.Cleanup(th.TearDown) immediately after creating th
so teardown always runs if setup fails, and update the TestHelper.TearDown
method (and any helper functions referenced by TearDown) to be safe to call
multiple times (idempotent) by checking/clearing resource pointers/flags and
ignoring already-cleaned resources; apply the same t.Cleanup registration
pattern to other setup constructors noted (lines around the other occurrences)
so all setup paths register cleanup.
---
Nitpick comments:
In `@testhelper/slack_fixtures_test.go`:
- Line 203: Replace the hard-coded filename equality (e.g., the check comparing
file.Name == "general/2025-01-01.json") with a more resilient check that looks
for any JSON post file inside the channel folder; add and use a helper like
findChannelPostFile that scans zip files for strings.HasPrefix(f.Name,
channel+"/") && strings.HasSuffix(f.Name, ".json") and return the matching
*zip.File, then update the assertions at the locations referencing the dated
filenames (including the occurrences around the original check and the other
noted lines) to use this helper and assert the returned file is non-nil and has
the expected contents instead of relying on an exact filename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e171c0a1-63af-4e6a-af15-c2986dcee5b8
📒 Files selected for processing (2)
testhelper/slack_fixtures_test.gotesthelper/testhelper.go
larkox
approved these changes
Mar 5, 2026
larkox
left a comment
larkox
left a comment
There was a problem hiding this comment.
LGTM. I left many comments to consider, but probably most of them are not valid or not relevant for this.
Replace network.WithNetwork with a partial DockerNetwork struct with the direct network.WithNetworkName call. This avoids constructing a DockerNetwork with an empty ID field and is the idiomatic API for attaching a container to an existing network by name.
Register t.Cleanup(th.TearDown) immediately after creating the TestHelper so that Docker resources are always cleaned up, even if a later setup step (e.g. CreateMattermostContainer) calls require.NoError and aborts before the caller's defer runs. Make TearDown() idempotent by clearing th.tearDowns after execution so it is safe to call from both t.Cleanup and an explicit defer.
The connection string includes username and password, which would be visible in CI logs. Replace with a generic log message.
Only checking the first two bytes ("PK") can false-positive on
non-ZIP files. Check the full 4-byte magic for local file header
(PK\x03\x04), end of central directory (PK\x05\x06), and spanned
archive (PK\x07\x08).
filepath.Rel returns OS-specific separators (backslashes on Windows). The ZIP specification requires forward slashes for entry paths. Add filepath.ToSlash after filepath.Rel to ensure compliance.
time.Now().UnixNano()%10000 has only 4 digits of entropy and can collide when parallel CI jobs run simultaneously. Replace with crypto/rand hex suffix for reliable uniqueness.
The constant is used as a deadline for time.After, not as an attempt count. With exponential backoff only ~8-10 polls actually occur. Rename to importJobTimeout with an explicit time.Duration value and inline the initial poll interval to make the intent clearer.
Replace ${GO_TEST_FLAGS} with $(GO_TEST_FLAGS) to match the $(...)
convention used everywhere else in the Makefile.
allChannels() was called twice, allocating two identical slices. Store the result in a local variable and reuse it.
Extract the log filename to a constant and use a single t.Cleanup
in each parent test function instead of repeating
defer os.Remove("transform-slack.log") in every subtest.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
testhelper/slack_fixtures.go (1)
199-209: Consider sanitizing channel names to prevent path traversal.While this is test infrastructure with controlled inputs, the
channelNameis used directly infilepath.Join(tempDir, channelName)at line 201. If a channel name contained path separators like../malicious, it could write files outside the intended directory.Since this mirrors how the parser expects the structure (channel name as directory), consider adding validation in
validate()to reject channel names with path separators:🛡️ Suggested validation addition
// Validate posts reference existing channels and users for channelName, posts := range b.posts { + // Reject channel names that could cause path traversal + if strings.Contains(channelName, "/") || strings.Contains(channelName, "\\") || strings.Contains(channelName, "..") { + return fmt.Errorf("channel name %q contains invalid path characters", channelName) + } if !channelNames[channelName] { return fmt.Errorf("posts exist for non-existent channel %q", channelName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/slack_fixtures.go` around lines 199 - 209, The test fixture writes directories using channel names from b.posts directly, which allows path traversal (e.g., "../malicious") when used in filepath.Join(tempDir, channelName); update the validation logic (add checks in the existing validate() method for the test fixture type that contains b.posts) to reject or sanitize any channel name containing path separators or traversal sequences (e.g., "/" or string(os.PathSeparator) and ".."); ensure writeJSONFile and the loop that builds channelDir continue to assume validated safe names so creation uses only allowed directory names.testhelper/containers.go (1)
231-259: TheMM_SERVICESETTINGS_SITEURLis set to a placeholder value.The environment variable
MM_SERVICESETTINGS_SITEURLat line 238 is hardcoded to"http://localhost:8065", but the actualsiteURLis computed dynamically at lines 285 based on the mapped host/port. While this likely works because Mattermost uses this primarily for generating links in notifications/emails rather than for request handling, consider whether this could cause issues in certain test scenarios.For consistency, you could either:
- Document that this is intentional (e.g., comment explaining the placeholder is fine for tests)
- Or defer container startup and set it after port mapping (though this adds complexity)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/containers.go` around lines 231 - 259, The MM_SERVICESETTINGS_SITEURL env var in the ContainerRequest (key MM_SERVICESETTINGS_SITEURL) is hardcoded to "http://localhost:8065"; either document this as intentional or set it dynamically after the container's mapped port is known: simplest fix — add a short comment next to MM_SERVICESETTINGS_SITEURL explaining the placeholder is fine for tests and does not affect request handling; alternative fix — start the container, read the mapped port, compute siteURL (the variable currently computed later), then inject that value into the container environment (or restart/configure the app) before running tests so the computed siteURL is used instead of the hardcoded value.testhelper/testhelper.go (1)
353-361: Status parsing relies on exact string matching in mmctl output.The status detection relies on substring matching for
"Status: success","Status: error", and"Status: canceled". If mmctl output format changes across versions (e.g., different casing or wording), this could break.Since the Mattermost container version can vary (via environment variables), consider adding a comment documenting the expected output format or adding more flexible matching:
📝 Suggested documentation
+ // mmctl import job show output format (Mattermost 10.5+): + // "Status: success" | "Status: error" | "Status: pending" | "Status: canceled" if strings.Contains(statusOutput, "Status: success") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/testhelper.go` around lines 353 - 361, The status parsing in the import status check uses exact substring matches on statusOutput which is brittle across mmctl/Mattermost versions; update the check to perform case-insensitive, whitespace-tolerant matching (e.g., compile a regexp like (?i)\bstatus:\s*(success|error|canceled)\b and switch on the captured status) for the logic that currently looks for "Status: success"/"Status: error"/"Status: canceled" (referencing the statusOutput variable and the block that logs jobID via th.t.Logf), and add a short comment above the check documenting the assumed mmctl output format and why a case-insensitive regexp is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commands/transform_slack_e2e_test.go`:
- Around line 39-43: The helper uniqueTeamName currently ignores the error from
rand.Read which can produce non-unique names on failure; update uniqueTeamName
to check the error returned by rand.Read(b) and propagate it instead of
discarding it (e.g., change uniqueTeamName to return (string, error) and return
a wrapped error on rand.Read failure), then update all callers to handle the
error (or fail the test) so we never generate a name when random bytes couldn't
be read. Use the function name uniqueTeamName and the rand.Read call site to
locate and fix the code.
---
Nitpick comments:
In `@testhelper/containers.go`:
- Around line 231-259: The MM_SERVICESETTINGS_SITEURL env var in the
ContainerRequest (key MM_SERVICESETTINGS_SITEURL) is hardcoded to
"http://localhost:8065"; either document this as intentional or set it
dynamically after the container's mapped port is known: simplest fix — add a
short comment next to MM_SERVICESETTINGS_SITEURL explaining the placeholder is
fine for tests and does not affect request handling; alternative fix — start the
container, read the mapped port, compute siteURL (the variable currently
computed later), then inject that value into the container environment (or
restart/configure the app) before running tests so the computed siteURL is used
instead of the hardcoded value.
In `@testhelper/slack_fixtures.go`:
- Around line 199-209: The test fixture writes directories using channel names
from b.posts directly, which allows path traversal (e.g., "../malicious") when
used in filepath.Join(tempDir, channelName); update the validation logic (add
checks in the existing validate() method for the test fixture type that contains
b.posts) to reject or sanitize any channel name containing path separators or
traversal sequences (e.g., "/" or string(os.PathSeparator) and ".."); ensure
writeJSONFile and the loop that builds channelDir continue to assume validated
safe names so creation uses only allowed directory names.
In `@testhelper/testhelper.go`:
- Around line 353-361: The status parsing in the import status check uses exact
substring matches on statusOutput which is brittle across mmctl/Mattermost
versions; update the check to perform case-insensitive, whitespace-tolerant
matching (e.g., compile a regexp like (?i)\bstatus:\s*(success|error|canceled)\b
and switch on the captured status) for the logic that currently looks for
"Status: success"/"Status: error"/"Status: canceled" (referencing the
statusOutput variable and the block that logs jobID via th.t.Logf), and add a
short comment above the check documenting the assumed mmctl output format and
why a case-insensitive regexp is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b0b4458-b92d-43e3-9380-cc7f7d0fb8f5
📒 Files selected for processing (5)
Makefilecommands/transform_slack_e2e_test.gotesthelper/containers.gotesthelper/slack_fixtures.gotesthelper/testhelper.go
The SiteURL env var is set to the container-internal address because the host-mapped port is not known until after the container starts. Add a comment explaining this is intentional and only affects Mattermost's self-referential links, not the test API client.
Guard against infinite pagination in fetchLatestStableTag by capping at 5 pages (500 tags). This prevents the loop from running forever if the Docker Hub API returns a malformed or circular next link.
The resolved tag is already validated by assertions; logging it adds noise without aiding debugging.
…lder A malformed channel name like "../../etc" would cause the export builder to write files outside the temporary directory. Validate that the resolved channel directory stays within the temp dir.
The reader opened via os.Open is assigned to the same variable that already has a defer reader.Close() registered at line 366. The extra defer inside the USERS_JSON_FILE branch is redundant and misleading.
Remove extra alignment whitespace in env map literals that was causing golangci-lint gofmt check failures in CI.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
testhelper/slack_fixtures.go (1)
239-247: PropagateClose()errors when finalizing the ZIP archive.
Line 246andLine 243defer closes but discard close failures. For archive writers, close errors are important because final flush happens there.🔧 Proposed refactor
import ( "archive/zip" "encoding/json" + "errors" "fmt" "io" "os" "path/filepath" "strings" @@ -func (b *SlackExportBuilder) createZipFile(outputPath, sourceDir string) error { +func (b *SlackExportBuilder) createZipFile(outputPath, sourceDir string) (retErr error) { zipFile, err := os.Create(outputPath) if err != nil { return fmt.Errorf("failed to create zip file: %w", err) } - defer zipFile.Close() + defer func() { + retErr = errors.Join(retErr, zipFile.Close()) + }() archive := zip.NewWriter(zipFile) - defer archive.Close() + defer func() { + retErr = errors.Join(retErr, archive.Close()) + }() - return filepath.Walk(sourceDir, func(path string, info os.FileInfo, err error) error { + retErr = filepath.Walk(sourceDir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } @@ _, err = io.Copy(writer, file) return err }) + return retErr }Also applies to: 248-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/slack_fixtures.go` around lines 239 - 247, The deferred Close calls on zipFile and archive (created via os.Create -> zipFile and zip.NewWriter -> archive) are currently discarding errors; change them to propagate Close() failures by capturing their returned error and returning it if non-nil (e.g., replace plain deferred Close() with defers that assign to the function error return or check/return the error after closing), ensuring the archive.Close() error (final flush) is not ignored and any zipFile.Close() error is returned to the caller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testhelper/containers.go`:
- Around line 236-255: The file testhelper/containers.go has incorrect
formatting in the Env map literal (around the map initialization for
MM_SQLSETTINGS_DRIVERNAME / MM_LOGSETTINGS_ENABLEFILE) causing CI gofmt
failures; run gofmt -w testhelper/containers.go (or your editor's
gofmt/format-on-save) to reformat the file so the map keys/values and comments
comply with gofmt, then re-run CI.
In `@testhelper/slack_fixtures.go`:
- Around line 201-205: The loop that writes fixtures accepts channelName with
path separators which creates nested dirs and silently drops posts; in the block
iterating over b.posts (variables: channelName, posts, channelDir,
filepath.Join, filepath.Rel, strings.HasPrefix) explicitly reject any
channelName containing path separators (both '/' and '\\' and os.PathSeparator)
and return an error instead of attempting to join; update the same validation in
the other similar posts iteration (the second block that builds channelDir) so
channel names must match the single-segment pattern expected by the parser (no
nested paths).
---
Nitpick comments:
In `@testhelper/slack_fixtures.go`:
- Around line 239-247: The deferred Close calls on zipFile and archive (created
via os.Create -> zipFile and zip.NewWriter -> archive) are currently discarding
errors; change them to propagate Close() failures by capturing their returned
error and returning it if non-nil (e.g., replace plain deferred Close() with
defers that assign to the function error return or check/return the error after
closing), ensuring the archive.Close() error (final flush) is not ignored and
any zipFile.Close() error is returned to the caller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6a00b6b-f482-4ea7-b808-1e42625a5304
📒 Files selected for processing (3)
testhelper/containers.gotesthelper/containers_test.gotesthelper/slack_fixtures.go
Comment on lines
+201
to
+205
| for channelName, posts := range b.posts { | ||
| channelDir := filepath.Join(tempDir, channelName) | ||
| // Guard against path traversal via malformed channel names | ||
| if rel, err := filepath.Rel(tempDir, channelDir); err != nil || strings.HasPrefix(rel, "..") { | ||
| return fmt.Errorf("channel name %q results in path traversal", channelName) |
There was a problem hiding this comment.
Channel names with path separators can silently drop posts during parse.
Line 202 currently accepts names containing / or \. That creates nested paths (e.g., team/a/2025-01-01.json), while the parser only accepts exactly <channel>/<file>.json, so posts get skipped.
🔧 Proposed fix
for channelName, posts := range b.posts {
+ if channelName == "" || strings.ContainsAny(channelName, `/\`) {
+ return fmt.Errorf("invalid channel name %q: must not be empty or contain path separators", channelName)
+ }
channelDir := filepath.Join(tempDir, channelName)
// Guard against path traversal via malformed channel names
if rel, err := filepath.Rel(tempDir, channelDir); err != nil || strings.HasPrefix(rel, "..") {
return fmt.Errorf("channel name %q results in path traversal", channelName)
}Also applies to: 211-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testhelper/slack_fixtures.go` around lines 201 - 205, The loop that writes
fixtures accepts channelName with path separators which creates nested dirs and
silently drops posts; in the block iterating over b.posts (variables:
channelName, posts, channelDir, filepath.Join, filepath.Rel, strings.HasPrefix)
explicitly reject any channelName containing path separators (both '/' and '\\'
and os.PathSeparator) and return an error instead of attempting to join; update
the same validation in the other similar posts iteration (the second block that
builds channelDir) so channel names must match the single-segment pattern
expected by the parser (no nested paths).
Manage golangci-lint locally in tools/bin/ pinned to v2.10.1, downloaded automatically from GitHub releases instead of requiring a global install. Also add a time-based fallback in uniqueTeamName when crypto/rand fails.
This was referenced Mar 5, 2026
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
2 participants
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.
Summary
Closes #72
Summary by CodeRabbit
New Features
Tests
Chores