Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds GitHub Agentic Workflows support, new Makefile targets and ADR tooling, expanded stress/security tests and test helpers, CI/workflow and SBOM enhancements, shell script messaging refinements, several Rhiza build/config updates, documentation expansions, and removal of Renovate config. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add security note to conftest.py docstring to satisfy the test_test_security_exceptions_documented security policy check. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.rhiza/.cfg.toml (1)
19-29:⚠️ Potential issue | 🟠 Major
"a"and"b"as sequential release tiers will produce unexpected bumps.In bump-my-version, the
valueslist is an ordered enum — bumping thereleasepart advances to the next entry. The current sequence:dev → alpha → a → beta → b → rc → prodmeans bumping from
alphalands onarather thanbeta. Sinceais the PEP 440 alias ofalpha(not a successor stage), this produces semantically incorrect version transitions. Similarly forb/beta.If the intent is to allow parsing both forms without changing bump semantics, the right approach is to handle the aliasing only in the
parseregex (already done) and leave thevalueslist with one canonical form per stage:♻️ Proposed fix
values = [ "dev", "alpha", - "a", # PEP 440 short form for alpha "beta", - "b", # PEP 440 short form for beta "rc", "prod" ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/.cfg.toml around lines 19 - 29, The release "values" list contains PEP 440 aliases "a" and "b" which break the ordered bump semantics; remove the duplicate aliases so the enum contains only canonical stages (e.g., "dev", "alpha", "beta", "rc", "prod") and rely on the existing parse regex to accept the short forms, ensuring bumping advances through alpha → beta (not alpha → a → beta).
🧹 Nitpick comments (13)
.rhiza/tests/sync/test_rhiza_version.py (1)
163-167: Redundant explicitdry_run=True.
run_makealready defaults todry_run=True, so passing it explicitly on lines 165 and 178 is noise. Removing these kwargs makes the calls consistent with the rest of the suite.♻️ Proposed cleanup
def test_sync_experimental_skips_in_rhiza_repo(self, logger): - proc = run_make(logger, ["sync-experimental"], dry_run=True) + proc = run_make(logger, ["sync-experimental"]) def test_sync_experimental_shows_beta_warning(self, logger): - proc = run_make(logger, ["sync-experimental"], dry_run=True) + proc = run_make(logger, ["sync-experimental"])Also applies to: 176-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/tests/sync/test_rhiza_version.py around lines 163 - 167, In the test function test_sync_experimental_skips_in_rhiza_repo remove the redundant explicit dry_run=True argument from the call(s) to run_make (and the other run_make call in the same test block mentioned by the reviewer) since run_make defaults to dry_run=True; simply call run_make(logger, ["sync-experimental"]) to make the test consistent with the rest of the suite and eliminate the unnecessary kwarg..rhiza/tests/integration/test_sbom.py (1)
66-110: Consider adding metadata assertions to the XML test for NTIA consistency.
test_sbom_generation_jsonnow validatesmetadata.component(name and version) for NTIA compliance, buttest_sbom_generation_xml— despite receiving the same--pyprojectflag — has no equivalent assertion on the XML metadata block.♻️ Suggested addition for the XML test
# Check for components element components = root.find(".//{*}components") assert components is not None, "SBOM XML missing components element" + + # Verify primary component (metadata/component) for NTIA compliance + ns = {"cdx": root.tag.split("}")[0].lstrip("{")} if "}" in root.tag else {} + metadata = root.find(".//{*}metadata") + assert metadata is not None, "SBOM XML missing metadata element" + component = metadata.find("{*}component") if not ns else metadata.find(".//{*}component") + assert component is not None, "SBOM XML missing primary component (metadata/component)" + name = component.findtext(".//{*}name") + version = component.findtext(".//{*}version") + assert name, "Primary XML component missing name" + assert version, "Primary XML component missing version"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/tests/integration/test_sbom.py around lines 66 - 110, test_sbom_generation_xml lacks assertions verifying the SBOM metadata component (name/version) for NTIA consistency; update the test to parse the XML tree (using defusedxml.ElementTree.parse on sbom_file), locate the metadata/component element (e.g., find ".//{*}metadata" then ".//{*}component" or use XPath), extract the component name and version text and assert they match the expected values from the pyproject fixture (the same values validated in test_sbom_generation_json); keep existing checks (components element) and add these metadata assertions to test_sbom_generation_xml to mirror NTIA checks..rhiza/tests/stress/test_git_stress.py (2)
28-36: Same missingtimeoutconcern astest_makefile_stress.py— addtimeouttosubprocess.runcalls.None of the
subprocess.runcalls in this file specify a timeout. A hanging git process (e.g., due to a lock file) would block the stress test indefinitely. This is especially relevant for concurrent tests where lock contention could stall git.♻️ Example fix (apply to all subprocess.run calls in this file)
result = subprocess.run( # nosec [GIT, "--no-pager", "status", "--short"], cwd=root, capture_output=True, text=True, + timeout=30, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/tests/stress/test_git_stress.py around lines 28 - 36, The subprocess.run calls (e.g., in run_git_status) lack a timeout and can hang; update every subprocess.run invocation in this file to include a timeout argument (e.g., timeout=some_seconds) so git commands cannot block indefinitely—add the timeout parameter to the subprocess.run calls that use GIT and any other subprocess.run usages, handle the possible TimeoutExpired where appropriate (catch subprocess.TimeoutExpired where applicable) and choose a sensible timeout constant at the top of the file for reuse.
141-141: SHA length check assumes SHA-1 (40 chars) — will break if the repo enables SHA-256 object format (64 chars).This is unlikely today but worth a brief note. If future-proofing matters, consider
len(...) in (40, 64).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/tests/stress/test_git_stress.py at line 141, The SHA length check in the test appends a boolean using results.append(result.returncode == 0 and len(result.stdout.strip()) == 40) which will fail for repos using SHA-256 (64 chars); update the condition to accept both lengths (e.g., check len(result.stdout.strip()) in (40, 64)) so results.append(result.returncode == 0 and len(result.stdout.strip()) in (40, 64)) and keep referencing the same result.stdout and results list..rhiza/tests/shell/test_scripts.sh (2)
106-116: Repeated inline pass/fail logic duplicates the assertion helpers.Tests 2, 5, 7, 8, 9, and 10 all manually increment counters and print pass/fail messages instead of reusing
assert_contains. For example, thegrep -q "set -euo pipefail"checks could be replaced with:content=$(cat "$REPO_ROOT/.github/hooks/session-start.sh") assert_contains "$content" "set -euo pipefail" "session-start.sh uses strict error handling"This would eliminate ~60 lines of boilerplate and keep test reporting consistent.
Also applies to: 134-144, 156-166, 169-179, 182-192, 195-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/tests/shell/test_scripts.sh around lines 106 - 116, Replace the repeated manual pass/fail blocks that directly manipulate TESTS_RUN/TESTS_PASSED/TESTS_FAILED and echo messages with the existing assertion helper by reading the file into a variable and calling assert_contains; e.g., for the session-start.sh check, set content=$(cat "$REPO_ROOT/.github/hooks/session-start.sh") and call assert_contains "$content" "set -euo pipefail" "session-start.sh uses strict error handling" (similarly convert the other duplicated blocks at the other ranges); keep VERBOSE logic inside assert_contains if needed and remove the direct increments/echoes so all checks use assert_contains consistently.
48-66:grep -qtreats$needleas a regex pattern — considergrep -qFfor literal matching.If
needlecontains regex metacharacters (e.g.,.,*,[),grep -qcould produce false positives. Usinggrep -qFfor fixed-string matching would be more robust for an assertion helper.♻️ Proposed fix
- if grep -q "$needle" <<< "$haystack"; then + if grep -qF "$needle" <<< "$haystack"; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/tests/shell/test_scripts.sh around lines 48 - 66, The assertion helper assert_contains currently uses grep -q which treats the second argument as a regex; update the check in assert_contains to use fixed-string matching by switching to grep -qF (and keep the existing here-string <<< "$haystack" and quoted "$needle") so that literal needles with regex metacharacters are matched literally and avoid false positives; adjust only the grep invocation inside assert_contains..rhiza/tests/security/test_security_patterns.py (1)
60-67: String check'"S"' in contentmay produce false positives.The substring check
'"S"' in contentwould match any occurrence of"S"in the file, including comments, unrelated strings, or rule codes like"S101". This works today because ruff.toml files tend to be small and focused, but a more precise check (e.g., regex for a standalone"S"in aselectarray) would be more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/tests/security/test_security_patterns.py around lines 60 - 67, The current substring check on content (variable content loaded from ruff_config in the test) can yield false positives; replace it with a regex-based assertion that searches for a standalone "S" entry inside a select or extend-select array (e.g., use re.search on content for patterns like select = [ ... "S" ... ] or extend-select = [ ... 'S' ... ], with DOTALL to allow multiline arrays) so the test asserts that the ruff.toml's select/extend-select explicitly contains the "S" rule rather than matching any incidental `"S"` substring..rhiza/make.d/test.mk (1)
129-137:coverage-badgedepends ontest— this will re-run the entire test suite even if coverage data already exists.If the user just wants to regenerate the badge (e.g., after tweaking styling), they'll have to wait for the full test suite. Consider making the dependency optional or adding a note that users can run
make coverage-badgeknowing it will re-run tests.This is fine as-is for correctness (ensures fresh data), just noting the UX trade-off.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/make.d/test.mk around lines 129 - 137, The coverage-badge Make target currently depends on test causing the whole suite to re-run; update the Makefile so the coverage-badge target (symbol: coverage-badge) does not implicitly run test unless requested — either remove the dependency on test, add a separate quick target (e.g., coverage-badge-fast) that reads _tests/coverage.json without running tests, or make the dependency conditional (only run test if _tests/coverage.json is missing) and document the behavior in the target's comment; change references in the same rule that call ${UVX_BIN} genbadge to preserve existing output path assets/coverage-badge.svg..rhiza/tests/stress/test_makefile_stress.py (1)
28-36: Missingtimeoutonsubprocess.run— tests can hang indefinitely ifmakeblocks.Line 142 in
test_concurrent_variable_printingusesf.result(timeout=30), but none of thesubprocess.runcalls in this file specify a timeout. Ifmakehangs (e.g., a recipe waiting on stdin), the stress test will block the entire CI pipeline.Consider adding a
timeoutparameter to allsubprocess.runcalls:♻️ Example fix (apply to all subprocess.run calls in this file)
result = subprocess.run( # nosec [MAKE, "help"], cwd=root, capture_output=True, text=True, + timeout=30, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/tests/stress/test_makefile_stress.py around lines 28 - 36, The subprocess.run calls (e.g., in run_help()) lack a timeout and can hang the tests; update every subprocess.run invocation in this file to include a reasonable timeout (e.g., timeout=30) and wrap the call in a try/except subprocess.TimeoutExpired block so you can treat timeouts as failures (return False or raise/assert accordingly). Locate functions like run_help() and any other spots using subprocess.run and add timeout=<seconds> to the call signature and handle subprocess.TimeoutExpired to avoid indefinite test hangs..rhiza/tests/api/test_makefile_targets.py (1)
161-172: Misleading setup in dry-run test — file creation has no effect
make -nprints all recipe lines without executing them, so shell guards likeif [ -f "_tests/coverage.json" ]are printed, not evaluated. Thetests_dir / "coverage.json"setup doesn't change what appears in the dry-run output; the assertions will pass (or fail) purely based on whether the Makefile recipe contains those strings.The comment
# Create a mock coverage JSON file so the target proceeds past the guardis inaccurate for a dry-run test, and the three setup lines can be removed.♻️ Proposed simplification
def test_coverage_badge_target_dry_run(self, logger, tmp_path): """Coverage-badge target should invoke genbadge via uvx in dry-run output.""" - # Create a mock coverage JSON file so the target proceeds past the guard - tests_dir = tmp_path / "_tests" - tests_dir.mkdir(exist_ok=True) - (tests_dir / "coverage.json").write_text("{}") - proc = run_make(logger, ["coverage-badge"]) out = proc.stdout assert "genbadge coverage" in out assert "_tests/coverage.json" in out assert "assets/coverage-badge.svg" in out🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/tests/api/test_makefile_targets.py around lines 161 - 172, The test test_coverage_badge_target_dry_run contains a misleading setup that creates _tests/coverage.json which has no effect for a dry-run; remove the three setup lines that create tests_dir and write coverage.json and update the inline comment (and any mention of proceeding past the guard) to reflect that this is a dry-run asserting printed Makefile recipe lines only; look for references to tests_dir, (tests_dir / "coverage.json").write_text and the comment and delete or replace them so the test solely calls run_make and asserts on proc.stdout..rhiza/make.d/gh-aw.mk (1)
8-8:GH_AW_BINis computed at parse time but never used
$(shell ...)is eagerly evaluated every time the Makefile is parsed, even for unrelated targets. Since every recipe usesgh awdirectly rather than$(GH_AW_BIN), this variable is dead code that adds a subprocess execution on everymakeinvocation.Either remove it, or actually use it in the recipes so the extension check is centralised:
♻️ Proposed fix
-# Detect if gh-aw extension is installed -GH_AW_BIN ?= $(shell gh extension list 2>/dev/null | grep -q gh-aw && echo "gh aw" || echo "")And then use
$(GH_AW_BIN)instead ofgh awin each recipe (replacing@gh aw compilewith@$(GH_AW_BIN) compile, etc.), so the variable actually drives behaviour.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/make.d/gh-aw.mk at line 8, The GH_AW_BIN variable is computed at Makefile parse time but never referenced; either remove the GH_AW_BIN assignment entirely to avoid the unnecessary shell invocation, or centralize the check by keeping GH_AW_BIN and replacing every direct use of the literal command (instances of "gh aw" in recipes such as the "@gh aw compile" calls) with "$(GH_AW_BIN)" so the extension check drives behavior; ensure the variable is defined exactly once (the existing GH_AW_BIN assignment) and all recipe lines use "$(GH_AW_BIN)" instead of the literal "gh aw"..rhiza/make.d/book.mk (1)
89-104: Outboundcurlcall in the book-build target adds latency and a network dependencyThe CodeFactor HTTP check uses
--max-time 5and falls back on failure, so it fails safely. However, everymake bookinvocation incurs up to a 5-second wait whenever codefactor.io is slow or unreachable (air-gapped CI, rate-limited, etc.). Consider moving this link check to a dedicated, optional target (e.g.make book-links) or making it conditional on a flag:- if [ -n "$$CF_REPO" ]; then \ - CF_URL="..."; \ - HTTP_CODE=$$(curl -s ...); \ + if [ -n "$$CF_REPO" ] && [ -n "$(CHECK_CODEFACTOR)" ]; then \ + CF_URL="..."; \ + HTTP_CODE=$$(curl -s ...); \This keeps the default build hermetic and fast while still allowing CI to opt in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/make.d/book.mk around lines 89 - 104, The book build currently performs an outbound curl check (CF_REPO/CF_URL and HTTP_CODE logic) which adds latency; remove that curl/HTTP probing block from the default book target and either (A) create a new optional Make target (e.g., book-links) that contains the CF_REPO detection, CF_URL construction and curl HTTP_CODE check and appends to _book/links.json when invoked, or (B) wrap the existing curl/HTTP_CODE logic in a conditional guard controlled by an environment variable/flag (e.g., ENABLE_LINK_CHECKS or SKIP_LINK_CHECKS) so the default `book` run is hermetic; keep the same CF_REPO, CF_URL and output behavior but relocate it to the new target or gated block so CI can opt-in without adding the 5s network wait to normal builds..rhiza/tests/api/test_gh_aw_targets.py (1)
35-87: Consider adding a negative test forgh-aw-runwith missingWORKFLOWThe error path in
gh-aw-run(exit 1whenWORKFLOWis empty) has no test coverage. A quick dry-run can't easily exercise it (dry-run skips execution), but running without-nand withoutWORKFLOWset should verify the error message and non-zero exit.def test_gh_aw_run_missing_workflow_errors(logger): """gh-aw-run should exit non-zero and print an error when WORKFLOW is unset.""" result = run_make(logger, ["gh-aw-run"], check=False, dry_run=False) assert result.returncode != 0 assert "WORKFLOW" in result.stdout or "WORKFLOW" in result.stderr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rhiza/tests/api/test_gh_aw_targets.py around lines 35 - 87, Add a negative test that runs the Make target "gh-aw-run" without setting WORKFLOW and without dry-run to exercise the error path: create a test function named test_gh_aw_run_missing_workflow_errors that calls run_make(logger, ["gh-aw-run"], check=False, dry_run=False), asserts result.returncode != 0, and asserts that the string "WORKFLOW" appears in result.stdout or result.stderr so the expected error message is validated; reference run_make and the "gh-aw-run" target when locating where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/secret_scanning.yml:
- Around line 14-20: The current secret_scanning rule uses blanket ignores
"docs/**/*.md" and "book/**" which can hide real secrets; instead remove those
broad patterns and narrowly ignore only known example or fixture paths (e.g.,
change "docs/**/*.md" and "book/**" to more specific patterns like
"docs/**/*example*/**", "docs/**/fixtures/**", or ".rhiza/tests/**" style
paths); update the paths-ignore block to delete "docs/**/*.md" and "book/**" and
add targeted ignores for specific example/fixture directories so documentation
remains scanned for accidental real secrets while still excluding harmless
examples.
In @.github/workflows/renovate_rhiza_sync.yml:
- Around line 41-45: The fallback version used in the "Get Rhiza version" step
(id: rhiza-version) is hardcoded as "0.9.0" via VERSION=$(cat
.rhiza/.rhiza-version 2>/dev/null || echo "0.9.0"); update that fallback to the
current baseline "0.11.2" so fresh clones without .rhiza/.rhiza-version will
install the expected rhiza version; change the echoed fallback string to
"0.11.2" while keeping the same variable NAME (VERSION) and output logic.
- Around line 66-82: The current git config command uses --global and rewrites
all github.com URLs; change it to configure only this repository and scope the
substitution to the specific repo remote: replace git config --global
url."https://x-access-token:${{ secrets.PAT_TOKEN || github.token
}}@github.com/".insteadOf "https://github.com/" with a local config that targets
only the current repository (omit --global) and match the repository path (e.g.
insteadOf "https://github.com/${{ github.repository }}" and url
"https://x-access-token:${{ secrets.PAT_TOKEN || github.token }}@github.com/${{
github.repository }}") so only pushes to this repo use the PAT.
In @.github/workflows/rhiza_release.yml:
- Around line 139-146: Wrap the normalization step that calls
packaging.version.Version('$TAG_VERSION') in a try/except that catches
packaging.version.InvalidVersion and prints a clear GitHub Actions error message
before exiting; specifically, update the shell invocation that sets
NORMALIZED_TAG (the command using uv run --with packaging --no-project python3
-c "from packaging.version import Version; print(Version('$TAG_VERSION'))") to
handle InvalidVersion and emit something like "Invalid tag format:
'$TAG_VERSION' cannot be normalized to PEP 440; ensure tag is PEP 440 or adjust
release workflow" (and exit nonzero) so the subsequent comparison between
PROJECT_VERSION and NORMALIZED_TAG behaves predictably.
In @.gitignore:
- Around line 84-86: Remove the duplicate .bandit-baseline.json entry in
.gitignore: keep the new annotated block containing ".bandit-baseline.json" (the
security scanning baselines block) and delete the older bare
".bandit-baseline.json" entry elsewhere in the file so the file contains only
the single annotated baseline entry.
In @.rhiza/make.d/quality.mk:
- Around line 26-39: The todos make rule currently prints the "Found the
following items" header unconditionally and never reaches the success message
when there are no matches; change the pipeline that runs find | xargs | grep
-nHE "(TODO|FIXME|HACK):" into a two-step check: run the search command and
capture its output (e.g., into a temp file or shell variable) and then branch —
if the captured output is non-empty, print the header and emit the captured
results (preserving the awk formatting used now), otherwise print the
"${GREEN}[SUCCESS] No TODO/FIXME/HACK comments found!${RESET}" message; update
references to the existing pipeline (the sequence starting with find . ... |
xargs -0 grep -nHE "(TODO|FIXME|HACK):" and the following grep -v/awk) so you
reuse the same grep/awk formatting when printing results.
In @.rhiza/tests/README.md:
- Around line 99-109: The Markdown code fence in the "Run stress tests with
custom parameters" section currently uses triple backticks; change that fence to
use triple tildes (~~~bash ... ~~~) to satisfy MD048 and match the repository's
configured markdownlint rule—update the opening ```bash and closing ``` around
the pytest examples in the README to ~~~bash and ~~~ respectively.
In @.rhiza/tests/security/test_security_patterns.py:
- Around line 33-35: The comment and regex disagree: chmod_pattern currently
matches any 0o77x but the comment says "world-writable"; change the test to only
detect true world-writable modes by narrowing the regex used in chmod_pattern
(the compiled pattern that looks for ".chmod(0o77...") so it only matches last
octal digits that set the "other" write bit (digits 2,3,6,7), and update the
comment to say "world-writable (e.g., 0o772, 0o773, 0o776, 0o777)" so the intent
and pattern align.
- Around line 135-158: The test_test_security_exceptions_documented currently
asserts every conftest.py mentions security markers; update it so it only checks
conftest files that actually suppress security rules by first reading each
conftest (conftest in conftest_files), skipping files that don't contain
suppression tokens (e.g., "noqa: S", "noqa:S", "# nosec", "nosec", or "noqa")
and only then asserting that those containing suppressions include security docs
(has_security_docs). Keep references to conftest_files, conftest, and
has_security_docs when implementing the filtering.
In @.rhiza/tests/sync/test_rhiza_version.py:
- Around line 157-174: The tests test_sync_experimental_uses_beta_version and
test_sync_experimental_command_format hardcode "rhiza>=0.11.1b1" which makes
them brittle; update them to derive the expected version instead of hardcoding:
either read the pinned version from the Makefile (or a shared constant) at test
runtime (use run_make or open the Makefile to extract the rhiza pin) or change
the assertions to use a regex that matches the version pattern (e.g.,
rhiza>=<semver>b<prerelease>) so the tests assert the command format without a
fixed numeric literal; locate these tests by name in test_rhiza_version.py and
adjust their assertions accordingly.
In `@CONTRIBUTING.md`:
- Around line 65-67: markdownlint rule MD040 flags the three fenced code blocks
in CONTRIBUTING.md for missing language identifiers; update each of the three
code fences (the commit message template "<type>(<scope>): <short summary>", the
commit examples block, and the breaking-change example block) to use a language
tag by changing the opening fences from ``` to ```text so each fenced block
begins with ```text to satisfy MD040 and markdownlint.
In `@Makefile`:
- Around line 22-29: The Makefile's ADR interactive recipe uses the bash-only
`read -p` (seen in the adr target), which will fail or hang under POSIX sh
(e.g., dash) and in non-interactive CI; fix by making the recipe portable or
forcing bash: either set `SHELL := /bin/bash` at the top of the Makefile to
guarantee bash features for the adr target, or replace `read -p ...` with a
POSIX-safe sequence (use `printf` to prompt then plain `read` to capture into
`title`/`context`, and guard against non-interactive shells by checking `-t 0`
before prompting and erroring with a clear message if not interactive); also
update the target documentation to state it requires an interactive terminal.
- Around line 31-33: The Makefile is invoking a non-existent workflow file
"adr-create.md"; either create the corresponding workflow or fix the invocation:
if this is meant to be a GitHub Actions workflow, add
.github/workflows/adr-create.yml (or .yaml) and update the Makefile invocations
that call "gh workflow run adr-create.md" to "gh workflow run adr-create.yml"
(preserve the -f title/ -f context flags), otherwise if it's an agentic workflow
use the agentic CLI and change the Makefile to call "gh aw run adr-create" (no
extension) and add the agentic spec file (e.g., .rhiza/…/adr-create)
accordingly; update the lines invoking gh workflow run adr-create.md to the
chosen command and ensure the new workflow/spec file exists.
In `@SECURITY.md`:
- Around line 28-30: Update the "Email" section (the heading "2. **Email**" and
its bullet text) to include a concrete contact: either a dedicated security@...
address or a link to the maintainer's GitHub profile or security contact page;
replace or augment "Send details to the repository maintainers" with the actual
email or URL and include guidance on PGP/encryption if available.
In `@tests/benchmarks/conftest.py`:
- Around line 6-9: Remove the misleading "Security Notes" S101 mention from the
module docstring in this conftest (the top-level module docstring in
tests/benchmarks/conftest.py); edit the module docstring used by the conftest
module to either delete the entire Security Notes block or replace it with an
accurate note that does not reference S101/assert usage since there are no
asserts in this file.
- Around line 12-14: Add pytest-html to the project's canonical test
dependencies under pyproject.toml's [project.dependency-groups] (or equivalent
test/dev group) so the pytest_html_report_title hook in
tests/benchmarks/conftest.py works for users installing via pyproject, and
remove the misleading S101/assert usage note from the module docstring in that
file (it’s irrelevant here); reference the pytest_html_report_title hook and the
pytest-html package when making these changes.
---
Outside diff comments:
In @.rhiza/.cfg.toml:
- Around line 19-29: The release "values" list contains PEP 440 aliases "a" and
"b" which break the ordered bump semantics; remove the duplicate aliases so the
enum contains only canonical stages (e.g., "dev", "alpha", "beta", "rc", "prod")
and rely on the existing parse regex to accept the short forms, ensuring bumping
advances through alpha → beta (not alpha → a → beta).
---
Nitpick comments:
In @.rhiza/make.d/book.mk:
- Around line 89-104: The book build currently performs an outbound curl check
(CF_REPO/CF_URL and HTTP_CODE logic) which adds latency; remove that curl/HTTP
probing block from the default book target and either (A) create a new optional
Make target (e.g., book-links) that contains the CF_REPO detection, CF_URL
construction and curl HTTP_CODE check and appends to _book/links.json when
invoked, or (B) wrap the existing curl/HTTP_CODE logic in a conditional guard
controlled by an environment variable/flag (e.g., ENABLE_LINK_CHECKS or
SKIP_LINK_CHECKS) so the default `book` run is hermetic; keep the same CF_REPO,
CF_URL and output behavior but relocate it to the new target or gated block so
CI can opt-in without adding the 5s network wait to normal builds.
In @.rhiza/make.d/gh-aw.mk:
- Line 8: The GH_AW_BIN variable is computed at Makefile parse time but never
referenced; either remove the GH_AW_BIN assignment entirely to avoid the
unnecessary shell invocation, or centralize the check by keeping GH_AW_BIN and
replacing every direct use of the literal command (instances of "gh aw" in
recipes such as the "@gh aw compile" calls) with "$(GH_AW_BIN)" so the extension
check drives behavior; ensure the variable is defined exactly once (the existing
GH_AW_BIN assignment) and all recipe lines use "$(GH_AW_BIN)" instead of the
literal "gh aw".
In @.rhiza/make.d/test.mk:
- Around line 129-137: The coverage-badge Make target currently depends on test
causing the whole suite to re-run; update the Makefile so the coverage-badge
target (symbol: coverage-badge) does not implicitly run test unless requested —
either remove the dependency on test, add a separate quick target (e.g.,
coverage-badge-fast) that reads _tests/coverage.json without running tests, or
make the dependency conditional (only run test if _tests/coverage.json is
missing) and document the behavior in the target's comment; change references in
the same rule that call ${UVX_BIN} genbadge to preserve existing output path
assets/coverage-badge.svg.
In @.rhiza/tests/api/test_gh_aw_targets.py:
- Around line 35-87: Add a negative test that runs the Make target "gh-aw-run"
without setting WORKFLOW and without dry-run to exercise the error path: create
a test function named test_gh_aw_run_missing_workflow_errors that calls
run_make(logger, ["gh-aw-run"], check=False, dry_run=False), asserts
result.returncode != 0, and asserts that the string "WORKFLOW" appears in
result.stdout or result.stderr so the expected error message is validated;
reference run_make and the "gh-aw-run" target when locating where to add the
test.
In @.rhiza/tests/api/test_makefile_targets.py:
- Around line 161-172: The test test_coverage_badge_target_dry_run contains a
misleading setup that creates _tests/coverage.json which has no effect for a
dry-run; remove the three setup lines that create tests_dir and write
coverage.json and update the inline comment (and any mention of proceeding past
the guard) to reflect that this is a dry-run asserting printed Makefile recipe
lines only; look for references to tests_dir, (tests_dir /
"coverage.json").write_text and the comment and delete or replace them so the
test solely calls run_make and asserts on proc.stdout.
In @.rhiza/tests/integration/test_sbom.py:
- Around line 66-110: test_sbom_generation_xml lacks assertions verifying the
SBOM metadata component (name/version) for NTIA consistency; update the test to
parse the XML tree (using defusedxml.ElementTree.parse on sbom_file), locate the
metadata/component element (e.g., find ".//{*}metadata" then ".//{*}component"
or use XPath), extract the component name and version text and assert they match
the expected values from the pyproject fixture (the same values validated in
test_sbom_generation_json); keep existing checks (components element) and add
these metadata assertions to test_sbom_generation_xml to mirror NTIA checks.
In @.rhiza/tests/security/test_security_patterns.py:
- Around line 60-67: The current substring check on content (variable content
loaded from ruff_config in the test) can yield false positives; replace it with
a regex-based assertion that searches for a standalone "S" entry inside a select
or extend-select array (e.g., use re.search on content for patterns like select
= [ ... "S" ... ] or extend-select = [ ... 'S' ... ], with DOTALL to allow
multiline arrays) so the test asserts that the ruff.toml's select/extend-select
explicitly contains the "S" rule rather than matching any incidental `"S"`
substring.
In @.rhiza/tests/shell/test_scripts.sh:
- Around line 106-116: Replace the repeated manual pass/fail blocks that
directly manipulate TESTS_RUN/TESTS_PASSED/TESTS_FAILED and echo messages with
the existing assertion helper by reading the file into a variable and calling
assert_contains; e.g., for the session-start.sh check, set content=$(cat
"$REPO_ROOT/.github/hooks/session-start.sh") and call assert_contains "$content"
"set -euo pipefail" "session-start.sh uses strict error handling" (similarly
convert the other duplicated blocks at the other ranges); keep VERBOSE logic
inside assert_contains if needed and remove the direct increments/echoes so all
checks use assert_contains consistently.
- Around line 48-66: The assertion helper assert_contains currently uses grep -q
which treats the second argument as a regex; update the check in assert_contains
to use fixed-string matching by switching to grep -qF (and keep the existing
here-string <<< "$haystack" and quoted "$needle") so that literal needles with
regex metacharacters are matched literally and avoid false positives; adjust
only the grep invocation inside assert_contains.
In @.rhiza/tests/stress/test_git_stress.py:
- Around line 28-36: The subprocess.run calls (e.g., in run_git_status) lack a
timeout and can hang; update every subprocess.run invocation in this file to
include a timeout argument (e.g., timeout=some_seconds) so git commands cannot
block indefinitely—add the timeout parameter to the subprocess.run calls that
use GIT and any other subprocess.run usages, handle the possible TimeoutExpired
where appropriate (catch subprocess.TimeoutExpired where applicable) and choose
a sensible timeout constant at the top of the file for reuse.
- Line 141: The SHA length check in the test appends a boolean using
results.append(result.returncode == 0 and len(result.stdout.strip()) == 40)
which will fail for repos using SHA-256 (64 chars); update the condition to
accept both lengths (e.g., check len(result.stdout.strip()) in (40, 64)) so
results.append(result.returncode == 0 and len(result.stdout.strip()) in (40,
64)) and keep referencing the same result.stdout and results list.
In @.rhiza/tests/stress/test_makefile_stress.py:
- Around line 28-36: The subprocess.run calls (e.g., in run_help()) lack a
timeout and can hang the tests; update every subprocess.run invocation in this
file to include a reasonable timeout (e.g., timeout=30) and wrap the call in a
try/except subprocess.TimeoutExpired block so you can treat timeouts as failures
(return False or raise/assert accordingly). Locate functions like run_help() and
any other spots using subprocess.run and add timeout=<seconds> to the call
signature and handle subprocess.TimeoutExpired to avoid indefinite test hangs.
In @.rhiza/tests/sync/test_rhiza_version.py:
- Around line 163-167: In the test function
test_sync_experimental_skips_in_rhiza_repo remove the redundant explicit
dry_run=True argument from the call(s) to run_make (and the other run_make call
in the same test block mentioned by the reviewer) since run_make defaults to
dry_run=True; simply call run_make(logger, ["sync-experimental"]) to make the
test consistent with the rest of the suite and eliminate the unnecessary kwarg.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
.github/copilot-instructions.md.github/hooks/session-end.sh.github/hooks/session-start.sh.github/secret_scanning.yml.github/workflows/renovate_rhiza_sync.yml.github/workflows/rhiza_release.yml.gitignore.rhiza/.cfg.toml.rhiza/.env.rhiza/.rhiza-version.rhiza/docs/CONFIG.md.rhiza/history.rhiza/make.d/book.mk.rhiza/make.d/docs.mk.rhiza/make.d/gh-aw.mk.rhiza/make.d/marimo.mk.rhiza/make.d/quality.mk.rhiza/make.d/test.mk.rhiza/requirements/tools.txt.rhiza/rhiza.mk.rhiza/scripts/.gitkeep.rhiza/template.yml.rhiza/tests/README.md.rhiza/tests/api/conftest.py.rhiza/tests/api/test_gh_aw_targets.py.rhiza/tests/api/test_makefile_targets.py.rhiza/tests/conftest.py.rhiza/tests/integration/test_book_targets.py.rhiza/tests/integration/test_sbom.py.rhiza/tests/security/test_security_patterns.py.rhiza/tests/shell/test_scripts.sh.rhiza/tests/stress/README.md.rhiza/tests/stress/__init__.py.rhiza/tests/stress/conftest.py.rhiza/tests/stress/test_git_stress.py.rhiza/tests/stress/test_makefile_stress.py.rhiza/tests/sync/conftest.py.rhiza/tests/sync/test_readme_validation.py.rhiza/tests/sync/test_rhiza_version.py.rhiza/tests/test_utils.pyCONTRIBUTING.mdMakefileSECURITY.mddocs/ARCHITECTURE.mddocs/CUSTOMIZATION.mddocs/GLOSSARY.mddocs/QUICK_REFERENCE.mddocs/SECURITY.mddocs/TESTS.mdrenovate.jsontests/benchmarks/conftest.pytests/benchmarks/test_benchmarks.py
💤 Files with no reviewable changes (3)
- docs/ARCHITECTURE.md
- .rhiza/.env
- renovate.json
| paths-ignore: | ||
| # Ignore test fixtures that may contain example/fake secrets | ||
| - ".rhiza/tests/**" | ||
| - "tests/**" | ||
| # Ignore documentation that references example tokens/keys | ||
| - "docs/**/*.md" | ||
| - "book/**" |
There was a problem hiding this comment.
Avoid blanket ignores for docs/book to prevent missed real secrets.
Ignoring all docs and book content disables secret scanning in places where real secrets might accidentally be committed. Consider narrowing the ignore list to specific fixture/example paths instead.
🔧 Example adjustment (remove broad ignores)
paths-ignore:
# Ignore test fixtures that may contain example/fake secrets
- ".rhiza/tests/**"
- "tests/**"
- # Ignore documentation that references example tokens/keys
- - "docs/**/*.md"
- - "book/**"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/secret_scanning.yml around lines 14 - 20, The current
secret_scanning rule uses blanket ignores "docs/**/*.md" and "book/**" which can
hide real secrets; instead remove those broad patterns and narrowly ignore only
known example or fixture paths (e.g., change "docs/**/*.md" and "book/**" to
more specific patterns like "docs/**/*example*/**", "docs/**/fixtures/**", or
".rhiza/tests/**" style paths); update the paths-ignore block to delete
"docs/**/*.md" and "book/**" and add targeted ignores for specific
example/fixture directories so documentation remains scanned for accidental real
secrets while still excluding harmless examples.
| - name: Get Rhiza version | ||
| id: rhiza-version | ||
| run: | | ||
| VERSION=$(cat .rhiza/.rhiza-version 2>/dev/null || echo "0.9.0") | ||
| echo "version=${VERSION}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Stale fallback version may trigger an outdated rhiza install.
If .rhiza/.rhiza-version is absent (e.g., a fresh clone), the fallback "0.9.0" is two minor versions behind the current 0.11.2. Consider updating the fallback to match the current baseline.
♻️ Proposed fix
- VERSION=$(cat .rhiza/.rhiza-version 2>/dev/null || echo "0.9.0")
+ VERSION=$(cat .rhiza/.rhiza-version 2>/dev/null || echo "0.11.2")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Get Rhiza version | |
| id: rhiza-version | |
| run: | | |
| VERSION=$(cat .rhiza/.rhiza-version 2>/dev/null || echo "0.9.0") | |
| echo "version=${VERSION}" >> "$GITHUB_OUTPUT" | |
| - name: Get Rhiza version | |
| id: rhiza-version | |
| run: | | |
| VERSION=$(cat .rhiza/.rhiza-version 2>/dev/null || echo "0.11.2") | |
| echo "version=${VERSION}" >> "$GITHUB_OUTPUT" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/renovate_rhiza_sync.yml around lines 41 - 45, The fallback
version used in the "Get Rhiza version" step (id: rhiza-version) is hardcoded as
"0.9.0" via VERSION=$(cat .rhiza/.rhiza-version 2>/dev/null || echo "0.9.0");
update that fallback to the current baseline "0.11.2" so fresh clones without
.rhiza/.rhiza-version will install the expected rhiza version; change the echoed
fallback string to "0.11.2" while keeping the same variable NAME (VERSION) and
output logic.
| - name: Commit and push changes | ||
| if: steps.sync.outputs.changes == 'true' | ||
| run: | | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "41898282+github-actions[bot]@users.noreply.github.com" | ||
| git config --global url."https://x-access-token:${{ secrets.PAT_TOKEN || github.token }}@github.com/".insteadOf "https://github.com/" | ||
|
|
||
| git add -A | ||
| git commit -m "$(cat <<'EOF' | ||
| chore: sync rhiza template files | ||
|
|
||
| Automatically synced template files after updating .rhiza/template.yml | ||
|
|
||
| Co-Authored-By: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> | ||
| EOF | ||
| )" | ||
|
|
There was a problem hiding this comment.
git config --global url.insteadOf rewrites ALL GitHub URLs for the runner.
Line 71 sets a global URL substitution that injects the PAT into every git operation against github.com for the remainder of the job, not just the push to this repository. If additional git checkouts or remote operations are added later, they'll silently inherit this credential.
Scoping the credential rewrite to the specific remote is safer:
🛡️ Proposed fix (scope to origin only)
- git config --global url."https://x-access-token:${{ secrets.PAT_TOKEN || github.token }}@github.com/".insteadOf "https://github.com/"
-
- git add -A
+ git remote set-url origin "https://x-access-token:${{ secrets.PAT_TOKEN || github.token }}@github.com/${{ github.repository }}.git"
+ git add -A📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Commit and push changes | |
| if: steps.sync.outputs.changes == 'true' | |
| run: | | |
| git config user.name "github-actions[bot]" | |
| git config user.email "41898282+github-actions[bot]@users.noreply.github.com" | |
| git config --global url."https://x-access-token:${{ secrets.PAT_TOKEN || github.token }}@github.com/".insteadOf "https://github.com/" | |
| git add -A | |
| git commit -m "$(cat <<'EOF' | |
| chore: sync rhiza template files | |
| Automatically synced template files after updating .rhiza/template.yml | |
| Co-Authored-By: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> | |
| EOF | |
| )" | |
| - name: Commit and push changes | |
| if: steps.sync.outputs.changes == 'true' | |
| run: | | |
| git config user.name "github-actions[bot]" | |
| git config user.email "41898282+github-actions[bot]@users.noreply.github.com" | |
| git remote set-url origin "https://x-access-token:${{ secrets.PAT_TOKEN || github.token }}@github.com/${{ github.repository }}.git" | |
| git add -A | |
| git commit -m "$(cat <<'EOF' | |
| chore: sync rhiza template files | |
| Automatically synced template files after updating .rhiza/template.yml | |
| Co-Authored-By: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> | |
| EOF | |
| )" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/renovate_rhiza_sync.yml around lines 66 - 82, The current
git config command uses --global and rewrites all github.com URLs; change it to
configure only this repository and scope the substitution to the specific repo
remote: replace git config --global url."https://x-access-token:${{
secrets.PAT_TOKEN || github.token }}@github.com/".insteadOf
"https://github.com/" with a local config that targets only the current
repository (omit --global) and match the repository path (e.g. insteadOf
"https://github.com/${{ github.repository }}" and url
"https://x-access-token:${{ secrets.PAT_TOKEN || github.token }}@github.com/${{
github.repository }}") so only pushes to this repo use the PAT.
| # Normalize tag version to PEP 440 format for comparison. | ||
| # Tags use semver format (e.g., 0.11.1-beta.1) while uv version --short | ||
| # returns PEP 440 normalized format (e.g., 0.11.1b1). | ||
| NORMALIZED_TAG=$(uv run --with packaging --no-project python3 -c "from packaging.version import Version; print(Version('$TAG_VERSION'))") | ||
|
|
||
| if [[ "$PROJECT_VERSION" != "$NORMALIZED_TAG" ]]; then | ||
| echo "::error::Version mismatch: pyproject.toml has '$PROJECT_VERSION' but tag is '$NORMALIZED_TAG' (from tag '$TAG_VERSION')" | ||
| exit 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
uv run --with packaging --no-project python3 - <<'PY'
from packaging.version import Version, InvalidVersion
tags = ["0.11.1-beta.1", "0.11.1-rc.1", "0.11.1", "0.11.1b1"]
for t in tags:
try:
print(f"{t} -> {Version(t)}")
except InvalidVersion as e:
print(f"{t} -> InvalidVersion: {e}")
PYRepository: tschm/monkeys
Length of output: 99
🏁 Script executed:
# Check the actual tag format used in the workflow file
grep -n "TAG_VERSION\|tag_version" .github/workflows/rhiza_release.yml | head -20Repository: tschm/monkeys
Length of output: 451
🏁 Script executed:
# Check if there's a tag pattern or naming convention documented
cat .github/workflows/rhiza_release.yml | grep -A 10 -B 10 "tag"Repository: tschm/monkeys
Length of output: 10611
🏁 Script executed:
git tag -l | grep -E 'beta|rc|alpha|pre|dev|a|b|c' | head -20Repository: tschm/monkeys
Length of output: 39
🏁 Script executed:
# Also check if there are any existing tags at all
git tag -l | head -20Repository: tschm/monkeys
Length of output: 39
🌐 Web query:
packaging.Version InvalidVersion semver beta hyphen format PEP 440
💡 Result:
packaging.version.Version only accepts PEP 440 versions; many SemVer 2.0 strings (especially pre-release/build forms) are therefore rejected with packaging.version.InvalidVersion. Version() raises InvalidVersion whenever the input “does not conform to PEP 440 in any way”. [1]
Key mismatch: SemVer -beta... vs PEP 440 prereleases
- SemVer: prerelease is introduced with a hyphen and can be a dot-separated set of identifiers (e.g.
1.2.3-beta.1.feature). [2] - PEP 440: prerelease segment is restricted to specific tags
{a|b|rc}(with accepted spellings likealpha,beta,preview/pre,c/rc) plus an (optional) number; separators like.,-,_are allowed and missing numbers imply0. [3]
So common conversions are:
| SemVer | PEP 440 equivalent |
|---|---|
1.2.3-beta |
1.2.3b0 (number omitted ⇒ 0) [3] |
1.2.3-beta.1 |
1.2.3b1 [3] |
1.2.3-rc.2 |
1.2.3rc2 [3] |
1.2.3-alpha.4 |
1.2.3a4 [3] |
Typical reasons you still get InvalidVersion with “SemVer-looking” strings
- Extra prerelease identifiers beyond the single PEP 440 prerelease field, e.g.
1.2.3-beta.1.feature(SemVer allows; PEP 440 doesn’t). [2][3] - SemVer build metadata (
+build.5). PEP 440 has+localversions, but semantics/order rules differ, and not all SemVer build strings map cleanly. [3]
If you paste the exact version string that triggers InvalidVersion, I can show the precise PEP 440 rewrite for it.
Sources: [1] [2] [3]
Add error handling for InvalidVersion when normalizing semver-style tags.
The workflow comment suggests tags may use semver format (e.g., 0.11.1-beta.1), but packaging.Version strictly enforces PEP 440 and will raise InvalidVersion on semver-hyphenated prerelease strings, causing the workflow to crash with a raw Python traceback. Add explicit error handling to provide a clear, actionable message.
Suggested fix
- NORMALIZED_TAG=$(uv run --with packaging --no-project python3 -c "from packaging.version import Version; print(Version('$TAG_VERSION'))")
+ NORMALIZED_TAG=$(uv run --with packaging --no-project python3 - <<'PY'
+from packaging.version import Version, InvalidVersion
+import os, sys
+
+tag = os.environ["TAG_VERSION"]
+try:
+ print(Version(tag))
+except InvalidVersion:
+ print(f"::error::Invalid tag version '{tag}'. Expected PEP 440 format (e.g., 0.11.1, 0.11.1b1).")
+ sys.exit(1)
+PY
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Normalize tag version to PEP 440 format for comparison. | |
| # Tags use semver format (e.g., 0.11.1-beta.1) while uv version --short | |
| # returns PEP 440 normalized format (e.g., 0.11.1b1). | |
| NORMALIZED_TAG=$(uv run --with packaging --no-project python3 -c "from packaging.version import Version; print(Version('$TAG_VERSION'))") | |
| if [[ "$PROJECT_VERSION" != "$NORMALIZED_TAG" ]]; then | |
| echo "::error::Version mismatch: pyproject.toml has '$PROJECT_VERSION' but tag is '$NORMALIZED_TAG' (from tag '$TAG_VERSION')" | |
| exit 1 | |
| # Normalize tag version to PEP 440 format for comparison. | |
| # Tags use semver format (e.g., 0.11.1-beta.1) while uv version --short | |
| # returns PEP 440 normalized format (e.g., 0.11.1b1). | |
| NORMALIZED_TAG=$(uv run --with packaging --no-project python3 - <<'PY' | |
| from packaging.version import Version, InvalidVersion | |
| import os, sys | |
| tag = os.environ["TAG_VERSION"] | |
| try: | |
| print(Version(tag)) | |
| except InvalidVersion: | |
| print(f"::error::Invalid tag version '{tag}'. Expected PEP 440 format (e.g., 0.11.1, 0.11.1b1).") | |
| sys.exit(1) | |
| PY | |
| ) | |
| if [[ "$PROJECT_VERSION" != "$NORMALIZED_TAG" ]]; then | |
| echo "::error::Version mismatch: pyproject.toml has '$PROJECT_VERSION' but tag is '$NORMALIZED_TAG' (from tag '$TAG_VERSION')" | |
| exit 1 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/rhiza_release.yml around lines 139 - 146, Wrap the
normalization step that calls packaging.version.Version('$TAG_VERSION') in a
try/except that catches packaging.version.InvalidVersion and prints a clear
GitHub Actions error message before exiting; specifically, update the shell
invocation that sets NORMALIZED_TAG (the command using uv run --with packaging
--no-project python3 -c "from packaging.version import Version;
print(Version('$TAG_VERSION'))") to handle InvalidVersion and emit something
like "Invalid tag format: '$TAG_VERSION' cannot be normalized to PEP 440; ensure
tag is PEP 440 or adjust release workflow" (and exit nonzero) so the subsequent
comparison between PROJECT_VERSION and NORMALIZED_TAG behaves predictably.
| # Security scanning baselines (regenerate as needed) | ||
| .bandit-baseline.json | ||
|
|
There was a problem hiding this comment.
Duplicate .bandit-baseline.json entry in .gitignore.
The newly added entry at line 85 duplicates the pre-existing entry at line 107. The duplicate is functionally harmless but creates unnecessary noise. Remove one of them.
🧹 Proposed fix
Remove the old bare entry at line 107:
-.bandit-baseline.json
-…keeping only the new annotated block at lines 84–85.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 84 - 86, Remove the duplicate .bandit-baseline.json
entry in .gitignore: keep the new annotated block containing
".bandit-baseline.json" (the security scanning baselines block) and delete the
older bare ".bandit-baseline.json" entry elsewhere in the file so the file
contains only the single annotated baseline entry.
| @read -p "Enter ADR title (e.g., 'Use PostgreSQL for data storage'): " title; \ | ||
| echo ""; \ | ||
| read -p "Enter brief context (optional, press Enter to skip): " context; \ | ||
| echo ""; \ | ||
| if [ -z "$$title" ]; then \ | ||
| echo "Error: Title is required"; \ | ||
| exit 1; \ | ||
| fi; \ |
There was a problem hiding this comment.
read -p is not POSIX-compliant and the target will fail non-interactively.
read -p is a bash extension not supported by POSIX sh (e.g., dash on Ubuntu). If SHELL is not explicitly set to bash in the Makefile, this recipe may fail. More critically, the adr target silently hangs or errors in any non-interactive context (CI pipelines, make -B, piped execution).
Consider adding #!/bin/bash or SHELL := /bin/bash at the top of the Makefile, and documenting that this target requires an interactive terminal.
♻️ Proposed guard
+SHELL := /bin/bash
+
.PHONY: adr
adr: install-gh-aw ## Create a new Architecture Decision Record (ADR) using AI assistance (interactive)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 22 - 29, The Makefile's ADR interactive recipe uses
the bash-only `read -p` (seen in the adr target), which will fail or hang under
POSIX sh (e.g., dash) and in non-interactive CI; fix by making the recipe
portable or forcing bash: either set `SHELL := /bin/bash` at the top of the
Makefile to guarantee bash features for the adr target, or replace `read -p ...`
with a POSIX-safe sequence (use `printf` to prompt then plain `read` to capture
into `title`/`context`, and guard against non-interactive shells by checking `-t
0` before prompting and erroring with a clear message if not interactive); also
update the target documentation to state it requires an interactive terminal.
| gh workflow run adr-create.md -f title="$$title"; \ | ||
| else \ | ||
| gh workflow run adr-create.md -f title="$$title" -f context="$$context"; \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find workflow files and adr-create files
find . -type f \( -name "*.yml" -o -name "*.yaml" -o -name "adr-create*" \) | head -30Repository: tschm/monkeys
Length of output: 707
🏁 Script executed:
#!/bin/bash
# Check for .github/workflows directory
ls -la .github/workflows/ 2>/dev/null || echo "No .github/workflows directory found"Repository: tschm/monkeys
Length of output: 1060
🏁 Script executed:
#!/bin/bash
# Look at the Makefile to understand context
if [ -f Makefile ]; then
head -40 Makefile
fiRepository: tschm/monkeys
Length of output: 1343
🏁 Script executed:
#!/bin/bash
# Search for any ADR-related files
find . -type f -iname "*adr*" | head -20Repository: tschm/monkeys
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check .rhiza directory structure
ls -la .rhiza/ 2>/dev/null | head -30Repository: tschm/monkeys
Length of output: 939
🏁 Script executed:
#!/bin/bash
# Search for gh-aw related files (GitHub Copilot Agentic Workflows)
find . -type f -name "*gh-aw*" -o -name "*agentic*" | head -20Repository: tschm/monkeys
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Check gh-aw.mk file
cat -n .rhiza/make.d/gh-aw.mkRepository: tschm/monkeys
Length of output: 3106
🏁 Script executed:
#!/bin/bash
# Check agentic.mk file
cat -n .rhiza/make.d/agentic.mkRepository: tschm/monkeys
Length of output: 3701
🏁 Script executed:
#!/bin/bash
# Search for adr-create reference in the entire repo
rg "adr-create" --no-headingRepository: tschm/monkeys
Length of output: 346
🏁 Script executed:
#!/bin/bash
# Look for any prompt or spec files that might be agentic workflows
find . -type f -name "*.md" | grep -E "(workflow|prompt|agentic|adr)" | head -20Repository: tschm/monkeys
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if there's documentation about the adr target
git log --oneline -n 20 -- Makefile 2>/dev/null | head -10Repository: tschm/monkeys
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Search for any workflow files in .rhiza or elsewhere that might define adr-create
find .rhiza -type f \( -name "*.md" -o -name "*.yml" -o -name "*.yaml" \) | xargs grep -l "adr" 2>/dev/nullRepository: tschm/monkeys
Length of output: 41
Create the missing adr-create workflow file or fix the invocation command.
The gh workflow run adr-create.md command references a workflow that does not exist. Additionally, GitHub Actions workflows must use .yml or .yaml extensions, not .md.
Verify the intent:
- If this should be a GitHub Actions workflow: Create
.github/workflows/adr-create.ymland update the invocations to use the.ymlextension. - If this should be a GitHub Agentic Workflow: Use
gh aw run adr-createinstead (no extension), and ensure the agentic workflow prompt/spec file is created.
The repository includes agentic workflow support (.rhiza/make.d/gh-aw.mk with GH_AW_ENGINE=copilot), so clarify which mechanism is intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 31 - 33, The Makefile is invoking a non-existent
workflow file "adr-create.md"; either create the corresponding workflow or fix
the invocation: if this is meant to be a GitHub Actions workflow, add
.github/workflows/adr-create.yml (or .yaml) and update the Makefile invocations
that call "gh workflow run adr-create.md" to "gh workflow run adr-create.yml"
(preserve the -f title/ -f context flags), otherwise if it's an agentic workflow
use the agentic CLI and change the Makefile to call "gh aw run adr-create" (no
extension) and add the agentic spec file (e.g., .rhiza/…/adr-create)
accordingly; update the lines invoking gh workflow run adr-create.md to the
chosen command and ensure the new workflow/spec file exists.
| 2. **Email** | ||
| - Send details to the repository maintainers | ||
| - Include "SECURITY" in the subject line |
There was a problem hiding this comment.
No email address provided for security vulnerability reporting.
The email reporting option says "Send details to the repository maintainers" but doesn't include an actual email address. Security reporters need a concrete contact method — consider adding a dedicated security email address or linking to the maintainer's GitHub profile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SECURITY.md` around lines 28 - 30, Update the "Email" section (the heading
"2. **Email**" and its bullet text) to include a concrete contact: either a
dedicated security@... address or a link to the maintainer's GitHub profile or
security contact page; replace or augment "Send details to the repository
maintainers" with the actual email or URL and include guidance on PGP/encryption
if available.
| Security Notes: | ||
| - S101 (assert usage): Asserts are the standard way to validate test conditions in pytest. | ||
| They provide clear test failure messages and are expected in test code. | ||
| """ |
There was a problem hiding this comment.
S101 security note is not applicable to this file.
The module docstring's Security Notes block references S101 (assert usage), but this conftest contains no assert statements. The note appears to be copy-pasted from another conftest and is misleading here.
📝 Proposed fix
-"""Pytest configuration for benchmark tests.
-
-This file can be used to add custom fixtures or configuration
-for your benchmark tests.
-
-Security Notes:
-- S101 (assert usage): Asserts are the standard way to validate test conditions in pytest.
- They provide clear test failure messages and are expected in test code.
-"""
+"""Pytest configuration for benchmark tests.
+
+This file can be used to add custom fixtures or configuration
+for your benchmark tests.
+"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Security Notes: | |
| - S101 (assert usage): Asserts are the standard way to validate test conditions in pytest. | |
| They provide clear test failure messages and are expected in test code. | |
| """ | |
| """Pytest configuration for benchmark tests. | |
| This file can be used to add custom fixtures or configuration | |
| for your benchmark tests. | |
| """ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/benchmarks/conftest.py` around lines 6 - 9, Remove the misleading
"Security Notes" S101 mention from the module docstring in this conftest (the
top-level module docstring in tests/benchmarks/conftest.py); edit the module
docstring used by the conftest module to either delete the entire Security Notes
block or replace it with an accurate note that does not reference S101/assert
usage since there are no asserts in this file.
| def pytest_html_report_title(report): | ||
| """Set the HTML report title.""" | ||
| report.title = "Benchmark Tests" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for pytest-html in dependency manifests
rg -n "pytest-html" --type toml --type cfg --type ini -g "!*.lock"Repository: tschm/monkeys
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Search for pytest-html in common dependency files
rg "pytest-html" pyproject.toml setup.py setup.cfg requirements*.txt 2>/dev/null || echo "Not found in standard locations"
# Also list common dependency files to understand project structure
echo "=== Common dependency files ==="
git ls-files | grep -E "(pyproject|setup|requirements)"Repository: tschm/monkeys
Length of output: 371
🏁 Script executed:
#!/bin/bash
# Check pyproject.toml for pytest-html
echo "=== pyproject.toml ==="
cat pyproject.toml | head -100
# Check tests.txt requirements
echo -e "\n=== .rhiza/requirements/tests.txt ==="
cat .rhiza/requirements/tests.txtRepository: tschm/monkeys
Length of output: 2573
🏁 Script executed:
#!/bin/bash
# Check .rhiza/requirements README
echo "=== .rhiza/requirements/README.md ==="
cat .rhiza/requirements/README.md
# Check CI/CD workflows to see how dependencies are installed
echo -e "\n=== GitHub workflows that might install dependencies ==="
fd "workflow" .github/workflows -x grep -l "pytest-html\|requirements/tests" {} \;Repository: tschm/monkeys
Length of output: 967
🏁 Script executed:
#!/bin/bash
# Read the conftest.py file to check the security note and asserts
cat -n tests/benchmarks/conftest.pyRepository: tschm/monkeys
Length of output: 588
Add pytest-html to pyproject.toml and remove the irrelevant S101 security note.
pytest-html is declared in .rhiza/requirements/tests.txt but missing from pyproject.toml's [dependency-groups]. The canonical dependency manifest should list all test dependencies, including pytest-html, so the hook works for anyone installing via pyproject.toml. Additionally, the S101 (assert usage) security note in the module docstring is misleading — this file contains no assert statements and the note appears to be copy-pasted boilerplate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/benchmarks/conftest.py` around lines 12 - 14, Add pytest-html to the
project's canonical test dependencies under pyproject.toml's
[project.dependency-groups] (or equivalent test/dev group) so the
pytest_html_report_title hook in tests/benchmarks/conftest.py works for users
installing via pyproject, and remove the misleading S101/assert usage note from
the module docstring in that file (it’s irrelevant here); reference the
pytest_html_report_title hook and the pytest-html package when making these
changes.
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation