Repository navigation
fix(sources): resolve symlinks when computing repo-relative lock paths - #365
Conversation
go-git reports the project worktree root with symlinks resolved, while paths derived from the project config keep the invocation directory. When the project is reached through a symlink, the repo-relative lock file path became "../../..", the committed lock was not found at HEAD, and 'component render' silently dropped the synthetic changelog history and regressed the release number. Add git.RepoRelPath, which resolves symlinks on both sides before relativizing and rejects paths that escape the repository root, and use it for the lock file and upstream-commit TOML paths in synthetic history. Fixes microsoft#188 Signed-off-by: IzanVil <soyoquienmas@outlook.com>
'component changed' failed with "escapes repository root" and 'component history' silently dropped TOML and lock metrics when the project was reached through a symlinked path, for the same reason as microsoft#188. Replace the local repoRelPath helper with git.RepoRelPath. Signed-off-by: IzanVil <soyoquienmas@outlook.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address the dangling-symlink escape and inconsistent TOML sharing-key handling.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Fixes symlink-related repository path resolution for synthetic history and component metrics.
Changes:
- Adds shared symlink-aware
git.RepoRelPathwith escape detection. - Applies it to synthetic history and component commands.
- Adds filesystem-backed regression tests.
Review findings:
- Critical (4 votes): Dangling symlinks may still be treated as inside the repository.
- Moderate (2 votes): Canonical TOML caching is inconsistent with
tomlSharing, breaking--shared=omitfor symlink aliases.
| File | Description |
|---|---|
internal/utils/git/gogit.go |
Adds repository-relative path resolution. |
internal/utils/git/gogit_test.go |
Tests symlink and escape cases. |
internal/app/azldev/core/sources/synthistory.go |
Uses the helper for lock paths. |
internal/app/azldev/core/sources/synthistory_upstreamcommit.go |
Uses the helper for TOML paths. |
internal/app/azldev/core/sources/synthistory_internal_test.go |
Tests synthetic history through symlinked paths. |
internal/app/azldev/cmds/component/history_gitmetrics.go |
Uses shared path resolution for metrics. |
internal/app/azldev/cmds/component/changed.go |
Replaces the local path helper. |
internal/app/azldev/cmds/component/changed_upstreamcommit.go |
Uses shared path resolution. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A symlink whose target does not exist made EvalSymlinks fail, and the fallback appended the link name to its resolved parent without following it. An in-repo link pointing outside the repository was then reported as inside it. Read the link target and keep resolving it instead. Signed-off-by: IzanVil <soyoquienmas@outlook.com>
|
Friendly ping: this one has been open for a few weeks with no human review yet. Copilot's two findings were addressed (the dangling-symlink case is fixed in d3b16cf, with tests), and it still merges cleanly with current |
| } | ||
|
|
||
| lockRelDir, err := repoRelPath(repoRoot, env.Config().Project.LockDir) | ||
| lockRelDir, err := git.RepoRelPath(repoRoot, env.Config().Project.LockDir) |
There was a problem hiding this comment.
Please remove the unused strings import.
| }) | ||
| } | ||
|
|
||
| func RepoRelPath(repoRoot, absPath string) (string, error) { |
Nan Liu (liunan-ms)
left a comment
Nan Liu (liunan-ms)
left a comment
There was a problem hiding this comment.
Thanks for contributing! I rebased this PR to main and ran the CI checks. Would you mind taking another look and fixing them?
| target = filepath.Join(resolvedParent, target) | ||
| } | ||
|
|
||
| return resolveSymlinks(target) |
There was a problem hiding this comment.
Could we add a cycle guard here? A link pointing to missing/../link can hit the missing-file fallback, then filepath.Join cleans the target back to the same link. That keeps calling resolveSymlinks until it crashes with a stack overflow. A visited-path check or traversal limit, plus a test for this case, would help.
Signed-off-by: IzanVil <soyoquienmas@outlook.com>
…lic links Signed-off-by: IzanVil <soyoquienmas@outlook.com>
|
Done Nan Liu (@liunan-ms) |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
7 open issues
changed.gonow referencesgit.RepoRelPath, but the import block shown here does not include… · Newchanged.gonow referencesgit.RepoRelPath, but the import block shown here does not include… · NewabsPathimplies callers must pass an absolute path, but the function doesn’t… · Newos.Symlinkto succeed. On some environments (notably Windows without… · New
Review effort: Lite
| func TestRepoRelPath(t *testing.T) { | ||
| tempDir := t.TempDir() | ||
|
|
||
| realRepoDir := filepath.Join(tempDir, "real", "repo") | ||
| linkedRepoDir := filepath.Join(tempDir, "link", "repo") | ||
|
|
||
| require.NoError(t, os.MkdirAll(filepath.Join(realRepoDir, "locks"), fileperms.PrivateDir)) | ||
| require.NoError(t, os.WriteFile(filepath.Join(realRepoDir, "locks", "curl.lock"), nil, fileperms.PrivateFile)) | ||
| require.NoError(t, os.Symlink(filepath.Join(tempDir, "real"), filepath.Join(tempDir, "link"))) |
Fixes #188
Problem
When the project is reached through a symlinked directory,
azldev component rendersilently drops the synthetic changelog history and regressesrelease_number.go-git reports the worktree root with symlinks resolved (go-billy's
osfscallsfilepath.EvalSymlinkson its base directory), while paths derived from the project config (LockDir,SourceConfigFile) keep the invocation path.filepath.Relbetween the two produced../../../home/user/repos/azurelinux/locks/rust.lock,readLockFileAtHEADtreated the lock as not committed, and synthetic history was skipped with only a debug log.Changes
git.RepoRelPath, which resolves symlinks in both the repository root and the target path (tolerating trailing elements that do not exist yet) before relativizing, and rejects results that escape the repository root.buildSyntheticCommitsand the upstream-commit TOML path inbuildUpstreamCommitSyntheticCommits.repoRelPathincmds/componentwith the shared helper.component changedfailed with "escapes repository root" andcomponent historysilently dropped TOML/lock metrics through a symlinked path for the same reason.Behavior change: a lock path that resolves outside the project repository is now an error in synthetic history instead of a silent skip, as suggested under "Additional hardening" in the issue.
Testing
TestBuildSyntheticCommits_SymlinkedProjectPath: loads a real project config through a symlinked directory against an on-disk go-git repository, withgit logstubbed through the test command factory. It fails before this change (the lock is never found, no changes are returned) and passes after it.TestRepoRelPath/TestRepoRelPath_EscapesRoot: table tests for the helper (symlinked path, symlinked root, missing file under a symlink, escape via an in-repo symlink).t.TempDir()because symlink resolution cannot be replicated in memory.mage unitandmage check lintpass.