fix(automate): name App Automate sessions per test instead of only at worker teardown (SDK-7270)

[anish353]

What is this about?

App Automate session names show the static sessionName capability instead of the per-test title. This is the follow-up to #100 — that fix shipped in 9.33.1 and the customer still reproduced on it.

Why #100 didn't resolve it. #100 fixed endpoint routing (isAppAutomate() now detects appium:app, so the PUT targets /app-automate/... instead of 404-ing against /automate/...). That fix is present and working — the customer's 9.33.1 log shows the correct App-Automate endpoint in use. But routing only matters once a rename is actually issued, and in this customer's shape none ever is.

Actual cause. Since 9.27 the SDK self-bootstraps the platform binary, so BrowserstackCLI.getInstance().isRunning() is true on every run. That flips session naming off the legacy per-test path:

• service.ts skips the legacy rename entirely when the CLI runs — beforeSuite is guarded (service.ts:385), beforeTest early-returns before _setSessionName (service.ts:495-500), and _setSessionName's _updateJob({ name }) is itself guarded by !isRunning() (service.ts:1006-1009). Customer log: Update job with sessionId = 0.
• Ownership moves to automateModule, which records the per-test name in sessionMap but issues no PUT until onAfterExecute — reached only from service.ts:584 inside WDIO's after() hook, i.e. once, at worker teardown.

So every session's name depends on a single event at the very end of the worker. Suites that reload the session per test (the customer's does — 28 Session Reloaded in the captured run) have already closed those sessions by then, and a worker that never reaches after() — interrupted run, hard exit, crash — never fires onAfterExecute at all. Customer log confirms: trackEvent: automationFrameworkState=…EXECUTE = 0, ending at Handling CLI cleanup in exit handler. Result: zero renames, every session keeps its creation-time sessionName capability.

Change. Name the session from onBeforeTest, while it is still the live session, via a new flushSessionName() helper de-duped on SessionData.appliedName. onAfterExecute now calls the same helper, becoming a final sweep that no-ops for sessions already named. Restores the pre-9.27 per-test timing; no extra API calls in the steady state.

Verified on real App Automate builds (appium:app capability, reloadSession() per test — the customer's exact shape):

Scenario	Renames issued	Dashboard result
9.33.1, after() reached	3	correct per-test titles
9.33.1, after() not reached	0	all sessions "STATIC CAP NAME…" ← reported symptom
This PR, after() not reached	2	correct per-test titles
This PR, clean run	3 (not 6)	correct; teardown sweep de-dupes

The first row isolates the defect: on shipped code the rename works only if the worker reaches after().

Full vitest suite is identical to the clean-tree control (same 7 files / 70 pre-existing failures with and without this change — zero new); automateModule.test.ts 31/31; build + eslint clean.

Related Jira task/s

https://browserstack.atlassian.net/browse/SDK-7270 (clone of https://browserstack.atlassian.net/browse/SDK-7093)

Release (mandatory for every PR — required for the ready-for-review label)

Version bump: (required — tick exactly one)

• minor (backwards-compatible feature)
• patch (bug fix or other small change)

Release notes type: (optional)

• New Feature
• Bug Fix
• Other Improvement

Release notes (customer-facing): (optional but encouraged)

• Fixed App Automate and Automate session names staying on the static sessionName capability instead of the test title, for suites that reload the session between tests or whose run ends before the WebdriverIO after hook.

Release notes (internal): (required — engineer-facing; what actually changed / why)

• automateModule issued the session-name PUT only from onAfterExecute (WDIO after(), once per worker at teardown). Sessions reloaded per test were already closed by then, and a worker that never reaches after() never named anything. flushSessionName() now fires from onBeforeTest while the session is live, de-duped via SessionData.appliedName; onAfterExecute reuses it as a no-op final sweep.

Checklist

• Ready to review
• Has it been tested locally?

PR Validations

Run Tests: Comment RUN_TESTS to trigger sanity tests.

[anish353]

RUN_TESTS

[github-actions]

🔴 SDK PR Review gate is red. Pending:

• The SDK PR Review Agent has not reviewed the current head commit yet — run the SDK PR Review Agent.

It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge.

[minionhelperappqa]

[SDK Wdio Test] TRA build state: failed | Stability 98% — verdict: success. Passed: 85, Failed: 2, Aggregate: 87. TRA: https://observability.browserstack.com/builds/falsds1jgm1yqrnixtx9fionzmyyxrj9lph4jinf

[anish353]

Claude Code Review (automated) — 5 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[anish353]

[Medium] This records attempted, not applied — so a failed PUT disarms the final sweep

markSessionName never checks response.ok (:274-276 — await fetch(...) → await response.json() → debug-log, no status check) and swallows everything in its catch (:277-279). A 401/404/429/5xx is indistinguishable from success. This line then sets appliedName unconditionally, so the teardown sweep at :220-221 no-ops. The field comment at :28 says "last name successfully PUT" — something the code can't actually know.

Failure scenario: the reload-per-test shape this PR targets puts one test per session. Test A's onBeforeTest PUT hits a 429 or a socket blip → appliedName = "A" → sweep suppressed → that session permanently keeps its creation-time sessionName capability. That's the exact bug this PR fixes, reintroduced under transient failure — while the new "Final sweep" comment advertises a guarantee it no longer provides. It also compounds with the per-test PUT volume: more requests make rate-limiting more likely.

Suggestion: have markSessionName return response.ok and set appliedName only on 2xx. Alternatively, make the teardown sweep unconditional and de-dupe only the per-test calls — that costs one extra PUT per session, which is exactly what main does today, so it's a safe fallback.

Reviewer: fallback independent reviewer

[anish353]

[Medium] Skipped tests also fire TEST/PRE, so a hook failure fans this out into hundreds of blocking PUTs

TEST/PRE isn't fired only by service.ts:498. skipReporter.ts:58 awaits framework.trackEvent(TEST, PRE, …) for every skipped test, and reportSuiteSkipped (:72-92) walks every remaining test and nested suite calling reportSkippedTest when a before all / beforeEach hook fails — all inside the awaited afterHook. Each of those now issues a blocking rename PUT, and because the titles are distinct the appliedName de-dupe collapses none of them.

Failure scenario: a failed before all in a 300-test suite adds ~300 sequential HTTPS PUTs to teardown — roughly a minute at typical latency — inside an awaited WDIO hook. Statically-skipped tests also transiently rename the live session to a test that never ran.

UNVERIFIED: whether this trips the WDIO afterHook timeout — I didn't confirm the configured value. If it's in the tens of seconds, this escalates to High, so worth a check on your side.

Suggestion: pass a marker in reportSkippedTest's TEST/PRE args (e.g. skipped: true) and skip the flush for it; or make the per-test PUT fire-and-forget and let the awaited sweep stay authoritative.

Reviewer: fallback independent reviewer

[anish353]

[Medium] Awaited network round trip now on the per-test hot path, with no timeout

Under Mocha with defaults name embeds the test title (:80-85), so lastTestName changes every test and the de-dupe never suppresses. A conventional suite — one session, N tests, no reload — goes from 1 PUT at teardown to N PUTs, each awaited before the test body runs. _fetch (fetchWrapper.ts:19-35) sets no timeout, no retry, and no abort signal, so a slow or hanging API stalls every test start. At ~150–300 ms that's roughly 1–2.5 min added per worker on a 500-test suite.

This also makes the PR body's "no extra API calls in the steady state" inaccurate for the non-reload case — it holds only when consecutive titles repeat.

Mitigating, and worth saying: this is parity with the legacy non-CLI path — service.ts:503 awaits _setSessionName per test, de-duped via _fullTitle at :1006. So it's restored prior behavior, not a new design.

Suggestion: bound it with AbortSignal.timeout(~5000), or make the onBeforeTest flush fire-and-forget and keep the teardown sweep authoritative.

Reviewer: fallback independent reviewer

[anish353]

[Low] Unguarded testContextOptions deref (pre-existing pattern, reproduced here)

testContextOptions is genuinely reachable as undefined: unConfigureModules() (cli/index.ts:427) re-calls configure() with no 4th arg so config defaults to {}; setConfig swallows a parse failure leaving {}; and the binary returns a degenerate config on auth failure. The as TestContextOptions cast hides all of that from the compiler.

No new exposure — :66 already derefs before this line is reachable from onBeforeTest, and the onAfterExecute path derefs inside a try/catch in both old and new code. So this isn't a regression; it just reproduces the pattern where ?. would cost nothing.

Note this guard is also unreachable from onBeforeTest specifically (:66 has already returned by then) — it's live and correct only on the onAfterExecute path.

Reviewer: fallback independent reviewer

[anish353]

[Medium] The reload clause is contradicted by this PR's own evidence

The note credits the fix to "suites that reload the session between tests or whose run ends before the WebdriverIO after hook." Row 1 of your own evidence table disproves the first clause: 9.33.1 (shipped code), after() reached → 3 renames, correct per-test titles — on a build you describe as the customer's exact reload-per-test shape.

The mechanism agrees: onReload refreshes KEY_FRAMEWORK_SESSION_ID (service.ts:809-812), so each reloaded session already got its own sessionMap entry and the old sweep already PUT a name for every one. Reload alone was never a failure mode. The real cause is solely the second clause — the worker never reaching after() (service.ts:584 is the only EXECUTE/POST trigger; the customer log shows EXECUTE trackEvent = 0).

This matters because it's the customer-facing release note: users who reload per test but do reach after() would read this as fixing something that was never broken for them.

Suggestion: drop the reload clause here and in the internal note. Or substantiate it — it would hold only if the Automate REST API refuses to rename a terminated session, which is worth confirming either way since it's a useful fact.

Reviewer: fallback independent reviewer

[anish353]

Claude Code PR Review

PR: #131 • Head: 0986e13 • Reviewers: fallback independent reviewer — this repo has no .claude/ and therefore no installed project reviewer

Summary

Moves the Automate/App-Automate session rename from the single onAfterExecute sweep at worker teardown into onBeforeTest, via a new flushSessionName() de-duped on SessionData.appliedName; onAfterExecute reuses the same helper as a final sweep. Two files, +44/−3 including the changeset.

The diagnosis is correct and the mechanism is sound. The PR's evidence table isolates the defect cleanly, and the two things most likely to make this change dangerous both came back clean under direct verification — there is no off-by-one in the name, and a failed PUT cannot break a customer's test.

Five Medium findings, three Low. None is High or Critical. The Mediums are robustness, blast radius, and release-note accuracy — not broken logic.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Credentials read from config, same expressions as the code being replaced.
High	Security	Authentication/authorization checks present	N/A	No auth surface introduced.
High	Security	Input validation and sanitization	Pass	No new parsing of untrusted input.
High	Security	No IDOR — resource ownership validated	N/A	Session id comes from the SDK's own tracked instance.
High	Security	No SQL injection (parameterized queries)	N/A	No datastore access.
High	Correctness	Logic is correct, handles edge cases	Pass	The crux was checked and disproven: lastTestName is assigned in onBeforeTest itself at :87-96, ten lines above the flush at :106 — no lag, no off-by-one. Reload de-dupe verified correct. The one unhandled edge — a failed PUT still marking the name applied — is Finding 1 (Medium).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Verified in three layers that a failed PUT cannot reject out of onBeforeTest. The pre-existing swallow inside markSessionName is nonetheless what makes Finding 1 possible.
High	Correctness	No race conditions or concurrency issues	Pass	onReload is awaited by WebdriverIO before reloadSession() resolves, so the new session id is in place before the next onBeforeTest.
Medium	Testing	New code has corresponding tests	Fail	Zero tests added. _fetch is already mocked in the suite, so flushSessionName is trivially testable — Finding 5.
Medium	Testing	Error paths and edge cases tested	Fail	Integration evidence covers the happy paths and the primary failure mode well, but nothing covers a failed PUT, the de-dupe, or the skipSessionName gate.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI green across Build & test (node 18.20/20/22), Lint, CodeQL, Semgrep, and SDK Wdio Test (98%, 85/87). The author reports the vitest suite is byte-identical to the clean-tree control — same 70 pre-existing failures, zero new. tsc --noEmit at head: exit 0.
Medium	Performance	No N+1 queries or unbounded data fetching	Fail	Finding 3 — a conventional suite goes from 1 PUT at teardown to N PUTs, one per test, and _fetch sets no timeout, retry, or abort signal.
Medium	Performance	Long-running tasks use background jobs	Fail	The per-test PUT is awaited on the hot path rather than fire-and-forget. Mitigated by being parity with the legacy non-CLI path.
Medium	Quality	Follows existing codebase patterns	Pass	appliedName faithfully re-implements the legacy _fullTitle de-dupe idiom at service.ts:1006.
Medium	Quality	Changes are focused (single concern)	Pass	One module, one concern, plus its changeset.
Low	Quality	Meaningful names, no dead code	Pass	Clear naming; the redundant sessionMap.set is noted in Finding 7.
Low	Quality	Comments explain why, not what	Fail	Finding 7 — the 6-line block at :100-105 carries ticket ID and pre-9.27 history that belongs in the changeset.
Low	Quality	No unnecessary dependencies added	Pass	None.

Findings

1. appliedName records attempted, not applied — a failed PUT disarms the final sweep

• File: packages/browserstack-service/src/cli/modules/automateModule.ts:133
• Severity: Medium
• Issue: markSessionName never checks response.ok (:274-276 — await fetch(...) → await response.json() → debug-log, no status check) and swallows everything in its catch (:277-279). A 401/404/429/5xx is indistinguishable from success. flushSessionName then sets appliedName = name unconditionally at :133, so the teardown sweep (:220-221) no-ops. The field's own comment at :28 — "last name successfully PUT" — asserts something the code cannot know.
• Failure scenario: the reload-per-test shape that motivates this PR puts one test per session. Test A's onBeforeTest PUT gets a 429 or a socket blip → appliedName = "A" → sweep suppressed → that session permanently keeps its creation-time sessionName capability. That is exactly the bug this PR fixes, reintroduced under transient failure — while the new "Final sweep" comment advertises a guarantee it no longer provides. It compounds with Finding 3: more PUT volume makes rate-limiting more likely.
• Evidence: automateModule.ts:274-279, :133, :28, :220-221.
• Suggestion: Have markSessionName return response.ok and set appliedName only on 2xx; or make the teardown sweep unconditional and de-dupe only the per-test calls — that costs one extra PUT per session, which is exactly what main already does today.

2. Skipped tests fire TEST/PRE, so a hook failure fans out into hundreds of blocking PUTs

• File: packages/browserstack-service/src/cli/modules/automateModule.ts:106
• Severity: Medium
• Issue: TEST/PRE is not fired only by service.ts:498. skipReporter.ts:58 awaits framework.trackEvent(TEST, PRE, …) for every skipped test, and reportSuiteSkipped (:72-92) walks every remaining test and nested suite calling reportSkippedTest when a before all / beforeEach hook fails — all inside the awaited afterHook. Each now issues a blocking rename PUT, and since the titles are distinct the appliedName de-dupe collapses nothing.
• Failure scenario: a failed before all in a 300-test suite adds ~300 sequential HTTPS PUTs to teardown — roughly a minute at typical latency — inside an awaited WDIO hook. Statically-skipped tests also transiently rename the live session to a test that never ran.
• UNVERIFIED: whether this trips the WDIO afterHook timeout. The configured timeout value was not confirmed; if it is in the tens of seconds this escalates to High, so it is worth you checking.
• Evidence: skipReporter.ts:48, :57-60, :72-92; automateModule.ts:106.
• Suggestion: Pass a marker in reportSkippedTest's TEST/PRE args (e.g. skipped: true) and skip the flush for it; or make the per-test PUT fire-and-forget and let the awaited sweep stay authoritative.

3. Per-test awaited PUT on the hot path, unbounded

• File: packages/browserstack-service/src/cli/modules/automateModule.ts:129 (and :106)
• Severity: Medium
• Issue: Under Mocha with defaults, name embeds the test title (:80-85), so lastTestName changes every test and the de-dupe never suppresses. A conventional suite — one session, N tests, no reload — goes from 1 PUT at teardown to N PUTs, each awaited before the test body. _fetch (fetchWrapper.ts:19-35) sets no timeout, no retry, no abort signal. At ~150–300 ms that is ~1–2.5 min added per worker for 500 tests, and a hanging API stalls every test start. This makes the PR body's "no extra API calls in the steady state" inaccurate for the non-reload case.
• Mitigating: this is parity with the legacy non-CLI path — service.ts:503 awaits _setSessionName per test, de-duped via _fullTitle at :1006. Restored prior behavior, not a novel design.
• Suggestion: Bound it (AbortSignal.timeout(~5000)), or make the onBeforeTest flush fire-and-forget and keep the sweep authoritative.

4. The changeset overclaims the reloadSession failure mode

• File: .changeset/pr-131.md:5
• Severity: Medium (customer-facing release note)
• Issue: The note credits the fix to "suites that reload the session between tests or whose run ends before the WebdriverIO after hook." The first clause is contradicted by the PR's own evidence table, row 1: 9.33.1 (shipped code), after() reached → 3 renames, correct per-test titles — on a build described as the customer's exact reload-per-test shape. Mechanically this follows: onReload refreshes KEY_FRAMEWORK_SESSION_ID (service.ts:809-812), so each reloaded session already had its own sessionMap entry and the old sweep already PUT a name for every one. Reload alone was never a failure mode; the real cause is solely the second clause — the worker not reaching after() (service.ts:584 is the only EXECUTE/POST trigger; the customer log shows EXECUTE trackEvent = 0).
• Suggestion: Drop the reload clause from the customer-facing note and the internal one, or substantiate it — it would only hold if the Automate REST API refuses to rename a terminated session, which is worth confirming empirically since it would be a useful fact either way.

5. No test coverage for any new behavior

• File: packages/browserstack-service/tests/cli/modules/automateModule.test.ts
• Severity: Medium
• Issue: _fetch is already mocked (:49-51) and measureWrapper stubbed (:43-47), so flushSessionName is trivially testable — yet nothing covers the per-test flush, the de-dupe, the in-flush skipSessionName gate, or reload→rename. Existing assertions are not call-count-sensitive on fetch, so a regression here lands silently green; both onAfterExecute tests assert expect(true).toBe(true) (:289, :316). Tests calling onBeforeTest without an explicit mockResolvedValue (:162-172, :488-496) now silently drive a failing PUT and pass anyway.
• Suggestion: Three cases: different titles → fetch twice; same title → once; onBeforeTest then onAfterExecute → no second name PUT.

6. Pre-existing: setSessionName: false silently disables session status too

• File: packages/browserstack-service/src/cli/modules/automateModule.ts:66 (pre-existing, not an added line)
• Severity: Low for this PR
• Issue: :66 returns before the sessionMap entry is created (:87-96); onAfterTest only records results when an entry exists (:180); onAfterExecute then iterates an empty map (:202), so markSessionStatus never runs — and in CLI mode that is the only status path, since the legacy one is gated at service.ts:598. cliUtils.ts:84 maps setSessionName: false → skipSessionName. So a user who only wanted to suppress naming also loses pass/fail status.
• Suggestion: Now that flushSessionName re-checks skipSessionName itself (:115), the :66 guard can be narrowed to keep the isBrowserstackSession bail while still populating the map — cheap to fix in this PR.

7. Redundant sessionMap.set, unreachable guard, over-verbose comment

• File: packages/browserstack-service/src/cli/modules/automateModule.ts:134, :115, :100-105
• Severity: Low
• Issue: :134 re-sets an existing key with the same object reference — a no-op — and it happens while onAfterExecute iterates entries() (:202), which is safe but forces every reader to prove it. It does match the file's existing idiom (:95, :188). The skipSessionName guard at :115 is unreachable from onBeforeTest (:66 already returned) though live and correct from onAfterExecute. The 6-line comment at :100-105 carries ticket ID and pre-9.27 history that belongs in the changeset.

8. Unguarded testContextOptions deref reproduced (pre-existing pattern)

• File: packages/browserstack-service/src/cli/modules/automateModule.ts:115
• Severity: Low
• Issue: testContextOptions is genuinely reachable as undefined — unConfigureModules() (cli/index.ts:427) re-calls configure() with no 4th arg so config defaults to {}; setConfig swallows a parse failure leaving {}; the binary returns a degenerate config on auth failure. The as TestContextOptions cast hides this. No new exposure — :66 already derefs before :115 is reachable, and the onAfterExecute path derefs inside try/catch in both old and new code — but the new line reproduces the pattern instead of using ?..
• Adjacent, separate ticket: eventDispatcher.ts:42-49 dispatches observers as a bare for … await with no per-callback try/catch, and wdioMochaTestFramework.ts:92-97 places runHooks outside its catch. AutomateModule is registered first on TEST/PRE (cli/index.ts:160, ahead of TestHub, CustomTags, Accessibility, Percy), so any throw there would silently drop those four modules' TEST/PRE handling for that test.

Verified clean — the two highest-risk hypotheses both failed

• No off-by-one in the session name. The concern was that the flush reads sessionMap[sessionId].lastTestName — a different store from the setState call above it — and would therefore see the previous test's title. It doesn't: lastTestName is assigned in onBeforeTest itself at :87-96 (:94), ten lines above the flush at :106. onAfterTest writes only testResults (:179-189). The PR does not alter the name↔sessionId pairing at all; only PUT timing changed.
• A failed PUT cannot break the customer's test — three independent layers. (i) It cannot reject: :248-279 wraps everything in try/catch, and safeMark/safeMeasure swallow. (ii) If it did, it would propagate unguarded through eventDispatcher.ts:42-49 → wdioMochaTestFramework.ts:96 → service.ts:498. (iii) But WDIO swallows it — @wdio/utils testFrameworkFnWrapper calls resolve(e) rather than rejecting, reserving rejection for skip/pending markers. Caveat: @wdio/mocha-framework is absent from node_modules, so this was proven via the shared wrapper all adapters route through.
• Credentials identical — onAfterExecute locals :198-199 use the same this.config.userName/accessKey expressions as :130-131.
• Reload de-dupe correct — service.ts:812 writes newSessionId into the tracked instance, getTrackedInstance() returns the same instance per worker, and WebdriverIO awaits onReload inside reloadSession() before it resolves. New id → sessionMap miss → fresh entry → rename proceeds. Identical consecutive titles are correctly suppressed; a retry that reloads gets a new id and is renamed.
• Types/build — tsc --noEmit at head: exit 0, zero diagnostics. SessionData is class-private (referenced only at :26, :36), so appliedName?: string cannot cause narrowing issues elsewhere.
• Endpoint routing unaffected by the earlier call time — isAppAutomate() reads capabilities set at driver creation, well before beforeTest.

Coordination and evidence

• Layer: thin-layer SDK change confined to one module's hook timing. No gRPC/proto surface, no binary dependency, no cross-SDK impact.
• Test evidence: the integration control is strong and genuinely isolating — 9.33.1 with after() reached → 3 renames correct; 9.33.1 without after() → 0 renames (the reported symptom); this PR without after() → 2 correct; clean run → 3 not 6, confirming the sweep de-dupes. Rows 2–3 pin the defect. Note that row 1 is also what undercuts the reload narrative in Finding 4. The gap is unit-level (Finding 5).
• Changeset / release: correct in form — package name matches packages/browserstack-service/package.json, and patch matches precedent (PR fix(automate): detect App Automate from appium:app capability for session name/status (SDK-7093) #100, the previous fix to this same bug, plus fix(SDK-7061): resilient TRA build-stop — retry + success-gated flag + synchronous shutdown stop #99 and fix(browserstack-service): run a11y scans for jasmine framework (SDK-7190) #123 were patch; features feat(browserstack-service): stamp hook_run_uuid on App A11y scans fired inside hooks [APPA11Y-5542] #48/feat(browserstack-service): test-level custom metadata hook handling (beforeEach/afterEach) for WDIO+Mocha #60 were minor). The body follows the repo's PR template section-for-section with exactly one bump ticked. The defect is content, not form — Finding 4.
• CI: everything green except gate, which is red only because a separate agent's sdk-pr-review:verdict marker is absent for this SHA — not a code failure.

Note on how this review was produced

This repo has no .claude/ directory, so there is no installed project reviewer and the orchestrator's fallback applied: use Claude Code's built-in review as the sole reviewer signal.

That fallback misfired and its output was discarded. The built-in code-review skill runs as a fork inheriting the session's working directory (browserstack-csharp-sdk) rather than the target checkout — and that repo also happens to have a PR #131. It reviewed browserstack/browserstack-csharp-sdk PR #131 ("Release 1.6.5", ee748c6, 8 files) instead. All 15 of its findings concerned a different PR and were excluded in full.

Generic-reviewer coverage was obtained instead from a second independent reviewer pointed explicitly at the correct diff; its findings were then re-verified directly against skipReporter.ts:48-92, automateModule.ts:274-276 and service.ts:598 before being accepted here. Everything above rests on code read at 0986e13b plus one tsc --noEmit run. Worth knowing if you run this skill against other repos without an installed reviewer.

---

Verdict: PASS — correct diagnosis, sound mechanism, and both high-risk hypotheses disproven under direct verification. Worth addressing before merge: Finding 1 (the sweep should not be disarmed by a failed PUT), Finding 2 (check the afterHook timeout — it is the one item that could escalate), and Finding 4 (the reload clause in the customer-facing note).

[github-actions]

🔴 SDK PR Review gate is red. Pending:

• The SDK PR Review Agent has not reviewed the current head commit yet — run the SDK PR Review Agent.

It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge.