docs(ai): add the local-stack workflow for multi-PR specs - #13770
Open
artkorotkikh-dfinity wants to merge 16 commits into
Open
docs(ai): add the local-stack workflow for multi-PR specs#13770artkorotkikh-dfinity wants to merge 16 commits into
artkorotkikh-dfinity wants to merge 16 commits into
Conversation
A spec that plans several atomic PRs has no page describing how to build it. The gap shows up as soon as you try: the feature only works once the last PR exists, but each PR has to land on its own, and nothing said where the whole thing gets assembled or tested. Records the linear stack — one branch per planned PR, each cut from its parent, so the tip is the complete feature and needs no integration branch. Adds the two rules that keep it honest: a fix goes to the branch that owns the code and then cascades, and syncing is a merge, never a rebase. Also adds the tips spec's build plan in its asset folder, where Step 7 already removes it once the feature ships, so the tracker cannot go stale in main. It carries two findings from the repo that shrink the spike: spender_subaccount is already present on TransferFromArgs in the vendored cycles-ledger types and in the frontend SDK, so open question 1 is behavioural only. The docs/ai/README.md map was missing the whole spec-driven-development subtree, which would have left the new page unreachable from AGENTS.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 4164efc. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds documentation for building spec-driven features as a linear local branch stack (one branch per planned PR) and records the first concrete multi-PR build plan (tips-via-link) using that convention, wiring both into the existing spec-driven workflow docs and the docs map.
Changes:
- Documented the “local stack” approach for multi-PR specs (
stacked-builds.md), including naming and sync/fix workflows. - Added a per-spec stacked build tracker example for the tips-via-link spec (
build-plan.md). - Linked the new guidance into the spec-driven workflow and
docs/aidocumentation map.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/ai/spec-driven-development/workflow.md | Links the build step to the new stacked-build guidance for multi-PR specs. |
| docs/ai/spec-driven-development/stacked-builds.md | New guide describing linear stacked branches, operations (add/fix/sync), and remote/CI expectations. |
| docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md | New concrete stacked build plan/tracker for the tips-via-link spec, including spike checklist and local rig commands. |
| docs/ai/README.md | Updates the docs map to include PRODUCT.md and the spec-driven-development subtree. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ├── README.md ← you are here | ||
| ├── governance.md Truth hierarchy, boundaries, capabilities, meta-update rule | ||
| ├── pr-and-ci.md PR title regex, body template, CI cheatsheet, local gates | ||
| ├── PRODUCT.md Living description of all current product behaviours |
Open question 1 is answered at the interface level: the ledger candid the local replica installs declares TransferFromArgs.spender_subaccount and all three icrc2_* methods, in both implementations that matter — the ICRC-1 ledger shared by ckBTC, ckETH and ckUSDC, and the ICP ledger. ApproveArgs also carries expected_allowance and expires_at, which is what lets the allowance hold the tip's own deadline. That resolves the ckETH pending decision: all four v1 candidates expose ICRC-2, so ckETH can join v1. What is left is behaviour rather than interface — a declared field is not a working one — so the spike shrinks to the live approve/transfer_from round-trip, expected_allowance on replace, and a read-only check that mainnet runs a version matching these candids. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The behavioural half of open question 1 is answered, driven by two dfx identities against the local ckBTC ledger. The load-bearing result is that a spender subaccount genuinely scopes an allowance, three ways: the allowance for the bare spender reads zero, transfer_from without a spender_subaccount is refused, and so is transfer_from with a different one. One tip therefore cannot draw on another tip's allowance even with the same sender and the same spender canister, which is what makes per-tip reservations safe under a single backend principal. Also measured rather than assumed: the payout fee comes out of the allowance rather than on top of it; a stale expected_allowance fails with the current value attached, so a retry can reconcile; approve replaces rather than adds; and expires_at is enforced by the ledger, with a past deadline rejected outright. Only the read-only mainnet check remains. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Read-only queries confirm every v1 candidate reports ICRC-2 in production, so the token list is all five the spec named: ICP, ckBTC, ckETH, ckUSDC and ckUSDT. PR-0 is complete and nothing in the stack is blocked on the mechanism. One trap found on the way: dfx.json maps ckusdc_ledger's ic entry to yfumr-cyaaa-aaaar-qaela-cai, which answers icrc1_symbol with ckSepoliaUSDC — the testnet ledger, on mainnet. Production ck-ERC20 ids live in tokens.ckerc20.json, which is what the frontend reads. ICP, ckBTC and ckETH are the ledgers their dfx.json ic entries claim; the ck-ERC20 ones are not, so the token picker must not source them there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tracker still said "not started" for all six branches. It now carries each branch's head, the seventh (the flag flip), and why the claim page had to leave the `(app)` group — the layout guard was discarding it for exactly the visitor it serves. Splits the leftovers by who can act. Analytics and the `Uncovered` history status are code. The fragment surviving Internet Identity in an in-app webview, a fresh anchor seeing its token, and a claim crossing a canister upgrade are assertions nothing has measured yet — kept separate from the sign-offs, because "blocked on a person" and "nobody has checked" fail very differently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tracker had the stack table and the spike results but nothing about how the work reaches be1, and the two mechanics that produced wrong diagnoses were only in a conversation. Adds four sections: - **Deploying to be1 / fe1** — the three commits the deploy branch carries and must never leak into a feature PR, the dispatch commands, and why the frontend must go first: adding a case to a variant the canister *returns* is breaking, and three of ours are. - **Mechanics that cost real time** — the vetKD key name frozen by `Cell::init` at first init, `ECDSA_KEY_NAME` doubling as the vetKD key name with `test_be_*` falling through to a local-only key, the cost of swallowing a failure silently, and the tooling that reflows `backend.did` behind your back. - **Open decisions** — five, each with the default that applies if nobody answers. - **Where be1 stands** — deployed, tips working, recovery still blocked on a reserved-cycles limit that needs a controller, with both remedies written out. Also refreshes the branch heads (241 commits over main now), and marks analytics and `Uncovered`-in-History as done and superseded respectively rather than deleting the original notes, so the reasoning stays readable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three sections, all of them things that were only in my head or in a chat log. **Before these can become PRs** — per-branch gate status, measured branch by branch rather than from the top of the stack, which hides what the lower branches are missing. Branches 1-3 are CI-clean; 4-7 fail `npm run test` at the tsc step, which means CI goes red having run no tests at all. Names the three spec type errors, which branch owns each, and the fix already proven in the tree for one of them. **Merging main** — the memory-id collision and how it was settled, so the next reader does not rediscover that `MemoryId::new(20)` was claimed twice while this branch was away. Three conflicts remain unattempted. **be1's memory pin** — the deploy branch now carries a fourth deploy-only commit, so the invariant to check before pushing it is four files rather than three. Records why: the alternative is a reinstall, and a reinstall wipes every profile, contact and note on a shared environment, not just tips. And that it defers the reinstall rather than removing it. Branch heads refreshed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four of the five confirmed what was already built and needed no code. The rate-limit one did: leaving the vetKey tiers alone is only defensible if meeting one is legible, and it was not — a limit read as a generic failure whose copy said 'try again', which is the one thing that cannot work until the window passes. Also records that the type errors and the main merge are done, and that bare 'main' is an ambiguous refname in this repo, since every size in the table depends on which ref was measured.
The upgrade test that landed on branch 1 covers the part that was genuinely unmeasured — that tips survive an upgrade of the regions they were renumbered into — but not the lost callback open question 4 names. pocket-ic 15 cannot reach it: upgrade_canister drives rounds to answer its own install_code ingress and cannot while the claim's ingress is outstanding, and awaiting the claim to unblock it completes the claim. Recorded with the round table and the exact error so the next person does not rediscover it.
Sizes and gates re-run per branch, plus the full suite once from the top. The one finding worth carrying: run tsc per branch, not just check and the targeted specs. A spy returning undefined where a symbol was expected passed lint, svelte-check and vitest, and failed only the spec typecheck — on three branches at once. Also retires the be1 invariant as written. Branch 7 has moved far past the point be1 was cut from, so that diff no longer means anything; the check that matters is that no deploy-only content reached the stack, which it has not.
Reviewed on 30 Aug and cleared, so branches 3, 4 and 5 are no longer waiting on the undrawn states or the light theme.
The compliance sign-off was carried from the spec's open questions and repeated as the longest-lead blocker without anyone testing whether it deserved that weight. It does not, and the evidence is in this repo: the frontend already calls approve in swap, ck conversions, trade deposits and crypto-pay, and tips grants a narrower authority than any of them — exact amount, per-tip subaccount, revocable, and capped at seven days by the canister as well as the ledger. Records the one thing that is genuinely different (OISY's own backend as the spender, standing for days rather than seconds), corrects the spec's claim that this backend already uses transfer_from, and names the lever if anyone objects. With design signed off and the Uncovered wording resolved in code, nothing in that section gates a PR any more.
Recorded that the mid-flight upgrade case was not expressible in pocket-ic 15. It is — CI hits it on every run. It is not controllable, which is a different thing: locally the claim completes before the upgrade, on CI the upgrade lands between the ledger call and its reply and the callback dies. The local evidence was real but was one scheduling, not the only one. The test now asserts what holds on both, and the end state is identical either way, so the lost-callback path is exercised rather than argued about.
icrc1-ledger.wasm.gz is a symlink to a fixture download-immutable.sh caches, and it landed here in dc409e1 via a broad git add. The tips backend branch adds it to .gitignore alongside the other ledger fixtures; this branch does not carry that change, so nothing stopped it. Removed rather than rewritten — it is a symlink to a public fixture, not anything that needs history erased.
A reviewer asked for the rate-limiter rename to land before the backend, so the plan now has a PR at the bottom that is not tips. Also writes down the squash-merge mechanic that makes a stacked PR replay everything below it unless main is merged in first. Sixteen review findings accepted and two declined, with the four that touch the money path named. Two of them were mistakes this doc asserted: the mid-flight upgrade case is reachable, and the reason given for writing our own ICRC-2 client was untrue.
The README map gained an entry on both sides. Kept both, with the flat files together above the directories the way the listing already reads. Also stops icrc1-ledger.wasm.gz coming back. It is a fixture symlink that download-immutable.sh caches, and it has now been swept in twice by a broad git add — removing it once was not enough, because this branch does not carry the .gitignore entry the tips backend branch adds. Added it here so it cannot happen a third time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The tips spec (#13768) plans six atomic PRs, and there is no page saying how a spec like that gets built. The gap is immediate: the feature only works end to end once the last PR exists, but every PR has to land on its own and stay small enough to review. Without a written convention each agent invents its own branch topology, and the ones that look tidiest — siblings off a shared parent — are exactly the ones that need an integration branch rebuilt after every fix.
Changes
docs/ai/spec-driven-development/stacked-builds.md: build a multi-PR spec as a linear stack, one branch per planned PR, each cut from its parent, so the tip of the stack is the whole feature and no integration branch exists. Covers naming, the three operations (add / propagate a fix / sync withmain), how it maps onto stacked PRs on the remote, where the gates run, when to fold a branch into its parent, and where the tracker lives.pr-and-ci.md§7).specs/2026-08-05-feat-tips-via-link/build-plan.md: the tips stack, branch by branch, plus the PR-0 spike checklist, the local rig commands, and what is blocked on people rather than code. It also records two findings that shrink the spike —spender_subaccountis already present onTransferFromArgsin the vendored cycles-ledger types and in the frontend SDK, so open question 1 is behavioural only, not a candid unknown.workflow.md(Step 4 — Build) anddocs/ai/README.md. Meta-update rule: the README map was missing the entirespec-driven-development/subtree, so a new page there would have been unreachable fromAGENTS.md.Atomicity: the general page and the tips build plan are one change. The page prescribes that a per-feature tracker lives in the spec's asset folder — where Step 7 — Post-merge cleanup already deletes it — and the tips plan is the first instance of that convention. Landing the rule without an example, or the example without the rule, would leave the next agent guessing.
Tests
Docs only, so the frontend and backend jobs are skipped by the path filters. Checked by hand:
npm run lintpasses (prettier + eslint, exit 0).../../../../../hops out of the asset folder, which are one level deeper than the spec's own links.dfx 0.26.1is the version pinned indfx.json,scripts/deploy.shdoes deployinternet_identityplus the four v1 ledgers, andscripts/send.tokens.shfunds exactly ckBTC / ckETH / ckUSDC / ICP.spender_subaccountclaims were read out ofsrc/cycles_ledger/types/src/lib.rsand@icp-sdk/canisters/ledger/icrc, not from memory.