Feat/eng 2697#733
Conversation
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
|
Claude finished @DatPham-6996's task in 2m 37s —— View job Review Summary — Score: 4 / 5
A focused, well-scoped documentation PR. The behavioral contract change ("after What's strong
What's worth addressing
Untouched but worth a quick second look
5 inline comments queued (1 praise on the test, 1 suggestion on SKILL.md, 1 nit + 1 issue + 1 question on curate.md). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Update ByteRover skill docs to avoid hardcoded localhost:7700 links now that brv webui resolves the active port. - point post-curate and onboarding guidance to the URL printed by brv webui - document brv webui --port <port> for port conflicts or custom ports - add skill connector regression coverage to reject fixed Web UI localhost URLs
|
Claude finished @DatPham-6996's task in 4m 45s —— View job Review Summary — Score: 4 / 5
The latest commit ( The JSON envelope examples in What's strong
What's worth addressing
Untouched but worth a quick second look
6 inline comments queued (2 praises, 1 issue, 2 suggestions, 1 question, 1 nit — plus the carry-overs from the prior round that the latest commit did not address). · Branch |
Update ByteRover skill docs to avoid hardcoded localhost:7700 links now that brv webui resolves the active port. - point post-curate and onboarding guidance to the URL printed by brv webui - document brv webui --port <port> for port conflicts or custom ports - add skill connector regression coverage to reject fixed Web UI localhost URLs
|
Claude finished @DatPham-6996's task in 4m 37s —— View job Review Summary — Score: 3 / 5
The latest commit ( What's strong
What's worth addressing
Untouched but worth a quick look
7 inline comments queued (3 praises, 2 issues, 2 suggestions, 1 question, 1 nit — including the carry-overs the latest commit did not address). · Branch |
| ``` | ||
| 4. Branch on `data.status`: | ||
| - `done` - report `data.filePath`. | ||
| - `done` - report `data.filePath`, then give the user the served Web UI link, such as `http://localhost:<served-port>`, so they can see the saved topic in the Contexts page. Use default `http://localhost:7700` unless a custom port is already serving. If that link does not open, tell the user they can run `brv webui` to open the dashboard; use `brv webui --port <port>` only when the user asks to open/change the dashboard port or the current port has a conflict. |
There was a problem hiding this comment.
issue (the central one — still not fixed by templates alone): http://localhost:<served-port> is a literal placeholder. The agent has no signal that lets it substitute <served-port> — the curate done envelope (src/oclif/lib/curate-session.ts:111-145) only carries filePath, ok, status, and optional warnings. There is no dashboardUrl, no port, no daemon hint. So the agent is left with three bad options:
- Echo
<served-port>verbatim. The user sees a broken-looking URL with angle brackets — not clickable, not informative. - Default to
http://localhost:7700(what this paragraph says to do "unless a custom port is already serving"). The agent can't know whether a custom port is serving from the envelope, so it falls back to 7700 in every case. For users who setBRV_WEBUI_PORTor persisted a different port, that URL is wrong — same drift the previous round set out to fix. - Run
brv webuito discover the port. That command unconditionally callsopen(url)(src/oclif/commands/webui.ts:49) — every curate spawns a browser tab.
The previous round's fix (delete the literal 7700 from templates) was right in spirit, but the templates can't actually surface the real port without help from the runtime. The clean closure is the one previously proposed: add dashboardUrl to the curate done envelope by resolving the persisted port via webui:getPort, then have the docs say data.filePath and data.dashboardUrl — no placeholder, no brv webui shell-out, no port drift.
This is the same point made on curate.md and webui.ts in earlier rounds. It is the only fix that makes "tell the user where to see what was saved" honest across BRV_WEBUI_PORT overrides, persisted custom ports, and silent/chained curates.
| # After a successful curate, share the served Web UI link: http://localhost:<served-port> | ||
| # default http://localhost:7700 unless a custom Web UI port is already serving | ||
| # If that link does not open, tell the user they can run brv webui |
There was a problem hiding this comment.
nit (carry-over from previous rounds, still applies): These three comments sit directly under brv review pending --format json, so on a first read the "after a successful curate" lines associate with the review command rather than the two brv curate examples above. Hoisting them up to immediately after brv curate view --detail (line 29) keeps the topical grouping tight: curate examples → "and here's how to show the user what was saved" → unrelated review command. This was flagged in both prior reviews.
| # After a successful curate, share the served Web UI link: http://localhost:<served-port> | |
| # default http://localhost:7700 unless a custom Web UI port is already serving | |
| # If that link does not open, tell the user they can run brv webui | |
| brv curate view --detail | |
| # After a successful curate, share the served Web UI link: http://localhost:<served-port> | |
| # default http://localhost:7700 unless a custom Web UI port is already serving | |
| # If that link does not open, tell the user they can run brv webui | |
| brv review pending --format json |
| | Omitting `keywords` when retrieval terms are obvious | Add comma-separated `keywords` on `<bv-topic>` | | ||
| | Reporting completion before a session reaches `data.status: "done"` | Wait for `done` before telling the user the topic is saved | | ||
| | Overwriting an existing path without preserving prior facts | Merge existing content unless the user explicitly wants replacement | | ||
| | Saying the topic is saved without showing the user where to see it | After `done`, give the user `data.filePath` and the served Web UI link, such as `http://localhost:<served-port>` | |
There was a problem hiding this comment.
question (carry-over, third round): This row still mandates "give the user data.filePath and the served Web UI link" on every done. For curates that the agent runs silently as part of a larger task (mid-implementation curates during a long Claude Code session, brv dream chains, autonomous loops), surfacing a URL on every successful save becomes noise — and per the issue on curate.md:50, the agent's only way to actually obtain a valid URL is to spawn brv webui, which opens a browser tab.
Consider scoping the rule to user-initiated curates, e.g.:
"When the user explicitly asked to curate, give them
data.filePathand the dashboard URL. For curates the agent initiates inside a larger task, just recorddata.filePathfor the run summary."
This was raised in both the May 29 and May 31 reviews and is still relevant after the latest commit.
| } | ||
| ``` | ||
|
|
||
| → Only now is the topic saved. Report `data.filePath` and hand the user the served Web UI link, such as `http://localhost:<served-port>`, so they can open the Contexts page and see it. Use default `http://localhost:7700` unless a custom port is already serving. If that link does not open, tell the user they can run `brv webui` to open the dashboard; use `brv webui --port <port>` only when the user asks to open/change the dashboard port or the current port has a conflict. |
There was a problem hiding this comment.
suggestion (still relevant after the rewrite): Same "port conflict" phrasing here as in the previous round. Bare brv webui cannot conflict — it just reads the daemon's persisted port via webui:getPort (src/oclif/commands/webui.ts:30-41). EADDRINUSE only fires when the user explicitly passes --port (src/server/infra/webui/webui-server.ts:38-39). So the --port <port> fallback is really for user preference ("I want to move the dashboard"), not conflict recovery.
The same wording is repeated on curate.md:50, curate.md:197, in the Quick Reference block (L31-33), in the SKILL.md command map (L177), and on onboarding.md:138. Worth tightening to something like "if the user wants a different port, run brv webui --port <port>" so the agent doesn't go searching for a conflict that won't appear from bare brv webui.
| - `brv swarm <query|curate|status>` — cross-source memory federation. See `swarm.md`. | ||
| - `brv vc <init|status|add|commit|...>` — git-style version control of the context tree. See `vc.md`. | ||
| - `brv dream <scan|finalize|undo>` — three-phase context-tree cleanup (link / merge / prune / synthesize). See `dream.md`. | ||
| - `brv webui [--port <n>]` — open or reconfigure the ByteRover dashboard when needed. For routine curate and onboarding closeouts, share the served Web UI link directly: `http://localhost:<served-port>`; default `http://localhost:7700` unless a custom port is already serving. The **Contexts page** renders everything saved under `.brv/context-tree/`. If that link does not open, tell the user they can run `brv webui` to open the dashboard; use `brv webui --port <port>` only when the user asks to open/change the dashboard port or the current port has a conflict. See `curate.md`. |
There was a problem hiding this comment.
suggestion (non-blocking): This command-map row is long and leads with brv webui as the action. The line tries to balance "share the URL" vs "run the command," but at this scale (3 sentences in one bullet) the agent will likely zero in on "open or reconfigure the ByteRover dashboard when needed" as the lead intent. The other detail files (onboarding.md:138, curate.md:50/112/197) all lead with the URL and demote brv webui to the fallback. Mirroring that order here keeps the three documents internally consistent — the agent reads one ordering rule across the whole bundle.
Same <served-port> placeholder issue applies as raised on curate.md:50 — the agent has no signal to fill in the port, so this line will resolve to either a literal localhost:<served-port> echo or a literal localhost:7700 regardless of the user's actual config. Templating data.dashboardUrl from the curate envelope is the durable fix.
| for (const content of contents) { | ||
| expect(content).not.to.include('printed Web UI URL') | ||
| expect(content).not.to.include('run `brv webui` after every successful curate') | ||
| } |
There was a problem hiding this comment.
praise: This negative-assertion-over-every-file pattern is exactly the right guardrail for template drift. Pinning the absence of printed Web UI URL and run \brv webui` after every successful curateacross the wholeSKILL_FILE_NAMES` set means a future template tweak that reintroduces either phrase fails one test instead of leaking into agent guidance. Cheap, durable, and the right granularity.
nit (carry-over from May 31 review): This test still exercises only the Claude Code agent. The SkillConnector ships the same template bundle to every agent in AGENT_CONNECTOR_CONFIG that supports 'skill' (see the EXPECTED_SUPPORTED_AGENTS list at the top of this file — Cursor, Codex, Github Copilot, Hermes, OpenClaw). A tiny for (const agent of EXPECTED_SUPPORTED_AGENTS) wrapper would catch any agent-specific override that reintroduces the literal port for free — same logic that drove EXPECTED_SUPPORTED_AGENTS in the first place.
| The browser URL is the **verifiable** trust proof — the user can click it in 2 seconds and see their memory in a real local dashboard. Stronger than any worded assurance. | ||
|
|
||
| Do NOT tell the user to "run `brv webui`" — the daemon auto-starts the web server on the persisted port (default 7700). The URL works as soon as the daemon is alive, which it already is. | ||
| Use the served Web UI link, such as `http://localhost:<served-port>`; default `http://localhost:7700` unless a custom port is already serving. Do NOT run `brv webui` as part of every tour closeout just to produce this link. If that link does not open, tell the user they can run `brv webui` to open the dashboard. Use `brv webui --port <port>` only when the user asks to open/change the dashboard port or the current port has a conflict. |
There was a problem hiding this comment.
praise: Replacing the old "Do NOT tell the user to run brv webui" line — which directly contradicted the new curate guidance — with a single coherent rule is the right reconciliation. SKILL.md, curate.md, and onboarding.md now agree on "share the URL first, fall back to the command if the link doesn't open."
issue (same root cause as the comment on curate.md:50): The <served-port> literal still appears on onboarding.md:103 ("http://localhost:<served-port> — or run brv webui if that link doesn't open.") and again on onboarding.md:130 (inside the example artifact block). In the artifact block specifically, the user sees that text rendered during the tour — the trust opener message tells the user "see it in your browser: http://localhost:<served-port>". If the agent doesn't substitute the port (and it has no clean way to), the trust-proof URL the onboarding tour shows is literally broken with angle brackets. That's the opposite of what the "verifiable trust proof" line one paragraph below promises.
The most robust fix is the same one raised on curate.md:50: have the daemon hand the agent a real port (via webui:getPort exposed in the curate/onboarding flow), and have the docs say "use the port the daemon returned" instead of carrying a placeholder through to user-visible text.
1 participant
Summary
brv curate, the skill instructed the in-daemon agent to report onlydata.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.http://localhost:7700by 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."done, the agent now hands the user the clickable dashboard URL (Contexts page) alongsidefilePath. Addedbrv webuito 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.brv webuicommand, the webui server, the curate engine, and port handling are untouched — this is documentation/behavioral-contract text plus a test assertion. The7700default is referenced, not introduced.Type of change
Scope (select all touched areas)
(Skill templates live under
src/server/templates/skill/and are deployed/read via the SkillConnector inserver/infra/connectors/skill/.)Linked issues
Root cause (bug fixes only, otherwise write
N/A)Test plan
test/unit/infra/connectors/skill/skill-connector.test.tsSKILL.mdincludesbrv webui.curate.mdincludeslocalhost:7700and the## Example Session Responsessection.User-visible changes
brv curate, the agent now gives the user the dashboard URLhttp://localhost:7700(Contexts page) alongside the savedfilePath, instead of reporting only the path.SKILL.mdQuick Reference and command list now documentbrv webui.brvCLI flags, defaults, or command output changed.Evidence
Attach at least one:
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
npm testnot run locally)npm run lint) — not runnpm run typecheck) — not runnpm run build) — not rundocs(skill): point users to the webui dashboard after curate)main(origin/main is an ancestor of HEAD)Risks and mitigations
http://localhost:7700; if the user changed the webui port (BRV_WEBUI_PORTor a persisted port), the URL the agent surfaces will be wrong.7700is the documented first-run default and the most common case; the docs also tell the agent thatbrv webuiopens the same dashboard. Templating the actual port into the message is a reasonable follow-up.brvis running, so a curate succeeding implies the daemon is up; worst case is a stale link, no functional impact.