Skip to content

chore/docs: cleanup committed root artifacts + minor docs/diagnostics fixes - #91712

Closed
EAbaracus wants to merge 3 commits into
NousResearch:mainfrom
EAbaracus:audit-cleanup-good-first
Closed

EAbaracus wants to merge 3 commits into
NousResearch:mainfrom
EAbaracus:audit-cleanup-good-first

Conversation

@EAbaracus

Copy link
Copy Markdown
Contributor

Summary

Small, low-risk hygiene PR (from a static read-only audit of the repo) covering 5 "good first" items:

  • Remove committed root artifacts (HA-D11-001 / HA-D3-001): log.txt (empty 0-byte), sqlite_leak_fix.png (832 KB, unreferenced), default.tar.gz (1.96 MB — only ever written by tests/hermes_cli/test_profiles.py into a temp dir, never read from repo root). Added ignore rules so they cannot be re-committed.
  • Diagnostics (HA-D6-001): acp_adapter/server.py fell back to HERMES_VERSION = "0.0.0" silently on import failure; now logs a warning via the existing module logger.
  • Docs (HA-D12-001 / HA-D9-001 / HA-D2-001):
    • AGENTS.md: convert the ui-tui dev-command inline bash snippet to a numbered, copy-paste-friendly list.
    • README.md: add an AGENTS.md cross-link on the Windows install note so contributor/dev setup has a single source of truth.
    • agent/secret_sources/base.py: document the deliberate shell=True exception in command_token_source (operator's own key_cmd), resolving the apparent contradiction with the no-shell rule.

Test plan

  • git rebase upstream/main — clean, no conflicts
  • git diff --stat upstream/main.. shows only the intended files
  • Confirmed the 3 root artifacts still exist on upstream/main (deletion is legit cleanup, not accidental)
  • CI (tests.yml / lint.yml) should pass — changes are docs + 1 log line + file deletion

Notes

Not security-critical; no behavioral change except the new warning log. Branch is on the EAbaracus fork; base is NousResearch/hermes-agent@main.

… default.tar.gz)

These were committed to the repo root but are build/debug byproducts:
- log.txt: empty 0-byte file
- sqlite_leak_fix.png: unreferenced 832KB image
- default.tar.gz: 1.96MB, only used as a test fixture OUTPUT (tests write it
  to a temp dir, never read from repo root)

Add ignore rules so they cannot be re-committed. Part of audit cleanup
(HA-D11-001 / HA-D3-001).
The module-level try/except fell back to HERMES_VERSION='0.0.0' silently.
Surface the failure via the existing module logger so ACP version
negotiation issues are diagnosable. Part of audit cleanup (HA-D6-001).
- AGENTS.md: convert the ui-tui dev-command block from an inline bash
  snippet to a numbered list (copy-paste friendly).
- README.md: add AGENTS.md cross-link to the Windows install note so
  contributor/dev setup has a single source of truth.
- secret_sources/base.py: document the deliberate shell=True exception in
  command_token_source (operator's own key_cmd), resolving the apparent
  contradiction with the no-shell rule. Part of audit cleanup
  (HA-D12-001 / HA-D9-001 / HA-D2-001).
@alt-glitch alt-glitch added type/docs Documentation improvements comp/acp Agent Communication Protocol adapter P3 Low — cosmetic, nice to have labels Aug 21, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

  1. .gitignore:100–105 — the new patterns are unanchored (log.txt, sqlite_leak_fix.png, default.tar.gz), so they ignore files with those names at every depth, not just repo root — why it matters: log.txt in particular is a plausible name for a test fixture or plugin sample data; when someone adds one later, git will silently refuse to track it and the failure mode ("why won't my file commit?") wastes an hour — suggestion: anchor them (/log.txt, /sqlite_leak_fix.png, /default.tar.gz) since the stated intent is "repo-root build/debug artifacts".

  2. AGENTS.md:505–516 — converting the Dev Commands fenced bash block into a numbered plain-text list loses syntax highlighting and one-shot copy/paste of the whole sequence, with no apparent gain over the original — why it matters: AGENTS.md is contributor-facing; the fenced block was the more useful rendering — suggestion: keep the fence and just prepend the "run from ui-tui/" note as a comment or lead-in line.

  3. acp_adapter/server.py:~227logger.warning added to the HERMES_VERSION import fallback runs at module import time, before logging configuration exists in most entry paths, so it emits via the stdlib last-resort handler straight to stderr — why it matters: in packaging modes where hermes_cli is legitimately absent from the ACP sidecar this becomes recurring stderr noise rather than a diagnosable signal — suggestion: keep the message but route it through the same once-per-process/debug-level pattern used by other import fallbacks in this file, or attach the reason (exc_info=True).

  4. Nit (.gitignore:103): *.png.bak is likewise unanchored; if it's meant for editor swap files next to the deleted screenshot, scope it (/*.png.bak) for symmetry with item 1.

Overall: worthwhile housekeeping — dropping committed binaries plus documenting the deliberate shell=True exception in secret_sources/base.py are genuine improvements; no blocking issues found.

— reviewer-a · automated agent review (Hermes week-review)

teknium1 added a commit that referenced this pull request Aug 22, 2026
…ker image layers

Follow-up to the cherry-picked cleanup: the default.tar.gz profile export
was also carried into published container images by the Dockerfile's
'COPY . .' layer because .dockerignore had no matching pattern. Anchor
the .gitignore rules to repo root (per review feedback on #91712) and
add the same set + /*.tar.gz to .dockerignore so root archives can never
reach an image layer again.
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks @EAbaracus — the artifact cleanup half of this PR was salvaged into #92394 (your commit cherry-picked with authorship preserved) and is now merged as 0cde4dd, together with .gitignore root-anchoring per the review feedback and matching .dockerignore rules so root archives can't reach Docker image layers via COPY . . again.

One correction to the assessment here: default.tar.gz was not just an unreferenced test byproduct — it was a full profile export whose config.yaml contained a populated webhook secret, so the removal was security-relevant and we fast-tracked it. The credential has been handled separately.

The other two commits (ACP version-import warning, AGENTS.md doc tweaks) had open review concerns (import-time logging before logging config exists; fenced-block regression), so we're not taking them as-is — feel free to reopen those as separate focused PRs if you'd like to iterate. Closing this one since the substantive fix has landed. Appreciate the audit work!

@teknium1 teknium1 closed this Aug 22, 2026
melon-xf added a commit to melon-xf/hermes-agent that referenced this pull request Sep 3, 2026
…ker image layers

Follow-up to the cherry-picked cleanup: the default.tar.gz profile export
was also carried into published container images by the Dockerfile's
'COPY . .' layer because .dockerignore had no matching pattern. Anchor
the .gitignore rules to repo root (per review feedback on NousResearch#91712) and
add the same set + /*.tar.gz to .dockerignore so root archives can never
reach an image layer again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter P3 Low — cosmetic, nice to have type/docs Documentation improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants