Skip to content
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions scripts/plugins/Modules/PluginHelpers.psm1
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💡 Documentation — Low: Generate-Plugins.ps1 comments reference obsolete text-stub behavior

The comment at line 296 in Generate-Plugins.ps1 reads "Fix git index modes for text stubs on non-symlink systems" and the conditional block invokes Repair-PluginSymlinkIndex. Both reference the old text-stub pattern that this PR eliminates. Should be updated or removed consistent with the resolution for Repair-PluginSymlinkIndex.

Original file line number Diff line number Diff line change
Expand Up @@ -460,23 +460,23 @@ function Test-SymlinkCapability {
function New-PluginLink {
<#
.SYNOPSIS
Links a source path into a plugin destination via symlink or text stub.
Links a source path into a plugin destination via symlink or inlined content.

.DESCRIPTION
When SymlinkCapable is set, creates a relative symbolic link from
DestinationPath to SourcePath. Otherwise writes a text stub file
containing the relative path, matching the format git produces when
core.symlinks is false. Text stubs keep git status clean on Windows
without Developer Mode or elevated privileges.
DestinationPath to SourcePath. Otherwise copies the actual file content
to the destination. This ensures agent files have valid YAML frontmatter
in the installed plugin rather than path references that Copilot CLI
cannot parse.

.PARAMETER SourcePath
Absolute path to the real file or directory.

.PARAMETER DestinationPath
Absolute path where the link or text stub will be created.
Absolute path where the link or copied content will be created.

.PARAMETER SymlinkCapable
When set, create a symbolic link; otherwise write a text stub.
When set, create a symbolic link; otherwise copy the file content.
#>
[CmdletBinding()]
param(
Expand All @@ -501,7 +501,9 @@ function New-PluginLink {
New-Item -ItemType SymbolicLink -Path $DestinationPath -Value $relativePath -Force | Out-Null
}
else {
[System.IO.File]::WriteAllText($DestinationPath, $relativePath)
# Copy the actual file content instead of writing a path reference
# This ensures agent files have valid YAML frontmatter in installed plugins
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💡 Code Quality — Low: Dead code — $relativePath unused in non-symlink branch

$relativePath is computed unconditionally (line 499) but only the symlink branch uses it now. Consider moving it inside the if ($SymlinkCapable) block to avoid dead code.

Copy-Item -Path $SourcePath -Destination $DestinationPath -Force
}
Comment on lines +505 to 507
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ Functional Correctness — High: Copy-Item on directory sources lacks -Recurse

New-PluginLink is also called for shared directories (docs/templates, scripts/lib) at line 657 in Write-PluginDirectory. Without -Recurse, Copy-Item on a directory source copies only the top-level directory, not its contents. The old WriteAllText created a single file acting as a text-based symlink — Copy-Item on a directory is fundamentally different.

Suggestion: Add directory detection:

if (Test-Path -Path $SourcePath -PathType Container) {
    Copy-Item -Path $SourcePath -Destination $DestinationPath -Recurse -Force
}
else {
    Copy-Item -Path $SourcePath -Destination $DestinationPath -Force
}

Or keep directory sources on the symlink-only path and limit Copy-Item to file sources.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔍 Functional Correctness — Medium: Existing test will fail

The test 'Writes text stub when SymlinkCapable is false' in scripts/tests/plugins/PluginHelpers.Tests.ps1 (line 233) asserts the destination file content equals the computed relative path. After this change, the destination contains the source file's actual content, not the relative path. This test will break.

Suggestion: Update the test to validate the new copy behavior:

It 'Copies file content when SymlinkCapable is false' {
    $src = Join-Path $script:linkRoot 'src-stub.txt'
    Set-Content -Path $src -Value 'content' -NoNewline
    $dest = Join-Path $script:linkRoot 'dest-stub.txt'

    New-PluginLink -SourcePath $src -DestinationPath $dest

    Test-Path $dest | Should -BeTrue
    $destContent = [System.IO.File]::ReadAllText($dest)
    $destContent | Should -Be 'content'
}

}

Comment on lines +505 to 509
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ Functional Correctness — High: Repair-PluginSymlinkIndex is now a silent no-op

Repair-PluginSymlinkIndex detects text stubs by checking for files < 500 bytes whose content matches ^\.\./ (a relative path starting with ../). With New-PluginLink now using Copy-Item, the non-symlink fallback produces full file content that will never match this heuristic — making the entire function a silent no-op.

The caller in Generate-Plugins.ps1 (line 299) still invokes it and logs "entries fixed", but nothing will ever be fixed.

Since copied files should be committed as regular files (not mode 120000 symlinks), this function likely needs to be removed entirely, along with its call site and the comment-based help that still references "text stubs containing relative paths." If git index mode repair is still needed for some other reason, the detection logic needs reworking.

Expand Down