[#2368] Switched git-artifact to download from GitHub Releases with SHA256 verification.#2370
Merged
AlexSkrypnyk merged 1 commit intomainfrom Mar 12, 2026
Merged
Conversation
WalkthroughReplaces Composer-based git-artifact install with a curl-downloaded binary verified by SHA256; adds env defaults for Changes
Sequence DiagramsequenceDiagram
participant Script as deploy-artifact.sh
participant GitHub as GitHub Releases
participant FS as File System
participant Verifier as SHA256 Verifier
participant Binary as git-artifact Binary
Script->>Script: Set `VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION` and `..._SHA256` defaults
Script->>GitHub: Download git-artifact binary (curl)
GitHub-->>Script: Binary bytes
Script->>FS: Write binary to temporary path
Script->>Verifier: Compute SHA256 of downloaded binary
Verifier->>FS: Read binary
Verifier-->>Script: Computed SHA256
Script->>Script: Compare computed vs expected SHA256
alt checksum matches
Script->>FS: chmod +x temporary binary
Script->>Binary: Execute git-artifact from temp location
else checksum mismatch
Script->>Script: Abort with error (checksum failure)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vortex/docs/content/development/variables.mdx:
- Around line 189-190: The table currently presents
VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION as independently configurable;
update the documentation so it clearly states that
VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION and
VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256 must be updated together (i.e., when
overriding the VERSION you must also update the SHA256), by adding a concise
note or warning next to the table entry (referencing the variables
VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION and
VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256) and optionally linking to the
validation script (scripts/vortex/deploy-artifact.sh) for context.
In @.vortex/tests/bats/_helper.bash:
- Around line 251-269: The restore function fixture_global_gitignore_restore
currently reads hardcoded /tmp files (/tmp/git_config_global_exclude_path and
/tmp/git_config_global_exclude); update it to use the same temporary-path
variables that the setup function writes (i.e., replace the literal
/tmp/git_config_global_exclude_path and /tmp/git_config_global_exclude
references with the shared temp-path variables used in setup) so both setup and
fixture_global_gitignore_restore use the same temp file locations and remain
consistent; adjust the rm -f and -f checks to use those variables as well.
- Around line 198-212: The temp files written by the helper (the echo to
/tmp/git_config_global_exclude and /tmp/git_config_global_exclude_path) use
hardcoded /tmp and fixed names which can break when TMPDIR is different and
cause races in parallel runs; change these to use a base dir of ${TMPDIR:-/tmp}
and include a unique suffix (e.g. $$) in the filenames, updating the variables
current_excludes_file and filename handling as well as the restore function that
reads them so it reads from ${TMPDIR:-/tmp}/git_config_global_exclude.$$ and
${TMPDIR:-/tmp}/git_config_global_exclude_path.$$ (keep the same logic for
resolving filename/current_excludes_file and mirror the convention used by
fixture_robo).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06dabf34-a1c7-4e2c-aef5-4901255222ab
📒 Files selected for processing (5)
.vortex/docs/content/development/variables.mdx.vortex/docs/content/tools/git-artifact.mdx.vortex/tests/bats/_helper.bash.vortex/tests/bats/unit/deploy-artifact.batsscripts/vortex/deploy-artifact.sh
Comment on lines
+198
to
+212
| # Get current git global core.excludesfile setting. | ||
| current_excludes_file="$(git config --global core.excludesfile 2>/dev/null || echo '')" | ||
|
|
||
| # If no excludesfile is configured, use default location | ||
| # Save original excludesfile config for restore. | ||
| echo "${current_excludes_file}" >/tmp/git_config_global_exclude | ||
|
|
||
| # If no excludesfile is configured, use default location. | ||
| if [ -z "${current_excludes_file}" ]; then | ||
| filename="${HOME}/.gitignore" | ||
| else | ||
| filename="${current_excludes_file}" | ||
| fi | ||
|
|
||
| # Save the resolved path for restore. | ||
| echo "${filename}" >/tmp/git_config_global_exclude_path |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded /tmp paths may cause issues in environments where TMPDIR differs.
The temp files for storing git config state use hardcoded /tmp paths (lines 202, 212), but other parts of the codebase (including the new fixture_robo at line 301) use ${TMPDIR:-/tmp}. This inconsistency could cause issues if tests run in an environment where TMPDIR is set to a different location.
Additionally, using fixed filenames like /tmp/git_config_global_exclude may cause race conditions in parallel test runs.
♻️ Suggested fix to use TMPDIR and unique filenames
- # Save original excludesfile config for restore.
- echo "${current_excludes_file}" >/tmp/git_config_global_exclude
+ # Save original excludesfile config for restore.
+ echo "${current_excludes_file}" >"${TMPDIR:-/tmp}/git_config_global_exclude_$$"
...
- # Save the resolved path for restore.
- echo "${filename}" >/tmp/git_config_global_exclude_path
+ # Save the resolved path for restore.
+ echo "${filename}" >"${TMPDIR:-/tmp}/git_config_global_exclude_path_$$"And update the restore function accordingly to use ${TMPDIR:-/tmp} and $$ suffix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tests/bats/_helper.bash around lines 198 - 212, The temp files
written by the helper (the echo to /tmp/git_config_global_exclude and
/tmp/git_config_global_exclude_path) use hardcoded /tmp and fixed names which
can break when TMPDIR is different and cause races in parallel runs; change
these to use a base dir of ${TMPDIR:-/tmp} and include a unique suffix (e.g. $$)
in the filenames, updating the variables current_excludes_file and filename
handling as well as the restore function that reads them so it reads from
${TMPDIR:-/tmp}/git_config_global_exclude.$$ and
${TMPDIR:-/tmp}/git_config_global_exclude_path.$$ (keep the same logic for
resolving filename/current_excludes_file and mirror the convention used by
fixture_robo).
This comment has been minimized.
This comment has been minimized.
Member
Author
|
Member
Author
|
Member
Author
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2370 +/- ##
==========================================
- Coverage 79.41% 78.91% -0.50%
==========================================
Files 125 118 -7
Lines 6654 6521 -133
Branches 44 0 -44
==========================================
- Hits 5284 5146 -138
- Misses 1370 1375 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/2368-git-artifact-release
branch
from
8e1b2fa to
a71cf2b
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
.vortex/tests/bats/_helper.bash (1)
198-212:⚠️ Potential issue | 🟠 MajorStore restore-state files under
TMPDIRwith unique names.These metadata files are still written to fixed
/tmp/...paths, so they can collide across parallel bats runs and break when the environment redirects temp files away from/tmp.fixture_global_gitignore_restore()depends on the same hardcoded paths, so the restore path inherits the same race.♻️ Proposed fix
+ export FIXTURE_GIT_CONFIG_GLOBAL_EXCLUDE_FILE="${TMPDIR:-/tmp}/git_config_global_exclude.$$" + export FIXTURE_GIT_CONFIG_GLOBAL_EXCLUDE_PATH_FILE="${TMPDIR:-/tmp}/git_config_global_exclude_path.$$" + # Save original excludesfile config for restore. - echo "${current_excludes_file}" >/tmp/git_config_global_exclude + echo "${current_excludes_file}" >"${FIXTURE_GIT_CONFIG_GLOBAL_EXCLUDE_FILE}" ... # Save the resolved path for restore. - echo "${filename}" >/tmp/git_config_global_exclude_path + echo "${filename}" >"${FIXTURE_GIT_CONFIG_GLOBAL_EXCLUDE_PATH_FILE}"Update
fixture_global_gitignore_restore()to read and delete the same exported paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vortex/tests/bats/_helper.bash around lines 198 - 212, The test helper writes restore-state files to fixed paths (/tmp/git_config_global_exclude and /tmp/git_config_global_exclude_path) causing collisions; change the logic in the setup block to create unique temp files (use TMPDIR or mktemp) and export those variable names (e.g., GIT_EXCLUDE_RESTORE_FILE and GIT_EXCLUDE_RESTORE_PATH) instead of hardcoded paths, and update fixture_global_gitignore_restore() to read and delete the same exported variables/files so both save and restore use the dynamically generated TMPDIR-backed filenames..vortex/docs/content/development/variables.mdx (1)
189-190:⚠️ Potential issue | 🟡 MinorDocument that these overrides must stay in sync.
These entries still read as independently configurable, but overriding
VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSIONwithout the matchingVORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256will make checksum verification fail. Since this table is auto-generated, the fix likely belongs in the source variable comments used byscripts/vortex/deploy-artifact.sh, so the rendered descriptions state that both values must be updated together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vortex/docs/content/development/variables.mdx around lines 189 - 190, The table currently lists VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION and VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256 as independent variables, but changing the version without updating the SHA256 breaks verification; update the source comments used to generate this table (the variable docs in scripts/vortex/deploy-artifact.sh or the metadata that feeds the auto-generator) so the rendered descriptions for VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION and VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256 clearly state they must be updated together (e.g., “When overriding VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION also update VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256”).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vortex/tests/bats/_helper.bash:
- Around line 251-269: The restore helper fixture_global_gitignore_restore()
currently restores a backup and resets core.excludesfile but never removes a
newly created excludes file when no original existed; update it to detect when
/tmp/git_config_global_exclude contains an empty or missing original (i.e.,
fixture_global_gitignore created the file) and delete the file at the path read
from /tmp/git_config_global_exclude_path if there was no backup (no
${filename}.bak) so generated ignore files (like .claude/.artifacts) are
removed; reference fixture_global_gitignore_restore() and the temp markers
/tmp/git_config_global_exclude and /tmp/git_config_global_exclude_path to
implement the conditional deletion.
In `@scripts/vortex/deploy-artifact.sh`:
- Around line 94-99: The script writes the downloaded git-artifact to a
predictable shared path ("${TMPDIR:-/tmp}/git-artifact"), which is vulnerable to
race/TOCTOU attacks; change the flow in the deploy-artifact.sh download/checksum
block to create a unique private temp file (use mktemp to produce a unique
path), download the binary into that unique temp file, verify the checksum using
VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256 against that temp file, chmod +x the
temp file, and then either atomically move it into the final location or execute
it from the temp path; also ensure the temp file is removed on failure—adjust
references to "${TMPDIR:-/tmp}/git-artifact" to use the mktemp-managed temp file
and keep the fail call intact.
- Around line 72-76: The preflight check loop currently only validates curl and
should also validate sha256sum; update the for loop that starts with "for cmd in
curl; do" to include sha256sum (e.g., "for cmd in curl sha256sum; do") so the
existing failure path using the fail function and exit 1 is triggered if either
command is missing, preserving the shellcheck disable comment and the current
error message behavior.
---
Duplicate comments:
In @.vortex/docs/content/development/variables.mdx:
- Around line 189-190: The table currently lists
VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION and
VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256 as independent variables, but
changing the version without updating the SHA256 breaks verification; update the
source comments used to generate this table (the variable docs in
scripts/vortex/deploy-artifact.sh or the metadata that feeds the auto-generator)
so the rendered descriptions for VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION and
VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256 clearly state they must be updated
together (e.g., “When overriding VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION
also update VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256”).
In @.vortex/tests/bats/_helper.bash:
- Around line 198-212: The test helper writes restore-state files to fixed paths
(/tmp/git_config_global_exclude and /tmp/git_config_global_exclude_path) causing
collisions; change the logic in the setup block to create unique temp files (use
TMPDIR or mktemp) and export those variable names (e.g.,
GIT_EXCLUDE_RESTORE_FILE and GIT_EXCLUDE_RESTORE_PATH) instead of hardcoded
paths, and update fixture_global_gitignore_restore() to read and delete the same
exported variables/files so both save and restore use the dynamically generated
TMPDIR-backed filenames.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6447939b-4e2d-44b1-b213-795c92cad76b
📒 Files selected for processing (5)
.vortex/docs/content/development/variables.mdx.vortex/docs/content/tools/git-artifact.mdx.vortex/tests/bats/_helper.bash.vortex/tests/bats/unit/deploy-artifact.batsscripts/vortex/deploy-artifact.sh
Comment on lines
251
to
+269
| fixture_global_gitignore_restore() { | ||
| filename=${HOME}/.gitignore | ||
| filename_backup="${filename}".bak | ||
| [ -f "${filename_backup}" ] && cp "${filename_backup}" "${filename}" | ||
| [ -f "/tmp/git_config_global_exclude" ] && git config --global core.excludesfile "$(cat /tmp/git_config_global_exclude)" | ||
| # Restore the gitignore file from backup using the path saved during setup, | ||
| # not ${HOME} which may have been changed during the test. | ||
| if [ -f "/tmp/git_config_global_exclude_path" ]; then | ||
| filename="$(cat /tmp/git_config_global_exclude_path)" | ||
| else | ||
| filename="${HOME}/.gitignore" | ||
| fi | ||
| filename_backup="${filename}.bak" | ||
| [ -f "${filename_backup}" ] && cp "${filename_backup}" "${filename}" && rm "${filename_backup}" | ||
|
|
||
| # Restore the original core.excludesfile config. | ||
| if [ -f "/tmp/git_config_global_exclude" ]; then | ||
| original="$(cat /tmp/git_config_global_exclude)" | ||
| if [ -n "${original}" ]; then | ||
| git config --global core.excludesfile "${original}" | ||
| fi | ||
| rm -f /tmp/git_config_global_exclude /tmp/git_config_global_exclude_path | ||
| fi |
There was a problem hiding this comment.
Remove the generated ignore file when there was no original to restore.
If fixture_global_gitignore() created a new excludes file, there is no .bak. The restore path resets core.excludesfile, but it never deletes the file it created, so a local bats run can leave .claude/.artifacts permanently added in the user or runner home directory.
🧹 Proposed fix
filename_backup="${filename}.bak"
- [ -f "${filename_backup}" ] && cp "${filename_backup}" "${filename}" && rm "${filename_backup}"
+ if [ -f "${filename_backup}" ]; then
+ cp "${filename_backup}" "${filename}"
+ rm "${filename_backup}"
+ elif [ -f "${filename}" ]; then
+ rm -f "${filename}"
+ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tests/bats/_helper.bash around lines 251 - 269, The restore helper
fixture_global_gitignore_restore() currently restores a backup and resets
core.excludesfile but never removes a newly created excludes file when no
original existed; update it to detect when /tmp/git_config_global_exclude
contains an empty or missing original (i.e., fixture_global_gitignore created
the file) and delete the file at the path read from
/tmp/git_config_global_exclude_path if there was no backup (no ${filename}.bak)
so generated ignore files (like .claude/.artifacts) are removed; reference
fixture_global_gitignore_restore() and the temp markers
/tmp/git_config_global_exclude and /tmp/git_config_global_exclude_path to
implement the conditional deletion.
Comment on lines
+72
to
+76
| # shellcheck disable=SC2043 | ||
| for cmd in curl; do command -v "${cmd}" >/dev/null || { | ||
| fail "Command ${cmd} is not available." | ||
| exit 1 | ||
| }; done |
There was a problem hiding this comment.
Preflight-check sha256sum too.
The new install path now depends on sha256sum, but the dependency loop only validates curl. On hosts without it, the script fails later with command not found instead of the existing explicit failure path.
🩹 Proposed fix
-# shellcheck disable=SC2043
-for cmd in curl; do command -v "${cmd}" >/dev/null || {
+# shellcheck disable=SC2043
+for cmd in curl sha256sum; do command -v "${cmd}" >/dev/null || {
fail "Command ${cmd} is not available."
exit 1
}; done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/vortex/deploy-artifact.sh` around lines 72 - 76, The preflight check
loop currently only validates curl and should also validate sha256sum; update
the for loop that starts with "for cmd in curl; do" to include sha256sum (e.g.,
"for cmd in curl sha256sum; do") so the existing failure path using the fail
function and exit 1 is triggered if either command is missing, preserving the
shellcheck disable comment and the current error message behavior.
Comment on lines
+94
to
+99
| curl -sS -L "https://github.com/drevops/git-artifact/releases/download/${VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION}/git-artifact" -o "${TMPDIR:-/tmp}"/git-artifact | ||
| if ! echo "${VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256} "${TMPDIR:-/tmp}"/git-artifact" | sha256sum -c; then | ||
| fail "SHA256 checksum verification failed for git-artifact binary." | ||
| exit 1 | ||
| fi | ||
| chmod +x "${TMPDIR:-/tmp}"/git-artifact |
There was a problem hiding this comment.
Use a private temp path for the downloaded binary.
${TMPDIR:-/tmp}/git-artifact is a predictable shared path. Another process or parallel deployment can replace that file after the checksum passes and before Line 119 executes it, which makes the verification step non-authoritative.
🔒 Proposed fix
+git_artifact_dir="$(mktemp -d "${TMPDIR:-/tmp}/git-artifact.XXXXXX")"
+trap 'rm -rf "${git_artifact_dir}"' EXIT
+git_artifact_bin="${git_artifact_dir}/git-artifact"
+
task "Installing artifact builder."
-curl -sS -L "https://github.com/drevops/git-artifact/releases/download/${VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION}/git-artifact" -o "${TMPDIR:-/tmp}"/git-artifact
-if ! echo "${VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256} "${TMPDIR:-/tmp}"/git-artifact" | sha256sum -c; then
+curl -sS -L "https://github.com/drevops/git-artifact/releases/download/${VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION}/git-artifact" -o "${git_artifact_bin}"
+if ! printf '%s %s\n' "${VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256}" "${git_artifact_bin}" | sha256sum -c; then
fail "SHA256 checksum verification failed for git-artifact binary."
exit 1
fi
-chmod +x "${TMPDIR:-/tmp}"/git-artifact
+chmod +x "${git_artifact_bin}"
...
-"${TMPDIR:-/tmp}"/git-artifact "${VORTEX_DEPLOY_ARTIFACT_GIT_REMOTE}" \
+"${git_artifact_bin}" "${VORTEX_DEPLOY_ARTIFACT_GIT_REMOTE}" \Also applies to: 119-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/vortex/deploy-artifact.sh` around lines 94 - 99, The script writes
the downloaded git-artifact to a predictable shared path
("${TMPDIR:-/tmp}/git-artifact"), which is vulnerable to race/TOCTOU attacks;
change the flow in the deploy-artifact.sh download/checksum block to create a
unique private temp file (use mktemp to produce a unique path), download the
binary into that unique temp file, verify the checksum using
VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256 against that temp file, chmod +x the
temp file, and then either atomically move it into the final location or execute
it from the temp path; also ensure the temp file is removed on failure—adjust
references to "${TMPDIR:-/tmp}/git-artifact" to use the mktemp-managed temp file
and keep the fail call intact.
Member
Author
|
Member
Author
|
Member
Author
|
… SHA256 verification.
AlexSkrypnyk
force-pushed
the
feature/2368-git-artifact-release
branch
from
a71cf2b to
da97993
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.vortex/docs/content/tools/git-artifact.mdx (1)
48-50:⚠️ Potential issue | 🟡 MinorDocumentation has swapped descriptions for name and email variables.
The descriptions are reversed:
VORTEX_DEPLOY_ARTIFACT_GIT_USER_NAMEsays "Email address of the user..."VORTEX_DEPLOY_ARTIFACT_GIT_USER_EMAILsays "Name of the user..."📝 Proposed fix
-- `VORTEX_DEPLOY_ARTIFACT_GIT_USER_NAME`: Email address of the user who will be +- committing to a remote repository. -- `VORTEX_DEPLOY_ARTIFACT_GIT_USER_EMAIL`: Name of the user who will be +- committing to a remote repository. ++ `VORTEX_DEPLOY_ARTIFACT_GIT_USER_NAME`: Name of the user who will be ++ committing to a remote repository. ++ `VORTEX_DEPLOY_ARTIFACT_GIT_USER_EMAIL`: Email address of the user who will be ++ committing to a remote repository.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vortex/docs/content/tools/git-artifact.mdx around lines 48 - 50, The two env var descriptions are swapped; update the documentation so VORTEX_DEPLOY_ARTIFACT_GIT_USER_NAME is described as the "Name of the user who will be committing to a remote repository" and VORTEX_DEPLOY_ARTIFACT_GIT_USER_EMAIL is described as the "Email address of the user who will be committing to a remote repository", ensuring the text for those identifiers in git-artifact.mdx is corrected accordingly.scripts/vortex/deploy-artifact.sh (1)
112-115: 🧹 Nitpick | 🔵 TrivialSilent failure on copy operations may mask issues.
The
|| truesuffix silently ignores copy failures for.gitand.gitignore.artifact. While this prevents script abortion when files don't exist, it also masks other errors (permission issues, disk full, etc.) that could cause deployment failures later.Consider logging when copies fail:
♻️ Suggested improvement
task "Copying git repo files meta file to the deploy code repo." -cp -a "${VORTEX_DEPLOY_ARTIFACT_ROOT}"/.git "${VORTEX_DEPLOY_ARTIFACT_SRC}" || true +cp -a "${VORTEX_DEPLOY_ARTIFACT_ROOT}"/.git "${VORTEX_DEPLOY_ARTIFACT_SRC}" || note "Note: .git directory not copied (may not exist)." task "Copying deployment .gitignore as it may not exist in deploy code source files." -cp -a "${VORTEX_DEPLOY_ARTIFACT_ROOT}"/.gitignore.artifact "${VORTEX_DEPLOY_ARTIFACT_SRC}" || true +cp -a "${VORTEX_DEPLOY_ARTIFACT_ROOT}"/.gitignore.artifact "${VORTEX_DEPLOY_ARTIFACT_SRC}" || note "Note: .gitignore.artifact not copied (may not exist)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/vortex/deploy-artifact.sh` around lines 112 - 115, The two cp commands that copy "${VORTEX_DEPLOY_ARTIFACT_ROOT}"/.git and .gitignore.artifact to "${VORTEX_DEPLOY_ARTIFACT_SRC}" are currently suffixed with "|| true", which hides real copy failures; change them to perform an existence check and explicit error handling: test for the source file/directory (e.g., [ -e "${VORTEX_DEPLOY_ARTIFACT_ROOT}/.git" ]) before cp, run cp only when present, and on failure capture the exit status and call task or processLogger (or echo to stderr) to log a clear error including the source path and exit code so permission/disk errors are visible; use the same pattern for ".gitignore.artifact" and avoid blanket "|| true".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.vortex/docs/content/tools/git-artifact.mdx:
- Around line 48-50: The two env var descriptions are swapped; update the
documentation so VORTEX_DEPLOY_ARTIFACT_GIT_USER_NAME is described as the "Name
of the user who will be committing to a remote repository" and
VORTEX_DEPLOY_ARTIFACT_GIT_USER_EMAIL is described as the "Email address of the
user who will be committing to a remote repository", ensuring the text for those
identifiers in git-artifact.mdx is corrected accordingly.
In `@scripts/vortex/deploy-artifact.sh`:
- Around line 112-115: The two cp commands that copy
"${VORTEX_DEPLOY_ARTIFACT_ROOT}"/.git and .gitignore.artifact to
"${VORTEX_DEPLOY_ARTIFACT_SRC}" are currently suffixed with "|| true", which
hides real copy failures; change them to perform an existence check and explicit
error handling: test for the source file/directory (e.g., [ -e
"${VORTEX_DEPLOY_ARTIFACT_ROOT}/.git" ]) before cp, run cp only when present,
and on failure capture the exit status and call task or processLogger (or echo
to stderr) to log a clear error including the source path and exit code so
permission/disk errors are visible; use the same pattern for
".gitignore.artifact" and avoid blanket "|| true".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d0072eb-702f-43c9-8287-940e08800ba1
📒 Files selected for processing (5)
.vortex/docs/content/development/variables.mdx.vortex/docs/content/tools/git-artifact.mdx.vortex/tests/bats/_helper.bash.vortex/tests/bats/unit/deploy-artifact.batsscripts/vortex/deploy-artifact.sh
|
Member
Author
|
Member
Author
|
Member
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2368
Summary by CodeRabbit
Documentation
New Features
Bug Fixes / Reliability
Tests