Replies: 1 comment
|
对着代码确认了,你说得没错。dotenvFallback(packages/credentials/credentials-local/src/index.ts)现在是: getFrom 按 trust 顺序拿第一个"有值"的,而 launch-environment 把空串当成有值(只跳 undefined),所以空 project-env 先中选,再被末尾那个 length>0 判掉,直接返回 undefined,不会继续落到 user-env。空值这一层的判断只加在了汇总后的赢家上,没加在每一层。 最小改法:逐层判非空,第一个非空才返回: 这样空 project-env 会被跳过,落到 user-env 的真值上,跟 CredentialProvider 那条"空值即缺失"的规则就一致了。 没改之前,临时办法就是把 project .env 里那个空占位行删掉或注释,别留 |
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Summary
LocalCredentialProvider.dotenvFallback(packages/credentials/credentials-local/src/index.ts) resolves both.envlayers through a single call:getFromreturns the first source in trust order that has any value, and it treats the empty string as present (launch-environment/src/index.tsskips onlyvalue === undefined). So an emptyproject-enventry wins the lookup, and thevalue.length > 0guard — applied only to that single winner — returnsundefinedinstead of falling through to a non-emptyuser-envvalue.This violates the seam-wide rule stated on
CredentialProvider(packages/credentials/credentials/src/index.ts:54-59):The rule is honored per-layer everywhere else (the
inheritedlayer, and the empty-process-env fallthrough test), but the two-layer.envfallback applies it to the aggregated winner instead of to each layer. Bothresolveanddescribeare affected.Impact
A user with a blank placeholder
DEEPSEEK_API_KEY=in a project.envand a real key in~/.dsh/.envis reported unconfigured. Becausellm-deepseekresolves the API key per request viacredentials.resolve(packages/llm/llm-deepseek/src/index.ts:231), requests fail as "no credential configured" despite a valid key being present. Blank placeholders in checked-in.envtemplates are common, so this is a realistic availability bug. It is a correctness/availability issue, not a secret leak.Reproduction
.env(invoking cwd):DEEPSEEK_API_KEY=(empty)~/.dsh/.env:DEEPSEEK_API_KEY=sk-real-keyResult:
credentials.resolve(DEEPSEEK_API_KEY)returnsundefinedanddescribereportsconfigured: false, even though a valid non-empty key is configured in the user's home.env.Suggested fix
Judge the empty-is-absent rule per layer: iterate the two
.envsources in trust order and return the first with a non-empty value.An empty higher-priority entry then correctly falls through to a non-empty lower-priority one — mirroring how an empty inherited env value already falls through to the file/dotenv layers.
Testing done
layer ladderregression test (emptyproject-envabove non-emptyuser-env). It fails on the current one-call form (resolvereturnsundefined) and passes with the fix.vitest run packages/credentials packages/util/launch-environment→ 72 passed.tsc -bclean; fullpnpm run typecheckclean.Branch with the patch
Since this repo has PRs disabled, I've pushed the fix + regression test to a fork branch for reference:
fix/credentials-empty-dotenv-layer-maskingonMajor818/deepseek-harnessHappy to adjust anything if a maintainer wants to pull it in. Thanks for the great project!
All reactions