Skip to content

feat(cloud-engine): document the proxy, threshold-signing and cycles funding flow - #363

Merged
AntonioVentilii merged 6 commits into
mainfrom
av/opencloud-skills-flow-38116d
Aug 26, 2026
Merged

feat(cloud-engine): document the proxy, threshold-signing and cycles funding flow#363
AntonioVentilii merged 6 commits into
mainfrom
av/opencloud-skills-flow-38116d

Conversation

@AntonioVentilii

@AntonioVentilii AntonioVentilii commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

What

Documents the flow an engine app follows when it needs chain-key services (get a proxy, sign through it, keep it funded, and know which balance the money comes out of), split along the existing boundary between the two cloud-engine skills.

Both skills already mentioned the proxy, but in a single paragraph, and pointed at a console page that no longer exists.

Why

Derived from dfinity/control-panel at main (d40cf8100). Three things the skills did not say, each of which an agent gets wrong without help:

  1. The console proxy and the icp-cli proxy are different canisters. The console one is a fork of dfinity/proxy-canister that authorizes the engine's canister-id ranges; upstream authorizes controllers. So the console proxy rejects your CLI identity, and a self-deployed proxy rejects your engine canisters until each is added as a controller.
  2. Derived keys belong to the proxy's principal. The fork forces canister_id = None and prefixes the caller into derivation_path / context, so the management canister derives under the proxy's id. Deleting a proxy, which the console offers with a cycles refund that makes it look like a tidy-up, changes every derived address and strands funds at the old ones.
  3. The chain-key helper libraries cannot be used on an engine. ic-vetkeys / ic-cdk attach the fee themselves, which is exactly the cross-subnet cycle-bearing call the engine rejects.

Changes

deploy-to-cloud-engine (the ops half)

  • New Step 5: getting a proxy. Console flow (engine → CanistersProxy canisters, $5 minimum, card-funded, automatic top-up below 500 G cycles, refund on delete) and the self-deployed icp new --subfolder proxy alternative, with a table of which to use when.
  • Wiring the proxy id into the app as a canister env var, and why it is permanent configuration.
  • New section on the three balances (engine subscription, engine emergency reserve capped at 4 weeks and owner-only to extend, and proxy cycles), since "top up the canister" means something different for each, and engine canisters have none of them.
  • 5 new pitfalls (15–19).
  • Fixes stale console pointers: the subnet id is on the engine's Settings page, and the .icp bundle upload is Applications → Build and deploy app. Both previously said "App Center".

cloud-engine-canisters (the code half)

  • Worked ECDSA and vetKD recipes through the proxy, with the management-canister types spelled out, key_1 / test_key_1 fees, and the free *_public_key methods.
  • Why the helper libraries don't work here and what replaces them.
  • The derivation rules the proxy enforces, and the consequences for deleting or swapping one.
  • Notes that InsufficientCycles reports the proxy's liquid balance, that over-attaching is safe (excess is refunded to the proxy), and that a CallFailed can be a SYS_UNKNOWN, not proof the call didn't happen.
  • 4 new pitfalls (10–13).

Deliberate omissions

  • No REST endpoints. The console actions are backed by /v1/engines/{id}/proxy etc., but they authenticate with a jwt cookie minted by the II browser login (no token auth), so an agent cannot call them. Documenting them would invite agents to try. This matches the repo's precedent: custom-domains and ic-dashboard document public, unauthenticated APIs; nothing here documents a session-authenticated one.
  • No MCP. The console links mcp.internetcomputer.org from its command palette, but it is a separate DFINITY preview, OAuth-gated (so its tool list can't be verified), and no skill in this repo mentions MCP.

Verification

No live deploy; nothing here cost money to confirm.

  • Proxy canister behaviour: ran cargo test -p proxy-canister --lib in control-panel: 10/10 pass, including ecdsa_sign_prepends_caller_and_preserves_the_rest, ecdsa_public_key_forces_canister_id_none_and_prefixes, vetkd_prefixes_caller_into_context_length_tagged, and distinct_callers_get_distinct_derivations. The ingress/range authorization claims come from proxy_tests.rs (PocketIC), read but not run; they need a wasm build and a pocket-ic binary.
  • icp CLI: every command used verified with --help against both the installed 1.0.2 and a scratch-installed 1.3.0: identical surface for --proxy, --cycles, canister top-up, cycles mint/balance/transfer, settings update --add-controller. Ran icp new --subfolder proxy to confirm the template exists; it pulls the pre-built dfinity/proxy-canister v0.1.0 wasm and takes no init args (versus the console fork's InitArgs { allowed_ranges }).
  • Fees: key_1 = 26_153_846_153, test_key_1 = 10_000_000_000, *_public_key free, excess refunded, all per the IC gas-cost reference.
  • Console wording, amounts and page locations: read from en.json, the proxy handlers, ProxyConfig, and the nav constants, i.e. from source, not from a live console. Worth a second pair of eyes from someone with the console open.
  • Motoko examples: the ECDSA and vetKD worked examples were compile-checked verbatim (wrapped in a minimal actor supplying the free variables) against moc 1.14.1 + mo:core: clean after one fix this check itself caught: Debug.trap does not exist in mo:core, so the snippets now use Runtime.trap. The file's pre-existing Rule 2/3/4 sketches were left as they were.

npm run validate: 29 skills, all passed (18 warnings, all pre-existing in kind).

Eval results

6 new cases, 3 per skill. Each was run with baseline; one was rewritten after its first run showed no delta.

Output evals: with skill vs baseline
cloud-engine-canisters
  7. Threshold ECDSA address and signature from an engine canister   WITH 5/5 | WITHOUT 2/5
  8. Adversarial: reaching for a chain-key helper library            WITH 3/3 | WITHOUT 1/3
  9. Adversarial: cleaning up a proxy an app signs through           WITH 4/4 | WITHOUT 3/4

deploy-to-cloud-engine
 18. Set up a proxy so an engine app can sign Bitcoin transactions   WITH 4/4 | WITHOUT 0/4
 19. Adversarial: self-deployed proxy onto the engine's subnet       WITH 3/3 | WITHOUT 0/3
 20. Adversarial: 'my engine canister is out of cycles'              WITH 3/3 | WITHOUT 0/3

Re-run at head (2f1e92f): a later style commit touched five of these cases' prompt/behavior strings after the original runs, so all five were re-run against the final text. Four match the numbers above exactly; case 7's baseline came back 1/5 instead of 2/5 (judge variance on the baseline side; the with-skill side is identical), so the table keeps the more conservative published pair.

Case 9 is the weak one, and I'm reporting it rather than tuning it. Its first version scored 4/4 both with and without the skill: the prompt named the derivation dependency, so it tested nothing. Rewritten to describe the deletion as an innocent cost-saving cleanup, it now separates, but only by one behaviour: a strong baseline still infers that swapping a proxy changes derived addresses. What it does not do is offer the mitigation (top up instead, or move the assets first). Treat that case as a low-value regression guard, not evidence the skill carries the finding.

The other five separate cleanly, and three of them are the ones that matter operationally: baseline scored 0/4, 0/3 and 0/3 on setting up a proxy, on deploying one onto the engine's subnet, and on "my engine canister is out of cycles" (where it recommended icp canister top-up, which cannot work).

Trigger evals: 3 new should_trigger and 2 new should_not_trigger on cloud-engine-canisters, 4 new should_trigger on deploy-to-cloud-engine, drawing the line so proxy deployment and funding routes to deploy-to-cloud-engine while signing, calling through, and diagnosing it routes to cloud-engine-canisters. Review found one regression on a pre-existing query (the InsufficientCycles diagnosis query rerouted to the funding skill); fixed on both sides of the boundary in a734022 and both full suites re-run:

cloud-engine-canisters  should-trigger 11/11 | should-not-trigger  8/8
deploy-to-cloud-engine  should-trigger 16/16 | should-not-trigger 10/10

Open question for review

Both descriptions were already within a few characters of the 1024-byte cap, so fitting the new triggers meant trimming existing wording rather than adding. More importantly, the bodies grew:

Skill Before After
deploy-to-cloud-engine 7,144 tokens (already over the 5,000 guidance) 9,555
cloud-engine-canisters under 5,000 (no warning) 6,629 (new warning)

The worked ECDSA/vetKD examples and the console proxy walkthrough are the bulk of it. If we would rather keep the SKILL.md bodies lean, the natural split is a references/ file per skill; neither has one today. Happy to do that; it seemed better to show the content in place first.

…funding flow

Both cloud-engine skills described the proxy canister in a single
paragraph, and pointed at an App Center page that no longer exists. This
documents the flow an engine app actually follows when it needs chain-key
services, split along the existing boundary between the two skills.

deploy-to-cloud-engine (the ops half):
- Step 5 covers getting a proxy: the console flow (engine -> Canisters ->
  Proxy canisters, $5 minimum, card-funded, automatic top-up, refund on
  delete) and the self-deployed `icp new --subfolder proxy` alternative,
  with a table of when each applies.
- Documents wiring the proxy id into the app as a canister env var, and
  the three balances an engine has (subscription, emergency reserve,
  proxy cycles) — only one of which is a canister cycle balance at all.
- Fixes stale console pointers: the subnet id is on the engine's Settings
  page, and the .icp bundle upload is Applications -> Build and deploy app.

cloud-engine-canisters (the code half):
- Worked ECDSA and vetKD recipes through the proxy, with the management
  canister types spelled out, the key_1 / test_key_1 fees, and the free
  *_public_key methods.
- Why the chain-key helper libraries (ic-vetkeys, ic-cdk) cannot be used
  on an engine, and what replaces them.
- The derivation rules the console proxy enforces, and the consequence:
  keys belong to the proxy's principal, so deleting or swapping a proxy
  changes every derived address and strands funds at the old ones.

Adds 6 eval cases (3 per skill) covering the new pitfalls.
…separates

Its first form scored 4/4 with and without the skill: the prompt stated
that keys were derived through the proxy, which is the finding under
test. Reframed as an innocent cost-saving cleanup, with the auto-top-up
threshold qualified as a default in the skill.
@github-actions

Copy link
Copy Markdown

Skill Validation Report

Project Checks


✓ Project checks passed for 2 skills (0 warnings)

@AntonioVentilii
AntonioVentilii marked this pull request as ready for review August 25, 2026 15:47
@AntonioVentilii
AntonioVentilii requested review from a team and JoshDFN as code owners August 25, 2026 15:47
…xy signing examples

mo:core has no Debug.trap — trapping moved to Runtime.trap. Caught by
compile-checking the worked examples verbatim (wrapped in a minimal
actor supplying the free variables) with moc 1.14.1 + mo:core: after
this one substitution both the ECDSA and vetKD examples type-check
clean.

@marc0olo marc0olo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed this against the sources rather than the description: dfinity/control-panel@main (proxy canister, console config, billing docs), dfinity/proxy-canister@main, the IC cycle-cost reference, icp 1.3.0, and a moc compile check.

Verdict: the technical content is sound and it holds up under checking — unusually well. One process blocker, one accuracy nit inline. Both are small next to what's here.

What I verified

The proxy — every structural claim checks out (src/canisters/proxy-canister/{src/lib.rs,proxy-canister.did}):

  • The two-canisters framing is exactly right. Console fork: service : (InitArgs) -> {…} with allowed_ranges, is_authorized = is_controller(caller) || ranges_contain(…). Upstream dfinity/proxy-canister: service : { proxy … }no init args, UnauthorizedUser documented as "not in the controllers list", and no caller prefixing anywhere. So "controllers only / no isolation / raw pass-through" is precise, not a paraphrase.
  • Pitfall 13's "rejected by inspect_message before it reaches replicated state" is exactly the hook's behaviour — it accepts ingress only from controllers, and the fork's own comment says the range check is re-done in proxy because inter-canister calls bypass it.
  • Derivation: derivation_path.insert(0, caller_path) → "first element" ✓. prefix_context = one length byte + principal + context → "length-tagged" ✓. canister_id = None forced on the three *_public_key methods and only those ✓. sign_with_ecdsa genuinely has no canister_id field ✓.
  • available is the liquid balance — the ic-cdk variant is literally CallFailed::InsufficientLiquidCycleBalance, so "freezing reserve already excluded" is right, and it's the kind of detail that's usually wrong.
  • Call::bounded_wait on the relay leg ✓, so the SYS_UNKNOWNCallFailed pitfall follows.

Numbers — all exact: PROXY_MIN_CENTS = 500 ($5) and PROXY_MAX_CENTS = 100_000 ($1000); low_cycle_threshold = 500_000_000_000 (0.5T = the "500 G cycles" default); MAX_FREEZE_WEEKS (4) with the docs noting "a flat global 4 weeks"; EmergencyBalanceCard.svelte carries "Owner-only: when true the panel offers a top-up action", so "owner-only to extend" is right. Fees 26_153_846_153 / 10_000_000_000 and free *_public_key match the IC cycle-cost reference for all three key types.

icp CLI — re-verified independently on 1.3.0: new --subfolder, canister top-up --amount (with the t suffix), cycles mint --icp, cycles balance, canister settings update --add-controller, canister status --id-only plus positional, canister call --proxy/--cycles, deploy --proxy (help text: "route management canister calls through"). Nothing invented.

Motoko — I reproduced the compile check: both worked examples, verbatim, in a minimal actor under moc 1.14.1 + core 2.6.1 → clean. Negative control (Runtime.trap(42)) errors as expected, so the check bites. And mo:core/Debug really has no trap, so de60241 fixed a genuine bug that would have shipped broken copy-paste code.

Evals — the two trigger sets are mutually consistent: no query in both should_trigger sets, no self-contradiction, and the deployment/funding vs signing/calling split holds on both sides. npm run validate: 29 skills, all passed.

Blocker: the trigger evals were changed but not run

Both descriptions were rewritten, and they now sit at 1020 and 1019 bytes against the 1024 cap — i.e. existing wording was trimmed to make room for the new proxy triggers. Nine new queries were added across the two eval files. None of it was run.

The PR body says "Not re-run — … worth a run before this leaves draft", but this PR is not a draft. .claude/CLAUDE.md requires running every eval case added or changed, with baseline, and including the results. Rewriting two descriptions under a hard byte cap by trimming existing wording is the single change most likely to regress existing trigger behaviour, and right now nothing measures whether it did.

node scripts/evaluate-skills.js cloud-engine-canisters --triggers-only and the same for deploy-to-cloud-engine should settle it. Happy to run them if that's easier.

Not blocking

  • Eval case 9 (4/4 vs 3/4). Reporting it as weak rather than tuning it is the right call, and the write-up saying what it does not demonstrate is better practice than a clean number would have been. No change needed.
  • Body size (your open question). deploy-to-cloud-engine 7,144 → 9,555 tokens; cloud-engine-canisters picks up a new warning at 6,629. My read: the worked ECDSA/vetKD examples are the natural references/ candidates — they're material an agent consults once while writing the call, not rules it needs resident — but splitting them here would bury a diff that is currently easy to check against source. Reasonable to land as-is and split separately.
  • The billing table simplifies two caveats. engine-billing-lifecycle.md notes that "a frozen engine can have no way back" (paying the renewal only works while the invoice is payable, i.e. when the freeze came from Stripe), and that a subscription forfeits its emergency balance however it ends. The three-balances table implies renewal always recovers. Probably out of scope, but flagging so it's a deliberate omission rather than an accidental one.

The deliberate omissions section is well judged, incidentally — declining to document cookie-authenticated console endpoints because an agent would try them, and matching the repo's existing precedent for public-only APIs, is exactly the right reasoning.

Comment thread skills/cloud-engine-canisters/SKILL.md Outdated
@marc0olo

Copy link
Copy Markdown
Member

Trigger evals run — one real regression

Ran the two --triggers-only suites on de60241, plus main as the comparison. deploy-to-cloud-engine is clean; cloud-engine-canisters regresses on a pre-existing query.

deploy-to-cloud-engine  (PR)    should-trigger 16/16 | should-not-trigger 10/10
cloud-engine-canisters  (main)  should-trigger  8/8  | should-not-trigger  6/6
cloud-engine-canisters  (PR)    should-trigger 10/11 | should-not-trigger  8/8

All four new deploy-to-cloud-engine triggers pass, both new should_not_trigger entries on cloud-engine-canisters pass, and all three new should_trigger entries there pass. The single failure is an existing query:

❌ "Every outcall through my engine's proxy canister fails with InsufficientCycles
    — can we raise the outcall budget?"
   → selected "deploy-to-cloud-engine" instead

It passes on main and fails on the PR. I ran it twice on the PR to rule out classifier noise — same verdict both times, with the same reasoning: "Funding/topping up the proxy's cycle balance is a deployment/funding concern covered by deploy-to-cloud-engine."

Why this one matters more than the score suggests

That query is the trigger for output eval case 5, "Adversarial: proxied outcalls drain the proxy budget" — whose entire point (Rule 3, pitfall 8) is that an HTTPS outcall should never be on the proxy at all, so the right answer is move the outcall onto your own canister, where it is free, not top up the proxy. Routing it to deploy-to-cloud-engine hands the user precisely the funding answer the adversarial case exists to prevent. 10/11 understates it: this is the one query where the reroute produces a wrong answer rather than a merely different one.

Cause — it is two-sided, and both sides are new in this PR

cloud-engine-canisters now disclaims the case:

…Do NOT use for deploying or funding a proxy (deploy-to-cloud-engine).

(was: "Do NOT use for deploying to an engine (deploy-to-cloud-engine) or outcalls on normal subnets.")

…while deploy-to-cloud-engine now claims it:

…when a proxy must be deployed, funded or topped up; or on which balance to top up…

A query phrased in funding language ("raise the outcall budget") gets pushed away by one description and pulled in by the other. The nuance both now flatten is that an InsufficientCycles on a proxied outcall is a wrong-tool diagnosis, not a funding problem.

The fix needs a trade-off decision, not just a clause

I tried to reclaim it by adding an outcall-diagnosis signal back into cloud-engine-canisters's description. The tightest version I could get — reinstating "an InsufficientCycles there means move the outcall, not fund the proxy", shortening the closing clause to "deploying or funding", dropping "the cost API returns 0 here", and trimming the opening parenthetical — still lands at 1053 bytes against the 1024 cap. Everything left to cut is trigger-bearing: the IC0504 marker, the verbatim 'Unbounded-wait calls…' string, the (Bitcoin, Ethereum, VetKeys) gloss that carries your two new signing triggers, and the ic-vetkeys mention that carries the third.

So the honest finding is that the description is over-subscribed — the new proxy content and the old outcall-diagnosis signal do not both fit in 1024 bytes. Three ways out, and it is your call which:

  1. Narrow deploy-to-cloud-engine's claim instead (my suggestion). It has 5 bytes of headroom, and its own new triggers are explicit about topping up ("How do I top up my engine's proxy canister?"), so tightening "deployed, funded or topped up" so it does not swallow error-diagnosis queries may well hold 16/16. Cheapest thing to try first.
  2. Trim trigger-bearing content from cloud-engine-canisters and re-run — costs coverage elsewhere.
  3. Accept the reroute and delete eval case 5 rather than leave a case whose trigger no longer routes to the skill that answers it correctly. I would not pick this one, but leaving case 5 in place while its query routes elsewhere is the one option that is clearly worse than deciding.

Whichever you pick, worth re-running both suites afterwards — a fix on either side moves the boundary for both.

Everything in my earlier review stands: the technical content verified cleanly against control-panel, proxy-canister, the cycle-cost reference, icp 1.3.0 and a moc compile. This is the only regression the evals surfaced.

…rrect the decode-mechanism claim

Review findings on #363 (marc0olo):

1. Trigger regression — the rewritten descriptions flattened a boundary:
   cloud-engine-canisters disclaimed 'funding a proxy' while
   deploy-to-cloud-engine claimed 'deployed, funded or topped up', so the
   pre-existing diagnosis query 'Every outcall through my engine's proxy
   canister fails with InsufficientCycles — can we raise the outcall
   budget?' rerouted to the funding skill, whose answer (top up) is the
   one the adversarial case exists to prevent. Fixed on both sides:
   cloud-engine-canisters now claims the diagnosis ('its budget drains
   into InsufficientCycles: move the outcall, don't top up') and only
   disclaims deploying; deploy-to-cloud-engine's claim carries the
   repellent inline ('failing proxied calls: cloud-engine-canisters').
   Both suites re-run after the fix: cloud-engine-canisters 11/11 + 8/8,
   deploy-to-cloud-engine 16/16 + 10/10.

2. Decode mechanism — 'an unmodeled field fails the decode' (inherited
   from the proxy's own source comment) over-promises: candid record
   width subtyping means an extra field decodes fine and is silently
   dropped on re-encode; only missing/misnamed fields and unknown
   variant tags fail closed. Reworded per the reviewer's tested
   suggestion.

3. Billing hedge — the operating-budget row now notes recovery is
   possible only while the failed invoice is still payable.
@AntonioVentilii

AntonioVentilii commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

Both findings were real. Addressed in a734022.

1. Decode mechanism (inline thread): applied the suggested wording verbatim. Fail-closed is now scoped to missing or misnamed fields and unknown variant tags, and the width-subtyping gap is named. The upstream lib.rs comment makes the same over-promise; that gets raised on control-panel separately.

2. Trigger regression: fixed on both sides of the boundary, since the cause was two-sided.

  • cloud-engine-canisters no longer disclaims funding ("or funding" dropped from the Do NOT clause) and now claims the diagnosis: "its budget drains into InsufficientCycles: move the outcall, don't top up". To fit the cap it drops the cost-API clause plus two connective trims; IC0504, the verbatim reject string, the Bitcoin/Ethereum/VetKeys gloss, and ic-vetkeys are all kept. 1022 bytes.
  • deploy-to-cloud-engine's claim carries the repellent inline: "when a proxy must be deployed, funded or topped up (failing proxied calls: cloud-engine-canisters)". Paid for by dropping service:version (the body still documents it; service:git:sha carries the trigger), shortening "emergency reserve" to "reserve", and two connective trims. 1021 bytes.

Both suites re-run after the fix:

cloud-engine-canisters  (a734022)  should-trigger 11/11 | should-not-trigger 8/8
deploy-to-cloud-engine  (a734022)  should-trigger 16/16 | should-not-trigger 10/10

The regressed query routes back to cloud-engine-canisters. The two queries most at risk from the fix both held: "Turn on automatic top-up for my engine's proxy" still repels from cloud-engine-canisters despite its description now containing "top up" in negative framing, and "How do I top up my engine's proxy canister?" still triggers deploy-to-cloud-engine despite the narrowed claim.

3. Billing caveat: hedged the operating-budget row: recovery is possible only while the failed invoice is still payable. The forfeit-on-any-end nuance stays out; it belongs with the billing lifecycle, not a deploy skill.

On process: correct. The trigger suites should have been run before opening, and the draft status stopped being a defense when the PR was marked ready. They are run now, on both skills, against the post-fix descriptions.

Replaced with colons, commas, semicolons, or parentheses as each sentence
called for. Pre-existing lines keep their punctuation; the one
pre-existing trigger query that was touched only by a trailing comma
keeps its original text so the reported trigger results stay comparable.
@marc0olo

Copy link
Copy Markdown
Member

Re-checked at 1425124 — both findings closed

Verified independently rather than from the write-up, and re-ran both trigger suites at the actual head rather than at a734022. That distinction turned out to matter (see below), but the result holds:

cloud-engine-canisters  (1425124)  should-trigger 11/11 | should-not-trigger 8/8
deploy-to-cloud-engine  (1425124)  should-trigger 16/16 | should-not-trigger 10/10

The regressed query routes back to cloud-engine-canisters, and the two queries the fix put at risk both hold. Also re-confirmed at head: descriptions at 1022 and 1021 bytes; the decode sentence now scopes fail-closed to missing/misnamed fields and unknown variant tags and names the width-subtyping gap, which matches what I measured; the billing row is hedged with "possible only while the failed invoice is still payable"; both Motoko examples still compile clean on moc 1.14.1 + core 2.6.1 (the style commit did not touch the code blocks); npm run validate 29/29.

On substance this is done. One loose end, from the commit that came after the fix.

1425124 edited eval inputs after the evals were run

The em-dash pass was described as a style change to skill content, but it also touched both eval files: three output-eval prompts, two expected_behaviors, and one trigger query ("…live exchange rates — what…""…live exchange rates: what…").

I covered the trigger query by re-running at head. The three output-eval cases are still reported in the PR description against prompts that no longer exist in the tree. The risk is genuinely low (em dash to colon), but "the edit was trivial" is exactly the reasoning that left the trigger suites unrun in the first place.

Cheapest fix is not to re-run them: revert the eight lines in evaluations/*.json. Nothing about a skill-content style pass required touching eval prompts, and reverting restores correspondence with the results already published in the description at zero cost.

And a note on the em-dash pass itself

It only stripped em dashes from content added in this PR, so both files are now mixed: 57 remain in deploy-to-cloud-engine and 30 in cloud-engine-canisters, all in untouched pre-existing prose. New paragraphs read with colons directly alongside old ones using dashes. The repo at large uses them freely (writing-motoko 65, icp-cli 63), so this is a file-local divergence from house norm rather than a move toward it.

A couple of the substitutions also read worse than what they replaced, e.g. "…must be added as a controller before it can call it: unlike the console proxy, which authorizes…" — a colon does not introduce an "unlike X" contrast the way the dash did.

No objection if this is a deliberate style direction, but it would be better as a separate repo-wide pass than folded into a content PR, where it churns the diff and, here, invalidated eval inputs. Reverting it from this PR would also be a perfectly good answer.

Neither point blocks: the content, the numbers, the CLI surface, the code and the routing all check out.

@marc0olo marc0olo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving.

Both review findings are closed and verified at the head commit, not from the write-up: trigger suites re-run at 1425124 (11/11 | 8/8 and 16/16 | 10/10), descriptions at 1022 and 1021 bytes, the decode sentence matching what I measured against candid 0.10, the billing row hedged, both Motoko examples compiling clean on moc 1.14.1 + core 2.6.1, and npm run validate at 29/29. The regression is gone and nothing else moved.

The two points in my previous comment are entirely your call and explicitly not conditions of this approval:

  • reverting the eight punctuation edits in evaluations/*.json (or re-running those cases) to restore correspondence with the results in the description;
  • whether the em-dash pass stays in this PR at all, given it leaves both files mixed against the rest of the repo.

Neither affects behaviour. Everything load-bearing was re-measured at head; the only thing left unmeasured is punctuation inside three prompts and two grading criteria, which cannot move routing or grading. Take them, leave them, or split them into a follow-up as you prefer.

Nice work on this one. The proxy section in particular held up against control-panel and proxy-canister line by line, which is not the usual outcome when a skill documents an internal service.

…ontrast in eval 19

A colon cannot introduce an 'unlike X' clause; flagged in review of the
em-dash pass. Eval 19 re-run with this text: WITH 3/3, WITHOUT 0/3,
matching the published result.
@AntonioVentilii

Copy link
Copy Markdown
Collaborator Author

Both points acted on.

Eval inputs edited after the runs: correct, and the reasoning you called out is the same mistake twice; I re-ran instead of reverting. Reverting the eight lines would have reintroduced em dashes into files the repo owner has explicitly banned them from, so the durable fix is re-running the five touched cases at head rather than restoring the old inputs. Results, all with baseline:

cec  7  Threshold ECDSA address and signature      WITH 5/5 | WITHOUT 1/5   (published: 5/5 | 2/5)
cec  9  Cleaning up a proxy an app signs through    WITH 4/4 | WITHOUT 3/4   (published: 4/4 | 3/4)
dce 18  Set up a proxy for Bitcoin signing          WITH 4/4 | WITHOUT 0/4   (published: 4/4 | 0/4)
dce 19  Self-deployed proxy onto the engine subnet  WITH 3/3 | WITHOUT 0/3   (published: 3/3 | 0/3)
dce 20  'my engine canister is out of cycles'       WITH 3/3 | WITHOUT 0/3   (published: 3/3 | 0/3)

Four of five match the published numbers exactly. Case 7's baseline moved from 2/5 to 1/5 (the with-skill side is identical); that is judge variance on the baseline run and only widens the reported delta, so the description keeps the published, more conservative pair.

The "unlike" substitution: fixed in 2f1e92f with a comma in the expected_behaviors string of eval 19, and that case was re-run with the corrected text (3/3 | 0/3 above).

On the mixed style: deliberate direction from the repo owner, applied to PR-touched content only so the diff stays reviewable; a repo-wide pass over the remaining pre-existing dashes (and the other skills) is a separate cleanup. The one pre-existing trigger query that had been swept by accident was already restored in 1425124 for exactly the comparability reason you raised.

The PR description's eval section now reports the re-run numbers at head.

@AntonioVentilii
AntonioVentilii merged commit 6546dfb into main Aug 26, 2026
6 checks passed
@AntonioVentilii
AntonioVentilii deleted the av/opencloud-skills-flow-38116d branch August 26, 2026 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants