fix(ci): persist fullsend version and validate before use - #90
Conversation
PR Summary by QodoFix CI by persisting Fullsend version and validating resolved tags
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
🤖 Finished Review · ✅ Success · Started 8:03 PM UTC · Completed 8:10 PM UTC |
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewVerdict: comment — one medium finding on commit format (unchanged from prior review); no code-level blockers. The code is identical to the prior review (945f946); the branch was rebased only. This PR correctly fixes a CI resilience gap where independent What changed since the prior reviewThe branch was rebased onto the latest main (945f946 → 4bf27d1). No code changes. All three commits retain the same content — only the committer timestamps and parent chain changed. All prior findings remain as-is. CorrectnessThe change is logically sound:
SecurityNo critical, high, or medium security findings. The workflow's security model ( Intent & ScopeEvery modified line traces to the stated goal of persisting the version and adding validation, plus hardening changes prompted by prior reviews. The lack of a linked issue is acceptable for a reactive CI fix prompted by a specific failure (PR #89). Findings
Previous runReviewVerdict: comment — one medium finding on commit format (unchanged from prior review); no code-level blockers. The new commit (945f946) addresses the prior review's remaining low-severity workflow-command-injection finding. This PR correctly fixes a CI resilience gap where independent What changed since the prior reviewThe new commit (945f946) addresses the prior review's low-severity finding #2 (workflow-command-injection):
The prior review's finding #3 (single-line OPENSHELL GITHUB_ENV writes) remains a pre-existing pattern, not a regression. CorrectnessThe change is logically sound:
SecurityNo critical, high, or medium security findings. The workflow's security model ( Every modified line traces to the stated goal of persisting the version and adding validation, plus hardening changes prompted by the prior review. The lack of a linked issue is acceptable for a reactive CI fix prompted by a specific failure (PR #89). Findings
Previous runReviewVerdict: comment — one medium finding on commit format (unchanged from prior review); no code-level blockers. The new commit (043298e) addresses both low-severity findings from the prior review. This PR correctly fixes a CI resilience gap where independent What changed since the prior reviewThe new commit (043298e) addresses both low-severity findings from the prior review (SHA 2bb9473, app-verified):
Both improvements are defense-in-depth hardening. The prior review's medium finding about commit format remains unaddressed. CorrectnessThe change is logically sound:
SecurityNo critical, high, or medium security findings. The workflow's security model ( Intent & ScopeEvery modified line traces to the stated goal of persisting the version and adding validation, plus the hardening changes prompted by the prior review. The lack of a linked issue is acceptable for a reactive CI fix prompted by a specific failure (PR #89). Findings
Previous runReviewVerdict: comment — one medium finding on commit format; no code-level blockers. This PR correctly addresses a CI resilience gap where CorrectnessThe change is logically sound:
SecurityNo high or medium security findings. The workflow's security model ( Intent & ScopeThe change is appropriately scoped — every modified line traces to the stated goal of persisting the version and adding validation. No scope creep. The lack of a linked issue is acceptable for a reactive CI fix prompted by a specific failure (PR #89). Style & ConventionsThe new DocumentationNo documentation staleness found. Findings
Previous runReviewVerdict: comment — one medium finding on commit format (unchanged from prior review); no code-level blockers. The new commit (945f946) addresses the prior review's remaining low-severity workflow-command-injection finding. This PR correctly fixes a CI resilience gap where independent What changed since the prior reviewThe new commit (945f946) addresses the prior review's low-severity finding #2 (workflow-command-injection):
The prior review's finding #3 (single-line OPENSHELL GITHUB_ENV writes) remains a pre-existing pattern, not a regression. CorrectnessThe change is logically sound:
SecurityNo critical, high, or medium security findings. The workflow's security model ( Intent & ScopeEvery modified line traces to the stated goal of persisting the version and adding validation, plus hardening changes prompted by the prior review. The lack of a linked issue is acceptable for a reactive CI fix prompted by a specific failure (PR #89). Findings
Previous run (2)ReviewVerdict: comment — one medium finding on commit format (unchanged from prior review); no code-level blockers. The new commit (043298e) addresses both low-severity findings from the prior review. This PR correctly fixes a CI resilience gap where independent What changed since the prior reviewThe new commit (043298e) addresses both low-severity findings from the prior review (SHA 2bb9473, app-verified):
Both improvements are defense-in-depth hardening. The prior review's medium finding about commit format remains unaddressed. CorrectnessThe change is logically sound:
SecurityNo critical, high, or medium security findings. The workflow's security model ( Intent & ScopeEvery modified line traces to the stated goal of persisting the version and adding validation, plus the hardening changes prompted by the prior review. The lack of a linked issue is acceptable for a reactive CI fix prompted by a specific failure (PR #89). Findings
Previous runReviewVerdict: comment — one medium finding on commit format; no code-level blockers. This PR correctly addresses a CI resilience gap where CorrectnessThe change is logically sound:
SecurityNo high or medium security findings. The workflow's security model ( Intent & ScopeThe change is appropriately scoped — every modified line traces to the stated goal of persisting the version and adding validation. No scope creep. The lack of a linked issue is acceptable for a reactive CI fix prompted by a specific failure (PR #89). Style & ConventionsThe new DocumentationNo documentation staleness found. Findings
Previous run (3)ReviewVerdict: comment — one medium finding on commit format (unchanged from prior review); no code-level blockers. The new commit (043298e) addresses both low-severity findings from the prior review. This PR correctly fixes a CI resilience gap where independent What changed since the prior reviewThe new commit (043298e) addresses both low-severity findings from the prior review (SHA 2bb9473, app-verified):
Both improvements are defense-in-depth hardening. The prior review's medium finding about commit format remains unaddressed. CorrectnessThe change is logically sound:
SecurityNo critical, high, or medium security findings. The workflow's security model ( Intent & ScopeEvery modified line traces to the stated goal of persisting the version and adding validation, plus the hardening changes prompted by the prior review. The lack of a linked issue is acceptable for a reactive CI fix prompted by a specific failure (PR #89). Findings
Previous run (4)ReviewVerdict: comment — one medium finding on commit format; no code-level blockers. This PR correctly addresses a CI resilience gap where CorrectnessThe change is logically sound:
SecurityNo high or medium security findings. The workflow's security model ( Intent & ScopeThe change is appropriately scoped — every modified line traces to the stated goal of persisting the version and adding validation. No scope creep. The lack of a linked issue is acceptable for a reactive CI fix prompted by a specific failure (PR #89). Style & ConventionsThe new DocumentationNo documentation staleness found. Findings
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 8:17 PM UTC · Completed 8:25 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 9:27 PM UTC · Completed 9:40 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
|
See #96 for a potential fix to the merge queue problems. |
The "Install fullsend" step resolved FULLSEND_VERSION as a local shell
variable but never wrote it to $GITHUB_ENV. Subsequent steps
("Configure OpenShell gateway", "Install OpenShell CLI") each made
independent gh release view calls as a fallback. When any of those
calls hit a transient GitHub API error, the version resolved to empty,
producing an invalid Docker image reference (supervisor:) that
crash-looped the gateway.
- Persist FULLSEND_VERSION to $GITHUB_ENV after resolving it once
- Validate FULLSEND_VERSION is non-empty before proceeding
- Validate OPENSHELL_VERSION and OPENSHELL_SHA are non-empty
- Remove redundant gh release view fallbacks from downstream steps
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
Use heredoc delimiter syntax for FULLSEND_VERSION GITHUB_ENV write to prevent theoretical newline-based env var injection. Sanitize version string before interpolation into ::error:: workflow commands. Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Strip %0A/%0D sequences from FULLSEND_VERSION before interpolation into workflow ::error:: commands, as a defense-in-depth measure. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
945f946 to
4bf27d1
Compare
|
🤖 Finished Review · ✅ Success · Started 3:14 PM UTC · Completed 3:21 PM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Retro · ✅ Success · Started 3:25 PM UTC · Completed 3:32 PM UTC |
|
PR #90 was a human-authored CI fix (3 commits by ralphbean with Claude assistance) that persisted FULLSEND_VERSION to GITHUB_ENV and added fail-fast validation in the functional-tests workflow. The fullsend-ai-review agent ran 4 times across pushes and a rebase, producing high-quality security-focused feedback that the author acted on. Three improvement opportunities emerged: (1) the agent repeatedly re-raised a pre-existing finding the author explicitly declined as out-of-scope, creating noise; (2) a tension between COMMITS.md calling commit-format violations 'required changes' and the verdict logic treating a single medium finding as non-blocking comment-only, allowing the forbidden Proposals filed
|
The trusted-decline mechanism from 5393b60 never activated: the pulls/comments endpoint doesn't populate performed_via_github_app, so the app-identity gate never matched and DECLINED_FINDINGS stayed empty. Switch the gate to user.login/user.type, which the endpoint actually returns (verified against this repo's own review comments). Filed fullsend-ai/fullsend#6045 to expose the harness slug as a FULLSEND_SLUG env var so this can stop hardcoding the app's identity. Also, since exact file/line matching plus a file-level changed-since-prior guard couldn't survive the line drift a declined finding's location goes through across rounds (the motivating PR #90 case this feature targets), switch decline matching to file + category + presence of the actual declined code, dropping the round-boundary check entirely — a decline now expires exactly when its code changes, not when some unrelated line in the file does. Smaller fixes bundled in: - exclude the PR author from qualifying as a decliner of their own findings - most recent qualifying reply in a thread wins, so a maintainer can reverse an earlier decline - fall back to original_line/original_start_line when GitHub nulls line on outdated comments - reword the author_association fallback to match what check-e2e-authorization.sh actually documents (defense-in-depth for membership-visibility edge cases, not an API bug) - project the review-comments fetch to needed fields and cache collaborator-permission lookups per login Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
2 participants
Summary
FULLSEND_VERSIONto$GITHUB_ENVafter the "Install fullsend" step resolves it, so downstream steps reuse the same value instead of each making independentgh release viewAPI callsFULLSEND_VERSION,OPENSHELL_VERSION, andOPENSHELL_SHAare non-empty before proceeding — fail fast with a clear error instead of silently producing an invalid Docker image reference (supervisor:) that crash-loops the gatewayContext
PR #89's functional-tests job failed because
gh release viewreturned an HTML error page during the "Configure OpenShell gateway" step. The version resolved to empty, producingghcr.io/nvidia/openshell/supervisor:which Docker rejected. The gateway crash-looped for 30s until the job timed out. Meanwhile, the separate "Install OpenShell CLI" step's owngh release viewcall succeeded — so the CLI installed fine but the gateway config was already broken.Test plan
🤖 Generated with Claude Code