Skip to content

fix: correct LOC calculation, cache integrity, and release health evaluation - #1

Merged
lis186 merged 4 commits into
mainfrom
fix/release-analysis-data-integrity
Jul 30, 2026
Merged

fix: correct LOC calculation, cache integrity, and release health evaluation#1
lis186 merged 4 commits into
mainfrom
fix/release-analysis-data-integrity

Conversation

@lis186

@lis186 lis186 commented Jul 30, 2026

Copy link
Copy Markdown
Owner

變更說明

同一條 release:analyze 指令在不同輸出模式下互相矛盾(table 報 43 個 MR、--json 報 0),追查後發現十二個獨立缺陷,橫跨安全性與資料正確性。分三個 commit:

1. 安全性-v 會把整包 flags(含 --token)dump 進日誌,GitLab 憑證明文外流;logger.debug/info 使用 console.debug/info,兩者在 Node 都寫 stdout,導致 -v --json 的輸出無法被任何 JSON parser 解析。

2. LOC 計算getMergeRequestChanges 把 GitLab 的 changes_count 當成 "+50 -20" 行數格式解析,但它其實是變更檔案數。regex 永不匹配,else 分支把檔案數存進 additionsdeletions 恆為 0。所有 LOC 數字都錯,常低估一到兩個數量級,而批量閾值一直在跟檔案數比較。改用 allDiffs() + 既有的 calculateTotalChanges()size-analyzer.ts 早就用對了)。

3. 健康度評估與資料完整性 — 三個最嚴重的:

  • evaluate_batch_size 從未生效:用 release_typesname 欄位比對 classifyReleaseType 回傳的 key,永遠不匹配,health_level 恆為 null。
  • 有 MR 的 release 會靜默消失:快取存 JSON,merged_at 讀回是字串,calculateFreezeDays 對它呼叫 .getTime() 拋錯,又被 batch 的 skip 策略吞掉。
  • 降級結果被當成功寫入快取(兩層),一次 API 抖動就在整個 TTL 內持續回錯數字。

其餘:metrics.level 只看 MR 數、時間過濾切掉 predecessor anchor、readiness 對合成的 freeze_days = 0 誤報「當天發布,測試時間不足」、init 靜默丟掉 loc_additions、新增 loc_additions 閾值(加總會把清技術債判成和大批新功能同級)。

兩個對外契約變更(刻意)

  1. releases[].health_level 新增 null — 表示未評估:發布類型未啟用 evaluate_batch_size,或缺前一個標籤而無法界定 MR 區間。對未測量的 release 回報 healthy 比回報「不知道」更糟。
  2. 閾值文件從 <= 改為 < — 實作一直是 < healthy,剛好等於 healthy 的值算 warning。這裡是改文件對齊實作,不是改行為(改實作會變更既有 mr_count 邊界),並加了邊界測試釘住。

已知未修

gitlab-client.ts 的 MR 查詢是 perPage: 100, maxPages: 10 搭配 orderBy: 'updated_at'——先抓整條分支最近 1000 筆 merged MR,才用 merge_commit_sha 對 tag 區間過濾。後果是 (a) 分支累積破千後舊 release 靜默低估、(b) 舊 MR 被更新就撞回視窗,同指令不同天可能回不同結果、(c) 錯誤會被 (fromSha, toSha) 快取固化。這是成功路徑產生錯數字,現有以錯誤為訊號的降級機制偵測不到。

實測目前餘裕約 2.3 倍(14 個月區間 440 筆 / 上限 1000),觸發門檻約 32 個月的查詢區間,故本 PR 不處理,另案追蹤。

安全性檢查清單

在提交此 PR 前,請確認:

  • 我已確認沒有提交任何敏感資訊(tokens, passwords, 內部 URLs)
  • 我已確認沒有包含真實的公司/專案名稱
  • 我已使用 .env 管理所有配置資訊
  • 我已執行 ./scripts/check-secrets.sh 並通過檢查
  • 我已執行測試並通過:npm test

補充說明(本 repo 是 public,故逐項複驗過):

  • 測試 fixture 全部匿名:使用者代號 dev-a、MR IID 101、分支 dev-a/i18n、host gitlab.example.com、專案 group/project。原本引用的真實 LOC 數字已改為維持相同大小關係的合成值(測試斷言的是「看新增」與「看加總」判斷不同這個行為,不是特定數值)。
  • check-secrets.sh 的檔案內容掃描 ✅ 通過。它另報「6 個可疑 commit」,經逐一核對是兩類假陽性:commit 1 的訊息文字描述「修掉 token 洩漏」必然包含 token 一詞,以及 .env.example 既有的 # 格式範例: glpat-... 佔位說明。三個 commit 的 diff 對真實密鑰模式命中為 0。
  • token 遮蔽測試的哨兵值刻意不用 glpat- 前綴——那會讓 check-secrets.sh 對該檔案永遠報警,久了會訓練大家忽略掃描器。遮蔽是按欄位名做的,不看 token 格式。
  • .gitlab-analysis.yml 已由 .gitignore 忽略(git check-ignore 驗過)。

⚠️ 順帶回報兩個本 PR 未觸及的既有問題:core.hooksPath 未設定,所以 .githooks/pre-push(禁止直接 push main)實際不會執行;git-secrets 已安裝但這個 repo 沒註冊任何 pattern。兩層自動防護目前都是失效的。

測試計畫

npm run lint                             # tsc --noEmit,0 錯誤
npx vitest run --no-file-parallelism      # 80 檔案 / 1169 passed / 16 skipped

新增 7 個測試檔(44 個測試),全部針對「revert 掉修復也不會被抓到」的缺陷設計:

  • release-analyzer-regression.test.ts — 走 analyzeBatchSize 完整路徑,涵蓋 key/name 混淆、快取 JSON hydration、降級不得寫快取、schemaVersion、predecessor pool
  • release-health-level.test.ts — 多維取最差、loc_additions 優先、閾值邊界(釘住 < healthy
  • release-readiness-unmeasured.test.ts — 未測量發布不進凍結期評估,含一條防過度過濾的守衛
  • gitlab-client-mr-changes.test.ts — 行數必須來自 diff
  • logger.test.ts — 斷言用了哪個 console 方法(vitest 會接管 console,stream 層的 spy 不會觸發)
  • token-redaction.test.ts — 走命令真實流程,確認哨兵值不入日誌且 dump 真的發生過
  • thresholds-schema.test.tsloc_additions schema 與 refine

已對三個關鍵修復做突變測試(把修復 revert 掉確認測試會紅):key/name 修復 revert 後 regression test 紅而既有 health-level 測試仍綠;readiness 守衛 revert 後 3 紅 2 綠(綠的兩條是防過度過濾的守衛,本來就該不敏感)。

LOC 修復另有 golden acceptance:用獨立的 Python 腳本自行分頁並解析 unified diff,與工具輸出完全吻合。

相關 Issue

Justin Lee and others added 4 commits July 30, 2026 12:06
Two independent problems made verbose mode unsafe to use:

- `release:analyze -v` dumped the entire parsed flags object, which
  includes `--token`, so the plaintext GitLab token landed in logs.
  Only the token is redacted; other flags keep their debugging value.

- `logger.debug()` and `logger.info()` used `console.debug`/`console.info`,
  which write to **stdout** in Node, not stderr. Running `-v --json`
  therefore produced output that no JSON parser could read. Both now go
  to stderr, leaving stdout for command output only. `warn`/`error` were
  already on stderr and are unchanged.

`logger.info` had zero call sites, and all 47 `logger.debug` calls are
diagnostic, so no user-visible output depended on the old stream.

The logger test asserts which console method is used rather than spying on
the streams: vitest replaces `console`, so stream-level spies never fire.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`getMergeRequestChanges` parsed GitLab's `changes_count` as if it were a
`"+50 -20"` line-count string. It is not — it is the number of **changed
files**. The regex never matched, so the else branch stored the file count
in `additions` and left `deletions` permanently 0.

Every LOC figure the release analysis reported was therefore wrong, often
by one to two orders of magnitude, and the batch-size thresholds were
being compared against file counts.

The fix reuses what the codebase already had right: `allDiffs()` plus
`calculateTotalChanges()` from `src/utils/diff-parser.ts`, which
`size-analyzer.ts` has been using correctly all along. The 36 lines of
`changes_count` parsing are gone. Signature and return type are unchanged,
so no caller needed adjusting, and the API call count is the same as
before — only the payload is larger.

Truncated diffs now throw instead of silently under-counting. The check
keys on GitLab's `collapsed`/`too_large` flags rather than "the diff is
empty", because binary files legitimately produce empty diffs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The symptom was one command contradicting itself: the table reported 43
MRs for a release while `--json` reported 0. Root-causing that turned up
several independent defects in the release analyzer.

Health evaluation never ran at all. `calculateHealthLevelIfNeeded` looked
up `config.release_types` by the `name` field while `classifyReleaseType`
returns the key, so the lookup always missed and `health_level` was
permanently null. Not a configuration problem — the code could not read it.

Releases with MRs silently disappeared. The cache stores JSON, so
`merged_at` came back as a string; `calculateFreezeDays` called
`.getTime()` on it and threw, and the batch processor's `skip` strategy
swallowed the error. Dates are now revived on read, and the failures the
processor collects are logged with their tag name instead of discarded.

Degraded results were cached as if they had succeeded, at two levels, so a
transient API failure kept returning wrong numbers for the whole TTL. The
inner calls now throw and the outer layer degrades, which keeps
`cache.set` on the success path only. Cache keys also carry a schema
version so entries written by the old LOC algorithm cannot be read back.

Additional fixes in the same area:

- Batch size can now be judged by `loc_additions`, which takes priority
  over `loc_changes`. Summing additions and deletions rates a large
  technical-debt cleanup the same as a large feature batch; the risk being
  measured is how much new code shipped.
- The overall `metrics.level` considered only MR count, ignoring the LOC
  dimension entirely. It now takes the worst of the dimensions in play.
- Time filtering removed the tag needed as a predecessor anchor, so the
  oldest release in range lost its MR window. Predecessor lookup now uses
  an unfiltered pool.
- Release readiness assessed synthesized freeze periods. With no
  predecessor tag, `lastMergeDate` defaults to the tag date, giving
  `freeze_days = 0`, which `assessFreezePeriod` reports as "same-day
  release, insufficient test time" — a false alarm. Those releases are
  now skipped.
- `init` silently dropped `loc_additions` when writing config from a
  template.
- Recommendations name the dimension that triggered them instead of
  always citing MR count.

Two contract changes, both deliberate:

- `health_level` can now be `null`, meaning not evaluated — either the
  release type has `evaluate_batch_size` off, or no predecessor tag exists
  so the MR window cannot be defined. Reporting `healthy` for an
  unmeasured release is worse than reporting nothing.
- Threshold docs now say `<` rather than `<=`. The implementation has
  always used `< healthy`, so a value exactly equal to `healthy` counts as
  a warning. The docs were aligned to the code rather than the reverse,
  since changing the comparison would alter existing `mr_count` behaviour.
  Boundary tests pin this down.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of this PR turned up three ways the "unmeasured" state still leaked
into confident output. All three share a root cause: `lastMergeDate`
defaults to the tag date and is only overwritten when MRs are found, so an
empty MR list is indistinguishable from a same-day release.

**A failed MR list query no longer reports `healthy`.** This one was made
worse by the previous commit, not merely left unfixed. Before the key/name
lookup was repaired, `calculateHealthLevelIfNeeded` always returned null,
so a release whose MR query failed showed `N/A`. With the lookup working,
the same degraded path produced 0 MRs, 0 LOC, and a confident `healthy` —
and since the error goes to stderr while `--json` goes to stdout, a
consumer received a clean, parseable, entirely wrong report. The list fetch
now reports whether it failed, and a failed fetch marks the release
unevaluated with a warning naming the tag. The release itself is still
emitted: the tag existing is a fact, only its batch size is unknown.

`getMergeRequestsBetweenReleases` keeps its signature and delegates to a
private method that returns the failure flag alongside the events, so the
public contract and its existing callers are untouched.

**Readiness now skips releases with no MRs in range.** The previous guard
covered only "no predecessor tag", but a range containing no merges reaches
the same synthesized `freeze_days = 0` — a retagged release, a tag cut from
the same commit as its predecessor, or the failed-fetch case above. All of
them were reported as "same-day release, insufficient test time".

The guard keys on `mr_count`, not on `freeze_days === 0`, so a genuine
same-day release still reports critical. The test covering that case now
uses a non-zero `mr_count`, which is also what it means: a real zero-day
freeze had MRs merged on tag day.

**Truncation detection is now tested.** It was the only data-integrity
check in the diff-based line counting and had no coverage at all — deleting
the entire check left every test green. Added cases for `collapsed` and
`too_large`, plus one pinning that an empty diff *without* those flags must
not throw, since binary files legitimately produce empty diffs.

All three fixes were mutation-tested: reverting each one turns its tests
red (1, 1, and 2 failures respectively).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lis186

lis186 commented Jul 30, 2026

Copy link
Copy Markdown
Owner Author

Code review 記錄(claude-review workflow 無法執行的替代)

claude-review 在此 PR 失敗的原因是 repo 未設定 secret,與程式碼無關:

##[error] Environment variable validation failed:
  - Either ANTHROPIC_API_KEY, CLAUDE_CODE_OAUTH_TOKEN, or workload identity
    federation (ANTHROPIC_FEDERATION_RULE_ID and ANTHROPIC_ORGANIZATION_ID)
    is required when using direct Anthropic API

gh secret list 為空。這是 repo 的第一個 PR,所以該 workflow 從未被執行過,缺口一直存在只是沒人碰到。

因此改以人工 code review 取代,結果如下。找到 3 個 blocker,已全部修復(commit f3bc77c)。

B1 — mr_count === 0 仍會產生假的「當天發布」critical

新守衛只擋「沒有前一個標籤」,但 lastMergeDate 被合成為 tagDate 的條件有兩個:預設值,以及「只有查到 MR 才覆寫」。previousTag 存在但區間內 0 筆 MR(重打標籤、從同一 commit 切標籤、或 B2 的查詢失敗)同樣得到 freeze_days = 0assessFreezePeriod 判 critical。

實測(getMergeRequestsBetweenCommits[]):

{"avgFreezeDays":0,"criticalCount":2,
 "recommendation":"發現當天發布情況,建議增加測試緩衝時間至少 1-2 天"}

守衛改為 !release.previous_release_tag || release.mr_count === 0刻意不用 freeze_days === 0 當判準,否則真正的當天發布也會被吞掉。原本那條「有前序而凍結期真的是 0 天仍要報 critical」的測試因此改用非零 mr_count——那也才是它的語意:真實的零日凍結代表有 MR 在打標籤當天合併。

B2 — MR 列表查詢失敗會輸出「0 MR / healthy / 批量健康」

這是本 PR 造成的迴歸,不只是既有缺陷。 在 key/name 混淆被修好之前,calculateHealthLevelIfNeeded 恆回 null,所以查詢失敗的 release 頂多顯示 N/A;修好之後,同一條降級路徑會產出一個有自信的 healthy

實測(getMergeRequestsBetweenCommits 全數 reject 500):

newest:  {"mr":0,"health":"healthy","freeze":0}
metrics: {"level":"healthy","recommendation":"發布批量健康,維持當前節奏"}

logger.error 在 stderr、--json 在 stdout,所以下游拿到一份乾淨、可解析、完全錯誤的「健康」報告——與本 PR 起因的「table 說 43、--json 說 0」是同一個 bug class,只是換了方向。

修法:getMergeRequestsBetweenReleases 保持原簽名,委派給一個回傳 {events, listFetchFailed} 的 private 方法;失敗時該 release 標為未評估並 warn 出 tag 名稱。release 本身仍然輸出——標籤存在是事實,只是批量不可測量。

B3 — 截斷偵測完全沒有測試

collapsed/too_large 檢查是 diff 行數計算唯一的資料完整性防線,也是全新程式碼,但 5 個測試沒有任何一條餵這兩個旗標。突變測試證實:整段刪掉,5 個測試全綠。

補了 collapsedtoo_large 各一條,另加一條釘住「空 diff 但沒有旗標時不得拋錯」——二進位檔的 diff 合法地是空字串,判準必須是旗標而非「diff 是否為空」。

驗證

三個修復都做了突變測試,revert 後分別轉紅 1 / 1 / 2 條。

npx tsc --noEmit                        → 0 錯誤
npx vitest run --no-file-parallelism    → 80 檔 / 1174 passed / 16 skipped
./scripts/check-secrets.sh              → 檔案內容掃描 ✅

已知未修(review 列為 major,另案)

  • table 輸出缺 average_loc_additions:設了 loc_additions 之後等級由平均新增行數決定,但畫面只印「平均 LOC 變更」並用整體等級的顏色染它,三個數字可能互相矛盾。JSON 有帶,只有人類看的那份缺。
  • 三個 preset 都沒有 loc_additions 範例,README 也只寫 loc_changes,所以這個新的主要維度在預設安裝下不生效,要使用者自己手改 YAML 才會啟用。
  • readiness 區塊在 0 筆評估時整段消失,analyzer 已備好的「無足夠資料進行評估」被 formatter 的 length > 0 守衛擋掉。B1/B2 的守衛讓這個情況更容易發生。
  • degraded 仍只用於決定是否寫快取,輸出層無欄位可表達「這個數字不可信」;且 collapsed 若是常態,該 release 的 mr_list 會永遠寫不進快取。GitLab 建議的處理是 fallback 到 access_raw_diffs=true,而非把整個 MR 降成 0 行。

另外回報兩個本 PR 未觸及的既有問題

  • core.hooksPath 未設定,所以 .githooks/pre-push(禁止直接 push main)實際不會執行。
  • git-secrets 已安裝但此 repo 未註冊任何 pattern。

兩層自動防護目前都是失效的。

@lis186
lis186 merged commit d290461 into main Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant