From d1a8ac9fe5195d307f6f151a55608ceef3f2ed55 Mon Sep 17 00:00:00 2001 From: Carlos Scheidegger Date: Mon, 10 Aug 2026 13:38:19 -0500 Subject: [PATCH] feature-porting process docs (bd-fu16z22k retrospective) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two-phase contract distilled from the project-profiles port (PR #492): Phase 1 (interactive scoping — research fan-out, divergence table, decisions locked with the user, plan doc) and Phase 2 (autonomous implementation — red-first TDD per phase, commit-and-continue at clean boundaries, cross-cutting audit list, E2E through the real binary, ship to a feature-port-labeled PR without merging). Companion append-only file feature-porting-lessons.md holds the pitfalls-and-lessons list (12 entries seeded from this session); Phase 2 agents append to it in their PRs and the review process reviews its diff on every feature-port PR. Also ticks off the final plan items in the project-profiles plan. Co-Authored-By: Claude Fable 5 --- .../instructions/feature-porting-lessons.md | 93 +++++++ claude-notes/instructions/feature-porting.md | 232 ++++++++++++++++++ .../plans/2026-08-10-project-profiles-port.md | 13 +- 3 files changed, 334 insertions(+), 4 deletions(-) create mode 100644 claude-notes/instructions/feature-porting-lessons.md create mode 100644 claude-notes/instructions/feature-porting.md diff --git a/claude-notes/instructions/feature-porting-lessons.md b/claude-notes/instructions/feature-porting-lessons.md new file mode 100644 index 000000000..3e202be31 --- /dev/null +++ b/claude-notes/instructions/feature-porting-lessons.md @@ -0,0 +1,93 @@ +# Feature porting: pitfalls and lessons + +A living, append-only companion to +[`feature-porting.md`](feature-porting.md). Implementing agents +(Phase 2) **append entries as part of their port's PR** whenever they +hit something the process doc didn't predict — a trap, a pattern +worth reusing, a Q1 behavior class that will recur. The review +process **explicitly reviews this file's diff** on every +`feature-port` PR: new entries are part of the deliverable, and a +port that hit visible trouble but added no lesson is a review +question in itself. + +Entry format: a bold one-line claim, then the mechanism and the +move. Tag each entry with the PR it came from. Newest at the bottom; +never rewrite old entries (correct them with a follow-up entry). + +--- + +## From PR #492 (project profiles, bd-fu16z22k) + +- **Q1's schema is load-bearing.** Q1 validates config via YAML + schemas Q2 doesn't have; a closed-object check Q1 got for free + must be hand-written in Q2 (profiles: the `profile:` key shape). + Budget an explicit validation step for any ported config surface. + +- **Q1 env-var mutation is a pattern, not an accident.** Several Q1 + features work by writing env vars (`QUARTO_PROFILE`, dotenv + loading). Q2 policy is *never mutate the process environment* — + carry the value as data, apply to children via `Command::env`, and + special-case any shortcode/API that must see the resolved value in + preference to the real env. + +- **The `q2` bin crate's tracing targets are `q2::…`**, not + `quarto::…` — logging added there was invisible until + `verbose_to_filter` gained a `q2=` directive. The general shape: + when adding logs, verify they actually print at the intended `-v` + level through the real binary before writing a test against them. + +- **`Attr` is a map.** Pandoc-style repeated attributes don't exist + in q2 (`Attr.2` is a `LinkedHashMap`); Q1 semantics relying on + duplicate keys need a redesign (profiles: comma-OR replaced + repeated-attribute OR). + +- **`.local` file order.** Q1's local-override files are + `_quarto.yml.local` (`.yml` *then* `.local`), not + `_quarto.local.yml`. + +- **Signature widening conflicts semantically, not textually.** A + port that adds a parameter to a shared constructor compiles + locally and still fails PR CI if main gained a new caller in the + meantime — git merges cleanly, the merge doesn't compile. + `git fetch && git rebase origin/main` + a full workspace build + **immediately before opening the PR**, and again whenever PR CI + fails in code you didn't touch. + +- **Don't `git add -A` while fixing rebase fallout.** In-progress + local files (this document's ancestor, once) ride along into the + wrong commit. Stage the fix explicitly, or check `git status` + before committing. + +- **Search the skein before designing.** Two existing strands had + already scoped parts of the profiles work and captured a wrinkle + (Q1's dotenv `QUARTO_PROFILE` bootstrap) that fresh research + would have found late or not at all. Prior-art search is a Phase 1 + step, not an optimization. + +- **The stale-binary trap.** `cargo nextest run -p ` rebuilds + test binaries, not `target/debug/q2`; a manual E2E check against + the old binary can "fail" for code that is actually correct (or + worse, "pass" for code that isn't there). Rebuild `-p quarto` + before trusting any manual binary run. + +- **A diagnostic nobody prints is invisible.** Pushing warnings into + a vector proves nothing; verify the vector reaches a CLI print + site, and test the message through the real binary's stderr + (profiles: `config_diagnostics` happened to be printed by both + drivers, but that was checked, not assumed). + +- **Config-driven activation makes features testable everywhere.** + The smoke-all runners can't pass CLI flags, but a feature that can + activate from config alone (`profile.default`) gets exercised by + all three runners — native, WASM, Playwright — with one fixture. + When designing a ported feature, keep a no-flags activation path; + it's also what makes the WASM/hub story work at all. + +- **Concurrent sessions contaminate machine-level evidence.** A + full-workspace test run failed with timeouts caused by another + session's build saturating the machine, and orphan-process counts + were double-attributed for the same reason. Before blaming (or + filing) machine-level symptoms, check `ps` for concurrent test + runs and re-measure alone. (Same session, the flip side: a + reproducible orphan-kernel leak *was* real and became bd-hxhnnlzs + — fixed by another session the same day.) diff --git a/claude-notes/instructions/feature-porting.md b/claude-notes/instructions/feature-porting.md new file mode 100644 index 000000000..958e8ffbc --- /dev/null +++ b/claude-notes/instructions/feature-porting.md @@ -0,0 +1,232 @@ +# Feature porting: Quarto 1 → Quarto 2 + +A two-phase process for porting a Quarto 1 feature to Q2, distilled +from the project-profiles port (bd-fu16z22k, PR #492, +`claude-notes/plans/2026-08-10-project-profiles-port.md`). The +long-term goal is to run Phase 2 as an independent agent workflow; +this document is the process contract both phases follow. + +**The tension every port must resolve:** we want the feature to feel +like Quarto 1, but Q1's weak source-location infrastructure made it +guess at user intent instead of validating. Q2 errs toward strict +validation with good, actionable, span-carrying diagnostics. Phase 1 +exists to decide — with the user — where each feature lands on that +line. Phase 2 executes those decisions without re-litigating them. + +--- + +## Phase 1 — scoping (user + agent, interactive) + +All user involvement happens here. The output is a plan document an +independent agent can execute without asking further questions. + +### 1. Track the work + +- Create a braid strand for the port (`braid create … --json`). +- **Search the skein for prior art before designing anything**: + `braid list` / grep the snapshot for the feature name. In the + profiles port, two existing strands (bd-ev8mk1rp, bd-mlj6) had + already scoped parts of the work and captured wrinkles (e.g. Q1's + dotenv bootstrap) that the fresh research would have found late. + Link them (`related`), and close them at the end if implemented. + +### 2. Research — three parallel investigations + +Run these as parallel background agents; each produces a +self-contained report: + +1. **Q1 implementation** (`external-sources/quarto-cli`): exact + files, functions, line numbers; the real algorithm including + undocumented behavior, silent fallbacks, and bugs (the profiles + port found: whitespace producing empty profile names, silent + mixed-shape groups, an undocumented Posit Connect auto-detect, + `--profile` implemented by env-var mutation). Ask for verbatim + snippets of the core logic — the report must stand alone. +2. **Q1 documentation** (`external-sources/quarto-web`): the + *documented contract* — syntax, precedence, examples worth + reusing as test cases, documented warts (e.g. "metadata-files + are not resolved in profiles"), and gaps where the docs are + silent (those gaps become explicit design decisions). +3. **Q2 architecture** (this repo): where the feature plugs in — + the seams (specific files/functions), existing types to reuse, + diagnostic-code ranges, **terminology collisions** (in the + profiles port, "profile" already meant `DocumentProfile`), and + the print/plumbing paths a new config value must reach. + +Also check **in-flight PRs** that overlap (`gh pr list`): PR #486 +was mid-flight during the profiles port; the plan sequenced the +dependent phase after its merge and everything else before, so +nothing blocked. + +### 3. Synthesize into three artifacts + +- **Divergence table** — one row per behavior where Q2 will differ + from Q1, with the Q1 behavior, the Q2 behavior, and why. This is + the heart of the port: it drives the design questions, becomes + test cases, and ends up (in user-facing form) in the docs page. +- **Strictness list** — every place Q1 is silent where Q2 will emit + a diagnostic, with proposed severity and a new `Q-*` code from the + right subsystem range. +- **Architecture proposal** — the seam(s), new types/fields, and the + cross-cutting surfaces the feature must touch (see the Phase 2 + audit list below), so their cost is visible before approval. + +### 4. Decide with the user + +Bring the genuinely user-owned decisions as structured questions +(AskUserQuestion for crisp choices; prose for nuanced ones), with a +recommendation each. Typical categories: + +- scope (which sub-features now, which deferred to strands); +- fidelity vs. adaptation for each divergence-table row; +- severities (silent / warning / error) for the strictness list; +- naming/CLI surface questions. + +Record every answer in a **"Decisions locked"** section of the plan +with the date. Phase 2 treats these as settled. + +### 5. Write the plan + +`claude-notes/plans/YYYY-MM-DD--port.md`, containing: +overview; terminology warnings; condensed Q1 reference (so Phase 2 +never has to re-research); divergence table; decisions locked; +architecture; **phased checklist where every phase's first item is +its failing tests**; deferred/out-of-scope list with strand IDs; +coordination notes for in-flight PRs. Point +`claude-notes/plans/CURRENT.md` at it and reference it from the +strand. Iterate with the user until they give the go-ahead — that +go-ahead is what authorizes Phase 2's autonomy. + +--- + +## Phase 2 — implementation (agent, autonomous) + +The agent works the plan alone. No design questions back to the +user; if execution reveals that a locked decision is wrong or a +genuinely new decision appears, stop and report rather than +improvise (a workaround that undoes a locked decision means the plan +was not good enough — CLAUDE.md's rule). + +### Execution loop, per plan phase + +1. **Tests first, observed failing.** Write the phase's tests before + its implementation and run them: assertion failures against a + stub or missing wiring, not compile errors. Unit tests for pure + logic; integration tests at the crate level; **binary-driven + tests** (`env!("CARGO_BIN_EXE_q2")`) for anything CLI-visible — + they also solve env-var isolation (`.env_remove()` on the child). +2. Implement to green. +3. **Phase gate**: full workspace `cargo nextest run` (monorepo — + crate-local green is not enough), clippy, fmt, the + `claude-notes/instructions/review.md` checklist. +4. **Commit-and-continue at the clean boundary** — one plan phase + per commit, with a commit message that summarizes behavior, key + decisions, and test evidence. (Policy in CLAUDE.md §Git + Workflow.) +5. Update the plan checklist as items complete; record E2E evidence + inline (exact invocation + observed output snippet). + +### End-to-end verification is not optional + +Tests passing is necessary, not sufficient (CLAUDE.md +§End-to-end verification). Each user-visible phase gets a real +`cargo run --bin q2 -- …` invocation with the output inspected. Two +traps from the profiles port: + +- **Stale binary**: `cargo nextest -p ` rebuilds test + binaries, *not* `target/debug/q2`. Rebuild `-p quarto` before + trusting a manual E2E run — a "failing" feature may just be an + old binary. +- **Real engines**: if the feature touches engine subprocesses, + verify with a live kernel once (a jupyter cell printing the + variable), not just the spawn-site unit test. + +### Cross-cutting audit list + +Q2 features rarely live in one file. For each new config +value/file/flag, sweep these (all bit the profiles port): + +- **Source tracking**: every new file whose values merge into config + must join the `bind_config_source` candidate lists *and* the + `MetadataMergeStage` register closure, or diagnostics degrade to + span-less. Grep `extension_manifest_paths` for the full list of + candidate sites. +- **Cache keys**: new inputs that change render output must join + `Pass1KeyInputs` (count-prefixed, order-sensitive if order is + semantic) or stale pass-1 results get served. +- **All construction sites** of any widened struct (the compiler + finds them — but budget for them; `RenderScriptsContext` had 6). +- **Both pipelines**: native + WASM/preview (`build_transform_pipeline` + and its preview variant; `StageContext`). The WASM leg only + compiles under `cargo xtask verify` — run the full verify before + declaring done. +- **Print paths**: a diagnostic pushed somewhere nobody prints is + invisible; verify the vector you append to reaches the CLI (and + through the real binary, not just in-process). +- **Terminology**: honor collisions identified in Phase 1 + consistently (never bare `profiles` in the port; comments where + both meanings meet, e.g. the cache key). + +### Discovered work + +File it immediately as a strand with `--deps discovered-from:` +and move on — do not scope-creep the port. This includes +infrastructure bugs found en route (the profiles port filed a +jupyter kernel leak that another session fixed the same day) and +sub-features whose sound implementation is bigger than the port +needs (preview `--profile` threading). + +### Shipping (divergence from the default no-PR policy) + +When the plan is complete and `cargo xtask verify` (full, WASM leg) +is green, the agent — **without further permission** — does: + +1. Push the work to a feature branch + (`feature/-`), never to `main`. +2. Open a PR labeled **`feature-port`**. The label is the workflow + tag: a separate review process picks these up, reviews, merges, + and closes the strand. The implementing agent does *not* merge. +3. The PR body carries the full session summary — the same report + the agent would give the user: a **commit-per-phase table**, + design highlights (precedence rules, policy compliance, + terminology, strictness upgrades), the test plan with counts and + red-first evidence, E2E evidence, gates run, incidental fixes, + and the deferred-work strand list. The PR is the durable record; + assume the reviewer has not seen the session. +4. Comment the PR URL on the strand; leave the strand open for the + review process to close. + +### Handoff checklist (what must exist when Phase 2 ends) + +- [ ] PR open, labeled `feature-port`, body = full summary +- [ ] One commit per plan phase, each with workspace-green evidence +- [ ] Plan doc fully checked off, E2E evidence recorded inline +- [ ] Docs page(s) under `docs/` rendered with q2 and inspected, + including a user-facing "Differences from Quarto 1" section +- [ ] A smoke-all fixture if the feature can activate without CLI + flags (config-driven activation exercises all three runners, + including WASM) +- [ ] Deferred-work strands filed and linked; superseded strands + closed +- [ ] New pitfalls/lessons appended to + `feature-porting-lessons.md` (or a conscious "nothing new") +- [ ] Strand updated with the PR URL + +--- + +## Pitfalls and lessons + +The accumulated pitfalls-and-lessons list lives in its own +append-only file: +[`feature-porting-lessons.md`](feature-porting-lessons.md). + +- **Phase 1**: read it before designing — several entries are + Q1-behavior classes that recur across features. +- **Phase 2**: read it before implementing, and **append an entry + (tagged with the PR) whenever the port hits something this + process doc didn't predict**. The addition ships in the port's + PR. +- **Review process**: explicitly review that file's diff on every + `feature-port` PR. New entries are part of the deliverable; a + port that visibly hit trouble but added no lesson is itself a + review question. diff --git a/claude-notes/plans/2026-08-10-project-profiles-port.md b/claude-notes/plans/2026-08-10-project-profiles-port.md index 8271ce5a4..113391c4f 100644 --- a/claude-notes/plans/2026-08-10-project-profiles-port.md +++ b/claude-notes/plans/2026-08-10-project-profiles-port.md @@ -426,10 +426,15 @@ waits; nothing else blocks. (preview watch/restart on profile-config change), bd-47hhbmaj (auto-gitignore /_*.local). bd-mlj6 and bd-ev8mk1rp closed as implemented. -- [ ] Full gates: `cargo xtask verify` (full, WASM leg) — running; - commit after green -- [ ] Write the "feature porting" process doc (session - retrospective) — on request, per the session brief +- [x] Full gates: `cargo xtask verify` (full, WASM leg) green; + PR #492 opened (label `feature-port`), CI green after one + rebase-fallout fix (semantic conflict with a concurrently + landed test), merged as 73064984. Strand bd-fu16z22k closed. +- [x] "Feature porting" process doc written: + `claude-notes/instructions/feature-porting.md` (two-phase + contract) + `feature-porting-lessons.md` (append-only + pitfalls-and-lessons file, 12 entries seeded from this + session) ## Open questions for plan iteration