docs(glossary): customization vocabulary for agents and skills - #6383
docs(glossary): customization vocabulary for agents and skills#6383ascerra wants to merge 8 commits into
Conversation
Site previewPreview: https://069e43fe-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 4:11 PM UTC · Completed 4:28 PM UTC Commit: |
ReviewFindingsCritical
Low
Next steps:
Previous runReviewFindingsLow
Labels: PR modifies glossary and agent classification documentation under docs/ Previous run (2)ReviewFindingsLow
Labels: PR modifies glossary and agent classification documentation under docs/ Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Next steps:
Previous run (5)ReviewFindingsLow
Info
Previous run (6)ReviewFindingsLow
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
/fs-fix |
PR Summary by Qododocs(glossary): add customization vocabulary for agents, skills, and scripts
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
🤖 Finished Fix · ✅ Success · Started 6:56 PM UTC · Completed 7:01 PM UTC Commit: |
|
🤖 Review · Commit: |
- Remove redundant qualifier "that do not replace identity fields" from Configured Default Agent (skill overrides inherently do not touch identity fields) - Replace imprecise "higher-precedence config layer" with accurate mechanism in Repo Skill (repo skills do not participate in base composition's dedup-by-basename merge) - Remove atypical bold on "not" in Repo Skill entry to match glossary conventions Addresses review feedback on #6383
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 3 low-severity review findings: removed a redundant qualifier in Configured Default Agent, corrected the mechanism description in Repo Skill from precedence-based to layer-isolation-based, and removed atypical bold emphasis in Repo Skill. Fixed (3):
Tests: passedNext steps:
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
🤖 Finished Review · ✅ Success · Started 7:03 PM UTC · Completed 7:19 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings from an automated pass (3 inline comments on docs/glossary.md plus one below on a linked doc that isn't part of this PR's diff).
[MEDIUM] AGENTS.md glossary entry funnels readers to a citation that misattributes the open standard
File: docs/guides/user/customizing-with-agents-md.md (line 13) — not touched by this PR's diff, so noted here rather than as an inline comment.
This PR adds a new, prominent AGENTS.md glossary entry (docs/glossary.md lines 68-70) that links to docs/guides/user/customizing-with-agents-md.md, which states "AGENTS.md is the open standard (https://agentskills.io/) that any agent tool can discover." Checking both sites: agentskills.io is entirely about the unrelated Agent Skills / SKILL.md format ("a lightweight, open format for extending AI agent capabilities... originally developed by Anthropic"), not AGENTS.md files. The actual AGENTS.md standard is at https://agents.md/ ("A simple, open format for guiding coding agents," now stewarded by the Agentic AI Foundation under the Linux Foundation). The linked guide conflates two unrelated open formats, and this PR's new glossary term makes that wrong citation more visible/consequential by pointing readers straight at it.
Suggestion: fix the citation in docs/guides/user/customizing-with-agents-md.md to point to https://agents.md/, keeping agentskills.io only where the topic is genuinely Agent Skills (as already done correctly in docs/guides/user/customizing-with-skills.md). Consider doing this in the same PR or a fast-follow since this PR is specifically the vocabulary/reference-hygiene pass touching this exact term.
029de12 to
b7097cf
Compare
cc965f3 to
8fd7bb0
Compare
|
🤖 Finished Review · ✅ Success · Started 11:50 AM UTC · Completed 12:08 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.85 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
ready-for-merge
Record Option B: metadata.apply always fills a soft Skill-tool directive with skill names (no SKILL.md body paste). Include playground activation matrix evidence and update architecture plus skills guide. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep ADR 0091 evidence out of fullsend; link the public playground HTML report instead of shipping the activation matrix in this PR. Co-authored-by: Cursor <cursoragent@cursor.com>
0091 on main is now per-agent runtime/model/effort; renumber this ADR and update cross-references accordingly. Co-authored-by: Cursor <cursoragent@cursor.com>
c3ace2b to
94bd8d8
Compare
Rebase customization vocabulary onto the current ADR 0091 base (keep OTEL/Fail-Open entries). Point default harness lineage at fullsend-ai/agents, drop the merge-dependency note and Identity "ladder" wording, align always-on with ADR 0091 Option B, and document repo-skill basename fail-fast. Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- Rename "Agent identity" heading to "Agent classification" in the Customization vocabulary index to avoid overloading the existing Identity term (GitHub App installation sense) defined elsewhere in the glossary. - Reword BYOA entry to replace "org/repo config" with "config.yaml registration and base: composition" per ADR 0058, dropping the deprecated per-org install mode framing (ADR 0044). Addresses review feedback on #6383
…adowing Fail-fast only applies to harness SkillDirs() uploads; same-named repo skills are silently shadowed by personal-level built-ins (no bootstrap error), matching customizing-with-skills skill-precedence. Co-authored-by: Cursor <cursoragent@cursor.com>
…ill semantics Rebase onto renumbered ADR base; update four always-on links from 0091 to 0092; qualify Skill-tool and repo-skill discovery as Claude Code behavior and note pi ignores project .agents/skills under disabled project trust. Co-authored-by: Cursor <cursoragent@cursor.com>
- Rename "Agent identity" heading to "Agent classification" in the Customization vocabulary index to avoid overloading the existing Identity term (GitHub App installation sense) defined elsewhere in the glossary. - Reword BYOA entry to replace "org/repo config" with "config.yaml registration and base: composition" per ADR 0058, dropping the deprecated per-org install mode framing (ADR 0044). Addresses review feedback on #6383
a701ed0 to
7fd71ee
Compare
|
🤖 Finished Review · ✅ Success · Started 9:43 PM UTC · Completed 10:04 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.61 |
|
Risk Assessment: moderate (2/5) DetailsDocs-only change (7 files, 280 lines) from a known contributor with no protected/security/CI/dependency signals. Tier 2 elevated primarily because docs/architecture.md is an extreme churn hotspot (69 commits/30d, 12 authors/90d), but docs changes carry inherently lower deployment risk, keeping the composite at moderate. Previous runRisk Assessment: moderate (2/5) DetailsLow-risk docs-only change (2 files, 112 lines, no protected/security/CI/dependency signals, known contributor). Tier 2 elevates moderately due to glossary.md being a high-churn hotspot with many recent fix/revert commits and multiple authors. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep at head 7fd71eef — 6 MEDIUM findings. Five are inline (3 on docs/glossary.md, 2 file-level on docs/agents/topics/default-vs-custom.md whose anchor lines fall outside this PR's single hunk). The sixth is PR metadata, so it lives here.
[MEDIUM] ready-for-merge + undrafted while the base is still the unmerged #6378 branch; body still claims draft status
Confirmed via the GitHub API at head 7fd71eef: draft: false, mergeable_state: "clean", labels include ready-for-merge, and base.ref is docs/adr-0091-always-on-skills — the head branch of #6378, which is still open (state: open, merged: false, mergeable_state: "blocked").
The PR body still reads: "This PR is opened as a draft against the 0091 branch ... After #6378 lands on main, retarget this PR to main (or rebase) and undraft". The stated gate no longer holds — the PR is already undrafted and labeled ready-for-merge while #6378 has not landed. Merging in this state lands the glossary changes onto #6378's branch, not main.
It also matters for link integrity: the four ADRs/0092-always-on-harness-skills.md links added by this PR (lines 21, 40, 72, 235) resolve only because the base branch supplies that file. #6378 adds it and is unmerged, so a retarget to main before #6378 lands would 404 all four. (The review bot's sticky comment flags the forward-reference risk; this finding is about the merge-readiness state that makes it live.)
No existing review thread covers this: r3855765706 covered the 0091→0092 renumber (fixed in 7fd71eef) and r3833070378 covered removing the in-doc merge-dependency blockquote (fixed in 8fd7bb04) — neither the draft/label state.
Suggestion: Either re-mark the PR as draft and remove the ready-for-merge label until #6378 is on main, or land #6378 first and then rebase this PR onto main. Either way, update the body so it describes the actual state (it currently claims a draft status the PR does not have), and tick or correct the three unchecked test-plan boxes — including "Confirm relative links" and the stale "ADR 0045/0064/0091" reference (the ADR is now 0092).
|
|
||
| ### Repo Skill | ||
|
|
||
| A [skill](#skill) committed under the target repo (typically `.agents/skills/`, often symlinked as `.claude/skills`). Under the Claude Code runtime, discovered for agents on that repo. Novel names are [additive](#additive-skill). A repo skill whose name matches a [built-in skill](#built-in-skill) is **shadowed**: built-ins upload to the personal-level config dir (`CLAUDE_CONFIG_DIR/skills/`), while repo skills stay at the project level (`.claude/skills/`); Claude Code's personal-over-project precedence silently ignores the repo copy — there is no bootstrap error. That silent shadowing is not a [skill override](#skill-override). Use a unique name, or replace intentionally via [base composition](#base-composition) (same basename on the child harness `skills:` list). With `runtime: pi`, project trust is disabled (`--no-approve` / `defaultProjectTrust: never`), so repo-committed skills under `.agents/skills` are not discovered at all — see [runtimes](runtimes.md). |
There was a problem hiding this comment.
[MEDIUM] pi project-trust claim is cited to runtimes.md, which contains no such text — and whose skills row reads the opposite
This Repo Skill entry ends: "With runtime: pi, project trust is disabled (--no-approve / defaultProjectTrust: never), so repo-committed skills under .agents/skills are not discovered at all — see runtimes." The Always-on Skill entry (line 72) likewise cites runtimes.md for "(pi has no Skill tool — skills are prompt-driven via read of SKILL.md)".
I read docs/runtimes.md at head 7fd71eef in full: it contains no occurrence of --no-approve, defaultProjectTrust, project trust, .pi/, or the Skill tool. Its only skills row (the "Harness config keys per runtime" table) reads:
| skills | CLAUDE_CONFIG_DIR/skills/ | PI_CODING_AGENT_DIR/skills/, discovered natively |
So a reader following the citation lands on "discovered natively" — the opposite of what the glossary claims.
The claims themselves are true; only the citation is wrong. The evidence lives in docs/contributing/runtime-implementation.md, which states verbatim that --no-approve sets the project-trust override so the trust-gated project resources — .pi/{settings.json,extensions,skills,...} and .agents/skills — are ignored, and that defaultProjectTrust: never in global settings covers the no-flag case. Corroborated by internal/runtime/pi_bootstrap.go, pi_run.go, and pi_bootstrap_test.go (which also asserts enableSkillCommands: false).
This is new relative to thread r3855765727: that thread asked for the pi caveat plus a cross-link, and the fix commit 7fd71eef added the caveat pointed at the wrong page. (For what it's worth, that earlier comment's own "docs/runtimes.md:299" citation does not hold at head either — the file is far shorter than 299 lines. Apologies for sending you there.)
Suggestion: Repoint both citations (here and line 72) from runtimes.md to contributing/runtime-implementation.md, where the --no-approve / defaultProjectTrust: never / .agents/skills-ignored evidence actually lives. Additionally, amend the skills row in docs/runtimes.md's "Harness config keys per runtime" table so "discovered natively" carries the trust caveat — harness skills under PI_CODING_AGENT_DIR/skills/ are discovered; repo-committed .agents/skills are not, because project trust is disabled. Otherwise the two pages contradict each other.
|
|
||
| ### On-demand Skill | ||
|
|
||
| A [skill](#skill) load mode: uploaded and listed for the run, but under the Claude Code runtime the full body loads only when the model opens it (Skill tool). Default when frontmatter omits `metadata.apply` / `apply`, or sets `on-demand` ([ADR 0092](ADRs/0092-always-on-harness-skills.md)). Contrast with [always-on skill](#always-on-skill). |
There was a problem hiding this comment.
[MEDIUM] Always-on / on-demand load modes are documented as shipped behavior, but nothing at this head implements metadata.apply
This On-demand Skill entry states as current fact: "Default when frontmatter omits metadata.apply / apply, or sets on-demand". Nothing parses those keys at this head.
internal/skill/skill.go defines the entire parsed frontmatter surface as SkillMeta{Name, Description, Dependencies, Policy} — no metadata, no apply — and it is the only frontmatter parser. Code searches for metadata.apply and for AlwaysOn / ALWAYS_ON across the repo return zero hits, so ADR 0092's mechanism is entirely unwired. This PR is docs-only (2 files), so the branch adds no implementation either. Issue #6380 is still open and its only linked closing PR is the docs-only #6378.
A glossary is read as a current-state reference, so a reader will set metadata.apply: always and get a silent no-op — YAML frontmatter that SkillMeta does not declare is simply discarded by the unmarshal, and nothing downstream reads it.
The sibling Always-on Skill entry (line 72) has the same defect ("after upload, bootstrap injects a short directive naming those skills"). I am not raising it as a separate finding because resolved thread r3832926231 already covered docs/glossary.md[72-74] with this same substance — but that thread's requested > **Planned:** callout was never added, only the #6380 link, so the fix should cover both entries.
Suggestion: Mark both entries as not-yet-implemented using the repo's existing convention, e.g. a leading blockquote:
> **Planned:** not yet wired in the runtime — see [ADR 0092](ADRs/0092-always-on-harness-skills.md) and [#6380](https://github.com/fullsend-ai/fullsend/issues/6380).
(the same **Planned:** form already used in customizing-with-skills.md). Convert to present tense in the PR that lands the bootstrap/frontmatter change.
|
|
||
| ### Built-in Skill | ||
|
|
||
| A [skill](#skill) that ships with a [default agent](#default-agent) (for example `code-implementation`, `code-review`, `issue-labels`). Listed in that agent's harness and documented under [Agents reference](agents/). Teams typically [add](#additive-skill) alongside built-ins; replacing one by basename via [base composition](#base-composition) or the historical overlay is a [skill override](#skill-override). |
There was a problem hiding this comment.
[MEDIUM] Built-in Skill entry hardcodes agents-repo skill names, against main's runtime-discovery rule
This new entry names three skills inline: "A skill that ships with a default agent (for example code-implementation, code-review, issue-labels)".
main's AGENTS.md carries the rule: "When writing skills, documentation, or guides that reference the fullsend agent/skill/sub-agent inventory (agent names, skill names, harness configs, sub-agent rosters), use runtime discovery commands against fullsend-ai/agents rather than hardcoded tables or static listings. The agents repo evolves independently and hardcoded references go stale."
Two caveats that bound this, both verified:
- The rule is not present in
AGENTS.mdat this PR's head — the base branch predates it — so this bites on retarget tomain, not today. - All three names are accurate right now (
fullsend-ai/agentsskills/currently holds agent-scaffolding, autonomy-readiness, code-implementation, code-review, docs-review, finding-agent-runs, fix-review, github-forge, gitlab-forge, issue-labels, jira-forge, merge-queue, pr-review, pr-risk-assessment, retro-analysis).
So this is a staleness-coupling / convention concern, not a factual error — hence MEDIUM rather than HIGH.
The Extension Point entry's customer-research (line 168) is deliberately not part of this finding: docs/agents/prioritize.md documents it as a user-supplied repo skill placed at .agents/skills/customer-research/, not agents-repo inventory, so the rule does not reach it.
Suggestion: Drop the three literal skill names and define built-in skill structurally instead — e.g. "a skill listed in a default agent's harness in fullsend-ai/agents" — pointing readers at the per-agent reference pages (which already carry a Source link to the agent's harness YAML) rather than naming instances. If a canonical inventory is wanted, it belongs in fullsend-ai/agents. Note the linked target guides/user/customizing-with-skills.md#built-in-skills reportedly carries a static table with the same coupling — worth a follow-up there.
There was a problem hiding this comment.
[MEDIUM] Line 71: the new Repo Skill entry contradicts a table row in this file
(File-level comment — line 71 is pre-existing text outside this PR's single hunk, so there is no inline anchor.)
Line 71 of the "Classification by harness field" table still reads:
| Add repo-level skills in .agents/skills/ | Configured default | Repo skills are discovered automatically; no harness change needed. |
The new glossary Repo Skill entry (docs/glossary.md:263) establishes two exceptions to "discovered automatically" that this row now contradicts:
- A repo skill whose basename matches a built-in is silently shadowed by Claude Code's personal-over-project precedence.
- Under
runtime: pi, project trust is disabled, so.agents/skillsis not read at all.
Both are correct (see my comment on docs/glossary.md:263 for the primary sources). This PR edits this exact file at line 56, and the glossary points at it as the classification source of truth, so the two pages would ship disagreeing.
To be clear: the anchor line itself is not in the diff — it is pre-existing text that this PR's new claims newly contradict.
Suggestion: Amend the rationale cell in this same PR, e.g. "Discovered automatically under the Claude Code runtime; a basename matching a built-in skill is silently shadowed, and runtime: pi does not read .agents/skills (see Repo Skill in the glossary)."
There was a problem hiding this comment.
[MEDIUM] Line 7: bare "three tiers" persists, against tier-conventions.md and this PR's own "categories, not ordered steps" framing
(File-level comment — line 7 is pre-existing text outside this PR's single hunk, so there is no inline anchor.)
Line 7 reads "This document defines three tiers:".
docs/contributing/tier-conventions.md is explicit: "Always use a descriptive prefix to avoid ambiguity" and "Do not use bare 'Tier N' or 'tier' without a prefix", listing three unrelated in-repo meanings (credential delivery tier, intent authorization tier, configuration tier). AGENTS.md routes reviewers to that page for exactly this case ("Using the term 'tier' in code or docs").
Compounding it: this PR's new glossary index calls the same four classes "categories, not ordered steps" (docs/glossary.md:11), while the page the glossary names as its classification source calls them tiers — an ordered-sounding word. This PR already edits this file (line 56), so the fix is in scope.
As with the table-row comment, line 7 is pre-existing text not in the diff; it is the PR's new index wording that puts it out of step.
Suggestion: Replace "three tiers" with "three categories" (or "three classifications") to satisfy docs/contributing/tier-conventions.md and match the new glossary index's "categories, not ordered steps" wording.
|
/fs-fix address all review comments including those from waynesun09 |
|
🤖 Finished Fix · ✅ Success · Started 6:30 PM UTC · Completed 6:43 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.96 |
- Demote ## Customization vocabulary to bold paragraph header with HTML anchor so ## stays reserved for alphabetical sections - Add > **Planned:** callouts to Always-on Skill and On-demand Skill entries since metadata.apply is not yet wired in the runtime - Repoint pi project-trust citations from runtimes.md to contributing/runtime-implementation.md where the evidence lives - Remove hardcoded skill names from Built-in Skill entry; define structurally as "listed in a default agent's harness" - Amend repo-skill table row in default-vs-custom.md to note shadowing caveat and pi trust limitation - Replace bare "three tiers" with "three categories" in default-vs-custom.md per tier-conventions.md Addresses review feedback on #6383
🔧 Fix agent — iteration 2 (human-triggered)Validation retry: the previous iteration's code changes passed schema validation but failed the pre-commit gate because 6 commits on the PR branch contained Signed-off-by trailers. Used git filter-branch --msg-filter to strip all Signed-off-by lines from the 8 commits on the branch. No code changes were needed — only commit message cleanup. Fixed (7):
Tests: passed Decision points
Next steps:
|
7fd71ee to
9988a55
Compare
|
🤖 Finished Review · ✅ Success · Started 6:45 PM UTC · Completed 7:26 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.60 |
| @@ -0,0 +1,115 @@ | |||
| --- | |||
There was a problem hiding this comment.
[critical] ADR numbering collision
ADR 0092 already exists on main as docs/ADRs/0092-openai-wif-credential-delivery.md. This PR creates a second file with the same number. 19 files across the codebase reference ADR 0092 pointing to the OpenAI WIF ADR. Every cross-reference this PR adds links to 0092-always-on-harness-skills.md, which would coexist with the other file.
Suggested fix: Renumber the new ADR to 0093 (or the next available number). Update all references in this PR's changed files.
| An external process running alongside (but outside) the agent's sandbox that mediates access to resources the sandbox cannot natively constrain. Example: an ephemeral Git server that receives `git push` from the agent and forwards it only to the one branch the agent is authorized to write to. Unlike an MCP server (which the agent explicitly calls as a tool), a sidecar can be transparent — the agent may not know it's interacting with a mediator rather than the real service. | ||
| See [architecture.md](architecture.md) and [#101](https://github.com/fullsend-ai/fullsend/issues/101). | ||
|
|
||
| ### Script Override |
There was a problem hiding this comment.
[low] naming-convention
Script Override uses Details: as its reference-line prefix instead of the established See prefix used by every other glossary entry.
Suggested fix: Change Details: to See for consistency.
fullsend-ai-review
Bot
removed
the
ready-for-merge
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep at head 9988a553 — 4 findings (1 CRITICAL, 2 HIGH, 1 MEDIUM), all inline. They share one root cause: the always-on skill design this PR documents (ADR 0092 / metadata.apply: always / placeholder directive) was reversed after the earlier review rounds. #6378 was closed unmerged ("we have decided to go with a slightly different strategy"), #6380 was closed and split into #6681 (inject every harness-listed skill into the agent definition's skills: frontmatter) and #6682 (future per-skill optional mode), and PR #6859 implements #6681 with no metadata.apply, placeholder, or directive. The ADR is inside this PR's own diff (same merge-base with main and with the stacked base), so the fix is to drop it and the cross-references rather than renumber it.
| **Trade-offs:** Good safety net ([ADR 0022](0022-harness-level-output-schema-enforcement.md)). | ||
| Does not teach style or procedure. | ||
|
|
||
| ## Decision |
There was a problem hiding this comment.
[CRITICAL] PR carries a superseded ADR as status: Accepted plus four cross-references to a reversed decision
Verified at head 9988a553. This ADR is already inside this PR's own diff, not a retarget risk: the merge-base of the head with its base branch docs/adr-0091-always-on-skills (94bd8d8) and with main is the same commit edb57c2, so git diff origin/main...head and GitHub's file list both show docs/ADRs/0092-always-on-harness-skills.md (+115) with status: Accepted and **We choose Option B.** (metadata.apply: always + __FULLSEND_ALWAYS_ON_SKILLS__ placeholder + Skill-tool directive).
The decision it records was reversed: PR #6378 (the ADR PR this one claims to depend on) was CLOSED unmerged on 2026-08-28 with the comment "we have decided to go with a slightly different strategy"; issue #6380 was closed completed on 2026-08-27 and split into #6681 (inject every harness-listed skill into the agent definition's skills: frontmatter, explicitly "differs from PR #6378's strategy of using metadata.apply: always with a placeholder") and #6682 (future per-skill optional mode). PR #6859 (open) implements #6681 as InjectFrontmatterSkills in internal/runtime/frontmatter_skills.go; its diff contains no metadata.apply, no placeholder, no directive. The PR body's gate "merge after #6378 merges" can never be satisfied.
Four in-diff edits assert the dead decision as settled: docs/ADRs/0024-harness-definitions.md:788-791 ("is decided in ADR 0092"), docs/architecture.md:169-174 and :179, docs/problems/codebase-context.md:68.
The only existing thread on this file (bot, line 1) is about the number colliding with 0092-openai-wif-credential-delivery.md on main and proposes renumbering to 0093 — that remedy would merge the rejected design under a fresh number.
Two further ADR-internal defects are moot once it is dropped but worth recording: the Decision's prerequisite (placeholder in the agent file, Skill in every tools:) points at agents-repo commit 49a1e31, which sits on a diverged branch (ahead 1 / behind 202) and was never merged; and the Options section never evaluates native skills: frontmatter preload even though agents/triage.md and agents/retro.md on fullsend-ai/agents main already carry skills: lists — the mechanism #6859 actually uses.
Suggested fix: Remove docs/ADRs/0092-always-on-harness-skills.md from this PR (do not renumber it) and revert the three cross-reference hunks in docs/ADRs/0024-harness-definitions.md, docs/architecture.md, and docs/problems/codebase-context.md. Retarget to main (rebase over the 423 commits behind) and rewrite the glossary/guide skill-load-mode text against #6681/#6682 and #6859, or hold those entries until #6859 lands. Update the PR body to drop the #6378 dependency. If the team wants an ADR for the frontmatter-injection approach, it belongs in its own PR with the next free number.
|
|
||
| ### Always-on Skill | ||
|
|
||
| > **Planned:** The always-on load mode is decided but not yet wired in the runtime — see [ADR 0092](ADRs/0092-always-on-harness-skills.md) and [#6380](https://github.com/fullsend-ai/fullsend/issues/6380). |
There was a problem hiding this comment.
[HIGH] Always-on / On-demand glossary entries describe the abandoned metadata.apply design and cite a closed issue as pending
Verified at head 9988a553. Both > **Planned:** callouts (lines 73 and 238) say the load mode is "decided but not yet wired in the runtime — see ADR 0092 and #6380". #6380 is closed completed with a different outcome (#6681/#6682), and what is being wired (#6859) is frontmatter skills: injection of every harness-listed skill, so the callouts now point readers at a reversed decision.
Line 75 states "frontmatter metadata.apply: always (legacy top-level apply: always also accepted) opts the skill into always-on" and "bootstrap will inject a short directive naming those skills so the model opens each with the Skill tool" — none of that is planned. Line 240 inverts the new default: it calls on-demand the "Default when frontmatter omits metadata.apply / apply, or sets on-demand", whereas under #6681/#6859 every harness-listed skill becomes always-on with no opt-out key, and the optional mode is a future per-skill setting (#6682). The Skills index bullet (line 22) and goals-table row (line 42) present the same pair as settled via ADR 0092.
Note on the existing thread: the resolved thread on the On-demand entry (original line 235, "documented as shipped behavior") requested exactly these Planned banners pointing at ADR 0092/#6380, and commit 9988a55 added them; that fix is now itself wrong because the decision changed after the thread was written, so re-adding the banner is not the remedy.
Suggested fix: Rewrite both entries: always-on = harness skills: entries are injected into the agent definition's skills: frontmatter at bootstrap and become the default for every harness-listed skill (#6681, implemented by #6859); on-demand/optional = a planned per-skill opt-out tracked in #6682 whose syntax is undecided. Drop the metadata.apply / apply: always / directive / "requires Skill in tools:" sentences, replace the #6380 and ADR 0092 links with #6681/#6682 (and #6859 once merged), and repoint the index bullet (line 22) and table row (line 42) the same way. Keep a Planned marker only until #6859 lands.
| Skills can reference companion scripts and data files in the same directory, | ||
| giving agents the ability to dynamically gather information at runtime. | ||
|
|
||
| ### Always-on vs on-demand |
There was a problem hiding this comment.
[HIGH] User guide gives present-tense copy-paste instructions for the abandoned metadata.apply: always key with no Planned marker
Verified at head 9988a553. The new "Always-on vs on-demand" section (lines 63-90) is a full present-tense recipe: set metadata:\n apply: always in SKILL.md, "Bootstrap fills a soft always-use directive with that skill's name on the copied agent definition", "The model must open the skill with the Skill tool", "The agent's tools: must include Skill", linking ADR 0092 and the playground adr-0091-skill-activation-matrix.html. Lines 63-90 contain no Planned wording, although the same file already uses the > **Planned:** banner convention at lines 205-207 for #6158.
Nothing on this head or on main parses metadata.apply (confirmed by the resolved glossary thread: SkillMeta{Name, Description, Dependencies, Policy} is the only frontmatter parser), and the key was dropped from the plan entirely — #6681/#6859 make listing the skill in the harness the always-on signal with no frontmatter key. A user following this section gets a silent no-op today and, after #6859, a key that is still ignored.
No existing review comment touches this file. Because the design was reversed rather than merely unimplemented, gating the section as Planned would still document the wrong interface. The section does say "listed in the harness" (line 65), so the harness-only scoping is adequate and only needs sharpening on rewrite.
Suggested fix: Remove lines 63-90 from this PR. When #6859 merges, replace them with a short walkthrough of the shipped behavior: skills listed under harness skills: are injected into the agent definition's skills: frontmatter and preloaded on every run of that harness (harness-listed only; repo-discovered skills remain #237), with the per-skill optional mode noted as tracked in #6682. Do not instruct users to set metadata.apply or to edit tools: for this purpose.
| property (source system, event type, etc.) rather than only the forge | ||
| platform. `forge:` is deprecated but remains functional | ||
| ([ADR 0088](ADRs/0088-cel-guarded-overlays.md)). | ||
| - Always-on harness skills: skill frontmatter `metadata.apply: always` |
There was a problem hiding this comment.
[MEDIUM] architecture.md living-doc bullet describes the reversed always-on design as current runtime behavior
Verified at head 9988a553. Lines 169-174 add to the current-state architecture list: "Always-on harness skills: skill frontmatter metadata.apply: always (default on-demand) causes bootstrap to name those skills in a soft always-use directive on the copied agent definition; the model opens each via the Skill tool ... (ADR 0092)", and line 179 amends an open question with "Always-on vs on-demand skill load mode for harness-listed skills is decided in ADR 0092".
The phrasing is present-tense with no Planned marker (the glossary entries in this same PR carry one), and the mechanism described was abandoned in favor of frontmatter skills: injection (#6681, PR #6859). architecture.md is the living current-state doc, so this both overstates implementation status and records the wrong decision. No existing review comment covers this file.
Suggested fix: Revert the lines 169-174 bullet and the line 179 amendment from this PR. Once #6859 merges, add a bullet stating that bootstrap injects harness-listed skills into the agent definition's skills: frontmatter (always-on by default), and keep the per-skill optional mode (#6682) and pi-runtime handling as open questions.
3 participants
Summary
Dependency / merge order
Depends on: #6378 (
docs(adr): add ADR 0091 always-on harness skills)Merge after #6378 merges. This PR is opened as a draft against the 0091 branch. After #6378 lands on
main, retarget this PR tomain(or rebase) and undraft.Test plan
mainand undraft