DXBE-20: Add source:config:push command with SAS API client

[phenaproxima]

⚠️ Heads-up: this is completely vibe-coded.

I pair-wrote this with an AI assistant. It's not really reviewed, but it passes PHPStan / PHPCS / PHPUnit locally, and the container boots — but it has not been run against a live SAS instance (the SAS endpoint doesn't exist yet), and the end-to-end execute() path is not covered by tests. Treat it as a working draft of the shape, not finished code. All assumptions are marked @todo DXBE-20.

(I vibe-coded this because I am totally unfamiliar with this repo, and with the service layers this feature touches, including SAT, SAS, and maybe others. Under demands to produce an outcome, this is what you end up with. This needs proper review from someone familiar with these spaces, because I cannot truly stand behind this code.)

What it does

Adds acli source:config:push. It reads every .yml file under .acquia/config/, assembles them into a single YAML document (keyed by config collection — root is "", subdirectories become dotted names like language.es — then by config name), POSTs it to the Sites Aggregation Service (SAS), and polls the resulting async operation until it finishes. The payload structure mirrors what drush source:config:dump --single-yaml produces (what a future source:config:pull would write out).

How

• New SasApi/ client layer following the existing AcsfApi/ pattern. SAS shares the Accounts authentication layer with the Cloud API, so the connector reuses the standard OAuth2 client-credentials Bearer flow; the only new configuration is the SAS base URI (ACLI_SAS_API_BASE_URI).
• Site identity is resolved from the current repository's git remote — no arguments needed; --siteInstanceId and [environmentId] are optional overrides.
• Confirmation prompt before pushing (a remote mutation), with --force for CI.

Deliberately stubbed (pending the SAS endpoint)

• Endpoint paths and response field names (placeholders)
• Payload YAML vs. JSON encoding (pending SAS team input)
• SAS base URI value / env var name

Tested

• php -l, PHPStan, PHPCS all clean
• New unit tests for payload assembly (3/3)
• No regressions in existing Remote/Acsf command tests (33/33)
• acli list and source:config:push --help boot and render correctly

Not yet done

