feat(adt-server): add bounded ADT server sidecar with REST API#134
Closed
ThePlenkov wants to merge 1044 commits into
Closed
feat(adt-server): add bounded ADT server sidecar with REST API#134ThePlenkov wants to merge 1044 commits into
ThePlenkov wants to merge 1044 commits into
Conversation
…rison Agent-Logs-Url: https://github.com/abapify/adt-cli/sessions/cdc98640-eebb-45fb-a9e6-e211fcbd88d6 Co-authored-by: ThePlenkov <6381507+ThePlenkov@users.noreply.github.com>
… docs + openspec Sample refresh: - samples/petstore3-client/package.json: drop the two v1 generate:* scripts, use a single "generate" targeting --base petstore3 --format abapgit,gcts. - samples/petstore3-client/generated/: fully regenerated against the v2 pipeline (11 abapGit files + 11 gCTS files: types/ops interfaces, exception class, minimal implementation class with locals_def/locals_imp includes). - samples/petstore3-client/e2e/abapgit/src/zcl_ps3_client_tests.clas.abap: update to v2 naming — uses `zif_petstore3_types=>pet` / `=>order`, and instantiates `NEW zcl_petstore3( destination = 'PETSTORE' )` through the `zif_petstore3` interface reference. - samples/petstore3-client/e2e/README.md: rewritten around the v2 output contract, lists the 11 artefacts, generic BTP deploy recipe. Documentation: - packages/abap-ast/README.md + AGENTS.md: document the new optional `abapDoc: readonly string[]` field on TypeDef / MethodDef / AttributeDef / InterfaceDef / ClassDef nodes and the printer's `"! <line>` emission. - packages/openai-codegen/README.md + AGENTS.md: rewrite for v2 — 3-interfaces-plus-one-class architecture, `json=>`/`lcl_http=>` bundle, per-op CASE response->status( ) dispatch, NamesConfig + CLI flag table, type mapping table, round-trip ABAPDoc markers, deploy recipe. - website/docs/sdk/packages/abap-ast.md + openai-codegen.md: matching docusaurus pages. - .agents/skills/openapi-to-abap/SKILL.md: rewrite under the v2 architecture (how to add profiles, mappings, CLI flags; common pitfalls of the monolithic v1 design to avoid). OpenSpec: - proposal.md: full rewrite describing v2 motivation (v1 monolithic rejected), the 11-file bundle contract, and the "typed AST + 3-layer contract" design. - tasks.md: wave-by-wave breakdown matching reality (Wave 0-4). - specs/openai-codegen/spec.md: new requirements (3-layer output, minimal impl class, zero Z-deps, configurable names, ABAPDoc markers, deterministic output, abaplint-clean, <ABAP_LANGUAGE_VERSION>5</...>, transport-only fetch, abapify/json + abapify/fetch API shape). - specs/abap-ast/spec.md: add ABAPDoc comment requirement. Verification: - `bunx nx run-many -t typecheck,test,build,lint -p abap-ast,openai-codegen` all 8 targets green. - `bunx nx build website` succeeds. - grep "Booking|BHF" over all changed files returns empty. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…implementation plan Agent-Logs-Url: https://github.com/abapify/adt-cli/sessions/cdc98640-eebb-45fb-a9e6-e211fcbd88d6 Co-authored-by: ThePlenkov <6381507+ThePlenkov@users.noreply.github.com>
Fixes discovered while activating the generated petstore3 client on a
real SAP BTP Steampunk (s4-cloud) system:
* abap-ast/print-class: emit INTERFACES inside PUBLIC SECTION when the
class body uses explicit sections. ABAP cloud rejects INTERFACES at
class-body level with "There is no PUBLIC/PROTECTED/PRIVATE SECTION
statement" during activation.
* openai-codegen/locals-imp template:
- cl_http_destination_provider=>create_by_destination uses parameter
i_destination (not i_name, not i_destination_name)
- Use if_web_http_request=>name_value_pairs for response header table
(if_web_http_response=>name_value_pairs does not exist on cloud)
- All cl_web_http_utility=>escape_url / set_binary / describe_by_data
kernel whitelist adjustments are preserved
* petstore3 sample regenerated under samples/petstore3-client/generated
(flattened output — no more abapgit/ subdir).
Tests: openai-codegen 142/142 passing; abap-ast 115/115 passing.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Two Steampunk activation blockers, both driven by real errors observed
when activating the generated petstore3 client:
* **Array query params**: OpenAPI operations like `GET /pet/findByTags`
declare `tags: array<string>` as a query parameter. The emitter now
detects table-typed params (`pluckType(schema) === 'array'`) and emits
a `FOR _<name> IN <name> ( name = '...' value = _<name> )` row
comprehension inside the `VALUE #( ... )` table, producing one
`name/value` pair per array element. Non-array params keep the simple
inline entry. Fixes activation error `MESSAGE(GXD): The type of "TAGS"
cannot be converted to the type of "VALUE".`
* **HTTP exceptions**: the method bodies now wrap the fetch + CASE block
in a TRY/CATCH that translates `cx_web_http_client_error` and
`cx_http_dest_provider_error` into the generated `ZCX_*_ERROR`. The
public method signature still RAISES only the generated exception, so
Steampunk no longer flags `MESSAGE(G-Q): The exception … is not caught
or declared in the RAISING clause`.
Validated on a real SAP BTP Steampunk system (TRL, us10):
- activation: preauditRequested=false returns
`checkExecuted="false" activationExecuted="false"
generationExecuted="true"` with no errors or warnings
- active source on SAP contains both fixes (TRY blocks + `i_destination =`)
- `adt aunit -c ZCL_PS3_CLIENT_TESTS` → 3/3 tests pass:
TEST_INSTANTIATION pass
TEST_TYPED_PET pass
TEST_TYPED_ORDER pass
Added test `emitImplementationClass (Petstore v3) > snapshots find_pets_by_tags`
to lock the new FOR-loop rendering. All 142 openai-codegen tests pass.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Responding to CodeQL + SonarCloud + Devin/CodeRabbit/Copilot review
findings on feat/openai-codegen.
### Security hotspots / CodeQL
* `packages/openai-codegen/src/types/naming.ts`
- Rewrite camelCase-splitter regex with lookahead-only patterns (no
overlapping quantifiers). Kills CodeQL `js/polynomial-redos`.
Verified same semantics on HTTPServer / camelCase / APIKey / etc.
- Switch `shortHash()` from SHA-1 to SHA-256. The hash is only used
as a collision tag inside identifiers, but using a weak algorithm
trips Sonar `typescript:S4790`.
* `packages/openai-codegen/src/oas/load.ts` — replace
`synthesizeOperationId`'s chained regex pipeline with an explicit
character-by-character loop. Kills Sonar `typescript:S5852` regex
super-linear runtime warnings.
### SonarCloud BUGs (→ reliability rating)
* `typescript:S2871` — every `Array#sort()` without a compare function
across emit + format + tests now uses
`(a, b) => a.localeCompare(b)`.
* `typescript:S7739` — rename `If.then` / `ifStmt({ then })` to
`If.thenBody` / `ifStmt({ thenBody })`. `then` on a plain object makes
JS engines treat the node as a thenable/Promise.
### Review comments
* **Cloud profile ↔ locals_imp mismatch** (kilo-code-bot) — the s4-cloud
profile description claimed "no /ui2/cl_json" and excluded it from the
whitelist, yet the generated locals_imp template uses `/ui2/cl_json`.
Updated the profile to reflect reality: `/ui2/cl_json` is released for
ABAP for Cloud Development per SAP Note 2931335 and IS in the allow-
list. JSON strategy is now `ui2_cl_json` (matching what we actually
emit and verified on live BTP Steampunk).
* **gCTS `abapLanguageVersion` missing** (Devin review) — added
`"abapLanguageVersion": "5"` to `buildClasJson()`. Without it the
gCTS layout defaults to Standard ABAP and the class fails to activate
on BTP Steampunk with S_ABPLNGVS.
* **`Return` with value emits invalid ABAP `rv = ...`** (Copilot) —
added required `target` field on `Return` AST node. ABAP has no
implicit `rv` identifier; callers must supply the RETURNING parameter
name explicitly. Factory throws if `value` is set without `target`.
* **Nested BinOp lost parens** (CodeRabbit) — `printExpression` now
wraps nested BinOp operands in `( ... )` so `a AND (b OR c)` no
longer flattens to `a AND b OR c`.
* **Sample README `$TMP` misleading** (Copilot) — switched example to
`--package ZPEPL` and documented why `$TMP` typically fails on ABAP
Cloud (S_ABPLNGVS / 403). Also refreshed the output-layout and test
commands.
### SonarCloud code smells addressed
* `typescript:S3358` — removed nested ternary in
`print-types.ts` TableType printer, replaced with a lookup table.
### Housekeeping
* Regenerated petstore3 sample with the updated cloud profile + gcts
metadata. abapGit output now under `generated/abapgit/src/`, gCTS
under `generated/gcts/CLAS|INTF/`.
Tests: openai-codegen 142/142, abap-ast 115/115 passing.
Live verification on BTP (TRL) still holds — the previously landed
active source matches what this PR emits for the cloud profile.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… class, default-is-success ordering Four additional PR #109 review fixes (CodeRabbit + Devin): * **`lcl_http.fetch` now prefixes the configured `server` path.** Specs with a non-root server base (`/v1`, `/api/v3`, …) were silently losing the prefix. Emits `|{ server }{ path }|` when `server` is non-empty. * **Wrap the HTTP round-trip in `TRY … CLEANUP … ENDTRY`** so the client is always `close()`d — previously a kernel exception during `execute()`, binary read, or codepage conversion could leak the connection. * **`renderFetchCall` now uses the configured local JSON class** (pass `ctx.jsonLocal` through) instead of hard-coding `json=>stringify`. Response parsing already used `ctx.jsonLocal`; request serialization is now symmetric and custom `--local-json-class` names work end-to-end. * **`mapResponseHandling` — fix WHEN-ordering when `default` is the success response.** Two related issues: 1. `pickSuccessStatus` no longer treats a `default` response as success when it's marked `isError: true`. 2. When `default` IS the success branch (`WHEN OTHERS.`), error WHENs are now emitted *before* it so that `OTHERS` remains the final branch (ABAP forbids any WHEN after OTHERS; previously errors after OTHERS were unreachable). Tests: openai-codegen 142/142 passing, abap-ast 115/115 passing. Live TRL verification: deployed to ZPEPL, activation pre-audit clean, `adt aunit -c ZCL_PS3_CLIENT_TESTS` → 3/3 pass. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
* `emit-local-classes.test.ts` — broaden the Z-prefix guard to also
catch solid identifiers (`ZFOO`, `ZAPI`, `ZCLIENT`) that the previous
underscore-only regex missed. Strip banner/star/line-comment rows
before matching so the doc header ("* ZERO Z-dependencies…") remains
allowed while the body is strictly zero-Z.
* `samples/petstore3-client/e2e/abapgit/src/zcl_ps3_client_tests.clas.xml`
— add `<WITH_UNIT_TESTS>X</WITH_UNIT_TESTS>` to VSEOCLASS so the
abapGit metadata matches the class body and ABAP Unit's discovery
path does not rely purely on ABAP-level `FOR TESTING` semantics.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Addresses additional CodeRabbit + Devin review comments: * **gCTS interface envelope now carries `abapLanguageVersion: '5'`** (Devin, 🔴 critical). Parallel to the class envelope fix; required for BTP Steampunk gCTS imports so interfaces don't default back to Standard ABAP. * **`call()` node factory validates `callKind`** (CodeRabbit, 🟠 major). Previously accepted `undefined` at runtime, which fell into the instance-call path on print. Now throws `AbapAstError` unless `callKind` is `'static'` or `'instance'`. * **gCTS docblock lists `locals_imp.abap`** (CodeRabbit, 🟡 minor). The emitter has written this file for a while; docblock was stale. * **Sample README header grammar** (CodeRabbit, 🟡 minor). "requires an authenticated `adt-cli` session". * Regenerated petstore3 sample to pick up the interface `abapLanguageVersion: '5'` field. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
feat: OpenAPI → ABAP client codegen v2 (3-layer, fetch-style, zero Z-deps)
… + multi-system routing Adds an HTTP transport alongside the existing stdio transport for @abapify/adt-mcp, backed by stateful MCP sessions that own a cached AdtClient plus a lock registry. Credentials are supplied once per MCP session via a new `sap_connect` handshake tool (resolvable by explicit args, multi-system config, or existing adt-cli auth store), rather than on every tool call. This unlocks running the MCP server as a long-lived service, drops the per-call SAP CSRF/security-session handshake, and prepares the surface for Wave 2 (MCP auth) and Wave 4 (Okta / OAuth). Context: PR #110 feature-parity analysis vs AWS ABAP Accelerator. W0 — OpenSpec change proposal (docs only): openspec/changes/add-mcp-http-transport/{proposal,design,tasks}.md openspec/changes/add-mcp-http-transport/specs/adt-mcp/spec.md W1-A — transport + registry + multi-system: new src/bin/adt-mcp-http.ts (second bin, node:http + SDK StreamableHTTPServerTransport) new src/lib/http/{server,multi-system}.ts new src/lib/session/{registry,types}.ts edit src/lib/server.ts (ToolContext gains optional registry + resolveSystem) edit src/lib/types.ts, src/index.ts, tsdown.config.ts, package.json W1-B — handshake tools + tests: new src/lib/tools/sap-connect.ts (priority: explicit baseUrl → multi-system → ~/.adt auth store) new src/lib/tools/sap-disconnect.ts new src/lib/tools/session-helpers.ts (resolveClient helper for future tool migration) edit src/lib/tools/{index,shared-schemas}.ts edit packages/adt-cli/src/index.ts (re-export getAdtClientV2) new packages/adt-mcp/tests/http-integration.test.ts (6 tests, all pass) Verified: - bunx nx build adt-mcp adt-cli → OK - 84 existing stdio + 6 new HTTP integration tests all pass - Live smoke against real BTP Steampunk (TRL) — initialize → sap_connect via ~/.adt browser-SSO cookie → SAP discovery 200 → sap_disconnect → DELETE /mcp all succeeded. Known follow-ups (see tasks.md): - W1-C: migrate the 90+ existing tools to session-aware args so they can run with just an active MCP session (today they still require per-call baseUrl); wrap getAdtClientV2 to throw instead of process.exit(1) on auth failures. - W2: MCP auth layer (MCP_AUTH_TOKEN bearer + reverse-proxy mode). - W3: transactional changesets (ChangesetService in adt-cli for parity). - W4: Okta / OAuth 2.1 bearer. - W5: Dockerfile.mcp + deployment docs. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…sessions + multi-system routing" This reverts commit 19157a5.
…+ CORS)
Adds configurable authentication and CORS to the Streamable HTTP
transport introduced in Wave 1:
- new 'src/lib/http/auth.ts' — createAuthMiddleware() supports three
modes: 'none' (default), 'bearer' (constant-time token compare via
crypto.timingSafeEqual), and 'proxy' (trust X-Forwarded-User /
-Email / -Groups from an authenticating reverse proxy, e.g.
oauth2-proxy). Bearer mode returns 401 {"error":"unauthorized"}
on mismatch with no detail leak.
- new 'src/lib/http/cors.ts' — exact-match origin allow-list (plus '*'
shortcut for dev), preflight short-circuit, response headers for
regular requests, Access-Control-Expose-Headers: mcp-session-id so
browser clients can read the session id off initialize.
- 'src/lib/http/server.ts' — wires CORS + auth into the pipeline
(CORS → host validation → /healthz escape-hatch → auth → route).
/healthz stays outside auth for monitoring probes. Adds authMode,
authToken, trustForwardedAuth, allowedOrigins to
HttpServerOptions. Preserves prior behaviour when unconfigured.
- 'src/bin/adt-mcp-http.ts' — new flags (--auth-token,
--auth-mode, --trust-forwarded-auth, --cors-origin) and env vars
(MCP_AUTH_TOKEN, TRUST_FORWARDED_AUTH, MCP_CORS_ORIGIN). CLI flag
wins over env. Emits a stderr WARN banner at startup when auth is
off on a non-loopback bind, and when proxy trust is active on a
non-loopback bind.
- new 'tests/http-auth.test.ts' — 13 node:test integration tests
covering no-auth passthrough, bearer missing/wrong/equal-length-
wrong/correct, proxy missing/present, misconfig rejection, CORS
preflight for allowed and disallowed origins, and response-header
propagation.
Smoke-verified: MCP_AUTH_TOKEN=secret123 start → no-token=401,
bad-token=401, good-token=200.
Refs: openspec/changes/add-mcp-http-transport/{proposal,design,specs}
…tClientV2Safe
Goal 1: getAdtClientV2Safe
- Add AdtAuthError (typed error with code + systemId) to adt-cli
- Factor error-exit paths in getAdtClientV2 into small helpers
(reportNoSessionAndExit, reportUnsupportedAuthAndExit)
- Add getAdtClientV2Safe: same contract as getAdtClientV2 but throws
AdtAuthError instead of calling process.exit(1). Required by
long-running processes (adt-mcp HTTP transport, daemons) where
process.exit would tear down unrelated connections.
- Re-export getAdtClientV2Safe + AdtAuthError from @abapify/adt-cli
- sap_connect now uses getAdtClientV2Safe; AdtAuthError code surfaces
in the MCP error response
- Unit test verifies getAdtClientV2Safe throws (does not exit) when
the SID has no session on disk
Goal 2: session-aware tools
- Migrated 82 tool files (every MCP tool except sap-connect, sap-disconnect,
object-creation helpers, and non-handler utility modules)
- Input schemas now use sessionOrConnectionShape instead of connectionShape,
so baseUrl/client/username/password/systemId are all optional
- Handlers take (args, extra) and resolve the client via
resolveClient(ctx, args, extra ?? {}) — which prefers the session-scoped
client from the registry when the MCP session has already run sap_connect,
and otherwise falls back to the legacy ephemeral-client path
- update-source and run-abap: client resolution wrapped in try/catch so
resolveClient throws do not escape the tool handler
- call-rfc: manual migration (variable was renamed to adtClient)
No functional changes to tool behaviour beyond the optional connection args
and session reuse. All parity tests pass.
Verification:
- bunx nx build adt-cli adt-mcp — green
- bunx nx test adt-cli — 194 tests pass (includes all parity.* tests)
- adt-mcp integration + http-integration + http-auth — 103 tests pass
- bunx nx lint adt-mcp — 2 pre-existing errors unrelated to this wave
(adt-mcp-http.ts no-fallthrough, multi-system.ts preserve-caught-error)
- bunx nx format:write — applied
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…/ Cognito) - New authMode='oauth' in HTTP auth middleware - JWT validation via jose: issuer, audience, expiry, required scopes, JWKS fetch - OIDC discovery fallback (.well-known/openid-configuration) - User identity + email + groups extracted from configurable claims - RFC 6750 compliant 401 responses - CLI flags: --oauth-issuer/--oauth-audience/--oauth-jwks-uri/--oauth-required-scope Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…arity)
- Changeset state machine (open/committing/committed/rolled_back) bound to MCP session
- ChangesetService in adt-cli: begin/add/commit/rollback — transport-agnostic
- 4 new MCP tools: changeset_begin/add/commit/rollback — session-aware
- 4 matching CLI commands: adt changeset {begin,add,commit,rollback} with file-based state under ~/.adt/changesets/
- Parity test proving CLI ↔ MCP behavioural equivalence
- Auto-rollback of open changesets on MCP session close
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…docs, AGENTS/README updates - Dockerfile.mcp (multi-stage, non-root, healthcheck) - docker-compose.mcp.yaml with oauth2-proxy skeleton - docs/deployment/mcp-http.md: full deployment guide (quickstart, auth modes, Okta recipe, multi-system, changesets, troubleshooting) - packages/adt-mcp/README.md: HTTP transport + changeset sections - packages/adt-mcp/AGENTS.md: transports/sessions/auth/changesets invariants - root AGENTS.md: package guide pointer Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…y-research-sap-abap-accelerator # Conflicts: # packages/adt-cli/src/index.ts # packages/adt-mcp/src/bin/adt-mcp-http.ts # packages/adt-mcp/src/lib/http/server.ts # packages/adt-mcp/src/lib/session/types.ts # packages/adt-mcp/src/lib/tools/index.ts # packages/adt-mcp/src/lib/tools/sap-connect.ts
Fixes 2 pre-existing ESLint errors blocking CI on PR #110: - src/bin/adt-mcp-http.ts: move no-fallthrough disable before default: - src/lib/http/multi-system.ts: pass { cause: err } to preserve caught error Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Replace /^Bearer\s+(.+)$/ regex with plain-string + charCode check to eliminate polynomial backtracking risk on uncontrolled Authorization header input flagged by CodeQL on PR #110. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…-code CORS branch + Dockerfile hardening - Extract changeset-helpers.ts (requireSession/requireOpenChangeset/textError/textOk) used by all 4 changeset_* tools; drops ~120 lines of duplicated precondition + error-wrap boilerplate. - cors.ts: remove 'allowed[0] === "*" ? origin : origin' dead-code branch (SonarCloud typescript:S3923 bug). - Dockerfile.mcp: --chmod=0555/0444 on COPY to satisfy CWE-732 / docker:S6504. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
SonarCloud quality gate still flags COPY --chmod=0555/0444 as missing defence against write permissions. Add a follow-up 'chmod -R a-w' RUN step that makes the read-only intent unambiguous to the scanner. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…liant) Sonar's compliant example: COPY --chown=root:root --chmod=755 … Runtime user (appuser) falls into 'others' mode bits — r-x, no write. Files remain readable + executable without granting appuser ownership. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1. (security) TRUST_FORWARDED_AUTH and --trust-forwarded-auth may no longer silently downgrade an explicit --auth-mode=oauth|bearer to 'proxy'. Stale env vars in the shell would otherwise bypass JWT validation entirely. Gate the override on authMode === 'none' in both bin/adt-mcp-http.ts and lib/http/server.ts. 2. (stability) changeset_rollback catch block now clears session.changeset and tracked locks on rollback failure — previously the session was left wedged in 'open' state and subsequent changeset_begin calls required force=true to recover. Review: https://app.devin.ai/review/abapify/adt-cli/pull/110 Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- Dockerfile.mcp CMD: drop hardcoded --port, let MCP_PORT env flow
through so containers run on whatever port the user sets. Fixes
perpetually-unhealthy container when -e MCP_PORT=… overridden.
- http/server.ts: return 403 (not 421) on host-header reject, aligned
with openspec/changes/add-mcp-http-transport/specs/adt-mcp/spec.md.
- http/server.ts: touch() the session on every POST that carries an
existing Mcp-Session-Id. Previously only GET/DELETE refreshed the
TTL, so actively-used sessions could be swept as idle between tool
calls.
- sap-connect: reject ambiguous baseUrl+systemId input instead of
silently preferring baseUrl — prevents accidental wrong-target
connections.
- multi-system registry: forbid credentials on disk (drop with
stderr warning on load). Credentials must flow through sap_connect
at runtime, never persist to the config file. Registry schema now
baseUrl + client only.
- sap-connect priority B: merge tool-call credentials with the
registry-resolved metadata so the multi-system flow still works
with the narrowed schema.
- OpenSpec: rename sap_{begin,add_to,commit,rollback}_changeset →
changeset_{begin,add,commit,rollback} in proposal/design/tasks/spec
to match the implemented tool names.
- README: rewrite the changeset tool table to match real semantics
(lock+PUT on add, batch-activate on commit, no source revert on
rollback).
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…vin finding) Mirror the sap-connect fix in session-helpers.ts: when resolveClient takes path 3 (systemId + resolveSystem) without an active session, it now merges args.username/args.password into the resolved params. Previously it built the client from the bare registry entry (baseUrl + client), silently dropping any credentials the caller supplied on the tool call. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…match code (Devin findings) - changeset_begin force=true: on rollback failure, return an explicit error instead of silently overwriting session.changeset. Previously a throwing rollback caused the old changeset object to be orphaned, leaking its lock handles until the SAP security session expired. Option 1 from the reviewer's suggestion (safer than lock-by-lock release because the service already does that best-effort inside rollback). - docs/deployment/mcp-http.md multi-system section: drop username / password from the schema example + fields table and state that credentials must come via sap_connect at runtime — matches the 'secrets are not persisted' security policy enforced in multi-system.ts:normalise. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…Client (Devin findings) - sap_connect: when the session is already bound to systemId=X and the caller requests systemId=Y (Y != X), return isError instead of ok:true bound to X. Prevents a PROD-requesting agent from silently operating on a DEV session. - session-helpers.resolveClient: return type Promise<ResolvedClient> with async keyword, matching the fact that every caller already await's it. Keeps the door open for future async resolution paths (OIDC token exchange, etc) without silently returning an unhandled Promise. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Add deterministic OpenAPI client generation, bounded rooted package trees, and sidecar capability redemption for the ARM transport. Refs: FSINN-2266
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Skipping CodeAnt AI review — this PR changes more than 100 files, which usually means a migration, codemod, or vendored drop. Line-level review on diffs this large produces duplicate findings on the same rewrite pattern and drowns out anything that actually matters. If you still want a review, comment |
✅ Deploy Preview for adt-cli ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Important Review skippedToo many files! This PR contains 2115 files, which is 2015 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (196)
📒 Files selected for processing (2115)
You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests (beta)
|
MergerPending PR has merge conflicts. Commit |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review Summary
This PR is extraordinarily large (1491 files, 398,768 additions, 1 deletion) representing what appears to be a complete new system implementation rather than an incremental change. Given the massive scope, a comprehensive review is not feasible. However, based on examination of security-critical authentication and server files, the implementation appears to follow sound security practices.
Critical Observations
Positive Security Patterns:
- Timing-safe token comparison using
crypto.timingSafeEqualprevents timing attacks - Proper separation of authentication modes with fail-secure defaults
- JWT verification with strict validation including issuer, audience, and algorithm checks
- Token lifetime limits (5 minutes max) prevent long-term exposure
- Private keys never mounted in runtime - only public verification keys
- Proper privilege dropping in Docker (runs as unprivileged user
appuser) - Secrets handled securely via BuildKit secrets and proper file permissions
- Fail-closed approach where validation failures return undefined without exposing details
Process Concerns:
- PR Scope: This PR is far too large for meaningful review. Consider breaking massive features into smaller, reviewable increments
- Missing Context: No migration strategy, rollback plan, or deployment documentation visible
- Test Coverage: With 398K+ additions, comprehensive test verification is critical but not visible in this review
Recommendations
- Split Future PRs: Break large features into reviewable chunks (ideally < 500 lines per PR)
- Security Testing: Ensure penetration testing covers all authentication modes
- Deployment Strategy: Document deployment sequence, monitoring, and rollback procedures
- Audit Logging: Verify authentication failures are logged appropriately for security monitoring
The authentication implementation itself appears solid, but the PR's massive scope makes comprehensive review impractical.
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.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| // This will fail at runtime (no real server), but MUST compile | ||
| try { | ||
| const sysInfo = | ||
| await client.core.http.systeminformation.getSystemInformation(); |
| v == null || | ||
| v === '' || | ||
| (Array.isArray(v) && v.length === 0) || | ||
| (typeof v === 'object' && v !== null && Object.keys(v).length === 0); |
| function toCanonicalAtcFindings(response: unknown): CanonicalAtcFinding[] { | ||
| const worklist = record(response)?.worklist; | ||
| const objects = records( | ||
| record(worklist)?.objects && record(worklist)?.objects, |
| ); | ||
| } | ||
|
|
||
| if (!current && !loading) { |
|
|
||
| // These property accesses should be type-safe | ||
| const _systemId: string | undefined = sysInfo.systemID; | ||
| const client: string | undefined = sysInfo.client; |
| // This will fail at runtime (no real server), but MUST compile | ||
| try { | ||
| const sysInfo = | ||
| await client.core.http.systeminformation.getSystemInformation(); |
| rootSchema: SchemaLike, | ||
| prefix: string | undefined, | ||
| ): void { | ||
| if (value === undefined || value === null) return; |
| prefix: string | undefined, | ||
| elementForm: string | undefined, | ||
| ): void { | ||
| if (value === undefined || value === null) return; |
| const capitalize = (s: string) => s.charAt(0).toUpperCase() + s.slice(1); | ||
|
|
||
| // Main index file | ||
| const mainIndex = [ |
| console.log('User deleted'); | ||
|
|
||
| // Using composedClient (same API, different structure) | ||
| const composedUsers = await composedClient.users.list(); |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 2 high |
| Security | 82 critical 10 high |
| Performance | 1 medium |
🟢 Metrics 19280 complexity · 1998 duplication
Metric Results Complexity 19280 Duplication 1998
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.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Comment on lines
+1173
to
+1187
| async searchObjects(destination, criteria: ObjectSearchCriteria = {}) { | ||
| return await withClient(destination, 'search_objects', async (client) => { | ||
| const cap = criteria.maxResults ?? 5_000; | ||
| const response = | ||
| await client.adt.repository.informationsystem.search.quickSearch({ | ||
| query: adtPrefixQuery(criteria.query), | ||
| maxResults: cap + 1, | ||
| ...(criteria.packageName | ||
| ? { packageName: criteria.packageName.toUpperCase() } | ||
| : {}), | ||
| ...(adtSearchObjectType(criteria.objectType) | ||
| ? { objectType: adtSearchObjectType(criteria.objectType) } | ||
| : {}), | ||
| }); | ||
| const objects = toCanonicalRepositoryObjects(response); |
There was a problem hiding this comment.
💡 Edge Case: Pagination overflow check off-by-one reports false truncation
searchObjects and listPackageObjects request maxResults: cap + 1 specifically to detect whether more results exist, but then compute truncated: quickSearchReferences(response).length >= cap. When exactly cap results exist, the backend returns cap references (not cap + 1), so >= cap evaluates to true and the response is falsely marked truncated — clients will chase a nextCursor/next page that returns nothing. Compare getPackageTree (line 1164) which correctly uses > MAX_PACKAGE_SEARCH_RESULTS. Change the comparison to > cap to match the cap + 1 request semantics.
Use strict greater-than so truncation is only reported when the extra (cap+1)th result was actually returned. Apply the same change in listPackageObjects.:
truncated: quickSearchReferences(response).length > cap,
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsImplements a bounded ADT server sidecar with typed REST clients and scoped access controls. Correct the off-by-one pagination logic in searchObjects and listPackageObjects to ensure truncation flags are reported accurately. 💡 Edge Case: Pagination overflow check off-by-one reports false truncation📄 packages/adt-server/src/broker.ts:1173-1187 📄 packages/adt-server/src/broker.ts:1296-1310 📄 packages/adt-server/src/broker.ts:1150-1164
Use strict greater-than so truncation is only reported when the extra (cap+1)th result was actually returned. Apply the same change in listPackageObjects.🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Important Your trial ends in 3 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request consolidates rules, skills, and workflows into a unified .agents/ directory as the single source of truth, deprecating older .windsurf/ and .cursor/ paths. It introduces a docs-sync utility to prevent documentation drift, a monitor-ci tool for self-healing CI pipelines, and a Docker configuration for the ADT server. The review feedback highlights several improvement opportunities: replacing fragile sed-based JSON parsing in shell hooks with robust node execution, wrapping readdirSync calls in try-catch blocks to prevent ENOENT crashes on missing directories, handling missing trailing commas when dynamically patching sidebars.ts, and replacing the hardcoded package list in Dockerfile.adt-server with an automated nx build command.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+12
to
+17
| prompt=$(echo "$stdin_data" | sed -n 's/.*"prompt":"\([^"]*\)".*/\1/p' | head -1) | ||
|
|
||
| if [[ -z "$prompt" ]]; then | ||
| exit 0 | ||
| fi | ||
|
|
There was a problem hiding this comment.
Using sed to parse JSON is highly fragile and insecure. If the prompt contains escaped quotes (\") or newlines, the sed pattern will truncate the prompt at the first quote, potentially bypassing the secret scanner and leaking sensitive information. Since Node.js is guaranteed to be installed in this environment, use node to parse the JSON robustly and safely.
Suggested change
| prompt=$(echo "$stdin_data" | sed -n 's/.*"prompt":"\([^"]*\)".*/\1/p' | head -1) | |
| if [[ -z "$prompt" ]]; then | |
| exit 0 | |
| fi | |
| # Extract prompt field using node (no external dependency) | |
| prompt=$(echo "$stdin_data" | node -e ' | |
| const fs = require("fs"); | |
| try { | |
| const input = JSON.parse(fs.readFileSync(0, "utf-8")); | |
| console.log(input.prompt || ""); | |
| } catch (e) {} | |
| ') | |
| if [[ -z "$prompt" ]]; then | |
| exit 0 | |
| fi |
Comment on lines
+12
to
+21
| # Extract tool_name and file_path using sed (no jq dependency) | ||
| tool_name=$(echo "$stdin_data" | sed -n 's/.*"tool_name":"\([^"]*\)".*/\1/p' | head -1) | ||
|
|
||
| if [[ "$tool_name" != "Read" ]]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Extract file_path from tool_input | ||
| file_path=$(echo "$stdin_data" | sed -n 's/.*"tool_input":\s*{\([^}]*\)}.*/\1/p' | \ | ||
| sed -n 's/.*"file_path":"\([^"]*\)".*/\1/p' | head -1) |
There was a problem hiding this comment.
Parsing JSON with sed is fragile and insecure. If the JSON contains newlines, spaces, or escaped characters, the extraction of tool_name or file_path can fail or be bypassed. Since Node.js is guaranteed to be available in this workspace, use node to parse the JSON robustly and safely.
Suggested change
| # Extract tool_name and file_path using sed (no jq dependency) | |
| tool_name=$(echo "$stdin_data" | sed -n 's/.*"tool_name":"\([^"]*\)".*/\1/p' | head -1) | |
| if [[ "$tool_name" != "Read" ]]; then | |
| exit 0 | |
| fi | |
| # Extract file_path from tool_input | |
| file_path=$(echo "$stdin_data" | sed -n 's/.*"tool_input":\s*{\([^}]*\)}.*/\1/p' | \ | |
| sed -n 's/.*"file_path":"\([^"]*\)".*/\1/p' | head -1) | |
| # Extract tool_name and file_path using node (no external dependency) | |
| read -r tool_name file_path <<EOF | |
| $(echo "$stdin_data" | node -e ' | |
| const fs = require("fs"); | |
| try { | |
| const input = JSON.parse(fs.readFileSync(0, "utf-8")); | |
| if (input.tool_name === "Read" && input.tool_input?.file_path) { | |
| console.log(`Read ${input.tool_input.file_path}`); | |
| } | |
| } catch (e) {} | |
| ') | |
| EOF | |
| if [[ "$tool_name" != "Read" ]]; then | |
| exit 0 | |
| fi |
Comment on lines
+63
to
+68
| const docPkgsDir = join(docsDir, 'sdk/packages'); | ||
| const docPkgs = new Set( | ||
| readdirSync(docPkgsDir) | ||
| .filter((f) => f.endsWith('.md') && f !== 'overview.md') | ||
| .map((f) => f.replace(/\.md$/, '')), | ||
| ); |
There was a problem hiding this comment.
Calling readdirSync directly on docPkgsDir without checking if the directory exists will cause the script to crash with ENOENT if the directory is missing. Use a try-catch block to safely handle missing directories without introducing external dependencies or modifying imports outside the diff hunk.
const docPkgsDir = join(docsDir, 'sdk/packages');
let docPkgsList: string[] = [];
try { docPkgsList = readdirSync(docPkgsDir); } catch {}
const docPkgs = new Set(
docPkgsList
.filter((f) => f.endsWith('.md') && f !== 'overview.md')
.map((f) => f.replace(/\.md$/, ''))
);
Comment on lines
+113
to
+118
| const docToolsDir = join(docsDir, 'mcp/tools'); | ||
| const docTools = new Set( | ||
| readdirSync(docToolsDir) | ||
| .filter((f) => f.endsWith('.md')) | ||
| .map((f) => f.replace(/\.md$/, '')), | ||
| ); |
There was a problem hiding this comment.
Calling readdirSync directly on docToolsDir without checking if the directory exists will cause the script to crash with ENOENT if the directory is missing. Use a try-catch block to safely handle missing directories.
const docToolsDir = join(docsDir, 'mcp/tools');
let docToolsList: string[] = [];
try { docToolsList = readdirSync(docToolsDir); } catch {}
const docTools = new Set(
docToolsList
.filter((f) => f.endsWith('.md'))
.map((f) => f.replace(/\.md$/, ''))
);
Comment on lines
+501
to
+504
| // Inject added entries before the closing ']' | ||
| const indent = ' '; | ||
| const addedLines = added.map((id) => `${indent}'${id}',`).join('\n') + '\n'; | ||
| const modifiedSlice = slice.slice(0, -1) + addedLines + ']'; |
There was a problem hiding this comment.
If the target array in sidebars.ts does not have a trailing comma (e.g., ['sdk/packages/adk']), appending new entries directly will result in a syntax error (e.g., ['sdk/packages/adk' 'sdk/packages/new-package',]). Check if the last non-whitespace character in the slice is a comma or bracket, and prepend a comma if necessary.
Suggested change
| // Inject added entries before the closing ']' | |
| const indent = ' '; | |
| const addedLines = added.map((id) => `${indent}'${id}',`).join('\n') + '\n'; | |
| const modifiedSlice = slice.slice(0, -1) + addedLines + ']'; | |
| // Inject added entries before the closing ']' | |
| const indent = ' '; | |
| const lastChar = slice.slice(0, -1).trim().slice(-1); | |
| const prefix = (lastChar && lastChar !== ',' && lastChar !== '[') ? ',\n' : ''; | |
| const addedLines = prefix + added.map((id) => `${indent}'${id}',`).join('\n') + '\n'; | |
| const modifiedSlice = slice.slice(0, -1) + addedLines + ']'; |
Comment on lines
+27
to
+33
| RUN for package in \ | ||
| ts-xsd speci acds adt-config adt-lint adt-rfc \ | ||
| adt-schemas adt-contracts adt-client adt-locks adk adt-plugin adt-atc \ | ||
| adt-plugin-abapgit adt-plugin-gcts adt-tui adt-proxy \ | ||
| adt-plugin-gcts-cli adt-auth adt-aunit adt-cli adt-mcp adt-server; do \ | ||
| bun x tsdown --config "packages/$package/tsdown.config.ts"; \ | ||
| done |
There was a problem hiding this comment.
Using a hardcoded list of packages in a shell loop is fragile and hard to maintain. If a new package is added to the monorepo, this Dockerfile will need to be manually updated. Since Nx is installed and configured in the workspace, use npx nx run-many -t build to build all packages in the correct dependency order automatically.
RUN npx nx run-many -t build --parallel=2 --exclude=adt-cli-docs
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
There was a problem hiding this comment.
(2 files improve in Code Health)
Gates Failed
(2 hotspots with Complex Method, Lines of Code in a Single File, Number of Functions in a Single Module)
(17 new files with code health below 9.00)
(4 files with Bumpy Road Ahead)
(44 files with Complex Method, Complex Conditional, Overall Code Complexity, Large Method, Primitive Obsession, String Heavy Function Arguments, Code Duplication, Overall Function Size, Lines of Code in a Single File, Number of Functions in a Single Module)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| integration.test.ts | 2 rules in this hotspot | 8.55 → 7.79 | Suppress |
| routes.ts | 1 rule in this hotspot | 2.21 → 2.20 | Suppress |
| New code is healthy | Violations | Code Health Impact | |
|---|---|---|---|
| invocation.ts | 5 rules | 3.69 | Suppress |
| broker.ts | 4 rules | 4.43 | Suppress |
| server.ts | 4 rules | 4.95 | Suppress |
| broker.test.ts | 4 rules | 5.61 | Suppress |
| server.test.ts | 3 rules | 6.20 | Suppress |
| source-history.ts | 3 rules | 7.29 | Suppress |
| http-destination-scope.test.ts | 3 rules | 7.59 | Suppress |
| transport-source-manifest.ts | 5 rules | 7.67 | Suppress |
| source-capabilities.ts | 2 rules | 7.91 | Suppress |
| atc-documentation-capabilities.ts | 2 rules | 7.91 | Suppress |
| generate.ts | 3 rules | 7.95 | Suppress |
| destination-mode.ts | 4 rules | 8.00 | Suppress |
| openapi.test.ts | 3 rules | 8.17 | Suppress |
| http-invocation-auth.test.ts | 1 rule | 8.33 | Suppress |
| scope-catalogue.ts | 2 rules | 8.82 | Suppress |
| get-frozen-source.ts | 2 rules | 8.88 | Suppress |
| page-cursors.ts | 2 rules | 8.96 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| invocation.ts | 1 critical rule | 3.69 | Suppress |
| server.ts | 1 critical rule | 4.95 | Suppress |
| destination-mode.ts | 1 critical rule | 8.00 | Suppress |
| sap-connect.ts | 1 critical rule | 8.53 → 8.15 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| invocation.ts | 4 advisory rules | 3.69 | Suppress |
| broker.ts | 4 advisory rules | 4.43 | Suppress |
| server.ts | 3 advisory rules | 4.95 | Suppress |
| broker.test.ts | 4 advisory rules | 5.61 | Suppress |
| server.test.ts | 3 advisory rules | 6.20 | Suppress |
| server.ts | 3 advisory rules | 9.69 → 6.32 | Suppress |
| source-history.ts | 3 advisory rules | 7.29 | Suppress |
| http-destination-scope.test.ts | 3 advisory rules | 7.59 | Suppress |
| transport-source-manifest.ts | 5 advisory rules | 7.67 | Suppress |
| source-capabilities.ts | 2 advisory rules | 7.91 | Suppress |
| atc-documentation-capabilities.ts | 2 advisory rules | 7.91 | Suppress |
| generate.ts | 3 advisory rules | 7.95 | Suppress |
| destination-mode.ts | 3 advisory rules | 8.00 | Suppress |
| openapi.test.ts | 3 advisory rules | 8.17 | Suppress |
| http-invocation-auth.test.ts | 1 advisory rule | 8.33 | Suppress |
| scope-catalogue.ts | 2 advisory rules | 8.82 | Suppress |
| get-frozen-source.ts | 2 advisory rules | 8.88 | Suppress |
| page-cursors.ts | 2 advisory rules | 8.96 | Suppress |
| atc-run.ts | 2 advisory rules | 10.00 → 9.10 | Suppress |
| transport-source-manifest.test.ts | 1 advisory rule | 9.10 | Suppress |
| sourceversions.ts | 2 advisory rules | 9.10 | Suppress |
| integration.test.ts | 2 advisory rules | 8.55 → 7.79 | Suppress |
| generated.ts | 1 advisory rule | 9.38 | Suppress |
| incl.test.ts | 1 advisory rule | 10.00 → 9.39 | Suppress |
| scope-enforcement.test.ts | 1 advisory rule | 9.39 | Suppress |
| mcp-runtime.ts | 2 advisory rules | 9.39 | Suppress |
| service.ts | 1 advisory rule | 9.50 → 8.92 | Suppress |
| get-source-version.ts | 1 advisory rule | 9.44 | Suppress |
| cts-transport-source-manifest.ts | 1 advisory rule | 9.52 | Suppress |
| sap-disconnect.ts | 1 advisory rule | 10.00 → 9.53 | Suppress |
| get-source.ts | 1 advisory rule | 10.00 → 9.53 | Suppress |
| destination-registry.ts | 1 advisory rule | 9.55 | Suppress |
| destination-registry.test.ts | 1 advisory rule | 9.58 | Suppress |
| source-history.test.ts | 1 advisory rule | 9.59 | Suppress |
| plugin.ts | 1 advisory rule | 9.60 | Suppress |
| sap-connect.ts | 1 advisory rule | 8.53 → 8.15 | Suppress |
| adapter.ts | 1 advisory rule | 7.03 → 6.66 | Suppress |
| server.ts | 1 advisory rule | 10.00 → 9.64 | Suppress |
| source-capabilities.ts | 1 advisory rule | 9.69 | Suppress |
| transport-import.ts | 1 advisory rule | 9.24 → 9.05 | Suppress |
| auth.ts | 1 advisory rule | 9.07 → 8.96 | Suppress |
| session-helpers.ts | 1 advisory rule | 9.54 → 9.46 | Suppress |
| index.ts | 1 advisory rule | 9.45 → 9.43 | Suppress |
| routes.ts | 1 advisory rule | 2.21 → 2.20 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| find-references.ts | 9.00 → 9.12 | Complex Method, Large Method |
| grep-objects.ts | 9.46 → 9.49 | Large Method |
Absence of Expected Change Pattern
- adt-cli/packages/adt-mcp/src/lib/tools/delete-object.ts is usually changed with: adt-cli/packages/adt-mcp/src/lib/tools/object-creation.ts
- adt-cli/packages/openai-codegen/src/format/abapgit.ts is usually changed with: adt-cli/packages/openai-codegen/src/format/types.ts
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Comment on lines
+185
to
+249
| function snapshotFrozenSourceAccess( | ||
| access: McpFrozenSourceAccess | undefined, | ||
| ): McpFrozenSourceAccess | undefined { | ||
| if (!access) return undefined; | ||
| if ( | ||
| typeof access.systemSid !== 'string' || | ||
| access.systemSid.length === 0 || | ||
| access.systemSid.length > 16 || | ||
| !Number.isSafeInteger(access.maxSourceBytes) || | ||
| access.maxSourceBytes < 1 || | ||
| access.maxSourceBytes > 2 * 1024 * 1024 || | ||
| !Array.isArray(access.sources) || | ||
| access.sources.length === 0 || | ||
| access.sources.length > 500 | ||
| ) { | ||
| return undefined; | ||
| } | ||
| const sourceKeys = new Set<string>(); | ||
| const sourceRefs = new Set<string>(); | ||
| const sources: { | ||
| canonicalKey: string; | ||
| componentId: string; | ||
| sourceRef: string; | ||
| }[] = []; | ||
| for (const source of access.sources) { | ||
| const sourceKey = | ||
| source && | ||
| typeof source.canonicalKey === 'string' && | ||
| typeof source.componentId === 'string' | ||
| ? `${source.canonicalKey}\u0000${source.componentId}` | ||
| : undefined; | ||
| if ( | ||
| !source || | ||
| typeof source.canonicalKey !== 'string' || | ||
| !/^[A-Z0-9_]+:.+$/u.test(source.canonicalKey) || | ||
| typeof source.componentId !== 'string' || | ||
| source.componentId.length === 0 || | ||
| source.componentId.length > 512 || | ||
| /[\s\u0000-\u001f\u007f]/u.test(source.componentId) || | ||
| typeof source.sourceRef !== 'string' || | ||
| source.sourceRef.length === 0 || | ||
| source.sourceRef.length > 8 * 1024 || | ||
| /[\s\u0000-\u001f\u007f]/u.test(source.sourceRef) || | ||
| !sourceKey || | ||
| sourceKeys.has(sourceKey) || | ||
| sourceRefs.has(source.sourceRef) | ||
| ) { | ||
| return undefined; | ||
| } | ||
| sourceKeys.add(sourceKey); | ||
| sourceRefs.add(source.sourceRef); | ||
| sources.push( | ||
| Object.freeze({ | ||
| canonicalKey: source.canonicalKey, | ||
| componentId: source.componentId, | ||
| sourceRef: source.sourceRef, | ||
| }), | ||
| ); | ||
| } | ||
| return Object.freeze({ | ||
| systemSid: access.systemSid, | ||
| sources: Object.freeze(sources), | ||
| maxSourceBytes: access.maxSourceBytes, | ||
| }); | ||
| } |
There was a problem hiding this comment.
❌ New issue: Complex Method
snapshotFrozenSourceAccess has a cyclomatic complexity of 29, threshold = 9
Comment on lines
+683
to
+686
| const handle = async ( | ||
| req: http.IncomingMessage, | ||
| res: http.ServerResponse, | ||
| ): Promise<void> => { |
There was a problem hiding this comment.
❌ New issue: Complex Method
handle has a cyclomatic complexity of 9, threshold = 9
Comment on lines
+491
to
+492
| userHint: UserHint | undefined, | ||
| invocation: TrustedMcpInvocationClaims | undefined, |
There was a problem hiding this comment.
❌ New issue: Complex Method
handleMcp (top-level context) has a cyclomatic complexity of 9, threshold = 9
Comment on lines
+157
to
+183
| function snapshotRequestAccess( | ||
| access: McpRequestAccess | undefined, | ||
| ): McpRequestAccess | undefined { | ||
| if (!access) return undefined; | ||
|
|
||
| const classes = access.classes; | ||
| const destinationKeys = access.destinationKeys; | ||
| if (!Array.isArray(classes) || !Array.isArray(destinationKeys)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if ( | ||
| !classes.every(isMcpOperationClass) || | ||
| !destinationKeys.every(isMcpDestinationKey) | ||
| ) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const frozenSource = snapshotFrozenSourceAccess(access.frozenSource); | ||
| if (access.frozenSource && !frozenSource) return undefined; | ||
|
|
||
| return Object.freeze({ | ||
| classes: Object.freeze([...classes]), | ||
| destinationKeys: Object.freeze([...destinationKeys]), | ||
| ...(frozenSource ? { frozenSource } : {}), | ||
| }); | ||
| } |
There was a problem hiding this comment.
❌ New issue: Complex Method
snapshotRequestAccess has a cyclomatic complexity of 9, threshold = 9
Comment on lines
+189
to
+229
| if ( | ||
| typeof access.systemSid !== 'string' || | ||
| access.systemSid.length === 0 || | ||
| access.systemSid.length > 16 || | ||
| !Number.isSafeInteger(access.maxSourceBytes) || | ||
| access.maxSourceBytes < 1 || | ||
| access.maxSourceBytes > 2 * 1024 * 1024 || | ||
| !Array.isArray(access.sources) || | ||
| access.sources.length === 0 || | ||
| access.sources.length > 500 | ||
| ) { | ||
| return undefined; | ||
| } | ||
| const sourceKeys = new Set<string>(); | ||
| const sourceRefs = new Set<string>(); | ||
| const sources: { | ||
| canonicalKey: string; | ||
| componentId: string; | ||
| sourceRef: string; | ||
| }[] = []; | ||
| for (const source of access.sources) { | ||
| const sourceKey = | ||
| source && | ||
| typeof source.canonicalKey === 'string' && | ||
| typeof source.componentId === 'string' | ||
| ? `${source.canonicalKey}\u0000${source.componentId}` | ||
| : undefined; | ||
| if ( | ||
| !source || | ||
| typeof source.canonicalKey !== 'string' || | ||
| !/^[A-Z0-9_]+:.+$/u.test(source.canonicalKey) || | ||
| typeof source.componentId !== 'string' || | ||
| source.componentId.length === 0 || | ||
| source.componentId.length > 512 || | ||
| /[\s\u0000-\u001f\u007f]/u.test(source.componentId) || | ||
| typeof source.sourceRef !== 'string' || | ||
| source.sourceRef.length === 0 || | ||
| source.sourceRef.length > 8 * 1024 || | ||
| /[\s\u0000-\u001f\u007f]/u.test(source.sourceRef) || | ||
| !sourceKey || | ||
| sourceKeys.has(sourceKey) || |
There was a problem hiding this comment.
❌ New issue: Complex Conditional
snapshotFrozenSourceAccess has 2 complex conditionals with 21 branches, threshold = 2
Comment on lines
+319
to
+466
| publicKey, | ||
| keyId, | ||
| issuer, | ||
| audience, | ||
| }), | ||
| multiSystem: { systems: {}, resolve: () => undefined }, | ||
| destinationServer: { | ||
| destinationRegistry, | ||
| requestIdentity: () => ({ principal: 'untrusted-callback-principal' }), | ||
| requestAccess: () => ({ classes: ['read'], destinationKeys: ['dev'] }), | ||
| async resolveFrozenSource(input) { | ||
| resolverCalls.push(input); | ||
| return { | ||
| sourceUri: '/sap/bc/adt/oo/classes/zcl_scope_test/source/main', | ||
| }; | ||
| }, | ||
| }, | ||
| log: () => undefined, | ||
| }); | ||
| const credential = await signInvocation(privateKey, { | ||
| agentId: 'ai-review', | ||
| constraint: { | ||
| kind: 'ai-review-frozen-v1', | ||
| reviewId: '11111111-1111-4111-8111-111111111111', | ||
| runId: '22222222-2222-4222-8222-222222222222', | ||
| systemSid: 'DEV', | ||
| frozenSources: [ | ||
| { | ||
| canonicalKey: 'CLAS:ZCL_SCOPE_TEST', | ||
| componentId: 'main', | ||
| sourceRef: 'v1.opaque-reference', | ||
| }, | ||
| ], | ||
| }, | ||
| limits: { maxSourceBytes: 65_536 }, | ||
| }); | ||
| const transport = new StreamableHTTPClientTransport(new URL(server.url), { | ||
| requestInit: { headers: { Authorization: `Bearer ${credential}` } }, | ||
| }); | ||
| const client = new Client({ | ||
| name: 'ai-review-source-test', | ||
| version: '0.0.1', | ||
| }); | ||
|
|
||
| try { | ||
| await client.connect(transport); | ||
| const denied = await client.callTool({ | ||
| name: 'get_frozen_source', | ||
| arguments: { | ||
| destination: 'dev', | ||
| canonicalKey: 'CLAS:ZCL_SCOPE_TEST', | ||
| componentId: 'inactive', | ||
| }, | ||
| }); | ||
| assert.strictEqual(denied.isError, true); | ||
| assert.strictEqual( | ||
| (denied.content as Array<{ type: 'text'; text: string }>)[0]?.text, | ||
| 'mcp_scope_denied', | ||
| ); | ||
| assert.strictEqual(leases, 0); | ||
| assert.strictEqual(contexts, 0); | ||
| assert.deepStrictEqual(resolverCalls, []); | ||
|
|
||
| const accepted = await client.callTool({ | ||
| name: 'get_frozen_source', | ||
| arguments: { | ||
| destination: 'dev', | ||
| canonicalKey: 'CLAS:ZCL_SCOPE_TEST', | ||
| componentId: 'main', | ||
| }, | ||
| }); | ||
| assert.strictEqual(accepted.isError, undefined); | ||
| assert.deepStrictEqual(resolverCalls, [ | ||
| { | ||
| destination: 'dev', | ||
| systemSid: 'DEV', | ||
| sourceRef: 'v1.opaque-reference', | ||
| }, | ||
| ]); | ||
| assert.strictEqual(leases, 1); | ||
| assert.strictEqual(contexts, 1); | ||
| assert.strictEqual(boundedSourceReads, 1); | ||
| assert.deepStrictEqual( | ||
| JSON.parse( | ||
| (accepted.content as Array<{ type: 'text'; text: string }>)[0]?.text ?? | ||
| '', | ||
| ), | ||
| { | ||
| canonicalKey: 'CLAS:ZCL_SCOPE_TEST', | ||
| componentId: 'main', | ||
| bytes: 39, | ||
| source: 'CLASS zcl_scope_test DEFINITION PUBLIC.', | ||
| }, | ||
| ); | ||
| } finally { | ||
| await transport.close(); | ||
| await server.close(); | ||
| await destinationRegistry.shutdown(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
❌ New issue: Large Method
'AI Review redeems only the signed source component before acquiring its destination' has 146 lines, threshold = 70
Comment on lines
+63
to
+173
| version: 1, | ||
| material: {}, | ||
| release: async () => undefined, | ||
| }; | ||
| }, | ||
| }, | ||
| contextFactory: { | ||
| async create() { | ||
| contexts++; | ||
| return { client: {} as never, close: async () => undefined }; | ||
| }, | ||
| }, | ||
| ttlMs: 0, | ||
| }); | ||
| const verifier = createMcpInvocationVerifier({ | ||
| publicKey, | ||
| keyId, | ||
| issuer, | ||
| audience, | ||
| }); | ||
| const server = await startHttpServer({ | ||
| port: 0, | ||
| host: '127.0.0.1', | ||
| authMode: 'invocation', | ||
| invocationVerifier: verifier, | ||
| multiSystem: { systems: {}, resolve: () => undefined }, | ||
| destinationServer: { | ||
| destinationRegistry, | ||
| requestIdentity: () => ({ | ||
| principal: 'untrusted-callback-principal', | ||
| agentId: 'untrusted-callback-agent', | ||
| }), | ||
| requestAccess: () => ({ | ||
| classes: ['write'], | ||
| destinationKeys: ['prod'], | ||
| }), | ||
| }, | ||
| log: () => undefined, | ||
| }); | ||
| const credential = await signInvocation(privateKey); | ||
| const transport = new StreamableHTTPClientTransport(new URL(server.url), { | ||
| requestInit: { headers: { Authorization: `Bearer ${credential}` } }, | ||
| }); | ||
| const client = new Client({ name: 'invocation-auth-test', version: '0.0.1' }); | ||
|
|
||
| try { | ||
| await client.connect(transport); | ||
| assert.ok(transport.sessionId); | ||
|
|
||
| const tools = await client.listTools(); | ||
| assert.ok(tools.tools.some((tool) => tool.name === 'system_info')); | ||
| assert.ok(!tools.tools.some((tool) => tool.name === 'lock_object')); | ||
|
|
||
| const denied = await client.callTool({ | ||
| name: 'lock_object', | ||
| arguments: { destination: 'dev', objectName: 'ZCL_SCOPE_TEST' }, | ||
| }); | ||
| assert.strictEqual(denied.isError, true); | ||
| assert.strictEqual( | ||
| (denied.content as Array<{ type: 'text'; text: string }>)[0]?.text, | ||
| 'mcp_scope_denied', | ||
| ); | ||
| assert.strictEqual(leases, 0); | ||
| assert.strictEqual(contexts, 0); | ||
|
|
||
| const replacementCredential = await signInvocation(privateKey, { | ||
| tokenId: 'invocation-jti-2', | ||
| }); | ||
| const replacementResponse = await fetch(server.url, { | ||
| method: 'POST', | ||
| headers: { | ||
| Accept: 'application/json, text/event-stream', | ||
| Authorization: `Bearer ${replacementCredential}`, | ||
| 'Content-Type': 'application/json', | ||
| 'Mcp-Session-Id': transport.sessionId, | ||
| }, | ||
| body: JSON.stringify({ | ||
| jsonrpc: '2.0', | ||
| id: 1, | ||
| method: 'tools/call', | ||
| params: { | ||
| name: 'system_info', | ||
| arguments: { destination: 'dev' }, | ||
| }, | ||
| }), | ||
| }); | ||
| assert.strictEqual(replacementResponse.status, 403); | ||
| assert.deepStrictEqual(await replacementResponse.json(), { | ||
| jsonrpc: '2.0', | ||
| error: { code: -32000, message: 'mcp_session_identity_mismatch' }, | ||
| id: null, | ||
| }); | ||
| assert.strictEqual(leases, 0); | ||
| assert.strictEqual(contexts, 0); | ||
| } finally { | ||
| await transport.close(); | ||
| await server.close(); | ||
| await destinationRegistry.shutdown(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
❌ New issue: Large Method
'ARM invocation auth snapshots verified read scope and binds continuation to its JTI' has 107 lines, threshold = 70
Comment on lines
+175
to
+247
| test('ARM invocation auth fails closed for an AI Review policy the sidecar cannot yet enforce', async () => { | ||
| const { privateKey, publicKey } = await generateKeyPair('ES256'); | ||
| let leases = 0; | ||
| let contexts = 0; | ||
| const destinationRegistry = createDestinationContextRegistry({ | ||
| leaseProvider: { | ||
| async acquire({ destination }) { | ||
| leases++; | ||
| return { | ||
| destination, | ||
| expiresAt: Date.now() + 60_000, | ||
| version: 1, | ||
| material: {}, | ||
| release: async () => undefined, | ||
| }; | ||
| }, | ||
| }, | ||
| contextFactory: { | ||
| async create() { | ||
| contexts++; | ||
| return { client: {} as never, close: async () => undefined }; | ||
| }, | ||
| }, | ||
| ttlMs: 0, | ||
| }); | ||
| const server = await startHttpServer({ | ||
| port: 0, | ||
| host: '127.0.0.1', | ||
| authMode: 'invocation', | ||
| invocationVerifier: createMcpInvocationVerifier({ | ||
| publicKey, | ||
| keyId, | ||
| issuer, | ||
| audience, | ||
| }), | ||
| multiSystem: { systems: {}, resolve: () => undefined }, | ||
| destinationServer: { | ||
| destinationRegistry, | ||
| requestIdentity: () => ({ principal: 'untrusted-callback-principal' }), | ||
| requestAccess: () => ({ classes: ['read'], destinationKeys: ['dev'] }), | ||
| }, | ||
| log: () => undefined, | ||
| }); | ||
| const credential = await signInvocation(privateKey, { agentId: 'ai-review' }); | ||
| const transport = new StreamableHTTPClientTransport(new URL(server.url), { | ||
| requestInit: { headers: { Authorization: `Bearer ${credential}` } }, | ||
| }); | ||
| const client = new Client({ | ||
| name: 'ai-review-policy-test', | ||
| version: '0.0.1', | ||
| }); | ||
|
|
||
| try { | ||
| await client.connect(transport); | ||
| assert.deepStrictEqual((await client.listTools()).tools, []); | ||
|
|
||
| const denied = await client.callTool({ | ||
| name: 'system_info', | ||
| arguments: { destination: 'dev' }, | ||
| }); | ||
| assert.strictEqual(denied.isError, true); | ||
| assert.strictEqual( | ||
| (denied.content as Array<{ type: 'text'; text: string }>)[0]?.text, | ||
| 'mcp_scope_denied', | ||
| ); | ||
| assert.strictEqual(leases, 0); | ||
| assert.strictEqual(contexts, 0); | ||
| } finally { | ||
| await transport.close(); | ||
| await server.close(); | ||
| await destinationRegistry.shutdown(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
❌ New issue: Large Method
'ARM invocation auth fails closed for an AI Review policy the sidecar cannot yet enforce' has 71 lines, threshold = 70
| @@ -0,0 +1,1579 @@ | |||
| import { randomUUID } from 'node:crypto'; | |||
There was a problem hiding this comment.
❌ New issue: Lines of Code in a Single File
This module has 1139 lines of code, improve code health by reducing it to 1000
Comment on lines
+246
to
+317
| function toCanonicalAtcFindings(response: unknown): CanonicalAtcFinding[] { | ||
| const worklist = record(response)?.worklist; | ||
| const objects = records( | ||
| record(worklist)?.objects && record(worklist)?.objects, | ||
| ).flatMap((objects) => records(objects.object)); | ||
| return objects.flatMap((object) => | ||
| records(record(object.findings)?.finding).map((finding) => { | ||
| const line = atcLineFromLocation(finding.location); | ||
| const quickfixes = record(finding.quickfixes); | ||
| return { | ||
| checkId: stringField(finding, 'checkId') ?? '', | ||
| checkTitle: stringField(finding, 'checkTitle') ?? '', | ||
| messageText: stringField(finding, 'messageTitle') ?? '', | ||
| priority: Number.parseInt(stringField(finding, 'priority') ?? '3', 10), | ||
| objectType: stringField(object, 'type') ?? '', | ||
| objectName: stringField(object, 'name') ?? '', | ||
| ...(line ? { lineStart: line, lineEnd: line } : {}), | ||
| ...(optionalText(finding.messageId) | ||
| ? { messageId: optionalText(finding.messageId) } | ||
| : {}), | ||
| ...(optionalText(object.packageName) | ||
| ? { packageName: optionalText(object.packageName) } | ||
| : {}), | ||
| ...(optionalText(object.description) | ||
| ? { objectDescription: optionalText(object.description) } | ||
| : {}), | ||
| ...(optionalText(finding.contactPerson) | ||
| ? { contactPerson: optionalText(finding.contactPerson) } | ||
| : {}), | ||
| ...(optionalText(finding.processor) | ||
| ? { processor: optionalText(finding.processor) } | ||
| : {}), | ||
| ...(optionalText(finding.lastChangedBy) | ||
| ? { lastChangedBy: optionalText(finding.lastChangedBy) } | ||
| : {}), | ||
| ...(optionalText(finding.exemptionKind) | ||
| ? { exemptionKind: optionalText(finding.exemptionKind) } | ||
| : {}), | ||
| ...(optionalText(finding.exemptionApproval) | ||
| ? { exemptionApproval: optionalText(finding.exemptionApproval) } | ||
| : {}), | ||
| ...(optionalAtcBoolean(finding.noExemption) === undefined | ||
| ? {} | ||
| : { noExemption: optionalAtcBoolean(finding.noExemption) }), | ||
| ...(optionalText(finding.quickfixInfo) | ||
| ? { quickfixInfo: optionalText(finding.quickfixInfo) } | ||
| : {}), | ||
| ...(quickfixes | ||
| ? { | ||
| quickfixes: { | ||
| ...(optionalAtcBoolean(quickfixes.manual) === undefined | ||
| ? {} | ||
| : { manual: optionalAtcBoolean(quickfixes.manual) }), | ||
| ...(optionalAtcBoolean(quickfixes.automatic) === undefined | ||
| ? {} | ||
| : { automatic: optionalAtcBoolean(quickfixes.automatic) }), | ||
| ...(optionalAtcBoolean(quickfixes.pseudo) === undefined | ||
| ? {} | ||
| : { pseudo: optionalAtcBoolean(quickfixes.pseudo) }), | ||
| }, | ||
| } | ||
| : {}), | ||
| ...(finding.checksum === undefined | ||
| ? {} | ||
| : { checksum: String(finding.checksum) }), | ||
| ...(documentationUriForFinding(finding) | ||
| ? { documentationUri: documentationUriForFinding(finding) } | ||
| : {}), | ||
| }; | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
❌ New issue: Complex Method
toCanonicalAtcFindings has a cyclomatic complexity of 23, threshold = 9
PR Summary by Qodofeat(adt-server): add bounded ADT server sidecar with REST API
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Generated with Devin
Summary by cubic
Adds a bounded ADT server sidecar that exposes a typed REST API for ATC, object source, history/metadata, and package/object search. Also introduces a guarded MCP sidecar with signed, capability-scoped reads and enforced destination scopes for frozen reads.
New Features
Migration
Written for commit 4fcb826. Summary will update on new commits.