[SDLC] Fix merge conflicts in contract push-with-retry logic#394
[SDLC] Fix merge conflicts in contract push-with-retry logic#394
Conversation
Creates a shared helper script that implements the 'reset-and-reapply' pattern for pushing contract updates. When push fails due to concurrent modifications: 1. Aborts any in-progress rebase (fixing the stuck state issue) 2. Resets to remote HEAD 3. Re-applies the jq transformation on fresh remote state 4. Creates a new commit and retries Supports both inline jq filters and external scripts for complex transformations. Includes soft-fail mode for non-critical pushes (checkpoints). Fixes #385
Replaces the vulnerable push-with-retry patterns across 6 locations: 1. Line 363: Add retry logic to bare git push (populate contract tasks) 2. Line 438: Checkpoint push with soft-failure mode 3. Line 940: Advance to PR phase 4. Line 1465: Update refine review state (complex multi-step jq) 5. Line 1784: Populate contract from plan 6. Line 2144: Update plan review state (complex multi-step jq) All locations now use the push-contract-update.sh helper which implements the 'reset-and-reapply' pattern to handle merge conflicts on JSON files. Fixes #385
Replaces the vulnerable push-with-retry patterns across 3 locations: 1. Line 230: Resolve HITL decision (multi-step jq transformation) 2. Line 331: Advance to next phase 3. Line 552: Approve and advance phase (includes audit log entry) All locations now use the push-contract-update.sh helper which implements the 'reset-and-reapply' pattern to handle merge conflicts on JSON files. Fixes #385
|
egg is investigating the Lint check failure... |
Use export statements instead of inline variable assignments when calling push-contract-update.sh to avoid shellcheck warnings about variables assigned in the same statement as their expansion. Authored-by: egg
|
egg autofix completed. View run logs |
There was a problem hiding this comment.
Agent-mode design review: No concerns
This PR is infrastructure code—a shared helper script for handling git push retries with merge conflict recovery. It doesn't involve agent prompts, output parsing, or pre-fetching patterns. The changes are purely about workflow robustness.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review Summary
This PR introduces a shared helper script for conflict-resistant contract pushes, which is a good architectural improvement. However, there are several correctness and reliability issues that must be addressed before merging.
Issues Requiring Changes
1. Missing -e in set options (Helper Script - Line 55)
File: .github/scripts/push-contract-update.sh:55
set -uo pipefailThe script omits -e (errexit), which means commands can fail silently without stopping execution. Given this is a retry-loop script where intermediate failures are expected, the omission appears intentional to avoid early exit on git push failures. However, this creates a subtle bug: if validation or setup fails before the retry loop, the script continues with undefined state.
Recommendation: Add explicit error checks after the validation section (lines 57-64), or restructure to use -e with explicit || true for expected failures in the retry loop.
2. Reapply Scripts Reference Undefined RUNNER_TEMP Variable
Files:
sdlc-pipeline.yml:367-368(reapply-populate.sh)sdlc-pipeline.yml:1839-1840(reapply-populate-plan.sh)
The reapply scripts are created with a quoted heredoc (<< 'REAPPLY_EOF'), which prevents variable expansion. Inside these scripts:
PYTHONPATH="${RUNNER_TEMP}/trusted-shared:${PYTHONPATH:-}" \
python3 "${RUNNER_TEMP}/trusted-action/populate-contract-tasks.py"RUNNER_TEMP is a GitHub Actions runner variable that will not be expanded in the heredoc context because of the single quotes around REAPPLY_EOF. When the helper script executes this reapply script via bash "$JQ_SCRIPT_PATH", RUNNER_TEMP will be empty or undefined, causing the Python script path to resolve incorrectly.
Fix: Export RUNNER_TEMP before creating the heredoc and reference it as an exported variable:
export RUNNER_TEMP
cat > /tmp/reapply-populate.sh << 'REAPPLY_EOF'
...
REAPPLY_EOFOr use an unquoted heredoc (<< REAPPLY_EOF) to expand variables at script creation time rather than execution time.
3. Inconsistent Variable Export Patterns
Files:
sdlc-hitl.yml:191exports withexport SENDER_LOGIN DECISION_ID ...sdlc-pipeline.yml:1523-1529uses individualexportstatementssdlc-pipeline.yml:372-375uses inline environment variables without export
The helper script runs reapply scripts via bash "$JQ_SCRIPT_PATH". For the child script to access environment variables, they must be exported. Several locations set variables inline when calling the helper script but do not export them, which means the reapply script won't have access to critical variables.
Example problem at sdlc-pipeline.yml:469-475:
CONTRACT_PATH="$CONTRACT_PATH" \
CHECKPOINT_REASON="$CHECKPOINT_REASON" \
...
.github/scripts/push-contract-update.shThe inline variable assignment makes these available to the helper script itself, but when the helper script runs bash "$JQ_SCRIPT_PATH", the reapply script at /tmp/reapply-checkpoint.sh will not have CHECKPOINT_REASON in its environment (it's in a nested subprocess).
Recommendation: Consistently use export for all variables that reapply scripts need.
4. Helper Script Attempts First Push Without Initial Commit
File: .github/scripts/push-contract-update.sh:143-147
The push loop starts with git push origin "${BRANCH_NAME}", but the helper script never creates the initial commit. The calling workflow is expected to have already committed before invoking the helper script. This is documented in the usage but creates a semantic mismatch: the script is called push-contract-update.sh but its primary function is actually "retry push with conflict recovery."
If the initial commit is uncommitted (e.g., workflow step fails between git commit and script invocation), the push succeeds vacuously but the contract update is lost.
Recommendation: Consider having the script optionally handle the initial commit, or at minimum verify there are local commits to push before entering the retry loop.
5. Race Condition in "Nothing to Commit" Handling
File: .github/scripts/push-contract-update.sh:130-135
if ! git commit -m "$COMMIT_MESSAGE"; then
echo "::warning::Nothing to commit after re-applying transformation"
# This could happen if our changes are already in the remote
# In that case, we're done - no push needed
return 0
fiThis assumes "nothing to commit" means success, but git commit can also fail for other reasons (e.g., pre-commit hooks, invalid commit message characters). Returning 0 (success) on any commit failure could mask real errors.
Recommendation: Check the specific failure reason or at minimum log the git output for debugging.
6. Duplicate Logic in Review State Updates (Code Smell)
Files:
sdlc-pipeline.yml:1427-1473(reapply script) duplicated at1476-1517(initial application)sdlc-pipeline.yml:2145-2191(reapply script) duplicated at2194-2235(initial application)
The "Update contract with review state" steps duplicate the entire jq transformation logic twice: once in the reapply heredoc script and once inline for the initial application. This is ~45 lines duplicated in each location.
While not a correctness bug, this doubles the maintenance burden and increases the risk of the two copies diverging.
Recommendation: Consider applying the transformation only once via the reapply script pattern (create script, then invoke it immediately for initial application as well).
Minor Issues / Observations
7. Hardcoded /tmp Paths Could Collide
Multiple reapply scripts write to paths like /tmp/reapply-populate.sh, /tmp/contract.json, /tmp/contract-update.json. In a self-hosted runner with concurrent jobs, these could collide. GitHub-hosted runners isolate each job, so this is likely fine in practice.
8. Missing Error Handling for jq Index Lookup
File: sdlc-hitl.yml:197-203
If jq returns an empty string or multiple matches for the decision index lookup, the subsequent commands will misbehave. The code does check for empty, but doesn't handle the case where multiple decisions match.
Positive Aspects
- The reset-and-reapply pattern is a sound approach for handling JSON merge conflicts.
- Good documentation in the helper script header explaining the design rationale.
- Consistent use of
::warning::and::error::for GitHub Actions logging. - Soft-fail mode for checkpoint pushes is appropriate.
Verdict
The architectural approach is correct, but the RUNNER_TEMP variable scoping issue (#2) will cause failures in production for the populate-contract retry paths. Combined with the inconsistent export patterns (#3), several retry scenarios will fail silently or with confusing errors.
Request changes. Please address issues #2 and #3 at minimum before merge.
— Authored by egg
There was a problem hiding this comment.
Contract Verification Report
I have verified all 13 acceptance criteria against the implementation. All criteria are satisfied.
Phase 1: Create Shared Helper Script ✅
| Task | Criterion | Status | Evidence |
|---|---|---|---|
| TASK-1-1 | Directory exists | ✅ Verified | .github/scripts/push-contract-update.sh created with executable permissions |
| TASK-1-2 | Script handles reset-and-reapply pattern, accepts jq filter and commit message | ✅ Verified | Script implements apply_transformation(), cleanup_rebase(), handle_failure() functions with proper retry loop. Accepts JQ_FILTER, JQ_SCRIPT_PATH, COMMIT_MESSAGE, CONTRACT_PATH, BRANCH_NAME env vars |
Phase 2: Update sdlc-pipeline.yml Push Locations ✅
| Task | Criterion | Status | Evidence |
|---|---|---|---|
| TASK-2-1 | Uses helper script for populate contract tasks | ✅ Verified | Lines 364-376: Creates /tmp/reapply-populate.sh and calls .github/scripts/push-contract-update.sh |
| TASK-2-2 | Uses helper script with soft-failure mode | ✅ Verified | Lines 451-475: Sets SOFT_FAIL=true for checkpoint push |
| TASK-2-3 | Uses helper script for advance to PR phase | ✅ Verified | Lines 962-967: Uses JQ_FILTER='.current_phase = "pr"' |
| TASK-2-4 | Uses helper script for refine review state | ✅ Verified | Lines 1427-1530: Creates complex reapply script with circuit breaker logic |
| TASK-2-5 | Uses helper script for populate contract from plan | ✅ Verified | Lines 1836-1848: Creates reapply script for populate-contract-tasks.py |
| TASK-2-6 | Uses helper script for plan review state | ✅ Verified | Lines 2145-2248: Creates complex reapply script with circuit breaker logic |
Phase 3: Update sdlc-hitl.yml Push Locations ✅
| Task | Criterion | Status | Evidence |
|---|---|---|---|
| TASK-3-1 | Uses helper script for HITL decision | ✅ Verified | Lines 188-288: Creates reapply script with decision update and audit entry |
| TASK-3-2 | Uses helper script for advance to next phase | ✅ Verified | Lines 371-376: Uses JQ_FILTER=".current_phase = \"${NEXT_PHASE}\"" |
| TASK-3-3 | Uses helper script for approve and advance phase | ✅ Verified | Lines 562-613: Creates reapply script with audit log entry |
Phase 4: Testing and Verification ✅
| Task | Criterion | Status | Evidence |
|---|---|---|---|
| TASK-4-1 | No lint errors in workflow files | ✅ Verified | YAML validation passes for both files. Shellcheck passes on helper script |
| TASK-4-2 | Comments explain reset-and-reapply pattern | ✅ Verified | Script header (lines 1-53) comprehensively documents the pattern with examples |
Implementation Quality Notes
- Helper script design: Clean separation of concerns with
apply_transformation(),cleanup_rebase(), andhandle_failure()functions - Flexibility: Supports both simple
JQ_FILTERand complexJQ_SCRIPT_PATHapproaches - Soft-failure mode: Properly implemented for checkpoint scenarios
- Reapply scripts: All complex transformations create proper reapply scripts that can re-derive state from the fresh remote HEAD
Verdict
All 13 acceptance criteria verified. The implementation matches the contract specification.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Add explicit validation checks after required env var validation (issue #1) - ensures script exits early if validation fails since set -e is intentionally omitted for retry loop behavior - Export RUNNER_TEMP before heredocs in populate-contract reapply scripts (issue #2) - fixes variable scoping so child bash processes can access RUNNER_TEMP when executing reapply scripts - Standardize on export pattern for all variables that reapply scripts need (issue #3) - converts inline VAR=val invocations to explicit exports to ensure child processes inherit required variables - Improve git commit error handling in recovery path (issue #5) - distinguishes between "nothing to commit" (success) and actual commit failures (pre-commit hooks, invalid messages) Authored-by: egg
Addressing Review FeedbackI've pushed commit 87f9909 addressing the critical issues: Fixed:
Not addressed (by design):
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Agent-mode design re-review: No concerns
The delta since my last review (commit 5b36c5c → 87f9909) addresses review feedback for the push-contract-update helper script. The changes are:
- Added explicit validation guards after env var checks
- Exported
RUNNER_TEMPbefore heredocs so child scripts can access it - Standardized on explicit exports for all variables that reapply scripts need
- Improved git commit error handling to distinguish "nothing to commit" from actual failures
These are infrastructure improvements to shell scripts and GitHub Actions workflows—no agent prompts, output parsing, or pre-fetching patterns involved. The PR remains purely about workflow robustness.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-Review of Changes Since 5b36c5c
This is a re-review checking commit 87f9909 which claims to address feedback from my previous review.
Verification of Previously Raised Issues
Issue #1 (Missing -e validation guards) — Partially Fixed
The fix added explicit checks at lines 57-66:
# Fail fast if validation failed (since we don't use set -e)
if [[ -z "$CONTRACT_PATH" || -z "$BRANCH_NAME" || -z "$COMMIT_MESSAGE" ]]; then
echo "::error::Required environment variables not set"
exit 1
fiHowever, this check is redundant and never reachable. The preceding lines (58-60) use ${VAR:?...} syntax which will cause the script to exit immediately if any variable is unset or empty. The explicit if-check can never be reached because the script would have already exited.
This is harmless but indicates the fix wasn't fully understood. No action required — the :? syntax does provide the early exit behavior needed.
Issue #2 (RUNNER_TEMP undefined in reapply scripts) — NOT FIXED
The fix added export RUNNER_TEMP before the heredocs:
# Export RUNNER_TEMP so it's available in the child script
export RUNNER_TEMP
cat > /tmp/reapply-populate.sh << 'REAPPLY_EOF'
pip install -q pyyaml pydantic
PYTHONPATH="${RUNNER_TEMP}/trusted-shared:${PYTHONPATH:-}" \
python3 "${RUNNER_TEMP}/trusted-action/populate-contract-tasks.py"
REAPPLY_EOFThis does not fix the issue. The exporting of RUNNER_TEMP before the heredoc works because when the helper script runs bash "$JQ_SCRIPT_PATH", it spawns a child shell that inherits exported environment variables. However, the way the variables are referenced inside the heredoc is still problematic.
Wait — actually re-analyzing: the heredoc uses single quotes << 'REAPPLY_EOF', which prevents expansion at creation time. But when the helper script runs bash "$JQ_SCRIPT_PATH", the child bash process will expand $RUNNER_TEMP at runtime because it's in the child's environment (since it was exported). So this is correct.
Status: Fixed. My apologies for the confusion in my initial analysis.
Issue #3 (Inconsistent variable export patterns) — Fixed
All locations now properly export variables before invoking the helper script:
sdlc-hitl.yml:191:export SENDER_LOGIN DECISION_ID SELECTED_OPTION DECISION_INDEX CONTRACT_PATHsdlc-hitl.yml:560:export SENDER_LOGIN NEXT_PHASE PREVIOUS_PHASEsdlc-pipeline.yml:455-456:export CHECKPOINT_REASONandexport CONTRACT_PATHsdlc-pipeline.yml:1526-1532: exports for refine review statesdlc-pipeline.yml:2240-2248: exports for plan review state
Status: Fixed.
Issue #5 (Race condition in commit handling) — Fixed
The fix now distinguishes between "nothing to commit" (success case) and actual commit failures:
if commit_output=$(git commit -m "$COMMIT_MESSAGE" 2>&1); then
echo "$commit_output"
else
local commit_exit_code=$?
if echo "$commit_output" | grep -qE "(nothing to commit|no changes added)"; then
echo "::warning::Nothing to commit after re-applying transformation"
return 0
else
echo "::error::git commit failed: $commit_output"
return $commit_exit_code
fi
fiStatus: Fixed.
New Issues Found in This Commit
Issue #9: CONTRACT_PATH Not Exported in Reapply Script for Approve Phase (sdlc-hitl.yml:560)
At line 560, CONTRACT_PATH is not included in the export:
export SENDER_LOGIN NEXT_PHASE PREVIOUS_PHASEBut the reapply script at line 580 uses $CONTRACT_PATH:
"$CONTRACT_PATH" > /tmp/contract.jsonCONTRACT_PATH is set as an environment variable via the workflow's env: block and is exported at line 609 before calling the helper script. Since the env: block makes it available in the step, it needs to be exported before creating the heredoc for the reapply script to access it.
However, looking more carefully: The reapply script references $CONTRACT_PATH, and CONTRACT_PATH is exported at line 609 before calling the helper. Since bash heredocs with single quotes create literal content but the script runs in a child process that inherits exports, this should work because the export happens before the helper invocation.
Actually, the timing is:
- Heredoc is created at line 563 (script saved to file)
- Export happens at line 609
- Helper script runs at line 613
- Helper spawns
bash "$JQ_SCRIPT_PATH"which inherits exported vars
Status: Not a bug — the export at line 609 occurs before the helper runs, so the child bash process will have CONTRACT_PATH in its environment.
Issue #10: Missing export CONTRACT_PATH in reapply script for HITL decision (sdlc-hitl.yml)
At line 191:
export SENDER_LOGIN DECISION_ID SELECTED_OPTION DECISION_INDEX CONTRACT_PATHThis is correct — CONTRACT_PATH is exported here.
At line 281:
export CONTRACT_PATHThis is also correct — CONTRACT_PATH is re-exported before calling the helper.
Status: Not a bug.
Summary
The commit successfully addresses issues #2, #3, and #5 from my previous review. Issue #1 was partially addressed (the validation guards work via the :? syntax, even though the explicit if-check is dead code).
No new blocking issues were found. The implementation is now sound.
Verdict
Approve. All critical issues from the previous review have been addressed. The reset-and-reapply pattern is correctly implemented across all 9 push locations.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Contract Verification Re-Review
Re-reviewing commit 87f9909 (delta from 5b36c5c)
Delta Summary
Single commit addressing review feedback:
push-contract-update.sh: +30/-5 linessdlc-hitl.yml: +14/-14 linessdlc-pipeline.yml: +23/-18 lines
Changes Verified
All issues raised in previous review have been addressed:
-
Explicit validation after env var checks (issue #1) ✅
- Added explicit
if [[ -z ... ]]check at lines 65-68 sinceset -eis not used
- Added explicit
-
RUNNER_TEMP export before heredocs (issue #2) ✅
- Added
export RUNNER_TEMPat lines 361-362 and 1839-1840 in sdlc-pipeline.yml
- Added
-
Standardized export pattern (issue #3) ✅
- All variable exports now use separate
exportstatements instead of inlineVAR=valinvocations
- All variable exports now use separate
-
JQ_SCRIPT_PATH existence check ✅
- Added validation at lines 77-80 to verify script file exists before use
-
Improved git commit error handling (issue #5) ✅
- Lines 142-160 now distinguish "nothing to commit" from actual failures
- Properly captures exit code and handles pre-commit hook failures
Acceptance Criteria Status
| Criterion | Status | Evidence |
|---|---|---|
| ac-1: Directory exists | ✅ | .github/scripts/ created |
| ac-2: Script handles reset-and-reapply | ✅ | Lines 1-53 document pattern, lines 104-162 implement |
| ac-3: Populate tasks uses helper | ✅ | sdlc-pipeline.yml lines 371-375 |
| ac-4: Soft-fail checkpoint | ✅ | sdlc-pipeline.yml line 477 |
| ac-5: Advance PR uses helper | ✅ | sdlc-pipeline.yml lines 967-970 |
| ac-6: Refine review uses helper | ✅ | Verified in previous review |
| ac-7: Populate from plan uses helper | ✅ | sdlc-pipeline.yml lines 1847-1850 |
| ac-8: Plan review uses helper | ✅ | Verified in previous review |
| ac-9: HITL decision uses helper | ✅ | sdlc-hitl.yml lines 281-285 |
| ac-10: Advance phase uses helper | ✅ | sdlc-hitl.yml lines 372-375 |
| ac-11: Approve advance uses helper | ✅ | sdlc-hitl.yml lines 609-612 |
| ac-12: No lint errors | ✅ | CI "All Linters" passed |
| ac-13: Documentation complete | ✅ | Script header documents pattern |
CI Status
- ✅ Integration Tests: SUCCESS
- ✅ All Linters: SUCCESS
- ✅ Unit Tests: SUCCESS
- ✅ Security Scan: SUCCESS
Minor Observation (Non-blocking)
The approve-phase section in sdlc-hitl.yml exports CONTRACT_PATH at line 609 (after the heredoc) rather than at line 561 (before the heredoc with other exports). This is functionally correct since the export occurs before script invocation, but differs from the pattern used in other sections where exports precede heredocs. This inconsistency is cosmetic and doesn't affect correctness.
Verdict
APPROVE — All acceptance criteria verified. The changes correctly address the review feedback.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Addressing Minor Review ObservationI've pushed commit 056ab8f to address the non-blocking observation from the re-review: Fixed:
This makes the export pattern consistent with other sections in the file and improves readability. — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Agent-mode design re-review: No concerns
The delta since commit 87f9909 is a single cleanup commit that consolidates the CONTRACT_PATH export with other variable exports at the top of the approve-phase step. This is a cosmetic improvement to shell script style with no functional change.
This PR remains infrastructure code—shell scripts and GitHub Actions workflows for git push retry logic. No agent prompts, output parsing, or pre-fetching patterns.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-Review of Changes Since Commit 87f9909
This is a re-review of PR #394 at commit 056ab8f.
Delta Summary
Single commit addressing the minor observation from my previous review:
- Consolidated
CONTRACT_PATHexport at line 560 with other exports (SENDER_LOGIN NEXT_PHASE PREVIOUS_PHASE CONTRACT_PATH) instead of exporting it separately at line 609
This change makes the export pattern consistent with other sections in the file.
Verification
The change is correct:
CONTRACT_PATHis now exported before the heredoc at line 563- The child bash process spawned by the helper script will have
CONTRACT_PATHin its environment - The export at line 609 was removed since it's now handled earlier
- This matches the pattern used in other sections (sdlc-pipeline.yml and the HITL decision step)
CI Status
All checks passing:
- ✅ Integration Tests
- ✅ All Linters
- ✅ Unit Tests
- ✅ Security Scan
Conclusion
No issues found in this commit. The change addresses the cosmetic inconsistency I noted in my previous review.
All critical issues from the original review have been addressed:
- ✅ Validation guards added (via
:?syntax) - ✅
RUNNER_TEMPexported before heredocs - ✅ Consistent export patterns across all locations
- ✅ Improved git commit error handling
Approve. The implementation is sound and ready for merge.
— Authored by egg
There was a problem hiding this comment.
Contract Verification Re-Review
Incremental review: 87f9909...056ab8f
Delta Since Last Review
One commit since last review:
056ab8f: Consolidate CONTRACT_PATH export with other exports in approve-phase step
This minor cleanup:
- Moves
CONTRACT_PATHfrom a later standaloneexport(line ~608) to the earlier combined export statement (line 560) - Ensures proper variable scoping for both the heredoc reapply script and the
push-contract-update.shhelper
The change follows the same pattern already used elsewhere in the file (lines 191, 281, 372).
Contract Compliance
All acceptance criteria remain verified:
| Criterion | Status | Notes |
|---|---|---|
| ac-1 (Directory exists) | ✓ Verified | .github/scripts/ present |
| ac-2 (Helper script) | ✓ Verified | Reset-and-reapply pattern, accepts params |
| ac-3 (TASK-2-1) | ✓ Verified | Line 378 uses helper |
| ac-4 (TASK-2-2) | ✓ Verified | Line 478 uses SOFT_FAIL=true |
| ac-5 (TASK-2-3) | ✓ Verified | Line 970 uses helper |
| ac-6 (TASK-2-4) | ✓ Verified | Line 1533 uses JQ_SCRIPT_PATH |
| ac-7 (TASK-2-5) | ✓ Verified | Line 1853 uses helper |
| ac-8 (TASK-2-6) | ✓ Verified | Line 2253 uses JQ_SCRIPT_PATH |
| ac-9 (TASK-3-1) | ✓ Verified | Line 285 uses helper |
| ac-10 (TASK-3-2) | ✓ Verified | Line 376 uses helper |
| ac-11 (TASK-3-3) | ✓ Verified | Line 612 uses helper with audit log |
| ac-12 (Linting) | ✓ Verified | Shellcheck passes |
| ac-13 (Comments) | ✓ Verified | Lines 1-14 document pattern |
Regression Check
No regressions introduced. The consolidation of export CONTRACT_PATH with other variable exports is consistent with the codebase pattern and doesn't change behavior.
Verdict
Approved. All acceptance criteria verified. Ready for human merge.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
Status UpdateAll review feedback has been addressed. The PR is ready for human merge. Issues addressed:
Issues intentionally not addressed (per earlier justification):
All CI checks pass. Awaiting human review and merge. — Authored by egg |
|
egg feedback addressed. View run logs |
|
egg contract-verification failed. View run logs |
|
egg review failed. View run logs |
|
egg agent-mode-design failed. View run logs |
Update documentation to reflect the new conflict-resistant contract push mechanism introduced in PR #394. The .github/scripts/push-contract-update.sh script implements a reset-and-reapply pattern for handling concurrent contract updates without merge conflicts. Changes: - Added .github/scripts/ directory to STRUCTURE.md - Documented conflict-resistant contract updates in sdlc-pipeline.md - Added push-contract-update.sh to Key Files reference table Triggered by: PR #394 (commit 93024ab) Authored-by: egg
Update documentation to reflect the new conflict-resistant contract push mechanism introduced in PR #394. The .github/scripts/push-contract-update.sh script implements a reset-and-reapply pattern for handling concurrent contract updates without merge conflicts. Changes: - Added .github/scripts/ directory to STRUCTURE.md - Documented conflict-resistant contract updates in sdlc-pipeline.md - Added push-contract-update.sh to Key Files reference table Triggered by: PR #394 (commit 93024ab) Authored-by: egg Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
Summary
Fixes #385: The push-with-retry logic fails when git rebase encounters merge
conflicts on contract JSON files. This PR introduces a shared helper script
that uses the "reset-and-reapply" pattern: on push failure, abort any in-progress
rebase, reset to remote HEAD, and re-apply the jq transformation from scratch.
This eliminates the merge conflict vulnerability across all 9 affected locations
in sdlc-pipeline.yml and sdlc-hitl.yml.
Closes #385
Branch:
egg/issue-385This PR is managed by the SDLC pipeline. It will be marked ready for review once the implementation passes automated review.