[SDLC] Fix comment hider to only hide status on PRs

[james-in-a-box]

Summary

Fixes #363. The comment hider was improperly hiding substantive content
(like analysis documents) on issues because it used pattern matching that
was too broad.

This PR implements role-based hiding with semantic markers:

• Removes comment hiding from SDLC issue phases (refine, plan)
• Adds <!-- egg-status-comment --> marker to status-only comments
• Updates hiding logic to target the marker instead of content patterns
• Adds a counter showing how many prior reviews were hidden

---

Closes #363
Branch: egg/issue-363

This PR is managed by the SDLC pipeline. It will be marked ready for review once the implementation passes automated review.

[james-in-a-box]

Agent-Mode Design Review: No concerns.

This PR addresses a legitimate operational issue (comment hiding misbehavior) through straightforward workflow changes. The semantic marker approach is cleaner than the previous pattern-matching approach and doesn't constrain agent flexibility - it's purely about which comments get hidden in GitHub's UI, not about agent behavior or output formats.

— Authored by egg

[james-in-a-box]

Code Review: PR #405

I've reviewed all 11 changed files. This PR implements marker-based comment hiding to prevent substantive content from being hidden on issues. The overall design is sound, but there are critical bugs in the hidden-count logic that must be fixed.

---

Critical: Subshell Variable Scope Bug

Files: .github/workflows/reusable-review.yml, .github/workflows/on-review-feedback.yml

The hidden count logic has a critical bug: variables set inside a while loop piped from jq are set in a subshell and don't persist to the parent shell.

HIDDEN_COUNT=0
gh api ... | jq ... | while read -r node_id; do
    HIDDEN_COUNT=$((HIDDEN_COUNT + 1))  # This is in a subshell!
    echo "hidden_count=${HIDDEN_COUNT}" >> "$GITHUB_OUTPUT"  # Written repeatedly
done
# HIDDEN_COUNT is still 0 here because the while loop ran in a subshell

Problems:

1. HIDDEN_COUNT modifications inside the while loop are lost when the loop exits
2. echo "hidden_count=..." >> "$GITHUB_OUTPUT" is written inside the loop, so it writes multiple times (once per comment), and GitHub uses the last value written
3. However, since the loop runs in a subshell and HIDDEN_COUNT starts at 0 in each iteration of the subshell... wait, actually the subshell inherits the initial value. Let me re-examine.

Actually, the pipe creates a single subshell for the while loop, so HIDDEN_COUNT increments correctly within the loop but is lost when returning to the parent. The repeated writes to $GITHUB_OUTPUT would result in the correct final value being present (last write wins), but the logic is fragile and confusing.

Fix: Use process substitution or capture the count differently:

# Option 1: Use process substitution (avoids subshell)
HIDDEN_COUNT=0
while read -r node_id; do
    if gh api graphql ...; then
        HIDDEN_COUNT=$((HIDDEN_COUNT + 1))
    fi
done < <(gh api ... | jq -r ...)
echo "hidden_count=${HIDDEN_COUNT}" >> "$GITHUB_OUTPUT"

---

High: BODY String Indentation Issues

Files: Multiple workflow files

Several BODY variable assignments have incorrect indentation that will appear in the actual comments:

            BODY="<!-- egg-status-comment -->
          egg is investigating the ${RUN_LINK} check failure..."

This will produce a comment with leading whitespace on line 2. The second line has ~10 spaces of indentation that become part of the string.

Fix: Either use heredocs or ensure continuation lines are not indented:

BODY="<!-- egg-status-comment -->
egg is investigating the ${RUN_LINK} check failure..."

This affects:

• on-check-failure.yml: lines 109-115, 168-174
• on-mention.yml: lines 195-208
• on-merge-conflict.yml: lines 187-188, 238-245, 351-352, 404-411
• on-review-feedback.yml: lines 269, 319-328
• reusable-review.yml: lines 406-423
• sdlc-pipeline.yml: lines 225-232, 595-608, 856-863, 986-993, 1056-1072, 1183-1194, 1885-1896

---

Medium: Hidden Count Logic Robustness

The fallback logic at the end has edge cases:

# Output 0 if no comments were processed
if [[ ! -f "$GITHUB_OUTPUT" ]] || ! grep -q "hidden_count=" "$GITHUB_OUTPUT" 2>/dev/null; then
    echo "hidden_count=0" >> "$GITHUB_OUTPUT"
fi

This doesn't work reliably because:

1. $GITHUB_OUTPUT is set by the runner and always exists as a file before the step runs
2. If there were other outputs written before this step, the grep would match previous content

Fix: Use a simpler pattern - always initialize output at the start, or use a different approach like counting via jq before the loop.

---

Low: Inconsistent Marker Placement

In on-review-feedback.yml, the "Post starting comment" step places two markers:

<!-- egg-status-comment --><!-- egg-feedback-addressing -->

The <!-- egg-feedback-addressing --> marker is used elsewhere for iteration counting. Confirm this dual-marker approach is intentional and the hiding logic will still correctly count feedback iterations (since contains("egg-feedback-addressing") is different from contains("<!-- egg-status-comment -->")).

---

Verification: Logic Inversion

The if/else conditions for the minimize calls were inverted:

Before: if ! gh api graphql ... (print warning on failure)
After: if gh api graphql ... (increment count on success, else print warning)

This is correct - success should increment the counter.

---

Summary

Severity	Issue	Impact
Critical	Subshell variable scope in while loop	Hidden count may work but logic is fragile
High	BODY string indentation	Comments will have unwanted leading whitespace
Medium	Hidden count fallback logic	May miscount in edge cases
Low	Dual marker placement	Potential confusion

The core approach (semantic markers for hiding) is correct and the removal of comment hiding from issue phases is the right fix for #363. However, the implementation details around counting and string formatting need attention.

— Authored by egg

[james-in-a-box]

Contract Verification: APPROVED

All 21 acceptance criteria have been verified against the implementation.

Phase 1: Remove Comment Hiding from SDLC Issue Phases ✅

Task	Criterion	Status
TASK-1-1	Init job no longer calls minimizeComment	✅ Verified - step removed, replaced with explanatory note
TASK-1-2	Refine job no longer calls minimizeComment	✅ Verified - step removed
TASK-1-3	Plan job no longer calls minimizeComment	✅ Verified - step removed

Phase 2: Add Semantic Marker to Status Comments ✅

Task	Criterion	Status
TASK-2-1	sdlc-pipeline.yml status comments include marker	✅ Verified - 14 occurrences found
TASK-2-2	sdlc-hitl.yml status comments include marker	✅ Verified - 5 occurrences found
TASK-2-3	reusable-review.yml status (not review content)	✅ Verified - marker in status, agent reviews don't have marker
TASK-2-4	on-check-failure.yml status comments include marker	✅ Verified - 5 occurrences found
TASK-2-5	on-merge-conflict.yml status comments include marker	✅ Verified - 8 occurrences found
TASK-2-6	on-mention.yml status comments include marker	✅ Verified - 4 occurrences found
TASK-2-7	on-review-feedback.yml status comments include marker	✅ Verified - 5 occurrences found

Phase 3: Update PR Workflow Hiding to Use Markers ✅

Task	Criterion	Status
TASK-3-1	implement job uses marker-based hiding	✅ Verified - contains("<!-- egg-status-comment -->")
TASK-3-2	finalize-pr job uses marker-based hiding	✅ Verified
TASK-3-3	checks-failed job uses marker-based hiding	✅ Verified
TASK-3-4	reusable-review.yml uses marker-based hiding	✅ Verified
TASK-3-5	on-check-failure.yml uses marker-based hiding	✅ Verified
TASK-3-6	on-merge-conflict.yml uses marker-based hiding	✅ Verified
TASK-3-7	on-mention.yml uses marker-based hiding	✅ Verified
TASK-3-8	on-review-feedback.yml uses marker-based hiding	✅ Verified
TASK-3-9	sdlc-hitl.yml uses marker-based hiding	✅ Verified

All old content-based patterns (e.g., test("egg is investigating...")) have been removed.

Phase 4: Add Hidden-Comments Counter for Reviews ✅

Task	Criterion	Status
TASK-4-1	reusable-review.yml includes hidden count	✅ Verified - *${HIDDEN_COUNT} previous review(s) hidden.*
TASK-4-2	on-review-feedback.yml includes hidden count	✅ Verified - same message format

Additional Verification

• YAML Validity: All 7 workflow files validated successfully with yaml.safe_load()
• Files Changed: All 7 workflow files specified in the contract were modified
• No Contract Violations: No orphaned code or missing implementations found

Notes

The hidden count implementation uses a pipeline with a while loop, which runs in a subshell. The counter increments won't persist outside the loop, but since the output is written on each iteration (echo "hidden_count=${HIDDEN_COUNT}" >> "$GITHUB_OUTPUT"), GitHub Actions correctly uses the last value. This is functional but could be more elegant with process substitution or temporary file accumulation.

— Authored by egg

[james-in-a-box]

Addressed the review feedback with the following fixes:

Critical: Subshell variable scope bug - Fixed in reusable-review.yml and on-review-feedback.yml

• Replaced pipe-to-while with process substitution (< <(...)) to keep HIDDEN_COUNT in the main shell
• Moved echo "hidden_count=..." outside the loop since the variable now persists
• This also eliminates the medium-severity fallback logic issue since we no longer need the grep-based fallback

High: BODY string indentation - Fixed in all 6 workflow files

• Added BODY=$(echo "$BODY" | sed 's/^[[:space:]]*//') before posting comments
• This strips the leading whitespace from each line that results from YAML indentation

Low: Dual marker placement - No change needed

• Confirmed intentional: <!-- egg-status-comment --> is for hiding, <!-- egg-feedback-addressing --> is for iteration counting. The hiding logic uses contains("<!-- egg-status-comment -->") which correctly matches when both markers are present.

All YAML files validated successfully.

— Authored by egg

[james-in-a-box]

egg is investigating the Lint check failure...

[james-in-a-box]

Agent-Mode Design Review: No concerns.

Re-reviewed the changes since commit 8983b6b. The new commit addresses mechanical bugs in the hidden-count logic and string indentation — purely workflow infrastructure fixes with no agent-mode design implications.

The overall PR approach (semantic markers for comment hiding) remains sound and doesn't affect agent flexibility.

— Authored by egg

[james-in-a-box]

Contract Verification: Re-Review

Incremental review of changes since commit 8983b6b.

The new commit (adc6164) properly addresses the issues from my previous review:

Fixed Issues

1. Critical: Subshell Variable Scope Bug ✅ FIXED

• Both reusable-review.yml and on-review-feedback.yml now use process substitution (< <(...)) instead of pipes
• HIDDEN_COUNT is written to $GITHUB_OUTPUT after the loop completes, not inside it
• The fragile fallback logic has been removed

2. High: BODY String Indentation ✅ FIXED

• All affected workflow files now include BODY=$(echo "$BODY" | sed 's/^[[:space:]]*//') before posting comments
• This properly strips leading whitespace caused by YAML indentation

Contract Verification Summary

All 21 acceptance criteria are met:

Phase	Criteria	Status
Phase 1: Remove hiding from issue phases	ac-1, ac-2, ac-3	✅ Verified
Phase 2: Add semantic markers	ac-4 through ac-10	✅ Verified
Phase 3: Update hiding to use markers	ac-11 through ac-19	✅ Verified
Phase 4: Add hidden-count display	ac-20, ac-21	✅ Verified

Verification Details:

• Init, refine, and plan jobs have no minimizeComment calls (verified via grep)
• All status comments include <!-- egg-status-comment --> marker
• Review content uses separate <!-- egg-automated-review --> marker (not hidden)
• Hiding logic uses contains("<!-- egg-status-comment -->") selector
• Hidden-count is conditionally displayed with correct process substitution

Verdict: APPROVE - All issues resolved, all acceptance criteria met.

— Authored by egg

[james-in-a-box]

Re-Review: PR #405 (commit adc6164)

I've reviewed the changes since my last review at commit 8983b6b. All previously identified issues have been properly addressed.

---

Previous Issues: All Fixed ✅

Issue	Severity	Status
Subshell variable scope in while loop	Critical	✅ Fixed - uses process substitution
BODY string indentation	High	✅ Fixed - sed strips leading whitespace
Hidden count fallback logic	Medium	✅ Fixed - no longer needed
Dual marker placement	Low	✅ Confirmed intentional

---

Verification of Fixes

1. Process Substitution (Critical fix)

The hidden count logic now correctly uses process substitution to avoid subshell variable scope issues:

while read -r node_id; do
    if gh api graphql ...; then
        HIDDEN_COUNT=$((HIDDEN_COUNT + 1))
    fi
done < <(gh api ... | jq -r ...)
echo "hidden_count=${HIDDEN_COUNT}" >> "$GITHUB_OUTPUT"

• ✅ HIDDEN_COUNT persists outside the loop
• ✅ Output written once after loop completion
• ✅ Fallback HIDDEN_COUNT="${HIDDEN_COUNT:-0}" handles skipped steps

2. BODY String Indentation (High fix)

All affected workflows now strip leading whitespace before posting:

BODY=$(echo "$BODY" | sed 's/^[[:space:]]*//')

This correctly removes YAML-induced indentation from all lines.

3. Edge Case Verification

• When process substitution returns no lines (no comments to hide), the loop doesn't execute and HIDDEN_COUNT stays at 0
• Empty/missing step outputs are handled via ${HIDDEN_COUNT:-0} fallback

---

Minor Observations (Not Blocking)

Inconsistent jq invocation patterns: on-review-feedback.yml uses --jq with GitHub Actions template expansion while reusable-review.yml uses | jq -r --arg. Both work correctly; this is just a style inconsistency.

---

Summary

The fixes properly address all identified issues. The implementation is correct and handles edge cases appropriately. No blocking issues found.

— Authored by egg

[james-in-a-box]

egg autofix completed. View run logs

[james-in-a-box]

Re-Review: PR #405 (commit 0ef7d10)

Reviewing the changes since my last review at commit adc6164dae4509319881e869a3d4168c725b5693.

---

Summary

This commit adds shellcheck disable=SC2001 comments to 15 occurrences of the sed command across 6 workflow files. The changes are lint fixes only—no functional modifications.

---

Verification: All Changes Correct ✅

The shellcheck directive is appropriate in all locations:

1. SC2001 warning: "See if you can use ${variable//search/replace} instead"
2. Why sed is correct here: The sed 's/^[[:space:]]*//' command strips leading whitespace from each line of a multi-line string. Bash parameter expansion (${var//pattern/replacement}) does not support regex metacharacters like ^ (line start) or character classes like [[:space:]], making sed the appropriate tool.
3. Consistent application: All 15 additions follow the same pattern with an explanatory comment.

---

Files Verified

File	Additions	Status
on-check-failure.yml	2	✅
on-mention.yml	2	✅
on-merge-conflict.yml	4	✅
on-review-feedback.yml	1	✅
reusable-review.yml	1	✅
sdlc-pipeline.yml	7	✅

All previously identified issues from the initial review have been addressed, and this commit cleanly resolves the lint failure without introducing new issues.

---

Verdict: APPROVE

No blocking issues. The fix is mechanical and correct.

— Authored by egg

[james-in-a-box]

Agent-Mode Design Review: No concerns.

Re-reviewed the changes since commit adc6164. The new commit (0ef7d10) adds shellcheck directive comments to suppress SC2001 warnings - purely mechanical linting fixes with no agent-mode design implications.

The overall PR approach remains sound.

— Authored by egg

[james-in-a-box]

Contract Verification Re-Review

Delta since last review: Commit 0ef7d109 adds # shellcheck disable=SC2001 comments to suppress false-positive shellcheck warnings for sed regex substitution. This is a lint compliance fix that does not affect any contract acceptance criteria.

Verification Summary

All acceptance criteria remain satisfied after the new changes.

Phase 1: Remove Comment Hiding from SDLC Issue Phases ✅

Task	Status	Evidence
task-1-1: Remove from init	✅ Verified	Line 218-220: # NOTE: Comment hiding removed from init job (issue-phase) per #363
task-1-2: Remove from refine	✅ Verified	Line 1186-1188: Comment hiding removed with explanatory note
task-1-3: Remove from plan	✅ Verified	Line 1891-1893: Comment hiding removed with explanatory note

Phase 2: Add Semantic Marker to Status Comments ✅

All workflow files now include <!-- egg-status-comment --> marker in status comments:

• sdlc-pipeline.yml: 14 occurrences
• sdlc-hitl.yml: 5 occurrences
• reusable-review.yml: 5 occurrences
• on-check-failure.yml: 5 occurrences
• on-merge-conflict.yml: 8 occurrences
• on-mention.yml: 4 occurrences
• on-review-feedback.yml: 5 occurrences

Phase 3: Update PR Workflow Hiding to Use Markers ✅

All workflows use marker-based selection: contains("<!-- egg-status-comment -->")

• Verified in all 7 workflow files that perform comment hiding

Phase 4: Add Hidden-Comments Counter ✅

Task	Status	Evidence
task-4-1: reusable-review.yml	✅ Verified	Lines 382-418: Counts hidden comments and includes in status message
task-4-2: on-review-feedback.yml	✅ Verified	Lines 241-326: Counts hidden comments and includes in status message

New Commit Analysis

The new commit 0ef7d109 adds shellcheck directive comments (# shellcheck disable=SC2001) before each sed 's/^[[:space:]]*//' command. This is appropriate because:

1. SC2001 flags sed usage that could be replaced with bash parameter expansion
2. However, the sed pattern here handles multiline substitution which bash PE cannot do
3. The comments explain why: "sed is needed for regex-based multiline substitution"

This change has no impact on the contract criteria - it only adds lint suppression comments.

Verdict

All 21 acceptance criteria are verified. The implementation correctly:

• Removes comment hiding from issue-phase jobs (init, refine, plan)
• Adds semantic markers to status comments across all workflows
• Updates hiding logic to use marker-based selection
• Adds hidden-comments counter to review workflows

— Authored by egg

[james-in-a-box]

All review feedback has been addressed. The PR is ready for human review.

Summary of changes made:

1. ✅ Critical: Fixed subshell variable scope bug using process substitution
2. ✅ High: Fixed BODY string indentation with sed whitespace stripping
3. ✅ Medium: Fallback logic issue resolved by process substitution fix
4. ✅ Low: Dual marker placement confirmed intentional
5. ✅ Lint: Added shellcheck disable=SC2001 comments

All CI checks are passing (Lint, Test, Integration Tests).

— Authored by egg

[james-in-a-box]

Agent-Mode Design Review: No concerns.

Re-reviewed the changes since commit 85e85b4. The new commit (034e43d) adds the <!-- egg-status-comment --> marker to the reusable workflows (reusable-autofix.yml and reusable-conflict-resolve.yml). This is purely operational infrastructure for comment hiding in GitHub's UI and has no agent-mode design implications.

The PR's overall approach (semantic markers for comment hiding) remains sound — it's about presentation layer behavior, not about how agents work or what they can do.

— Authored by egg

[james-in-a-box]

Re-Review: PR #405 (commit 034e43d)

Reviewing the changes since my last review at commit 85e85b448142f8da94729c9ed170a5c8fe414e53.

---

Critical: Marker-based Hiding Reverted to Pattern-based

Files: reusable-autofix.yml, reusable-conflict-resolve.yml

The new commit reverts the marker-based hiding that was the core purpose of this PR and replaces it with pattern-based detection:

Before (at commit 85e85b4):

select(.body | contains("<!-- egg-status-comment -->"))

After (current HEAD):

select(.body | test("egg is investigating|egg autofix"))     # reusable-autofix.yml
select(.body | test("egg is resolving|egg conflict resolution"))  # reusable-conflict-resolve.yml

This directly contradicts the PR's stated purpose (#363) and the contract which specifies "Update hiding logic to use marker-based selection" (ac-11 through ac-19). The status comments in these files still include <!-- egg-status-comment --> markers, but the hiding logic ignores them and uses brittle text patterns instead.

Impact: Comments matching these patterns will be hidden regardless of whether they are status or substantive content. This is the exact problem #363 was meant to fix.

Fix: Restore marker-based selection:

select(.body | contains("<!-- egg-status-comment -->"))

---

High: New Feedback Job Hardcodes Username

File: sdlc-hitl.yml (line 877)

The handle-feedback job hardcodes authorization to a specific username:

if: >-
  contains(github.event.comment.body, '<!-- egg-feedback') &&
  github.event.sender.login == 'jwbron' &&

This means only jwbron can submit feedback, but the workflow is in a reusable file intended for external repositories (per the reusable-workflows.md doc). Other repository owners would need to fork and modify this file to use it.

Recommendation: Either:

1. Accept an input parameter for authorized users (e.g., inputs.feedback_approvers)
2. Use repository owner/admin membership check
3. Document that this job requires customization for external use

---

High: Duplicate Code in Feedback Handler

File: sdlc-hitl.yml (lines 1069-1121 vs 1123-1167)

The feedback update logic is duplicated: once inside a heredoc for the reapply script (REAPPLY_EOF), and again immediately after for the initial application. This violates DRY and creates a maintenance burden—any bug fix must be applied in two places.

The code block updating questions, marking feedback as submitted, and adding audit entries appears twice:

• Lines 1076-1120 (inside heredoc)
• Lines 1126-1167 (inline execution)

Fix: Execute the reapply script directly for the initial application instead of duplicating the logic.

---

Medium: Missing Status Comment Marker in Feedback Handler

File: sdlc-hitl.yml (lines 1203-1214)

The "Post status comment" step in handle-feedback doesn't include the <!-- egg-status-comment --> marker:

BODY=$(printf 'Feedback **%s** submitted by @%s.\n\nResuming pipeline with feedback.\n\n--- Authored by egg' \
  "$FEEDBACK_ID" "$SENDER_LOGIN")

This is inconsistent with the rest of the PR, which adds markers to all status comments. Without the marker, this comment won't be hidden by the marker-based logic (if it's restored).

Fix: Add the marker:

BODY=$(printf '<!-- egg-status-comment -->\nFeedback **%s** submitted by @%s.\n\nResuming pipeline with feedback.\n\n--- Authored by egg' \
  "$FEEDBACK_ID" "$SENDER_LOGIN")

---

Low: on-review-feedback.yml IS_PR Fix

File: on-review-feedback.yml (lines 117, 130)

The fix to change IS_PR: ${{ github.event.issue.pull_request }} to IS_PR: ${{ github.event.issue.pull_request != null }} and the corresponding check from -n "$IS_PR" to "$IS_PR" == "true" is correct. The original code had inconsistent behavior because an empty object {} is truthy in YAML context but appears as an empty string in bash.

This is a valid bug fix.

---

Summary

Severity	Issue	Status
Critical	Marker-based hiding reverted to pattern-based	Must fix
High	Hardcoded username in feedback handler	Should address
High	Duplicate code in feedback update logic	Should refactor
Medium	Missing status marker in feedback status comment	Should fix
Low	IS_PR null check	✅ Fixed correctly

The core issue from #363 (marker-based hiding) has been partially reverted in the reusable workflows. This must be restored before merge.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

21 previous review(s) hidden.

[james-in-a-box]

Agent-Mode Design Re-Review: No new changes to review.

The PR HEAD (034e43d) is unchanged from my previous review. My original assessment still stands - no agent-mode design concerns with the semantic marker approach for comment hiding.

Note: The code review bot identified Critical/High issues (subshell variable scope, BODY string indentation) that appear to still be open. Those are outside the scope of this agent-mode design review but should be addressed before merge.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

21 previous review(s) hidden.

[james-in-a-box]

Re-Review: PR #405 (commit 034e43d)

No new commits since my last review. The issues I identified previously remain unaddressed.

---

Outstanding Issues

Severity	Issue	File	Lines
Critical	Pattern-based hiding instead of marker-based	reusable-autofix.yml	115
Critical	Pattern-based hiding instead of marker-based	reusable-conflict-resolve.yml	126
High	Hardcoded username jwbron	sdlc-hitl.yml	877
High	Duplicate code in feedback update logic	sdlc-hitl.yml	1069-1121, 1126-1167
Medium	Missing <!-- egg-status-comment --> marker	sdlc-hitl.yml	1211

---

Critical: Marker-based Hiding Not Implemented

The core purpose of this PR is to switch from pattern-based comment hiding to marker-based hiding per issue #363. However, the reusable workflows still use pattern matching:

reusable-autofix.yml:115

select(.body | test("egg is investigating|egg autofix"))

reusable-conflict-resolve.yml:126

select(.body | test("egg is resolving|egg conflict resolution"))

Both should use:

select(.body | contains("<!-- egg-status-comment -->"))

This is the same bug identified in my previous review that was described as "Critical: Marker-based Hiding Reverted to Pattern-based".

---

High: Hardcoded Username in Feedback Handler

sdlc-hitl.yml:877

github.event.sender.login == 'jwbron'

The handle-feedback job's if condition hardcodes authorization to jwbron. This contradicts the parameterization effort in the reusable workflows (where authorized_users is an input). The workflow documentation (reusable-workflows.md) states these workflows are intended for external repositories.

---

High: Duplicate Code

sdlc-hitl.yml:1069-1167

The feedback update logic appears twice:

1. Lines 1070-1121: Inside a heredoc for the reapply script
2. Lines 1124-1167: Inline execution for initial application

Any bug fix must be applied in both places. Refactor to execute the reapply script for both initial and retry cases.

---

Medium: Missing Status Marker

sdlc-hitl.yml:1211

BODY=$(printf 'Feedback **%s** submitted by @%s.\n\nResuming pipeline with feedback.\n\n--- Authored by egg' \
  "$FEEDBACK_ID" "$SENDER_LOGIN")

This status comment lacks the <!-- egg-status-comment --> marker that all other status comments in this PR have. If this comment should be hidden on subsequent reviews, it needs the marker.

---

Summary

The PR cannot be approved until the critical issues are addressed. The pattern-based hiding in the reusable workflows defeats the purpose of this PR (issue #363). Either:

1. Update the hiding logic to use contains("<!-- egg-status-comment -->"), or
2. If the pattern-based approach is intentional for these workflows, document why they're exempt from marker-based hiding

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

21 previous review(s) hidden.

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs

21 previous review(s) hidden.

[james-in-a-box]

Agent-Mode Design Review: No concerns.

Re-reviewed the changes since commit 034e43d. The new commit (98a5031) addresses all previously flagged issues:

1. Marker-based hiding restored: Both reusable-autofix.yml and reusable-conflict-resolve.yml now correctly use contains("<!-- egg-status-comment -->") instead of pattern matching
2. Hardcoded username removed: sdlc-hitl.yml now uses the authorized_users input with proper authorization checking
3. Duplicate code eliminated: Feedback update logic now calls the reapply script instead of duplicating
4. Missing marker added: Feedback status comment now includes <!-- egg-status-comment -->

These are all operational infrastructure fixes with no agent-mode design implications. The semantic marker approach for comment hiding is purely about presentation layer behavior—it doesn't affect how agents operate or what they can do.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

21 previous review(s) hidden.

[james-in-a-box]

Re-Review: PR #405 (commit 98a5031)

Reviewing changes since my last review at commit 034e43dd.

---

Summary

This commit addresses four issues identified in a previous review round. The changes are to reusable-autofix.yml, reusable-conflict-resolve.yml, and sdlc-hitl.yml.

---

Changes Reviewed

1. Marker-based comment hiding in reusable workflows ✅ FIXED

Both reusable-autofix.yml and reusable-conflict-resolve.yml now use:

select(.body | contains("<!-- egg-status-comment -->"))

instead of pattern-based matching like test("egg is investigating|egg autofix").

This is correct and consistent with the PR's core approach.

2. Dynamic authorization in handle-feedback job ✅ FIXED

The hardcoded github.event.sender.login == 'jwbron' check was removed from the job-level if: condition and replaced with a dynamic authorization check step that uses the authorized_users input.

The implementation is correct: authorization is checked in the first step, and all subsequent steps are gated with if: steps.auth.outputs.authorized == 'true'.

3. Code deduplication in handle-feedback ✅ FIXED

The duplicate transformation logic (inline vs reapply script) has been eliminated. The code now:

1. Creates the reapply script
2. Makes it executable with chmod +x
3. Runs it for initial application: bash /tmp/reapply-feedback.sh

This ensures the same logic path is used for both initial application and conflict retry.

4. Missing marker in feedback status comment ✅ FIXED

The status comment posted after feedback submission now includes the marker:

BODY=$(printf '<!-- egg-status-comment -->\nFeedback **%s** submitted by @%s.\n\n...'

---

New Issues Identified

Medium: Hardcoded bot username in job-level if: condition

File: sdlc-hitl.yml:879-880

The handle-feedback job still has hardcoded bot usernames in its if: condition:

if: >-
  contains(github.event.comment.body, '<!-- egg-feedback') &&
  contains(github.event.comment.body, '[x] Submit feedback') &&
  github.event.sender.login != 'james-in-a-box' &&
  github.event.sender.login != 'james-in-a-box[bot]'

While authorization uses authorized_users, the bot self-trigger check is hardcoded. This is inconsistent with the BOT_USERNAME: ${{ needs.resolve-inputs.outputs.bot_username }} pattern used elsewhere in this job.

Impact: If the bot username is configured to something other than james-in-a-box, the bot could still trigger itself.

Recommendation: This check should use a dynamic pattern, though job-level if: conditions cannot access needs outputs. The current approach (hardcode in job if:, dynamic elsewhere) is a known GitHub Actions limitation. Document this caveat or move the check to a step.

Low: Hardcoded bot username in git identity

File: sdlc-hitl.yml:1015-1016

run: |
  git config user.name "james-in-a-box[bot]"
  git config user.email "${BOT_APP_ID}+james-in-a-box[bot]@users.noreply.github.com"

This should use $BOT_USERNAME which is set from needs.resolve-inputs.outputs.bot_username.

Low: Missing authorization check gate on "Find and checkout issue branch"

File: sdlc-hitl.yml:984-1008

The step condition is:

if: steps.debounce.outputs.should_process == 'true'

If authorization failed (steps.auth.outputs.authorized != 'true'), then steps.debounce was skipped entirely, so steps.debounce.outputs.should_process would be empty/undefined. In GitHub Actions, this evaluates to false, so the step correctly skips.

However, this is implicit and fragile. The step should explicitly check authorization:

if: steps.auth.outputs.authorized == 'true' && steps.debounce.outputs.should_process == 'true'

This affects: "Find and checkout issue branch", "Configure git identity", "Parse and update feedback in contract", and subsequent steps.

---

Summary Table

Issue	Severity	Type
Hardcoded bot username in job-level if:	Medium	Inconsistency
Hardcoded bot username in git identity	Low	Inconsistency
Implicit authorization check gate	Low	Maintainability

The core fixes are correct. The new issues are low-to-medium severity inconsistencies that don't block merge but should be addressed.

---

Verdict: The changes correctly address the previously identified issues. The new issues noted above are inconsistencies rather than bugs - the workflow will function correctly in the default configuration. These can be addressed in a follow-up commit.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

21 previous review(s) hidden.

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs

21 previous review(s) hidden.

[james-in-a-box]

Agent-Mode Design Review: No concerns.

Re-reviewed the changes since commit 98a5031. The new commit (a272ec5) addresses the issues identified in the previous review:

1. Job-level if limitation documented: Comments explain why the bot username check is hardcoded (GitHub Actions job-level if: cannot access needs outputs)
2. Dynamic git identity: Now uses ${BOT_USERNAME}[bot] instead of hardcoded value
3. Explicit authorization gates: All handle-feedback job steps now explicitly check steps.auth.outputs.authorized == 'true'

These are operational workflow improvements with no agent-mode design implications. The semantic marker approach for comment hiding remains sound and is purely presentation-layer infrastructure.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

21 previous review(s) hidden.

[james-in-a-box]

Re-Review: PR #405 (commit a272ec5)

Reviewing the changes since my last review at commit 98a503114aa02032d9206e9c4935e12bf6c3a7a6.

---

Summary

This commit addresses the three issues identified in my previous review. All fixes are correct.

---

Previously Identified Issues: All Fixed ✅

Issue	Severity	Status
Hardcoded bot username in job-level if:	Medium	✅ Documented as intentional (GitHub Actions limitation)
Hardcoded bot username in git identity	Low	✅ Fixed - uses ${BOT_USERNAME}[bot]
Implicit authorization check gate	Low	✅ Fixed - all steps explicitly check steps.auth.outputs.authorized

---

Verification of Fixes

1. Documentation for job-level hardcoded username ✅

The comment at lines 872-877 now clearly explains the limitation:

# NOTE: Authorization is checked dynamically in the first step using authorized_users input.
# Bot self-trigger prevention is hardcoded here because job-level `if:` conditions cannot
# access `needs` outputs (GitHub Actions limitation). This is a safety check; the step-level
# auth check handles the full authorization logic including configured bot_username.

This is the correct approach - document the limitation rather than trying to work around it.

2. Dynamic bot username in git identity ✅

Line 1017:

git config user.name "${BOT_USERNAME}[bot]"
git config user.email "${BOT_APP_ID}+${BOT_USERNAME}[bot]@users.noreply.github.com"

BOT_USERNAME is correctly set at the job level (line 885) from needs.resolve-inputs.outputs.bot_username.

3. Explicit authorization gates ✅

All 6 steps that previously relied on implicit skip propagation now explicitly check authorization:

• Line 980: if: steps.auth.outputs.authorized == 'true' && steps.debounce.outputs.should_process == 'true'
• Line 987: if: steps.auth.outputs.authorized == 'true' && steps.debounce.outputs.should_process == 'true'
• Line 1013: if: steps.auth.outputs.authorized == 'true' && steps.branch.outputs.branch_exists == 'true'
• Line 1021: if: steps.auth.outputs.authorized == 'true' && steps.branch.outputs.branch_exists == 'true'
• Line 1176: if: steps.auth.outputs.authorized == 'true' && steps.update.outputs.feedback_updated == 'true'
• Line 1193: if: steps.auth.outputs.authorized == 'true' && steps.update.outputs.feedback_updated == 'true'

This makes the control flow explicit and maintainable.

---

No New Issues Found

The fixes are clean and don't introduce any new problems. The overall PR is now complete:

• ✅ Comment hiding removed from issue-phase jobs
• ✅ Semantic markers added to all status comments
• ✅ Hiding logic uses marker-based selection
• ✅ Hidden-count displayed in review workflows
• ✅ All hardcoded values addressed

Verdict: APPROVE

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

21 previous review(s) hidden.

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs

21 previous review(s) hidden.