Skip to content

Commit f2c9a1c

Browse files
committed
fix(tests): forward-only env-restore comments in auth and telemetry tests
Also record the review-thread reply convention in agent memory. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
1 parent b187edc commit f2c9a1c

4 files changed

Lines changed: 24 additions & 7 deletions

File tree

.claude/agent-memory/archgate-developer/MEMORY.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to inv
2424
- [Pick the right enforcement layer](feedback_prefer_tests_over_adr_rules.md) — static syntax → custom oxlint rule; executable behavior → tests; cross-file/governance → ADR `.rules.ts`. Never write an ADR rule that only asserts implementation shape — `rules: false` is a valid outcome
2525
- [This repo is PUBLIC — no private sibling-repo internals, no Claude session links in PRs/commits](feedback_public_repo_privacy.md)
2626
- [Keep code comments and memory entries concise](feedback_concise_comments.md) — code side now machine-enforced by GEN-004 (oxlint `archgate/*` + `archgate check`); memory-entry conciseness still manual
27+
- [Answer every review finding on its own thread](feedback_reply_on_review_threads.md) — especially declined ones; a summary PR comment does not close the loop (recurring miss)
2728
- [Throw UserError in boundary-wrapped guards](feedback_throw_usererror_in_guards.md) — not logError + exitWith(1); the action's handleCommandError boundary does that
2829
- [Docs are forward-only and version-independent](feedback_forward_only_docs.md) — describe current state; no "previously"/"rather than"/"shipped" framing, no pinned release version or drift-prone counts; git & package.json are the source of truth
2930

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
---
2+
name: reply-on-review-threads
3+
description: Answer every review finding on its own thread — especially declined ones; a summary PR comment does not close the loop
4+
metadata:
5+
type: feedback
6+
---
7+
8+
Reply to each PR review finding **on its own thread**, not only in a summary PR comment. Declined findings especially: the reviewer (human or bot) must see the reasoning attached to the code line they raised.
9+
10+
**Why:** User feedback 2026-07-25 on PR #496: I addressed 14 CodeRabbit threads with one consolidated PR comment that explained three declines. The user flagged it as recurring — "the ones you declined you must answer at each thread. not the first time this is happening." An unanswered thread stays open and reads as ignored; the reviewer cannot resolve it, and a summary comment is not linked to the line.
11+
12+
**How to apply:**
13+
14+
- Find unresolved threads with the GraphQL `reviewThreads.isResolved` query (see [[project-pr-review-thread-triage]]) — REST does not expose resolved state.
15+
- Reply per thread: `gh api repos/<owner>/<repo>/pulls/<n>/comments/<first_comment_databaseId>/replies -f body='...'` (the reply targets the thread's FIRST comment id).
16+
- Declined → state the reason on the thread (out of scope / came from main / conflicts with an ADR). Accepted → state the fixing commit SHA. A summary comment is optional on top, never a substitute.
17+
- Applies to bot reviewers too — CodeRabbit re-reviews per thread.

tests/helpers/auth.test.ts

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,10 @@ describe("auth", () => {
3636

3737
afterEach(() => {
3838
// `Bun.env.X = undefined` assigns the STRING "undefined" and leaves the key
39-
// present — it does not unset. Since HOME and GIT_CONFIG_GLOBAL are normally
40-
// unset on Windows, a plain restore leaked HOME="undefined" into the shared
41-
// process env, and every later test that spawned a subprocess inherited it.
42-
// Bun.env is process-global and test files share one process, so this must
43-
// delete when the original was absent.
39+
// present — it does not unset. HOME and GIT_CONFIG_GLOBAL are normally unset
40+
// on Windows, and Bun.env is process-global across test files, so a plain
41+
// restore would publish HOME="undefined" to every later test and any
42+
// subprocess it spawns. restoreEnv deletes when the capture was absent.
4443
restoreEnv("HOME", originalHome);
4544
restoreEnv("USERPROFILE", originalUserProfile);
4645
restoreEnv("GIT_CONFIG_NOSYSTEM", originalGitConfigNoSystem);

tests/helpers/telemetry.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@ describe("telemetry", () => {
2727

2828
afterEach(async () => {
2929
// `env.X = undefined` assigns the string "undefined" rather than unsetting,
30-
// so HOME (normally unset on Windows) leaked into every later test file.
31-
// Bun.env and process.env are the same store, so restoreEnv covers both.
30+
// which would leak HOME (normally unset on Windows) into every later test
31+
// file. Bun.env and process.env are the same store, so restoreEnv covers both.
3232
restoreEnv("HOME", originalHome);
3333
restoreEnv("ARCHGATE_TELEMETRY", originalTelemetryEnv);
3434
restoreEnv("NODE_ENV", originalNodeEnv);

0 commit comments

Comments
 (0)