feat: global configuration for ADO feeds - #1831
feat: global configuration for ADO feeds#1831jamesadevine with Copilot wants to merge 5 commits into
Conversation
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
/review |
|
✅ Compiler Contract Reviewer completed the compiler contract review. |
|
✅ PR Security Reviewer completed the security review. |
|
✅ Test Quality Sentinel completed the test quality analysis. |
|
✅ Compiler Contract Reviewer completed the compiler contract review. |
|
✅ Rust Code Quality Reviewer completed the Rust code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Compiler contract review — clean
Checked this PR against the ado-aw compiler contracts (front-matter grammar, validated newtypes, IR/docs sync, codegen/lock drift, bash lint):
- Front matter:
packagesisOption<PackageFeedConfig>with#[serde(default)]onSupplyChainConfig— existing workflows without it still parse. No rename/removal, so no codemod is needed. - Newtypes:
feed,organization, andprojectall go through validatedsrc/secure.rsnewtypes (FeedRef,AdoUrlSegment) rather than rawString, matching the safe-output identifier rule, andis_valid_ado_url_segmentcorrectly bans the leading-./space//cases that would otherwise produce a malformed URL. - Docs:
docs/front-matter.md,docs/supply-chain.md, anddocs/runtimes.mdare all updated in the same PR and stay consistent with each other and with the derived-URL/precedence logic intypes.rsand the three runtime extensions. - Network: the new
pkgs.dev.azure.comendpoint is already insrc/allowed_hosts.rs, so nonetwork:allowlist change is required or missing. - Bash: the only touched
bash:-generating helper (ensure_nuget_config_bash_step/ensure_npmrc_bash_step) is a signature refactor, not new script content — no new shellcheck surface. - No codegen/lock/smoke drift: this PR does not touch
filter_ir.rs/Fact,.github/workflows/*.md, ortests/safe-outputs/, so none of the drift classes I check for apply here.
Good test coverage for precedence (feed-url > config > supply-chain.packages > public default), per-ecosystem opt-out, and the fail-closed no-org-resolvable case. No merge-blocking findings from the compiler-contract angle.
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 47.6 AIC · ⌖ 2.33 AIC · ⊞ 11.8K
Comment /review to run again
There was a problem hiding this comment.
Security review of this diff: no regressions found.
The new supply-chain.packages feature composes derived feed URLs from organization/project/feed segments, but each of those is typed with a new validated newtype (AdoUrlSegment, reusing FeedRef) restricted to [A-Za-z0-9._-] (no /, no leading ., rejects at deserialization time via deny_unknown_fields). The fully-assembled URL is then passed through the existing validate_feed_url() (quote/injection rejection, scheme check) before being interpolated into the generated bash: steps in dotnet/node/python extensions — the same pattern already used for per-runtime feed-url:.
No write-capable token is touched, no new domain is added to the AWF allowlist (pkgs.dev.azure.com was already present in src/allowed_hosts.rs), and no existing validation was loosened — is_valid_ado_url_segment reuses the same strict allowlist as is_safe_path_segment. Test coverage for the unsafe-segment rejection paths looks thorough.
🔒 Security review by PR Security Reviewer · auto · 54.5 AIC · ⌖ 2.23 AIC · ⊞ 11.2K
Comment /review to run again
There was a problem hiding this comment.
Security review
No security regressions found in this diff. The new supply-chain.packages feature reuses established patterns safely:
- The organization/project segments used to build the derived feed URL are validated via the new
AdoUrlSegmentnewtype ([A-Za-z0-9._-]), and the feed name via the existingFeedRefnewtype (max one/, no.., no injection chars). The git-remote-inferred organization (not covered by front-matter deserialization) gets an explicitis_valid_ado_url_segmentcheck inurl_forbefore being embedded — good defense-in-depth. - The fully-derived URL is re-validated with
validate_feed_urlinCompileContext::package_feed_urlbefore use, matching the existingfeed-url:validation path. - No write-capable token or secret is introduced or newly projected into the Agent (Stage 1) step; these changes only affect which package-index URL/auth task (
PipAuthenticate/npmAuthenticate/NuGetAuthenticate) is emitted, mirroring the pre-existing per-runtimefeed-url:mechanism. src/compile/ado_bundle.rs(the token-projection chokepoint) is untouched by this PR.
Nothing merge-blocking. Nice test coverage for precedence, opt-out, and the fail-closed no-org case.
🔒 Security review by PR Security Reviewer · auto · 39.8 AIC · ⌖ 13.6 AIC · ⊞ 11.3K
Comment /review to run again
jamesadevine
changed the title
There was a problem hiding this comment.
Reviewed the new supply-chain.packages shared feed feature (types.rs, extensions/mod.rs, and the three runtime extensions). One recurring correctness issue across all three runtime extensions: the config-skip warning branch calls ctx.package_feed_url()?, which performs org resolution and URL validation via ? even though the result is discarded except for .is_some() -- this can turn an unrelated, unused supply-chain.packages misconfiguration into a hard compile failure for runtimes that already opted out via config:. Flagged identically on all three files.
The rest of the new code (PackageFeedConfig deserialization, URL derivation, validation, AdoUrlSegment) looks sound and is well covered by tests.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 80.4 AIC · ⌖ 2.1 AIC · ⊞ 11K
Comment /review to run again
| let effective_feed_url: Option<String> = match self.config.feed_url() { | ||
| Some(url) => Some(url.to_string()), | ||
| None if self.config.config().is_some() => { | ||
| if ctx.package_feed_url(PackageEcosystem::Dotnet)?.is_some() { |
There was a problem hiding this comment.
? here can hard-fail compilation for a config that should just be silently skipped
This branch only exists to decide whether to emit a warning when runtimes.dotnet.config is set. But package_feed_url() also resolves the org and validates the derived URL, so if supply-chain.packages is configured without a resolvable org (or with an invalid derived URL), compilation now fails with an unrelated error -- even though the .NET runtime was never going to use that feed at all (it explicitly defers to config).
Suggested fix
Use a non-failing check here, e.g. inspect whether front_matter.supply_chain()...packages is present and applies_to(ecosystem) without invoking url_for/validate_feed_url, or swallow/log the error instead of propagating with ? in this diagnostic-only branch.
| let effective_feed_url: Option<String> = match self.config.feed_url() { | ||
| Some(url) => Some(url.to_string()), | ||
| None if self.config.config().is_some() => { | ||
| if ctx.package_feed_url(PackageEcosystem::Node)?.is_some() { |
There was a problem hiding this comment.
Same eager-? issue as the .NET/Python extensions
Calling package_feed_url() here (only to decide whether to warn) resolves the org and validates the URL even though this branch never uses the result -- Node defers to config. An unresolvable org or invalid derived segment turns an unrelated config:-based setup into a hard compile failure.
Suggested fix
Check applies_to/presence without invoking url_for in this diagnostic-only path, consistent across all three runtime extensions.
| let effective_feed_url: Option<String> = match self.config.feed_url() { | ||
| Some(url) => Some(url.to_string()), | ||
| None if self.config.config().is_some() => { | ||
| if ctx.package_feed_url(PackageEcosystem::Python)?.is_some() { |
There was a problem hiding this comment.
Same eager-? issue as the other runtime extensions
package_feed_url() performs org resolution + URL validation even in this diagnostic-only branch (deciding whether to emit the config: warning), so a bad/unresolvable supply-chain.packages config can fail Python compilation even though the feed is never applied here.
Suggested fix
Use a cheap presence/applies_to check instead of the full url_for resolution when only deciding whether to warn.
There was a problem hiding this comment.
Verdict: no merge-blocking issues found.
Reviewed the new supply-chain.packages shared feed feature (PackageFeedConfig in types.rs, package_feed_url in extensions/mod.rs, and the dotnet/node/python extension wiring). Validation ordering is sound — SupplyChainConfig::validate() (which validates packages, including the leading-dot rejection) runs before any extension calls ctx.package_feed_url() in every compile target (build_pipeline_context in agentic_pipeline.rs), so an unsafe feed/org/project value cannot reach a generated URL unvalidated. is_valid_ado_url_segment correctly reuses the strict path-segment allowlist to avoid unescaped URL injection. Test coverage for the new type is thorough (precedence, opt-out, mutual exclusivity, org inference failure).
One maintainability note left inline: the feed-resolution precedence (feed-url → config: skip-with-warning → supply-chain.packages → default) is duplicated near-verbatim across the three runtime extensions and would benefit from a shared helper.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 72.4 AIC · ⌖ 2.23 AIC · ⊞ 11K
Comment /review to run again
There was a problem hiding this comment.
Solid test coverage overall for the new supply-chain.packages feature — precedence, opt-out, project-scoping, and the fail-closed no-org path are all exercised at both the unit (types.rs) and integration (compiler_tests.rs, per-runtime extension) levels. Two minor gaps flagged inline: (1) the config:-set + unresolvable-org interaction in each runtime extension isn't tested, and (2) the no-org integration test doesn't distinguish "no git remote" from "remote present but not ADO". Neither is merge-blocking; the core logic is well covered by direct unit tests on PackageFeedConfig.
🧪 Test quality analysis by Test Quality Sentinel · auto · 71.7 AIC · ⌖ 2.11 AIC · ⊞ 10.8K
Comment /review to run again
There was a problem hiding this comment.
Reviewed for ado-aw compiler contract drift (front-matter grammar, safe-output/security newtypes, docs sync, codegen/lock drift). No issues found:
SupplyChainConfig.packages: Option<PackageFeedConfig>is additive and defaults via#[serde(default)], so no codemod is required for existing workflows.PackageFeedConfig/AdoUrlSegmentroute through validated newtypes (FeedRef,AdoUrlSegment) andsrc/validate.rsprimitives rather than rawString— consistent with thesrc/secure.rscontract.- New behavior is documented in
docs/front-matter.md,docs/runtimes.md, anddocs/supply-chain.md, and the cross-doc anchor (supply-chain.md#shared-package-feed-for-the-runtimes-packages) resolves correctly. - No
.github/workflows/*.md/ IR / gate-schema changes in this PR, so no lock-file or codegen drift to flag. pkgs.dev.azure.comallowlist claim in the new doc section checks out againstsrc/allowed_hosts.rs.
No merge-blocking findings.
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 68.4 AIC · ⌖ 12.3 AIC · ⊞ 11.8K
Comment /review to run again
|
@copilot address reviewer suggestions |
There was a problem hiding this comment.
Test Quality review
Good breadth of coverage for the new supply-chain.packages feature — precedence, opt-out, project-scoping, and the org-resolution failure are all exercised at the PackageFeedConfig/types.rs unit level, and each of the three runtime extensions has matching precedence/opt-out/config-skip tests.
Two findings posted inline:
- Untested integration error path — none of the three runtime-extension test suites (
python,node,dotnet) exercisedeclarations()whensupply-chain.packagesis set but no ADO org can be resolved. Every test there usesCompileContext::for_test_with_org, so the?-propagation ofctx.package_feed_url()'s error through the actual call site used in production is never verified — only the lower-levelPackageFeedConfig::url_forunit test intypes.rschecks that. A regression that dropped or mis-formatted the error during extension integration would ship silently. - Test clarity —
test_packages_rejects_unsafe_url_segmentsloops over four cases that fail for two unrelated reasons (charset validation vs.deny_unknown_fields); splitting the unknown-field case out would make a future failure immediately diagnosable instead of a generic "expected rejection" message.
Neither is merge-blocking; the second is a nice-to-have, but the first (item 1) is worth adding given it's the actual failure mode a user would hit.
🧪 Test quality analysis by Test Quality Sentinel · auto · 79.4 AIC · ⌖ 2.18 AIC · ⊞ 10.8K
Comment /review to run again
Comments that could not be inline-anchored
src/runtimes/python/extension.rs:95
No test exercises the error path here: when supply-chain.packages is configured but the org cannot be inferred, ctx.package_feed_url() returns Err, and this ? propagates it out of declarations(). Every existing packages-related test in this file uses CompileContext::for_test_with_org, so a regression that silently swallowed or mishandled this error during runtime-extension integration (as opposed to the unit-level PackageFeedConfig::url_for test in types.rs) would ship undetec…
src/compile/types.rs:568
This table-driven loop conflates two different failure mechanisms (rejected at Deserialize vs. only reachable via validate()), so when one case in the loop stops failing, the assertion just reports "expected rejection for: (yaml)" without saying which rule broke or why.
<details><summary>💡 Why it matters</summary>
The fourth case, unknown: true, tests #[serde(deny_unknown_fields)] — an orthogonal concern (schema strictness) from the URL-segment charset checks in the other three. If s…
…paths Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Addressed in All three reviewer comments were the same issue: the diagnostic-only branch that decides whether to warn " Added a non-failing Also added |
Pull request created by AI Agent