fix(responses): tell TLS hostname mismatches apart from an unreachable provider - #575
Conversation
The first interactive `ocx start` asked with a typed `[y/N]` line. Replace it with an inline selector that shows both choices, moves with the arrow keys, and accepts `y`/`n` directly. No stays highlighted, so Enter alone still declines, and escape or Ctrl-C decline as well. The prompt now also requires `gh auth status` to succeed: a logged-out `gh` cannot fulfil a Yes, so asking would only produce a failure message. Terminals without raw mode fall back to the typed question. Declining remains terminal — no persisted decline state and nothing injected into any model prompt to keep nudging afterwards.
Three changes to the one-time GitHub-star question: - The highlighted choice is now Yes, so Enter accepts. The selector shows which side is highlighted, so Enter never does something unannounced. - The question names its mechanism: starring goes through the user's own `gh` login, so the prompt says so. - When an agent is driving `ocx start`, the prompt is not auto-answered. The agent is told to ask the user and to run the star command only on a yes. The one-time marker stays unwritten, so the real selector still appears on the user's own hand-typed run. Agent detection lives in src/cli/agent-driven.ts and is env-var based: a false positive only postpones the prompt, while a false negative would let an agent spend the user's GitHub identity.
WP1 docs-only cycle. Five decade docs at diff-level precision, ordered by dependency (auth gate -> response layer -> adapter -> PR cleanup) rather than effort: - 010 SSH remote proxy: isLoopbackRequestHost couples loopback identity to port equality, so ssh -L on a different local port 403s the whole /v1/* data plane. The sibling isLoopbackOriginValue already dropped its port check in e4e0612 for the same reason. - 020 issue #553: the same three-line 'Provider unreachable' shape repeats at core.ts 1197/1746/1788; fold into one helper and give ERR_TLS_CERT_ALTNAME_INVALID its own wording plus a verification command. - 030 issue #545: the anthropic adapter prepends the Claude Code identity unconditionally on OAuth, so a classifier that already carries it loses output budget it capped at 64 tokens and retries. - 040 PR #527: its first commit already landed on dev as 9dd3c42, which is what makes the branch dirty; rebase keeping only a64aa58. - 050 PR #557: no code work remains, only a security-boundary decision. No production code changed in this cycle.
Independent read-only review returned GO-WITH-FIXES with 4 blockers. Retires WP4 outright. Its premise was inverted: skipSystemPromptPrefix means Claude Code does NOT prepend its prefix, so the proposed dedup guard could never fire — while the unit tests would have passed by feeding the constant to itself. The compared strings differ anyway, and a maintainer had already ruled on issue #545, from an outbound capture, that removing the OAuth identity block is not a safe fix. Also folds back: - the ssh -g / devcontainer residual risk the research doc carried but the plan had dropped, now stated as accepted risk - the CODEOWNERS security-review boundary on src/server/auth-cors.ts, so WP2 is NEEDS_HUMAN-eligible rather than self-merging - OcxConfig imports from src/types, not src/config (tsc TS2459) - a tsconfig-independent error code cast - a range-diff precondition before skipping 1ba588e on PR #527 - server-auth.test.ts:582-602 named as WP2's must-not-break oracle
The plan assumed 1ba588e could simply be skipped because dev already carries 9dd3c42. Measured: same six files, but dev's version is +122 lines larger (208 vs 86 insertions, 150 differing range-diff lines) — it is a rewrite that absorbed review feedback, not a rename. Skipping the commit blindly and resolving conflicts toward the PR side would have silently reverted those 122 lines. The rebase procedure now pins dev as the canonical side and requires a post-cherry-pick diff against origin/dev to prove the delta is confined to the stale app-server warning.
…e provider A certificate that does not cover the host it was served for surfaced as `Provider unreachable: ERR_TLS_CERT_ALTNAME_INVALID fetching https://...`, which reads as if opencodex had built a wrong endpoint. Issue #553 was filed on that reading — the reporter concluded the adapter was mangling the Copilot URL, and a maintainer could not reproduce it because the URL was correct all along. That error almost always means TLS interception (corporate proxy, VPN, local MITM tooling) or a poisoned DNS answer, none of which the proxy can fix or is at fault for. The message now names the host, says the proxy did not rewrite it, and gives the openssl s_client command that settles which certificate is actually being served. The same three-line shape was duplicated at three catch sites in core.ts, so it moves into one helper. Sites 2 and 3 delegate entirely; site 1 keeps its own `outcome` ternary because that value also feeds recordCodexUpstreamOutcome. Verified byte-identical output against the old expression for TimeoutError, plain Error, empty-message Error, string, undefined, null, object and AbortError. While rewriting the message, redact URL userinfo. A provider base URL can carry credentials as `https://user:token@host/` and the runtime error echoes the URL it was fetching, so the old text handed that straight back to the caller. Both branches now strip it. The code match is anchored to the head of the message rather than a bare substring, so text that merely quotes the code back keeps the generic wording.
📝 WalkthroughWalkthroughThe PR adds decision ledgers and bug-resolution plans, introduces agent-aware interactive CLI confirmation, tightens GitHub CLI readiness checks, and centralizes upstream connection error formatting with TLS-specific diagnostics and tests. ChangesOwner decision ledger
Bug-bundle resolution plans
CLI and server runtime updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StartupFlow
participant StarPrompt
participant InteractiveConfirm
participant GitHubCLI
StartupFlow->>StarPrompt: maybeShowStarPrompt()
StarPrompt->>GitHubCLI: Check binary and authentication
StarPrompt->>StarPrompt: Check agent-driven environment
StarPrompt->>InteractiveConfirm: Request human Yes/No choice
InteractiveConfirm-->>StarPrompt: Return confirmation
StarPrompt-->>StartupFlow: Continue startup
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eef9a86da9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const yes = await interactiveConfirm({ | ||
| question: "\n \x1b[38;5;141m⭐ Enjoying opencodex? Star it on GitHub (via gh)?\x1b[0m", | ||
| defaultYes: true, |
There was a problem hiding this comment.
This changes the first interactive ocx start from default-No to default-Yes, so a bare Enter now mutates the user's GitHub account through authenticated gh; the commit also adds an agent-mediated variant, but no English or translated docs-site/ page explains either behavior. Update the CLI/startup documentation to disclose when the prompt appears, its default, and the account action it performs.
AGENTS.md reference: AGENTS.md:L96-L97
Useful? React with 👍 / 👎.
| const onData = (chunk: Buffer | string) => { | ||
| const key = typeof chunk === "string" ? chunk : chunk.toString("utf8"); | ||
| if (key === KEY_INTERRUPT) return finish(false, true); | ||
| if (key === KEY_ESCAPE) return finish(false, false); | ||
| if (KEY_ENTER.has(key)) return finish(yes, false); |
There was a problem hiding this comment.
Raw TTY data events are arbitrary chunks rather than guaranteed one-key packets, so fast input or pasted input such as n\r or \x1b[D\r is treated as one unknown key and ignored. This can leave startup apparently hung, and a later Enter may accept the still-default Yes selection; split or decode the chunk into individual key sequences before applying the selector logic.
Useful? React with 👍 / 👎.
| return `Provider TLS certificate does not match ${target}: ${redactUrlUserinfo(detail)}. ` | ||
| + "opencodex did not rewrite this hostname — a certificate that does not cover it normally " | ||
| + "means TLS interception (corporate proxy, VPN, or local MITM tooling) or a poisoned DNS " | ||
| + `answer. Check with: openssl s_client -connect ${probe}:443 -servername ${probe} ` |
There was a problem hiding this comment.
When a custom HTTPS provider uses a nonstandard port, such as https://provider.example:8443, extractHostname discards that port and the suggested command probes port 443 instead. It can therefore display a different certificate or fail to connect, defeating the diagnosis; parse and retain the URL's effective port when constructing -connect.
Useful? React with 👍 / 👎.
| if (isAgentDriven()) { | ||
| printAgentDeferral(); | ||
| return; |
There was a problem hiding this comment.
Most coding-agent shell invocations expose stdin/stdout as pipes, so the earlier non-TTY return runs before this new guard and printAgentDeferral() never tells the agent to ask the user. Move agent detection ahead of the interactive-TTY gate while retaining the service and marker checks, and add a runtime regression using agent environment variables with non-TTY streams rather than only inspecting source text.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| return `Provider TLS certificate does not match ${target}: ${redactUrlUserinfo(detail)}. ` | ||
| + "opencodex did not rewrite this hostname — a certificate that does not cover it normally " | ||
| + "means TLS interception (corporate proxy, VPN, or local MITM tooling) or a poisoned DNS " | ||
| + `answer. Check with: openssl s_client -connect ${probe}:443 -servername ${probe} ` |
There was a problem hiding this comment.
For a custom or redirected upstream whose URL hostname contains shell metacharacters, the URL parser accepts values such as $(id).example, and this message inserts that hostname unquoted into a command explicitly offered for the user to run. If the user follows the diagnostic, their shell executes the substitution; validate the extracted value as a DNS name/IP and pass a safely quoted authority and SNI value instead.
Useful? React with 👍 / 👎.
| const auth = spawnSync("gh", ["auth", "status"], { stdio: "ignore", timeout: 5000, windowsHide: true }); | ||
| return !auth.error && auth.status === 0; |
There was a problem hiding this comment.
When gh is installed but logged out, expired, or otherwise returns a nonzero auth status, this new check makes maybeShowStarPrompt() return without writing .star-prompted. Because src/update/notify.ts returns at line 135 whenever hasStarPromptRun() is false, every later startup suppresses update notifications indefinitely; record completion of the first-run yield independently of whether the star action is currently available.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/responses/core.ts (1)
1186-1200: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winResidual duplicate timeout-message branch — finish delegating to the helper here too.
describeUpstreamConnectFailure(src/server/responses/upstream-error.ts) already returns the identicalProvider connect timeout after ${connectMs}msstring whenevererr.name === "TimeoutError". Theoutcome === "timeout"ternary at lines 1196-1197 duplicates that exact branch instead of delegating, unlike the other two call sites in this file (1745, 1785) which were fully consolidated.outcomeis still needed forrecordCodexUpstreamOutcome(1191), butmsgdoesn't need its own copy.♻️ Proposed cleanup
- const msg = outcome === "timeout" - ? `Provider connect timeout after ${connectMs}ms` - : describeUpstreamConnectFailure(err, connectMs); + const msg = describeUpstreamConnectFailure(err, connectMs);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/responses/core.ts` around lines 1186 - 1200, Update transportFailureResponse so msg is always produced by describeUpstreamConnectFailure(err, connectMs), removing the duplicated outcome === "timeout" ternary branch. Preserve outcome for recordCodexUpstreamOutcome and leave the existing response formatting unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260727_owner_decision_ledger/008_wsl_ssh_report_intake.md`:
- Around line 18-24: Revise the decision framing in 008_wsl_ssh_report_intake.md
so C1 and C2 are treated as independently possible rather than mutually
exclusive. Update the follow-up branching plan around the referenced decision
section to investigate the login path and data-plane behavior in parallel,
ensuring evidence for one failure does not exclude checking the other.
- Around line 3-4: Update the status statement in the ledger to distinguish the
newly reported WSL SSH issue, which has no repository registration, from issue
`#131` identified later as a related historical issue. Preserve the existing
intake details and make the wording consistent with the related-issue reference.
In `@devlog/_plan/260727_owner_decision_ledger/010_bug_bundle_fixability.md`:
- Around line 95-104: Revise the `#553` entry to avoid claiming that the URL
configuration is correct based solely on the *.githubcopilot.com allowlist in
github-copilot.ts. State only that the reported hostname is allowed, while
endpoint path, URL construction, and adapter rewriting remain unverified; retain
the proposed TLS-specific diagnostic messaging separately.
- Around line 3-16: Update the open-bug tally in the “한눈에” summary to account
for issue `#570`, which is dated 2026-07-28 and labeled bug in
009_ssh_remote_proxy_rootcause.md; either include it and recalculate all
affected totals and recommendations, or state the exact measurement cutoff that
excludes it.
In `@devlog/_plan/260728_bug_bundle_resolution/000_plan.md`:
- Around line 91-98: Remove the WP4 어댑터 row from the “SoT 동기화 대상 (SOT-SYNC-01)”
table, or mark it explicitly as N/A. Ensure the plan no longer instructs
operators to inspect Anthropic adapter documentation for the retired WP4 work.
In `@devlog/_plan/260728_bug_bundle_resolution/010_ssh_loopback_gate.md`:
- Around line 121-137: Do not merge the port-removal change in
isLoopbackRequestHost as a routine consistency fix; require an explicit
security/product decision for forwarded loopback exposure and implement a
mitigation, such as authentication for forwarded access or a distinct forwarding
mode. Add a regression test covering ssh -g -L exposure to LAN clients with Host
localhost:20100 and no Origin, ensuring unauthenticated management/data-plane
access is rejected.
In `@devlog/_plan/260728_bug_bundle_resolution/020_tls_altname_diagnosis.md`:
- Line 62: Update the TLS error classification condition to replace the
unanchored detail.includes check with a start-of-message check for
ERR_TLS_CERT_ALTNAME_INVALID, while preserving the exact err.code ===
"ERR_TLS_CERT_ALTNAME_INVALID" branch.
- Around line 60-71: Sanitize URL userinfo in the error-detail handling before
constructing either response from the relevant error-formatting function, while
preserving hostname extraction and existing TLS/generic wording. Add a
regression test covering provider errors containing URLs with username and
password, asserting neither credential appears in the returned message.
In `@devlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.md`:
- Around line 88-105: Replace the blanket “dev is canonical” conflict rule in
the cherry-pick procedure with a symbol- and test-level comparison of every
unique hunk from 1ba588eff and a64aa5856 against 9dd3c42da. Resolve each
conflict by preserving all intended behavior, including stale app-server warning
handling and catalog-write behavior, then verify with relevant tests and git
diff origin/dev -- src/codex/ that no unique changes were dropped.
In `@devlog/_plan/260728_bug_bundle_resolution/050_pr557_boundary.md`:
- Around line 93-102: Update the acceptance criteria in section c6 so the DONE
path requires recorded evidence of the second maintainer security approval, not
merely PR MERGED/CLOSED states. Require inspection and documentation of the
approval evidence, or an explicit human security decision recorded as
NEEDS_HUMAN with its rationale; ensure the D summary identifies the actual
determination.
In `@src/cli/interactive-confirm.ts`:
- Around line 33-38: Replace the hand-rolled per-chunk escape matching in onData
with node:readline’s emitKeypressEvents-based keypress parsing, preserving the
existing yes/no, Enter, Escape, interrupt, and cleanup behavior. Ensure split
escape sequences are buffered until the complete key is identified, and add a
regression test in interactive-confirm tests that writes \x1b and [D across
separate input.write calls.
In `@src/cli/star-prompt.ts`:
- Around line 25-36: Reorder the guards in the star-prompt flow so
isAgentDriven() is evaluated before ghAvailable(), returning through the
existing deferral path immediately for agent-driven runs. Ensure ghAvailable()
and its synchronous gh probes execute only for non-agent-driven runs, without
changing the existing behavior for interactive prompts.
In `@src/server/responses/upstream-error.ts`:
- Line 22: Update both return paths in the upstream error response to sanitize
detail with the shared redactSecretString utility before interpolation, while
preserving redactUrlUserinfo for URL-specific masking. Follow the existing
sanitization pattern in core.ts and ensure no unsanitized detail reaches the 502
response body.
---
Outside diff comments:
In `@src/server/responses/core.ts`:
- Around line 1186-1200: Update transportFailureResponse so msg is always
produced by describeUpstreamConnectFailure(err, connectMs), removing the
duplicated outcome === "timeout" ternary branch. Preserve outcome for
recordCodexUpstreamOutcome and leave the existing response formatting unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9d5f7e76-80fa-4a98-8ad5-24d3adfb8a5d
📒 Files selected for processing (21)
devlog/_plan/260727_owner_decision_ledger/000_scope.mddevlog/_plan/260727_owner_decision_ledger/007_delta_260728.mddevlog/_plan/260727_owner_decision_ledger/008_wsl_ssh_report_intake.mddevlog/_plan/260727_owner_decision_ledger/009_ssh_remote_proxy_rootcause.mddevlog/_plan/260727_owner_decision_ledger/010_bug_bundle_fixability.mddevlog/_plan/260728_bug_bundle_resolution/000_plan.mddevlog/_plan/260728_bug_bundle_resolution/010_ssh_loopback_gate.mddevlog/_plan/260728_bug_bundle_resolution/020_tls_altname_diagnosis.mddevlog/_plan/260728_bug_bundle_resolution/030_claude_system_dedup.mddevlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.mddevlog/_plan/260728_bug_bundle_resolution/050_pr557_boundary.mdsrc/cli/agent-driven.tssrc/cli/index.tssrc/cli/interactive-confirm.tssrc/cli/star-prompt.tssrc/server/responses/core.tssrc/server/responses/upstream-error.tstests/agent-driven.test.tstests/interactive-confirm.test.tstests/startup-prompt.test.tstests/upstream-connect-error.test.ts
| 접수: 2026-07-28, 오너 구두 제보 — "Windows WSL에서 SSH가 꼬이는 버그가 있다더라" | ||
| 상태: **저장소에 등록된 흔적 없음**. 이슈·PR·devlog 어디에도 없다. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Distinguish the unregistered report from the existing related issue.
Lines 3-4 say there is no repository trace, but Lines 18-22 identify #131 as the matching existing issue. Clarify that the exact new report is unregistered while #131 is a related historical issue; otherwise the ledger contradicts itself.
Proposed wording
-상태: **저장소에 등록된 흔적 없음**. 이슈·PR·devlog 어디에도 없다.
+상태: **이 exact report is not registered**. Related issue `#131` exists and may cover the same regression.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 접수: 2026-07-28, 오너 구두 제보 — "Windows WSL에서 SSH가 꼬이는 버그가 있다더라" | |
| 상태: **저장소에 등록된 흔적 없음**. 이슈·PR·devlog 어디에도 없다. | |
| 접수: 2026-07-28, 오너 구두 제보 — "Windows WSL에서 SSH가 꼬이는 버그가 있다더라" | |
| 상태: **이 exact report is not registered**. Related issue `#131` exists and may cover the same regression. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260727_owner_decision_ledger/008_wsl_ssh_report_intake.md`
around lines 3 - 4, Update the status statement in the ledger to distinguish the
newly reported WSL SSH issue, which has no repository registration, from issue
`#131` identified later as a related historical issue. Preserve the existing
intake details and make the wording consistent with the related-issue reference.
| **미등록이 아니다.** #131이 바로 "원격 GUI·SSH·loopback 차단 환경에서 로그인을 | ||
| 끝낼 방법이 없다"는 이슈이고, 07-15에 닫혔다. 따라서 이번 제보는 둘 중 하나다: | ||
|
|
||
| - **#131의 회귀** — 고쳐진 경로가 다시 깨졌다 | ||
| - **다른 버그** — #131이 덮지 않는 별개 증상 | ||
|
|
||
| 어느 쪽인지가 다음 행동을 완전히 가른다. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not treat C1 and C2 as mutually exclusive.
009_ssh_remote_proxy_rootcause.md shows that the login path and data plane can fail independently. The current either/or framing, followed by the branching plan in Lines 75-78, can cause one failure to be missed. Investigate both dimensions in parallel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260727_owner_decision_ledger/008_wsl_ssh_report_intake.md`
around lines 18 - 24, Revise the decision framing in
008_wsl_ssh_report_intake.md so C1 and C2 are treated as independently possible
rather than mutually exclusive. Update the follow-up branching plan around the
referenced decision section to investigate the login path and data-plane
behavior in parallel, ensuring evidence for one failure does not exclude
checking the other.
| 측정: 2026-07-28, `origin/dev` = `461de3961` | ||
| 범위: `bug` 라벨이 붙은 **열린 PR 6건 + 열린 이슈 7건** | ||
| 방법: 두 개의 read-only 판정 워커 병렬 + 주요 주장 직접 재확인 | ||
|
|
||
| ## 한눈에 | ||
|
|
||
| | 구분 | 총 | 우리가 지금 할 수 있음 | 보안 리뷰 필요 | 의존/차단 | | ||
| | --- | --- | --- | --- | --- | | ||
| | PR | 6 | **1** (#527) | 3 (#557, #447, #429) | 2 (#533, #528) | | ||
| | 이슈 | 7 | **2** (#553, #545 — 부분) | 0 | 5 | | ||
|
|
||
| **즉답: 지금 당장 우리 손으로 진행 가능한 건 3건이다** — PR #527, 이슈 #553, | ||
| 이슈 #545. 여기에 #557은 코드 작업이 0이고 보안 경계 판단만 남았으므로, | ||
| 오너가 그 판단을 내리면 즉시 4건이 된다. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files 'devlog/_plan/260727_owner_decision_ledger/*' | sed -n '1,20p'
printf '\n== 010 snippet ==\n'
cat -n devlog/_plan/260727_owner_decision_ledger/010_bug_bundle_fixability.md | sed -n '1,180p'
printf '\n== search for `#570` and measurement metadata ==\n'
rg -n '`#570`|570|측정:|origin/dev|bug 라벨|open PR|열린 이슈|열린 PR' devlog/_plan/260727_owner_decision_ledger/*.mdRepository: lidge-jun/opencodex
Length of output: 10843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- 010 ---'
sed -n '1,140p' devlog/_plan/260727_owner_decision_ledger/010_bug_bundle_fixability.md | nl -ba
echo '--- 009 grep ---'
rg -n '`#570`|570|bug' devlog/_plan/260727_owner_decision_ledger/009_ssh_remote_proxy_rootcause.mdRepository: lidge-jun/opencodex
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n devlog/_plan/260727_owner_decision_ledger/009_ssh_remote_proxy_rootcause.md | sed -n '1,60p'Repository: lidge-jun/opencodex
Length of output: 2458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n devlog/_plan/260727_owner_decision_ledger/009_ssh_remote_proxy_rootcause.md | sed -n '1,80p'Repository: lidge-jun/opencodex
Length of output: 3265
Include #570 in the open-bug tally, or record the cutoff that excludes it.
devlog/_plan/260727_owner_decision_ledger/009_ssh_remote_proxy_rootcause.md:3-7 dates #570 to 2026-07-28 and labels it bug, but devlog/_plan/260727_owner_decision_ledger/010_bug_bundle_fixability.md:3-16 still counts only 7 open issues and never accounts for it. Add #570 and recalculate the totals/recommendation order, or state the exact measurement cutoff used to leave it out.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260727_owner_decision_ledger/010_bug_bundle_fixability.md`
around lines 3 - 16, Update the open-bug tally in the “한눈에” summary to account
for issue `#570`, which is dated 2026-07-28 and labeled bug in
009_ssh_remote_proxy_rootcause.md; either include it and recalculate all
affected totals and recommendations, or state the exact measurement cutoff that
excludes it.
| **#553** GitHub Copilot 502 / TLS 호스트명 불일치 · `FIXABLE-PARTIAL` | ||
| 우리 URL 구성은 옳다 — `src/oauth/github-copilot.ts:136`이 `*.githubcopilot.com`을 | ||
| 허용하므로 신고된 호스트는 정상이다. 진짜 원인은 리포터 환경의 TLS/DNS(가로채기, | ||
| VPN, 프록시)일 가능성이 높고 그건 우리 것이 아니다. | ||
|
|
||
| **우리가 할 수 있는 것**: 지금 `Provider unreachable:` 문자열이 | ||
| `src/server/responses/core.ts`의 세 지점(1197, 1746, 1788)에서 똑같이 나온다. | ||
| `ERR_TLS_CERT_ALTNAME_INVALID`를 따로 분기해 DNS/VPN/TLS 가로채기를 지목하고 | ||
| `openssl s_client` 복구 명령을 주면, 지금처럼 "어댑터 URL 버그"로 읽히지 않는다. | ||
| 사용자 대면 오류가 정확한 복구 명령을 담아야 한다는 우리 원칙에 정확히 맞는 일이다. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not infer correct URL construction from the hostname allowlist.
Allowing *.githubcopilot.com proves only that the hostname passes validation; it does not prove the endpoint path, URL construction, or absence of adapter rewriting. The PR objective is diagnostic clarification, not confirmation that the underlying URL bug is impossible. Rephrase this as “the hostname is allowed; URL construction remains separately unverified.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260727_owner_decision_ledger/010_bug_bundle_fixability.md`
around lines 95 - 104, Revise the `#553` entry to avoid claiming that the URL
configuration is correct based solely on the *.githubcopilot.com allowlist in
github-copilot.ts. State only that the reported hostname is allowed, while
endpoint path, URL construction, and adapter rewriting remain unverified; retain
the proposed TLS-specific diagnostic messaging separately.
| ## SoT 동기화 대상 (SOT-SYNC-01) | ||
|
|
||
| | 변경 | 패치할 SoT | | ||
| | --- | --- | | ||
| | WP2 원격 접근 | `docs-site/src/content/docs/reference/configuration.md` "Remote access" 절 | | ||
| | WP3 오류 메시지 | 해당 없음 (오류 문자열은 코드가 SoT) | | ||
| | WP4 어댑터 | `structure/` 내 anthropic 어댑터 불변식 문서가 있으면 확인 | | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the discarded WP4 synchronization task.
WP4 is explicitly retired at Lines 54-62, but this table still instructs the operator to inspect Anthropic adapter documentation for WP4. Mark this row as N/A or remove it so the execution plan cannot reintroduce discarded work.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260728_bug_bundle_resolution/000_plan.md` around lines 91 - 98,
Remove the WP4 어댑터 row from the “SoT 동기화 대상 (SOT-SYNC-01)” table, or mark it
explicitly as N/A. Ensure the plan no longer instructs operators to inspect
Anthropic adapter documentation for the retired WP4 work.
| 파일 목록은 6개로 **동일**하다: | ||
| `src/codex/catalog/sync.ts`, `src/codex/refresh.ts`, `src/codex/sync.ts`, | ||
| `tests/codex-refresh.test.ts`, `tests/codex-sync-api.test.ts`, | ||
| `tests/injection-model-api.test.ts`. | ||
|
|
||
| 그런데 dev의 `9dd3c42da`가 **+122줄 더 많다.** range-diff 차이 라인 150줄. | ||
| 즉 `9dd3c42da`는 `1ba588eff`의 재작성본이며 그 **상위집합**으로 보인다 | ||
| (리뷰 반영으로 커버리지가 늘어난 형태). | ||
|
|
||
| **따라서 계획의 "그냥 건너뛴다"는 그대로 실행하면 안 된다.** 수정된 절차: | ||
|
|
||
| 1. `a64aa5856`만 체리픽하되, **충돌이 나는 것이 정상**이다 — `a64aa5856`는 | ||
| `1ba588eff` 위에 쓰였고 dev에는 그 상위집합이 있다. | ||
| 2. 충돌 해소 시 **dev(`9dd3c42da`) 쪽을 무조건 정본**으로 삼는다. `1ba588eff` | ||
| 버전의 코드가 남으면 리뷰 반영분이 되돌려진다. | ||
| 3. `a64aa5856`가 **새로 추가하는 것만** 얹는다 — stale app-server 경고. | ||
| 4. 체리픽 후 `git diff origin/dev -- src/codex/` 로 실제 델타가 stale 경고에만 | ||
| 국한되는지 확인한다. `9dd3c42da`의 라인이 사라졌으면 잘못된 해소다. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not infer semantic equivalence from the diffstat.
The range-diff explicitly shows that 9dd3c42da and 1ba588eff are not equivalent. A larger diff does not prove that 9dd3c42da is a semantic superset, so always choosing the dev side during conflict resolution can silently discard unique behavior from 1ba588eff or a64aa5856.
Require a symbol- and test-level comparison of every unique hunk before resolving conflicts, then verify that git diff origin/dev -- src/codex/ contains all intended stale-warning behavior and no dropped catalog-write behavior.
🧰 Tools
🪛 LanguageTool
[grammar] ~102-~102: Ensure spelling is correct
Context: ...삼는다. 1ba588eff 버전의 코드가 남으면 리뷰 반영분이 되돌려진다. 3. a64aa5856가 새로 추가하는 것만 얹는다 — s...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.md` around lines
88 - 105, Replace the blanket “dev is canonical” conflict rule in the
cherry-pick procedure with a symbol- and test-level comparison of every unique
hunk from 1ba588eff and a64aa5856 against 9dd3c42da. Resolve each conflict by
preserving all intended behavior, including stale app-server warning handling
and catalog-write behavior, then verify with relevant tests and git diff
origin/dev -- src/codex/ that no unique changes were dropped.
| ## 수용 기준 (c6) | ||
|
|
||
| 둘 중 하나: | ||
|
|
||
| - `gh pr view 557 --json state,mergedAt` → MERGED **그리고** | ||
| `gh pr view 533 --json state` → CLOSED (크레딧 코멘트 포함) | ||
| - 또는 보안 경계 판단이 `NEEDS_HUMAN`으로 기록되고 그 근거가 남음 | ||
|
|
||
| 두 번째 경로도 정당한 종료다. 판단을 회피한 것이 아니라 **권한 경계를 지킨 | ||
| 것**이며, D 요약에 실제 판정으로 명시한다. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require maintainer-approval evidence in the DONE criteria.
MERGED and CLOSED do not prove that the required second maintainer security review occurred; this document already records zero approvals and cites a prior merge that violated the intended governance boundary. The success path must inspect and record the required approvals or an explicit human security decision, rather than relying only on PR state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260728_bug_bundle_resolution/050_pr557_boundary.md` around lines
93 - 102, Update the acceptance criteria in section c6 so the DONE path requires
recorded evidence of the second maintainer security approval, not merely PR
MERGED/CLOSED states. Require inspection and documentation of the approval
evidence, or an explicit human security decision recorded as NEEDS_HUMAN with
its rationale; ensure the D summary identifies the actual determination.
| const KEY_ENTER = new Set(["\r", "\n"]); | ||
| const KEY_YES_SIDE = new Set(["\x1b[D", "\x1b[A", "\x1bOD", "\x1bOA"]); // left / up | ||
| const KEY_NO_SIDE = new Set(["\x1b[C", "\x1b[B", "\x1bOC", "\x1bOB"]); // right / down | ||
| const KEY_ESCAPE = "\x1b"; | ||
| const KEY_INTERRUPT = "\x03"; | ||
| const KEY_TAB = "\t"; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Hand-rolled escape-sequence matching can misfire on split chunks.
onData treats each data event as one complete key (line 105-106). If \x1b and its trailing bytes ([D, [C, …) ever arrive in separate chunks — plausible over a laggy SSH/pty — KEY_ESCAPE (line 107) fires first and declines before the arrow-key bytes are read, since finish() already removed the data listener. node:readline's built-in emitKeypressEvents() already solves this exact ambiguity with a completion timer, and this file already depends on node:readline/promises for the fallback path, so swapping to it is a natural, low-risk change.
♻️ Sketch: use readline's keypress parser instead of raw byte matching
-import { createInterface } from "node:readline/promises";
+import { createInterface } from "node:readline/promises";
+import { emitKeypressEvents } from "node:readline";
@@
- const onData = (chunk: Buffer | string) => {
- const key = typeof chunk === "string" ? chunk : chunk.toString("utf8");
- if (key === KEY_INTERRUPT) return finish(false, true);
- if (key === KEY_ESCAPE) return finish(false, false);
- if (KEY_ENTER.has(key)) return finish(yes, false);
- const lower = key.toLowerCase();
- if (lower === "y") return finish(true, false);
- if (lower === "n") return finish(false, false);
- if (KEY_YES_SIDE.has(key)) { yes = true; paint(); return; }
- if (KEY_NO_SIDE.has(key) || key === KEY_TAB) { yes = key === KEY_TAB ? !yes : false; paint(); return; }
- };
+ emitKeypressEvents(input);
+ const onKeypress = (_str: string, key: { name?: string; sequence: string; ctrl?: boolean }) => {
+ if (key.ctrl && key.name === "c") return finish(false, true);
+ if (key.name === "escape") return finish(false, false);
+ if (key.name === "return" || key.name === "enter") return finish(yes, false);
+ if (key.name === "y") return finish(true, false);
+ if (key.name === "n") return finish(false, false);
+ if (key.name === "left" || key.name === "up") { yes = true; paint(); return; }
+ if (key.name === "right" || key.name === "down") { yes = false; paint(); return; }
+ if (key.name === "tab") { yes = !yes; paint(); return; }
+ };
@@
input.setRawMode(true);
input.resume();
- input.on("data", onData);
+ input.on("keypress", onKeypress);
paint();Worth a small regression test in tests/interactive-confirm.test.ts simulating a split \x1b / [D across two input.write() calls to lock in whichever fix is chosen.
Also applies to: 104-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/interactive-confirm.ts` around lines 33 - 38, Replace the hand-rolled
per-chunk escape matching in onData with node:readline’s
emitKeypressEvents-based keypress parsing, preserving the existing yes/no,
Enter, Escape, interrupt, and cleanup behavior. Ensure split escape sequences
are buffered until the complete key is identified, and add a regression test in
interactive-confirm tests that writes \x1b and [D across separate input.write
calls.
| /** | ||
| * Whether `gh` is both installed and logged in. Starring goes through the | ||
| * user's own `gh` auth, so an unauthenticated CLI cannot fulfil a "Yes" — in | ||
| * that case the prompt stays silent instead of asking for something it would | ||
| * then fail to do. | ||
| */ | ||
| function ghAvailable(): boolean { | ||
| const r = spawnSync("gh", ["--version"], { stdio: "ignore", timeout: 3000, windowsHide: true }); | ||
| return !r.error && r.status === 0; | ||
| const version = spawnSync("gh", ["--version"], { stdio: "ignore", timeout: 3000, windowsHide: true }); | ||
| if (version.error || version.status !== 0) return false; | ||
| const auth = spawnSync("gh", ["auth", "status"], { stdio: "ignore", timeout: 5000, windowsHide: true }); | ||
| return !auth.error && auth.status === 0; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
gh --version
gh auth status --helpRepository: lidge-jun/opencodex
Length of output: 2300
🏁 Script executed:
sed -n '1,140p' src/cli/star-prompt.tsRepository: lidge-jun/opencodex
Length of output: 4490
🏁 Script executed:
rg -n "MARKER|maybeShowStarPrompt|ghAvailable|isAgentDriven|star prompt" src/cli/star-prompt.ts src -g '!**/dist/**'Repository: lidge-jun/opencodex
Length of output: 9319
🌐 Web query:
gh auth status network call documentation CLI auth status blocked DNS hangs
💡 Result:
When gh auth status reports an authentication failure in environments with blocked DNS or network access, the issue is often a transport-level connectivity problem—such as a DNS resolution failure, connection timeout, or proxy obstruction—being incorrectly identified as an invalid credential issue [1][2][3]. Key details regarding this behavior and how to troubleshoot it include: Underlying Causes The GitHub CLI (gh) relies on the Go standard library for network requests [4]. If DNS resolution fails, connections time out, or corporate firewalls/proxies intercept traffic, these transport-level failures may cause gh auth status to report that the token is invalid [1][2]. This misleading output often incorrectly suggests re-authentication via gh auth login, which will also fail if the underlying connectivity issue persists [2][3]. Diagnostic Steps To confirm whether the issue is a network/transport failure rather than an invalid credential: 1. Use Debugging Output: Run the command with GH_DEBUG=api to inspect the underlying HTTPS requests and identify specific transport errors (e.g., dial tcp timeouts, DNS lookup failures) [2][5]. 2. Machine-Readable Output: Use gh auth status --json hosts to view structured data, which may provide more clarity than the default human-readable output [2]. 3. Isolate the Network: Verify connectivity by testing access to GitHub endpoints independently of the CLI, such as using curl -vvv https://api.github.com [3][6][7]. 4. Check Environment Variables: Ensure proxy settings (HTTPS_PROXY, NO_PROXY) are correctly configured if you are behind a corporate firewall [5][8][4]. Note that gh respects standard environment variables consumed by Go's networking stack [4]. Status Updates The GitHub CLI development team has been actively addressing this, with recent updates working to improve error classification in gh auth status to explicitly distinguish network and timeout errors from invalid credential states [1][9][10]. If you are experiencing this in a restricted environment (e.g., CI, containers, or corporate networks), these updates aim to provide more accurate feedback to prevent unnecessary re-authentication attempts [2]. For further connectivity troubleshooting, refer to official documentation on diagnosing network-related issues with GitHub [11].
Citations:
- 1: Fix auth status messaging for network failures cli/cli#12878
- 2: gh auth status should distinguish transport failures from invalid credentials cli/cli#12891
- 3: Connection timeout is incorrectly reported as invalid token cli/cli#8293
- 4: gh cli does not consider no_proxy cli/cli#5244
- 5: cannot authenticate through cli, with no error to debug on dnf(linux) based system cli/cli#4083
- 6: Error: read: connection reset by peer for any action cli/cli#9130
- 7: Github api : connection timeout cli/cli#8328
- 8: Document proxy support cli/cli#2037
- 9: https://github.com/cli/cli/blob/trunk/pkg/cmd/auth/status/status.go
- 10: Add timeout error in
gh auth statuscli/cli#8337 - 11: https://docs.github.com/en/get-started/using-github/troubleshooting-connectivity-problems
Move isAgentDriven() ahead of ghAvailable()
In src/cli/star-prompt.ts:73-87, the CLI runs ghAvailable() first, so agent-driven starts still pay for spawnSync("gh", ["--version"]) and spawnSync("gh", ["auth", "status"]) before the deferral branch can return. That blocks startup on an auth probe the agent path will never use, and gh auth status can also stall or misreport on restricted networks. Reorder the guard so agent-driven runs skip the gh check entirely; if you still need the probe elsewhere, cache it briefly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/star-prompt.ts` around lines 25 - 36, Reorder the guards in the
star-prompt flow so isAgentDriven() is evaluated before ghAvailable(), returning
through the existing deferral path immediately for agent-driven runs. Ensure
ghAvailable() and its synchronous gh probes execute only for non-agent-driven
runs, without changing the existing behavior for interactive prompts.
| const host = extractHostname(detail); | ||
| const target = host ?? "the provider host"; | ||
| const probe = host ?? "<host>"; | ||
| return `Provider TLS certificate does not match ${target}: ${redactUrlUserinfo(detail)}. ` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# Check whether any provider transport embeds credentials as URL query parameters
rg -nP '(apiKey|api_key|access_token|[?&]key=)' src/providers src/codex --type=ts -C2Repository: lidge-jun/opencodex
Length of output: 20046
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/server/responses/upstream-error.ts ==\n'
wc -l src/server/responses/upstream-error.ts
cat -n src/server/responses/upstream-error.ts
printf '\n== redact utilities / usages ==\n'
rg -n "redactUrlUserinfo|redactSecretString" src -C 2
printf '\n== surrounding upstream error handling ==\n'
rg -n "upstream_error|Provider TLS certificate does not match|Provider unreachable" src -C 3Repository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/server/responses/upstream-error.ts
printf '\n---\n'
rg -n "export function redactSecretString|function redactSecretString|redactSecretString" src -C 3
printf '\n---\n'
rg -n "export function redactUrlUserinfo|function redactUrlUserinfo|redactUrlUserinfo" src -C 3Repository: lidge-jun/opencodex
Length of output: 41566
Broaden upstream error redaction. Both return paths here only strip user:pass@, so any secret echoed elsewhere in detail can still reach the 502 body. Wrap detail with the shared sanitizer (redactSecretString at minimum) before interpolating it here, like the other upstream error paths in src/server/responses/core.ts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/responses/upstream-error.ts` at line 22, Update both return paths
in the upstream error response to sanitize detail with the shared
redactSecretString utility before interpolation, while preserving
redactUrlUserinfo for URL-specific masking. Follow the existing sanitization
pattern in core.ts and ensure no unsanitized detail reaches the 502 response
body.
1 participant
Closes nothing on its own — see the note on #553 at the end.
Summary
A certificate that does not cover the host it was served for surfaced as:
That reads as if opencodex built a wrong endpoint, and #553 was filed on exactly that reading — the reporter concluded the adapter was mangling the Copilot URL. It was not:
src/oauth/github-copilot.ts:136allows*.githubcopilot.com, so the host was correct, and a maintainer could not reproduce the failure.ERR_TLS_CERT_ALTNAME_INVALIDalmost always means TLS interception (corporate proxy, VPN, local MITM tooling) or a poisoned DNS answer — none of which the proxy causes or can fix.The message now names the host, states that the proxy did not rewrite it, and gives the command that settles which certificate is actually being served:
Refactor and its safety
The same three-line shape was duplicated at three catch sites in
core.ts(1197, 1746, 1788), so it moves into one helper. Sites 2 and 3 delegate entirely; site 1 keeps its ownoutcometernary because that value also feedsrecordCodexUpstreamOutcome— andoutcomeis derived from the sameerr.name === "TimeoutError"predicate atcore.ts:1189, so the two cannot disagree.Equivalence was checked rather than assumed. The old inline expression was replayed against the helper for eight input classes —
TimeoutError, plainError, empty-messageError, string,undefined,null, a plain object, andAbortError— with zero differences on every non-TLS input.Credential redaction
While rewriting this message: a provider base URL can carry credentials as
https://user:token@host/, and the runtime error echoes the URL it was fetching. The old text handed that straight back to the caller. Measured before the fix:Both branches now strip userinfo, so the detail renders as
<redacted>@api.example.com.The code match is also anchored to the head of the message rather than a bare substring, so text that merely quotes the code back keeps the generic wording. Only transport failures reach these call sites, so that is defensive rather than load-bearing, but it is pinned by a test.
Tests
Activation evidence, not just a green suite — neutering the TLS branch to
if (false):The fixture shape was validated against a real rejection rather than an invented one:
Regression and gates:
On issue #553
This does not close #553. The reporter's TLS failure is still almost certainly local to their network, and that part is unchanged — we cannot fix a certificate we do not serve. What changes is that the next person to hit it is pointed at their own TLS path with a command to confirm it, instead of at a proxy bug that does not exist. #553 stays open under
needs-infofor the reporter's environment check.Planning and evidence:
devlog/_plan/260728_bug_bundle_resolution/020_tls_altname_diagnosis.md.Summary by CodeRabbit
New Features
Documentation
Tests