Skip to content

Feat/eng 2697#733

Open
DatPham-6996 wants to merge 2 commits into
mainfrom
feat/ENG-2697
Open

Feat/eng 2697#733
DatPham-6996 wants to merge 2 commits into
mainfrom
feat/ENG-2697

Conversation

@DatPham-6996
Copy link
Copy Markdown
Collaborator

Summary

  • Problem: On a successful brv curate, the skill instructed the in-daemon agent to report only data.filePath. It never pointed users at the ByteRover dashboard where the saved topic is actually viewable, so users had no obvious way to see what was just curated.
  • Why it matters: The dashboard already exists and the daemon serves it on http://localhost:7700 by default — but the skill never surfaced it, leaving a working capability undiscovered and the curate loop ending at "a file path" instead of "here's your knowledge, rendered."
  • What changed: Updated the skill templates the curate agent reads at runtime. On done, the agent now hands the user the clickable dashboard URL (Contexts page) alongside filePath. Added brv webui to SKILL.md's Quick Reference + command list, an "Example Session Responses" JSON walkthrough and a "View What You Saved" section to curate.md, plus a new Common Mistakes row. Tightened the connector test to guard the new content.
  • What did NOT change (scope boundary): No runtime, CLI, or transport code. The brv webui command, the webui server, the curate engine, and port handling are untouched — this is documentation/behavioral-contract text plus a test assertion. The 7700 default is referenced, not introduced.

Type of change

  • Bug fix
  • New feature
  • Refactor (no behavior change)
  • Documentation
  • Test
  • Chore (build, dependencies, CI)

Scope (select all touched areas)

  • TUI / REPL
  • Agent / Tools
  • LLM Providers
  • Server / Daemon
  • Shared (constants, types, transport events)
  • CLI Commands (oclif)
  • Hub / Connectors
  • Cloud Sync
  • CI/CD / Infra

(Skill templates live under src/server/templates/skill/ and are deployed/read via the SkillConnector in server/infra/connectors/skill/.)

Linked issues

  • Closes #N/A (tracker is Linear, no GitHub issue)
  • Related: Linear ENG-2697 — "Rewrite ByteRover skill based on Superpowers skill" (https://linear.app/byterover/issue/ENG-2697). This is an enhancement layered on that rewrite.

Root cause (bug fixes only, otherwise write N/A)

  • Root cause: N/A
  • Why this was not caught earlier: N/A

Test plan

  • Coverage added:
    • Unit test
    • Integration test
    • Manual verification only
  • Test file(s): test/unit/infra/connectors/skill/skill-connector.test.ts
  • Key scenario(s) covered:
    • Installed SKILL.md includes brv webui.
    • Installed curate.md includes localhost:7700 and the ## Example Session Responses section.

User-visible changes

  • After a successful brv curate, the agent now gives the user the dashboard URL http://localhost:7700 (Contexts page) alongside the saved filePath, instead of reporting only the path.
  • SKILL.md Quick Reference and command list now document brv webui.
  • No brv CLI flags, defaults, or command output changed.

Evidence

Attach at least one:

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording

Before (old templates against the new assertions):

37 passing
2 failing
AssertionError: expected '---\nname: byterover...' to include 'brv webui'
AssertionError: expected '---\nname: byterover-curate...' to include 'localhost:7700'

After (npx mocha "test/unit/infra/connectors/skill/skill-connector.test.ts"):
39 passing (62ms)

Checklist

  • Tests added or updated and passing (ran the touched suite — 39 passing; full npm test not run locally)
  • Lint passes (npm run lint) — not run
  • Type check passes (npm run typecheck) — not run
  • Build succeeds (npm run build) — not run
  • Commits follow Conventional Commits format (docs(skill): point users to the webui dashboard after curate)
  • Documentation updated (this PR is the documentation)
  • No breaking changes
  • Branch is up to date with main (origin/main is an ancestor of HEAD)

Risks and mitigations

  • Risk: The dashboard URL is hardcoded to http://localhost:7700; if the user changed the webui port (BRV_WEBUI_PORT or a persisted port), the URL the agent surfaces will be wrong.
    • Mitigation: 7700 is the documented first-run default and the most common case; the docs also tell the agent that brv webui opens the same dashboard. Templating the actual port into the message is a reasonable follow-up.
  • Risk: The agent may surface the URL even when the daemon/webui isn't reachable.
    • Mitigation: The daemon serves the dashboard by default whenever brv is running, so a curate succeeding implies the daemon is up; worst case is a stale link, no functional impact.

Guide the in-daemon curate agent to surface the ByteRover dashboard
(http://localhost:7700, Contexts page) once a topic is saved, so users
can actually see what was curated.

- SKILL.md: add `brv webui` to the Quick Reference and command list,
  noting the daemon already serves the dashboard on port 7700
- curate.md: on `data.status: "done"`, hand the user the clickable
  dashboard URL alongside `filePath`; add a "View What You Saved"
  section and a "Example Session Responses" walkthrough (kickoff →
  correction → completion JSON envelopes); add a Common Mistakes row
  for reporting a save without showing where to view it
- skill-connector tests: assert SKILL.md mentions `brv webui` and
  curate.md mentions `localhost:7700` + the new examples section
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 29, 2026

Claude finished @DatPham-6996's task in 2m 37s —— View job


Review Summary — Score: 4 / 5

  • Read the three changed files
  • Check PR diff against origin/main
  • Review SKILL.md changes
  • Review curate.md changes
  • Review test changes
  • Post inline comments for any issues found
  • Post overall review summary with score

A focused, well-scoped documentation PR. The behavioral contract change ("after done, point the user at the dashboard") is sensible and matches the daemon's actual runtime — WEBUI_DEFAULT_PORT = 7700 in src/server/constants.ts:63 is served by webui-server.ts by default. The JSON envelopes added to curate.md are accurate to the codebase (json-response.ts:5 for {command,data,success,timestamp}; html-writer.ts:230 for the unknown-bv-element shape), and the test pins on substring assertions rather than wording, which keeps it robust without overfitting.

What's strong

  • praise: JSON examples reflect real runtime — success correctly mirrors data.ok, error kind matches the actual writer, and the needs-llm-step / done branching is faithful to curate-session.ts.
  • praise: Test additions are minimal and pin the right things (brv webui, localhost:7700, ## Example Session Responses).
  • praise: Risk section in the PR description already self-discloses the hardcoded port — that level of honesty is appreciated.

What's worth addressing

  • suggestion: SKILL.md's Quick Reference (L123) and Command Map (L177) lead with brv webui as the action. onboarding.md:138 in the same template set explicitly tells the agent not to do that ("Do NOT tell the user to 'run brv webui' — the daemon auto-starts the web server"). curate.md itself handles this well by leading with the URL; the SKILL.md entries should mirror that ordering so the three docs agree.
  • issue (acknowledged): 7700 is hardcoded in 6 spots in curate.md and 2 in SKILL.md. BRV_WEBUI_PORT / persisted port overrides will make every one of those URLs wrong. The daemon already exposes webui:getPort and curate's done envelope is a natural place to attach a real dashboardUrl field. Worth the small follow-up; docs alone can't keep this honest.
  • question: The new Common Mistakes row promotes "surface the URL after done" to mandatory. For silent / chained curates (mid-task agent curates, brv dream flows) this becomes noise. Consider scoping to user-initiated curates.
  • nit: curate.md:31 comment lands next to brv review pending and reads like a comment about review, not curate. Hoist it next to the curate examples.

Untouched but worth a quick second look

  • lint / typecheck / build were not run by the author; for a docs-only change that's low risk but the templates are loaded at runtime via the SkillConnector, so a npm run lint over the test file is cheap insurance.

5 inline comments queued (1 praise on the test, 1 suggestion on SKILL.md, 1 nit + 1 issue + 1 question on curate.md).
· Branch feat/ENG-2697

Comment thread src/server/templates/skill/SKILL.md
Comment thread src/server/templates/skill/curate.md
Comment thread src/server/templates/skill/curate.md
Comment thread src/server/templates/skill/curate.md
Comment thread test/unit/infra/connectors/skill/skill-connector.test.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant