fix: keep import-batch running after token failures #69

Merged

andres merged 6 commits from build/65-import-batch-token-failure into main 2026-09-04 08:02:45 +00:00

Conversation 7 Commits 6 Files changed 3 +272 -23

codex-bot-andresmgsl commented 2026-09-04 06:58:27 +00:00

Member

Copy link

Refs #65

Summary

Keep repo import-batch running after one item cannot resolve its source token, preserving per-item reporting and the final batch summary.

Worklog

• Claim #65 and post the implementation plan

• Add and push a red mixed-manifest regression test

• Move payload and token resolution inside the per-item try

• Cover unchanged all-success, batch-level failure, and invalid-entry behavior

• Add the changelog fragment

• Run mutation proof and the full test suite

• Verify exact diff scope and complete the round handoff

• Reproduce the Node 22.12.0 warning contamination and confirm the root cause

• Keep Node runtime diagnostics out of strict application-stderr assertions

• Add explicit GitHub-token payload and redaction coverage

• Run focused mutation proof and the full supported-runtime test suite

• Reply to the whole round and verify the final handoff head

Acceptance criteria

• A missing GitHub token fails only that item; the next item imports, the summary says Batch complete: 1/2 imported., and exit status is 1.
• All-importable output and exit status remain byte-identical.
• Missing or invalid manifest files still use the action-level Batch import failed: path.
• Invalid manifest entries are still skipped without entering results.
• No token value appears in output or captured test data.
• The diff touches exactly src/cli.js, test/import-batch.test.js, and changelog.d/65.md.
• npm test passes locally (154/154); the engine holds panel requests until ci / test is green on this head.

Verification

• Node 22.12.0: node --test test/import-batch.test.js — 5/5 passed at the completed head.
• Mutation proof with token resolution restored above the per-item try — the mixed-manifest test failed on the original action-level abort; the other 3 original tests passed.
• Mutation proof with auth_token removed from the payload — the explicit-token test failed on the missing field, then passed after restoration.
• npm test — 154/154 passed.
• npm run check:governance — 4 identities and 5 scope rows valid.
• git diff --check and exact three-path scope checks passed; worktree is clean.

Round log

Round at 4333ce63

🔧 addressing round on head 4333ce63bf

Round analysis:

• GLM — agree (blocking): Node 22.12.0 emits the CommonJS→ESM ExperimentalWarning into spawned-child stderr, so the two exact stderr assertions fail at the package's declared engine floor even though CI's floating Node 22 passes. I will keep strict application-stderr assertions and suppress only Node's ExperimentalWarning in the test harness spawn environment, then run the focused tests and full suite on the available floor runtime.
• GLM — agree (non-blocking): preserve the headline test's whole-request deepEqual payload pin while changing the harness.
• Claude — agree (non-blocking): add an explicit github_token success case that proves the token reaches payload.auth_token but never appears in stdout/stderr. This closes the stated coverage gap without changing production behavior or diff scope.
• Claude — agree (verification boundary): no live migration endpoint test is needed; the issue's no-network plan and unchanged request client make the local stub the correct boundary. I will retain that boundary.
• Kimi — agree: preserve the discriminating mixed-manifest mutation coverage and exact request-body assertions; no production-code change is needed for this round.
• Kimi — agree (verification boundary): CI logs were not directly inspected, but local full-suite verification plus the head status is sufficient; this fix will be verified locally before handoff.

I will append these fix steps to the Worklog, push the test-only correction and coverage, reply once covering the whole round, then signal the final head while the PR is still draft before marking it ready.

Round reply for head 37e6a2ad5a0141a344272004d0ef5c3d85d6de17:

• GLM blocker: fixed in 82494e9. The child-process harness now appends only --disable-warning=ExperimentalWarning to NODE_OPTIONS, keeping Node 22.12.0's Commander interop diagnostic out of application stderr while preserving strict stderr assertions and every other warning category. The exact floor-runtime reproduction moved from 2/4 to 4/4; the final focused file is 5/5 and the full Node 22.12.0 suite is 154/154.
• GLM non-blocking point: preserved the headline test's whole-request deepEqual payload assertion unchanged.
• Claude non-blocking point: added an explicit github_token success case in 37e6a2a. It asserts the complete captured migration body includes auth_token and independently asserts the token is absent from stdout and stderr. Removing auth_token from production made this test fail on the missing field; restoring it returned the test to green.
• Claude verification boundary: retained the issue-mandated no-network local HTTP stub. The migration client and endpoint behavior remain outside this diff.
• Kimi points: retained the mixed-manifest discriminator and exact request-body assertions; this round changes tests only, with production behavior untouched.
• Verification boundary: ran the focused test and full suite locally on exact Node 22.12.0, plus governance and diff checks. The diff remains exactly src/cli.js, test/import-batch.test.js, and changelog.d/65.md; the worktree is clean and the remote PR head matches locally.

This answers every blocking and non-blocking review point in the completed round.

📣 round answered at head 37e6a2ad5a

Round at 37e6a2ad

Round passed with no written reply.

Refs #65 ## Summary Keep `repo import-batch` running after one item cannot resolve its source token, preserving per-item reporting and the final batch summary. ## Worklog - [x] Claim #65 and post the implementation plan - [x] Add and push a red mixed-manifest regression test - [x] Move payload and token resolution inside the per-item `try` - [x] Cover unchanged all-success, batch-level failure, and invalid-entry behavior - [x] Add the changelog fragment - [x] Run mutation proof and the full test suite - [x] Verify exact diff scope and complete the round handoff - [x] Reproduce the Node 22.12.0 warning contamination and confirm the root cause - [x] Keep Node runtime diagnostics out of strict application-stderr assertions - [x] Add explicit GitHub-token payload and redaction coverage - [x] Run focused mutation proof and the full supported-runtime test suite - [x] Reply to the whole round and verify the final handoff head ## Acceptance criteria - [x] A missing GitHub token fails only that item; the next item imports, the summary says `Batch complete: 1/2 imported.`, and exit status is 1. - [x] All-importable output and exit status remain byte-identical. - [x] Missing or invalid manifest files still use the action-level `Batch import failed:` path. - [x] Invalid manifest entries are still skipped without entering results. - [x] No token value appears in output or captured test data. - [x] The diff touches exactly `src/cli.js`, `test/import-batch.test.js`, and `changelog.d/65.md`. - [x] `npm test` passes locally (154/154); the engine holds panel requests until `ci / test` is green on this head. ## Verification - Node 22.12.0: `node --test test/import-batch.test.js` — 5/5 passed at the completed head. - Mutation proof with token resolution restored above the per-item `try` — the mixed-manifest test failed on the original action-level abort; the other 3 original tests passed. - Mutation proof with `auth_token` removed from the payload — the explicit-token test failed on the missing field, then passed after restoration. - `npm test` — 154/154 passed. - `npm run check:governance` — 4 identities and 5 scope rows valid. - `git diff --check` and exact three-path scope checks passed; worktree is clean. ## Round log <!-- round:4333ce63bf5f5761247094a3fc798b69239c4b20 --> **Round at 4333ce63** 🔧 addressing round on head 4333ce63bf5f5761247094a3fc798b69239c4b20 Round analysis: - GLM — **agree (blocking):** Node 22.12.0 emits the CommonJS→ESM `ExperimentalWarning` into spawned-child stderr, so the two exact stderr assertions fail at the package's declared engine floor even though CI's floating Node 22 passes. I will keep strict application-stderr assertions and suppress only Node's `ExperimentalWarning` in the test harness spawn environment, then run the focused tests and full suite on the available floor runtime. - GLM — **agree (non-blocking):** preserve the headline test's whole-request `deepEqual` payload pin while changing the harness. - Claude — **agree (non-blocking):** add an explicit `github_token` success case that proves the token reaches `payload.auth_token` but never appears in stdout/stderr. This closes the stated coverage gap without changing production behavior or diff scope. - Claude — **agree (verification boundary):** no live migration endpoint test is needed; the issue's no-network plan and unchanged request client make the local stub the correct boundary. I will retain that boundary. - Kimi — **agree:** preserve the discriminating mixed-manifest mutation coverage and exact request-body assertions; no production-code change is needed for this round. - Kimi — **agree (verification boundary):** CI logs were not directly inspected, but local full-suite verification plus the head status is sufficient; this fix will be verified locally before handoff. I will append these fix steps to the Worklog, push the test-only correction and coverage, reply once covering the whole round, then signal the final head while the PR is still draft before marking it ready. Round reply for head `37e6a2ad5a0141a344272004d0ef5c3d85d6de17`: - GLM blocker: fixed in `82494e9`. The child-process harness now appends only `--disable-warning=ExperimentalWarning` to `NODE_OPTIONS`, keeping Node 22.12.0's Commander interop diagnostic out of application stderr while preserving strict stderr assertions and every other warning category. The exact floor-runtime reproduction moved from 2/4 to 4/4; the final focused file is 5/5 and the full Node 22.12.0 suite is 154/154. - GLM non-blocking point: preserved the headline test's whole-request `deepEqual` payload assertion unchanged. - Claude non-blocking point: added an explicit `github_token` success case in `37e6a2a`. It asserts the complete captured migration body includes `auth_token` and independently asserts the token is absent from stdout and stderr. Removing `auth_token` from production made this test fail on the missing field; restoring it returned the test to green. - Claude verification boundary: retained the issue-mandated no-network local HTTP stub. The migration client and endpoint behavior remain outside this diff. - Kimi points: retained the mixed-manifest discriminator and exact request-body assertions; this round changes tests only, with production behavior untouched. - Verification boundary: ran the focused test and full suite locally on exact Node 22.12.0, plus governance and diff checks. The diff remains exactly `src/cli.js`, `test/import-batch.test.js`, and `changelog.d/65.md`; the worktree is clean and the remote PR head matches locally. This answers every blocking and non-blocking review point in the completed round. 📣 round answered at head 37e6a2ad5a0141a344272004d0ef5c3d85d6de17 <!-- round:37e6a2ad5a0141a344272004d0ef5c3d85d6de17 --> **Round at 37e6a2ad** _Round passed with no written reply._

codex-bot-andresmgsl added 1 commit 2026-09-04 06:58:27 +00:00

test: reproduce import-batch token abort 721ba343cc

codex-bot-andresmgsl changed title from fix: keep import-batch running after token failures to WIP: fix: keep import-batch running after token failures 2026-09-04 06:58:41 +00:00

forgejo-actions added the

blocker:ci-red

state:building

labels 2026-09-04 06:58:45 +00:00

codex-bot-andresmgsl added 1 commit 2026-09-04 06:59:07 +00:00

fix: isolate import-batch token failures 1bb4bd608c

forgejo-actions added

scope:cli

and removed

blocker:ci-red

labels 2026-09-04 06:59:23 +00:00

codex-bot-andresmgsl added 1 commit 2026-09-04 07:00:09 +00:00

test: cover import-batch result boundaries 5f2f58a249

codex-bot-andresmgsl added 1 commit 2026-09-04 07:00:28 +00:00

docs: note import-batch token handling 4333ce63bf

forgejo-actions added the

scope:packaging

label 2026-09-04 07:00:56 +00:00

codex-bot-andresmgsl commented 2026-09-04 07:02:26 +00:00

Author

Member

Copy link

📣 round answered at head 4333ce63bf

📣 round answered at head 4333ce63bf5f5761247094a3fc798b69239c4b20

codex-bot-andresmgsl changed title from WIP: fix: keep import-batch running after token failures to fix: keep import-batch running after token failures 2026-09-04 07:02:27 +00:00

forgejo-actions added

state:addressing

and removed

state:building

labels 2026-09-04 07:03:14 +00:00

codex-bot-andresmgsl requested review from glm-bot-andresmgsl 2026-09-04 07:11:00 +00:00

codex-bot-andresmgsl requested review from claude-bot-andresmgsl 2026-09-04 07:11:00 +00:00

codex-bot-andresmgsl requested review from kimi-bot-andresmgsl 2026-09-04 07:11:00 +00:00

codex-bot-andresmgsl added the

state:bots-reviewing

label 2026-09-04 07:11:01 +00:00

forgejo-actions removed the

state:addressing

label 2026-09-04 07:11:20 +00:00

claude-bot-andresmgsl commented 2026-09-04 07:15:48 +00:00

Member

Copy link

🔎 reviewing head 37e6a2ad5a

Round 2, whole PR re-reviewed at this head in a detached worktree (npm ci first). Production diff is byte-identical to the round-1 head; the round-2 commits are test-only.

• Mixed manifest: second item attempted, Failed to import missing-token: No GitHub token found., Batch complete: 1/2 imported., exit 1 — test 1.
• All-importable output/exit 0 byte-identical — test 2.
• Missing / malformed manifest still Batch import failed: + exit 1 — test 4.
• Skipping invalid manifest entry still continues without entering results — test 5.
• No token in output or captured data — new test 3 pins auth_token in the request body and its absence from stdout/stderr.
• Diff scope exactly src/cli.js, test/import-batch.test.js, changelog.d/65.md (vs merge-base 74e52b42).
• npm test 154/154 locally; ci / test and labels / labels both success on this head.
• GLM's blocker: injected an ExperimentalWarning into the spawned child via ambient NODE_OPTIONS — shipped harness 5/5; with the --disable-warning line removed, tests 2/3/5 fail. Suppression is narrow: an injected DeprecationWarning still breaks the same three.
• Mutation proof: 6 mutants, each fails only the tests it should.

Verdict submitted: approve.

🔎 reviewing head 37e6a2ad5a0141a344272004d0ef5c3d85d6de17 Round 2, whole PR re-reviewed at this head in a detached worktree (`npm ci` first). Production diff is byte-identical to the round-1 head; the round-2 commits are test-only. - [x] Mixed manifest: second item attempted, `Failed to import missing-token: No GitHub token found.`, `Batch complete: 1/2 imported.`, exit 1 — test 1. - [x] All-importable output/exit 0 byte-identical — test 2. - [x] Missing / malformed manifest still `Batch import failed:` + exit 1 — test 4. - [x] `Skipping invalid manifest entry` still `continue`s without entering `results` — test 5. - [x] No token in output or captured data — new test 3 pins `auth_token` in the request body and its absence from stdout/stderr. - [x] Diff scope exactly `src/cli.js`, `test/import-batch.test.js`, `changelog.d/65.md` (vs merge-base 74e52b42). - [x] `npm test` 154/154 locally; `ci / test` and `labels / labels` both success on this head. - [x] GLM's blocker: injected an `ExperimentalWarning` into the spawned child via ambient `NODE_OPTIONS` — shipped harness 5/5; with the `--disable-warning` line removed, tests 2/3/5 fail. Suppression is narrow: an injected `DeprecationWarning` still breaks the same three. - [x] Mutation proof: 6 mutants, each fails only the tests it should. Verdict submitted: approve.

kimi-bot-andresmgsl commented 2026-09-04 07:18:01 +00:00

Member

Copy link

🔎 reviewing head 37e6a2ad5a
Verdict APPROVE submitted at this head via submit-verdict.sh (exit 0, verified landed).

Review basis (Node 22.23.2, npm ci): import-batch tests 5/5; npm test 154/154; check:governance green; diff --check clean. Mutation proof: base src/cli.js fails exactly the mixed-manifest test (4 pass/1 fail). Scope exactly src/cli.js, test/import-batch.test.js, changelog.d/65.md. ci / test (pull_request) and labels / labels both success on this head. Not verified at exact Node 22.12.0 — relied on the 82494e9 NODE_OPTIONS warning guard plus green ci / test.

🔎 reviewing head 37e6a2ad5a0141a344272004d0ef5c3d85d6de17 Verdict APPROVE submitted at this head via submit-verdict.sh (exit 0, verified landed). Review basis (Node 22.23.2, npm ci): import-batch tests 5/5; npm test 154/154; check:governance green; diff --check clean. Mutation proof: base src/cli.js fails exactly the mixed-manifest test (4 pass/1 fail). Scope exactly src/cli.js, test/import-batch.test.js, changelog.d/65.md. ci / test (pull_request) and labels / labels both success on this head. Not verified at exact Node 22.12.0 — relied on the 82494e9 NODE_OPTIONS warning guard plus green ci / test.

glm-bot-andresmgsl commented 2026-09-04 07:18:35 +00:00

Member

Copy link

🔎 reviewing head 37e6a2ad5a

• [progress] prior round closed at 4333ce6 with request-changes (test-portability blocker); builder answered and moved head — reviewing the whole PR fresh at 37e6a2a
• [progress] fresh npm ci of the lockfile (commander 15.0.0) on Node 22.12.0; npm test 154/154; import-batch 5/5
• [progress] mutation proofs: pre-fix src fails only the mixed-manifest test; auth_token removed fails the explicit-token test — failure cases genuinely fail
• [progress] blocker answered as prescribed: NODE_OPTIONS --disable-warning=ExperimentalWarning in the harness, strict stderr assertions kept; warning no longer reproduces on this install even without the flag
• [progress] criteria 1–7 each settled (see verdict); diff scope exact (src/cli.js, test/import-batch.test.js, changelog.d/65.md); ci / test green on head; Refs hygiene clean
• [progress] composing verdict: approve

🔎 reviewing head 37e6a2ad5a0141a344272004d0ef5c3d85d6de17 - [progress] prior round closed at 4333ce6 with request-changes (test-portability blocker); builder answered and moved head — reviewing the whole PR fresh at 37e6a2a - [progress] fresh npm ci of the lockfile (commander 15.0.0) on Node 22.12.0; npm test 154/154; import-batch 5/5 - [progress] mutation proofs: pre-fix src fails only the mixed-manifest test; auth_token removed fails the explicit-token test — failure cases genuinely fail - [progress] blocker answered as prescribed: NODE_OPTIONS --disable-warning=ExperimentalWarning in the harness, strict stderr assertions kept; warning no longer reproduces on this install even without the flag - [progress] criteria 1–7 each settled (see verdict); diff scope exact (src/cli.js, test/import-batch.test.js, changelog.d/65.md); ci / test green on head; Refs hygiene clean - [progress] composing verdict: approve

claude-bot-andresmgsl approved these changes 2026-09-04 07:19:00 +00:00

Dismissed

claude-bot-andresmgsl left a comment

Member

Copy link

Approve — head 4333ce63bf5f5761247094a3fc798b69239c4b20

The fix is the one #65 specified: the payload construction, and with it auth_token: resolveSourceToken(...), moves inside the existing per-item try at src/cli.js:664. Nothing else in the loop, the message wording, the summary, or the exit code changed — which is exactly right, because those were already correct and are what the change restores. resolveSourceToken and the cachedGhToken memo are untouched, and no negative caching was smuggled in.

Acceptance criteria — all verified at this head

Reviewed in a detached worktree (npm ci first — the worktree is otherwise dependency-less and every spawn test phantom-fails).

#	Criterion	Evidence
1	Mixed manifest: item 2 attempted, Failed to import <first>: No GitHub token found…, Batch complete: 1/2 imported., exit 1	test 1 asserts all four, including stdout by exact string equality
2	All-importable output and exit 0 byte-identical	test 2, exact stdout/stderr equality
3	Missing / malformed manifest still Batch import failed: + exit 1	test 3, both cases, and asserts stdout stayed empty
4	Skipped invalid entries still continue without entering results	test 4 — Batch complete: 1/1, not 1/2, is the discriminating assertion
5	No token value in output or captured data	assert.doesNotMatch on the forge token over stdout+stderr, plus a deepEqual on the whole request body that pins the absence of auth_token for the git-service entry
6	Diff touches exactly three paths	git diff --stat 74e52b4..HEAD → changelog.d/65.md, src/cli.js, test/import-batch.test.js
7	npm test passes, ci / test green	153/153 locally; ci / test (pull_request) and labels / labels (pull_request) both success on this SHA

The failure cases actually fail

I did not take the worklog's mutation claim on faith — I ran four mutants, one targeting each test, and confirmed each fails only its own test:

Mutant	Result
Hoist the payload back above the per-item try (the pre-fix code)	not ok 1, 2–4 pass
results.push({status:'failed'}) before the Skipping invalid manifest entry continue	not ok 4, 1–3 pass
Swallow JSON.parse errors into an empty manifest	not ok 3, 1/2/4 pass
Drop the \n prefix on the Batch complete: line	not ok 1 and not ok 2

The first is the one that matters: it reproduces the exact defect #65 describes and test 1 catches it. Test 1 also clears the test plan's three named traps — two entries, not one; it asserts the summary line and the second import, not just the exit code (which was 1 before the fix too); and it does not lean on ambient credentials.

Hermeticity, checked rather than assumed. Test 1 forces the token failure with PATH pointed at an empty directory and GITHUB_TOKEN: undefined in the spawn env. The latter relies on Node dropping undefined-valued keys when building envPairs, which is easy to get wrong, so I re-ran the file with GITHUB_TOKEN=polluted-ambient-token exported in the ambient environment: still 4/4. The scrubbing holds, and a developer with gh on PATH and a token in the environment gets the same result I did.

Other checks

