Added file permission tests.#175
Conversation
📝 WalkthroughWalkthroughAdds a developer guide (CLAUDE.md), configures Git to respect file permissions by running git config core.filemode true during ArtifactGitRepository initialization, and introduces a functional test verifying file permission preservation across artifact creation and transfer in force-push mode. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ArtifactCommand
participant ArtifactGitRepository as GitRepository
participant Git
User->>ArtifactCommand: Run artifact command (force-push)
ArtifactCommand->>GitRepository: Construct repository helper
activate GitRepository
GitRepository->>Git: config core.filemode true
Note right of GitRepository: Ensure Git respects file permissions
deactivate GitRepository
ArtifactCommand->>GitRepository: Prepare artifact from source
GitRepository->>Git: Commit files (preserving modes)
ArtifactCommand->>GitRepository: Push artifact to target branch
GitRepository->>Git: Force-push
User-->>ArtifactCommand: Validate result (e.g., tests check perms)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #175 +/- ##
=======================================
Coverage 95.17% 95.17%
=======================================
Files 5 5
Lines 394 394
=======================================
Hits 375 375
Misses 19 19 ☔ View full report in Codecov by Sentry. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CLAUDE.md(1 hunks)tests/Functional/ExecutableBitTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Functional/ExecutableBitTest.php (3)
tests/Functional/FunctionalTestBase.php (2)
FunctionalTestBase(15-220)assertArtifactCommandSuccess(109-120)tests/Traits/FixtureTrait.php (1)
fixtureCreateFile(48-66)tests/Traits/GitTrait.php (2)
gitCommitAll(241-245)gitCheckout(41-59)
🪛 markdownlint-cli2 (0.17.2)
CLAUDE.md
11-11: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
12-12: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
15-15: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
16-16: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
20-20: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
24-24: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
25-25: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
32-32: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
33-33: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
36-36: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
38-38: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
41-41: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
43-43: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
50-50: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
52-52: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
57-57: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
58-58: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
61-61: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
62-62: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
88-88: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
94-94: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
96-96: Files should end with a single newline character
(MD047, single-trailing-newline)
🪛 PHPMD (2.15.0)
tests/Functional/ExecutableBitTest.php
32-32: The variable $file_path is not named in camelCase. (undefined)
(CamelCaseVariableName)
45-45: The variable $expected_perm is not named in camelCase. (undefined)
(CamelCaseVariableName)
46-46: The variable $dst_file is not named in camelCase. (undefined)
(CamelCaseVariableName)
48-48: The variable $actual_perm is not named in camelCase. (undefined)
(CamelCaseVariableName)
🔇 Additional comments (1)
tests/Functional/ExecutableBitTest.php (1)
14-24: Good: targeted coverage with FORCE_PUSH and precise @Covers.Covers the right classes and forces the intended mode in setup.
| public function testFilePermissions(): void { | ||
| $permissions = [0644, 0755]; | ||
| $files = []; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Skip on Windows where chmod/fileperms are unreliable.
Prevents false negatives on Windows workers.
public function testFilePermissions(): void {
+ if (PHP_OS_FAMILY === 'Windows') {
+ $this->markTestSkipped('File permission bits are not reliable on Windows.');
+ }📝 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.
| public function testFilePermissions(): void { | |
| $permissions = [0644, 0755]; | |
| $files = []; | |
| public function testFilePermissions(): void { | |
| if (PHP_OS_FAMILY === 'Windows') { | |
| $this->markTestSkipped('File permission bits are not reliable on Windows.'); | |
| } | |
| $permissions = [0644, 0755]; | |
| $files = []; |
🤖 Prompt for AI Agents
In tests/Functional/ExecutableBitTest.php around lines 26 to 29, the test should
be skipped on Windows because chmod/fileperms behavior is unreliable there; add
a platform check at the start of testFilePermissions (for example using
PHP_OS_FAMILY === 'Windows' or a WIN prefix check) and call
$this->markTestSkipped('Skipping on Windows where chmod/fileperms are
unreliable.') if running on Windows so the test does not run and produce false
negatives.
| foreach ($permissions as $perm) { | ||
| $filename = sprintf('file_%o.txt', $perm); | ||
| $file_path = $this->fixtureCreateFile($this->src, $filename, sprintf("Content with %o permissions\n", $perm)); | ||
| chmod($file_path, $perm); | ||
| $files[$filename] = $perm; | ||
|
|
||
| $this->assertSame($perm, fileperms($file_path) & 0777); | ||
| } | ||
|
|
||
| $this->gitCommitAll($this->src, 'Added files with various permissions'); | ||
|
|
||
| $this->assertArtifactCommandSuccess(['--branch' => 'testbranch']); | ||
|
|
||
| $this->gitCheckout($this->dst, 'testbranch'); | ||
|
|
||
| foreach ($files as $filename => $expected_perm) { | ||
| $dst_file = $this->dst . DIRECTORY_SEPARATOR . $filename; | ||
| $this->assertFileExists($dst_file); | ||
| $actual_perm = fileperms($dst_file) & 0777; | ||
| $this->assertSame($expected_perm, $actual_perm, | ||
| sprintf('File %s should preserve %o permissions, got %o', $filename, $expected_perm, $actual_perm)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the test robust across different umasks: assert only the executable bit.
Git tracks only the executable bit; exact 0644/0755 bits after checkout depend on the runner’s umask (e.g., 002 → 0664/0775), which can make this test flaky. Assert owner exec bit instead and fix camelCase names while at it.
@@
- foreach ($permissions as $perm) {
- $filename = sprintf('file_%o.txt', $perm);
- $file_path = $this->fixtureCreateFile($this->src, $filename, sprintf("Content with %o permissions\n", $perm));
- chmod($file_path, $perm);
- $files[$filename] = $perm;
-
- $this->assertSame($perm, fileperms($file_path) & 0777);
- }
+ foreach ($permissions as $perm) {
+ $filename = sprintf('file_%o.txt', $perm);
+ $filePath = $this->fixtureCreateFile($this->src, $filename, sprintf("Content with %o permissions\n", $perm));
+ chmod($filePath, $perm);
+ // Store only the expectation for the executable bit (owner).
+ $files[$filename] = (bool) ($perm & 0100);
+
+ // Local pre-check can assert exact perms after chmod.
+ $this->assertSame($perm, fileperms($filePath) & 0777);
+ }
@@
- foreach ($files as $filename => $expected_perm) {
- $dst_file = $this->dst . DIRECTORY_SEPARATOR . $filename;
- $this->assertFileExists($dst_file);
- $actual_perm = fileperms($dst_file) & 0777;
- $this->assertSame($expected_perm, $actual_perm,
- sprintf('File %s should preserve %o permissions, got %o', $filename, $expected_perm, $actual_perm));
- }
+ foreach ($files as $filename => $expectedExec) {
+ $dstFile = $this->dst . DIRECTORY_SEPARATOR . $filename;
+ $this->assertFileExists($dstFile);
+ $actualPerm = fileperms($dstFile) & 0777;
+ $actualExec = (bool) ($actualPerm & 0100);
+ $this->assertSame(
+ $expectedExec,
+ $actualExec,
+ sprintf('File %s should preserve owner executable bit; got perms %o', $filename, $actualPerm)
+ );
+ }📝 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.
| foreach ($permissions as $perm) { | |
| $filename = sprintf('file_%o.txt', $perm); | |
| $file_path = $this->fixtureCreateFile($this->src, $filename, sprintf("Content with %o permissions\n", $perm)); | |
| chmod($file_path, $perm); | |
| $files[$filename] = $perm; | |
| $this->assertSame($perm, fileperms($file_path) & 0777); | |
| } | |
| $this->gitCommitAll($this->src, 'Added files with various permissions'); | |
| $this->assertArtifactCommandSuccess(['--branch' => 'testbranch']); | |
| $this->gitCheckout($this->dst, 'testbranch'); | |
| foreach ($files as $filename => $expected_perm) { | |
| $dst_file = $this->dst . DIRECTORY_SEPARATOR . $filename; | |
| $this->assertFileExists($dst_file); | |
| $actual_perm = fileperms($dst_file) & 0777; | |
| $this->assertSame($expected_perm, $actual_perm, | |
| sprintf('File %s should preserve %o permissions, got %o', $filename, $expected_perm, $actual_perm)); | |
| } | |
| foreach ($permissions as $perm) { | |
| $filename = sprintf('file_%o.txt', $perm); | |
| $filePath = $this->fixtureCreateFile($this->src, $filename, sprintf("Content with %o permissions\n", $perm)); | |
| chmod($filePath, $perm); | |
| // Store only the expectation for the executable bit (owner). | |
| $files[$filename] = (bool) ($perm & 0100); | |
| // Local pre-check can assert exact perms after chmod. | |
| $this->assertSame($perm, fileperms($filePath) & 0777); | |
| } | |
| $this->gitCommitAll($this->src, 'Added files with various permissions'); | |
| $this->assertArtifactCommandSuccess(['--branch' => 'testbranch']); | |
| $this->gitCheckout($this->dst, 'testbranch'); | |
| foreach ($files as $filename => $expectedExec) { | |
| $dstFile = $this->dst . DIRECTORY_SEPARATOR . $filename; | |
| $this->assertFileExists($dstFile); | |
| $actualPerm = fileperms($dstFile) & 0777; | |
| $actualExec = (bool) ($actualPerm & 0100); | |
| $this->assertSame( | |
| $expectedExec, | |
| $actualExec, | |
| sprintf('File %s should preserve owner executable bit; got perms %o', $filename, $actualPerm) | |
| ); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
32-32: The variable $file_path is not named in camelCase. (undefined)
(CamelCaseVariableName)
45-45: The variable $expected_perm is not named in camelCase. (undefined)
(CamelCaseVariableName)
46-46: The variable $dst_file is not named in camelCase. (undefined)
(CamelCaseVariableName)
48-48: The variable $actual_perm is not named in camelCase. (undefined)
(CamelCaseVariableName)
🤖 Prompt for AI Agents
In tests/Functional/ExecutableBitTest.php around lines 30 to 51, the test
currently compares full permission bits which is flaky due to differing umasks;
change it to assert only the owner-executable bit (mask 0100) by storing
$files[$filename] = $perm & 0100 when creating fixtures and later comparing
(fileperms($dstFile) & 0100) to the expected value; also rename snake_case
variables to camelCase (e.g., $file_path → $filePath, $dst_file → $dstFile,
$expected_perm → $expectedPerm, $actual_perm → $actualPerm) and update the
assertion message accordingly so it reports the owner-exec bit only.
There was a problem hiding this comment.
but I actually need a bulletproof way to preserve those bits
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
git preserve file permissions umask core.filemode
💡 Result:
Short answer
- Git only records the owner-executable bit (two modes: 100644 non-executable, 100755 executable). It does not track full rwx permissions or ownership. [1][3]
- core.fileMode tells Git whether to pay attention to executable-bit differences (true by default). Set it false to ignore mode-only changes, true to record/notice executable changes. [1][3]
- umask affects the permission bits that the OS gives newly checked-out files (and Git’s checkout behavior), so checkouts may lose non-tracked bits; Git will apply the stored exec/non-exec mode but the final mode is masked by your umask (and core.sharedRepository). [2][3]
Useful commands
- See current setting for this repo:
git config core.fileMode - Make Git ignore exec-bit changes (per-repo or global):
git config core.fileMode false
git config --global core.fileMode false - Make a file executable in the repo (so Git records it):
git update-index --chmod=+x path/to/file
git commit -m "make script executable" - Control repository checkout permissions (instead of umask):
git config core.sharedRepository 0644 - If you need to preserve full permissions/ownership you must use external tooling (hooks, tar, git-archive, or metadata plugins) because Git itself only stores the exec bit. [1][2][3]
References
[1] Stack Overflow — Git stores only exec bit; core.fileMode docs.
[2] Unix.StackExchange — checkout uses umask and core.sharedRepository.
[3] Baeldung (Linux) / other Git docs — explanation of 100644 vs 100755 and core.fileMode.
🏁 Script executed:
#!/bin/bash
# Look for permission-related code in the Git Artifact codebase
echo "=== Searching for permission-related code ==="
rg -n -C3 "chmod|fileperms|permission|0[67][4-7][4-7]" --type=php
echo -e "\n=== Looking for git config usage ==="
rg -n -C3 "fileMode|sharedRepository|core\." --type=php
echo -e "\n=== Checking for hooks or special git handling ==="
rg -n -C3 "hook|git.*config|update-index" --type=php
echo -e "\n=== Looking at the main Git repository class ==="
fd -e php ArtifactGitRepository | head -5 | xargs cat
Length of output: 21108
@AlexSkrypnyk I understand you need bulletproof permission preservation. Based on my research, Git only tracks the executable bit (100644 vs 100755), not full rwx permissions. Here are several approaches to achieve bulletproof permission preservation:
Option 1: Metadata File Approach (Recommended)
Store permissions in a separate metadata file that gets committed alongside your code:
Option 2: Enhanced Test with umask Control
For your test, you can control the umask to ensure consistent behavior:
public function testFilePermissions(): void {
// Store original umask and set predictable one
$originalUmask = umask(0022);
try {
$permissions = [0644, 0755];
$files = [];
foreach ($permissions as $perm) {
$filename = sprintf('file_%o.txt', $perm);
$filePath = $this->fixtureCreateFile($this->src, $filename, sprintf("Content with %o permissions\n", $perm));
chmod($filePath, $perm);
$files[$filename] = $perm;
$this->assertSame($perm, fileperms($filePath) & 0777);
}
// Store permissions metadata before git operations
$this->storePermissionsMetadata($this->src, $files);
$this->gitCommitAll($this->src, 'Added files with various permissions');
$this->assertArtifactCommandSuccess(['--branch' => 'testbranch']);
$this->gitCheckout($this->dst, 'testbranch');
// Restore permissions from metadata after checkout
$this->restorePermissionsFromMetadata($this->dst);
foreach ($files as $filename => $expectedPerm) {
$dstFile = $this->dst . DIRECTORY_SEPARATOR . $filename;
$this->assertFileExists($dstFile);
$actualPerm = fileperms($dstFile) & 0777;
$this->assertSame($expectedPerm, $actualPerm,
sprintf('File %s should preserve %o permissions, got %o', $filename, $expectedPerm, $actualPerm));
}
} finally {
// Restore original umask
umask($originalUmask);
}
}Option 3: Git Configuration Approach
Configure git settings to be more predictable:
Option 4: Integration into ArtifactCommand
You could extend your ArtifactCommand to handle permissions:
// Add to ArtifactCommand class
private function preservePermissions(string $sourcePath, string $destPath): void {
$permissionsFile = $sourcePath . '/.permissions.json';
$permissions = [];
// Collect all file permissions
$iterator = new RecursiveIteratorIterator(
new RecursiveDirectoryIterator($sourcePath)
);
foreach ($iterator as $file) {
if ($file->isFile() && !str_contains($file->getPathname(), '.git/')) {
$relativePath = substr($file->getPathname(), strlen($sourcePath) + 1);
$permissions[$relativePath] = fileperms($file->getPathname()) & 0777;
}
}
// Store permissions metadata
file_put_contents($permissionsFile, json_encode($permissions, JSON_PRETTY_PRINT));
}
private function restorePermissions(string $basePath): void {
$permissionsFile = $basePath . '/.permissions.json';
if (!file_exists($permissionsFile)) {
return;
}
$permissions = json_decode(file_get_contents($permissionsFile), true);
foreach ($permissions as $relativePath => $perm) {
$fullPath = $basePath . '/' . $relativePath;
if (file_exists($fullPath)) {
chmod($fullPath, $perm);
}
}
}Recommendation
For bulletproof permission preservation, I recommend Option 1 (Metadata File Approach) because:
- Git-agnostic: Works regardless of git's limitations
- Comprehensive: Preserves all permission bits, not just executable
- Versioned: Permissions are tracked in git alongside code
- Portable: Works across different systems and umasks
- Auditable: Permission changes are visible in git history
Would you like me to help you implement this metadata-based approach in your Git Artifact tool?
🏁 Script executed:
#!/bin/bash
# Create example implementation for permission preservation
cat > /tmp/permission_preservation_example.php << 'EOF'
<?php
// Example: Store and restore file permissions using metadata file
class PermissionPreserver {
private const METADATA_FILE = '.permissions.json';
public function storePermissions(string $basePath): void {
$permissions = [];
$iterator = new RecursiveIteratorIterator(
new RecursiveDirectoryIterator($basePath)
);
foreach ($iterator as $file) {
if ($file->isFile()) {
$relativePath = substr($file->getPathname(), strlen($basePath) + 1);
$permissions[$relativePath] = fileperms($file->getPathname()) & 0777;
}
}
file_put_contents(
$basePath . '/' . self::METADATA_FILE,
json_encode($permissions, JSON_PRETTY_PRINT)
);
}
public function restorePermissions(string $basePath): void {
$metadataPath = $basePath . '/' . self::METADATA_FILE;
if (!file_exists($metadataPath)) {
return;
}
$permissions = json_decode(file_get_contents($metadataPath), true);
foreach ($permissions as $relativePath => $perm) {
$fullPath = $basePath . '/' . $relativePath;
if (file_exists($fullPath)) {
chmod($fullPath, $perm);
}
}
}
}
EOF
echo "=== Example Permission Preservation Implementation ==="
cat /tmp/permission_preservation_example.php
Length of output: 1385
🏁 Script executed:
#!/bin/bash
# Show git configuration options for permission handling
cat << 'EOF'
# In your Git Artifact tool, you could set these configurations:
# Ensure git tracks executable bit changes
git config core.fileMode true
# Set shared repository permissions (affects new files)
git config core.sharedRepository 0644 # or 0755 for group writable
# Alternative: Use git hooks to preserve/restore permissions
# Create pre-commit hook to store permissions
# Create post-checkout hook to restore permissions
EOF
Length of output: 423
The metadata file approach is the most reliable solution for bulletproof permission preservation. Here's why it's superior to the other options:
Why Metadata File Approach Works Best:
- Complete Independence from Git: Stores full permission data separately from git's limited tracking
- Umask Immunity: Works regardless of the system's umask settings
- Version Controlled: Permission changes are tracked in git history
- Cross-Platform: Consistent behavior across different environments
- Auditable: Easy to see what permissions changed and when
For your test to be bulletproof, you'd need to:
public function testFilePermissions(): void {
$permissions = [0644, 0755];
$files = [];
// Create files with specific permissions
foreach ($permissions as $perm) {
$filename = sprintf('file_%o.txt', $perm);
$filePath = $this->fixtureCreateFile($this->src, $filename, sprintf("Content with %o permissions\n", $perm));
chmod($filePath, $perm);
$files[$filename] = $perm;
$this->assertSame($perm, fileperms($filePath) & 0777);
}
// Store permissions metadata BEFORE git operations
$this->storePermissionsMetadata($this->src, $files);
$this->gitCommitAll($this->src, 'Added files with various permissions');
$this->assertArtifactCommandSuccess(['--branch' => 'testbranch']);
$this->gitCheckout($this->dst, 'testbranch');
// Restore permissions AFTER checkout
$this->restorePermissionsFromMetadata($this->dst);
// Now assert the restored permissions
foreach ($files as $filename => $expectedPerm) {
$dstFile = $this->dst . DIRECTORY_SEPARATOR . $filename;
$this->assertFileExists($dstFile);
$actualPerm = fileperms($dstFile) & 0777;
$this->assertSame($expectedPerm, $actualPerm,
sprintf('File %s should preserve %o permissions, got %o', $filename, $expectedPerm, $actualPerm));
}
}Next Steps:
- Integrate into ArtifactCommand: Add permission preservation logic to your artifact creation/restoration process
- Add configuration option: Allow users to enable/disable permission preservation
- Handle .permissions.json in .gitignore: Decide if you want it tracked or not
- Add logging: Track when permissions are stored/restored
Would you like me to help you implement this permission preservation system directly into your ArtifactCommand and ArtifactGitRepository classes? I can show you exactly where to add the hooks and how to integrate it with your existing workflow.
✏️ Learnings added
Learnt from: AlexSkrypnyk
PR: drevops/git-artifact#175
File: tests/Functional/ExecutableBitTest.php:30-51
Timestamp: 2025-09-11T08:24:01.852Z
Learning: The user AlexSkrypnyk needs bulletproof file permission preservation in the Git Artifact tool, not just executable bit tracking that git provides natively. Full rwx permission bits must be preserved during artifact operations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
|
||
| $this->gitCommitAll($this->src, 'Added files with various permissions'); | ||
|
|
||
| $this->assertArtifactCommandSuccess(['--branch' => 'testbranch']); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove redundant branch arg.
The helper already defaults to branch "testbranch".
- $this->assertArtifactCommandSuccess(['--branch' => 'testbranch']);
+ $this->assertArtifactCommandSuccess();📝 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.
| $this->assertArtifactCommandSuccess(['--branch' => 'testbranch']); | |
| $this->assertArtifactCommandSuccess(); |
🤖 Prompt for AI Agents
In tests/Functional/ExecutableBitTest.php around line 41, the test calls
$this->assertArtifactCommandSuccess(['--branch' => 'testbranch']) but the helper
already defaults to branch "testbranch"; remove the redundant ['--branch' =>
'testbranch'] argument and call the helper with no branch override so the
default is used.
AlexSkrypnyk
force-pushed
the
feature/exec-bit
branch
from
bf9a0b9 to
9f3fc7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
CLAUDE.md (1)
1-96: Fix markdownlint issues (blank lines, trailing spaces, fenced blocks, final newline).Brings the doc in line with MD022/MD032/MD031/MD047.
# CLAUDE.md This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. ## Project Overview + Git Artifact is a PHP CLI tool that packages code artifacts from a source repository and pushes them to a destination repository. It's designed for deployment workflows where built artifacts need to be deployed to a separate repository. ## Development Commands ### Code Quality and Linting + - `composer lint` - Run code quality checks (PHPCS, PHPStan, Rector dry-run) - `composer lint-fix` - Fix code quality issues (Rector, PHPCBF) -### Testing +### Testing + - `composer test` - Run PHPUnit tests without coverage - `composer test-coverage` - Run PHPUnit tests with coverage ### Building + - `composer build` - Build standalone PHAR executable using Box ## Architecture ### Core Components + - **ArtifactCommand** (`src/Commands/ArtifactCommand.php`) - Main CLI command that orchestrates the artifact creation process - **ArtifactGitRepository** (`src/Git/ArtifactGitRepository.php`) - Extended Git repository class with artifact-specific operations - **Traits** (`src/Traits/`) - Reusable functionality: - `TokenTrait` - Handle token replacement in branch names and commit messages - `LoggerTrait` - Logging functionality - `FilesystemTrait` - File system operations ### Application Entry Points + - `git-artifact` - Main executable script with autoloader discovery - `src/app.php` - Application bootstrap using Symfony Console ### Deployment Modes + The tool supports two deployment modes: + 1. **force-push** (default) - Pushes to same branch, overwrites destination history 2. **branch** - Creates new branches in destination based on source tags ### Token System + Supports dynamic token replacement in branch names and commit messages: + - `[timestamp:FORMAT]` - Current timestamp with PHP date format - `[branch]` - Current branch name - `[safebranch]` - Branch name with non-alphanumeric chars replaced - `[tags:DELIMITER]` - Tags from latest commit ## Testing Structure ### Functional Tests (`tests/Functional/`) + Integration tests that verify end-to-end functionality: + - `BranchModeTest.php` - Tests branch mode deployment - `ForcePushModeTest.php` - Tests force-push mode deployment - `GeneralTest.php` - General functionality tests - `TagTest.php` - Token replacement and tagging tests ### Unit Tests (`tests/Unit/`) + - Component-level tests for individual classes - Follow PHPUnit conventions with strict typing ### Test Fixtures Naming Convention + - `f*` - Files with counter suffix - `i` - Ignored files - `c` - Committed files - `u` - Uncommitted files - `d` - Deleted files - `d_` - Directories - `sub_` - Sub-directories/files for wildcard testing - `f*_l` - Symlinks ## Code Standards + - Follows Drupal coding standards via PHPCS - Uses PHPStan level 9 for static analysis - Rector for automated code modernization to PHP 8.2+ - PSR-4 autoloading: `DrevOps\GitArtifact\` namespace maps to `src/` ## Key Dependencies + - `symfony/console` - CLI framework - `czproject/git-php` - Git operations - `symfony/filesystem` & `symfony/finder` - File operations - `monolog/monolog` - Logging ## Single Test Execution + Run specific test classes: + ```bash ./vendor/bin/phpunit tests/Unit/TokenTest.php ./vendor/bin/phpunit tests/Functional/BranchModeTest.phpRun specific test methods:
+./vendor/bin/phpunit --filter testTokenReplacement tests/Unit/TokenTest.php
</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: ASSERTIVE **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between bf9a0b9cffaa4a3782080881593955ed7647fd0e and 9f3fc7a471da47612fcf98cfed58b750983be333. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `CLAUDE.md` (1 hunks) * `src/Git/ArtifactGitRepository.php` (1 hunks) * `tests/Functional/FilePermissionsTest.php` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (2)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: AlexSkrypnyk
PR: #175
File: tests/Functional/ExecutableBitTest.php:30-51
Timestamp: 2025-09-11T08:24:01.852Z
Learning: The user AlexSkrypnyk needs bulletproof file permission preservation in the Git Artifact tool, not just executable bit tracking that git provides natively. Full rwx permission bits must be preserved during artifact operations.</details> <details> <summary>📚 Learning: 2025-09-11T08:24:01.852Z</summary>Learnt from: AlexSkrypnyk
PR: #175
File: tests/Functional/ExecutableBitTest.php:30-51
Timestamp: 2025-09-11T08:24:01.852Z
Learning: The user AlexSkrypnyk needs bulletproof file permission preservation in the Git Artifact tool, not just executable bit tracking that git provides natively. Full rwx permission bits must be preserved during artifact operations.**Applied to files:** - `tests/Functional/FilePermissionsTest.php` </details> </details><details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>tests/Functional/FilePermissionsTest.php (4)</summary><blockquote> <details> <summary>src/Git/ArtifactGitRepository.php (1)</summary> * `ArtifactGitRepository` (20-479) </details> <details> <summary>tests/Functional/FunctionalTestBase.php (1)</summary> * `FunctionalTestBase` (15-220) </details> <details> <summary>tests/Traits/FixtureTrait.php (1)</summary> * `fixtureCreateFile` (48-66) </details> <details> <summary>tests/Traits/GitTrait.php (2)</summary> * `gitCommitAll` (241-245) * `gitCheckout` (41-59) </details> </blockquote></details> </details><details> <summary>🪛 PHPMD (2.15.0)</summary> <details> <summary>tests/Functional/FilePermissionsTest.php</summary> 32-32: The variable $file_path is not named in camelCase. (undefined) (CamelCaseVariableName) --- 45-45: The variable $expected_perm is not named in camelCase. (undefined) (CamelCaseVariableName) --- 46-46: The variable $dst_file is not named in camelCase. (undefined) (CamelCaseVariableName) --- 48-48: The variable $actual_perm is not named in camelCase. (undefined) (CamelCaseVariableName) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>CLAUDE.md</summary> 11-11: Headings should be surrounded by blank lines Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) --- 12-12: Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 15-15: Headings should be surrounded by blank lines Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) --- 16-16: Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 19-19: Headings should be surrounded by blank lines Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) --- 20-20: Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 24-24: Headings should be surrounded by blank lines Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) --- 25-25: Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 32-32: Headings should be surrounded by blank lines Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) --- 33-33: Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 36-36: Headings should be surrounded by blank lines Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) --- 38-38: Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 41-41: Headings should be surrounded by blank lines Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) --- 43-43: Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 50-50: Headings should be surrounded by blank lines Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) --- 52-52: Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 57-57: Headings should be surrounded by blank lines Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) --- 58-58: Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 61-61: Headings should be surrounded by blank lines Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) --- 62-62: Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 88-88: Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) --- 94-94: Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) --- 96-96: Files should end with a single newline character (MD047, single-trailing-newline) </details> </details> </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>src/Git/ArtifactGitRepository.php (1)</summary><blockquote> `47-49`: **Good call: enforce Git filemode tracking.** Setting core.filemode=true at repo init ensures executable-bit changes are detected. </blockquote></details> <details> <summary>tests/Functional/FilePermissionsTest.php (1)</summary><blockquote> `26-51`: **Solid end-to-end check for 0644 vs 0755 preservation.** Covers creation, commit, artifact run, and verification on destination. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| // Ensure Git respects file permissions. | ||
| $this->execute('config', ['core.filemode', 'true']); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
This only preserves the executable bit; full rwx preservation needs extra handling.
If the goal is to keep all permission bits (e.g., 0600 vs 0644), Git alone won’t suffice. Consider:
- Archive-based transfer (git archive | tar) and extract with preserved modes.
- Sidecar metadata (e.g., .gitartifact-perms.json mapping path→mode) applied post-checkout.
- Clean/smudge filter to store/restore modes via .gitattributes.
Also verify both source and destination repos used by ArtifactCommand are instantiated as ArtifactGitRepository so this config is applied end-to-end.
🏁 Script executed:
#!/bin/bash
# Locate ArtifactGitRepository usage in the app code (not tests).
rg -nP --type=php -C3 '\bnew\s+ArtifactGitRepository\s*\(' src || true
# Sanity-check: where repos for src/dst are created inside ArtifactCommand.
rg -nP --type=php -C3 'class\s+ArtifactCommand\b|ArtifactGitRepository' src/Commands || trueLength of output: 3017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- new ArtifactGitRepository usages ----"
rg -nP --type=php '\bnew\s+ArtifactGitRepository\b' -S || true
echo
echo "---- class definition locations for ArtifactGitRepository ----"
rg -nP --type=php 'class\s+ArtifactGitRepository\b' -n -S || true
echo
echo "---- show src/Git/ArtifactGitRepository.php (first 240 lines) ----"
sed -n '1,240p' src/Git/ArtifactGitRepository.php || true
echo
echo "---- search for core.filemode occurrences ----"
rg -n 'core.filemode' -S || true
echo
echo "---- show src/Commands/ArtifactCommand.php (lines 1-520) ----"
sed -n '1,520p' src/Commands/ArtifactCommand.php || true
echo
echo "---- search for destination-related variables/usages in src/Commands ----"
rg -nP --type=php '\bdestination\b|\bdestinationDir\b|\bdestinationRepo\b|\bdest\b' src/Commands -S || true
echo
echo "---- search for archive/tar/gitattributes/filter/smudge/clean mentions ----"
rg -nP --type=php 'archive|tar\b|gitattributes|smudge|clean|filter' -S || trueLength of output: 25316
Preserve full file modes — core.filemode only tracks the executable bit
- core.filemode only preserves the executable bit (does not keep full rwx like 0600 vs 0644). The constructor currently sets it: src/Git/ArtifactGitRepository.php:48.
- Options to implement full-mode preservation: (a) transfer via git archive | tar and extract (preserves modes); (b) sidecar metadata (e.g. .gitartifact-perms.json mapping path→mode) reapplied after checkout; (c) clean/smudge filter with .gitattributes to encode/restore modes.
- ArtifactCommand instantiates ArtifactGitRepository only for the source ($this->repo = new ArtifactGitRepository(...); src/Commands/ArtifactCommand.php:327). The destination remote is not instantiated/configured — if the remote can be a local repo you must either configure the destination repo or use a transfer mechanism that does not rely on remote git config.
- Action: choose and implement one of the preservation strategies and ensure destination handling for local remotes; update tests to assert mode preservation.
🤖 Prompt for AI Agents
In src/Git/ArtifactGitRepository.php around lines 47-49, the constructor enables
Git core.filemode which only preserves the executable bit and not full rwx
modes; replace this by implementing one preservation strategy across the
repository transfer path and ensure destination handling for local remotes. Pick
one approach (recommended: use git archive | tar for transfers to preserve full
filesystem modes, or alternatively implement a sidecar JSON mapping of path→mode
written at source and reapplied at destination, or implement a clean/smudge
filter with .gitattributes to encode modes), then: update ArtifactGitRepository
to perform transfers via git archive piped to tar when exporting/importing
artifacts (or write a sidecar file after checkout and apply chmods on checkout
at destination), ensure ArtifactCommand handles local destination repos by
detecting local remotes and applying the same transfer path without relying on
remote git config, and add/adjust unit/integration tests to assert that original
file modes (e.g., 0600 vs 0644 and executable bits) are preserved after
transfer.
| protected function setUp(): void { | ||
| parent::setUp(); | ||
| $this->mode = ArtifactCommand::MODE_FORCE_PUSH; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Skip on Windows to avoid false failures.
Windows filesystems don’t reliably reflect POSIX permission bits; guard in setUp().
protected function setUp(): void {
parent::setUp();
+ if (PHP_OS_FAMILY === 'Windows') {
+ $this->markTestSkipped('POSIX file permission bits are not reliable on Windows.');
+ }
$this->mode = ArtifactCommand::MODE_FORCE_PUSH;
}🤖 Prompt for AI Agents
In tests/Functional/FilePermissionsTest.php around lines 21 to 24, the test must
be skipped on Windows because NTFS doesn't reliably reflect POSIX permission
bits; modify setUp() to detect Windows (e.g. PHP_OS_FAMILY === 'Windows' or
DIRECTORY_SEPARATOR === '\\') and call $this->markTestSkipped('Skipping on
Windows: filesystem permissions not reliable') before setting $this->mode so the
rest of the test doesn't run on Windows.
9f3fc7a to
fe375a6
Compare
2 participants
Summary by CodeRabbit