• Live test against a real SAS endpoint
• End-to-end execute() test coverage (needs the heavier command-test mock scaffolding; waiting on the SAS endpoint's real shape)

[codecov]

Codecov Report

❌ Patch coverage is 89.39394% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.51%. Comparing base (9afe4e7) to head (8c1a0a3).

Files with missing lines	Patch %	Lines
src/Command/Source/ConfigCommandBase.php	87.50%	7 Missing ⚠️
src/SasApi/SasCredentials.php	37.50%	5 Missing ⚠️
src/Command/Source/ConfigPullCommand.php	96.29%	1 Missing ⚠️
src/SasApi/SourceConfig.php	90.90%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2035      +/-   ##
============================================
+ Coverage     92.49%   92.51%   +0.01%     
- Complexity     1995     2036      +41     
============================================
  Files           123      131       +8     
  Lines          7238     7370     +132     
============================================
+ Hits           6695     6818     +123     
- Misses          543      552       +9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Adds a new source:config:push Symfony Console command to package local .acquia/config/**/*.yml into a single “collections → config name → values” document and submit it to a new SAS (Sites Aggregation Service) API client layer, with polling for async completion. This fits alongside the existing Cloud/Acsf client patterns and command set.

Changes:

• Introduces source:config:push command, payload assembly, confirmation prompt, and async polling.
• Adds a new SasApi/ client layer (connector, client service, endpoint wrapper) and wires it into the production service container.
• Adds unit tests for payload assembly behavior (collections and empty cases).

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/phpunit/src/Commands/Source/ConfigPushCommandTest.php	Adds unit tests for config payload assembly from .acquia/config directory structure.
src/Command/Source/ConfigPushCommand.php	Implements the source:config:push command: reads YAML config files, submits to SAS, and polls operation status.
src/SasApi/SourceConfig.php	Defines SAS endpoint wrapper methods for submitting a config push and checking operation status.
src/SasApi/SasCredentials.php	Provides SAS base URI configuration (env var + default).
src/SasApi/SasConnectorFactory.php	Adds a connector factory for SAS requests.
src/SasApi/SasConnector.php	Adds a SAS connector class extending the Cloud API connector.
src/SasApi/SasClientService.php	Adds a client-service factory for producing configured SasClient instances.
src/SasApi/SasClient.php	Adds a SAS client class extending the Cloud API client.
config/prod/services.yml	Registers SAS credentials + connector factory wiring and excludes SourceConfig from auto-service registration (instantiated manually by the command).

Suppressed comments (2)

src/Command/Source/ConfigPushCommand.php:93

• execute() currently calls determineSiteInstance(), which only returns a value when --siteInstanceId is provided (see CommandBase::determineSiteInstance()), so the command will always throw in the default “no args” flow and the [environmentId] argument is effectively ignored. Using CommandBase::determineEnvironment() here would allow the documented environment resolution (including git-remote inference) while still supporting --siteInstanceId as an override.

        $siteInstance = $this->determineSiteInstance($input);
        if ($siteInstance === null) {
            throw new AcquiaCliException(
                'Could not determine a Source site instance. Run this command from a repository linked to an Acquia Cloud application, or pass --siteInstanceId.'
            );

src/Command/Source/ConfigPushCommand.php:157

• Yaml::parseFile() will throw on invalid YAML and can return non-array values; right now that bubbles up as an unhandled exception or produces an invalid payload shape. Consider catching YAML parse failures per-file and throwing an AcquiaCliException that includes the offending path, and validate that each config file parses to an array/map.

            $collection = $relativeDir === '' ? '' : str_replace('/', '.', $relativeDir);
            $name = $file->getBasename('.yml');
            $payload[$collection][$name] = Yaml::parseFile($file->getPathname());
        }

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[phenaproxima]

Fair point on the convention, but we're deliberately holding off on an execute()-level test for now: the SAS endpoint doesn't exist yet, so any test would just cement a placeholder request/response shape we'd have to redo once the real API lands. The payload assembly (the novel logic) is covered. We'll add command-level coverage once the endpoint's contract is settled — tracked as part of DXBE-20.

[github-actions]

Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/2035/acli.phar

curl -OL https://acquia-cli.s3.amazonaws.com/build/pr/2035/acli.phar
chmod +x acli.phar

[wimleers]

but it has not been run against a live SAS instance (the SAS endpoint doesn't exist yet)

This AFAICT needs both

• https://github.com/acquia/sites-aggregation-service/pull/971
• https://github.com/acquia/sites-aggregation-tasks/pull/261

[wimleers]

This PR is correctly hard-blocked on SAS + SAT PRs (see prior comment), but AFAICT neither PR currently does enough to allow this PR to work: https://github.com/acquia/sites-aggregation-service/pull/971#pullrequestreview-4960954310

[wimleers]

acquia/sites-aggregation-service#971 specifies POST /api/sites/{siteId}/config-sync, authorised on site admin for the site. This sends POST /environments/{environmentId}/config-import — wrong resource and wrong path.

determineSiteInstance() already resolves $siteInstance->site (CommandBase.php:895), but execute() keeps only the environment (line 112). Pass $siteInstance->site->id.

[wimleers]

acquia/sites-aggregation-service#971 returns 202 Accepted with a Message and no id, so this throws an exception on every invocation. testExecuteThrowsWhenOperationIdMissing() is currently testing the production path.

Needs upstream first, then still changes here. Point 2 of my review on acquia/sites-aggregation-service#971 asks for config-sync to create a notification. If it does, the identifier comes off that notification rather than off the 202 body, so this block still has to be rewritten — it just stops being unfixable.

[wimleers]

GET /config-operation/{id} doesn't exist. acquia/sites-aggregation-service#971's description: "No notification record is created and no status polling is wired up, so the caller gets a 202 and no way to observe the outcome beyond the task log."

So getStatus(), waitForOperation() and the pending/running/succeeded/failed states are all guesses.

There is a real surface to aim at, though. SAS already exposes GET /sites/{siteId}/notifications, with status, progress and completed_at on each Notification — and Site::createBackup(), restoreBackup() and export() all create one. syncConfig() is the exception. So the shape is polling notifications once config-sync creates one, not a bespoke /config-operation/{id} resource.

Needs upstream first, then still changes here. Point 2 of my review on acquia/sites-aggregation-service#971 decides which: if config-sync creates a notification, this polls that instead of /config-operation/{id}; if fire-and-forget is the intended contract, this method comes out entirely. Either way the code changes.

[wimleers]

DXBE-19 asks for the current folder of config to be sent as the POST body. Per acquia/haas-drupal#2233, dr source:config:import --single-yaml reads exactly that from stdin — and the assembly this PR originally had (keyed by collection, then config name) matched it.

Commit e44a7a1 deleted that to match acquia/sites-aggregation-service#971's input: false. The result is a push that sends nothing from the developer's machine and imports whatever is already deployed. So pull → edit → push silently ignores local edits until they're committed and deployed.

Needs upstream first, then still changes here. Point 1 of my review on acquia/sites-aggregation-service#971 asks for an optional payload; if it lands, the deleted assembly has to be restored in this PR. If the answer is that the payload path is out of scope for DXBE-20, then DXBE-19 needs revisiting rather than this code.

[wimleers]

Pull is DXBE-8 scope, so not out of place — but acquia/sites-aggregation-service#971 adds only the import direction, so config-export and /payload have nothing to run against.

Not covered by my review on acquia/sites-aggregation-service#971 either — that asks about the push payload, not a config export endpoint. POST /sites/{siteId}/exports does exist, but it's a whole-site export (MysqlSiteExportAdapter), not CMS config. The export endpoint is DXBE-21, so this half is blocked on that landing first.

That matters because pull is the half that deletes files. testWritePayloadCreatesConfigDirWhenPayloadEmpty() shows it as a passing test: {} parses to [], passes the is_array() guard at line 65, wipes .acquia/config/ and writes nothing back. Any SAS-side bug returning an empty export destroys uncommitted local config, and the prompt says nothing about local deletion.

Suggest splitting pull into its own PR. If it stays: name the directory in the prompt, write to a temp dir and swap, and refuse to wipe on an empty payload.

[wimleers]

The PR description says site identity is resolved from the git remote with no arguments needed. determineSiteInstance() (CommandBase.php:884-906) reads only --siteInstanceId and otherwise returns null, so line 106 throws unless that option is passed. acceptEnvironmentId() (line 69) registers an argument nothing reads.

CommandBase::determineEnvironment() (CommandBase.php:690-708) is the existing ladder: --siteInstanceId → environmentId argument → codebase environment → git-remote-matched application → prompt. It also normalises through EnvironmentTransformer, which would make commit 4768081 unnecessary.

All six execute() tests pass --siteInstanceId, which is why this wasn't caught.

[wimleers]

operationLabel() is the whole of the push wording, and ConfigCommandBase.php:116 renders it as Importing configuration on the <env> environment?. Per acquia/sites-aggregation-tasks#261, confirming it puts the site in maintenance mode, backs up the database, checkpoints config, imports, and on failure reverts the checkpoint or restores the database. acquia/sites-aggregation-service#971 opens its risk section with "This triggers a destructive operation on a live site … The site is offline for the duration."

Someone pointing this at production can't tell from that sentence that the site goes down. EnvMirrorCommand.php:61 and PushDatabaseCommand.php:48 are the models — they name what gets overwritten. Please say that the site goes into maintenance mode and is unavailable while this runs, and that a database backup is taken first.

Anchoring here because the warning is push-specific: pull doesn't take the site offline. But the base's sprintf('%s on the %s environment?', ...) template only has room for a short label, so either that grows or the base needs a hook for the extra warning text.

[wimleers]

⚠️ With -n and no --force, confirm(..., false) returns false and the command returns SUCCESS having done nothing. A CI job gets exit 0 and no sync.

DXBE-8: "failure exits non-zero with a plain-language error." Throw an exception when the session is non-interactive and --force wasn't given, naming --force.

That fixes the no-op locally. Exiting non-zero when the remote operation fails needs upstream first — point 2 of my review on acquia/sites-aggregation-service#971, since without a notification to poll the command can't know it failed.

[wimleers]

Two things.

The value: sites-aggregation-service.acquia.com appears in no source I can find. The SAS repo itself only carries sites-aggregation-service.acquia.test/api (local) and a relative url: / in docs/sas-spec.yaml, so it offers no production host either. The only one with evidence behind it is DXBE-19's working curl: https://sites-aggregation-service-prod.prod.cicd.acquia.io/api. Use that unless the SAS team names a stable alias — prod.cicd.acquia.io reads like a deployment host, so it's worth one question before hardcoding.

The fallback: getenv() matches CloudCredentials and AcsfCredentials and is right. Hardcoding a production host doesn't — both of those return null. They can afford to: Cloud's SDK default is already correct, and ACSF's URI is required per-customer input. Neither holds for SAS, so an unset env var sends SAS traffic to cloud.acquia.com. SasConnectorTest::testConstructorDefaultsToCloudBaseUriWhenNotOverridden() documents that.

Either hardcode a confirmed host, or throw an exception when the env var is unset.

[wimleers]

class SasClient extends Client {}. AcsfClient exists because it overrides processResponse(); this overrides nothing. Use AcquiaCloudApi\Connector\Client directly — SasClientService is still the prophecy seam the tests need.

[wimleers]

Constructor forwards all three arguments unchanged and the class adds nothing. AcsfConnector swaps in basic auth, which is why it's a subclass.

Deleting this also deletes SasConnectorTest and the services.yml:166 alias mapping a Connector type to a factory that isn't one.

[wimleers]

To be precise about what's duplicated: it isn't that this resembles the CloudApi version, it's that the return on line 30 and the fallback return on line 46 are the same statement — new SasConnector($this->config, $this->baseUri, $this->accountsUri) both times. So whichever way the if goes, the same object comes back, and the branch is dead. connectorProvider() confirms it: every non-token case expects SasConnector::class.

Commits 4c0ca17 → 8c1a0a3 found this, then deleted the distinguishing test and added the ignore. Deleting the if is the fix; Connector reads credentials out of $config either way.

(Inherited from CloudApi\ConnectorFactory:39-41,57, which has it too.)

[wimleers]

getCloudKey() and getCloudSecret() return null with // Unused: comments. The class implements ApiCredentialsInterface only to be allowed to exist, and is used solely for getBaseUri().

There's a trap in that: SasClientService doesn't override checkAuthentication(), so it inherits ClientService.php:79, which calls getCloudAccessToken() — not declared on that interface. It works only because autowiring supplies CloudCredentials via services.yml:88. Wire sas.credentials in and it throws a fatal error. AcsfClientService overrides checkAuthentication() for this reason.

Dropping implements ApiCredentialsInterface and the two null methods leaves a class that just provides getBaseUri(), which is all the @=service(...) expression needs. Also clears most of the codecov/patch failure.

[wimleers]

is_string($response) can never be true: Client::processResponse() runs json_decode with JSON_THROW_ON_ERROR, so a text/yaml body throws before this line. If SAS does return YAML, SasClient would need a processResponse() override, as AcsfClient has.

[wimleers]

Returning '' means Yaml::parse('') => null => "invalid config payload" two frames later. A site with no exportable config gets the same message as a broken response. Throw here instead, naming the operation ID and the missing field.

[wimleers]

⚠️ $name is a key from the decoded remote response, interpolated straight into a path, so ../ in a config name writes outside .acquia/config/ — arbitrary file write from a response body, and with dumpFile() overwriting, potential data loss outside the config directory too. Validate the name (basename(), or reject anything containing a separator) before building the path.

[wimleers]

DXBE-8 requires a configurable target directory for both commands. A private const can't be configured — this wants an option defaulting to .acquia/config.

The option is needed here regardless. The push side additionally needs upstream first: the directory can't be communicated to SAS at all while acquia/sites-aggregation-service#971 takes no body — point 1 of my review on acquia/sites-aggregation-service#971. See my comment on SourceConfig::push().

[wimleers]

DXBE-8: "Push can point at an arbitrary prior Git commit to restore that state (revert path)." There's no option for it.

To be clear about the mechanism: the rollback in acquia/haas-drupal#2233 is dr checkpoint:create / checkpoint:revert, but that's automatic recovery inside one import — the checkpoint is made in step 4 and reverted only if that same import fails. It isn't a user-selectable restore point, and acquia/sites-aggregation-service#971 exposes no way to list checkpoints or revert to a chosen one.

The workable route is the payload: git show <ref>:.acquia/config/ locally, sent as --single-yaml. That needs upstream first — point 1 of my review on acquia/sites-aggregation-service#971 — but upstream alone doesn't deliver this. Once a body is accepted, this PR still has to add the --ref option, read the config out of that commit, and serialise it. Upstream unblocks it; it doesn't implement it.

[wimleers]

⚠️ No other command here has --force. -f is taken elsewhere by --factory-url (AuthAcsfLoginCommand.php:22) and --format (MakeDocsCommand.php:34), so no collision — but worth a deliberate call on a new flag versus --no-interaction semantics, and it needs to be consistent with the -n behaviour I flagged on line 119.

[wimleers]

Five of six pull tests go through this reflection helper. Driving them through executeCommand() with varied mocked payloads instead would also cover onSuccess(), the Yaml::parse() failure and the is_array() guard — most of the seven ConfigCommandBase lines codecov reports as uncovered.

[wimleers]

Nine @infection-ignore-all annotations were added. The DUMP_DEPTH/DUMP_INDENT one is fair. This one and SasClientService.php:21 would both become observable via the executeCommand() tests above, and the SasConnectorFactory one is suppressing a true positive (see my comment on SasConnectorFactory.php:24). Worth rechecking which survive once the empty subclasses and dead branch are gone.

[wimleers]

Note

I reviewed this with significant help from AI (Claude Opus 5). I'm not familiar with the ACLI codebase, so I leaned on it to navigate CommandBase, the CloudApi/AcsfApi layers and the DI wiring. I've read and stand behind the findings; the code archaeology isn't mine.

Thanks for the honest description — it made this much faster to review. Findings are inline; this covers only what isn't in the inline code reviews.

The three sibling PRs answer most of the open @todo DXBE-20 markers, differently from the placeholders here: acquia/sites-aggregation-service#971 fixes the endpoint and its contract, acquia/sites-aggregation-tasks#261 the task behaviour, and acquia/haas-drupal#2233 documents what the import actually does. Worth a read before the next revision — most of the todos can be closed rather than deferred.

Three tickets, not one

Titled DXBE-20, but it delivers DXBE-19 (push), DXBE-8 (pull), and the DXBE-20 client. Worth retitling or splitting: the two ACLI tickets carry acceptance criteria this doesn't meet, and nobody reviewing a PR labelled DXBE-20 would think to check them.

Requirement this PR does not yet address:

• Configurable target directory
• The revert path
• "Same validation / rejection rules as auto-import apply"
• Documentation for DIY customers on other providers

Some of this is blocked on SAS, and I've asked there

I left a review on acquia/sites-aggregation-service#971 with three points, because they're cheaper to settle before that PR merges than after. Two of them decide code here:

1. Whether the endpoint accepts a payload. If yes, the payload assembly deleted in e44a7a1 comes back and push can honour DXBE-19 and the revert path. If no, DXBE-19 needs revisiting rather than this code — because push currently sends nothing from the developer's machine.
2. Whether config-sync creates a notification. That decides whether polling here targets GET /sites/{siteId}/notifications, or comes out entirely and the command becomes fire-and-forget. As written it polls an endpoint that doesn't exist.

Third point there is the missing concurrency guard — nothing to change in acli until a 409 exists, but this command can't detect a sync already in flight, and a CLI makes double-firing much easier than a dashboard button did.

The inline comments flag which findings need that decision first. Worth noting that "needs upstream first" isn't the same as "resolved upstream" — in each case this PR still changes, it just stops being blocked.

What I'd fix regardless

Roughly in order of how much I'd want them addressed:

Area	Gist
Endpoint path	Keyed on environment; SAS keys on site, and the site ID is already resolved and dropped
Site resolution	The no-argument path in the description doesn't work; CommandBase::determineEnvironment() is the existing ladder
Confirmation prompt	Doesn't mention that the site goes into maintenance mode and is offline for the duration
pull's wipe	An empty-but-valid payload deletes .acquia/config/ and writes nothing back
Base URI	Hardcodes a host I can't find anywhere; unset env var silently sends SAS traffic to the Cloud API
SasApi surface	SasClient and SasConnector are empty pass-throughs; SasCredentials implements an interface it doesn't need, and hides a latent fatal
SasConnectorFactory	The key/secret branch and the fallback are the same statement, so the branch is dead
Tests	The execute() tests all pass --siteInstanceId, so they exercise a path no user takes; the pull tests drive private methods by reflection

Between the empty subclasses, the dead branch and the reflection tests, most of the nine @infection-ignore-all annotations should stop being necessary — worth rechecking which survive.

CI: autolabel / require_label and codecov/patch are red. The label is a one-liner; the coverage gap is mostly SasCredentials.

Naming

Both DXBE-8 and DXBE-19 fold config sync into the existing source:cms push/pull — "not building a new unified command" — where this adds a separate source:config:* pair. There's no source:cms in acquia/cli today, so presumably it migrates from Canvas CLI. Worth settling before these names become public API; renaming a shipped command is painful.

Separately, "push" is a misleading verb for what this currently does, and would stay misleading even if the naming question resolves — see the payload point above.

Suggested shape

If the answer upstream is "no payload, fire and forget", the honest version of this is a good deal smaller than what's here:

1. One command, one direction: resolve the site, POST, print the returned message. No polling, no getStatus(), no ConfigCommandBase (there'd be one subclass), no pull.
2. No SasClient, no SasConnector, no interface on SasCredentials — just SasConnectorFactory returning a plain Connector, plus SasClientService as the test seam.
3. Resolution through determineEnvironment().
4. A prompt that says the site goes offline.
5. Payload, revert path and configurable directory as follow-ups gated on that review; pull gated on DXBE-21.

Happy to talk any of it through.