• npm run check:governance — 4 identities resolved, 5 scope rows valid.
• changelog.d/65.md matches the established fragment shape (- <sentence>. (#65).), consistent with 57/60/62/64.
• Refs #65 is the shape #65 sanctions ("a Closes is equally correct here"). I grepped the whole PR body for a closing keyword preceding an issue number: the only two #65 occurrences are Refs #65 and Claim #65 and post the implementation plan. Neither closes the issue; triage keeps that call.
• scope:cli + scope:packaging match .github/labeler.yml for the changed paths (test/** maps to no scope, which is the config's behaviour, not this PR's doing).
• README.md's import-batch section documents options, manifest shape and the endpoint — it never claimed the batch aborts on a token failure, so no prose contract went stale here.

What I could not verify

• The real migration endpoint. Every import in these tests goes to a local http stub that always answers 201; I did not exercise a live Forgejo POST /api/v1/repos/migrate. That is the test plan's own instruction ("no network"), and the change does not touch the request or the client, so the stub is sufficient for what is under review.
• A successful service: 'github' import. No test drives the path where resolveSourceToken returns a token and auth_token reaches the payload. I read resolveSourceToken at src/cli.js:346 to confirm the token-supplied branch (if (tokenOption) return tokenOption;) is untouched by this diff, but that is reading, not running.

Non-blocking nit (approval rides on it — builder's discretion)

Following from that second gap: a fifth case with a manifest entry carrying an explicit "github_token" would close the loop on criterion 5 in the direction that actually matters — proving the token does reach payload.auth_token (assert it in the captured request body) while never appearing in stdout/stderr. Today the token-redaction assertions only ever run against a payload that has no auth_token in it, so they would still pass if the redaction were removed. Worth adding whenever import-batch is next touched; nothing here is wrong without it.

## Approve — head `4333ce63bf5f5761247094a3fc798b69239c4b20` The fix is the one #65 specified: the payload construction, and with it `auth_token: resolveSourceToken(...)`, moves inside the existing per-item `try` at `src/cli.js:664`. Nothing else in the loop, the message wording, the summary, or the exit code changed — which is exactly right, because those were already correct and are what the change restores. `resolveSourceToken` and the `cachedGhToken` memo are untouched, and no negative caching was smuggled in. ### Acceptance criteria — all verified at this head Reviewed in a detached worktree (`npm ci` first — the worktree is otherwise dependency-less and every spawn test phantom-fails). | # | Criterion | Evidence | |---|---|---| | 1 | Mixed manifest: item 2 attempted, `Failed to import <first>: No GitHub token found…`, `Batch complete: 1/2 imported.`, exit 1 | test 1 asserts all four, including `stdout` by exact string equality | | 2 | All-importable output and exit 0 byte-identical | test 2, exact `stdout`/`stderr` equality | | 3 | Missing / malformed manifest still `Batch import failed:` + exit 1 | test 3, both cases, and asserts `stdout` stayed empty | | 4 | Skipped invalid entries still `continue` without entering `results` | test 4 — `Batch complete: 1/1`, not `1/2`, is the discriminating assertion | | 5 | No token value in output or captured data | `assert.doesNotMatch` on the forge token over `stdout+stderr`, plus a `deepEqual` on the whole request body that pins the *absence* of `auth_token` for the `git`-service entry | | 6 | Diff touches exactly three paths | `git diff --stat 74e52b4..HEAD` → `changelog.d/65.md`, `src/cli.js`, `test/import-batch.test.js` | | 7 | `npm test` passes, `ci / test` green | 153/153 locally; `ci / test (pull_request)` and `labels / labels (pull_request)` both `success` on this SHA | ### The failure cases actually fail I did not take the worklog's mutation claim on faith — I ran four mutants, one targeting each test, and confirmed each fails *only* its own test: | Mutant | Result | |---|---| | Hoist the payload back above the per-item `try` (the pre-fix code) | `not ok 1`, 2–4 pass | | `results.push({status:'failed'})` before the `Skipping invalid manifest entry` `continue` | `not ok 4`, 1–3 pass | | Swallow `JSON.parse` errors into an empty manifest | `not ok 3`, 1/2/4 pass | | Drop the `\n` prefix on the `Batch complete:` line | `not ok 1` and `not ok 2` | The first is the one that matters: it reproduces the exact defect #65 describes and test 1 catches it. Test 1 also clears the test plan's three named traps — two entries, not one; it asserts the summary line and the second import, not just the exit code (which was `1` before the fix too); and it does not lean on ambient credentials. **Hermeticity, checked rather than assumed.** Test 1 forces the token failure with `PATH` pointed at an empty directory and `GITHUB_TOKEN: undefined` in the spawn env. The latter relies on Node dropping `undefined`-valued keys when building `envPairs`, which is easy to get wrong, so I re-ran the file with `GITHUB_TOKEN=polluted-ambient-token` exported in the ambient environment: still 4/4. The scrubbing holds, and a developer with `gh` on `PATH` and a token in the environment gets the same result I did. ### Other checks - `npm run check:governance` — 4 identities resolved, 5 scope rows valid. - `changelog.d/65.md` matches the established fragment shape (`- <sentence>. (#65).`), consistent with 57/60/62/64. - `Refs #65` is the shape #65 sanctions ("a `Closes` is equally correct here"). I grepped the whole PR body for a closing keyword preceding an issue number: the only two `#65` occurrences are `Refs #65` and `Claim #65 and post the implementation plan`. Neither closes the issue; triage keeps that call. - `scope:cli` + `scope:packaging` match `.github/labeler.yml` for the changed paths (`test/**` maps to no scope, which is the config's behaviour, not this PR's doing). - `README.md`'s `import-batch` section documents options, manifest shape and the endpoint — it never claimed the batch aborts on a token failure, so no prose contract went stale here. ### What I could not verify - **The real migration endpoint.** Every import in these tests goes to a local `http` stub that always answers `201`; I did not exercise a live Forgejo `POST /api/v1/repos/migrate`. That is the test plan's own instruction ("no network"), and the change does not touch the request or the client, so the stub is sufficient for what is under review. - **A successful `service: 'github'` import.** No test drives the path where `resolveSourceToken` *returns* a token and `auth_token` reaches the payload. I read `resolveSourceToken` at `src/cli.js:346` to confirm the token-supplied branch (`if (tokenOption) return tokenOption;`) is untouched by this diff, but that is reading, not running. ### Non-blocking nit (approval rides on it — builder's discretion) Following from that second gap: a fifth case with a manifest entry carrying an explicit `"github_token"` would close the loop on criterion 5 in the direction that actually matters — proving the token *does* reach `payload.auth_token` (assert it in the captured request body) while *never* appearing in `stdout`/`stderr`. Today the token-redaction assertions only ever run against a payload that has no `auth_token` in it, so they would still pass if the redaction were removed. Worth adding whenever `import-batch` is next touched; nothing here is wrong without it.

kimi-bot-andresmgsl approved these changes 2026-09-04 07:24:18 +00:00

Dismissed

kimi-bot-andresmgsl left a comment

Member

Copy link

Approved at head 4333ce63bf.

Reviewed the whole PR in a detached worktree against #65's spec. Verified, not assumed:

• Fix shape: payload construction including auth_token: resolveSourceToken(...) moved inside the per-item try (src/cli.js:658-688); loop, per-item catch wording, summary line and exit code untouched; cachedGhToken memo unchanged (no negative caching added); action-level Batch import failed: catch retained for manifest file/JSON failures. Exactly the spec's decision, no silent riders.
• Mixed-manifest criterion: test 1 drives a real child CLI with PATH pointed at an empty dir and GITHUB_TOKEN unset against the local http stub, and asserts exit 1, Failed to import missing-token: No GitHub token found. on stderr, byte-exact stdout ending Batch complete: 1/2 imported., and that only the second entry reached the server. The test plan's three inadequate-case traps are all avoided (two entries, not one; second-attempt + summary asserted, not exit code alone; no reliance on ambient gh or GITHUB_TOKEN).
• Remaining criteria: all-success output asserted byte-exact with exit 0 (test 2); missing/malformed manifest still exits 1 via the action-level Batch import failed: path (test 3); invalid entries still skipped without entering results (test 4, Batch complete: 1/1); no token value in output or captured payloads (test 1's doesNotMatch over stdout+stderr plus the captured request bodies, which carry no auth_token for non-GitHub items).
• Mutation proof, run by me: restored the pre-fix src/cli.js (git show 74e52b4:src/cli.js) and re-ran test/import-batch.test.js — exactly the mixed-manifest test failed, the other three passed. The regression test genuinely discriminates the defect.
• Full suite: npm test 153/153 pass locally on this head (node v22.17.0, deps installed via npm ci; an initial run in the fresh worktree failed only because node_modules was absent, not because of the change). npm run check:governance passes; git diff --check clean; the diff touches exactly src/cli.js, test/import-batch.test.js, changelog.d/65.md; fragment format matches the existing changelog.d entries.
• ci / test is green on this head per the commit status endpoint (state: success).

Could not verify: the CI job logs themselves — I read only the commit status endpoint plus my local full-suite run, which agree. Nothing else was left to opinion.

Approved at head 4333ce63bf5f5761247094a3fc798b69239c4b20. Reviewed the whole PR in a detached worktree against #65's spec. Verified, not assumed: - Fix shape: payload construction including `auth_token: resolveSourceToken(...)` moved inside the per-item `try` (src/cli.js:658-688); loop, per-item catch wording, summary line and exit code untouched; `cachedGhToken` memo unchanged (no negative caching added); action-level `Batch import failed:` catch retained for manifest file/JSON failures. Exactly the spec's decision, no silent riders. - Mixed-manifest criterion: test 1 drives a real child CLI with PATH pointed at an empty dir and GITHUB_TOKEN unset against the local http stub, and asserts exit 1, `Failed to import missing-token: No GitHub token found.` on stderr, byte-exact stdout ending `Batch complete: 1/2 imported.`, and that only the second entry reached the server. The test plan's three inadequate-case traps are all avoided (two entries, not one; second-attempt + summary asserted, not exit code alone; no reliance on ambient `gh` or GITHUB_TOKEN). - Remaining criteria: all-success output asserted byte-exact with exit 0 (test 2); missing/malformed manifest still exits 1 via the action-level `Batch import failed:` path (test 3); invalid entries still skipped without entering results (test 4, `Batch complete: 1/1`); no token value in output or captured payloads (test 1's `doesNotMatch` over stdout+stderr plus the captured request bodies, which carry no `auth_token` for non-GitHub items). - Mutation proof, run by me: restored the pre-fix src/cli.js (`git show 74e52b4:src/cli.js`) and re-ran test/import-batch.test.js — exactly the mixed-manifest test failed, the other three passed. The regression test genuinely discriminates the defect. - Full suite: `npm test` 153/153 pass locally on this head (node v22.17.0, deps installed via `npm ci`; an initial run in the fresh worktree failed only because node_modules was absent, not because of the change). `npm run check:governance` passes; `git diff --check` clean; the diff touches exactly `src/cli.js`, `test/import-batch.test.js`, `changelog.d/65.md`; fragment format matches the existing changelog.d entries. - `ci / test` is green on this head per the commit status endpoint (state: success). Could not verify: the CI job logs themselves — I read only the commit status endpoint plus my local full-suite run, which agree. Nothing else was left to opinion.

glm-bot-andresmgsl requested changes 2026-09-04 07:25:39 +00:00

Dismissed

glm-bot-andresmgsl left a comment

Member

Copy link

Verdict: request changes — one test-portability blocker; the product fix itself is correct and verified against every other criterion.

What I verified (ran, not opined) — at head 4333ce63 in a detached throwaway worktree, Node v22.12.0 (this box's only node):

• Mutation proof: with the pre-fix src/cli.js (the tree of 721ba34), repo import-batch continues after one item has no source token fails; at the head it passes. The regression test genuinely discriminates the defect — second item attempted, Batch complete: 1/2 imported. printed, exactly one migrate POST captured.
• Full suite at head: 151/153. The two failures are the blocker below; all 149 pre-existing tests pass, so main→head changes only what this PR adds.
• npm run check:governance: 4 identities, 5 scope rows — passes.
• Diff touches exactly src/cli.js, test/import-batch.test.js, changelog.d/65.md (criterion 6 OK); git diff --check clean.
• The fix is the spec's decision verbatim: payload construction (including auth_token: resolveSourceToken(...)) moved inside the per-item try; cachedGhToken memo untouched (no negative caching); the action-level catch still owns manifest file/JSON errors (test 3 green — criterion 3 OK); the skip guard still continues without entering results (criterion 4 OK); no token value in any output or capture (criterion 5 OK).
• ci / test is green on this head.
• Refs #65 hygiene: no closing keyword precedes #65 anywhere in the PR body; triage owns the hand close per the issue.

Blocker (criterion 7: "npm test passes") — on Node 22.12.0, the engines floor this package declares, require()ing the ESM commander 15 makes node print (node:NNNN) ExperimentalWarning: CommonJS module … loading ES Module … using require() to every spawned CLI child's stderr. Two of the new tests assert the exact stderr of spawned children — test/import-batch.test.js:120 (assert.equal(result.stderr, '')) and :179 (exact skip-line equality) — and fail on that runtime. Main is green on the same box, so this PR regresses npm test on a declared-supported runtime; any contributor or fleet box on the floor node gets a red suite. Unblock: keep the strict stderr-contract assertions but keep node diagnostics out of them — e.g. spawn with NODE_OPTIONS: '--disable-warning=ExperimentalWarning' in the harness run() env, or filter ^(node:\d+) .*Warning lines from captured stderr before asserting. Test-side only; src/cli.js needs no change.

Could not verify / relied on instead — no Node newer than 22.12.0 exists on this box, so I could not reproduce the builder's 153/153 locally; I relied on ci / test green at this head (floating node:22-bookworm) plus reading the two failure diffs, which are byte-for-byte the node warning text.

Non-blocking, builder's discretion: the requests deep-equal in the headline test is exactly the right payload pin — keep that pattern when touching the harness.

Verdict: **request changes** — one test-portability blocker; the product fix itself is correct and verified against every other criterion. **What I verified (ran, not opined)** — at head 4333ce63 in a detached throwaway worktree, Node v22.12.0 (this box's only node): - Mutation proof: with the pre-fix `src/cli.js` (the tree of 721ba34), `repo import-batch continues after one item has no source token` fails; at the head it passes. The regression test genuinely discriminates the defect — second item attempted, `Batch complete: 1/2 imported.` printed, exactly one migrate POST captured. - Full suite at head: 151/153. The two failures are the blocker below; all 149 pre-existing tests pass, so main→head changes only what this PR adds. - `npm run check:governance`: 4 identities, 5 scope rows — passes. - Diff touches exactly `src/cli.js`, `test/import-batch.test.js`, `changelog.d/65.md` (criterion 6 OK); `git diff --check` clean. - The fix is the spec's decision verbatim: payload construction (including `auth_token: resolveSourceToken(...)`) moved inside the per-item `try`; `cachedGhToken` memo untouched (no negative caching); the action-level `catch` still owns manifest file/JSON errors (test 3 green — criterion 3 OK); the skip guard still `continue`s without entering results (criterion 4 OK); no token value in any output or capture (criterion 5 OK). - `ci / test` is green on this head. - `Refs #65` hygiene: no closing keyword precedes `#65` anywhere in the PR body; triage owns the hand close per the issue. **Blocker (criterion 7: "`npm test` passes")** — on Node 22.12.0, the `engines` floor this package declares, `require()`ing the ESM commander 15 makes node print `(node:NNNN) ExperimentalWarning: CommonJS module … loading ES Module … using require()` to every spawned CLI child's stderr. Two of the new tests assert the exact stderr of spawned children — `test/import-batch.test.js:120` (`assert.equal(result.stderr, '')`) and `:179` (exact skip-line equality) — and fail on that runtime. Main is green on the same box, so this PR regresses `npm test` on a declared-supported runtime; any contributor or fleet box on the floor node gets a red suite. Unblock: keep the strict stderr-contract assertions but keep node diagnostics out of them — e.g. spawn with `NODE_OPTIONS: '--disable-warning=ExperimentalWarning'` in the harness `run()` env, or filter `^(node:\d+) .*Warning` lines from captured stderr before asserting. Test-side only; `src/cli.js` needs no change. **Could not verify / relied on instead** — no Node newer than 22.12.0 exists on this box, so I could not reproduce the builder's 153/153 locally; I relied on `ci / test` green at this head (floating `node:22-bookworm`) plus reading the two failure diffs, which are byte-for-byte the node warning text. Non-blocking, builder's discretion: the `requests` deep-equal in the headline test is exactly the right payload pin — keep that pattern when touching the harness.

codex-bot-andresmgsl changed title from fix: keep import-batch running after token failures to WIP: fix: keep import-batch running after token failures 2026-09-04 07:25:41 +00:00

glm-bot-andresmgsl added the

state:addressing

label 2026-09-04 07:25:56 +00:00

forgejo-actions removed the

state:bots-reviewing

label 2026-09-04 07:26:10 +00:00

codex-bot-andresmgsl commented 2026-09-04 07:26:55 +00:00

Author

Member

Copy link

🔧 addressing round on head 4333ce63bf

Round analysis:

• GLM — agree (blocking): Node 22.12.0 emits the CommonJS→ESM ExperimentalWarning into spawned-child stderr, so the two exact stderr assertions fail at the package's declared engine floor even though CI's floating Node 22 passes. I will keep strict application-stderr assertions and suppress only Node's ExperimentalWarning in the test harness spawn environment, then run the focused tests and full suite on the available floor runtime.
• GLM — agree (non-blocking): preserve the headline test's whole-request deepEqual payload pin while changing the harness.
• Claude — agree (non-blocking): add an explicit github_token success case that proves the token reaches payload.auth_token but never appears in stdout/stderr. This closes the stated coverage gap without changing production behavior or diff scope.
• Claude — agree (verification boundary): no live migration endpoint test is needed; the issue's no-network plan and unchanged request client make the local stub the correct boundary. I will retain that boundary.
• Kimi — agree: preserve the discriminating mixed-manifest mutation coverage and exact request-body assertions; no production-code change is needed for this round.
• Kimi — agree (verification boundary): CI logs were not directly inspected, but local full-suite verification plus the head status is sufficient; this fix will be verified locally before handoff.

I will append these fix steps to the Worklog, push the test-only correction and coverage, reply once covering the whole round, then signal the final head while the PR is still draft before marking it ready.

🔧 addressing round on head 4333ce63bf5f5761247094a3fc798b69239c4b20 Round analysis: - GLM — **agree (blocking):** Node 22.12.0 emits the CommonJS→ESM `ExperimentalWarning` into spawned-child stderr, so the two exact stderr assertions fail at the package's declared engine floor even though CI's floating Node 22 passes. I will keep strict application-stderr assertions and suppress only Node's `ExperimentalWarning` in the test harness spawn environment, then run the focused tests and full suite on the available floor runtime. - GLM — **agree (non-blocking):** preserve the headline test's whole-request `deepEqual` payload pin while changing the harness. - Claude — **agree (non-blocking):** add an explicit `github_token` success case that proves the token reaches `payload.auth_token` but never appears in stdout/stderr. This closes the stated coverage gap without changing production behavior or diff scope. - Claude — **agree (verification boundary):** no live migration endpoint test is needed; the issue's no-network plan and unchanged request client make the local stub the correct boundary. I will retain that boundary. - Kimi — **agree:** preserve the discriminating mixed-manifest mutation coverage and exact request-body assertions; no production-code change is needed for this round. - Kimi — **agree (verification boundary):** CI logs were not directly inspected, but local full-suite verification plus the head status is sufficient; this fix will be verified locally before handoff. I will append these fix steps to the Worklog, push the test-only correction and coverage, reply once covering the whole round, then signal the final head while the PR is still draft before marking it ready.

codex-bot-andresmgsl added 1 commit 2026-09-04 07:30:37 +00:00

test: support node engine floor 82494e94fd

codex-bot-andresmgsl added 1 commit 2026-09-04 07:31:38 +00:00

test: cover explicit import token handling 37e6a2ad5a

codex-bot-andresmgsl commented 2026-09-04 07:33:26 +00:00

Author

Member

Copy link

Round reply for head 37e6a2ad5a0141a344272004d0ef5c3d85d6de17:

• GLM blocker: fixed in 82494e9. The child-process harness now appends only --disable-warning=ExperimentalWarning to NODE_OPTIONS, keeping Node 22.12.0's Commander interop diagnostic out of application stderr while preserving strict stderr assertions and every other warning category. The exact floor-runtime reproduction moved from 2/4 to 4/4; the final focused file is 5/5 and the full Node 22.12.0 suite is 154/154.
• GLM non-blocking point: preserved the headline test's whole-request deepEqual payload assertion unchanged.
• Claude non-blocking point: added an explicit github_token success case in 37e6a2a. It asserts the complete captured migration body includes auth_token and independently asserts the token is absent from stdout and stderr. Removing auth_token from production made this test fail on the missing field; restoring it returned the test to green.
• Claude verification boundary: retained the issue-mandated no-network local HTTP stub. The migration client and endpoint behavior remain outside this diff.
• Kimi points: retained the mixed-manifest discriminator and exact request-body assertions; this round changes tests only, with production behavior untouched.
• Verification boundary: ran the focused test and full suite locally on exact Node 22.12.0, plus governance and diff checks. The diff remains exactly src/cli.js, test/import-batch.test.js, and changelog.d/65.md; the worktree is clean and the remote PR head matches locally.

This answers every blocking and non-blocking review point in the completed round.

Round reply for head `37e6a2ad5a0141a344272004d0ef5c3d85d6de17`: - GLM blocker: fixed in `82494e9`. The child-process harness now appends only `--disable-warning=ExperimentalWarning` to `NODE_OPTIONS`, keeping Node 22.12.0's Commander interop diagnostic out of application stderr while preserving strict stderr assertions and every other warning category. The exact floor-runtime reproduction moved from 2/4 to 4/4; the final focused file is 5/5 and the full Node 22.12.0 suite is 154/154. - GLM non-blocking point: preserved the headline test's whole-request `deepEqual` payload assertion unchanged. - Claude non-blocking point: added an explicit `github_token` success case in `37e6a2a`. It asserts the complete captured migration body includes `auth_token` and independently asserts the token is absent from stdout and stderr. Removing `auth_token` from production made this test fail on the missing field; restoring it returned the test to green. - Claude verification boundary: retained the issue-mandated no-network local HTTP stub. The migration client and endpoint behavior remain outside this diff. - Kimi points: retained the mixed-manifest discriminator and exact request-body assertions; this round changes tests only, with production behavior untouched. - Verification boundary: ran the focused test and full suite locally on exact Node 22.12.0, plus governance and diff checks. The diff remains exactly `src/cli.js`, `test/import-batch.test.js`, and `changelog.d/65.md`; the worktree is clean and the remote PR head matches locally. This answers every blocking and non-blocking review point in the completed round.

codex-bot-andresmgsl commented 2026-09-04 07:34:07 +00:00

Author

Member

Copy link

📣 round answered at head 37e6a2ad5a

📣 round answered at head 37e6a2ad5a0141a344272004d0ef5c3d85d6de17

codex-bot-andresmgsl changed title from WIP: fix: keep import-batch running after token failures to fix: keep import-batch running after token failures 2026-09-04 07:34:08 +00:00

claude-bot-andresmgsl approved these changes 2026-09-04 07:40:37 +00:00

claude-bot-andresmgsl left a comment

Member

Copy link

Approve — head 37e6a2ad5a0141a344272004d0ef5c3d85d6de17

Round 2. I re-reviewed the whole PR at this head in a detached worktree, not just the delta. The production diff is byte-for-byte the one I approved at 4333ce63 — 82494e9 and 37e6a2a are test-only — so this round is about whether GLM's floor-portability blocker is actually fixed and whether the new coverage discriminates. Both hold.

GLM's blocker, verified by construction. The box has only Node v22.23.2, so I cannot reproduce Node 22.12.0's require(esm) ExperimentalWarning directly. Instead I injected an equivalent warning into the spawned child through the same channel the harness uses, and drove the real test file:

• NODE_OPTIONS="--require /tmp/emitwarn.js" node --test test/import-batch.test.js (the required module emits an ExperimentalWarning) — 5/5 pass with the harness as shipped.
• Same command with the --disable-warning=ExperimentalWarning append at test/import-batch.test.js:14-17 deleted — tests 2, 3 and 5 fail, the strict-stderr trio, which is exactly the failure shape GLM reported at the floor.

So the harness change is load-bearing and does what it claims. It is also narrow, which was the thing worth checking: injecting a DeprecationWarning the same way still breaks the same three tests, and a standalone probe confirmed --disable-warning=ExperimentalWarning in NODE_OPTIONS leaves DeprecationWarning on stderr. The assertions stayed strict; only one warning category is masked.

Mutation proof — 6 mutants, each fails only what it should.

Mutant	Fails
Payload + token resolution hoisted back above the per-item try (pre-fix shape)	1 only
auth_token: line deleted from the payload	1, 3
payload.auth_token interpolated into the Imported: line	1, 2, 3
Batch import failed: console.error deleted	4 only
Skipped invalid entries pushed into results	5 only
--disable-warning append removed (under injected warning)	2, 3, 5

The token-leak mutant is the one that matters for the new test 3: it is caught by both the exact-stdout assertion and the independent assert.doesNotMatch on the token, so the redaction claim is not resting on the stdout string alone.

Acceptance criteria (#65), all seven met.

1. Mixed manifest — second item attempted, Failed to import missing-token: No GitHub token found., Batch complete: 1/2 imported., exit 1. Test 1, and mutant 1 shows it is the discriminator.
2. All-importable output and exit 0 byte-identical — test 2 asserts exact stdout and empty stderr.
3. Missing / malformed manifest still exits 1 via the action-level catch with Batch import failed: — test 4, anchored /^Batch import failed: ENOENT:/.
4. Skipping invalid manifest entry still continues without entering results — test 5, and mutant 5 confirms.
5. No token value in output or captured data — test 3 pins the whole request body (auth_token: sourceToken present) and asserts the token is absent from stdout+stderr; test 1 does the same for the forge token.
6. Diff touches exactly src/cli.js, test/import-batch.test.js, changelog.d/65.md — git diff --name-only 74e52b42..HEAD against the merge-base returns those three and nothing else.
7. npm test 154/154 locally; ci / test and labels / labels are both success on this exact head per the commit statuses endpoint.

Spec conformance beyond the criteria: resolveSourceToken and the cachedGhToken memo are untouched (outside the diff hunk entirely), the action-level catch still owns batch-level failures, and service/isPrivate moving inside the try is safe — neither is referenced after the block. Refs #65 is what the issue's Tasks direct, and no closing keyword precedes #65 anywhere in the PR body. Changelog fragment matches the house style of changelog.d/57,60,62,64. npm run check:governance clean (4 identities, 5 scope rows); git diff --check clean; worktree clean.

Hermeticity. Re-ran the file with GITHUB_TOKEN=polluted-ambient-token and a PATH containing a gh stub that prints a token — still 5/5, so the PATH: emptyPath / GITHUB_TOKEN: undefined scrubbing holds and no test is riding on ambient credentials.

What I could not verify, and what I relied on instead.

• Node 22.12.0 itself. Only v22.23.2 is installed here and no version manager is present, so I could not run the engines-floor runtime. I relied on the injection experiment above (which reproduces the mechanism, not the exact Node build), on the builder's reported 154/154 on exact 22.12.0, and on --disable-warning being available since v21.3.0 — well below the >=22.12.0 floor. If that flag were somehow rejected at 22.12.0 the child would fail to start and every test would fail loudly, not silently pass, so the failure mode is safe.
• CI does not exercise the floor. .forgejo/workflows/ci.yml runs node:22-bookworm, i.e. floating Node 22, so green CI is not evidence about 22.12.0 either — it is the same gap that let this land in round 1. Non-blocking and out of scope for #65; if the panel wants the floor pinned in CI that is a proposal, not a change to this PR.
• No live migration endpoint was exercised; the issue's test plan mandates the local http stub, and the migration client is outside this diff. That boundary is correct.

Nothing blocking. Nice round: the fix answered GLM's point without loosening a single assertion.

## Approve — head `37e6a2ad5a0141a344272004d0ef5c3d85d6de17` Round 2. I re-reviewed the whole PR at this head in a detached worktree, not just the delta. The production diff is byte-for-byte the one I approved at `4333ce63` — `82494e9` and `37e6a2a` are test-only — so this round is about whether GLM's floor-portability blocker is actually fixed and whether the new coverage discriminates. Both hold. **GLM's blocker, verified by construction.** The box has only Node v22.23.2, so I cannot reproduce Node 22.12.0's `require(esm)` `ExperimentalWarning` directly. Instead I injected an equivalent warning into the spawned child through the same channel the harness uses, and drove the real test file: - `NODE_OPTIONS="--require /tmp/emitwarn.js" node --test test/import-batch.test.js` (the required module emits an `ExperimentalWarning`) — **5/5 pass** with the harness as shipped. - Same command with the `--disable-warning=ExperimentalWarning` append at `test/import-batch.test.js:14-17` deleted — **tests 2, 3 and 5 fail**, the strict-stderr trio, which is exactly the failure shape GLM reported at the floor. So the harness change is load-bearing and does what it claims. It is also narrow, which was the thing worth checking: injecting a `DeprecationWarning` the same way still breaks the same three tests, and a standalone probe confirmed `--disable-warning=ExperimentalWarning` in `NODE_OPTIONS` leaves `DeprecationWarning` on stderr. The assertions stayed strict; only one warning category is masked. **Mutation proof — 6 mutants, each fails only what it should.** | Mutant | Fails | | --- | --- | | Payload + token resolution hoisted back above the per-item `try` (pre-fix shape) | 1 only | | `auth_token:` line deleted from the payload | 1, 3 | | `payload.auth_token` interpolated into the `Imported:` line | 1, 2, 3 | | `Batch import failed:` console.error deleted | 4 only | | Skipped invalid entries pushed into `results` | 5 only | | `--disable-warning` append removed (under injected warning) | 2, 3, 5 | The token-leak mutant is the one that matters for the new test 3: it is caught by both the exact-stdout assertion and the independent `assert.doesNotMatch` on the token, so the redaction claim is not resting on the stdout string alone. **Acceptance criteria (#65), all seven met.** 1. Mixed manifest — second item attempted, `Failed to import missing-token: No GitHub token found.`, `Batch complete: 1/2 imported.`, exit 1. Test 1, and mutant 1 shows it is the discriminator. 2. All-importable output and exit 0 byte-identical — test 2 asserts exact stdout and empty stderr. 3. Missing / malformed manifest still exits 1 via the action-level catch with `Batch import failed:` — test 4, anchored `/^Batch import failed: ENOENT:/`. 4. `Skipping invalid manifest entry` still `continue`s without entering `results` — test 5, and mutant 5 confirms. 5. No token value in output or captured data — test 3 pins the whole request body (`auth_token: sourceToken` present) and asserts the token is absent from stdout+stderr; test 1 does the same for the forge token. 6. Diff touches exactly `src/cli.js`, `test/import-batch.test.js`, `changelog.d/65.md` — `git diff --name-only 74e52b42..HEAD` against the merge-base returns those three and nothing else. 7. `npm test` 154/154 locally; `ci / test` and `labels / labels` are both `success` on this exact head per the commit statuses endpoint. Spec conformance beyond the criteria: `resolveSourceToken` and the `cachedGhToken` memo are untouched (outside the diff hunk entirely), the action-level catch still owns batch-level failures, and `service`/`isPrivate` moving inside the `try` is safe — neither is referenced after the block. `Refs #65` is what the issue's Tasks direct, and no closing keyword precedes `#65` anywhere in the PR body. Changelog fragment matches the house style of `changelog.d/57,60,62,64`. `npm run check:governance` clean (4 identities, 5 scope rows); `git diff --check` clean; worktree clean. **Hermeticity.** Re-ran the file with `GITHUB_TOKEN=polluted-ambient-token` and a `PATH` containing a `gh` stub that prints a token — still 5/5, so the `PATH: emptyPath` / `GITHUB_TOKEN: undefined` scrubbing holds and no test is riding on ambient credentials. **What I could not verify, and what I relied on instead.** - **Node 22.12.0 itself.** Only v22.23.2 is installed here and no version manager is present, so I could not run the engines-floor runtime. I relied on the injection experiment above (which reproduces the mechanism, not the exact Node build), on the builder's reported 154/154 on exact 22.12.0, and on `--disable-warning` being available since v21.3.0 — well below the `>=22.12.0` floor. If that flag were somehow rejected at 22.12.0 the child would fail to start and every test would fail loudly, not silently pass, so the failure mode is safe. - **CI does not exercise the floor.** `.forgejo/workflows/ci.yml` runs `node:22-bookworm`, i.e. floating Node 22, so green CI is not evidence about 22.12.0 either — it is the same gap that let this land in round 1. Non-blocking and out of scope for #65; if the panel wants the floor pinned in CI that is a proposal, not a change to this PR. - **No live migration endpoint** was exercised; the issue's test plan mandates the local `http` stub, and the migration client is outside this diff. That boundary is correct. Nothing blocking. Nice round: the fix answered GLM's point without loosening a single assertion.

codex-bot-andresmgsl requested review from glm-bot-andresmgsl 2026-09-04 07:40:55 +00:00

codex-bot-andresmgsl requested review from kimi-bot-andresmgsl 2026-09-04 07:40:56 +00:00

codex-bot-andresmgsl added the

state:bots-reviewing

label 2026-09-04 07:40:56 +00:00

forgejo-actions removed the

state:addressing

label 2026-09-04 07:41:14 +00:00

glm-bot-andresmgsl approved these changes 2026-09-04 07:46:07 +00:00

glm-bot-andresmgsl left a comment

Member

Copy link

Verdict: approve — every acceptance criterion of #65 is met at this head; the prior round's blocker is answered exactly as prescribed.

What I verified (ran, not opined) — at head 37e6a2a in a detached throwaway worktree, Node v22.12.0 (this box's only node, and the engines floor), fresh npm ci of the lockfile (commander 15.0.0):

• Full suite: 154/154 (npm test). node --test test/import-batch.test.js: 5/5.
• Mutation proof 1: with the pre-fix src/cli.js (tree of 1bb4bd6^), only repo import-batch continues after one item has no source token fails — the discriminating evidence the issue's test plan demands (second entry attempted, summary printed), not just an exit code.
• Mutation proof 2: with auth_token dropped from the payload, the new explicit-token test fails on the missing field.
• Mutation proof 3: with --disable-warning=ExperimentalWarning removed from the harness, all 5 tests still pass on this install — node src/cli.js --help emits zero stderr here, so last round's require(esm) contamination does not reproduce on a faithful lockfile install of this box. The flag stays as belt-and-braces for runtimes where the diagnostic does fire: supported since Node 21.3.0 (below the floor), merges with rather than clobbers ambient NODE_OPTIONS, and keeps the strict stderr assertions application-only. That was unblock option A from my round, taken verbatim.
• Criterion sweep: 1) mixed-manifest test asserts exit 1, Failed to import missing-token: No GitHub token found., Batch complete: 1/2 imported., and the captured POSTs prove only the second item was imported; 2) all-success output/exit pinned byte-exactly and passes against both pre-fix and post-fix src (byte-identity across the change); 3) missing/invalid manifest files still hit Batch import failed: with exit 1; 4) skip guard still continues without entering results (1/1 imported.); 5) doesNotMatch on both forge and source tokens across stdout+stderr, and the captured payload pins auth_token delivery; 6) git diff main...HEAD touches exactly src/cli.js, test/import-batch.test.js, changelog.d/65.md, git diff --check clean; 7) ci / test green on this head (combined commit status success) and npm test green locally on the floor runtime.
• npm run check:governance: 4 identities, 5 scope rows valid. Changelog fragment matches house style.
• Spec fidelity: the whole payload construction moved inside the per-item try (the spec's "if cleaner" option); cachedGhToken memo untouched, no negative-caching rider; the action-level catch still owns batch-level errors; all out-of-scope regions untouched.
• Refs #65 hygiene: no closing keyword precedes #65 anywhere in the PR body; triage owns the hand close.

Could not verify / relied on instead — no Node newer than 22.12.0 on this box, so the builder's run on a newer runtime is taken from the worklog plus green ci / test at this head; the Forgejo actions-runs endpoint 404s for me, so CI is evidenced by the combined commit-status endpoint (success for ci / test at 37e6a2a), not by reading job logs.

Non-blocking, builder's discretion: none this round — the explicit-token test's payload deep-equal is exactly the pin to keep.

Verdict: **approve** — every acceptance criterion of #65 is met at this head; the prior round's blocker is answered exactly as prescribed. **What I verified (ran, not opined)** — at head 37e6a2a in a detached throwaway worktree, Node v22.12.0 (this box's only node, and the `engines` floor), fresh `npm ci` of the lockfile (commander 15.0.0): - Full suite: **154/154** (`npm test`). `node --test test/import-batch.test.js`: 5/5. - Mutation proof 1: with the pre-fix `src/cli.js` (tree of 1bb4bd6^), only `repo import-batch continues after one item has no source token` fails — the discriminating evidence the issue's test plan demands (second entry attempted, summary printed), not just an exit code. - Mutation proof 2: with `auth_token` dropped from the payload, the new explicit-token test fails on the missing field. - Mutation proof 3: with `--disable-warning=ExperimentalWarning` removed from the harness, all 5 tests still pass on this install — `node src/cli.js --help` emits zero stderr here, so last round's require(esm) contamination does not reproduce on a faithful lockfile install of this box. The flag stays as belt-and-braces for runtimes where the diagnostic does fire: supported since Node 21.3.0 (below the floor), merges with rather than clobbers ambient `NODE_OPTIONS`, and keeps the strict stderr assertions application-only. That was unblock option A from my round, taken verbatim. - Criterion sweep: 1) mixed-manifest test asserts exit 1, `Failed to import missing-token: No GitHub token found.`, `Batch complete: 1/2 imported.`, and the captured POSTs prove only the second item was imported; 2) all-success output/exit pinned byte-exactly and passes against both pre-fix and post-fix src (byte-identity across the change); 3) missing/invalid manifest files still hit `Batch import failed:` with exit 1; 4) skip guard still `continue`s without entering results (`1/1 imported.`); 5) `doesNotMatch` on both forge and source tokens across stdout+stderr, and the captured payload pins `auth_token` delivery; 6) `git diff main...HEAD` touches exactly `src/cli.js`, `test/import-batch.test.js`, `changelog.d/65.md`, `git diff --check` clean; 7) `ci / test` green on this head (combined commit status `success`) and `npm test` green locally on the floor runtime. - `npm run check:governance`: 4 identities, 5 scope rows valid. Changelog fragment matches house style. - Spec fidelity: the whole payload construction moved inside the per-item `try` (the spec's "if cleaner" option); `cachedGhToken` memo untouched, no negative-caching rider; the action-level `catch` still owns batch-level errors; all out-of-scope regions untouched. - `Refs #65` hygiene: no closing keyword precedes `#65` anywhere in the PR body; triage owns the hand close. **Could not verify / relied on instead** — no Node newer than 22.12.0 on this box, so the builder's run on a newer runtime is taken from the worklog plus green `ci / test` at this head; the Forgejo actions-runs endpoint 404s for me, so CI is evidenced by the combined commit-status endpoint (`success` for `ci / test` at 37e6a2a), not by reading job logs. Non-blocking, builder's discretion: none this round — the explicit-token test's payload deep-equal is exactly the pin to keep.

kimi-bot-andresmgsl approved these changes 2026-09-04 07:47:00 +00:00

kimi-bot-andresmgsl left a comment

Member

Copy link

Approved at head 37e6a2ad5a.

I previously approved 4333ce63; this head adds only two test-file commits (82494e9 engine-floor warning guard, 37e6a2a explicit-token coverage). I reviewed the whole PR at this head anyway.

Against #65's acceptance criteria, all verified:

• Missing-token isolation: test/import-batch.test.js drives a two-entry mixed manifest with PATH emptied and GITHUB_TOKEN unset — the second item is attempted, Failed to import missing-token: No GitHub token found. is asserted, Batch complete: 1/2 imported. prints, exit 1. The test has two entries and asserts the summary, so it distinguishes "batch continued" from "died on the last item", and it cannot rely on ambient gh credentials.
• Byte-identical all-success path: the fix is a pure move of the payload construction (including auth_token: resolveSourceToken(...)) inside the per-item try; the success path is unchanged code and the exact-output test passes against both the fixed tree and the base tree.
• Batch-level failures preserved: missing file (ENOENT) and malformed JSON still exit 1 via Batch import failed:, asserted.
• Invalid-entry skip: Skipping invalid manifest entry still continues without entering results, asserted.
• No token leakage: both the forge config token and the explicit GitHub token are asserted absent from stdout+stderr.
• Scope: git diff origin/main...HEAD --name-only = exactly src/cli.js, test/import-batch.test.js, changelog.d/65.md; cachedGhToken memo untouched (no negative caching added).
• Changelog fragment present; CommonJS / node:test conventions followed.

Ran, not opined (Node 22.23.2, after npm ci): the new test file 5/5, full npm test 154/154, npm run check:governance green, git diff --check clean. Mutation proof: with the base (defective) src/cli.js, exactly the mixed-manifest test fails (4 pass / 1 fail) — the failure cases genuinely fail. Commit status on this head: ci / test (pull_request) success and labels / labels success.

One environment note: I ran on Node 22.23.2, not the exact 22.12.0 engine floor; for the floor I rely on the 82494e9 --disable-warning=ExperimentalWarning guard plus green ci / test on this head rather than a 22.12.0 run of my own.

The PR body uses Refs #65 with no closing keyword anywhere — correct shape. Nothing blocking.

Approved at head 37e6a2ad5a0141a344272004d0ef5c3d85d6de17. I previously approved 4333ce63; this head adds only two test-file commits (`82494e9` engine-floor warning guard, `37e6a2a` explicit-token coverage). I reviewed the whole PR at this head anyway. Against #65's acceptance criteria, all verified: - Missing-token isolation: `test/import-batch.test.js` drives a two-entry mixed manifest with `PATH` emptied and `GITHUB_TOKEN` unset — the second item is attempted, `Failed to import missing-token: No GitHub token found.` is asserted, `Batch complete: 1/2 imported.` prints, exit 1. The test has two entries and asserts the summary, so it distinguishes "batch continued" from "died on the last item", and it cannot rely on ambient `gh` credentials. - Byte-identical all-success path: the fix is a pure move of the payload construction (including `auth_token: resolveSourceToken(...)`) inside the per-item `try`; the success path is unchanged code and the exact-output test passes against both the fixed tree and the base tree. - Batch-level failures preserved: missing file (`ENOENT`) and malformed JSON still exit 1 via `Batch import failed:`, asserted. - Invalid-entry skip: `Skipping invalid manifest entry` still continues without entering `results`, asserted. - No token leakage: both the forge config token and the explicit GitHub token are asserted absent from stdout+stderr. - Scope: `git diff origin/main...HEAD --name-only` = exactly `src/cli.js`, `test/import-batch.test.js`, `changelog.d/65.md`; `cachedGhToken` memo untouched (no negative caching added). - Changelog fragment present; CommonJS / `node:test` conventions followed. Ran, not opined (Node 22.23.2, after `npm ci`): the new test file 5/5, full `npm test` 154/154, `npm run check:governance` green, `git diff --check` clean. Mutation proof: with the base (defective) `src/cli.js`, exactly the mixed-manifest test fails (4 pass / 1 fail) — the failure cases genuinely fail. Commit status on this head: `ci / test (pull_request)` success and `labels / labels` success. One environment note: I ran on Node 22.23.2, not the exact 22.12.0 engine floor; for the floor I rely on the `82494e9` `--disable-warning=ExperimentalWarning` guard plus green `ci / test` on this head rather than a 22.12.0 run of my own. The PR body uses `Refs #65` with no closing keyword anywhere — correct shape. Nothing blocking.

codex-bot-andresmgsl requested review from andres 2026-09-04 07:51:00 +00:00

codex-bot-andresmgsl added the

state:needs-human

label 2026-09-04 07:51:01 +00:00

forgejo-actions removed the

state:bots-reviewing

label 2026-09-04 07:51:13 +00:00

andres merged commit 92f2eb3949 into main 2026-09-04 08:02:45 +00:00

andres referenced this pull request from a commit 2026-09-04 08:02:45 +00:00

Merge pull request 'fix: keep import-batch running after token failures' (#69) from build/65-import-batch-token-failure into main

claude-bot-andresmgsl referenced this pull request 2026-09-04 09:53:06 +00:00

src/cli.js — repo import-batch resolves each item's source token inside that item's try, so one missing token cannot end the batch #65

claude-bot-andresmgsl referenced this pull request 2026-09-04 09:53:37 +00:00

src/cli.js — repo import-batch resolves each item's source token inside that item's try, so one missing token cannot end the batch #65

Sign in to join this conversation.

Reviewers

No reviewers

claude-bot-andresmgsl

glm-bot-andresmgsl

kimi-bot-andresmgsl

andres

Labels

Clear labels   

attention

A demand is parked here for the assignee: pick up the thread, ack by removing this label

  

blocked

Waiting on another PR or issue to land first

  

blocker:ci-red

A check is failing — the agent owes a fix (not a rebase)

  

blocker:conflict

Does not merge — the branch conflicts and the agent owes a rebase

  

blocker:drill-pending

A release PR whose version has no drills/X.Y.Z.md record — correct but unevidenced

  

blocker:unrequested

Somebody still owes a verdict and nobody was asked for one

  

bug

Something isn't working

  

claimed

A builder owns it: assignee set, draft PR expected shortly

  

documentation

Improvements or additions to documentation

  

enhancement

New feature or request

  

epic

Organizes other issues via a dependency-ordered task list — builders never pick it

  

merge-next

Head of the merge queue — merge this one next (set by hand/agent, cleared here)

  

needs-ruling

A human decision is pending — question, options and a recommendation are in the comment

  

needs-triage

Did not come through triage — owes normalization into work or a reasoned refusal

  

offsite

Issue deliverable is a PR in another repository — claim clock paused

  

post-merge

Refs-linked PR merged; post-merge criteria remain and triage owns completion

  

ready

Triaged, spec complete, unblocked — a builder can start now and succeed

  

release

Release flow and version/packaging work

  

scope:ci

.forgejo/workflows/, .github/, and .ceremony/ — the test, label, and governance gates

  

scope:cli

src/ — the command surface (cli.js, api.js, config.js)

  

scope:docs

root Markdown and docs/ — the prose contract

  

scope:manifests

manifests/ — the fleet repo registry data

  

scope:packaging

scripts/, release workflow, package manifests, changelog, and fragments — release packaging and version surfaces

  

stale

No activity for 48h — needs a poke (sweep-managed)

  

state:addressing

All bots reviewed — coding agent owes the single reply + fixes

  

state:bots-reviewing

Waiting on the bot reviewers to finish the round

  

state:building

Pre-round: the builder is still building — draft is evidence for it, not the definition

  

state:needs-human

No blockers, all bots approve — waiting on the human reviewer

No labels

attention

blocked

blocker:ci-red

blocker:conflict

blocker:drill-pending

blocker:unrequested

bug

claimed

documentation

enhancement

epic

merge-next

needs-ruling

needs-triage

offsite

post-merge

ready

release

scope:ci

scope:cli

scope:docs

scope:manifests

scope:packaging

stale

state:addressing

state:bots-reviewing

state:building

state:needs-human

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

4 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: heavy-duty/stoke#69

No description provided.