Skip to content
This repository was archived by the owner on Aug 24, 2026. It is now read-only.

Fix false-green network failures + robustness gaps (review round 2) - #22

Merged
amazon7737 merged 1 commit into
mainfrom
fix/review-round2-network-classification
Jun 1, 2026
Merged

amazon7737 merged 1 commit into
mainfrom
fix/review-round2-network-classification

Conversation

@amazon7737

Copy link
Copy Markdown
Member

배경

멀티 에이전트 코드리뷰(5개 차원 + 적대적 검증, 설치된 chrome-devtools-mcp@1.1.0 소스 대조)에서 나온 확정 발견을 수정합니다. 각 수정은 실제 dep 소스로 검증했고, 변경분은 자기 적대적 검증(회귀/vacuous 테스트 탐지)도 통과했습니다.

핵심 버그 — transport 실패가 PASS로 처리됨 (false-green) 🔴

NetworkFormatter.#getStatusFromRequest는 status를 문자열로 방출합니다:

  • 응답 도착 시 숫자 코드("200", "502")
  • transport 실패 시 failure.errorText ("net::ERR_CONNECTION_REFUSED", DNS 실패, 차단)
  • 진행 중이면 "pending"

기존 Number(status) >= 500/400 비교는 Number('net::ERR_...') → NaN이라 둘 다 false. 즉 요청이 완전히 실패해도 failure도 warning도 아님 → analyzeQuality가 'pass' 반환, exit 0. API가 전부 연결 거부되는 최악의 경우가 CI에서 초록불로 통과했습니다. assert-no-http-errors도 동일한 NaN 맹점이 있었습니다.

→ 공유 헬퍼 classifyNetworkStatus()(신규 core/network.mjs)로 두 경로를 통일해 drift를 방지했습니다.

그 외 수정

영역 내용
devtools-client execFile에 maxBuffer(64 MiB) — 대형 snapshot/network/console JSON이 Node 1 MiB 기본값에서 런을 크래시시키던 문제
runner post-scenario 수집(console/network/lighthouse)을 개별 격리 — Lighthouse 타임아웃 하나가 전체 리포트를 폐기하던 문제. writeReport를 finally로 옮겨 항상 best-effort 저장
cli --timeout 검증(값 없음/NaN/≤0 거부), profile.name/--out 경로 방어(cwd 상위 디렉터리 fs.rm 방지)
quality favicon-404 무시를 URL pathname으로 비교(쿼리스트링/캐시버스터 대응), 콘솔 assert 타입도 surface
snapshot findBySpec가 양성 매처(role/nameIncludes) 필수 — 누락 셀렉터가 root에 매칭되던 문제
chatbot empty-input에 answerDoneText 카운트 오라클 추가(노드 재사용으로 빈 입력을 echo하는 앱 탐지)
profile consent 시나리오에 consentAgreeButton 필수, viewport 형식·maxNodeDelta 정수 검증
docs 공통 시나리오 필드, consent 필수, screenshot/fileName, transport 실패 처리 문서화

검증

  • npm run check 통과, npm test 31/31 pass (+16 신규: 네트워크 분류기, 시나리오 오라클, 프로필 검증, findBySpec 가드, transport 실패 회귀)
  • 동봉 프로필 3종 모두 강화된 검증 통과
  • 변경분 적대적 자기검증(outDir 가드/분류기/runner finally/timeout) 4개 항목 clean

리뷰에서 false positive로 걸러낸 것 (수정 안 함)

페이지네이션 기본값(전체 반환이 맞음), 이미 수정된 answerDoneText/maxNodeDelta/pollSnapshot/parseJsonOutput, README drift 등은 거짓으로 확인되어 제외.

Multi-dimension review (verified against installed chrome-devtools-mcp@1.1.0)
surfaced a class of false-negatives plus several robustness/validation gaps.

Headline bug — transport-layer failures scored as PASS:
NetworkFormatter emits `status` as a STRING: a numeric code for completed
responses, but `failure.errorText` ("net::ERR_CONNECTION_REFUSED", DNS/blocked)
for transport failures and "pending" for in-flight requests. The old
`Number(status) >= 500/400` checks turn those into NaN, which is neither >=500
nor >=400, so a request that flat-out failed produced neither a failure nor a
warning — analyzeQuality returned 'pass' and the run exited 0. The
assert-no-http-errors scenario had the same NaN blind spot. Both now share a
classifyNetworkStatus() helper (new core/network.mjs) so they cannot drift.

Also fixed:
- devtools-client: pass maxBuffer (64 MiB) to execFile; large snapshot/network/
  console JSON would otherwise crash the run at Node's 1 MiB stdout default.
- runner: isolate each post-scenario collection (console/network/lighthouse) so
  one failure (e.g. a Lighthouse timeout) no longer discards the whole report;
  writeReport now runs in finally so the report is always persisted best-effort.
- cli: validate --timeout (reject valueless/NaN/<=0 instead of failing every wait
  scenario instantly); sanitize profile.name and guard outDir so a malicious
  --out/name can't fs.rm an ancestor of cwd.
- quality: favicon-404 ignore now compares URL pathname (matches cache-buster
  query strings); also surface console 'assert' messages, not just 'error'.
- snapshot: findBySpec requires a positive matcher (role/nameIncludes) so a
  missing selector no longer silently matches the root node.
- chatbot: empty-input adds an answerDoneText-count oracle (catches an app that
  echoes blank input by reusing nodes, which the node-delta heuristic misses).
- profile: require consentAgreeButton for consent scenarios; validate viewport
  format and integer maxNodeDelta.
- docs: document common scenario fields, consent requirement, screenshot/fileName,
  transport-failure handling.
- tests: +16 (network classifier, scenario oracles, profile validation,
  findBySpec guard, transport-failure regression). 31 pass.
@amazon7737
amazon7737 merged commit f305118 into main Jun 1, 2026
1 check passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant