diff --git a/.trellis/spec/backend/direct-edit-api.md b/.trellis/spec/backend/direct-edit-api.md index 8ea4ff3..1dc9b80 100644 --- a/.trellis/spec/backend/direct-edit-api.md +++ b/.trellis/spec/backend/direct-edit-api.md @@ -94,3 +94,56 @@ saveEditDocument(activeEdits, { ...draft, documentVersion: current.documentVersi The service checks the version before the atomic version-bumping write, while the pure resolver remains reusable by preview and formal compilation. + +## 8. Left and right + +`left` is `heading - 90`. Three places agree and must keep agreeing: + +- `offsetLine()` (`native-road.js`) offsets a positive value counter-clockwise + from the direction of travel, i.e. toward geographic left. +- `centerlineShift = (edgeOffsets.left - edgeOffsets.right) / 2`, so widening the + left edge moves the centerline left. +- Sidewalks use `heading + (side === 'left' ? -90 : 90)`. + +The handle manifest reports `axisAzimuth = tangent + 90` for both sides, which is +therefore *right*. Handle placement must offset the left handle by `tangent - 90`. + +**Regression**: `makeRoadHandles()` once placed the left handle at `tangent + 90`, +drawing it over the right kerb, so dragging the visually-left handle moved the +right edge. Assert sides by geography (a north-heading road's left handle is west +of its centerline), never by axis sign — a sign convention can be wrong on both +sides of the wire at once and still look self-consistent. + +## 9. What the geometry stage actually reads + +`compileGeometry()` honours only four profile fields: + +| Field | Read at | +| --- | --- | +| `edgeOffsets.left/right` | `native-road.js` centerline shift | +| `widthMeters` | road width | +| `sidewalkWidths.left/right` | `sidewalkRing()` | +| `laneDividerOffsets` | lane separator placement | + +`profile.interval` and `profile.transitions` are written by the solver and read by +nothing: every direct edit currently applies to the whole road. Do not add UI for +interval-scoped editing until that changes — see +`research/interval-not-applied.md` in the map-editor task. + +## 10. Saving is not applying + +`POST /api/edits` writes the v2 document only. `/api/state` serves the outputs of +the last compile, so a save without a recompile leaves a refresh showing pre-edit +geometry — indistinguishable from a failed save. + +`compileInput()` is the only caller that passes `editsFile`, and +`session.context.compileFresh` must be installed by **every** path that sets +`session.area`, including `/api/import`. It was originally wired only when the +server started with an area on the command line, so every UI-imported session +answered `请先导入 OSM 文件` to `/api/compile`. + +An identity test cannot catch a `editsFile` regression: an ignored document and an +empty one produce identical output. Assert that a **non-empty** document changes +the compiled output. Wiring gaps between two handlers need an HTTP-level test, and +a regression test is only trustworthy once you have watched it fail without the +fix. diff --git a/.trellis/spec/frontend/direct-edit-client.md b/.trellis/spec/frontend/direct-edit-client.md new file mode 100644 index 0000000..4bde685 --- /dev/null +++ b/.trellis/spec/frontend/direct-edit-client.md @@ -0,0 +1,164 @@ +# Direct Edit Client + +> Trigger: any change under `workbench/client/src/edit/`, `MapCanvas.tsx`, or the +> direct-edit wiring in `App.tsx`. Read this before editing handles, ghosts, +> previews, or the save path. + +Related: [Direct Edit Solver And API](../backend/direct-edit-api.md) for the wire +contract this client consumes. + +--- + +## 1. Layer ownership + +Four layer groups, and only three of them are ever written by the client: + +| Group | Written by | Rule | +| --- | --- | --- | +| baseline (`createLayers()`) | `updateLayers()` only | **Never** written by edit code | +| `editHandles` | `EditHandleLayer.render()` | Handle features carry only `handleId` | +| `editGhost` | `EditGhostLayer` | Client-side, non-authoritative | +| `editPreview` | `EditPreviewLayer.show()` | Server geometry; **hides** baseline, never overwrites it | + +`EditPreviewLayer` builds its own layer set from the same `createLayers()` factory +so preview and baseline cannot drift in styling, and `show()` hides only the +baseline layers it actually supersedes. `clear()` restores visibility from the +layer switches, never from a hardcoded default. + +**Forbidden**: writing compiler output into a baseline source. It is the invariant +the whole split exists to protect, and nothing in the type system enforces it. + +--- + +## 2. A handle carries nothing but its id + +Handle features hold `handleId` and nothing else. Kind, anchor, axis, range and +disabled reason are looked up in the manifest. That is why `EditHandleLayer` owns +both the source and the index — they cannot fall out of step. + +**Forbidden**: copying manifest fields onto the feature. It creates a second +constraint model on the client, which is the failure the cross-layer guide warns +about. + +--- + +## 3. Handle position comes from the value, never the cursor + +`handlePositionFor(handle, value)` is the inverse of `projectHandleValue()`. A +drag projects onto the manifest's axis, clamps to the manifest's range, and the +handle is then placed from the clamped value. + +This is not cosmetic. The ol-ext probe translated its proxy by the raw pointer +delta, and a drag reading `-24.146 m` left the handle 24 m out while the +constraint clamped at `-5.400 m` — the handle pointed at a road shape that cannot +exist. Any input adapter must own the position update for this reason. + +--- + +## 4. Sides: left is `tangent - 90` + +The manifest reports `axisAzimuth = tangent + 90` for **both** sides, and that +azimuth is the geometry compiler's *right*. See the backend spec for why. On the +client this means `outwardSign()` negates the left side, and a test that asserts a +side must name the direction geographically (`OUTWARD_LEFT` / `OUTWARD_RIGHT`) +rather than by axis sign. + +**Mistake made**: the first implementation assumed left was the positive axis, so +dragging the visually-left handle moved the right kerb. Both the server placement +and the client sign were wrong together, so neither side's tests caught it. + +--- + +## 5. A constraint must travel with its operation + +`validateEditDocument()` rejects any constraint whose `provenance.operationId` is +not a recorded operation, and the check covers already-saved constraints too. +Build the pair with `draftConstraint()` + `operationFor()`, and send +`EditSession.fragment()`, which assembles both halves. + +One gesture mints one operation id, released on pointerup. Reusing an id across +gestures puts two operations with the same id in the document, which is also +rejected. + +**Mistake made**: sending `constraints` alone returned HTTP 400 on every drag. + +--- + +## 6. Never compute inside a `setState` updater + +React defers updater functions and, under StrictMode, calls them more than once. +By the time one runs, closure variables captured during the gesture may already be +cleared. + +```ts +// WRONG — threw `toLonLat(null)` and white-screened the page +setReport((current) => ({ ...current, meters: projectHandleValue(handle, start!, now) })); + +// RIGHT — compute in the event handler, pass plain values in +const meters = projectHandleValue(handle, start, now); +setReport((current) => ({ ...current, meters })); +``` + +The `start!` non-null assertion is what hid the runtime problem from the type +checker. The same rule covers reading live OpenLayers state inside an updater: it +samples a different moment than the event did. + +--- + +## 7. Per-frame feedback does not go through React + +The ghost writes its OpenLayers source directly. Driving a readout from state on +every `pointermove` produced ~1600 renders in a single probe session. +`requestAnimationFrame` coalescing is the minimum; writing straight to the source +is the rule for production. + +--- + +## 8. `EditSession` owns the constraint set, and is mutated in place + +`EditSession` holds the saved baseline, its operations, the command stack, and the +`previewSeq` watermark. `PreviewRequester` captures the session object once, so +adopting a server document uses `load()` rather than constructing a new session — +a swapped object would leave the requester arbitrating for a session nobody reads. + +`previewSeq` arbitration lives in `acceptPreview()`, not in the request layer: the +request layer only sends and cancels. The discard rule is worth unit-testing and +must not depend on network timing. + +Undo of a **saved** gesture appends an inverse operation; persisted history is +never rewritten. Undo of an unsaved one just moves the cursor. + +--- + +## 9. The `directEdit` flag means nothing exists + +With the flag off: no edit source is created, no layer is added, no interaction is +registered, and no manifest request is made. The network trace and the canvas must +match `main` exactly. Guard once at construction, not per call site. + +`intervalEditingSupported` is a second, narrower switch: range handles stay hidden +while `compileGeometry()` ignores `profile.interval`. A control whose drag changes +nothing is worse than no control. + +--- + +## 10. Saving is not the same as applying + +`POST /api/edits` writes the document; `/api/state` serves the outputs of the last +compile. Saving without recompiling leaves a refresh showing the pre-edit geometry, +which is indistinguishable from a failed save. Report the save and the recompile +separately — if the recompile fails, the save still succeeded and the message must +say so. + +--- + +## Checklist before committing edit-client changes + +- [ ] No baseline source is written +- [ ] No manifest field is copied onto a feature +- [ ] Handle positions derive from clamped values +- [ ] Sides asserted geographically, not by axis sign +- [ ] Constraints sent with their operations +- [ ] No geometry or OL reads inside a `setState` updater +- [ ] No per-`pointermove` React state updates +- [ ] Flag off ⇒ no sources, no layers, no interactions, no requests diff --git a/.trellis/spec/frontend/index.md b/.trellis/spec/frontend/index.md index a5e51b8..5753c4f 100644 --- a/.trellis/spec/frontend/index.md +++ b/.trellis/spec/frontend/index.md @@ -14,6 +14,7 @@ This directory contains guidelines for frontend development. Fill in each file w | Guide | Description | Status | |-------|-------------|--------| +| [Direct Edit Client](./direct-edit-client.md) | Handle/ghost/preview layer ownership, drag projection, session and save contracts | Filled | | [Directory Structure](./directory-structure.md) | Module organization and file layout | To fill | | [Component Guidelines](./component-guidelines.md) | Component patterns, props, composition | To fill | | [Hook Guidelines](./hook-guidelines.md) | Custom hooks, data fetching patterns | To fill | diff --git a/.trellis/spec/guides/code-reuse-thinking-guide.md b/.trellis/spec/guides/code-reuse-thinking-guide.md new file mode 100644 index 0000000..bb789e9 --- /dev/null +++ b/.trellis/spec/guides/code-reuse-thinking-guide.md @@ -0,0 +1,223 @@ +# Code Reuse Thinking Guide + +> **Purpose**: Stop and think before creating new code - does it already exist? + +--- + +## The Problem + +**Duplicated code is the #1 source of inconsistency bugs.** + +When you copy-paste or rewrite existing logic: +- Bug fixes don't propagate +- Behavior diverges over time +- Codebase becomes harder to understand + +--- + +## Before Writing New Code + +### Step 1: Search First + +```bash +# Search for similar function names +grep -r "functionName" . + +# Search for similar logic +grep -r "keyword" . +``` + +### Step 2: Ask These Questions + +| Question | If Yes... | +|----------|-----------| +| Does a similar function exist? | Use or extend it | +| Is this pattern used elsewhere? | Follow the existing pattern | +| Could this be a shared utility? | Create it in the right place | +| Am I copying code from another file? | **STOP** - extract to shared | + +--- + +## Common Duplication Patterns + +### Pattern 1: Copy-Paste Functions + +**Bad**: Copying a validation function to another file + +**Good**: Extract to shared utilities, import where needed + +### Pattern 2: Similar Components + +**Bad**: Creating a new component that's 80% similar to existing + +**Good**: Extend existing component with props/variants + +### Pattern 3: Repeated Constants + +**Bad**: Defining the same constant in multiple files + +**Good**: Single source of truth, import everywhere + +### Pattern 4: Repeated Payload Field Extraction + +**Bad**: Multiple consumers cast the same JSON/event fields locally: + +```typescript +const description = (ev as { description?: string }).description; +const context = (ev as { context?: ContextEntry[] }).context; +``` + +This is duplicated contract logic even when the code is only two lines. Each +consumer now has its own definition of what a valid payload means. + +**Good**: Put the decoder, type guard, or projection next to the data owner: + +```typescript +if (isThreadEvent(ev)) { + renderThreadEvent(ev); +} +``` + +**Rule**: If the same untyped payload field is read in 2+ places, create a +shared type guard / normalizer / projection before adding a third reader. + +--- + +## When to Abstract + +**Abstract when**: +- Same code appears 3+ times +- Logic is complex enough to have bugs +- Multiple people might need this + +**Don't abstract when**: +- Only used once +- Trivial one-liner +- Abstraction would be more complex than duplication + +--- + +## After Batch Modifications + +When you've made similar changes to multiple files: + +1. **Review**: Did you catch all instances? +2. **Search**: Run grep to find any missed +3. **Consider**: Should this be abstracted? + +### Reducers Should Use Exhaustive Structure + +When state is derived from action-like values (`action`, `kind`, `status`, +`phase`), prefer a reducer with one `switch` over scattered `if/else` updates. + +```typescript +// BAD - action-specific state transitions are hard to audit +if (action === "opened") { ... } +else if (action === "comment") { ... } +else if (action === "status") { ... } + +// GOOD - one reducer owns the transition table +switch (event.action) { + case "opened": + ... + return; + case "comment": + ... + return; +} +``` + +This matters when the event log is the source of truth. A reducer is the +documented replay model; display code and commands should not duplicate pieces +of that replay model. + +--- + +## Checklist Before Commit + +- [ ] Searched for existing similar code +- [ ] No copy-pasted logic that should be shared +- [ ] No repeated untyped payload field extraction outside a shared decoder +- [ ] Constants defined in one place +- [ ] Similar patterns follow same structure +- [ ] Reducer/action transitions live in one reducer or command dispatcher + +--- + +## Gotcha: Python if/elif/else Exhaustive Check + +**Problem**: Python's if/elif/else chains have no compile-time exhaustive check. When you add a new value to a `Literal` type (e.g., `Platform`), existing if/elif/else chains silently fall through to `else` with wrong defaults. + +**Symptom**: New platform works partially — some methods return Claude defaults instead of platform-specific values. No error is raised. + +**Example** (`cli_adapter.py`): +```python +# BAD: "gemini" falls through to else, returns "claude" +@property +def cli_name(self) -> str: + if self.platform == "opencode": + return "opencode" + else: + return "claude" # gemini silently gets "claude"! + +# GOOD: explicit branch for every platform +@property +def cli_name(self) -> str: + if self.platform == "opencode": + return "opencode" + elif self.platform == "gemini": + return "gemini" + else: + return "claude" +``` + +**Prevention**: When adding a new value to a Python `Literal` type, search for ALL if/elif/else chains that switch on that type and add explicit branches. Don't rely on `else` being correct for new values. + +--- + +## Gotcha: Asymmetric Mechanisms Producing Same Output + +**Problem**: When two different mechanisms must produce the same file set (e.g., recursive directory copy for init vs. manual `files.set()` for update), structural changes (renaming, moving, adding subdirectories) only propagate through the automatic mechanism. The manual one silently drifts. + +**Symptom**: Init works perfectly, but update creates files at wrong paths or misses files entirely. + +**Prevention**: +- **Best**: Eliminate the asymmetry — have the manual path call the automatic one (e.g., `collectTemplateFiles()` calls `getAllScripts()` instead of maintaining its own list) +- **If asymmetry is unavoidable**: Add a regression test that compares outputs from both mechanisms +- When migrating directory structures, search for ALL code paths that reference the old structure + +**Real example**: `trellis update` had a manual `files.set()` list for 11 scripts that `getAllScripts()` already tracked. Fix: replaced the manual list with a `for..of getAllScripts()` loop. See `update.ts` refactor in v0.4.0-beta.3. + +--- + +## Template File Registration (Trellis-specific) + +When adding new files to `src/templates/trellis/scripts/`: + +**Single registration point**: `src/templates/trellis/index.ts` + +1. Add `export const xxxScript = readTemplate("scripts/path/file.py");` +2. Add to `getAllScripts()` Map + +That's it. `commands/update.ts` uses `getAllScripts()` directly — no manual sync needed. + +**Why this matters**: Without registration in `getAllScripts()`, `trellis update` won't sync the file to user projects. Bug fixes and features won't propagate. + +**History**: Before v0.4.0-beta.3, `update.ts` had its own hand-maintained file list that frequently fell out of sync with `getAllScripts()`. This caused 11 Python files to be silently skipped during `trellis update`. The fix was to eliminate the duplicate list and use `getAllScripts()` as the single source of truth. + +### Quick Checklist for New Scripts + +```bash +# After adding a new .py file, verify it's in getAllScripts(): +grep -l "newFileName" src/templates/trellis/index.ts # Should match +``` + +### Template Sync Convention + +`.trellis/scripts/` (dogfooded) and `packages/cli/src/templates/trellis/scripts/` (template) must stay identical. After editing `.trellis/scripts/`, always sync: + +```bash +rsync -av --delete --exclude='__pycache__' .trellis/scripts/ packages/cli/src/templates/trellis/scripts/ +``` + +**Gotcha**: Running rsync with wrong source/destination paths can create nested garbage directories (e.g., `.trellis/scripts/packages/cli/...`). Always double-check paths before running. diff --git a/.trellis/spec/guides/cross-layer-thinking-guide.md b/.trellis/spec/guides/cross-layer-thinking-guide.md new file mode 100644 index 0000000..9686546 --- /dev/null +++ b/.trellis/spec/guides/cross-layer-thinking-guide.md @@ -0,0 +1,327 @@ +# Cross-Layer Thinking Guide + +> **Purpose**: Think through data flow across layers before implementing. + +--- + +## The Problem + +**Most bugs happen at layer boundaries**, not within layers. + +Common cross-layer bugs: + +- API returns format A, frontend expects format B +- Database stores X, service transforms to Y, but loses data +- Multiple layers implement the same logic differently + +--- + +## Before Implementing Cross-Layer Features + +### Step 1: Map the Data Flow + +Draw out how data moves: + +``` +Source → Transform → Store → Retrieve → Transform → Display +``` + +For each arrow, ask: + +- What format is the data in? +- What could go wrong? +- Who is responsible for validation? + +### Step 2: Identify Boundaries + +| Boundary | Common Issues | +| --------------------- | --------------------------------- | +| API ↔ Service | Type mismatches, missing fields | +| Service ↔ Database | Format conversions, null handling | +| Backend ↔ Frontend | Serialization, date formats | +| Component ↔ Component | Props shape changes | + +### Step 3: Define Contracts + +For each boundary: + +- What is the exact input format? +- What is the exact output format? +- What errors can occur? + +--- + +## Common Cross-Layer Mistakes + +### Mistake 1: Implicit Format Assumptions + +**Bad**: Assuming date format without checking + +**Good**: Explicit format conversion at boundaries + +### Mistake 2: Scattered Validation + +**Bad**: Validating the same thing in multiple layers + +**Good**: Validate once at the entry point + +### Mistake 3: Leaky Abstractions + +**Bad**: Component knows about database schema + +**Good**: Each layer only knows its neighbors + +### Mistake 4: Every Consumer Parses The Same Payload + +**Bad**: A command reads JSONL events and casts fields inline: + +```typescript +const thread = (ev as { thread?: string }).thread; +const labels = (ev as { labels?: string[] }).labels; +``` + +This looks local, but it means every consumer owns a private version of the +event contract. The next field change will update one command and miss another. + +**Good**: Decode once at the event boundary, then export typed projections: + +```typescript +if (!isThreadEvent(ev)) return false; +return ev.thread === filter.thread; +``` + +**Rule**: For append-only logs, JSON streams, RPC payloads, or config files, +create one owner for: + +- event / payload type definitions +- type guards and normalization from `unknown` +- metadata projections used by UI commands +- reducers that replay state from the source of truth + +Rendering code may format fields, but it must not redefine the payload contract. + +--- + +## Checklist for Cross-Layer Features + +Before implementation: + +- [ ] Mapped the complete data flow +- [ ] Identified all layer boundaries +- [ ] Defined format at each boundary +- [ ] Decided where validation happens + +After implementation: + +- [ ] Tested with edge cases (null, empty, invalid) +- [ ] Verified error handling at each boundary +- [ ] Checked data survives round-trip +- [ ] Checked that consumers import shared decoders / projections instead of + casting payload fields locally +- [ ] Checked that derived state points back to the source event identifier + (`seq`, `id`, `version`) instead of inventing a second cursor + +--- + +## Cross-Platform Template Consistency + +In Trellis, command templates (e.g., `record-session.md`) exist in **multiple platforms** with identical or near-identical content. This is a cross-layer boundary. + +### Checklist: After Modifying Any Command Template + +- [ ] Find all platforms with the same command: `find src/templates/*/commands/trellis/ -name ".*"` +- [ ] Update all platform copies (Markdown `.md` and TOML `.toml`) +- [ ] For Gemini TOML: adapt line continuations (`\\` vs `\`) and triple-quoted strings +- [ ] Run `/trellis:check-cross-layer` to verify nothing was missed + +**Real-world example**: Updated `record-session.md` in Claude to use `--mode record`, but forgot iFlow, Kilo, OpenCode, and Gemini — caught by cross-layer check. + +--- + +## Generated Runtime Template Upgrade Consistency + +Some generated files are both documentation and runtime input. In Trellis, +`.trellis/workflow.md` is parsed by `get_context.py`, `workflow_phase.py`, +SessionStart filters, and per-turn hooks. Template changes must be validated +against both fresh init and upgrade paths. + +### Checklist: After Modifying A Runtime-Parsed Template + +- [ ] Identify every runtime parser that reads the template, not just the file + writer that installs it +- [ ] Check whether relevant syntax lives outside obvious managed regions + such as tag blocks +- [ ] Verify fresh `init` output and a versioned `update` scenario that writes + the older `.trellis/.version` +- [ ] Add an upgrade regression using an older pristine template fixture, then + assert the installed file reaches the current packaged shape +- [ ] Update the backend spec that owns the runtime contract + +--- + +## Versioned Documentation Boundary + +Versioned documentation is a cross-layer boundary: source paths, `docs.json` +version routing, and the rendered version selector must all describe the same +release line. + +### Checklist: Before Editing Versioned Docs + +- [ ] Identify the target release line: stable, beta, or RC +- [ ] Verify the edited MDX path matches that line: + - stable: `docs-site/{start,advanced,...}` and `docs-site/zh/{start,advanced,...}` + - beta: `docs-site/beta/**` and `docs-site/zh/beta/**` + - RC: `docs-site/rc/**` and `docs-site/zh/rc/**` +- [ ] Verify `docs.json` navigation points the version label to the same paths +- [ ] Grep the opposite tree for release-line-specific terms before committing +- [ ] Treat beta content appearing under root release paths as a source-path bug, + not a rendering bug + +**Real-world example**: A beta-only task workflow change documented +`prd.md` + `design.md` + `implement.md`, task-creation consent, and Codex +mode banners under root `start/` and `advanced/` paths. The docs site then +served 0.6 beta behavior under the Release selector. The fix was to restore root +release docs, move the 0.6 content to `beta/` and `zh/beta/`, and add a grep +audit for beta markers against the root release tree. + +**Real-world example**: Codex inline mode changed workflow platform markers from +`[Codex]` / `[Kilo, Antigravity, Windsurf]` to `[codex-sub-agent]` / +`[codex-inline, Kilo, Antigravity, Windsurf]`. Fresh init was correct, but +`trellis update` only merged `[workflow-state:*]` blocks and preserved stale +markers outside those blocks. Result: upgraded projects got new hook scripts +but old workflow routing, so `get_context.py --mode phase --platform codex` +could return empty Phase 2.1 detail. + +--- + +## Mode-Detection Probe Checklist + +When a CLI auto-detects a mode by probing a remote resource (e.g., checking if `index.json` exists to decide marketplace vs direct download): + +### Before implementing: + +- [ ] Probe runs in **ALL** code paths that use the result (interactive, `-y`, `--flag` combos) +- [ ] 404 vs transient error are distinguished — don't treat both as "not found" +- [ ] Transient errors **abort or retry**, never silently switch modes +- [ ] Shared state (caches, prefetched data) is **reset** when context changes (e.g., user switches source) +- [ ] **Shortcut paths** (e.g., `--template` skipping picker) must have the same error-handling quality as the probed path — check that downstream functions don't call catch-all wrappers + +### After implementing: + +- [ ] Trace every path from probe result to the mode-decision branch — no fallthrough +- [ ] External format contracts (giget URI, raw URLs) are tested or at least documented as comments +- [ ] Metadata reads consume a complete response or use a streaming parser — never parse a fixed-size prefix as full JSON +- [ ] When reconstructing a composite identifier from parsed parts, verify **all** fields are included and in the **correct position** (e.g., `provider:repo/path#ref` not `provider:repo#ref/path`) +- [ ] Verify that **action functions** called after a shortcut don't internally use the old catch-all fetch — they must use the probe-quality variant when error distinction matters + +**Real-world example**: Custom registry flow had 8 bugs across 3 review rounds: (1) probe only ran in interactive mode, (2) transient errors fell through to wrong mode, (3) giget URI had `#ref` in wrong position, (4) prefetched templates leaked across source switches, (5) `--template` shortcut bypassed probe but `downloadTemplateById` internally used catch-all `fetchTemplateIndex`, turning timeouts into "Template not found". + +**Real-world example**: Agent-session update hints fetched npm `latest` metadata with `response.read(4096)` and then parsed it as complete JSON. The `@mindfoldhq/trellis` package metadata exceeded 4 KB, so the JSON was truncated, parse failed silently, and the first session injection showed no update hint. Fix: read the complete response before parsing, and add a regression where `version` is followed by an 8 KB metadata tail. + +--- + +## Cross-Platform Template Consistency + +In Trellis, command templates (e.g., `record-session.md`) exist in **multiple platforms** with identical or near-identical content. This is a cross-layer boundary. + +### Checklist: After Modifying Any Command Template + +- [ ] Find all platforms with the same command: `find src/templates/*/commands/trellis/ -name ".*"` +- [ ] Update all platform copies (Markdown `.md` and TOML `.toml`) +- [ ] For Gemini TOML: adapt line continuations (`\\` vs `\`) and triple-quoted strings +- [ ] Run `/trellis:check-cross-layer` to verify nothing was missed + +**Real-world example**: Updated `record-session.md` in Claude to use `--mode record`, but forgot iFlow, Kilo, OpenCode, and Gemini — caught by cross-layer check. + +--- + +## Generated Runtime Template Upgrade Consistency + +Some generated files are both documentation and runtime input. In Trellis, +`.trellis/workflow.md` is parsed by `get_context.py`, `workflow_phase.py`, +SessionStart filters, and per-turn hooks. Template changes must be validated +against both fresh init and upgrade paths. + +### Checklist: After Modifying A Runtime-Parsed Template + +- [ ] Identify every runtime parser that reads the template, not just the file + writer that installs it +- [ ] Check whether relevant syntax lives outside obvious managed regions + such as tag blocks +- [ ] Verify fresh `init` output and a versioned `update` scenario that writes + the older `.trellis/.version` +- [ ] Add an upgrade regression using an older pristine template fixture, then + assert the installed file reaches the current packaged shape +- [ ] Update the backend spec that owns the runtime contract + +**Real-world example**: Codex inline mode changed workflow platform markers from +`[Codex]` / `[Kilo, Antigravity, Windsurf]` to `[codex-sub-agent]` / +`[codex-inline, Kilo, Antigravity, Windsurf]`. Fresh init was correct, but +`trellis update` only merged `[workflow-state:*]` blocks and preserved stale +markers outside those blocks. Result: upgraded projects got new hook scripts +but old workflow routing, so `get_context.py --mode phase --platform codex` +could return empty Phase 2.1 detail. + +--- + +## Mode-Detection Probe Checklist + +When a CLI auto-detects a mode by probing a remote resource (e.g., checking if `index.json` exists to decide marketplace vs direct download): + +### Before implementing: +- [ ] Probe runs in **ALL** code paths that use the result (interactive, `-y`, `--flag` combos) +- [ ] 404 vs transient error are distinguished — don't treat both as "not found" +- [ ] Transient errors **abort or retry**, never silently switch modes +- [ ] Shared state (caches, prefetched data) is **reset** when context changes (e.g., user switches source) +- [ ] **Shortcut paths** (e.g., `--template` skipping picker) must have the same error-handling quality as the probed path — check that downstream functions don't call catch-all wrappers + +### After implementing: +- [ ] Trace every path from probe result to the mode-decision branch — no fallthrough +- [ ] External format contracts (giget URI, raw URLs) are tested or at least documented as comments +- [ ] Metadata reads consume a complete response or use a streaming parser — never parse a fixed-size prefix as full JSON +- [ ] When reconstructing a composite identifier from parsed parts, verify **all** fields are included and in the **correct position** (e.g., `provider:repo/path#ref` not `provider:repo#ref/path`) +- [ ] Verify that **action functions** called after a shortcut don't internally use the old catch-all fetch — they must use the probe-quality variant when error distinction matters + +**Real-world example**: Custom registry flow had 8 bugs across 3 review rounds: (1) probe only ran in interactive mode, (2) transient errors fell through to wrong mode, (3) giget URI had `#ref` in wrong position, (4) prefetched templates leaked across source switches, (5) `--template` shortcut bypassed probe but `downloadTemplateById` internally used catch-all `fetchTemplateIndex`, turning timeouts into "Template not found". + +**Real-world example**: Agent-session update hints fetched npm `latest` metadata with `response.read(4096)` and then parsed it as complete JSON. The `@mindfoldhq/trellis` package metadata exceeded 4 KB, so the JSON was truncated, parse failed silently, and the first session injection showed no update hint. Fix: read the complete response before parsing, and add a regression where `version` is followed by an 8 KB metadata tail. + +--- + +## When to Create Flow Documentation + +Create detailed flow docs when: + +- Feature spans 3+ layers +- Multiple teams are involved +- Data format is complex +- Feature has caused bugs before + +--- + +## Event Log / Projection Boundary + +Append-only logs are cross-layer contracts. A single event travels through: + +``` +CLI input → event writer → events.jsonl → reader → filter → reducer → display +``` + +### Checklist: After Adding A New Event Kind Or Field + +- [ ] Add the event kind to the central event taxonomy +- [ ] Add a typed event variant or type guard at the event layer +- [ ] Add normalization helpers for array/object fields that come from + user input or JSON +- [ ] Keep `seq` / `id` assignment in the event writer only +- [ ] Make filters and reducers consume the typed event guard, not local casts +- [ ] Make display code consume reducer output or typed events, not raw JSON +- [ ] Add at least one regression that proves history replay and live filtering + use the same filter model + +**Real-world example**: Thread channels added `kind: "thread"`, `description`, +`context`, labels, and `lastSeq`. The first implementation replayed thread +state correctly, but several commands still re-parsed event payload fields with +local casts. The fix was to make the core event layer own `ThreadChannelEvent` +and `isThreadEvent`, make `reduceChannelMetadata` the only channel metadata +projection, and make `reduceThreads` the only thread replay reducer. diff --git a/.trellis/spec/guides/index.md b/.trellis/spec/guides/index.md new file mode 100644 index 0000000..56c6d77 --- /dev/null +++ b/.trellis/spec/guides/index.md @@ -0,0 +1,97 @@ +# Thinking Guides + +> **Purpose**: Expand your thinking to catch things you might not have considered. + +--- + +## Why Thinking Guides? + +**Most bugs and tech debt come from "didn't think of that"**, not from lack of skill: + +- Didn't think about what happens at layer boundaries → cross-layer bugs +- Didn't think about code patterns repeating → duplicated code everywhere +- Didn't think about edge cases → runtime errors +- Didn't think about future maintainers → unreadable code + +These guides help you **ask the right questions before coding**. + +--- + +## Available Guides + +| Guide | Purpose | When to Use | +|-------|---------|-------------| +| [Code Reuse Thinking Guide](./code-reuse-thinking-guide.md) | Identify patterns and reduce duplication | When you notice repeated patterns | +| [Cross-Layer Thinking Guide](./cross-layer-thinking-guide.md) | Think through data flow across layers | Features spanning multiple layers | + +--- + +## Quick Reference: Thinking Triggers + +### When to Think About Cross-Layer Issues + +- [ ] Feature touches 3+ layers (API, Service, Component, Database) +- [ ] Data format changes between layers +- [ ] Multiple consumers need the same data +- [ ] You're not sure where to put some logic +- [ ] You are adding an event kind, JSONL record, RPC payload, or config field +- [ ] UI / command code starts casting raw payload fields directly + +→ Read [Cross-Layer Thinking Guide](./cross-layer-thinking-guide.md) + +### When to Think About Code Reuse + +- [ ] You're writing similar code to something that exists +- [ ] You see the same pattern repeated 3+ times +- [ ] You're adding a new field to multiple places +- [ ] **You're modifying any constant or config** +- [ ] **You're creating a new utility/helper function** ← Search first! +- [ ] Two files read the same untyped payload field with local casts +- [ ] Multiple branches update the same derived state from `kind` / `action` + +→ Read [Code Reuse Thinking Guide](./code-reuse-thinking-guide.md) + +### When Verifying AI Cross-Review Results + +- [ ] Reviewer claims "user input can be malicious" → Check the actual data source (internal manifest? user config? external API?) +- [ ] Reviewer flags "missing validation" → Is the data from a trusted internal source? +- [ ] Reviewer says "behavior change" → Read the code comments — is it intentional design? +- [ ] Reviewer identifies a "bug" in test → Mentally delete the feature being tested — does the test still pass? If yes → tautological test + +**Common AI reviewer false-positive patterns**: +1. **Trust boundary confusion**: Treating internal data (bundled JSON manifests) as untrusted external input +2. **Ignoring design comments**: Flagging intentional behavior documented in code comments as bugs +3. **Variable misreading**: Not tracing a variable to its actual definition (e.g., Map keyed by path vs name) + +**Verification rule**: Every CRITICAL/WARNING finding must be verified against the actual code before prioritizing. Budget ~35% false-positive rate for AI reviews. + +--- + +## Pre-Modification Rule (CRITICAL) + +> **Before changing ANY value, ALWAYS search first!** + +```bash +# Search for the value you're about to change +grep -r "value_to_change" . +``` + +This single habit prevents most "forgot to update X" bugs. + +--- + +## How to Use This Directory + +1. **Before coding**: Skim the relevant thinking guide +2. **During coding**: If something feels repetitive or complex, check the guides +3. **After bugs**: Add new insights to the relevant guide (learn from mistakes) + +--- + +## Contributing + +Found a new "didn't think of that" moment? Add it to the relevant guide. + +--- + +**Core Principle**: 30 minutes of thinking saves 3 hours of debugging.