fix(adt): parseActivationResult and parseSyntaxCheckResults silently ignore SAP error messages due to XML namespace mismatch

[txape10]

Summary

parseActivationResult reports Success: true even when SAP returns E-type activation errors. parseSyntaxCheckResults may also silently drop error messages. Root cause: Go's encoding/xml does not match namespace-qualified attributes (adtcomp:type="E") against unqualified struct tags (xml:"type,attr").

Root cause

SAP ADT activation responses use namespace-prefixed attributes:

<adtcomp:activationLog xmlns:adtcomp="http://www.sap.com/adt/activation">
  <adtcomp:messages>
    <adtcomp:msg adtcomp:type="E" adtcomp:objDescr="ZTEST" adtcomp:line="5">
      <adtcomp:shortText><adtcomp:txt>Syntax error</adtcomp:txt></adtcomp:shortText>
    </adtcomp:msg>
  </adtcomp:messages>
</adtcomp:activationLog>

Go's xml.Unmarshal matches element names by local name regardless of namespace prefix (so <adtcomp:msg> is found by xml:"msg"), but attribute matching is strict: xml:"type,attr" will NOT match adtcomp:type="E". Result: msg.Type is always "", strings.ContainsAny("", "EAX") is false, and Success stays true.

parseSyntaxCheckResults has a partial workaround (strings.ReplaceAll(xmlStr, "chkrun:", "")) but it turns xmlns:chkrun="..." into a default namespace declaration xmlns="...", which can cause Go's parser to miss elements in that namespace.

Impact

• Activation errors (syntax errors, ATC findings blocking activation) are silently swallowed — vsp reports success even when SAP refused to activate the object
• Syntax check results may be missed depending on SAP version / namespace declaration style
• Affects all object types, but most visible when activating INCL objects where SAP returns error messages without inactiveObjects (the fallback check)

Fix

Add a stripXMLNamespaces() helper that removes all namespace declarations and all prefix: prefixes before unmarshaling. Apply to both parsers.

var (
    reXMLNSDecl   = regexp.MustCompile(`\s*xmlns(?::\w+)?="[^"]*"`)
    reXMLNSPrefix = regexp.MustCompile(`(</?|\s)(\w[\w-]*):([\w])`)
)

func stripXMLNamespaces(data []byte) []byte {
    s := reXMLNSDecl.ReplaceAllString(string(data), "")
    s = reXMLNSPrefix.ReplaceAllString(s, "${1}${3}")
    return []byte(s)
}

We have this fix running on-prem S/4HANA in our fork (txape10/vibing-steampunk, commit dedfebc) with 6 unit tests covering both parsers and both failure modes (with and without inactiveObjects).

[txape10]

This has been fixed in our fork txape10/vibing-steampunk.

Root cause: Go's encoding/xml matches elements by local name regardless of namespace prefix, but does not match namespace-qualified attributes (adtcomp:type="E") against unqualified struct tags (xml:"type,attr"). This caused msg.Type to always be "" → strings.ContainsAny("", "EAX") = false → Success = true even when SAP returned error messages.

The same issue existed partially in parseSyntaxCheckResults, which had a manual strings.ReplaceAll(xmlStr, "chkrun:", "") that additionally created a default namespace declaration, potentially confusing the parser further.

Fix (commit dedfebc): Added stripXMLNamespaces() helper in pkg/adt/devtools.go using two compiled regexes:

var (
    reXMLNSDecl   = regexp.MustCompile(`\s*xmlns(?::\w+)?="[^"]*"`)
    reXMLNSPrefix = regexp.MustCompile(`(</?|\s)(\w[\w-]*):([\w])`)
)

func stripXMLNamespaces(data []byte) []byte {
    s := reXMLNSDecl.ReplaceAllString(string(data), "")
    s = reXMLNSPrefix.ReplaceAllString(s, "${1}${3}")
    return []byte(s)
}

Applied to both parseActivationResult and parseSyntaxCheckResults. 6 unit tests added covering: namespace stripping preserves URL values in attributes, activation response with namespaced error message correctly detects failure, syntax check response with namespaced errors correctly parses error details.

Verified on-prem: syntax check now correctly reports ABAP errors on includes, and activate no longer returns Success=true when SAP sends error-level messages.

[txape10]

Follow-up: the namespace fix alone is not sufficient on S/4HANA on-prem.

After deploying commit dedfebc we found that on our S/4HANA on-prem system activation still silently returned success:true even with syntax errors. Running a direct HTTP trace (VSP_HTTP_TRACE=1) against the activation endpoint revealed the actual cause:

S/4HANA returns a completely different XML root element, not covered by the namespace fix:

HTTP 200
<chkl:messages xmlns:chkl="http://www.sap.com/abapxml/checklist">
  <chkl:properties checkExecuted="true" activationExecuted="false" generationExecuted="false"/>
  <msg objDescr="" type="W" line="0" href="">
    <shortText><txt>Activation was cancelled.</txt></shortText>
  </msg>
  <msg objDescr="Programa ZPROG_WITH_INCL" type="E" line="1"
       href="/sap/bc/adt/programs/programs/zprog_with_incl/source/main#start=2,8" forceSupported="true">
    <shortText><txt>INCLUDE report "ZRCG1_INCL" not found.</txt></shortText>
  </msg>
</chkl:messages>

Key differences from adtcomp:activationLog:

• Root element is <chkl:messages> (maps to http://www.sap.com/abapxml/checklist)
• Failure is signalled by activationExecuted="false" on <chkl:properties>
• <msg> attributes are not namespace-prefixed (type="E", not chkl:type="E") — so the namespace strip from dedfebc had no effect here
• <shortText> contains multiple <txt> children (primary text + localized key); single-string struct tag only captured the first or nothing

The old parser only branched on <activationLog> → found nothing → success: true.

Complete fix (commit b2f0564, our fork txape10/vibing-steampunk):

parseActivationResult now detects which format SAP returned and handles both:

• Format A (<activationLog> — legacy/some SAP versions): namespace strip + existing struct, unchanged
• Format B (<chkl:messages> — S/4HANA on-prem observed): parse <properties activationExecuted="..."/> + direct <msg> children; mark failure if activationExecuted=false or any E/A/X message

Also fixed ShortText struct tag from Text string \xml:"txt"`toTxts []string `xml:"txt"`to capture the first` element correctly when SAP sends multiple.

Two new tests added: TestParseActivationResult_ChklMessages_DetectsFailure (uses exact captured live SAP XML) and TestParseActivationResult_ChklMessages_SuccessWhenActivated.

Verified on-prem today: activating an include with a real syntax error now returns:

{
  "success": false,
  "messages": [
    {"type": "W", "shortText": "Activation was cancelled."},
    {"type": "E", "objDescr": "Include ZREDI_CREATE_IDOC_FILE_CL1", "line": 1107,
     "shortText": "\"IV_TITLE\" is not type-compatible with formal parameter \"I_GRIDTITLE\"."}
  ]
}

[txape10]

Second follow-up: a third root-element shape was still unhandled (found via #148).

While reviewing PR #148 (which independently hits this same issue) I ran its test payloads against our fork's parser. Two passed, but the one using a root-level <ioc:inactiveObjects> response (no <activationLog>, no <chkl:messages> wrapper — just <ioc:inactiveObjects> as the top-level element) failed: our Format A/Format B branching in parseActivationResult only recognized <inactiveObjects> as a child of <activationLog>, so this third shape fell through to Format B, found no <msg>/<properties> at that level, and silently reported success:true, inactive:[].

Root cause was the same class of bug as dedfebc/b2f0564 above: a nested xml:"inactiveObjects" struct field only matches a child element, not the root itself.

Fix (commit a418f7d, our fork): declared the shared entry/ref types directly on the struct that maps onto Format B's root, alongside the existing <msg> children, so it now matches regardless of whether the root is <messages> or <inactiveObjects>. New regression test added using #148's exact payload.

So as of this commit our fork's parseActivationResult handles all three observed root shapes (<activationLog>, <messages>, <inactiveObjects>) plus namespace-prefixed attributes plus the parallel bug in parseSyntaxCheckResults.

Separately: #148 is still open/unmerged and its description doesn't reference Closes #136, so merging it as-is wouldn't auto-close this issue — worth linking explicitly when it lands, since it's tackling the same root cause from a slightly different angle (struct-shape fix without namespace stripping).