Skip to content

feature: Prune stale files on upgrade via manifest.json (#443) - #448

Merged
TheWitness merged 3 commits into
developfrom
feature/manifest-prune
Nov 13, 2026
Merged

TheWitness merged 3 commits into
developfrom
feature/manifest-prune

Conversation

@TheWitness

Copy link
Copy Markdown
Member

Description

Adds upgrade-time "housecleaning" to the Syslog plugin so stale files from older
layouts are removed automatically. A new root manifest.json describes the
plugin tree in three lists — tombstones (paths older versions shipped that have
since moved or been deleted), expected (the files/directories that ship today),
and whitelist (user-data paths that must never be touched) — and a new
syslog_prune_files() runs during plugin_syslog_upgrade() on a version change.

On upgrade it:

  • Deletes every tombstoned path plus the dev-only tests/ tree and phpunit.xml.
  • Never touches whitelisted user data (config.php, config_local.php) or VCS
    metadata (.git*) and dotfiles (.md*).
  • Refuses any tombstone that resolves outside the plugin directory (a tampered
    manifest.json), including through .. segments or a symlinked directory, and
    logs the refusal.
  • Logs — without removing — any top-level entry the manifest does not account for,
    and logs anything it cannot delete (e.g. read-only).

The initial tombstone list captures the top-level files/directories removed across
the plugin's history that are gone from the current tree, notably the old
top-level database.php and functions.php that now live under includes/ — the
exact files that redeclare functions and fatal the plugin on load after a
4.2 → 4.11 upgrade.

A CI step and tests/bin/validate-manifest.php keep the manifest's expected
list from silently drifting from the real top-level tree.

This mirrors the approach already used by FlowView (flowview_prune_files()) and
Weathermap.

Related Issue

Closes #443

Motivation and Context

Upgrading Syslog from 4.2 to 4.11 left behind stale top-level files that declared
functions now defined in the reorganized includes/ modules. The duplicate
function declarations made Syslog unload until the offending file was removed by
hand. This change cleans those files up automatically on upgrade and gives the
plugin a maintainable, CI-guarded manifest to track future moves/removals.

How Has This Been Tested?

  • php tests/bin/validate-manifest.php → manifest.json 'expected' matches the plugin tree. (exit 0)
  • php -l on setup.php, tests/bin/validate-manifest.php and
    tests/Unit/PruneFilesTest.php → no syntax errors.
  • New tests/Unit/PruneFilesTest.php (6 cases) covering tombstone/tests/
    removal, whitelist/.git/dotfile protection, missing/malformed manifest
    no-ops, traversal refusal, symlink-escape refusal, and unremovable-path
    logging — all pass.
  • SetupModuleBoundaryTest updated for the two new facade functions — passes.
  • Full Pest suite run against an installed Cacti checkout
    (include/vendor/bin/pest --configuration=plugins/syslog/phpunit.xml):
    650 passed (2705 assertions), 1 pre-existing unrelated warning
    (ProcessHelperCompatTest /proc read).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation repository accordingly.

Add a root manifest.json (tombstones/expected/whitelist) and a
syslog_prune_files() housecleaning pass that plugin_syslog_upgrade()
runs on a version change. It deletes files and directories that older
layouts shipped but have since moved or been removed (for example the
old top-level database.php and functions.php that now live under
includes/), so a stale module can no longer redeclare a function and
fatal the plugin on load after an upgrade.

User-data paths (config.php, config_local.php) and VCS metadata are
never touched, the dev-only tests/ tree and phpunit.xml are removed,
and any tombstone that resolves outside the plugin directory (a
tampered manifest.json) is refused and logged. A CI check,
tests/bin/validate-manifest.php, keeps the manifest expected-file
list from drifting from the real top-level tree.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 13, 2026 14:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Upgrade paths can record the new version without pruning stale modules, while tombstone deletion can remove protected configuration through symlinked directories and destroy nested repository metadata.

3 open issues
What changed in this PR

Adds manifest-driven upgrade cleanup to remove obsolete Syslog plugin files that can cause duplicate-function failures, addressing #443.

Changes:

  • Defines obsolete, expected, and protected paths in a root manifest.
  • Adds upgrade-time pruning, path-safety checks, and logging.
  • Adds focused tests and CI validation to detect manifest drift.
File Description
tests/​Unit/​SetupModuleBoundaryTest.php Allows the two new cleanup helpers in the setup facade.
tests/​Unit/​PruneFilesTest.php Tests pruning, protected paths, invalid manifests, and failure logging.
tests/​bin/​validate-manifest.php Checks expected manifest entries against the top-level plugin tree.
setup.php Adds the upgrade cleanup call and manifest-driven deletion helpers.
manifest.json Lists obsolete files, expected entries, and protected configuration files.
CHANGELOG.md Documents the cleanup feature and manifest validation.
.github/​workflows/​plugin-ci-workflow.yml Runs manifest validation in CI.

Review effort: Balanced

Comment thread setup.php Resolved
Comment thread setup.php Resolved
Comment thread setup.php Resolved
#443)

Respond to the three review findings on the upgrade housecleaning:

- Run pruning from the shared, version-aware syslog_check_upgrade() path
  instead of only plugin_syslog_upgrade(), so the re-enable path and
  cli/syslog_schema.php clean up too. It is gated on a genuinely older
  installed version so a fresh install or same-version repair never
  removes the dev-only tests/ tree, and it runs before the stored
  version is bumped.
- Reject any tombstone reached through a symlinked parent directory, so
  a tombstone such as 'alias/config.php' (alias -> plugin root) can no
  longer alias and delete a protected, whitelisted file. A tombstone
  that is itself a symlink is still unlinked without following it.
- Preserve .git*/.md* descendants nested inside a tombstoned directory
  during recursive deletion, retaining their containing directories
  without reporting the intentional retention as a failure.

Adds regression tests for the shared-path gating, the symlinked-parent
alias attack and nested .git preservation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@TheWitness

Copy link
Copy Markdown
Member Author

Thanks for the review — all three findings are addressed in cb09b97, and the threads are resolved:

  1. Run module pruning in every version-aware upgrade path — pruning moved into the shared syslog_check_upgrade() (used by plugin_syslog_upgrade(), the re-enable path, and cli/syslog_schema.php), before the version bump. It's gated on a genuinely older installed version so a fresh install or same-version repair never removes the dev-only tests/ tree.
  2. Reject symlinked parents before tombstone deletion — the containment check now requires every parent to realpath() to its own lexical location under the plugin root, so alias/config.php (alias → plugin root) is refused instead of deleting the whitelisted config. A tombstone that is itself a symlink is still unlinked without following it.
  3. Preserve protected metadata during recursive deletion — syslog_rmtree() skips .git*/.md* descendants and only removes a directory once it's genuinely empty, retaining nested VCS metadata (and its parent) without counting the retention as a failure.

Added regression tests for all three (shared-path gating, symlinked-parent alias attack, nested .git preservation).

Local validation

  • Full Pest suite against an installed Cacti checkout: 653 passed (2714 assertions), 1 pre-existing unrelated warning (ProcessHelperCompatTest /proc read).
  • php tests/bin/validate-manifest.php → manifest.json 'expected' matches the plugin tree. (exit 0)
  • php -l setup.php → no syntax errors.

@sonarqubecloud

Copy link
Copy Markdown

@TheWitness
TheWitness merged commit a9f254f into develop Nov 13, 2026
11 checks passed
@TheWitness
TheWitness deleted the feature/manifest-prune branch November 13, 2026 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Syslog checks and cleans up stale files after upgrade

3 participants