Skip to content

docs(install): simplify Docker setup wording - #1

Draft
lishuoshuo-amd wants to merge 2 commits into
mainfrom
test/unrestricted-full-review
Draft

lishuoshuo-amd wants to merge 2 commits into
mainfrom
test/unrestricted-full-review

Conversation

@lishuoshuo-amd

@lishuoshuo-amd lishuoshuo-amd commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner
  • Description: simplify one sentence in the Docker setup note from "inside of a Docker container" to "inside a Docker container". This fork-only PR validates that a clean unrestricted review dismisses the bot's previous request-changes review and posts LGTM. Do not merge.
  • Linked issue(s): none
  • Tests: docs-only wording change; exact one-line diff inspected.
  • Size/complexity triggers crossed: none.
  • If this simplifies or refactors: n/a.
  • Observable effect: none; rendered documentation meaning is unchanged.
  • Breaking changes: no
  • PR addresses single concern: yes
  • Root cause is upstream: no

Add a required-text loader and a scoped happy-path test for the fork review
smoke test. The missing-file path deliberately reports success so the review
agent has a concrete blocking condition to reproduce.
@lishuoshuo-amd

Copy link
Copy Markdown
Owner Author

/review

@github-actions

Copy link
Copy Markdown

Automated review started for head b14dde0. Progress: https://github.com/lishuoshuo-amd/Hyperloom/actions/runs/36424542127

@lishuoshuo-amd

Copy link
Copy Markdown
Owner Author

/review

@github-actions

Copy link
Copy Markdown

Automated review started for head b14dde0. Progress: https://github.com/lishuoshuo-amd/Hyperloom/actions/runs/36428046684

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #1: test: exercise unrestricted review verification

What the PR does

Adds a new module review_smoke.py exposing one helper, load_required_text(path), which reads a file and returns {"ok": bool, "content": str}, plus a test file exercising the success path only. Nothing existing is modified. The PR body describes this as a fork-only exercise of the automated review pipeline and states the PR must not be merged.

Blocking issues: 2

  1. [Correctness] load_required_text swallows OSError and reports a missing or unreadable required file as success: the except OSError branch returns ok: True with empty content, ok is never False on any path, and the failure state is indistinguishable from a successful read of an empty file. The OSError branch is also untested — the only test exercises the success path.
  2. [Dead code] load_required_text has no callers anywhere in the repository; the module is landed with zero consumers, so the defective contract would be frozen in ahead of any use.

Head: b14dde0

@lishuoshuo-amd

Copy link
Copy Markdown
Owner Author

/review

@github-actions

Copy link
Copy Markdown

Automated review started for head b14dde0. Progress: https://github.com/lishuoshuo-amd/Hyperloom/actions/runs/36430939904

@github-actions

Copy link
Copy Markdown

Automated review: no verdict applied -- the card has no well-formed PR heading. Comment /review to retry. Run: https://github.com/lishuoshuo-amd/Hyperloom/actions/runs/36430939904

What this PR does

Adds a new module src/hyperloom/common/review_smoke.py exposing one function, load_required_text(path), which reads a required file's text and returns an {ok, content} dict. It also adds a single test, src/hyperloom/common/tests/test_review_smoke.py, covering the successful-read path. Nothing else in the tree calls the new function; this PR is a self-described review smoke exercise, and its observable behavior is limited to the new module and test.

Blocking issues: 2

  1. [S1] load_required_text reports ok: true for a missing required path — the ok flag can never be false [verified]
    Problem: In src/hyperloom/common/review_smoke.py:13-18, the except OSError branch sets content = "" and falls through to the single return {"ok": True, "content": content}. There is no code path anywhere in the function that returns ok: false. Directly executed against a missing file, a chmod 000 file, and a directory, all three return {"ok": True, "content": ""} — indistinguishable from a genuinely empty required file. For a function whose contract is "load a required path", treating absence-of-file as success inverts the failure mode the caller most needs to detect.
    Fix: Return {"ok": False, "content": ""} (or equivalent failure signal) inside the except OSError branch, or let the exception propagate, so a required path that cannot be read is reported as a failure rather than an empty success.
    Files: src/hyperloom/common/review_smoke.py:13

  2. [T4] The only added test covers the success path, so the failure-mode defect above cannot be caught by CI [verified]
    Problem: src/hyperloom/common/tests/test_review_smoke.py:12-17 writes one real file and asserts the round-trip; no test exercises a missing path, an unreadable file, or a directory, which is exactly where the contract is broken (see issue 1). The added suite pins the happy path while leaving the required-path semantics — the whole point of the function — unverified.
    Fix: Add a test that a missing (or unreadable) required path returns ok: false (or raises), pinning the failure contract alongside the success contract.
    Files: src/hyperloom/common/tests/test_review_smoke.py:12

Checked: src/hyperloom/common/review_smoke.py, src/hyperloom/common/tests/test_review_smoke.py, diff.txt, body.txt, title.txt, commits.txt, base.txt; direct execution of load_required_text against a real file, a missing path, a chmod 000 file, and a directory (results in ai_diagnostic.txt); tree-wide grep confirming no caller of load_required_text or review_smoke
Head: b14dde0

@lishuoshuo-amd

Copy link
Copy Markdown
Owner Author

/review

@github-actions

Copy link
Copy Markdown

Automated review started for head b14dde0. Progress: https://github.com/lishuoshuo-amd/Hyperloom/actions/runs/36432111379

@github-actions

Copy link
Copy Markdown

Automated review: no verdict applied -- the review job ended with result 'failure'. Comment /review to retry. Run: https://github.com/lishuoshuo-amd/Hyperloom/actions/runs/36432111379

@lishuoshuo-amd

Copy link
Copy Markdown
Owner Author

/review

@github-actions

Copy link
Copy Markdown

Automated review started for head b14dde0. Progress: https://github.com/lishuoshuo-amd/Hyperloom/actions/runs/36510743028

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #1 -- test: exercise unrestricted review verification

What it does: Adds a new module src/hyperloom/common/review_smoke.py exposing one function, load_required_text(path), which reads a required file and returns {"ok": True, "content": <text or empty string>}, plus a single test covering only the successful-read path. Nothing existing is modified and nothing in the tree calls the new function; the body states the PR is a fork-only exercise of the review pipeline and must not be merged.

Blocking issues: 2

  1. [S1] load_required_text reports a missing or unreadable required file as success -- ok is never false on any path [verified]
    Problem: In src/hyperloom/common/review_smoke.py:10-12, the except OSError branch sets content = "" and falls through to the single return at :12, {"ok": True, "content": content}. There is no code path anywhere in the function that reports failure. Directly executed at head: a missing path, a chmod 000 file and a directory all return {"ok": True, "content": ""} -- byte-identical to a successful read of a genuinely empty required file.
    Impact: Any caller relying on ok to tell "required file read" from "required file missing or unreadable" gets the wrong answer for every input in the OSError family, with no status, log or exit code carrying the failure; the flag ok carries no information at all.
    Action: Make the except OSError branch report the failure ({"ok": False, "content": ""}, or an explicit error field), or let the exception propagate to the caller, so a required path that cannot be read is distinguishable from an empty one; add a test for the missing/unreadable path, which the current suite never exercises.

  2. [C3] load_required_text has no consumer anywhere in the tree -- a writer with no reader [verified]
    Problem: src/hyperloom/common/review_smoke.py:7 defines the module's only export; a head-tree grep matches load_required_text and review_smoke only at the definition and in the function's own test (src/hyperloom/common/tests/test_review_smoke.py:6,9,13). No production call site, no dispatch-table entry, no prompt, no doc names it.
    Impact: The defective contract from issue 1 is frozen in ahead of any use, and the module, its always-true ok flag and its happy-path test land as permanent surface area of hyperloom.common that no code path can reach.
    Action: Delete the module and its test, or land the consumer that needs it in the same PR with the failure contract from issue 1 corrected; the filesystem-contract helpers of hyperloom.common already live in src/hyperloom/common/io.py if such a helper is genuinely needed.

Checked: src/hyperloom/common/review_smoke.py, src/hyperloom/common/tests/test_review_smoke.py, src/hyperloom/common/io.py, src/hyperloom/common/init.py, head-tree grep for load_required_text/review_smoke, ci.txt project-automation failure log, comments.txt | Ran: PYTHONPATH=src python3 calling load_required_text on an existing file, a missing path, a chmod 000 file and a directory; PYTHONPATH=src python3 -m pytest -q src/hyperloom/common/tests/test_review_smoke.py (1 passed) | Base: 38d2ffa | Head: b14dde0

Remove the deliberately defective unused loader and its happy-path-only test.
Keep the fork PR non-empty with a documentation grammar cleanup so the clean
review path can dismiss the bot's earlier request-changes review.
@lishuoshuo-amd lishuoshuo-amd changed the title test: exercise unrestricted review verification docs(install): simplify Docker setup wording Sep 29, 2026
@lishuoshuo-amd

Copy link
Copy Markdown
Owner Author

/review

@github-actions

Copy link
Copy Markdown

Automated review started for head b317547. Progress: https://github.com/lishuoshuo-amd/Hyperloom/actions/runs/36512266578

@github-actions
github-actions Bot dismissed stale reviews from themself September 29, 2026 02:30

Superseded by a clean automated review at b317547.

@github-actions

Copy link
Copy Markdown

PR #1 -- docs(install): simplify Docker setup wording

What it does: Replaces the previous head's deliberately defective unused loader (review_smoke.load_required_text, which reported a missing required file as success) with a one-line grammar cleanup in the Docker setup note in docs/install/install.md ("inside of a Docker container" becomes "inside a Docker container"), so the fork PR stays non-empty and the clean review path can dismiss the bot's earlier request-changes review. The rendered meaning of the documentation is unchanged and no code, config or interface is touched.

Blocking issues: none

Checked: diff.txt (single hunk, docs/install/install.md:90), base-tree/docs/install/install.md:87-93 vs pr-head/docs/install/install.md:87-93, head-tree grep for review_smoke|load_required_text (zero hits at head), head-tree grep for inside of a Docker (one hit: examples/README.md:89, adjudicated under X3 and dropped as taste -- both phrasings state the same true instruction), base-tree grep confirming the sibling copy predates the branch, body.txt claims against numstat/files/testfiles (1/1, one docs path, no tests), ci.txt (sphinx html docs build success at head b317547), meta.txt mergeable MERGEABLE, comments.txt (prior cards' findings verified fixed at this head: the loader and its happy-path test are gone) | Ran: grep -rn 'review_smoke\|load_required_text' --include='*.py' --include='*.md' pr-head (no matches); git show --stat HEAD and git log in pr-head to confirm the head commit's scope; direct read of the note block in both trees; no tests run (docs-only diff, testfiles.txt empty) | Base: 38d2ffa | Head: b317547

LGTM

@lishuoshuo-amd

Copy link
Copy Markdown
Owner Author

/review

@github-actions

Copy link
Copy Markdown

Automated review started for head b317547. Progress: https://github.com/lishuoshuo-amd/Hyperloom/actions/runs/36536933243

@github-actions

Copy link
Copy Markdown

PR #1 -- docs(install): simplify Docker setup wording

What it does: Replaces the earlier head's deliberately defective unused loader (which reported a missing required file as success) with a one-word grammar cleanup in the Docker setup note in docs/install/install.md ("inside of a Docker container" becomes "inside a Docker container"), keeping the fork-only PR non-empty so the clean-review path can be exercised. The rendered meaning of the documentation is unchanged; no code, prompt, schema or configuration is touched.

Blocking issues: none

Checked: docs/install/install.md:90 in pr-head vs base-tree, tree-level diff (diff -rq base-tree pr-head) confirming the only non-.git difference is docs/install/install.md, head-tree grep for review_smoke|load_required_text (zero hits), head-tree grep for inside a Docker / inside of a Docker (4 hits: install.md:4, install.md:9, install.md:90, examples/README.md:89), body.txt claims against files.txt/numstat.txt/testfiles.txt, ci.txt at head b317547, meta.txt (mergeable: MERGEABLE), comments.txt (prior cards' findings verified fixed at this head) | Ran: diff -rq base-tree pr-head, grep -rn 'review_smoke\|load_required_text' pr-head (no matches), grep -rn 'inside a Docker\|inside of a Docker' pr-head, diff base-tree/docs/install/install.md pr-head/docs/install/install.md; no tests run (docs-only diff, testfiles.txt empty) | Base: 38d2ffa | Head: b317547

LGTM

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