Hochfrequenz / Hochfrequenz/Z_ABAPGIT_PULL_MCP_SHORTCUT
PULL can bind the wrong repository (CS + first hit); LIST cannot signal truncation
- Dominant language
- ABAP
- Stars
- 0
- Forks
- 0
- Avg merge
- 1h 3m
- Merged PRs (30d)
- 1
Description
Two defects found while driving this report through `sapgui.mcp`, on **`HF S/4 Mandant 100`** (S/4HANA, `S4U`, client 100). Both are reproducible there. The second is the dangerous one.
## 1. PULL matches on `CS` and takes the first hit — it can deserialize into the wrong package
```abap
LOOP AT zcl_abapgit_repo_srv=>get_instance( )->list( iv_offline = abap_false ) INTO DATA(li_repo).
IF li_repo->get_name( ) CS p_repo.
lo_repo ?= li_repo.
EXIT.
ENDIF.
ENDLOOP.
```
Three properties combine badly:
- **`CS` is a substring test**, so a short `P_REPO` matches every registered repository whose name merely *contains* it.
- **`CS` ignores case**, widening it further.
- **`EXIT` takes the first match in `list( )` order** — which happens to be registration order today, but is not part of any contract and is not something a caller should rely on.
So a caller who supplies a name that is not unique gets *an* arbitrary repo, silently. `deserialize` then overwrites the objects of **that repo's package** — with no confirmation, because this report deliberately auto-answers every overwrite decision with `'Y'`:
```abap
LOOP AT ls_checks-overwrite ASSIGNING FIELD-SYMBOL().
-decision = 'Y'.
ENDLOOP.
```
That auto-confirm is right for non-interactive automation, but it removes the last thing that would have caught a mis-bound repo. The failure mode is a silent overwrite of an unrelated package, reported as `Pull successful:`.
Not hypothetical: this system has 53 registered repositories and two of them have names where one is a substring of the other, both online.
**Suggested fix** — proposed in the linked PR: collect hits instead of taking the first, match exactly, and make an ambiguous `P_REPO` a distinct error rather than a silent pick.
Matching on the **URL** as well as the name is worth having, because a URL is unique per repository whereas `get_name( )` falls back to a URL-derived value when the stored name is blank — which it is for 45 of our 53 entries. It has to be **normalised**, though: registered URLs differ in case, in a trailing slash and in a `.git` suffix, and abapGit itself compares them case-insensitively rather than literally, so a naive `=` would reject URLs that are demonstrably registered.
## 2. LIST output is silently truncated when read back
`LIST` mode writes one line per repo to a classic list:
```abap
LOOP AT zcl_abapgit_repo_srv=>get_instance( )->list( ) INTO DATA(li_repo_list).
...
WRITE: / lv_line.
ENDLOOP.
```
The report is doing its part — the loop is over all repositories. But a classic list is **paged**, and the consumer reads only the visible page, so the result comes back short with no indication that it is partial:
| | |
|---|---|
| rows the report writes (both list pages) | **53** |
| rows on the first, visible list page | **40** |
| repos returned to the caller | **37** |
**None of those numbers is a constant.** The report writes all 53 lines; the consumer reads only the first page of the classic list; and how many lines that page holds is simply the height of the SAP GUI window. Resize it and the figures move. Reading the list page by page in the GUI confirms the report is innocent: page 1 holds keys `…01`–`…44` (40 rows, no gaps), page 2 holds `…45`–`…57` (13 rows).
**Three separate things lose rows, and only the first is paging.**
- **13 lost positionally** — the second page, never read. Because keys are assigned in registration order, these are the **most recently registered** repositories: exactly the ones an automation caller is likely to want.
- **3 lost to a content filter** — the offline repositories that sit *on the visible page*. The report writes them (they appear as `~~~~~~X`), yet no offline entry is returned at all and every returned row has `is_offline: false`. The consumer appears to discard rows whose URL field is empty — which looks like a parser bug, since the output contract has an `is_offline` field.
- **Horizontal truncation at 151 characters**, which is the one that *corrupts* rather than omits. `LINE-SIZE 1023` is honoured by the report; the consumer just cannot see past the window width, so longer lines are cut mid-field. Three repositories came back with `last_pull_at` values of `"2026"`, `"2026042"` and `"202605151"` — fragments of a timestamp — with `last_pull_by` null and the offline flag silently gone. A short read is at least detectable in principle; a mangled timestamp reads as a plausible value.
We hit the paging loss concretely: a repository at key `000000000057`, demonstrably registered and with a populated `DESERIALIZED_AT`, did not appear in the listing at all.
(For anyone reaching for the obvious hypothesis: a blank `` is *not* the discriminator. 45 of the 53 entries have it blank — the displayed label comes from `get_name( )`, not the persisted `NAME` — and most of those were listed.)
**Why this compounds defect 1.** An agent calls LIST, does not find the repository it needs, and concludes the name must be different. It then guesses — and `CS` binds the guess to whatever matches first. The two defects together turn "I could not find my repo" into "I overwrote somebody else's package".
**Suggested fix.** The first item is in this repo and is proposed in the linked PR; the other two are consumer-side.
- Emit the count as the **first** line (`TOTAL~`) so it is always on the visible page. A *trailing* total lands on the page a truncating consumer never reads — i.e. exactly the case it exists to detect. Note this is a **breaking change** for any consumer that splits every line into seven fields, so it has to land with or after the reader change.
- Page down until the page indicator stops advancing, rather than reading one screen.
- Stop discarding rows with an empty URL, and read the full line width.
## Smaller notes
- **`LIST` and `PULL` see different repo sets.** `LIST` calls `list( )` (all repositories) while `PULL` calls `list( iv_offline = abap_false )` (online only). A repository shown by `LIST` can therefore be un-findable by `PULL`, with the error `Repository not found:` and no hint as to why. Either align them or say in the `LIST` output which entries are online.
- `REPORT ... LINE-SIZE 1023` is honoured and is **not** the limit anyone is hitting — the 151-character cut described above is the consumer's viewport, not this. Worth knowing so the `LINE-SIZE` is not "fixed" in pursuit of it.
## Reproduction
System **`HF S/4 Mandant 100`** (S/4HANA, `S4U`, client 100), where all figures above were taken.
```
1. Run the report with P_ACTION = 'LIST' and count the visible lines.
2. Press page-down: the remaining rows are there. The report is not at fault.
3. Compare the total with: SELECT COUNT( * ) FROM zabapgit WHERE type = 'REPO'
4. Check a long line against the same row in the table - it is cut at the
window width, mid-field.
5. For defect 1: pick any P_REPO that is a substring of two registered
repository names and observe which one is bound - no error is raised.
```
**It reproduces on our ECC 7.40 system too (`HF R3 Mandant 100`, `HFQ`, client 100), harder:** 270 registered repositories, 57 lines on the visible page, 44 returned to the caller — about 16 % of what is registered.
Two systems with two different cut-offs (40 lines and 57) is the clearest evidence that this is a viewport limit rather than a row cap, and is why no fixed number in this report should be treated as reproducible.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by tracing the report's P_ACTION='LIST' and P_ACTION='PULL' branches, especially calls to zcl_abapgit_repo_srv=>get_instance( )->list( ) and the P_REPO matching loop. Reproduce the LIST paging and PULL ambiguity on the described systems, then verify that repository selection reports ambiguity and that listing preserves count, offline rows, and full-width fields.
Written by the indexing model from the issue text.
Assessment
- Domain
- backend-api-design, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 43/100