/abap-code-review-helper

name: abap-code-review-helper
description: 'Primary correctness and runtime-safety review guidance for ABAP reports, classes, function modules, enhancements, BAPIs, interfaces, and other ABAP objects. Use first when the user asks to review ABAP code or an ABAP report for wrong results, dumps, lost updates, security defects, or performance risks. Covers old values left in loop variables, SELECT and FOR ALL ENTRIES, binary search, parallel cursors, duplicate keys, conversions, commits, locks, and retries. Report problems, not lists of passed checks; use clean-abap separately for style-focused findings when that skill is available, and continue the correctness review without it when it is disabled or unavailable.'

ABAP Coding Pitfalls

These requirements describe what the code must do. For reviews, report violations; for authorized changes, make the code meet the applicable requirements. Report important unknowns, not passed-check lists unless asked. Never claim checks or tests that were not performed.

Review safeguards

1. **A finding must describe a reachable failure in the current code.** A hypothetical future change is not a current defect. 2. **Existing protections must be traced before reporting a missing check.** An enclosing branch, earlier exit, full assignment, or earlier compatible sort can already satisfy the requirement. 3. **A safe equivalent must be accepted.** The code need not use the reviewer's preferred syntax. 4. **A claim disproved during investigation must be removed from the findings and severity totals.** 5. **Quoted code, types, keys, and signatures must match inspected source.** Missing information must remain unknown. 6. **Security findings require an actual input/control path to a security-sensitive operation.** Dynamic ABAP conditions are not automatically SQL injection. 7. **Correctness defects, conditional risks, measured performance problems, and maintainability suggestions must be kept separate.**

Examples of mistakes that pass syntax checks

These are examples, not the complete list. The later requirements also apply wherever relevant to the code.

• **Old data must not be used after a failed read.** After `READ TABLE ... INTO` or `SELECT SINGLE`, `sy-subrc` must be checked or saved before another operation can overwrite it. The result must be checked before the target is used. If no row was found, the target work area must not be used as a successful result: it may still hold data from the previous read. The return code must determine whether a row was found, not whether the work area is initial; a real row can contain blank or zero values.
• **Temporary loop variables must be cleared or fully assigned before use on each iteration.** A skipped assignment must not leave the previous row's value in use. Running totals and other values needed across iterations must not be cleared prematurely. Clearing is unnecessary when the value is always replaced completely before use.
• **Internal-table inserts and updates must use the statement that matches the intended action.**
• `INSERT ... INTO TABLE` adds a row. `MODIFY TABLE` changes an existing row found by the primary table key; it must not be used to insert a missing row. `MODIFY ... INDEX` changes the row at that index.
• Duplicate handling must match the actual statement and table keys. A duplicate can set a failure return code, raise an exception, or cause a dump. `sy-subrc` must be checked when the statement sets it; exceptions must be handled where required. This also applies to bulk inserts, constructors, and unique secondary keys.
• `INSERT` must not be replaced with `MODIFY` just to hide duplicates. The business rule must determine whether a duplicate is rejected, skipped, merged, or updated.
• **Database `INSERT`, `UPDATE`, and `MODIFY` must match the allowed create/change behavior.**
• `INSERT` is for create-only and `UPDATE` for change-only. Database `MODIFY` must be used only when insert-or-update by primary key is intended. Unlike internal-table `MODIFY TABLE`, it can insert a missing row.
• Duplicate primary keys and conflicts with unique secondary indexes must be handled. Depending on the statement, these can return a failure code, raise an exception, or cause a dump. `MODIFY` can still fail on a unique secondary index.
• A duplicate must not be ignored unless the business rule allows it. A preceding existence check is not enough: another session can insert the row before this session does.
• **The table must be sorted correctly before `BINARY SEARCH`.** On a standard table, the search fields must be the leading sort fields, in ascending order and in the same sequence as the search key. A descending sort or `AS TEXT` sort is not a substitute. Appends or changes must not break that order before the search. A suitable declared sorted or hashed key is preferable when it removes the need to maintain this order manually.
• **An index-based parallel cursor must have the correct sort, start index, and exit check.**
• The tables must be ordered by the grouping fields as required by the algorithm. The first matching inner row's `sy-tabix` must be saved before another operation can overwrite it, and the inner loop must start `FROM` that saved index.
• The inner loop must exit when any grouping field differs (`NE`/`<>`). Comparing only part of a composite group key or continuing into the next group is incorrect.
• Duplicate keys in both tables must be handled. An inner group must remain available to any later outer row that also needs it. The last successful index must not be reused after a failed search.
• **The FAE table must be checked for empty before the SELECT.** `SELECT ... FOR ALL ENTRIES IN @lt_keys` must be guarded by `IF lt_keys IS NOT INITIAL` or an earlier return/skip when it is empty. With an empty FAE table, the entire explicit `WHERE` condition can be ignored, not just conditions using `lt_keys`. The query can then read every row allowed by implicit restrictions su