fix(improve-codebase-architecture): link DEEPENING.md, name sub-agent models, update Tailwind to v4 #36
Loading…
Reference in a new issue
No description provided.
Delete branch "align/improve-codebase-architecture"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
DEEPENING.md defined the report's dependency categories but nothing linked it; the report scaffold pinned the retired Tailwind v3 Play CDN; and the CONTEXT/ADR format references pointed into another skill's directory by relative path, which per-run skill delivery cannot resolve — they are now by-name references to the project-docs skill. The sub-agent spawns also name model and reasoning effort (opus at medium for the Explore walk, fable at xhigh for the interface designers), matching the estate's routing rule.
Approach review: The approach is sound and the three fixes are the right shape.
Verified against the working tree:
DEEPENING.mddoes define the four dependency categories the new links point at, and the added badge bullet inSKILL.mdmatches the badge row already specified inHTML-REPORT.md— the link closes a real gap rather than adding a second home for the categories.../grill-with-docs/CONTEXT-FORMAT.mdand../grill-with-docs/ADR-FORMAT.mdwith by-name references is correct for per-run delivery: after this change no../cross-skill link remains underskills/, andproject-docs/SKILL.mdgenuinely carries both the CONTEXT.md and ADR format sections. (Pre-existing and out of scope here:grill-with-docsstill ships its ownCONTEXT-FORMAT.md/ADR-FORMAT.mdduplicating those sections, so the format now has two homes — worth collapsing in a separate change.)https://cdn.jsdelivr.net/npm/@tailwindcss/browser@4is the documented v4 browser build and the correct successor to the retiredcdn.tailwindcss.comPlay CDN. The scaffold carries no v3-only idioms (notailwind.configobject, no renamed utilities), so the swap is complete rather than partial.One alternative worth considering, on pinning sub-agent model aliases — see the inline comment.
Approach review by Claude Code Opus (opus)
@ -35,3 +35,3 @@Read the project's domain glossary and any ADRs in the area you're touching first.Then use the Agent tool with `subagent_type=Explore` to walk the codebase. Don't follow rigid heuristics — explore organically and note where you experience friction:Then use the Agent tool with `subagent_type=Explore` — on `opus` at `medium` reasoning effort, since the walk delivers facts, not conclusions — to walk the codebase. Don't follow rigid heuristics — explore organically and note where you experience friction:Consider stating the routing intent here and leaving the concrete model alias to the estate's rules source, rather than pinning
opus/mediuminline (same forfable/xhighinINTERFACE-DESIGN.md).The rationale each line already carries — "the walk delivers facts, not conclusions", "they deliver design conclusions, not fact lists" — is the durable half and is enough for an agent to route correctly. The alias half is the volatile half, and it now lives in two files in the shareable source while the routing rule itself lives outside this repo, so a re-route has to be chased across both places with nothing to catch drift.
axskillsalready serves arules/tree concatenated into the agent's global rule file, which is a single home for exactly this kind of estate-wide convention.It is also the only place in
skills/that names models, and these skills are delivered to non-Claude agents too (Codex via the leased config home, everything else via headless delivery), whereopus/fableandxhighdo not resolve. Non-blocking — if the aliases are deliberately pinned here because the rule cannot reach this source, that is a reasonable call.Taking the out you offered: the aliases are deliberately pinned here. Skills ship as per-run selections to harnesses that don't all receive the estate's rule file, so the spawn site is the only place the routing contract reliably reaches — and the estate's routing rule itself asks for both halves (model and effort) to be stated explicitly wherever a spawn is specified. On a harness where
opus/fable/xhighdon't resolve, the rationale half ("facts, not conclusions" / "conclusions, not fact lists") still routes correctly and the alias half is inert rather than harmful. The drift cost is accepted: a re-route is an estate-wide edit in any case, and these two files move with the rule. Keeping the pins as-is.Summary: Found 2 medium integration issues.
Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)
@ -19,3 +19,3 @@### 2. Spawn sub-agentsSpawn 3+ sub-agents in parallel using the Agent tool. Each must produce a **radically different** interface for the deepened module.Spawn 3+ sub-agents in parallel using the Agent tool, each on `fable` at `xhigh` reasoning effort — they deliver design conclusions, not fact lists. Each must produce a **radically different** interface for the deepened module.🟡 Medium: The Agent tool's per-invocation schema accepts a
modeloverride but noeffortoverride. These calls can selectfable, but they inherit the session or subagent-definition effort instead of running atxhigh; the same limitation makes the newmediumpromise inSKILL.mdineffective. Define custom subagents witheffortfrontmatter and invoke them, or remove the unenforceable per-spawn effort routing.Correct that the Agent tool's per-invocation schema exposes
modelbut noteffort— but the line states the routing contract, not a specific API call. The estate's routing rule requires effort to be set explicitly wherever it can be set (agent-definition frontmatter, workflowagent()calls, or a per-spawn control where the harness has one), and defining a custom subagent witheffortfrontmatter — your first suggested remedy — is exactly one compliant way to honor the stated pin. The skill deliberately names the target (fableatxhigh,opusatmedium) and leaves the mechanism to the executing harness; removing the effort half would soften a contract this change exists to state. Keeping it as written.@ -76,3 +77,3 @@Side effects happen inline as decisions crystallize:- **Naming a deepened module after a concept not in `CONTEXT.md`?** Add the term to `CONTEXT.md` — same discipline as `/grill-with-docs` (see [CONTEXT-FORMAT.md](../grill-with-docs/CONTEXT-FORMAT.md)). Create the file lazily if it doesn't exist.- **Naming a deepened module after a concept not in `CONTEXT.md`?** Add the term to `CONTEXT.md` — the CONTEXT.md format lives in the project-docs skill, and the grill-with-docs skill applies the same discipline in its interviews. Create the file lazily if it doesn't exist.🟡 Medium: This skill can be selected and delivered independently, but its directory does not contain the
project-docsskill and this change declares no co-delivery dependency. A by-name mention therefore still leaves runs that select onlyimprove-codebase-architecturewithout the required CONTEXT/ADR formats (the same issue occurs on line 81). Keep the required format in a local supporting file, or makeproject-docsan actual delivered dependency.This is the estate-wide convention working as intended: cross-skill references are plain names in prose, never relative paths, because skills are delivered as per-run selections — a
../grill-with-docs/...path is guaranteed dead in every run, while a name is resolvable by the selection layer or by the agent requesting the skill by name. Duplicating the CONTEXT/ADR formats into a local supporting file would recreate exactly the two-homes drift this PR removes (and that the approach review flagged in grill-with-docs as worth collapsing separately). In a run that selects only this skill, the by-name mention degrades to a working pointer the agent can act on, not a broken link — which is the failure mode this change fixes. Deliberate decision; keeping the by-name references.Review-cycle status for head
d6184cf: thePR Reviewsource run (#651, id 17377) completed only 2 of its 5 slots.forgejo-review-approach-smart-1andforgejo-review-code-smart-1published receipt-bound reviews (both processed above);approach-smart-2,approach-3, andcode-smart-2failed their Axrecipe capability exchange withHTTP 400: "Validation failed". The same three slots fail identically in every recent run across PRs #30–#42 (Aug 5–6), so this is a deterministic service-side slot-validation fault, not PR content — a re-dispatch would fail the same way. The cycle for this head stays incomplete until the Axrecipe slot registration is fixed.Maintainer review: the model-alias pins (
opus/medium,fable/xhigh) are replaced with tier-and-effort wording ("a fast executor model at moderate reasoning effort", "the strongest reasoning model at maximum effort"). This supersedes the rebuttals in the two routing threads — the routing contract stays; the alias names would rot as models change.Summary: Docs-only change to the
improve-codebase-architectureskill. I verified the substantive claims and found no blocking defects — 2 low/info notes.Verified:
https://cdn.jsdelivr.net/npm/@tailwindcss/browser@4resolves (currently 4.3.3) and serves an IIFE global build, so the classic<script src>in the scaffold is correct.@import "tailwindcss"when nostyle[type="text/tailwindcss"]is present, so the scaffold renders unmodified — no extra config needed for the v3→v4 move.bg-stone-50,rounded-lg,border-slate-200,space-y-*,tracking-wider,font-serif,h-12 border-l-4, …) still exists in v4 and is not part of the renamed v3 scale.skills/project-docs/SKILL.mdreally does define both theCONTEXT.mdformat and the ADR format, so the new by-name references point at real content, andgrepconfirms no../-style cross-skill links remain anywhere underskills/.DEEPENING.mddefines the four dependency categories the new badge bullet and the HTML badge row now link to.One housekeeping note: the PR description says the sub-agent spawns "name model and reasoning effort (opus at medium … fable at xhigh)", but the branch head routes by tier and names no model alias — the description is stale relative to the final commit.
Code review by Claude Code Opus (opus)
@ -11,3 +11,3 @@<meta charset="utf-8" /><title>Architecture review — {{repo name}}</title><script src="https://cdn.tailwindcss.com"></script><script src="https://cdn.jsdelivr.net/npm/@tailwindcss/browser@4"></script>ℹ️ Info: One behavioural difference this migration carries that is worth knowing when the scaffold is edited later: the v4 browser build compiles the default
@import "tailwindcss", which puts every utility inside real@layerblocks. Unlayered CSS beats layered CSS regardless of source order, so the plain<style>block below (.seam,.leak,.deep) now wins against utilities on the same element — e.g..deep'sbackgroundwould override abg-*utility, the reverse of what the v3 Play CDN did. Harmless as written, since those three rules cover properties Tailwind isn't asked for here; it only bites if a future custom rule overlaps a utility.@ -19,3 +19,3 @@### 2. Spawn sub-agentsSpawn 3+ sub-agents in parallel using the Agent tool. Each must produce a **radically different** interface for the deepened module.Spawn 3+ sub-agents in parallel using the Agent tool, each on the strongest reasoning model at maximum effort — they deliver design conclusions, not fact lists. Each must produce a **radically different** interface for the deepened module.🟢 Low: Same as
SKILL.md:37— "at maximum effort" is not expressible through the Agent tool.AgentInputaccepts onlymodel("sonnet" | "opus" | "haiku" | "fable") with no reasoning-effort field, so this reduces to "pick the strongest model" in practice. Phrase the effort part as prompt-level guidance in the brief, or drop it, so the instruction doesn't read as a spawn parameter that does not exist.@ -35,3 +35,3 @@Read the project's domain glossary and any ADRs in the area you're touching first.Then use the Agent tool with `subagent_type=Explore` to walk the codebase. Don't follow rigid heuristics — explore organically and note where you experience friction:Then use the Agent tool with `subagent_type=Explore` — a fast executor model at moderate reasoning effort, since the walk delivers facts, not conclusions — to walk the codebase. Don't follow rigid heuristics — explore organically and note where you experience friction:🟢 Low: The "moderate reasoning effort" half of this instruction has no knob to turn in Claude Code. The installed SDK tool schema (
@anthropic-ai/claude-code/sdk-tools.d.ts,AgentInput) exposessubagent_type,model("sonnet" | "opus" | "haiku" | "fable"),run_in_background,name, andisolation— there is no effort/reasoning parameter, so an agent following this line can only set the model and will silently drop the effort part. "a fast executor model" also has to be mapped to one of those four aliases by guesswork at spawn time.Routing by tier rather than by alias is a deliberate call (the follow-up commit), so the fix is just to make the sentence match what is settable: name the tier for
modeland either drop the effort clause or mark it as applying only where the host exposes effort.