RHIDP-15776: Align inherit matching with the operator - #4808
subhashkhileri merged 5 commits into
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4808 +/- ##
==========================================
+ Coverage 63.26% 63.29% +0.03%
==========================================
Files 2675 2675
Lines 106439 106553 +114
Branches 29767 29815 +48
==========================================
+ Hits 67340 67447 +107
- Misses 37320 37327 +7
Partials 1779 1779
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
/fs-review |
|
🤖 Finished Review · ❌ Failure (agent timed out after 20m0s without completing (timeout: 20m0s)) · Started 12:42 PM UTC · Completed 1:04 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 |
|
/fs-review |
|
🤖 Finished Review · ❌ Failure (agent timed out after 20m0s without completing (timeout: 20m0s)) · Started 2:53 PM UTC · Completed 3:16 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 5:38 AM UTC · Completed 6:16 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $13.38 |
Our code/fix/review harnesses pinned fullsend-ai/agents@4bbe4f50e, set once in redhat-developer#4496 and never bumped — 794 commits behind main. Move to the agents@v0 tag (7ab05564b, 794 commits later) to pick up two fixes relevant to the /fs-review timeout on PR redhat-developer#4808 (run 35097265739): - fullsend-ai/agents#1116 (PR redhat-developer#1181): sonnet-tier review sub-agents now request the bare `sonnet` alias, pinned via ANTHROPIC_DEFAULT_SONNET_MODEL=claude-sonnet-4-6 in env/gcp-vertex.env, instead of the hardcoded `claude-sonnet-4-6@default` literal our pin still carries. That literal was falling through the CLI's own alias resolution to claude-sonnet-4 (Sonnet 4.5 not available on our Vertex project), landing on a base model with a 120k output-tokens/min quota vs. 300k for sonnet-4-6 — three parallel sonnet sub-agents blew through it and all failed with 429 RESOURCE_EXHAUSTED. - review harness timeout_minutes raised 20 -> 45, which independently would have absorbed the degraded run instead of hard-timing-out. Scope: base: URL + content sha256 only. Our rhdh/policies/*.yaml overrides are left as-is for now — upstream moved network policy from policy: to composable providers:/openshell: profiles: in the interim (ADR 0065), which likely makes the claude.exe allowlist in our policy fork redundant (profiles/fullsend-vertex-ai.yaml already allowlists it upstream), but retiring that fork is deferred to a follow-up so this change stays a minimal, low-risk version bump.
Our code/fix/review harnesses pinned fullsend-ai/agents@4bbe4f50e, set once in redhat-developer#4496 and never bumped — 794 commits behind main. Move to the agents@v0 tag (7ab05564b, 794 commits later) to pick up two fixes relevant to the /fs-review timeout on PR redhat-developer#4808 (run 35097265739): - fullsend-ai/agents#1116 (PR redhat-developer#1181): sonnet-tier review sub-agents now request the bare `sonnet` alias, pinned via ANTHROPIC_DEFAULT_SONNET_MODEL=claude-sonnet-4-6 in env/gcp-vertex.env, instead of the hardcoded `claude-sonnet-4-6@default` literal our pin still carries. That literal was falling through the CLI's own alias resolution to claude-sonnet-4 (Sonnet 4.5 not available on our Vertex project), landing on a base model with a 120k output-tokens/min quota vs. 300k for sonnet-4-6 — three parallel sonnet sub-agents blew through it and all failed with 429 RESOURCE_EXHAUSTED. - review harness timeout_minutes raised 20 -> 45, which independently would have absorbed the degraded run instead of hard-timing-out. Scope: base: URL + content sha256 only. Our rhdh/policies/*.yaml overrides are left as-is for now — upstream moved network policy from policy: to composable providers:/openshell: profiles: in the interim (ADR 0065), which likely makes the claude.exe allowlist in our policy fork redundant (profiles/fullsend-vertex-ai.yaml already allowlists it upstream), but retiring that fork is deferred to a follow-up so this change stays a minimal, low-risk version bump.
ReviewFindingsMedium
Low
Next steps:
Previous runReviewReason: stale-head The review agent reviewed commit Previous run (2)Review — commentPR: RHIDP-15776: Align inherit matching with the operator SummaryThis PR replaces the old same-registry The changeset bump level ( No critical or high findings. One medium observation about code maintainability, and several low-severity style/coverage notes. FindingsMedium1. Dual resolution path with divergent candidate sets (
In the normal Recommendation: Consider documenting the precondition that Low2. Missing test coverage for cross-version explicit-path inherit ( When candidates from the same registry have different versions (e.g., 3. Dropped same-registry explicit override test ( The old test verifying that a main entry with an explicit tag correctly overrides an include's version on the same registry was removed without a direct replacement. The normal merge path is unmodified, so the risk is low, but the interaction boundary is worth covering. 4. Every other error site in this file throws inline with 5.
6. Architecture table incomplete ( The architecture table does not mention Positive observations
Previous run (3)ReviewThis PR refactors The FindingsMedium1. False name-collision when mixing
Example: # Include
- package: oci://quay.io/rhdh/my-plugin:1.0!plugin-a
# Main
- package: oci://registry.redhat.io/rhdh/my-plugin:{{inherit}}!plugin-a # resolves to quay.io
- package: oci://registry.redhat.io/rhdh/my-plugin:2.0!plugin-b # stays registry.redhat.io
# → false collision: quay.io vs registry.redhat.io for name 'my-plugin'Remediation: Either snapshot the original main packages before 2. Name-based matching reduces registry isolation (by design) ( The old code scoped Remediation: Document in operator-facing docs that include file contents directly influence cross-registry Low3. Stale reference to renamed function ( Comment references the old function name 4. Stale comment about matching semantics ( Comment says "The merger will match against an earlier included plugin from the same image" but matching is now by plugin name across registries, not by "same image." 5. Disabled-candidate asymmetry (
6. Collision detection applies beyond
Labels: PR modifies the install-dynamic-plugins workspace with a feature-level change to {{inherit}} resolution |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 7:49 AM UTC · Completed 8:10 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $8.53 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
|
/fs-review |
|
🤖 Finished Review · ❌ Failure (agent timed out after 20m0s without completing (timeout: 20m0s)) · Started 8:21 AM UTC · Completed 8:43 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 |
subhashkhileri
changed the title
…4843) Our code/fix/review harnesses pinned fullsend-ai/agents@4bbe4f50e, set once in #4496 and never bumped — 794 commits behind main. Move to the agents@v0 tag (7ab05564b, 794 commits later) to pick up two fixes relevant to the /fs-review timeout on PR #4808 (run 35097265739): - fullsend-ai/agents#1116 (PR #1181): sonnet-tier review sub-agents now request the bare `sonnet` alias, pinned via ANTHROPIC_DEFAULT_SONNET_MODEL=claude-sonnet-4-6 in env/gcp-vertex.env, instead of the hardcoded `claude-sonnet-4-6@default` literal our pin still carries. That literal was falling through the CLI's own alias resolution to claude-sonnet-4 (Sonnet 4.5 not available on our Vertex project), landing on a base model with a 120k output-tokens/min quota vs. 300k for sonnet-4-6 — three parallel sonnet sub-agents blew through it and all failed with 429 RESOURCE_EXHAUSTED. - review harness timeout_minutes raised 20 -> 45, which independently would have absorbed the degraded run instead of hard-timing-out. Scope: base: URL + content sha256 only. Our rhdh/policies/*.yaml overrides are left as-is for now — upstream moved network policy from policy: to composable providers:/openshell: profiles: in the interim (ADR 0065), which likely makes the claude.exe allowlist in our policy fork redundant (profiles/fullsend-vertex-ai.yaml already allowlists it upstream), but retiring that fork is deferred to a follow-up so this change stays a minimal, low-risk version bump.
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 12:03 PM UTC · Completed 12:35 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $10.73 |
4e545bd to
30dbd15
Compare
|
Risk Assessment: moderate (2/5) DetailsThe PR is large in raw size (1301 lines, large blast radius across 9 files) but is well-mitigated by a 0.33 test file ratio, no protected paths, no security-sensitive or CI/dependency changes, low file churn, and minimal regression history, yielding a moderate composite risk. Previous runRisk Assessment: moderate (2/5) DetailsThe PR touches 9 files with 1234 lines changed and a large blast radius within the install-dynamic-plugins workspace, but risk is moderated by good test coverage (ratio 0.33), no security-sensitive or protected-path changes, no CI modifications, low churn, and a familiar non-first-time author; three recent fix commits on core files in the past 90 days add a small regression signal. |
|
/fs-review |
4e545bd to
84b8264
Compare
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 1:23 PM UTC · Completed 1:53 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $10.36 |
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
subhashkhileri
changed the title
3 participants
Summary
This PR ports the operator's name-based OCI
{{inherit}}lookup to the dynamic-plugins init container.{{inherit}}by the final OCI image name, independent of registry host and namespace.{{inherit}}entry to re-enable.!plugin-pathbehavior. Removing that syntax remains separate work in RHIDP-16807.Problem
The init container previously keyed OCI plugins by their full registry URL. That made inheritance depend on the registry used at each deployment stage.
For example, a catalog might contain:
while the deployment configuration contains:
Both packages describe
backstage-plugin-catalog, but the old full-URL lookup treated them as unrelated and failed withno existing plugin configuration found. This could abort the init container even though the catalog already provided the version to inherit.Resolution behavior
The new flow mirrors the operator:
{{inherit}}reference—for example,backstage-plugin-catalog.!plugin-path, when present.For the example above, the main entry becomes:
The registry written on the
{{inherit}}reference is therefore only a lookup hint. The selected catalog entry remains the source of the concrete registry and version installed by the init container.Candidate selection and ambiguity
Candidate selection is deterministic and follows the accepted compatibility rules:
{{inherit}}cannot select one safely. The error names both conflicting packages and their source files.{{inherit}}entry remains a no-op and is removed by disabled filtering instead of aborting startup.Explicit
!plugin-pathbehaviorExisting explicit-path support is preserved:
bundle:1.0!plugin-aandbundle:1.0!plugin-b.Example:
The result contains both:
Compatibility
The following existing behavior is unchanged:
{{inherit}}OCI entries continue to use their concrete registry, image, version, and plugin path for merging.ref://behavior is unaffected.The intentional validation change required by RHIDP-15776 is that two enabled images at the same merge level cannot share the same final OCI image name, even when their registry hosts or namespaces differ. Such a catalog is ambiguous for host-independent
{{inherit}}lookup and now fails with a diagnostic instead of relying on input order.Acceptance criteria coverage
{{inherit}}across registry host and namespace differences.!plugin-pathoverrides and multi-plugin image entries.Validation
CI=true yarn test --runInBand— 20 suites, 275 tests passedyarn tscyarn lint:allyarn prettier:checkyarn workspace @red-hat-developer-hub/cli-module-install-dynamic-plugins buildyarn build:api-reports:only --ciRelated work
!plugin-path: RHIDP-16807