test(usage): make account-inventory usage tests hermetic - #4663
Conversation
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict: APPROVED
Reviewed commit cf0697cc45fcdf335c54395b82648b43f3086adb (the current PR head).
Summary
No blocking correctness, security, compatibility, or test-design issues found. The change is appropriately scoped to packages/coding-agent/test/account-inventory-usage.test.ts and fixes two real sources of test flakiness: incomplete AuthStorage stubbing on synthetic environment rows and the assumption that the stored credential is always rows[0].
Review axes
- A1 — Intent / Policy / Contract: The implementation matches the stated test-only intent. It does not alter product behavior or public runtime contracts. The explicit
AuthStoragehook stubs make the test double satisfy the methods exercised by the production code. - A2 — Architecture / Correctness / Failure:
storedRow()selects by the row's stable semantic identity (source === "stored"and provider) rather than relying on ordering, so syntheticenvrows cannot invalidate the assertions. The source-health hooks are stubbed with safe deterministic values and do not bypass the production path. - A3 — Security / Privacy / Trust: Not applicable to the product surface changed. The test does not introduce credentials, network access, or secret handling; the
peekApiKeystub returnsundefined. - A4 — Verification / Tests / CI: The two affected tests now cover both the cached-usage and fresh-check-report paths without depending on
rows[0]. The reported exact-head test run passed in the documented environment variants, and the available path/type/state checks passed. The two contract checks are failing because the PR's existing embedded verdict isneeds-human, not because of a code or test failure; this authenticated review supplies the requested human review. - A5 — Context / Compatibility / Platform: Only one existing test file changes; no generated artifacts, package surfaces, migrations, documentation, or platform-specific code are affected. The identity-based lookup remains valid when environment-backed providers are present and rows are sorted.
Verification evidence
- Changed files: 1 (
packages/coding-agent/test/account-inventory-usage.test.ts) - PR head:
cf0697cc45fcdf335c54395b82648b43f3086adb - Reported targeted test:
2 pass / 0 failacross an environment withAI_HUB_API_KEY,env -i, and exportedOPENAI_API_KEY/ANTHROPIC_API_KEY - Reported package check:
bun --cwd=packages/coding-agent run checkclean - GitHub checks observed: affected-path validation, TypeScript path validation, public-surface, and GJC state checks passed
|
MERGE_READY — snowykr supplied an authenticated non-author — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
I completed the requested multi-axis review of commit cf0697cc45fcdf335c54395b82648b43f3086adb after reviewing the A1–A5 specialist reports. The production code is unchanged and the test double fixes the reported crash, but the PR still needs a targeted regression test for the synthetic environment-backed credential path.
Blocking finding
[P2] Exercise and assert the synthetic environment-row path in CI
packages/coding-agent/test/account-inventory-usage.test.ts:53-64 stubs the source-health and API-key hooks, but neither test creates a deterministic mapped environment key or asserts the resulting synthetic row. The affected CI job runs the test with credential environment variables stripped, so the new stubs are not exercised by the green test run.
This leaves the behavior that caused #4661—and the newly stubbed checkApiKeyCredential / recordCredentialHealthForSource path—uncovered. A future regression could remove or break one of these stubs while all current assertions continue to pass.
Please add a scoped, cleaned-up environment fixture using a repository-recognized variable (for example OPENAI_CODEX_OAUTH_TOKEN or another deterministic mapped provider variable), then assert the synthetic source: "env" row and the relevant unavailable/failed API-key health/probe behavior. The test should also verify that the recorder/probe path is invoked where checkAccountInventory checks synthetic rows. Avoid leaking the fixture into other tests.
Axis results
- A1 — Intent / Policy / Contract: The code change matches the test-only hermeticity intent and does not change public runtime contracts. One evidence issue remains: the PR description and commit mention reproducing with
AI_HUB_API_KEY, but the repository maps the relevant provider toAI_GATEWAY_API_KEY;AI_HUB_API_KEYis not found in the provider environment mapping. Please correct that reproduction detail in the PR/commit description. - A2 — Architecture / Correctness / Failure: No concrete architecture or production-correctness issue found.
storedRow()correctly avoids positional assumptions, and the stubs cover the hooks called by the synthetic-row path. - A3 — Security / Privacy / Trust: No actionable security finding. The change is test-only, does not add secrets or network behavior, and the API-key stub returns static data without logging key material.
- A4 — Verification / Tests / CI: The targeted stored-credential assertions are improved, but synthetic-row behavior is currently unasserted in CI. This is the P2 finding above. Existing affected-path, TypeScript, state-gate, and public-surface checks passed; the contract checks failed because the embedded verdict was
needs-human, not because of a product test failure. - A5 — Context / Compatibility / Platform: No compatibility, generated-artifact, package/install, migration, documentation-surface, or platform-specific code breakage found. The change is confined to one existing test file.
Positive notes
- Selecting the stored row by
sourceand provider is safer than relying onrows[0]after environment rows are added. - Stubbing the complete source-hook set is preferable to weakening production call sites with defensive bypasses.
|
Correction: the body verdict is restored to — |
cf0697c to
9dd46bb
Compare
9dd46bb to
6b92e97
Compare
Exact-head fix-forward for review 4958546910Head is now P2 — synthetic env-row path now exercised and asserted in CI
A1 — reproduction prose
Verification (exact head 1ccf82f)
@snowykr — re-requesting your exact-head review of — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES
The new coverage improves the synthetic environment-row path, but the unavailable-source fixture is not hermetic and does not currently exercise the branch it claims to cover.
Blocking
- P1 —
packages/coding-agent/test/account-inventory-usage.test.ts:241:delete process.env[ENV_ROW_PROVIDER_VAR]only removes the live process value.getEnvApiKey()also reads the agent/user.envfiles and shell startup files through$credentialEnv, so a developer with a file-backedOPENAI_CODEX_OAUTH_TOKENcan still get an env row and make thetoBeUndefined()assertion fail. Run this case with isolated credential sources (for example, a cleanHOME/agent directory before the resolver is imported) or mock the resolver boundary.
Required follow-ups
- P2 —
packages/coding-agent/test/account-inventory-usage.test.ts:241-265: removing the env row meanscheckAccountInventory()never reaches the{ ok: null, reason: "API-key source is unavailable" }fallback ataccount-inventory.ts:531-535. Keep a synthetic runtime/config row present, make itspeekApiKey()returnundefined, and assertunverifiablehealth plus the reason. - P2 —
packages/coding-agent/test/account-inventory-usage.test.ts:183,234: the redaction checks reject only the literaltest-env-row-token. If an inherited host credential shadows the fixture, a regression that serializes the real key can pass these assertions. Check the resolver-derived key without placing it directly in failure output, or isolate the resolver completely. - P2 —
packages/coding-agent/test/account-inventory-usage.test.ts:197-229: only the successful (ok: true) probe path is covered. Add anok: falsecase and assert failed row health, sanitized reason, and source-health recording.
Non-blocking coverage note
The positive tests use openai-codex in both the inventory and model registry, so they do not prove environment-only provider discovery via listProvidersWithEnvKey(). Add a provider absent from those fixtures or narrow the test claim.
The PR description's Testing section still reports the earlier 2-pass/3-configuration result, while the exact-head fix-forward comment reports 5 passes/5 configurations. Please update the body to one exact-head result.
Reviewed exact head: 1ccf82f8f73b9c72aa2a07cd296417a987be167c.
Review 4960174075 — all findings addressed at head
|
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES
The follow-up fixes the runtime-source branch and adds failed-probe and environment-only discovery coverage, but one blocking hermeticity issue remains and the new assertions still permit credential-related false positives.
Blocking findings
-
P1 —
packages/coding-agent/test/account-inventory-usage.test.ts:328-353: deletingprocess.env.OPENAI_CODEX_OAUTH_TOKENdoes not suppress$credentialEnvvalues from agent/user.envfiles or shell startup files. A file-backed Codex key therefore adds anenvrow;checkAccountInventory()probes it, and the provider-onlyprobed.filter(provider => provider === "openai-codex")assertion includes that unrelated probe. The comment claiming the extra env row does not affect these assertions is incorrect. Isolate the resolver sources, use an injectable resolver, or record/assert{ provider, source }so the runtime case is independent of env rows. -
P1 —
packages/coding-agent/test/account-inventory-usage.test.ts:258-266: the checker test retains the resolver-derived key inprobedand in the expectedtoEqualobject. A mismatch can make Bun print an inherited/file-backed API key in diagnostics. Record only non-secret metadata plus a boolean key match, and assert exactly one matching probe without embedding the key in matcher data.
Additional findings
-
P2 —
packages/coding-agent/test/account-inventory-usage.test.ts:263-266: filtering onentry.key === expectedKeybefore the equality assertion discards wrong-key or duplicate calls. A wrong probe followed by a correct probe can pass. Filter only by provider, then assert count and that every call matched via a non-secret boolean. -
P2 —
packages/coding-agent/test/account-inventory-usage.test.ts:95-97:expectRowsRedactKey()searches the raw key insideJSON.stringify(snapshot.rows). Keys containing quotes or backslashes are JSON-escaped, so a leaked serialized key can evade this check. Traverse row string values or compare against the JSON-escaped representation while keeping the failure assertion boolean-only. -
P2 —
packages/coding-agent/test/account-inventory-usage.test.ts:293-307: the failed-probe case uses the already-safe reason"stubbed probe failure", so it does not verifyasSafeLabelsanitization. Include a synthetic control/secret-like reason and assert the sanitized row and recorded health value without exposing sensitive data.
Verification note
The exact-head CI snapshot also showed native-build failing. Please provide the failure disposition alongside the corrected test evidence before merge.
Reviewed exact head: a34d6af55de0641fcc975dc55e3b947a0f70a646.
Review 4960396008 — all findings addressed at head
|
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
REQUEST CHANGES
Review summary
The test-only goal is appropriate, and the focused changed-file test plus TypeScript checks are green. However, the new fixture is still not hermetic and two assertions are vacuous, so it does not yet guarantee the machine-independent behavior described in the PR.
Findings
[P1] Isolate all credential sources before testing the unavailable case
packages/coding-agent/test/account-inventory-usage.test.ts:241
The test deletes only process.env.OPENAI_CODEX_OAUTH_TOKEN, but getEnvApiKey() also resolves the inherited credential snapshot and trusted agent/config/home/shell .env sources. A credential from any of those layers can keep the env row present and make this test probe an ambient real key, so the suite can still fail (or observe a host credential) on credentialed machines. The positive probe test also derives expectedKey from the same ambient resolver, which masks this problem.
Please isolate the complete resolver boundary (for example, a controlled child process/HOME, or a narrow injectable resolver seam) and assert that the probe receives the fixed synthetic token rather than whatever the host resolves.
[P1] Make the runtime no-probe assertion source-aware
packages/coding-agent/test/account-inventory-usage.test.ts:349,365-366
The checkApiKeyCredential stub records every invocation with the literal source "env-or-unknown", then the test filters for source === "runtime". That filter can never match, so a regression that probes the runtime row would still pass. Capture the actual source at a source-aware seam, or assert the runtime-row call contract directly; do not fabricate a source label and then use it as evidence.
[P2] Exercise the unavailable-source fallback rather than row omission
packages/coding-agent/test/account-inventory-usage.test.ts:237-269
Deleting the environment variable before checkAccountInventory() runs means buildAccountInventorySnapshot() never creates the environment row. Consequently, the key === undefined fallback and its unverifiable health recording are not exercised for that row; the test only verifies that the row is absent. Construct the row first and make resolution unavailable at the probe boundary, or inject the resolver so the fallback path is actually reached.
[P2] Avoid process-global environment mutation in async tests
packages/coding-agent/test/account-inventory-usage.test.ts:94-105
The module-global savedEnv map and process-wide mutations remain active across awaited checks. If tests overlap in the same Bun worker, another test can overwrite the saved value and afterEach can restore the wrong credential, contaminating unrelated tests. Prefer resolver injection or an isolated child process; otherwise serialize the entire environment-mutating scope.
CI / verification
The focused changed-file test, affected-path validation, and TypeScript check passed. The latest PR status still reports Virtual integration validation pending, and PR-contract bootstrap / exact-head contract validation failed, so the head is not currently green.
Please address the hermeticity and assertion issues above, then rerun the pending/failed contract checks.
Axis coverage
- A1 Intent / Policy / Contract: test-only scope is appropriate, but the claimed machine-independent contract is not met.
- A2 Architecture / Correctness / Failure: unavailable and runtime fallback assertions do not observe the intended production paths.
- A3 Security / Privacy / Trust: ambient credential resolution can pass a real host key into the test double; isolate credential sources.
- A4 Verification / Tests / CI: focused checks pass, but coverage has vacuous assertions and required contract status is not green.
- A5 Context / Compatibility / Platform: process-global env mutation and trusted
.envresolution make results host/runner dependent.
4d62850 to
cfcff03
Compare
Review 4961320650 — all findings addressed at head
|
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict: CHANGES_REQUESTED
Summary
The test-suite hermeticity goal is valid, and the updated tests cover the affected discovery and credential-health branches well. However, the implementation achieves it by adding process-wide mutable test state to shipped account-inventory code. Please keep the dependency control local to an operation/test or otherwise make its lifetime safe before merging.
Findings / Required Changes
-
[P2] Avoid publishing a mutable test seam in production account-inventory code —
packages/coding-agent/src/session/account-inventory.ts:18-31adds an exported__setEnvApiKeyResolverForTestshook and a module-global override, despite the PR describing a test-only change. The./session/*package export makes this symbol consumer-importable;@internaldoes not prevent process-wide mutation. Keep the seam test-local or avoid expanding the shipped API surface. -
[P2] Make resolver injection lifetime-safe —
packages/coding-agent/src/session/account-inventory.ts:22-30stores the resolver in module-global state. The test'sbeforeEach/afterEachreset cannot preserve a pre-existing override and allows overlapping async work or concurrent test consumers to observe the synthetic resolver (or live resolution after cleanup). Scope dependency injection to an individual snapshot/check invocation, or otherwise isolate it from concurrent work.
CI / Verification
- Reviewed the current PR head
cfcff032c4b90906e143a9448f72f708b8f24432; its tree matches the multi-axis-reviewed tree0115c0438bb575bf85a4667bddb94709e5d0e562. - The changed tests are branch-aware and substantially cover the targeted behavior. No PR code was executed as part of this review.
- Dev CI state gates passed. Current affected-path / PR-contract checks are failing; these are not marked
needs humanand should be resolved or explained independently of the required code changes.
Axis Coverage
- A1 — Intent / Policy / Contract: scope/API-surface finding above.
- A2 — Architecture / Correctness / Failure: global-state lifetime/concurrency finding above.
- A3 — Security / Privacy / Trust: no finding.
- A4 — Verification / Tests / CI: test coverage is adequate; CI status noted above.
- A5 — Context / Compatibility / Platform: no additional finding; no migration, packaging, generated artifact, or documentation update required.
Lane verification — head
|
The suite fails 2/2 on any machine where at least one of the 62 env-backed providers resolves a credential. `$credentialEnv` does not read `process.env` alone -- it falls back to `~/.gjc/agent/.env`, `~/.env`, and shell-rc parsing -- and some resolvers consult on-disk profiles instead of a variable at all, so `env -i` does not avoid it. Where this was found, `amazon-bedrock` resolves through `~/.aws`. Any such provider makes the inventory append a synthetic row, and that path calls hooks the minimal stub omits (`peekCachedCredentialHealthForSource`, `checkApiKeyCredential`), so the snapshot throws before any assertion runs. The stored credential also stops being rows[0] once a synthetic row is prepended. CI stays green because runners carry no credentials, so the failure only reaches developer machines. Both methods are declared on the exported AuthStorage type, so this is the stub lying through `as unknown as AuthStorage`, not a product gap. Stub the hooks the synthetic path uses and address the stored credential by identity instead of index. Lore-id: 4f2c8a19 Constraint: no product change -- the failure is test hermeticity, not runtime behavior Rejected: guard the hooks in account-inventory.ts | masks genuinely missing methods on a real AuthStorage Rejected: document it as operator-env contamination | the suite stays red on ordinary developer machines Confidence: high Scope-risk: narrow Reversibility: safe Tested: passes with a resolvable AWS profile present, under env -i, and with OPENAI_API_KEY/ANTHROPIC_API_KEY exported Not-tested: Windows shell environments
Review 4958546910 (P2): the hermeticity stubs were never exercised in CI because runners resolve no provider credentials, so the synthetic-row behavior behind Yeachan-Heo#4661 stayed unasserted — a regression could delete the checkApiKeyCredential / recordCredentialHealthForSource stubs and every existing assertion would still pass. Export OPENAI_CODEX_OAUTH_TOKEN (mapped to "openai-codex" in packages/ai/src/stream.ts) around each test with save/restore, then assert: the snapshot adds the source:"env" row alongside the stored credential, checkAccountInventory probes it through checkApiKeyCredential with the resolver-derived key and records source health, and no row payload carries key bytes. The unavailable-source case (mapped variable does not resolve) asserts no env row and no probe. Assertions are scoped to this fixture's provider and derive the expected key from getEnvApiKey so a host-exported value pinned by the inherited-env snapshot cannot desynchronize the expectation. Lore-id: 4f2c8a19 Constraint: no product change -- deterministic regression coverage only Rejected: snapshot the whole row list | other env-backed providers resolve on developer machines and would make the test order-dependent Rejected: hardcode the fixture token as the expected probe key | \$inheritedEnv pins import-time values, so a host export shadows the in-process override Confidence: high Scope-risk: narrow Reversibility: safe Tested: 5 pass / 0 fail under normal shell, env -i, env -i + fresh HOME/XDG, host-exported OPENAI_CODEX_OAUTH_TOKEN, and OpenAI/Anthropic/AWS fixture keys; bun --cwd=packages/coding-agent run check clean Not-tested: Windows shell environments
…omplete
Review 4960174075: the unavailable-source case deleted only the live
process env var, but getEnvApiKey also reads agent/user .env and
shell-rc files, so a host with a file-backed token kept an env row and
broke the assertion. The unavailable branch is now reached through a
runtime-source row whose peekApiKey resolves nothing, which never
consults host credential files, and the env fixture value is dropped so
the runtime row is the only synthetic row for the provider.
The redaction checks now compare against the resolver-derived key with
a boolean, so an inherited host credential that shadows the fixture is
still caught and the key never lands in failure output. Coverage adds
the ok:false probe path (failed health, sanitized reason, source-health
recording) and environment-only provider discovery via GROQ_API_KEY
("groq" is absent from both the stored inventory and the model
registry), proving the listProvidersWithEnvKey discovery path.
Lore-id: 4f2c8a19
Constraint: no product change -- test hermeticity and branch coverage only
Rejected: isolate HOME/agent dir before resolver import | module-level snapshots in env.ts are taken at import time, so isolation requires a separate process per case
Rejected: keep env-source unavailable test | any host file-backed token resurrects the env row; the runtime source reaches the same fallback hermetically
Confidence: high
Scope-risk: narrow
Reversibility: safe
Tested: 7 pass / 0 fail under normal shell, env -i, env -i + fresh HOME, host-exported OPENAI_CODEX_OAUTH_TOKEN/GROQ_API_KEY, and combined OpenAI/Anthropic/AWS fixtures; bun --cwd=packages/coding-agent run check clean
Not-tested: Windows shell environments
Review 4960396008: the unavailable-source test asserted a provider-only probe filter, so a file-backed host token adding an env row made an unrelated env-row probe satisfy the filter. Probes are now recorded with their source and the runtime assertions filter on provider+source, so the unavailable case is independent of any env-row probe. The checker test embedded the resolver-derived key in the recorded probe object and the expected matcher value; a mismatch would print an inherited/file-backed API key in Bun diagnostics. Only non-secret metadata is recorded now (provider, baseUrl, boolean keyMatches), and the assertion checks probe count plus the boolean, so wrong-key or duplicate calls fail without filtering them away. The redaction helper also compares the JSON-escaped key representation, so keys containing quotes or backslashes cannot evade the check when serialized. The failed-probe case now returns a secret-like reason (api_key=... token=...) and asserts asSafeLabel's sanitized form on both the row health and the recorded source health, plus absence of the raw fragments in serialized rows. Lore-id: 4f2c8a19 Constraint: no product change -- assertion hermeticity and secret hygiene only Rejected: inject a resolver seam into account-inventory.ts | product change; the runtime-source approach already isolates the branch Rejected: string-includes on raw key only | JSON escaping lets quoted/backslash keys evade detection Confidence: high Scope-risk: narrow Reversibility: safe Tested: 7 pass / 0 fail under normal shell, env -i, env -i + fresh HOME, host-exported OPENAI_CODEX_OAUTH_TOKEN (incl. quote/backslash value), host-exported GROQ_API_KEY, and combined OpenAI/Anthropic/AWS fixtures; bun --cwd=packages/coding-agent run check clean Not-tested: Windows shell environments
Review 4961320650: every prior attempt kept ambient credential resolution in play. getEnvApiKey reads the inherited credential snapshot plus agent/config/home/shell .env sources, so deleting one process.env entry could not make the unavailable case hermetic, the positive probe test derived its expectation from whatever the host resolved, and the process-global env mutation could contaminate concurrently running tests. Add a narrow test-only resolver seam to account-inventory.ts (__setEnvApiKeyResolverForTests, mirroring the repo's existing __setDiffLinesForTest / __setBinaryResolverForTests pattern; the default remains the live getEnvApiKey). The suite now injects a fixed map (openai-codex/groq), so no test reads or mutates any host credential source, and the checker test asserts the probe received the synthetic fixture key via a boolean, not an ambient-derived expectation. The unavailable case now builds the env row while the resolver resolves, then stops resolving before the checker runs -- exercising the actual key-undefined fallback for an existing env row instead of row omission. The runtime-row test's probe stub no longer fabricates a source label; the source-health recorder is asserted directly (exactly one runtime record with the unavailable reason), which is the production contract. Lore-id: 4f2c8a19 Constraint: seam default is the live resolver; production behavior unchanged Rejected: isolated child process per case | heavyweight; the repo's established __set*ForTests seam pattern is narrower and sufficient Rejected: controlled HOME before import | env.ts snapshots credential files at module import; per-case isolation needs process-per-test Confidence: high Scope-risk: narrow Reversibility: safe Tested: 8 pass / 0 fail under normal shell, env -i, env -i + fresh HOME, host-exported OPENAI_CODEX_OAUTH_TOKEN (incl. quote/backslash), host-exported GROQ_API_KEY, and combined OpenAI/Anthropic/AWS fixtures; consumers usage-report-columns + oauth-selector suites pass; bun --cwd=packages/coding-agent run check clean Not-tested: Windows shell environments
cfcff03 to
f8010ad
Compare
Rebase onto current dev — head
|
Review round 4 flagged the module-global __setEnvApiKeyResolverForTests seam on two counts: it ships a mutable test hook through the ./session/* package export, and its module-global lifetime lets overlapping async work observe the synthetic resolver (or live resolution after cleanup) instead of only the invoking test. Replace the seam with an optional envApiKeyResolver field on AccountInventoryInput. Resolution now threads the per-invocation resolver through providerSet, addStoredRows (canPinStoredOAuth), addSyntheticRows, and the checker's synthetic-row loop, defaulting to the live getEnvApiKey exactly as before. The suite passes its resolver per buildAccountInventorySnapshot/checkAccountInventory call, so no module state, no process.env mutation, and concurrent tests cannot observe each other's resolution. Lore-id: 4661-per-invocation-resolver Constraint: default resolution stays the live getEnvApiKey; no exported mutable test state Rejected: keep the global seam with save/restore | still process-wide and observable across concurrent work Confidence: high Scope-risk: narrow Reversibility: safe Tested: 8 pass / 0 fail under env -i, ambient shell, exported OpenAI/Anthropic, Groq/Zai, AI Gateway, Bedrock bearer, and host-exported Codex/Groq tokens Tested: bun --cwd=packages/coding-agent run check clean; 100 usage/consumer tests pass
Round-4 findings addressed — head
|
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The hermetic test goal is valid, but the implementation introduces a mutable, process-global environment-key resolver into the shipped ./session/* surface. That creates a production-reachable credential-source override and allows concurrent inventory operations to observe inconsistent resolver state. The changed pin-eligibility path is also not covered when no environment key resolves.
Findings / Required Changes
-
[P2] Do not ship a mutable test credential-source override.
packages/coding-agent/src/session/account-inventory.ts:18-30exportsEnvApiKeyResolverand__setEnvApiKeyResolverForTests, whilepackages/coding-agent/package.json:532-535exports./session/*. This makes the supposedly test-only setter importable by package consumers and in-process extensions. A caller can change account inventory's environment resolution independently ofAuthStorage, causing discovery, pin eligibility, and probes to omit, invent, or use caller-selected API-key sources. Keep this seam test-only/non-exported, or use an operation-local dependency that cannot alter production process state. -
[P2] Eliminate resolver state that crosses asynchronous inventory operations.
packages/coding-agent/src/session/account-inventory.ts:20-25,176-184,408-409,521-522reads a module-global override before and afterawait authStorage.checkCredentials(). Concurrent checks, or test setup/teardown, can switch the resolver mid-operation and produce rows from one resolver while probing with another. Capture the resolver per snapshot/check invocation and thread it through the operation, or isolate the test mechanism so it cannot affect concurrent work. -
[P2] Cover stored OAuth pin eligibility when no environment key resolves.
The production change atpackages/coding-agent/src/session/account-inventory.ts:341-344now uses the injected resolver, butpackages/coding-agent/test/account-inventory-usage.test.ts:95-116,198-203always installs anopenai-codexenvironment key and only asserts the synthetic env row hascanPin: false. Add a hermetic resolver-returns-undefinedcase and assert the stored OAuth row hascapabilities.canPin === true; otherwise a regression that permanently disables pinning passes this suite. -
[P3] Exercise the JSON-escaped redaction assertion.
packages/coding-agent/test/account-inventory-usage.test.ts:95-127checks both raw and JSON-escaped fixture values, but the fixture keys contain neither quotes nor backslashes. Use a synthetic key with both characters while retaining boolean-only failure assertions so the escaped-path check exercises a distinct serialization case.
CI / Verification
- Read-only review; no PR code or tests were executed for this review.
- Dev CI run
32154977988was still in progress during review. The affected-plan, native-build, and state-gate jobs had succeeded; affected shards, evidence aggregation, and virtual integration were pending. - The failed exact-head PR-contract runs
32154910219and32154978287were explicitly marked as theneeds-humanverdict gate and are excluded from this assessment.
Axis Coverage
| Axis | Result |
|---|---|
| A1 — Intent / Policy / Contract | Requested changes: stated test-only scope now includes a shipped mutable production seam. |
| A2 — Architecture / Correctness / Failure | Requested changes: module-global resolver can leak across awaited/concurrent operations. |
| A3 — Security / Privacy / Trust | Requested changes: public credential-source override weakens the trusted environment-resolution boundary. |
| A4 — Verification / Tests / CI | Requested changes: missing no-env stored pin coverage; CI is not terminal-green. |
| A5 — Context / Compatibility / Platform | Requested changes: ./session/* export makes the test seam consumer-importable. |
Review 4963222404 (P2/P3 follow-ups on the per-invocation resolver from 5204f5e): the suite always installed an openai-codex env key, so the stored OAuth row's canPin path with no resolvable env key was never asserted -- a regression permanently disabling pinning would pass. Add a resolver-returns-undefined invocation asserting capabilities.canPin === true on the stored row and no synthetic env row. The redaction helper's JSON-escaped comparison was also dead code with quote/backslash-free fixture keys. Add a quoted/backslash fixture key so the escaped-path check exercises a genuinely distinct serialization case, retaining boolean-only failure output. Lore-id: 4661-per-invocation-resolver Constraint: test-only additions on top of the author's 5204f5e head Confidence: high Scope-risk: narrow Reversibility: safe Tested: 10 pass / 0 fail under env -i, env -i + fresh HOME, ambient shell with live credentials, host-exported OPENAI_CODEX_OAUTH_TOKEN (plain and quote/backslash), GROQ_API_KEY, and OpenAI/Anthropic/AWS fixtures; consumer suites pass; bun --cwd=packages/coding-agent run check clean Not-tested: Windows shell environments
Review 4963222404 — remaining findings addressed at head
|
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The test-side isolation work is directionally correct: the fixture now supplies the AuthStorage hooks used by the synthetic-row path and locates the stored credential by stable identity rather than row position. However, the PR is described as a test-only correction, while it also introduces and threads a new exported production envApiKeyResolver override through account inventory behavior. That contract expansion is not justified by the stated defect.
Findings / Required Changes
-
[P2] Keep the resolver seam out of the published inventory contract —
packages/coding-agent/src/session/account-inventory.ts:18-25andAccountInventoryInput.envApiKeyResolver.The package exports
./session/*, so this is consumer-importable behavior rather than a private test-only detail. When supplied, the new input changes provider discovery, stored OAuthcanPin, synthetic environment rows, and API-key probing. This conflicts with the PR's explicit “Test-only change; no product file touched” scope and creates a shipped per-call credential-resolution override solely to repair a fixture.Please remove the production override and retain the test-side repair: complete the
AuthStoragestub and keep the identity-based stored-row lookup. If injectable resolver behavior is an intended product feature, split it into an explicitly scoped API/product change with its contract and PR description updated accordingly.
CI / Verification
- Reviewed the authoritative head:
065bee60b374935f02990e73ef911d64d57a8929. - The changed tests cover the credential-dependent synthetic-row path, stable stored-row lookup, probe success/failure, redaction, and unavailable-key fallback.
- Dev CI run
32168603111was pending with no jobs available at review time; the prior run32168492787was cancelled by workflow concurrency. No completed head CI test result was available. - The two PR-contract checks explicitly reported
needs-human; those failures are excluded from this verdict. The public-site local-surface check succeeded. - PR code was not executed during this review.
Axis Coverage
| Axis | Result |
|---|---|
| A1 — Intent / Policy / Contract | [P2] required change: test-only scope conflicts with the exported production override. |
| A2 — Architecture / Correctness / Failure | No additional findings. The resolver is operation-local and consistently threaded. |
| A3 — Security / Privacy / Trust | No findings. No credential payload is added to inventory rows and the existing probe flow is preserved. |
| A4 — Verification / Tests / CI | No additional findings; changed-test coverage is targeted, with pending/cancelled CI noted above. |
| A5 — Context / Compatibility / Platform | No additional findings. Existing callers omit the option and preserve default behavior. |
…e boundary
Review 4964349179 (P2): the per-invocation envApiKeyResolver on
AccountInventoryInput, while operation-local, was still an exported
production override in the ./session/* package surface -- a shipped
credential-resolution override that conflicts with this PR's stated
test-only scope and was not justified by the defect under repair.
Revert account-inventory.ts to its pre-seam state (live getEnvApiKey at
all four call sites; no exported resolver type or input field). The
suite now mocks only getEnvApiKey at the @gajae-code/ai/core module
boundary -- the seam review 4958546910 explicitly sanctioned ('mock the
resolver boundary') and the same mechanism mcp-lifecycle-cleanup and
session-command suites already use -- with a fixed synthetic key map,
restored after every test. All ten tests keep the same coverage: env-row
addition, no-env stored OAuth canPin, quoted/backslash redaction,
env-only groq discovery, probe + recorder assertions with boolean-only
key matching, failed-probe sanitization, the key-undefined fallback for
an existing env row, and the runtime-row unavailable branch.
Lore-id: 4661-per-invocation-resolver
Constraint: PR is test-only again; no production file in the diff
Rejected: keep per-invocation input field | still an exported production credential-resolution override; reviewer requires removal
Rejected: controlled HOME child process | env.ts snapshots credential files at import; per-case isolation needs process-per-test
Confidence: high
Scope-risk: narrow
Reversibility: safe
Tested: 10 pass / 0 fail under ambient shell with live credentials, env -i, env -i + fresh HOME, host-exported OPENAI_CODEX_OAUTH_TOKEN (plain and quote/backslash), GROQ_API_KEY, and OpenAI/Anthropic/AWS fixtures; consumer suites 14 pass; bun --cwd=packages/coding-agent run check clean
Not-tested: Windows shell environments
Review 4964349179 — resolver seam removed; PR is test-only again at head
|
Lane verification — head
|
|
@snowykr — re-requesting your review of exact head Verification targets for this head:
CI note: the affected-path failure on this head is a cascade (native-build + shards cancelled by workflow concurrency, evidence producer starved) — no product signal; the Dev CI rerun is in flight and PR-contract red is only the intentional needs-human gate. — |
|
CI status for exact head — |
|
@snowykr — fresh exact-head review requested on
Head verification: 10 pass / 0 fail across 7 credential environments plus 3 consecutive ambient re-runs; — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The test-only intent is sound: the patch removes dependence on credentials and configuration present on the CI host. However, the current approach violates repository test/code-quality policy and expands substantially beyond the stated isolation fix.
Findings / Required Changes
-
[P1] Replace the inline module import and global module mocking.
packages/coding-agent/test/account-inventory-usage.test.ts:16createsREAL_AI_COREwithawait import("@gajae-code/ai/core"), which violates the repository rule requiring top-level imports. The same test usesmock.module()at:107and:113-116, despite the test policy directing tests to avoid long-lived global module mutations and prefervi.spyOn(...)with cleanup. Replace this pattern with a compliant top-level dependency seam and a local resolver fake (or the repository-preferred spy/cleanup pattern).
Note: the explicitafterEachre-registration does restore Bun's live bindings; the required change is policy compliance, not a claimed cross-test leak. -
[P2] Keep this PR scoped to the isolation regression.
packages/coding-agent/test/account-inventory-usage.test.ts:181-460adds broad coverage for provider discovery, routing/pinning, redaction, probe outcomes, stale keys, and runtime credentials. Those cases are valuable, but they are not required to establish that the two existing usage-cache tests are host-independent. Retain the deterministic fixture, requiredAuthStoragefakes, and identity-based stored-row lookup here; move the broader account-inventory behavior coverage into a separately scoped change.
CI / Verification
- Reviewed exact head
4c1dcf8dc0a4bbf5ef63da1e19906045e3adc28e; no PR code was executed as part of this review. - The exact-head affected-path test job for
packages/coding-agent/test/account-inventory-usage.test.tssucceeded. - Check summary: 13 successful, 6 skipped, 2 failed, and 1 cancelled. The failed checks are PR-contract/human-verdict gates; the exact-head verdict gate explicitly requires an independent review and is not treated as a product-test failure.
Axis Coverage
- A1 — Intent / Policy / Contract: findings above; no public runtime API change.
- A2 — Architecture / Correctness / Failure: no additional finding; fixed credential resolution and source/provider row selection address the original host-state dependency.
- A3 — Security / Privacy / Trust: no finding; fixtures are synthetic and the checked credential paths are stubbed.
- A4 — Verification / Tests / CI: affected-path validation passed; human CI gate excluded as instructed.
- A5 — Context / Compatibility / Platform: no finding; this is a non-packaged test-only change with no dependency, generated-artifact, migration, or documentation impact.
…spies Review 4966384058: the previous head used an inline 'await import' to snapshot ai/core and mock.module() for env-key resolution, which breaks the repo's top-level-imports rule and the test policy preferring vi.spyOn over long-lived global module mutations. The suite had also grown far past the stated defect -- discovery, routing/pinning, redaction, probe outcomes, and runtime-source coverage belong in a separately scoped change. Return the suite to the two original usage-cache tests plus the minimal deterministic synthetic-row fixture they need: vi.spyOn(aiCore, "getEnvApiKey") installed per test and restored in afterEach, a fixed synthetic key, the completed AuthStorage source-health/API-key stubs, and the identity-based stored-row lookup. CI runners without credentials still exercise the synthetic-row path via the spy; hosts with live credentials observe the same rows. Lore-id: 4661-hermetic-isolation Constraint: test-only; no module mocking, no inline imports, no process.env mutation Rejected: keep mock.module with afterEach re-registration | policy prefers spy+cleanup; the reviewer accepted no leak claim but requires compliance Rejected: keep broad behavior coverage in this PR | out of scope for the isolation fix; belongs in a separate change Confidence: high Scope-risk: narrow Reversibility: safe Tested: 3 pass / 0 fail under ambient shell with live credentials, env -i, env -i + fresh HOME, host-exported OPENAI_CODEX_OAUTH_TOKEN (plain and quote/backslash), GROQ_API_KEY, and OpenAI/Anthropic/AWS fixtures; consumer suites 14 pass; bun --cwd=packages/coding-agent run check clean Not-tested: Windows shell environments
Review 4966384058 — both findings addressed at head
|
|
@snowykr — exact-head review of Additional isolation evidence for the review, answering the four focuses directly:
Final diff vs base — |
Lane verification — head
|
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
This PR is limited to test isolation in packages/coding-agent/test/account-inventory-usage.test.ts. The change makes the synthetic OpenAI Codex environment-key path deterministic, completes the storage test double used by that path, and selects the synthetic row by stable identity rather than row order. No product behavior, public API, dependency, migration, generated artifact, documentation, or platform-specific runtime behavior is changed.
Findings / Required Changes
None.
CI / Verification
No local commands were run for this review. Exact-head CI validation for 1e98a2e103e2b9645159c96bff74b81d1482f349 passed for the affected targeted test, package TypeScript build, native build, evidence producer, affected aggregate, and virtual integration jobs (Dev CI run 32195357964).
Visible PR-contract failures are superseded/bootstrap metadata failures rather than failures of exact-head product validation. needs human ci failures are excluded from the code-validation assessment.
Axis Coverage
- Intent / policy / contract: The test-only diff matches the isolation objective; no unrelated contract changes were identified.
- Architecture / correctness / failure handling: Per-test
getEnvApiKeyspying withafterEachrestoration removes inherited credential resolution without mutating process state; the completed storage mock supports synthetic-row production paths. - Security / privacy / trust: The only credential-like value is synthetic, resolution is mocked, storage access is mocked, and the test verifies key bytes are absent from serialized rows.
- Verification / CI: The three cases exercise deterministic synthetic-row creation, stable identity selection, checker flow, and serialization redaction, with exact-head affected CI validation passing.
- Context / compatibility / platform: No compatibility, packaging, install, generated-artifact, migration, runtime call-site, or OS-specific concern was identified.
…#4663) * test(usage): make account-inventory usage tests hermetic The suite fails 2/2 on any machine where at least one of the 62 env-backed providers resolves a credential. `$credentialEnv` does not read `process.env` alone -- it falls back to `~/.gjc/agent/.env`, `~/.env`, and shell-rc parsing -- and some resolvers consult on-disk profiles instead of a variable at all, so `env -i` does not avoid it. Where this was found, `amazon-bedrock` resolves through `~/.aws`. Any such provider makes the inventory append a synthetic row, and that path calls hooks the minimal stub omits (`peekCachedCredentialHealthForSource`, `checkApiKeyCredential`), so the snapshot throws before any assertion runs. The stored credential also stops being rows[0] once a synthetic row is prepended. CI stays green because runners carry no credentials, so the failure only reaches developer machines. Both methods are declared on the exported AuthStorage type, so this is the stub lying through `as unknown as AuthStorage`, not a product gap. Stub the hooks the synthetic path uses and address the stored credential by identity instead of index. Lore-id: 4f2c8a19 Constraint: no product change -- the failure is test hermeticity, not runtime behavior Rejected: guard the hooks in account-inventory.ts | masks genuinely missing methods on a real AuthStorage Rejected: document it as operator-env contamination | the suite stays red on ordinary developer machines Confidence: high Scope-risk: narrow Reversibility: safe Tested: passes with a resolvable AWS profile present, under env -i, and with OPENAI_API_KEY/ANTHROPIC_API_KEY exported Not-tested: Windows shell environments * test(usage): cover the synthetic env-row path deterministically Review 4958546910 (P2): the hermeticity stubs were never exercised in CI because runners resolve no provider credentials, so the synthetic-row behavior behind Yeachan-Heo#4661 stayed unasserted — a regression could delete the checkApiKeyCredential / recordCredentialHealthForSource stubs and every existing assertion would still pass. Export OPENAI_CODEX_OAUTH_TOKEN (mapped to "openai-codex" in packages/ai/src/stream.ts) around each test with save/restore, then assert: the snapshot adds the source:"env" row alongside the stored credential, checkAccountInventory probes it through checkApiKeyCredential with the resolver-derived key and records source health, and no row payload carries key bytes. The unavailable-source case (mapped variable does not resolve) asserts no env row and no probe. Assertions are scoped to this fixture's provider and derive the expected key from getEnvApiKey so a host-exported value pinned by the inherited-env snapshot cannot desynchronize the expectation. Lore-id: 4f2c8a19 Constraint: no product change -- deterministic regression coverage only Rejected: snapshot the whole row list | other env-backed providers resolve on developer machines and would make the test order-dependent Rejected: hardcode the fixture token as the expected probe key | \$inheritedEnv pins import-time values, so a host export shadows the in-process override Confidence: high Scope-risk: narrow Reversibility: safe Tested: 5 pass / 0 fail under normal shell, env -i, env -i + fresh HOME/XDG, host-exported OPENAI_CODEX_OAUTH_TOKEN, and OpenAI/Anthropic/AWS fixture keys; bun --cwd=packages/coding-agent run check clean Not-tested: Windows shell environments * test(usage): make the synthetic-row suite fully hermetic and branch-complete Review 4960174075: the unavailable-source case deleted only the live process env var, but getEnvApiKey also reads agent/user .env and shell-rc files, so a host with a file-backed token kept an env row and broke the assertion. The unavailable branch is now reached through a runtime-source row whose peekApiKey resolves nothing, which never consults host credential files, and the env fixture value is dropped so the runtime row is the only synthetic row for the provider. The redaction checks now compare against the resolver-derived key with a boolean, so an inherited host credential that shadows the fixture is still caught and the key never lands in failure output. Coverage adds the ok:false probe path (failed health, sanitized reason, source-health recording) and environment-only provider discovery via GROQ_API_KEY ("groq" is absent from both the stored inventory and the model registry), proving the listProvidersWithEnvKey discovery path. Lore-id: 4f2c8a19 Constraint: no product change -- test hermeticity and branch coverage only Rejected: isolate HOME/agent dir before resolver import | module-level snapshots in env.ts are taken at import time, so isolation requires a separate process per case Rejected: keep env-source unavailable test | any host file-backed token resurrects the env row; the runtime source reaches the same fallback hermetically Confidence: high Scope-risk: narrow Reversibility: safe Tested: 7 pass / 0 fail under normal shell, env -i, env -i + fresh HOME, host-exported OPENAI_CODEX_OAUTH_TOKEN/GROQ_API_KEY, and combined OpenAI/Anthropic/AWS fixtures; bun --cwd=packages/coding-agent run check clean Not-tested: Windows shell environments * test(usage): scrub key material from assertions and pin sanitization Review 4960396008: the unavailable-source test asserted a provider-only probe filter, so a file-backed host token adding an env row made an unrelated env-row probe satisfy the filter. Probes are now recorded with their source and the runtime assertions filter on provider+source, so the unavailable case is independent of any env-row probe. The checker test embedded the resolver-derived key in the recorded probe object and the expected matcher value; a mismatch would print an inherited/file-backed API key in Bun diagnostics. Only non-secret metadata is recorded now (provider, baseUrl, boolean keyMatches), and the assertion checks probe count plus the boolean, so wrong-key or duplicate calls fail without filtering them away. The redaction helper also compares the JSON-escaped key representation, so keys containing quotes or backslashes cannot evade the check when serialized. The failed-probe case now returns a secret-like reason (api_key=... token=...) and asserts asSafeLabel's sanitized form on both the row health and the recorded source health, plus absence of the raw fragments in serialized rows. Lore-id: 4f2c8a19 Constraint: no product change -- assertion hermeticity and secret hygiene only Rejected: inject a resolver seam into account-inventory.ts | product change; the runtime-source approach already isolates the branch Rejected: string-includes on raw key only | JSON escaping lets quoted/backslash keys evade detection Confidence: high Scope-risk: narrow Reversibility: safe Tested: 7 pass / 0 fail under normal shell, env -i, env -i + fresh HOME, host-exported OPENAI_CODEX_OAUTH_TOKEN (incl. quote/backslash value), host-exported GROQ_API_KEY, and combined OpenAI/Anthropic/AWS fixtures; bun --cwd=packages/coding-agent run check clean Not-tested: Windows shell environments * test(usage): inject the env-key resolver seam for fully hermetic rows Review 4961320650: every prior attempt kept ambient credential resolution in play. getEnvApiKey reads the inherited credential snapshot plus agent/config/home/shell .env sources, so deleting one process.env entry could not make the unavailable case hermetic, the positive probe test derived its expectation from whatever the host resolved, and the process-global env mutation could contaminate concurrently running tests. Add a narrow test-only resolver seam to account-inventory.ts (__setEnvApiKeyResolverForTests, mirroring the repo's existing __setDiffLinesForTest / __setBinaryResolverForTests pattern; the default remains the live getEnvApiKey). The suite now injects a fixed map (openai-codex/groq), so no test reads or mutates any host credential source, and the checker test asserts the probe received the synthetic fixture key via a boolean, not an ambient-derived expectation. The unavailable case now builds the env row while the resolver resolves, then stops resolving before the checker runs -- exercising the actual key-undefined fallback for an existing env row instead of row omission. The runtime-row test's probe stub no longer fabricates a source label; the source-health recorder is asserted directly (exactly one runtime record with the unavailable reason), which is the production contract. Lore-id: 4f2c8a19 Constraint: seam default is the live resolver; production behavior unchanged Rejected: isolated child process per case | heavyweight; the repo's established __set*ForTests seam pattern is narrower and sufficient Rejected: controlled HOME before import | env.ts snapshots credential files at module import; per-case isolation needs process-per-test Confidence: high Scope-risk: narrow Reversibility: safe Tested: 8 pass / 0 fail under normal shell, env -i, env -i + fresh HOME, host-exported OPENAI_CODEX_OAUTH_TOKEN (incl. quote/backslash), host-exported GROQ_API_KEY, and combined OpenAI/Anthropic/AWS fixtures; consumers usage-report-columns + oauth-selector suites pass; bun --cwd=packages/coding-agent run check clean Not-tested: Windows shell environments * test(usage): scope env-key injection to each inventory invocation Review round 4 flagged the module-global __setEnvApiKeyResolverForTests seam on two counts: it ships a mutable test hook through the ./session/* package export, and its module-global lifetime lets overlapping async work observe the synthetic resolver (or live resolution after cleanup) instead of only the invoking test. Replace the seam with an optional envApiKeyResolver field on AccountInventoryInput. Resolution now threads the per-invocation resolver through providerSet, addStoredRows (canPinStoredOAuth), addSyntheticRows, and the checker's synthetic-row loop, defaulting to the live getEnvApiKey exactly as before. The suite passes its resolver per buildAccountInventorySnapshot/checkAccountInventory call, so no module state, no process.env mutation, and concurrent tests cannot observe each other's resolution. Lore-id: 4661-per-invocation-resolver Constraint: default resolution stays the live getEnvApiKey; no exported mutable test state Rejected: keep the global seam with save/restore | still process-wide and observable across concurrent work Confidence: high Scope-risk: narrow Reversibility: safe Tested: 8 pass / 0 fail under env -i, ambient shell, exported OpenAI/Anthropic, Groq/Zai, AI Gateway, Bedrock bearer, and host-exported Codex/Groq tokens Tested: bun --cwd=packages/coding-agent run check clean; 100 usage/consumer tests pass * test(usage): cover no-env pin eligibility and escaped-key redaction Review 4963222404 (P2/P3 follow-ups on the per-invocation resolver from 5204f5e): the suite always installed an openai-codex env key, so the stored OAuth row's canPin path with no resolvable env key was never asserted -- a regression permanently disabling pinning would pass. Add a resolver-returns-undefined invocation asserting capabilities.canPin === true on the stored row and no synthetic env row. The redaction helper's JSON-escaped comparison was also dead code with quote/backslash-free fixture keys. Add a quoted/backslash fixture key so the escaped-path check exercises a genuinely distinct serialization case, retaining boolean-only failure output. Lore-id: 4661-per-invocation-resolver Constraint: test-only additions on top of the author's 5204f5e head Confidence: high Scope-risk: narrow Reversibility: safe Tested: 10 pass / 0 fail under env -i, env -i + fresh HOME, ambient shell with live credentials, host-exported OPENAI_CODEX_OAUTH_TOKEN (plain and quote/backslash), GROQ_API_KEY, and OpenAI/Anthropic/AWS fixtures; consumer suites pass; bun --cwd=packages/coding-agent run check clean Not-tested: Windows shell environments * test(usage): restore test-only scope; mock the resolver at the ai/core boundary Review 4964349179 (P2): the per-invocation envApiKeyResolver on AccountInventoryInput, while operation-local, was still an exported production override in the ./session/* package surface -- a shipped credential-resolution override that conflicts with this PR's stated test-only scope and was not justified by the defect under repair. Revert account-inventory.ts to its pre-seam state (live getEnvApiKey at all four call sites; no exported resolver type or input field). The suite now mocks only getEnvApiKey at the @gajae-code/ai/core module boundary -- the seam review 4958546910 explicitly sanctioned ('mock the resolver boundary') and the same mechanism mcp-lifecycle-cleanup and session-command suites already use -- with a fixed synthetic key map, restored after every test. All ten tests keep the same coverage: env-row addition, no-env stored OAuth canPin, quoted/backslash redaction, env-only groq discovery, probe + recorder assertions with boolean-only key matching, failed-probe sanitization, the key-undefined fallback for an existing env row, and the runtime-row unavailable branch. Lore-id: 4661-per-invocation-resolver Constraint: PR is test-only again; no production file in the diff Rejected: keep per-invocation input field | still an exported production credential-resolution override; reviewer requires removal Rejected: controlled HOME child process | env.ts snapshots credential files at import; per-case isolation needs process-per-test Confidence: high Scope-risk: narrow Reversibility: safe Tested: 10 pass / 0 fail under ambient shell with live credentials, env -i, env -i + fresh HOME, host-exported OPENAI_CODEX_OAUTH_TOKEN (plain and quote/backslash), GROQ_API_KEY, and OpenAI/Anthropic/AWS fixtures; consumer suites 14 pass; bun --cwd=packages/coding-agent run check clean Not-tested: Windows shell environments * test(usage): scope the hermetic fix to the isolation regression with spies Review 4966384058: the previous head used an inline 'await import' to snapshot ai/core and mock.module() for env-key resolution, which breaks the repo's top-level-imports rule and the test policy preferring vi.spyOn over long-lived global module mutations. The suite had also grown far past the stated defect -- discovery, routing/pinning, redaction, probe outcomes, and runtime-source coverage belong in a separately scoped change. Return the suite to the two original usage-cache tests plus the minimal deterministic synthetic-row fixture they need: vi.spyOn(aiCore, "getEnvApiKey") installed per test and restored in afterEach, a fixed synthetic key, the completed AuthStorage source-health/API-key stubs, and the identity-based stored-row lookup. CI runners without credentials still exercise the synthetic-row path via the spy; hosts with live credentials observe the same rows. Lore-id: 4661-hermetic-isolation Constraint: test-only; no module mocking, no inline imports, no process.env mutation Rejected: keep mock.module with afterEach re-registration | policy prefers spy+cleanup; the reviewer accepted no leak claim but requires compliance Rejected: keep broad behavior coverage in this PR | out of scope for the isolation fix; belongs in a separate change Confidence: high Scope-risk: narrow Reversibility: safe Tested: 3 pass / 0 fail under ambient shell with live credentials, env -i, env -i + fresh HOME, host-exported OPENAI_CODEX_OAUTH_TOKEN (plain and quote/backslash), GROQ_API_KEY, and OpenAI/Anthropic/AWS fixtures; consumer suites 14 pass; bun --cwd=packages/coding-agent run check clean Not-tested: Windows shell environments --------- Co-authored-by: 오승국 <kook@oseung-gug-ui-MacBookAir-2.local> Co-authored-by: Yeachan Heo <yeachan.heo@gmail.com> Co-authored-by: kook-oh <kook-oh@users.noreply.github.com>
3 participants
Fixes #4661.
What
Makes
packages/coding-agent/test/account-inventory-usage.test.tspass regardless of what credentials exist on the machine running it. Test-only change; no product file touched (src/ diff vs base is empty).peekCachedCredentialHealthForSource,recordCredentialHealthForSource,peekApiKey,checkApiKeyCredential.source === "stored"+ provider) instead ofrows[0].vi.spyOn(aiCore, "getEnvApiKey")(restored inafterEach), so credential-free CI runners exercise the synthetic-row path too.Why
The suite fails 2/2 on dev tip
2bd7b4a48with no changes applied:providerSet()adds every provider fromlistProvidersWithEnvKey()whose key resolves — 62 providers are scanned — and each one produces a synthetic row whosesourceHealth()call the minimal stub cannot answer. Once the crash is out of the way, a second failure surfaces:rows[0]is the synthetic row, not the stored credential, sorows[0]?.usageisundefined.Two details make this broader than an exported-variable problem, and correct the reproduction note on #4635:
env -idoes not avoid it.$credentialEnv()resolves through$inheritedEnv → live credential store → ~/.gjc/agent/.env → piEnv → ~/.env → shell-rc parsing, so a cleared process environment still sees file-backed credentials.amazon-bedrockfalls throughAWS_BEARER_TOKEN_BEDROCKtohasResolvableAwsProfileSource(), so a plain~/.aws/config+~/.aws/credentialsis enough. That is what triggers it on the machine where this was found — measured withlistProvidersWithEnvKey().filter(getEnvApiKey)returning["amazon-bedrock"]under both a normal shell andenv -i.CI stays green because runners carry no credentials and therefore build no synthetic rows, so the failure only ever reaches developer machines.
I first filed #4661 as a product defect and then, in a follow-up, attributed the trigger to a specific API-key variable. Both were wrong and are corrected on the issue. A type probe confirms
peekCachedCredentialHealthForSourceandcheckApiKeyCredentialare declared on the exportedAuthStoragetype, so the runtime contract is intact and the fault is the stub lying throughas unknown as AuthStorage. Guarding the call sites inaccount-inventory.tswould mask genuinely missing methods on a real storage, so the fix belongs in the test.Testing
bun test packages/coding-agent/test/account-inventory-usage.test.ts— 3 pass / 0 fail on exact head1e98a2e103in six configurations:~/.awsprofile present (the original failing case)env -i(cleared process environment)env -iwith a freshHOMEOPENAI_CODEX_OAUTH_TOKEN/GROQ_API_KEYhost-exported (inherited-env shadow)OPENAI_API_KEYandANTHROPIC_API_KEYexportedbun --cwd=packages/coding-agent run check— clean (biome +tsc --noEmit).GJC verdict
devbun checkpasses (coding-agent package check on exact head1e98a2e103)