feat(#3693): add AIResourceOciProcessor for OCI ingestion validation#3764
Conversation
Implement AIResourceOciProcessor as a CatalogProcessor that validates OCI-backed AIResource entities during catalog ingestion. The processor checks that spec.location.target uses the oci:// URI scheme when spec.location.type is oci, and rejects malformed targets with actionable error messages including the field path, received value, and expected format. The processor makes zero outbound HTTP or registry network calls — validation is format-only, keeping ingestion stateless and air-gap safe per design decision D4. Changes: - AIResourceOciProcessor.ts: new processor with oci:// prefix, non-empty, and registry/repository path validation - module.ts: register AIResourceOciProcessor alongside the existing AIResourceExtensionsProcessor - index.ts: export AIResourceOciProcessor for public API - AIResourceOciProcessor.test.ts: 22 tests covering valid targets, malformed targets, non-OCI passthrough, non-AIResource passthrough, zero-network behavior, and error quality Closes #3693 Assisted-by: Claude
|
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 #3764 +/- ##
==========================================
+ Coverage 54.68% 54.70% +0.01%
==========================================
Files 2366 2367 +1
Lines 90284 90312 +28
Branches 25270 25278 +8
==========================================
+ Hits 49374 49401 +27
+ Misses 40621 40612 -9
- Partials 289 299 +10
*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-fix address failing CI checks, also ensure API exports updated |
|
🤖 Fix · ❌ Terminated · Started 10:17 PM UTC · Ended 10:31 PM UTC |
Regenerate report.api.md to include the new AIResourceOciProcessor class in the public API surface. This fixes the CI "check api reports" step that was failing because the export was added to index.ts but the API report was not updated. Addresses review feedback on #3764 Assisted-by: Claude
🔧 Fix agent — iteration 1 (human-triggered)Fixed failing CI by regenerating the API report (report.api.md) to include the new AIResourceOciProcessor export. The CI 'check api reports' step was failing because the new processor class was added to index.ts but the API report was not updated. All 41 tests pass. Fixed (2):
Tests: passed Updated by fullsend fix agent |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 2:39 PM UTC · Completed 2:44 PM UTC |
Review — commentPR #3764 adds Findings1. Multi-error reporting trade-off ·
|
| Dimension | Assessment |
|---|---|
| Correctness | ✅ Logic is sound; validation checks are sequentially dependent and ordered correctly |
| Security | ✅ Input is sanitized in error messages (control char stripping, 200-char truncation); zero network surface |
| Intent & coherence | ✅ Change matches issue #3693 scope exactly; no scope creep |
| Style/conventions | ✅ Follows patterns established by AIResourceExtensionsProcessor |
| Documentation | ✅ API report updated; JSDoc present on the public class |
| Cross-repo contracts | ✅ New public export is additive only; no breaking changes |
Observations
Strengths:
- Comprehensive test suite (22 tests) covering valid targets, malformed targets, non-OCI passthrough, non-AIResource passthrough, zero-network verification, and error quality.
- Error messages include field path (
spec.location.target), the received value, and expected format — meeting the spec's actionable-error requirement. - Sanitization of user-controlled values in error messages mirrors the existing processor's approach.
- Module registration is correct — the new processor runs alongside
AIResourceExtensionsProcessor.
Findings
1. OCI URI validation allows degenerate edge cases — low
File: AIResourceOciProcessor.ts (line 80 in diff)
The path check ociPath.length === 0 || !ociPath.includes('/') only verifies the path is non-empty and contains at least one /. This allows semantically degenerate URIs:
oci:///foopasses (no registry host —ociPathis/foo)oci://host/passes (empty repository —ociPathishost/)
These are unlikely in practice, and the validation is intentionally lightweight per design D4. A future tightening (e.g., requiring at least one non-empty segment on each side of the first /) would close these gaps without adding real complexity.
2. Misleading variable name locationType — low
File: AIResourceOciProcessor.ts (line 54 in diff)
const locationType = (entity.spec as Record<string, unknown> | undefined)
?.location as Record<string, unknown> | undefined;locationType actually holds the entire spec.location object, not the location type string. A name like specLocation or locationObj would be clearer and prevent future confusion.
3. Stale code comment in existing processor — low
File: AIResourceExtensionsProcessor.ts (line 51, not in this diff)
The existing processor's JSDoc says "Additional extension validators (for example OCI location rules) should be added here so errors can be reported together." This comment is now stale since OCI validation lives in a separate processor. Updating it would prevent confusion for future contributors.
All findings are low-severity follow-up suggestions. The implementation is correct, well-tested, and ready to merge.
![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
the
ready-for-merge
|
/fs-fix address review comments |
|
🤖 Finished Fix · ✅ Success · Started 7:01 PM UTC · Completed 7:13 PM UTC |
- Tighten OCI URI validation to reject degenerate edge cases like oci:///foo (no registry) and oci://host/ (empty repository) by splitting the path and verifying both registry and repository segments are non-empty - Rename misleading variable 'locationType' to 'specLocation' since it holds the entire spec.location object, not the type string - Update stale JSDoc in AIResourceExtensionsProcessor that incorrectly suggested OCI rules should be added there - Add test cases for the newly rejected degenerate URIs Addresses review feedback on #3764 Assisted-by: Claude
🔧 Fix agent — iteration 2 (human-triggered)Addressed all 3 review findings: tightened OCI URI validation to reject degenerate edge cases, renamed misleading variable, and updated stale JSDoc comment. Added 2 new test cases. All 43 tests pass. Fixed (3):
Tests: passed Updated by fullsend fix agent |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 7:49 PM UTC · Completed 8:02 PM UTC |
fullsend-ai-review
Bot
removed
the
ready-for-merge
fullsend-ai-review
Bot
added
requires-manual-review
Reject trailing-slash and whitespace edge cases in OCI targets, document AIResourceOciProcessor in the plugin README, and add a minor changeset. Addresses review feedback on #3764 Assisted-by: Cursor Grok 4.5 Co-authored-by: Cursor <cursoragent@cursor.com>
|
gabemontero
left a comment
gabemontero
left a comment
There was a problem hiding this comment.
had no other comments other than my take on the fullsend review comments @johnmcollier
|
🤖 Finished Retro · ✅ Success · Started 5:02 PM UTC · Completed 5:14 PM UTC |
|
PR #3764 added AIResourceOciProcessor for OCI ingestion validation in the ai-integrations workspace. The code agent produced a clean initial implementation with 22 tests, but missed the API report regeneration step (already tracked in #3757), causing one avoidable fix iteration. The review agent delivered high-quality findings across 2 review rounds (8 unique issues), but the fix agent's validation tightening was incomplete — it missed trailing-slash and whitespace edge cases of the same pattern it modified. The first review submitted as APPROVED despite its body stating a 'comment' verdict (additional evidence for fullsend-ai/fullsend#4585). A human reviewer agreed with all agent findings and raised no novel issues, suggesting strong autonomy readiness for this change class. The PR merged in ~2 days with 4 commits. Proposals filed
Evidence notes (not filed as issues)
|
…tion validation (redhat-developer#3764) * feat(redhat-developer#3693): add AIResourceOciProcessor for OCI ingestion validation Implement AIResourceOciProcessor as a CatalogProcessor that validates OCI-backed AIResource entities during catalog ingestion. The processor checks that spec.location.target uses the oci:// URI scheme when spec.location.type is oci, and rejects malformed targets with actionable error messages including the field path, received value, and expected format. The processor makes zero outbound HTTP or registry network calls — validation is format-only, keeping ingestion stateless and air-gap safe per design decision D4. Changes: - AIResourceOciProcessor.ts: new processor with oci:// prefix, non-empty, and registry/repository path validation - module.ts: register AIResourceOciProcessor alongside the existing AIResourceExtensionsProcessor - index.ts: export AIResourceOciProcessor for public API - AIResourceOciProcessor.test.ts: 22 tests covering valid targets, malformed targets, non-OCI passthrough, non-AIResource passthrough, zero-network behavior, and error quality Closes redhat-developer#3693 Assisted-by: Claude * fix(redhat-developer#3693): update API report for AIResourceOciProcessor export Regenerate report.api.md to include the new AIResourceOciProcessor class in the public API surface. This fixes the CI "check api reports" step that was failing because the export was added to index.ts but the API report was not updated. Addresses review feedback on redhat-developer#3764 Assisted-by: Claude * fix: address review feedback on OCI processor validation - Tighten OCI URI validation to reject degenerate edge cases like oci:///foo (no registry) and oci://host/ (empty repository) by splitting the path and verifying both registry and repository segments are non-empty - Rename misleading variable 'locationType' to 'specLocation' since it holds the entire spec.location object, not the type string - Update stale JSDoc in AIResourceExtensionsProcessor that incorrectly suggested OCI rules should be added there - Add test cases for the newly rejected degenerate URIs Addresses review feedback on redhat-developer#3764 Assisted-by: Claude * fix: tighten OCI URI validation and address review feedback Reject trailing-slash and whitespace edge cases in OCI targets, document AIResourceOciProcessor in the plugin README, and add a minor changeset. Addresses review feedback on redhat-developer#3764 Assisted-by: Cursor Grok 4.5 Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Co-authored-by: John Collier <jcollier@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Implement AIResourceOciProcessor as a CatalogProcessor that validates OCI-backed AIResource entities during catalog ingestion. The processor checks that spec.location.target uses the oci:// URI scheme when spec.location.type is oci, and rejects malformed targets with actionable error messages including the field path, received value, and expected format.
The processor makes zero outbound HTTP or registry network calls — validation is format-only, keeping ingestion stateless and air-gap safe per design decision D4.
Changes:
non-empty, and registry/repository path validation
AIResourceExtensionsProcessor
malformed targets, non-OCI passthrough, non-AIResource passthrough,
zero-network behavior, and error quality
Assisted-by: Claude
Closes #3693
Post-script verification
agent/3693-ai-resource-oci-processor)6e585f281bb664af5c79c8b717c8b3a949c247ea..HEAD)