Skip to content

[Feature] Add SDK guide docs and multi-agent instruction files for AI-assisted development#1249

Draft
marshacb wants to merge 8 commits into
mainnetfrom
feat/agent-builder-skill
Draft

[Feature] Add SDK guide docs and multi-agent instruction files for AI-assisted development#1249
marshacb wants to merge 8 commits into
mainnetfrom
feat/agent-builder-skill

Conversation

@marshacb
Copy link
Copy Markdown
Collaborator

Motivation

AI coding assistants currently have no guidance for generating Provable SDK code. This adds a generalized builder skill and universal safety rules so they produce correct, secure code by default.

Related PRs

#1245

@iamalwaysuncomfortable
Copy link
Copy Markdown
Member

Review: Agent/MCP Skill Architecture

The content here is solid — the reference docs cover the right topics, the safety rules in INJECT.md are genuinely useful, and the verification scripts are a clever self-check mechanism. My feedback is about the delivery mechanism, not the substance.

Discovery Problem

The .agents/skills/ structure and .claude/INJECT.md are Claude Code conventions. Other agents that developers on this repo are likely using won't discover or parse any of this:

  • Cursor — reads .cursorrules or .cursor/rules/. No concept of .agents/skills/ or SKILL.md frontmatter.
  • GitHub Copilot — reads .github/copilot-instructions.md. Ignores .claude/ entirely.
  • Windsurf — reads .windsurfrules.
  • Cline / Roo Code — reads .clinerules or .roo/rules/.
  • Aider — reads .aider.conf.yml and CONVENTIONS.md.
  • OpenAI Codex CLI — reads AGENTS.md and codex.md.
  • LangChain/CrewAI/AutoGen agents — operate on tool descriptions and system prompts, not filesystem conventions.
  • MCP servers — no file-discovery protocol; would need to be explicitly programmed to look in .agents/skills/.

The !cat ${CLAUDE_SKILL_DIR}/../../.claude/INJECT.md shell injection syntax is Claude Code-specific — every other agent will see it as a broken markdown line.

Duplication Concern

The same safety rules appear in three places: .claude/CLAUDE.md (summary), .claude/INJECT.md (full version), and SKILL.md (references INJECT.md via shell command). Agents that load CLAUDE.md will get the rules, then get them again from INJECT.md. Agents where the shell injection fails get nothing from SKILL.md.

Drift Risk

The reference docs are hand-written API docs with code examples. When the SDK changes (e.g., provingRequest() signature, RecordScanner constructor options), these become wrong with no mechanism to catch it. The verification scripts only check surface patterns via regex, not actual API correctness.

Suggested Approach

1. Multi-agent pointer files — Add thin instruction files for each major agent, all pointing to the same canonical content:

.cursorrules                      → "Read docs/sdk-guide/ before generating SDK code"
.github/copilot-instructions.md   → same pointer
.claude/CLAUDE.md                  → same pointer (already loaded automatically)
.windsurfrules                     → same pointer
AGENTS.md                          → same pointer (Codex)

Each file is 5-10 lines. The actual reference content lives once, in one place. Write once, discover everywhere.

2. Keep the skill structure for Claude Code — It's fine as a power-user feature, just don't treat it as the single source of truth for all agents. Make it a thin pointer to the canonical docs rather than a parallel documentation tree.

3. Inline code documentation — The most drift-proof approach for API-level guidance. JSDoc on ProgramManager.transfer(), ProgramManager.provingRequest(), etc. with usage examples. Every agent that reads source code gets the guidance for free.

4. MCP resource serving — If building MCP servers that help developers use the SDK, the reference content should be served as MCP resources via the protocol, not discovered via filesystem conventions.

TL;DR

The content is valuable. I'd keep the Claude Code skill but make it point to canonical docs rather than maintaining parallel documentation. Add ~5 thin pointer files so Cursor, Copilot, Windsurf, Cline, and Codex users all get the safety rules automatically. This gets you broad coverage for minimal maintenance cost.

@marshacb marshacb changed the title [Feature] Add aleo-sdk-builder skill and INJECT.md for AI-assisted development [Feature] Add SDK guide docs and multi-agent instruction files for AI-assisted development Mar 17, 2026
Copy link
Copy Markdown
Member

@iamalwaysuncomfortable iamalwaysuncomfortable left a comment

Choose a reason for hiding this comment

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

On the first pass, this is great, operationally let's test it out via the MCP server to see how it does in action.

@iamalwaysuncomfortable
Copy link
Copy Markdown
Member

Additional Findings on Close Review

A few more issues beyond the previous comment:

1. estimateDeploymentFee() doesn't exist as a user-facing method

docs/sdk-guide/README.md Fee Management section says:

always use estimateExecutionFee() or estimateDeploymentFee() or estimateFeeForAuthorization()

But estimateDeploymentFee() is only available as WasmProgramManager.estimateDeploymentFee(programString, imports) — it's an internal WASM static method, not exposed on ProgramManager. Users can't call pm.estimateDeploymentFee(). The deployment methods handle fee estimation internally. Either remove estimateDeploymentFee() from the guidance, or note that deployment fee estimation is handled automatically by buildDeploymentTransaction.

2. Inconsistent .toString() on transaction submission

The docs inconsistently call .toString() before submitting:

  • transfers.md: await pm.networkClient.submitTransaction(tx) — no .toString()
  • delegated-proving.md: await pm.networkClient.submitTransaction(tx.toString()) — with .toString()
  • deployment.md: both patterns used in the same file

