Skip to content

update readme#90

Merged
cyyeh merged 1 commit into
mainfrom
cyyeh-patch-1
Mar 3, 2026
Merged

update readme#90
cyyeh merged 1 commit into
mainfrom
cyyeh-patch-1

Conversation

@cyyeh
Copy link
Copy Markdown
Owner

@cyyeh cyyeh commented Mar 3, 2026

No description provided.

Reintroduce security section detailing Bifrost LLM gateway and container isolation.
@cyyeh cyyeh merged commit fbb785c into main Mar 3, 2026
2 checks passed
@cyyeh cyyeh deleted the cyyeh-patch-1 branch March 3, 2026 03:05
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Mar 3, 2026

Claude finished @cyyeh's task in 1m 43s —— View job


Code Review — README Update

  • Gather context and understand the request
  • Review README.md changes
  • Provide detailed feedback

The README is comprehensive and well-written overall. Below are specific findings from the review.


Issues Found

1. Incorrect language hint on architecture diagram code block (README.md:221)

The architecture diagram is wrapped in a ```markdown fenced block. This is semantically wrong — the content is an ASCII art diagram, not Markdown. Some renderers may interpret the content and break the diagram's alignment. Use ```text or a bare ``` instead.

```text

Fix this →


2. Stale/inaccurate sidecar description in architecture diagram (~line 382)

The sidecar box in the architecture diagram says:

│  - Spawns CLI subprocess  │

The sidecar (sidecar/src/server.ts) uses the Claude Agent SDK directly via TypeScript — it does not spawn a CLI subprocess. This description was likely accurate in an earlier implementation but is now misleading.

Fix this →


3. Filename pattern inconsistency in persistence diagram (~line 405)

The persistence layer diagram shows:

│  │    *.SKILL.md           │   │

But the project uses skills/<name>/SKILL.md (no wildcard prefix). The * prefix is not part of the filename — it should read SKILL.md.

Fix this →


4. BIFROST_PORT referenced but not in the environment variables table

The Configuration section mentions http://localhost:8081 (or $BIFROST_PORT) but BIFROST_PORT is not listed in the Container Isolation environment variables table or anywhere else as a configurable variable. Either add it to the relevant table or remove the $BIFROST_PORT reference.

Fix this →


5. .entire/ directory not documented in Project Structure

The repo contains a .entire/ directory (with .gitignore and settings.json) which is absent from the Project Structure section. If it is an internal tooling directory that shouldn't be visible to contributors, add it to .gitignore. If it is relevant, document it.


Minor Observations

  • API key example valueANTHROPIC_API_KEY=sk-ant-... in the Configuration section is fine for docs context; no issue here.
  • Duplicate setup stepmake install appears in both Installation and the Development section. The note in Development ("First-time setup — install dependencies…: make install") is redundant if the user followed the Installation section. Consider removing the duplication or making it a cross-reference.
  • README length — At 698 lines the README is thorough but quite long. Consider whether a condensed top-level README pointing to separate docs pages (e.g., a dedicated SECURITY.md or CONTRIBUTING.md) would improve navigability for new contributors.

Overall the README is high quality with excellent coverage of features, architecture, and security model. The issues above are minor and largely cosmetic/accuracy improvements.

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