feat(crossseed): allow custom instance link dirs#1574
Open
feat(crossseed): allow custom instance link dirs#1574
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds per-instance hardlink/reflink directory override ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API Handler"
participant Service
participant LinkDir as "internal/linkdir"
participant DB
participant FS as "Filesystem"
Client->>API: POST/PUT /api/instances (linkDirName)
API->>DB: INSERT/UPDATE instance (link_dir_name)
Client->>API: POST /cross-seed (request)
API->>Service: start cross-seed
Service->>LinkDir: FindMatchingBaseDir(configuredDirs, sourcePath)
LinkDir-->>Service: selectedBaseDir
Service->>LinkDir: BuildDestDir(baseDir,preset,group,hash,name,files)
LinkDir-->>Service: destDir
Service->>FS: ensure destDir exists / create dirs
Service->>FS: perform hardlink/reflink operations
Service->>API: return result
API->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/api/handlers/instances.go (2)
637-727:⚠️ Potential issue | 🟡 MinorNormalize
LinkDirNamebefore storing it.
linkdir.EffectiveInstanceDirNametrims the override at use time, but this handler persistsreq.LinkDirNameverbatim. A whitespace-only value is therefore stored and echoed back by the API even though the linkdir logic will ignore it as empty.Small fix
// Validate input if req.Name == "" || req.Host == "" { RespondError(w, http.StatusBadRequest, "Name and host are required") return } + + if req.LinkDirName != nil { + trimmed := strings.TrimSpace(*req.LinkDirName) + req.LinkDirName = &trimmed + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/instances.go` around lines 637 - 727, The handler currently persists req.LinkDirName verbatim which allows whitespace-only values to be stored; normalize LinkDirName by trimming whitespace (e.g., strings.TrimSpace) and treat an empty result as nil/empty before building models.InstanceUpdateParams so UpdateInstanceRequest.LinkDirName is saved in normalized form (ensure the normalized value is assigned into updateParams.LinkDirName and that linkdir.EffectiveInstanceDirName will then behave consistently).
374-385:⚠️ Potential issue | 🟠 MajorWire
linkDirNameinto instance creation too.The new override is update-only right now. A create payload that includes
linkDirNamewill be silently ignored here, so API clients cannot create an instance with its by-instance link directory configured in a single request.Suggested direction
type CreateInstanceRequest struct { Name string `json:"name"` Host string `json:"host"` Username string `json:"username"` Password string `json:"password"` BasicUsername *string `json:"basicUsername,omitempty"` BasicPassword *string `json:"basicPassword,omitempty"` TLSSkipVerify bool `json:"tlsSkipVerify,omitempty"` HasLocalFilesystemAccess *bool `json:"hasLocalFilesystemAccess,omitempty"` + LinkDirName *string `json:"linkDirName,omitempty"` ReannounceSettings *InstanceReannounceSettingsPayload `json:"reannounceSettings,omitempty"` }Then thread that value through
InstanceStore.Create(...)(preferred), or apply it immediately after create before building the response.Also applies to: 591-625
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/instances.go` around lines 374 - 385, Add a LinkDirName field to the CreateInstanceRequest struct (json:"linkDirName,omitempty") and ensure the CreateInstance handler threads that value into instance creation by passing it into InstanceStore.Create (preferred) or by setting the created instance's linkDirName immediately after creation and before persisting/building the response (e.g., in the CreateInstance handler or before buildInstanceResponse). Update the InstanceStore.Create signature/implementation to accept and persist the linkDirName, and mirror the same fix for the update path referenced around the 591-625 change so creates and updates both honor linkDirName.internal/services/crossseed/service.go (1)
10557-10561:⚠️ Potential issue | 🔴 CriticalVerify
LinkDirNameescape vulnerability—path traversal sequences are not sanitized.The per-instance
LinkDirNameoverride flows throughlinkdir.EffectiveInstanceDirName(which only trims whitespace) intolinkdir.BuildDestDir, where it's passed topathutil.SanitizePathSegment. However,SanitizePathSegmentremoves only illegal Windows filesystem characters (<>:"/\|?*) and control characters—it does not filter..or reject absolute paths. A crafted override like../../../etcwill pass through sanitization and then traverse outsideselectedBaseDirwhen joined viafilepath.Join.Recommend:
- Reject overrides containing
..or/inEffectiveInstanceDirName, or- Validate the final joined path explicitly contains
baseDirusingfilepath.Reloreval-symlinkspattern (seeinternal/api/handlers/torrents.go:2644-2646for reference).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/service.go` around lines 10557 - 10561, The EffectiveInstanceDirName (from LinkDirName) currently only trims whitespace and then flows into linkdir.BuildDestDir which uses pathutil.SanitizePathSegment but does not block path traversal like ".." or absolute paths; update validation so that EffectiveInstanceDirName is rejected if it contains path separators or parent-traversal segments (e.g., "/" or "\" or "..") before calling BuildDestDir, or after joining validate the final destination against selectedBaseDir (the base dir used in BuildDestDir) by computing filepath.Rel (or resolving symlinks) and ensuring the resulting path is within selectedBaseDir; adjust functions LinkDirName/EffectiveInstanceDirName and linkdir.BuildDestDir to enforce this check and return an error when validation fails.
🧹 Nitpick comments (3)
internal/models/instance_test.go (1)
154-207: Add one explicitLinkDirNameround-trip assertion here.These fixture updates keep the schema in sync, but the test still never creates/updates/fetches an instance with
LinkDirNameset. That leaves the newSELECT/Scan/UPDATEwiring unverified. One assertion covering update + get/list would lock the feature in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/models/instance_test.go` around lines 154 - 207, The test adds LinkDirName to schema but never verifies it; update the instance_test to create/update an Instance with LinkDirName set and assert it round-trips via the repository methods (e.g. create via the existing insert/UpdateInstance method and fetch via GetInstance/GetInstances or instances_view query); specifically set a non-empty LinkDirName on the test instance, call the code path that persists updates (function/methods handling instance creation/update and the instances_view SELECT/Scan wiring), then assert the fetched Instance.LinkDirName equals the original value to cover both UPDATE and SELECT/Scan behavior.internal/linkdir/linkdir_test.go (1)
17-71: Prefer a table-driven suite for these linkdir cases.These are all small input/output permutations of the same helpers, so a single table-driven test would be easier to extend for additional presets and error cases without duplicating setup.
As per coding guidelines: "Prefer table-driven test cases in Go tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/linkdir/linkdir_test.go` around lines 17 - 71, The tests for EffectiveInstanceDirName, BuildDestDir (both by-instance and by-tracker variants) and FindMatchingBaseDir should be refactored into one or more table-driven test(s) instead of multiple separate functions; create a slice of test cases that include inputs (e.g., instanceName, overrideDir, mode "by-instance"/"by-tracker", infoHash, title, files) and expected outputs and iterate over them inside a single TestLinkdirHelpers loop, running t.Run for each case and using t.Parallel where appropriate; for the FindMatchingBaseDir case include the temporary directory setup (currently in TestFindMatchingBaseDir) as part of a specific table entry (or a separate subtest) so setup/teardown remains correct while avoiding duplicated test functions, and assert expected errors/results per case using require/assert as in the originals (referencing EffectiveInstanceDirName, BuildDestDir, FindMatchingBaseDir).internal/services/dirscan/inject.go (1)
445-447: Resolve tracker display names lazily.For
by-instanceandflat,buildLinkDestDirignorestrackerDisplayName, but this path still parses the announce URL and may calltrackerCustomizationStore.List(ctx)on every inject. Guard the lookup behindinstance.HardlinkDirPreset == "by-tracker"so the extra work only happens when it can affectdestDir.♻️ Proposed simplification
- incomingTrackerDomain := crossseed.ParseTorrentAnnounceDomain(req.TorrentBytes) - trackerDisplayName := i.resolveTrackerDisplayName(ctx, incomingTrackerDomain, indexerName(req.SearchResult)) + trackerDisplayName := "" + if instance.HardlinkDirPreset == "by-tracker" { + incomingTrackerDomain := crossseed.ParseTorrentAnnounceDomain(req.TorrentBytes) + trackerDisplayName = i.resolveTrackerDisplayName(ctx, incomingTrackerDomain, indexerName(req.SearchResult)) + } destDir := buildLinkDestDir(instance, selectedBaseDir, req.ParsedTorrent.InfoHash, req.ParsedTorrent.Name, trackerDisplayName, incomingFiles)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/dirscan/inject.go` around lines 445 - 447, Avoid parsing announce domain and resolving trackerDisplayName on every inject when it won't affect destination dir: move the call to crossseed.ParseTorrentAnnounceDomain(req.TorrentBytes) and i.resolveTrackerDisplayName(...) behind a conditional that checks instance.HardlinkDirPreset == "by-tracker"; only compute incomingTrackerDomain and trackerDisplayName in that branch (so buildLinkDestDir is called with a nil/empty trackerDisplayName for other presets). This prevents unnecessary calls such as trackerCustomizationStore.List(ctx) inside i.resolveTrackerDisplayName when the hardlink preset is not "by-tracker".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/models/instance.go`:
- Around line 594-595: The LinkDirName field is only applied on updates because
InstanceStore.Create and its INSERT omit the link_dir_name column; add
link_dir_name to the Create path so new instances persist the provided value.
Update the InstanceCreateParams/constructor used by InstanceStore.Create (or the
Create signature) to accept LinkDirName, propagate that value into the SQL
INSERT in InstanceStore.Create (include link_dir_name in the column list and
bind the parameter), and ensure the Get/List/db scan logic remains compatible
with nullable string pointers so newly created rows show the provided
LinkDirName immediately (reference symbols: LinkDirName, InstanceUpdateParams,
InstanceStore.Create, Get, List, and the INSERT for link_dir_name).
---
Outside diff comments:
In `@internal/api/handlers/instances.go`:
- Around line 637-727: The handler currently persists req.LinkDirName verbatim
which allows whitespace-only values to be stored; normalize LinkDirName by
trimming whitespace (e.g., strings.TrimSpace) and treat an empty result as
nil/empty before building models.InstanceUpdateParams so
UpdateInstanceRequest.LinkDirName is saved in normalized form (ensure the
normalized value is assigned into updateParams.LinkDirName and that
linkdir.EffectiveInstanceDirName will then behave consistently).
- Around line 374-385: Add a LinkDirName field to the CreateInstanceRequest
struct (json:"linkDirName,omitempty") and ensure the CreateInstance handler
threads that value into instance creation by passing it into
InstanceStore.Create (preferred) or by setting the created instance's
linkDirName immediately after creation and before persisting/building the
response (e.g., in the CreateInstance handler or before buildInstanceResponse).
Update the InstanceStore.Create signature/implementation to accept and persist
the linkDirName, and mirror the same fix for the update path referenced around
the 591-625 change so creates and updates both honor linkDirName.
In `@internal/services/crossseed/service.go`:
- Around line 10557-10561: The EffectiveInstanceDirName (from LinkDirName)
currently only trims whitespace and then flows into linkdir.BuildDestDir which
uses pathutil.SanitizePathSegment but does not block path traversal like ".." or
absolute paths; update validation so that EffectiveInstanceDirName is rejected
if it contains path separators or parent-traversal segments (e.g., "/" or "\" or
"..") before calling BuildDestDir, or after joining validate the final
destination against selectedBaseDir (the base dir used in BuildDestDir) by
computing filepath.Rel (or resolving symlinks) and ensuring the resulting path
is within selectedBaseDir; adjust functions LinkDirName/EffectiveInstanceDirName
and linkdir.BuildDestDir to enforce this check and return an error when
validation fails.
---
Nitpick comments:
In `@internal/linkdir/linkdir_test.go`:
- Around line 17-71: The tests for EffectiveInstanceDirName, BuildDestDir (both
by-instance and by-tracker variants) and FindMatchingBaseDir should be
refactored into one or more table-driven test(s) instead of multiple separate
functions; create a slice of test cases that include inputs (e.g., instanceName,
overrideDir, mode "by-instance"/"by-tracker", infoHash, title, files) and
expected outputs and iterate over them inside a single TestLinkdirHelpers loop,
running t.Run for each case and using t.Parallel where appropriate; for the
FindMatchingBaseDir case include the temporary directory setup (currently in
TestFindMatchingBaseDir) as part of a specific table entry (or a separate
subtest) so setup/teardown remains correct while avoiding duplicated test
functions, and assert expected errors/results per case using require/assert as
in the originals (referencing EffectiveInstanceDirName, BuildDestDir,
FindMatchingBaseDir).
In `@internal/models/instance_test.go`:
- Around line 154-207: The test adds LinkDirName to schema but never verifies
it; update the instance_test to create/update an Instance with LinkDirName set
and assert it round-trips via the repository methods (e.g. create via the
existing insert/UpdateInstance method and fetch via GetInstance/GetInstances or
instances_view query); specifically set a non-empty LinkDirName on the test
instance, call the code path that persists updates (function/methods handling
instance creation/update and the instances_view SELECT/Scan wiring), then assert
the fetched Instance.LinkDirName equals the original value to cover both UPDATE
and SELECT/Scan behavior.
In `@internal/services/dirscan/inject.go`:
- Around line 445-447: Avoid parsing announce domain and resolving
trackerDisplayName on every inject when it won't affect destination dir: move
the call to crossseed.ParseTorrentAnnounceDomain(req.TorrentBytes) and
i.resolveTrackerDisplayName(...) behind a conditional that checks
instance.HardlinkDirPreset == "by-tracker"; only compute incomingTrackerDomain
and trackerDisplayName in that branch (so buildLinkDestDir is called with a
nil/empty trackerDisplayName for other presets). This prevents unnecessary calls
such as trackerCustomizationStore.List(ctx) inside i.resolveTrackerDisplayName
when the hardlink preset is not "by-tracker".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 657d6f43-a090-48d4-ae3f-508141b4b7a0
📒 Files selected for processing (16)
documentation/docs/features/cross-seed/hardlink-mode.mddocumentation/docs/features/cross-seed/link-directories.mddocumentation/static/openapi.yamlinternal/api/handlers/instances.gointernal/database/migrations/067_add_instance_link_dir_name.sqlinternal/database/postgres_migrations/068_add_instance_link_dir_name.sqlinternal/linkdir/linkdir.gointernal/linkdir/linkdir_test.gointernal/models/instance.gointernal/models/instance_test.gointernal/services/crossseed/hardlink_mode_test.gointernal/services/crossseed/service.gointernal/services/dirscan/inject.gointernal/web/swagger/openapi.yamlweb/src/pages/CrossSeedPage.tsxweb/src/types/index.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/api/handlers/instances.go (1)
199-223:⚠️ Potential issue | 🟡 MinorCopy
FallbackToRegularModein the cancellation fallback too.This branch now returns
LinkDirName, but it still dropsFallbackToRegularMode, so a canceled/timed-out list response can silently report that flag asfalseeven when the instance is configured otherwise.Suggested fix
UseReflinks: instances[i].UseReflinks, + FallbackToRegularMode: instances[i].FallbackToRegularMode, Connected: false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/instances.go` around lines 199 - 223, The cancellation/timed-out response builder for InstanceResponse is missing the FallbackToRegularMode field, so canceled responses can incorrectly show it as false; update the fallback branch that constructs InstanceResponse (the same place setting LinkDirName, Connected, HasDecryptionError, ConnectionStatus, etc.) to also copy FallbackToRegularMode from the model (instances[i].FallbackToRegularMode) into the response struct so the flag mirrors the real instance configuration.internal/models/instance.go (1)
360-392:⚠️ Potential issue | 🟠 MajorReject invalid
link_dir_nameon write.
CreateandUpdatepersist the raw override as-is. A value like../xseedormovies/4kwill save successfully here, then fail later wheninternal/linkdir/linkdir.govalidates the name during dirscan/cross-seed path construction. Validate this beforeINSERT/UPDATEso the request fails at save time instead of storing unusable state.Also applies to: 724-727
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/models/instance.go` around lines 360 - 392, Create and Update are persisting raw link_dir_name values that may be invalid (e.g., "../xseed" or "movies/4k"); before executing the INSERT/UPDATE in internal/models/instance.go (the block that builds linkDirValue and calls tx.QueryRowContext), validate linkDirName when non-nil by calling the linkdir package's validation function (e.g., linkdir.ValidateName or the appropriate validator in internal/linkdir/linkdir.go) and return a descriptive error if validation fails; apply the same pre-insert/pre-update check in both Create and Update (also the code paths around the other mentioned lines) so invalid names are rejected at save time instead of being stored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/api/handlers/instances.go`:
- Around line 199-223: The cancellation/timed-out response builder for
InstanceResponse is missing the FallbackToRegularMode field, so canceled
responses can incorrectly show it as false; update the fallback branch that
constructs InstanceResponse (the same place setting LinkDirName, Connected,
HasDecryptionError, ConnectionStatus, etc.) to also copy FallbackToRegularMode
from the model (instances[i].FallbackToRegularMode) into the response struct so
the flag mirrors the real instance configuration.
In `@internal/models/instance.go`:
- Around line 360-392: Create and Update are persisting raw link_dir_name values
that may be invalid (e.g., "../xseed" or "movies/4k"); before executing the
INSERT/UPDATE in internal/models/instance.go (the block that builds linkDirValue
and calls tx.QueryRowContext), validate linkDirName when non-nil by calling the
linkdir package's validation function (e.g., linkdir.ValidateName or the
appropriate validator in internal/linkdir/linkdir.go) and return a descriptive
error if validation fails; apply the same pre-insert/pre-update check in both
Create and Update (also the code paths around the other mentioned lines) so
invalid names are rejected at save time instead of being stored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c724f854-c447-4a74-a127-2495dd1df1cc
📒 Files selected for processing (14)
internal/api/handlers/instances.gointernal/api/handlers/torrents_download_test.gointernal/linkdir/linkdir.gointernal/linkdir/linkdir_test.gointernal/models/crossseed_blocklist_test.gointernal/models/dirscan_run_status_test.gointernal/models/instance.gointernal/models/instance_test.gointernal/models/postgres_bool_args_test.gointernal/services/crossseed/hardlink_mode_test.gointernal/services/crossseed/service.gointernal/services/crossseed/service_search_test.gointernal/services/dirscan/cancel_scan_test.gointernal/services/dirscan/inject.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/linkdir/linkdir_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/handlers/instances.go (1)
609-624:⚠️ Potential issue | 🟠 MajorReturn 400 for invalid
LinkDirNameinput.The new store test in
internal/models/instance_test.goshowsCreate/Updatecan now returninvalid link directory namefor bad user input, but both handlers still collapse store validation failures into500. MalformedlinkDirNamepayloads will therefore surface as internal server errors instead of bad requests.Also applies to: 745-753
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/instances.go` around lines 609 - 624, When h.instanceStore.Create returns an error, treat a validation error for the link directory as a 400 not 500: check the returned error for the text "invalid link directory name" (or errors.Is against the store's specific sentinel error if available) and call RespondError(w, http.StatusBadRequest, ...) in that case; otherwise keep the existing 500 handling. Apply the same change to the corresponding Update call (h.instanceStore.Update) around the other handler block (the code region at ~745-753) so malformed LinkDirName payloads produce HTTP 400 responses.
🧹 Nitpick comments (2)
internal/linkdir/linkdir_test.go (1)
17-106: Prefer a table-driven suite for these linkdir cases.This is already a compact input/output matrix, so folding it into table-driven tests would cut duplication and make new edge cases much easier to add.
As per coding guidelines, "Prefer table-driven test cases in Go tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/linkdir/linkdir_test.go` around lines 17 - 106, Collapse the separate tests into table-driven subtests: create a slice of test cases (structs) for each logical group that include fields for name, inputs and expected output/error, then range over them calling t.Run(tc.name, func(t *testing.T){ t.Parallel(); ... assertions ... }); replace the standalone functions TestEffectiveInstanceDirName, TestEffectiveInstanceDirName_RejectsTraversal, TestValidateInstanceDirName, TestBuildDestDir_ByInstanceUsesOverride, TestBuildDestDir_ByTrackerFallsBackToUnknown, TestBuildDestDir_ByInstanceRejectsTraversal and TestFindMatchingBaseDir with table-driven variants that exercise EffectiveInstanceDirName, ValidateInstanceDirName, BuildDestDir and FindMatchingBaseDir respectively, using require.NoError/require.ErrorContains/assert.Equal/assert.Contains as in the originals so behavior and checks remain identical while reducing duplication.internal/models/instance_test.go (1)
451-455: Assert the row is unchanged after the rejected update.This only verifies that
Updatereturns an error. It will not catch a partial write if validation runs after some fields are persisted. Please reload the instance after the failure and assert thatLinkDirNameis still the previous value.🧪 Suggested assertion
_, err = store.Update(ctx, instance.ID, "Valid Instance", "http://localhost:8080", "testuser", "", nil, nil, &InstanceUpdateParams{LinkDirName: &invalidLinkDirName}) require.ErrorContains(t, err, "invalid link directory name") + + reloaded, err := store.Get(ctx, instance.ID) + require.NoError(t, err) + assert.Equal(t, "", reloaded.LinkDirName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/models/instance_test.go` around lines 451 - 455, After verifying Update returned an error, reload the instance from the store (e.g., call store.Get(ctx, instance.ID) or the appropriate fetch method) and assert that instance.LinkDirName still equals the original value (the value returned by the initial store.Create); this ensures no partial write occurred when Update with InstanceUpdateParams{LinkDirName: &invalidLinkDirName} failed. Use the same context and instance.ID to fetch and compare the LinkDirName field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/handlers/instances.go`:
- Around line 609-620: The handler currently calls instanceStore.Create (in
CreateInstance) and then persistReannounceSettings separately, allowing partial
commits if the second write fails; change this to perform both writes atomically
by using a transaction or an atomic store operation provided by the store (e.g.,
begin a transaction, call instanceStore.Create and persistReannounceSettings
inside it, then commit; rollback on any error) so no instance is visible without
its reannounce settings; apply the same pattern to the UpdateInstance path and
the other mentioned call sites (around persistReannounceSettings at 627-630,
735-758, 760-765) to ensure either both instance and reannounce settings persist
or neither do.
In `@internal/models/instance.go`:
- Around line 361-367: The validation currently treats any non-nil linkDirName
as an override and fails on empty/whitespace; change the logic in the block that
sets linkDirValue (and the similar block at 703-737) to treat "" or
all-whitespace as “no override”: trim the string pointer value, if the trimmed
result is empty then leave linkDirValue empty (do not call
linkdir.ValidateInstanceDirName), otherwise validate the trimmed value with
linkdir.ValidateInstanceDirName and assign the trimmed value to linkDirValue;
ensure you only call ValidateInstanceDirName when there is a non-empty trimmed
value.
---
Outside diff comments:
In `@internal/api/handlers/instances.go`:
- Around line 609-624: When h.instanceStore.Create returns an error, treat a
validation error for the link directory as a 400 not 500: check the returned
error for the text "invalid link directory name" (or errors.Is against the
store's specific sentinel error if available) and call RespondError(w,
http.StatusBadRequest, ...) in that case; otherwise keep the existing 500
handling. Apply the same change to the corresponding Update call
(h.instanceStore.Update) around the other handler block (the code region at
~745-753) so malformed LinkDirName payloads produce HTTP 400 responses.
---
Nitpick comments:
In `@internal/linkdir/linkdir_test.go`:
- Around line 17-106: Collapse the separate tests into table-driven subtests:
create a slice of test cases (structs) for each logical group that include
fields for name, inputs and expected output/error, then range over them calling
t.Run(tc.name, func(t *testing.T){ t.Parallel(); ... assertions ... }); replace
the standalone functions TestEffectiveInstanceDirName,
TestEffectiveInstanceDirName_RejectsTraversal, TestValidateInstanceDirName,
TestBuildDestDir_ByInstanceUsesOverride,
TestBuildDestDir_ByTrackerFallsBackToUnknown,
TestBuildDestDir_ByInstanceRejectsTraversal and TestFindMatchingBaseDir with
table-driven variants that exercise EffectiveInstanceDirName,
ValidateInstanceDirName, BuildDestDir and FindMatchingBaseDir respectively,
using require.NoError/require.ErrorContains/assert.Equal/assert.Contains as in
the originals so behavior and checks remain identical while reducing
duplication.
In `@internal/models/instance_test.go`:
- Around line 451-455: After verifying Update returned an error, reload the
instance from the store (e.g., call store.Get(ctx, instance.ID) or the
appropriate fetch method) and assert that instance.LinkDirName still equals the
original value (the value returned by the initial store.Create); this ensures no
partial write occurred when Update with InstanceUpdateParams{LinkDirName:
&invalidLinkDirName} failed. Use the same context and instance.ID to fetch and
compare the LinkDirName field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 922e14f7-e184-40e9-8740-a428f9e1bd44
📒 Files selected for processing (6)
internal/api/handlers/instances.gointernal/linkdir/linkdir.gointernal/linkdir/linkdir_test.gointernal/models/instance.gointernal/models/instance_test.gointernal/web/swagger/openapi.yaml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/models/instance.go (1)
748-754: Side effect on input parameterparams.LinkDirName.Line 753 mutates the caller's
params.LinkDirNamepointer by reassigning it to the normalized value. This works but leaks internal normalization state back to the caller, which could cause subtle bugs if the caller reusesparams. Consider using a local variable instead.♻️ Suggested fix
if params != nil { + var normalizedLinkDirName *string if params.LinkDirName != nil { linkDirValue, err := normalizeLinkDirName(params.LinkDirName) if err != nil { return nil, err } - params.LinkDirName = &linkDirValue + normalizedLinkDirName = &linkDirValue } if params.TLSSkipVerify != nil { ... - if params.LinkDirName != nil { + if normalizedLinkDirName != nil { query += ", link_dir_name = ?" - args = append(args, *params.LinkDirName) + args = append(args, *normalizedLinkDirName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/models/instance.go` around lines 748 - 754, The code mutates the caller's params by overwriting params.LinkDirName with the normalized value; instead, call normalizeLinkDirName(params.LinkDirName) into a local variable and use that local normalized value for subsequent logic without assigning it back to params.LinkDirName so the original input pointer is not mutated (refer to normalizeLinkDirName and the params.LinkDirName usage in this block).internal/api/handlers/instances.go (1)
801-808:normalizeOptionalStringalways returns a pointer, even for empty strings.This function trims whitespace but returns
&trimmedeven whentrimmedis empty. This means an empty/whitespace input will pass""to the store rather thannil. The store'snormalizeLinkDirNamehandles this correctly (treating""as no override), so this works, but the semantic distinction between "user explicitly cleared the field" vs "user didn't provide the field" is lost.If preserving
nilfor "not provided" is intended, consider:♻️ Alternative that preserves nil for empty
func normalizeOptionalString(value *string) *string { if value == nil { return nil } trimmed := strings.TrimSpace(*value) + if trimmed == "" { + return nil + } return &trimmed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/instances.go` around lines 801 - 808, The function normalizeOptionalString currently trims input but always returns a non-nil pointer, losing the distinction between "not provided" and "explicitly cleared"; update normalizeOptionalString to return nil when the trimmed result is empty (i.e., if value==nil -> nil; else trim and if trimmed=="" -> nil; otherwise return a pointer to the trimmed string), ensuring you allocate a stable string variable for the pointer; this preserves the semantic difference (and keeps normalizeLinkDirName's behavior unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/api/handlers/instances.go`:
- Around line 801-808: The function normalizeOptionalString currently trims
input but always returns a non-nil pointer, losing the distinction between "not
provided" and "explicitly cleared"; update normalizeOptionalString to return nil
when the trimmed result is empty (i.e., if value==nil -> nil; else trim and if
trimmed=="" -> nil; otherwise return a pointer to the trimmed string), ensuring
you allocate a stable string variable for the pointer; this preserves the
semantic difference (and keeps normalizeLinkDirName's behavior unchanged).
In `@internal/models/instance.go`:
- Around line 748-754: The code mutates the caller's params by overwriting
params.LinkDirName with the normalized value; instead, call
normalizeLinkDirName(params.LinkDirName) into a local variable and use that
local normalized value for subsequent logic without assigning it back to
params.LinkDirName so the original input pointer is not mutated (refer to
normalizeLinkDirName and the params.LinkDirName usage in this block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4bf21dac-1c3f-4fbd-bb28-1839623b2411
📒 Files selected for processing (4)
internal/api/handlers/instances.gointernal/models/instance.gointernal/models/instance_reannounce.gointernal/models/instance_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/models/instance_test.go
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
1 participant
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.
Allow per-instance overrides for by-instance hardlink/reflink directory names, and reuse the same link directory resolution logic for cross-seed + dirscan. Closes #1085.
Summary by CodeRabbit
New Features
Documentation