feat: add opt-in capture engine orchestration for CLI billing [IANDT-240]

[brettgurman-snyk]

Summary

Completes GAF-side command lifecycle for IANDT-240 on top of lazy-open capture (#671). Provides pkg/clibilling host helpers: register failure cleanup via post-invoke hook, and FinishCommand at CLI teardown to close the capture session and emit ingest POSTs.

Pairs with snyk/cli#7067.

Stacks on IANDT-238 (#671). Reflects architecture review feedback: no CaptureEngine, no HTTP transport bag injection, no eager session open at command start.

Lazy-open flow (end-to-end)

snyk monitor
  → analytics.SetCommand("monitor")           # cliv2 — normal instrumentation only
  → first billable HTTP
       → middleware lazy-opens capture session  # IANDT-238
       → records project UUIDs in bag
  → post-invoke hook (on workflow failure)      # IANDT-240 — discards session
  → tearDown → FinishCommand(success)           # IANDT-240 — close + EmitFromCapture

Not in this stack: BeginCommand, WithContributorBillingCapture(), contributor_billing_active_command config key.

What it does

pkg/clibilling

• EnableIfConfigured — registers ContributorBillingPostInvokeHook before Init. Hook discards the capture session when a billable top-level workflow fails; successful workflows keep the session open until tearDown.
• FinishCommand — closes the active capture session and, on success + non-empty bag, calls EmitFromCapture → IANDT-237 ingest POSTs. Waits for in-flight emitter work.
• ActiveCommand(engine) — resolves in-flight command from analytics, with RAW_CMD_ARGS fallback (lives in config.go to avoid import cycles with middleware).
• ConfigurationKeyCaptureEnabled — re-exports capture flag constant for hosts.

Engine post-invoke hooks

• Cherry-picks Peter's AddPostInvokeHook infrastructure (7830036) so the failure-cleanup hook fires after each top-level Invoke.

What was removed vs earlier draft

• BeginCommand — session opens lazily in middleware (feat: add internal contributorbilling capture middleware for CLI billing [IANDT-238] #671), not at command start
• WithContributorBillingCapture() — capture flag is a normal config key; no app option needed
• CaptureEngine / WrapEngine / HTTP transport bag injection
• Emit-on-finish deferred to FinishCommand (not post-invoke hook), so tearDown sees final exit code and runs once per process

cliv2 wiring (#7067)

globalEngine = clibilling.EnableIfConfigured(app.CreateAppEngineWithOptions(...))

globalEngine.GetAnalytics().SetCommand(name)   // existing analytics — not billing-specific
// engine.Invoke(...) — capture opens on first billable HTTP inside GAF middleware

clibilling.FinishCommand(teardownCtx, globalEngine, globalConfiguration, exitCode == 0)

GAF version for CLI

CLI #7067 pins: github.com/snyk/go-application-framework v0.10.1-0.20260806080023-f868e847d10f

Test plan

• go test -race ./pkg/clibilling/... ./internal/contributorbilling/... ./pkg/workflow/...
• GAF CI green
• Lazy-open E2E validated locally with #7067 (snyk monitor → ingest POST at tearDown)
• Production E2E after merge

Merge order

IANDT-237 (#664) → IANDT-238 (#671) → IANDT-240 (GAF #681 + CLI #7067) → E2E validation

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues
✅	Secrets	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[cursor]

Stale comment

Automated verification review

Ran the verification pipeline (semantic analysis, adversarial review, security scan, code review) against the diff between bg/contributor-capture-middleware and this branch (13 files, no go.mod/go.sum changes).

This PR is not a dependency-only change (it touches pkg/clibilling, pkg/analytics, pkg/app, pkg/workflow, pkg/networking/middleware, and internal/contributorbilling), so per policy this review does not auto-approve — leaving inline findings for human review instead.

CI is green (unit tests, secrets scan, Snyk security scans all pass), but two of the findings below are empirically reproducible bugs the CI matrix doesn't catch (a logic bug in billable-command detection, and a flaky test race), so I'm flagging this as request-changes rather than a clean pass.

Critical

• isBillableCommand's --report substring check can never match the real command string GAF receives from cliv2 (getFullCommandString never includes flags), so iac test --report / code test --report — 2 of the 3 documented billable commands — silently never open a capture session.
• Four new tests in clibilling_test.go call t.Parallel() while sharing/mutating the same package-level capture.commandSession global — reproducibly flaky under -race -count=300. This is the exact hazard already fixed (by removing t.Parallel()) in the sibling test files touched by this same PR, but the fix wasn't applied here.

Should Fix

• The capture config-key literal is now defined in three places (capture.ConfigurationKeyCaptureEnabled, clibilling.ConfigurationKeyCaptureEnabled, and a third raw string literal in pkg/app/options.go) instead of reusing the existing exported constant.
• EngineImpl.SetNetworkAccess is dead, untested public API with zero call sites — a leftover of the HTTP-transport-injection design the PR description says was explicitly removed per architecture review.
• Adding GetCommand() to the public Analytics interface is a breaking change for any external implementer of that interface; none of this PR's commits carry a feat!/BREAKING CHANGE: marker per this repo's own stability checklist.
• WithContributorBillingCapture() silently no-ops if applied before WithConfiguration() in the Opts slice, since it mutates whatever configuration object is current at that point.

See inline comments for details, file:line references, and suggested fixes. Suggestions (doc-comment accuracy, unused parameters, no-op ctx threading) are also called out inline as lower-priority cleanup.

  

_{Sent by Cursor Automation: Automatic PR verification}

[cursor]

Critical — This --report substring check can never match the real command string.

The only production caller (cliv2, snyk/cli#7067) sets the command via analytics.SetCommand(getFullCommandString(cmd)), which builds the string purely from the cobra command-name chain — it never includes flags. --report is a boolean flag on snyk iac test/snyk code test, not part of the command path, so commandName(engine) will be exactly "iac test" or "code test", which never contains "--report". isBillableCommand therefore always returns false for those two command types, silently disabling capture for 2 of the 3 documented billable commands (monitor, iac test --report, code test --report).

Suggested fix — derive billability from a signal actually available at BeginCommand time, e.g. read the --report flag from configuration instead of parsing it out of the command name:

func isBillableCommand(command string, config configuration.Configuration) bool {
	command = strings.TrimSpace(strings.ToLower(command))
	if command == "monitor" {
		return true
	}
	reportRequested := config != nil && config.GetBool("report")
	return reportRequested && (command == "iac test" || command == "code test")
}

[cursor]

Should Fix — Adding GetCommand() to the exported Analytics interface is a breaking change for any external implementer of this interface (Go requires the complete method set). None of this PR's commits carry a feat!/BREAKING CHANGE: marker, which this repo's own stability checklist (AGENTS.md) requires for exactly this kind of change.

Either confirm no external re-implementations exist and note that explicitly in the PR body, or mark the introducing commit with the repo's breaking-change convention.

[cursor]

Suggestion — This doc comment (and RepoPathFromConfig's below) claims the return value is a "git root," but the implementation does no git-root resolution — it's a plain passthrough of the configured/working directory (or . if empty). Harmless today since the value is currently unused by FinishCommand, but will mislead whoever wires up the deferred emit path. Consider rewording to describe what the code actually does.

Also, on the non-empty branch (line 18) the function returns the untrimmed workingDirectory even though the emptiness check above it trims — consider return strings.TrimSpace(workingDirectory) for consistency.

[cursor]

Stale comment

Ran full verification (semantic analysis, adversarial review, security scan, independent code review) on this diff.

Auto-approval check: this PR touches 13 Go source/test files (internal/contributorbilling, pkg/analytics, pkg/app, pkg/clibilling, pkg/networking/middleware, pkg/workflow) with no go.mod/go.sum changes — it is not a dependency-only bump, so per policy this cannot be auto-approved regardless of findings.

CI is green (unit tests, race, Snyk Code/SCA/secrets all pass on this exact commit). No Critical or exploitable-security findings. Several agents independently converged on the same handful of Should Fix issues, detailed inline. Summary:

• Flaky test race (found independently by all 4 review passes): the new pkg/clibilling/clibilling_test.go runs t.Parallel() on tests that read/write the shared global capture.commandSession singleton — this exact hazard was already fixed in this same PR for contributor_capture_test.go but not applied to the new file. This will cause intermittent CI failures.
• Config-key duplication with no safety net: pkg/app/options.go re-literals "contributor_billing_capture_enabled" instead of importing the existing capture/clibilling constant, and there's no test on WithContributorBillingCapture() to catch drift if the key is ever renamed.
• Dead/leftover API surface: EngineImpl.SetNetworkAccess (also has an unsynchronized write to a field every other mutator locks) and clibilling.EnableIfConfigured have zero callers and reference the transport-injection design the PR description says was explicitly rejected.
• Silent session overwrite: capture.OpenCommandSession unconditionally replaces an already-active session with no guard/log, which could silently drop captured records or (per the security pass) misattribute billing data across repos/orgs if ever called re-entrantly from a multi-invocation host.
• Process note: analytics.Analytics gained a new interface method (GetCommand()), which is a breaking change for any external implementer per this repo's own stability rules — worth confirming with downstream consumers, no code fix needed if none exist.

None of this blocks the interim, opt-in design itself — gating logic and the opt-in default are sound and well tested. Recommend fixing the test race and config-key duplication before merge; the dead-code and session-guard items can be a fast follow if preferred.

  

_{Sent by Cursor Automation: Automatic PR verification}

[cursor]

[Suggestion — process check, not a code fix] Adding a method to the exported Analytics interface is a breaking change for any type outside this repo that implements it directly (per this repo's own stability checklist). Both in-repo implementers (AnalyticsImpl, analyticsWrapper) are updated correctly, so this is fine as-is if no external type implements Analytics directly — worth a quick confirmation with CLI/consumer teams, otherwise no action needed.

[cursor]

Stale comment

Ran full verification (semantic analysis, adversarial review, security scan, independent code review) on the diff between bg/contributor-capture-middleware and this branch's current head (524a17c).

Auto-approval check: this PR touches 37 files (.circleci/config.yml, go.mod, and 35 Go source/test/doc files across pkg/clibilling, pkg/analytics, pkg/app, pkg/auth, pkg/workflow, pkg/networking/middleware, internal/contributorbilling, internal/metrics, etc.). This is not a dependency-only change, so per policy it cannot be auto-approved regardless of findings — leaving this as a comment-only review.

CI status: ❌ CircleCI "Unit tests" is currently failing on this exact commit (524a17c) — pkg/clibilling fails to build/pass:

--- FAIL: TestBeginCommand_noOpWhenCaptureDisabled (0.00s)
    clibilling_test.go:37: Expected nil, but got: &capture.Capture{...}
FAIL	github.com/snyk/go-application-framework/pkg/clibilling	0.107s

Windows tests, secrets scan, and Snyk Code/SCA/license/security checks are all green.

Re-verification of prior review rounds' findings against the current diff:

Prior finding	Status now
isBillableCommand's --report substring check can never match the real command string GAF receives	STILL PRESENT — unchanged, no test covers iac test --report/code test --report
New t.Parallel() tests in clibilling_test.go race on the shared capture.commandSession global	STILL PRESENT — this is the confirmed CI failure. The fix (drop t.Parallel() + ResetCommandSession()) was applied to capture/session_test.go and contributor_capture_test.go in this same PR, but never to the new clibilling_test.go
Capture config-key literal duplicated in 3 places	FIXED — single literal, aliased everywhere
EngineImpl.SetNetworkAccess dead, untested public API	STILL PRESENT — zero call sites; also unguarded by the mutex every other mutator of the same field uses, so it's a live data race in addition to being dead code
Analytics.GetCommand() added to public interface, no breaking-change marker	STILL PRESENT — no feat!/BREAKING CHANGE: marker in any commit on this branch. The same issue now also applies to Engine.AddPostInvokeHook (new, flagged below)
WithContributorBillingCapture() silently no-ops before WithConfiguration()	MOOT — the function was removed entirely in favor of FFS-gated wiring in pkg/app/app.go, but the README still documents it as if it exists (flagged below)
capture.OpenCommandSession unconditionally replaces an active session, no guard/log	STILL PRESENT — unchanged

None of this blocks the interim, opt-in design itself — the FFS-gated default and the new post-invoke-hook recursion guard (Test_EngineWrapper_HookRecursionGuard, Test_EngineWrapper_NestedInvocationsSkipHooks) are sound and well tested. But the CI-failing test race and the --report gating bug mean 2 of the 3 documented billable commands still silently never open a capture session, and the currently-failing test needs a fix before this can go green.

See inline comments (existing threads updated, two new ones added below) for details and suggested fixes.

Note on review status: this automation no longer sets PRs to "Changes Requested" — this review is comment-only regardless of findings. Two earlier reviews from this automation on this PR are still recorded as "Changes Requested"; our review-management tooling can only dismiss reviews in an Approved state, so those could not be programmatically dismissed here. A maintainer with write access may want to dismiss them manually if they are blocking merge via branch protection.

  

_{Sent by Cursor Automation: Automatic PR verification}

[cursor]

[Should Fix] AddPostInvokeHook(hook PostInvokeHook) error is a new method on the exported workflow.Engine interface (introduced by commit 8ffe2b2, merged into this branch). Per this repo's own stability checklist (AGENTS.md), adding a method to a public interface is breaking for any external type implementing Engine — and pkg/mocks/workflow.go needed regeneration to keep compiling, which is direct evidence of that. None of the commits introducing this carry a feat!:/BREAKING CHANGE: marker.

Same remedy as the Analytics.GetCommand() finding from earlier review rounds: mark the introducing commit feat!:/add a BREAKING CHANGE: footer, or land this via an additive companion interface (type-asserted) instead of widening Engine directly.

[cursor]

Verification report — not eligible for automatic approval

This PR changes 37 files (~2,000 added / ~86 removed lines) across pkg/clibilling (new), internal/contributorbilling, pkg/workflow, pkg/auth, pkg/app, pkg/utils, pkg/analytics, pkg/networking/middleware, and CI config — plus a 2-line go.mod retract directive. This is not a dependency-only change, so per policy it is never auto-approved. (Policy update: this automation no longer sets a CHANGES_REQUESTED review decision — findings are posted as review comments instead.)

CI status at review time

CircleCI Unit tests, security-scans, secrets scan, GitGuardian, and all Snyk code/license/secrets/security PR checks are green on this exact commit (10790f78). "Windows tests" and test_and_release were still in progress at review time. The local go1.26 toolchain could not be exercised in this sandbox (network egress to the Go module proxy is blocked), so build/lint were not re-run locally — the CircleCI "Unit tests" result is taken as the tooled signal for correctness.

Consolidated findings (4 independent review passes: semantic analysis, adversarial review, security scan, code review)

All four passes independently converged on the same top issue:

• Critical — process-wide capture-session singleton (internal/contributorbilling/capture/session.go): commandSession is a single package-level global with no guard against a second concurrent BeginCommand/OpenCommandSession call clobbering an in-flight session. This directly contradicts this repo's own architecture rule against package-level mutable state for multi-invocation hosts (IDE/MCP), and is corroborated by this PR's own last commit, which had to strip t.Parallel() from four test files specifically to route around races on this global. Tracked in an open thread from a prior run.

Security scan: no new exploitable findings beyond what CI already caught; the pkg/auth changes are a net security hardening (fixes a real host-validation scheme-detection bug), not a regression.

Prior review threads reconciled

• 3 threads fully addressed and resolved (the t.Parallel() data races on the session singleton in tests — fixed by the latest commit; the now-obsolete "dead function" note on EnableIfConfigured, which now has real behavior).
• 9 threads remain open — re-verified against current code and still valid: the singleton overwrite/data-loss risk, the untestable --report substring match in isBillableCommand, the dead/unsynchronized EngineImpl.SetNetworkAccess, the undeclared breaking changes to the exported Engine and Analytics interfaces (no feat!/BREAKING CHANGE: marker), the misleading "git root" doc comment on RepoPathFromConfig, and the README's reference to a WithContributorBillingCapture() option that no longer exists in the codebase.

New findings from this pass

See inline comments for: an unguarded read of EngineImpl.initialized in Invoke (the same PR added a mutex for the write but missed this read site), a safety-critical untested success == false path in finalizeContributorBilling (must not bill on a failed command — nothing tests this), speculative unused metrics plumbing in pkg/utils/file_filter.go for an unrelated ticket (CLI-1740), and a second README passage that now contradicts the actual (already-wired) emit behavior.

Also worth flagging for the author: the pkg/auth/authHost.go / oauth2authenticator.go changes (~200 lines, a security-relevant OAuth host-validation rewrite) appear unrelated to this PR's stated billing-capture purpose and might be easier to review as a separate PR.

Bottom line: solid direction and well-tested for the happy path, but the capture-session singleton is a real architectural gap for this repo's multi-consumer goals, and a few loose ends (breaking-change markers, README accuracy, untested failure path) are worth tightening before merge.

  

_{Sent by Cursor Automation: Automatic PR verification}

[cursor]

Should Fix — This read of e.initialized is unguarded, while this same PR added e.mu.Lock()/Unlock() around the write at Init() (a few lines above) and around the read in the new AddPostInvokeHook. A goroutine calling Invoke() concurrently with Init() (or with AddPostInvokeHook, which now also takes e.mu) will race on this field — exactly the kind of race go test -race flags, and a real hazard for hosts that don't serialize engine setup and first invocation.

Suggested fix:

e.mu.RLock()
initialized := e.initialized
e.mu.RUnlock()
if !initialized {
    return output, fmt.Errorf("workflow must be initialized with init() before it can be invoked")
}

[cursor]

Should Fix — This is the safety-critical branch (a failed CLI command must never emit a billing record), but no test in pkg/clibilling/*_test.go calls FinishCommand/finalizeContributorBilling with success=false — every call site in the test suite passes true. Notably, an earlier commit message on this branch (test(clibilling): serialize Finish tests using shared emitter) references a TestFinish_skipsEmitOnFailure test by name, but no such test exists in the current tree — it appears to have been dropped in a later rebase/merge without anyone noticing.

Suggest adding a test that opens a session, adds a record, calls FinishCommand(ctx, engine, config, false), and asserts the session was closed but no HTTP request reached the ingest server.

[cursor]

Verification report — not eligible for automatic approval

Ran the verification pipeline (semantic analysis, adversarial review, security scan, independent code review) against the diff between bg/contributor-capture-middleware and this branch's current head (f868e847, 2 new commits since the last round: "Introduce Engine Invoke Post Hooks" and "FinishCommand teardown and failure cleanup hook").

Auto-approval check: this PR touches 17 files (~1,146 added / 13 removed lines) — all Go source/test/doc under pkg/clibilling (new package), pkg/app, pkg/workflow, pkg/mocks, internal/contributorbilling. No go.mod/go.sum changes. This is not a dependency-only change, so per policy it is never auto-approved, regardless of findings.

CI status

All checks green on this exact commit (f868e847): CircleCI Build, Unit tests, Windows tests, security-scans, secrets scan; GitGuardian; Snyk code/license/secrets/security PR checks (devex_cli + devex_ide). Notably, the CI-failing t.Parallel() race from the prior round is now fixed — clibilling_test.go/hook_test.go dropped t.Parallel() in favor of a shared resetCaptureSession(t) helper.

Prior findings re-verified against the current code (read directly, not just re-run agents)

Fixed since the last round:

• t.Parallel() race on the shared capture.commandSession global — CI-confirmed fixed.
• EngineImpl.SetNetworkAccess (dead, unsynchronized method) — removed entirely.
• README's app.WithContributorBillingCapture() example — updated to the real clibilling.EnableIfConfigured(engine) wiring; the stale "emit not wired here yet" text was also corrected to match the now-implemented emit-on-success behavior.
• pkg/utils/file_filter.go's speculative CLI-1740 dead code (metricFileFilterPrefix etc.) — removed.

Partially addressed:

• capture.OpenCommandSession's unconditional session overwrite is unchanged, but production code no longer calls it — the new EnsureCaptureSessionForConfig path goes through EnsureCommandSession, which does not clobber an active session. Only test helpers still call OpenCommandSession directly, so the residual risk is a latent footgun in exported API rather than an active production bug.

Still present:

• Critical — billable-command detection for --report commands still doesn't work end-to-end. internal/contributorbilling/capture/gate.go's IsBillableCommand matches --report via substring, but the new pkg/clibilling/config.go:ActiveCommand() returns engine.GetAnalytics().GetCommand() whenever it's non-empty — and per this PR's own README, cliv2 sets that via "normal instrumentation (not billing-specific)", i.e. the cobra command path without flags — before ever reaching the RAW_CMD_ARGS fallback that would actually contain --report. In the real cliv2 flow this means iac test --report / code test --report still never open a capture session; only monitor does. TestIsBillableCommand only calls IsBillableCommand("iac test --report") with a hand-built string, so it doesn't exercise ActiveCommand()'s actual precedence and doesn't catch this.
• Should Fix — data race. pkg/workflow/engineimpl.go:320 reads e.initialized in Invoke() with no lock, while this same PR added locked access to the same field in Init() and the new AddPostInvokeHook(). A concurrent Init()/AddPostInvokeHook() with Invoke() races on this field.
• Should Fix — undeclared breaking change. workflow.Engine.AddPostInvokeHook is a new method added to the exported Engine interface (same class of issue as the earlier Analytics.GetCommand() addition) with no feat!/BREAKING CHANGE: marker on the introducing commit, despite this repo's own AGENTS.md stability checklist requiring one.
• Should Fix — untested safety-critical path. No test calls FinishCommand/finalizeContributorBilling with success=false — the "never bill on a failed command" branch (pkg/clibilling/finalize.go:28) is logically sound on inspection but unexercised by any test.
• Suggestion. pkg/clibilling/config.go's DefaultRepoPath/RepoPathFromConfig doc comments still describe a "git root" the implementation doesn't compute (plain passthrough/first INPUT_DIRECTORY entry).

Thread reconciliation

13 previously-open automation threads re-verified against the current head: 6 resolved on GitHub as fully addressed, 1 marked partially addressed (residual risk noted above, thread kept open), 6 remain open (re-confirmed still valid against current code, not just carried over).

Note on review status

This automation does not set PRs to "Changes Requested" — findings are posted as comments only, this round included. Two earlier reviews from this automation on this PR remain recorded as "Changes Requested" from before that policy took effect; this automation's tooling can only programmatically dismiss reviews in an Approved state (re-confirmed this round), so those two could not be cleared here. A maintainer with write access should dismiss them manually if they are blocking merge via branch protection.

  

_{Sent by Cursor Automation: Automatic PR verification}

[cursor]

Follow-up correction to my review above

A deeper security pass (run in parallel, results just back) surfaced something I under-scoped in my main review: I marked the capture.OpenCommandSession overwrite thread as partially addressed, reasoning that production only calls the non-overwriting EnsureCommandSession now. That reasoning was incomplete — EnsureCommandSession doesn't fix the underlying problem, it's the same problem in a different shape:

[FILE:internal/contributorbilling/capture/session.go LINE:33] Critical — the capture session is a single process-wide global with no engine/org scoping, enabling cross-tenant billing-data leakage.

var commandSession session (line 13) is one package-level singleton shared by every workflow.Engine instance in the process. EnsureCommandSession (line 33) returns the existing bag whenever one is active — it does not check which engine/org is asking, and does not update repoPath on a second caller. Whoever calls FinishCommand/CloseCommandSession first receives everything accumulated in that shared bag and emits it via finalizeContributorBilling (pkg/clibilling/finalize.go:24) under their own config's ORGANIZATION scope and auth token — including CollectContributors-driven git-log data keyed to whichever repoPath was recorded on first open.

Concrete path: this PR's own new engine-hook infrastructure explicitly supports and tests concurrent top-level Invoke() calls in one process (pkg/workflow/engine_test.go's Test_PostInvokeHook_ConcurrentTopLevelInvocations, Test_EngineInvocationConcurrent), and GAF's own architecture rules require supporting multiple engines/orgs per process (IDE/MCP hosts). Two engines for two different orgs running billable commands concurrently in the same process can result in Org A's captured project/entity IDs and repo/contributor data being emitted to entitlements-service tagged with Org B's ScopeID and signed with Org B's auth header, if Org B's teardown races ahead of Org A's.

This is the same root cause flagged as "process-wide capture-session singleton" in an earlier round's review (still open thread, previously resolved as partially_addressed by me this round — retracting that downgrade). Fix: scope the session per engine/invocation (e.g. key the session store by a stable engine/session identifier, or have open/close take and verify an opaque handle) instead of one shared package-level var.

Everything else in my review above stands; this note only upgrades the severity/status of the OpenCommandSession/session-singleton thread back to not addressed (Critical), superseding the partially_addressed verdict I recorded earlier this round.

  

_{Sent by Cursor Automation: Automatic PR verification}

[cursor]

Follow-up — remaining findings from the full 4-pass review (semantic analysis, adversarial review, security scan, independent code review)

All four passes are now in. Three of the four independently reconfirmed the --report gating bug, the unguarded e.initialized read, the missing breaking-change marker on Engine.AddPostInvokeHook, and the untested finalizeContributorBilling(..., success=false) path — already covered above, no changes to those verdicts. Two more findings converged across independent lenses and are worth flagging:

[FILE:pkg/clibilling/hook.go LINE:15-24] Should Fix — the capture session has no identity linking it to the command that opened it, so the failure-cleanup hook can both over- and under-fire.
ContributorBillingPostInvokeHook decides whether to discard the active session by re-checking capture.IsBillableCommand(ActiveCommand(engine)) for whichever top-level workflow just finished — not by checking whether the open session belongs to that workflow. Two independent lenses traced concrete failure directions from this same root cause: (a) a billable command's session can be wrongly discarded if some other, unrelated top-level workflow happens to fail while the billable command is still "active" per analytics (only FILTER_FINDINGS/OUTPUT_WORKFLOW are exempted via a hardcoded skip-list); (b) conversely, a billable command's session can be left open and un-discarded if a later, non-billable command fails after it, since the current-command billability check now evaluates false. Both stem from internal/contributorbilling/capture/session.go's session struct carrying no owner/command identity.
Fix: correlate the session to the command that opened it (e.g. store the command name alongside commandSession.capture/repoPath and compare against hctx), or gate on "is a session currently open" rather than re-deriving billability of whichever command just happened to run.

[FILE:pkg/app/options.go LINE:53-57] Should Fix — AddPostInvokeHook registration failures are swallowed to a warn-log at both call sites, silently disabling the billing safety net if host wiring is ever reordered.
Both WithPostInvokeHooks (pkg/app/options.go:53-57) and pkg/clibilling.EnableIfConfigured do if err := engine.AddPostInvokeHook(...); err != nil { ...Warn()... }. AddPostInvokeHook hard-errors when called after Init() (pkg/workflow/engineimpl.go:458-460). If a future refactor calls either of these after Init(), the hook that guarantees "discard capture on billable-command failure" never registers — with only a log line as evidence, no test failure, no panic. Given this hook is billing-safety-critical, consider surfacing the registration error to the caller (or asserting ordering) rather than swallowing it.

[FILE:pkg/clibilling/config.go LINE:14-58] Suggestion — 5 exported symbols (EnsureCaptureSession, DefaultRepoPath, RepoPathFromConfig, IsBillableCommand, CaptureEnabledForConfig) have no caller anywhere outside their own package's tests. Verified via repo-wide grep: the documented wiring (internal/contributorbilling/README.md) and the actual production HTTP-capture path use capture.EnsureCaptureSessionForConfig directly, bypassing this wrapper layer entirely; RepoPathFromConfig/DefaultRepoPath also duplicate the already-existing private repoPathFromConfig in internal/contributorbilling/capture/gate.go. Per this repo's own AGENTS.md ("exported API is a forever-commitment in a library"), consider trimming these until there's a real external call site, or keep the logic unexported.

No change to the bottom line: this PR remains ineligible for auto-approval (no go.mod/go.sum changes — not a dependency bump), and this review remains comment-only per policy.

  

_{Sent by Cursor Automation: Automatic PR verification}