refactor(installer): preserve local templates during updates - #7584
refactor(installer): preserve local templates during updates#7584dwisiswant0 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe PR adds atomic config and template writes, symlink-safe path resolution, ownership recovery, platform-specific filesystem helpers, metadata and checksum regeneration, and expanded filesystem and path-safety tests. ChangesTemplate integrity
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TemplateManager
participant OwnershipRecovery
participant TemplateWriter
participant MetadataRegenerator
participant Filesystem
TemplateManager->>OwnershipRecovery: recover or bootstrap ownership
TemplateManager->>TemplateWriter: write template outputs
TemplateWriter->>Filesystem: atomically replace files and sync directories
TemplateManager->>MetadataRegenerator: finalize ownership and metadata
MetadataRegenerator->>Filesystem: write index and checksum maps
MetadataRegenerator-->>TemplateManager: finalized checksums and version
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Track files installed by each official release and remove only retired templates whose contents still match the recorded digest. Bootstrap ownership for existing installations so upgrades keep unknown and locally modified templates intact. Write templates, ownership state, and config thru synced temporary files. Recover interrupted cleanup before another update and record the new version only after metadata finalization succeeds. Signed-off-by: Dwi Siswanto <git@dw1.io>
ab55db7 to
c7b025e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
pkg/catalog/config/nucleiconfig.go (1)
409-419: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
WriteTemplatesIndexstill writes non-atomically.The config write now goes through the temp-file + replace path, but the index (Line 419) remains a direct
os.WriteFile, so an interrupted update can still leave a truncated/partial index next to a fully-written config. Consider routing it throughwriteTemplatesConfigFile-style atomic replacement for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/catalog/config/nucleiconfig.go` around lines 409 - 419, Update Config.WriteTemplatesIndex to write the generated index through the existing writeTemplatesConfigFile-style temporary-file-and-replace mechanism instead of calling os.WriteFile directly, preserving the current index contents and file path while ensuring interrupted updates cannot leave partial data.pkg/catalog/config/nucleiconfig_unix_test.go (1)
12-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the build tag to
unix!windowsstill includes targets where these syscalls aren’t available, so this test file can be selected on unsupported platforms.//go:build unixmatches the intended scope better.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/catalog/config/nucleiconfig_unix_test.go` around lines 12 - 27, Change the build constraint for the tests containing TestWriteTemplatesConfigFileHonorsUmaskForNewFile from !windows to unix so the file is selected only on platforms supporting syscall.Umask; keep the test implementation unchanged.pkg/installer/template_ownership.go (1)
525-528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCommented-out warnings leave every preservation decision silent.
Lines 526, 540, 623, 634, and 652 are the only places where the installer would tell a user that a retired template was preserved (symlink, non-regular, locally modified) and that ownership was relinquished. All five are commented out, so the reconciliation outcome is now unobservable. Either restore them (
gologger.Verbose()ifWarningis too loud for thousands of files) or delete the dead comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/installer/template_ownership.go` around lines 525 - 528, The retired-template preservation decisions in the reconciliation logic are currently silent because the logging calls at the symlink, non-regular, locally modified, and ownership-relinquished branches are commented out. Restore these messages using gologger.Verbose() (or Warning() if appropriate) at the five referenced branches, or remove the dead comments if logging is intentionally not retained.pkg/installer/template_ownership_sync_windows_test.go (1)
37-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the cross-platform tests out of
pkg/installer/template_ownership_sync_windows_test.go.TestSyncTemplateOwnershipDirectoryandTestRenameTemplateRestoreNoReplaceUsesOpenedRootshould live in a non-Windows test file so Linux/macOS exercise the opened-root path too; keep onlyTestSyncTemplateOwnershipFileRestoresModeAfterOpenFailurehere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/installer/template_ownership_sync_windows_test.go` around lines 37 - 62, Move TestSyncTemplateOwnershipDirectory and TestRenameTemplateRestoreNoReplaceUsesOpenedRoot into a non-Windows test file so they run on Linux and macOS as well as Windows. Remove those tests from template_ownership_sync_windows_test.go, leaving only TestSyncTemplateOwnershipFileRestoresModeAfterOpenFailure there and preserving all test logic unchanged.pkg/installer/template.go (1)
269-299: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStream the prior-release archive instead of materializing it all in memory.
fetchTemplateReleaseArchivereads the whole zipball into abytes.Bufferbefore returning it, so larger releases can spike RSS; a plainhttp.Clientplus a streaming or size-bounded reader would keep this path lighter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/installer/template.go` around lines 269 - 299, Update fetchTemplateReleaseArchive to return a streaming or size-bounded reader backed by the HTTP response instead of copying the entire archive into bytes.Buffer and bytes.Reader. Preserve the existing request, status validation, and error context, and ensure the response body is properly closed when the returned reader no longer needs it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/installer/template_ownership_publish_linux.go`:
- Around line 12-26: Classify unsupported flag errors in
renameTemplateRestoreNoReplace in
pkg/installer/template_ownership_publish_linux.go lines 12-26 by wrapping
ENOSYS, EINVAL, and EOPNOTSUPP from unix.Renameat2 as errors.ErrUnsupported;
apply the equivalent ENOTSUP classification in
pkg/installer/template_ownership_publish_darwin.go lines 12-26 for
unix.RenameatxNp, allowing callers to select the existing
copyQuarantinedTemplateWithLink fallback while preserving no-overwrite behavior.
In `@pkg/installer/template_ownership_publish_other.go`:
- Around line 10-12: Update renameTemplateRestoreNoReplace so unsupported
platforms return the non-fatal fallback signal expected by the generic restore
flow instead of propagating errors.ErrUnsupported. Ensure template
restore/update continues through the move/copy fallback on non-linux, darwin,
and windows targets while preserving exclusive rename behavior where supported.
In `@pkg/installer/template_ownership.go`:
- Around line 202-205: Update the recovery error handling around
loadTemplateOwnership in updateIfOutdatedLocked to include the pending
quarantine or restore-state artifact paths alongside the manifest-load failure.
Preserve error wrapping while naming the paths needed for users to identify and
remove the blocking recovery state.
In `@pkg/installer/template_test.go`:
- Around line 264-278: Initialize a logger before configuring the test config in
TestGetTemplateOutputLocationRoutesIgnoreFile, following the existing
gologger.DefaultLogger setup used by
TestFinalizeTemplateReleaseCommitsVersionLast. Ensure config.Config has a
non-nil Logger before calling SetConfigDir so its error paths remain safe.
---
Nitpick comments:
In `@pkg/catalog/config/nucleiconfig_unix_test.go`:
- Around line 12-27: Change the build constraint for the tests containing
TestWriteTemplatesConfigFileHonorsUmaskForNewFile from !windows to unix so the
file is selected only on platforms supporting syscall.Umask; keep the test
implementation unchanged.
In `@pkg/catalog/config/nucleiconfig.go`:
- Around line 409-419: Update Config.WriteTemplatesIndex to write the generated
index through the existing writeTemplatesConfigFile-style
temporary-file-and-replace mechanism instead of calling os.WriteFile directly,
preserving the current index contents and file path while ensuring interrupted
updates cannot leave partial data.
In `@pkg/installer/template_ownership_sync_windows_test.go`:
- Around line 37-62: Move TestSyncTemplateOwnershipDirectory and
TestRenameTemplateRestoreNoReplaceUsesOpenedRoot into a non-Windows test file so
they run on Linux and macOS as well as Windows. Remove those tests from
template_ownership_sync_windows_test.go, leaving only
TestSyncTemplateOwnershipFileRestoresModeAfterOpenFailure there and preserving
all test logic unchanged.
In `@pkg/installer/template_ownership.go`:
- Around line 525-528: The retired-template preservation decisions in the
reconciliation logic are currently silent because the logging calls at the
symlink, non-regular, locally modified, and ownership-relinquished branches are
commented out. Restore these messages using gologger.Verbose() (or Warning() if
appropriate) at the five referenced branches, or remove the dead comments if
logging is intentionally not retained.
In `@pkg/installer/template.go`:
- Around line 269-299: Update fetchTemplateReleaseArchive to return a streaming
or size-bounded reader backed by the HTTP response instead of copying the entire
archive into bytes.Buffer and bytes.Reader. Preserve the existing request,
status validation, and error context, and ensure the response body is properly
closed when the returned reader no longer needs it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6d25dde-98b7-4381-90b5-2ca7477a1e4f
📒 Files selected for processing (22)
go.modpkg/catalog/config/nucleiconfig.gopkg/catalog/config/nucleiconfig_replace.gopkg/catalog/config/nucleiconfig_replace_windows.gopkg/catalog/config/nucleiconfig_replace_windows_test.gopkg/catalog/config/nucleiconfig_test.gopkg/catalog/config/nucleiconfig_unix_test.gopkg/installer/template.gopkg/installer/template_output_unix_test.gopkg/installer/template_ownership.gopkg/installer/template_ownership_publish_darwin.gopkg/installer/template_ownership_publish_linux.gopkg/installer/template_ownership_publish_linux_test.gopkg/installer/template_ownership_publish_other.gopkg/installer/template_ownership_publish_windows.gopkg/installer/template_ownership_state_unix_test.gopkg/installer/template_ownership_sync.gopkg/installer/template_ownership_sync_windows.gopkg/installer/template_ownership_sync_windows_test.gopkg/installer/template_ownership_test.gopkg/installer/template_test.gopkg/installer/zipslip_unix_test.go
| previous, err := loadTemplateOwnership(dir) | ||
| if err != nil { | ||
| return fmt.Errorf("load template ownership for recovery: %w", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Recovery dead-ends without naming the artifacts that block it.
If a quarantine or restore-state file survives but the manifest is missing or corrupt, this returns an error on every subsequent run (via updateIfOutdatedLocked → line 108 of pkg/installer/template.go), and the message names only the manifest path. Quarantine names are SHA-256 digests, so a user cannot map them back to a template or know what to delete. Include the pending paths in the error so the state is actionable.
🛠️ Proposed change
previous, err := loadTemplateOwnership(dir)
if err != nil {
- return fmt.Errorf("load template ownership for recovery: %w", err)
+ return fmt.Errorf("load template ownership for recovery (pending quarantines %v, restore states %v): %w", quarantineEntries, restoreEntries, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| previous, err := loadTemplateOwnership(dir) | |
| if err != nil { | |
| return fmt.Errorf("load template ownership for recovery: %w", err) | |
| } | |
| previous, err := loadTemplateOwnership(dir) | |
| if err != nil { | |
| return fmt.Errorf("load template ownership for recovery (pending quarantines %v, restore states %v): %w", quarantineEntries, restoreEntries, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/installer/template_ownership.go` around lines 202 - 205, Update the
recovery error handling around loadTemplateOwnership in updateIfOutdatedLocked
to include the pending quarantine or restore-state artifact paths alongside the
manifest-load failure. Preserve error wrapping while naming the paths needed for
users to identify and remove the blocking recovery state.
| func TestGetTemplateOutputLocationRoutesIgnoreFile(t *testing.T) { | ||
| previousConfig := config.DefaultConfig | ||
| cfg := &config.Config{} | ||
| cfg.SetConfigDir(t.TempDir()) | ||
| config.DefaultConfig = cfg | ||
| t.Cleanup(func() { config.DefaultConfig = previousConfig }) | ||
|
|
||
| ignoreEntry := filepath.Join(t.TempDir(), config.NucleiIgnoreFileName) | ||
| require.NoError(t, os.WriteFile(ignoreEntry, nil, 0o600)) | ||
| info, err := os.Stat(ignoreEntry) | ||
| require.NoError(t, err) | ||
| rootDir, writePath := (&TemplateManager{}).getTemplateOutputLocation(t.TempDir(), "nuclei-templates/"+config.NucleiIgnoreFileName, info) | ||
| require.Equal(t, cfg.GetConfigDir(), rootDir) | ||
| require.Equal(t, cfg.GetIgnoreFilePath(), writePath) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
config.Config built without a Logger before SetConfigDir.
SetConfigDir calls ReadTemplatesConfig/WriteTemplatesConfig and copyIgnoreFile, all of which log via c.Logger on failure (Logger.Fatal() / Logger.Error()). With Logger nil, any of those failure paths panics instead of failing the assertion — copyIgnoreFile in particular logs when the fallback ignore file can't be copied, which is environment-dependent. TestFinalizeTemplateReleaseCommitsVersionLast already sets gologger.DefaultLogger; do the same here.
🛡️ Proposed fix
- cfg := &config.Config{}
+ cfg := &config.Config{Logger: gologger.DefaultLogger}
cfg.SetConfigDir(t.TempDir())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestGetTemplateOutputLocationRoutesIgnoreFile(t *testing.T) { | |
| previousConfig := config.DefaultConfig | |
| cfg := &config.Config{} | |
| cfg.SetConfigDir(t.TempDir()) | |
| config.DefaultConfig = cfg | |
| t.Cleanup(func() { config.DefaultConfig = previousConfig }) | |
| ignoreEntry := filepath.Join(t.TempDir(), config.NucleiIgnoreFileName) | |
| require.NoError(t, os.WriteFile(ignoreEntry, nil, 0o600)) | |
| info, err := os.Stat(ignoreEntry) | |
| require.NoError(t, err) | |
| rootDir, writePath := (&TemplateManager{}).getTemplateOutputLocation(t.TempDir(), "nuclei-templates/"+config.NucleiIgnoreFileName, info) | |
| require.Equal(t, cfg.GetConfigDir(), rootDir) | |
| require.Equal(t, cfg.GetIgnoreFilePath(), writePath) | |
| } | |
| func TestGetTemplateOutputLocationRoutesIgnoreFile(t *testing.T) { | |
| previousConfig := config.DefaultConfig | |
| cfg := &config.Config{Logger: gologger.DefaultLogger} | |
| cfg.SetConfigDir(t.TempDir()) | |
| config.DefaultConfig = cfg | |
| t.Cleanup(func() { config.DefaultConfig = previousConfig }) | |
| ignoreEntry := filepath.Join(t.TempDir(), config.NucleiIgnoreFileName) | |
| require.NoError(t, os.WriteFile(ignoreEntry, nil, 0o600)) | |
| info, err := os.Stat(ignoreEntry) | |
| require.NoError(t, err) | |
| rootDir, writePath := (&TemplateManager{}).getTemplateOutputLocation(t.TempDir(), "nuclei-templates/"+config.NucleiIgnoreFileName, info) | |
| require.Equal(t, cfg.GetConfigDir(), rootDir) | |
| require.Equal(t, cfg.GetIgnoreFilePath(), writePath) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/installer/template_test.go` around lines 264 - 278, Initialize a logger
before configuring the test config in
TestGetTemplateOutputLocationRoutesIgnoreFile, following the existing
gologger.DefaultLogger setup used by
TestFinalizeTemplateReleaseCommitsVersionLast. Ensure config.Config has a
non-nil Logger before calling SetConfigDir so its error paths remain safe.
2 participants
Proposed changes
Track files installed by each official release and
remove only retired templates whose contents still
match the recorded digest. Bootstrap ownership for
existing installations so upgrades keep unknown
and locally modified templates intact.
Write templates, ownership state, and config thru
synced temporary files. Recover interrupted
cleanup before another update and record the new
version only after metadata finalization succeeds.
Proof
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests