Repository navigation
fix(import): preserve workbench subtypes and include serialization - #237
Conversation
✅ Deploy Preview for adt-cli canceled.
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly summarizes the main changes: preserving workbench subtypes during import and adding include serialization support. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 9… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 9 files.
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Reviewer's GuidePreserve registered workbench subtypes and ABAP include identity across transport and single-object imports by using full ADT types for object resolution, while retaining CTS base types for compatibility and format selection; extend abapGit serialization to distinguish PROG/I includes from executable reports. Sequence diagram for subtype-aware transport importsequenceDiagram
participant Transport as TransportImport
participant Registry as TypeRegistry
participant Factory as ADKFactory
participant Endpoint as ObjectEndpoint
Transport->>Registry: getRegisteredTypes()
Transport->>Factory: get(name, workbenchType)
Factory->>Endpoint: load()
Endpoint-->>Factory: subtypeObject
Factory-->>Transport: loadedObject
Sequence diagram for full-type single-object importsequenceDiagram
participant Import as ImportService
participant Factory as ADKFactory
participant Endpoint as SubtypeEndpoint
participant Format as FormatRegistry
Import->>Factory: get(name, fullType)
Factory->>Endpoint: load()
Endpoint-->>Factory: loadedObject
Import->>Format: serialize(baseType)
Format-->>Import: serializedObject
Flow diagram for include and report ABAP serializationflowchart LR
Source[ABAP object] --> Handler[ProgramHandler]
Handler -->|AdkInclude| Include[PROG/I]
Handler -->|AdkProgram| Report[PROG/P]
Include --> IncludeFiles[.prog.abap and .prog.xml with SUBC I]
Report --> ReportFiles[.prog.abap and .prog.xml with SUBC 1]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Secrets |  |
Oct 22, 2026 5:37p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR correctly addresses the core problem: workbench subtypes (e.g. PROG/I) being collapsed to their CTS base type during transport import, and adds PROG/I include serialization as .prog.abap/.prog.xml.
Critical Issue Found
packages/adt-plugin-abapgit/src/lib/handlers/objects/prog.ts — fromAbapGit type asymmetry
The toAbapGit serializer is correctly updated to emit SUBC: 'I' for includes, but fromAbapGit still hard-codes type: 'PROG/P' for all objects regardless of SUBC. When deserializing (Git → SAP) an include, the reconstructed ADK data will carry PROG/P instead of PROG/I, which will cause the factory to resolve it against the executable-program endpoint rather than the includes endpoint. The fix is to return type: subc === 'I' ? 'PROG/I' : 'PROG/P' in fromAbapGit.
Changes That Look Correct
transport-import.ts: Thewbtypelookup logic (startsWith(${ctsType}/)+ registry check) correctly selects the registered workbench subtype for factory resolution while falling back to the CTS base type.incl.model.ts: Dual registration ofINCLandPROG/Iis the right approach for backward compatibility.service.ts(importObject): PassingfullType(e.g.PROG/I) instead ofbaseTypetofactory.get()ensures the correct ADK endpoint is used while still usingbaseTypefor the format registry — this is the correct fix.adk.ts:AdkIncludeexport added correctly.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 39 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
View your CI Pipeline Execution ↗ for commit b5394ab
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
codeant-ai
Bot
added
the
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/adt-plugin-abapgit/src/lib/handlers/objects/prog.ts" line_range="9" />
<code_context>
import { createHandler } from '../base';
-export const programHandler = createHandler(AdkProgram, {
+export const programHandler = createHandler<AdkProgram | AdkInclude>('PROG', {
schema: prog,
version: 'v1.0.0',
</code_context>
<issue_to_address>
**Same-name includes overwrite reports**
When a transport contains an include and executable report with the same name, `importTransport` processes objects in parallel, and the PROG handler gives both includes and executable programs the same `.prog.abap` and `.prog.xml` paths. If a transport contains a same-named include and report, their writes race and one object’s files overwrite the other’s.
Detect conflicting output paths before writing, or serialize each object to a distinct path when names collide.
</issue_to_address>
### Comment 2
<location path="packages/adk/src/objects/repository/incl/incl.model.ts" line_range="178" />
<code_context>
registerObjectType('INCL', IncludeKind, AdkInclude, {
endpoint: 'programs/includes',
});
+registerObjectType('PROG/I', IncludeKind, AdkInclude, {
+ endpoint: 'programs/includes',
+});
</code_context>
<issue_to_address>
**Includes resolve as programs**
When a caller asks for the kind of `PROG/I`, `getKindForType` strips the subtype and looks up `PROG`, returning `ProgramKind`; callers using the type-to-kind mapping therefore classify includes as programs.
Preserve the `PROG/I` subtype in type-to-kind resolution and return `IncludeKind`.
</issue_to_address>
### Comment 3
<location path="packages/adt-plugin-abapgit/src/lib/handlers/objects/prog.ts" line_range="43" />
<code_context>
return {
name: (PROGDIR?.NAME ?? '').toUpperCase(),
- type: 'PROG/P',
+ type: subc === 'I' ? 'PROG/I' : 'PROG/P',
description: descriptionEntry?.ENTRY ?? 'Program',
fixPointArithmetic: PROGDIR?.FIXPT === 'X',
</code_context>
<issue_to_address>
**Include metadata says subroutine pool**
When abapGit metadata has `PROGDIR.SUBC` equal to `I`, `fromAbapGit` sets `type` to `PROG/I` for an include but sets `programType` to `subroutinePool` for every `SUBC` other than `1`. Deserialized include data therefore carries conflicting type metadata to callers.
Map `SUBC === 'I'` to an include-specific `programType`, or omit `programType` when that property does not apply to includes.
</issue_to_address>
### Comment 4
<location path="packages/adt-services/tests/services/import/object.test.ts" line_range="125" />
<code_context>
+ });
+
+ expect(factoryGet).toHaveBeenCalledWith('ZAPI_STRUCTURE', 'TABL/DS');
+ expect(result.objectType).toBe('TABL');
+ });
});
</code_context>
<issue_to_address>
**Subtype format check goes unverified**
When the import implementation checks format support using the full search type instead of the CTS base type, when `ImportService.importObject()` passes `TABL/DS` to the format support check, the test's `isSupported: vi.fn(() => true)` mock accepts it, and `result.objectType` still comes from the base type, so this test passes without verifying that format support is checked with `TABL`.
Mock `isSupported` with an argument-sensitive implementation and assert it is called with `TABL`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 4 findings to address first, and if a subtype resolves to the wrong object or an include is serialized incorrectly, the import can leave incorrect .prog.abap or metadata files in the output directory. Reverting restores the old behavior, but already-generated files remain until removed or regenerated; the damage is bounded and repairable.
Blocking findings: packages/adt-plugin-abapgit/src/lib/handlers/objects/prog.ts:9, packages/adk/src/objects/repository/incl/incl.model.ts:178, packages/adt-plugin-abapgit/src/lib/handlers/objects/prog.ts:43, packages/adt-services/tests/services/import/object.test.ts:125
 [Medium impact] The PR appears safe to merge; no blocking issues remain.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
CodeAnt PR Risk: Low Risk
Assessed commit: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
/q review |
|
@greptileai Please review the current head b5394ab. The prior schema-generic finding has been addressed and current-head CI has passed. |
There was a problem hiding this comment.
Review Summary
This PR correctly introduces PROG/I subtype preservation during transport/single-object imports and adds AdkInclude serialization support to the abapGit plugin. The structural approach — registering PROG/I as an alias in the registry and using wbtype to select the right factory type — is sound. However, there are several logic errors introduced that need to be fixed before merge:
Critical Issues
-
kindToAdtoverwrite (incl.model.ts) — RegisteringPROG/IwithIncludeKindoverwriteskindToAdtentry set by the earlierINCLregistration.getTypeForKind(Include)andresolveKind(Include)will returnPROG/Iafter module load, breaking any code that relies on the reverse kind→type mapping for theIncludekind. -
isTypeRegisteredmisses exact subtype (registry.ts) — The function only checksgetMainType(), soisTypeRegistered('PROG/I')returnsfalseeven thoughPROG/Iis registered. SincegetKindForTypewas already updated to prefer exact matches,isTypeRegisteredmust be updated consistently. -
factoryTypefallback uses rawthis.type(transport-import.ts) — The fallback forfactoryTypeisthis.type(the raw, possibly-dirty string) rather than the already-trimmed/uppercasedctsType. Additionally,getRegisteredTypes().includes()performs an O(n) linear scan on everyload()call;resolveType(workbenchType) !== undefinedachieves the same check in O(1). -
programSubctype narrowing (prog.ts) — The function accessesobj.dataSync.programTypeon aAdkProgram | AdkIncludeunion. While thekindcheck guards theAdkIncludebranch at runtime, TypeScript can't statically guarantee the narrowing inside theswitch, and the property access is not type-safe forAdkInclude. Explicitly narrowing toAdkProgrambefore accessing program-specific properties would prevent future regressions if the guard is accidentally moved or removed.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 39 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@coderabbitai review |
|
@greptileai review |
|
/q review |
|
There was a problem hiding this comment.
Review Summary
This PR correctly addresses the core goal of preserving workbench subtypes (e.g., PROG/I) during transport imports, registering PROG/I as a distinct subtype for includes, and serializing program subtypes (M, S, I) in the abapGit handler. The logic for resolveType and isTypeRegistered fixes are sound improvements.
Critical issues found:
-
registry.ts—kindToAdtlast-writer-wins collision (getKindForType, line 190-191 diff): The dual registration ofINCLandPROG/Ifor the sameIncludeKindinincl.model.tsmeanskindToAdtwill store whichever was registered last as the canonical type for includes.resolveKind(IncludeKind)andgetTypeForKind(IncludeKind)will returnPROG/I, notINCL. Any code relying on kind-to-type reverse lookup for includes will see the wrong type. -
transport-import.ts— wbtype-preservation logic edge case (lines 192-199): ThestartsWith(${ctsType}/)condition correctly handlesPROG→PROG/I, but will silently fall back toctsTypefor any case wherewbtypeequalsctsType(no slash in wbtype, e.g.,INCL). This is likely intentional, but worth confirming it doesn't regress include transport imports where bothtypeandwbtypeareINCL. -
prog.ts—programSubctype narrowing (lines 9-12): Theswitchonobj.dataSync.programTypeis reached only for non-include objects guarded byisLoaded, butdataSyncis typed from the unionAdkProgram | AdkInclude. IfAdkIncludedoes not have adataSyncproperty on its public type, TypeScript may still compile (due to the union) but runtime access could fail for any object that bypasses the kind guard. Verify that accessingdataSyncon the union type does not introduce a runtime crash path.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Summary
Validation