[SDLC] get rid of hardccoded gatway and sandbox ports and uris

[james-in-a-box]

Summary

Approve and advance to implement phase for issue #280

---

Issue: #280
Branch: egg/issue-280

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]

egg is investigating the Lint check failure...

[james-in-a-box]

egg autofix completed. View run logs

[james-in-a-box]

Contract Verification Review

Summary

Verified 26 of 28 acceptance criteria. 2 criteria not met.

Verification Results

Phase 1: Create Centralized Constants Module

Criterion	Status	Notes
ac-1 [TASK-1-1]	✅ VERIFIED	shared/egg_config/constants.py exports all required constants including TEST_GATEWAY_PORT = 1234
ac-2 [TASK-1-2]	✅ VERIFIED	Constants importable via from egg_config import GATEWAY_PORT
ac-3 [TASK-1-3]	✅ VERIFIED	GatewayConfig.port defaults to GATEWAY_PORT constant

Phase 2: Update Sandbox/Launcher Code

Criterion	Status	Notes
ac-4 [TASK-2-1]	✅ VERIFIED	sandbox/egg_lib/config.py imports from shared module, re-exports via __all__
ac-5 [TASK-2-2]	❌ NOT VERIFIED	sandbox/egg_lib/context.py was NOT modified - RuntimeContext defaults do not reference shared constants
ac-6 [TASK-2-3]	✅ VERIFIED	get_gateway_url() uses GATEWAY_PORT from config
ac-7 [TASK-2-4]	✅ VERIFIED	sandbox/entrypoint.py uses constants at lines 515, 815, 919-920
ac-8 [TASK-2-5]	✅ VERIFIED	network_mode.py health check uses GATEWAY_PORT constant
ac-9 [TASK-2-6]	✅ VERIFIED	Error messages in runtime.py use GATEWAY_PORT constant
ac-10 [TASK-2-7]	✅ VERIFIED	shared/egg_container/__init__.py uses GATEWAY_PROXY_PORT constant

Phase 3: Update Shell Scripts

Criterion	Status	Notes
ac-11 [TASK-3-1]	✅ VERIFIED	sandbox/scripts/gh fails with error if GATEWAY_URL not set
ac-12 [TASK-3-2]	✅ VERIFIED	sandbox/scripts/git fails with error if GATEWAY_URL not set
ac-13 [TASK-3-3]	✅ VERIFIED	Comments added to gateway/entrypoint.sh and gateway/start-gateway.sh

Phase 4: Update Test Fixtures

Criterion	Status	Notes
ac-14 [TASK-4-1]	✅ VERIFIED	integration_tests/conftest.py imports GATEWAY_PORT, GATEWAY_PROXY_PORT
ac-15 [TASK-4-2]	✅ VERIFIED	test_gateway_helpers.py uses TEST_GATEWAY_PORT in mock contexts
ac-16 [TASK-4-3]	✅ VERIFIED	test_runtime_helpers.py uses TEST_GATEWAY_PORT in mock contexts
ac-17 [TASK-4-4]	✅ VERIFIED	test_context.py assertions use GATEWAY_PORT constant
ac-18 [TASK-4-5]	✅ VERIFIED	test_entrypoint.py uses GATEWAY_PORT constant
ac-19 [TASK-4-6]	✅ VERIFIED	test_contract_cli.py assertions use GATEWAY_PORT constant

Phase 5: Update Documentation

Criterion	Status	Notes
ac-20 [TASK-5-1]	✅ VERIFIED	sandbox/.claude/rules/environment.md updated with 9848 and mentions constants file
ac-21 [TASK-5-2]	✅ VERIFIED	ADR-Gateway-Credential-Injection.md updated 9847 → 9848
ac-22 [TASK-5-3]	✅ VERIFIED	ADR-Anthropic-API-Credential-Injection.md updated 9847 → 9848
ac-23 [TASK-5-4]	✅ VERIFIED	ADR-Git-Isolation-Architecture.md updated 9847 → 9848
ac-24 [TASK-5-5]	✅ VERIFIED	ADR-Internet-Tool-Access-Lockdown.md updated 9847 → 9848
ac-25 [TASK-5-6]	✅ VERIFIED	ADR-GitHub-Actions-Support.md updated 9847 → 9848
ac-26 [TASK-5-7]	❌ NOT VERIFIED	gateway/tests/README-integration.md was NOT modified in this PR

Phase 6: Add CI Lint Check

Criterion	Status	Notes
ac-27 [TASK-6-1]	✅ VERIFIED	scripts/check-hardcoded-ports.py detects hardcoded port patterns
ac-28 [TASK-6-2]	✅ VERIFIED	.github/workflows/lint.yml runs hardcoded ports check

Issues Found

1. Missing: sandbox/egg_lib/context.py update (ac-5)

The contract requires updating sandbox/egg_lib/context.py so that RuntimeContext defaults reference shared constants. This file was not modified in the PR.

Required: RuntimeContext should import defaults from shared constants (GATEWAY_PORT, GATEWAY_PROXY_PORT, etc.) instead of defining them locally or using hardcoded values.

2. Missing: gateway/tests/README-integration.md update (ac-26)

The contract lists this file as needing updates for port consistency, but it was not modified in the PR.

Required: Verify if this file contains port 9847 references that need updating, or document why no changes are needed.

Minor Note

• ac-28 mentions .github/workflows/ci.yml but the implementation is in .github/workflows/lint.yml. This is acceptable as lint.yml is part of CI.

---

— Authored by egg

[james-in-a-box]

Response to Contract Verification Review

ac-5: sandbox/egg_lib/context.py

The file was not modified in this PR because it is already correctly using shared constants. Looking at the implementation:

1. context.py imports from .config (lines 16-28):

   from .config import (
       GATEWAY_PORT,
       GATEWAY_PROXY_PORT,
       ...
   )

2. config.py was updated in this PR to import from egg_config.constants (lines 18-30)

3. RuntimeContext defaults correctly use these imported constants:

   gateway_port: int = GATEWAY_PORT      # line 65
   gateway_proxy_port: int = GATEWAY_PROXY_PORT  # line 66

The design follows a single import chain: egg_config.constants → config.py → context.py. No hardcoded port values exist in context.py (verified via grep).

ac-26: gateway/tests/README-integration.md

Fixed. Updated the JSON example from port 9847 to 9848.

---

— Authored by egg

[james-in-a-box]

Agent-mode design review: No concerns.

This PR is a straightforward infrastructure refactoring—consolidating hardcoded port values into a centralized constants module. It doesn't touch agent prompts, workflows, or output formats.

(Unable to formally approve since this PR was created by the same bot account.)

— Authored by egg

[james-in-a-box]

Code Review: Hardcoded Port Centralization

This PR implements centralized gateway/sandbox port constants, replacing hardcoded port values across the codebase with imports from shared/egg_config/constants.py. The overall approach is sound, but I found several issues that need addressing.

---

Issues Requiring Changes

1. Lint Script False Positive Logic Bug (scripts/check-hardcoded-ports.py:114-117)

The allowlist pattern check at lines 114-117 returns early for any match, but the patterns include r"\d{5,}" which matches "any 5+ digit number". This is supposed to allow port numbers embedded in larger numbers (e.g., 39848), but the logic is flawed:

# Check for allowlisted patterns
for pattern in ALLOWLIST_PATTERNS:
    if re.search(pattern, line):
        return violations  # Returns EMPTY list, skipping all further checks

If a line contains any 5+ digit number anywhere, the entire line is skipped. For example:

# Bug: This line would be skipped entirely because "12345" matches \d{5,}
port = 9848  # used 12345 times

The intended behavior was to skip false positives where ports appear embedded in larger numbers, but the current implementation skips the entire line if any 5+ digit number exists anywhere.

Suggested fix: Move the larger-number check into the port-specific detection logic instead of the general pattern allowlist.

2. Inconsistent Test Port Usage (tests/sandbox/test_gateway_helpers.py:44-45)

The mock context uses TEST_GATEWAY_PORT (1234) for gateway_port, but the gateway_proxy_port uses TEST_GATEWAY_PROXY_PORT (5678). However, the original code at line 45 had:

- ctx.gateway_proxy_port = 3128
+ ctx.gateway_proxy_port = TEST_GATEWAY_PROXY_PORT

Note that 3128 was the old proxy port (Squid default), not 3129 (the production proxy port). This change is technically correct in that it now uses the test constant, but it's masking the fact that the original test was using a stale port value. This is fine if the tests are truly decoupled from production values, but verify that no test logic depends on specific port relationships.

3. Missing Constant for 3128

The comment in shared/egg_container/__init__.py:28 still references {GATEWAY_PROXY_PORT} in a string but wasn't updated to show the actual value:

proxy_url: str | None = None  # e.g. "http://egg-gateway:{GATEWAY_PROXY_PORT}"

This is now misleading—it's a docstring that looks like it should have been interpolated. Either keep the hardcoded example ("http://egg-gateway:3129") or remove the example entirely.

4. Incomplete Integration Test Port Update (integration_tests/conftest.py:49-50)

The diff shows:

-GATEWAY_PORT = 9848
-PROXY_PORT = 3129
+# Use constants from shared module for port configuration
+PROXY_PORT = GATEWAY_PROXY_PORT

GATEWAY_PORT is now imported from egg_config, which is correct. But notice that the local PROXY_PORT variable was kept as an alias to GATEWAY_PROXY_PORT. The code below uses PROXY_PORT directly. This asymmetry is a bit confusing—why alias one but not the other?

More importantly, line 191 does:

[*compose_cmd, "port", "gateway", "9848"],

This hardcoded 9848 should use the imported GATEWAY_PORT constant. The lint script's allowlist includes integration_tests/conftest.py, so this won't be caught automatically.

5. Documentation Comment Drift Risk (gateway/entrypoint.sh:18-22)

The comment added is helpful:

# PORT CONFIGURATION:
# The gateway uses hardcoded port values (9848 for API, 3129 for proxy).
# These are the source of truth - the Python constants in
# shared/egg_config/constants.py must match these values.

However, this creates a documentation maintenance burden. If someone changes the constants in Python, they must also update the shell script comments. Consider adding a test that verifies the shell script ports match the Python constants, or at least add a lint check that validates consistency.

---

Minor Observations (Non-Blocking)

1. sandbox/egg_lib/config.py fallback logic: The try/except import pattern at lines 17-48 is reasonable for development vs. installed package scenarios, but the duplication of the import list is maintenance-prone. Consider extracting to a helper function.

2. Lint script allowlist is extensive: The allowlist in check-hardcoded-ports.py includes 30+ entries. This many exceptions may indicate the lint is too aggressive or that some files should genuinely use constants but were grandfathered in.

3. Contract state file: The 647-line JSON blob in .egg-state/contracts/280.json tracks implementation progress. This is fine for the SDLC pipeline but adds noise to the PR diff.

---

Summary

The core approach is correct: centralize constants in one module and import everywhere else. The implementation is mostly complete, but the issues above—particularly the lint script logic bug (Issue #1) and the hardcoded port in conftest.py line 191 (Issue #4)—should be fixed before merge.

— Authored by egg

[james-in-a-box]

Contract Verification Review (Re-review)

Summary

All 28 acceptance criteria now verified. ✅

This re-review examined changes since commit de1344b05e2db6137d18bccd23742bbcddfa51f2.

Changes Since Last Review

One commit was added:

• a0df2b9: Update README-integration.md port reference from 9847 to 9848

This commit addresses ac-26 which was flagged in the previous review.

Updated Verification Results

Previously Flagged Issues - Now Resolved

Criterion	Previous Status	Current Status	Notes
ac-5 [TASK-2-2]	❌ NOT VERIFIED	✅ VERIFIED	Re-examined: sandbox/egg_lib/context.py correctly imports constants from .config which re-exports from shared module. RuntimeContext defaults use GATEWAY_PORT, GATEWAY_PROXY_PORT, and all network constants.
ac-26 [TASK-5-7]	❌ NOT VERIFIED	✅ VERIFIED	Fixed in commit a0df2b9. gateway/tests/README-integration.md now uses port 9848 consistently (lines 43, 80, 104).

All Criteria Status

All 28 acceptance criteria are verified:

• Phase 1 (ac-1 to ac-3): ✅ Constants module created and exported
• Phase 2 (ac-4 to ac-10): ✅ Sandbox/launcher code updated
• Phase 3 (ac-11 to ac-13): ✅ Shell scripts require GATEWAY_URL
• Phase 4 (ac-14 to ac-19): ✅ Test fixtures use constants
• Phase 5 (ac-20 to ac-26): ✅ Documentation updated with correct ports
• Phase 6 (ac-27 to ac-28): ✅ CI lint check added

Verification Evidence

• Lint check passes: python3 scripts/check-hardcoded-ports.py returns "OK: No hardcoded port numbers found"
• No stale 9847 references: grep confirms only contract JSON mentions 9847 (in acceptance criteria descriptions)
• CI passing: All linters, unit tests, integration tests, and security scan pass

Verdict

All contract requirements are satisfied. Recommend approval.

---

— Authored by egg

[james-in-a-box]

Agent-mode design review: No concerns.

This is pure infrastructure refactoring—centralizing hardcoded port values into shared/egg_config/constants.py and updating consumers to import from there. The changes are mechanical substitutions of literal values with constant references, plus a CI lint check to prevent future drift.

Nothing here affects agent workflows, prompt design, or how the agent interacts with the system.

— Authored by egg

[james-in-a-box]

Re-review: Address code review feedback on port centralization

All issues from my previous review have been addressed:

Verified Fixes

1. Lint script false positive bug (scripts/check-hardcoded-ports.py): Fixed. The \d{5,} pattern has been removed from ALLOWLIST_PATTERNS. The script now correctly uses word boundary checks ((?<!\d) and (?!\d)) at lines 118-126 to avoid matching ports embedded in larger numbers, without incorrectly skipping entire lines that happen to contain 5+ digit numbers elsewhere.

2. Hardcoded port in conftest.py line 191: Fixed. Changed from "9848" to str(GATEWAY_PORT). The logic remains correct: docker compose port gateway 9848 queries the internal container port and returns the random host port mapping.

3. Docstring in egg_container: Fixed. Changed from the confusing # e.g. "http://egg-gateway:{GATEWAY_PROXY_PORT}" (which looked like an uninterpolated f-string) to # Full proxy URL if provided; default uses GATEWAY_PROXY_PORT.

No new issues identified in the changes.

---

— Authored by egg

[james-in-a-box]

Incremental Contract Verification — APPROVED

Commit reviewed: d2522bea911dca788ce1d785e27daeaf91e6993b (delta since a0df2b98)

Delta Changes Verified

The commit "Address code review feedback on port centralization" correctly addresses all previous review feedback:

1. Lint script false positive fix ✓

   • Removed \d{5,} pattern from ALLOWLIST_PATTERNS that incorrectly skipped entire lines containing any 5+ digit number
   • The lint script now correctly only allows specific comment patterns

2. conftest.py constant usage ✓

   • integration_tests/conftest.py now imports GATEWAY_PORT, GATEWAY_PROXY_PORT from egg_config
   • Line 191 uses str(GATEWAY_PORT) instead of hardcoded "9848"
   • Line 50 uses PROXY_PORT = GATEWAY_PROXY_PORT

3. egg_container docstring fix ✓

   • shared/egg_container/__init__.py line 31: docstring now reads "Full proxy URL if provided; default uses GATEWAY_PROXY_PORT" instead of hardcoded :3129

Lint Check Verification

$ python3 scripts/check-hardcoded-ports.py
OK: No hardcoded port numbers found

Contract Compliance

All acceptance criteria from the SDLC contract are met:

• ac-1 through ac-3 (Phase 1): Constants module created with all exports
• ac-4 through ac-10 (Phase 2): Sandbox/launcher code updated to use shared constants
• ac-11 through ac-13 (Phase 3): Shell scripts require GATEWAY_URL with clear error messages
• ac-14 through ac-19 (Phase 4): Test fixtures use TEST_GATEWAY_PORT/GATEWAY_PORT constants
• ac-20 through ac-26 (Phase 5): Documentation updated with correct port 9848
• ac-27 through ac-28 (Phase 6): CI lint check added and functional

No Regressions

Previously verified acceptance criteria remain valid. The delta changes are additive fixes that do not break any existing functionality.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Review Feedback Status

All previous review feedback has been addressed:

Issue	Status	Fix
Lint script false positive bug	✅ Fixed	Removed \d{5,} pattern from allowlist; now uses word boundary checks
Hardcoded port in conftest.py:191	✅ Fixed	Changed to str(GATEWAY_PORT)
Misleading docstring in egg_container	✅ Fixed	Updated comment to reference GATEWAY_PROXY_PORT

All 28 contract acceptance criteria are verified. The hardcoded port lint check passes. No outstanding issues remain.

Ready for human review.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

[jwbron]

@james-in-a-box does #374 handle this issue?

[james-in-a-box]

Analysis: PR #374 does not handle this issue

I've compared PR #356 (issue #280) and PR #374 (issue #367). They address different concerns and are not duplicates.

Issue #280 (this PR #356)

Goal: Eliminate hardcoded ports/URIs across the codebase

• Creates centralized shared/egg_config/constants.py with GATEWAY_PORT, GATEWAY_PROXY_PORT, etc.
• Updates all Python code to import from shared constants
• Updates shell scripts (sandbox/scripts/gh, sandbox/scripts/git) to require GATEWAY_URL env var (no hardcoded fallback)
• Updates documentation to use consistent port 9848
• Adds CI lint check to catch future hardcoded ports

Issue #367 (PR #374)

Goal: Simplify deployment strategy with Docker Compose

• Adds bin/egg-deploy and docker-compose.yml
• Creates orchestration code (sandbox/egg_lib/compose.py, orchestration.py)
• Deprecates start-gateway.sh (adds deprecation warning)
• Does NOT create or use centralized port constants
• Does NOT update hardcoded ports throughout the codebase

File Overlap

The only overlapping file is gateway/start-gateway.sh:

• PR [SDLC] get rid of hardccoded gatway and sandbox ports and uris #356 adds documentation: "These ports are the source of truth; Python constants must match"
• PR [SDLC] rework egg deploy strategy #374 adds deprecation warning: "Use Docker Compose instead"

These are complementary changes, not conflicting.

Conclusion

PR #374 does not handle the port centralization issue. These PRs should be merged independently, and if PR #374 merges first, this PR may need a rebase to resolve the conflict in gateway/start-gateway.sh.

— Authored by egg

[james-in-a-box]

egg review failed. View run logs

[james-in-a-box]

egg contract-verification failed. View run logs

[james-in-a-box]

egg agent-mode-design failed. View run logs