Repository navigation
Conversation
|
👋 Some commits in this PR are not signed and verified by GitHub. Please sign your commits with a GPG or SSH key registered in your GitHub account, then force-push. Commits that are not verified: See GitHub’s guide on signing commits for setup instructions. I’ve added |
Use environment.Refs for the missing-env preflight so toolset env values that use the documented ${env.X} form are discovered with the same normalization the runtime applies, instead of being reported as the literal name env.X.
def156a to
e2e29e2
Compare
|
Thanks — the commit is now signed with an SSH signing key registered on this account and force-pushed as e2e29e2 ( |
aheritier
left a comment
There was a problem hiding this comment.
Code-quality assessment: no demonstrated blocking or nonblocking findings. The change makes MCP secret-reference discovery use the same normalization as runtime expansion. Source review confirms coverage of ${env.X}, ${X}, and $X with a static catalog; literal-value handling and required-variable deduplication remain intact.
Validation: local environment/gateway package tests and existing config preflight tests passed. The new PR-head regression test, full suite, build, and lint were not run; these local results do not establish PR-head validation.
CI / merge readiness: NOT merge-ready. At head e2e29e2, both ci and codeql completed with action_required and zero jobs. No check runs or commit statuses are present, so the required gate is not verified green. The PR is not draft and is conflict-free, but GitHub reports mergeable_state: blocked. CI runs: https://github.com/docker/docker-agent/actions/runs/40155151748 and https://github.com/docker/docker-agent/actions/runs/40155151752. The current commit's signature is verified, resolving the earlier signing comment.
Issue context: needs-decision. No GitHub issue or Jira acceptance reference is linked from the PR description/comments, and closingIssuesReferences is empty. Reviewed the documented syntax contract and background issue #2615, including its closing comment. Confirm that the PR description defines the complete acceptance scope. Approval is withheld pending that confirmation and green CI.
|
Thanks for the assessment. On the acceptance scope: Surface: one file, Why no linked issue: #2615's fix ledger covers runtime expansion per field (#2944, #3257, #3416). The preflight scan is a separate code path that kept Acceptance criteria I would check: with Re-verified today against main
On CI: |
The up-front missing-env preflight scans a toolset's
envvalues with rawos.Expand, while the runtime expands the same values throughenvironment.ExpandAll(which normalizes${env.X}toXfirst). For the canonical form — exactly whatexamples/mcp-definitions.yamluses — the preflight records the literal nameenv.GITHUB_PERSONAL_ACCESS_TOKEN:GatherEnvVarsForToolsthen returns["env.GITHUB_PERSONAL_ACCESS_TOKEN"], andCheckRequiredEnvVarsaborts the run even withGITHUB_PERSONAL_ACCESS_TOKENset:${env.X}is documented as the canonical syntax for every config field, including toolsetenvvalues (docs/configuration/overview/index.md), andenvironment.Refsis documented as the helper for exactly this kind of up-front check.Change
Use
environment.Refs(value)instead of rawos.Expand, so the preflight discovers the same names the runtime will expand.Test
pkg/config/gather_mcp_env_test.goserves a static MCP catalog and checks three forms (${env.X},${X},$X) with the variable set. Onmainthe canonical subtest fails with the error above; with the change all three pass and the reported name isGITHUB_PERSONAL_ACCESS_TOKEN.Validation
The full
go test ./...has unrelated baseline failures in this environment (pkg/hookshook commands exit 127 andpkg/runtime/TestDoCompactAfterHookFires); they reproduce without this change.Scope
In scope: the up-front missing-env preflight in
pkg/config—GatherEnvVarsForToolsreads toolsetenvvalues with the same normalization the runtime applies, so${env.X}discoversXrather than the literalenv.X;${X}and$Xkeep being discovered; literal values and duplicate dedupe are unchanged.Out of scope: runtime expansion itself, shell-style syntax inside JS-expression fields (#2615 / #2851), path-field expansion (#2944), docs. The preflight path is not one of the code paths #2615's fix ledger lists, which is why no issue is linked.
Re-verified 2026-10-26 against main
9d7700c8: the new test fails on main and passes with the change;go test ./pkg/config/... ./pkg/teamloader -count=1ok;go vet ./pkg/configclean;gofmt -l pkg/configempty.