feat: [ENG-2697] rewrite ByteRover skill — sub-skill split + Hermes connector#675
feat: [ENG-2697] rewrite ByteRover skill — sub-skill split + Hermes connector#675DatPham-6996 wants to merge 12 commits into
Conversation
feat: [ENG-2840] Update SKILL.md to force "brv swarm query" to run si…
…nnector Rewrite the ByteRover skill on the Superpowers model: a thin SKILL.md entrypoint with progressive disclosure into focused sub-skill files, plus first-class support for autonomous agents. - Skill: split monolithic SKILL.md into query/curate/review/swarm/vc/ history/troubleshooting.md; SKILL_FILE_NAMES enumerates all managed files and status/uninstall key off the full set - Hermes: add Hermes agent (skill + mcp), connector icon, and YAML MCP config writer; McpConfigFormat now supports 'yaml' (~/.hermes/config.yaml) - Autonomous attachments: write a managed BYTEROVER RULES block into Hermes/OpenClaw system-prompt files via autonomous-agent-attachments and upsert/remove/hasByteroverBlock; idempotent re-install, status reports stale/missing blocks as not-installed - Path resolution: shared agent-path-resolver + skill-path-resolver honor HERMES_HOME / OPENCLAW_STATE_DIR / OPENCLAW_CONFIG_PATH and per-agent globalRoot; OpenClaw skills move to openclaw-state root - Hub: drop hardcoded project scope; daemon infers per-agent default (global for global-only skill agents); writeSkillFiles takes object params with idempotent per-file existence checks - Tests: cover yaml writer, agent-path-resolver, attachment manager, rule-segment-patcher block ops, hub install scope, skill connector Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @DatPham-6996's task in 6m 55s —— View job Claude PR Review — SummaryOverall score: 4 / 5 — solid, well-tested feature work with a clean architectural split. A few concrete safety and consistency issues (mostly in connector edges, none blocking for the core skill split). See inline comments for specifics. What this PR does well
Issues (inline)
Nitpicks (inline)
Test coverageTDD ledger looks healthy:
Final checklist
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| No ByteRover LLM provider is required. The calling agent's own LLM authors the HTML response. | ||
|
|
||
| ***HIGHLY IMPORTANT***: For hermes agent, run brv commands at `workdir="~/.hermes/byterover/"`. |
There was a problem hiding this comment.
issue (consistency): This workdir="~/.hermes/byterover/" is hardcoded in all 7 sub-skill files (curate.md, query.md, swarm.md, vc.md, history.md, review.md, troubleshooting.md).
Two problems:
- It does not follow
HERMES_HOME. Everywhere else in this PR (resolveHermesHome,HermesMCPconfigPathResolver, the skill install root) honorsHERMES_HOME. A user who setsHERMES_HOME=/opt/hermeswill get the skill installed at/opt/hermes/skills/byterover/and the MCP config at/opt/hermes/config.yaml, but the skill itself will still tell the agent to runbrvat~/.hermes/byterover/— pointing at the non-relocated home. The agent will then operate on the wrong project root. - Minor typo: double space between
atand the backtick.
If ~/.hermes/byterover/ is the intended Hermes convention regardless of HERMES_HOME, please add a one-line note explaining why this one path is decoupled. Otherwise, either:
- Make the template parameterized (e.g., emit the resolved path at install time), or
- Re-phrase to
run brv commands inside the Hermes project workdir (default: ~/.hermes/byterover/, overridable via $HERMES_HOME).
| private defaultScopeForAgent(agent?: Agent): 'global' | 'project' { | ||
| if (!agent) return 'project' | ||
| const skillConfigs: Record<string, {projectPath: null | string}> = SKILL_CONNECTOR_CONFIGS | ||
| const config = skillConfigs[agent] | ||
| return config && !config.projectPath ? 'global' : 'project' | ||
| } |
There was a problem hiding this comment.
suggestion (typing): The widening cast as Record<string, {projectPath: null | string}> violates the CLAUDE.md guideline “Avoid as Type assertions — use type guards or proper typing instead.” A type guard works here without information loss:
| private defaultScopeForAgent(agent?: Agent): 'global' | 'project' { | |
| if (!agent) return 'project' | |
| const skillConfigs: Record<string, {projectPath: null | string}> = SKILL_CONNECTOR_CONFIGS | |
| const config = skillConfigs[agent] | |
| return config && !config.projectPath ? 'global' : 'project' | |
| } | |
| private defaultScopeForAgent(agent?: Agent): 'global' | 'project' { | |
| if (!agent) return 'project' | |
| if (!(agent in SKILL_CONNECTOR_CONFIGS)) return 'project' | |
| const config = SKILL_CONNECTOR_CONFIGS[agent as keyof typeof SKILL_CONNECTOR_CONFIGS] | |
| return config.projectPath ? 'project' : 'global' | |
| } |
Bonus: it makes the “non-skill agents fall through to project” fallback explicit rather than relying on config? being undefined.
| async function readOpenClawConfig(options?: AgentPathResolverOptions): Promise<UnknownRecord> { | ||
| const configPath = resolveOpenClawConfigPath(options) | ||
| let raw: string | ||
| try { | ||
| raw = await readFile(configPath, 'utf8') | ||
| } catch { | ||
| return {} | ||
| } | ||
|
|
||
| const parsed = yamlLoad(raw) | ||
| return isRecord(parsed) ? parsed : {} | ||
| } |
There was a problem hiding this comment.
nitpick: OpenClaw's config file is named openclaw.json (see resolveOpenClawConfigPath default at agent-path-resolver.ts:94), but it's parsed with yamlLoad. YAML is a superset of JSON for common shapes, so this works in practice, but it obscures the intent: a reader of readOpenClawConfig would reasonably expect the parser to match the file's .json extension.
Either use JSON.parse (and accept its stricter syntax) or rename the constant/comment to clarify that the file path is .json but YAML-compatible. As-is, if a user ever supplies an OPENCLAW_CONFIG_PATH ending in .yaml, this would silently keep working — which may be the intent, but is worth documenting.
| /** | ||
| * Boundary markers for the always-loaded BYTEROVER block that the SkillConnector | ||
| * writes into autonomous agents' system-prompt context files. | ||
| * | ||
| * The marker strings intentionally match rule files so legacy detection keeps | ||
| * treating all ByteRover-managed instruction blocks consistently. | ||
| */ | ||
| export const BYTEROVER_BLOCK_MARKERS = BRV_RULE_MARKERS |
There was a problem hiding this comment.
question: Aliasing BYTEROVER_BLOCK_MARKERS = BRV_RULE_MARKERS so they're byte-identical means the file content alone cannot distinguish "rules-connector managed block" from "skill autonomous-attachment managed block." Today this is safe because the two paths never collide (rules connectors live under projectRoot, autonomous attachments live in ~/.hermes/SOUL.md or <openclaw-workspace>/AGENTS.md).
But OpenClaw's autonomous attachment writes into AGENTS.md, which is the same filename used for shared rules-connector targets (Amp/Codex/OpenCode). If a future change ever pointed an OpenClaw workspace at the same project root, both flows would compete for the same markers and clobber each other.
Worth either:
- a comment here calling out the "ok because target paths never overlap" invariant, or
- giving the autonomous-attachment block distinct markers (e.g.
BEGIN BYTEROVER SKILL) so the two are textually disjoint.
| @@ -120,10 +131,14 @@ export class SkillConnector implements IConnector { | |||
| }), | |||
| ) | |||
|
|
|||
| await this.upsertAutonomousAttachment(config) | |||
|
|
|||
| return { | |||
| alreadyInstalled: false, | |||
| alreadyInstalled, | |||
| configPath: fullDir, | |||
| message: `Skill connector installed for ${agent} (created ${fullDir}/)`, | |||
| message: alreadyInstalled | |||
| ? `Skill connector refreshed for ${agent}` | |||
| : `Skill connector installed for ${agent} (created ${fullDir}/)`, | |||
| success: true, | |||
There was a problem hiding this comment.
issue (data loss on re-install): install writes all SKILL_FILE_NAMES with 'overwrite' unconditionally on every re-install. The PR description calls this out as intentional ("re-install refreshes (no longer short-circuits as 'already installed')"), but the side effect is that any user edits to query.md/curate.md/etc. are silently destroyed — the function returns success: true, alreadyInstalled: true with no indication that local content was overwritten.
writeSkillFiles below already has a per-file existence check that skips already-existing files. That pattern is the safer one for managed-but-user-editable docs: write missing files, leave existing ones alone, and require an explicit --force (or uninstall + install) to regenerate.
If the templates are not meant to be user-editable, consider documenting that in the generated SKILL.md ("Do not edit — regenerated by brv hub install") so users discover the contract before they lose work.
| function normalizeOpenClawAgentId(value: string | undefined): string { | ||
| const trimmed = value?.trim() ?? '' | ||
| if (!trimmed) return DEFAULT_OPENCLAW_AGENT_ID | ||
| const lowered = trimmed.toLowerCase() | ||
| if (VALID_OPENCLAW_ID_RE.test(trimmed)) { | ||
| return lowered | ||
| } | ||
|
|
||
| return ( | ||
| lowered | ||
| .replaceAll(INVALID_OPENCLAW_ID_CHARS_RE, '-') | ||
| .replace(LEADING_DASH_RE, '') | ||
| .replace(TRAILING_DASH_RE, '') | ||
| .slice(0, 64) || DEFAULT_OPENCLAW_AGENT_ID | ||
| ) | ||
| } |
There was a problem hiding this comment.
nitpick: INVALID_OPENCLAW_ID_CHARS_RE is declared with the g flag, which is required for replaceAll(regex, ...) to not throw, but obscures intent. Since replaceAll works with either a string or a global regex, this is correct — just consider a comment noting that the g flag is mandatory here, otherwise a future hand at "tidying the regex" by dropping g will get a TypeError at runtime that the test suite may not cover for every invalid input.
| export function resolveUserPath(input: string, options?: AgentPathResolverOptions): string { | ||
| const value = input.trim() | ||
| const homeDir = resolveHomeDir(options) | ||
| if (value === '~') { | ||
| return homeDir | ||
| } | ||
|
|
||
| if (value.startsWith('~/')) { | ||
| return path.join(homeDir, value.slice(2)) | ||
| } | ||
|
|
||
| if (path.isAbsolute(value)) { | ||
| return value | ||
| } | ||
|
|
||
| return path.join(homeDir, value) | ||
| } |
There was a problem hiding this comment.
nitpick (cross-platform): resolveUserPath (used by resolveHermesHome) handles ~ and ~/foo but not ~\foo (Windows backslash). resolveOpenClawUserPath correctly handles both via /^~[\\/]/u. For Windows users who set HERMES_HOME=~\custom, the path would silently fall through to path.join(homeDir, '~\\custom') — a path with a literal ~\ in it.
Cheap fix: tweak the second branch to match either separator:
if (value.startsWith('~/') || value.startsWith('~\\')) {
return path.join(homeDir, value.slice(2))
}or mirror the OpenClaw replace(/^~(?=$|[\\/])/u, homeDir) pattern.
| async write(filePath: string, serverConfig: McpServerConfig): Promise<void> { | ||
| let data: Record<string, unknown> = {} | ||
|
|
||
| if (await this.fileService.exists(filePath)) { | ||
| try { | ||
| data = parseYamlAsRecord(await this.fileService.read(filePath)) | ||
| } catch { | ||
| // File exists but contains invalid/empty YAML — start fresh | ||
| } | ||
| } | ||
|
|
||
| set(data, this.serverKeyPath, {...serverConfig}) | ||
| await this.fileService.write(yamlDump(data), filePath, 'overwrite') | ||
| } |
There was a problem hiding this comment.
issue (silent data loss): write opens an existing file, parses the YAML, and on parse failure silently resets data = {} (per the inline comment at line 96). That means if a Hermes user has hand-edited ~/.hermes/config.yaml and introduced a typo that makes the file invalid YAML, brv hub install ... --agent Hermes (or brv connectors install) will overwrite the entire file with just {mcp_servers: {brv: ...}}, destroying everything else they had.
exists() and remove() correctly degrade to "no-op / fileExists=false" on parse errors; write() should at least:
- back up the file (the codebase already has
IFileService.createBackup), or - refuse to write and return an error so the caller can surface it, or
- log a warning.
Right now this is fail-open destructive, which is the opposite of the "Non-destructive: Preserves user's existing MCP servers and other config" safety claim in mcp-connector.ts's header docstring.
| Hermes: { | ||
| configPathResolver: () => path.join(resolveHermesHome(), 'config.yaml'), | ||
| format: 'yaml', | ||
| mode: 'auto', | ||
| scope: 'global', | ||
| serverConfig: DEFAULT_SERVER_CONFIG, | ||
| serverKeyPath: ['mcp_servers', 'brv'], | ||
| }, |
There was a problem hiding this comment.
nitpick (consistency): The Hermes configPathResolver calls resolveHermesHome() with no options argument, which means it reads process.env.HERMES_HOME and os.homedir() directly — bypassing the env/homeDir injection path that the rest of this PR plumbs through AgentPathResolverOptions.
In production this is fine (the daemon reads the real env), but it makes getConfigPath('Hermes') non-injectable in tests — the Hermes-specific test at mcp-connector.test.ts:484 has to mutate process.env.HERMES_HOME directly to verify the override. If you ever introduce a McpConnectorOptions.env/homeDir parallel to the Skill connector's, this is the call site that won't honor it.
Not blocking, just flagging the asymmetry between Skill and MCP connector resolution paths.
|
Claude finished @DatPham-6996's task in 3m 48s —— View job Claude PR Review — re-review after syncOverall score: 4 / 5 — the architectural split and idempotent-block design remain strong; this sync commit was a bulk pull of Since last review (2026-05-18)
Status of prior-review findings against current HEAD
Issues (re-flagged in priority order)
Nitpicks (re-flagged)
Tests & build (unchanged from prior review)
Checklist
|
Add a post-curate self-review sub-skill and substantially strengthen the
curate authoring guide so saved topics are standalone explainers, not
changelogs.
- New src/server/templates/skill/curate-judgement.md: post-write
self-review. After `brv curate` reports done, judge the stored,
rendered topic against curate.md's Quality Bar with a hard cap of one
enhancement pass (references the rubric instead of restating it to
avoid drift).
- Register curate-judgement.md in SKILL_FILE_NAMES
(skill-connector-config.ts) and reference it from SKILL.md (detail
file list + command reference).
- curate.md: document the JSON envelope contract ({html, meta}
replaces bare HTML on --response), add the four-dimension Quality
Bar, decision-flow diagram, rationalizations/red-flags tables, a
richer worked example exercising all dimensions, and a pre-send
self-check; expand Required Preservation and Common Mistakes.
- Add unit coverage asserting curate-judgement.md installs as a lean
self-review guide that points at the Quality Bar, `brv read`, and the
one-pass --overwrite cap.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @DatPham-6996's task in 2m 31s —— View job Claude PR Review — synchronize re-reviewOverall score: 4 / 5 — the new Since last review (2026-05-19 03:03 UTC)
Notes on the new
|
| Prior issue | File:line | Status |
|---|---|---|
Hardcoded workdir="~/.hermes/byterover/" ignoring HERMES_HOME |
src/server/templates/skill/curate.md:12, query.md:10 |
Not addressed — still hardcoded in both; double-space typo (at ) still present in both |
YAML config writer silently resets to {} on parse failure |
src/server/infra/connectors/mcp/yaml-mcp-config-writer.ts:88-101 |
Not addressed — still fail-open destructive on invalid YAML |
install() overwrites all SKILL_FILE_NAMES unconditionally |
src/server/infra/connectors/skill/skill-connector.ts:126-132 |
Not addressed — still uses 'overwrite' for every file; writeSkillFiles keeps its safer per-file existence check, but install does not |
BYTEROVER_BLOCK_MARKERS = BRV_RULE_MARKERS identity |
src/server/infra/connectors/shared/constants.ts:22-29 |
Resolved via documentation — docstring now states the invariant (legacy detection compat) |
defaultScopeForAgent uses as Record<...> widening cast |
src/server/infra/transport/handlers/hub-handler.ts:95 |
Not addressed — still violates CLAUDE.md "Avoid as Type assertions" |
agent-path-resolver.ts:resolveUserPath Windows ~\foo gap |
src/server/infra/connectors/shared/agent-path-resolver.ts:29 |
Not addressed — ~/ only; resolveOpenClawUserPath (line 70-77) correctly handles both via /^~[\\/]/u — asymmetric |
mcp-connector-config.ts Hermes configPathResolver ignores injection |
src/server/infra/connectors/mcp/mcp-connector-config.ts:205 |
Not addressed — still () => path.join(resolveHermesHome(), 'config.yaml') with no options |
openclaw.json parsed with yamlLoad |
src/server/infra/connectors/skill/autonomous-agent-attachments.ts:83 |
Not addressed |
SKILL_FILE_NAMES[0] magic indexing in uninstall |
src/server/infra/connectors/skill/skill-connector.ts:255, 274 |
Not addressed — install() uses MAIN_SKILL_FILE_NAME (line 123) but uninstall still uses SKILL_FILE_NAMES[0] |
INVALID_OPENCLAW_ID_CHARS_RE g flag intent |
src/server/infra/connectors/skill/autonomous-agent-attachments.ts:22 |
Not addressed — no clarifying comment |
Re-flagged in priority order (carried forward, unchanged from prior review)
-
yaml-mcp-config-writer.ts:88-101— silent data destruction on invalid YAML.write()still falls through todata = {}on parse failure and overwrites the entire file with just{mcp_servers: {brv: ...}}. Contradicts the connector's own "Non-destructive" header docstring claim. Same destructive path runs for bothbrv hub install --agent Hermesandbrv connectors install. Suggest backup-or-refuse on invalid YAML (IFileService.createBackupis available). Fix this → -
skill-connector.ts:126-132— re-install overwrites user-edited sub-skill files unconditionally. Intended ("re-install refreshes"), but it silently clobbersquery.md/curate.md/curate-judgement.md/ etc. and still returnssuccess: true, alreadyInstalled: truewith no signal. Withcurate-judgement.mdnow in the managed set, this risk surface is wider — a user who tweaks their judgement guide loses it on nextbrv hub install.writeSkillFiles(lines 333-348) already shows the right pattern. Match it ininstall(), gate the refresh behind--force, or add a "Do not edit — regenerated bybrv hub install" header to generated files. Fix this → -
src/server/templates/skill/curate.md:12&query.md:10—workdirhardcoded, doesn't honorHERMES_HOME. Skill install root + Hermes MCP config path both honorHERMES_HOME, but the skill content tells the agent tocd ~/.hermes/byterover/. For a Hermes user withHERMES_HOME=/opt/hermes, the skill is installed at/opt/hermes/skills/byterover/SKILL.mdbut the agent reads "runbrvat~/.hermes/byterover/" — pointing at the non-relocated home. Also still:at(double space) in both files. Either parameterize at install time or reword as(default: ~/.hermes/byterover/, overridable via $HERMES_HOME). Fix this → -
hub-handler.ts:93-98—as Typewidening cast still violates CLAUDE.md. Replace theRecord<string, {projectPath: null | string}>cast with anagent in SKILL_CONNECTOR_CONFIGStype guard. Fix this →
Nitpicks (carried forward)
agent-path-resolver.ts:29— Windows backslash gap.resolveUserPath(used for Hermes) handles~/but not~\;resolveOpenClawUserPath(line 70) handles both. Trivial fix:value.startsWith('~/') || value.startsWith('~\\').skill-connector.ts:255 & 274—uninstallstill usesSKILL_FILE_NAMES[0];MAIN_SKILL_FILE_NAMEis imported but only used at line 123. Easy follow-up to the partial fix already applied.mcp-connector-config.ts:205— HermesconfigPathResolverdoesn't accept options, sogetConfigPath('Hermes')is not injectable in tests (forcesprocess.envmutation inmcp-connector.test.ts:484). Asymmetric with the Skill connector'spathResolverOptions()plumbing.autonomous-agent-attachments.ts:22—gflag onINVALID_OPENCLAW_ID_CHARS_REis load-bearing forreplaceAll; a one-line comment would prevent a future "tidy" regression.autonomous-agent-attachments.ts:83-84— parsingopenclaw.jsonwithyamlLoad: works (YAML ⊃ JSON) but obscures intent.
Tests & build
- New unit coverage in
eafd498for thecurate-judgement.mdinstall path looks healthy (asserts the file installs as a lean self-review guide that points at the Quality Bar,brv read, and the one-pass--overwritecap). - Existing unit suites unchanged and still healthy.
- Gap (same as prior review): no new test covers (a) the YAML-parse-failure path in
yaml-mcp-config-writer.write(), or (b) the re-install-clobbers-edited-sub-skill path inskill-connector.install(). Adding those would have surfaced issues Feat/init #1 and Feat/refactor structure #2 as red tests.
Todos
- Fetch latest branch and diff against
origin/proj/byterover-tool-mode - Re-verify status of prior-review findings against current HEAD
- Read key changed files (yaml-mcp-config-writer, skill-connector, hub-handler, agent-path-resolver, templates, curate-judgement)
- Check whether any new commits since last review address open issues — they don't
- Post final summary with overall score
|
Claude finished @DatPham-6996's task in 3m 36s —— View job Claude PR Review — synchronize re-review (4th pass)Overall score: 4 / 5 — the architectural split, idempotent block management, and OpenClaw-faithful path resolution remain strong. This sync pulled Since last review (2026-05-19 10:26 UTC)
No targeted fixes landed for the four open priority issues from the prior reviews. Status of prior-review findings against current HEAD
Re-flagged inline (priority order)
Nitpicks ( Tests & build
Notes on the new
|
| async write(filePath: string, serverConfig: McpServerConfig): Promise<void> { | ||
| let data: Record<string, unknown> = {} | ||
|
|
||
| if (await this.fileService.exists(filePath)) { | ||
| try { | ||
| data = parseYamlAsRecord(await this.fileService.read(filePath)) | ||
| } catch { | ||
| // File exists but contains invalid/empty YAML — start fresh | ||
| } | ||
| } | ||
|
|
||
| set(data, this.serverKeyPath, {...serverConfig}) | ||
| await this.fileService.write(yamlDump(data), filePath, 'overwrite') | ||
| } |
There was a problem hiding this comment.
issue (silent data loss, still unaddressed since 2026-05-18): On YAML parse failure, write() falls through to data = {} and overwrites the entire file with just {mcp_servers: {brv: ...}}. If a Hermes user has hand-edited ~/.hermes/config.yaml and accidentally introduced invalid YAML, brv hub install --agent Hermes or brv connectors install will destroy everything else they had.
This contradicts the "Non-destructive: preserves user's existing MCP servers and other config" claim in mcp-connector.ts's header docstring. exists() and remove() already degrade safely on parse errors — write() should too.
Suggested options (any one):
- Back up via
IFileService.createBackupbefore overwriting, or - Throw so the caller can surface it to the user, or
- At minimum log a warning before clobbering.
This is the top-priority open issue from the prior reviews and the only one that can lead to silent data loss on a hand-edited user config. Also worth adding a unit test exercising the invalid-YAML branch in yaml-mcp-config-writer.test.ts. Fix this →
| @@ -120,10 +131,14 @@ export class SkillConnector implements IConnector { | |||
| }), | |||
| ) | |||
There was a problem hiding this comment.
issue (re-install clobbers user edits, still unaddressed since 2026-05-18): Every re-install writes every SKILL_FILE_NAMES entry with 'overwrite', then returns success: true, alreadyInstalled: true — silent data loss with a misleadingly-green return value. With curate-judgement.md and dream.md now in the managed set the surface area has only grown.
writeSkillFiles at lines 333–348 already shows the right pattern (per-file existence check; skip files that exist). At minimum, match that pattern in install(). Better still, pick one:
- Gate the refresh behind an explicit
--forceflag (current behavior becomes opt-in), or - Emit a
Do not edit — regenerated by \brv hub install`` header in every generated sub-skill file so users see the contract before they lose work, or - Return a
refreshed: true(and changed-files list) in the result so the CLI can surface that something was overwritten.
uninstall at l. 255 & 274 also still uses SKILL_FILE_NAMES[0] — swap to the already-imported MAIN_SKILL_FILE_NAME while you're in here. Fix this →
|
|
||
| No ByteRover LLM provider is required. The calling agent's own LLM authors the HTML response. | ||
|
|
||
| ***HIGHLY IMPORTANT***: For hermes agent, run brv commands at `workdir="~/.hermes/byterover/"`. |
There was a problem hiding this comment.
issue (consistency, still unaddressed since 2026-05-18): This workdir="~/.hermes/byterover/" (and the matching line at query.md:10) is hardcoded and does not follow HERMES_HOME. Everywhere else in this PR — resolveHermesHome, the Hermes MCP configPathResolver, the skill install root — honors HERMES_HOME.
A user who sets HERMES_HOME=/opt/hermes will get the skill installed at /opt/hermes/skills/byterover/SKILL.md and the MCP config at /opt/hermes/config.yaml, but the skill content still tells the agent to cd ~/.hermes/byterover/ — pointing at the non-relocated home. The agent then operates on the wrong root.
Two viable fixes:
- Parameterize at install time: render the resolved path during
SkillContentLoader.loadSkillFileso the file emitted to disk already has the user's real workdir baked in (and drop the env override from this template altogether). - Reword to make the override discoverable: e.g.
For hermes agent, run brv commands inside the Hermes workdir (default: ~/.hermes/byterover/; overridable via $HERMES_HOME).
Also: the line still has a double space after at (at `workdir=…``) — trivial but visible to the LLM consumer. Fix this →
| private defaultScopeForAgent(agent?: Agent): 'global' | 'project' { | ||
| if (!agent) return 'project' | ||
| const skillConfigs: Record<string, {projectPath: null | string}> = SKILL_CONNECTOR_CONFIGS | ||
| const config = skillConfigs[agent] | ||
| return config && !config.projectPath ? 'global' : 'project' | ||
| } |
There was a problem hiding this comment.
suggestion (typing, still unaddressed since 2026-05-18): The widening cast const skillConfigs: Record<string, {projectPath: null | string}> = SKILL_CONNECTOR_CONFIGS violates CLAUDE.md's "Avoid as Type assertions — use type guards or proper typing instead" rule. (It's the same pattern even though there's no literal as keyword — the explicit type annotation widens the type.) A type guard preserves the inferred typing and makes the fallback explicit:
| private defaultScopeForAgent(agent?: Agent): 'global' | 'project' { | |
| if (!agent) return 'project' | |
| const skillConfigs: Record<string, {projectPath: null | string}> = SKILL_CONNECTOR_CONFIGS | |
| const config = skillConfigs[agent] | |
| return config && !config.projectPath ? 'global' : 'project' | |
| } | |
| private defaultScopeForAgent(agent?: Agent): 'global' | 'project' { | |
| if (!agent) return 'project' | |
| if (!(agent in SKILL_CONNECTOR_CONFIGS)) return 'project' | |
| const config = SKILL_CONNECTOR_CONFIGS[agent as keyof typeof SKILL_CONNECTOR_CONFIGS] | |
| return config.projectPath ? 'project' : 'global' | |
| } |
This also makes the "non-skill agents fall through to project" path explicit rather than relying on the cast leaving config as undefined. Fix this →
| return homeDir | ||
| } | ||
|
|
||
| if (value.startsWith('~/')) { |
There was a problem hiding this comment.
nitpick (cross-platform, still unaddressed since 2026-05-18): resolveUserPath (used by resolveHermesHome) handles ~/ but not ~\ (Windows backslash). resolveOpenClawUserPath at l. 64–77 correctly handles both via /^~[\\/]/u. A Windows user with HERMES_HOME=~\custom would silently get path.join(homeDir, '~\\custom') — a literal ~\ inside the path.
Cheap fix:
if (value.startsWith('~/') || value.startsWith('~\\')) {
return path.join(homeDir, value.slice(2))
}or mirror the OpenClaw pattern: value.replace(/^~(?=$|[\\/])/u, homeDir) + path.resolve.
Audited each sub-skill (excluding curate / query) against the actual `brv <topic> --help` surface and patched the gaps: - vc.md: added init, clone, config, checkout, merge, fetch, remote; added real examples for reset (was only mentioned as "avoid"). Regrouped Quick Reference by workflow phase (Setup / Inspect / Stage+commit / Branch+checkout / Sync / Merge / Undo). Expanded Safety + Common Mistakes to cover missing-upstream, missing-config, and switching with dirty tree. - swarm.md: added `brv swarm onboard` (interactive setup wizard) + a Setup section explaining when to use it. - review.md: added the toggle subcommands (`brv review`, `--enable`, `--disable`) that the description verb already implied but the body missed entirely. Updated frontmatter to include enable/disable so the matcher fires on those prompts. - history.md: surfaced the filter flags that were buried under `--help` — `--limit`, `--since`, `--before`, `--status`, `--tier`, `--last`, `--format narrative`. Regrouped Quick Reference into Curate / Query / Aggregate sections. - troubleshooting.md: added recovery commands (`brv restart` for crash recovery, `brv logout` + `brv login` reset flow, `brv update` for stale-CLI mismatch) + `brv connectors` to Quick Diagnosis.
|
Claude finished @cuongdo-byterover's task in 2m 17s —— View job Claude PR Review — synchronize re-review (5th pass)Overall score: 4 / 5 — the architectural split, idempotent block management, and OpenClaw-faithful path resolution remain strong. The new commit since the prior review ( Since last review (2026-05-20 09:32 UTC)
Status of prior-review priority findings against current HEAD
The inline comments from the 4th-pass review (posted 2026-05-20 09:34 UTC) at Notes on
|
cuongdo-byterover
changed the title
cuongdo-byterover
changed the title
|
Claude finished @DatPham-6996's task in 2m 10s —— View job Claude PR Review — synchronize re-review (6th pass)Overall score: 4 / 5 — architecture and idempotent-block design remain solid. The only new commit since the 5th-pass review ( To avoid further noise I am not re-posting those inline comments — please refer to the existing inline threads (most recently re-filed 2026-05-20 09:34 UTC) for fix suggestions / Since last review (2026-05-20 10:25 UTC)
Status of prior-review priority findings against current HEADRe-verified each against current files; nothing has shifted since the 5th-pass review except for the
Tests & build
RecommendationApprove once at least these two top-priority items land — they're the only ones that can lose user content silently:
Items 3 ( Checklist
|
Add a guided 90-second onboarding skill that runs when a user asks
"Show me how ByteRover works" (or any semantic equivalent). The tour
captures the user's persona and the pain that brought them to
ByteRover, persists both locally, and demonstrates how ByteRover
ends the re-explain tax — all without an account, API key, or
provider config.
src/server/templates/skill/onboarding.md (new):
- Msg 1: trust opener (no command names) → one-sentence concept
primer → quick interview (identity + pain in one open question) →
human-reaction beat before curate → visible artifact with pain
bullet + sticky label ("re-explain tax") + behavioral commitment
→ 2-beat concept map with visual separation before pause
- Msg 2: named action ("brv query"), one-line retrieval summary,
conditional bonus-context branch when existing project knowledge
is found, identity-and-pain reflection, future-self demonstration
of the pain ending, loop name + cross-session promise
- Msg 3: where memory lives (one sentence, no commands) →
activation step that detects CLAUDE.md/AGENTS.md/etc. and offers
to curate as starter project context (or prompts for one rule)
→ done signal with two equal-weight paths (curate more vs.
resume work) so users exit without homework
- Spec guards: don't manufacture pains, don't overpromise on
out-of-scope pains, don't turn the artifact into a confirmation,
don't tailor with hollow phrases, don't auto-fire next message
src/server/templates/skill/SKILL.md:
- Add First-Turn Routing block before the Iron Law that hands off
to onboarding.md on tour/intro/walkthrough requests
- Update frontmatter description so the skill router activates the
skill on tour-related phrasings
src/server/infra/connectors/skill/skill-connector-config.ts:
- Add onboarding.md to SKILL_FILE_NAMES so the connector copies it
during install
feat: [ENG-2536] add ByteRover onboarding tour skill
Msg 1 closes with a teaser line ("Also version-controlled, cloud-syncable,
and shareable across agents — more at the end") so users know more controls
exist without diluting the local-only trust climax. Msg 3 Part 1 delivers
on it with three short bullets covering brv vc, brv vc push/pull, and
connector cross-agent sharing.
Without this, the tour leaves users thinking ByteRover is local-only with
no team or sync story — they discover the full surface area accidentally
or not at all.
3 participants
Summary
SKILL.mdwith noprogressive disclosure, and autonomous agents (OpenClaw, and now Hermes)
had no reliable way to always load the ByteRover memory contract into their
system prompt. Hub install also hardcoded
scope: 'project', which threw"does not support project scope" for global-only skill agents.
focused sub-skills, predictable folder structure) makes the skill easier
for agents to navigate and keeps the always-on memory rule in front of
autonomous agents without bloating every invocation.
SKILL.mdinto a thin entrypoint plus focused sub-skills(
query/curate/review/swarm/vc/history/troubleshooting.md);SKILL_FILE_NAMESenumerates the full managed set and status/uninstallkey off it.
Hermesagent (skill + mcp), connector icon, and a YAML MCPconfig writer;
McpConfigFormatnow supports'yaml'(
~/.hermes/config.yaml).BYTEROVER RULESblock injection into Hermes/OpenClawsystem-prompt files via
autonomous-agent-attachments.ts+upsert/remove/hasByteroverBlock(idempotent re-install; stale/missingblock reported as not-installed).
agent-path-resolver/skill-path-resolverhonoringHERMES_HOME/OPENCLAW_STATE_DIR/OPENCLAW_CONFIG_PATHandper-agent
globalRoot; OpenClaw skills moved to the openclaw-state root.per-agent default (global for global-only skill agents);
writeSkillFilestakes object params with idempotent per-file existence checks.
context-tree storage, billing, or the transport schema beyond the existing
hub-install scope being optional. Existing home/project-rooted agents
(Claude, Cursor, etc.) keep their current paths and behavior.
Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)Test plan
test/unit/infra/connectors/mcp/yaml-mcp-config-writer.test.tstest/unit/infra/connectors/shared/agent-path-resolver.test.tstest/unit/infra/connectors/connector-manager-attachment.test.tstest/unit/infra/connectors/shared/rule-segment-patcher.test.tstest/unit/infra/connectors/skill/skill-connector.test.tstest/unit/infra/connectors/mcp/mcp-connector.test.tstest/commands/hub-install-scope.test.tstest/unit/infra/hub/hub-handler.test.tstest/unit/infra/hub/hub-install-service.test.tsresolution for Hermes/OpenClaw; autonomous-block upsert/remove/has
(idempotency + stale-block detection); per-agent default scope inference
in hub install; skill connector install/status/uninstall with attachment
block; object-param
writeSkillFilesidempotent file checks.User-visible changes
Hermesagent selectable in hub install and the WebUI connectors panel(with icon).
brv hub installno longer defaults--scopetoproject; the daemoninfers the default per agent (global-only agents like Hermes/OpenClaw
install globally without erroring).
a managed
BYTEROVER RULESblock in their system-prompt files.SKILL.md; re-install refreshes (no longer short-circuits as "alreadyinstalled").
Evidence
Attach at least one:
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
files could clobber user content.
<!-- BEGIN/END BYTEROVER RULES -->markers; upsert replaces only the bounded block and preserves surrounding
content; removal rejoins cleanly. Covered by rule-segment-patcher tests.
callers that previously relied on the
projectdefault.--scope/data.scopestill wins; default onlyflips to
globalfor agents with no project skill path. Covered byhub-install-scopeandhub-handlertests.HERMES_HOME,OPENCLAW_STATE_DIR,OPENCLAW_CONFIG_PATH) could resolve unexpected locations.unit-tested with injected env/home.