submitTransaction accepts Transaction | string, so both work. But an agent seeing both patterns may be confused about which is correct. Pick one and be consistent — since buildExecutionTransaction returns a Transaction object, just pass it directly without .toString().

3. getTransaction() throws, doesn't return null

The transaction confirmation polling pattern in README.md uses:

const result = await client.getTransaction(txId);
if (result) { confirmed = true; break; }

But getTransaction() throws on failure (HTTP error / not found), it doesn't return null. The catch block handles this correctly, but the if (result) check is misleading — if the call succeeds, you always have a result. The pattern works because the throw-catch flow is what actually drives the retry, but it reads like a null-check pattern. Minor, but could confuse an agent into thinking it needs to check for null rather than catch.


These are smaller than the PrivateKeyCiphertext issue from the previous review, but the estimateDeploymentFee() one will cause agents to search for a method that doesn't exist on ProgramManager.

Copy link
Copy Markdown
Member

@iamalwaysuncomfortable iamalwaysuncomfortable left a comment

Choose a reason for hiding this comment

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

Code Accuracy Review

Verified all code examples against the actual SDK source (sdk/src/program-manager.ts, sdk/src/network-client.ts, sdk/src/account.ts, sdk/src/record-scanner.ts, sdk/src/keys/provider/memory.ts). The structure and approach are great — inline comments flag the specific lines where the documented API diverges from the actual implementation.

Must-fix (will generate broken code):

  • PrivateKeyCiphertext API is fabricated — methods don't exist
  • estimateDeploymentFee() referenced as user-facing but it's an internal WASM-only static method

Should-fix (inconsistent/misleading):

  • Inconsistent .toString() before submitTransaction() across examples
  • getTransaction() throws on not-found, doesn't return null — polling pattern is misleading
  • buildDeploymentTransaction return type inconsistency (sometimes .toString(), sometimes not)

Comment thread docs/sdk-guide/keys-and-crypto.md Outdated
import { PrivateKeyCiphertext } from "@provablehq/sdk/testnet.js";

// Encrypt for storage
const encrypted = PrivateKeyCiphertext.encryptPrivateKey(privateKey, password);
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.

Bug: This API does not exist. PrivateKeyCiphertext does not have an encryptPrivateKey static method.

The actual SDK API (from sdk/src/account.ts):

// Encrypt
const encrypted = privateKey.toCiphertext(password);
const encryptedString = encrypted.toString();

// Decrypt
const restored = PrivateKey.fromPrivateKeyCiphertext(
    PrivateKeyCiphertext.fromString(encryptedString),
    password
);

An agent following this doc will generate code that throws TypeError: PrivateKeyCiphertext.encryptPrivateKey is not a function.

Comment thread docs/sdk-guide/README.md Outdated

### Fee Management

- NEVER hardcode fee amounts — always use `estimateExecutionFee()` or `estimateDeploymentFee()`
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.

Bug: estimateDeploymentFee() is not a user-facing method. It only exists as WasmProgramManager.estimateDeploymentFee(programString, imports) — an internal static method on the WASM module. It's not exposed on ProgramManager or AleoNetworkClient.

buildDeploymentTransaction calls it internally to estimate fees automatically. An agent searching for pm.estimateDeploymentFee() or client.estimateDeploymentFee() will find nothing.

Suggested fix: Remove estimateDeploymentFee() from this list, or note that deployment fee estimation is handled internally by buildDeploymentTransaction.

Comment thread docs/sdk-guide/README.md Outdated
for (let i = 0; i < 60; i++) {
try {
const result = await client.getTransaction(txId);
if (result) { confirmed = true; break; }
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.

Misleading pattern. getTransaction() throws on HTTP error / not-found — it never returns a falsy value on success. The if (result) null-check is dead code; if the call succeeds, result is always truthy.

The retry logic actually works because the catch block swallows the throw, but an agent reading this will think it needs to check for null. Consider:

try {
    const result = await client.getTransaction(txId);
    confirmed = true; break;
} catch { /* not confirmed yet */ }

Comment thread docs/sdk-guide/delegated-proving.md Outdated
feeAuthorization,
});

const txId = await pm.networkClient.submitTransaction(tx.toString());
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.

Inconsistency: .toString() used here but not elsewhere. This line calls tx.toString() before submitting, but deployment.md:27 and transfers.md:57 pass the Transaction object directly.

submitTransaction() accepts Transaction | string, so both work, but the inconsistency across guide files will confuse agents about which pattern is correct. Recommend picking one approach (preferably passing the Transaction directly without .toString()) and using it consistently.

Comment thread docs/sdk-guide/deployment.md Outdated
priorityFee: 0,
privateFee: false,
});
await pm.networkClient.submitTransaction(execTx.toString());
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.

Same .toString() inconsistency — this line calls execTx.toString() but line 27 above passes tx directly without .toString(). Pick one pattern.


```bash
cargo install leo-lang
leo devnode start --private-key APrivateKey1zkp8CZNn3yeCseEtxuVPbDCwSyhGW6yZKUYKfgXmcpoGPWH
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.

Note: This is a well-known devnet key so it's fine functionally, but the verify-execution.ts script in this same PR will flag this line with "SECURITY: Hardcoded private key detected." Consider adding a comment like # Standard devnet-only key — not used on mainnet so agents (and the verification script) understand it's intentional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants