Fix hardcoded port numbers in compose.py and orchestration.py - #401
Conversation
Import GATEWAY_PORT from egg_config.constants instead of using hardcoded port values. This fixes the check-hardcoded-ports.py lint check. Authored-by: egg
There was a problem hiding this comment.
Post-Merge Review
This PR has already been merged. I've conducted a thorough review and found no blocking issues. The changes are correct and well-implemented.
Summary
The PR correctly replaces hardcoded port numbers ("9848" and 9848) with imports from GATEWAY_PORT defined in shared/egg_config/constants.py.
Verification
- Import chain verified:
compose.pyandorchestration.pynow importGATEWAY_PORTfrom.config, which re-exports it fromegg_config.constants. - Lint check passes:
python scripts/check-hardcoded-ports.pyreturns OK. - Type correctness: In
compose.py, the code correctly usesstr(GATEWAY_PORT)since the context requires a string (port variable used in string formatting for health URL). - Default value correctness: In
orchestration.py,GATEWAY_PORT(an int) is correctly used as the default value forgateway_port: int.
Minor Observations (Non-blocking)
-
Redundant env file parsing: In
compose.py:204-210, the code loads the port from the constant but then immediately checks.envforGATEWAY_API_PORToverride. This is the correct behavior—allowing env file overrides while having a sane default. -
Integration tests have allowlisted hardcoded values: Files like
integration_tests/test_network_isolation.pystill haveGATEWAY_PORT = 9848hardcoded, but this is intentional and documented in the lint check allowlist.
Conclusion
The PR achieves its stated goal of fixing the hardcoded port lint violations. No issues identified.
— Authored by egg
|
egg review completed. View run logs |
Import GATEWAY_PORT from egg_config.constants instead of using hardcoded port values. This fixes the check-hardcoded-ports.py lint check. Authored-by: egg Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com>
Import GATEWAY_PORT from egg_config.constants instead of using hardcoded port values. This fixes the check-hardcoded-ports.py lint check. Authored-by: egg Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com>
* Initialize SDLC contract for issue #363 * Draft analysis for issue #363 * Refine review: approved for issue #363 * Update refine review state (cycle 1) * Approve and advance to plan phase for issue #363 * Draft plan for issue #363 * Update plan review state (cycle 1) * Approve and advance to implement phase for issue #363 * Populate contract tasks from plan for issue #363 * Remove comment hiding from SDLC issue-phase jobs This addresses the first phase of fixing issue #363 where substantive content like analysis documents was being improperly hidden on issues. Removed the "Minimize previous pipeline comments" step from: - init job (was hiding comments on new issues) - refine job (was hiding analysis documents) - plan job (was hiding plan documents) Comment hiding will remain in PR-phase jobs (implement, finalize-pr, checks-failed) but will be updated to use semantic markers instead of pattern matching to prevent false positives. * Add <!-- egg-status-comment --> marker to status comments This enables precise targeting of status-only comments for hiding logic. The marker is added to all status/notification comments that should be eligible for minimization, while substantive content (analysis documents, reviews, plans) remains unmarked and visible. Files updated: - sdlc-pipeline.yml: init, implement, checks, refine error, plan error - sdlc-hitl.yml: decision resolved, phase approved - reusable-review.yml: workflow completion status - on-check-failure.yml: investigating + result comments - on-merge-conflict.yml: starting + result comments (both jobs) - on-mention.yml: fallback status comments - on-review-feedback.yml: starting + result comments Phase 2 of fix for issue #363. * Update comment hiding logic to use semantic markers All hiding logic now targets the `<!-- egg-status-comment -->` marker instead of pattern-matching on content. This prevents false positives where substantive content matching status patterns was accidentally hidden. Files updated: - sdlc-pipeline.yml: implement, finalize-pr, checks-failed jobs - sdlc-hitl.yml: both decision and approval handlers - reusable-review.yml: status comment hiding - on-check-failure.yml: autofix comment hiding - on-merge-conflict.yml: both auto and manual jobs - on-mention.yml: fallback status comment hiding - on-review-feedback.yml: feedback comment hiding Phase 3 of fix for issue #363. * Add hidden-comments counter to review workflows When review bots hide prior reviews before posting new ones, the status comment now includes a count of how many were hidden. This addresses human feedback requesting visibility into review cycles. Example output: "egg code-review completed. [View run logs](...) *2 previous review(s) hidden.*" Files updated: - reusable-review.yml: Counter in minimize step, conditional message - on-review-feedback.yml: Counter in minimize step, conditional message Phase 4 of fix for issue #363. * Fix subshell variable scope and BODY string indentation issues Address code review feedback: 1. Critical: Fix subshell variable scope bug in hidden count logic - Use process substitution instead of pipe to avoid subshell - Write hidden_count output after the loop, not inside - Removes fragile fallback logic that was no longer needed 2. High: Fix BODY string indentation causing leading whitespace - Add sed command to strip leading whitespace from multiline strings before posting comments to GitHub - Affects status comments in all workflows that use the egg-status-comment marker Files: reusable-review.yml, on-review-feedback.yml, on-check-failure.yml, on-mention.yml, on-merge-conflict.yml, sdlc-pipeline.yml Authored-by: egg * Fix SC2001 shellcheck violations in workflow files Add shellcheck disable comments for SC2001 (style) warnings. The sed command is intentionally used here for regex-based multiline substitution to strip leading whitespace from YAML block literals, which cannot be achieved with bash parameter expansion. Authored-by: egg * Fix hardcoded port numbers in compose.py and orchestration.py (#401) Import GATEWAY_PORT from egg_config.constants instead of using hardcoded port values. This fixes the check-hardcoded-ports.py lint check. Authored-by: egg Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> * [SDLC] Make SDLC workflows reusable for external repositories (#404) * Initialize SDLC contract for issue #255 * Draft analysis for issue #255 * Update refine review state (cycle 1) * Approve and advance to plan phase for issue #255 * Draft plan for issue #255 * Update plan review state (cycle 1) * Approve and advance to implement phase for issue #255 * Populate contract tasks from plan for issue #255 * Add bot-username and action-ref inputs to reusable-review.yml Phase 1 of making SDLC workflows reusable: - Add bot_username input with default 'james-in-a-box' to reusable-review.yml - Add action_ref input with default 'jwbron/egg/action@main' (documented that GitHub Actions uses: field cannot be dynamic) - Update BOT_USERNAME env var to use input parameter - Update on-pull-request.yml, on-pull-request-agent-mode-design.yml, and on-pull-request-contract-verify.yml to pass bot_username Authored-by: egg * Create reusable autofix and conflict resolution workflows Phase 2 of making SDLC workflows reusable: - Create reusable-autofix.yml with parameterized bot_username, action_ref, prompt_script, and timeout inputs - Convert on-check-failure.yml to thin wrapper calling reusable-autofix.yml - Create reusable-conflict-resolve.yml with same parameterization - Convert on-merge-conflict.yml to use reusable-conflict-resolve.yml for both scheduled and manual resolution jobs - Discovery logic (find-conflicts job) remains in wrapper Authored-by: egg * Parameterize feedback and mention workflows Phase 3 of making SDLC workflows reusable: - Add workflow_call trigger to on-review-feedback.yml with inputs: bot_username, action_ref, authorized_users, max_feedback_rounds, prompt_script, timeout - Add workflow_call trigger to on-mention.yml with inputs: bot_username, action_ref, authorized_users, mention_patterns, prompt_script, timeout - Use resolve-inputs job workaround for job-level if conditions (GitHub Actions doesn't allow inputs in job-level if) - Replace hardcoded james-in-a-box with parameterized bot_username - Replace hardcoded jwbron authorization with authorized_users input - Replace hardcoded @james-in-a-box/@Egg patterns with mention_patterns Authored-by: egg * Parameterize SDLC pipeline and HITL workflows Phase 4 of making SDLC workflows reusable: - Add workflow_call trigger to sdlc-pipeline.yml with inputs: bot_username, action_ref, authorized_users, branch_prefix, sdlc_label - Add workflow_call trigger to sdlc-hitl.yml with inputs: bot_username, authorized_users, branch_prefix - Add resolve-inputs job to both workflows for job-level access - Replace hardcoded james-in-a-box git identity with BOT_USERNAME env var - Replace hardcoded egg/issue- branch prefix with BRANCH_PREFIX env var - Replace hardcoded jwbron authorization with authorized_users input - Update all jobs to depend on resolve-inputs for access to parameters Authored-by: egg * Parameterize utility workflows and add documentation Phase 5 of making SDLC workflows reusable: - Add workflow_call trigger to on-issue-closed.yml with inputs: branch_prefix, sdlc_label - Add workflow_call trigger to on-push-doc-updater.yml with inputs: commit_sha, dry_run, action_ref, prompt_script - Add workflow_call trigger to self-improvement.yml with inputs: since_hours, dry_run, action_ref - Create docs/guides/reusable-workflows.md documenting: - Available reusable workflows and their parameters - Common input parameters and their defaults - action_ref limitation and workaround - Required secrets - Complete example configurations - Custom prompt script creation - Security considerations Authored-by: egg * Fix actionlint failures in workflow files - Fix untrusted input vulnerabilities by passing event data through environment variables instead of inline script interpolation (on-mention.yml, on-review-feedback.yml, sdlc-hitl.yml) - Fix undefined 'resolve-inputs' property by adding it to finalize-pr job's needs list (sdlc-pipeline.yml) - Fix SC2129 shellcheck warnings by grouping multiple redirects with braces (on-check-failure.yml, on-mention.yml, sdlc-hitl.yml, sdlc-pipeline.yml) Authored-by: egg * Address review feedback for reusable workflows Fixes: - Add explicit null/"null" check for PR number in on-check-failure.yml gate job to prevent false positives when pull_requests[0] is null - Add set -euo pipefail to check-trigger shell block in on-mention.yml for consistency with other shell blocks - Handle "null" string explicitly for TRIGGER_TIME in on-mention.yml Post result comment step to prevent jq query failures --------- Co-authored-by: james-in-a-box[bot] <2365503+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> * Add <!-- egg-status-comment --> marker to reusable workflows Since on-check-failure.yml and on-merge-conflict.yml now use the reusable workflow versions (reusable-autofix.yml and reusable-conflict-resolve.yml), the status comment markers need to be added to the reusable workflows to maintain consistent hiding behavior across all comment-posting workflows. * Fix marker-based hiding and code issues in workflows Address review feedback on PR #405: - Critical: Switch from pattern-based to marker-based comment hiding in reusable-autofix.yml and reusable-conflict-resolve.yml (use `contains("<!-- egg-status-comment -->")` instead of text patterns) - High: Replace hardcoded 'jwbron' username in sdlc-hitl.yml handle-feedback job with dynamic authorization check using `authorized_users` input - High: Eliminate duplicate code in handle-feedback by running reapply-feedback.sh script for initial application (same logic used for retry) - Medium: Add missing `<!-- egg-status-comment -->` marker to feedback submission status comment in sdlc-hitl.yml Authored-by: egg * Fix handle-feedback job hardcoded values and implicit auth gates - Document job-level if condition limitation: bot self-trigger check must be hardcoded since job-level if cannot access needs outputs - Replace hardcoded 'james-in-a-box[bot]' with ${BOT_USERNAME}[bot] in git identity configuration step - Add explicit authorization check to all handle-feedback job steps for maintainability instead of relying on implicit skip propagation Authored-by: egg --------- Co-authored-by: james-in-a-box[bot] <2365503+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com>
Summary
Fixes the hardcoded port numbers in
sandbox/egg_lib/compose.pyandsandbox/egg_lib/orchestration.pythat were introduced in PR #356 byimporting
GATEWAY_PORTfromegg_config.constantsinstead of usinghardcoded values.
This fix is required for the
check-hardcoded-ports.pylint check to pass.Changes
sandbox/egg_lib/compose.py: ImportGATEWAY_PORTfrom.configand usestr(GATEWAY_PORT)instead of hardcoded"9848"sandbox/egg_lib/orchestration.py: ImportGATEWAY_PORTfrom.configand use it as the default value for
gateway_portin theOrchestrationResultdataclass
Test plan
python scripts/check-hardcoded-ports.py- should pass with no violationsruff checkon the modified files - should passmypyon the modified files - should passNote: This fix is blocking PR #395 which is failing the lint check due to
these hardcoded ports.
— Authored by egg